replace version identifier and require user action #49

Kriechi · 2017-08-19T19:32:41Z

No description provided.

Lukasa · 2017-08-19T20:00:08Z

@jeamland So I think this looks reasonable but I want you to cast your eye over it when you get a chance.

njsmith · 2017-11-11T23:40:41Z

wsproto/connection.py

@@ -378,8 +378,10 @@ def _process_connection_request(self, event):
        if b'sec-websocket-version' not in headers:
            return ConnectionFailed(CloseReason.PROTOCOL_ERROR,
                                    "Missing Sec-WebSocket-Version header")
-        # XX FIXME: need to check Sec-Websocket-Version, and respond with a
-        # 400 if it's not what we expect
+        versions = _split_comma_header(headers[b'sec-websocket-version'])


According to the spec, the client's sec-websocket-version header is not comma-delimited. It just has to be the literal string b"13", nothing else. So instead of using _split_comma_header, this should just check that headers[b'sec-websocket-version'] == b'13'.

You can see this from the ABNF:

Sec-WebSocket-Version-Client = version version = DIGIT | (NZDIGIT DIGIT) | ("1" DIGIT DIGIT) | ("2" DIGIT DIGIT) ; Limited to 0-255 range, with no leading zeros

And it's also in the text: "The request MUST include a header field with the name |Sec-WebSocket-Version|. The value of this header field MUST be 13." (section 4.1). Also, "the |Sec-WebSocket-Version| header field MUST NOT appear more than once in an HTTP request." (section 11.3.5).

Not sure which RFC you are reading, but I am using this section:
https://tools.ietf.org/html/rfc6455#section-4.4

which contains these examples:

The response from the server might look as follows: HTTP/1.1 400 Bad Request ... Sec-WebSocket-Version: 13, 8, 7 Note that the last response from the server might also look like: HTTP/1.1 400 Bad Request ... Sec-WebSocket-Version: 13 Sec-WebSocket-Version: 8, 7

Oh wait - you are talking about the client request of the handshake.
Sorry, I was referring to the server's response!

Yeah, it's confusing -- they use Sec-WebSocket-Version for two almost, but not quite, entirely different things. The client always sends it, and it has to send exactly the string b"13" (for the final RFC 6455 version of the protocol). And, if the server doesn't like the version the client sent, then the server has to send back a comma-separated list of versions that it does like.

Right - I will change this block to only check if the header value is == b'13'.

Sorry, I totally forgot to push my changes - now this should be fixed.

njsmith · 2017-11-11T23:45:35Z

wsproto/connection.py

+        versions = _split_comma_header(headers[b'sec-websocket-version'])
+        if WEBSOCKET_VERSION not in versions:
+            return ConnectionFailed(CloseReason.PROTOCOL_ERROR,
+                                    "Unsupported Sec-WebSocket-Version")


So... this is a little tricky. According to the spec (section 4.2.2), servers that get an unrecognized version MUST respond in a particular way:

The |Sec-WebSocket-Version| header field in the client's handshake includes the version of the WebSocket Protocol with which the client is attempting to communicate. If this version does not match a version understood by the server, the server MUST abort the WebSocket handshake described in this section and instead send an appropriate HTTP error code (such as 426 Upgrade Required) and a |Sec-WebSocket-Version| header field indicating the version(s) the server is capable of understanding.

Right now we don't really have any way to handle this, right? When a connection fails, we just bail out, and we expect the user to somehow construct an appropriate HTTP response? This seems a bit unfriendly in general, and even more so in this case where we're not even giving the user enough information to construct that response correctly... maybe we should be generating the response ourselves? @jeamland, @Lukasa would appreciate thoughts here.

You are right - this change simply dumps the problem onto the user to take care off.
The problem for me (in the context of @mitmproxy) is that we want to craft our own response, so wsproto should not respond directly.

I agree it is not perfect, however, still better than simply ignoring the version identifier at all.
Not sure about which version are actually out there, but I am not aware of anything other than version 13...?

There are other versions that represent draft versions of the spec, before it was finalized and became an RFC. Some libraries still support them. In the future there could in theory be more versions. But 13 is currently the only standard version, so that's all wsproto supports.

I'm not sure why you want to craft your own response, given that the spec kinda only gives you one option :-). But also, if you want to hack some odd behavior in here, you do always have the option of ignoring the bytes that wsproto tells you to send, and replacing them with your own instead.

mitmproxy is a man-in-the-middle web proxy for HTTP 1 & 2, WebSockets, raw TCP,...
so there all sorts of reasons why our users might want to do things 😄
We also want to to do the handshake ourself, because it is part of our HTTP parsing pipeline already.

Everything else in _process_connection_request raises an exception, so I think it is reasonable to do the same for unsupported versions.

How about we introduce a class UnsupportedVersion(ConnectionFailed) exception, and provide a def generate_unsupported_version() function to return a response with the appropriate data?

If you're doing the handshake yourself, then why do you care how wsproto handles the handshake? :-)

How about we introduce a class UnsupportedVersion(ConnectionFailed) exception, and provide a def generate_unsupported_version() function to return a response with the appropriate data?

This seems like a lot of boilerplate for users to worry about...

So, to summarize and finally get a consensus on this:

You don't like throwing an exception if the version doesn't match what wsproto supports.
wsproto should directly respond with the HTTP response message to indicate this error.

This is what I currently plan to use in mitmproxy:
https://github.com/mitmproxy/mitmproxy/blob/c9b8b2e3a3a3fef18dfa66bcd9288f12bef357b6/mitmproxy/proxy/protocol/websocket.py#L51-L75

In L67 https://github.com/mitmproxy/mitmproxy/blob/c9b8b2e3a3a3fef18dfa66bcd9288f12bef357b6/mitmproxy/proxy/protocol/websocket.py#L67 I need some way of identifying the error case (version mismatch). If wsproto only tells me this via an HTTP response, I have to rely on parsing the output of bytes_to_send to check this...

Do you feel more comfortable with a new connection state: ConnectionState.VersionMismatch to indicate this error?

@njsmith any update on this?

pgjones · 2018-09-23T08:27:17Z

wsproto/connection.py

@@ -380,8 +380,9 @@ def _process_connection_request(self, event):
        if b'sec-websocket-version' not in headers:
            return ConnectionFailed(CloseReason.PROTOCOL_ERROR,
                                    "Missing Sec-WebSocket-Version header")
-        # XX FIXME: need to check Sec-Websocket-Version, and respond with a
-        # 400 if it's not what we expect
+        if headers[b'sec-websocket-version'] != WEBSOCKET_VERSION:


@Kriechi I think the RFC allows for a client to send multiple allowed versions, with the server allowed to respond with the one it wants to use (or 400 with a set of possible versions). I think therefore this is too strict, in this form.

Kriechi · 2019-01-06T11:06:28Z

@pgjones anything salvageable from this PR - or just close & delete?

pgjones · 2019-01-06T15:07:41Z

I think I got it all in #96.

Kriechi force-pushed the version-identifier branch from 9c1c62f to aa527ab Compare August 19, 2017 19:40

Kriechi mentioned this pull request Aug 19, 2017

api++ #47

Closed

Kriechi mentioned this pull request Aug 27, 2017

Allow WSConnection to be initialised with open state #37

Closed

njsmith requested changes Nov 11, 2017

View reviewed changes

replace version identifier and require user action

8e2c79e

Kriechi force-pushed the version-identifier branch from 1ab8280 to 8e2c79e Compare June 10, 2018 12:26

pgjones requested changes Sep 23, 2018

View reviewed changes

pgjones closed this Jan 6, 2019

Kriechi deleted the version-identifier branch January 6, 2019 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace version identifier and require user action #49

replace version identifier and require user action #49

Kriechi commented Aug 19, 2017

Lukasa commented Aug 19, 2017

njsmith Nov 11, 2017

Kriechi Nov 12, 2017

Kriechi Nov 12, 2017

njsmith Nov 12, 2017

Kriechi Nov 12, 2017

Kriechi Dec 11, 2017

njsmith Nov 11, 2017

Kriechi Nov 12, 2017

njsmith Nov 12, 2017

Kriechi Nov 12, 2017

njsmith Nov 13, 2017

Kriechi Dec 11, 2017

Kriechi Jun 10, 2018

pgjones Sep 23, 2018

Kriechi commented Jan 6, 2019

pgjones commented Jan 6, 2019

replace version identifier and require user action #49

replace version identifier and require user action #49

Conversation

Kriechi commented Aug 19, 2017

Lukasa commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kriechi commented Jan 6, 2019

pgjones commented Jan 6, 2019