tls: do not crash on STARTTLS when OCSP requested by indutny · Pull Request #10706 · nodejs/node · GitHub

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.

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

cc @nodejs/crypto

cc @nodejs/collaborators

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)

Can you change this to assert.strictEqual().

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

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

can just use this, and delete the const client =

Ack.

sort requires

Ack

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.

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.

Ack

One suggested tweek to in-source comment, otherwise LGTM

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.

ping

Agreed this comment is rather cryptic.

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

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

LGTM if you clarify the comment.

Agreed this comment is rather cryptic.

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.

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

LGTM with a suggestion.

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

@bnoordhuis ACK

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

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.

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

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>

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

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

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

@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

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

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

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

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

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

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

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

tls: do not crash on STARTTLS when OCSP requested by indutny · Pull Request #10706 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

tls: do not crash on STARTTLS when OCSP requested#10706indutny wants to merge 7 commits intonodejs:masternodejs/node:masterfrom indutny:fix/gh-10704indutny/io.js:fix/gh-10704Copy head branch name to clipboard

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

tls: do not crash on STARTTLS when OCSP requested#10706
indutny wants to merge 7 commits into
nodejs:masterfrom
indutny:fix/gh-10704