code review 7307098: net/http, net/http/httptest: don't use 127.0.0.1 for lo...

net/http, net/http/httptest: don't use 127.0.0.1 for localhost TLS tests

This introduces httptest.(*Server).Transport() to return an
*http.Transport which special-cases the hostname "go-local-test"
to mean localhost, and makes the httptest certificates use
that hostname, instead of "127.0.0.1"

Update Issue 4658

[bradfitz, 2013-02-12 01:48:00]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-02-12 01:49:56]

Per the other thread, this doesn't work yet:

A couple tests fail with:

--- FAIL: TestClientWithCorrectTLSServerName (0.06 seconds)
client_test.go:607:  expected successful TLS connection, got error: Get
https://go-local-test: x509: certificate signed by unknown authority
--- FAIL: TestNextProtoUpgrade (0.06 seconds)
npn_test.go:49:  Get https://go-local-test: x509: certificate signed by
unknown authority
--- FAIL: TestTLSServer (0.05 seconds)
serve_test.go:695:  Get https://go-local-test: x509: certificate signed by
unknown authority

Maybe I generated the cert wrong?  I want it to be self-signed.  I added
that cert to the RootCAs list as used by the Transport.

Any clues?


On Mon, Feb 11, 2013 at 5:48 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http, net/http/httptest: don't use 127.0.0.1 for localhost TLS tests
>
> This introduces httptest.(*Server).Transport() to return an
> *http.Transport which special-cases the hostname "go-local-test"
> to mean localhost, and makes the httptest certificates use
> that hostname, instead of "127.0.0.1"
>
> Update Issue 4658
>
> Please review this at
https://codereview.appspot.**com/7307098/<https://codereview.appspot.com/7307...
>
> Affected files:
>   M src/pkg/crypto/tls/generate_**cert.go
>   M src/pkg/net/http/client_test.**go
>   M src/pkg/net/http/httptest/**server.go
>   M src/pkg/net/http/npn_test.go
>   M src/pkg/net/http/serve_test.go
>
>
>

[bradfitz, 2013-02-12 01:53:42]

Also, this is kind of an API change.

Anybody using httptest.NewTLSServer will likely have broken tests, and will
need to use the new httptest.(*Server).Transport.

I can't think of any way to remove IP address validation and not break the
API.  Unless: we could special-case the strings "127.0.0.1" and "[::1]" in
the crypto/x509 validation and keep allowing those.  Seems harmless(tm).

Thoughts?


On Mon, Feb 11, 2013 at 5:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Per the other thread, this doesn't work yet:
>
> A couple tests fail with:
>
> --- FAIL: TestClientWithCorrectTLSServerName (0.06 seconds)
> client_test.go:607:  expected successful TLS connection, got error: Get
> https://go-local-test: x509: certificate signed by unknown authority
> --- FAIL: TestNextProtoUpgrade (0.06 seconds)
> npn_test.go:49:  Get https://go-local-test: x509: certificate signed by
> unknown authority
> --- FAIL: TestTLSServer (0.05 seconds)
> serve_test.go:695:  Get https://go-local-test: x509: certificate signed
> by unknown authority
>
> Maybe I generated the cert wrong?  I want it to be self-signed.  I added
> that cert to the RootCAs list as used by the Transport.
>
> Any clues?
>
>
> On Mon, Feb 11, 2013 at 5:48 PM, <bradfitz@golang.org> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> net/http, net/http/httptest: don't use 127.0.0.1 for localhost TLS tests
>>
>> This introduces httptest.(*Server).Transport() to return an
>> *http.Transport which special-cases the hostname "go-local-test"
>> to mean localhost, and makes the httptest certificates use
>> that hostname, instead of "127.0.0.1"
>>
>> Update Issue 4658
>>
>> Please review this at
https://codereview.appspot.**com/7307098/<https://codereview.appspot.com/7307...
>>
>> Affected files:
>>   M src/pkg/crypto/tls/generate_**cert.go
>>   M src/pkg/net/http/client_test.**go
>>   M src/pkg/net/http/httptest/**server.go
>>   M src/pkg/net/http/npn_test.go
>>   M src/pkg/net/http/serve_test.go
>>
>>
>>
>

[shanemhansen, 2013-02-12 08:08:40]

https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_...
File src/pkg/crypto/tls/generate_cert.go (right):

https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:45: 
I think one issue is that this cert template config is out of date for
generating self signed certs. You want:

        KeyUsage:     x509.KeyUsageKeyEncipherment |
x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
        BasicConstraintsValid: true,
        IsCA: true,

After I update the test with cert/keys generated with those settings, I get:

--- FAIL: TestClientInsecureTransport (0.04 seconds)
client_test.go:578: 	insecure=false: got unexpected err=<nil>
--- FAIL: TestNextProtoUpgrade (0.04 seconds)
npn_test.go:79: 	certificate is valid for go-local-test, not 127.0.0.1
FAIL
FAIL	net/http	6.072s

Which seems like a step in the right direction. The remaining test failures seem
to be using 127.0.0.1 instead of go-local-test.

[shanemhansen, 2013-02-12 08:12:01]

On 2013/02/12 08:08:40, shanemhansen wrote:
>
https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_...
> File src/pkg/crypto/tls/generate_cert.go (right):
> 
>
https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_...
> src/pkg/crypto/tls/generate_cert.go:45: 
> I think one issue is that this cert template config is out of date for
> generating self signed certs. You want:
> 
>         KeyUsage:     x509.KeyUsageKeyEncipherment |
> x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
>         BasicConstraintsValid: true,
>         IsCA: true,
> 
> After I update the test with cert/keys generated with those settings, I get:
> 
> --- FAIL: TestClientInsecureTransport (0.04 seconds)
> client_test.go:578: 	insecure=false: got unexpected err=<nil>
> --- FAIL: TestNextProtoUpgrade (0.04 seconds)
> npn_test.go:79: 	certificate is valid for go-local-test, not 127.0.0.1
> FAIL
> FAIL	net/http	6.072s
> 
> Which seems like a step in the right direction. The remaining test failures
seem
> to be using 127.0.0.1 instead of go-local-test.
You can do a most basic check by extracting the pem file to cert.pem and
running:

openssl verify -CAfile foo.pem foo.pem

If that doesn't pass, you haven't correctly generated a self signed certificate.

[bradfitz, 2013-02-12 16:18:45]

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

Please take another look.

[bradfitz, 2013-02-12 16:19:15]

On Tue, Feb 12, 2013 at 12:08 AM, <shanemhansen@gmail.com> wrote:

>
> https://codereview.appspot.**com/7307098/diff/4001/src/pkg/**
>
crypto/tls/generate_cert.go<https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_cert.go>
> File src/pkg/crypto/tls/generate_**cert.go (right):
>
> https://codereview.appspot.**com/7307098/diff/4001/src/pkg/**
>
crypto/tls/generate_cert.go#**newcode45<https://codereview.appspot.com/7307098/diff/4001/src/pkg/crypto/tls/generate_cert.go#newcode45>
> src/pkg/crypto/tls/generate_**cert.go:45:
> I think one issue is that this cert template config is out of date for
> generating self signed certs. You want:
>
>         KeyUsage:     x509.KeyUsageKeyEncipherment |
> x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
>         BasicConstraintsValid: true,
>         IsCA: true,
>

Ah, great. Thanks!

PTAL

It all passes now.

[agl2, 2013-02-12 19:08:33]

On Tue, Feb 12, 2013 at 11:19 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Ah, great. Thanks!

Ah, hmm, I just read a different thread where I understood that this
was a royal pain. Implementing IP addresses correctly in crypto/x509
isn't that much of a bother. Is it the better solution here? (Note:
you would still have to update the cert, although you could still
connect to 127.0.0.1.)


Cheers

AGL

[shanemhansen, 2013-02-12 19:52:13]

On 2013/02/12 19:08:33, agl2 wrote:
> On Tue, Feb 12, 2013 at 11:19 AM, Brad Fitzpatrick
<mailto:bradfitz@golang.org> wrote:
> > Ah, great. Thanks!
> 
> Ah, hmm, I just read a different thread where I understood that this
> was a royal pain. Implementing IP addresses correctly in crypto/x509
> isn't that much of a bother. Is it the better solution here? (Note:
> you would still have to update the cert, although you could still
> connect to 127.0.0.1.)
> 
> 
> Cheers
> 
> AGL


Also tests still aren't passing. It looks like somehow you've introduced an
extra file descriptor?
exec_test.go:264: 	iteration 9, process B got:
		fd3: listener 127.0.0.1:61554
		fd11: listener

		want:
		fd3: listener 127.0.0.1:61554

[bradfitz, 2013-02-12 19:54:15]

Works for me.
 On Feb 12, 2013 11:52 AM, <shanemhansen@gmail.com> wrote:

> On 2013/02/12 19:08:33, agl2 wrote:
>
>> On Tue, Feb 12, 2013 at 11:19 AM, Brad Fitzpatrick
>>
> <mailto:bradfitz@golang.org> wrote:
>
>> > Ah, great. Thanks!
>>
>
>  Ah, hmm, I just read a different thread where I understood that this
>> was a royal pain. Implementing IP addresses correctly in crypto/x509
>> isn't that much of a bother. Is it the better solution here? (Note:
>> you would still have to update the cert, although you could still
>> connect to 127.0.0.1.)
>>
>
>
>  Cheers
>>
>
>  AGL
>>
>
>
> Also tests still aren't passing. It looks like somehow you've introduced
> an extra file descriptor?
> exec_test.go:264:       iteration 9, process B got:
>                 fd3: listener 127.0.0.1:61554
>                 fd11: listener
>
>                 want:
>                 fd3: listener 127.0.0.1:61554
>
>
>
https://codereview.appspot.**com/7307098/<https://codereview.appspot.com/7307...
>

[shanemhansen, 2013-02-12 20:57:59]

On 2013/02/12 19:54:15, bradfitz wrote:
> Works for me.
>  On Feb 12, 2013 11:52 AM, <mailto:shanemhansen@gmail.com> wrote:
> 
> > On 2013/02/12 19:08:33, agl2 wrote:
> >
> >> On Tue, Feb 12, 2013 at 11:19 AM, Brad Fitzpatrick
> >>
> > <mailto:bradfitz@golang.org> wrote:
> >
> >> > Ah, great. Thanks!
> >>
> >
> >  Ah, hmm, I just read a different thread where I understood that this
> >> was a royal pain. Implementing IP addresses correctly in crypto/x509
> >> isn't that much of a bother. Is it the better solution here? (Note:
> >> you would still have to update the cert, although you could still
> >> connect to 127.0.0.1.)
> >>
> >
> >
> >  Cheers
> >>
> >
> >  AGL
> >>
> >
> >
> > Also tests still aren't passing. It looks like somehow you've introduced
> > an extra file descriptor?
> > exec_test.go:264:       iteration 9, process B got:
> >                 fd3: listener 127.0.0.1:61554
> >                 fd11: listener
> >
> >                 want:
> >                 fd3: listener 127.0.0.1:61554
> >
> >
> >
>
https://codereview.appspot.**com/7307098/%3Chttps://codereview.appspot.com/73...>
> >

I may have screwed something up. Here's what I did to repro:
hg revert -a --no-backup
./all.bash (all tests paa)
hg clpatch 7307098
./all.bash (all tests do not pass)

go version devel +797127fc279b Wed Feb 13 02:27:42 2013 +0800 darwin/amd64

[bradfitz, 2013-02-13 01:05:22]

Hello golang-dev@googlegroups.com, shanemhansen@gmail.com, agl@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-13 01:06:43]

Sorry, misfire.  Bad copy/paste on CL number.  No new changes here.

On Tue, Feb 12, 2013 at 5:05 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, shanemhansen@gmail.com,
> agl@google.com (cc: golang-dev@googlegroups.com),
>
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/7307098/<https://codereview.appspot.com/7307...
>

[rsc, 2013-02-13 23:04:55]

You're going to abandon this CL, right?

[bradfitz, 2013-02-13 23:23:14]

Sure, if the IP SAN stuff is actually happening.  I did this in prep to
make your guys' other CLs workable, but I guess it's no longer needed.

On Wed, Feb 13, 2013 at 2:35 PM, Russ Cox <rsc@golang.org> wrote:

> You're going to abandon this CL, right?
>
>  --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[rsc, 2013-02-14 02:44:54]

Sounds like agl is going to fix IP SAN for us, so let's leave it at that.