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

test: do not assume tls handshake order #25508

Closed
wants to merge 11 commits into from
9 changes: 7 additions & 2 deletions test/parallel/test-tls-alert-handling.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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);
});
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the only change that I don’t understand – why is .write('x') necessary? Or is it not necessary and does it just expand the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the delay of the BAD_RECORD until .on(data) that is necessary. What's happening on the server side at the point that the client handshake completes depends on the protocol version, so the behaviour with a bad record varies, and the relative ordering of the client secureConnect and server secureConnection are undefined. IIRC, on 1.3 the bad record interrupts the server handshake, causing a tlsClientError (thus the common.mustNotCall() I introduced for that event) instead of a secureConnection event. So, I introduced a data exchange. The server is doing client.pipe(client) to echo data back, so I write from the client, and when the data echoes back, I know that the handshake must have completed on the server.

}));
client.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION');
Expand Down
17 changes: 10 additions & 7 deletions test/parallel/test-tls-alpn-server-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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);
});
}
});
}
Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-tls-client-getephemeralkeyinfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}));
}

Expand Down
38 changes: 23 additions & 15 deletions test/parallel/test-tls-client-reject.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
8 changes: 5 additions & 3 deletions test/parallel/test-tls-connect-address-family.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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');
Expand All @@ -44,4 +46,4 @@ dns.lookup('localhost', { family: 6, all: true }, (err, addresses) => {
runTest();
else
common.skip('localhost does not resolve to ::1');
});
}));
7 changes: 4 additions & 3 deletions test/parallel/test-tls-friendly-error-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
}));
11 changes: 9 additions & 2 deletions test/parallel/test-tls-set-encoding.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}));

Expand All @@ -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.
Expand All @@ -75,5 +82,5 @@ server.listen(0, function() {
assert.strictEqual(messageAscii, buffer);

server.close();
});
}));
});
2 changes: 1 addition & 1 deletion test/parallel/test-tls-sni-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -135,7 +136,6 @@ function startTest() {
clientResults.push(
client.authorizationError &&
(client.authorizationError === 'ERR_TLS_CERT_ALTNAME_INVALID'));
client.destroy();

next();
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-sni-server-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -107,7 +108,6 @@ function startTest() {
clientResults.push(
client.authorizationError &&
(client.authorizationError === 'ERR_TLS_CERT_ALTNAME_INVALID'));
client.destroy();

// Continue
start();
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-tls-socket-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
17 changes: 4 additions & 13 deletions test/parallel/test-tls-socket-constructor-alpn-options-parsing.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
}));
}));

Expand All @@ -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' },
]);
});
}));
}));
5 changes: 3 additions & 2 deletions test/parallel/test-tls-socket-default-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down