tls: emit a warning when servername is an IP address #18127

rcombs · 2018-01-13T00:02:32Z

Refs: #18071

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tls

bnoordhuis

It would be nice to have a regression test for this.

bnoordhuis · 2018-01-13T11:44:21Z

lib/_tls_wrap.js

+      process.emitWarning(
+        'Setting the TLS ServerName to an IP address is not supported by ' +
+        'RFC6066. This will be ignored in a future version.',
+        'UnsupportedWarning'


Can you emit 'DeprecationWarning' plus a DEPxxxx code here? The code needs to be added to doc/api/deprecations.md.

Just a note, making this a DeprecationWarning would make this semver-major

I'm a little out of my depth here with regards to both node's deprecation and its versioning semantics.
Depending on what level the "drop hostname on IP" thing ultimately ends up at (could be here; could be tls.js?), I'm not sure this makes sense to consider a "deprecation" per se? Most if not all existing external users should see the warning for a version, then see it silently go away with no change necessary in a future version.

I think the sentence This will be ignored in a future version. implies that it is a deprecation. Making it a DeprecationWarning gives us a proper documentation for the behavior change.

Also this reminds me, can you add a entry to the changes YAML field in the documentation of tls.connect? Note that in PRs the version field should be REPLACEME.

starkwang · 2018-01-13T15:08:15Z

lib/_tls_wrap.js

@@ -1138,8 +1140,17 @@ exports.connect = function(...args /* [port,] [host,] [options,] [cb] */) {
  if (options.session)
    socket.setSession(options.session);

-  if (options.servername)
+  if (options.servername) {
+    if (ipServernameWarned === false && net.isIP(options.servername)) {


Using !ipServernameWarned instead of ipServernameWarned === false might be better.

Just a quick note here that may be worth noting in the docs... the net.isIP() function returns 0 if an ipv6 address is given with the surrounding [ ] ... e.g. net.isIP('[fe80::a00:27ff:fec0:eb1a]') === 0. This is, of course, correct because the square brackets are not actually a part of the address, but it could catch some users.

I went with === false to match an existing one-time warning (dunno if it optimizes slightly better or what); I can switch to ! if you prefer.

url.parse('https://[fe80::a00:27ff:fec0:eb1a]:3333/'), for instance, fills hostname with fe80::a00:27ff:fec0:eb1a (bracketless), and if you manually build a URL with brackets around an IPv6 address in hostname, https.get() on it fails on DNS lookup, so I think we should be fine there?

BridgeAR · 2018-02-01T17:45:17Z

@rcombs would you be so kind and address the comments? I guess the second one is fine as is (Pinging @jasnell anyway).

BridgeAR · 2018-02-12T13:33:38Z

Ping @rcombs

rcombs · 2018-02-13T05:48:56Z

Rebased and made the requested style change. I don't know how to add regression tests or how to handle deprecation rules, so if someone who does wants to take this that'd be 👍.

BridgeAR · 2018-02-13T05:58:16Z

@rcombs seems like something went wrong while rebasing? There are still conflicts?

You can easily check how a deprecation works when looking at e.g.

node/lib/assert.js

Line 92 in cccddc5

'DEP0094'

and

node/test/parallel/test-assert-fail-deprecation.js

Line 10 in 70dcacd

);

.

Refs: nodejs#18071

joyeecheung · 2018-02-14T06:12:27Z

You can use common.expectWarning to test the warning. The docs on how to write a test is here

BridgeAR · 2018-02-16T22:59:32Z

@rcombs I wrote a small gist to summarize how to create a test.

BridgeAR · 2018-03-02T04:05:21Z

Ping @rcombs

bnoordhuis · 2018-03-13T16:30:27Z

Ping @rcombs - do you still want to follow up?

rcombs · 2018-03-15T05:00:11Z

Sorry, I haven't had a chance to get around to this. I'm not sure I'm really the best person to go through the internal process for this (and then follow it up in the next release).

bnoordhuis · 2018-03-15T07:48:19Z

If you mean back-porting, that's usually taken care of by collaborators.

rcombs · 2018-03-15T07:50:12Z

I meant removing the warning and changing the behavior.

bnoordhuis · 2018-03-15T07:52:27Z

Oh, I can open an issue for that once this PR lands.

BridgeAR · 2018-04-10T00:35:34Z

@rcombs I would love to land this but it still needs a test. I am not sure if your former comment was about adding a test or not. Do you think you get the time to address that? :-)

BridgeAR · 2018-05-15T11:57:16Z

I am closing this due to no further progress.

@rcombs please feel free to leave a comment if you would like to work on this again to add a test or open a new PR.

BridgeAR · 2018-05-15T11:57:52Z

@rcombs thanks a lot for your contribution anyway! It is much appreciated and I still think it would be good to get a solution for #18071.

oyyd · 2018-10-08T08:18:26Z

if someone who does wants to take this that'd be 👍.

I would like to take this and help to close #18071 basing on your work. @rcombs Thanks for your investigation!

Setting the TLS ServerName to an IP address is not permitted by RFC6066. This will be ignored in a future version. Closes: nodejs#18071 Refs: nodejs#18127

Setting the TLS ServerName to an IP address is not permitted by RFC6066. This will be ignored in a future version. Refs: #18127 PR-URL: #23329 Fixes: #18071 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jan 13, 2018

bnoordhuis reviewed Jan 13, 2018

View reviewed changes

starkwang reviewed Jan 13, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

rcombs force-pushed the tls-sni-ip branch from 66a8e00 to 906cbf2 Compare February 13, 2018 05:46

tls: emit a warning when servername is an IP address

53957d5

Refs: nodejs#18071

rcombs force-pushed the tls-sni-ip branch from 906cbf2 to 53957d5 Compare February 13, 2018 06:00

BridgeAR closed this May 15, 2018

oyyd mentioned this pull request Oct 8, 2018

tls: emit a warning when servername is an IP address #23329

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: emit a warning when servername is an IP address #18127

tls: emit a warning when servername is an IP address #18127

rcombs commented Jan 13, 2018

bnoordhuis left a comment

bnoordhuis Jan 13, 2018

jasnell Jan 13, 2018

rcombs Jan 13, 2018

joyeecheung Jan 13, 2018 •

edited

Loading

starkwang Jan 13, 2018

jasnell Jan 13, 2018

rcombs Jan 13, 2018

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 12, 2018

rcombs commented Feb 13, 2018 •

edited

Loading

BridgeAR commented Feb 13, 2018

joyeecheung commented Feb 14, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Mar 2, 2018

bnoordhuis commented Mar 13, 2018

rcombs commented Mar 15, 2018

bnoordhuis commented Mar 15, 2018

rcombs commented Mar 15, 2018

bnoordhuis commented Mar 15, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented May 15, 2018

BridgeAR commented May 15, 2018

oyyd commented Oct 8, 2018

tls: emit a warning when servername is an IP address #18127

tls: emit a warning when servername is an IP address #18127

Conversation

rcombs commented Jan 13, 2018

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jan 13, 2018

Choose a reason for hiding this comment

jasnell Jan 13, 2018

Choose a reason for hiding this comment

rcombs Jan 13, 2018

Choose a reason for hiding this comment

joyeecheung Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

starkwang Jan 13, 2018

Choose a reason for hiding this comment

jasnell Jan 13, 2018

Choose a reason for hiding this comment

rcombs Jan 13, 2018

Choose a reason for hiding this comment

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 12, 2018

rcombs commented Feb 13, 2018 • edited Loading

BridgeAR commented Feb 13, 2018

joyeecheung commented Feb 14, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Mar 2, 2018

bnoordhuis commented Mar 13, 2018

rcombs commented Mar 15, 2018

bnoordhuis commented Mar 15, 2018

rcombs commented Mar 15, 2018

bnoordhuis commented Mar 15, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented May 15, 2018

BridgeAR commented May 15, 2018

oyyd commented Oct 8, 2018

joyeecheung Jan 13, 2018 •

edited

Loading

rcombs commented Feb 13, 2018 •

edited

Loading