code review 6448154: net/http: Set TLSClientConfig.ServerName on every HTTP ...

net/http: Set TLSClientConfig.ServerName on every HTTP request.

This makes SNI "just work" for callers using the standard http.Client.

Since we now have a test that depends on the httptest.Server cert, change
the cert to be a CA (keeping all other fields the same).

[dborowitz, 2012-08-15 19:15:39]

Hello dsymonds (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dborowitz, 2012-08-15 19:19:01]

This includes my attempt at a test. However, I'm unable to get the client's TLS
config to accept the test server's self-signed cert; the test fails with the
error:
expected successful TLS connection, got error: Get https://127.0.0.1:60955:
x509: certificate signed by unknown authority

Note that I do parse the cert out of the server and add it to the RootCAs pool,
but this apparently doesn't work. A cursory inspection of
x509.Certificate.Verify suggests that there needs to be another cert in the
chain for it to work, but I'm wading in a bit deep here.

On 2012/08/15 19:15:39, dborowitz wrote:
> Hello dsymonds (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

[dborowitz, 2012-08-15 19:19:34]

Hello dsymonds@golang.org (cc: agl1, golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-08-15 19:34:13]

On Wed, Aug 15, 2012 at 12:19 PM,  <dborowitz@google.com> wrote:
> Note that I do parse the cert out of the server and add it to the
> RootCAs pool, but this apparently doesn't work. A cursory inspection of
> x509.Certificate.Verify suggests that there needs to be another cert in
> the chain for it to work, but I'm wading in a bit deep here.

I can't think of a reason why a single, self-signed certificate
wouldn't work but it looks like the test is using the cert from
httptest/server.go. That certificate isn't marked as a CA so would be
rejected for that reason if nothing else.

I can mint a new, self-signed, CA cert for the test if you like, but
it would have to be later tonight.


Cheers

AGL

[dborowitz, 2012-08-15 19:38:52]

On Wed, Aug 15, 2012 at 12:34 PM, Adam Langley <agl@golang.org> wrote:

> On Wed, Aug 15, 2012 at 12:19 PM,  <dborowitz@google.com> wrote:
> > Note that I do parse the cert out of the server and add it to the
> > RootCAs pool, but this apparently doesn't work. A cursory inspection of
> > x509.Certificate.Verify suggests that there needs to be another cert in
> > the chain for it to work, but I'm wading in a bit deep here.
>
> I can't think of a reason why a single, self-signed certificate
> wouldn't work but it looks like the test is using the cert from
> httptest/server.go. That certificate isn't marked as a CA so would be
> rejected for that reason if nothing else.
>

Good call, I should have actually inspected the cert a little more closely
(i.e. at all :).


> I can mint a new, self-signed, CA cert for the test if you like, but
> it would have to be later tonight.
>

I don't mind minting the cert myself and including it in this change, if
you think that's the right way to go.


>
> Cheers
>
> AGL
>

[agl1, 2012-08-15 19:45:48]

On Wed, Aug 15, 2012 at 12:38 PM, Dave Borowitz <dborowitz@google.com> wrote:
> I don't mind minting the cert myself and including it in this change, if you
> think that's the right way to go.

I think it'll fix the problem. (Although you need to be careful to
match the existing cert as much as possible, save for the additional
basicConstraints.)


cheers

AGL

[dborowitz, 2012-08-15 19:53:35]

On Wed, Aug 15, 2012 at 12:45 PM, Adam Langley <agl@golang.org> wrote:

> On Wed, Aug 15, 2012 at 12:38 PM, Dave Borowitz <dborowitz@google.com>
> wrote:
> > I don't mind minting the cert myself and including it in this change, if
> you
> > think that's the right way to go.
>
> I think it'll fix the problem. (Although you need to be careful to
> match the existing cert as much as possible, save for the additional
> basicConstraints.)
>

Will do, thanks for the suggestions. While we're at it, is there any reason
we can't just generate the cert on the fly in httptest? That would make
future small changes easier.


>
> cheers
>
> AGL
>

[agl1, 2012-08-15 19:56:57]

On Wed, Aug 15, 2012 at 12:53 PM, Dave Borowitz <dborowitz@google.com> wrote:
> Will do, thanks for the suggestions. While we're at it, is there any reason
> we can't just generate the cert on the fly in httptest? That would make
> future small changes easier.

RSA keygen and signing is rather slow: esp on things like the ARM builder.


Cheers

AGL

[dborowitz, 2012-08-15 21:23:49]

Hello dsymonds@golang.org (cc: agl@golang.org, golang-dev@googlegroups.com),

Please take another look.

[dborowitz, 2012-08-15 21:31:25]

Ok, I give up. How do I get hg+codereview to upload my changes? (Git is my day
job, so my brain may just be permanently warped :). The contribution guide just
says to run "hg mail" when I have new changes.

$ hg status
M src/pkg/net/http/client_test.go
M src/pkg/net/http/httptest/server.go
M src/pkg/net/http/transport.go
$ hg diff **/server.go
diff -r 9bc8fafec2f5 src/pkg/net/http/httptest/server.go
--- a/src/pkg/net/http/httptest/server.go       Wed Aug 15 11:09:34 2012 -0700
+++ b/src/pkg/net/http/httptest/server.go       Wed Aug 15 14:28:30 2012 -0700
@@ -184,15 +184,15 @@
 // "127.0.0.1" and "[::1]", expiring at the last second of 2049 (the end
 // of ASN.1 time).
 var localhostCert = []byte(`-----BEGIN CERTIFICATE-----
-MIIBOTCB5qADAgECAgEAMAsGCSqGSIb3DQEBBTAAMB4XDTcwMDEwMTAwMDAwMFoX
+MIIBTTCB+qADAgECAgEAMAsGCSqGSIb3DQEBBTAAMB4XDTcwMDEwMTAwMDAwMFoX
 DTQ5MTIzMTIzNTk1OVowADBaMAsGCSqGSIb3DQEBAQNLADBIAkEAsuA5mAFMj6Q7
 qoBzcvKzIq4kzuT5epSp2AkcQfyBHm7K13Ws7u+0b5Vb9gqTf5cAiIKcrtrXVqkL
-8i1UQF6AzwIDAQABo08wTTAOBgNVHQ8BAf8EBAMCACQwDQYDVR0OBAYEBAECAwQw
-DwYDVR0jBAgwBoAEAQIDBDAbBgNVHREEFDASggkxMjcuMC4wLjGCBVs6OjFdMAsG
-CSqGSIb3DQEBBQNBAJH30zjLWRztrWpOCgJL8RQWLaKzhK79pVhAx6q/3NrF16C7
-+l1BRZstTwIGdoGId8BRpErK1TXkniFb95ZMynM=
------END CERTIFICATE-----
-`)
+8i1UQF6AzwIDAQABo2MwYTAOBgNVHQ8BAf8EBAMCACQwEgYDVR0TAQH/BAgwBgEB
+/wIBATANBgNVHQ4EBgQEAQIDBDAPBgNVHSMECDAGgAQBAgMEMBsGA1UdEQQUMBKC
+CTEyNy4wLjAuMYIFWzo6MV0wCwYJKoZIhvcNAQEFA0EAj1Jsn/h2KHy7dgqutZNB
+nCGlNN+8vw263Bax9MklR85Ti6a0VWSvp/fDQZUADvmFTDkcXeA24pqmdUxeQDWw
+Pg==
+-----END CERTIFICATE-----`)
 
 // localhostKey is the private key for localhostCert.
 var localhostKey = []byte(`-----BEGIN RSA PRIVATE KEY-----
$ hg mail 6448154
Issue updated. URL: http://codereview.appspot.com/6448154






Here are the changes I made to the cert, this should be the minimal set
necessary:

$ diff -c1 <(openssl x509 -in ~/w/httptest_cert.txt -noout -text) <(openssl x509
-in ~/w/httptest_cert_new.txt -noout -text)
*** /proc/self/fd/11    2012-08-15 14:29:56.366319610 -0700
--- /proc/self/fd/12    2012-08-15 14:29:56.366319610 -0700
***************
*** 23,24 ****
--- 23,26 ----
                  Key Encipherment, Certificate Sign
+             X509v3 Basic Constraints: critical
+                 CA:TRUE, pathlen:1
              X509v3 Subject Key Identifier: 
***************
*** 31,35 ****
      Signature Algorithm: sha1WithRSAEncryption
!          91:f7:d3:38:cb:59:1c:ed:ad:6a:4e:0a:02:4b:f1:14:16:2d:
!          a2:b3:84:ae:fd:a5:58:40:c7:aa:bf:dc:da:c5:d7:a0:bb:fa:
!          5d:41:45:9b:2d:4f:02:06:76:81:88:77:c0:51:a4:4a:ca:d5:
!          35:e4:9e:21:5b:f7:96:4c:ca:73
--- 33,37 ----
      Signature Algorithm: sha1WithRSAEncryption
!          8f:52:6c:9f:f8:76:28:7c:bb:76:0a:ae:b5:93:41:9c:21:a5:
!          34:df:bc:bf:0d:ba:dc:16:b1:f4:c9:25:47:ce:53:8b:a6:b4:
!          55:64:af:a7:f7:c3:41:95:00:0e:f9:85:4c:39:1c:5d:e0:36:
!          e2:9a:a6:75:4c:5e:40:35:b0:3e

[agl1, 2012-08-15 22:16:33]

Assuming that the test passes, the certificate changes LGTM.

[dborowitz, 2012-08-17 01:35:41]

Friendly ping.

[gobot, 2012-08-17 04:29:02]

R=bradfitz (assigned by dsymonds)

[bradfitz, 2012-08-20 04:58:25]

Thanks for doing this.  Few little things, then good.

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go
File src/pkg/net/http/client_test.go (right):

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test....
src/pkg/net/http/client_test.go:494: t.Fatalf("expected client to set ServerName
127.0.0.1, got: %q", r.TLS.ServerName)
HTTP handlers run in their own goroutine, and Fatalf shouldn't be used from a
different goroutine.  Just do Errorf here instead of Fatalf.

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test....
src/pkg/net/http/client_test.go:496: w.Write([]byte("Hello"))
could probably remove these lines if you're never checking the body. writing
nothing is still a 200 OK and not an error for the HTTP client.

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:379: if err != nil {
I'm not sure these 3 error checking lines are worth anything.  cm.addr() will be
valid, and even if it's not, host will just be "", which is what ServerName
already is.  If the concern is that cm.addr() might be bogus, that should be
fixed and/or checked elsewhere.

host, _, _ := net.SplitHostPort(cm.addr())

http://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:385: cfgcopy := *cfg
cfgcopy isn't Go's normalCaseStyle.

I'd say cfgCopy or cfg2 or clone.  Your preference.

Noting that it's a shallow clone but that's acceptable might be nice too.

clone := *cfg // shallow clone
clone.ServerName = host
cfg = &clone

[dborowitz, 2012-08-20 15:05:14]

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go
File src/pkg/net/http/client_test.go (right):

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test...
src/pkg/net/http/client_test.go:494: t.Fatalf("expected client to set ServerName
127.0.0.1, got: %q", r.TLS.ServerName)
On 2012/08/20 04:58:25, bradfitz wrote:
> HTTP handlers run in their own goroutine, and Fatalf shouldn't be used from a
> different goroutine.  Just do Errorf here instead of Fatalf.

Done.

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test...
src/pkg/net/http/client_test.go:496: w.Write([]byte("Hello"))
On 2012/08/20 04:58:25, bradfitz wrote:
> could probably remove these lines if you're never checking the body. writing
> nothing is still a 200 OK and not an error for the HTTP client.

Done.

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:379: if err != nil {
On 2012/08/20 04:58:25, bradfitz wrote:
> I'm not sure these 3 error checking lines are worth anything.  cm.addr() will
be
> valid, and even if it's not, host will just be "", which is what ServerName
> already is.  If the concern is that cm.addr() might be bogus, that should be
> fixed and/or checked elsewhere.
> 
> host, _, _ := net.SplitHostPort(cm.addr())

No specific concerns here, I just hadn't inspected SplitHostPort to see what
might cause an error, so I did the safe thing and checked it. Done.

https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:385: cfgcopy := *cfg
On 2012/08/20 04:58:25, bradfitz wrote:
> cfgcopy isn't Go's normalCaseStyle.
> 
> I'd say cfgCopy or cfg2 or clone.  Your preference.
> 
> Noting that it's a shallow clone but that's acceptable might be nice too.
> 
> clone := *cfg // shallow clone
> clone.ServerName = host
> cfg = &clone

Done.

[dborowitz, 2012-08-22 15:45:35]

Ping.

On 2012/08/20 15:05:14, dborowitz wrote:
>
https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go
> File src/pkg/net/http/client_test.go (right):
> 
>
https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test...
> src/pkg/net/http/client_test.go:494: t.Fatalf("expected client to set
ServerName
> 127.0.0.1, got: %q", r.TLS.ServerName)
> On 2012/08/20 04:58:25, bradfitz wrote:
> > HTTP handlers run in their own goroutine, and Fatalf shouldn't be used from
a
> > different goroutine.  Just do Errorf here instead of Fatalf.
> 
> Done.
> 
>
https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test...
> src/pkg/net/http/client_test.go:496: w.Write([]byte("Hello"))
> On 2012/08/20 04:58:25, bradfitz wrote:
> > could probably remove these lines if you're never checking the body. writing
> > nothing is still a 200 OK and not an error for the HTTP client.
> 
> Done.
> 
> https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go
> File src/pkg/net/http/transport.go (right):
> 
>
https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.g...
> src/pkg/net/http/transport.go:379: if err != nil {
> On 2012/08/20 04:58:25, bradfitz wrote:
> > I'm not sure these 3 error checking lines are worth anything.  cm.addr()
will
> be
> > valid, and even if it's not, host will just be "", which is what ServerName
> > already is.  If the concern is that cm.addr() might be bogus, that should be
> > fixed and/or checked elsewhere.
> > 
> > host, _, _ := net.SplitHostPort(cm.addr())
> 
> No specific concerns here, I just hadn't inspected SplitHostPort to see what
> might cause an error, so I did the safe thing and checked it. Done.
> 
>
https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.g...
> src/pkg/net/http/transport.go:385: cfgcopy := *cfg
> On 2012/08/20 04:58:25, bradfitz wrote:
> > cfgcopy isn't Go's normalCaseStyle.
> > 
> > I'd say cfgCopy or cfg2 or clone.  Your preference.
> > 
> > Noting that it's a shallow clone but that's acceptable might be nice too.
> > 
> > clone := *cfg // shallow clone
> > clone.ServerName = host
> > cfg = &clone
> 
> Done.

[bradfitz, 2012-08-22 16:13:45]

LGTM

I'm back.

On Thu, Aug 23, 2012 at 1:45 AM, <dborowitz@google.com> wrote:

> Ping.
>
>
> On 2012/08/20 15:05:14, dborowitz wrote:
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/client_test.go<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go>
>
>> File src/pkg/net/http/client_test.**go (right):
>>
>
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/client_test.go#**newcode494<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go#newcode494>
>
>> src/pkg/net/http/client_test.**go:494: t.Fatalf("expected client to set
>>
> ServerName
>
>> 127.0.0.1, got: %q", r.TLS.ServerName)
>> On 2012/08/20 04:58:25, bradfitz wrote:
>> > HTTP handlers run in their own goroutine, and Fatalf shouldn't be
>>
> used from a
>
>> > different goroutine.  Just do Errorf here instead of Fatalf.
>>
>
>  Done.
>>
>
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/client_test.go#**newcode496<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/client_test.go#newcode496>
>
>> src/pkg/net/http/client_test.**go:496: w.Write([]byte("Hello"))
>> On 2012/08/20 04:58:25, bradfitz wrote:
>> > could probably remove these lines if you're never checking the body.
>>
> writing
>
>> > nothing is still a 200 OK and not an error for the HTTP client.
>>
>
>  Done.
>>
>
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/transport.go<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go>
>
>> File src/pkg/net/http/transport.go (right):
>>
>
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/transport.go#**newcode379<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go#newcode379>
>
>> src/pkg/net/http/transport.go:**379: if err != nil {
>> On 2012/08/20 04:58:25, bradfitz wrote:
>> > I'm not sure these 3 error checking lines are worth anything.
>>
> cm.addr() will
>
>> be
>> > valid, and even if it's not, host will just be "", which is what
>>
> ServerName
>
>> > already is.  If the concern is that cm.addr() might be bogus, that
>>
> should be
>
>> > fixed and/or checked elsewhere.
>> >
>> > host, _, _ := net.SplitHostPort(cm.addr())
>>
>
>  No specific concerns here, I just hadn't inspected SplitHostPort to
>>
> see what
>
>> might cause an error, so I did the safe thing and checked it. Done.
>>
>
>
> https://codereview.appspot.**com/6448154/diff/1002/src/pkg/**
>
net/http/transport.go#**newcode385<https://codereview.appspot.com/6448154/diff/1002/src/pkg/net/http/transport.go#newcode385>
>
>> src/pkg/net/http/transport.go:**385: cfgcopy := *cfg
>> On 2012/08/20 04:58:25, bradfitz wrote:
>> > cfgcopy isn't Go's normalCaseStyle.
>> >
>> > I'd say cfgCopy or cfg2 or clone.  Your preference.
>> >
>> > Noting that it's a shallow clone but that's acceptable might be nice
>>
> too.
>
>> >
>> > clone := *cfg // shallow clone
>> > clone.ServerName = host
>> > cfg = &clone
>>
>
>  Done.
>>
>
>
>
>
https://codereview.appspot.**com/6448154/<https://codereview.appspot.com/6448...
>

[bradfitz, 2012-08-22 16:15:49]

*** Submitted as http://code.google.com/p/go/source/detail?r=8d39afcd18b1 ***

net/http: Set TLSClientConfig.ServerName on every HTTP request.

This makes SNI "just work" for callers using the standard http.Client.

Since we now have a test that depends on the httptest.Server cert, change
the cert to be a CA (keeping all other fields the same).

R=bradfitz
CC=agl, dsymonds, gobot, golang-dev
http://codereview.appspot.com/6448154

Committer: Brad Fitzpatrick <bradfitz@golang.org>