-
-
Notifications
You must be signed in to change notification settings - Fork 195
Pass CSRF token using Sec-WebSocket-Protocol header #465
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
base: master
Are you sure you want to change the base?
Conversation
|
I opted for prefixing the actual CSRF token value with If you feel this is over-engineering the solution, feel free to strip that out. |
|
@iarenaza Thank you Iñaki, this looks good! Will try take a closer look + merge by end of tomorrow 👍 |
|
Whoops. I had forgotten the other side of the connection: extracting the CRSF token from the Sec-WebSocket-Protocol header on the server side. |
|
@iarenaza Hi Iñaki, sorry for the delay on this. Before merging, I'd like to step back a moment and re-evaluate what we're trying to do here. The current PR seems to be based on this suggestion made earlier by @kaosko, right? But it's actually not obvious to me what the motivation is/was behind that suggestion. @kaosko Sorry for digging this up after so long, but could you maybe explain why you've previously suggested that the Sente CSRF token be sent in the Even though the Protocol header seems to be an application-level header, it seems we'd be abusing the semantic intention of the header no? I imagine that stands a non-zero chance of causing issues in the future, and seems like it anyway could be surprising. What's the anticipated upside? |
|
It's a JavaScript spec/API thing. It doesn't allow you to set any other headers for the WebSocket request object than "Sec-WebSocket-Protocol" so if you want to pass auth/CSRF tokens in the headers, that's the way to do it. I wouldn't call it abusing the semantic intention, perhaps merely stretching it, again as an application level protocol. In the grand scheme of things, I guess not that different from manufacturer specific Wi-Fi Information Elements or DHCP Vendor Options. Almost anything has a non-zero chance of causing issues in the future, but the server is certainly free to ignore any values in Sec-WebSocket-Protocol that it doesn't understand. Plenty of web pages, frameworks still sending various "X-*" headers while the naming practice was deprecated in 2012 and the servers may or may not act on those (and vice versa). That said, I'm not strongly arguing for adding this but don't see how it could hurt much either. Finally, I have used and still use the Sec-WebSocket-Protocol header for sending authentication tokens with the websocket request. |
|
Yeah, exactly what @kaosko said. In any case, for us this is the only acceptable way. We develop SPAs that use OpenID Connect ID tokens to authenticate the user. We have no sessions, use no cookies, etc. So having a CSRF token generated on the backend and sent to the frontend is not something that we can do that easily. We already have tokens that are short lived (the ID tokens) and cannot be stolen via JS, or automatically sent by the browser on your behalf (you need to explicitly add them in the requests, as a Bearer token). So we are leveraging those tokens for Sente CSRF purposes too. But passing those tokens in the URL as a parameter is not something that we want to do. They leak in the server logs, in any intermediate proxy, etc. That is why we went with the |
…@iarenaza) Before this commit: WebSocket client conveys CSRF token to server via `:csrf-token` query param. After this commit: WebSocket client conveys CSRF token to server via `Sec-WebSocket-Protocol` header. Motivation: Moving the token from query param to a header helps reduce the likelihood of accidental leaking (e.g. via logging). While the `Sec-WebSocket-Protocol` header isn't specifically intended for conveying metadata like a CSRF token, the consensus seems to be that this is anyway a practical choice without major downsides or obviously better alternatives. As implemented the change tries to respect other values in the `Sec-WebSocket-Protocol` header that may have been set when calling `make-channel-socket-client!`.
|
Thanks both for the extra info and quick feedback! 🙏 I've just pushed |
|
@ptaoussanis Just to confirm that Side comment: we still need to use some of the available "callback functions" (like So, thanks a lot @ptaoussanis ! |
|
Great, thanks for the confirmation @iarenaza! |
|
@ptaoussanis We've found that the approach used in this pull request is not enough to work with Google Chrome. We did all our development and testing with Firefox, and it worked great. But when we tried with Chrome, it refused to open the websocket connection. The problem seems to be that the web server (http-kit 2.8.0 in our case) doesn't send any Firefox is fine with this, and considers the websocket negotiation handshake successfully done. But apparently Chrome doesn't consider it successful, if it doesn't have a According to RFC 6455, section 1.3 Opening handshake (emphasis mine):
and later on, when talking about the handshake from the server side:
But the whole 1.3 section is marked as non-normative. Which I interpret as not being mandatory to follow (even if it is the recommended way). Later on, in section 1.9 it says:
It clearly states that the server needs to include the same field and the selected protocol. But then again, the whole 1.9 section ir marked as "non-normative". Given that sente doesn't control what headers are returned in the server response for the connection upgrade rquest (they are fully in control of http-kit at the moment), I think we need to step back a bit an re-asses our options. And in the mean time, revert this commit to avoid the breakage it introduced. What do you say? P.S. We are so sorry that we didn't test it with Chrome before submitting this PR and wasted your valuable time 😞 |
|
What about Aleph, Undertow etc? It seems that http-kit just returns whatever you stick in :ring.websocket/protocol key of the ring response, no? |
I don't know about Aleph, Undertow, and the rest of the community adapters (I haven't used them). But I know that http-kit only does that if you are using the Ring Websocket Protocol library. If you are using the [1] https://github.com/taoensso/sente/blob/master/src/taoensso/sente.cljc#L1001 |
|
@iarenaza Hi Iñaki, thanks for the detailed+clear update 🙏
No problem at all, I appreciate the time + effort you've put into this! I'm sorry that you've been running into issues with Sente and/or http-kit! Re: how to proceed- I've unfortunately got limited knowledge + bandwidth on this topic since I'm currently quite deeply buried in a few other things. I am planning a big batch of Sente work for June, but hopefully we can make progress here in the meantime! Can we first back up a bit so that I can confirm my understanding? @iarenaza Your motivation for #465 is described here, right? So:
So the goal is to get the CSRF token from the client to the server in a way that:
Is that right? If we were to enumerate all the options we have for getting this done- what would that include?
I haven't had the opportunity to properly investigate this myself, so any input here would be very welcome! IIUC the main impediment to (1) is:
Is that right? So it seems like our options are something like:
Any thoughts on which direction might be simplest? What do other libraries like Sente tend to do? |
|
Any update on this issue? I'm aiming to cut a new Sente release around the end of this month, so this would be a good time to get in any pending changes. |
…@iarenaza) Before this commit: WebSocket client conveys CSRF token to server via `:csrf-token` query param. After this commit: WebSocket client conveys CSRF token to server via `Sec-WebSocket-Protocol` header. Motivation: Moving the token from query param to a header helps reduce the likelihood of accidental leaking (e.g. via logging). While the `Sec-WebSocket-Protocol` header isn't specifically intended for conveying metadata like a CSRF token, the consensus seems to be that this is anyway a practical choice without major downsides or obviously better alternatives. As implemented the change tries to respect other values in the `Sec-WebSocket-Protocol` header that may have been set when calling `make-channel-socket-client!`.
84e09ed to
6088457
Compare
|
Sorry for not coming back to you earlier. We've been in a sustained rush at work for a few months 😞
Correct on all counts!
Ideally, that would be our goal, yes.
As far as I know, there is no other header that you can use with websockets from a web browser (nodejs may be a different kettle of fish). According to Mozilla Developer Network, when you create a websocket you can only specify the URL for the connection end point, and an optional list of sub-protocols that the client would like to negotiate with the server side of the connection.
I guess that would be possibility, but that would mean that the websocket would always be opened, no matter what is sent in the first message. And later, once the server side has checked the token and decided on whether the token is ok or not, either keep the connection opened or close it for good. This option would also imply that sente would be unusable for those use cases where sub-protocol negotiation is a must. That is certainly not our case. And probably not something that most people try to use, or even know about. So I would say this is not a show stopper (IMHO).
Not that I can think of at the moment.
As far as I know, that is 100% right. I have a patch set (for both Sente and http-kit) that would:
The patch set is little more than a proof of concept, to see that we could theoretically make it work. But I'm not sure of the unintended side-effects of those changes (I'm not an http-kit expert by any means!). I could clean it up (it's full of
I haven't looked at other libraries like Sente, so I don't really know. But most of the examples I've seen out there (in a quick search) don't even try to prevent CSRF attacks, or don't use CSRF tokens (and only rely on |
bb88305 to
16fbe7c
Compare
The proposed change tries to respect other values in the Sec-WebSocket-Protocol header, that the caller may have set in the headers option when calling make-channel-socket-client! Use the pre-ws-handshake option introduced in http-kit commit to Add the Sec-WebSocket-Protocol response header in the websocket upgrade. Some browsers (like Google Chrome and Google Chrome derived ones) fail to complete the upgrade if the upgrade response doesn't include the Sec-WebSocket-Protocol with one of the sub-protocols proposed in the request. [Re: 465]
|
Hi @ptaoussanis , I have finally extracted my original changes from my local test repos, cleaned them up and removed all the Then I have created a separate git repo, using the This is what I have done:
Do the changes look like something that you could consider including? |
The proposed change tries to respect other values in the Sec-WebSocket-Protocol header, that the caller may have set in the
headersoption when callingmake-channel-socket-client![Re: #418]