diff --git a/ca/ca.go b/ca/ca.go index b99fe702f4c..fa15fc119e5 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -184,8 +184,8 @@ func makeCertificateProfilesMap(defaultName string, profiles map[string]issuance return certProfilesMaps{}, fmt.Errorf("defaultCertificateProfileName:\"%s\" was configured, but a profile object was not found for that name", defaultName) } - profileByName := make(map[string]*certProfileWithID, len(profiles)) - profileByHash := make(map[[32]byte]*certProfileWithID, len(profiles)) + profilesByName := make(map[string]*certProfileWithID, len(profiles)) + profilesByHash := make(map[[32]byte]*certProfileWithID, len(profiles)) for name, profileConfig := range profiles { profile, err := issuance.NewProfile(profileConfig, lints) @@ -208,30 +208,22 @@ func makeCertificateProfilesMap(defaultName string, profiles map[string]issuance } hash := sha256.Sum256(encodedProfile.Bytes()) - _, ok := profileByName[name] - if !ok { - profileByName[name] = &certProfileWithID{ - name: name, - hash: hash, - profile: profile, - } - } else { - return certProfilesMaps{}, fmt.Errorf("duplicate certificate profile name %s", name) + cpwid := certProfileWithID{ + name: name, + hash: hash, + profile: profile, } - _, ok = profileByHash[hash] - if !ok { - profileByHash[hash] = &certProfileWithID{ - name: name, - hash: hash, - profile: profile, - } - } else { + profilesByName[name] = &cpwid + + _, found := profilesByHash[hash] + if found { return certProfilesMaps{}, fmt.Errorf("duplicate certificate profile hash %d", hash) } + profilesByHash[hash] = &cpwid } - return certProfilesMaps{defaultName, profileByHash, profileByName}, nil + return certProfilesMaps{defaultName, profilesByHash, profilesByName}, nil } // NewCertificateAuthorityImpl creates a CA instance that can sign certificates diff --git a/ca/ca_test.go b/ca/ca_test.go index e1608b9b407..4f41bbe7a7e 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -158,19 +158,16 @@ func setup(t *testing.T) *testCtx { certProfiles := make(map[string]issuance.ProfileConfig, 0) certProfiles["legacy"] = issuance.ProfileConfig{ - AllowMustStaple: true, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + AllowMustStaple: true, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } certProfiles["modern"] = issuance.ProfileConfig{ - AllowMustStaple: true, - OmitCommonName: true, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + AllowMustStaple: true, + OmitCommonName: true, + OmitKeyEncipherment: true, + OmitClientAuth: true, + OmitSKID: true, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 6}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } @@ -556,157 +553,112 @@ func TestUnpredictableIssuance(t *testing.T) { test.Assert(t, seenR3, "Expected at least one issuance from active issuer") } -func TestProfiles(t *testing.T) { +func TestMakeCertificateProfilesMap(t *testing.T) { t.Parallel() testCtx := setup(t) test.AssertEquals(t, len(testCtx.certProfiles), 2) - sa := &mockSA{} - - // These profiles contain the same data which will produce an identical - // hash, even though the names are different. - duplicateProfiles := make(map[string]issuance.ProfileConfig, 0) - duplicateProfiles["legacy"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, - MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, - MaxValidityBackdate: config.Duration{Duration: time.Hour}, - } - duplicateProfiles["modern"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, + testProfile := issuance.ProfileConfig{ + AllowMustStaple: false, MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, MaxValidityBackdate: config.Duration{Duration: time.Hour}, } - test.AssertEquals(t, len(duplicateProfiles), 2) - - badProfiles := make(map[string]issuance.ProfileConfig, 0) - badProfiles["ruhroh"] = issuance.ProfileConfig{ - AllowMustStaple: false, - Policies: []issuance.PolicyConfig{ - {OID: "2.23.140.1.2.1"}, - }, - MaxValidityPeriod: config.Duration{Duration: time.Hour * 24 * 90}, - MaxValidityBackdate: config.Duration{Duration: time.Hour}, - } - test.AssertEquals(t, len(badProfiles), 1) type nameToHash struct { name string hash [32]byte } - emptyMap := make(map[string]issuance.ProfileConfig, 0) testCases := []struct { name string - profileConfigs map[string]issuance.ProfileConfig defaultName string + profileConfigs map[string]issuance.ProfileConfig expectedErrSubstr string expectedProfiles []nameToHash }{ { - name: "no profiles", - profileConfigs: emptyMap, + name: "nil profile map", + profileConfigs: nil, expectedErrSubstr: "at least one certificate profile", }, { - name: "nil profile map", - profileConfigs: nil, + name: "no profiles", + profileConfigs: map[string]issuance.ProfileConfig{}, expectedErrSubstr: "at least one certificate profile", }, { - name: "duplicate hash", - profileConfigs: duplicateProfiles, + name: "no profile matching default name", + defaultName: "default", + profileConfigs: map[string]issuance.ProfileConfig{ + "notDefault": testProfile, + }, + expectedErrSubstr: "profile object was not found for that name", + }, + { + name: "duplicate hash", + defaultName: "default", + profileConfigs: map[string]issuance.ProfileConfig{ + "default": testProfile, + "default2": testProfile, + }, expectedErrSubstr: "duplicate certificate profile hash", }, { - name: "default profiles from setup func", - profileConfigs: testCtx.certProfiles, + name: "empty profile config", + defaultName: "empty", + profileConfigs: map[string]issuance.ProfileConfig{ + "empty": {}, + }, expectedProfiles: []nameToHash{ { - name: "legacy", - hash: [32]byte{0x41, 0xda, 0xe6, 0x97, 0xec, 0x2, 0x4c, 0x68, 0x7, 0x45, 0x57, 0xa2, 0x25, 0x86, 0xbb, 0xbe, 0x5, 0x8a, 0x40, 0x5, 0x72, 0xf5, 0x3f, 0x9f, 0x89, 0x2b, 0x1a, 0x24, 0x10, 0xab, 0xfc, 0x95}, - }, - { - name: "modern", - hash: [32]byte{0x85, 0xfb, 0x36, 0xdc, 0xef, 0x1b, 0x77, 0xc9, 0x50, 0x29, 0x36, 0xe7, 0xf2, 0xf8, 0xc, 0xed, 0xc, 0x14, 0xa8, 0x11, 0x18, 0xe9, 0x9f, 0xb3, 0xc1, 0xc7, 0x78, 0x81, 0xa2, 0x6, 0x2d, 0x12}, + name: "empty", + hash: [32]byte{0xec, 0xf4, 0x43, 0xe, 0x26, 0x1, 0x8c, 0x8b, 0x85, 0x83, 0x0, 0x6f, 0x55, 0xbd, 0x45, 0xd8, 0x66, 0x88, 0x95, 0x41, 0xd0, 0x9f, 0x8, 0x2e, 0x9b, 0x2b, 0xc9, 0x8a, 0xb4, 0x86, 0x69, 0x59}, }, }, }, { - name: "no profile matching default name", - profileConfigs: badProfiles, - expectedErrSubstr: "profile object was not found for that name", - }, - { - name: "certificate profile hash changed mid-issuance", - profileConfigs: badProfiles, - defaultName: "ruhroh", + name: "default profiles from setup func", + defaultName: testCtx.defaultCertProfileName, + profileConfigs: testCtx.certProfiles, expectedProfiles: []nameToHash{ { - // We'll change the mapped hash key under the hood during - // the test. - name: "ruhroh", - hash: [32]byte{0x12, 0x20, 0xb4, 0x5e, 0xf5, 0x9, 0x68, 0x37, 0x71, 0xb8, 0x2b, 0x2d, 0x1, 0xf6, 0xd5, 0x8c, 0xae, 0x9c, 0x6d, 0xc, 0x81, 0xb8, 0x60, 0xad, 0xfe, 0x31, 0x7, 0x60, 0x7e, 0x58, 0xed, 0xa4}, + name: "legacy", + hash: [32]byte{0x1e, 0x35, 0x19, 0x16, 0x69, 0xff, 0x4, 0x7b, 0xa5, 0xb5, 0xf, 0x2b, 0x59, 0x88, 0x7e, 0xe4, 0x22, 0x23, 0xb1, 0xad, 0xfc, 0x18, 0xe9, 0x9c, 0x57, 0x3f, 0x3, 0x95, 0xe3, 0xac, 0xff, 0x3c}, + }, + { + name: "modern", + hash: [32]byte{0xa7, 0x9b, 0xb1, 0x14, 0x8e, 0x12, 0x4, 0xb4, 0xb1, 0x14, 0xe9, 0x78, 0x2e, 0x4c, 0x20, 0x91, 0x90, 0xd0, 0x2d, 0x4e, 0x30, 0x96, 0xb2, 0xcf, 0xe0, 0xff, 0x70, 0x1f, 0x4e, 0x51, 0xf, 0x1e}, }, }, }, } for _, tc := range testCases { - // This is handled by boulder-ca, not the CA package. - if tc.defaultName == "" { - tc.defaultName = testCtx.defaultCertProfileName - } t.Run(tc.name, func(t *testing.T) { t.Parallel() - tCA, err := NewCertificateAuthorityImpl( - sa, - testCtx.pa, - testCtx.boulderIssuers, - tc.defaultName, - tc.profileConfigs, - testCtx.lints, - testCtx.serialPrefix, - testCtx.maxNames, - testCtx.keyPolicy, - testCtx.logger, - testCtx.metrics, - testCtx.fc, + profiles, err := makeCertificateProfilesMap( + tc.defaultName, tc.profileConfigs, lint.NewRegistry(), ) if tc.expectedErrSubstr != "" { + test.AssertError(t, err, "profile construction should have failed") test.AssertContains(t, err.Error(), tc.expectedErrSubstr) - test.AssertError(t, err, "No profile found during CA construction.") } else { - test.AssertNotError(t, err, "Profiles should exist, but were not found") + test.AssertNotError(t, err, "profile construction should have succeeded") } if tc.expectedProfiles != nil { - test.AssertEquals(t, len(tCA.certProfiles.profileByName), len(tc.expectedProfiles)) + test.AssertEquals(t, len(profiles.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") + cpwid, ok := profiles.profileByName[expected.name] + test.Assert(t, ok, fmt.Sprintf("expected profile %q not found", expected.name)) 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 - // name remaining the same on a CA node in the duration - // between CA1 sending capb.IssuePrecerticateResponse and - // before the RA calls - // capb.IssueCertificateForPrecertificate. We expect the - // receiving CA2 to error that the hash we expect could not - // be found in the map. - originalHash := cpwid.hash - cpwid.hash = [32]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 6, 6, 6} - test.AssertNotEquals(t, originalHash, cpwid.hash) - } + cpwid, ok = profiles.profileByHash[expected.hash] + test.Assert(t, ok, fmt.Sprintf("expected profile %q not found", expected.hash)) + test.AssertEquals(t, cpwid.name, expected.name) } }) } diff --git a/issuance/cert.go b/issuance/cert.go index 6e33112879b..cad4c001720 100644 --- a/issuance/cert.go +++ b/issuance/cert.go @@ -49,7 +49,7 @@ type ProfileConfig struct { // OmitKeyEncipherment causes the keyEncipherment bit to be omitted from the // Key Usage field of all certificates (instead of only from ECDSA certs). OmitKeyEncipherment bool - // OmitClientAuth causes the id-kp-clientAuth OID (TLS CLient Authentication) + // OmitClientAuth causes the id-kp-clientAuth OID (TLS Client Authentication) // to be omitted from the EKU extension. OmitClientAuth bool // OmitSKID causes the Subject Key Identifier extension to be omitted. diff --git a/test/config-next/ca.json b/test/config-next/ca.json index 7df7939b028..a946b940b03 100644 --- a/test/config-next/ca.json +++ b/test/config-next/ca.json @@ -46,11 +46,6 @@ "certProfiles": { "legacy": { "allowMustStaple": true, - "policies": [ - { - "oid": "2.23.140.1.2.1" - } - ], "maxValidityPeriod": "7776000s", "maxValidityBackdate": "1h5m" }, @@ -60,11 +55,6 @@ "omitKeyEncipherment": true, "omitClientAuth": true, "omitSKID": true, - "policies": [ - { - "oid": "2.23.140.1.2.1" - } - ], "maxValidityPeriod": "583200s", "maxValidityBackdate": "1h5m" }