[go] crypto/x509: remove GODEBUG=x509ignoreCN=0 flag
632 views
Skip to first unread message
Filippo Valsorda (Gerrit)
Apr 29, 2021, 2:00:17 PM4/29/21
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda would like Roland Shoemaker to review this change.
crypto/x509: remove GODEBUG=x509ignoreCN=0 flag
Common Name and NameConstraintsWithoutSANs are no more.
Fixes #24151 ᕕ(ᐛ)ᕗ
Change-Id: I15058f2a64f981c69e9ee620d3fab00f68967e49
---
M src/crypto/x509/name_constraints_test.go
M src/crypto/x509/verify.go
M src/crypto/x509/verify_test.go
3 files changed, 9 insertions(+), 151 deletions(-)
diff --git a/src/crypto/x509/name_constraints_test.go b/src/crypto/x509/name_constraints_test.go
index 3826c82..c59a7dc 100644
--- a/src/crypto/x509/name_constraints_test.go
+++ b/src/crypto/x509/name_constraints_test.go
@@ -614,7 +614,8 @@
},
},
- // #30: without SANs, a certificate with a CN is rejected in a constrained chain.
+ // #30: without SANs, a certificate with a CN is still accepted in a
+ // constrained chain, since we ignore the CN in VerifyHostname.
{
roots: []constraintsSpec{
{
@@ -630,7 +631,6 @@
sans: []string{},
cn: "foo.com",
},
- expectedError: "leaf doesn't have a SAN extension",
},
// #31: IPv6 addresses work in constraints: roots can permit them as
@@ -1595,26 +1595,6 @@
cn: "foo.bar",
},
},
-
- // #85: without SANs, a certificate with a valid CN is accepted in a
- // constrained chain if x509ignoreCN is set.
- {
- roots: []constraintsSpec{
- {
- ok: []string{"dns:foo.com", "dns:.foo.com"},
- },
- },
- intermediates: [][]constraintsSpec{
- {
- {},
- },
- },
- leaf: leafSpec{
- sans: []string{},
- cn: "foo.com",
- },
- ignoreCN: true,
- },
}
func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.PrivateKey, parent *Certificate, parentKey *ecdsa.PrivateKey) (*Certificate, error) {
@@ -1865,10 +1845,6 @@
}
func TestConstraintCases(t *testing.T) {
- defer func(savedIgnoreCN bool) {
- ignoreCN = savedIgnoreCN
- }(ignoreCN)
-
privateKeys := sync.Pool{
New: func() interface{} {
priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
@@ -1960,7 +1936,6 @@
}
}
- ignoreCN = test.ignoreCN
verifyOpts := VerifyOptions{
Roots: rootPool,
Intermediates: intermediatePool,
@@ -1999,7 +1974,6 @@
for _, key := range keys {
privateKeys.Put(key)
}
- keys = keys[:0]
}
}
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 2432d9b..7a3c143 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -10,7 +10,6 @@
"fmt"
"net"
"net/url"
- "os"
"reflect"
"runtime"
"strings"
@@ -18,9 +17,6 @@
"unicode/utf8"
)
-// ignoreCN disables interpreting Common Name as a hostname. See issue 24151.
-var ignoreCN = !strings.Contains(os.Getenv("GODEBUG"), "x509ignoreCN=0")
-
type InvalidReason int
const (
@@ -43,14 +39,7 @@
// NameMismatch results when the subject name of a parent certificate
// does not match the issuer name in the child.
NameMismatch
- // NameConstraintsWithoutSANs results when a leaf certificate doesn't
- // contain a Subject Alternative Name extension, but a CA certificate
- // contains name constraints, and the Common Name can be interpreted as
- // a hostname.
- //
- // This error is only returned when legacy Common Name matching is enabled
- // by setting the GODEBUG environment variable to "x509ignoreCN=1". This
- // setting might be removed in the future.
+ // NameConstraintsWithoutSANs is a legacy error and is not returned anymore.
NameConstraintsWithoutSANs
// UnconstrainedName results when a CA certificate contains permitted
// name constraints, but leaf certificate contains a name of an
@@ -110,15 +99,7 @@
c := h.Certificate
if !c.hasSANExtension() && matchHostnames(c.Subject.CommonName, h.Host) {
- if !ignoreCN && !validHostnamePattern(c.Subject.CommonName) {
- // This would have validated, if it weren't for the validHostname check on Common Name.
- return "x509: Common Name is not a valid hostname: " + c.Subject.CommonName
- }
- if ignoreCN && validHostnamePattern(c.Subject.CommonName) {
- // This would have validated if x509ignoreCN=0 were set.
- return "x509: certificate relies on legacy Common Name field, " +
- "use SANs or temporarily enable Common Name matching with GODEBUG=x509ignoreCN=0"
- }
+ return "x509: certificate relies on legacy Common Name field, use SANs instead"
}
var valid string
@@ -134,11 +115,7 @@
valid += san.String()
}
} else {
- if c.commonNameAsHostname() {
- valid = c.Subject.CommonName
- } else {
- valid = strings.Join(c.DNSNames, ", ")
- }
+ valid = strings.Join(c.DNSNames, ", ")
}
if len(valid) == 0 {
@@ -620,15 +597,8 @@
leaf = currentChain[0]
}
- checkNameConstraints := (certType == intermediateCertificate || certType == rootCertificate) && c.hasNameConstraints()
- if checkNameConstraints && leaf.commonNameAsHostname() {
- // This is the deprecated, legacy case of depending on the commonName as
- // a hostname. We don't enforce name constraints against the CN, but
- // VerifyHostname will look for hostnames in there if there are no SANs.
- // In order to ensure VerifyHostname will not accept an unchecked name,
- // return an error here.
- return CertificateInvalidError{c, NameConstraintsWithoutSANs, ""}
- } else if checkNameConstraints && leaf.hasSANExtension() {
+ if (certType == intermediateCertificate || certType == rootCertificate) &&
+ c.hasNameConstraints() && leaf.hasSANExtension() {
err := forEachSAN(leaf.getSANExtension(), func(tag int, data []byte) error {
switch tag {
case nameTypeEmail:
@@ -960,18 +930,6 @@
return true
}
-// commonNameAsHostname reports whether the Common Name field should be
-// considered the hostname that the certificate is valid for. This is a legacy
-// behavior, disabled by default or if the Subject Alt Name extension is present.
-//
-// It applies the strict validHostname check to the Common Name field, so that
-// certificates without SANs can still be validated against CAs with name
-// constraints if there is no risk the CN would be matched as a hostname.
-// See NameConstraintsWithoutSANs and issue 24151.
-func (c *Certificate) commonNameAsHostname() bool {
- return !ignoreCN && !c.hasSANExtension() && validHostnamePattern(c.Subject.CommonName)
-}
-
func matchExactly(hostA, hostB string) bool {
if hostA == "" || hostA == "." || hostB == "" || hostB == "." {
return false
@@ -1046,10 +1004,7 @@
// against the DNSNames field. If the names are valid hostnames, the certificate
// fields can have a wildcard as the left-most label.
//
-// The legacy Common Name field is ignored unless it's a valid hostname, the
-// certificate doesn't have any Subject Alternative Names, and the GODEBUG
-// environment variable is set to "x509ignoreCN=0". Support for Common Name is
-// deprecated will be entirely removed in the future.
+// Note that the legacy Common Name field is ignored.
func (c *Certificate) VerifyHostname(h string) error {
// IP addresses may be written in [ ].
candidateIP := h
@@ -1067,15 +1022,10 @@
return HostnameError{c, candidateIP}
}
- names := c.DNSNames
- if c.commonNameAsHostname() {
- names = []string{c.Subject.CommonName}
- }
-
candidateName := toLowerCaseASCII(h) // Save allocations inside the loop.
validCandidateName := validHostnameInput(candidateName)
- for _, match := range names {
+ for _, match := range c.DNSNames {
// Ideally, we'd only match valid hostnames according to RFC 6125 like
// browsers (more or less) do, but in practice Go is used in a wider
// array of contexts and can't even assume DNS resolution. Instead,
diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 8e0a7be..9954a67 100644
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -30,7 +30,6 @@
systemSkip bool
systemLax bool
keyUsages []ExtKeyUsage
- ignoreCN bool
errorCallback func(*testing.T, error)
expectedChains [][]string
@@ -297,8 +296,6 @@
errorCallback: expectNotAuthorizedError,
},
{
- // If any SAN extension is present (even one without any DNS
- // names), the CN should be ignored.
name: "IgnoreCNWithSANs",
leaf: ignoreCNWithSANLeaf,
dnsName: "foo.example.com",
@@ -325,7 +322,6 @@
// verify error.
name: "CriticalExtLeaf",
leaf: criticalExtLeafWithExt,
- dnsName: "example.com",
intermediates: []string{criticalExtIntermediate},
roots: []string{criticalExtRoot},
currentTime: 1486684488,
@@ -338,7 +334,6 @@
// cause a verify error.
name: "CriticalExtIntermediate",
leaf: criticalExtLeaf,
- dnsName: "example.com",
intermediates: []string{criticalExtIntermediateWithExt},
roots: []string{criticalExtRoot},
currentTime: 1486684488,
@@ -347,18 +342,6 @@
errorCallback: expectUnhandledCriticalExtension,
},
{
- // Test that invalid CN are ignored.
- name: "InvalidCN",
- leaf: invalidCNWithoutSAN,
- dnsName: "foo,invalid",
- roots: []string{invalidCNRoot},
- currentTime: 1540000000,
- systemSkip: true, // does not chain to a system root
-
- errorCallback: expectHostnameError("Common Name is not a valid hostname"),
- },
- {
- // Test that valid CN are respected.
name: "ValidCN",
leaf: validCNWithoutSAN,
dnsName: "foo.example.com",
@@ -366,42 +349,6 @@
currentTime: 1540000000,
systemSkip: true, // does not chain to a system root
- expectedChains: [][]string{
- {"foo.example.com", "Test root"},
- },
- },
- // Replicate CN tests with ignoreCN = true
- {
- name: "IgnoreCNWithSANs/ignoreCN",
- leaf: ignoreCNWithSANLeaf,
- dnsName: "foo.example.com",
- roots: []string{ignoreCNWithSANRoot},
- currentTime: 1486684488,
- systemSkip: true, // does not chain to a system root
- ignoreCN: true,
-
- errorCallback: expectHostnameError("certificate is not valid for any names"),
- },
- {
- name: "InvalidCN/ignoreCN",
- leaf: invalidCNWithoutSAN,
- dnsName: "foo,invalid",
- roots: []string{invalidCNRoot},
- currentTime: 1540000000,
- systemSkip: true, // does not chain to a system root
- ignoreCN: true,
-
- errorCallback: expectHostnameError("certificate is not valid for any names"),
- },
- {
- name: "ValidCN/ignoreCN",
- leaf: validCNWithoutSAN,
- dnsName: "foo.example.com",
- roots: []string{invalidCNRoot},
- currentTime: 1540000000,
- systemSkip: true, // does not chain to a system root
- ignoreCN: true,
-
errorCallback: expectHostnameError("certificate relies on legacy Common Name field"),
},
{
@@ -503,9 +450,6 @@
}
func testVerify(t *testing.T, test verifyTest, useSystemRoots bool) {
- defer func(savedIgnoreCN bool) { ignoreCN = savedIgnoreCN }(ignoreCN)
-
- ignoreCN = test.ignoreCN
opts := VerifyOptions{
Intermediates: NewCertPool(),
DNSName: test.dnsName,
@@ -1589,16 +1533,6 @@
XzZZl0eW/ugCICgOfXeZ2GGy3wIC0352BaC3a8r5AAb2XSGNe+e9wNN6
-----END CERTIFICATE-----`
-const invalidCNWithoutSAN = `-----BEGIN CERTIFICATE-----
-MIIBJDCBywIUB7q8t9mrDAL+UB1OFaMN5BEWFKIwCgYIKoZIzj0EAwIwFDESMBAG
-A1UECwwJVGVzdCByb290MB4XDTE4MDcxMTE4MzUyMVoXDTI4MDcwODE4MzUyMVow
-FjEUMBIGA1UEAwwLZm9vLGludmFsaWQwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNC
-AASnpnwiM6dHfwiTLV9hNS7aRWd28pdzGLABEkoa1bdvQTy7BWn0Bl3/6yunhQtM
-90VOgUB6qcYdu7rZuSazylCQMAoGCCqGSM49BAMCA0gAMEUCIQCFlnW2cjxnEqB/
-hgSB0t3IZ1DXX4XAVFT85mtFCJPTKgIgYIY+1iimTtrdbpWJzAB2eBwDgIWmWgvr
-xfOcLt/vbvo=
------END CERTIFICATE-----`
-
const validCNWithoutSAN = `-----BEGIN CERTIFICATE-----
MIIBJzCBzwIUB7q8t9mrDAL+UB1OFaMN5BEWFKQwCgYIKoZIzj0EAwIwFDESMBAG
A1UECwwJVGVzdCByb290MB4XDTE4MDcxMTE4NDcyNFoXDTI4MDcwODE4NDcyNFow
To view, visit change 315209. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 29, 2021, 2:57:11 PM4/29/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 1:Code-Review +2
3 comments:
Commit Message:
Patch Set #1, Line 1:
Parent: be28caf0 (cmd/compile/internal/types2: respect IgnoreFuncBodies for function literals)(not really sure where to put this comment) Should this include a release note?
Patchset:
🎉🎉🎉
File src/crypto/x509/verify.go:
Patch Set #1, Line 42:
not returned anymorenit: "no longer returned" might scan a bit better.
To view, visit change 315209. To unsubscribe, or for help writing mail filters, visit settings.
Katie Hockman (Gerrit)
May 4, 2021, 9:41:18 AM5/4/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Roland Shoemaker, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda.
Patch set 1:Code-Review +2Trust +1
1 comment:
Patchset:
\o/
To view, visit change 315209. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
May 8, 2021, 1:11:44 AM5/8/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Katie Hockman, Roland Shoemaker, Go Bot, golang-co...@googlegroups.com
Patch set 2:Trust +1
2 comments:
Commit Message:
Patch Set #1, Line 1:
Parent: be28caf0 (cmd/compile/internal/types2: respect IgnoreFuncBodies for function literals)(not really sure where to put this comment) Should this include a release note?
Yeah, looks like we'll have to do a big round of release notes anyway.
RELNOTE=yes
File src/crypto/x509/verify.go:
Patch Set #1, Line 42:
not returned anymorenit: "no longer returned" might scan a bit better.
Better, done.
To view, visit change 315209. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
May 8, 2021, 1:12:12 AM5/8/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Katie Hockman, Roland Shoemaker, Go Bot, golang-co...@googlegroups.com
Filippo Valsorda submitted this change.
Approvals:
Roland Shoemaker: Looks good to me, approved
Katie Hockman: Looks good to me, approved; Trusted
Filippo Valsorda: Trusted
crypto/x509: remove GODEBUG=x509ignoreCN=0 flag
Common Name and NameConstraintsWithoutSANs are no more.
Fixes #24151 ᕕ(ᐛ)ᕗ
Change-Id: I15058f2a64f981c69e9ee620d3fab00f68967e49
Reviewed-on: https://go-review.googlesource.com/c/go/+/315209
Trust: Filippo Valsorda <fil...@golang.org>
Trust: Katie Hockman <ka...@golang.org>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
index 2432d9b..9ef1146 100644+ // NameConstraintsWithoutSANs is a legacy error and is no longer returned.
1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
The name of the file: src/crypto/x509/verify.go
Insertions: 1, Deletions: 1.
```
@@ -41:42, +41:42 @@
- // NameConstraintsWithoutSANs is a legacy error and is not returned anymore.
+ // NameConstraintsWithoutSANs is a legacy error and is no longer returned.
```
To view, visit change 315209. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages