From 7656d58eed6524a5b1750fb0be86e67d338d7f90 Mon Sep 17 00:00:00 2001
From: Sam Roberts <vieuxtech@gmail.com>
Date: Wed, 30 Jan 2019 12:18:04 -0800
Subject: [PATCH] tls: introduce client 'session' event

OpenSSL has supported async notification of sessions and tickets since
1.1.0 using SSL_CTX_sess_set_new_cb(), for all versions of TLS. Using
the async API is optional for TLS1.2 and below, but for TLS1.3 it will
be mandatory. Future-proof applications should start to use async
notification immediately. In the future, for TLS1.3, applications that
don't use the async API will silently, but gracefully, fail to resume
sessions and instead do a full handshake.

See: https://wiki.openssl.org/index.php/TLS1.3#Sessions

PR-URL: https://github.com/nodejs/node/pull/25831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
---
 doc/api/tls.md                                | 49 +++++++++++++++++--
 lib/_tls_wrap.js                              | 21 ++++++++
 lib/https.js                                  | 24 ++++-----
 src/node_crypto.cc                            |  6 ++-
 src/tls_wrap.cc                               |  5 ++
 test/parallel/test-https-client-resume.js     | 27 +++++-----
 .../test-tls-async-cb-after-socket-end.js     | 10 +++-
 test/parallel/test-tls-client-resume.js       | 42 +++++++++-------
 test/parallel/test-tls-ticket-cluster.js      |  3 +-
 test/parallel/test-tls-ticket.js              | 10 ++++
 10 files changed, 148 insertions(+), 49 deletions(-)

diff --git a/doc/api/tls.md b/doc/api/tls.md
index 10300182e2e07a..0e8eb30cef0c41 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -152,9 +152,9 @@ will create a new session. See [RFC 2246][] for more information, page 23 and
 Resumption using session identifiers is supported by most web browsers when
 making HTTPS requests.
 
-For Node.js, clients must call [`tls.TLSSocket.getSession()`][] after the
-[`'secureConnect'`][] event to get the session data, and provide the data to the
-`session` option of [`tls.connect()`][] to reuse the session. Servers must
+For Node.js, clients wait for the [`'session'`][] event to get the session data,
+and provide the data to the `session` option of a subsequent [`tls.connect()`][]
+to reuse the session. Servers must
 implement handlers for the [`'newSession'`][] and [`'resumeSession'`][] events
 to save and restore the session data using the session ID as the lookup key to
 reuse sessions. To reuse sessions across load balancers or cluster workers,
@@ -614,6 +614,45 @@ determine if the server certificate was signed by one of the specified CAs. If
 `tlsSocket.alpnProtocol` property can be checked to determine the negotiated
 protocol.
 
+### Event: 'session'
+<!-- YAML
+added: REPLACEME
+-->
+
+* `session` {Buffer}
+
+The `'session'` event is emitted on a client `tls.TLSSocket` when a new session
+or TLS ticket is available. This may or may not be before the handshake is
+complete, depending on the TLS protocol version that was negotiated. The event
+is not emitted on the server, or if a new session was not created, for example,
+when the connection was resumed. For some TLS protocol versions the event may be
+emitted multiple times, in which case all the sessions can be used for
+resumption.
+
+On the client, the `session` can be provided to the `session` option of
+[`tls.connect()`][] to resume the connection.
+
+See [Session Resumption][] for more information.
+
+Note: For TLS1.2 and below, [`tls.TLSSocket.getSession()`][] can be called once
+the handshake is complete.  For TLS1.3, only ticket based resumption is allowed
+by the protocol, multiple tickets are sent, and the tickets aren't sent until
+later, after the handshake completes, so it is necessary to wait for the
+`'session'` event to get a resumable session.  Future-proof applications are
+recommended to use the `'session'` event instead of `getSession()` to ensure
+they will work for all TLS protocol versions.  Applications that only expect to
+get or use 1 session should listen for this event only once:
+
+```js
+tlsSocket.once('session', (session) => {
+  // The session can be used immediately or later.
+  tls.connect({
+    session: session,
+    // Other connect options...
+  });
+});
+```
+
 ### tlsSocket.address()
 <!-- YAML
 added: v0.11.4
@@ -880,6 +919,9 @@ for debugging.
 
 See [Session Resumption][] for more information.
 
+Note: `getSession()` works only for TLS1.2 and below. Future-proof applications
+should use the [`'session'`][] event.
+
 ### tlsSocket.getTLSTicket()
 <!-- YAML
 added: v0.11.4
@@ -1538,6 +1580,7 @@ where `secureSocket` has the same API as `pair.cleartext`.
 [`'resumeSession'`]: #tls_event_resumesession
 [`'secureConnect'`]: #tls_event_secureconnect
 [`'secureConnection'`]: #tls_event_secureconnection
+[`'session'`]: #tls_event_session
 [`--tls-cipher-list`]: cli.html#cli_tls_cipher_list_list
 [`NODE_OPTIONS`]: cli.html#cli_node_options_options
 [`crypto.getCurves()`]: crypto.html#crypto_crypto_getcurves
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index ad5d8c5f57893a..43947bb8c68b91 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -212,6 +212,12 @@ function requestOCSPDone(socket) {
 }
 
 
+function onnewsessionclient(sessionId, session) {
+  debug('client onnewsessionclient', sessionId, session);
+  const owner = this[owner_symbol];
+  owner.emit('session', session);
+}
+
 function onnewsession(sessionId, session) {
   const owner = this[owner_symbol];
 
@@ -512,6 +518,21 @@ TLSSocket.prototype._init = function(socket, wrap) {
 
     if (options.session)
       ssl.setSession(options.session);
+
+    ssl.onnewsession = onnewsessionclient;
+
+    // Only call .onnewsession if there is a session listener.
+    this.on('newListener', newListener);
+
+    function newListener(event) {
+      if (event !== 'session')
+        return;
+
+      ssl.enableSessionCallbacks();
+
+      // Remover this listener since its no longer needed.
+      this.removeListener('newListener', newListener);
+    }
   }
 
   ssl.onerror = onerror;
diff --git a/lib/https.js b/lib/https.js
index 96111176994a25..041eb4241cdb15 100644
--- a/lib/https.js
+++ b/lib/https.js
@@ -117,18 +117,20 @@ function createConnection(port, host, options) {
     }
   }
 
-  const socket = tls.connect(options, () => {
-    if (!options._agentKey)
-      return;
+  const socket = tls.connect(options);
 
-    this._cacheSession(options._agentKey, socket.getSession());
-  });
-
-  // Evict session on error
-  socket.once('close', (err) => {
-    if (err)
-      this._evictSession(options._agentKey);
-  });
+  if (options._agentKey) {
+    // Cache new session for reuse
+    socket.on('session', (session) => {
+      this._cacheSession(options._agentKey, session);
+    });
+
+    // Evict session on error
+    socket.once('close', (err) => {
+      if (err)
+        this._evictSession(options._agentKey);
+    });
+  }
 
   return socket;
 }
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 76b6d4d5ec5c8b..297bf7f602c4ab 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -495,6 +495,7 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
 
   // SSL session cache configuration
   SSL_CTX_set_session_cache_mode(sc->ctx_.get(),
+                                 SSL_SESS_CACHE_CLIENT |
                                  SSL_SESS_CACHE_SERVER |
                                  SSL_SESS_CACHE_NO_INTERNAL |
                                  SSL_SESS_CACHE_NO_AUTO_CLEAR);
@@ -1513,7 +1514,10 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
       reinterpret_cast<const char*>(session_id_data),
       session_id_length).ToLocalChecked();
   Local<Value> argv[] = { session_id, session };
-  w->awaiting_new_session_ = true;
+  // On servers, we pause the handshake until callback of 'newSession', which
+  // calls NewSessionDoneCb(). On clients, there is no callback to wait for.
+  if (w->is_server())
+    w->awaiting_new_session_ = true;
   w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);
 
   return 0;
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index c332b124d44122..5fe99db937d308 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -756,6 +756,11 @@ void TLSWrap::EnableSessionCallbacks(
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
   CHECK_NOT_NULL(wrap->ssl_);
   wrap->enable_session_callbacks();
+
+  // Clients don't use the HelloParser.
+  if (wrap->is_client())
+    return;
+
   crypto::NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength);
   wrap->hello_parser_.Start(SSLWrap<TLSWrap>::OnClientHello,
                             OnClientHelloParseEnd,
diff --git a/test/parallel/test-https-client-resume.js b/test/parallel/test-https-client-resume.js
index 04a89364fe0e94..cf1bbdf2626823 100644
--- a/test/parallel/test-https-client-resume.js
+++ b/test/parallel/test-https-client-resume.js
@@ -43,37 +43,34 @@ const server = https.createServer(options, common.mustCall((req, res) => {
 }, 2));
 
 // start listening
-server.listen(0, function() {
-
-  let session1 = null;
+server.listen(0, common.mustCall(function() {
   const client1 = tls.connect({
     port: this.address().port,
     rejectUnauthorized: false
-  }, () => {
+  }, common.mustCall(() => {
     console.log('connect1');
-    assert.ok(!client1.isSessionReused(), 'Session *should not* be reused.');
-    session1 = client1.getSession();
+    assert.strictEqual(client1.isSessionReused(), false);
     client1.write('GET / HTTP/1.0\r\n' +
                   'Server: 127.0.0.1\r\n' +
                   '\r\n');
-  });
+  }));
 
-  client1.on('close', () => {
-    console.log('close1');
+  client1.on('session', common.mustCall((session) => {
+    console.log('session');
 
     const opts = {
       port: server.address().port,
       rejectUnauthorized: false,
-      session: session1
+      session,
     };
 
-    const client2 = tls.connect(opts, () => {
+    const client2 = tls.connect(opts, common.mustCall(() => {
       console.log('connect2');
-      assert.ok(client2.isSessionReused(), 'Session *should* be reused.');
+      assert.strictEqual(client2.isSessionReused(), true);
       client2.write('GET / HTTP/1.0\r\n' +
                     'Server: 127.0.0.1\r\n' +
                     '\r\n');
-    });
+    }));
 
     client2.on('close', () => {
       console.log('close2');
@@ -81,7 +78,7 @@ server.listen(0, function() {
     });
 
     client2.resume();
-  });
+  }));
 
   client1.resume();
-});
+}));
diff --git a/test/parallel/test-tls-async-cb-after-socket-end.js b/test/parallel/test-tls-async-cb-after-socket-end.js
index 6ca38461fdec6a..5c812c8f0432ea 100644
--- a/test/parallel/test-tls-async-cb-after-socket-end.js
+++ b/test/parallel/test-tls-async-cb-after-socket-end.js
@@ -6,9 +6,15 @@ const fixtures = require('../common/fixtures');
 const SSL_OP_NO_TICKET = require('crypto').constants.SSL_OP_NO_TICKET;
 const tls = require('tls');
 
-// Check tls async callback after socket ends
+// Check that TLS1.2 session resumption callbacks don't explode when made after
+// the tls socket is destroyed. Disable TLS ticket support to force the legacy
+// session resumption mechanism to be used.
+
+// TLS1.2 is the last protocol version to support TLS sessions, after that the
+// new and resume session events will never be emitted on the server.
 
 const options = {
+  maxVersion: 'TLSv1.2',
   secureOptions: SSL_OP_NO_TICKET,
   key: fixtures.readSync('test_key.pem'),
   cert: fixtures.readSync('test_cert.pem')
@@ -25,6 +31,8 @@ server.on('newSession', common.mustCall((key, session, done) => {
 
 server.on('resumeSession', common.mustCall((id, cb) => {
   sessionCb = cb;
+  // Destroy the client and then call the session cb, to check that the cb
+  // doesn't explode when called after the handle has been destroyed.
   next();
 }));
 
diff --git a/test/parallel/test-tls-client-resume.js b/test/parallel/test-tls-client-resume.js
index db4c898d746790..9f868fdcdc0a49 100644
--- a/test/parallel/test-tls-client-resume.js
+++ b/test/parallel/test-tls-client-resume.js
@@ -20,9 +20,9 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 'use strict';
-// Create an ssl server.  First connection, validate that not resume.
-// Cache session and close connection.  Use session on second connection.
-// ASSERT resumption.
+
+// Check that the ticket from the first connection causes session resumption
+// when used to make a second connection.
 
 const common = require('../common');
 if (!common.hasCrypto)
@@ -43,20 +43,28 @@ const server = tls.Server(options, common.mustCall((socket) => {
 }, 2));
 
 // start listening
-server.listen(0, function() {
+server.listen(0, common.mustCall(function() {
 
+  let sessionx = null;
   let session1 = null;
   const client1 = tls.connect({
     port: this.address().port,
     rejectUnauthorized: false
-  }, () => {
+  }, common.mustCall(() => {
     console.log('connect1');
-    assert.ok(!client1.isSessionReused(), 'Session *should not* be reused.');
-    session1 = client1.getSession();
-  });
+    assert.strictEqual(client1.isSessionReused(), false);
+    sessionx = client1.getSession();
+  }));
+
+  client1.once('session', common.mustCall((session) => {
+    console.log('session1');
+    session1 = session;
+  }));
 
-  client1.on('close', () => {
-    console.log('close1');
+  client1.on('close', common.mustCall(() => {
+    assert(sessionx);
+    assert(session1);
+    assert.strictEqual(sessionx.compare(session1), 0);
 
     const opts = {
       port: server.address().port,
@@ -64,18 +72,18 @@ server.listen(0, function() {
       session: session1
     };
 
-    const client2 = tls.connect(opts, () => {
+    const client2 = tls.connect(opts, common.mustCall(() => {
       console.log('connect2');
-      assert.ok(client2.isSessionReused(), 'Session *should* be reused.');
-    });
+      assert.strictEqual(client2.isSessionReused(), true);
+    }));
 
-    client2.on('close', () => {
+    client2.on('close', common.mustCall(() => {
       console.log('close2');
       server.close();
-    });
+    }));
 
     client2.resume();
-  });
+  }));
 
   client1.resume();
-});
+}));
diff --git a/test/parallel/test-tls-ticket-cluster.js b/test/parallel/test-tls-ticket-cluster.js
index 32a5c671d800dd..98fe533b6969d6 100644
--- a/test/parallel/test-tls-ticket-cluster.js
+++ b/test/parallel/test-tls-ticket-cluster.js
@@ -45,7 +45,6 @@ if (cluster.isMaster) {
       session: lastSession,
       rejectUnauthorized: false
     }, () => {
-      lastSession = c.getSession();
       c.end();
 
       if (++reqCount === expectedReqCount) {
@@ -55,6 +54,8 @@ if (cluster.isMaster) {
       } else {
         shoot();
       }
+    }).once('session', (session) => {
+      lastSession = session;
     });
   }
 
diff --git a/test/parallel/test-tls-ticket.js b/test/parallel/test-tls-ticket.js
index 187dd22cee6b76..d11535dd3a5255 100644
--- a/test/parallel/test-tls-ticket.js
+++ b/test/parallel/test-tls-ticket.js
@@ -81,6 +81,15 @@ const shared = net.createServer(function(c) {
   });
 });
 
+// 'session' events only occur for new sessions. The first connection is new.
+// After, for each set of 3 connections, the middle connection is made when the
+// server has random keys set, so the client's ticket is silently ignored, and a
+// new ticket is sent.
+const onNewSession = common.mustCall((s, session) => {
+  assert(session);
+  assert.strictEqual(session.compare(s.getSession()), 0);
+}, 4);
+
 function start(callback) {
   let sess = null;
   let left = servers.length;
@@ -99,6 +108,7 @@ function start(callback) {
       else
         connect();
     });
+    s.once('session', (session) => onNewSession(s, session));
   }
 
   connect();