http2: reject incompatible TLS ALPN handshakes

Emit `unknownProtocol` event or silently destroy the socket

PR-URL: nodejs#144
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

doc/api/http2.md

@@ -437,7 +437,7 @@ an `Http2Stream`.
 
 #### Event: 'fetchTrailers'
 
-The `'fetchTrailers`' event is emitted by the `Http2Stream` immediately after
+The `'fetchTrailers'` event is emitted by the `Http2Stream` immediately after
 queuing the last chunk of payload data to be sent. The listener callback is
 passed a single object (with a `null` prototype) that the listener may used
 to specify the trailing header fields to send to the peer.
@@ -696,7 +696,7 @@ an `Http2Session` object. If no listener is registered for this event, an
 
 #### Event: 'socketError'
 
-The `'socketError`' event is emitted when an `'error'` event is emitted by
+The `'socketError'` event is emitted when an `'error'` event is emitted by
 a `Socket` associated with the server. If no listener is registered for this
 event, an `'error'` event is emitted.
 
@@ -745,10 +745,17 @@ an `Http2Session` object. If no listener is registered for this event, an
 
 #### Event: 'socketError'
 
-The `'socketError`' event is emitted when an `'error'` event is emitted by
+The `'socketError'` event is emitted when an `'error'` event is emitted by
 a `Socket` associated with the server. If no listener is registered for this
 event, an `'error'` event is emitted on the `Socket` instance instead.
 
+#### Event: 'unknownProtocol'
+
+The `'unknownProtocol'` event is emitted when a connecting client fails to
+negotiate an allowed protocol (i.e. HTTP/2 or HTTP/1.1). The event handler
+receives the socket for handling. If no listener is registered for this event,
+the connection is terminated. See the
+
 #### Event: 'stream'
 
 The `'stream'` event is emitted when a `'stream'` event has been emitted by
@@ -864,7 +871,7 @@ server.listen(80);
 * `options` {Object}
   * `allowHTTP1` {boolean} Incoming client connections that do not support
     HTTP/2 will be downgraded to HTTP/1.x when set to `true`. The default value
-    is `false`, which rejects non-HTTP/2 client connections.
+    is `false`. See the [`'unknownProtocol'`][] event.
   * `maxDefaultDynamicTableSize` {number} (TODO: Add detail)
   * `maxReservedRemoteStreams` {number} (TODO: Add detail)
   * `maxSendHeaderBlockLength` {number} (TODO: Add detail)
@@ -1111,3 +1118,4 @@ TBD
 [Settings Object]: #http2_settings_object
 [Using options.selectPadding]: #http2_using_options_selectpadding
 [error code]: #error_codes
+[`'unknownProtocol'`]: #http2_event_unknownprotocol

lib/internal/http2/core.js

@@ -1619,11 +1619,17 @@ function connectionListener(socket) {
     socket.on('timeout', socketOnTimeout);
   }
 
-  // TLS ALPN fallback to HTTP/1.1
-  if (options.allowHTTP1 === true &&
-    (socket.alpnProtocol === false ||
-    socket.alpnProtocol === 'http/1.1')) {
-    return httpConnectionListener.call(this, socket);
+  if (socket.alpnProtocol === false || socket.alpnProtocol === 'http/1.1') {
+    if (options.allowHTTP1 === true) {
+      // Fallback to HTTP/1.1
+      return httpConnectionListener.call(this, socket);
+    } else if (this.emit('unknownProtocol', socket)) {
+      // Let event handler deal with the socket
+      return;
+    } else {
+      // Reject non-HTTP/2 client
+      return socket.destroy();
+    }
   }
 
   socket.on('error', socketOnError);
@@ -1658,10 +1664,11 @@ function initializeOptions(options) {
 
 function initializeTLSOptions(options, servername) {
   options = initializeOptions(options);
-  options.ALPNProtocols = ['h2', 'http/1.1'];
-  if (servername !== undefined && options.servername === undefined) {
+  options.ALPNProtocols = ['h2'];
+  if (options.allowHTTP1 === true)
+    options.ALPNProtocols.push('http/1.1');
+  if (servername !== undefined && options.servername === undefined)
     options.servername = servername;
-  }
   return options;
 }
 

test/parallel/test-http2-https-fallback.js

@@ -1,16 +1,33 @@
 'use strict';
 
-const { fixturesDir, mustCall, mustNotCall } = require('../common');
+const {
+  fixturesDir,
+  mustCall,
+  mustNotCall,
+  platformTimeout
+} = require('../common');
 const { strictEqual } = require('assert');
 const { join } = require('path');
 const { readFileSync } = require('fs');
 const { createSecureContext } = require('tls');
 const { createSecureServer, connect } = require('http2');
 const { get } = require('https');
 const { parse } = require('url');
+const { connect: tls } = require('tls');
 
 const countdown = (count, done) => () => --count === 0 && done();
 
+function expire(callback, ttl) {
+  const timeout = setTimeout(
+    mustNotCall('Callback expired'),
+    platformTimeout(ttl)
+  );
+  return function expire() {
+    clearTimeout(timeout);
+    return callback();
+  };
+}
+
 function loadKey(keyname) {
   return readFileSync(join(fixturesDir, 'keys', keyname));
 }
@@ -68,7 +85,7 @@ function onSession(session) {
   server.listen(0);
 
   server.on('listening', mustCall(() => {
-    const port = server.address().port;
+    const { port } = server.address();
     const origin = `https://localhost:${port}`;
 
     const cleanup = countdown(2, () => server.close());
@@ -110,13 +127,17 @@ function onSession(session) {
     mustCall(onRequest)
   );
 
+  server.on('unknownProtocol', mustCall((socket) => {
+    socket.destroy();
+  }, 2));
+
   server.listen(0);
 
   server.on('listening', mustCall(() => {
-    const port = server.address().port;
+    const { port } = server.address();
     const origin = `https://localhost:${port}`;
 
-    const cleanup = countdown(2, () => server.close());
+    const cleanup = countdown(3, () => server.close());
 
     // HTTP/2 client
     connect(
@@ -128,5 +149,9 @@ function onSession(session) {
     // HTTP/1.1 client
     get(Object.assign(parse(origin), clientOptions), mustNotCall())
       .on('error', mustCall(cleanup));
+
+    // Incompatible ALPN TLS client
+    tls(Object.assign({ port, ALPNProtocols: ['fake'] }, clientOptions))
+      .on('error', expire(mustCall(cleanup), 200));
   }));
 }