happens since: 4dbcacd... crypto/x509: load all trusted certs on darwin (nocgo)

4dbcacda96... crypto/x509: load all trusted certs on darwin (nocgo)
ender:x509 martisch$ go test -run=TestSystemRoots
--- FAIL: TestSystemRoots (0.74s)
	root_darwin_test.go:31:     cgo sys roots: 124.339674ms
	root_darwin_test.go:32: non-cgo sys roots: 614.074109ms
	root_darwin_test.go:44: got 169 roots
	root_darwin_test.go:44: got 453 roots
	root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 226, have 168
FAIL
exit status 1
FAIL	crypto/x509	0.752s

/cc @dsnet @ianlancetaylor

adding authors and more reviewers from the CL
/cc @mastercactapus @josharian @agl

I tagged this Go1.9 since i traced it to a CL merged at end of 1.9 cycle and it would be nice to have a working test suite on darwin with 1.9 release and also counter confusion and issue tickets from any others that will encounter the above failure when compiling 1.9 from source on darwin.

I suspect this is due to the cgo counterpart to that fix not having made it into 1.9 (as it's still in-progress)
https://go-review.googlesource.com/c/36942

Change https://golang.org/cl/57830 mentions this issue: crypto/x509: skip TestSystemRoots

If this is a release blocker why did 1.9 get released without this?

Because the label was a suggestion by me but the as far as i know the decision was made by the go team that the issue should not hold up the release of 1.9 (which sounds like the right call to me). It can be considered for 1.9.x.

If I'm reading this correctly, the fix for this test failure is to disable the test.

I guess that is OK for a backport to 1.9.

Sure, we can disable the test that non-cgo works here.

CL 57830 OK for Go 1.9.2.

I'm going to remilestone this to Go 1.10 for a proper fix, since there is good context in this thread, and I created #22256 for the Go 1.9.2 cherry-pick.

Change https://golang.org/cl/70847 mentions this issue: [release-branch.go1.9] crypto/x509: skip TestSystemRoots

Change https://golang.org/cl/117055 mentions this issue: crypto/x509: re-enable TestSystemRoots on darwin

reopening because after submission of https://golang.org/cl/117055
./all.bash on tip is broken again on my private macbook pro (Darwin ender 16.7.0 Darwin Kernel Version 16.7.0: Fri Apr 27 17:59:46 PDT 2018; root:xnu-3789.73.13~1/RELEASE_X86_64 x86_64
):

$ git checkout 2642df9
$ ./all.bash
...
--- FAIL: TestSystemRoots (4.97s)
root_darwin_test.go:31: cgo sys roots: 420.836945ms
root_darwin_test.go:32: non-cgo sys roots: 4.545627256s
root_darwin_test.go:44: got 177 roots
root_darwin_test.go:44: got 467 roots
root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 233, have 172
FAIL
FAIL crypto/x509 6.895s

@bradfitz
can we rollback cl/117055 until the underlying issue is fixed (happy to provide more information until root cause of problem is found)?

Change https://golang.org/cl/122435 mentions this issue: Revert "crypto/x509: re-enable TestSystemRoots on darwin"

@martisch Could you try with these CL's and see if the test is fixed?

#24652 (comment)

applied 3 patches from #24652 to tip:
ec09ea9fa4 (HEAD -> x509test) crypto/x509: on darwin verify-cert with ssl policy
54a3bdbe1e crypto/x509: collect keychain certs without explicit usage constraints
a6f6cfa570 crypto/x509: check darwin keychain policy in cgo path
8a33045 (origin/master, origin/HEAD, master) net/url: don't escape sub-delims in fragment

result:
--- FAIL: TestSystemRoots (1.58s)
root_darwin_test.go:31: cgo sys roots: 126.320357ms
root_darwin_test.go:32: non-cgo sys roots: 1.44816439s
root_darwin_test.go:44: got 177 roots
root_darwin_test.go:44: got 467 roots
root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 233, have 171

any other CLs i should apply?

@martisch 400+ roots seems like a lot compared to my system. I only have two custom CA's installed.

I tried from 8a33045 (in your comment) and ebdba42 with both passing:

// https://github.com/golang/go/commit/ebdba42d9e1de46ebf611baec98d53f01c534cac
$ ../bin/go test -v -count 1 -run TestSystemRoots crypto/x509 
=== RUN   TestSystemRoots
--- PASS: TestSystemRoots (3.03s)
    root_darwin_test.go:31:     cgo sys roots: 1.467407679s
    root_darwin_test.go:32: non-cgo sys roots: 1.5584415s
    root_darwin_test.go:44: got 169 roots
    root_darwin_test.go:44: got 173 roots
PASS
ok  	crypto/x509	3.043s

Could you explain more about the certificates picked up in the non-cgo route? Adding GODEBUG=x509roots=1 will list them out as they're picked up. (Please from from tip as there's better debugging.)

I have a lot of email certificates in my login key chain from smime signed emails from a common authority (e.g. DFN-PKI) . AFAIK apple mail adds them automatically to my keychain when i receive signed mails and they have been verified against a trusted authority. I just deleted one of those and the number of "got roots" decreased by one. The trust on those certificates is system default and they are not system root certificates.

GODEBUG=x509roots=1 go test
crypto/x509: 12 certs have a trust policy
Prints some custom roots for email signing within orgs, expired roots (accumulated over 10 years), system management software root certs. There are only ~20 in total listed with some expired. Some of them were double likely because i have them in two different key chains.

It's been mentioned crypto/x509 is just for the web PKI, so we could probably exclude these certs from the SystemCertPool() result.

Do these certificates only have SubjectAltNames's for emails or Extended KeyUsage for email?

The certificates have:
X509v3 Key Usage: Digital Signature, Non Repudiation, Key Encipherment
X509v3 Extended Key Usage: TLS Web Client Authentication, E-mail Protection, Microsoft Smartcardlogin

and:
X509v3 Subject Alternative Name:
email: X, email:Y .....

(Where X and Y are placeholders here for actual email addresses)

@FiloSottile Should we ignore picking up certs that only have email SANs?

Also, I don't really see crypto/x509 documented as "web pki only", but should we?

Change https://golang.org/cl/125259 mentions this issue: crypto/x509: skip TestSystemRoots

This is failing again on the 10.11 builders (but not the other darwin builders) as of https://golang.org/cl/128117.

--- FAIL: TestSystemRoots (0.30s)
    root_darwin_test.go:34:     cgo sys roots: 66.259292ms
    root_darwin_test.go:35: non-cgo sys roots: 201.101857ms
    root_darwin_test.go:76: signed certificate only present in non-cgo pool (acceptable): CN=Developer ID Certification Authority,OU=Apple Certification Authority,O=Apple Inc.,C=US
    root_darwin_test.go:103: expired certificate only present in cgo pool (acceptable): OU=Equifax Secure Certificate Authority,O=Equifax,C=US
    root_darwin_test.go:107: certificate only present in cgo pool: CN=Thawte Premium Server CA,OU=Certification Services Division,O=Thawte Consulting cc,L=Cape Town,ST=Western Cape,C=ZA
    root_darwin_test.go:103: expired certificate only present in cgo pool (acceptable): CN=GTE CyberTrust Global Root,OU=GTE CyberTrust Solutions\, Inc.,O=GTE Corporation,C=US
    root_darwin_test.go:107: certificate only present in cgo pool: CN=Thawte Server CA,OU=Certification Services Division,O=Thawte Consulting cc,L=Cape Town,ST=Western Cape,C=ZA
    root_darwin_test.go:107: certificate only present in cgo pool: OU=Class 3 Public Primary Certification Authority,O=VeriSign\, Inc.,C=US

I'm working on it. I have so far not figured out where those three certificates come from.

Just a note that this is still failing the darwin 10.11 builds.

There is also a similar failure for darwin-amd64-race. Example: https://build.golang.org/log/a93b418cfdfb8f67374c79b7bb2fb2a804c53fc6

Change https://golang.org/cl/156329 mentions this issue: crypto/x509: ignore 5 phantom 1024-bit roots in TestSystemRoots