From d122f185404d95126629c2f2ae4d462bad04af4b Mon Sep 17 00:00:00 2001 From: kruti-s Date: Tue, 15 Oct 2024 17:07:44 -0400 Subject: [PATCH 01/29] edited names, bucket, overrides, etc --- ratelimits/bucket.go | 69 +++++++++++++++++++ ratelimits/bucket_test.go | 37 ++++++++++ ratelimits/limiter.go | 2 +- ratelimits/limiter_test.go | 17 +++++ ratelimits/names.go | 15 +++- ratelimits/names_test.go | 28 ++++++++ .../testdata/working_override_13371338.yml | 7 ++ test/config-next/wfe2-ratelimit-defaults.yml | 4 ++ 8 files changed, 177 insertions(+), 2 deletions(-) diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 4a8dd4435cd..05757b22241 100644 --- a/ratelimits/bucket.go +++ b/ratelimits/bucket.go @@ -304,6 +304,75 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO return txn, nil } +// IssuancePausedPerDomainPerAccountCheckOnlyTransactions returns a slice +// of Transactions for the provided order domain names. An error is returned if +// any of the order domain names are invalid. This method should be used for +// checking capacity, before allowing more authorizations to be created. +// +// Precondition: len(orderDomains) < maxNames. +func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountCheckOnlyTransactions(regId int64, orderDomains []string) ([]Transaction, error) { + // IssuancePausedPerDomainPerAccount limit uses the 'enum:regId' + // bucket key format for overrides. + perAccountBucketKey, err := newRegIdBucketKey(IssuancePausedPerDomainPerAccount, regId) + if err != nil { + return nil, err + } + limit, err := builder.getLimit(IssuancePausedPerDomainPerAccount, perAccountBucketKey) + if err != nil && !errors.Is(err, errLimitDisabled) { + return nil, err + } + + var txns []Transaction + for _, name := range orderDomains { + // IssuancePausedPerDomainPerAccount limit uses the + // 'enum:regId:domain' bucket key format for transactions. + perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, name) + if err != nil { + return nil, err + } + + // Add a check-only transaction for each per domain per account bucket. + // The cost is 0, as we are only checking that the account and domain + // pair aren't already over the limit. + txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 1) + if err != nil { + return nil, err + } + txns = append(txns, txn) + } + return txns, nil +} + +// IssuancePausedPerDomainPerAccountSpendOnlyTransaction returns a spend- +// only Transaction for the provided order domain name. An error is returned if +// the order domain name is invalid. This method should be used for spending +// capacity, as a result of a failed authorization. +func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountSpendOnlyTransaction(regId int64, orderDomain string) (Transaction, error) { + // IssuancePausedPerDomainPerAccount limit uses the 'enum:regId' + // bucket key format for overrides. + perAccountBucketKey, err := newRegIdBucketKey(IssuancePausedPerDomainPerAccount, regId) + if err != nil { + return Transaction{}, err + } + limit, err := builder.getLimit(IssuancePausedPerDomainPerAccount, perAccountBucketKey) + if err != nil && !errors.Is(err, errLimitDisabled) { + return Transaction{}, err + } + + // IssuancePausedPerDomainPerAccount limit uses the + // 'enum:regId:domain' bucket key format for transactions. + perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, orderDomain) + if err != nil { + return Transaction{}, err + } + txn, err := newSpendOnlyTransaction(limit, perDomainPerAccountBucketKey, 1) + if err != nil { + return Transaction{}, err + } + + return txn, nil +} + // certificatesPerDomainCheckOnlyTransactions returns a slice of Transactions // for the provided order domain names. An error is returned if any of the order // domain names are invalid. This method should be used for checking capacity, diff --git a/ratelimits/bucket_test.go b/ratelimits/bucket_test.go index 747d14210bf..39f7bc1e110 100644 --- a/ratelimits/bucket_test.go +++ b/ratelimits/bucket_test.go @@ -102,6 +102,43 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { test.Assert(t, txn.limit.isOverride(), "should be an override") } +func TestIssuancePausedPerDomainPerAccountTransactions(t *testing.T) { + t.Parallel() + + tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") + test.AssertNotError(t, err, "creating TransactionBuilder") + + // A check-only transaction for the default per-account limit. + txns, err := tb.IssuancePausedPerDomainPerAccountCheckOnlyTransactions(123456789, []string{"so.many.labels.here.example.com"}) + test.AssertNotError(t, err, "creating transactions") + test.AssertEquals(t, len(txns), 1) + test.AssertEquals(t, txns[0].bucketKey, "8:123456789:so.many.labels.here.example.com") + test.Assert(t, txns[0].checkOnly(), "should be check-only") + test.Assert(t, !txns[0].limit.isOverride(), "should not be an override") + + // A spend-only transaction for the default per-account limit. + txn, err := tb.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(123456789, "so.many.labels.here.example.com") + test.AssertNotError(t, err, "creating transaction") + test.AssertEquals(t, txn.bucketKey, "8:123456789:so.many.labels.here.example.com") + test.Assert(t, txn.spendOnly(), "should be spend-only") + test.Assert(t, !txn.limit.isOverride(), "should not be an override") + + // A check-only transaction for the per-account limit override. + txns, err = tb.IssuancePausedPerDomainPerAccountCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com"}) + test.AssertNotError(t, err, "creating transactions") + test.AssertEquals(t, len(txns), 1) + test.AssertEquals(t, txns[0].bucketKey, "8:13371338:so.many.labels.here.example.com") + test.Assert(t, txns[0].checkOnly(), "should be check-only") + test.Assert(t, txns[0].limit.isOverride(), "should be an override") + + // A spend-only transaction for the per-account limit override. + txn, err = tb.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(13371338, "so.many.labels.here.example.com") + test.AssertNotError(t, err, "creating transaction") + test.AssertEquals(t, txn.bucketKey, "8:13371338:so.many.labels.here.example.com") + test.Assert(t, txn.spendOnly(), "should be spend-only") + test.Assert(t, txn.limit.isOverride(), "should be an override") +} + func TestCertificatesPerDomainTransactions(t *testing.T) { t.Parallel() diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index f4eca6c8db5..df26f92cbdf 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -137,7 +137,7 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) - case FailedAuthorizationsPerDomainPerAccount: + case FailedAuthorizationsPerDomainPerAccount, IssuancePausedPerDomainPerAccount: // Uses bucket key 'enum:regId:domain'. idx := strings.LastIndex(d.transaction.bucketKey, ":") if idx == -1 { diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 097495fe4ba..eec22f75405 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -525,6 +525,23 @@ func TestRateLimitError(t *testing.T) { expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", expectedErrType: berrors.RateLimit, }, + { + name: "IssuancePausedPerDomainPerAccount limit reached", + decision: &Decision{ + allowed: false, + retryIn: 15 * time.Second, + transaction: Transaction{ + limit: limit{ + name: IssuancePausedPerDomainPerAccount, + Burst: 7, + Period: config.Duration{Duration: time.Hour}, + }, + bucketKey: "4:12345:example.com", + }, + }, + expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", + expectedErrType: berrors.RateLimit, + }, { name: "CertificatesPerDomain limit reached", decision: &Decision{ diff --git a/ratelimits/names.go b/ratelimits/names.go index 4495f863865..5b390a6f671 100644 --- a/ratelimits/names.go +++ b/ratelimits/names.go @@ -76,6 +76,9 @@ const ( // Note: When this is referenced in an overrides file, the fqdnSet MUST be // passed as a comma-separated list of domain names. CertificatesPerFQDNSet + + // TODO: @kruti-s + IssuancePausedPerDomainPerAccount ) // nameToString is a map of Name values to string names. @@ -85,9 +88,10 @@ var nameToString = map[Name]string{ NewRegistrationsPerIPv6Range: "NewRegistrationsPerIPv6Range", NewOrdersPerAccount: "NewOrdersPerAccount", FailedAuthorizationsPerDomainPerAccount: "FailedAuthorizationsPerDomainPerAccount", - CertificatesPerDomain: "CertificatesPerDomain", + CertificatesPerDomain: "CertificatesPerDomain", CertificatesPerDomainPerAccount: "CertificatesPerDomainPerAccount", CertificatesPerFQDNSet: "CertificatesPerFQDNSet", + IssuancePausedPerDomainPerAccount: "IssuancePausedPerDomainPerAccount", } // isValid returns true if the Name is a valid rate limit name. @@ -231,6 +235,15 @@ func validateIdForName(name Name, id string) error { // 'enum:fqdnSet' return validateFQDNSet(id) + case IssuancePausedPerDomainPerAccount: + if strings.Contains(id, ":") { + // 'enum:regId:domain' for transaction + return validateRegIdDomain(id) + } else { + // 'enum:regId' for overrides + return validateRegId(id) + } + case Unknown: fallthrough diff --git a/ratelimits/names_test.go b/ratelimits/names_test.go index c4d5812d8f4..51c049dcc43 100644 --- a/ratelimits/names_test.go +++ b/ratelimits/names_test.go @@ -134,6 +134,34 @@ func TestValidateIdForName(t *testing.T) { id: "12ea5", err: "invalid regId", }, + { + limit: IssuancePausedPerDomainPerAccount, + desc: "transaction: valid regId and domain", + id: "12345:example.com", + }, + { + limit: IssuancePausedPerDomainPerAccount, + desc: "transaction: invalid regId", + id: "12ea5:example.com", + err: "invalid regId", + }, + { + limit: IssuancePausedPerDomainPerAccount, + desc: "transaction: invalid domain", + id: "12345:examplecom", + err: "name needs at least one dot", + }, + { + limit: IssuancePausedPerDomainPerAccount, + desc: "override: valid regId", + id: "12345", + }, + { + limit: IssuancePausedPerDomainPerAccount, + desc: "override: invalid regId", + id: "12ea5", + err: "invalid regId", + }, { limit: CertificatesPerDomainPerAccount, desc: "transaction: valid regId and domain", diff --git a/ratelimits/testdata/working_override_13371338.yml b/ratelimits/testdata/working_override_13371338.yml index a260fedb5f5..a4c17cf4774 100644 --- a/ratelimits/testdata/working_override_13371338.yml +++ b/ratelimits/testdata/working_override_13371338.yml @@ -12,3 +12,10 @@ ids: - id: 13371338 comment: Used to test the TransactionBuilder +- IssuancePausedPerDomainPerAccount: + burst: 1337 + count: 1337 + period: 5m + ids: + - id: 13371338 + comment: Used to test the TransactionBuilder diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index c24e854c2b3..db594bc3f2a 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -14,6 +14,10 @@ FailedAuthorizationsPerDomainPerAccount: count: 3 burst: 3 period: 5m +IssuancePausedPerDomainPerAccount: + count: 3600 + burst: 3600 + period: 960h NewOrdersPerAccount: count: 1500 burst: 1500 From e966a9a9ad41293ff05aab12d4b1cf82ac13a930 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Wed, 16 Oct 2024 14:03:21 -0400 Subject: [PATCH 02/29] more changes made --- ratelimits/limiter_test.go | 2 +- ratelimits/testdata/working_overrides.yml | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index eec22f75405..eed4aa1b5d6 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -536,7 +536,7 @@ func TestRateLimitError(t *testing.T) { Burst: 7, Period: config.Duration{Duration: time.Hour}, }, - bucketKey: "4:12345:example.com", + bucketKey: "8:12345:example.com", }, }, expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", diff --git a/ratelimits/testdata/working_overrides.yml b/ratelimits/testdata/working_overrides.yml index 584676e87da..a4b507ec076 100644 --- a/ratelimits/testdata/working_overrides.yml +++ b/ratelimits/testdata/working_overrides.yml @@ -22,3 +22,12 @@ - id: 5678 comment: Foo +- IssuancePausedPerDomainPerAccount: + burst: 60 + count: 60 + period: 3s + ids: + - id: 1234 + comment: Foo + - id: 5678 + comment: Foo From d6fed536d822462c8b7d6d908123d3bdbff54da8 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Wed, 16 Oct 2024 14:24:36 -0400 Subject: [PATCH 03/29] change error message for limiter --- ratelimits/limiter.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index df26f92cbdf..484f5f3bf23 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -137,7 +137,7 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) - case FailedAuthorizationsPerDomainPerAccount, IssuancePausedPerDomainPerAccount: + case FailedAuthorizationsPerDomainPerAccount: // Uses bucket key 'enum:regId:domain'. idx := strings.LastIndex(d.transaction.bucketKey, ":") if idx == -1 { @@ -153,6 +153,22 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) + case IssuancePausedPerDomainPerAccount: + // Uses bucket key 'enum:regId:domain'. + idx := strings.LastIndex(d.transaction.bucketKey, ":") + if idx == -1 { + return berrors.InternalServerError("unrecognized bucket key while generating error") + } + domain := d.transaction.bucketKey[idx+1:] + return berrors.FailedValidationError( + retryAfter, + "too many failed validation attempts (%d) for %q in the last %s, retry after %s", + d.transaction.limit.Burst, + domain, + d.transaction.limit.Period.Duration, + retryAfterTs, + ) + case CertificatesPerDomain, CertificatesPerDomainPerAccount: // Uses bucket key 'enum:domain' or 'enum:regId:domain' respectively. idx := strings.LastIndex(d.transaction.bucketKey, ":") From fb3a966aea4c9ccdc3d720d23d54a495226d3761 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Wed, 16 Oct 2024 14:41:42 -0400 Subject: [PATCH 04/29] edit limiter_test for err msg --- ratelimits/limiter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index eed4aa1b5d6..45b3d262df3 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -539,7 +539,7 @@ func TestRateLimitError(t *testing.T) { bucketKey: "8:12345:example.com", }, }, - expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", + expectedErr: "too many failed validation attempts (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", expectedErrType: berrors.RateLimit, }, { From bdc2a1359d977305748a97b2211b08ddc4590385 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Fri, 18 Oct 2024 12:42:13 -0400 Subject: [PATCH 05/29] Increment ratelimit for IssuancePaused in ra.go --- ra/ra.go | 14 ++++++++++++++ ratelimits/bucket.go | 1 + 2 files changed, 15 insertions(+) diff --git a/ra/ra.go b/ra/ra.go index 62486f51531..5927aa18628 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1855,6 +1855,20 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } + + // Increment ratelimit for IssuancePaused + txn, err = ra.txnBuilder.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(regId, name) + if err != nil { + ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) + } + + _, err = ra.limiter.Spend(ctx, txn) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return + } + ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) + } } // PerformValidation initiates validation for a specific challenge associated diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 05757b22241..6360eb7cd6a 100644 --- a/ratelimits/bucket.go +++ b/ratelimits/bucket.go @@ -573,6 +573,7 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names transactions = append(transactions, txn) } + // TODO: @kruti-s do I need something like this for IssuancePaused? txns, err := builder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names) if err != nil { return nil, makeTxnError(err, FailedAuthorizationsPerDomainPerAccount) From 5bd14abeadc438ee5ebc68cc407f8c43e62b4c0e Mon Sep 17 00:00:00 2001 From: kruti-s Date: Fri, 18 Oct 2024 17:23:13 -0400 Subject: [PATCH 06/29] SpendorCheck to SpendAndCheck --- ra/ra.go | 34 ++++++++++++++++++--- ratelimits/bucket.go | 64 ++++++++------------------------------- ratelimits/bucket_test.go | 29 ++---------------- ratelimits/limiter.go | 2 +- 4 files changed, 47 insertions(+), 82 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 5927aa18628..0e94f9b99a9 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1837,7 +1837,9 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI // countFailedValidation increments the failed authorizations per domain per // account rate limit. There is no reason to surface errors from this function // to the Subscriber, spends against this limit are best effort. -func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, name string) { +func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, iden identifier.ACMEIdentifier) { + var name = iden.Value + if ra.limiter == nil || ra.txnBuilder == nil { // Limiter is disabled. return @@ -1857,18 +1859,36 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } // Increment ratelimit for IssuancePaused - txn, err = ra.txnBuilder.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(regId, name) + txn, err = ra.txnBuilder.IssuancePausedPerDomainPerAccountTransaction(regId, name) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) } - _, err = ra.limiter.Spend(ctx, txn) + decision, err := ra.limiter.Spend(ctx, txn) if err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return } ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) } + // TODO: is this how we want to catch this error + if decision == nil { + ra.log.Warningf("decision and error are both nil for the %s rate limit", ratelimits.IssuancePausedPerDomainPerAccount) + return + } + + if decision.Result(ra.clk.Now()) != nil { + ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ + RegistrationID: regId, + Identifiers: []*corepb.Identifier{ + { + Type: string(iden.Type), + Value: iden.Value, + }, + }, + }) + } + } // PerformValidation initiates validation for a specific challenge associated @@ -1992,9 +2012,15 @@ func (ra *RegistrationAuthorityImpl) PerformValidation( if prob != nil { challenge.Status = core.StatusInvalid challenge.Error = prob - go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier.Value) + go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier) } else { challenge.Status = core.StatusValid + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.IssuancePausedPerDomainPerAccount, authz.RegistrationID, authz.Identifier.Value) + if err != nil { + ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", + authz.RegistrationID, authz.ID, err) + } + ra.limiter.Reset(ctx, bucketKey) } challenge.Validated = &vStart authz.Challenges[challIndex] = *challenge diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 6360eb7cd6a..26dc4bdd4a1 100644 --- a/ratelimits/bucket.go +++ b/ratelimits/bucket.go @@ -64,9 +64,9 @@ func newDomainBucketKey(name Name, orderName string) (string, error) { return joinWithColon(name.EnumString(), orderName), nil } -// newRegIdDomainBucketKey validates and returns a bucketKey for limits that use -// the 'enum:regId:domain' bucket key format. -func newRegIdDomainBucketKey(name Name, regId int64, orderName string) (string, error) { +// NewRegIdDomainBucketKey validates and returns a bucketKey for limits that use +// the 'enum:regId:domain' bucket key format. This function is exported for use in ra.PerformValidation +func NewRegIdDomainBucketKey(name Name, regId int64, orderName string) (string, error) { regIdStr := strconv.FormatInt(regId, 10) err := validateIdForName(name, joinWithColon(regIdStr, orderName)) if err != nil { @@ -257,7 +257,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckO for _, name := range orderDomains { // FailedAuthorizationsPerDomainPerAccount limit uses the // 'enum:regId:domain' bucket key format for transactions. - perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, name) + perDomainPerAccountBucketKey, err := NewRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, name) if err != nil { return nil, err } @@ -292,7 +292,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO // FailedAuthorizationsPerDomainPerAccount limit uses the // 'enum:regId:domain' bucket key format for transactions. - perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, orderDomain) + perDomainPerAccountBucketKey, err := NewRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, orderDomain) if err != nil { return Transaction{}, err } @@ -304,50 +304,11 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO return txn, nil } -// IssuancePausedPerDomainPerAccountCheckOnlyTransactions returns a slice -// of Transactions for the provided order domain names. An error is returned if -// any of the order domain names are invalid. This method should be used for -// checking capacity, before allowing more authorizations to be created. -// -// Precondition: len(orderDomains) < maxNames. -func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountCheckOnlyTransactions(regId int64, orderDomains []string) ([]Transaction, error) { - // IssuancePausedPerDomainPerAccount limit uses the 'enum:regId' - // bucket key format for overrides. - perAccountBucketKey, err := newRegIdBucketKey(IssuancePausedPerDomainPerAccount, regId) - if err != nil { - return nil, err - } - limit, err := builder.getLimit(IssuancePausedPerDomainPerAccount, perAccountBucketKey) - if err != nil && !errors.Is(err, errLimitDisabled) { - return nil, err - } - - var txns []Transaction - for _, name := range orderDomains { - // IssuancePausedPerDomainPerAccount limit uses the - // 'enum:regId:domain' bucket key format for transactions. - perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, name) - if err != nil { - return nil, err - } - - // Add a check-only transaction for each per domain per account bucket. - // The cost is 0, as we are only checking that the account and domain - // pair aren't already over the limit. - txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 1) - if err != nil { - return nil, err - } - txns = append(txns, txn) - } - return txns, nil -} - -// IssuancePausedPerDomainPerAccountSpendOnlyTransaction returns a spend- -// only Transaction for the provided order domain name. An error is returned if +// IssuancePausedPerDomainPerAccountTransaction returns a +// Transaction for the provided order domain name. An error is returned if // the order domain name is invalid. This method should be used for spending // capacity, as a result of a failed authorization. -func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountSpendOnlyTransaction(regId int64, orderDomain string) (Transaction, error) { +func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountTransaction(regId int64, orderDomain string) (Transaction, error) { // IssuancePausedPerDomainPerAccount limit uses the 'enum:regId' // bucket key format for overrides. perAccountBucketKey, err := newRegIdBucketKey(IssuancePausedPerDomainPerAccount, regId) @@ -361,11 +322,12 @@ func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountSpendOnlyTra // IssuancePausedPerDomainPerAccount limit uses the // 'enum:regId:domain' bucket key format for transactions. - perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, orderDomain) + perDomainPerAccountBucketKey, err := NewRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, orderDomain) if err != nil { return Transaction{}, err } - txn, err := newSpendOnlyTransaction(limit, perDomainPerAccountBucketKey, 1) + + txn, err := newTransaction(limit, perDomainPerAccountBucketKey, 1) if err != nil { return Transaction{}, err } @@ -402,7 +364,7 @@ func (builder *TransactionBuilder) certificatesPerDomainCheckOnlyTransactions(re if perAccountLimit.isOverride() { // An override is configured for the CertificatesPerDomainPerAccount // limit. - perAccountPerDomainKey, err := newRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) + perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) if err != nil { return nil, err } @@ -468,7 +430,7 @@ func (builder *TransactionBuilder) CertificatesPerDomainSpendOnlyTransactions(re if perAccountLimit.isOverride() { // An override is configured for the CertificatesPerDomainPerAccount // limit. - perAccountPerDomainKey, err := newRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) + perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) if err != nil { return nil, err } diff --git a/ratelimits/bucket_test.go b/ratelimits/bucket_test.go index 39f7bc1e110..def28435b35 100644 --- a/ratelimits/bucket_test.go +++ b/ratelimits/bucket_test.go @@ -108,34 +108,11 @@ func TestIssuancePausedPerDomainPerAccountTransactions(t *testing.T) { tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") test.AssertNotError(t, err, "creating TransactionBuilder") - // A check-only transaction for the default per-account limit. - txns, err := tb.IssuancePausedPerDomainPerAccountCheckOnlyTransactions(123456789, []string{"so.many.labels.here.example.com"}) - test.AssertNotError(t, err, "creating transactions") - test.AssertEquals(t, len(txns), 1) - test.AssertEquals(t, txns[0].bucketKey, "8:123456789:so.many.labels.here.example.com") - test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, !txns[0].limit.isOverride(), "should not be an override") - - // A spend-only transaction for the default per-account limit. - txn, err := tb.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(123456789, "so.many.labels.here.example.com") - test.AssertNotError(t, err, "creating transaction") - test.AssertEquals(t, txn.bucketKey, "8:123456789:so.many.labels.here.example.com") - test.Assert(t, txn.spendOnly(), "should be spend-only") - test.Assert(t, !txn.limit.isOverride(), "should not be an override") - - // A check-only transaction for the per-account limit override. - txns, err = tb.IssuancePausedPerDomainPerAccountCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com"}) - test.AssertNotError(t, err, "creating transactions") - test.AssertEquals(t, len(txns), 1) - test.AssertEquals(t, txns[0].bucketKey, "8:13371338:so.many.labels.here.example.com") - test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, txns[0].limit.isOverride(), "should be an override") - - // A spend-only transaction for the per-account limit override. - txn, err = tb.IssuancePausedPerDomainPerAccountSpendOnlyTransaction(13371338, "so.many.labels.here.example.com") + // A transaction for the per-account limit override. + txn, err := tb.IssuancePausedPerDomainPerAccountTransaction(13371338, "so.many.labels.here.example.com") test.AssertNotError(t, err, "creating transaction") test.AssertEquals(t, txn.bucketKey, "8:13371338:so.many.labels.here.example.com") - test.Assert(t, txn.spendOnly(), "should be spend-only") + test.Assert(t, txn.check && txn.spend, "should be check and spend") test.Assert(t, txn.limit.isOverride(), "should be an override") } diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 484f5f3bf23..3756a8069ff 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -153,7 +153,7 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) - case IssuancePausedPerDomainPerAccount: + case IssuancePausedPerDomainPerAccount: // Uses bucket key 'enum:regId:domain'. idx := strings.LastIndex(d.transaction.bucketKey, ":") if idx == -1 { From 03c3e711f443595127d20de397e97b714e1155ba Mon Sep 17 00:00:00 2001 From: kruti-s Date: Thu, 24 Oct 2024 16:13:17 -0400 Subject: [PATCH 07/29] addressed comments for ra.go --- ra/ra.go | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 0e94f9b99a9..b966ddcea97 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1835,10 +1835,11 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI } // countFailedValidation increments the failed authorizations per domain per -// account rate limit. There is no reason to surface errors from this function -// to the Subscriber, spends against this limit are best effort. -func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, iden identifier.ACMEIdentifier) { - var name = iden.Value +// account rate limit. It also increments the issuance paused counter per +// domain per account rate limit. There is no reason to surface errors from +// this function to the Subscriber, spends against this limit are best effort. +func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { + var name = ident.Value if ra.limiter == nil || ra.txnBuilder == nil { // Limiter is disabled. @@ -1871,19 +1872,14 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) } - // TODO: is this how we want to catch this error - if decision == nil { - ra.log.Warningf("decision and error are both nil for the %s rate limit", ratelimits.IssuancePausedPerDomainPerAccount) - return - } if decision.Result(ra.clk.Now()) != nil { ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ RegistrationID: regId, Identifiers: []*corepb.Identifier{ { - Type: string(iden.Type), - Value: iden.Value, + Type: string(ident.Type), + Value: ident.Value, }, }, }) @@ -1891,6 +1887,18 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } +// resetAccountPausingLimit resets bucket to maximum capacity for given account. +// There is no reason to surface errors from this function to the Subscriber + +func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.IssuancePausedPerDomainPerAccount, regId, ident.Value) + if err != nil { + ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", + regId, ident, err) + } + ra.limiter.Reset(ctx, bucketKey) +} + // PerformValidation initiates validation for a specific challenge associated // with the given base authorization. The authorization and challenge are // updated based on the results. @@ -2015,12 +2023,7 @@ func (ra *RegistrationAuthorityImpl) PerformValidation( go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier) } else { challenge.Status = core.StatusValid - bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.IssuancePausedPerDomainPerAccount, authz.RegistrationID, authz.Identifier.Value) - if err != nil { - ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", - authz.RegistrationID, authz.ID, err) - } - ra.limiter.Reset(ctx, bucketKey) + ra.resetAccountPausingLimit(vaCtx, authz.RegistrationID, authz.Identifier) } challenge.Validated = &vStart authz.Challenges[challIndex] = *challenge From d860f88fd79d4952d9f48f6c71c7526181d3f513 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Thu, 24 Oct 2024 16:51:11 -0400 Subject: [PATCH 08/29] change IssuancePausedPerDomainPerAccount to FailedAuthorizationsForPausingPerDomainPerAccount --- ra/ra.go | 10 ++++---- ratelimits/limiter.go | 2 +- ratelimits/limiter_test.go | 4 ++-- ratelimits/names.go | 24 +++++++++---------- ratelimits/names_test.go | 10 ++++---- .../testdata/working_override_13371338.yml | 2 +- ratelimits/testdata/working_overrides.yml | 2 +- ratelimits/transaction.go | 14 +++++------ ratelimits/transaction_test.go | 4 ++-- test/config-next/wfe2-ratelimit-defaults.yml | 2 +- 10 files changed, 37 insertions(+), 37 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index ccca1864155..fbbc4fe3f45 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1769,7 +1769,7 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI } // countFailedValidation increments the failed authorizations per domain per -// account rate limit. It also increments the issuance paused counter per +// account rate limit. It also increments the failed authorizations for pausing per // domain per account rate limit. There is no reason to surface errors from // this function to the Subscriber, spends against this limit are best effort. func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { @@ -1794,9 +1794,9 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } // Increment ratelimit for IssuancePaused - txn, err = ra.txnBuilder.IssuancePausedPerDomainPerAccountTransaction(regId, name) + txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, name) if err != nil { - ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) + ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } decision, err := ra.limiter.Spend(ctx, txn) @@ -1804,7 +1804,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return } - ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.IssuancePausedPerDomainPerAccount, err) + ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } if decision.Result(ra.clk.Now()) != nil { @@ -1825,7 +1825,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, // There is no reason to surface errors from this function to the Subscriber func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { - bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.IssuancePausedPerDomainPerAccount, regId, ident.Value) + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, regId, ident.Value) if err != nil { ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", regId, ident, err) diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 3756a8069ff..9e1f7e9b8fd 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -153,7 +153,7 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) - case IssuancePausedPerDomainPerAccount: + case FailedAuthorizationsForPausingPerDomainPerAccount: // Uses bucket key 'enum:regId:domain'. idx := strings.LastIndex(d.transaction.bucketKey, ":") if idx == -1 { diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 45b3d262df3..5101eeb7ab1 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -526,13 +526,13 @@ func TestRateLimitError(t *testing.T) { expectedErrType: berrors.RateLimit, }, { - name: "IssuancePausedPerDomainPerAccount limit reached", + name: "FailedAuthorizationsForPausingPerDomainPerAccount limit reached", decision: &Decision{ allowed: false, retryIn: 15 * time.Second, transaction: Transaction{ limit: limit{ - name: IssuancePausedPerDomainPerAccount, + name: FailedAuthorizationsForPausingPerDomainPerAccount, Burst: 7, Period: config.Duration{Duration: time.Hour}, }, diff --git a/ratelimits/names.go b/ratelimits/names.go index 5b390a6f671..a44b16fd490 100644 --- a/ratelimits/names.go +++ b/ratelimits/names.go @@ -77,21 +77,21 @@ const ( // passed as a comma-separated list of domain names. CertificatesPerFQDNSet - // TODO: @kruti-s - IssuancePausedPerDomainPerAccount + // TODO: @kruti-s + FailedAuthorizationsForPausingPerDomainPerAccount ) // nameToString is a map of Name values to string names. var nameToString = map[Name]string{ - Unknown: "Unknown", - NewRegistrationsPerIPAddress: "NewRegistrationsPerIPAddress", - NewRegistrationsPerIPv6Range: "NewRegistrationsPerIPv6Range", - NewOrdersPerAccount: "NewOrdersPerAccount", - FailedAuthorizationsPerDomainPerAccount: "FailedAuthorizationsPerDomainPerAccount", - CertificatesPerDomain: "CertificatesPerDomain", - CertificatesPerDomainPerAccount: "CertificatesPerDomainPerAccount", - CertificatesPerFQDNSet: "CertificatesPerFQDNSet", - IssuancePausedPerDomainPerAccount: "IssuancePausedPerDomainPerAccount", + Unknown: "Unknown", + NewRegistrationsPerIPAddress: "NewRegistrationsPerIPAddress", + NewRegistrationsPerIPv6Range: "NewRegistrationsPerIPv6Range", + NewOrdersPerAccount: "NewOrdersPerAccount", + FailedAuthorizationsPerDomainPerAccount: "FailedAuthorizationsPerDomainPerAccount", + CertificatesPerDomain: "CertificatesPerDomain", + CertificatesPerDomainPerAccount: "CertificatesPerDomainPerAccount", + CertificatesPerFQDNSet: "CertificatesPerFQDNSet", + FailedAuthorizationsForPausingPerDomainPerAccount: "FailedAuthorizationsForPausingPerDomainPerAccount", } // isValid returns true if the Name is a valid rate limit name. @@ -235,7 +235,7 @@ func validateIdForName(name Name, id string) error { // 'enum:fqdnSet' return validateFQDNSet(id) - case IssuancePausedPerDomainPerAccount: + case FailedAuthorizationsForPausingPerDomainPerAccount: if strings.Contains(id, ":") { // 'enum:regId:domain' for transaction return validateRegIdDomain(id) diff --git a/ratelimits/names_test.go b/ratelimits/names_test.go index 51c049dcc43..89eca6baa56 100644 --- a/ratelimits/names_test.go +++ b/ratelimits/names_test.go @@ -135,29 +135,29 @@ func TestValidateIdForName(t *testing.T) { err: "invalid regId", }, { - limit: IssuancePausedPerDomainPerAccount, + limit: FailedAuthorizationsForPausingPerDomainPerAccount, desc: "transaction: valid regId and domain", id: "12345:example.com", }, { - limit: IssuancePausedPerDomainPerAccount, + limit: FailedAuthorizationsForPausingPerDomainPerAccount, desc: "transaction: invalid regId", id: "12ea5:example.com", err: "invalid regId", }, { - limit: IssuancePausedPerDomainPerAccount, + limit: FailedAuthorizationsForPausingPerDomainPerAccount, desc: "transaction: invalid domain", id: "12345:examplecom", err: "name needs at least one dot", }, { - limit: IssuancePausedPerDomainPerAccount, + limit: FailedAuthorizationsForPausingPerDomainPerAccount, desc: "override: valid regId", id: "12345", }, { - limit: IssuancePausedPerDomainPerAccount, + limit: FailedAuthorizationsForPausingPerDomainPerAccount, desc: "override: invalid regId", id: "12ea5", err: "invalid regId", diff --git a/ratelimits/testdata/working_override_13371338.yml b/ratelimits/testdata/working_override_13371338.yml index a4c17cf4774..03280a5357e 100644 --- a/ratelimits/testdata/working_override_13371338.yml +++ b/ratelimits/testdata/working_override_13371338.yml @@ -12,7 +12,7 @@ ids: - id: 13371338 comment: Used to test the TransactionBuilder -- IssuancePausedPerDomainPerAccount: +- FailedAuthorizationsForPausingPerDomainPerAccount: burst: 1337 count: 1337 period: 5m diff --git a/ratelimits/testdata/working_overrides.yml b/ratelimits/testdata/working_overrides.yml index a4b507ec076..f73a5c1893b 100644 --- a/ratelimits/testdata/working_overrides.yml +++ b/ratelimits/testdata/working_overrides.yml @@ -22,7 +22,7 @@ - id: 5678 comment: Foo -- IssuancePausedPerDomainPerAccount: +- FailedAuthorizationsForPausingPerDomainPerAccount: burst: 60 count: 60 period: 3s diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index 26dc4bdd4a1..c3c91272403 100644 --- a/ratelimits/transaction.go +++ b/ratelimits/transaction.go @@ -304,25 +304,25 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO return txn, nil } -// IssuancePausedPerDomainPerAccountTransaction returns a +// FailedAuthorizationsForPausingPerDomainPerAccountTransaction returns a // Transaction for the provided order domain name. An error is returned if // the order domain name is invalid. This method should be used for spending // capacity, as a result of a failed authorization. -func (builder *TransactionBuilder) IssuancePausedPerDomainPerAccountTransaction(regId int64, orderDomain string) (Transaction, error) { - // IssuancePausedPerDomainPerAccount limit uses the 'enum:regId' +func (builder *TransactionBuilder) FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId int64, orderDomain string) (Transaction, error) { + // FailedAuthorizationsForPausingPerDomainPerAccount limit uses the 'enum:regId' // bucket key format for overrides. - perAccountBucketKey, err := newRegIdBucketKey(IssuancePausedPerDomainPerAccount, regId) + perAccountBucketKey, err := newRegIdBucketKey(FailedAuthorizationsForPausingPerDomainPerAccount, regId) if err != nil { return Transaction{}, err } - limit, err := builder.getLimit(IssuancePausedPerDomainPerAccount, perAccountBucketKey) + limit, err := builder.getLimit(FailedAuthorizationsForPausingPerDomainPerAccount, perAccountBucketKey) if err != nil && !errors.Is(err, errLimitDisabled) { return Transaction{}, err } - // IssuancePausedPerDomainPerAccount limit uses the + // FailedAuthorizationsForPausingPerDomainPerAccount limit uses the // 'enum:regId:domain' bucket key format for transactions. - perDomainPerAccountBucketKey, err := NewRegIdDomainBucketKey(IssuancePausedPerDomainPerAccount, regId, orderDomain) + perDomainPerAccountBucketKey, err := NewRegIdDomainBucketKey(FailedAuthorizationsForPausingPerDomainPerAccount, regId, orderDomain) if err != nil { return Transaction{}, err } diff --git a/ratelimits/transaction_test.go b/ratelimits/transaction_test.go index def28435b35..f8003e2f404 100644 --- a/ratelimits/transaction_test.go +++ b/ratelimits/transaction_test.go @@ -102,14 +102,14 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { test.Assert(t, txn.limit.isOverride(), "should be an override") } -func TestIssuancePausedPerDomainPerAccountTransactions(t *testing.T) { +func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testing.T) { t.Parallel() tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") test.AssertNotError(t, err, "creating TransactionBuilder") // A transaction for the per-account limit override. - txn, err := tb.IssuancePausedPerDomainPerAccountTransaction(13371338, "so.many.labels.here.example.com") + txn, err := tb.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(13371338, "so.many.labels.here.example.com") test.AssertNotError(t, err, "creating transaction") test.AssertEquals(t, txn.bucketKey, "8:13371338:so.many.labels.here.example.com") test.Assert(t, txn.check && txn.spend, "should be check and spend") diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index db594bc3f2a..2ea50039224 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -14,7 +14,7 @@ FailedAuthorizationsPerDomainPerAccount: count: 3 burst: 3 period: 5m -IssuancePausedPerDomainPerAccount: +FailedAuthorizationsForPausingPerDomainPerAccount: count: 3600 burst: 3600 period: 960h From 542b7c57b83b374c2c0794bac45115df626e7ebc Mon Sep 17 00:00:00 2001 From: kruti-s Date: Thu, 24 Oct 2024 17:23:31 -0400 Subject: [PATCH 09/29] change count and period for FailedAuthorizationsForPausingPerDomainPerAccount --- test/config-next/wfe2-ratelimit-defaults.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index 2ea50039224..d8894b85a45 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -14,10 +14,16 @@ FailedAuthorizationsPerDomainPerAccount: count: 3 burst: 3 period: 5m +# 40 failures per day across 90 days for FailedAuthorizationsForPausingPerDomainPerAccount +# If someone fails the fastest they can possibly fail (5 times per hour, or 120 times per day, as enforced by the existing FailedAuthorizations limit), then they'll burn through the burst in 30 days, and burn through those 30 extra tokens in a few hours. 30 days is the fastest we want to pause anyone +# If someone fails 40 times per day (i.e. right at our intended threshold), they'll get paused after ~92 days: 90 days to burn through their burst, and then a little bit more time to burn through the 90 tokens they've accumulated during that time. +# If someone fails 30 times per day, just below our target threshold, they'll get paused after about 125 days +# If someone fails 4 times per day, they'll take 900 days to burn through their initial burst, during which time they'll earn 900 more tokens, which they'll burn through in 225 days, during which time they'll end up being paused after about 1200 days, or just over 3 years. +# If someone fails once per day, they'll never be paused. FailedAuthorizationsForPausingPerDomainPerAccount: - count: 3600 + count: 1 burst: 3600 - period: 960h + period: 24h NewOrdersPerAccount: count: 1500 burst: 1500 From 35a6f6379cd90bc12f1dbbd5a7281b4106912fed Mon Sep 17 00:00:00 2001 From: kruti-s Date: Fri, 25 Oct 2024 15:02:11 -0400 Subject: [PATCH 10/29] override and default explanation changes --- ratelimits/testdata/working_override_13371338.yml | 6 +++--- test/config-next/wfe2-ratelimit-defaults.yml | 14 ++++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ratelimits/testdata/working_override_13371338.yml b/ratelimits/testdata/working_override_13371338.yml index 03280a5357e..14f1cf7eebb 100644 --- a/ratelimits/testdata/working_override_13371338.yml +++ b/ratelimits/testdata/working_override_13371338.yml @@ -13,9 +13,9 @@ - id: 13371338 comment: Used to test the TransactionBuilder - FailedAuthorizationsForPausingPerDomainPerAccount: - burst: 1337 - count: 1337 - period: 5m + burst: 500 + count: 500 + period: 12h ids: - id: 13371338 comment: Used to test the TransactionBuilder diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index d8894b85a45..e461607f4cb 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -14,12 +14,14 @@ FailedAuthorizationsPerDomainPerAccount: count: 3 burst: 3 period: 5m -# 40 failures per day across 90 days for FailedAuthorizationsForPausingPerDomainPerAccount -# If someone fails the fastest they can possibly fail (5 times per hour, or 120 times per day, as enforced by the existing FailedAuthorizations limit), then they'll burn through the burst in 30 days, and burn through those 30 extra tokens in a few hours. 30 days is the fastest we want to pause anyone -# If someone fails 40 times per day (i.e. right at our intended threshold), they'll get paused after ~92 days: 90 days to burn through their burst, and then a little bit more time to burn through the 90 tokens they've accumulated during that time. -# If someone fails 30 times per day, just below our target threshold, they'll get paused after about 125 days -# If someone fails 4 times per day, they'll take 900 days to burn through their initial burst, during which time they'll earn 900 more tokens, which they'll burn through in 225 days, during which time they'll end up being paused after about 1200 days, or just over 3 years. -# If someone fails once per day, they'll never be paused. +# The burst represents failing 40 times per day for 90 days. +# The count and period grant one "freebie" failure per day. +# In combination, these parameters mean that: +# - Failing 120 times per day results in being paused after 30.25 days +# - Failing 40 times per day results in being paused after 92.3 days +# - Failing 20 times per day results in being paused after 6.2 months +# - Failing 4 times per day results in being paused after 3.3 years +# - Failing once per day results in never being paused FailedAuthorizationsForPausingPerDomainPerAccount: count: 1 burst: 3600 From 6c4e2fe49f1f6171ace0da9a5b95306119dc838c Mon Sep 17 00:00:00 2001 From: kruti-s Date: Fri, 25 Oct 2024 15:44:09 -0400 Subject: [PATCH 11/29] change working override test vals --- ratelimits/testdata/working_override_13371338.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ratelimits/testdata/working_override_13371338.yml b/ratelimits/testdata/working_override_13371338.yml index 14f1cf7eebb..97327e510d6 100644 --- a/ratelimits/testdata/working_override_13371338.yml +++ b/ratelimits/testdata/working_override_13371338.yml @@ -13,9 +13,9 @@ - id: 13371338 comment: Used to test the TransactionBuilder - FailedAuthorizationsForPausingPerDomainPerAccount: - burst: 500 - count: 500 - period: 12h + burst: 1337 + count: 1 + period: 24h ids: - id: 13371338 comment: Used to test the TransactionBuilder From 061f4987a3e273932d5ad43be285e01b094a86d3 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Wed, 30 Oct 2024 16:25:44 -0400 Subject: [PATCH 12/29] limiter test fixes --- ratelimits/limiter.go | 2 +- ratelimits/limiter_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index d6973df2716..504619b0b01 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -160,7 +160,7 @@ func (d *Decision) Result(now time.Time) error { return berrors.InternalServerError("unrecognized bucket key while generating error") } domain := d.transaction.bucketKey[idx+1:] - return berrors.FailedValidationError( + return berrors.FailedAuthorizationsPerDomainPerAccountError( retryAfter, "too many failed validation attempts (%d) for %q in the last %s, retry after %s", d.transaction.limit.Burst, diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 8ea67e05359..b9bd9fc70fe 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -538,7 +538,7 @@ func TestRateLimitError(t *testing.T) { bucketKey: "4:12345:example.com", }, }, - expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", + expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", expectedErrType: berrors.RateLimit, }, { From a5ce802ea94c5fced99b8318b4c7ecd73ea14a7a Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 30 Oct 2024 13:34:34 -0700 Subject: [PATCH 13/29] Fix test --- ratelimits/limiter_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index b9bd9fc70fe..3b209b9a5e9 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -538,7 +538,7 @@ func TestRateLimitError(t *testing.T) { bucketKey: "4:12345:example.com", }, }, - expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", + expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", expectedErrType: berrors.RateLimit, }, { @@ -555,7 +555,7 @@ func TestRateLimitError(t *testing.T) { bucketKey: "8:12345:example.com", }, }, - expectedErr: "too many failed validation attempts (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC", + expectedErr: "too many failed validation attempts (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", expectedErrType: berrors.RateLimit, }, { From 8a28d9ccd743c6d1d6351c44b53bc68a89d50fa4 Mon Sep 17 00:00:00 2001 From: kruti-s Date: Wed, 30 Oct 2024 17:06:44 -0400 Subject: [PATCH 14/29] started writing TesetResetAccountPausingLimit in ra_test.go --- ra/ra_test.go | 90 +++++++++++++++++++++++++++++++++++++++++---- ratelimits/limit.go | 2 +- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index ca045cb1d00..c50bd11528c 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -217,14 +217,15 @@ var ctx = context.Background() // dummyRateLimitConfig satisfies the rl.RateLimitConfig interface while // allowing easy mocking of the individual RateLimitPolicy's type dummyRateLimitConfig struct { - CertificatesPerNamePolicy ratelimit.RateLimitPolicy - RegistrationsPerIPPolicy ratelimit.RateLimitPolicy - RegistrationsPerIPRangePolicy ratelimit.RateLimitPolicy - PendingAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy - NewOrdersPerAccountPolicy ratelimit.RateLimitPolicy - InvalidAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy - CertificatesPerFQDNSetPolicy ratelimit.RateLimitPolicy - CertificatesPerFQDNSetFastPolicy ratelimit.RateLimitPolicy + CertificatesPerNamePolicy ratelimit.RateLimitPolicy + RegistrationsPerIPPolicy ratelimit.RateLimitPolicy + RegistrationsPerIPRangePolicy ratelimit.RateLimitPolicy + PendingAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy + NewOrdersPerAccountPolicy ratelimit.RateLimitPolicy + InvalidAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy + CertificatesPerFQDNSetPolicy ratelimit.RateLimitPolicy + CertificatesPerFQDNSetFastPolicy ratelimit.RateLimitPolicy + FailedAuthorizationsForPausingPerDomainPerAccountPolicy ratelimit.RateLimitPolicy } func (r *dummyRateLimitConfig) CertificatesPerName() ratelimit.RateLimitPolicy { @@ -880,6 +881,79 @@ func TestPerformValidationVAError(t *testing.T) { test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") } +func TestResetAccountPausingLimit(t *testing.T) { + va, sa, ra, fc, cleanUp := initAuthorities(t) + defer cleanUp() + + ra.orderLifetime = 5 * 24 * time.Hour + + // Set up a rate limit policy that allows 1 order every 5 minutes. + rateLimitDuration := 5 * time.Minute + ra.rlPolicies = &dummyRateLimitConfig{ + FailedAuthorizationsForPausingPerDomainPerAccountPolicy: ratelimit.RateLimitPolicy{ + Threshold: 1, + Window: config.Duration{Duration: rateLimitDuration}, + }, + } + + authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + + va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong") + + challIdx := dnsChallIdx(t, authzPB.Challenges) + authzPB, err := ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + Authz: authzPB, + ChallengeIndex: challIdx, + }) + + test.AssertNotError(t, err, "PerformValidation completely failed") + + var vaRequest *vapb.PerformValidationRequest + select { + case r := <-va.performValidationRequest: + vaRequest = r + case <-time.After(time.Second): + t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") + } + + // Verify that the VA got the request, and it's the same as the others + test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) + test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) + + // Sleep so the RA has a chance to write to the SA + time.Sleep(100 * time.Millisecond) + + dbAuthzPB := getAuthorization(t, authzPB.Id, sa) + t.Log("dbAuthz:", dbAuthzPB) + + // Verify that the responses are reflected + challIdx = dnsChallIdx(t, dbAuthzPB.Challenges) + challenge, err := bgrpc.PBToChallenge(dbAuthzPB.Challenges[challIdx]) + test.AssertNotError(t, err, "Failed to marshall corepb.Challenge to core.Challenge.") + test.Assert(t, challenge.Status == core.StatusInvalid, "challenge was not marked as invalid") + test.AssertContains(t, challenge.Error.Error(), "Could not communicate with VA") + test.Assert(t, challenge.ValidationRecord == nil, "challenge had a ValidationRecord") + + // Check that validated timestamp was recorded, stored, and retrieved + expectedValidated := fc.Now() + test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") + + authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + Authz: authzPB, + ChallengeIndex: challIdx, + }) + + test.AssertNotError(t, err, "PerformValidation completely failed") + + select { + case r := <-va.performValidationRequest: + vaRequest = r + case <-time.After(time.Second): + t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") + } + +} + func TestCertificateKeyNotEqualAccountKey(t *testing.T) { _, sa, ra, _, cleanUp := initAuthorities(t) defer cleanUp() diff --git a/ratelimits/limit.go b/ratelimits/limit.go index df2cd268c55..f90a71082fa 100644 --- a/ratelimits/limit.go +++ b/ratelimits/limit.go @@ -172,7 +172,7 @@ func loadAndParseOverrideLimits(path string) (limits, error) { } limit.overrideKey = joinWithColon(name.EnumString(), id) if name == CertificatesPerFQDNSet { - // FQDNSet hashes are not a nice thing to ask for in a + // FQDNSet hashs are not a nice thing to ask for in a // config file, so we allow the user to specify a // comma-separated list of FQDNs and compute the hash here. id = fmt.Sprintf("%x", core.HashNames(strings.Split(id, ","))) From a207b74f6d3d3d2ad9df19a9fee462b951adaa53 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 31 Oct 2024 14:54:43 -0400 Subject: [PATCH 15/29] Progress --- ra/ra.go | 5 + ra/ra_test.go | 150 +++++++++++------- ...w-one-failed-validation-before-pausing.yml | 4 + 3 files changed, 106 insertions(+), 53 deletions(-) create mode 100644 ra/testdata/only-allow-one-failed-validation-before-pausing.yml diff --git a/ra/ra.go b/ra/ra.go index bf81f7a543b..9c971b5c672 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1780,6 +1780,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, return } + fmt.Println("countFailedValidation 1") txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, name) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) @@ -1792,6 +1793,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } + fmt.Println("countFailedValidation 2") // Increment ratelimit for IssuancePaused txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, name) @@ -1799,6 +1801,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } + fmt.Println("countFailedValidation 3") decision, err := ra.limiter.Spend(ctx, txn) if err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { @@ -1807,7 +1810,9 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } + fmt.Printf("countFailedValidation 4, %v\n", ra.clk.Now()) if decision.Result(ra.clk.Now()) != nil { + fmt.Println("countFailedValidation 5") ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ RegistrationID: regId, Identifiers: []*corepb.Identifier{ diff --git a/ra/ra_test.go b/ra/ra_test.go index c50bd11528c..d3794fab2b6 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -217,15 +217,14 @@ var ctx = context.Background() // dummyRateLimitConfig satisfies the rl.RateLimitConfig interface while // allowing easy mocking of the individual RateLimitPolicy's type dummyRateLimitConfig struct { - CertificatesPerNamePolicy ratelimit.RateLimitPolicy - RegistrationsPerIPPolicy ratelimit.RateLimitPolicy - RegistrationsPerIPRangePolicy ratelimit.RateLimitPolicy - PendingAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy - NewOrdersPerAccountPolicy ratelimit.RateLimitPolicy - InvalidAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy - CertificatesPerFQDNSetPolicy ratelimit.RateLimitPolicy - CertificatesPerFQDNSetFastPolicy ratelimit.RateLimitPolicy - FailedAuthorizationsForPausingPerDomainPerAccountPolicy ratelimit.RateLimitPolicy + CertificatesPerNamePolicy ratelimit.RateLimitPolicy + RegistrationsPerIPPolicy ratelimit.RateLimitPolicy + RegistrationsPerIPRangePolicy ratelimit.RateLimitPolicy + PendingAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy + NewOrdersPerAccountPolicy ratelimit.RateLimitPolicy + InvalidAuthorizationsPerAccountPolicy ratelimit.RateLimitPolicy + CertificatesPerFQDNSetPolicy ratelimit.RateLimitPolicy + CertificatesPerFQDNSetFastPolicy ratelimit.RateLimitPolicy } func (r *dummyRateLimitConfig) CertificatesPerName() ratelimit.RateLimitPolicy { @@ -834,21 +833,65 @@ func TestPerformValidationSuccess(t *testing.T) { test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") } -func TestPerformValidationVAError(t *testing.T) { +// mockSAPaused is a mock which always succeeds. It records the PauseRequest it +// received, and returns the number of identifiers as a +// PauseIdentifiersResponse. It does not maintain state of repaused identifiers. +type mockSAPaused struct { + sapb.StorageAuthorityClient +} + +func (msa *mockSAPaused) GetRegistration(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*corepb.Registration, error) { + fmt.Println("Here GetRegistration") + return Registration, nil +} + +func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, in *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { + fmt.Println("Here PausedIdentifiers") + return &sapb.PauseIdentifiersResponse{Paused: int64(len(in.Identifiers))}, nil +} + +func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.FinalizeAuthorizationRequest, _ ...grpc.CallOption) (*emptypb.Empty, error) { + fmt.Println("Here FinalizeAuthorization2") + return &emptypb.Empty{}, nil +} + +func TestResetAccountPausingLimit(t *testing.T) { va, sa, ra, fc, cleanUp := initAuthorities(t) defer cleanUp() + mockSA := &mockSAPaused{} + ra.SA = mockSA + + // Override the default ratelimits + txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/only-allow-one-failed-validation-before-pausing.yml", "") + test.AssertNotError(t, err, "making transaction composer") + ra.txnBuilder = txnBuilder + + // We know this is OK because of TestNewAuthorization authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) - va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong") + // TODO(Phil): Explain why this has a problem filled out + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ + Records: []*corepb.ValidationRecord{ + { + AddressUsed: []byte("192.168.0.1"), + Hostname: "example.com", + Port: "8080", + Url: "http://example.com/", + ResolverAddrs: []string{"rebound"}, + }, + }, + Problems: &corepb.ProblemDetails{ + Detail: "CAA invalid for example.com", + }, + } challIdx := dnsChallIdx(t, authzPB.Challenges) - authzPB, err := ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ Authz: authzPB, ChallengeIndex: challIdx, }) - - test.AssertNotError(t, err, "PerformValidation completely failed") + test.AssertNotError(t, err, "PerformValidation failed") var vaRequest *vapb.PerformValidationRequest select { @@ -865,37 +908,53 @@ func TestPerformValidationVAError(t *testing.T) { // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) - dbAuthzPB := getAuthorization(t, authzPB.Id, sa) - t.Log("dbAuthz:", dbAuthzPB) + // A second failed validation should result in the identifier being paused + // due to the strict ratelimit. + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ + Records: []*corepb.ValidationRecord{ + { + AddressUsed: []byte("192.168.0.1"), + Hostname: "example.com", + Port: "8080", + Url: "http://example.com/", + ResolverAddrs: []string{"rebound"}, + }, + }, + Problems: &corepb.ProblemDetails{ + Detail: "CAA invalid for example.com", + }, + } - // Verify that the responses are reflected - challIdx = dnsChallIdx(t, dbAuthzPB.Challenges) - challenge, err := bgrpc.PBToChallenge(dbAuthzPB.Challenges[challIdx]) - test.AssertNotError(t, err, "Failed to marshall corepb.Challenge to core.Challenge.") - test.Assert(t, challenge.Status == core.StatusInvalid, "challenge was not marked as invalid") - test.AssertContains(t, challenge.Error.Error(), "Could not communicate with VA") - test.Assert(t, challenge.ValidationRecord == nil, "challenge had a ValidationRecord") + challIdx = dnsChallIdx(t, authzPB.Challenges) + authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + Authz: authzPB, + ChallengeIndex: challIdx, + }) + test.AssertNotError(t, err, "PerformValidation failed") - // Check that validated timestamp was recorded, stored, and retrieved - expectedValidated := fc.Now() - test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") + select { + case r := <-va.performValidationRequest: + vaRequest = r + case <-time.After(time.Second): + t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") + } + + // Verify that the VA got the request, and it's the same as the others + test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) + test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) + + // Sleep so the RA has a chance to write to the SA + time.Sleep(100 * time.Millisecond) + + identifiers, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: 1}, nil) + test.AssertNotError(t, err, "Should not have errored getting paused identifiers") + test.AssertEquals(t, len(identifiers.Identifiers), 1) } -func TestResetAccountPausingLimit(t *testing.T) { +func TestPerformValidationVAError(t *testing.T) { va, sa, ra, fc, cleanUp := initAuthorities(t) defer cleanUp() - ra.orderLifetime = 5 * 24 * time.Hour - - // Set up a rate limit policy that allows 1 order every 5 minutes. - rateLimitDuration := 5 * time.Minute - ra.rlPolicies = &dummyRateLimitConfig{ - FailedAuthorizationsForPausingPerDomainPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: rateLimitDuration}, - }, - } - authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong") @@ -937,21 +996,6 @@ func TestResetAccountPausingLimit(t *testing.T) { // Check that validated timestamp was recorded, stored, and retrieved expectedValidated := fc.Now() test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") - - authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ - Authz: authzPB, - ChallengeIndex: challIdx, - }) - - test.AssertNotError(t, err, "PerformValidation completely failed") - - select { - case r := <-va.performValidationRequest: - vaRequest = r - case <-time.After(time.Second): - t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") - } - } func TestCertificateKeyNotEqualAccountKey(t *testing.T) { diff --git a/ra/testdata/only-allow-one-failed-validation-before-pausing.yml b/ra/testdata/only-allow-one-failed-validation-before-pausing.yml new file mode 100644 index 00000000000..57bf710666f --- /dev/null +++ b/ra/testdata/only-allow-one-failed-validation-before-pausing.yml @@ -0,0 +1,4 @@ +FailedAuthorizationsForPausingPerDomainPerAccount: + count: 1 + burst: 1 + period: 24h From f98337344ef5bee8649863c0a52ccacaaf89e19a Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 1 Nov 2024 14:11:15 -0400 Subject: [PATCH 16/29] More progress --- ra/ra.go | 1 - ra/ra_test.go | 128 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 110 insertions(+), 19 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 9c971b5c672..9d66f836fd2 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1828,7 +1828,6 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, // resetAccountPausingLimit resets bucket to maximum capacity for given account. // There is no reason to surface errors from this function to the Subscriber - func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, regId, ident.Value) if err != nil { diff --git a/ra/ra_test.go b/ra/ra_test.go index d3794fab2b6..29e623c6db8 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -838,51 +838,75 @@ func TestPerformValidationSuccess(t *testing.T) { // PauseIdentifiersResponse. It does not maintain state of repaused identifiers. type mockSAPaused struct { sapb.StorageAuthorityClient + identType string + identValue string + receivedRegID int64 } func (msa *mockSAPaused) GetRegistration(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*corepb.Registration, error) { - fmt.Println("Here GetRegistration") - return Registration, nil + if msa.receivedRegID == req.Id { + return Registration, nil + } else { + return nil, fmt.Errorf("Could not find registration") + } } -func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, in *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { - fmt.Println("Here PausedIdentifiers") - return &sapb.PauseIdentifiersResponse{Paused: int64(len(in.Identifiers))}, nil +func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, req *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { + if len(req.Identifiers) <= 0 { + return nil, fmt.Errorf("No identifiers found to pause") + } + msa.identType = req.Identifiers[0].Type + msa.identValue = req.Identifiers[0].Value + return &sapb.PauseIdentifiersResponse{Paused: int64(len(req.Identifiers))}, nil +} + +func (msa *mockSAPaused) GetPausedIdentifiers(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*sapb.Identifiers, error) { + fmt.Printf("%s, %s\n", msa.identType, msa.identValue) + if msa.identType != "" && msa.identValue != "" { + return &sapb.Identifiers{ + Identifiers: []*corepb.Identifier{{ + Type: msa.identType, + Value: msa.identValue, + }}, + }, nil + } + return nil, fmt.Errorf("No identifiers paused yet") } func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.FinalizeAuthorizationRequest, _ ...grpc.CallOption) (*emptypb.Empty, error) { - fmt.Println("Here FinalizeAuthorization2") return &emptypb.Empty{}, nil } -func TestResetAccountPausingLimit(t *testing.T) { +func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { va, sa, ra, fc, cleanUp := initAuthorities(t) defer cleanUp() mockSA := &mockSAPaused{} ra.SA = mockSA - // Override the default ratelimits + // Override the default ratelimits to only allow one failed validation. txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/only-allow-one-failed-validation-before-pausing.yml", "") test.AssertNotError(t, err, "making transaction composer") ra.txnBuilder = txnBuilder // We know this is OK because of TestNewAuthorization authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + mockSA.receivedRegID = authzPB.RegistrationID - // TODO(Phil): Explain why this has a problem filled out + // We induce the bad path by setting a problem. This will consume all + // available capacity in the rate limit bucket. va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.com", + Hostname: "example.net", Port: "8080", - Url: "http://example.com/", + Url: "http://example.net/", ResolverAddrs: []string{"rebound"}, }, }, Problems: &corepb.ProblemDetails{ - Detail: "CAA invalid for example.com", + Detail: "CAA invalid for example.net", }, } @@ -907,6 +931,12 @@ func TestResetAccountPausingLimit(t *testing.T) { // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) + dbAuthzPB := getAuthorization(t, authzPB.Id, sa) + t.Log("dbAuthz:", dbAuthzPB) + + got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) + test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") + test.AssertBoxedNil(t, got, "Should have received nil response, but did not") // A second failed validation should result in the identifier being paused // due to the strict ratelimit. @@ -914,14 +944,14 @@ func TestResetAccountPausingLimit(t *testing.T) { Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.com", + Hostname: "example.net", Port: "8080", - Url: "http://example.com/", + Url: "http://example.net/", ResolverAddrs: []string{"rebound"}, }, }, Problems: &corepb.ProblemDetails{ - Detail: "CAA invalid for example.com", + Detail: "CAA invalid for example.net", }, } @@ -939,16 +969,78 @@ func TestResetAccountPausingLimit(t *testing.T) { t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") } - // Verify that the VA got the request, and it's the same as the others + // Verify that the VA got the request, and it's the same as the others. test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) + dbAuthzPB = getAuthorization(t, authzPB.Id, sa) + t.Log("dbAuthz:", dbAuthzPB) - identifiers, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: 1}, nil) + // Ensure the account:domain we expect to be paused actually is. + got, err = ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertNotError(t, err, "Should not have errored getting paused identifiers") - test.AssertEquals(t, len(identifiers.Identifiers), 1) + test.AssertEquals(t, len(got.Identifiers), 1) + test.AssertEquals(t, got.Identifiers[0].Value, Identifier) + + ///////////// + ///////////// A successful validation by the RegID for a non-paused identifier should return capacity to the ratelimit bucket and allow another failed validation. This is the successful validation. + ///////////// + // We know this is OK because of TestNewAuthorization + authzPB = createPendingAuthorization(t, sa, "example.com", fc.Now().Add(12*time.Hour)) + + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ + Records: []*corepb.ValidationRecord{ + { + AddressUsed: []byte("192.168.0.1"), + Hostname: "example.com", + Port: "8080", + Url: "http://example.com/", + ResolverAddrs: []string{"rebound"}, + }, + }, + Problems: nil, + } + + challIdx = dnsChallIdx(t, authzPB.Challenges) + authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + Authz: authzPB, + ChallengeIndex: challIdx, + }) + test.AssertNotError(t, err, "PerformValidation failed") + + select { + case r := <-va.performValidationRequest: + vaRequest = r + case <-time.After(time.Second): + t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") + } + + // Verify that the VA got the request, and it's the same as the others + test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) + test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) + + // Sleep so the RA has a chance to write to the SA + time.Sleep(100 * time.Millisecond) + dbAuthzPB = getAuthorization(t, authzPB.Id, sa) + t.Log("dbAuthz:", dbAuthzPB) + + // Verify that the responses are reflected + challIdx = dnsChallIdx(t, dbAuthzPB.Challenges) + challenge, err := bgrpc.PBToChallenge(dbAuthzPB.Challenges[challIdx]) + test.AssertNotError(t, err, "Failed to marshall corepb.Challenge to core.Challenge.") + + test.AssertNotNil(t, vaRequest.Challenge, "Request passed to VA has no challenge") + test.Assert(t, challenge.Status == core.StatusValid, "challenge was not marked as valid") + + // The DB authz's expiry should be equal to the current time plus the + // configured authorization lifetime + test.AssertEquals(t, dbAuthzPB.Expires.AsTime(), fc.Now().Add(ra.authorizationLifetime)) + + // Check that validated timestamp was recorded, stored, and retrieved + expectedValidated := fc.Now() + test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") } func TestPerformValidationVAError(t *testing.T) { From 49b959f5298a1b2e8ef5b4645e433f787df36039 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 4 Nov 2024 13:14:23 -0500 Subject: [PATCH 17/29] Progress is progressing --- ra/ra.go | 10 +- ra/ra_test.go | 217 ++++++++++++------ ... one-failed-validation-before-pausing.yml} | 0 .../two-failed-validations-before-pausing.yml | 4 + 4 files changed, 157 insertions(+), 74 deletions(-) rename ra/testdata/{only-allow-one-failed-validation-before-pausing.yml => one-failed-validation-before-pausing.yml} (100%) create mode 100644 ra/testdata/two-failed-validations-before-pausing.yml diff --git a/ra/ra.go b/ra/ra.go index 9d66f836fd2..d6c81af7ff3 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1827,14 +1827,16 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } // resetAccountPausingLimit resets bucket to maximum capacity for given account. -// There is no reason to surface errors from this function to the Subscriber +// There is no reason to surface errors from this function to the Subscriber. func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, regId, ident.Value) if err != nil { - ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", - regId, ident, err) + ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", regId, ident, err) + } + err = ra.limiter.Reset(ctx, bucketKey) + if err != nil { + ra.log.Warningf("Can't reset domain bucket key for regID=[%d] authzID=[%s] err=[%s]", regId, ident, err) } - ra.limiter.Reset(ctx, bucketKey) } // PerformValidation initiates validation for a specific challenge associated diff --git a/ra/ra_test.go b/ra/ra_test.go index 29e623c6db8..f8b4e542bc6 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -277,7 +277,7 @@ func newAcctKey(t *testing.T) []byte { return acctKey } -func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAuthorityClient, *RegistrationAuthorityImpl, clock.FakeClock, func()) { +func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAuthorityClient, *RegistrationAuthorityImpl, *ratelimits.RedisSource, clock.FakeClock, func()) { err := json.Unmarshal(AccountKeyJSONA, &AccountKeyA) test.AssertNotError(t, err, "Failed to unmarshal public JWK") err = json.Unmarshal(AccountKeyJSONB, &AccountKeyB) @@ -348,6 +348,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho }, }, nil, nil, 0, log, metrics.NoopRegisterer) + var source *ratelimits.RedisSource var limiter *ratelimits.Limiter var txnBuilder *ratelimits.TransactionBuilder if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { @@ -394,11 +395,11 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho ra.CA = ca ra.OCSP = &mocks.MockOCSPGenerator{} ra.PA = pa - return va, sa, ra, fc, cleanUp + return va, sa, ra, source, fc, cleanUp } func TestValidateContacts(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ansible := "ansible:earth.sol.milkyway.laniakea/letsencrypt" @@ -475,7 +476,7 @@ func TestValidateContacts(t *testing.T) { } func TestNewRegistration(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() mailto := "mailto:foo@letsencrypt.org" acctKeyB, err := AccountKeyB.MarshalJSON() @@ -502,7 +503,7 @@ func TestNewRegistration(t *testing.T) { } func TestNewRegistrationContactsPresent(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() testCases := []struct { Name string @@ -574,7 +575,7 @@ func (sa *mockSAFailsNewRegistration) NewRegistration(_ context.Context, _ *core } func TestNewRegistrationSAFailure(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.SA = &mockSAFailsNewRegistration{} acctKeyB, err := AccountKeyB.MarshalJSON() @@ -592,7 +593,7 @@ func TestNewRegistrationSAFailure(t *testing.T) { } func TestNewRegistrationNoFieldOverwrite(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() mailto := "mailto:foo@letsencrypt.org" acctKeyC, err := AccountKeyC.MarshalJSON() @@ -614,7 +615,7 @@ func TestNewRegistrationNoFieldOverwrite(t *testing.T) { } func TestNewRegistrationBadKey(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() mailto := "mailto:foo@letsencrypt.org" shortKey, err := ShortKey.MarshalJSON() @@ -629,7 +630,7 @@ func TestNewRegistrationBadKey(t *testing.T) { } func TestRegistrationsPerIPOverrideUsage(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() regIP := net.ParseIP("4.5.6.7") @@ -676,7 +677,7 @@ func (sa NoUpdateSA) UpdateRegistration(_ context.Context, _ *corepb.Registratio } func TestUpdateRegistrationSame(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() mailto := "mailto:foo@letsencrypt.org" @@ -713,7 +714,7 @@ func TestUpdateRegistrationSame(t *testing.T) { } func TestPerformValidationExpired(t *testing.T) { - _, sa, ra, fc, cleanUp := initAuthorities(t) + _, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() authz := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(-2*time.Hour)) @@ -726,7 +727,7 @@ func TestPerformValidationExpired(t *testing.T) { } func TestPerformValidationAlreadyValid(t *testing.T) { - va, _, ra, _, cleanUp := initAuthorities(t) + va, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() // Create a finalized authorization @@ -771,7 +772,7 @@ func TestPerformValidationAlreadyValid(t *testing.T) { } func TestPerformValidationSuccess(t *testing.T) { - va, sa, ra, fc, cleanUp := initAuthorities(t) + va, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() // We know this is OK because of TestNewAuthorization @@ -877,15 +878,15 @@ func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.F return &emptypb.Empty{}, nil } -func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { - va, sa, ra, fc, cleanUp := initAuthorities(t) +func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t *testing.T) { + va, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() mockSA := &mockSAPaused{} ra.SA = mockSA // Override the default ratelimits to only allow one failed validation. - txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/only-allow-one-failed-validation-before-pausing.yml", "") + txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/one-failed-validation-before-pausing.yml", "") test.AssertNotError(t, err, "making transaction composer") ra.txnBuilder = txnBuilder @@ -978,17 +979,79 @@ func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { dbAuthzPB = getAuthorization(t, authzPB.Id, sa) t.Log("dbAuthz:", dbAuthzPB) - // Ensure the account:domain we expect to be paused actually is. + // Ensure the identifier:account:domain we expect to be paused actually is. got, err = ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertNotError(t, err, "Should not have errored getting paused identifiers") test.AssertEquals(t, len(got.Identifiers), 1) test.AssertEquals(t, got.Identifiers[0].Value, Identifier) +} + +func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit(t *testing.T) { + va, sa, ra, redis, fc, cleanUp := initAuthorities(t) + defer cleanUp() + + mockSA := &mockSAPaused{} + ra.SA = mockSA + + // Override the default ratelimits to only allow one failed validation. + txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/two-failed-validations-before-pausing.yml", "") + test.AssertNotError(t, err, "making transaction composer") + ra.txnBuilder = txnBuilder + + // We know this is OK because of TestNewAuthorization + authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + mockSA.receivedRegID = authzPB.RegistrationID + + // We induce the bad path by setting a problem. This will consume all + // available capacity in the rate limit bucket. + va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ + Records: []*corepb.ValidationRecord{ + { + AddressUsed: []byte("192.168.0.1"), + Hostname: "example.net", + Port: "8080", + Url: "http://example.net/", + ResolverAddrs: []string{"rebound"}, + }, + }, + Problems: &corepb.ProblemDetails{ + Detail: "CAA invalid for example.net", + }, + } + + challIdx := dnsChallIdx(t, authzPB.Challenges) + authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ + Authz: authzPB, + ChallengeIndex: challIdx, + }) + test.AssertNotError(t, err, "PerformValidation failed") + + var vaRequest *vapb.PerformValidationRequest + select { + case r := <-va.performValidationRequest: + vaRequest = r + case <-time.After(time.Second): + t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") + } + + // Verify that the VA got the request, and it's the same as the others + test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) + test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) + + // Sleep so the RA has a chance to write to the SA + time.Sleep(100 * time.Millisecond) + dbAuthzPB := getAuthorization(t, authzPB.Id, sa) + t.Log("dbAuthz:", dbAuthzPB) + + got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) + test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") + test.AssertBoxedNil(t, got, "Should have received nil response, but did not") + + // Now the goal is to perform a successful validation which should reset the + // FailedAuthorizationsForPausingPerDomainPerAccount ratelimit. - ///////////// - ///////////// A successful validation by the RegID for a non-paused identifier should return capacity to the ratelimit bucket and allow another failed validation. This is the successful validation. - ///////////// // We know this is OK because of TestNewAuthorization - authzPB = createPendingAuthorization(t, sa, "example.com", fc.Now().Add(12*time.Hour)) + authzPB = createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ @@ -1003,6 +1066,7 @@ func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { Problems: nil, } + now := fc.Now() challIdx = dnsChallIdx(t, authzPB.Challenges) authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ Authz: authzPB, @@ -1023,6 +1087,7 @@ func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) + dbAuthzPB = getAuthorization(t, authzPB.Id, sa) t.Log("dbAuthz:", dbAuthzPB) @@ -1036,15 +1101,27 @@ func TestPerformValidation_PauseIdentifiersRatelimit(t *testing.T) { // The DB authz's expiry should be equal to the current time plus the // configured authorization lifetime - test.AssertEquals(t, dbAuthzPB.Expires.AsTime(), fc.Now().Add(ra.authorizationLifetime)) + test.AssertEquals(t, dbAuthzPB.Expires.AsTime(), now.Add(ra.authorizationLifetime)) // Check that validated timestamp was recorded, stored, and retrieved expectedValidated := fc.Now() test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") + + // We need the bucket key to scan for in Redis + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsPerDomainPerAccount, authzPB.RegistrationID, Identifier) + test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") + + // Verify that we receive a TAT for the current time. This indicates the + // identifier:accountID:domain bucket has regained capacity avoiding being + // inadvertently paused. + tat, err := redis.Get(ctx, bucketKey) + test.AssertNotError(t, err, "Could not retrieve TAT from source") + test.AssertEquals(t, tat, fc.Now()) + } func TestPerformValidationVAError(t *testing.T) { - va, sa, ra, fc, cleanUp := initAuthorities(t) + va, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) @@ -1091,7 +1168,7 @@ func TestPerformValidationVAError(t *testing.T) { } func TestCertificateKeyNotEqualAccountKey(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() exp := ra.clk.Now().Add(365 * 24 * time.Hour) @@ -1130,7 +1207,7 @@ func TestCertificateKeyNotEqualAccountKey(t *testing.T) { } func TestNewOrderRateLimiting(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = 5 * 24 * time.Hour @@ -1181,7 +1258,7 @@ func TestNewOrderRateLimiting(t *testing.T) { // TestEarlyOrderRateLimiting tests that NewOrder applies the certificates per // name/per FQDN rate limits against the order names. func TestEarlyOrderRateLimiting(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = 5 * 24 * time.Hour @@ -1243,7 +1320,7 @@ func (sa *mockInvalidAuthorizationsAuthority) CountInvalidAuthorizations2(ctx co } func TestAuthzFailedRateLimitingNewOrder(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.rlPolicies = &dummyRateLimitConfig{ @@ -1293,7 +1370,7 @@ func (m *mockSAWithNameCounts) FQDNSetExists(ctx context.Context, req *sapb.FQDN } func TestCheckCertificatesPerNameLimit(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() rlp := ratelimit.RateLimitPolicy{ @@ -1383,7 +1460,7 @@ func TestCheckCertificatesPerNameLimit(t *testing.T) { // TestCheckExactCertificateLimit tests that the duplicate certificate limit // applied to FQDN sets is respected. func TestCheckExactCertificateLimit(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() // Create a rate limit with a small threshold @@ -1633,7 +1710,7 @@ func (m mockSAWithFQDNSet) FQDNSetTimestampsForWindow(_ context.Context, req *sa // without the feature flag for the fix enabled. // See https://github.com/letsencrypt/boulder/issues/2681 func TestExactPublicSuffixCertLimit(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() // Simple policy that only allows 2 certificates per name. @@ -1684,7 +1761,7 @@ func TestExactPublicSuffixCertLimit(t *testing.T) { } func TestDeactivateAuthorization(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() exp := ra.clk.Now().Add(365 * 24 * time.Hour) @@ -1698,7 +1775,7 @@ func TestDeactivateAuthorization(t *testing.T) { } func TestDeactivateRegistration(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() // Deactivate failure because incomplete registration provided @@ -1753,7 +1830,7 @@ func (cr *caaRecorder) IsCAAValid( // Test that the right set of domain names have their CAA rechecked, based on // their `Validated` (attemptedAt in the database) timestamp. func TestRecheckCAADates(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() recorder := &caaRecorder{names: make(map[string]bool)} ra.caa = recorder @@ -1943,7 +2020,7 @@ func (cf *caaFailer) IsCAAValid( } func TestRecheckCAAEmpty(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() err := ra.recheckCAA(context.Background(), nil) test.AssertNotError(t, err, "expected nil") @@ -1957,7 +2034,7 @@ func makeHTTP01Authorization(domain string) *core.Authorization { } func TestRecheckCAASuccess(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() authzs := []*core.Authorization{ makeHTTP01Authorization("a.com"), @@ -1969,7 +2046,7 @@ func TestRecheckCAASuccess(t *testing.T) { } func TestRecheckCAAFail(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.caa = &caaFailer{} authzs := []*core.Authorization{ @@ -2020,7 +2097,7 @@ func TestRecheckCAAFail(t *testing.T) { } func TestRecheckCAAInternalServerError(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.caa = &caaFailer{} authzs := []*core.Authorization{ @@ -2034,7 +2111,7 @@ func TestRecheckCAAInternalServerError(t *testing.T) { } func TestNewOrder(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = time.Hour @@ -2103,7 +2180,7 @@ func TestNewOrder(t *testing.T) { // an identical order results in only one order being created & subsequently // reused. func TestNewOrderReuse(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() ctx := context.Background() @@ -2212,7 +2289,7 @@ func TestNewOrderReuse(t *testing.T) { } func TestNewOrderReuseInvalidAuthz(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ctx := context.Background() @@ -2273,7 +2350,7 @@ func (mock *mockSACountPendingFails) CountPendingAuthorizations2(ctx context.Con // Ensure that we don't bother to call the SA to count pending authorizations // when an "unlimited" limit is set. func TestPendingAuthorizationsUnlimited(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.rlPolicies = &dummyRateLimitConfig{ @@ -2310,7 +2387,7 @@ func (sa *mockInvalidPlusValidAuthzAuthority) CountInvalidAuthorizations2(ctx co // Test that the failed authorizations limit is checked before authz reuse. func TestNewOrderCheckFailedAuthorizationsFirst(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() // Create an order (and thus a pending authz) for example.com @@ -2437,7 +2514,7 @@ func (msa *mockSAWithAuthzs) NewOrderAndAuthzs(ctx context.Context, req *sapb.Ne // for background - this safety check was previously broken! // https://github.com/letsencrypt/boulder/issues/3420 func TestNewOrderAuthzReuseSafety(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ctx := context.Background() @@ -2513,7 +2590,7 @@ func TestNewOrderAuthzReuseSafety(t *testing.T) { } func TestNewOrderWildcard(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = time.Hour @@ -2678,7 +2755,7 @@ func TestNewOrderWildcard(t *testing.T) { } func TestNewOrderExpiry(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ctx := context.Background() @@ -2746,7 +2823,7 @@ func TestNewOrderExpiry(t *testing.T) { } func TestFinalizeOrder(t *testing.T) { - _, sa, ra, fc, cleanUp := initAuthorities(t) + _, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = time.Hour @@ -3032,7 +3109,7 @@ func TestFinalizeOrder(t *testing.T) { } func TestFinalizeOrderWithMixedSANAndCN(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = time.Hour @@ -3094,7 +3171,7 @@ func TestFinalizeOrderWithMixedSANAndCN(t *testing.T) { } func TestFinalizeOrderWildcard(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() // Pick an expiry in the future @@ -3196,7 +3273,7 @@ func TestFinalizeOrderWildcard(t *testing.T) { } func TestFinalizeOrderDisabledChallenge(t *testing.T) { - _, sa, ra, fc, cleanUp := initAuthorities(t) + _, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() // Create a random domain @@ -3251,7 +3328,7 @@ func TestFinalizeOrderDisabledChallenge(t *testing.T) { } func TestIssueCertificateAuditLog(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() // Set up order and authz expiries @@ -3374,7 +3451,7 @@ func TestIssueCertificateAuditLog(t *testing.T) { } func TestIssueCertificateCAACheckLog(t *testing.T) { - _, sa, ra, fc, cleanUp := initAuthorities(t) + _, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() // Set up order and authz expiries. @@ -3485,7 +3562,7 @@ func TestIssueCertificateCAACheckLog(t *testing.T) { // // See https://github.com/letsencrypt/boulder/issues/3201 func TestUpdateMissingAuthorization(t *testing.T) { - _, sa, ra, fc, cleanUp := initAuthorities(t) + _, sa, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() ctx := context.Background() @@ -3505,7 +3582,7 @@ func TestUpdateMissingAuthorization(t *testing.T) { } func TestPerformValidationBadChallengeType(t *testing.T) { - _, _, ra, fc, cleanUp := initAuthorities(t) + _, _, ra, _, fc, cleanUp := initAuthorities(t) defer cleanUp() pa, err := policy.New(map[core.AcmeChallenge]bool{}, blog.NewMock()) test.AssertNotError(t, err, "Couldn't create PA") @@ -3545,7 +3622,7 @@ func (mp *timeoutPub) SubmitToSingleCTWithResult(_ context.Context, _ *pubpb.Req } func TestCTPolicyMeasurements(t *testing.T) { - _, ssa, ra, _, cleanup := initAuthorities(t) + _, ssa, ra, _, _, cleanup := initAuthorities(t) defer cleanup() ra.ctpolicy = ctpolicy.New(&timeoutPub{}, loglist.List{ @@ -3678,7 +3755,7 @@ func (ca *mockCAFailCertForPrecert) IssueCertificateForPrecertificate( // `ra.issueCertificateInner` are propagated correctly, with the part of the // issuance process that failed prefixed on the error message. func TestIssueCertificateInnerErrs(t *testing.T) { - _, sa, ra, _, cleanUp := initAuthorities(t) + _, sa, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = 24 * time.Hour @@ -3811,7 +3888,7 @@ func (sa *mockSAWithFinalize) FQDNSetExists(ctx context.Context, in *sapb.FQDNSe } func TestIssueCertificateInnerWithProfile(t *testing.T) { - _, _, ra, fc, cleanup := initAuthorities(t) + _, _, ra, _, fc, cleanup := initAuthorities(t) defer cleanup() // Generate a reasonable-looking CSR and cert to pass the matchesCSR check. @@ -3847,7 +3924,7 @@ func TestIssueCertificateInnerWithProfile(t *testing.T) { } func TestIssueCertificateOuter(t *testing.T) { - _, sa, ra, fc, cleanup := initAuthorities(t) + _, sa, ra, _, fc, cleanup := initAuthorities(t) defer cleanup() ra.orderLifetime = 24 * time.Hour @@ -3901,7 +3978,7 @@ func TestIssueCertificateOuter(t *testing.T) { } func TestNewOrderMaxNames(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.maxNames = 2 @@ -4096,7 +4173,7 @@ func (msgo *mockSAGenerateOCSP) GetCertificateStatus(_ context.Context, req *sap } func TestGenerateOCSP(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4136,7 +4213,7 @@ func (msgo *mockSALongExpiredSerial) GetSerialMetadata(_ context.Context, req *s } func TestGenerateOCSPLongExpiredSerial(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4164,7 +4241,7 @@ func (msgo *mockSAUnknownSerial) GetSerialMetadata(_ context.Context, req *sapb. } func TestGenerateOCSPUnknownSerial(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4182,7 +4259,7 @@ func TestGenerateOCSPUnknownSerial(t *testing.T) { } func TestRevokeCertByApplicant_Subscriber(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4257,7 +4334,7 @@ func (msa *mockSARevocationWithAuthzs) GetValidAuthorizations2(ctx context.Conte } func TestRevokeCertByApplicant_Controller(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4298,7 +4375,7 @@ func TestRevokeCertByApplicant_Controller(t *testing.T) { } func TestRevokeCertByKey(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4350,7 +4427,7 @@ func TestRevokeCertByKey(t *testing.T) { } func TestAdministrativelyRevokeCertificate(t *testing.T) { - _, _, ra, clk, cleanUp := initAuthorities(t) + _, _, ra, _, clk, cleanUp := initAuthorities(t) defer cleanUp() ra.OCSP = &mockOCSPA{} @@ -4495,7 +4572,7 @@ func TestAdministrativelyRevokeCertificate(t *testing.T) { } func TestNewOrderRateLimitingExempt(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() ra.orderLifetime = 5 * 24 * time.Hour @@ -4533,7 +4610,7 @@ func TestNewOrderRateLimitingExempt(t *testing.T) { } func TestNewOrderFailedAuthzRateLimitingExempt(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() exampleOrder := &rapb.NewOrderRequest{ @@ -4594,7 +4671,7 @@ func (sa *mockNewOrderMustBeReplacementAuthority) NewOrderAndAuthzs(ctx context. } func TestNewOrderReplacesSerialCarriesThroughToSA(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() exampleOrder := &rapb.NewOrderRequest{ @@ -4630,7 +4707,7 @@ func (sa *mockSAUnpauseAccount) UnpauseAccount(_ context.Context, req *sapb.Regi // the requested RegID to the SA, and correctly passes the SA's count back to // the caller. func TestUnpauseAccount(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() mockSA := mockSAUnpauseAccount{identsToUnpause: 0} @@ -4652,7 +4729,7 @@ func TestUnpauseAccount(t *testing.T) { } func TestGetAuthorization(t *testing.T) { - _, _, ra, _, cleanup := initAuthorities(t) + _, _, ra, _, _, cleanup := initAuthorities(t) defer cleanup() ra.SA = &mockSAWithAuthzs{ diff --git a/ra/testdata/only-allow-one-failed-validation-before-pausing.yml b/ra/testdata/one-failed-validation-before-pausing.yml similarity index 100% rename from ra/testdata/only-allow-one-failed-validation-before-pausing.yml rename to ra/testdata/one-failed-validation-before-pausing.yml diff --git a/ra/testdata/two-failed-validations-before-pausing.yml b/ra/testdata/two-failed-validations-before-pausing.yml new file mode 100644 index 00000000000..5024aad5c0d --- /dev/null +++ b/ra/testdata/two-failed-validations-before-pausing.yml @@ -0,0 +1,4 @@ +FailedAuthorizationsForPausingPerDomainPerAccount: + count: 2 + burst: 1 + period: 24h From 37fb8828a61b3a070023957812756197dcc6b789 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 4 Nov 2024 13:15:01 -0500 Subject: [PATCH 18/29] Comment wrapping --- ratelimits/transaction.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index c3c91272403..dc28e4b8fbc 100644 --- a/ratelimits/transaction.go +++ b/ratelimits/transaction.go @@ -65,7 +65,8 @@ func newDomainBucketKey(name Name, orderName string) (string, error) { } // NewRegIdDomainBucketKey validates and returns a bucketKey for limits that use -// the 'enum:regId:domain' bucket key format. This function is exported for use in ra.PerformValidation +// the 'enum:regId:domain' bucket key format. This function is exported for use +// in ra.resetAccountPausingLimit. func NewRegIdDomainBucketKey(name Name, regId int64, orderName string) (string, error) { regIdStr := strconv.FormatInt(regId, 10) err := validateIdForName(name, joinWithColon(regIdStr, orderName)) From 794a9bfd9073e9573db005c0ad80a246c00171ea Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 4 Nov 2024 16:31:20 -0500 Subject: [PATCH 19/29] Add new feature flag --- features/features.go | 7 +++ ra/ra.go | 58 ++++++++++---------- ra/ra_test.go | 111 ++++++++++++++++++++++++++------------- test/config-next/ra.json | 3 +- 4 files changed, 112 insertions(+), 67 deletions(-) diff --git a/features/features.go b/features/features.go index ce677a99ed9..33f7e9d1492 100644 --- a/features/features.go +++ b/features/features.go @@ -109,6 +109,13 @@ type Config struct { // get the AUTO_INCREMENT ID of each new authz without relying on MariaDB's // unique "INSERT ... RETURNING" functionality. InsertAuthzsIndividually bool + + // UseKvLimitsForZombieClientPausing when enabled, causes the key-value rate + // limiter to be the authoritative source of rate limiting information for + // automatically pausing clients who systemically fail every validation + // attempt. When disabled, only manually paused identifier:accountID:domain + // pairs will be rejected. + UseKvLimitsForZombieClientPausing bool } var fMu = new(sync.RWMutex) diff --git a/ra/ra.go b/ra/ra.go index d6c81af7ff3..d5325470c34 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1769,9 +1769,10 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI } // countFailedValidation increments the failed authorizations per domain per -// account rate limit. It also increments the failed authorizations for pausing per -// domain per account rate limit. There is no reason to surface errors from -// this function to the Subscriber, spends against this limit are best effort. +// account rate limit. If the UseKvLimitsForZombieClientPausing feature has been +// enabled, it also increments the failed authorizations for pausing per domain +// per account rate limit. There is no reason to surface errors from this +// function to the Subscriber, spends against this limit are best effort. func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { var name = ident.Value @@ -1780,7 +1781,6 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, return } - fmt.Println("countFailedValidation 1") txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, name) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) @@ -1793,37 +1793,33 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } - fmt.Println("countFailedValidation 2") - // Increment ratelimit for IssuancePaused - txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, name) - if err != nil { - ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) - } + if features.Get().UseKvLimitsForZombieClientPausing { + txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, name) + if err != nil { + ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) + } - fmt.Println("countFailedValidation 3") - decision, err := ra.limiter.Spend(ctx, txn) - if err != nil { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return + decision, err := ra.limiter.Spend(ctx, txn) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return + } + ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } - ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) - } - fmt.Printf("countFailedValidation 4, %v\n", ra.clk.Now()) - if decision.Result(ra.clk.Now()) != nil { - fmt.Println("countFailedValidation 5") - ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ - RegistrationID: regId, - Identifiers: []*corepb.Identifier{ - { - Type: string(ident.Type), - Value: ident.Value, + if decision.Result(ra.clk.Now()) != nil { + ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ + RegistrationID: regId, + Identifiers: []*corepb.Identifier{ + { + Type: string(ident.Type), + Value: ident.Value, + }, }, - }, - }) + }) + } } - } // resetAccountPausingLimit resets bucket to maximum capacity for given account. @@ -1963,7 +1959,9 @@ func (ra *RegistrationAuthorityImpl) PerformValidation( go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier) } else { challenge.Status = core.StatusValid - ra.resetAccountPausingLimit(vaCtx, authz.RegistrationID, authz.Identifier) + if features.Get().UseKvLimitsForZombieClientPausing { + ra.resetAccountPausingLimit(vaCtx, authz.RegistrationID, authz.Identifier) + } } challenge.Validated = &vStart authz.Challenges[challIndex] = *challenge diff --git a/ra/ra_test.go b/ra/ra_test.go index f8b4e542bc6..f3ab518e32a 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -372,8 +372,10 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho } ring, err := bredis.NewRingFromConfig(rc, stats, log) test.AssertNotError(t, err, "making redis ring client") - source := ratelimits.NewRedisSource(ring.Ring, fc, stats) + source = ratelimits.NewRedisSource(ring.Ring, fc, stats) test.AssertNotNil(t, source, "source should not be nil") + err = source.Ping(context.Background()) + test.AssertNotError(t, err, "Ping should not error") limiter, err = ratelimits.NewLimiter(fc, source, stats) test.AssertNotError(t, err, "making limiter") txnBuilder, err = ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") @@ -862,7 +864,6 @@ func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, req *sapb.PauseRe } func (msa *mockSAPaused) GetPausedIdentifiers(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*sapb.Identifiers, error) { - fmt.Printf("%s, %s\n", msa.identType, msa.identValue) if msa.identType != "" && msa.identValue != "" { return &sapb.Identifiers{ Identifiers: []*corepb.Identifier{{ @@ -879,7 +880,11 @@ func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.F } func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t *testing.T) { - va, sa, ra, _, fc, cleanUp := initAuthorities(t) + if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + t.Skip() + } + + va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() mockSA := &mockSAPaused{} @@ -891,7 +896,8 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * ra.txnBuilder = txnBuilder // We know this is OK because of TestNewAuthorization - authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + domain := "example.net" + authzPB := createPendingAuthorization(t, sa, domain, fc.Now().Add(12*time.Hour)) mockSA.receivedRegID = authzPB.RegistrationID // We induce the bad path by setting a problem. This will consume all @@ -900,14 +906,14 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.net", + Hostname: domain, Port: "8080", - Url: "http://example.net/", + Url: fmt.Sprintf("http://%s/", domain), ResolverAddrs: []string{"rebound"}, }, }, Problems: &corepb.ProblemDetails{ - Detail: "CAA invalid for example.net", + Detail: fmt.Sprintf("CAA invalid for %s", domain), }, } @@ -932,27 +938,39 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) - dbAuthzPB := getAuthorization(t, authzPB.Id, sa) - t.Log("dbAuthz:", dbAuthzPB) got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") test.AssertBoxedNil(t, got, "Should have received nil response, but did not") + // We need the bucket key to scan for in Redis + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) + test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") + + // Verify that a redis entry exists for this identifier:accountID:domain + tat, err := redisSrc.Get(ctx, bucketKey) + test.AssertNotError(t, err, "Should not have errored, but did") + + // There is no more capacity and the next failed validation will effectively + // pause issuance attempts. The ratelimit file is written to increment + // capacity every 24 hours, so we can check that the TAT states that, not + // that it particularly matters in this context. + test.AssertEquals(t, tat, fc.Now().Add(24*time.Hour)) + // A second failed validation should result in the identifier being paused // due to the strict ratelimit. va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.net", + Hostname: domain, Port: "8080", - Url: "http://example.net/", + Url: fmt.Sprintf("http://%s/", domain), ResolverAddrs: []string{"rebound"}, }, }, Problems: &corepb.ProblemDetails{ - Detail: "CAA invalid for example.net", + Detail: fmt.Sprintf("CAA invalid for %s", domain), }, } @@ -975,21 +993,27 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) // Sleep so the RA has a chance to write to the SA - time.Sleep(100 * time.Millisecond) - dbAuthzPB = getAuthorization(t, authzPB.Id, sa) - t.Log("dbAuthz:", dbAuthzPB) + time.Sleep(200 * time.Millisecond) // Ensure the identifier:account:domain we expect to be paused actually is. got, err = ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertNotError(t, err, "Should not have errored getting paused identifiers") test.AssertEquals(t, len(got.Identifiers), 1) - test.AssertEquals(t, got.Identifiers[0].Value, Identifier) + test.AssertEquals(t, got.Identifiers[0].Value, domain) + + err = ra.limiter.Reset(ctx, bucketKey) + test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit(t *testing.T) { - va, sa, ra, redis, fc, cleanUp := initAuthorities(t) + if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + t.Skip() + } + + va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() + originalSA := sa mockSA := &mockSAPaused{} ra.SA = mockSA @@ -999,7 +1023,8 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR ra.txnBuilder = txnBuilder // We know this is OK because of TestNewAuthorization - authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + domain := "example.net" + authzPB := createPendingAuthorization(t, sa, "example.net", fc.Now().Add(12*time.Hour)) mockSA.receivedRegID = authzPB.RegistrationID // We induce the bad path by setting a problem. This will consume all @@ -1008,14 +1033,14 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.net", + Hostname: domain, Port: "8080", - Url: "http://example.net/", + Url: fmt.Sprintf("http://%s/", domain), ResolverAddrs: []string{"rebound"}, }, }, Problems: &corepb.ProblemDetails{ - Detail: "CAA invalid for example.net", + Detail: fmt.Sprintf("CAA invalid for %s", domain), }, } @@ -1040,26 +1065,41 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) - dbAuthzPB := getAuthorization(t, authzPB.Id, sa) - t.Log("dbAuthz:", dbAuthzPB) got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") test.AssertBoxedNil(t, got, "Should have received nil response, but did not") + // Avoid a datarace + time.Sleep(100 * time.Millisecond) + + // We need the bucket key to scan for in Redis + bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) + test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") + + // Verify that a redis entry exists for this identifier:accountID:domain + tat, err := redisSrc.Get(ctx, bucketKey) + test.AssertNotError(t, err, "Should not have errored, but did") + // We should have capacity for 1 more failed validation, the next TAT should + // be immediately (despite the fact that this clearly says now + 12 hours). + test.AssertEquals(t, tat, fc.Now().Add(12*time.Hour)) + + // // Now the goal is to perform a successful validation which should reset the // FailedAuthorizationsForPausingPerDomainPerAccount ratelimit. + // + ra.SA = originalSA // We know this is OK because of TestNewAuthorization - authzPB = createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + authzPB = createPendingAuthorization(t, originalSA, domain, fc.Now().Add(12*time.Hour)) va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ { AddressUsed: []byte("192.168.0.1"), - Hostname: "example.com", + Hostname: domain, Port: "8080", - Url: "http://example.com/", + Url: fmt.Sprintf("http://%s/", domain), ResolverAddrs: []string{"rebound"}, }, }, @@ -1087,9 +1127,7 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) - - dbAuthzPB = getAuthorization(t, authzPB.Id, sa) - t.Log("dbAuthz:", dbAuthzPB) + dbAuthzPB := getAuthorization(t, authzPB.Id, originalSA) // Verify that the responses are reflected challIdx = dnsChallIdx(t, dbAuthzPB.Challenges) @@ -1108,16 +1146,17 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") // We need the bucket key to scan for in Redis - bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsPerDomainPerAccount, authzPB.RegistrationID, Identifier) + bucketKey, err = ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") - // Verify that we receive a TAT for the current time. This indicates the - // identifier:accountID:domain bucket has regained capacity avoiding being - // inadvertently paused. - tat, err := redis.Get(ctx, bucketKey) - test.AssertNotError(t, err, "Could not retrieve TAT from source") - test.AssertEquals(t, tat, fc.Now()) + // Verify that the bucket no longer exists (because the limiter reset has + // deleted it). This indicates the identifier:accountID:domain bucket has + // regained capacity avoiding being inadvertently paused. + _, err = redisSrc.Get(ctx, bucketKey) + test.AssertErrorIs(t, err, ratelimits.ErrBucketNotFound) + err = ra.limiter.Reset(ctx, bucketKey) + test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidationVAError(t *testing.T) { diff --git a/test/config-next/ra.json b/test/config-next/ra.json index ad320cab78a..9f28eb9d9fc 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -130,7 +130,8 @@ }, "features": { "AsyncFinalize": true, - "UseKvLimitsForNewOrder": true + "UseKvLimitsForNewOrder": true, + "UseKvLimitsForZombieClientPausing": true }, "ctLogs": { "stagger": "500ms", From bfbc51a8bd657902d106fdecf2e915a1b587b338 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 4 Nov 2024 16:46:31 -0500 Subject: [PATCH 20/29] Log error from calling sa.PauseIdentifiers --- ra/ra.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ra/ra.go b/ra/ra.go index d5325470c34..e13c5339ac5 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1809,7 +1809,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } if decision.Result(ra.clk.Now()) != nil { - ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ + _, err := ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ RegistrationID: regId, Identifiers: []*corepb.Identifier{ { @@ -1818,6 +1818,9 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, }, }, }) + if err != nil { + ra.log.Warningf("pausing identifier for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) + } } } } From 5ccd589618e86922a0d0d6ebfb54c6c65c0fcb22 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 4 Nov 2024 16:51:30 -0500 Subject: [PATCH 21/29] Fix spelling error --- ratelimits/limit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ratelimits/limit.go b/ratelimits/limit.go index f90a71082fa..df2cd268c55 100644 --- a/ratelimits/limit.go +++ b/ratelimits/limit.go @@ -172,7 +172,7 @@ func loadAndParseOverrideLimits(path string) (limits, error) { } limit.overrideKey = joinWithColon(name.EnumString(), id) if name == CertificatesPerFQDNSet { - // FQDNSet hashs are not a nice thing to ask for in a + // FQDNSet hashes are not a nice thing to ask for in a // config file, so we allow the user to specify a // comma-separated list of FQDNs and compute the hash here. id = fmt.Sprintf("%x", core.HashNames(strings.Split(id, ","))) From 4bce0785bbbbd81706a3fde47a5289e17d122843 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 5 Nov 2024 13:06:08 -0500 Subject: [PATCH 22/29] Enable new feature for specific tests --- ra/ra_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index f3ab518e32a..5724ce12d77 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -48,6 +48,7 @@ import ( "github.com/letsencrypt/boulder/ctpolicy" "github.com/letsencrypt/boulder/ctpolicy/loglist" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" bgrpc "github.com/letsencrypt/boulder/grpc" "github.com/letsencrypt/boulder/identifier" @@ -887,6 +888,9 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() + features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) + defer features.Reset() + mockSA := &mockSAPaused{} ra.SA = mockSA @@ -1001,8 +1005,8 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * test.AssertEquals(t, len(got.Identifiers), 1) test.AssertEquals(t, got.Identifiers[0].Value, domain) - err = ra.limiter.Reset(ctx, bucketKey) - test.AssertNotError(t, err, "Failed cleaning up redis") + //err = ra.limiter.Reset(ctx, bucketKey) + //test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit(t *testing.T) { @@ -1013,6 +1017,9 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() + features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) + defer features.Reset() + originalSA := sa mockSA := &mockSAPaused{} ra.SA = mockSA @@ -1155,8 +1162,8 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR _, err = redisSrc.Get(ctx, bucketKey) test.AssertErrorIs(t, err, ratelimits.ErrBucketNotFound) - err = ra.limiter.Reset(ctx, bucketKey) - test.AssertNotError(t, err, "Failed cleaning up redis") + //err = ra.limiter.Reset(ctx, bucketKey) + //test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidationVAError(t *testing.T) { From 12bf5e5ad6e5fb7e722a0a0982dc516d6df3bd0a Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 6 Nov 2024 16:08:30 -0500 Subject: [PATCH 23/29] Fix tests --- ra/ra_test.go | 106 +++++++++++++++++++++++++------------------------- 1 file changed, 52 insertions(+), 54 deletions(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index 5724ce12d77..cbd7d9cc383 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -837,43 +837,60 @@ func TestPerformValidationSuccess(t *testing.T) { test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") } -// mockSAPaused is a mock which always succeeds. It records the PauseRequest it -// received, and returns the number of identifiers as a -// PauseIdentifiersResponse. It does not maintain state of repaused identifiers. type mockSAPaused struct { + sync.RWMutex sapb.StorageAuthorityClient - identType string - identValue string - receivedRegID int64 + authorizationsForRegID map[int64]*corepb.Authorization + identifiersForRegID map[int64][]*corepb.Identifier + registrationsForRegID map[int64]*corepb.Registration +} + +func newMockSAPaused(sa sapb.StorageAuthorityClient) *mockSAPaused { + return &mockSAPaused{ + StorageAuthorityClient: sa, + authorizationsForRegID: make(map[int64]*corepb.Authorization), + identifiersForRegID: make(map[int64][]*corepb.Identifier), + registrationsForRegID: make(map[int64]*corepb.Registration), + } } func (msa *mockSAPaused) GetRegistration(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*corepb.Registration, error) { - if msa.receivedRegID == req.Id { - return Registration, nil - } else { - return nil, fmt.Errorf("Could not find registration") + msa.Lock() + defer msa.Unlock() + regPB, ok := msa.registrationsForRegID[req.Id] + if !ok { + return nil, fmt.Errorf("Unable to find registration for regID %d", req.Id) } + return regPB, nil } func (msa *mockSAPaused) PauseIdentifiers(ctx context.Context, req *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) { + msa.Lock() + defer msa.Unlock() if len(req.Identifiers) <= 0 { return nil, fmt.Errorf("No identifiers found to pause") } - msa.identType = req.Identifiers[0].Type - msa.identValue = req.Identifiers[0].Value - return &sapb.PauseIdentifiersResponse{Paused: int64(len(req.Identifiers))}, nil + msa.identifiersForRegID[req.RegistrationID] = req.Identifiers + + counts := make(map[int64]int64) + for range msa.identifiersForRegID { + counts[req.RegistrationID]++ + } + return &sapb.PauseIdentifiersResponse{Paused: counts[req.RegistrationID]}, nil } func (msa *mockSAPaused) GetPausedIdentifiers(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*sapb.Identifiers, error) { - if msa.identType != "" && msa.identValue != "" { - return &sapb.Identifiers{ - Identifiers: []*corepb.Identifier{{ - Type: msa.identType, - Value: msa.identValue, - }}, - }, nil + msa.Lock() + defer msa.Unlock() + _, ok := msa.registrationsForRegID[req.Id] + if !ok { + return nil, fmt.Errorf("Unable to find registration for regID %d", req.Id) + } + idents, ok := msa.identifiersForRegID[req.Id] + if !ok { + return nil, fmt.Errorf("No identifiers paused yet") } - return nil, fmt.Errorf("No identifiers paused yet") + return &sapb.Identifiers{Identifiers: idents}, nil } func (msa *mockSAPaused) FinalizeAuthorization2(ctx context.Context, req *sapb.FinalizeAuthorizationRequest, _ ...grpc.CallOption) (*emptypb.Empty, error) { @@ -891,7 +908,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) defer features.Reset() - mockSA := &mockSAPaused{} + mockSA := newMockSAPaused(sa) ra.SA = mockSA // Override the default ratelimits to only allow one failed validation. @@ -902,7 +919,8 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * // We know this is OK because of TestNewAuthorization domain := "example.net" authzPB := createPendingAuthorization(t, sa, domain, fc.Now().Add(12*time.Hour)) - mockSA.receivedRegID = authzPB.RegistrationID + mockSA.registrationsForRegID[authzPB.RegistrationID] = Registration + mockSA.authorizationsForRegID[authzPB.RegistrationID] = authzPB // We induce the bad path by setting a problem. This will consume all // available capacity in the rate limit bucket. @@ -941,7 +959,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) // Sleep so the RA has a chance to write to the SA - time.Sleep(100 * time.Millisecond) + time.Sleep(200 * time.Millisecond) got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") @@ -1005,8 +1023,8 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * test.AssertEquals(t, len(got.Identifiers), 1) test.AssertEquals(t, got.Identifiers[0].Value, domain) - //err = ra.limiter.Reset(ctx, bucketKey) - //test.AssertNotError(t, err, "Failed cleaning up redis") + err = ra.limiter.Reset(ctx, bucketKey) + test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersRatelimit(t *testing.T) { @@ -1020,8 +1038,8 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) defer features.Reset() - originalSA := sa - mockSA := &mockSAPaused{} + //originalSA := sa + mockSA := newMockSAPaused(sa) ra.SA = mockSA // Override the default ratelimits to only allow one failed validation. @@ -1032,7 +1050,8 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR // We know this is OK because of TestNewAuthorization domain := "example.net" authzPB := createPendingAuthorization(t, sa, "example.net", fc.Now().Add(12*time.Hour)) - mockSA.receivedRegID = authzPB.RegistrationID + mockSA.registrationsForRegID[authzPB.RegistrationID] = Registration + mockSA.authorizationsForRegID[authzPB.RegistrationID] = authzPB // We induce the bad path by setting a problem. This will consume all // available capacity in the rate limit bucket. @@ -1095,10 +1114,10 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR // Now the goal is to perform a successful validation which should reset the // FailedAuthorizationsForPausingPerDomainPerAccount ratelimit. // - ra.SA = originalSA // We know this is OK because of TestNewAuthorization - authzPB = createPendingAuthorization(t, originalSA, domain, fc.Now().Add(12*time.Hour)) + authzPB = createPendingAuthorization(t, sa, domain, fc.Now().Add(12*time.Hour)) + mockSA.authorizationsForRegID[authzPB.RegistrationID] = authzPB va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ @@ -1113,7 +1132,6 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR Problems: nil, } - now := fc.Now() challIdx = dnsChallIdx(t, authzPB.Challenges) authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ Authz: authzPB, @@ -1132,26 +1150,6 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) - // Sleep so the RA has a chance to write to the SA - time.Sleep(100 * time.Millisecond) - dbAuthzPB := getAuthorization(t, authzPB.Id, originalSA) - - // Verify that the responses are reflected - challIdx = dnsChallIdx(t, dbAuthzPB.Challenges) - challenge, err := bgrpc.PBToChallenge(dbAuthzPB.Challenges[challIdx]) - test.AssertNotError(t, err, "Failed to marshall corepb.Challenge to core.Challenge.") - - test.AssertNotNil(t, vaRequest.Challenge, "Request passed to VA has no challenge") - test.Assert(t, challenge.Status == core.StatusValid, "challenge was not marked as valid") - - // The DB authz's expiry should be equal to the current time plus the - // configured authorization lifetime - test.AssertEquals(t, dbAuthzPB.Expires.AsTime(), now.Add(ra.authorizationLifetime)) - - // Check that validated timestamp was recorded, stored, and retrieved - expectedValidated := fc.Now() - test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") - // We need the bucket key to scan for in Redis bucketKey, err = ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") @@ -1162,8 +1160,8 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR _, err = redisSrc.Get(ctx, bucketKey) test.AssertErrorIs(t, err, ratelimits.ErrBucketNotFound) - //err = ra.limiter.Reset(ctx, bucketKey) - //test.AssertNotError(t, err, "Failed cleaning up redis") + err = ra.limiter.Reset(ctx, bucketKey) + test.AssertNotError(t, err, "Failed cleaning up redis") } func TestPerformValidationVAError(t *testing.T) { From 85f21e86f919f208ae48eade36ded25766428016 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 6 Nov 2024 16:19:06 -0500 Subject: [PATCH 24/29] Fix? --- ra/ra_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index cbd7d9cc383..16d324fa73a 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -1117,7 +1117,6 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR // We know this is OK because of TestNewAuthorization authzPB = createPendingAuthorization(t, sa, domain, fc.Now().Add(12*time.Hour)) - mockSA.authorizationsForRegID[authzPB.RegistrationID] = authzPB va.PerformValidationRequestResultReturn = &vapb.ValidationResult{ Records: []*corepb.ValidationRecord{ @@ -1138,6 +1137,7 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR ChallengeIndex: challIdx, }) test.AssertNotError(t, err, "PerformValidation failed") + mockSA.authorizationsForRegID[authzPB.RegistrationID] = authzPB select { case r := <-va.performValidationRequest: From cc01553532efbc6c34cafdaeef006f5795b2471d Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 7 Nov 2024 09:03:24 -0500 Subject: [PATCH 25/29] Fix tests from merging main --- ra/ra_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index c8211ec955e..36dfe4ed64d 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -4863,7 +4863,7 @@ func (sa *mockSARecordingRegistration) UpdateRegistrationKey(ctx context.Context // to the SA; passes the updated Registration back to the caller; and can return // an error. func TestUpdateRegistrationContact(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() expectRegID := int64(1) @@ -4917,7 +4917,7 @@ func TestUpdateRegistrationContact(t *testing.T) { // correctly requires a registration ID and key, passes them to the SA, and // passes the updated Registration back to the caller. func TestUpdateRegistrationKey(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) + _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() expectRegID := int64(1) From fb547cdb0fa7a928c965b6def9595404b504e6cb Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 7 Nov 2024 09:43:38 -0500 Subject: [PATCH 26/29] Cleanup comments --- ra/ra.go | 6 ++---- ratelimits/names.go | 9 ++++++++- ratelimits/transaction.go | 1 - test/config-next/wfe2-ratelimit-defaults.yml | 6 +++--- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 1748f897a79..b1215431730 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1815,14 +1815,12 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI // per account rate limit. There is no reason to surface errors from this // function to the Subscriber, spends against this limit are best effort. func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { - var name = ident.Value - if ra.limiter == nil || ra.txnBuilder == nil { // Limiter is disabled. return } - txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, name) + txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, ident.Value) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } @@ -1836,7 +1834,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } if features.Get().UseKvLimitsForZombieClientPausing { - txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, name) + txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, ident.Value) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) } diff --git a/ratelimits/names.go b/ratelimits/names.go index a44b16fd490..99221ae0cec 100644 --- a/ratelimits/names.go +++ b/ratelimits/names.go @@ -77,7 +77,14 @@ const ( // passed as a comma-separated list of domain names. CertificatesPerFQDNSet - // TODO: @kruti-s + // FailedAuthorizationsForPausingPerDomainPerAccount is similar to + // FailedAuthorizationsPerDomainPerAccount in that it uses two different + // bucket keys depending on the context: + // - When referenced in an overrides file: uses bucket key 'enum:regId', + // where regId is the ACME registration Id of the account. + // - When referenced in a transaction: uses bucket key 'enum:regId:domain', + // where regId is the ACME registration Id of the account and domain is a + // domain name in the certificate. FailedAuthorizationsForPausingPerDomainPerAccount ) diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index dc28e4b8fbc..cb5eb489fa5 100644 --- a/ratelimits/transaction.go +++ b/ratelimits/transaction.go @@ -536,7 +536,6 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names transactions = append(transactions, txn) } - // TODO: @kruti-s do I need something like this for IssuancePaused? txns, err := builder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names) if err != nil { return nil, makeTxnError(err, FailedAuthorizationsPerDomainPerAccount) diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index e461607f4cb..d934b508cc8 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -14,9 +14,9 @@ FailedAuthorizationsPerDomainPerAccount: count: 3 burst: 3 period: 5m -# The burst represents failing 40 times per day for 90 days. -# The count and period grant one "freebie" failure per day. -# In combination, these parameters mean that: +# The burst represents failing 40 times per day for 90 days. The count and +# period grant one "freebie" failure per day. In combination, these parameters +# mean that: # - Failing 120 times per day results in being paused after 30.25 days # - Failing 40 times per day results in being paused after 92.3 days # - Failing 20 times per day results in being paused after 6.2 months From 30ea374ecf6309d77ddc993a7dbe34b82f54a245 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 8 Nov 2024 09:48:15 -0500 Subject: [PATCH 27/29] Address comment --- ra/ra.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ra/ra.go b/ra/ra.go index b1215431730..08f7ea9f074 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1858,7 +1858,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, }, }) if err != nil { - ra.log.Warningf("pausing identifier for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) + ra.log.Warningf("failed to auto-pause %d/%q: %s", regId, ident.Value, err) } } } From 2e65dc33b899556e86aba9d8fdc98b89233a0b53 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 8 Nov 2024 14:29:59 -0500 Subject: [PATCH 28/29] Address comments --- features/features.go | 4 ++-- ra/ra.go | 27 ++++++++++++++++++++------- ra/ra_test.go | 9 +++++---- ratelimits/limiter.go | 18 ++---------------- ratelimits/limiter_test.go | 17 ----------------- test/config-next/ra.json | 2 +- 6 files changed, 30 insertions(+), 47 deletions(-) diff --git a/features/features.go b/features/features.go index 099a4295f66..baf3cfa2538 100644 --- a/features/features.go +++ b/features/features.go @@ -119,12 +119,12 @@ type Config struct { // unique "INSERT ... RETURNING" functionality. InsertAuthzsIndividually bool - // UseKvLimitsForZombieClientPausing when enabled, causes the key-value rate + // AutomaticallyPauseZombieClients configures the RA to automatically track // limiter to be the authoritative source of rate limiting information for // automatically pausing clients who systemically fail every validation // attempt. When disabled, only manually paused identifier:accountID:domain // pairs will be rejected. - UseKvLimitsForZombieClientPausing bool + AutomaticallyPauseZombieClients bool // IncrementRateLimits uses Redis' IncrBy, instead of Set, for rate limit // accounting. This catches and denies spikes of requests much more diff --git a/ra/ra.go b/ra/ra.go index 08f7ea9f074..7fe7c5d916d 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -122,6 +122,7 @@ type RegistrationAuthorityImpl struct { orderAges *prometheus.HistogramVec inflightFinalizes prometheus.Gauge certCSRMismatch prometheus.Counter + pauseCounter *prometheus.CounterVec } var _ rapb.RegistrationAuthorityServer = (*RegistrationAuthorityImpl)(nil) @@ -241,6 +242,12 @@ func NewRegistrationAuthorityImpl( }) stats.MustRegister(certCSRMismatch) + pauseCounter := prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "paused_pairs", + Help: "Number of times a pause operation is performed, labeled by paused=[bool], repaused=[bool], grace=[bool]", + }, []string{"paused", "repaused", "grace"}) + stats.MustRegister(pauseCounter) + issuersByNameID := make(map[issuance.NameID]*issuance.Certificate) for _, issuer := range issuers { issuersByNameID[issuer.NameID()] = issuer @@ -276,6 +283,7 @@ func NewRegistrationAuthorityImpl( orderAges: orderAges, inflightFinalizes: inflightFinalizes, certCSRMismatch: certCSRMismatch, + pauseCounter: pauseCounter, } return ra } @@ -1810,7 +1818,7 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI } // countFailedValidation increments the failed authorizations per domain per -// account rate limit. If the UseKvLimitsForZombieClientPausing feature has been +// account rate limit. If the AutomaticallyPauseZombieClients feature has been // enabled, it also increments the failed authorizations for pausing per domain // per account rate limit. There is no reason to surface errors from this // function to the Subscriber, spends against this limit are best effort. @@ -1833,7 +1841,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } - if features.Get().UseKvLimitsForZombieClientPausing { + if features.Get().AutomaticallyPauseZombieClients { txn, err = ra.txnBuilder.FailedAuthorizationsForPausingPerDomainPerAccountTransaction(regId, ident.Value) if err != nil { ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, err) @@ -1848,7 +1856,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, } if decision.Result(ra.clk.Now()) != nil { - _, err := ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ + resp, err := ra.SA.PauseIdentifiers(ctx, &sapb.PauseRequest{ RegistrationID: regId, Identifiers: []*corepb.Identifier{ { @@ -1858,8 +1866,13 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, }, }) if err != nil { - ra.log.Warningf("failed to auto-pause %d/%q: %s", regId, ident.Value, err) + ra.log.Warningf("failed to pause %d/%q: %s", regId, ident.Value, err) } + ra.pauseCounter.With(prometheus.Labels{ + "paused": strconv.FormatBool(resp.Paused > 0), + "repaused": strconv.FormatBool(resp.Repaused > 0), + "grace": strconv.FormatBool(resp.Paused <= 0 && resp.Repaused <= 0), + }).Inc() } } } @@ -1869,11 +1882,11 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) { bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, regId, ident.Value) if err != nil { - ra.log.Warningf("Can't get domain bucket key for regID=[%d] authzID=[%s] err=[%s]", regId, ident, err) + ra.log.Warningf("creating bucket key for regID=[%d] identifier=[%s]: %s", regId, ident.Value, err) } err = ra.limiter.Reset(ctx, bucketKey) if err != nil { - ra.log.Warningf("Can't reset domain bucket key for regID=[%d] authzID=[%s] err=[%s]", regId, ident, err) + ra.log.Warningf("resetting bucket for regID=[%d] identifier=[%s]: %s", regId, ident.Value, err) } } @@ -2001,7 +2014,7 @@ func (ra *RegistrationAuthorityImpl) PerformValidation( go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier) } else { challenge.Status = core.StatusValid - if features.Get().UseKvLimitsForZombieClientPausing { + if features.Get().AutomaticallyPauseZombieClients { ra.resetAccountPausingLimit(vaCtx, authz.RegistrationID, authz.Identifier) } } diff --git a/ra/ra_test.go b/ra/ra_test.go index 36dfe4ed64d..ad24bfe8eec 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -916,7 +916,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() - features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) + features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() mockSA := newMockSAPaused(sa) @@ -975,6 +975,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") test.AssertBoxedNil(t, got, "Should have received nil response, but did not") + test.AssertMetricWithLabelsEquals(t, ra.pauseCounter, prometheus.Labels{"paused": "false", "repaused": "false", "grace": "false"}, 0) // We need the bucket key to scan for in Redis bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) @@ -1033,6 +1034,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * test.AssertNotError(t, err, "Should not have errored getting paused identifiers") test.AssertEquals(t, len(got.Identifiers), 1) test.AssertEquals(t, got.Identifiers[0].Value, domain) + test.AssertMetricWithLabelsEquals(t, ra.pauseCounter, prometheus.Labels{"paused": "true", "repaused": "false", "grace": "false"}, 1) err = ra.limiter.Reset(ctx, bucketKey) test.AssertNotError(t, err, "Failed cleaning up redis") @@ -1046,7 +1048,7 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR va, sa, ra, redisSrc, fc, cleanUp := initAuthorities(t) defer cleanUp() - features.Set(features.Config{UseKvLimitsForZombieClientPausing: true}) + features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() //originalSA := sa @@ -1106,8 +1108,7 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") test.AssertBoxedNil(t, got, "Should have received nil response, but did not") - // Avoid a datarace - time.Sleep(100 * time.Millisecond) + test.AssertMetricWithLabelsEquals(t, ra.pauseCounter, prometheus.Labels{"paused": "false", "repaused": "false", "grace": "false"}, 0) // We need the bucket key to scan for in Redis bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 0c9a1cd7be7..86aa3cfae6e 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -111,6 +111,8 @@ func (d *Decision) Result(now time.Time) error { retryAfter := d.retryIn + jitter retryAfterTs := now.UTC().Add(retryAfter).Format("2006-01-02 15:04:05 MST") + // There is no case for FailedAuthorizationsForPausingPerDomainPerAccount + // because the RA will pause clients who exceed that ratelimit. switch d.transaction.limit.name { case NewRegistrationsPerIPAddress: return berrors.RegistrationsPerIPAddressError( @@ -154,22 +156,6 @@ func (d *Decision) Result(now time.Time) error { retryAfterTs, ) - case FailedAuthorizationsForPausingPerDomainPerAccount: - // Uses bucket key 'enum:regId:domain'. - idx := strings.LastIndex(d.transaction.bucketKey, ":") - if idx == -1 { - return berrors.InternalServerError("unrecognized bucket key while generating error") - } - domain := d.transaction.bucketKey[idx+1:] - return berrors.FailedAuthorizationsPerDomainPerAccountError( - retryAfter, - "too many failed validation attempts (%d) for %q in the last %s, retry after %s", - d.transaction.limit.Burst, - domain, - d.transaction.limit.Period.Duration, - retryAfterTs, - ) - case CertificatesPerDomain, CertificatesPerDomainPerAccount: // Uses bucket key 'enum:domain' or 'enum:regId:domain' respectively. idx := strings.LastIndex(d.transaction.bucketKey, ":") diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 72f5138994d..41e89c36ad7 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -556,23 +556,6 @@ func TestRateLimitError(t *testing.T) { expectedErr: "too many failed authorizations (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", expectedErrType: berrors.RateLimit, }, - { - name: "FailedAuthorizationsForPausingPerDomainPerAccount limit reached", - decision: &Decision{ - allowed: false, - retryIn: 15 * time.Second, - transaction: Transaction{ - limit: limit{ - name: FailedAuthorizationsForPausingPerDomainPerAccount, - Burst: 7, - Period: config.Duration{Duration: time.Hour}, - }, - bucketKey: "8:12345:example.com", - }, - }, - expectedErr: "too many failed validation attempts (7) for \"example.com\" in the last 1h0m0s, retry after 1970-01-01 00:00:15 UTC: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account", - expectedErrType: berrors.RateLimit, - }, { name: "CertificatesPerDomain limit reached", decision: &Decision{ diff --git a/test/config-next/ra.json b/test/config-next/ra.json index 0a50b8beb87..6e84c2e7ba6 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -128,7 +128,7 @@ "features": { "AsyncFinalize": true, "UseKvLimitsForNewOrder": true, - "UseKvLimitsForZombieClientPausing": true, + "AutomaticallyPauseZombieClients": true, "IncrementRateLimits": true }, "ctLogs": { From 4c7be8cc9cb78d315ed7e8fe3f7d5b2f787191fe Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 8 Nov 2024 15:33:06 -0500 Subject: [PATCH 29/29] Address next round of comments --- features/features.go | 4 ++-- ra/ra_test.go | 39 ++++++++++----------------------------- 2 files changed, 12 insertions(+), 31 deletions(-) diff --git a/features/features.go b/features/features.go index baf3cfa2538..90f7c7315c7 100644 --- a/features/features.go +++ b/features/features.go @@ -122,8 +122,8 @@ type Config struct { // AutomaticallyPauseZombieClients configures the RA to automatically track // limiter to be the authoritative source of rate limiting information for // automatically pausing clients who systemically fail every validation - // attempt. When disabled, only manually paused identifier:accountID:domain - // pairs will be rejected. + // attempt. When disabled, only manually paused accountID:identifier pairs + // will be rejected. AutomaticallyPauseZombieClients bool // IncrementRateLimits uses Redis' IncrBy, instead of Set, for rate limit diff --git a/ra/ra_test.go b/ra/ra_test.go index ad24bfe8eec..7cb1a2cf842 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -957,20 +957,15 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * }) test.AssertNotError(t, err, "PerformValidation failed") - var vaRequest *vapb.PerformValidationRequest select { case r := <-va.performValidationRequest: - vaRequest = r + _ = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") } - // Verify that the VA got the request, and it's the same as the others - test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) - test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) - // Sleep so the RA has a chance to write to the SA - time.Sleep(200 * time.Millisecond) + time.Sleep(100 * time.Millisecond) got, err := ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) test.AssertError(t, err, "Should not have any paused identifiers yet, but found some") @@ -981,7 +976,7 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") - // Verify that a redis entry exists for this identifier:accountID:domain + // Verify that a redis entry exists for this accountID:identifier. tat, err := redisSrc.Get(ctx, bucketKey) test.AssertNotError(t, err, "Should not have errored, but did") @@ -1017,17 +1012,13 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * select { case r := <-va.performValidationRequest: - vaRequest = r + _ = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") } - // Verify that the VA got the request, and it's the same as the others. - test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) - test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) - // Sleep so the RA has a chance to write to the SA - time.Sleep(200 * time.Millisecond) + time.Sleep(100 * time.Millisecond) // Ensure the identifier:account:domain we expect to be paused actually is. got, err = ra.SA.GetPausedIdentifiers(ctx, &sapb.RegistrationID{Id: authzPB.RegistrationID}, nil) @@ -1051,7 +1042,6 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() - //originalSA := sa mockSA := newMockSAPaused(sa) ra.SA = mockSA @@ -1090,18 +1080,13 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR }) test.AssertNotError(t, err, "PerformValidation failed") - var vaRequest *vapb.PerformValidationRequest select { case r := <-va.performValidationRequest: - vaRequest = r + _ = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") } - // Verify that the VA got the request, and it's the same as the others - test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) - test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) - // Sleep so the RA has a chance to write to the SA time.Sleep(100 * time.Millisecond) @@ -1114,7 +1099,7 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") - // Verify that a redis entry exists for this identifier:accountID:domain + // Verify that a redis entry exists for this accountID:identifier tat, err := redisSrc.Get(ctx, bucketKey) test.AssertNotError(t, err, "Should not have errored, but did") @@ -1153,22 +1138,18 @@ func TestPerformValidation_FailedThenSuccessfulValidationResetsPauseIdentifiersR select { case r := <-va.performValidationRequest: - vaRequest = r + _ = r case <-time.After(time.Second): t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete") } - // Verify that the VA got the request, and it's the same as the others - test.AssertEquals(t, authzPB.Challenges[challIdx].Type, vaRequest.Challenge.Type) - test.AssertEquals(t, authzPB.Challenges[challIdx].Token, vaRequest.Challenge.Token) - // We need the bucket key to scan for in Redis bucketKey, err = ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, authzPB.RegistrationID, domain) test.AssertNotError(t, err, "Should have been able to construct bucket key, but could not") // Verify that the bucket no longer exists (because the limiter reset has - // deleted it). This indicates the identifier:accountID:domain bucket has - // regained capacity avoiding being inadvertently paused. + // deleted it). This indicates the accountID:identifier bucket has regained + // capacity avoiding being inadvertently paused. _, err = redisSrc.Get(ctx, bucketKey) test.AssertErrorIs(t, err, ratelimits.ErrBucketNotFound)