Profiles: replace AllowCommonName with OmitCommonName (#7620)

Add a new profile config key named "OmitCommonName" which, if set to
`true`, causes the issuance package to exclude the CN from the resulting
certificate even if the initiating IssuanceRequest specified one.
Deprecate the old "AllowCommonName" config key, so that it no longer has
any effect, rather than causing the issuance package to fully reject
IssuanceRequests containing a CN.

This allows for more graceful variation between profiles, since we know
that excluding the Common Name is always safe.

Part of https://github.com/letsencrypt/boulder/issues/7610

ca/ca_test.go

@@ -159,26 +159,21 @@ func setup(t *testing.T) *testCtx {
 	test.AssertNotError(t, err, "Couldn't set hostname policy")
 
 	certProfiles := make(map[string]issuance.ProfileConfig, 0)
-	certProfiles["defaultBoulderCertificateProfile"] = issuance.ProfileConfig{
+	certProfiles["legacy"] = issuance.ProfileConfig{
 		AllowMustStaple: true,
-		AllowCTPoison:   true,
-		AllowSCTList:    true,
-		AllowCommonName: true,
 		Policies: []issuance.PolicyConfig{
 			{OID: "2.23.140.1.2.1"},
 		},
-		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 8760},
+		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 24 * 365},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},
 	}
-	certProfiles["longerLived"] = issuance.ProfileConfig{
+	certProfiles["modern"] = issuance.ProfileConfig{
 		AllowMustStaple: true,
-		AllowCTPoison:   true,
-		AllowSCTList:    true,
-		AllowCommonName: true,
+		OmitCommonName:  true,
 		Policies: []issuance.PolicyConfig{
 			{OID: "2.23.140.1.2.1"},
 		},
-		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 8761},
+		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 24 * 90},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},
 	}
 	test.AssertEquals(t, len(certProfiles), 2)
@@ -249,7 +244,7 @@ func setup(t *testing.T) *testCtx {
 		pa:                     pa,
 		ocsp:                   ocsp,
 		crl:                    crl,
-		defaultCertProfileName: "defaultBoulderCertificateProfile",
+		defaultCertProfileName: "legacy",
 		lints:                  lints,
 		certProfiles:           certProfiles,
 		certExpiry:             8760 * time.Hour,
@@ -590,46 +585,37 @@ func TestProfiles(t *testing.T) {
 
 	sa := &mockSA{}
 
-	duplicateProfiles := make(map[string]issuance.ProfileConfig, 0)
 	// These profiles contain the same data which will produce an identical
 	// hash, even though the names are different.
-	duplicateProfiles["defaultBoulderCertificateProfile"] = issuance.ProfileConfig{
+	duplicateProfiles := make(map[string]issuance.ProfileConfig, 0)
+	duplicateProfiles["legacy"] = issuance.ProfileConfig{
 		AllowMustStaple: false,
-		AllowCTPoison:   false,
-		AllowSCTList:    false,
-		AllowCommonName: false,
 		Policies: []issuance.PolicyConfig{
 			{OID: "2.23.140.1.2.1"},
 		},
-		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 8760},
+		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 24 * 90},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},
 	}
-	duplicateProfiles["uhoh_ohno"] = issuance.ProfileConfig{
+	duplicateProfiles["modern"] = issuance.ProfileConfig{
 		AllowMustStaple: false,
-		AllowCTPoison:   false,
-		AllowSCTList:    false,
-		AllowCommonName: false,
 		Policies: []issuance.PolicyConfig{
 			{OID: "2.23.140.1.2.1"},
 		},
-		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 8760},
+		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 24 * 90},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},
 	}
 	test.AssertEquals(t, len(duplicateProfiles), 2)
 
-	jackedProfiles := make(map[string]issuance.ProfileConfig, 0)
-	jackedProfiles["ruhroh"] = issuance.ProfileConfig{
+	badProfiles := make(map[string]issuance.ProfileConfig, 0)
+	badProfiles["ruhroh"] = issuance.ProfileConfig{
 		AllowMustStaple: false,
-		AllowCTPoison:   false,
-		AllowSCTList:    false,
-		AllowCommonName: false,
 		Policies: []issuance.PolicyConfig{
 			{OID: "2.23.140.1.2.1"},
 		},
-		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 9000},
+		MaxValidityPeriod:   config.Duration{Duration: time.Hour * 24 * 399},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},
 	}
-	test.AssertEquals(t, len(jackedProfiles), 1)
+	test.AssertEquals(t, len(badProfiles), 1)
 
 	type nameToHash struct {
 		name string
@@ -664,30 +650,30 @@ func TestProfiles(t *testing.T) {
 			profileConfigs: testCtx.certProfiles,
 			expectedProfiles: []nameToHash{
 				{
-					name: testCtx.defaultCertProfileName,
-					hash: [32]byte{205, 182, 88, 236, 32, 18, 154, 120, 148, 194, 42, 215, 117, 140, 13, 169, 127, 196, 219, 67, 82, 36, 147, 67, 254, 117, 65, 112, 202, 60, 185, 9},
+					name: "legacy",
+					hash: [32]byte{0xf7, 0x53, 0xef, 0xcd, 0x5a, 0x17, 0x22, 0x2, 0x79, 0xc8, 0xcb, 0x7a, 0x3d, 0x60, 0x46, 0x8b, 0xa0, 0x3b, 0x8b, 0x1a, 0x81, 0x19, 0x7e, 0x84, 0x19, 0x7f, 0x16, 0xd7, 0x95, 0x39, 0xef, 0x8e},
 				},
 				{
-					name: "longerLived",
-					hash: [32]byte{80, 228, 198, 83, 7, 184, 187, 236, 113, 17, 103, 213, 226, 245, 172, 212, 135, 241, 125, 92, 122, 200, 34, 159, 139, 72, 191, 41, 1, 244, 86, 62},
+					name: "modern",
+					hash: [32]byte{0x44, 0x1d, 0x9d, 0xbc, 0x4c, 0xfc, 0xf, 0x95, 0xa4, 0x32, 0xc0, 0xcb, 0x76, 0xd1, 0x1b, 0xcb, 0xbe, 0xf1, 0x14, 0xe7, 0xa3, 0xf6, 0xfd, 0x7, 0x14, 0xb5, 0xfa, 0x2f, 0xb6, 0x1, 0x22, 0xc},
 				},
 			},
 		},
 		{
 			name:              "no profile matching default name",
-			profileConfigs:    jackedProfiles,
+			profileConfigs:    badProfiles,
 			expectedErrSubstr: "profile object was not found for that name",
 		},
 		{
 			name:           "certificate profile hash changed mid-issuance",
-			profileConfigs: jackedProfiles,
+			profileConfigs: badProfiles,
 			defaultName:    "ruhroh",
 			expectedProfiles: []nameToHash{
 				{
 					// We'll change the mapped hash key under the hood during
 					// the test.
 					name: "ruhroh",
-					hash: [32]byte{84, 131, 8, 59, 3, 244, 7, 36, 151, 161, 118, 68, 117, 183, 197, 177, 179, 232, 215, 10, 188, 48, 159, 195, 195, 140, 19, 204, 201, 182, 239, 235},
+					hash: [32]byte{0x4f, 0x23, 0x87, 0xe0, 0xc3, 0x1, 0xac, 0x3d, 0x40, 0x94, 0xa8, 0x5c, 0x71, 0xef, 0x40, 0xb5, 0xa, 0xfd, 0xb0, 0xb2, 0xb0, 0x6b, 0x2c, 0x2a, 0xf9, 0xb1, 0x5, 0xf, 0x37, 0x42, 0xf4, 0x23},
 				},
 			},
 		},
@@ -725,13 +711,13 @@ func TestProfiles(t *testing.T) {
 			}
 
 			if tc.expectedProfiles != nil {
-				test.AssertEquals(t, len(tc.expectedProfiles), len(tCA.certProfiles.profileByName))
+				test.AssertEquals(t, len(tCA.certProfiles.profileByName), len(tc.expectedProfiles))
 			}
 
 			for _, expected := range tc.expectedProfiles {
 				cpwid, ok := tCA.certProfiles.profileByName[expected.name]
 				test.Assert(t, ok, "Profile name was not found, but should have been")
-				test.AssertEquals(t, expected.hash, cpwid.hash)
+				test.AssertEquals(t, cpwid.hash, expected.hash)
 
 				if tc.name == "certificate profile hash changed mid-issuance" {
 					// This is an attempt to simulate the hash changing, but the
@@ -1029,7 +1015,7 @@ func TestIssueCertificateForPrecertificateWithSpecificCertificateProfile(t *test
 		testCtx.fc)
 	test.AssertNotError(t, err, "Failed to create CA")
 
-	selectedProfile := "longerLived"
+	selectedProfile := "legacy"
 	certProfile, ok := ca.certProfiles.profileByName[selectedProfile]
 	test.Assert(t, ok, "Certificate profile was expected to exist")
 

issuance/cert.go

@@ -38,10 +38,15 @@ type ProfileConfig struct {
 	// Deprecated: We intend to include SCTs in all final Certificates for the
 	// foreseeable future.
 	AllowSCTList bool
-	// AllowCommonName, when false, causes all IssuanceRequests which specify a CN
-	// to be rejected.
+	// AllowCommonName has no effect.
+	// Deprecated: Rather than rejecting IssuanceRequests which include a common
+	// name, we would prefer to simply drop the CN. Use `OmitCommonName` instead.
 	AllowCommonName bool
 
+	// OmitCommonName causes the CN field to be excluded from the resulting
+	// certificate, regardless of its inclusion in the IssuanceRequest.
+	OmitCommonName bool
+
 	MaxValidityPeriod   config.Duration
 	MaxValidityBackdate config.Duration
 
@@ -57,7 +62,7 @@ type PolicyConfig struct {
 // Profile is the validated structure created by reading in ProfileConfigs and IssuerConfigs
 type Profile struct {
 	allowMustStaple bool
-	allowCommonName bool
+	omitCommonName  bool
 
 	maxBackdate time.Duration
 	maxValidity time.Duration
@@ -69,7 +74,7 @@ type Profile struct {
 func NewProfile(profileConfig ProfileConfig, lints lint.Registry) (*Profile, error) {
 	sp := &Profile{
 		allowMustStaple: profileConfig.AllowMustStaple,
-		allowCommonName: profileConfig.AllowCommonName,
+		omitCommonName:  profileConfig.OmitCommonName,
 		maxBackdate:     profileConfig.MaxValidityBackdate.Duration,
 		maxValidity:     profileConfig.MaxValidityPeriod.Duration,
 		lints:           lints,
@@ -103,10 +108,6 @@ func (i *Issuer) requestValid(clk clock.Clock, prof *Profile, req *IssuanceReque
 		return errors.New("cannot include both ct poison and sct list extensions")
 	}
 
-	if !prof.allowCommonName && req.CommonName != "" {
-		return errors.New("common name cannot be included")
-	}
-
 	// The validity period is calculated inclusive of the whole second represented
 	// by the notAfter timestamp.
 	validity := req.NotAfter.Add(time.Second).Sub(req.NotBefore)
@@ -255,7 +256,7 @@ func (i *Issuer) Prepare(prof *Profile, req *IssuanceRequest) ([]byte, *issuance
 	// populate template from the issuance request
 	template.NotBefore, template.NotAfter = req.NotBefore, req.NotAfter
 	template.SerialNumber = big.NewInt(0).SetBytes(req.Serial)
-	if req.CommonName != "" {
+	if req.CommonName != "" && !prof.omitCommonName {
 		template.Subject.CommonName = req.CommonName
 	}
 	template.DNSNames = req.DNSNames

issuance/cert_test.go

@@ -106,19 +106,6 @@ func TestRequestValid(t *testing.T) {
 			},
 			expectedError: "cannot include both ct poison and sct list extensions",
 		},
-		{
-			name: "common name not allowed",
-			issuer: &Issuer{
-				active: true,
-			},
-			profile: &Profile{},
-			request: &IssuanceRequest{
-				PublicKey:    &ecdsa.PublicKey{},
-				SubjectKeyId: goodSKID,
-				CommonName:   "cn",
-			},
-			expectedError: "common name cannot be included",
-		},
 		{
 			name: "negative validity",
 			issuer: &Issuer{
@@ -386,13 +373,14 @@ func TestIssueCommonName(t *testing.T) {
 		PublicKey:       pk.Public(),
 		SubjectKeyId:    goodSKID,
 		Serial:          []byte{1, 2, 3, 4, 5, 6, 7, 8, 9},
-		CommonName:      "example.com",
 		DNSNames:        []string{"example.com", "www.example.com"},
 		NotBefore:       fc.Now(),
 		NotAfter:        fc.Now().Add(time.Hour - time.Second),
 		IncludeCTPoison: true,
 	}
 
+	// In the default profile, the common name is allowed if requested.
+	ir.CommonName = "example.com"
 	_, issuanceToken, err := signer.Prepare(cnProfile, ir)
 	test.AssertNotError(t, err, "Prepare failed")
 	certBytes, err := signer.Issue(issuanceToken)
@@ -401,10 +389,7 @@ func TestIssueCommonName(t *testing.T) {
 	test.AssertNotError(t, err, "failed to parse certificate")
 	test.AssertEquals(t, cert.Subject.CommonName, "example.com")
 
-	cnProfile.allowCommonName = false
-	_, _, err = signer.Prepare(cnProfile, ir)
-	test.AssertError(t, err, "Prepare should have failed")
-
+	// But not including the common name should be acceptable as well.
 	ir.CommonName = ""
 	_, issuanceToken, err = signer.Prepare(cnProfile, ir)
 	test.AssertNotError(t, err, "Prepare failed")
@@ -413,7 +398,17 @@ func TestIssueCommonName(t *testing.T) {
 	cert, err = x509.ParseCertificate(certBytes)
 	test.AssertNotError(t, err, "failed to parse certificate")
 	test.AssertEquals(t, cert.Subject.CommonName, "")
-	test.AssertDeepEquals(t, cert.DNSNames, []string{"example.com", "www.example.com"})
+
+	// And the common name should be omitted if the profile is so configured.
+	ir.CommonName = "example.com"
+	cnProfile.omitCommonName = true
+	_, issuanceToken, err = signer.Prepare(cnProfile, ir)
+	test.AssertNotError(t, err, "Prepare failed")
+	certBytes, err = signer.Issue(issuanceToken)
+	test.AssertNotError(t, err, "Issue failed")
+	cert, err = x509.ParseCertificate(certBytes)
+	test.AssertNotError(t, err, "failed to parse certificate")
+	test.AssertEquals(t, cert.Subject.CommonName, "")
 }
 
 func TestIssueCTPoison(t *testing.T) {

issuance/issuer_test.go

@@ -24,9 +24,6 @@ import (
 
 func defaultProfileConfig() ProfileConfig {
 	return ProfileConfig{
-		AllowCommonName:     true,
-		AllowCTPoison:       true,
-		AllowSCTList:        true,
 		AllowMustStaple:     true,
 		MaxValidityPeriod:   config.Duration{Duration: time.Hour},
 		MaxValidityBackdate: config.Duration{Duration: time.Hour},

test/config-next/ca.json

@@ -42,11 +42,21 @@
 			"hostOverride": "sa.boulder"
 		},
 		"issuance": {
-			"defaultCertificateProfileName": "defaultBoulderCertificateProfile",
+			"defaultCertificateProfileName": "legacy",
 			"certProfiles": {
-				"defaultBoulderCertificateProfile": {
+				"legacy": {
 					"allowMustStaple": true,
-					"allowCommonName": true,
+					"policies": [
+						{
+							"oid": "2.23.140.1.2.1"
+						}
+					],
+					"maxValidityPeriod": "7776000s",
+					"maxValidityBackdate": "1h5m"
+				},
+				"modern": {
+					"allowMustStaple": true,
+					"omitCommonName": true,
 					"policies": [
 						{
 							"oid": "2.23.140.1.2.1"