Skip to content

Commit

Permalink
crypto: remove unused access of tlsext_hostname
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
  • Loading branch information
davidben authored and sam-github committed Feb 13, 2017
1 parent 5ffb7d7 commit e34ee1d
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 23 deletions.
10 changes: 3 additions & 7 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand Down Expand Up @@ -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);

Expand Down
11 changes: 0 additions & 11 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
34 changes: 29 additions & 5 deletions test/parallel/test-tls-session-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});

Expand All @@ -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');
Expand All @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
});

Expand Down Expand Up @@ -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 e34ee1d

Please sign in to comment.