From 0e0c7ec34826a997fd12722f47a8ce690b457cb4 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 11 Jun 2024 11:47:26 -0400 Subject: [PATCH 1/2] FREEZE.unindexed --- ca/ca.go | 3 +-- ca/ca_test.go | 4 +--- features/features.go | 5 +---- test/config-next/ca.json | 4 +--- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 239a5a4c350..85fadab5236 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -34,7 +34,6 @@ import ( corepb "github.com/letsencrypt/boulder/core/proto" csrlib "github.com/letsencrypt/boulder/csr" berrors "github.com/letsencrypt/boulder/errors" - "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" "github.com/letsencrypt/boulder/issuance" "github.com/letsencrypt/boulder/linter" @@ -568,7 +567,7 @@ func (ca *certificateAuthorityImpl) issuePrecertificateInner(ctx context.Context // Select which pool of issuers to use, based on the to-be-issued cert's key // type and whether we're using the ECDSA Allow List. alg := csr.PublicKeyAlgorithm - if alg == x509.ECDSA && !features.Get().ECDSAForAll && ca.ecdsaAllowList != nil && !ca.ecdsaAllowList.permitted(issueReq.RegistrationID) { + if alg == x509.ECDSA && ca.ecdsaAllowList != nil && !ca.ecdsaAllowList.permitted(issueReq.RegistrationID) { alg = x509.RSA } diff --git a/ca/ca_test.go b/ca/ca_test.go index 3aaff77e88d..3f7336d2e8d 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -790,10 +790,8 @@ func TestECDSAAllowList(t *testing.T) { test.AssertNotError(t, err, "Certificate failed to parse") test.AssertEquals(t, cert.SignatureAlgorithm, x509.SHA256WithRSA) - // With empty allowlist but ECDSAForAll enabled, issuance should come from ECDSA issuer. + // With empty allowlist, issuance should come from ECDSA issuer. ca, _ = issueCertificateSubTestSetup(t, nil) - features.Set(features.Config{ECDSAForAll: true}) - defer features.Reset() result, err = ca.IssuePrecertificate(ctx, req) test.AssertNotError(t, err, "Failed to issue certificate") cert, err = x509.ParseCertificate(result.DER) diff --git a/features/features.go b/features/features.go index 8820ffbf972..927ab37749a 100644 --- a/features/features.go +++ b/features/features.go @@ -22,10 +22,7 @@ type Config struct { SHA256SubjectKeyIdentifier bool EnforceMultiVA bool MultiVAFullResults bool - - // ECDSAForAll enables all accounts, regardless of their presence in the CA's - // ecdsaAllowedAccounts config value, to get issuance from ECDSA issuers. - ECDSAForAll bool + ECDSAForAll bool // ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for // GET requests. WARNING: This feature is a draft and highly unstable. diff --git a/test/config-next/ca.json b/test/config-next/ca.json index 58c335d9ffc..cb02ee3dd31 100644 --- a/test/config-next/ca.json +++ b/test/config-next/ca.json @@ -149,9 +149,7 @@ "ocspLogMaxLength": 4000, "ocspLogPeriod": "500ms", "ctLogListFile": "test/ct-test-srv/log_list.json", - "features": { - "ECDSAForAll": true - } + "features": {} }, "pa": { "challenges": { From 2fb6443bfbe348c1a86312a1a38f16aa11ebadcf Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 24 Jun 2024 17:21:44 -0400 Subject: [PATCH 2/2] Remove ECDSA Allow List code and deprecated ECDSAForAll feature --- ca/ca.go | 10 +--- ca/ca_test.go | 52 +--------------- ca/ecdsa_allow_list.go | 45 -------------- ca/ecdsa_allow_list_test.go | 70 ---------------------- ca/ocsp_test.go | 1 - ca/testdata/ecdsa_allow_list.yml | 2 - ca/testdata/ecdsa_allow_list2.yml | 2 - ca/testdata/ecdsa_allow_list_malformed.yml | 1 - cmd/boulder-ca/main.go | 14 ----- issuance/issuer.go | 3 +- test/config-next/ecdsaAllowList.yml | 2 - test/config/ca.json | 1 - test/config/ecdsaAllowList.yml | 2 - 13 files changed, 4 insertions(+), 201 deletions(-) delete mode 100644 ca/ecdsa_allow_list.go delete mode 100644 ca/ecdsa_allow_list_test.go delete mode 100644 ca/testdata/ecdsa_allow_list.yml delete mode 100644 ca/testdata/ecdsa_allow_list2.yml delete mode 100644 ca/testdata/ecdsa_allow_list_malformed.yml delete mode 100644 test/config-next/ecdsaAllowList.yml delete mode 100644 test/config/ecdsaAllowList.yml diff --git a/ca/ca.go b/ca/ca.go index 85fadab5236..d38f7e2e5c4 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -127,9 +127,6 @@ type certificateAuthorityImpl struct { issuers issuerMaps certProfiles certProfilesMaps - // This is temporary, and will be used for testing and slow roll-out - // of ECDSA issuance, but will then be removed. - ecdsaAllowList *ECDSAAllowList prefix int // Prepended to the serial number validityPeriod time.Duration backdate time.Duration @@ -249,7 +246,6 @@ func NewCertificateAuthorityImpl( defaultCertProfileName string, certificateProfiles map[string]issuance.ProfileConfig, lints lint.Registry, - ecdsaAllowList *ECDSAAllowList, certExpiry time.Duration, certBackdate time.Duration, serialPrefix int, @@ -301,7 +297,6 @@ func NewCertificateAuthorityImpl( log: logger, metrics: metrics, clk: clk, - ecdsaAllowList: ecdsaAllowList, } return ca, nil @@ -565,11 +560,8 @@ func (ca *certificateAuthorityImpl) issuePrecertificateInner(ctx context.Context } // Select which pool of issuers to use, based on the to-be-issued cert's key - // type and whether we're using the ECDSA Allow List. + // type. alg := csr.PublicKeyAlgorithm - if alg == x509.ECDSA && ca.ecdsaAllowList != nil && !ca.ecdsaAllowList.permitted(issueReq.RegistrationID) { - alg = x509.RSA - } // Select a random issuer from among the active issuers of this key type. issuerPool, ok := ca.issuers.byAlg[alg] diff --git a/ca/ca_test.go b/ca/ca_test.go index f141df1e92a..64624455de5 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -275,7 +275,6 @@ func TestSerialPrefix(t *testing.T) { "", nil, nil, - nil, testCtx.certExpiry, testCtx.certBackdate, 0, @@ -293,7 +292,6 @@ func TestSerialPrefix(t *testing.T) { "", nil, nil, - nil, testCtx.certExpiry, testCtx.certBackdate, 128, @@ -352,7 +350,7 @@ func TestIssuePrecertificate(t *testing.T) { // the precertificates previously generated from the preceding // "precertificate" test. for _, mode := range []string{"precertificate", "certificate-for-precertificate"} { - ca, sa := issueCertificateSubTestSetup(t, nil) + ca, sa := issueCertificateSubTestSetup(t) t.Run(fmt.Sprintf("%s - %s", mode, testCase.name), func(t *testing.T) { t.Parallel() req, err := x509.ParseCertificateRequest(testCase.csr) @@ -388,12 +386,8 @@ func TestIssuePrecertificate(t *testing.T) { } } -func issueCertificateSubTestSetup(t *testing.T, e *ECDSAAllowList) (*certificateAuthorityImpl, *mockSA) { +func issueCertificateSubTestSetup(t *testing.T) (*certificateAuthorityImpl, *mockSA) { testCtx := setup(t) - ecdsaAllowList := &ECDSAAllowList{} - if e == nil { - e = ecdsaAllowList - } sa := &mockSA{} ca, err := NewCertificateAuthorityImpl( sa, @@ -402,7 +396,6 @@ func issueCertificateSubTestSetup(t *testing.T, e *ECDSAAllowList) (*certificate testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - e, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -450,7 +443,6 @@ func TestNoIssuers(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -475,7 +467,6 @@ func TestMultipleIssuers(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -553,7 +544,6 @@ func TestUnpredictableIssuance(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -722,7 +712,6 @@ func TestProfiles(t *testing.T) { tc.defaultName, tc.profileConfigs, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -766,37 +755,6 @@ func TestProfiles(t *testing.T) { } } -func TestECDSAAllowList(t *testing.T) { - t.Parallel() - req := &capb.IssueCertificateRequest{Csr: ECDSACSR, RegistrationID: arbitraryRegID} - - // With allowlist containing arbitraryRegID, issuance should come from ECDSA issuer. - regIDMap := makeRegIDsMap([]int64{arbitraryRegID}) - ca, _ := issueCertificateSubTestSetup(t, &ECDSAAllowList{regIDMap}) - result, err := ca.IssuePrecertificate(ctx, req) - test.AssertNotError(t, err, "Failed to issue certificate") - cert, err := x509.ParseCertificate(result.DER) - test.AssertNotError(t, err, "Certificate failed to parse") - test.AssertEquals(t, cert.SignatureAlgorithm, x509.ECDSAWithSHA384) - - // With allowlist not containing arbitraryRegID, issuance should fall back to RSA issuer. - regIDMap = makeRegIDsMap([]int64{2002}) - ca, _ = issueCertificateSubTestSetup(t, &ECDSAAllowList{regIDMap}) - result, err = ca.IssuePrecertificate(ctx, req) - test.AssertNotError(t, err, "Failed to issue certificate") - cert, err = x509.ParseCertificate(result.DER) - test.AssertNotError(t, err, "Certificate failed to parse") - test.AssertEquals(t, cert.SignatureAlgorithm, x509.SHA256WithRSA) - - // With empty allowlist, issuance should come from ECDSA issuer. - ca, _ = issueCertificateSubTestSetup(t, nil) - result, err = ca.IssuePrecertificate(ctx, req) - test.AssertNotError(t, err, "Failed to issue certificate") - cert, err = x509.ParseCertificate(result.DER) - test.AssertNotError(t, err, "Certificate failed to parse") - test.AssertEquals(t, cert.SignatureAlgorithm, x509.ECDSAWithSHA384) -} - func TestInvalidCSRs(t *testing.T) { t.Parallel() testCases := []struct { @@ -862,7 +820,6 @@ func TestInvalidCSRs(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -901,7 +858,6 @@ func TestRejectValidityTooLong(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -1002,7 +958,6 @@ func TestIssueCertificateForPrecertificate(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -1071,7 +1026,6 @@ func TestIssueCertificateForPrecertificateWithSpecificCertificateProfile(t *test testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -1191,7 +1145,6 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, @@ -1242,7 +1195,6 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, diff --git a/ca/ecdsa_allow_list.go b/ca/ecdsa_allow_list.go deleted file mode 100644 index d0007ca6e4b..00000000000 --- a/ca/ecdsa_allow_list.go +++ /dev/null @@ -1,45 +0,0 @@ -package ca - -import ( - "os" - - "github.com/letsencrypt/boulder/strictyaml" -) - -// ECDSAAllowList acts as a container for a map of Registration IDs. -type ECDSAAllowList struct { - regIDsMap map[int64]bool -} - -// permitted checks if ECDSA issuance is permitted for the specified -// Registration ID. -func (e *ECDSAAllowList) permitted(regID int64) bool { - return e.regIDsMap[regID] -} - -func makeRegIDsMap(regIDs []int64) map[int64]bool { - regIDsMap := make(map[int64]bool) - for _, regID := range regIDs { - regIDsMap[regID] = true - } - return regIDsMap -} - -// NewECDSAAllowListFromFile is exported to allow `boulder-ca` to construct a -// new `ECDSAAllowList` object. It returns the ECDSAAllowList, the size of allow -// list after attempting to load it (for CA logging purposes so inner fields don't need to be exported), or an error. -func NewECDSAAllowListFromFile(filename string) (*ECDSAAllowList, int, error) { - configBytes, err := os.ReadFile(filename) - if err != nil { - return nil, 0, err - } - - var regIDs []int64 - err = strictyaml.Unmarshal(configBytes, ®IDs) - if err != nil { - return nil, 0, err - } - - allowList := &ECDSAAllowList{regIDsMap: makeRegIDsMap(regIDs)} - return allowList, len(allowList.regIDsMap), nil -} diff --git a/ca/ecdsa_allow_list_test.go b/ca/ecdsa_allow_list_test.go deleted file mode 100644 index 78aed034881..00000000000 --- a/ca/ecdsa_allow_list_test.go +++ /dev/null @@ -1,70 +0,0 @@ -package ca - -import ( - "testing" -) - -func TestNewECDSAAllowListFromFile(t *testing.T) { - t.Parallel() - type args struct { - filename string - } - tests := []struct { - name string - args args - want1337Permitted bool - wantEntries int - wantErrBool bool - }{ - { - name: "one entry", - args: args{"testdata/ecdsa_allow_list.yml"}, - want1337Permitted: true, - wantEntries: 1, - wantErrBool: false, - }, - { - name: "one entry but it's not 1337", - args: args{"testdata/ecdsa_allow_list2.yml"}, - want1337Permitted: false, - wantEntries: 1, - wantErrBool: false, - }, - { - name: "should error due to no file", - args: args{"testdata/ecdsa_allow_list_no_exist.yml"}, - want1337Permitted: false, - wantEntries: 0, - wantErrBool: true, - }, - { - name: "should error due to malformed YAML", - args: args{"testdata/ecdsa_allow_list_malformed.yml"}, - want1337Permitted: false, - wantEntries: 0, - wantErrBool: true, - }, - } - - for _, tt := range tests { - // TODO(Remove this >= go1.22.3) This shouldn't be necessary due to - // go1.22 changing loopvars. - // https://github.com/golang/go/issues/65612#issuecomment-1943342030 - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - allowList, gotEntries, err := NewECDSAAllowListFromFile(tt.args.filename) - if (err != nil) != tt.wantErrBool { - t.Errorf("NewECDSAAllowListFromFile() error = %v, wantErr %v", err, tt.wantErrBool) - t.Error(allowList, gotEntries, err) - return - } - if allowList != nil && allowList.permitted(1337) != tt.want1337Permitted { - t.Errorf("NewECDSAAllowListFromFile() allowList = %v, want %v", allowList, tt.want1337Permitted) - } - if gotEntries != tt.wantEntries { - t.Errorf("NewECDSAAllowListFromFile() gotEntries = %v, want %v", gotEntries, tt.wantEntries) - } - }) - } -} diff --git a/ca/ocsp_test.go b/ca/ocsp_test.go index 9cea076565e..40dd4e56b88 100644 --- a/ca/ocsp_test.go +++ b/ca/ocsp_test.go @@ -36,7 +36,6 @@ func TestOCSP(t *testing.T) { testCtx.defaultCertProfileName, testCtx.certProfiles, testCtx.lints, - nil, testCtx.certExpiry, testCtx.certBackdate, testCtx.serialPrefix, diff --git a/ca/testdata/ecdsa_allow_list.yml b/ca/testdata/ecdsa_allow_list.yml deleted file mode 100644 index a648abda31b..00000000000 --- a/ca/testdata/ecdsa_allow_list.yml +++ /dev/null @@ -1,2 +0,0 @@ ---- -- 1337 diff --git a/ca/testdata/ecdsa_allow_list2.yml b/ca/testdata/ecdsa_allow_list2.yml deleted file mode 100644 index 3365f2b9c2b..00000000000 --- a/ca/testdata/ecdsa_allow_list2.yml +++ /dev/null @@ -1,2 +0,0 @@ ---- -- 1338 diff --git a/ca/testdata/ecdsa_allow_list_malformed.yml b/ca/testdata/ecdsa_allow_list_malformed.yml deleted file mode 100644 index 286888a0ab5..00000000000 --- a/ca/testdata/ecdsa_allow_list_malformed.yml +++ /dev/null @@ -1 +0,0 @@ -not yaml diff --git a/cmd/boulder-ca/main.go b/cmd/boulder-ca/main.go index 86be24a3ea4..05666242641 100644 --- a/cmd/boulder-ca/main.go +++ b/cmd/boulder-ca/main.go @@ -100,10 +100,6 @@ type Config struct { // Recommended to be around 500ms. OCSPLogPeriod config.Duration - // Path of a YAML file containing the list of int64 RegIDs - // allowed to request ECDSA issuance - ECDSAAllowListFilename string - // CTLogListFile is the path to a JSON file on disk containing the set of // all logs trusted by Chrome. The file must match the v3 log list schema: // https://www.gstatic.com/ct/log_list/v3/log_list_schema.json @@ -236,15 +232,6 @@ func main() { kp, err := sagoodkey.NewPolicy(&c.CA.GoodKey, sa.KeyBlocked) cmd.FailOnError(err, "Unable to create key policy") - var ecdsaAllowList *ca.ECDSAAllowList - var entries int - if c.CA.ECDSAAllowListFilename != "" { - // Create an allow list object. - ecdsaAllowList, entries, err = ca.NewECDSAAllowListFromFile(c.CA.ECDSAAllowListFilename) - cmd.FailOnError(err, "Unable to load ECDSA allow list from YAML file") - logger.Infof("Loaded an ECDSA allow list with %d entries", entries) - } - srv := bgrpc.NewServer(c.CA.GRPCCA, logger) if !c.CA.DisableOCSPService { @@ -286,7 +273,6 @@ func main() { c.CA.Issuance.DefaultCertificateProfileName, c.CA.Issuance.CertProfiles, lints, - ecdsaAllowList, c.CA.Expiry.Duration, c.CA.Backdate.Duration, c.CA.SerialPrefix, diff --git a/issuance/issuer.go b/issuance/issuer.go index 4206b65c618..6917f0728c5 100644 --- a/issuance/issuer.go +++ b/issuance/issuer.go @@ -157,8 +157,7 @@ type IssuerConfig struct { // (for which an issuance token is presented), OCSP responses, and CRLs. // All Active issuers of a given key type (RSA or ECDSA) are part of a pool // and each precertificate will be issued randomly from a selected pool. - // The selection of which pool depends on the precertificate's key algorithm, - // the ECDSAForAll feature flag, and the ECDSAAllowListFilename config field. + // The selection of which pool depends on the precertificate's key algorithm. Active bool IssuerURL string `validate:"required,url"` diff --git a/test/config-next/ecdsaAllowList.yml b/test/config-next/ecdsaAllowList.yml deleted file mode 100644 index a648abda31b..00000000000 --- a/test/config-next/ecdsaAllowList.yml +++ /dev/null @@ -1,2 +0,0 @@ ---- -- 1337 diff --git a/test/config/ca.json b/test/config/ca.json index cc4728363b5..5d0ce6cae1d 100644 --- a/test/config/ca.json +++ b/test/config/ca.json @@ -144,7 +144,6 @@ }, "ocspLogMaxLength": 4000, "ocspLogPeriod": "500ms", - "ecdsaAllowListFilename": "test/config/ecdsaAllowList.yml", "features": {} }, "pa": { diff --git a/test/config/ecdsaAllowList.yml b/test/config/ecdsaAllowList.yml deleted file mode 100644 index a648abda31b..00000000000 --- a/test/config/ecdsaAllowList.yml +++ /dev/null @@ -1,2 +0,0 @@ ---- -- 1337