crypto/x509: ignore Common Name by default

Common Name has been deprecated for 20 years, and has horrible interactions with Name Constraints. The browsers managed to drop it last year, let's try flicking the switch to disabled by default. Return helpful errors for things that would get unbroken by flipping the switch back with the environment variable. Had to refresh a test certificate that was too old to have SANs. Updates #24151 Change-Id: I2ab78577fd936ba67969d3417284dbe46e4ae02f Reviewed-on: https://go-review.googlesource.com/c/go/+/231379 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org>
golang · May 8, 2020 · d65e1b2 · d65e1b2
1 parent 9d1e120
commit d65e1b2
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 62 deletions.
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
@@ -19,7 +19,7 @@ import (
 )
 
 // ignoreCN disables interpreting Common Name as a hostname. See issue 24151.
-var ignoreCN = strings.Contains(os.Getenv("GODEBUG"), "x509ignoreCN=1")
+var ignoreCN = !strings.Contains(os.Getenv("GODEBUG"), "x509ignoreCN=0")
 
 type InvalidReason int
 
@@ -48,9 +48,9 @@ const (
 	// contains name constraints, and the Common Name can be interpreted as
 	// a hostname.
 	//
-	// You can avoid this error by setting the experimental GODEBUG environment
-	// variable to "x509ignoreCN=1", disabling Common Name matching entirely.
-	// This behavior might become the default in the future.
+	// 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
 	// UnconstrainedName results when a CA certificate contains permitted
 	// name constraints, but leaf certificate contains a name of an
@@ -109,10 +109,16 @@ type HostnameError struct {
 func (h HostnameError) Error() string {
 	c := h.Certificate
 
-	if !c.hasSANExtension() && !validHostname(c.Subject.CommonName) &&
-		matchHostnames(c.Subject.CommonName, h.Host) {
-		// 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 !c.hasSANExtension() && matchHostnames(c.Subject.CommonName, h.Host) {
+		if !ignoreCN && !validHostname(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 && validHostname(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"
+		}
 	}
 
 	var valid string
@@ -944,7 +950,7 @@ func validHostname(host string) bool {
 
 // commonNameAsHostname reports whether the Common Name field should be
 // considered the hostname that the certificate is valid for. This is a legacy
-// behavior, disabled if the Subject Alt Name extension is present.
+// 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
@@ -1028,10 +1034,10 @@ func toLowerCaseASCII(in string) string {
 // against the DNSNames field. If the names are valid hostnames, the certificate
 // fields can have a wildcard as the left-most label.
 //
-// If the Common Name field is a valid hostname, and the certificate doesn't
-// have any Subject Alternative Names, the name will also be checked against the
-// Common Name. This legacy behavior can be disabled by setting the GODEBUG
-// environment variable to "x509ignoreCN=1" and might be removed in the future.
+// 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.
 func (c *Certificate) VerifyHostname(h string) error {
 	// IP addresses may be written in [ ].
 	candidateIP := h

diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
@@ -374,7 +374,7 @@ var verifyTests = []verifyTest{
 		systemSkip:  true,
 		ignoreCN:    true,
 
-		errorCallback: expectHostnameError("Common Name is not a valid hostname"),
+		errorCallback: expectHostnameError("not valid for any names"),
 	},
 	{
 		leaf:        validCNWithoutSAN,
@@ -384,7 +384,7 @@ var verifyTests = []verifyTest{
 		systemSkip:  true,
 		ignoreCN:    true,
 
-		errorCallback: expectHostnameError("not valid for any names"),
+		errorCallback: expectHostnameError("certificate relies on legacy Common Name field"),
 	},
 	{
 		// A certificate with an AKID should still chain to a parent without SKID.

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
@@ -433,7 +433,7 @@ func TestMatchIP(t *testing.T) {
 }
 
 func TestCertificateParse(t *testing.T) {
-	s, _ := hex.DecodeString(certBytes)
+	s, _ := base64.StdEncoding.DecodeString(certBytes)
 	certs, err := ParseCertificates(s)
 	if err != nil {
 		t.Error(err)
@@ -452,7 +452,7 @@ func TestCertificateParse(t *testing.T) {
 		t.Error(err)
 	}
 
-	const expectedExtensions = 4
+	const expectedExtensions = 10
 	if n := len(certs[0].Extensions); n != expectedExtensions {
 		t.Errorf("want %d extensions, got %d", expectedExtensions, n)
 	}
@@ -496,48 +496,50 @@ func TestMismatchedSignatureAlgorithm(t *testing.T) {
 	}
 }
 
-var certBytes = "308203223082028ba00302010202106edf0d9499fd4533dd1297fc42a93be1300d06092a864886" +
-	"f70d0101050500304c310b3009060355040613025a4131253023060355040a131c546861777465" +
-	"20436f6e73756c74696e67202850747929204c74642e311630140603550403130d546861777465" +
-	"20534743204341301e170d3039303332353136343932395a170d3130303332353136343932395a" +
-	"3069310b3009060355040613025553311330110603550408130a43616c69666f726e6961311630" +
-	"140603550407130d4d6f756e7461696e205669657731133011060355040a130a476f6f676c6520" +
-	"496e63311830160603550403130f6d61696c2e676f6f676c652e636f6d30819f300d06092a8648" +
-	"86f70d010101050003818d0030818902818100c5d6f892fccaf5614b064149e80a2c9581a218ef" +
-	"41ec35bd7a58125ae76f9ea54ddc893abbeb029f6b73616bf0ffd868791fba7af9c4aebf3706ba" +
-	"3eeaeed27435b4ddcfb157c05f351d66aa87fee0de072d66d773affbd36ab78bef090e0cc861a9" +
-	"03ac90dd98b51c9c41566c017f0beec3bff391051ffba0f5cc6850ad2a590203010001a381e730" +
-	"81e430280603551d250421301f06082b0601050507030106082b06010505070302060960864801" +
-	"86f842040130360603551d1f042f302d302ba029a0278625687474703a2f2f63726c2e74686177" +
-	"74652e636f6d2f54686177746553474343412e63726c307206082b060105050701010466306430" +
-	"2206082b060105050730018616687474703a2f2f6f6373702e7468617774652e636f6d303e0608" +
-	"2b060105050730028632687474703a2f2f7777772e7468617774652e636f6d2f7265706f736974" +
-	"6f72792f5468617774655f5347435f43412e637274300c0603551d130101ff04023000300d0609" +
-	"2a864886f70d01010505000381810062f1f3050ebc105e497c7aedf87e24d2f4a986bb3b837bd1" +
-	"9b91ebcad98b065992f6bd2b49b7d6d3cb2e427a99d606c7b1d46352527fac39e6a8b6726de5bf" +
-	"70212a52cba07634a5e332011bd1868e78eb5e3c93cf03072276786f207494feaa0ed9d53b2110" +
-	"a76571f90209cdae884385c882587030ee15f33d761e2e45a6bc308203233082028ca003020102" +
-	"020430000002300d06092a864886f70d0101050500305f310b3009060355040613025553311730" +
-	"15060355040a130e566572695369676e2c20496e632e31373035060355040b132e436c61737320" +
-	"33205075626c6963205072696d6172792043657274696669636174696f6e20417574686f726974" +
-	"79301e170d3034303531333030303030305a170d3134303531323233353935395a304c310b3009" +
-	"060355040613025a4131253023060355040a131c54686177746520436f6e73756c74696e672028" +
-	"50747929204c74642e311630140603550403130d5468617774652053474320434130819f300d06" +
-	"092a864886f70d010101050003818d0030818902818100d4d367d08d157faecd31fe7d1d91a13f" +
-	"0b713cacccc864fb63fc324b0794bd6f80ba2fe10493c033fc093323e90b742b71c403c6d2cde2" +
-	"2ff50963cdff48a500bfe0e7f388b72d32de9836e60aad007bc4644a3b847503f270927d0e62f5" +
-	"21ab693684317590f8bfc76c881b06957cc9e5a8de75a12c7a68dfd5ca1c875860190203010001" +
-	"a381fe3081fb30120603551d130101ff040830060101ff020100300b0603551d0f040403020106" +
-	"301106096086480186f842010104040302010630280603551d110421301fa41d301b3119301706" +
-	"035504031310507269766174654c6162656c332d313530310603551d1f042a30283026a024a022" +
-	"8620687474703a2f2f63726c2e766572697369676e2e636f6d2f706361332e63726c303206082b" +
-	"0601050507010104263024302206082b060105050730018616687474703a2f2f6f6373702e7468" +
-	"617774652e636f6d30340603551d25042d302b06082b0601050507030106082b06010505070302" +
-	"06096086480186f8420401060a6086480186f845010801300d06092a864886f70d010105050003" +
-	"81810055ac63eadea1ddd2905f9f0bce76be13518f93d9052bc81b774bad6950a1eededcfddb07" +
-	"e9e83994dcab72792f06bfab8170c4a8edea5334edef1e53d906c7562bd15cf4d18a8eb42bb137" +
-	"9048084225c53e8acb7feb6f04d16dc574a2f7a27c7b603c77cd0ece48027f012fb69b37e02a2a" +
-	"36dcd585d6ace53f546f961e05af"
+var certBytes = "MIIE0jCCA7qgAwIBAgIQWcvS+TTB3GwCAAAAAGEAWzANBgkqhkiG9w0BAQsFADBCMQswCQYD" +
+	"VQQGEwJVUzEeMBwGA1UEChMVR29vZ2xlIFRydXN0IFNlcnZpY2VzMRMwEQYDVQQDEwpHVFMg" +
+	"Q0EgMU8xMB4XDTIwMDQwMTEyNTg1NloXDTIwMDYyNDEyNTg1NlowaTELMAkGA1UEBhMCVVMx" +
+	"EzARBgNVBAgTCkNhbGlmb3JuaWExFjAUBgNVBAcTDU1vdW50YWluIFZpZXcxEzARBgNVBAoT" +
+	"Ckdvb2dsZSBMTEMxGDAWBgNVBAMTD21haWwuZ29vZ2xlLmNvbTBZMBMGByqGSM49AgEGCCqG" +
+	"SM49AwEHA0IABO+dYiPnkFl+cZVf6mrWeNp0RhQcJSBGH+sEJxjvc+cYlW3QJCnm57qlpFdd" +
+	"pz3MPyVejvXQdM6iI1mEWP4C2OujggJmMIICYjAOBgNVHQ8BAf8EBAMCB4AwEwYDVR0lBAww" +
+	"CgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUI6pZhnQ/lQgmPDwSKR2A54G7" +
+	"AS4wHwYDVR0jBBgwFoAUmNH4bhDrz5vsYJ8YkBug630J/SswZAYIKwYBBQUHAQEEWDBWMCcG" +
+	"CCsGAQUFBzABhhtodHRwOi8vb2NzcC5wa2kuZ29vZy9ndHMxbzEwKwYIKwYBBQUHMAKGH2h0" +
+	"dHA6Ly9wa2kuZ29vZy9nc3IyL0dUUzFPMS5jcnQwLAYDVR0RBCUwI4IPbWFpbC5nb29nbGUu" +
+	"Y29tghBpbmJveC5nb29nbGUuY29tMCEGA1UdIAQaMBgwCAYGZ4EMAQICMAwGCisGAQQB1nkC" +
+	"BQMwLwYDVR0fBCgwJjAkoCKgIIYeaHR0cDovL2NybC5wa2kuZ29vZy9HVFMxTzEuY3JsMIIB" +
+	"AwYKKwYBBAHWeQIEAgSB9ASB8QDvAHYAsh4FzIuizYogTodm+Su5iiUgZ2va+nDnsklTLe+L" +
+	"kF4AAAFxNgmxKgAABAMARzBFAiEA12/OHdTGXQ3qHHC3NvYCyB8aEz/+ZFOLCAI7lhqj28sC" +
+	"IG2/7Yz2zK6S6ai+dH7cTMZmoFGo39gtaTqtZAqEQX7nAHUAXqdz+d9WwOe1Nkh90EngMnqR" +
+	"mgyEoRIShBh1loFxRVgAAAFxNgmxTAAABAMARjBEAiA7PNq+MFfv6O9mBkxFViS2TfU66yRB" +
+	"/njcebWglLQjZQIgOyRKhxlEizncFRml7yn4Bg48ktXKGjo+uiw6zXEINb0wDQYJKoZIhvcN" +
+	"AQELBQADggEBADM2Rh306Q10PScsolYMxH1B/K4Nb2WICvpY0yDPJFdnGjqCYym196TjiEvs" +
+	"R6etfeHdyzlZj6nh82B4TVyHjiWM02dQgPalOuWQcuSy0OvLh7F1E7CeHzKlczdFPBTOTdM1" +
+	"RDTxlvw1bAqc0zueM8QIAyEy3opd7FxAcGQd5WRIJhzLBL+dbbMOW/LTeW7cm/Xzq8cgCybN" +
+	"BSZAvhjseJ1L29OlCTZL97IfnX0IlFQzWuvvHy7V2B0E3DHlzM0kjwkkCKDUUp/wajv2NZKC" +
+	"TkhEyERacZRKc9U0ADxwsAzHrdz5+5zfD2usEV/MQ5V6d8swLXs+ko0X6swrd4YCiB8wggRK" +
+	"MIIDMqADAgECAg0B47SaoY2KqYElaVC4MA0GCSqGSIb3DQEBCwUAMEwxIDAeBgNVBAsTF0ds" +
+	"b2JhbFNpZ24gUm9vdCBDQSAtIFIyMRMwEQYDVQQKEwpHbG9iYWxTaWduMRMwEQYDVQQDEwpH" +
+	"bG9iYWxTaWduMB4XDTE3MDYxNTAwMDA0MloXDTIxMTIxNTAwMDA0MlowQjELMAkGA1UEBhMC" +
+	"VVMxHjAcBgNVBAoTFUdvb2dsZSBUcnVzdCBTZXJ2aWNlczETMBEGA1UEAxMKR1RTIENBIDFP" +
+	"MTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANAYz0XUi83TnORA73603WkhG8nP" +
+	"PI5MdbkPMRmEPZ48Ke9QDRCTbwWAgJ8qoL0SSwLhPZ9YFiT+MJ8LdHdVkx1L903hkoIQ9lGs" +
+	"DMOyIpQPNGuYEEnnC52DOd0gxhwt79EYYWXnI4MgqCMS/9Ikf9Qv50RqW03XUGawr55CYwX7" +
+	"4BzEY2Gvn2oz/2KXvUjZ03wUZ9x13C5p6PhteGnQtxAFuPExwjsk/RozdPgj4OxrGYoWxuPN" +
+	"pM0L27OkWWA4iDutHbnGjKdTG/y82aSrvN08YdeTFZjugb2P4mRHIEAGTtesl+i5wFkSoUkl" +
+	"I+TtcDQspbRjfPmjPYPRzW0krAcCAwEAAaOCATMwggEvMA4GA1UdDwEB/wQEAwIBhjAdBgNV" +
+	"HSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwEgYDVR0TAQH/BAgwBgEB/wIBADAdBgNVHQ4E" +
+	"FgQUmNH4bhDrz5vsYJ8YkBug630J/SswHwYDVR0jBBgwFoAUm+IHV2ccHsBqBt5ZtJot39wZ" +
+	"hi4wNQYIKwYBBQUHAQEEKTAnMCUGCCsGAQUFBzABhhlodHRwOi8vb2NzcC5wa2kuZ29vZy9n" +
+	"c3IyMDIGA1UdHwQrMCkwJ6AloCOGIWh0dHA6Ly9jcmwucGtpLmdvb2cvZ3NyMi9nc3IyLmNy" +
+	"bDA/BgNVHSAEODA2MDQGBmeBDAECAjAqMCgGCCsGAQUFBwIBFhxodHRwczovL3BraS5nb29n" +
+	"L3JlcG9zaXRvcnkvMA0GCSqGSIb3DQEBCwUAA4IBAQAagD42efvzLqlGN31eVBY1rsdOCJn+" +
+	"vdE0aSZSZgc9CrpJy2L08RqO/BFPaJZMdCvTZ96yo6oFjYRNTCBlD6WW2g0W+Gw7228EI4hr" +
+	"OmzBYL1on3GO7i1YNAfw1VTphln9e14NIZT1jMmo+NjyrcwPGvOap6kEJ/mjybD/AnhrYbrH" +
+	"NSvoVvpPwxwM7bY8tEvq7czhPOzcDYzWPpvKQliLzBYhF0C8otZm79rEFVvNiaqbCSbnMtIN" +
+	"bmcgAlsQsJAJnAwfnq3YO+qh/GzoEFwIUhlRKnG7rHq13RXtK8kIKiyKtKYhq2P/11JJUNCJ" +
+	"t63yr/tQri/hlQ3zRq2dnPXK"
 
 func parseCIDR(s string) *net.IPNet {
 	_, net, err := net.ParseCIDR(s)
@@ -2054,11 +2056,11 @@ func TestEmptyNameConstraints(t *testing.T) {
 }
 
 func TestPKIXNameString(t *testing.T) {
-	pem, err := hex.DecodeString(certBytes)
+	der, err := base64.StdEncoding.DecodeString(certBytes)
 	if err != nil {
 		t.Fatal(err)
 	}
-	certs, err := ParseCertificates(pem)
+	certs, err := ParseCertificates(der)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -2079,7 +2081,7 @@ func TestPKIXNameString(t *testing.T) {
 			Country:            []string{"GB"},
 		}, "SERIALNUMBER=RFC 2253,CN=Steve Kille,OU=RFCs,O=Isode Limited,POSTALCODE=TW9 1DT,STREET=The Square,L=Richmond,ST=Surrey,C=GB"},
 		{certs[0].Subject,
-			"CN=mail.google.com,O=Google Inc,L=Mountain View,ST=California,C=US"},
+			"CN=mail.google.com,O=Google LLC,L=Mountain View,ST=California,C=US"},
 		{pkix.Name{
 			Organization: []string{"#Google, Inc. \n-> 'Alphabet\" "},
 			Country:      []string{"US"},