This repository has been archived by the owner. It is now read-only.

TLS NPN problems #6168

Closed
molnarg opened this Issue Sep 2, 2013 · 10 comments

Comments

Projects
None yet
3 participants
@molnarg

molnarg commented Sep 2, 2013

When using NPN with node 0.10.17, I've run into the following issue.

I am running a server which support two protocol: x and y, and prints the negotiated protocol when there's an incoming connection:

var fs = require('fs');
var path = require('path');
var tls = require('tls');

tls.createServer({
  key: fs.readFileSync(path.join(__dirname, '/localhost.key')),
  cert: fs.readFileSync(path.join(__dirname, '/localhost.crt')),
  NPNProtocols: ['x', 'y']
}, function(socket) {
  console.log(socket.npnProtocol);
}).listen(1111);

I have a client that connects four times with different NPN protocol sets, and prints the negotiated protocol:

var tls = require('tls');

process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";

tls.connect({ port: 1111 }, function() {
  console.log(this.npnProtocol);
  tls.connect({ port: 1111, NPNProtocols: ['x'] }, function() {
    console.log(this.npnProtocol);
    tls.connect({ port: 1111 }, function() {
      console.log(this.npnProtocol);
      tls.connect({ port: 1111, NPNProtocols: ['z'] }, function() {
        console.log(this.npnProtocol);
      });
    });
  });
});

Now, when I run the client against the server, I get the following results:

#  Supported protocols    Negotiated protocol    Expected negotiated
   (client)   (server)    (client)   (server)    protocol (both)
--------------------------------------------------------------------
1  undefined  [x, y]      false      http/1.1    false
2  [x]        [x, y]      x          x           x
3  undefined  [x, y]      x          x           false
4  [z]        [x, y]      false      z           false

1, 3 and 4 seems to be incorrect:

  • in 1, the server sees http/1.1 as negotiated protocol
  • in 3, both the client and the server incorrectly shows the last successfully negotiated protocol instead of false
  • in 4, the server thinks that z is successfully negotiated while it's only supported by the client
@molnarg

This comment has been minimized.

molnarg commented Sep 9, 2013

Am I doing something wrong, or is it really a bug?

@indutny

This comment has been minimized.

Member

indutny commented Sep 9, 2013

Looks like a bug, I'll look into it.

@ghost ghost assigned indutny Sep 9, 2013

indutny added a commit to indutny/node that referenced this issue Sep 9, 2013

tls: fix setting NPN protocols
The NPN protocols was set on `require('tls')` or `global` object instead
of being a local property. This fact lead to strange persistence of NPN
protocols, and sometimes incorrect protocol selection (when no NPN
protocols were passed in client options).

fix nodejs#6168

@indutny indutny referenced this issue Sep 9, 2013

Merged

Fix/gh 6168 #6200

@indutny

This comment has been minimized.

Member

indutny commented Sep 9, 2013

Fixed in 1c3863a.

@indutny indutny closed this Sep 9, 2013

@molnarg

This comment has been minimized.

molnarg commented Sep 9, 2013

Thanks! I've just tested it. On the client side, it seems to work perfectly!

On the server side, though, it's not OK yet. I get the following result with the above test case:

http/1.1
x
http/1.1
z

Instead of:

false
x
false
false

http/1.1 as default is not a problem for me, but when ignoring that, its still not correct in the fourth test case. It's an independent bug maybe?

Edit: corrected x to false in the third row of the second table

@indutny indutny reopened this Sep 9, 2013

@indutny

This comment has been minimized.

Member

indutny commented Sep 9, 2013

@molnarg yeah, looks like a separate bug to me.

@shigeki

This comment has been minimized.

shigeki commented Sep 10, 2013

I think this is not an actually bug because in this case the client selected z as a fallback protocol and sent it to the server which is the case of 4 in https://github.com/joyent/node/blob/master/deps/openssl/openssl/ssl/ssl_lib.c#L1527-L1541 . But inconsistency of the value of socket.npnProtocol between a server and a client is confusing so it is a best to check it after https://github.com/joyent/node/blob/v0.10/src/node_crypto.cc#L1993 and return false in this case.

@indutny indutny closed this Sep 10, 2013

@indutny indutny reopened this Sep 10, 2013

@indutny

This comment has been minimized.

Member

indutny commented Sep 10, 2013

@molnarg Exactly what @shigeki said.

According to (I must admit) pretty rough [spec][0]:

In this case, clients may wish to opportunistically select a protocol that wasn't advertised by the server.

Thus behaviour in such cases is not specified. On a contrary to @shigeki's comment, I'd say that it works as intended and we won't fix it.

@indutny indutny closed this Sep 10, 2013

@molnarg

This comment has been minimized.

molnarg commented Sep 10, 2013

Thanks @shigeki and @indutny for sorting this out! I'm OK with the latter problem being 'wontfix'.

A nice additional thing would be to select http/1.1 as default in the HTTP code instead of TLS (and signaling false here). @indutny do you think that if I prepare a PR, it can be merged?

@shigeki

This comment has been minimized.

shigeki commented Sep 10, 2013

@indutny I'm okay with it. It's a very rare case and NPN is going to be phased out in future. In ALPN no_application_protocol alert is defined in this case but unfortunately the latest openssl seems not to support this alert.

@indutny

This comment has been minimized.

Member

indutny commented Sep 10, 2013

@molnarg sure, but please don't expect it to be landed in v0.10, as it may break compatibility with existing libs.

Looking forward for PR! ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.