From 1dc438fd8b1dd9471de5504c91df58b730b63304 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 11 Jan 2017 21:37:28 -0500 Subject: [PATCH] crypto: remove unused access of tlsext_hostname The return value of loadSession is ultimately ignored, so don't fill it in. This inches Node closer to 1.1.0 compatibility and is less code. Also remove a comment which appears to have long since become invalid. It dates to 048e0e77e0c341407ecea364cbe26c8f77be48b8 when the SNI value was actually extracted from the session. This also fixes a segfault should d2i_SSL_SESSION fail to parse the input and return NULL. Add a test for this case based on test-tls-session-cache.js. PR-URL: https://github.com/nodejs/node/pull/10882 Reviewed-By: Ben Noordhuis Reviewed-By: Fedor Indutny Reviewed-By: James M Snell Reviewed-By: Sam Roberts --- lib/_tls_wrap.js | 10 +++----- src/node_crypto.cc | 11 -------- test/parallel/test-tls-session-cache.js | 34 +++++++++++++++++++++---- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 721cdde142519c..83a0f109939dc5 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -66,12 +66,8 @@ function loadSession(self, hello, cb) { if (!self._handle) return cb(new Error('Socket is closed')); - // NOTE: That we have disabled OpenSSL's internal session storage in - // `node_crypto.cc` and hence its safe to rely on getting servername only - // from clienthello or this place. - var ret = self._handle.loadSession(session); - - cb(null, ret); + self._handle.loadSession(session); + cb(null); } if (hello.sessionId.length <= 0 || @@ -148,7 +144,7 @@ function requestOCSP(self, hello, ctx, cb) { function onclienthello(hello) { var self = this; - loadSession(self, hello, function(err, session) { + loadSession(self, hello, function(err) { if (err) return self.destroy(err); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 1dfc77a8cbbb5a..072a48f40c58dc 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1826,17 +1826,6 @@ void SSLWrap::LoadSession(const FunctionCallbackInfo& args) { if (w->next_sess_ != nullptr) SSL_SESSION_free(w->next_sess_); w->next_sess_ = sess; - - Local info = Object::New(env->isolate()); -#ifndef OPENSSL_NO_TLSEXT - if (sess->tlsext_hostname == nullptr) { - info->Set(env->servername_string(), False(args.GetIsolate())); - } else { - info->Set(env->servername_string(), - OneByteString(args.GetIsolate(), sess->tlsext_hostname)); - } -#endif - args.GetReturnValue().Set(info); } } diff --git a/test/parallel/test-tls-session-cache.js b/test/parallel/test-tls-session-cache.js index 0b2942215141e3..272978467167c7 100644 --- a/test/parallel/test-tls-session-cache.js +++ b/test/parallel/test-tls-session-cache.js @@ -13,7 +13,9 @@ if (!common.hasCrypto) { doTest({ tickets: false }, function() { doTest({ tickets: true }, function() { - console.error('all done'); + doTest({ tickets: false, invalidSession: true }, function() { + console.error('all done'); + }); }); }); @@ -23,6 +25,7 @@ function doTest(testOptions, callback) { const fs = require('fs'); const join = require('path').join; const spawn = require('child_process').spawn; + const Buffer = require('buffer').Buffer; const keyFile = join(common.fixturesDir, 'agent.key'); const certFile = join(common.fixturesDir, 'agent.crt'); @@ -36,6 +39,7 @@ function doTest(testOptions, callback) { }; let requestCount = 0; let resumeCount = 0; + let newSessionCount = 0; let session; const server = tls.createServer(options, function(cleartext) { @@ -50,6 +54,7 @@ function doTest(testOptions, callback) { cleartext.end(); }); server.on('newSession', function(id, data, cb) { + ++newSessionCount; // Emulate asynchronous store setTimeout(function() { assert.ok(!session); @@ -65,9 +70,17 @@ function doTest(testOptions, callback) { assert.ok(session); assert.strictEqual(session.id.toString('hex'), id.toString('hex')); + let data = session.data; + + // Return an invalid session to test Node does not crash. + if (testOptions.invalidSession) { + data = Buffer.from('INVALID SESSION'); + session = null; + } + // Just to check that async really works there setTimeout(function() { - callback(null, session.data); + callback(null, data); }, 100); }); @@ -118,14 +131,25 @@ function doTest(testOptions, callback) { }); process.on('exit', function() { + // Each test run connects 6 times: an initial request and 5 reconnect + // requests. + assert.strictEqual(requestCount, 6); + if (testOptions.tickets) { - assert.strictEqual(requestCount, 6); + // No session cache callbacks are called. assert.strictEqual(resumeCount, 0); + assert.strictEqual(newSessionCount, 0); + } else if (testOptions.invalidSession) { + // The resume callback was called, but each connection established a + // fresh session. + assert.strictEqual(resumeCount, 5); + assert.strictEqual(newSessionCount, 6); } else { - // initial request + reconnect requests (5 times) + // The resume callback was called, and only the initial connection + // establishes a fresh session. assert.ok(session); - assert.strictEqual(requestCount, 6); assert.strictEqual(resumeCount, 5); + assert.strictEqual(newSessionCount, 1); } }); }