[go] crypto/x509: verification with system and custom roots
291 views
Skip to first unread message
Roland Shoemaker (Gerrit)
Oct 1, 2021, 1:22:10 PM10/1/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Roland Shoemaker has uploaded this change for review.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
4 files changed, 69 insertions(+), 305 deletions(-)
diff --git a/src/crypto/x509/cert_pool.go b/src/crypto/x509/cert_pool.go
index d760dc1..7a5a983 100644
--- a/src/crypto/x509/cert_pool.go
+++ b/src/crypto/x509/cert_pool.go
@@ -29,6 +29,11 @@
// call getCert and otherwise negate savings from lazy getCert
// funcs).
haveSum map[sum224]bool
+
+ // systemPool indicates whether this is a special hybrid pool
+ // that requires doing two verifications, one using roots provided
+ // by the caller, and one using the system platform verifier.
+ systemPool bool
}
// lazyCert is minimal metadata about a Cert and a func to retrieve it
@@ -243,6 +248,10 @@
// Subjects returns a list of the DER-encoded subjects of
// all of the certificates in the pool.
+//
+// Deprecated: users should not rely on Subjects to determine
+// the contents of the CertPool. For system CertPools, on Windows
+// and macOS, Subjects will always return an empty slice.
func (s *CertPool) Subjects() [][]byte {
res := make([][]byte, s.len())
for i, lc := range s.lazyCerts {
diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go
index 52ea973..e352a71 100644
--- a/src/crypto/x509/root_darwin.go
+++ b/src/crypto/x509/root_darwin.go
@@ -93,5 +93,5 @@
}
func loadSystemRoots() (*CertPool, error) {
- return nil, nil
+ return &CertPool{systemPool: true}, nil
}
diff --git a/src/crypto/x509/root_windows.go b/src/crypto/x509/root_windows.go
index f77ea3a..efb53d8 100644
--- a/src/crypto/x509/root_windows.go
+++ b/src/crypto/x509/root_windows.go
@@ -10,308 +10,6 @@
"unsafe"
)
-// Creates a new *syscall.CertContext representing the leaf certificate in an in-memory
-// certificate store containing itself and all of the intermediate certificates specified
-// in the opts.Intermediates CertPool.
-//
-// A pointer to the in-memory store is available in the returned CertContext's Store field.
-// The store is automatically freed when the CertContext is freed using
-// syscall.CertFreeCertificateContext.
-func createStoreContext(leaf *Certificate, opts *VerifyOptions) (*syscall.CertContext, error) {
- var storeCtx *syscall.CertContext
-
- leafCtx, err := syscall.CertCreateCertificateContext(syscall.X509_ASN_ENCODING|syscall.PKCS_7_ASN_ENCODING, &leaf.Raw[0], uint32(len(leaf.Raw)))
- if err != nil {
- return nil, err
- }
- defer syscall.CertFreeCertificateContext(leafCtx)
-
- handle, err := syscall.CertOpenStore(syscall.CERT_STORE_PROV_MEMORY, 0, 0, syscall.CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, 0)
- if err != nil {
- return nil, err
- }
- defer syscall.CertCloseStore(handle, 0)
-
- err = syscall.CertAddCertificateContextToStore(handle, leafCtx, syscall.CERT_STORE_ADD_ALWAYS, &storeCtx)
- if err != nil {
- return nil, err
- }
-
- if opts.Intermediates != nil {
- for i := 0; i < opts.Intermediates.len(); i++ {
- intermediate, err := opts.Intermediates.cert(i)
- if err != nil {
- return nil, err
- }
- ctx, err := syscall.CertCreateCertificateContext(syscall.X509_ASN_ENCODING|syscall.PKCS_7_ASN_ENCODING, &intermediate.Raw[0], uint32(len(intermediate.Raw)))
- if err != nil {
- return nil, err
- }
-
- err = syscall.CertAddCertificateContextToStore(handle, ctx, syscall.CERT_STORE_ADD_ALWAYS, nil)
- syscall.CertFreeCertificateContext(ctx)
- if err != nil {
- return nil, err
- }
- }
- }
-
- return storeCtx, nil
-}
-
-// extractSimpleChain extracts the final certificate chain from a CertSimpleChain.
-func extractSimpleChain(simpleChain **syscall.CertSimpleChain, count int) (chain []*Certificate, err error) {
- if simpleChain == nil || count == 0 {
- return nil, errors.New("x509: invalid simple chain")
- }
-
- simpleChains := (*[1 << 20]*syscall.CertSimpleChain)(unsafe.Pointer(simpleChain))[:count:count]
- lastChain := simpleChains[count-1]
- elements := (*[1 << 20]*syscall.CertChainElement)(unsafe.Pointer(lastChain.Elements))[:lastChain.NumElements:lastChain.NumElements]
- for i := 0; i < int(lastChain.NumElements); i++ {
- // Copy the buf, since ParseCertificate does not create its own copy.
- cert := elements[i].CertContext
- encodedCert := (*[1 << 20]byte)(unsafe.Pointer(cert.EncodedCert))[:cert.Length:cert.Length]
- buf := make([]byte, cert.Length)
- copy(buf, encodedCert)
- parsedCert, err := ParseCertificate(buf)
- if err != nil {
- return nil, err
- }
- chain = append(chain, parsedCert)
- }
-
- return chain, nil
-}
-
-// checkChainTrustStatus checks the trust status of the certificate chain, translating
-// any errors it finds into Go errors in the process.
-func checkChainTrustStatus(c *Certificate, chainCtx *syscall.CertChainContext) error {
- if chainCtx.TrustStatus.ErrorStatus != syscall.CERT_TRUST_NO_ERROR {
- status := chainCtx.TrustStatus.ErrorStatus
- switch status {
- case syscall.CERT_TRUST_IS_NOT_TIME_VALID:
- return CertificateInvalidError{c, Expired, ""}
- case syscall.CERT_TRUST_IS_NOT_VALID_FOR_USAGE:
- return CertificateInvalidError{c, IncompatibleUsage, ""}
- // TODO(filippo): surface more error statuses.
- default:
- return UnknownAuthorityError{c, nil, nil}
- }
- }
- return nil
-}
-
-// checkChainSSLServerPolicy checks that the certificate chain in chainCtx is valid for
-// use as a certificate chain for a SSL/TLS server.
-func checkChainSSLServerPolicy(c *Certificate, chainCtx *syscall.CertChainContext, opts *VerifyOptions) error {
- servernamep, err := syscall.UTF16PtrFromString(opts.DNSName)
- if err != nil {
- return err
- }
- sslPara := &syscall.SSLExtraCertChainPolicyPara{
- AuthType: syscall.AUTHTYPE_SERVER,
- ServerName: servernamep,
- }
- sslPara.Size = uint32(unsafe.Sizeof(*sslPara))
-
- para := &syscall.CertChainPolicyPara{
- ExtraPolicyPara: (syscall.Pointer)(unsafe.Pointer(sslPara)),
- }
- para.Size = uint32(unsafe.Sizeof(*para))
-
- status := syscall.CertChainPolicyStatus{}
- err = syscall.CertVerifyCertificateChainPolicy(syscall.CERT_CHAIN_POLICY_SSL, chainCtx, para, &status)
- if err != nil {
- return err
- }
-
- // TODO(mkrautz): use the lChainIndex and lElementIndex fields
- // of the CertChainPolicyStatus to provide proper context, instead
- // using c.
- if status.Error != 0 {
- switch status.Error {
- case syscall.CERT_E_EXPIRED:
- return CertificateInvalidError{c, Expired, ""}
- case syscall.CERT_E_CN_NO_MATCH:
- return HostnameError{c, opts.DNSName}
- case syscall.CERT_E_UNTRUSTEDROOT:
- return UnknownAuthorityError{c, nil, nil}
- default:
- return UnknownAuthorityError{c, nil, nil}
- }
- }
-
- return nil
-}
-
-// windowsExtKeyUsageOIDs are the C NUL-terminated string representations of the
-// OIDs for use with the Windows API.
-var windowsExtKeyUsageOIDs = make(map[ExtKeyUsage][]byte, len(extKeyUsageOIDs))
-
-func init() {
- for _, eku := range extKeyUsageOIDs {
- windowsExtKeyUsageOIDs[eku.extKeyUsage] = []byte(eku.oid.String() + "\x00")
- }
-}
-
-func verifyChain(c *Certificate, chainCtx *syscall.CertChainContext, opts *VerifyOptions) (chain []*Certificate, err error) {
- err = checkChainTrustStatus(c, chainCtx)
- if err != nil {
- return nil, err
- }
-
- if opts != nil && len(opts.DNSName) > 0 {
- err = checkChainSSLServerPolicy(c, chainCtx, opts)
- if err != nil {
- return nil, err
- }
- }
-
- chain, err = extractSimpleChain(chainCtx.Chains, int(chainCtx.ChainCount))
- if err != nil {
- return nil, err
- }
- if len(chain) == 0 {
- return nil, errors.New("x509: internal error: system verifier returned an empty chain")
- }
-
- // Mitigate CVE-2020-0601, where the Windows system verifier might be
- // tricked into using custom curve parameters for a trusted root, by
- // double-checking all ECDSA signatures. If the system was tricked into
- // using spoofed parameters, the signature will be invalid for the correct
- // ones we parsed. (We don't support custom curves ourselves.)
- for i, parent := range chain[1:] {
- if parent.PublicKeyAlgorithm != ECDSA {
- continue
- }
- if err := parent.CheckSignature(chain[i].SignatureAlgorithm,
- chain[i].RawTBSCertificate, chain[i].Signature); err != nil {
- return nil, err
- }
- }
- return chain, nil
-}
-
-// systemVerify is like Verify, except that it uses CryptoAPI calls
-// to build certificate chains and verify them.
-func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) {
- storeCtx, err := createStoreContext(c, opts)
- if err != nil {
- return nil, err
- }
- defer syscall.CertFreeCertificateContext(storeCtx)
-
- para := new(syscall.CertChainPara)
- para.Size = uint32(unsafe.Sizeof(*para))
-
- keyUsages := opts.KeyUsages
- if len(keyUsages) == 0 {
- keyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
- }
- oids := make([]*byte, 0, len(keyUsages))
- for _, eku := range keyUsages {
- if eku == ExtKeyUsageAny {
- oids = nil
- break
- }
- if oid, ok := windowsExtKeyUsageOIDs[eku]; ok {
- oids = append(oids, &oid[0])
- }
- }
- if oids != nil {
- para.RequestedUsage.Type = syscall.USAGE_MATCH_TYPE_OR
- para.RequestedUsage.Usage.Length = uint32(len(oids))
- para.RequestedUsage.Usage.UsageIdentifiers = &oids[0]
- } else {
- para.RequestedUsage.Type = syscall.USAGE_MATCH_TYPE_AND
- para.RequestedUsage.Usage.Length = 0
- para.RequestedUsage.Usage.UsageIdentifiers = nil
- }
-
- var verifyTime *syscall.Filetime
- if opts != nil && !opts.CurrentTime.IsZero() {
- ft := syscall.NsecToFiletime(opts.CurrentTime.UnixNano())
- verifyTime = &ft
- }
-
- // The default is to return only the highest quality chain,
- // setting this flag will add additional lower quality contexts.
- // These are returned in the LowerQualityChains field.
- const CERT_CHAIN_RETURN_LOWER_QUALITY_CONTEXTS = 0x00000080
-
- // CertGetCertificateChain will traverse Windows's root stores in an attempt to build a verified certificate chain
- var topCtx *syscall.CertChainContext
- err = syscall.CertGetCertificateChain(syscall.Handle(0), storeCtx, verifyTime, storeCtx.Store, para, CERT_CHAIN_RETURN_LOWER_QUALITY_CONTEXTS, 0, &topCtx)
- if err != nil {
- return nil, err
- }
- defer syscall.CertFreeCertificateChain(topCtx)
-
- chain, topErr := verifyChain(c, topCtx, opts)
- if topErr == nil {
- chains = append(chains, chain)
- }
-
- if lqCtxCount := topCtx.LowerQualityChainCount; lqCtxCount > 0 {
- lqCtxs := (*[1 << 20]*syscall.CertChainContext)(unsafe.Pointer(topCtx.LowerQualityChains))[:lqCtxCount:lqCtxCount]
-
- for _, ctx := range lqCtxs {
- chain, err := verifyChain(c, ctx, opts)
- if err == nil {
- chains = append(chains, chain)
- }
- }
- }
-
- if len(chains) == 0 {
- // Return the error from the highest quality context.
- return nil, topErr
- }
-
- return chains, nil
-}
-
func loadSystemRoots() (*CertPool, error) {
- // TODO: restore this functionality on Windows. We tried to do
- // it in Go 1.8 but had to revert it. See Issue 18609.
- // Returning (nil, nil) was the old behavior, prior to CL 30578.
- // The if statement here avoids vet complaining about
- // unreachable code below.
- if true {
- return nil, nil
- }
-
- const CRYPT_E_NOT_FOUND = 0x80092004
-
- store, err := syscall.CertOpenSystemStore(0, syscall.StringToUTF16Ptr("ROOT"))
- if err != nil {
- return nil, err
- }
- defer syscall.CertCloseStore(store, 0)
-
- roots := NewCertPool()
- var cert *syscall.CertContext
- for {
- cert, err = syscall.CertEnumCertificatesInStore(store, cert)
- if err != nil {
- if errno, ok := err.(syscall.Errno); ok {
- if errno == CRYPT_E_NOT_FOUND {
- break
- }
- }
- return nil, err
- }
- if cert == nil {
- break
- }
- // Copy the buf, since ParseCertificate does not create its own copy.
- buf := (*[1 << 20]byte)(unsafe.Pointer(cert.EncodedCert))[:cert.Length:cert.Length]
- buf2 := make([]byte, cert.Length)
- copy(buf2, buf)
- if c, err := ParseCertificate(buf2); err == nil {
- roots.AddCert(c)
- }
- }
- return roots, nil
+ return &CertPool{systemPool: true}, nil
}
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 59852d9..cb1d8dc 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -6,6 +6,7 @@
import (
"bytes"
+ "crypto/sha256"
"errors"
"fmt"
"net"
@@ -742,8 +743,17 @@
}
// Use platform verifiers, where available
+ var platformChains [][]*Certificate
if opts.Roots == nil && (runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios") {
- return c.systemVerify(&opts)
+ if opts.Roots == nil {
+ return c.systemVerify(&opts)
+ }
+ if opts.Roots != nil && opts.Roots.systemPool {
+ platformChains, err = c.systemVerify(&opts)
+ if err != nil {
+ return nil, err
+ }
+ }
}
if opts.Roots == nil {
@@ -792,6 +802,33 @@
}
}
+ if len(platformChains) > 0 {
+ // In order to prevent returning duplicate chains we need
+ // to double check there isn't anything in platformChains
+ // which is already present in chains (we assume that
+ // both chains and platformChains each contain a unique
+ // set of chains.)
+ chainFingerprints := make(map[string]bool, len(chains))
+ fpChain := func(chain []*Certificate) string {
+ var fp string
+ for _, c := range chain {
+ h := sha256.Sum256(c.Raw)
+ fp += fmt.Sprintf("%s", h)
+ }
+ return fp
+ }
+ for _, chain := range chains {
+ chainFingerprints[fpChain(chain)] = true
+ }
+
+ for _, chain := range platformChains {
+ if chainFingerprints[fpChain(chain)] {
+ continue
+ }
+ chains = append(chains, chain)
+ }
+ }
+
if len(chains) == 0 {
return nil, CertificateInvalidError{c, IncompatibleUsage, ""}
}
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Oct 1, 2021, 1:25:17 PM10/1/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Patch set 1:Run-TryBot +1Trust +1
1 comment:
Patchset:
TRY=ios
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 5, 2021, 5:00:40 PM11/5/21
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
4 comments:
File src/crypto/x509/cert_pool.go:
Patch Set #1, Line 254:
// and macOS, Subjects will always return an empty slice.Deprecated: if s was returned by SystemCertPool, Subjects will not include the system roots.
File src/crypto/x509/root_windows.go:
Patch Set #1, Line 198:
func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) {Wait, why is the system verifier gone too, and why did tests not catch this?
File src/crypto/x509/verify.go:
Patch Set #1, Line 751:
if opts.Roots != nil && opts.Roots.systemPool {This is inside a "if opts.Roots == nil" so it will never hit. It needs a test.
I think it's fine for the sake of simplicity to only return the platform chains if the platform verification passes. That way, this can just be:
if GOOS ... {
if roots == nil || roots.systemPool && len(roots) == 0 { return systemVerify }
if roots.systemPool {
if s := systemVerify; s { return s }
}
}
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:09:12 PM11/5/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Roland Shoemaker uploaded patch set #2 to this change.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 245 insertions(+), 61 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:09:56 PM11/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 2:Run-TryBot +1Trust +1
4 comments:
File src/crypto/x509/cert_pool.go:
Patch Set #1, Line 254:
// and macOS, Subjects will always return an empty slice.Deprecated: if s was returned by SystemCertPool, Subjects will not include the system roots.
Done
File src/crypto/x509/root_windows.go:
Patch Set #1, Line 198:
func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) {Wait, why is the system verifier gone too, and why did tests not catch this?
Uh whoops. There aren't any windows verifier tests. Probably we should add something like what I added for the darwin verifier tests using badssl.
File src/crypto/x509/verify.go:
Patch Set #1, Line 751:
if opts.Roots != nil && opts.Roots.systemPool {This is inside a "if opts.Roots == nil" so it will never hit. It needs a test.
Done
I think it's fine for the sake of simplicity to only return the platform chains if the platform veri […]
Done
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:18:46 PM11/5/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Roland Shoemaker uploaded patch set #3 to this change.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 253 insertions(+), 61 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:19:06 PM11/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 3:Trust +1
1 comment:
Patchset:
TRY=
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:19:38 PM11/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 3:Run-TryBot +1
1 comment:
Patchset:
TRY=
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:45:43 PM11/5/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Roland Shoemaker uploaded patch set #4 to this change.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 247 insertions(+), 61 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 9:52:30 PM11/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 4:Run-TryBot +1Trust +1
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 5, 2021, 10:14:07 PM11/5/21
to goph...@pubsubhelper.golang.org, Go Bot, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
1 comment:
Patchset:
Build is still in progress... Status page: https://farmer.golang.org/try?commit=03780c84 […]
Everything passed (except an unrelated linux failure), but because of the ios builder being dead this will never resolve.
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 11:53:44 AM11/6/21
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Filippo Valsorda, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Patch set 4:Code-Review +2
2 comments:
File src/crypto/x509/hybrid_pool_test.go:
Patch Set #4, Line 69:
fmt.Println("goog a")Leftover debug, here and below.
File src/crypto/x509/verify.go:
If this is a pure system pool (that is, if opts.Roots.len() is zero), we should return the systemVerify error.
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:36:37 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
1 comment:
File src/crypto/x509/hybrid_pool_test.go:
Patch Set #4, Line 69:
fmt.Println("goog a")Leftover debug, here and below.
Done
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:36:37 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda uploaded patch set #5 to the change originally created by Roland Shoemaker.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 249 insertions(+), 62 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:38:47 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Patch set 5:Code-Review +2
1 comment:
File src/crypto/x509/verify.go:
If this is a pure system pool (that is, if opts.Roots. […]
Done
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:42:33 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda uploaded patch set #6 to the change originally created by Roland Shoemaker.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Updates #46287
Fixes #16736
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 250 insertions(+), 62 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:43:34 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda uploaded patch set #7 to the change originally created by Roland Shoemaker.
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Fixes #46287
Fixes #16736
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 250 insertions(+), 62 deletions(-)
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Nov 6, 2021, 12:43:47 PM11/6/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, golang-co...@googlegroups.com
Filippo Valsorda submitted this change.
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Approvals:
Filippo Valsorda: Looks good to me, approved
Roland Shoemaker: Trusted
crypto/x509: verification with system and custom roots
Make system cert pools special, such that when one has extra roots
added to it we run verifications twice, once using the platform
verifier, if available, and once using the Go verifier, merging the
results.
This change re-enables SystemCertPool on Windows, but explicitly does
not return anything from CertPool.Subjects (which matches the behavior
of macOS). CertPool.Subjects is also marked deprecated.
Fixes #46287
Fixes #16736
Change-Id: Idc1843f715ae2b2d0108e55ab942c287181a340a
Reviewed-on: https://go-review.googlesource.com/c/go/+/353589
Reviewed-by: Filippo Valsorda <fil...@golang.org>
Trust: Roland Shoemaker <rol...@golang.org>
---
M src/crypto/x509/cert_pool.go
A src/crypto/x509/root_windows_test.go
M src/crypto/x509/root_windows.go
M src/crypto/x509/root_darwin.go
M src/crypto/x509/verify.go
A src/crypto/x509/hybrid_pool_test.go
6 files changed, 253 insertions(+), 62 deletions(-)
diff --git a/src/crypto/x509/cert_pool.go b/src/crypto/x509/cert_pool.go
index d760dc1..873ffee 100644
--- a/src/crypto/x509/cert_pool.go
+++ b/src/crypto/x509/cert_pool.go
@@ -8,8 +8,6 @@
"bytes"
"crypto/sha256"
"encoding/pem"
- "errors"
- "runtime"
"sync"
)
@@ -29,6 +27,12 @@
// call getCert and otherwise negate savings from lazy getCert
// funcs).
haveSum map[sum224]bool
+
+ // systemPool indicates whether this is a special pool derived from the
+ // system roots. If it includes additional roots, it requires doing two
+ // verifications, one using the roots provided by the caller, and one using
+ // the system platform verifier.
+ systemPool bool
}
// lazyCert is minimal metadata about a Cert and a func to retrieve it
@@ -75,9 +79,10 @@
func (s *CertPool) copy() *CertPool {
p := &CertPool{
- byName: make(map[string][]int, len(s.byName)),
- lazyCerts: make([]lazyCert, len(s.lazyCerts)),
- haveSum: make(map[sum224]bool, len(s.haveSum)),
+ byName: make(map[string][]int, len(s.byName)),
+ lazyCerts: make([]lazyCert, len(s.lazyCerts)),
+ haveSum: make(map[sum224]bool, len(s.haveSum)),
+ systemPool: s.systemPool,
}
for k, v := range s.byName {
indexes := make([]int, len(v))
@@ -103,15 +108,6 @@
//
// New changes in the system cert pool might not be reflected in subsequent calls.
func SystemCertPool() (*CertPool, error) {
- if runtime.GOOS == "windows" {
- // Issue 16736, 18609:
- return nil, errors.New("crypto/x509: system root pool is not available on Windows")
- } else if runtime.GOOS == "darwin" {
- return nil, errors.New("crypto/x509: system root pool is not available on macOS")
- } else if runtime.GOOS == "ios" {
- return nil, errors.New("crypto/x509: system root pool is not available on iOS")
- }
-
if sysRoots := systemRootsPool(); sysRoots != nil {
return sysRoots.copy(), nil
}
@@ -243,6 +239,9 @@
// Subjects returns a list of the DER-encoded subjects of
// all of the certificates in the pool.
+//
+// Deprecated: if s was returned by SystemCertPool, Subjects
+// will not include the system roots.
func (s *CertPool) Subjects() [][]byte {
res := make([][]byte, s.len())
for i, lc := range s.lazyCerts {
diff --git a/src/crypto/x509/hybrid_pool_test.go b/src/crypto/x509/hybrid_pool_test.go
new file mode 100644
index 0000000..d4dd9d5
--- /dev/null
+++ b/src/crypto/x509/hybrid_pool_test.go
@@ -0,0 +1,95 @@
+// Copyright 2011 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.
+
+package x509_test
+
+import (
+ "crypto/ecdsa"
+ "crypto/elliptic"
+ "crypto/rand"
+ "crypto/tls"
+ "crypto/x509"
+ "crypto/x509/pkix"
+ "internal/testenv"
+ "math/big"
+ "runtime"
+ "testing"
+ "time"
+)
+
+func TestHybridPool(t *testing.T) {
+ if !(runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios") {
+ t.Skipf("platform verifier not available on %s", runtime.GOOS)
+ }
+ if !testenv.HasExternalNetwork() {
+ t.Skip()
+ }
+
+ // Get the google.com chain, which should be valid on all platforms we
+ // are testing
+ c, err := tls.Dial("tcp", "google.com:443", &tls.Config{InsecureSkipVerify: true})
+ if err != nil {
+ t.Fatalf("tls connection failed: %s", err)
+ }
+ googChain := c.ConnectionState().PeerCertificates
+
+ rootTmpl := &x509.Certificate{
+ SerialNumber: big.NewInt(1),
+ Subject: pkix.Name{CommonName: "Go test root"},
+ IsCA: true,
+ BasicConstraintsValid: true,
+ NotBefore: time.Now().Add(-time.Hour),
+ NotAfter: time.Now().Add(time.Hour * 10),
+ }
+ k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+ if err != nil {
+ t.Fatalf("failed to generate test key: %s", err)
+ }
+ rootDER, err := x509.CreateCertificate(rand.Reader, rootTmpl, rootTmpl, k.Public(), k)
+ if err != nil {
+ t.Fatalf("failed to create test cert: %s", err)
+ }
+ root, err := x509.ParseCertificate(rootDER)
+ if err != nil {
+ t.Fatalf("failed to parse test cert: %s", err)
+ }
+
+ pool, err := x509.SystemCertPool()
+ if err != nil {
+ t.Fatalf("SystemCertPool failed: %s", err)
+ }
+ opts := x509.VerifyOptions{Roots: pool}
+
+ _, err = googChain[0].Verify(opts)
+ if err != nil {
+ t.Fatalf("verification failed for google.com chain (empty pool): %s", err)
+ }
+
+ pool.AddCert(root)
+
+ _, err = googChain[0].Verify(opts)
+ if err != nil {
+ t.Fatalf("verification failed for google.com chain (hybrid pool): %s", err)
+ }
+
+ certTmpl := &x509.Certificate{
+ SerialNumber: big.NewInt(1),
+ NotBefore: time.Now().Add(-time.Hour),
+ NotAfter: time.Now().Add(time.Hour * 10),
+ DNSNames: []string{"example.com"},
+ }
+ certDER, err := x509.CreateCertificate(rand.Reader, certTmpl, rootTmpl, k.Public(), k)
+ if err != nil {
+ t.Fatalf("failed to create test cert: %s", err)
+ }
+ cert, err := x509.ParseCertificate(certDER)
+ if err != nil {
+ t.Fatalf("failed to parse test cert: %s", err)
+ }
+
+ _, err = cert.Verify(opts)
+ if err != nil {
+ t.Fatalf("verification failed for custom chain (hybrid pool): %s", err)
+ }
+}
diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go
index 7bc6ce0..a7ff1e7 100644
--- a/src/crypto/x509/root_darwin.go
+++ b/src/crypto/x509/root_darwin.go
@@ -107,5 +107,5 @@
}
func loadSystemRoots() (*CertPool, error) {
- return nil, nil
+ return &CertPool{systemPool: true}, nil
}
diff --git a/src/crypto/x509/root_windows.go b/src/crypto/x509/root_windows.go
index f77ea3a..d65d876 100644
--- a/src/crypto/x509/root_windows.go
+++ b/src/crypto/x509/root_windows.go
@@ -10,6 +10,10 @@
"unsafe"
)
+func loadSystemRoots() (*CertPool, error) {
+ return &CertPool{systemPool: true}, nil
+}
+
// Creates a new *syscall.CertContext representing the leaf certificate in an in-memory
// certificate store containing itself and all of the intermediate certificates specified
// in the opts.Intermediates CertPool.
@@ -271,47 +275,3 @@
return chains, nil
}
-
-func loadSystemRoots() (*CertPool, error) {
-}
diff --git a/src/crypto/x509/root_windows_test.go b/src/crypto/x509/root_windows_test.go
new file mode 100644
index 0000000..ce6d927
--- /dev/null
+++ b/src/crypto/x509/root_windows_test.go
@@ -0,0 +1,102 @@
+// Copyright 2021 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.
+
+package x509_test
+
+import (
+ "crypto/tls"
+ "crypto/x509"
+ "internal/testenv"
+ "testing"
+ "time"
+)
+
+func TestPlatformVerifier(t *testing.T) {
+ if !testenv.HasExternalNetwork() {
+ t.Skip()
+ }
+
+ getChain := func(host string) []*x509.Certificate {
+ t.Helper()
+ c, err := tls.Dial("tcp", host+":443", &tls.Config{InsecureSkipVerify: true})
+ if err != nil {
+ t.Fatalf("tls connection failed: %s", err)
+ }
+ return c.ConnectionState().PeerCertificates
+ }
+
+ tests := []struct {
+ name string
+ host string
+ verifyName string
+ verifyTime time.Time
+ expectedErr string
+ }{
+ {
+ // whatever google.com serves should, hopefully, be trusted
+ name: "valid chain",
+ host: "google.com",
+ },
+ {
+ name: "expired leaf",
+ host: "expired.badssl.com",
+ expectedErr: "x509: certificate has expired or is not yet valid: ",
+ },
+ {
+ name: "wrong host for leaf",
+ host: "wrong.host.badssl.com",
+ verifyName: "wrong.host.badssl.com",
+ expectedErr: "x509: certificate is valid for *.badssl.com, badssl.com, not wrong.host.badssl.com",
+ },
+ {
+ name: "self-signed leaf",
+ host: "self-signed.badssl.com",
+ expectedErr: "x509: certificate signed by unknown authority",
+ },
+ {
+ name: "untrusted root",
+ host: "untrusted-root.badssl.com",
+ expectedErr: "x509: certificate signed by unknown authority",
+ },
+ {
+ name: "expired leaf (custom time)",
+ host: "google.com",
+ verifyTime: time.Time{}.Add(time.Hour),
+ expectedErr: "x509: certificate has expired or is not yet valid: ",
+ },
+ {
+ name: "valid chain (custom time)",
+ host: "google.com",
+ verifyTime: time.Now(),
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ chain := getChain(tc.host)
+ var opts x509.VerifyOptions
+ if len(chain) > 1 {
+ opts.Intermediates = x509.NewCertPool()
+ for _, c := range chain[1:] {
+ opts.Intermediates.AddCert(c)
+ }
+ }
+ if tc.verifyName != "" {
+ opts.DNSName = tc.verifyName
+ }
+ if !tc.verifyTime.IsZero() {
+ opts.CurrentTime = tc.verifyTime
+ }
+
+ _, err := chain[0].Verify(opts)
+ if err != nil && tc.expectedErr == "" {
+ t.Errorf("unexpected verification error: %s", err)
+ } else if err != nil && err.Error() != tc.expectedErr {
+ t.Errorf("unexpected verification error: got %q, want %q", err.Error(), tc.expectedErr)
+ } else if err == nil && tc.expectedErr != "" {
+ t.Errorf("unexpected verification success: want %q", tc.expectedErr)
+ }
+ })
+ }
+}
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 59852d9..1562ee5 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -741,9 +741,20 @@
}
}
- // Use platform verifiers, where available
- if opts.Roots == nil && (runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios") {
- return c.systemVerify(&opts)
+ // Use platform verifiers, where available, if Roots is from SystemCertPool.
+ if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" {
+ if opts.Roots == nil {
+ return c.systemVerify(&opts)
+ }
+ if opts.Roots != nil && opts.Roots.systemPool {
+ platformChains, err := c.systemVerify(&opts)
+ // If the platform verifier succeeded, or there are no additional
+ // roots, return the platform verifier result. Otherwise, continue
+ // with the Go verifier.
+ if err == nil || opts.Roots.len() == 0 {
+ return platformChains, err
+ }
+ }
}
if opts.Roots == nil {
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Nov 8, 2021, 11:04:33 AM11/8/21
to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, Bryan C. Mills, Go Bot, golang-co...@googlegroups.com
1 comment:
Patchset:
It looks like this is causing a merge conflict in rolling back the macOS breakage (CL 362242 / #49435). If that can't be fixed quickly by other means, this CL may need to be rolled back too.
To view, visit change 353589. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages