crypto/x509: load roots from colon separated SSL_CERT_DIR in loadSyst…

…emRoots

"SSL_CERT_DIR" is meant to hold more than one directory, when a colon
is used as a delimiter. However, we assumed it'd be a single directory
for all root certificates.
OpenSSL and BoringSSL properly respected the colon separated
"SSL_CERT_DIR", as per:
* OpenSSL https://github.com/openssl/openssl/blob/12a765a5235f181c2f4992b615eb5f892c368e88/crypto/x509/by_dir.c#L153-L209
* BoringSSL https://github.com/google/boringssl/blob/3ba9586bc081f67903c89917f23e74a0662ba953/crypto/x509/by_dir.c#L194-L247

This change adds that parity to loadSystemRoots.

RELNOTE=yes

Fixes #35325

Change-Id: I0d554a00ccc34300a7f0529aa741ee7e2d5762f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/205237
Run-TryBot: Emmanuel Odeke <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

src/crypto/x509/root_unix.go

@@ -9,6 +9,7 @@ package x509
 import (
  "io/ioutil"
  "os"
+ "strings"
 )
 
 // Possible directories with certificate files; stop after successfully
@@ -29,6 +30,8 @@ const (
 
  // certDirEnv is the environment variable which identifies which directory
  // to check for SSL certificate files. If set this overrides the system default.
+ // It is a colon separated list of directories.
+ // See https://www.openssl.org/docs/man1.0.2/man1/c_rehash.html.
  certDirEnv = "SSL_CERT_DIR"
 )
 
@@ -58,7 +61,11 @@ func loadSystemRoots() (*CertPool, error) {
 
  dirs := certDirectories
  if d := os.Getenv(certDirEnv); d != "" {
- dirs = []string{d}
+ // OpenSSL and BoringSSL both use ":" as the SSL_CERT_DIR separator.
+ // See:
+ // * https://golang.org/issue/35325
+ // * https://www.openssl.org/docs/man1.0.2/man1/c_rehash.html
+ dirs = strings.Split(d, ":")
  }
 
  for _, directory := range dirs {
@@ -69,16 +76,12 @@ func loadSystemRoots() (*CertPool, error) {
  }
  continue
  }
- rootsAdded := false
  for _, fi := range fis {
  data, err := ioutil.ReadFile(directory + "/" + fi.Name())
- if err == nil && roots.AppendCertsFromPEM(data) {
- rootsAdded = true
+ if err == nil {
+ roots.AppendCertsFromPEM(data)
  }
  }
- if rootsAdded {
- return roots, nil
- }
  }
 
  if len(roots.certs) > 0 || firstErr == nil {

src/crypto/x509/root_unix_test.go

@@ -7,8 +7,13 @@
 package x509
 
 import (
+ "bytes"
  "fmt"
+ "io/ioutil"
  "os"
+ "path/filepath"
+ "reflect"
+ "strings"
  "testing"
 )
 
@@ -121,3 +126,79 @@ func TestEnvVars(t *testing.T) {
  })
  }
 }
+
+// Ensure that "SSL_CERT_DIR" when used as the environment
+// variable delimited by colons, allows loadSystemRoots to
+// load all the roots from the respective directories.
+// See https://golang.org/issue/35325.
+func TestLoadSystemCertsLoadColonSeparatedDirs(t *testing.T) {
+ origFile, origDir := os.Getenv(certFileEnv), os.Getenv(certDirEnv)
+ origCertFiles := certFiles[:]
+
+ // To prevent any other certs from being loaded in
+ // through "SSL_CERT_FILE" or from known "certFiles",
+ // clear them all, and they'll be reverting on defer.
+ certFiles = certFiles[:0]
+ os.Setenv(certFileEnv, "")
+
+ defer func() {
+ certFiles = origCertFiles[:]
+ os.Setenv(certDirEnv, origDir)
+ os.Setenv(certFileEnv, origFile)
+ }()
+
+ tmpDir, err := ioutil.TempDir(os.TempDir(), "x509-issue35325")
+ if err != nil {
+ t.Fatalf("Failed to create temporary directory: %v", err)
+ }
+ defer os.RemoveAll(tmpDir)
+
+ rootPEMs := []string{
+ geoTrustRoot,
+ googleLeaf,
+ startComRoot,
+ }
+
+ var certDirs []string
+ for i, certPEM := range rootPEMs {
+ certDir := filepath.Join(tmpDir, fmt.Sprintf("cert-%d", i))
+ if err := os.MkdirAll(certDir, 0755); err != nil {
+ t.Fatalf("Failed to create certificate dir: %v", err)
+ }
+ certOutFile := filepath.Join(certDir, "cert.crt")
+ if err := ioutil.WriteFile(certOutFile, []byte(certPEM), 0655); err != nil {
+ t.Fatalf("Failed to write certificate to file: %v", err)
+ }
+ certDirs = append(certDirs, certDir)
+ }
+
+ // Sanity check: the number of certDirs should be equal to the number of roots.
+ if g, w := len(certDirs), len(rootPEMs); g != w {
+ t.Fatalf("Failed sanity check: len(certsDir)=%d is not equal to len(rootsPEMS)=%d", g, w)
+ }
+
+ // Now finally concatenate them with a colon.
+ colonConcatCertDirs := strings.Join(certDirs, ":")
+ os.Setenv(certDirEnv, colonConcatCertDirs)
+ gotPool, err := loadSystemRoots()
+ if err != nil {
+ t.Fatalf("Failed to load system roots: %v", err)
+ }
+ subjects := gotPool.Subjects()
+ // We expect exactly len(rootPEMs) subjects back.
+ if g, w := len(subjects), len(rootPEMs); g != w {
+ t.Fatalf("Invalid number of subjects: got %d want %d", g, w)
+ }
+
+ wantPool := NewCertPool()
+ for _, certPEM := range rootPEMs {
+ wantPool.AppendCertsFromPEM([]byte(certPEM))
+ }
+ strCertPool := func(p *CertPool) string {
+ return string(bytes.Join(p.Subjects(), []byte("\n")))
+ }
+ if !reflect.DeepEqual(gotPool, wantPool) {
+ g, w := strCertPool(gotPool), strCertPool(wantPool)
+ t.Fatalf("Mismatched certPools\nGot:\n%s\n\nWant:\n%s", g, w)
+ }
+}