Issue 14736 tls ipv6 address #14772

mattiash · 2017-08-11T12:55:48Z

Resolves issue #14736.

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

mscdex · 2017-08-11T17:32:43Z

lib/_http_agent.js

+      // abc:123 => abc
+      // [::1] => ::1
+      // [::1]:123 => ::1
+      options.servername = hostHeader.replace(/:[^\]]+$/, '')


Why not combine both regexps for efficiency?:

options.servername = hostHeader.replace(/^(?:\[([^\]]*)\].*)|(?:([^:]*).*)$/, '$1$2');

I tried a few different variations on the above regexp, and that one seemed to execute the fastest, at least on node v8.3.0.

I ran a benchmark on your regexp (regexp1), my regexp (regexp2) and a variant that doesn't use regexps (t3). See https://gist.github.com/mattiash/f19f7d862dce54db955267e03319b2d2

The result from my benchmark was that the two regexp-solution was actually faster than your one-regexp-solution, but the fastest was my new variant without any regexp.

$ node --version v8.3.0 $ node index.js regexp1 x 360,816 ops/sec ±1.15% (88 runs sampled) regexp2 x 514,166 ops/sec ±1.21% (87 runs sampled) t3 x 1,195,766 ops/sec ±1.38% (87 runs sampled) Fastest is t3

$ node --version v6.3.1 $ node index.js regexp1 x 592,158 ops/sec ±0.95% (84 runs sampled) regexp2 x 758,064 ops/sec ±1.21% (87 runs sampled) t3 x 829,918 ops/sec ±1.18% (85 runs sampled) Fastest is t3

I'll update the PR to use the t3 variant

jasnell · 2017-08-13T22:32:13Z

lib/tls.js

@@ -163,6 +163,33 @@ function check(hostParts, pattern, wildcards) {
  return true;
 }

+exports._canonicalIp = function(address) {


I'd generally prefer not introducing new _-prefixed methods. Non-public methods should be moved into lib/internal/ and used from there.

mattiash · 2017-08-21T06:28:55Z

Is there anything else I need to do before this can be merged? We are using TLS with certificates that sign IPv4 addresses today, and we are very eager to add support for IPv6 which requires support for TLS with certificates that sign IPv6 addresses.

jasnell · 2017-08-21T21:56:45Z

It needs a bit of additional review. I've added a couple of folks to the "Reviewers" list.

jasnell · 2017-08-23T19:00:00Z

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

BridgeAR · 2017-08-29T06:59:13Z

lib/internal/tls.js

+    const b = ['0', '0', '0', '0', '0', '0', '0', '0'];
+
+    const s = address.split('::');
+    if (s.length === 2) {


Is there actually any IPv6 without the two dots?

Yes. The :: notation is just an abbreviation for any number of zero-bytes. There can very well be an IPv6-address without any zero-bytes. And the address comes from the Host-header, so it might be that the http-client requested an address with the IPv6 address spelled out, e.g. https://[0.0.0.0.0.0.0.1] for localhost.

BridgeAR · 2017-08-29T07:01:02Z

lib/internal/tls.js

+      }
+    }
+
+    return b.join(':');


If you want to improve the performance you could do the following to improve it further.

use string concatenation and not use the b array at all. And at the end add "0:".repeat(restLen).

replace the RegExp with a loop that checks something like var n = 0; while (str.charCodeAt(n++) === 48); so you know what part you want to slice out.

use address.indexOf('::') and address.split(':') directly. You know until where the first part and until where the second part will go because of the indexOf and you only have to split a single time instead of three times (you could optimize this the same as the RegExp by using a loop and not splitting at all. It should be even faster).

The if (s1[n]) is not necessary as far as I see it. The current test cases have no entry that does not match but in case e.g. :fe80::0000: is a valid entry, just check the very first/last entry if it is entry and count down the length.

But this is out of my perspective not that important as a first draft and should not hold off the PR from being merged.

This code is run once per https-request, and only if the host is specified with an IPv6 address. The code is already quite complex as it is and I would not like to complicate it further right now.

The if (s1[n]) is needed for '::1'

BridgeAR · 2017-08-29T07:01:32Z

test/internet/test-tls-canonical-ip.js

+assert.strictEqual(tls.canonicalIp('fe80::'), 'fe80:0:0:0:0:0:0:0');
+assert.strictEqual(
+  tls.canonicalIp('fe80::0000:0010:0001'),
+  'fe80:0:0:0:0:0:10:1');


It would be good to have some more test cases if there are any more out of the ordinary ones. It has been a long time that I checked how a valid IPv6 looks like.

Your comment made me realize that I didn't test IPv6-addresses without double colons. And of course that didn't work. Fix coming up.

BridgeAR · 2017-09-02T06:59:07Z

PTAL @nodejs/http @nodejs/crypto

BridgeAR · 2017-09-13T19:15:10Z

This needs some reviews @nodejs/collaborators

joyeecheung · 2017-09-17T01:45:50Z

test/internet/test-tls-canonical-ip.js

+// of a TLS certificate
+
+const assert = require('assert');
+const tls = require('internal/tls');


Does this test work without // Flags: --expose-internals? (See the testing guide)

You're right, it actually doesn't. I have added that flag now. While debugging this I looked at the other tests in the test/internet/ directory and realised that they all needed internet access (which my test doesn't). I therefore moved the test to test/parallel which should make sure that it gets run by make test

BridgeAR · 2017-09-22T21:44:30Z

@nodejs/tsc PTAL.

@mattiash this needs a rebase.

mattiash · 2017-09-22T21:50:14Z

@BridgeAR Should I rebase it now before the review is done, or will that interrupt the review process?

BridgeAR · 2017-09-22T22:02:45Z

@mattiash it is always best to rebase and to have a clean state to review. Otherwise changed could come in be missed afterwards.

mattiash · 2017-09-22T22:36:43Z

Branch rebased as requested. The only conflict was that someone had created internal/tls.js (which I also had) so I did a trivial merge. make -j4 test still passes for me locally.

mscdex · 2017-09-22T23:27:22Z

FWIW it looks like there's something up with the author part of the commits' metadata. Perhaps there is a difference in author and committer email addresses or something?

mattiash · 2017-09-23T07:27:16Z

FWIW it looks like there's something up with the author part of the commits' metadata. Perhaps there is a difference in author and committer email addresses or something?

Yes, I wrote the patch at work and rebased it from home, that's why the author/committer don't match. I'll rebase again to avoid any confusion. Sorry for the trouble.

BridgeAR · 2017-09-24T15:51:05Z

@nodejs/http @nodejs/crypto PTAL. This needs some LGs

mattiash · 2017-09-24T20:26:32Z

Note to reviewers: https://github.com/mattiash/test-checkServerIdentity explains what the problem is and contains a test-case that fails with node 8.5.0 but succeeds with the code from this PR.

bnoordhuis

This PR adds a fourth IPv6 address parser. We currently have:

net.isIPv6() (which calls out to uv_inet_pton()),
the --inspect=[::0]:1234 parser in src/node_debug_options.cc and
the URL parser (which also parses IPv6 addresses) in src/node_url.cc.

Rather than adding yet another parser, there is probably a way to share code with one of them.

Failing that, a binding method could be added that calls uv_inet_pton() and uv_inet_ntop() to normalize an address. That would ensure consistency with other parts of node.

uv_inet_ntop() will canonicalize them to short form, not long form. I don't think that matters but if it does, you could always replace it with a snprintf() that prints the octets to string.

jasnell · 2017-09-25T15:31:07Z

I have to agree with @bnoordhuis. Reusing one of the existing ipv6 parsing routines is best.

mattiash · 2017-09-26T18:02:21Z

Thanks for the review. I looked at the different IPv6 parsing routines already in node, but I couldn't find anything that I could use out of the box to convert IPv6 addresses to a canonical format. I used @bnoordhuis suggestion about uv_inet_ntop/pton which works for my purposes.

trevnorris · 2017-09-27T08:17:19Z

src/cares_wrap.cc

+
+    args.GetReturnValue().Set(String::NewFromUtf8(isolate, canonical_ip));
+  } else {
+    args.GetReturnValue().Set(Undefined(isolate));


Default return value is undefined. No reason to set it here if it hasn't be set prior.

mattiash · 2017-09-27T18:25:39Z

Default return value is undefined. No reason to set it here if it hasn't be set prior.

Fixed.

bnoordhuis

Left some comments but nearly there.

bnoordhuis · 2017-09-27T18:47:20Z

lib/_http_agent.js

+      // [::1] => ::1
+      // [::1]:123 => ::1
+      if (hostHeader.startsWith('[')) {
+        options.servername = hostHeader.split(']')[0].substr(1);


.split(']', 1) is more efficient.

For bonus performance points, if you're going to slice the string anyway, call .indexOf() and then .substr(1, index) (and default index to hostHeader.length if it's -1.)

Lastly, I would say that it should pass on the string untouched if it doesn't contain a matching ].

bnoordhuis · 2017-09-27T18:53:35Z

src/cares_wrap.cc

@@ -1935,6 +1935,30 @@ void IsIPv6(const FunctionCallbackInfo<Value>& args) {
  }
 }

+void canonicalIP(const FunctionCallbackInfo<Value>& args) {


Style: CanonicalIP

As well, I'd call it CanonicalizeIP for two reasons:

It makes it clear it's an imperative, not a predicate (i.e., it transforms, it doesn't just check.)

lI is almost indistinguishable from ll, II, etc. with some fonts. (Minor thing. Still.)

bnoordhuis · 2017-09-27T18:56:21Z

src/cares_wrap.cc

+    char canonical_ip[INET_ADDRSTRLEN];
+
+    int err = uv_inet_ntop(AF_INET, address_buffer, canonical_ip,
+      sizeof(canonical_ip));


Style: please line up the argument.

bnoordhuis · 2017-09-27T18:58:44Z

src/cares_wrap.cc

+
+    args.GetReturnValue().Set(String::NewFromUtf8(isolate, canonical_ip));
+  } else if (uv_inet_pton(AF_INET6, *ip, &address_buffer) == 0) {
+    char canonical_ip[INET6_ADDRSTRLEN];


I'd just move this up a scope, it's big enough to hold IPv4 and IPv6 address strings. Shaves off a few lines of code.

bnoordhuis · 2017-09-27T19:01:50Z

test/parallel/test-tls-canonical-ip.js

+assert.strictEqual(canonicalIP('fe80:0:0:0:0:0:0:0'), 'fe80::');
+assert.strictEqual(
+  canonicalIP('fe80::0000:0010:0001'),
+  'fe80::10:1');


This fits on one line. If it didn't, house style is to write it like this:

assert.strictEqual(canonicalIP('fe80::0000:0010:0001'), 'fe80::10:1');

Only if that wouldn't fit in 80 columns would you use one-argument-one-line style but then please indent by four spaces. (Truth be told, I would have expected the linter to complain about that.)

mattiash · 2017-09-28T12:50:40Z

Addressed all comments from @bnoordhuis.

apapirovski · 2017-11-03T01:38:05Z

Landed in b6df87e

Congrats on becoming a Contributor @mattiasholmlund!

- Properly handle IPv6 in Host header when setting servername. - When comparing IP addresses against addresses in the subjectAltName field of a certificate, format the address correctly before doing the string comparison. PR-URL: nodejs#14772 Fixes: nodejs#14736 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-11-14T17:22:51Z

Letting this bake in Current a bit longer before landing into LTS

- Properly handle IPv6 in Host header when setting servername. - When comparing IP addresses against addresses in the subjectAltName field of a certificate, format the address correctly before doing the string comparison. PR-URL: #14772 Fixes: #14736 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

mattiasholmlund · 2018-01-15T09:07:21Z

Has this baked in master for long enough now to be considered for the v8 lts? The PR was included in Version 9.1.0 2017-11-07, so it has been out in the wild for over two months now.

- Properly handle IPv6 in Host header when setting servername. - When comparing IP addresses against addresses in the subjectAltName field of a certificate, format the address correctly before doing the string comparison. PR-URL: nodejs#14772 Fixes: nodejs#14736 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Properly handle IPv6 in Host header when setting servername. - When comparing IP addresses against addresses in the subjectAltName field of a certificate, format the address correctly before doing the string comparison. PR-URL: #14772 Fixes: #14736 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

gibfahn · 2018-01-26T18:44:16Z

Has this baked in master for long enough now to be considered for the v8 lts?

I think so, I've landed it on v8.x-staging.

- Properly handle IPv6 in Host header when setting servername. - When comparing IP addresses against addresses in the subjectAltName field of a certificate, format the address correctly before doing the string comparison. PR-URL: #14772 Fixes: #14736 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-02-27T02:46:42Z

backport to v6.x-staging in 8e7ac25

Notable Changes: * http, tls: - better support for IPv6 addresses (Mattias Holmlund) #14772 PR-URL: #19027

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable Changes: * http, tls: - better support for IPv6 addresses (Mattias Holmlund) nodejs#14772 PR-URL: nodejs#19027

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

nodejs-github-bot added http Issues or PRs related to the http subsystem. tls Issues and PRs related to the tls subsystem. labels Aug 11, 2017

mscdex reviewed Aug 11, 2017

View reviewed changes

jasnell reviewed Aug 13, 2017

View reviewed changes

jasnell requested review from indutny and bnoordhuis August 21, 2017 21:55

BridgeAR reviewed Aug 29, 2017

View reviewed changes

joyeecheung reviewed Sep 17, 2017

View reviewed changes

mattiash force-pushed the issue-14736-tls-ipv6-address branch from ddcd49c to de4fa4c Compare September 22, 2017 22:34

mattiash force-pushed the issue-14736-tls-ipv6-address branch from de4fa4c to 347af99 Compare September 23, 2017 07:31

bnoordhuis requested changes Sep 25, 2017

View reviewed changes

trevnorris reviewed Sep 27, 2017

View reviewed changes

bnoordhuis requested changes Sep 27, 2017

View reviewed changes

apapirovski closed this Nov 3, 2017

lpinca added lts-watch-v6.x labels Nov 6, 2017

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

gibfahn added the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 14, 2017

mattiasholmlund mentioned this pull request Jan 23, 2018

[v8.x backport] http, tls: better support for IPv6 addresses #18317

Closed

gibfahn removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Jan 26, 2018

gibfahn added land-on-v8.x and removed lts-watch-v8.x labels Jan 26, 2018

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Feb 27, 2018

This was referenced Feb 27, 2018

v6.13.1 proposal #19025

Closed

v6.13.1 proposal #19027

Merged

MylesBorins added a commit that referenced this pull request Mar 6, 2018

2018-03-06, Version 6.13.1 'Boron' (LTS)

1246f72

Notable Changes: * http, tls: - better support for IPv6 addresses (Mattias Holmlund) #14772 PR-URL: #19027

MylesBorins added a commit that referenced this pull request Mar 6, 2018

2018-03-06, Version 6.13.1 'Boron' (LTS)

4e15679

Notable Changes: * http, tls: - better support for IPv6 addresses (Mattias Holmlund) #14772 PR-URL: #19027

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

2018-03-06, Version 6.13.1 'Boron' (LTS)

1a7e3d3

Notable Changes: * http, tls: - better support for IPv6 addresses (Mattias Holmlund) nodejs#14772 PR-URL: nodejs#19027

Issue 14736 tls ipv6 address #14772

Issue 14736 tls ipv6 address #14772

Conversation

mattiash commented Aug 11, 2017

Checklist

Affected core subsystem(s)

mscdex Aug 11, 2017 • edited Loading

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

mattiash commented Aug 21, 2017

jasnell commented Aug 21, 2017

jasnell commented Aug 23, 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

BridgeAR commented Sep 2, 2017

BridgeAR commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 22, 2017

mattiash commented Sep 22, 2017

BridgeAR commented Sep 22, 2017

mattiash commented Sep 22, 2017

mscdex commented Sep 22, 2017

mattiash commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

mattiash commented Sep 24, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

jasnell commented Sep 25, 2017

mattiash commented Sep 26, 2017

Choose a reason for hiding this comment

mattiash commented Sep 27, 2017

bnoordhuis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattiash commented Sep 28, 2017

apapirovski commented Nov 3, 2017 • edited Loading

MylesBorins commented Nov 14, 2017

mattiasholmlund commented Jan 15, 2018

gibfahn commented Jan 26, 2018

MylesBorins commented Feb 27, 2018

mscdex Aug 11, 2017 •

edited

Loading

apapirovski commented Nov 3, 2017 •

edited

Loading