Fix SAN to CN promotion (#4298)

Before #4275 if a CSR only contained SANs longer than the max CN limit
it would set the CN to one anyway and would cause the 'CN too long'
check to get triggered. After #4275 if all of the SANs were too long
the CN wouldn't get set and we didn't have a check for `forceCNFromSAN
&& cn == ""` which would allow empty CNs despite `forceCNFromSAN`
being set. This adds that check and a test for the corner case.

csr/csr.go

@@ -33,12 +33,13 @@ var goodSignatureAlgorithms = map[x509.SignatureAlgorithm]bool{
 }
 
 var (
-	invalidPubKey       = errors.New("invalid public key in CSR")
-	unsupportedSigAlg   = errors.New("signature algorithm not supported")
-	invalidSig          = errors.New("invalid signature on CSR")
-	invalidEmailPresent = errors.New("CSR contains one or more email address fields")
-	invalidIPPresent    = errors.New("CSR contains one or more IP address fields")
-	invalidNoDNS        = errors.New("at least one DNS name is required")
+	invalidPubKey        = errors.New("invalid public key in CSR")
+	unsupportedSigAlg    = errors.New("signature algorithm not supported")
+	invalidSig           = errors.New("invalid signature on CSR")
+	invalidEmailPresent  = errors.New("CSR contains one or more email address fields")
+	invalidIPPresent     = errors.New("CSR contains one or more IP address fields")
+	invalidNoDNS         = errors.New("at least one DNS name is required")
+	invalidAllSANTooLong = errors.New("CSR doesn't contain a SAN short enough to fit in CN")
 )
 
 // VerifyCSR checks the validity of a x509.CertificateRequest. Before doing checks it normalizes
@@ -68,6 +69,9 @@ func VerifyCSR(csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.Ke
 	if len(csr.DNSNames) == 0 && csr.Subject.CommonName == "" {
 		return invalidNoDNS
 	}
+	if forceCNFromSAN && csr.Subject.CommonName == "" {
+		return invalidAllSANTooLong
+	}
 	if len(csr.Subject.CommonName) > maxCNLength {
 		return fmt.Errorf("CN was longer than %d bytes", maxCNLength)
 	}

csr/csr_test.go

@@ -70,6 +70,9 @@ func TestVerifyCSR(t *testing.T) {
 	signedReqWithIPAddress := new(x509.CertificateRequest)
 	*signedReqWithIPAddress = *signedReq
 	signedReqWithIPAddress.IPAddresses = []net.IP{net.IPv4(1, 2, 3, 4)}
+	signedReqWithAllLongSANs := new(x509.CertificateRequest)
+	*signedReqWithAllLongSANs = *signedReq
+	signedReqWithAllLongSANs.DNSNames = []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com"}
 
 	cases := []struct {
 		csr           *x509.CertificateRequest
@@ -151,10 +154,18 @@ func TestVerifyCSR(t *testing.T) {
 			0,
 			invalidIPPresent,
 		},
+		{
+			signedReqWithAllLongSANs,
+			100,
+			testingPolicy,
+			&mockPA{},
+			0,
+			invalidAllSANTooLong,
+		},
 	}
 
 	for _, c := range cases {
-		err := VerifyCSR(c.csr, c.maxNames, c.keyPolicy, c.pa, false, c.regID)
+		err := VerifyCSR(c.csr, c.maxNames, c.keyPolicy, c.pa, true, c.regID)
 		test.AssertDeepEquals(t, c.expectedError, err)
 	}
 }

test/v2_integration.py

@@ -33,6 +33,8 @@ import josepy
 import tempfile
 import shutil
 import atexit
+import random
+import string
 
 import threading
 from http.server import HTTPServer, BaseHTTPRequestHandler
@@ -938,5 +940,16 @@ def test_new_order_policy_errs():
     if not ok:
         raise Exception('Expected problem, got no error')
 
+def test_long_san_no_cn():
+    try:
+        chisel2.auth_and_issue([''.join(random.choice(string.ascii_uppercase) for x in range(61)) + ".com"])
+        # if we get to this raise the auth_and_issue call didn't fail, so fail the test
+        raise Exception("Issuance didn't fail when the only SAN in a certificate was longer than the max CN length")
+    except messages.Error as e:
+        if e.typ != "urn:ietf:params:acme:error:malformed":
+            raise Exception('Expected malformed type problem, got {0}'.format(e.typ))
+        if e.detail != 'Error finalizing order :: issuing precertificate: CSR doesn\'t contain a SAN short enough to fit in CN':
+            raise Exception('Problem detail did not match expected')
+
 def run(cmd, **kwargs):
     return subprocess.check_output(cmd, shell=True, stderr=subprocess.STDOUT, **kwargs)