Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[proxy] Generate random sec-websocket-key if needed #4801

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

csweichel
Copy link
Contributor

This PR produces a random sec-websocket-key on the /api/gitpod if that header is not present. This has become necessary due to recent Google infrastructure woes.

fixes #4742

@csweichel csweichel requested review from a team and geropl and removed request for a team July 13, 2021 13:58
@geropl
Copy link
Member

geropl commented Jul 14, 2021

Is this still relevant after the GCp issue seems to be fixed? #4742 (comment)

@geropl
Copy link
Member

geropl commented Jul 14, 2021

/werft run

👍 started the job as gitpod-build-csweichel-proxy-supplement-sec-4742.4

@csweichel
Copy link
Contributor Author

Indeed. The GCP issue is gone, but we have no guarantee that it won't come back.

@geropl
Copy link
Member

geropl commented Jul 14, 2021

When testing this I end up in a redirect loop on https://csweichel-proxy-supplement-sec-4742.staging.gitpod-dev.com/sorry#Authorization failed. Please try again.:
Maybe due to bad deployment? 🤷
image

Last lines of proxy log:

{"level":"info","ts":1626273349.2459686,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:49 +0000] \"GET /api/login?host=github.com&returnTo=https%3A%2F%2Fcsweichel-proxy-supplement-sec-4742.staging.gitpod-dev.com%2Fcomplete-auth%3Fmessage%3Dsuccess HTTP/2.0\" 302 0","duration":0.060451977}
{"level":"info","ts":1626273350.3517618,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:50 +0000] \"GET /auth/github/callback?code=47e739a62b38ffd49459&state=csweichel-proxy-supplement-sec-4742 HTTP/2.0\" 302 284","duration":0.621180257}
{"level":"info","ts":1626273350.4023216,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:50 +0000] \"GET /sorry HTTP/2.0\" 200 2271","duration":0.002118655}
{"level":"info","ts":1626273350.5273762,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:50 +0000] \"GET /static/js/8.191f803c.chunk.js HTTP/2.0\" 0 0","duration":0.000763447}
{"level":"info","ts":1626273350.5281892,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:50 +0000] \"GET /static/js/main.08dd79f5.chunk.js HTTP/2.0\" 0 0","duration":0.001223802}
{"level":"info","ts":1626273355.8442624,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:55 +0000] \"GET /api/gitpod HTTP/1.1\" 0 0","duration":5.133882647}
{"level":"info","ts":1626273355.8998337,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:55 +0000] \"GET /api/login?host=github.com&returnTo=https%3A%2F%2Fcsweichel-proxy-supplement-sec-4742.staging.gitpod-dev.com%2Fcomplete-auth%3Fmessage%3Dsuccess HTTP/2.0\" 302 0","duration":0.039915593}
{"level":"info","ts":1626273357.159365,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:57 +0000] \"GET /auth/github/callback?code=38154a54f1504e2af6dd&state=csweichel-proxy-supplement-sec-4742 HTTP/2.0\" 302 284","duration":0.588428921}
{"level":"info","ts":1626273357.208571,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:57 +0000] \"GET /sorry HTTP/2.0\" 200 2271","duration":0.001254105}
{"level":"info","ts":1626273357.2675653,"logger":"http.log.access.log0","msg":"handled request","common_log":"10.60.45.166 - - [14/Jul/2021:14:35:57 +0000] \"GET /api/gitpod HTTP/1.1\" 0 0","duration":0.223141719}

@csweichel
Copy link
Contributor Author

csweichel commented Jul 14, 2021

/werft run

👍 started the job as gitpod-build-csweichel-proxy-supplement-sec-4742.5

@csweichel
Copy link
Contributor Author

csweichel commented Jul 14, 2021

When testing this I end up in a redirect loop on https://csweichel-proxy-supplement-sec-4742.staging.gitpod-dev.com/sorry#Authorization failed. Please try again.:
Maybe due to bad deployment? 🤷

Most certainly. During the login afaik there's no websocket involvement.

@akosyakov
Copy link
Member

akosyakov commented Jul 15, 2021

Looking at Chromium source code, it really surprises me that it worked at all: https://source.chromium.org/chromium/chromium/src/+/main:net/websockets/websocket_basic_handshake_stream.cc;l=508-510;drc=2ac6e19345379fa671bb83a4ff33a498f1dc33c7;bpv=1;bpt=1 Maybe a bug in GCP actually would respond with proper Sec-WebSocket-Accept and it's what made it work, regardless of our server response.

We should also consider another side of injecting such key, spec says that it's to protect from abuse, see https://datatracker.ietf.org/doc/html/rfc6455#section-11.3.1:

This helps ensure that the server
does not accept connections from non-WebSocket clients (e.g., HTTP
clients) that are being abused to send data to unsuspecting WebSocket
servers.

@geropl
Copy link
Member

geropl commented Jul 15, 2021

/werft run

👍 started the job as gitpod-build-csweichel-proxy-supplement-sec-4742.6

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM. I tested regular usage and it worked.

I can, however, not send ws requests without the header in Firefox (not sure how to do so in Chrome).

Plus I'm not 100% convinced that this change is necessary. But I don't see any harm (besides complexity) either.

@csweichel csweichel merged commit fa39408 into main Jul 15, 2021
@csweichel csweichel deleted the csweichel/proxy-supplement-sec-4742 branch July 15, 2021 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[proxy] Supplement sec-websocket-key with random value
3 participants