Set ServerName in tlsConf when using verify-full mode. #273

Merged
merged 1 commit into from Jul 5, 2014

Projects

None yet

3 participants

@echlebek

I found this to be necessary as my client broke when making SSL connects to postgres after the go 1.3 update. It seems that when using tls.Client, either ServerName or InsecureSkipVerify must be specified.

The tls authors seem to recommend using Dial; this takes care of setting ServerName for verification. That may be more appropriate, but this change at least fixes the immediate issue.

AFAICT, In go 1.2 if ServerName was not specified and tls.Client was used, no hostname checking was done, making MITM possible.

@johto

I found this to be necessary as my client broke when making SSL connects to postgres after the go 1.3 update. It seems that when using tls.Client, either ServerName or InsecureSkipVerify must be specified.

Urgh. OK..

The tls authors seem to recommend using Dial; this takes care of setting ServerName for verification. That may be more appropriate, but this change at least fixes the immediate issue.

Dial won't work with the postgres protocol since there's an unencrypted handshake before the TLS one.

AFAICT, In go 1.2 if ServerName was not specified and tls.Client was used, no hostname checking was done, making MITM possible.

I haven't looked at the changes in detail, but what exactly does this mean? What's the impact if, for example, you're connecting to an IP address and the remote host offers a certificate for "foobar.com"; will that not work after this change?

Also, I'm not saying you have to fix this, but we should really add a test case for this; the fact that we didn't have one is the reason nobody noticed this on 1.3 until now.

@echlebek

I haven't looked at the changes in detail, but what exactly does this mean? What's the impact if, for example, you're connecting to an IP address and the remote host offers a certificate for "foobar.com"; will that not work after this change?

Sorry, I should have referenced https://code.google.com/p/go/issues/detail?id=7342 and https://groups.google.com/forum/#!msg/golang-codereviews/xh56a98i8-s/s1suvegTzR0J

Yes, it appears that if you use a Config.ServerName that does not match the certificate server name this will break. For example if I spec an IP address in place of the FQDN I get the following:

x509: cannot validate certificate for 10.x.x.x because it doesn't contain any IP SANs

Which I think is the correct behaviour.

Also, I'm not saying you have to fix this, but we should really add a test case for this; the fact that we didn't have one is the reason nobody noticed this on 1.3 until now.

Definitely; I would be happy to do this, but it might take me longer than someone closer to the project.

@johto

@echlebek: OK, I've had a closer look at this and as far as this change goes, it looks reasonable.

Though it appears that we're not fully compatible with libpq here; we appear to only be supporting the system CA configuration, whereas libpq only supports a ~/.postgresql/root.crt (which can be overridden by sslrootcert or PGSSLROOTCERT) file. I haven't looked at how hard it would be to use the system certificate in Travis, but perhaps it would make sense to add the sslrootcert option now to make testing easier? I guess I'm volunteering to do the work if nobody else wants to.

@johto

Oh, I forgot that we actually have a pull request for that, #226. ping @cmelbye: what's your status? Are you waiting on #208? AFAICT that still hasn't been updated after my last round of review.

@cmelbye

We've been using a fork with #226 in production for the past few months. I can clean it up and add tests so it can be merged.

@johto johto merged commit d4912a4 into lib:master Jul 5, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@johto

@cmelbye: Thanks, that would be great. Let's move the discussion to the appropriate pull request.

In the meanwhile, I've verified locally that this patch fixes the problem and have pushed it into master in commit d4912a4. Thanks!

@echlebek

Thanks @johto !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment