test: do not assume tls handshake order #25508

sam-github · 2019-01-15T03:11:30Z

These are a number of cleanups to the TLS test suites that I noticed while trying to run the tests with TLS1.3, but they aren't specific to TLS1.3. I'd like them off my branch so its clear they are equally applicable to TLS1.2, that is, they are not API breaking changes being introduced with TLS1.3.

Mostly they involve removing assumptions about the relative ordering of client and server TLS handshake completion, graceful use of .end() to tear down tests (rather than destroy), and some conversions of tests to use common.mustCall()

PTAL @nodejs/crypto

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-01-15T03:11:31Z

@sam-github sadly an error occured when I tried to trigger a build :(

sam-github · 2019-01-15T03:15:57Z

ci: https://ci.nodejs.org/job/node-test-pull-request/20130/

sam-github · 2019-01-15T04:31:19Z

Sorry, best assume this is a WIP, I found another example of the same problem. I'll add any more I find rather than fix the same thing in multiple commits.

sam-github · 2019-02-01T21:20:57Z

ci: https://ci.nodejs.org/job/node-test-pull-request/20525/

@nodejs/crypto, I reworked the description, it was out of date.

sam-github · 2019-02-05T15:44:04Z

@nodejs/collaborators please take a look

sam-github · 2019-02-05T19:06:12Z

@indutny perhaps you could take a look at this one?

test/parallel/test-tls-socket-close.js

addaleax · 2019-02-05T19:22:23Z

test/parallel/test-tls-alert-handling.js

+    client.write('x');
+    client.on('data', (data) => {
+      socket.end(BAD_RECORD);
+    });


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?

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.

Test assumed server gets a handshake before the client destroyed it, and didn't assert that dns.lookup() callback occurred.

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.

Do not assume that server handshake event happens before client, it is not guaranteed.

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.

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: - nodejs#13184 (comment)

The timing of destroy between client/server is undefined, but end is guaranteed to occur.

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.

Instead of pushing state into global arrays and checking the results before exit, use common.mustCall() and make the checks immediately.

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 assumed server gets a handshake before the client destroyed it, and didn't assert that dns.lookup() callback occurred. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Do not assume that server handshake event happens before client, it is not guaranteed. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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: - #13184 (comment) PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

The timing of destroy between client/server is undefined, but end is guaranteed to occur. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Instead of pushing state into global arrays and checking the results before exit, use common.mustCall() and make the checks immediately. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

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. PR-URL: #25508 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>