[go] crypto/x509: add SystemCertPool, refactor system cert pool loading

[Brad Fitzpatrick (Gerrit)]

Reviewers: Adam Langley, Andrew Gerrand, Ian Lance Taylor

Brad Fitzpatrick uploaded a change:
https://go-review.googlesource.com/21293

crypto/x509: add SystemCertPool, refactor system cert pool loading

This exports the system cert pool.

The system cert loading was refactored to let it be run multiple times
(so callers get a copy, and can't mutate global state), and also to
not discard errors.

SystemCertPool returns an error on Windows. Maybe it's fixable later,
but so far we haven't used it, since the system verifies TLS.

Fixes #13335

Change-Id: I3dfb4656a373f241bae8529076d24c5f532f113c
---
M src/crypto/x509/cert_pool.go
M src/crypto/x509/root.go
M src/crypto/x509/root_cgo_darwin.go
M src/crypto/x509/root_darwin_arm_gen.go
M src/crypto/x509/root_darwin_armx.go
M src/crypto/x509/root_nocgo_darwin.go
M src/crypto/x509/root_plan9.go
M src/crypto/x509/root_unix.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/verify.go
10 files changed, 85 insertions(+), 41 deletions(-)



diff --git a/src/crypto/x509/cert_pool.go b/src/crypto/x509/cert_pool.go
index 2362e84..efaadd5 100644
--- a/src/crypto/x509/cert_pool.go
+++ b/src/crypto/x509/cert_pool.go
@@ -18,10 +18,27 @@
// NewCertPool returns a new, empty CertPool.
func NewCertPool() *CertPool {
return &CertPool{
- make(map[string][]int),
- make(map[string][]int),
- nil,
+ bySubjectKeyId: make(map[string][]int),
+ byName: make(map[string][]int),
}
+}
+
+// SystemCertPool returns a copy of the system cert pool.
+//
+// Any mutations to the returned pool are not written to disk and do
+// not affect any other pool.
+func SystemCertPool() *CertPool {
+ cp := NewCertPool()
+ sp := systemRootsPool()
+ println("subjects: ", len(sp.Subjects()))
+ for _, derCert := range sp.Subjects() {
+ if cert, err := ParseCertificate(derCert); err == nil {
+ cp.AddCert(cert)
+ } else {
+ println(err.Error())
+ }
+ }
+ return sp
}

// findVerifiedParents attempts to find certificates in s which have
signed the
@@ -107,10 +124,10 @@

// Subjects returns a list of the DER-encoded subjects of
// all of the certificates in the pool.
-func (s *CertPool) Subjects() (res [][]byte) {
- res = make([][]byte, len(s.certs))
+func (s *CertPool) Subjects() [][]byte {
+ res := make([][]byte, len(s.certs))
for i, c := range s.certs {
res[i] = c.RawSubject
}
- return
+ return res
}
diff --git a/src/crypto/x509/root.go b/src/crypto/x509/root.go
index 8aae14e..787d955 100644
--- a/src/crypto/x509/root.go
+++ b/src/crypto/x509/root.go
@@ -7,11 +7,16 @@
import "sync"

var (
- once sync.Once
- systemRoots *CertPool
+ once sync.Once
+ systemRoots *CertPool
+ systemRootsErr error
)

func systemRootsPool() *CertPool {
once.Do(initSystemRoots)
return systemRoots
}
+
+func initSystemRoots() {
+ systemRoots, systemRootsErr = loadSystemRoots()
+}
diff --git a/src/crypto/x509/root_cgo_darwin.go
b/src/crypto/x509/root_cgo_darwin.go
index bf4a5cd..f067cd7 100644
--- a/src/crypto/x509/root_cgo_darwin.go
+++ b/src/crypto/x509/root_cgo_darwin.go
@@ -61,19 +61,23 @@
}
*/
import "C"
-import "unsafe"
+import (
+ "errors"
+ "unsafe"
+)

-func initSystemRoots() {
+func loadSystemRoots() (*CertPool, error) {
roots := NewCertPool()

var data C.CFDataRef = nil
err := C.FetchPEMRoots(&data)
if err == -1 {
- return
+ // TODO: better error message
+ return nil, errors.New("crypto/x509: failed to load darwin system roots
with cgo")
}

defer C.CFRelease(C.CFTypeRef(data))
buf := C.GoBytes(unsafe.Pointer(C.CFDataGetBytePtr(data)),
C.int(C.CFDataGetLength(data)))
roots.AppendCertsFromPEM(buf)
- systemRoots = roots
+ return roots, nil
}
diff --git a/src/crypto/x509/root_darwin_arm_gen.go
b/src/crypto/x509/root_darwin_arm_gen.go
index 5817158..6b373b9 100644
--- a/src/crypto/x509/root_darwin_arm_gen.go
+++ b/src/crypto/x509/root_darwin_arm_gen.go
@@ -184,8 +184,9 @@

package x509

-func initSystemRoots() {
- systemRoots = NewCertPool()
- systemRoots.AppendCertsFromPEM([]byte(systemRootsPEM))
+func loadSystemRoots() (*CertPool, error) {
+ p := NewCertPool()
+ p.AppendCertsFromPEM([]byte(systemRootsPEM))
+ return p
}
`
diff --git a/src/crypto/x509/root_darwin_armx.go
b/src/crypto/x509/root_darwin_armx.go
index 37675b4..66b7051 100644
--- a/src/crypto/x509/root_darwin_armx.go
+++ b/src/crypto/x509/root_darwin_armx.go
@@ -10,9 +10,10 @@

package x509

-func initSystemRoots() {
- systemRoots = NewCertPool()
- systemRoots.AppendCertsFromPEM([]byte(systemRootsPEM))
+func loadSystemRoots() (*CertPool, error) {
+ p := NewCertPool()
+ p.AppendCertsFromPEM([]byte(systemRootsPEM))
+ return p
}

const systemRootsPEM = `
diff --git a/src/crypto/x509/root_nocgo_darwin.go
b/src/crypto/x509/root_nocgo_darwin.go
index d00e257..2ac4666 100644
--- a/src/crypto/x509/root_nocgo_darwin.go
+++ b/src/crypto/x509/root_nocgo_darwin.go
@@ -6,6 +6,6 @@

package x509

-func initSystemRoots() {
- systemRoots, _ = execSecurityRoots()
+func loadSystemRoots() (*CertPool, error) {
+ return execSecurityRoots()
}
diff --git a/src/crypto/x509/root_plan9.go b/src/crypto/x509/root_plan9.go
index 9965caa..ebeb7df 100644
--- a/src/crypto/x509/root_plan9.go
+++ b/src/crypto/x509/root_plan9.go
@@ -6,7 +6,10 @@

package x509

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

// Possible certificate files; stop after finding one.
var certFiles = []string{
@@ -17,17 +20,18 @@
return nil, nil
}

-func initSystemRoots() {
+func loadSystemRoots() (*CertPool, error) {
roots := NewCertPool()
+ var bestErr error
for _, file := range certFiles {
data, err := ioutil.ReadFile(file)
if err == nil {
roots.AppendCertsFromPEM(data)
- systemRoots = roots
- return
+ return roots, nil
+ }
+ if bestErr == nil || (os.IsNotExist(bestErr) && !os.IsNotExist(err)) {
+ bestErr = err
}
}
-
- // All of the files failed to load. systemRoots will be nil which will
- // trigger a specific error at verification time.
+ return nil, bestErr
}
diff --git a/src/crypto/x509/root_unix.go b/src/crypto/x509/root_unix.go
index 9f06f9d..7bcb3d6 100644
--- a/src/crypto/x509/root_unix.go
+++ b/src/crypto/x509/root_unix.go
@@ -6,7 +6,10 @@

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.
@@ -19,20 +22,26 @@
return nil, nil
}

-func initSystemRoots() {
+func loadSystemRoots() (*CertPool, error) {
roots := NewCertPool()
+ var firstErr error
for _, file := range certFiles {
data, err := ioutil.ReadFile(file)
if err == nil {
roots.AppendCertsFromPEM(data)
- systemRoots = roots
- return
+ return roots, nil
+ }
+ if firstErr == nil && !os.IsNotExist(err) {
+ firstErr = err
}
}

for _, directory := range certDirectories {
fis, err := ioutil.ReadDir(directory)
if err != nil {
+ if firstErr == nil && !os.IsNotExist(err) {
+ firstErr = err
+ }
continue
}
rootsAdded := false
@@ -43,11 +52,9 @@
}
}
if rootsAdded {
- systemRoots = roots
- return
+ return roots, nil
}
}

- // All of the files failed to load. systemRoots will be nil which will
- // trigger a specific error at verification time.
+ return nil, firstErr
}
diff --git a/src/crypto/x509/root_windows.go
b/src/crypto/x509/root_windows.go
index 51c3be3..392c869 100644
--- a/src/crypto/x509/root_windows.go
+++ b/src/crypto/x509/root_windows.go
@@ -225,5 +225,4 @@
return chains, nil
}

-func initSystemRoots() {
-}
+func loadSystemRoots() (*CertPool, error) { return nil, nil }
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index d3b62d1..85c083f 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -117,10 +117,16 @@
}

// SystemRootsError results when we fail to load the system root
certificates.
-type SystemRootsError struct{}
+type SystemRootsError struct {
+ Err error
+}

-func (SystemRootsError) Error() string {
- return "x509: failed to load system roots and no roots provided"
+func (se SystemRootsError) Error() string {
+ msg := "x509: failed to load system roots and no roots provided"
+ if se.Err != nil {
+ return msg + "; " + se.Err.Error()
+ }
+ return msg
}

// errNotParsed is returned when a certificate without ASN.1 contents is
@@ -240,7 +246,7 @@
if opts.Roots == nil {
opts.Roots = systemRootsPool()
if opts.Roots == nil {
- return nil, SystemRootsError{}
+ return nil, SystemRootsError{systemRootsErr}
}
}


--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1: Run-TryBot+1

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=fae7afc3

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1:

(2 comments)

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

Line 33: println("subjects: ", len(sp.Subjects()))
errant debug print


Line 38: println(err.Error())
what's happening here?
would you rather not return an error?

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: Yes

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/21293/1//COMMIT_MSG
Commit Message:

Line 15: SystemCertPool returns an error on Windows. Maybe it's fixable
later,
that function doesn't return an error value

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: Yes

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/21293/1//COMMIT_MSG
Commit Message:

Line 15: SystemCertPool returns an error on Windows. Maybe it's fixable
later,

> that function doesn't return an error value

Oh, uploaded wrong copy.

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: Yes

[Brad Fitzpatrick (Gerrit)]

Reviewers: Adam Langley, Andrew Gerrand, Ian Lance Taylor, Gobot Gobot

Brad Fitzpatrick uploaded a new patch set:

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

crypto/x509: add SystemCertPool, refactor system cert pool loading

This exports the system cert pool.

The system cert loading was refactored to let it be run multiple times
(so callers get a copy, and can't mutate global state), and also to
not discard errors.

SystemCertPool returns an error on Windows. Maybe it's fixable later,
but so far we haven't used it, since the system verifies TLS.

Fixes #13335

Change-Id: I3dfb4656a373f241bae8529076d24c5f532f113c
---
M src/crypto/x509/cert_pool.go
M src/crypto/x509/root.go
M src/crypto/x509/root_cgo_darwin.go
M src/crypto/x509/root_darwin_arm_gen.go
M src/crypto/x509/root_darwin_armx.go
M src/crypto/x509/root_nocgo_darwin.go
M src/crypto/x509/root_plan9.go
M src/crypto/x509/root_unix.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/verify.go

10 files changed, 80 insertions(+), 41 deletions(-)

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2: Run-TryBot+1

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=800b279c

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Andrew Gerrand (Gerrit)]

Andrew Gerrand has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2: Code-Review+2

Looks good to me.

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-HasComments: No

[Patrick Hemmer (Gerrit)]

Patrick Hemmer has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2:

(1 comment)

https://go-review.googlesource.com/#/c/21293/2/src/crypto/x509/cert_pool.go
File src/crypto/x509/cert_pool.go:

Line 32: func SystemCertPool() (*CertPool, error) {
I think we should mention that `error` is the first error encountered
loading the pool, and that `*CertPool` will still be valid and contain all
the certs that didn't error.

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-Reviewer: Patrick Hemmer <patrick...@gmail.com>
Gerrit-HasComments: Yes

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2:

(1 comment)

https://go-review.googlesource.com/#/c/21293/2/src/crypto/x509/cert_pool.go
File src/crypto/x509/cert_pool.go:

Line 32: func SystemCertPool() (*CertPool, error) {
> I think we should mention that `error` is the first error encountered
> loadi

I don't think it's worth deviating from normal Go convention here and
requiring more docs and mental load.

Only one of these should be non-nil. Did I mess up one of the files?

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

[Patrick Hemmer (Gerrit)]

Patrick Hemmer has posted comments on this change.

crypto/x509: add SystemCertPool, refactor system cert pool loading

Patch Set 2:

(1 comment)

https://go-review.googlesource.com/#/c/21293/2/src/crypto/x509/cert_pool.go
File src/crypto/x509/cert_pool.go:

Line 32: func SystemCertPool() (*CertPool, error) {

> I don't think it's worth deviating from normal Go convention here and
> requi

Oh, nevermind, you're good. When I first read loadSystemRoots I thought you
were returning roots & firstErr, but you're not.
Sorry for the noise :-)

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has submitted this change and it was merged.

crypto/x509: add SystemCertPool, refactor system cert pool loading

This exports the system cert pool.

The system cert loading was refactored to let it be run multiple times
(so callers get a copy, and can't mutate global state), and also to
not discard errors.

SystemCertPool returns an error on Windows. Maybe it's fixable later,
but so far we haven't used it, since the system verifies TLS.

Fixes #13335

Change-Id: I3dfb4656a373f241bae8529076d24c5f532f113c

Reviewed-on: https://go-review.googlesource.com/21293
Run-TryBot: Brad Fitzpatrick <brad...@golang.org>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Andrew Gerrand <a...@golang.org>

---
M src/crypto/x509/cert_pool.go
M src/crypto/x509/root.go
M src/crypto/x509/root_cgo_darwin.go
M src/crypto/x509/root_darwin_arm_gen.go
M src/crypto/x509/root_darwin_armx.go
M src/crypto/x509/root_nocgo_darwin.go
M src/crypto/x509/root_plan9.go
M src/crypto/x509/root_unix.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/verify.go

10 files changed, 80 insertions(+), 41 deletions(-)

Approvals:
Andrew Gerrand: Looks good to me, approved
Gobot Gobot: TryBots succeeded
Brad Fitzpatrick: Run TryBots

--
https://go-review.googlesource.com/21293
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Andrew Gerrand <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

Gerrit-Reviewer: Patrick Hemmer <patrick...@gmail.com>