NPNProtocols docs need clarification / example

[coolaj86]

@indutny The documentation on the HTTPS page refers to the TLS page which fails to give an example (obviously 'hello' and 'world' are not true internet protocols).

I'm assuming that it should be something like this:

https.createServer({
  key: fs.readFileSync(path.join(certsPath, 'my-server.key.pem'))
, cert: fs.readFileSync(path.join(certsPath, 'my-server.crt.pem'))
, ca: [
    fs.readFileSync(path.join(caCertsPath, 'intermediate.crt.pem'))
  , fs.readFileSync(path.join(caCertsPath, 'root.crt.pem'))
  ]

  , NPNProtocols: ['http/2.0', 'spdy', 'http/1.1', 'http/1.0']

});

But I'm not sure if 'http/2.0' is a valid option or how I would pass off http/2.0 requests to the http/2.0 handler, etc (I'm assuming that no http2 module has yet to make it into core yet).

[Fishrock123]

cc @shigeki also

[Fishrock123]

But, io.js does not support http2 at the current time.

[indutny]

Ok, it does work this way. But it does nothing but just supply the extension in the ServerHello/ClientHello. So there is no underlying protocol implementation, and the user is supposed to check the selected protocol with socket.npnProtocol to figure out what protocol should be used.

So, to restate, no implementation other than http/1.1 is provided. This is how I handle this in node-spdy: https://github.com/indutny/node-spdy/blob/master/lib/spdy/server.js#L236-L240

[indutny]

But I'm open to suggestions on how it could be improved! :)

[tellnes]

I'm going a little bit of topic and answer to how this can be improved.

What about splitting the secureConnection into multiple events based on protocol. Eg.

server.on('http/1.1', fn)
server.on('spdy', fn)

If we also changes the current http implementation to only listen on http/1.1 and http/1.0, then implementing extra protocols in user land would be as easy as adding an extra event listener.

If this gets positive feedback, I'll open a pull request.

[shigeki]

@tellnes Does your change have any difference with the below?

var server = https.createServer(opts);
server.removeAllListeners('secureConnection');
server.on('secureConnection', function(s) {
  this.emit(s.npnProtocol, s);
});
server.on('http/1.1', function(s) {
  http._connectionListener.call(this, s);
});

If it is the same, I think it is very easy to implement in userland.

[tellnes]

@shigeki Yes, that's basically my proposal. But your solution is not foolproof. What if another library already has added another listener to secureConnection?

I think we should try to make an api where this monkey patching of the server object which all those libraries are doing is unnecessary. Referencing #1101 here.

Here is a code example to iterate on.

var server = http.createServer(opts);
require('http2').attach(server)
var wss = require('ws').attach(server)

server.listen({ port: 80 })
server.listen({ port: 443, tls: true, http2: true, cert: '...', key: '...' })

server.on('request', function(req, res) {
  /* incoming request which is either
     - http
     - https
     - http2
  */
})

wss.on('connection', function () {
  // new websocket connection
})

I also think we should expose an api for easily pushing extra protocols as long as it is done before the listen method is called.

Also, are not npn going to be replaced with alpn and we will need to expose a similar api for that?

But if we are not doing something like this, then we should mark http._connectionListener as public, so we can bless that the way to do this is like @shigeki example above. Also maybe remove the underscore in the name before documenting it and deprecate the underscored version.

[indutny]

I'm with @shigeki here, it is really simple to implement and is only a matter of a couple of lines of code. Emitting random-named events on the server seems to be a bad idea to me, especially considering that the user may supply any protocol name to the NPN. Even secureConnection ;)

[tellnes]

@indutny your last argument convinces me also that that was a bad ide.

What about doing something like this in lib/https.js:

function(socket) {
  if (socket.npnProtocol === 'http/1.1' ||
      socket.npnProtocol === 'http/1.0' ||
      socket.npnProtocol === false)
    return http._connectionListener.call(this, socket);

  if (EventEmitter.listenerCount(this, 'secureConnection') !== 1)
    socket.destroy();
}

[indutny]

Hm... this might be the thing. What do you think about making it more generic, though? I have done that _connectionListener wrapping a couple of times in 3rd party libraries, and it will be quite interesting to explore a generic way of doing it.

Like supplying a function preconnection or something like this that will decide whether the default https listener should be called, or the custom one.

[tellnes]

I've done the same wrapping a couple of times myself, so I'm also interested in making it more generic.

If we are not going for some kind of events, I think the solution above is the best one. That will let you add multiple secureConnection listeners and they will only need one if to see if the connection is for them.

The only extra code an app implementor then will need is something like this:

var protocols = [ 'h2', 'http/1.1', 'http/1.0' ]
server.on('secureConnection', function(socket) {
  if (!~protocols.indexOf(socket.npnProtocol)
    socket.destroy()
})

Maybe we can implement the above with some kind of option?

[indutny]

@tellnes seems to be too high-level to me. What if we would just provide an option for http.Server to not listen on connection/secureConnection, and will allow user setting it up manually?

[tellnes]

@indutny I agree that it is more high-level, but I disagree that it is too high-level.

Would not exposing an option be the same as bless using http._connectionListener in combination with (net | tls).createServer?

In won't allow you to do something like my example above. But it would allow userland to use public apis to make a library which allows you to do that.

If we are going to solve this as low-level as possible I would say maybe renaming and documenting http._connectionListener with an example with how to do this.

[tellnes]

What about adding a new method to the server. Inspired by server.addContext(hostname, context) in the tls module.

server.addProtocol('h2', function(socket) {
  //
})

[indutny]

@tellnes this is actually starting to sound interesting :) @Fishrock123 @shigeki what are your thoughts on this?

@tellnes please keep it mind that it should have a fallback callback for empty ALPN/NPN.

[tellnes]

On https the fallback callback must be set to http._connectionListener to not break backward compatibility. But for a plain tls.Server, maybe an empty string can identify no protocol?

Also, if you call addProtocol multiple times, should it replace the handler or stack them up? I think it should replace it. That would make it easy to replace default handlers.

And can we make it so the (n|al)pnProtocols configuration option get filled automatically when addProtocol is called before .listen() or something?

[indutny]

I guess so, sounds like a nice APIs.

[shigeki]

I think we we should consider how much high level is provided for a general protocol selection in tls.Server.addProtocol() because ALPN is just one of the requirements to use HTTP/2 over TLS.

Other MUST requirements for TLS in HTTP/2 are

• TLS version is 1.2 or higher
• SNI (we should care about connection reuse of multiple fqdn and inconsistency with :authority)
• AEAD (We have only AES-GCM now)
• PFS with minimum key size (2048 bits in DHE, 224 bits in ECDHE)
• No compression
• No renegotiation after having connection preface

Of course、 we need not implement all of them in tls.Server.addProtocol（） but we should write caveats in the doc to describe what is not included in it.

If we just want to have a kind of syntax suger of ALPN callback such as if (socket.ALPNProtocol==='foo') cb(socket);, I think that tls.Server.addALPNCallback() is the right API name though it might lead a confusion with something like ALPNSelectCallback invoked during TLS handshake.

[Trott]

It looks like this issue may have morphed over time. It's been dormant for two years, though, so I'm going to close it. If that's not the right thing to do, leave a comment saying so or (if GitHub permissions allow) re-open the issue. Thanks.