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 <gobot@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>

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()
 	}
 }