New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: TestSystemRoots failure #21416

Open
martisch opened this Issue Aug 12, 2017 · 22 comments

Comments

Projects
None yet
@martisch
Member

martisch commented Aug 12, 2017

go tip 23cd87e and earlier in 1.9 candidates

while running ./all.bash i have a reproducible test error on darwin:
--- FAIL: TestSystemRoots (1.41s)
root_darwin_test.go:31: cgo sys roots: 443.728642ms
root_darwin_test.go:32: non-cgo sys roots: 964.424609ms
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

macOS Sierra 10.12.6 but i have seen that failure on macOS 10.11 too since i just upgraded.

Darwin ender 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64

another similar report here:
https://groups.google.com/forum/#!topic/golang-nuts/LZvj2N_8gs4

@martisch

This comment has been minimized.

Member

martisch commented Aug 12, 2017

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

@martisch

This comment has been minimized.

Member

martisch commented Aug 19, 2017

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

@martisch

This comment has been minimized.

Member

martisch commented Aug 19, 2017

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.

@mastercactapus

This comment has been minimized.

Contributor

mastercactapus commented Aug 19, 2017

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

@gopherbot

This comment has been minimized.

gopherbot commented Aug 22, 2017

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

gopherbot pushed a commit that referenced this issue Aug 22, 2017

crypto/x509: skip TestSystemRoots
golang.org/cl/36941 enabled loading of all trusted certs on darwin
for the non-cgo execSecurityRoots.

The corresponding cgo version golang.org/cl/36942 for systemRootsPool
has not been merged yet.

This tests fails reliably on some darwin systems:
--- FAIL: TestSystemRoots (1.28s)
        root_darwin_test.go:31:     cgo sys roots: 353.552363ms
        root_darwin_test.go:32: non-cgo sys roots: 921.85297ms
        root_darwin_test.go:44: got 169 roots
        root_darwin_test.go:44: got 455 roots
        root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 227, have 168
FAIL
FAIL    crypto/x509     2.445s

Updates #16532
Updates #21416

Change-Id: I52c2c847651fb3621fdb6ab858ebe8e28894c201
Reviewed-on: https://go-review.googlesource.com/57830
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
@adamryman

This comment has been minimized.

adamryman commented Aug 24, 2017

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

@martisch

This comment has been minimized.

Member

martisch commented Aug 25, 2017

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.

@dsnet dsnet removed the release-blocker label Aug 25, 2017

@aclements aclements modified the milestones: Go1.9, Go1.9.1 Aug 25, 2017

@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 13, 2017

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.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2017

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

CL 57830 OK for Go 1.9.2.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2017

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.

@rsc rsc modified the milestones: Go1.9.2, Go1.10 Oct 13, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Oct 15, 2017

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

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.9] crypto/x509: skip TestSystemRoots
golang.org/cl/36941 enabled loading of all trusted certs on darwin
for the non-cgo execSecurityRoots.

The corresponding cgo version golang.org/cl/36942 for systemRootsPool
has not been merged yet.

This tests fails reliably on some darwin systems:
--- FAIL: TestSystemRoots (1.28s)
        root_darwin_test.go:31:     cgo sys roots: 353.552363ms
        root_darwin_test.go:32: non-cgo sys roots: 921.85297ms
        root_darwin_test.go:44: got 169 roots
        root_darwin_test.go:44: got 455 roots
        root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 227, have 168
FAIL
FAIL    crypto/x509     2.445s

Updates #16532
Updates #21416

Change-Id: I52c2c847651fb3621fdb6ab858ebe8e28894c201
Reviewed-on: https://go-review.googlesource.com/57830
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-on: https://go-review.googlesource.com/70847
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Martin Möhrmann <moehrmann@google.com>

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@bradfitz bradfitz self-assigned this Jun 7, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Jun 7, 2018

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

@gopherbot gopherbot closed this in 2642df9 Jun 14, 2018

dna2github added a commit to dna2fork/go that referenced this issue Jun 14, 2018

crypto/x509: re-enable TestSystemRoots on darwin
It was apparently waiting on CL 36942, which was submitted.

Fixes golang#21416

Change-Id: I8f4ccc5a3176070abf0df019c82700c5761b5f53
Reviewed-on: https://go-review.googlesource.com/117055
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@martisch

This comment has been minimized.

Member

martisch commented Jul 6, 2018

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)?

@martisch martisch reopened this Jul 6, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Jul 6, 2018

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

@adamdecaf

This comment has been minimized.

Contributor

adamdecaf commented Jul 6, 2018

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

#24652 (comment)

@martisch

This comment has been minimized.

Member

martisch commented Jul 13, 2018

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?

@adamdecaf

This comment has been minimized.

Contributor

adamdecaf commented Jul 13, 2018

@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.)

@martisch

This comment has been minimized.

Member

martisch commented Jul 14, 2018

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.

@adamdecaf

This comment has been minimized.

Contributor

adamdecaf commented Jul 17, 2018

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?

@martisch

This comment has been minimized.

Member

martisch commented Jul 19, 2018

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)

@adamdecaf

This comment has been minimized.

Contributor

adamdecaf commented Jul 19, 2018

@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?

@gopherbot

This comment has been minimized.

gopherbot commented Jul 20, 2018

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

@FiloSottile FiloSottile assigned FiloSottile and unassigned bradfitz Jul 27, 2018

gopherbot pushed a commit that referenced this issue Jul 27, 2018

crypto/x509: skip TestSystemRoots
cgo and non-cgo code paths can disagree
on the number of root certificates:
=== RUN   TestSystemRoots
--- FAIL: TestSystemRoots (0.31s)
    root_darwin_test.go:31:     cgo sys roots: 93.605184ms
    root_darwin_test.go:32: non-cgo sys roots: 213.998586ms
    root_darwin_test.go:44: got 168 roots
    root_darwin_test.go:44: got 427 roots
    root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 213, have 168
FAIL
exit status 1

Updates #21416
Updates #24652

Change-Id: Idb6d35b17c142dfff79a10cf6b40a42d12f9d17e
Reviewed-on: https://go-review.googlesource.com/125259
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment