Allow CSRs whose CN is longer than acceptable (#7700)

Also rework comments & test names for clarity, add tests for this new CN
handling, and change/remove tests that should indeed no longer fail.

Fixes #7623

ca/ca_test.go

@@ -693,12 +693,6 @@ func TestInvalidCSRs(t *testing.T) {
 		// * Signature Algorithm: sha1WithRSAEncryption
 		{"RejectBadAlgorithm", "./testdata/bad_algorithm.der.csr", nil, "Issued a certificate based on a CSR with a bad signature algorithm.", berrors.BadCSR},
 
-		// CSR generated by Go:
-		// * Random RSA public key.
-		// * CN = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com
-		// * DNSNames = [none]
-		{"RejectLongCommonName", "./testdata/long_cn.der.csr", nil, "Issued a certificate with a CN over 64 bytes.", berrors.BadCSR},
-
 		// CSR generated by OpenSSL:
 		// Edited signature to become invalid.
 		{"RejectWrongSignature", "./testdata/invalid_signature.der.csr", nil, "Issued a certificate based on a CSR with an invalid signature.", berrors.BadCSR},

csr/csr.go

@@ -37,9 +37,9 @@ var (
 	invalidNoDNS        = berrors.BadCSRError("at least one DNS name is required")
 )
 
-// VerifyCSR checks the validity of a x509.CertificateRequest. Before doing checks it normalizes
-// the CSR which lowers the case of DNS names and subject CN, and hoist a DNS name into the CN
-// if it is empty.
+// VerifyCSR checks the validity of a x509.CertificateRequest. It uses
+// NamesFromCSR to normalize the DNS names before checking whether we'll issue
+// for them.
 func VerifyCSR(ctx context.Context, csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.KeyPolicy, pa core.PolicyAuthority) error {
 	key, ok := csr.PublicKey.(crypto.PublicKey)
 	if !ok {
@@ -67,6 +67,8 @@ func VerifyCSR(ctx context.Context, csr *x509.CertificateRequest, maxNames int,
 		return invalidIPPresent
 	}
 
+	// NamesFromCSR also performs normalization, returning values that may not
+	// match the literal CSR contents.
 	names := NamesFromCSR(csr)
 
 	if len(names.SANs) == 0 && names.CN == "" {
@@ -92,19 +94,25 @@ type names struct {
 }
 
 // NamesFromCSR deduplicates and lower-cases the Subject Common Name and Subject
-// Alternative Names from the CSR. If the CSR contains a CN, then it preserves
-// it and guarantees that the SANs also include it. If the CSR does not contain
-// a CN, then it also attempts to promote a SAN to the CN (if any is short
-// enough to fit).
+// Alternative Names from the CSR. If a CN was provided, it will be used if it
+// is short enough, otherwise there will be no CN. If no CN was provided, the CN
+// will be the first SAN that is short enough, which is done only for backwards
+// compatibility with prior Let's Encrypt behaviour. The resulting SANs will
+// always include the original CN, if any.
 func NamesFromCSR(csr *x509.CertificateRequest) names {
 	// Produce a new "sans" slice with the same memory address as csr.DNSNames
 	// but force a new allocation if an append happens so that we don't
 	// accidentally mutate the underlying csr.DNSNames array.
 	sans := csr.DNSNames[0:len(csr.DNSNames):len(csr.DNSNames)]
+
 	if csr.Subject.CommonName != "" {
 		sans = append(sans, csr.Subject.CommonName)
 	}
 
+	if len(csr.Subject.CommonName) > maxCNLength {
+		return names{SANs: core.UniqueLowerNames(sans)}
+	}
+
 	if csr.Subject.CommonName != "" {
 		return names{SANs: core.UniqueLowerNames(sans), CN: strings.ToLower(csr.Subject.CommonName)}
 	}

csr/csr_test.go

@@ -109,7 +109,7 @@ func TestVerifyCSR(t *testing.T) {
 			signedReqWithLongCN,
 			100,
 			&mockPA{},
-			berrors.BadCSRError("CN was longer than %d bytes", maxCNLength),
+			nil,
 		},
 		{
 			signedReqWithHosts,
@@ -189,7 +189,16 @@ func TestNamesFromCSR(t *testing.T) {
 			[]string{"a.com", "b.com"},
 		},
 		{
-			"no explicit CN, too long leading SANs",
+			"no explicit CN, all SANs too long to be the CN",
+			&x509.CertificateRequest{DNSNames: []string{
+				tooLongString + ".a.com",
+				tooLongString + ".b.com",
+			}},
+			"",
+			[]string{tooLongString + ".a.com", tooLongString + ".b.com"},
+		},
+		{
+			"no explicit CN, leading SANs too long to be the CN",
 			&x509.CertificateRequest{DNSNames: []string{
 				tooLongString + ".a.com",
 				tooLongString + ".b.com",
@@ -200,7 +209,7 @@ func TestNamesFromCSR(t *testing.T) {
 			[]string{"a.com", tooLongString + ".a.com", tooLongString + ".b.com", "b.com"},
 		},
 		{
-			"explicit CN, too long leading SANs",
+			"explicit CN, leading SANs too long to be the CN",
 			&x509.CertificateRequest{
 				Subject: pkix.Name{CommonName: "A.com"},
 				DNSNames: []string{
@@ -212,6 +221,24 @@ func TestNamesFromCSR(t *testing.T) {
 			"a.com",
 			[]string{"a.com", tooLongString + ".a.com", tooLongString + ".b.com", "b.com"},
 		},
+		{
+			"explicit CN that's too long to be the CN",
+			&x509.CertificateRequest{
+				Subject: pkix.Name{CommonName: tooLongString + ".a.com"},
+			},
+			"",
+			[]string{tooLongString + ".a.com"},
+		},
+		{
+			"explicit CN that's too long to be the CN, with a SAN",
+			&x509.CertificateRequest{
+				Subject: pkix.Name{CommonName: tooLongString + ".a.com"},
+				DNSNames: []string{
+					"b.com",
+				}},
+			"",
+			[]string{tooLongString + ".a.com", "b.com"},
+		},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {