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

+  };
+
+  const client = 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;
+}
+
+const assert = 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';
+const common = 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

@@ -114,6 +114,12 @@ function requestOCSP(self, hello, ctx, cb) {

  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

@@ -114,6 +114,12 @@ function requestOCSP(self, hello, ctx, cb) {

  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

@@ -115,7 +115,8 @@ function requestOCSP(self, hello, ctx, cb) {
  if (!ctx)
    ctx = self.server._sharedCreds;

-  // 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 <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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 <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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

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';
		const common = require('../common');

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

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

Conversation

indutny commented Jan 9, 2017

Checklist

Affected core subsystem(s)

indutny commented Jan 9, 2017

indutny commented Jan 9, 2017

jasnell commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Jan 9, 2017

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

sam-github left a 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

webertrlz commented Feb 1, 2017

sam-github commented Feb 1, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webertrlz commented Feb 3, 2017

indutny commented Feb 3, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Feb 3, 2017

addaleax commented Feb 22, 2017

webertrlz commented Feb 22, 2017

jasnell commented Feb 22, 2017

sam-github commented Feb 22, 2017

sam-github commented Feb 22, 2017

webertrlz commented Feb 22, 2017

addaleax commented Feb 22, 2017

webertrlz commented Feb 22, 2017