brianc
Pass host to tls.connect for certificate validation
Someone’ll need to write a test for this. I wonder what it does when host is a Unix domain socket?
I wonder what it does when
hostis 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.
@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.
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.
@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.)
@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:
- pg passes a socket to
tls.connect. tls.connecthas no idea which hostname it needs to check the identity for when it has a socket.tls.connectusesoptions.servername || options.host || 'localhost'to verify server identity.- pg used to always pass
servername, which as noted should not be done for IP addresses. - PR #1890 changed this to only do it for non-IP addresses, therefore the
tls.connectidentity verification uses 'localhost'. This is bad! - The snippet above shows why removing servername for IP addresses results in 'localhost' being used. You need to pass
host!
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.
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?
