Pass host to tls.connect for certificate validation

[RazerM]

Fixes #2263

[charmander]

Someone’ll need to write a test for this. I wonder what it does when host is a Unix domain socket?

[RazerM]

I wonder what it does when host is a Unix domain socket?

I think from the links I put in #2263 it's clear that host is only used for checking the server identity, and that for non-IP addresses this is currently set by servername. So nothing has changed for unix domain sockets in that regard.

[RazerM]

What can I do to get this merged?

[vitaly-t]

When indeed? :) I got another issue open about the same, it seems.

[charmander]

@vitaly-t That doesn’t sound like the same issue; the host in that one isn’t an IP address, and the claimed behaviour is missing SNI, not certificate validation failure.

@RazerM It needs a test.

[vitaly-t]

That doesn’t sound like the same issue; the host in that one isn’t an IP address, and the claimed behaviour is missing SNI

Yeah, sorry, I didn't look up close. But what's valid there is something else in the configuration here, since pg-promise just passes the config object on to this driver, without any modification.

[charmander]

@vitaly-t It looks like pg sets servername unconditionally, but we want to allow it to be overridden; working on a fix for that. (That should mean the SNI is the same as the host instead of missing, but that’s probably what the issue reporter means.)

[RazerM]

@charmander #1890 was merged without a test despite the consequences for IP addresses so I get the impression the test setup wouldn't easily support such a test. The existing tests all pass for this PR too.

I think it should be clear from the description in the issue what is wrong. It's quite frustrating because it's fairly easy to see why IP addresses don't work and how pg got like this:

1. pg passes a socket to tls.connect.
2. tls.connect has no idea which hostname it needs to check the identity for when it has a socket.
3. tls.connect uses options.servername || options.host || 'localhost' to verify server identity.
4. pg used to always pass servername, which as noted should not be done for IP addresses.
5. PR Skip TLS SNI if host is IP address #1890 changed this to only do it for non-IP addresses, therefore the tls.connect identity verification uses 'localhost'. This is bad!
6. The snippet above shows why removing servername for IP addresses results in 'localhost' being used. You need to pass host!

[charmander]

Of course it’s clear what’s wrong. That doesn’t mean it doesn’t need a test. It was able to break in the first place because there was no test.

I can write the test, but it’ll be a while.

[penous]

Hey all, any updates on this?

[jhnns]

Just stumbled upon this issue. I can confirm that this PR would fix the problem. Node's docs are quite clear in that regard that "path, host, and port are ignored, except for certificate validation".

@charmander Would you still merge it if it had tests?

[charmander]

@jhnns Yep!

[jhnns]

@charmander I created a PR with the implementation and the tests: #3756