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

lekoder · 2016-09-27T10:01:40Z

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 · 2016-09-27T14:53:57Z

test/parallel/test-tls-failed-handshake-emits-error.js

@@ -0,0 +1,31 @@
+'use strict';
+var common = require('../common');


nit: const

jasnell · 2016-09-27T14:54:04Z

test/parallel/test-tls-failed-handshake-emits-error.js

+  return;
+}
+var tls = require('tls');
+var net = require('net');


const here also

jasnell · 2016-09-27T14:54:53Z

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

mscdex · 2016-09-27T15:40:06Z

/cc @indutny ?

cjihrig · 2016-09-27T15:42:58Z

test/parallel/test-tls-failed-handshake-emits-error.js

+      server: server
+    });
+
+    s.on('error', common.mustCall(function() {}));


Is there any validation you can add to the error?

@cjihrig quite right, added basic validation.

mcollina · 2016-09-27T17:39:11Z

test/parallel/test-tls-failed-handshake-emits-error.js

+      server.close();
+      s.destroy();
+    });
+  }), 200);


this must be common.platformTimeout based.

mcollina · 2016-09-27T17:40:22Z

test/parallel/test-tls-failed-handshake-emits-error.js

+var bonkers = Buffer.alloc(1024, 42);
+
+var server = net.createServer(common.mustCall(function(c) {
+  setTimeout(common.mustCall(function() {


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

mcollina · 2016-09-27T17:40:49Z

test/parallel/test-tls-failed-handshake-emits-error.js

+
+var bonkers = Buffer.alloc(1024, 42);
+
+var server = net.createServer(common.mustCall(function(c) {


mcollina · 2016-09-27T17:41:02Z

test/parallel/test-tls-failed-handshake-emits-error.js

+var tls = require('tls');
+var net = require('net');
+
+var bonkers = Buffer.alloc(1024, 42);


mcollina · 2016-09-27T17:43:41Z

lib/_tls_wrap.js

+        self.destroy(err);
+      } else {
+        self.destroy(self._tlsError(err));
+      }


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

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

@mcollina @indutny self._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.

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

indutny · 2016-09-28T01:33:50Z

lib/_tls_wrap.js

+        self.destroy(err);
+      } else {
+        self.destroy(self._tlsError(err));
+      }


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

lekoder · 2016-09-28T06:43:03Z

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 · 2016-09-29T07:54:45Z

@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 · 2016-09-29T08:22:13Z

@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 · 2016-09-29T08:28:26Z

@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 · 2016-09-29T08:30:56Z

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

indutny · 2016-09-29T18:57:15Z

test/parallel/test-tls-failed-handshake-emits-error.js

+      server: server
+    });
+
+    s.on('error', common.mustCall(function(e) {


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

indutny · 2016-09-29T19:57:30Z

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 · 2016-09-30T06:12:54Z

@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).

Fixes nodejs#8803

Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and controll was not released, as it was never happening. Addedd test for tls.Server to ensure it still emits 'tlsClientError' as expected. Makes tests conform to defined linting rules.

lekoder · 2016-10-07T06:14:03Z

@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 · 2016-10-07T18:05:24Z

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

mcollina · 2016-10-10T08:05:22Z

Landed as c7bc9bc.

Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and control was not released, as it was never happening. Addedd test for tls.Server to ensure it still emits 'tlsClientError' as expected. Fixes: #8803 PR-URL: #8805 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins · 2016-11-22T06:35:24Z

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 · 2016-11-22T16:09:05Z

I'll take a look.

Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and control was not released, as it was never happening. Added test for tls.Server to ensure it still emits 'tlsClientError' as expected. Note that 'tlsClientError' does not exist in the v4.x branch so this back-port emits 'clientError' instead. See also pull request nodejs#4557. Fixes: nodejs#8803 PR-URL: nodejs#8805 Refs: nodejs#4557 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

bnoordhuis · 2016-11-22T16:18:19Z

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

Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and control was not released, as it was never happening. Added test for tls.Server to ensure it still emits 'tlsClientError' as expected. Note that 'tlsClientError' does not exist in the v4.x branch so this back-port emits 'clientError' instead. See also pull request #4557. Fixes: #8803 PR-URL: #8805 Refs: #4557 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Sep 27, 2016

jasnell reviewed Sep 27, 2016

View reviewed changes

cjihrig reviewed Sep 27, 2016

View reviewed changes

mcollina requested changes Sep 27, 2016

View reviewed changes

indutny suggested changes Sep 28, 2016

View reviewed changes

lekoder force-pushed the tls-socket-emit-clienterror branch from 4b4f028 to 68458e0 Compare September 29, 2016 08:18

mcollina approved these changes Sep 29, 2016

View reviewed changes

lekoder force-pushed the tls-socket-emit-clienterror branch from 68458e0 to 28af293 Compare September 29, 2016 08:40

indutny reviewed Sep 29, 2016

View reviewed changes

lekoder added 2 commits October 7, 2016 07:48

TLS/SSL: TLSSocket emits 'error' on handshake failure

f240da5

Fixes nodejs#8803

lekoder force-pushed the tls-socket-emit-clienterror branch from 1c2a46c to d3027e8 Compare October 7, 2016 06:03

indutny approved these changes Oct 7, 2016

View reviewed changes

mcollina closed this Oct 10, 2016

MylesBorins added the lts-watch-v4.x label Nov 22, 2016

bnoordhuis mentioned this pull request Nov 22, 2016

tls: TLSSocket emits 'error' on handshake failure #9742

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016

bnoordhuis mentioned this pull request Jan 27, 2017

https requests complete despite handshake timeout #11030

Closed

MylesBorins mentioned this pull request Apr 19, 2017

v4.8.3 proposal #12499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lekoder commented Sep 27, 2016 •

edited

Loading

jasnell Sep 27, 2016

jasnell Sep 27, 2016

jasnell commented Sep 27, 2016

mscdex commented Sep 27, 2016

cjihrig Sep 27, 2016

lekoder Sep 28, 2016

mcollina Sep 27, 2016

mcollina Sep 27, 2016

mcollina Sep 27, 2016

mcollina Sep 27, 2016

mcollina Sep 27, 2016

indutny Sep 28, 2016

lekoder Sep 28, 2016 •

edited

Loading

indutny Sep 29, 2016

indutny Sep 28, 2016

lekoder commented Sep 28, 2016

mcollina commented Sep 29, 2016

lekoder commented Sep 29, 2016

mcollina commented Sep 29, 2016

mcollina commented Sep 29, 2016

indutny Sep 29, 2016

indutny commented Sep 29, 2016

lekoder commented Sep 30, 2016

lekoder commented Oct 7, 2016

indutny left a comment

jasnell commented Oct 7, 2016

mcollina commented Oct 10, 2016

MylesBorins commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

		@@ -0,0 +1,31 @@
		'use strict';
		var common = require('../common');


		var bonkers = Buffer.alloc(1024, 42);

		var server = net.createServer(common.mustCall(function(c) {

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

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

Conversation

lekoder commented Sep 27, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 27, 2016

mscdex commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lekoder Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lekoder commented Sep 28, 2016

mcollina commented Sep 29, 2016

lekoder commented Sep 29, 2016

mcollina commented Sep 29, 2016

mcollina commented Sep 29, 2016

Choose a reason for hiding this comment

indutny commented Sep 29, 2016

lekoder commented Sep 30, 2016

lekoder commented Oct 7, 2016

indutny left a comment

Choose a reason for hiding this comment

jasnell commented Oct 7, 2016

mcollina commented Oct 10, 2016

MylesBorins commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

lekoder commented Sep 27, 2016 •

edited

Loading

lekoder Sep 28, 2016 •

edited

Loading