[Libwebsockets] Handling multiple protocols in handshake (was: Re: [PATCH 0/3] Some small fixes)

Fabian Kurz fabian at fkurz.net
Wed Feb 4 19:09:28 CET 2015


Hello Andy,

On Sat, Dec 13, 2014 at 02:06:21PM +0800, Andy Green wrote:
> On 12/11/2014 12:09 AM, Alejandro Mery wrote:
> > On Wed, Dec 10, 2014 at 11:08:42AM +0800, Andy Green wrote:
> >> There's an outstanding patch modified from one that had to be reverted
> >> on github, I did not get a chance to look at that yet or how it will
> >> play with your change there.  (In fact I am not sure what he's fixing
> >> since it's something outside of test-server / client I think).
> >
> > I don't follow you :(
> 
> I meant there is another guy who has sent a pull request about the same code
> 
> https://github.com/warmcat/libwebsockets/pull/211
> 
> I'd welcome your opinion on what he has done.  He hasn't made a patch on 
> our basis and when I last tried to import his patch, it didn't apply.
> 
> His previous attempt killed the test client protocol negotiation and had 
> to be reverted.
> 
> I guess his problem is real but it's not reproduced on test server / 
> client which work OK already.

I am late to the party, but I just found this thread after fixing
exactly the same problem (and with it, temporarily broke the test
server / client) for which the commit
[c0eb2c3e9d64de9e7dc92fc0d30d8c61e9206ca5] was made and then reverted.

Before submitting my own patch, I looked at the history and found the
patch/revert and subsequently this thread.

The intention of the original patch by Rene Jager was to improve the
protocol negotiation, and I think he got most of it right.

Here's what I found out:

In my attempts to use test-client.c to talk to a 3rd party websocket
server (in this case one implemented in Java with Netty) I noticed
that the test client sends

 "Sec-WebSocket-Protocol: dumb-increment-protocol,fake-nonexistant-protocol"

in the request header, which is in line with RFC6455 which allows
multiple sub protocols to be requested.

The target server (Netty) was configured to talk
"dumb-increment-protocol" only, and therefore only replied with

   "Sec-WebSocket-Protocol: dumb-increment-protocol".

The test-client then exited with a protocol failure because it
expected to receive exactly what's specified in
protocols[PROTOCOL_DUMB_INCREMENT].name (lws_client_int_s_hs: fail
protocol).

Contrary to Netty, the libwebsocket server implementation always
echoes the full, unmodified Sec-Websocket-Protocol header, even if it
contains unsupported protocols, if *one* of the protocols is
supported.

I think this behaviour of libwebsockets is not correct.

According to RFC6455, Sec. 11.3.4, the "Sec-WebSocket-Protocol" header
must not appear more than once in a response. It doesn't explicitly
say there that it must not contain more than one protocol, but that'd
be my interpretation when comparing it to the explicit permission in
the previous sentences to send more than one protocol (comma separated
or in separate lines) in the request.

In the (non-normative) protocol overview (Sec. 1.3, "Opening
Handshake"), it actually says quite clearly:

   .---[ RFC6455, 1.3 Opening Handshake ]----
   |  The server selects one or none of the acceptable protocols and echoes
   |  that value in its handshake to indicate that it has selected that
   |  protocol.
   '-----------------------------------------

So in my opinion the libwebsocket server handshake should therefore
only echo the (first) accepted procotol. 

The following patch does this. Contrary to the reverted patch it also
saves a few CPU cycles because it just checks for the
WSI_TOKEN_PROTOCOL pointer and doesn't bother to count the length. If
this pointer is NULL, no protocol was sent in the request and we don't
need to reply. Otherwise a protocol was requested and accepted
(because if it was not accepted, we would have bailed out earlier).

diff --git a/lib/server-handshake.c b/lib/server-handshake.c
index b4641d1..dd4ef73 100644
--- a/lib/server-handshake.c
+++ b/lib/server-handshake.c
@@ -212,12 +212,9 @@ handshake_0405(struct libwebsocket_context *context,
struct libwebsocket *wsi)
        strcpy(p, (char *)context->service_buffer);
        p += accept_len;
 
-       if (lws_hdr_total_length(wsi, WSI_TOKEN_PROTOCOL)) {
+       if (lws_hdr_simple_ptr(wsi, WSI_TOKEN_PROTOCOL) != NULL) {
                LWS_CPYAPP(p, "\x0d\x0aSec-WebSocket-Protocol: ");
-               n = lws_hdr_copy(wsi, p, 128, WSI_TOKEN_PROTOCOL);
-               if (n < 0)
-                       goto bail;
-               p += n;
+               LWS_CPYAPP(p, wsi->protocol->name);
        }
 
 #ifndef LWS_NO_EXTENSIONS

Also, the original patch included two lines to correctly parse
multiple sub protocols separated by ", " instead of just "," which is
allowed by the RFC.

diff --git a/lib/server.c b/lib/server.c
index df2524a..e6b0058 100644
--- a/lib/server.c
+++ b/lib/server.c
@@ -458,8 +458,10 @@ upgrade_ws:
                        protocol_name[n] = '\0';
                        if (*p)
                                p++;
+                       while (*p == ' ')
+                               p++;
 
-                       lwsl_info("checking %s\n", protocol_name);
+                       lwsl_info("checking '%s'\n", protocol_name);
 
                        n = 0;
                        while (wsi->protocol && context->protocols[n].callback)
{


What the original patch finally did in server.c was some modification
in code related to http2 switching, and I think it was not done right
(there, "protocol_list" was temporarily used to store the Upgrade
header, and he replaced it with only the requested protocol).

So far so good. After "fixing" the server, the test client/server breaks.

In test-client.c, the "name" members of the libwebsocket_protocols
struct protocols are used to list two protocols (a real one and a fake
one), which causes a failure as described above. The description of
the struct suggests that this is not intended, the name member is
described as:

   .---[ libwebsockets.h:921 969de6e ]----
   |  * @name:       Protocol name that must match the one given in the client
   |  *              Javascript new WebSocket(url, 'protocol') name.
   '--------------------------------------

In order to demonstrate protocol negotiation, if the server behaviour
is changed in the way proposed, I'd suggest something like the
attached patch against test-client.c which produces the same request
header (with two protocols), but with valid names in the protocol
struct which then won't fail to validate against the server response.

Concerning the later patch to client.c by Alejandro
[31f9eeb9d6891c2ba17284ce4a28325cf13b2ff3], which ensures proper
splitting of the protocols in the reply header, I am not sure if it is
needed at all, because as far as I understand there should only be a
single protocol in there.

What do you think?

Best regards,
Fabian

-- 
Fabian Kurz, DJ1YFK      Munich, Germany
fabian at fkurz.net         +49 176 24079617
http://fkurz.net/        http://lcwo.net/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test-client.patch
Type: text/x-diff
Size: 2169 bytes
Desc: not available
URL: <http://libwebsockets.org/pipermail/libwebsockets/attachments/20150204/dc651559/attachment.bin>


More information about the Libwebsockets mailing list