From: Emmanuel T Odeke <emmanuel@orijtech.com>
Date: Mon, 4 Nov 2019 17:19:59 +0000 (-0800)
Subject: crypto/x509: load roots from colon separated SSL_CERT_DIR in loadSystemRoots
X-Git-Tag: go1.15beta1~1057
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=7a03d79498a32eb099d6f82aa8b19e813630be65;p=gostls13.git

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 f1e174c894..1be4058bab 100644
--- 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 9e220192b9..5a27d639b5 100644
--- 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)
+	}
+}