code review 11691043: net/http: respect tls.Config.ServerName in Transport

net/http: respect tls.Config.ServerName in Transport

When making an HTTPS client request, respect the
ServerName field in the tls.Config.

Fixes issue 5829

[bradfitz, 2013-07-22 20:59:15]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-07-22 21:01:09]

R=agl

If this is correct, I can think about how/whether this is easily testable.



On Mon, Jul 22, 2013 at 1:59 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: respect tls.Config.ServerName in Transport
>
> When making an HTTPS client request, respect the
> ServerName field in the tls.Config.
>
> Fixes issue 5829
>
> Please review this at
https://codereview.appspot.**com/11691043/<https://codereview.appspot.com/116...
>
> Affected files:
>   M src/pkg/net/http/transport.go
>
>
> Index: src/pkg/net/http/transport.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/transport.**go
> +++ b/src/pkg/net/http/transport.**go
> @@ -500,8 +500,8 @@
>                 if err = conn.(*tls.Conn).Handshake(); err != nil {
>                         return nil, err
>                 }
> -               if t.TLSClientConfig == nil ||
!t.TLSClientConfig.**InsecureSkipVerify
> {
> -                       if err =
conn.(*tls.Conn).**VerifyHostname(cm.tlsHost());
> err != nil {
> +               if !cfg.InsecureSkipVerify {
> +                       if err = conn.(*tls.Conn).**
> VerifyHostname(cfg.ServerName)**; err != nil {
>                                 return nil, err
>                         }
>                 }
>
>
>

[agl1, 2013-07-22 22:16:17]

LGTM

[bradfitz, 2013-07-22 23:41:44]

Thanks. PTAL (anybody) .. now with a test.



On Mon, Jul 22, 2013 at 3:16 PM, <agl@golang.org> wrote:

> LGTM
>
>
https://codereview.appspot.**com/11691043/<https://codereview.appspot.com/116...
>

[adg, 2013-07-23 04:19:41]

Huh? Shouldn't this test fail? I'm confused.

[bradfitz, 2013-07-23 04:26:36]

The comment on line 679 says why it should pass.

It used to fail, but now passes.

See the bottom of http://tip.golang.org/src/pkg/net/http/httptest/server.go

   203 // localhostCert is a PEM-encoded TLS cert with SAN IPs
   204 // "127.0.0.1" and "[::1]", expiring at the last second of 2049 (the
end
   205 // of ASN.1 time).
   206 // generated from src/pkg/crypto/tls:
   207 // go run generate_cert.go  --rsa-bits 512 --host 127.0.0.1,::1,
example.com --ca --start-date "Jan 1 00:00:00 1970" --duration=1000000h
   208 var localhostCert = []byte(`-----BEGIN CERTIFICATE-----
   209 MIIBdzCCASOgAwIBAgIBADALBgkqhkiG9w0BAQUwEjEQMA4GA1UEChMHQWNtZSBD


... the httptest.NewServer thinks that it is "example.com".




On Mon, Jul 22, 2013 at 9:19 PM, <adg@golang.org> wrote:

> Huh? Shouldn't this test fail? I'm confused.
>
>
https://codereview.appspot.**com/11691043/<https://codereview.appspot.com/116...
>

[adg, 2013-07-23 04:30:03]

What's the purpose of Get("https://some-other-host.tld/") ?

What about a test the other way? Where the transport is expecting another
hostname?

[bradfitz, 2013-07-23 04:37:41]

hostname(that) != "127.0.0.1" && hostname(that) != "example.com" ... both
of which would've passed.

Hence the Dial hook.  It's forcing it to dial the wrong place and
configured to force-ask for a TLS cert of "example.com", and testing that
the tls.Config.ServerName (non-empty, set to "example.com") takes
precedence over "some-other-host.tld" which previously incorrectly took
precedence.



On Mon, Jul 22, 2013 at 9:29 PM, Andrew Gerrand <adg@golang.org> wrote:

> What's the purpose of Get("https://some-other-host.tld/") ?
>
> What about a test the other way? Where the transport is expecting another
> hostname?
>
>

[adg, 2013-07-23 04:39:18]

LGTM

A comment to that effect would be nice. :-)


On 23 July 2013 14:37, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> hostname(that) != "127.0.0.1" && hostname(that) != "example.com" ... both
> of which would've passed.
>
> Hence the Dial hook.  It's forcing it to dial the wrong place and
> configured to force-ask for a TLS cert of "example.com", and testing that
> the tls.Config.ServerName (non-empty, set to "example.com") takes
> precedence over "some-other-host.tld" which previously incorrectly took
> precedence.
>
>
>
> On Mon, Jul 22, 2013 at 9:29 PM, Andrew Gerrand <adg@golang.org> wrote:
>
>> What's the purpose of Get("https://some-other-host.tld/") ?
>>
>> What about a test the other way? Where the transport is expecting another
>> hostname?
>>
>>
>

[bradfitz, 2013-07-23 05:39:07]

Done.



On Mon, Jul 22, 2013 at 9:38 PM, Andrew Gerrand <adg@golang.org> wrote:

> LGTM
>
> A comment to that effect would be nice. :-)
>
>
> On 23 July 2013 14:37, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>> hostname(that) != "127.0.0.1" && hostname(that) != "example.com" ...
>> both of which would've passed.
>>
>> Hence the Dial hook.  It's forcing it to dial the wrong place and
>> configured to force-ask for a TLS cert of "example.com", and testing
>> that the tls.Config.ServerName (non-empty, set to "example.com") takes
>> precedence over "some-other-host.tld" which previously incorrectly took
>> precedence.
>>
>>
>>
>> On Mon, Jul 22, 2013 at 9:29 PM, Andrew Gerrand <adg@golang.org> wrote:
>>
>>> What's the purpose of Get("https://some-other-host.tld/") ?
>>>
>>> What about a test the other way? Where the transport is expecting
>>> another hostname?
>>>
>>>
>>
>

[bradfitz, 2013-07-23 05:39:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=4bfbd12d6f4f ***

net/http: respect tls.Config.ServerName in Transport

When making an HTTPS client request, respect the
ServerName field in the tls.Config.

Fixes issue 5829

R=golang-dev, agl, adg
CC=golang-dev
https://codereview.appspot.com/11691043