Skip to content

Commit

Permalink
https: evict cached sessions on error
Browse files Browse the repository at this point in the history
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
  • Loading branch information
indutny committed Feb 2, 2016
1 parent 7a2a551 commit 165b33f
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 0 deletions.
16 changes: 16 additions & 0 deletions lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ function createConnection(port, host, options) {

self._cacheSession(options._agentKey, socket.getSession());
});

// Evict session on error
socket.once('close', (err) => {
if (err)
this._evictSession(options._agentKey);
});

return socket;
}

Expand Down Expand Up @@ -164,6 +171,15 @@ Agent.prototype._cacheSession = function _cacheSession(key, session) {
this._sessionCache.map[key] = session;
};

Agent.prototype._evictSession = function _evictSession(key) {
const index = this._sessionCache.list.indexOf(key);
if (index === -1)
return;

this._sessionCache.list.splice(index, 1);
delete this._sessionCache.map[key];
};

const globalAgent = new Agent();

exports.globalAgent = globalAgent;
Expand Down
88 changes: 88 additions & 0 deletions test/parallel/test-https-agent-session-eviction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
'use strict';

const common = require('../common');

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
return;
}

const assert = require('assert');
const https = require('https');
const fs = require('fs');
const constants = require('constants');

const options = {
key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'),
secureOptions: constants.SSL_OP_NO_TICKET
};

// Create TLS1.2 server
https.createServer(options, function(req, res) {
res.end('ohai');
}).listen(common.PORT, function() {
first(this);
});

// Do request and let agent cache the session
function first(server) {
const req = https.request({
port: common.PORT,
rejectUnauthorized: false
}, function(res) {
res.resume();

server.close(function() {
faultyServer();
});
});
req.end();
}

// Create TLS1 server
function faultyServer() {
options.secureProtocol = 'TLSv1_method';
https.createServer(options, function(req, res) {
res.end('hello faulty');
}).listen(common.PORT, function() {
second(this);
});
}

// Attempt to request using cached session
function second(server, session) {
const req = https.request({
port: common.PORT,
rejectUnauthorized: false
}, function(res) {
res.resume();
});

// Let it fail
req.on('error', common.mustCall(function(err) {
assert(/wrong version number/.test(err.message));

req.on('close', function() {
third(server);
});
}));
req.end();
}

// Try on more time - session should be evicted!
function third(server) {
const req = https.request({
port: common.PORT,
rejectUnauthorized: false
}, function(res) {
res.resume();
assert(!req.socket.isSessionReused());
server.close();
});
req.on('error', function(err) {
// never called
assert(false);
});
req.end();
}

5 comments on commit 165b33f

@klinquist
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be backported into 4.2?

@indutny
Copy link
Member Author

@indutny indutny commented on 165b33f Feb 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

@klinquist
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@kapouer
Copy link
Contributor

@kapouer kapouer commented on 165b33f Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how the first request put any session in the cache, since server is configured with
secureOptions: constants.SSL_OP_NO_TICKET.
I'm reading this test because it fails with #8491

@indutny
Copy link
Member Author

@indutny indutny commented on 165b33f Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kapouer there are two types of sessions, old SSL sessions and new TLS tickets. That flag disables the latter ones so that the test could use first ones exclusively.

Please sign in to comment.