Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

https: reuse TLS sessions in Agent #2228

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ Agent.prototype.createSocket = function(req, options) {
}

var name = self.getName(options);
options._agentKey = name;

debug('createConnection', name, options);
options.encoding = null;
Expand Down
14 changes: 13 additions & 1 deletion lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,17 @@ TLSSocket.prototype._start = function() {
this._handle.start();
};

TLSSocket.prototype._isSessionResumed = function _isSessionResumed(session) {
if (!session)
return false;

var next = this.getSession();
if (!next)
return false;

return next.equals(session);
};

TLSSocket.prototype.setServername = function(name) {
this._handle.setServername(name);
};
Expand Down Expand Up @@ -999,7 +1010,8 @@ exports.connect = function(/* [port, host], options, cb */) {
var verifyError = socket._handle.verifyError();

// Verify that server's identity matches it's certificate's names
if (!verifyError) {
// Unless server has resumed our existing session
if (!verifyError && !socket._isSessionResumed(options.session)) {
var cert = socket.getPeerCertificate();
verifyError = options.checkServerIdentity(hostname, cert);
}
Expand Down
50 changes: 49 additions & 1 deletion lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,40 @@ function createConnection(port, host, options) {
}

debug('createConnection', options);
return tls.connect(options);

if (options._agentKey) {
const session = this._getSession(options._agentKey);
if (session) {
debug('reuse session for %j', options._agentKey);
options = util._extend({
session: session
}, options);
}
}

const self = this;
const socket = tls.connect(options, function() {
if (!options._agentKey)
return;

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


function Agent(options) {
http.Agent.call(this, options);
this.defaultPort = 443;
this.protocol = 'https:';
this.maxCachedSessions = this.options.maxCachedSessions;
if (this.maxCachedSessions === undefined)
this.maxCachedSessions = 100;

this._sessionCache = {
map: {},
list: []
};
}
inherits(Agent, http.Agent);
Agent.prototype.createConnection = createConnection;
Expand Down Expand Up @@ -100,6 +126,28 @@ Agent.prototype.getName = function(options) {
return name;
};

Agent.prototype._getSession = function _getSession(key) {
return this._sessionCache.map[key];
};

Agent.prototype._cacheSession = function _cacheSession(key, session) {
// Fast case - update existing entry
if (this._sessionCache.map[key]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allow inherited properties, is that okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, we generate key ourselves.

this._sessionCache.map[key] = session;
return;
}

// Put new entry
if (this._sessionCache.list.length >= this.maxCachedSessions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can list.length really ever be greater than this.maxCachedSessions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, easily. This is why I have this list in first place.

const oldKey = this._sessionCache.list.shift();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of shift()ing here, what if we used the "spliceOne" trick and just copied the data from indices 1+ down one position and then copied the new key to this._sessionCache.list.length - 1 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Premature optimization? This is a list of length 100, and it is unlikely that it will ever become a bottleneck. Especially, considering that how much time TLS handshake takes.

cc @bnoordhuis @trevnorris

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the probable size this will have to iterate over, I wouldn't worry about further optimization for the time being.

debug('evicting %j', oldKey);
delete this._sessionCache.map[oldKey];
Copy link
Contributor

Choose a reason for hiding this comment

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

And this will likely kill any gains in micro-optimizations you do anyway.

}

this._sessionCache.list.push(key);
this._sessionCache.map[key] = session;
};

const globalAgent = new Agent();

exports.globalAgent = globalAgent;
Expand Down
130 changes: 130 additions & 0 deletions test/parallel/test-https-agent-session-reuse.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
'use strict';
var common = require('../common');
var assert = require('assert');

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

var https = require('https');
var crypto = require('crypto');

var fs = require('fs');

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

var ca = fs.readFileSync(common.fixturesDir + '/keys/ca1-cert.pem');

var clientSessions = {};
var serverRequests = 0;

var agent = new https.Agent({
maxCachedSessions: 1
});

var server = https.createServer(options, function(req, res) {
if (req.url === '/drop-key')
server.setTicketKeys(crypto.randomBytes(48));

serverRequests++;
res.end('ok');
}).listen(common.PORT, function() {
var queue = [
{
name: 'first',

method: 'GET',
path: '/',
servername: 'agent1',
ca: ca,
port: common.PORT
},
{
name: 'first-reuse',

method: 'GET',
path: '/',
servername: 'agent1',
ca: ca,
port: common.PORT
},
{
name: 'cipher-change',

method: 'GET',
path: '/',
servername: 'agent1',

// Choose different cipher to use different cache entry
ciphers: 'AES256-SHA',
ca: ca,
port: common.PORT
},
// Change the ticket key to ensure session is updated in cache
{
name: 'before-drop',

method: 'GET',
path: '/drop-key',
servername: 'agent1',
ca: ca,
port: common.PORT
},

// Ticket will be updated starting from this
{
name: 'after-drop',

method: 'GET',
path: '/',
servername: 'agent1',
ca: ca,
port: common.PORT
},
{
name: 'after-drop-reuse',

method: 'GET',
path: '/',
servername: 'agent1',
ca: ca,
port: common.PORT
}
];

function request() {
var options = queue.shift();
options.agent = agent;
https.request(options, function(res) {
clientSessions[options.name] = res.socket.getSession();

res.resume();
res.on('end', function() {
if (queue.length !== 0)
return request();
server.close();
});
}).end();
}
request();
});

process.on('exit', function() {
assert.equal(serverRequests, 6);
assert.equal(clientSessions['first'].toString('hex'),
clientSessions['first-reuse'].toString('hex'));
assert.notEqual(clientSessions['first'].toString('hex'),
clientSessions['cipher-change'].toString('hex'));
assert.notEqual(clientSessions['first'].toString('hex'),
clientSessions['before-drop'].toString('hex'));
assert.notEqual(clientSessions['cipher-change'].toString('hex'),
clientSessions['before-drop'].toString('hex'));
assert.notEqual(clientSessions['before-drop'].toString('hex'),
clientSessions['after-drop'].toString('hex'));
assert.equal(clientSessions['after-drop'].toString('hex'),
clientSessions['after-drop-reuse'].toString('hex'));
});