tls: do not crash on STARTTLS when OCSP requested#10706

indutny · 2017-01-09T13:44:01Z

Checklist

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

Affected core subsystem(s)

tls

TLSSocket should not have a hard dependency on tls.Server, since it
may be running without it in cases like STARTTLS.

Fix: #10704

`TLSSocket` should not have a hard dependency on `tls.Server`, since it may be running without it in cases like `STARTTLS`. Fix: nodejs#10704

indutny · 2017-01-09T13:44:19Z

cc @nodejs/crypto

indutny · 2017-01-09T14:30:53Z

cc @nodejs/collaborators

jasnell · 2017-01-09T16:42:46Z

this looks like semver-patch but just to be safe, is there any possibility that this could break anything? (I highly doubt it but given the removal of the requirement there's always a slight possibility)

cjihrig · 2017-01-09T16:43:38Z

test/parallel/test-tls-starttls-server.js

+}),
+
+SNICallback: common.mustCall((hostname,callback)=>{
+assert.equal(hostname,'test.test');


Can you change this to assert.strictEqual().

Fixed, and removed that console.log() below.

indutny · 2017-01-09T16:44:48Z

@jasnell no possibility, everything that hit that code path was broken until this patch

sam-github · 2017-01-09T20:18:19Z

test/parallel/test-tls-starttls-server.js

+};
+
+constclient=tls.connect(opts,function(){
+client.end();


can just use this, and delete the const client =

sam-github · 2017-01-09T20:18:37Z

test/parallel/test-tls-starttls-server.js

+return;
+}
+
+constassert=require('assert');


sort requires

sam-github · 2017-01-09T20:22:42Z

test/parallel/test-tls-starttls-server.js

+SNICallback: common.mustCall((hostname,callback)=>{
+assert.deepEqual(hostname,'test.test');
+
+callback(null,null);


From https://github.com/nodejs/node/blob/master/doc/api/tls.md#tlscreateserveroptions-secureconnectionlistener
SNICallback should invoke cb(null, ctx), where ctx is a SecureContext instance.
Is (null, null) valid? Should it be valid?

Yes, it is valid and it should be valid.

sam-github · 2017-01-09T20:25:00Z

test/parallel/test-tls-starttls-server.js

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


https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
A test should start with a comment containing a brief description of what it is designed to test.

sam-github

One suggested tweek to in-source comment, otherwise LGTM

sam-github · 2017-01-10T18:59:12Z

lib/_tls_wrap.js

 if(!ctx)
 ctx=self.server._sharedCreds;
+
+// Running on non-TLS server


This comment confused me when I read it, and I only understood when reading the unit test and PR history. Can I suggest:
TLS socket is using a net.Server, instead of a tls.TLSServer, so some TLS properties will not be present.

Agreed this comment is rather cryptic.

webertrlz · 2017-02-01T17:17:45Z

Do you have any information on when this will be merged and released?

sam-github · 2017-02-01T17:49:55Z

@nodejs/crypto a review from one of you is required, I believe.

bnoordhuis

LGTM if you clarify the comment.

bnoordhuis · 2017-02-02T13:40:47Z

lib/_tls_wrap.js

 if(!ctx)
 ctx=self.server._sharedCreds;
+
+// Running on non-TLS server


Agreed this comment is rather cryptic.

webertrlz · 2017-02-03T14:57:41Z

Is it possible to catch this crash as an exception? I didn't succeed so far. This would be a good workaround until this is packaged.

indutny · 2017-02-03T17:22:04Z

Pushed the fix to the comment, PTAL. Landing if the wording is fine.

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2017-02-03T17:43:41Z

lib/_tls_wrap.js


-// Running on non-TLS server
+// TLS socket is using a `net.Server` instead of a tls.TLSServer.
+// Some TLS properties will not be present.


Consider giving one or two examples of properties that won't be there.

indutny · 2017-02-03T18:00:45Z

@bnoordhuis ACK

addaleax · 2017-02-22T16:14:41Z

CI: https://ci.nodejs.org/job/node-test-commit/8076/

webertrlz · 2017-02-22T16:22:53Z

hello,

sorry for asking this but I'd like to know if there is any planning on merging+packaging this PR.
otherwise I'll have to build a version with this fix for my production environment.

Thanks.

jasnell · 2017-02-22T16:32:40Z

@webertrlz Yes, this should land assuming that CI comes back good.

sam-github · 2017-02-22T17:36:56Z

Landed in a1802e6

`TLSSocket` should not have a hard dependency on `tls.Server`, since it may be running without it in cases like `STARTTLS`. Fix: #10704 PR-URL: #10706 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

sam-github · 2017-02-22T17:37:34Z

Won't show up packaged until the next 7.x release, which will be a couple weeks at least.

webertrlz · 2017-02-22T17:42:24Z

The post above means that it won't go packaged for 6.XX LTS?

addaleax · 2017-02-22T17:50:04Z

@webertrlz The rules are that changes first have to live an a Current release for two weeks before they are applied to the LTS branches. This change will almost certainly be available in the next one or two v6.x releases, and probably v4.x as well.

webertrlz · 2017-02-22T18:59:39Z

@addaleax understood! Thanks!

`TLSSocket` should not have a hard dependency on `tls.Server`, since it may be running without it in cases like `STARTTLS`. Fix: #10704 PR-URL: #10706 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

tls: do not crash on STARTTLS when OCSP requested
a1b114e
`TLSSocket` should not have a hard dependency on `tls.Server`, since it may be running without it in cases like `STARTTLS`. Fix: nodejs#10704

nodejs-github-bot added tls Issues and PRs related to the tls subsystem. lts-watch-v6.x labels Jan 9, 2017

indutny requested review from bnoordhuis and shigeki January 9, 2017 13:44

jasnell approved these changes Jan 9, 2017
View reviewed changes

cjihrig reviewed Jan 9, 2017
View reviewed changes

fix
caa7ba1

nodejs-github-bot added lts-watch-v6.x labels Jan 9, 2017

sam-github suggested changes Jan 9, 2017
View reviewed changes

fix
3ef170f

nodejs-github-bot added lts-watch-v6.x labels Jan 10, 2017

fix
7473f55

nodejs-github-bot added lts-watch-v6.x labels Jan 10, 2017

fix
584b455

nodejs-github-bot added lts-watch-v6.x labels Jan 10, 2017

sam-github approved these changes Jan 10, 2017
View reviewed changes

webertrlz approved these changes Jan 11, 2017
View reviewed changes

neeksor mentioned this pull request Jan 30, 2017
MailDev crashes when receiving message from exim SMTP client maildev/maildev#153
Closed

bnoordhuis approved these changes Feb 2, 2017
View reviewed changes

lib/_tls_wrap.js Outdated
if(!ctx)
ctx=self.server._sharedCreds;

// Running on non-TLS server
Copy link
Member
bnoordhuisFeb 2, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this comment is rather cryptic.

fix
20b187b

nodejs-github-bot removed lts-watch-v6.x labels Feb 3, 2017

bnoordhuis approved these changes Feb 3, 2017
View reviewed changes

fix
36fd2ce

sam-github closed this Feb 22, 2017

italoacasas mentioned this pull request Feb 25, 2017
7.7.0 Proposal #11553
Merged

MylesBorins mentioned this pull request Mar 9, 2017
v6.10.1 proposal #11759
Merged

MylesBorins mentioned this pull request Mar 9, 2017
v4.8.1 proposal #11760
Merged

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

Uh oh!

tls: do not crash on STARTTLS when OCSP requested#10706

tls: do not crash on STARTTLS when OCSP requested #10706

Uh oh!

Conversation

indutny commented Jan 9, 2017

Checklist

Affected core subsystem(s)

Uh oh!

indutny commented Jan 9, 2017

Uh oh!

indutny commented Jan 9, 2017

Uh oh!

jasnell commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webertrlz commented Feb 1, 2017

Uh oh!

sam-github commented Feb 1, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webertrlz commented Feb 3, 2017

Uh oh!

indutny commented Feb 3, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Feb 3, 2017

Uh oh!

addaleax commented Feb 22, 2017

Uh oh!

webertrlz commented Feb 22, 2017

Uh oh!

jasnell commented Feb 22, 2017

Uh oh!

sam-github commented Feb 22, 2017

Uh oh!

sam-github commented Feb 22, 2017

Uh oh!

webertrlz commented Feb 22, 2017

Uh oh!

addaleax commented Feb 22, 2017

Uh oh!