]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/x509: re-enable TestSystemRoots
authorFilippo Valsorda <hi@filippo.io>
Mon, 6 Aug 2018 22:38:38 +0000 (18:38 -0400)
committerFilippo Valsorda <filippo@golang.org>
Wed, 5 Dec 2018 22:54:01 +0000 (22:54 +0000)
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 <gobot@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
src/crypto/x509/root_darwin_test.go

index 68300c7955714e1e768cd734cc4e9d8ba02ff7df..27806538121f6035b241f634d2864f8706a7e16c 100644 (file)
@@ -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
+               }
+
+               // 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
+               }
 
-       var want int
-       if nsys, nexec := len(sysRoots.certs), len(execRoots.certs); nsys > nexec {
-               want = nsys / 2
-       } else {
-               want = nexec / 2
+               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()
        }
 }