[go] crypto/x509: simplify candidate chain filtering

2 views
Skip to first unread message

Roland Shoemaker (Gerrit)

unread,
11:58 AM (11 hours ago) 11:58 AM
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Roland Shoemaker has uploaded the change for review

Commit message

crypto/x509: simplify candidate chain filtering

Use slices.DeleteFunc to remove chains with invalid policies and
incompatible key usage, instead of iterating over the chains and
reconstructing the slice.
Change-Id: I8ad2bc1ac2469d0d18b2c090e3d4f702b1b577cb

Change diff

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 7cc0fb2..b590adf 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -17,6 +17,7 @@
 	"net/url"
 	"reflect"
 	"runtime"
+	"slices"
 	"strings"
 	"time"
 	"unicode/utf8"
@@ -777,7 +778,7 @@
 // Certificates other than c in the returned chains should not be modified.
 //
 // WARNING: this function doesn't do any revocation checking.
-func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err error) {
+func (c *Certificate) Verify(opts VerifyOptions) ([][]*Certificate, error) {
 	// Platform-specific verification needs the ASN.1 contents so
 	// this makes the behavior consistent across platforms.
 	if len(c.Raw) == 0 {
@@ -819,15 +820,15 @@
 		}
 	}
 
-	err = c.isValid(leafCertificate, nil, &opts)
+	err := c.isValid(leafCertificate, nil, &opts)
 	if err != nil {
-		return
+		return nil, err
 	}
 
 	if len(opts.DNSName) > 0 {
 		err = c.VerifyHostname(opts.DNSName)
 		if err != nil {
-			return
+			return nil, err
 		}
 	}
 
@@ -841,26 +842,10 @@
 		}
 	}
 
-	chains = make([][]*Certificate, 0, len(candidateChains))
-
-	var invalidPoliciesChains int
-	for _, candidate := range candidateChains {
-		if !policiesValid(candidate, opts) {
-			invalidPoliciesChains++
-			continue
-		}
-		chains = append(chains, candidate)
-	}
-
-	if len(chains) == 0 {
-		return nil, CertificateInvalidError{c, NoValidChains, "all candidate chains have invalid policies"}
-	}
-
+	anyKeyUsage := false
 	for _, eku := range opts.KeyUsages {
 		if eku == ExtKeyUsageAny {
-			// If any key usage is acceptable, no need to check the chain for
-			// key usages.
-			return chains, nil
+			anyKeyUsage = true
 		}
 	}
 
@@ -868,34 +853,38 @@
 		opts.KeyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
 	}
 
-	candidateChains = chains
-	chains = chains[:0]
-
+	var invalidPoliciesChains int
 	var incompatibleKeyUsageChains int
-	for _, candidate := range candidateChains {
-		if !checkChainForKeyUsage(candidate, opts.KeyUsages) {
-			incompatibleKeyUsageChains++
-			continue
+	candidateChains = slices.DeleteFunc(candidateChains, func(chain []*Certificate) bool {
+		if !policiesValid(chain, opts) {
+			invalidPoliciesChains++
+			return true
 		}
-		chains = append(chains, candidate)
-	}
+		// If any key usage is acceptable, no need to check the chain for
+		// key usages.
+		if !anyKeyUsage && !checkChainForKeyUsage(chain, opts.KeyUsages) {
+			incompatibleKeyUsageChains++
+			return true
+		}
+		return false
+	})
 
-	if len(chains) == 0 {
+	if len(candidateChains) == 0 {
 		var details []string
 		if incompatibleKeyUsageChains > 0 {
 			if invalidPoliciesChains == 0 {
 				return nil, CertificateInvalidError{c, IncompatibleUsage, ""}
 			}
-			details = append(details, fmt.Sprintf("%d chains with incompatible key usage", incompatibleKeyUsageChains))
+			details = append(details, fmt.Sprintf("%d candidate chains with incompatible key usage", incompatibleKeyUsageChains))
 		}
 		if invalidPoliciesChains > 0 {
-			details = append(details, fmt.Sprintf("%d chains with invalid policies", invalidPoliciesChains))
+			details = append(details, fmt.Sprintf("%d candidate chains with invalid policies", invalidPoliciesChains))
 		}
 		err = CertificateInvalidError{c, NoValidChains, strings.Join(details, ", ")}
 		return nil, err
 	}
 
-	return chains, nil
+	return candidateChains, nil
 }
 
 func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate {
diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 7991f49..30c6173 100644
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -3030,7 +3030,7 @@
 	testOID3 := mustNewOIDFromInts([]uint64{1, 2, 840, 113554, 4, 1, 72585, 2, 3})
 	root, intermediate, leaf := loadTestCert(t, "testdata/policy_root.pem"), loadTestCert(t, "testdata/policy_intermediate_require.pem"), loadTestCert(t, "testdata/policy_leaf.pem")
 
-	expectedErr := "x509: no valid chains built: all candidate chains have invalid policies"
+	expectedErr := "x509: no valid chains built: 1 candidate chains with invalid policies"
 
 	roots, intermediates := NewCertPool(), NewCertPool()
 	roots.AddCert(root)

Change information

Files:
  • M src/crypto/x509/verify.go
  • M src/crypto/x509/verify_test.go
Change size: M
Delta: 2 files changed, 25 insertions(+), 36 deletions(-)
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newchange
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I8ad2bc1ac2469d0d18b2c090e3d4f702b1b577cb
Gerrit-Change-Number: 708415
Gerrit-PatchSet: 1
Gerrit-Owner: Roland Shoemaker <rol...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Roland Shoemaker (Gerrit)

unread,
11:59 AM (11 hours ago) 11:59 AM
to goph...@pubsubhelper.golang.org, Daniel McCarney, golang-co...@googlegroups.com
Attention needed from Daniel McCarney and Filippo Valsorda

Roland Shoemaker voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Daniel McCarney
  • Filippo Valsorda
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: I8ad2bc1ac2469d0d18b2c090e3d4f702b1b577cb
Gerrit-Change-Number: 708415
Gerrit-PatchSet: 1
Gerrit-Owner: Roland Shoemaker <rol...@golang.org>
Gerrit-Reviewer: Daniel McCarney <dan...@binaryparadox.net>
Gerrit-Reviewer: Filippo Valsorda <fil...@golang.org>
Gerrit-Reviewer: Roland Shoemaker <rol...@golang.org>
Gerrit-Attention: Filippo Valsorda <fil...@golang.org>
Gerrit-Attention: Daniel McCarney <dan...@binaryparadox.net>
Gerrit-Comment-Date: Wed, 01 Oct 2025 15:59:01 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
unsatisfied_requirement
satisfied_requirement
open
diffy

Ingo Oeser (Gerrit)

unread,
5:04 PM (5 hours ago) 5:04 PM
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Go LUCI, Daniel McCarney, golang-co...@googlegroups.com
Attention needed from Daniel McCarney, Filippo Valsorda and Roland Shoemaker

Ingo Oeser added 2 comments

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Ingo Oeser . resolved

Suggestion to fix tiny performance regression.

File src/crypto/x509/verify.go
Line 848, Patchset 1 (Latest): anyKeyUsage = true
Ingo Oeser . unresolved
Please add a break after this, as the result won't change anymore. That was the purpose of the return here I guess.
```suggestion
			anyKeyUsage = true
			break
```
Open in Gerrit

Related details

Attention is currently required from:
  • Daniel McCarney
  • Filippo Valsorda
  • Roland Shoemaker
Submit Requirements:
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    • requirement satisfiedTryBots-Pass
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Change-Id: I8ad2bc1ac2469d0d18b2c090e3d4f702b1b577cb
    Gerrit-Change-Number: 708415
    Gerrit-PatchSet: 1
    Gerrit-Owner: Roland Shoemaker <rol...@golang.org>
    Gerrit-Reviewer: Daniel McCarney <dan...@binaryparadox.net>
    Gerrit-Reviewer: Filippo Valsorda <fil...@golang.org>
    Gerrit-Reviewer: Roland Shoemaker <rol...@golang.org>
    Gerrit-CC: Ingo Oeser <night...@googlemail.com>
    Gerrit-Attention: Roland Shoemaker <rol...@golang.org>
    Gerrit-Attention: Filippo Valsorda <fil...@golang.org>
    Gerrit-Attention: Daniel McCarney <dan...@binaryparadox.net>
    Gerrit-Comment-Date: Wed, 01 Oct 2025 21:04:03 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    satisfied_requirement
    open
    diffy
    Reply all
    Reply to author
    Forward
    0 new messages