Permalink
Browse files

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 048e0e7 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: #10882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
  • Loading branch information...
davidben authored and italoacasas committed Jan 12, 2017
1 parent 1085a46 commit 1dc438fd8b1dd9471de5504c91df58b730b63304
Showing with 32 additions and 23 deletions.
  1. +3 −7 lib/_tls_wrap.js
  2. +0 −11 src/node_crypto.cc
  3. +29 −5 test/parallel/test-tls-session-cache.js
View
@@ -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);
View
@@ -1826,17 +1826,6 @@ void SSLWrap<Base>::LoadSession(const FunctionCallbackInfo<Value>& args) {
if (w->next_sess_ != nullptr)
SSL_SESSION_free(w->next_sess_);
w->next_sess_ = sess;
-
- Local<Object> 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);
}
}
@@ -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);
}
});
}

0 comments on commit 1dc438f

Please sign in to comment.