-
-
Notifications
You must be signed in to change notification settings - Fork 635
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
Finalize web socket requirements #1908
Comments
Just quick feedback: 13.5.1 - in a way covered by 9.1.1
I don't know the answer, but can you use not secure web socket, when you have HSTS set? 13.5.2 - in a way covered by 4.1.1
What means "list of authorized origins"? 13.5.3 - remove as duplicate of 11.2.2? 13.5.4 - what is the message or goal for the requirement? Require session? For everyone? How it is aligned wit 13.5.5? 13.5.6 - what is/was the point for this? |
Looks like HSTS also applies to WebSocket: Sources to support
I agree it is theoretically a duplicate but it is also quite a specific case that would need to be specific configured, especially if for whatever reason HSTS is not in use. Hence suggesting leaving it in.
Again, this is a very specific case which is why I think it is worth including it
Yeah I agree, I think it should be worded slightly better:
Basically yes, I didn't tag it because it is ADDED
Basic problem is that websockets support cookie based sessions but not tokens in headers so if that is the required Auth mechanism you have to do something specific. At that point, how you get the token is important hence the need for 13.5.5 as well.
Not sure but it doesn't seem like it adds something specific on V2 and 13.5.4/13.5.5 |
Regarding 13.5.4/13.5.5, having seen a number of implementations, it seems the most common practice is to associate connections with standard sessions supplied during the handshake.
Unless I am misunderstanding, I think this is not correct. Cookies, headers, and query parameters equally (though the latter obviously not preferred) can be utilized from a handshake to authenticate a connection. From a complexity point of view, I think it's probably preferred to use the standard session management regardless. |
Hi @ryarmst :)
So I think I wasn't clear enough. What I should have said was:
So if I have an app where all my authentication involves using a header token, for the handshake request which will result in a 101 upgrade response, it will be necessary to somehow transmit that header separately. Does that match how you understand it? If so, I may try and reword/clarify 13.5.4/13.5.5. |
Hi @tghosth! Sorry, I was mistaken. I checked the protocol spec, but ignored the API spec that does not support custom headers. I had seen an implementation repurpose |
So from an access control/session management perspective, my understanding is that securing the websocket handshake is the crucial part so if a non-cookie session mechanism is in use then a session token of some sort would need to be obtained from the server and then sent in the handshake request. The server would then only perform the handshake "upgrade" if the session token was valid. It may be better if the regular session token is not used for this as sending it in the handshake request might expose it more than usual (e.g. if it goes in the request header). |
I am assuming the scenario where "securing the websocket handshake" means to associate the created WS connection with a user's existing web session/identity (derived from an HTTP-based web app). IMO the ideal approach would be with a properly secured cookie-based session mechanism, but obviously that does not account for the diversity of mechanisms as implemented. The challenge with authenticating in the handshake/upgrade without cookies is that there are essentially only 2 other options within this HTTP request to send a token to associate the connection with the existing session:
However, there are also other options following the creation of the WS connection, using messages within the connection (that each also have their own pros/cons):
The way I interpret
Agree 100% that a secondary and ideally transient token should be used when an existing cookie mechanism isn't available. I cannot immediately think of a phrasing I would recommend for the requirement, but I think it should either be adjusted to be more broad or more specific (endorsing a specific implementation, though I think flexibility will be required in practice). |
Thanks for the feedback @ryarmst :)
Do you think this updated version of 13.5.5 is more accommodating whilst also secure? |
@tghosth That looks great! It might also be worth adding an exclusion for when the primary session (and authentication) occurs over WS rather than HTTP (this is seemingly rare, but I have seen at least one case). What about this?
|
That looks great @ryarmst, fancy opening a PR with your change into the following branch? Note that the requirement will now be 13.5.4 because 13.5.3 got deleted. |
Additionally fixed a 13.5.3 typo
* intial trim of websocket section * Fix linting issue * Clarify 13.5.2 * Fix numbering * Updated 13.5.4 as per #1908 (#1936) * Updated 13.5.4 as per #1908 Additionally fixed a 13.5.3 typo * Updated wording of 13.5.4 to begin with "Verify that" --------- Co-authored-by: Ryan Armstrong <ryarmst@users.noreply.github.com>
Closed by #1909 |
* intial trim of websocket section * Fix linting issue * Clarify 13.5.2 * Fix numbering * Updated 13.5.4 as per #1908 (#1936) * Updated 13.5.4 as per #1908 Additionally fixed a 13.5.3 typo * Updated wording of 13.5.4 to begin with "Verify that" --------- Co-authored-by: Ryan Armstrong <ryarmst@users.noreply.github.com>
Web Socket requirements were introduced following discussions in #842 and #649.
In preparation for 5.0, I have been through them and would suggest making the following changes (additions in bold, deletions in
strikethrough.)V13.5 WebSocket
[ADDED] Verify that rate limiting is in place for WebSocket messages.Verify that tokens possess at least 128 bits of entropy and are generated using approved cryptographic algorithms if session or channel tokens specific to WebSockets are being used.[ADDED] Verify that authentication is done before opening the WebSocket connection if only authenticated users should be able to use WebSockets.I don't think 13.5.3, the original 13.5.4 and 13.5.6 are specific enough to web sockets. I think the others have specific application to web sockets so they should stay.
@Racater @elarlang any thoughts before I commit the changes?
The text was updated successfully, but these errors were encountered: