TLS/SSL: tls.TLSSocket emits error event on handshake failure

[lekoder]

Checklist

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

Affected core subsystem(s)

tls/ssl

Description of change

tls.TLSSocket emits now error event on handshake failure.

Fixes#8803

Notes

• upstream/master does not pass make -j8 test on time of this contribution - with unrelated errors. Tests related to tls/ssl and new test to highlight referenced issue pass.

[jasnell]

nit: const

[jasnell]

const here also

[jasnell]

Couple of nits. /cc @nodejs/streams for review

[mscdex]

/cc @indutny ?

[cjihrig]

Is there any validation you can add to the error?

[lekoder]

@cjihrig quite right, added basic validation.

[mcollina]

this must be common.platformTimeout based.

[mcollina]

we don't need common.mustCall here, but it does not hurt either. Any other opinions?

[mcollina]

const

[mcollina]

const

[mcollina]

it is not clear why this change fix the described issue. Can you please add a comment describing why?

[indutny]

Furthermore, _tlsError returns null if self._controlReleased === false. I don't see how this particular change makes it behave differently.

[lekoder]

@mcollina@indutnyself._tlsError returning null is exactly what i am addressing here. When handshake fails and ssl.onerror is called control is not released yet, so self.destroy is called with null, and error event is not fired in destroy.

Here is debug output from upstream:

NET 26944: listen2 null 1337 4 false undefined NET 26944: _listen2: create a handle NET 26944: bind to :: NET 26944: onconnection NET 26944: _read NET 26944: Socket._read readStart NET 26944: _read TLS 26944: onhandshakestart NET 26944: destroy null NET 26944: destroy NET 26944: close NET 26944: close handle NET 26944: destroy undefined NET 26944: destroy NET 26944: close NET 26944: close handle NET 26944: has server NET 26944: SERVER _emitCloseIfDrained NET 26944: SERVER handle? true connections? 0 NET 26944: emit close NET 26944: emit close 

...and here after the patch:

NET 31249: listen2 null 1337 4 0 undefined NET 31249: _listen2: create a handle NET 31249: bind to :: NET 31249: onconnection NET 31249: _read NET 31249: Socket._read readStart NET 31249: _read TLS 31249: onhandshakestart NET 31249: destroy Error: 140507750061888:error:1408A0C1:SSL routines:ssl3_get_client_hello:no shared cipher:../deps/openssl/openssl/ssl/s3_srvr.c:1418: NET 31249: destroy NET 31249: close NET 31249: close handle NET 31249: destroy undefined NET 31249: destroy NET 31249: close NET 31249: close handle NET 31249: has server NET 31249: SERVER _emitCloseIfDrained NET 31249: SERVER handle? true connections? 0 NET 31249: emit close NET 31249: emit close 

Now, just replacing self.destroy(self._tlsError(err)) with self.destroy(err) there also fixes #8803 and same tests pass, but since self._tlsError emits additional '_tlsError' event which could be used in something not covered with tests, i opted for safer approach.

[indutny]

_tlsError is used for emitting tlsClientError, which is in turn used in https module to kill the request.

[indutny]

Furthermore, _tlsError returns null if self._controlReleased === false. I don't see how this particular change makes it behave differently.

[lekoder]

Confused. I rebased my branch to upstream/master as per step 4 of contributing guide, but it doesn't feel right now. Should i rebase it back?

[mcollina]

@lekoder no it doesn't feel right. I think you might want to start clean and force push your individual changes here again.

Usually you rebase when you get some LGTMs, before it's merged.

[lekoder]

@mcollina I did just that. I would suggest adding this info to contributing guide, ie:

note: Do not rebase your branch if you already made pull request.

[mcollina]

@lekoder rebasing is fine, you might have made a mistake in the process. Your commits needs to sit on top of upstream master.

You should probably squash your commits into one.

[mcollina]

I'm LGTM for the streams point of view, but @indutny has the final say.

[indutny]

Is this a case you fixing? Creating a socket manually for a server?

[indutny]

I had more time to think about it, and it looks like the patch is correct. Whilst reviewing things I have found that we have quite a funny possible infinite loop in error event handler... Going to fix it in a follow up PR.

Please reduce the branching code, and this PR will be good to go. Thank you!

[lekoder]

@indutny Removed branch as suggested. Also added tests to make sure that tls.Server was not affected by that change (it wasn't, but i figured additional test won't hurt - i can drop it if you feel it's redundant).

[lekoder]

@mcollina I did some testing of the whole rebasing problem i had with. Problem was caused by doing:

$ git fetch upstream $ git rebase upstream/master $ git pull && git push 

...and that ended up with pull request that contained not only my changes, but also all commits from upstream/master to this point. What did work:

$ git fetch upstream $ git rebase upstream/master $ git push -f 

...or even better:

$ git fetch upstream $ git rebase master upstream/master $ git push origin $ git checkout my-branch $ git rebase master $ git push -f 

...which makes my origin/master in sync with upstream/master, and my-branch based on that.

[indutny]

LGTM. Sorry for delay!

[jasnell]

New CI: https://ci.nodejs.org/job/node-test-pull-request/4424/

[mcollina]

Landed as c7bc9bc.

[MylesBorins]

when backporting to v4.x we get some failing tests

=== release test-tls-server-failed-handshake-emits-clienterror === Path: parallel/test-tls-server-failed-handshake-emits-clienterror assert.js:85 throw new assert.AssertionError({^ AssertionError: tlsClientError should be emited at null._onTimeout (/Users/thealphanerd/code/node/v4.x/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js:34:10) at Timer.listOnTimeout (timers.js:92:15) Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js 

Would someone be willing to manually backport

[bnoordhuis]

I'll take a look.

[bnoordhuis]

#9742 - 'tlsClientError' wasn't introduced until v6.x, it emits 'clientError' instead.