From 6b484f44ba929d66b175a730a30a938835cc1fc3 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 24 Jul 2024 11:44:26 -0700 Subject: [PATCH] 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 | 64 ++++++++++++++++------------------------ issuance/cert.go | 19 ++++++------ issuance/cert_test.go | 33 +++++++++------------ issuance/issuer_test.go | 3 -- test/config-next/ca.json | 16 ++++++++-- 5 files changed, 62 insertions(+), 73 deletions(-) diff --git a/ca/ca_test.go b/ca/ca_test.go index 9e81ef08c..0ec950db4 100644 --- a/ca/ca_test.go +++ b/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") diff --git a/issuance/cert.go b/issuance/cert.go index 7e6c38549..b14adaf24 100644 --- a/issuance/cert.go +++ b/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 diff --git a/issuance/cert_test.go b/issuance/cert_test.go index f612895e8..28cde1f58 100644 --- a/issuance/cert_test.go +++ b/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) { diff --git a/issuance/issuer_test.go b/issuance/issuer_test.go index fe3b0a90f..4e31d6e08 100644 --- a/issuance/issuer_test.go +++ b/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}, diff --git a/test/config-next/ca.json b/test/config-next/ca.json index 4659fe4ca..42399ecbe 100644 --- a/test/config-next/ca.json +++ b/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"