SNI is incorrectly set to IP addresses using the https module

[rcombs]

• Version: v9.3.0
• Platform: Darwin MacBook-Pro.localdomain 17.4.0 Darwin Kernel Version 17.4.0: Tue Dec 19 11:20:50 PST 2017; root:xnu-4570.41.2~2/RELEASE_X86_64 x86_64
• Subsystem: https

Example code:

const https = require('https');
https.get('https://127.0.0.1:8888', (res) => { console.log('foo'); });

If you have a server listening on 8888, it will receive a TLS ClientHello containing a ServerName extension indicating the IPv4 address literal 127.0.0.1. RFC 6066 states:

Literal IPv4 and IPv6 addresses are not permitted in "HostName".

This can cause unexpected results depending on how the server handles invalid or unexpected SNI, which may be different from the no-SNI case (which is what's expected here).

I can imagine about three ways to address this:

• Modify the string case in https.request to drop the hostname field if it's an IP address
  • This has the benefit of simplicity, but still produces invalid requests if the user parses the URL themselves
• Modify _tls_wrap.connect to drop the server name if it's an IP address
  • This should handle most accidental cases.
• Modify TLSSocket.setServername to be a no-op (or set to null) if the passed value is an IP address
  • This handles all cases (short of the user calling into the C++ code directly), but prevents a user from explicitly doing the wrong thing via public API if they want to (not sure if that's considered desirable).

[addaleax]

@rcombs I think both options 2 & 3 are okay, and I’d have a preference for option 2 for the reason you mentioned.

Would you be willing to open a PR yourself?

[joyeecheung]

Since it's not permitted in the RFC, we might as well throw an error if cares.isIP(name) is thruthy instead of ignoring it silently.

[bnoordhuis]

People use SNI with IP addresses, see #14736. That means we can't just silently break it.

[rcombs]

That issue seems to be around SAN handling, not (deliberately) about SNI?
See example in libcurl, where they silently don't set the hostname if it's a v4 or v6 address: https://github.com/curl/curl/blob/fa3dbb9a147488a2943bda809c66fc497efe06cb/lib/vtls/openssl.c#L2445

[bnoordhuis]

not (deliberately) about SNI?

Not deliberately, no.

[rcombs]

Sounds like it should be fine to drop SNI for IPs, then, as long as we make sure not to break verification of IP certs?

[bnoordhuis]

What I mean is that I don't think their use case could work if the IP address wasn't forwarded in the SNI record. And if not them specifically, then it's probably still true for others since node.js (and openssl-based servers in general) accept addresses.

In case of doubt we usually start by emitting warnings for a while. That gives users time to update their code and doubles as a kind of poor man's telemetry - people tend to open issues about features dear to their hearts.

If there are pressing concerns, like if there were security implications, we could take a shortcut but I don't think that's the case here.

[rcombs]

Servers generally treat connections without SNI as if the hostname were the IP address the connection was made to for the purposes of vhosts and such; if there's a case that requires this, it's a buggy server that's only used with a node client (as I'm not aware of any other HTTPS stacks that do this; certainly not libcurl or any major browser).
If you think that's still a likely case, what would you say the best route going forward is? A warning if an IP is passed to setServername, later converted to a no-op at some level?
I do have a use-case that this breaks by triggering unexpected behavior in the server, but we're currently working around it by explicitly parsing the URL in the client and deleting the hostname field in the resulting object.

[bnoordhuis]

A warning if an IP is passed to setServername, later converted to a no-op at some level?

Yes, that's sounds like the way forward. That could land in node 10 and probably node 9.

[rcombs]

I'm slightly concerned that this could lead to warning spam for people who e.g. pass in a string (i.e. aren't clearly intending to trigger SNI). Is there a common pattern for one-time warnings?

[bnoordhuis]

Yep. Nothing fancy, see 560f797 for an example.