[go] crypto/x509: load certificates from paths in env vars, extra certs dirs

[Jakob Borg (Gerrit)]

Jakob Borg uploaded a change:
https://go-review.googlesource.com/20253

crypto/x509: load certificates from paths in env vars, extra certs dirs

This implements the following, related, changes:

- adds a file path and changes the search order for FreeBSD to prefer
system roots,

- adds the ".../certs" directories into the directory search path for
the case where the specific bundle files are not present, and

- adds the ability to use SSL_CERT_FILE and SSL_CERT_DIR environment
variables to influence the search paths at runtime.

Fixes #14311
Fixes #14022

*** Review notes: Both issues mention both environment variables and
path changes, which is why I've intertwined it into one commit. FreeBSD
libfetch actually uses SSL_CA_CERT_FILE and SSL_CA_CERT_DIR instead - do
we want to split this out to be compatible with that, or go with this as
is for all OSes? We'd need to add a root_freebsd.go for that as the
other BSDs use OpenSSL or some variant thereof (iirc)?

Change-Id: I5afea05edc4fd8243762571fd84eb000f0410fe0
---
M src/crypto/x509/root_bsd.go
M src/crypto/x509/root_unix.go
A src/crypto/x509/root_unix_test.go
A src/crypto/x509/testdata/test-ca.crt
4 files changed, 114 insertions(+), 3 deletions(-)



diff --git a/src/crypto/x509/root_bsd.go b/src/crypto/x509/root_bsd.go
index 9317283..7692ccf 100644
--- a/src/crypto/x509/root_bsd.go
+++ b/src/crypto/x509/root_bsd.go
@@ -8,7 +8,8 @@

// Possible certificate files; stop after finding one.
var certFiles = []string{
- "/usr/local/share/certs/ca-root-nss.crt", // FreeBSD/DragonFly
- "/etc/ssl/cert.pem", // OpenBSD
+ "/usr/local/etc/ssl/cert.pem", // FreeBSD
+ "/etc/ssl/cert.pem", // FreeBSD/OpenBSD
+ "/usr/local/share/certs/ca-root-nss.crt", // DragonFly
"/etc/openssl/certs/ca-certificates.crt", // NetBSD
}
diff --git a/src/crypto/x509/root_unix.go b/src/crypto/x509/root_unix.go
index 9f06f9d..e508527 100644
--- a/src/crypto/x509/root_unix.go
+++ b/src/crypto/x509/root_unix.go
@@ -6,14 +6,27 @@

package x509

-import "io/ioutil"
+import (
+ "io/ioutil"
+ "os"
+)

// Possible directories with certificate files; stop after successfully
// reading at least one file from a directory.
var certDirectories = []string{
"/etc/ssl/certs", // SLES10/SLES11,
https://golang.org/issue/12139
"/system/etc/security/cacerts", // Android
+ "/etc/pki/tls/certs", // Fedora/RHEL
+ "/usr/local/share/certs", // FreeBSD/DragonFly
+ "/etc/openssl/certs", // NetBSD
}
+
+// These environment variables are used by OpenSSL to specify the CA
+// certificate file to load, or directory to scan for certificates.
+const (
+ certFileEnvVar = "SSL_CERT_FILE"
+ certDirEnvVar = "SSL_CERT_DIR"
+)

func (c *Certificate) systemVerify(opts *VerifyOptions) (chains
[][]*Certificate, err error) {
return nil, nil
@@ -21,6 +34,11 @@

func initSystemRoots() {
roots := NewCertPool()
+
+ if file := os.Getenv(certFileEnvVar); file != "" {
+ certFiles = append([]string{file}, certFiles...)
+ }
+
for _, file := range certFiles {
data, err := ioutil.ReadFile(file)
if err == nil {
@@ -30,6 +48,10 @@
}
}

+ if directory := os.Getenv(certDirEnvVar); directory != "" {
+ certDirectories = append([]string{directory}, certDirectories...)
+ }
+
for _, directory := range certDirectories {
fis, err := ioutil.ReadDir(directory)
if err != nil {
diff --git a/src/crypto/x509/root_unix_test.go
b/src/crypto/x509/root_unix_test.go
new file mode 100644
index 0000000..ddc9090
--- /dev/null
+++ b/src/crypto/x509/root_unix_test.go
@@ -0,0 +1,68 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build dragonfly freebsd linux nacl netbsd openbsd solaris
+
+package x509
+
+import (
+ "os"
+ "testing"
+)
+
+func TestSystemRootsEnvFile(t *testing.T) {
+ // Should load a specific CA file when pointed directly to it using the
+ // environment variable SSL_CERT_FILE=path
+
+ oldEnv := os.Getenv(certFileEnvVar)
+ defer os.Setenv(certFileEnvVar, oldEnv)
+ os.Setenv(certFileEnvVar, "testdata/test-ca.crt")
+
+ verifyLoadTestCA(t)
+}
+
+func TestSystemRootsEnvDir(t *testing.T) {
+ // Should load a specific CA file when pointed directly to the directory
+ // containing it using the environment variable SSL_CERT_DIR=path
+
+ oldEnv := os.Getenv(certDirEnvVar)
+ defer os.Setenv(certDirEnvVar, oldEnv)
+ os.Setenv(certDirEnvVar, "testdata")
+
+ // Prevent successfull loading of a CA bundle file, as these are searched
+ // before directories.
+ oldCertFiles := certFiles
+ defer func() {
+ certFiles = oldCertFiles
+ }()
+ certFiles = nil
+
+ verifyLoadTestCA(t)
+}
+
+func verifyLoadTestCA(t *testing.T) {
+ // initSystemRoots touches several global variables. Save them and restore
+ // after the test.
+ oldSystemRoots := systemRoots
+ oldCertFiles := certFiles
+ oldCertDirectories := certDirectories
+ defer func() {
+ systemRoots = oldSystemRoots
+ certFiles = oldCertFiles
+ certDirectories = oldCertDirectories
+ }()
+
+ systemRoots = nil
+ initSystemRoots()
+
+ if systemRoots == nil {
+ t.Fatal("Should have loaded systemRoots (was nil)")
+ }
+ if l := len(systemRoots.certs); l != 1 {
+ t.Fatalf("Loaded %d certificates, expected 1", l)
+ }
+ if cn := systemRoots.certs[0].Subject.CommonName; cn != "Test CA" {
+ t.Fatalf(`Should have loaded "Test CA", got %q`, cn)
+ }
+}
diff --git a/src/crypto/x509/testdata/test-ca.crt
b/src/crypto/x509/testdata/test-ca.crt
new file mode 100644
index 0000000..df26668
--- /dev/null
+++ b/src/crypto/x509/testdata/test-ca.crt
@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDWTCCAsKgAwIBAgIJALawQzTtEh7uMA0GCSqGSIb3DQEBBQUAMHwxCzAJBgNV
+BAYTAlNFMQ0wCwYDVQQIEwRUZXN0MQ0wCwYDVQQHEwRUZXN0MQ0wCwYDVQQKEwRU
+ZXN0MQ0wCwYDVQQLEwRUZXN0MRAwDgYDVQQDEwdUZXN0IENBMR8wHQYJKoZIhvcN
+AQkBFhB0ZXN0QGV4YW1wbGUuY29tMB4XDTE2MDIxNTIzNTIyMloXDTE2MDMxNjIz
+NTIyMlowfDELMAkGA1UEBhMCU0UxDTALBgNVBAgTBFRlc3QxDTALBgNVBAcTBFRl
+c3QxDTALBgNVBAoTBFRlc3QxDTALBgNVBAsTBFRlc3QxEDAOBgNVBAMTB1Rlc3Qg
+Q0ExHzAdBgkqhkiG9w0BCQEWEHRlc3RAZXhhbXBsZS5jb20wgZ8wDQYJKoZIhvcN
+AQEBBQADgY0AMIGJAoGBALk9p+l8HcRJqb2zyVannDl1I0Bqb/rGSK9sU4ZDYwSR
+Mbf+0rd0p3mk81fnpKyykbYpJZVvg++7sSpMdzSqSbaytnIQDZjpHz/xfFKoVqGt
+QBPXXafzQ+FU/2bfaSXxp1MAFbDlzHdXn2Qtxi+tcm5qiXELrJ9smmfajaRrmYt7
+AgMBAAGjgeIwgd8wHQYDVR0OBBYEFN3Ok3UaUNlBgAfx5AQg/uOM2lQuMIGvBgNV
+HSMEgacwgaSAFN3Ok3UaUNlBgAfx5AQg/uOM2lQuoYGApH4wfDELMAkGA1UEBhMC
+U0UxDTALBgNVBAgTBFRlc3QxDTALBgNVBAcTBFRlc3QxDTALBgNVBAoTBFRlc3Qx
+DTALBgNVBAsTBFRlc3QxEDAOBgNVBAMTB1Rlc3QgQ0ExHzAdBgkqhkiG9w0BCQEW
+EHRlc3RAZXhhbXBsZS5jb22CCQC2sEM07RIe7jAMBgNVHRMEBTADAQH/MA0GCSqG
+SIb3DQEBBQUAA4GBAHxyxOg3b4Wdm3a58vfXv8d7pk1V8dQ4ZOtqzd9QUKeiti84
+FtaDc8Rwqm7bml+iYBFTAePF97YOaHfH9ffEfunPUChuZyCmooqKnUiqy5elhUA1
+79FkAmUxZI/EqH4BGrm23dYiRnbJEHNNBl9/zy63Y1ntMyFt+g5hQt8Ut/NZ
+-----END CERTIFICATE-----

--
https://go-review.googlesource.com/20253

[Krzysztof Malinowski (Gerrit)]

Krzysztof Malinowski has posted comments on this change.

crypto/x509: load certificates from paths in env vars, extra certs dirs

Patch Set 1:

(1 comment)

The directories should be parsed even when certificate bundle is found.

https://go-review.googlesource.com/#/c/20253/1/src/crypto/x509/root_unix.go
File src/crypto/x509/root_unix.go:

Line 47: return
Do not return here. Additional certificates may be located in SSL_CERT_DIR
or certDirectories, these should be taken into account as well.

The main idea here is that certificate file bundle will most probably be
delivered by system maintainer (e.g. from rpm package), while in
certificate dir one may place additional certificates (e.g. company's root
CA). This design comes from OpenSSL and it allows certificate bundle to be
updated with system updates while keeping custom certificates intact.


--
https://go-review.googlesource.com/20253
Gerrit-Reviewer: Krzysztof Malinowski <ras...@gmail.com>
Gerrit-HasComments: Yes

[Russ Cox (Gerrit)]

Russ Cox has posted comments on this change.

crypto/x509: load certificates from paths in env vars, extra certs dirs

Patch Set 1:

R=go1.8

Is there a canonical doc about the search order and what variable overrides
what other variable? I'm afraid that if we put this in then we'll end up
being different from other implementations in subtle ways (for example is
setting the env var a way to keep the system roots from being consulted,
and if not is there any way to override them completely?).

--
https://go-review.googlesource.com/20253
Gerrit-Reviewer: Krzysztof Malinowski <ras...@gmail.com>

Gerrit-Reviewer: Russ Cox <r...@golang.org>
Gerrit-HasComments: No

[Krzysztof Malinowski (Gerrit)]

Krzysztof Malinowski has posted comments on this change.

crypto/x509: load certificates from paths in env vars, extra certs dirs

Patch Set 1:

> Is there a canonical doc about the search order and what variable
> overrides what other variable?

These variables are defined and used in OpenSSL. OpenSSL itself does not
come with any predefined trusted CA store [1], so everything that gets
consulted is from SSL_CERT_FILE and SSL_CERT_DIR.

[2] states that "When looking up CA certificates, the OpenSSL library will
first search the certificates in CAfile, then those in CApath." Both are
read however.

I could not find any documentation on those variables being used, but
scanning through OpenSSL code shows that setting environment variable
overrides default value. Relevant code part from [3]:

file = (char *)getenv(X509_get_default_cert_file_env());
if (file)
ok = (X509_load_cert_crl_file(ctx, file,
X509_FILETYPE_PEM) != 0);
else
ok = (X509_load_cert_crl_file
(ctx, X509_get_default_cert_file(),
X509_FILETYPE_PEM) != 0);

So in general OpenSSL works like this:
- look for SSL_CERT_FILE EV
+- if defined load SSL_CERT_FILE
+- otherwise load default
- look for SSL_CERT_DIR EV
+- if defined load certs from SSL_CERT_DIR
+- otherwise load from default dir

Defaults for OpenSSL are "certs" for directory and "cert.pem" for file [4],
both checked under OPENSSLDIR, which is defined upon SSL compilation and
can be verified with "openssl version -d". That's why e.g. RedHat creates a
symlink /etc/pki/tls/cert.pem -> certs/ca-bundle.crt, since they compiled
SSL with OPENSSLDIR == "/etc/pki/tls" and openssl reads
/etc/pki/tls/cert.pem (not configurable without changing source).

I recommend that Go follows the same algorithm as OpenSSL, i.e. prefer EV
over defaults and prefer file over directory.

[1] https://www.openssl.org/docs/faq.html#USER16
[2]
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_default_verify_dir.html
[3] https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c
[4]
https://github.com/openssl/openssl/blob/master/crypto/include/internal/cryptlib.h