code review 5262041: crypto/tls: fetch root certificates using Mac OS API

crypto/tls: fetch root certificates using Mac OS API

Fixes issue 1009.

[mkrautz, 2011-10-13 01:06:22]

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

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

[mkrautz, 2011-10-13 01:16:11]

I haven't been able to test this on Snow Leopard yet. Neither a Go binary built
on Lion on Snow Leopard, nor a pure-Snow Leopard build.  If anyone could check
that nothing breaks in those two configurations, that would be much appreciated.

Also, note that this change set changes the default certificate verification
logic in handshake_client.go.  The old code would only verify the certificate
chain when c.config.RootCAs != nil.  However, the default emptyConfig for TLS
clients would always have RootCAs set to nil, so no verification was performed
unless a suitable custom tls.Config was provided.

The new logic makes sure that system-provided CAs are considered as well, when
using the default TLS config.  Another thing to note is that certificate chain
verification is completely skipped if rootCAs() == nil. (That is, if both the
RootCAs field of the tls.Config struct is nil, *and* the list of system default
CAs is nil).  This is the case for OSes that use the new root_stub.go file for
their default root implementation.

[adg, 2011-10-13 03:25:35]

On my Snow Leopard machine:

$ gomake test
gotest
rm -f _test/crypto/tls.a
CGOPKGPATH=crypto cgo --  root_darwin.go
touch _obj/_cgo_run
6g  -p crypto/tls -o _gotest_.6 alert.go cipher_suites.go common.go
conn.go handshake_client.go handshake_messages.go handshake_server.go
key_agreement.go prf.go tls.go   _obj/root_darwin.cgo1.go
_obj/_cgo_gotypes.go conn_test.go handshake_client_test.go
handshake_messages_test.go handshake_server_test.go prf_test.go
root_test.go
6c -FVw -I/Users/adg/go/pkg/darwin_amd64 -I . -o "_cgo_defun.6"
_obj/_cgo_defun.c
gcc -m64 -I . -g -fPIC -O2 -o _cgo_main.o -c  -Wno-error _obj/_cgo_main.c
gcc -m64 -I . -g -fPIC -O2 -o root_darwin.cgo2.o -c  -Wno-error
_obj/root_darwin.cgo2.c
gcc -m64 -I . -g -fPIC -O2 -o _cgo_export.o -c  -Wno-error _obj/_cgo_export.c
gcc -m64 -g -fPIC -O2 -o _cgo1_.o _cgo_main.o root_darwin.cgo2.o
_cgo_export.o  -framework CoreFoundation -framework Security
cgo -dynimport _cgo1_.o >_obj/_cgo_import.c_ && mv -f
_obj/_cgo_import.c_ _obj/_cgo_import.c
6c -FVw -I . -o "_cgo_import.6" _obj/_cgo_import.c
rm -f _test/crypto/tls.a
gopack grc _test/crypto/tls.a _gotest_.6  _cgo_defun.6 _cgo_import.6
root_darwin.cgo2.o _cgo_export.o
--- FAIL: tls.TestHandshakeClientRC4 (0.24 seconds)
	RC4 #2: unexpected EOF
FAIL
gotest: "./6.out" failed: exit status 1

[mkrautz, 2011-10-13 13:38:29]

On 2011/10/13 03:25:35, adg wrote:
> On my Snow Leopard machine:
> 
> $ gomake test
> gotest
> rm -f _test/crypto/tls.a
> CGOPKGPATH=crypto cgo --  root_darwin.go
> touch _obj/_cgo_run
> 6g  -p crypto/tls -o _gotest_.6 alert.go cipher_suites.go common.go
> conn.go handshake_client.go handshake_messages.go handshake_server.go
> key_agreement.go prf.go tls.go   _obj/root_darwin.cgo1.go
> _obj/_cgo_gotypes.go conn_test.go handshake_client_test.go
> handshake_messages_test.go handshake_server_test.go prf_test.go
> root_test.go
> 6c -FVw -I/Users/adg/go/pkg/darwin_amd64 -I . -o "_cgo_defun.6"
> _obj/_cgo_defun.c
> gcc -m64 -I . -g -fPIC -O2 -o _cgo_main.o -c  -Wno-error _obj/_cgo_main.c
> gcc -m64 -I . -g -fPIC -O2 -o root_darwin.cgo2.o -c  -Wno-error
> _obj/root_darwin.cgo2.c
> gcc -m64 -I . -g -fPIC -O2 -o _cgo_export.o -c  -Wno-error _obj/_cgo_export.c
> gcc -m64 -g -fPIC -O2 -o _cgo1_.o _cgo_main.o root_darwin.cgo2.o
> _cgo_export.o  -framework CoreFoundation -framework Security
> cgo -dynimport _cgo1_.o >_obj/_cgo_import.c_ && mv -f
> _obj/_cgo_import.c_ _obj/_cgo_import.c
> 6c -FVw -I . -o "_cgo_import.6" _obj/_cgo_import.c
> rm -f _test/crypto/tls.a
> gopack grc _test/crypto/tls.a _gotest_.6  _cgo_defun.6 _cgo_import.6
> root_darwin.cgo2.o _cgo_export.o
> --- FAIL: tls.TestHandshakeClientRC4 (0.24 seconds)
> 	RC4 #2: unexpected EOF
> FAIL
> gotest: "./6.out" failed: exit status 1

Thanks Andrew.  Seems to work (except for the test failures because of the CA
verify logic). 
If possible, we should also check whether a Lion-built Go executable using this
code still runs on Snow Leopard.
Something like this should suffice: http://goo.gl/pFPRn ... here's a binary:
http://db.tt/Hqu0Zo1z if anyone dares :-)

The test failure was because of the new root CA logic.  With the new code, the
client would reject the certificate chain presented in handshake_client_test.go.
To remedy this I've introduced a new tls.Config field called SkipVerification.
It'll skip verification of the server's certificate chain.

Likewise, there was a test failure in package http similar to this.  For now,
I've enabled SkipVerification for both of the tests.

I tried to check whether it was feasible to instead introduce a
'AllowSelfSigned' field instead, but I don't think crypto/x509 currently is able
to verify self-signed certificates (where self-signed means a chain that only
includes a single certificate that's signed by itself). That's for another CL.

[rsc, 2011-10-13 13:45:17]

Thank you!  This is great.
I will be able to test on Snow Leopard and on Lion later today.

Russ

[rsc, 2011-10-13 15:14:35]

Thanks very much for doing this.

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:157: // SkipVerification controls whether a client
should attempt to verify
// InsecureSkipVerify controls whether a client verifies the
// server's certificate chain and host name.
// If InsecureSkipVerify is true, TLS accepts any certificate
// presented by the server and any host name in that certificate.
// In this mode, TLS is susceptible to man-in-the-middle attacks.
// This should be used only for testing.
InsecureSkipVerify bool

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:100: // If we don't have a root CA set
configured and the system doesn't provide its
This was a hack for OS X users.  It's not the right default behavior.

if !c.config.InsecureSkipVerify {
    opts := x509.VerifyOptions{
        Roots: c.config.rootCAs(),
        ...

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/root_test.go
File src/pkg/crypto/tls/root_test.go (right):

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/root_test...
src/pkg/crypto/tls/root_test.go:15: // x509.CertPool is not easily testable in
isolation.
How about

roots := defaultRoots()
if testing.Short() {
    t.Logf("skipping certificate tests in short mode")
    return
}

... otherwise, connect to list of https servers and make
... sure the certs verify.

(global)
var tlsServers = []string{
    "www.google.com",
    "github.com",
    "twitter.com",
}

[mkrautz, 2011-10-13 15:37:50]

Thanks, I'll fix these.

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/5262041/diff/18001/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:100: // If we don't have a root CA set
configured and the system doesn't provide its
On 2011/10/13 15:14:35, rsc wrote:
> This was a hack for OS X users.  It's not the right default behavior.
> 
> if !c.config.InsecureSkipVerify {
>     opts := x509.VerifyOptions{
>         Roots: c.config.rootCAs(),
>         ...
> 
>     

I kept it around mainly to allow systems that use root_stub.go to still function
with the current behavior (basically SkipVerification == true).

I plan to submit a CL implementing access to Windows' cert stores as well, so
perhaps it's not too much of an issue.

[rsc, 2011-10-13 15:50:17]

> I kept it around mainly to allow systems that use root_stub.go to still
> function with the current behavior (basically SkipVerification == true).
> 
> I plan to submit a CL implementing access to Windows' cert stores as
> well, so perhaps it's not too much of an issue.

Now that we have the ability to do the right thing,
we should do the right thing.  I put that code in
when package net could not use cgo at all.
Keeping it just encourages us not to fix it.

Russ

[mkrautz, 2011-10-13 16:55:35]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-10-13 17:17:59]

LGTM

http://codereview.appspot.com/5262041/diff/28001/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/5262041/diff/28001/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:100: if c.config.InsecureSkipVerify ==
false {
if !c.config.InsecureSkipVerify {

[mkrautz, 2011-10-13 17:22:14]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-10-13 17:27:00]

LGTM

Thanks again.

[rsc, 2011-10-13 17:59:19]

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

crypto/tls: fetch root certificates using Mac OS API

Fixes issue 1009.

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/5262041

Committer: Russ Cox <rsc@golang.org>