From 9536c5fa69d619395015fcd526a979e17320b4b1 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 6 Aug 2018 18:38:38 -0400 Subject: [PATCH] crypto/x509: re-enable TestSystemRoots Now that the cgo and no-cgo paths should be correct and equivalent, re-enable the TestSystemRoots test without any margin of error (which was tripping anyway when users had too many of a certain edge-case). As a last quirk, the verify-cert invocation will validate certificates that aren't roots, but are signed by valid roots. Ignore them. Fixes #24652 Change-Id: I6a8ff3c2282136d7122a4e7e387eb8014da0d28a Reviewed-on: https://go-review.googlesource.com/c/128117 TryBot-Result: Gobot Gobot Run-TryBot: Filippo Valsorda Reviewed-by: Adam Langley --- src/crypto/x509/root_darwin_test.go | 108 +++++++++++++++++++--------- 1 file changed, 74 insertions(+), 34 deletions(-) diff --git a/src/crypto/x509/root_darwin_test.go b/src/crypto/x509/root_darwin_test.go index 68300c79557..27806538121 100644 --- a/src/crypto/x509/root_darwin_test.go +++ b/src/crypto/x509/root_darwin_test.go @@ -5,6 +5,9 @@ package x509 import ( + "os" + "os/exec" + "path/filepath" "runtime" "testing" "time" @@ -16,11 +19,6 @@ func TestSystemRoots(t *testing.T) { t.Skipf("skipping on %s/%s, no system root", runtime.GOOS, runtime.GOARCH) } - switch runtime.GOOS { - case "darwin": - t.Skipf("skipping on %s/%s until golang.org/issue/24652 has been resolved.", runtime.GOOS, runtime.GOARCH) - } - t0 := time.Now() sysRoots := systemRootsPool() // actual system roots sysRootsDuration := time.Since(t0) @@ -36,45 +34,87 @@ func TestSystemRoots(t *testing.T) { t.Logf(" cgo sys roots: %v", sysRootsDuration) t.Logf("non-cgo sys roots: %v", execSysRootsDuration) - for _, tt := range []*CertPool{sysRoots, execRoots} { - if tt == nil { - t.Fatal("no system roots") - } - // On Mavericks, there are 212 bundled certs, at least - // there was at one point in time on one machine. - // (Maybe it was a corp laptop with extra certs?) - // Other OS X users report - // 135, 142, 145... Let's try requiring at least 100, - // since this is just a sanity check. - t.Logf("got %d roots", len(tt.certs)) - if want, have := 100, len(tt.certs); have < want { - t.Fatalf("want at least %d system roots, have %d", want, have) - } + // On Mavericks, there are 212 bundled certs, at least there was at + // one point in time on one machine. (Maybe it was a corp laptop + // with extra certs?) Other OS X users report 135, 142, 145... + // Let's try requiring at least 100, since this is just a sanity + // check. + if want, have := 100, len(sysRoots.certs); have < want { + t.Errorf("want at least %d system roots, have %d", want, have) } - // Check that the two cert pools are roughly the same; - // |A∩B| > max(|A|, |B|) / 2 should be a reasonably robust check. + // Fetch any intermediate certificate that verify-cert might be aware of. + out, err := exec.Command("/usr/bin/security", "find-certificate", "-a", "-p", + "/Library/Keychains/System.keychain", + filepath.Join(os.Getenv("HOME"), "/Library/Keychains/login.keychain"), + filepath.Join(os.Getenv("HOME"), "/Library/Keychains/login.keychain-db")).Output() + if err != nil { + t.Fatal(err) + } + allCerts := NewCertPool() + allCerts.AppendCertsFromPEM(out) - isect := make(map[string]bool, len(sysRoots.certs)) + // Check that the two cert pools are the same. + sysPool := make(map[string]*Certificate, len(sysRoots.certs)) for _, c := range sysRoots.certs { - isect[string(c.Raw)] = true + sysPool[string(c.Raw)] = c } - - have := 0 for _, c := range execRoots.certs { - if isect[string(c.Raw)] { - have++ + if _, ok := sysPool[string(c.Raw)]; ok { + delete(sysPool, string(c.Raw)) + } else { + // verify-cert lets in certificates that are not trusted roots, but are + // signed by trusted roots. This should not be a problem, so confirm that's + // the case and skip them. + if _, err := c.Verify(VerifyOptions{ + Roots: sysRoots, + Intermediates: allCerts, + KeyUsages: []ExtKeyUsage{ExtKeyUsageAny}, + }); err != nil { + t.Errorf("certificate only present in non-cgo pool: %v (verify error: %v)", c.Subject, err) + } else { + t.Logf("signed certificate only present in non-cgo pool (acceptable): %v", c.Subject) + } } } + for _, c := range sysPool { + // The nocgo codepath uses verify-cert with the ssl policy, which also + // happens to check EKUs, so some certificates will appear only in the + // cgo pool. We can't easily make them consistent because the EKU check + // is only applied to the certificates passed to verify-cert. + var ekuOk bool + for _, eku := range c.ExtKeyUsage { + if eku == ExtKeyUsageServerAuth || eku == ExtKeyUsageNetscapeServerGatedCrypto || + eku == ExtKeyUsageMicrosoftServerGatedCrypto || eku == ExtKeyUsageAny { + ekuOk = true + } + } + if len(c.ExtKeyUsage) == 0 && len(c.UnknownExtKeyUsage) == 0 { + ekuOk = true + } + if !ekuOk { + t.Logf("off-EKU certificate only present in cgo pool (acceptable): %v", c.Subject) + continue + } - var want int - if nsys, nexec := len(sysRoots.certs), len(execRoots.certs); nsys > nexec { - want = nsys / 2 - } else { - want = nexec / 2 + // Same for expired certificates. We don't chain to them anyway. + now := time.Now() + if now.Before(c.NotBefore) || now.After(c.NotAfter) { + t.Logf("expired certificate only present in cgo pool (acceptable): %v", c.Subject) + continue + } + + t.Errorf("certificate only present in cgo pool: %v", c.Subject) } - if have < want { - t.Errorf("insufficient overlap between cgo and non-cgo roots; want at least %d, have %d", want, have) + if t.Failed() && debugDarwinRoots { + cmd := exec.Command("security", "dump-trust-settings") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Run() + cmd = exec.Command("security", "dump-trust-settings", "-d") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Run() } }