crypto/x509: load roots from colon separated SSL_CERT_DIR in loadSystemRoots

"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 <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/crypto/x509/root_unix.go b/src/crypto/x509/root_unix.go
index f1e174c894086e07c29b387ea273a8526289c955..1be4058bab7234b5a50bfbf3e38de0c745b1d25f 100644 (file)
--- a/src/crypto/x509/root_unix.go
+++ b/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 {

diff --git a/src/crypto/x509/root_unix_test.go b/src/crypto/x509/root_unix_test.go
index 9e220192b9f53ed265646e06076805ed3586f4e6..5a27d639b5383f1b5d970da8df35b84a937b4fe0 100644 (file)
--- a/src/crypto/x509/root_unix_test.go
+++ b/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)
+       }
+}