From 74f1db9766450f1fbab930a93c835d6b8af6f3d5 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 11:12:19 -0800 Subject: [PATCH 01/11] test: wait for TCP connect, not TLS handshake Test assumed server gets a handshake before the client destroyed it, and didn't assert that dns.lookup() callback occurred. --- test/parallel/test-tls-connect-address-family.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-tls-connect-address-family.js b/test/parallel/test-tls-connect-address-family.js index 569688ca8abd18..083208cc1d3ab0 100644 --- a/test/parallel/test-tls-connect-address-family.js +++ b/test/parallel/test-tls-connect-address-family.js @@ -15,7 +15,7 @@ function runTest() { tls.createServer({ cert: fixtures.readKey('agent1-cert.pem'), key: fixtures.readKey('agent1-key.pem'), - }, common.mustCall(function() { + }).on('connection', common.mustCall(function() { this.close(); })).listen(0, '::1', common.mustCall(function() { const options = { @@ -32,7 +32,9 @@ function runTest() { })); } -dns.lookup('localhost', { family: 6, all: true }, (err, addresses) => { +dns.lookup('localhost', { + family: 6, all: true +}, common.mustCall((err, addresses) => { if (err) { if (err.code === 'ENOTFOUND' || err.code === 'EAI_AGAIN') common.skip('localhost does not resolve to ::1'); @@ -44,4 +46,4 @@ dns.lookup('localhost', { family: 6, all: true }, (err, addresses) => { runTest(); else common.skip('localhost does not resolve to ::1'); -}); +})); From 6c13102a9833c51ed3aca87d4fae51615a6b2777 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 11:14:06 -0800 Subject: [PATCH 02/11] test: do not assume server gets secure connection Test assumed that server got the the connection before the client destroys it, but that is not guaranteed. Also, the test was closing the TCP connection 3 times, effectively: 1. on the server side, right after TLS connection occurs (if it does) 2. on the client side, internal to tls, when the cert is rejected 3. again on the client side, in the error event which is emitted by the internal tls destroy from 2 This is too often, and the dependency on 1 occurring is fragile. --- test/parallel/test-tls-friendly-error-message.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-tls-friendly-error-message.js b/test/parallel/test-tls-friendly-error-message.js index b52918be27ac9b..4ae9d3f3f956d4 100644 --- a/test/parallel/test-tls-friendly-error-message.js +++ b/test/parallel/test-tls-friendly-error-message.js @@ -31,14 +31,15 @@ const tls = require('tls'); const key = fixtures.readKey('agent1-key.pem'); const cert = fixtures.readKey('agent1-cert.pem'); -tls.createServer({ key, cert }, common.mustCall(function(conn) { - conn.end(); +tls.createServer({ key, cert }).on('connection', common.mustCall(function() { + // Server only receives one TCP connection, stop listening when that + // connection is destroyed by the client, which it should do after the cert is + // rejected as unauthorized. this.close(); })).listen(0, common.mustCall(function() { const options = { port: this.address().port, rejectUnauthorized: true }; tls.connect(options).on('error', common.mustCall(function(err) { assert.strictEqual(err.code, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE'); assert.strictEqual(err.message, 'unable to verify the first certificate'); - this.destroy(); })); })); From 78565d79dabd975b3eb4e7fa599ad99d06f2f0a4 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 7 Jan 2019 16:30:31 -0800 Subject: [PATCH 03/11] test: do not assume tls handshake order Do not assume that server handshake event happens before client, it is not guaranteed. --- test/parallel/test-tls-alpn-server-client.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/parallel/test-tls-alpn-server-client.js b/test/parallel/test-tls-alpn-server-client.js index 2540831a38f25c..76604e05875c65 100644 --- a/test/parallel/test-tls-alpn-server-client.js +++ b/test/parallel/test-tls-alpn-server-client.js @@ -23,9 +23,10 @@ function runTest(clientsOptions, serverOptions, cb) { serverOptions.key = loadPEM('agent2-key'); serverOptions.cert = loadPEM('agent2-cert'); const results = []; - let index = 0; + let clientIndex = 0; + let serverIndex = 0; const server = tls.createServer(serverOptions, function(c) { - results[index].server = { ALPN: c.alpnProtocol }; + results[serverIndex++].server = { ALPN: c.alpnProtocol }; }); server.listen(0, serverIP, function() { @@ -38,16 +39,18 @@ function runTest(clientsOptions, serverOptions, cb) { opt.host = serverIP; opt.rejectUnauthorized = false; - results[index] = {}; + results[clientIndex] = {}; const client = tls.connect(opt, function() { - results[index].client = { ALPN: client.alpnProtocol }; - client.destroy(); + results[clientIndex].client = { ALPN: client.alpnProtocol }; + client.end(); if (options.length) { - index++; + clientIndex++; connectClient(options); } else { server.close(); - cb(results); + server.on('close', () => { + cb(results); + }); } }); } From c523483bf453e627e474b1a3f9fba8a36439b204 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 9 Jan 2019 14:03:31 -0800 Subject: [PATCH 04/11] test: do not race connection and rejection Existing code assumed that the server completed the handshake before the client rejected the certificate, and destroyed the socket. This assumption is fragile, remove it, and instead check explicitly that data can or cannot be exchanged via TLS, whichever is expected. --- test/parallel/test-tls-client-reject.js | 38 +++++++++++++++---------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-tls-client-reject.js b/test/parallel/test-tls-client-reject.js index 955d97da6fb6fc..9eff6cb9cead01 100644 --- a/test/parallel/test-tls-client-reject.js +++ b/test/parallel/test-tls-client-reject.js @@ -33,49 +33,57 @@ const options = { cert: fixtures.readSync('test_cert.pem') }; -const server = tls.createServer(options, common.mustCall(function(socket) { - socket.on('data', function(data) { - console.error(data.toString()); - assert.strictEqual(data.toString(), 'ok'); - }); -}, 3)).listen(0, function() { +const server = tls.createServer(options, function(socket) { + socket.pipe(socket); + socket.on('end', () => socket.end()); +}).listen(0, common.mustCall(function() { unauthorized(); -}); +})); function unauthorized() { + console.log('connect unauthorized'); const socket = tls.connect({ port: server.address().port, servername: 'localhost', rejectUnauthorized: false }, common.mustCall(function() { + console.log('... unauthorized'); assert(!socket.authorized); - socket.end(); - rejectUnauthorized(); + socket.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString(), 'ok'); + })); + socket.on('end', () => rejectUnauthorized()); })); socket.on('error', common.mustNotCall()); - socket.write('ok'); + socket.end('ok'); } function rejectUnauthorized() { + console.log('reject unauthorized'); const socket = tls.connect(server.address().port, { servername: 'localhost' }, common.mustNotCall()); + socket.on('data', common.mustNotCall()); socket.on('error', common.mustCall(function(err) { - console.error(err); + console.log('... rejected:', err); authorized(); })); - socket.write('ng'); + socket.end('ng'); } function authorized() { + console.log('connect authorized'); const socket = tls.connect(server.address().port, { ca: [fixtures.readSync('test_cert.pem')], servername: 'localhost' }, common.mustCall(function() { + console.log('... authorized'); assert(socket.authorized); - socket.end(); - server.close(); + socket.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString(), 'ok'); + })); + socket.on('end', () => server.close()); })); socket.on('error', common.mustNotCall()); - socket.write('ok'); + socket.end('ok'); } From e981f202bfc42f83255ee297bda9259293636382 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Tue, 15 Jan 2019 12:49:45 -0800 Subject: [PATCH 05/11] test: pin regression test for #8074 to TLS 1.2 This test has a dependency on the order in which the TCP connection is made, and TLS server handshake completes. It assumes those server side events occur before the client side write callback, which is not guaranteed by the TLS API. It usually passes with TLS1.3, but TLS1.3 didn't exist at the time the bug existed. Pin the test to TLS1.2, since the test shouldn't be changed in a way that doesn't trigger a segfault in 7.7.3: - https://github.com/nodejs/node/issues/13184#issuecomment-303700377 --- test/parallel/test-tls-socket-close.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js index 3b2e18184b408e..4fe58bc7f2dbf2 100644 --- a/test/parallel/test-tls-socket-close.js +++ b/test/parallel/test-tls-socket-close.js @@ -8,6 +8,19 @@ const tls = require('tls'); const net = require('net'); const fixtures = require('../common/fixtures'); +// Regression test for https://github.com/nodejs/node/issues/8074 +// +// This test has a dependency on the order in which the TCP connection is made, +// and TLS server handshake completes. It assumes those server side events occur +// before the client side write callback, which is not guaranteed by the TLS +// API. It usally passes with TLS1.3, but TLS1.3 didn't exist at the time the +// bug existed. +// +// Pin the test to TLS1.2, since the test shouldn't be changed in a way that +// doesn't trigger a segfault in Node.js 7.7.3: +// https://github.com/nodejs/node/issues/13184#issuecomment-303700377 +tls.DEFAULT_MAX_VERSION = 'TLSv1.2'; + const key = fixtures.readKey('agent2-key.pem'); const cert = fixtures.readKey('agent2-cert.pem'); From 39327b21df673e4cd262315b6dd327b6fd8dae7d Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:00:01 -0800 Subject: [PATCH 06/11] test: end tls gracefully, rather than destroy The timing of destroy between client/server is undefined, but end is guaranteed to occur. --- test/parallel/test-tls-sni-option.js | 2 +- test/parallel/test-tls-sni-server-client.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-sni-option.js b/test/parallel/test-tls-sni-option.js index 375575c78ab635..3a6a231b477f5d 100644 --- a/test/parallel/test-tls-sni-option.js +++ b/test/parallel/test-tls-sni-option.js @@ -115,6 +115,7 @@ let clientError; const server = tls.createServer(serverOptions, function(c) { serverResults.push({ sni: c.servername, authorized: c.authorized }); + c.end(); }); server.on('tlsClientError', function(err) { @@ -135,7 +136,6 @@ function startTest() { clientResults.push( client.authorizationError && (client.authorizationError === 'ERR_TLS_CERT_ALTNAME_INVALID')); - client.destroy(); next(); }); diff --git a/test/parallel/test-tls-sni-server-client.js b/test/parallel/test-tls-sni-server-client.js index 073e95988a3a08..8ab025667ea68a 100644 --- a/test/parallel/test-tls-sni-server-client.js +++ b/test/parallel/test-tls-sni-server-client.js @@ -86,6 +86,7 @@ const clientResults = []; const server = tls.createServer(serverOptions, function(c) { serverResults.push(c.servername); + c.end(); }); server.addContext('a.example.com', SNIContexts['a.example.com']); @@ -107,7 +108,6 @@ function startTest() { clientResults.push( client.authorizationError && (client.authorizationError === 'ERR_TLS_CERT_ALTNAME_INVALID')); - client.destroy(); // Continue start(); From 79412a49a404ee0da50eb08fa38050734787e557 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:02:44 -0800 Subject: [PATCH 07/11] test: send a bad record only after connection done Connection is known to be completely setup only after data has exchanged, so wait unil data echo before sending a bad record. Otherwise, the bad record could interrupt completion of the server's handshake, and whether the error is emitted on the connection or server is a matter of timing. Also, assert that server errors do not occur. 'error' would crash node with and unhandled event, but 'tlsClientError' is ignored by default. --- test/parallel/test-tls-alert-handling.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-alert-handling.js b/test/parallel/test-tls-alert-handling.js index 79d2006d6bd261..63b845122fc1f0 100644 --- a/test/parallel/test-tls-alert-handling.js +++ b/test/parallel/test-tls-alert-handling.js @@ -48,6 +48,9 @@ server.listen(0, common.mustCall(function() { sendClient(); })); +server.on('tlsClientError', common.mustNotCall()); + +server.on('error', common.mustNotCall()); function sendClient() { const client = tls.connect(server.address().port, { @@ -78,8 +81,10 @@ function sendBADTLSRecord() { socket: socket, rejectUnauthorized: false }, common.mustCall(function() { - socket.write(BAD_RECORD); - socket.end(); + client.write('x'); + client.on('data', (data) => { + socket.end(BAD_RECORD); + }); })); client.on('error', common.mustCall((err) => { assert.strictEqual(err.code, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION'); From 431fd20f0c440469448b3fa9b38a961c4342ab4f Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:11:34 -0800 Subject: [PATCH 08/11] test: use mustCall in ephemeralkeyinfo test --- test/parallel/test-tls-client-getephemeralkeyinfo.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-client-getephemeralkeyinfo.js b/test/parallel/test-tls-client-getephemeralkeyinfo.js index a5db18a5652b45..8a9cc65a1cac25 100644 --- a/test/parallel/test-tls-client-getephemeralkeyinfo.js +++ b/test/parallel/test-tls-client-getephemeralkeyinfo.js @@ -40,13 +40,14 @@ function test(size, type, name, cipher) { const client = tls.connect({ port: server.address().port, rejectUnauthorized: false - }, function() { + }, common.mustCall(function() { const ekeyinfo = client.getEphemeralKeyInfo(); assert.strictEqual(ekeyinfo.type, type); assert.strictEqual(ekeyinfo.size, size); assert.strictEqual(ekeyinfo.name, name); server.close(); - }); + })); + client.on('secureConnect', common.mustCall()); })); } From 55e0809ea98d8675305f39997a44a2c0651c3a99 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:18:52 -0800 Subject: [PATCH 09/11] test: use common.mustCall(), and log the events --- test/parallel/test-tls-set-encoding.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-set-encoding.js b/test/parallel/test-tls-set-encoding.js index 77cb5763aeecca..ad0fcf325d69a3 100644 --- a/test/parallel/test-tls-set-encoding.js +++ b/test/parallel/test-tls-set-encoding.js @@ -40,6 +40,7 @@ const messageUtf8 = 'x√ab c'; const messageAscii = 'xb\b\u001aab c'; const server = tls.Server(options, common.mustCall(function(socket) { + console.log('server: on secureConnection', socket.getProtocol()); socket.end(messageUtf8); })); @@ -55,12 +56,18 @@ server.listen(0, function() { client.setEncoding('ascii'); client.on('data', function(d) { + console.log('client: on data', d); assert.ok(typeof d === 'string'); buffer += d; }); + client.on('secureConnect', common.mustCall(() => { + console.log('client: on secureConnect'); + })); + + client.on('close', common.mustCall(function() { + console.log('client: on close'); - client.on('close', function() { // readyState is deprecated but we want to make // sure this isn't triggering an assert in lib/net.js // See https://github.com/nodejs/node-v0.x-archive/issues/1069. @@ -75,5 +82,5 @@ server.listen(0, function() { assert.strictEqual(messageAscii, buffer); server.close(); - }); + })); }); From 3dab493098c562d8e9d1c6917b35c86681827029 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:21:01 -0800 Subject: [PATCH 10/11] test: use mustCall(), not global state checks Instead of pushing state into global arrays and checking the results before exit, use common.mustCall() and make the checks immediately. --- ...s-socket-constructor-alpn-options-parsing.js | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/test/parallel/test-tls-socket-constructor-alpn-options-parsing.js b/test/parallel/test-tls-socket-constructor-alpn-options-parsing.js index 6b0a23f31b6eeb..686f6444ef9d2f 100644 --- a/test/parallel/test-tls-socket-constructor-alpn-options-parsing.js +++ b/test/parallel/test-tls-socket-constructor-alpn-options-parsing.js @@ -20,8 +20,6 @@ const fixtures = require('../common/fixtures'); const key = fixtures.readKey('agent1-key.pem'); const cert = fixtures.readKey('agent1-cert.pem'); -const protocols = []; - const server = net.createServer(common.mustCall((s) => { const tlsSocket = new tls.TLSSocket(s, { isServer: true, @@ -32,10 +30,9 @@ const server = net.createServer(common.mustCall((s) => { }); tlsSocket.on('secure', common.mustCall(() => { - protocols.push({ - alpnProtocol: tlsSocket.alpnProtocol, - }); + assert.strictEqual(tlsSocket.alpnProtocol, 'http/1.1'); tlsSocket.end(); + server.close(); })); })); @@ -46,13 +43,7 @@ server.listen(0, common.mustCall(() => { ALPNProtocols: ['h2', 'http/1.1'] }; - tls.connect(alpnOpts, function() { + tls.connect(alpnOpts, common.mustCall(function() { this.end(); - - server.close(); - - assert.deepStrictEqual(protocols, [ - { alpnProtocol: 'http/1.1' }, - ]); - }); + })); })); From 619b034ecf4e134a9d859e7598fe0d465c067c52 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 1 Feb 2019 12:23:07 -0800 Subject: [PATCH 11/11] test: clarify confusion over "client" in comment Fix perplexing comment. It's not that TLS "clients" don't support 'secureConnect', it's that client sockets created with `new TLSSocket` (as opposed to `tls.connect()`) don't support that event. --- test/parallel/test-tls-socket-default-options.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-socket-default-options.js b/test/parallel/test-tls-socket-default-options.js index f086377b0faa20..87f785dab5e1b3 100644 --- a/test/parallel/test-tls-socket-default-options.js +++ b/test/parallel/test-tls-socket-default-options.js @@ -54,8 +54,9 @@ function test(client, callback) { })); })); - // Client doesn't support the 'secureConnect' event, and doesn't error if - // authentication failed. Caller must explicitly check for failure. + // `new TLSSocket` doesn't support the 'secureConnect' event on client side, + // and doesn't error if authentication failed. Caller must explicitly check + // for failure. (new tls.TLSSocket(null, client)).connect(pair.server.server.address().port) .on('connect', common.mustCall(function() { this.end('hello');