Use the httputil.NewSingleHostReverseProxy instead of yhat/wsutil for … #1348
Conversation
…websocket proxying. This correctly handles 404 responses with keep-alive by terminating the tunnel rather than keeping it alive
|
This is the one tricky area with websockets in particular that likely need a manual test: If I recall, if this isn't working right, our access logs will drop authenticated user information (email & user fields in the logs). Eyeballing those in your manual tests might be a good final check. |
No problem. As @NickMeves mentioned, this is quite hard to test in an automated way. It's certainly beyond the scope of my Go knowledge to set up and tear down the necessary HTTP servers. I have a test rig, but it's written in Prolog [don't ask]. The gist is that the server can respond with HTTP 404 to everything while logging all the headers it receives. If you try and do a websocket connection to the server twice, the first time you'll see the cookie translated into an authentication header, and the second time you won't. |
|
Just trying to work out how to improve testing around this area and came across https://ieftimov.com/post/testing-in-go-websockets/, @thetrime I don't know if this is something you'd want to look into as a learning exercise? I know you said your Go knowledge isn't massive. If you don't want to delve to deep, I can look into extending the tests here to make sure we are covering this change at some point, let me know what you want to do |
Unfortunately I don't have time to do this now; realistically it'd probably be months before I could find time to sit down and teach myself another language. If you have the resources to implement a test framework, or can find someone else to do it, that'd definitely be the fastest road to getting it resolved |
… … (oauth2-proxy#1348) * Use the httputil.NewSingleHostReverseProxy instad of yhat/wsutil for websocket proxying. This correctly handles 404 responses with keep-alive by terminating the tunnel rather than keeping it alive * Tidy up dependencies - yhat/wsutil is no longer required * Update changelog to include reference to 1348 Co-authored-by: Matt Lilley <matt.lilley@securitease.com> Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
… … (oauth2-proxy#1348) * Use the httputil.NewSingleHostReverseProxy instad of yhat/wsutil for websocket proxying. This correctly handles 404 responses with keep-alive by terminating the tunnel rather than keeping it alive * Tidy up dependencies - yhat/wsutil is no longer required * Update changelog to include reference to 1348 Co-authored-by: Matt Lilley <matt.lilley@securitease.com> Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
…websocket proxying. This correctly handles 404 responses with keep-alive by terminating the tunnel rather than keeping it alive
Description
httputil has handled websockets natively since go1.12. The upstream package yhat/wsutil hasn't been updated in years and fails to handle some edge cases
Motivation and Context
This proposal fixes #1347
How Has This Been Tested?
The change is relatively localised. The way I tested it was as follows:
HTTP/1.1 404 Not Found\nContent-Type: text/plain\nConnection: keep-alive\nContent-length: 7\n\nMissingChecklist:
This should probably be mentioned in the changelog, but it shouldn't break anything that doesn't rely on what should be considered broken behaviour