From 14876f176286c93650601fe474e715469eb5181a Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 4 Nov 2014 11:14:55 -0500 Subject: [PATCH 1/2] tls: do not hang without `newSession` handler When listening for client hello parser events (like OCSP requests), do not hang if `newSession` event handler is not present. Fix: https://github.com/joyent/node/issues/8660 Fix: https://github.com/joyent/node/issues/25735 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell PR-URL: https://github.com/node-forward/node/pull/47 --- lib/_tls_legacy.js | 7 +++++-- lib/_tls_wrap.js | 7 +++++-- test/simple/test-tls-ocsp-callback.js | 28 ++++++++++++++++++++------- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/_tls_legacy.js b/lib/_tls_legacy.js index 6dc5c3493cfc..347b95e63456 100644 --- a/lib/_tls_legacy.js +++ b/lib/_tls_legacy.js @@ -662,14 +662,17 @@ function onnewsession(key, session) { var self = this; var once = false; - self.server.emit('newSession', key, session, function() { + if (!self.server.emit('newSession', key, session, done)) + done(); + + function done() { if (once) return; once = true; if (self.ssl) self.ssl.newSessionDone(); - }); + }; } diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 6e2a430840a9..441776ad8791 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -201,7 +201,10 @@ function onnewsession(key, session) { var once = false; this._newSessionPending = true; - this.server.emit('newSession', key, session, function() { + if (!this.server.emit('newSession', key, session, done)) + done(); + + function done() { if (once) return; once = true; @@ -212,7 +215,7 @@ function onnewsession(key, session) { if (self._securePending) self._finishInit(); self._securePending = false; - }); + } } diff --git a/test/simple/test-tls-ocsp-callback.js b/test/simple/test-tls-ocsp-callback.js index fd45586b5ea0..4c7dd1d4423f 100644 --- a/test/simple/test-tls-ocsp-callback.js +++ b/test/simple/test-tls-ocsp-callback.js @@ -31,16 +31,19 @@ if (!common.opensslCli) { process.exit(0); } +var assert = require('assert'); +var tls = require('tls'); +var constants = require('constants'); +var fs = require('fs'); +var join = require('path').join; + test({ response: false }, function() { - test({ response: 'hello world' }); + test({ response: 'hello world' }, function() { + test({ ocsp: false }); + }); }); function test(testOptions, cb) { - var assert = require('assert'); - var tls = require('tls'); - var fs = require('fs'); - var join = require('path').join; - var spawn = require('child_process').spawn; var keyFile = join(common.fixturesDir, 'keys', 'agent1-key.pem'); var certFile = join(common.fixturesDir, 'keys', 'agent1-cert.pem'); @@ -54,6 +57,7 @@ function test(testOptions, cb) { ca: [ca] }; var requestCount = 0; + var clientSecure = 0; var ocspCount = 0; var ocspResponse; var session; @@ -83,9 +87,12 @@ function test(testOptions, cb) { server.listen(common.PORT, function() { var client = tls.connect({ port: common.PORT, - requestOCSP: true, + requestOCSP: testOptions.ocsp !== false, + secureOptions: testOptions.ocsp === false ? + constants.SSL_OP_NO_TICKET : 0, rejectUnauthorized: false }, function() { + clientSecure++; }); client.on('OCSPResponse', function(resp) { ocspResponse = resp; @@ -98,12 +105,19 @@ function test(testOptions, cb) { }); process.on('exit', function() { + if (testOptions.ocsp === false) { + assert.equal(requestCount, clientSecure); + assert.equal(requestCount, 1); + return; + } + if (testOptions.response) { assert.equal(ocspResponse.toString(), testOptions.response); } else { assert.ok(ocspResponse === null); } assert.equal(requestCount, testOptions.response ? 0 : 1); + assert.equal(clientSecure, requestCount); assert.equal(ocspCount, 1); }); } From 16c02f02b8b66742b4c0ab197f83e17210c58051 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 18 Jul 2015 12:36:20 -0700 Subject: [PATCH 2/2] test: add regression test for #25735 See: https://github.com/joyent/node/issues/25736 --- test/simple/test-tls-new-session-hang.js | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 test/simple/test-tls-new-session-hang.js diff --git a/test/simple/test-tls-new-session-hang.js b/test/simple/test-tls-new-session-hang.js new file mode 100644 index 000000000000..6fdf89817ab6 --- /dev/null +++ b/test/simple/test-tls-new-session-hang.js @@ -0,0 +1,42 @@ +var common = require('../common'); + +if (!process.features.tls_ocsp) { + console.error('Skipping because node compiled without OpenSSL or ' + + 'with old OpenSSL version.'); + process.exit(0); +} + +var assert = require('assert'); +var tls = require('tls'); +var constants = require('constants'); +var fs = require('fs'); +var join = require('path').join; + +var keyFile = join(common.fixturesDir, 'keys', 'agent1-key.pem'); +var certFile = join(common.fixturesDir, 'keys', 'agent1-cert.pem'); +var caFile = join(common.fixturesDir, 'keys', 'ca1-cert.pem'); +var key = fs.readFileSync(keyFile); +var cert = fs.readFileSync(certFile); + +var server = tls.createServer({ + cert: cert, + key: key +}, function (socket) { + socket.destroySoon(); +}); + +server.on('resumeSession', common.mustCall(function() { + // Should not be actually called +}, 0)); + +server.listen(common.PORT, function() { + var client = tls.connect({ + rejectUnauthorized: false, + port: common.PORT, + + // Just to make sure that `newSession` is going to be called + secureOptions: constants.SSL_OP_NO_TICKET + }, function() { + server.close(); + }); +});