From 62299362bd8fab5fcdbaaef227c0ef52713f7e34 Mon Sep 17 00:00:00 2001 From: James Renken Date: Wed, 18 Dec 2024 14:23:13 -0800 Subject: [PATCH] ra/ratelimits: Update tests, use new TransactionBuilder constructor, fix ARI rate limit exception (#7869) Add a new `ratelimits.NewTransactionBuilderWithLimits` constructor which takes pre-populated rate limit data, instead of filenames for reading it off disk. Use this new constructor to change rate limits during RA tests, instead of using extra `testdata` files. Fix ARI renewals' exception from rate limits: consider `isARIRenewal` as part of the `isRenewal` arg to `checkNewOrderLimits`. Remove obsolete RA tests for rate limits that are now only checked in the WFE. Update remaining new order rate limit tests from deprecated `ratelimit`s to new Redis `ratelimits`. --- cmd/boulder-ra/main.go | 2 +- cmd/boulder-wfe2/main.go | 2 +- ra/ra_test.go | 369 ++---------------- .../one-failed-validation-before-pausing.yml | 4 - ratelimits/gcra.go | 6 +- ratelimits/gcra_test.go | 5 +- ratelimits/limit.go | 158 +++++--- ratelimits/limit_test.go | 94 +++-- ratelimits/limiter.go | 26 +- ratelimits/limiter_test.go | 26 +- ratelimits/transaction.go | 25 +- ratelimits/transaction_test.go | 46 ++- wfe2/wfe.go | 19 +- wfe2/wfe_test.go | 110 +++++- 14 files changed, 382 insertions(+), 510 deletions(-) delete mode 100644 ra/testdata/one-failed-validation-before-pausing.yml diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go index 9710e4b5176..e71cbb37bc8 100644 --- a/cmd/boulder-ra/main.go +++ b/cmd/boulder-ra/main.go @@ -273,7 +273,7 @@ func main() { source := ratelimits.NewRedisSource(limiterRedis.Ring, clk, scope) limiter, err = ratelimits.NewLimiter(clk, source, scope) cmd.FailOnError(err, "Failed to create rate limiter") - txnBuilder, err = ratelimits.NewTransactionBuilder(c.RA.Limiter.Defaults, c.RA.Limiter.Overrides) + txnBuilder, err = ratelimits.NewTransactionBuilderFromFiles(c.RA.Limiter.Defaults, c.RA.Limiter.Overrides) cmd.FailOnError(err, "Failed to create rate limits transaction builder") } diff --git a/cmd/boulder-wfe2/main.go b/cmd/boulder-wfe2/main.go index 699ed0d787d..2ad988180fc 100644 --- a/cmd/boulder-wfe2/main.go +++ b/cmd/boulder-wfe2/main.go @@ -365,7 +365,7 @@ func main() { source := ratelimits.NewRedisSource(limiterRedis.Ring, clk, stats) limiter, err = ratelimits.NewLimiter(clk, source, stats) cmd.FailOnError(err, "Failed to create rate limiter") - txnBuilder, err = ratelimits.NewTransactionBuilder(c.WFE.Limiter.Defaults, c.WFE.Limiter.Overrides) + txnBuilder, err = ratelimits.NewTransactionBuilderFromFiles(c.WFE.Limiter.Defaults, c.WFE.Limiter.Overrides) cmd.FailOnError(err, "Failed to create rate limits transaction builder") } diff --git a/ra/ra_test.go b/ra/ra_test.go index e7f439d688e..143178e4ead 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -261,55 +261,6 @@ var ( 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 -} - -func (r *dummyRateLimitConfig) CertificatesPerName() ratelimit.RateLimitPolicy { - return r.CertificatesPerNamePolicy -} - -func (r *dummyRateLimitConfig) RegistrationsPerIP() ratelimit.RateLimitPolicy { - return r.RegistrationsPerIPPolicy -} - -func (r *dummyRateLimitConfig) RegistrationsPerIPRange() ratelimit.RateLimitPolicy { - return r.RegistrationsPerIPRangePolicy -} - -func (r *dummyRateLimitConfig) PendingAuthorizationsPerAccount() ratelimit.RateLimitPolicy { - return r.PendingAuthorizationsPerAccountPolicy -} - -func (r *dummyRateLimitConfig) NewOrdersPerAccount() ratelimit.RateLimitPolicy { - return r.NewOrdersPerAccountPolicy -} - -func (r *dummyRateLimitConfig) InvalidAuthorizationsPerAccount() ratelimit.RateLimitPolicy { - return r.InvalidAuthorizationsPerAccountPolicy -} - -func (r *dummyRateLimitConfig) CertificatesPerFQDNSet() ratelimit.RateLimitPolicy { - return r.CertificatesPerFQDNSetPolicy -} - -func (r *dummyRateLimitConfig) CertificatesPerFQDNSetFast() ratelimit.RateLimitPolicy { - return r.CertificatesPerFQDNSetFastPolicy -} - -func (r *dummyRateLimitConfig) LoadPolicies(contents []byte) error { - return nil // NOP - unrequired behaviour for this mock -} - func newAcctKey(t *testing.T) []byte { key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) jwk := &jose.JSONWebKey{Key: key.Public()} @@ -392,7 +343,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho rlSource := ratelimits.NewInmemSource() limiter, err := ratelimits.NewLimiter(fc, rlSource, stats) test.AssertNotError(t, err, "making limiter") - txnBuilder, err := ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + txnBuilder, err := ratelimits.NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "making transaction composer") testKeyPolicy, err := goodkey.NewPolicy(nil, nil) @@ -844,8 +795,14 @@ func TestPerformValidation_FailedValidationsTriggerPauseIdentifiersRatelimit(t * out: pauseChan, } - // Override the default ratelimits to only allow one failed validation per 24 hours. - txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/one-failed-validation-before-pausing.yml", "") + // Set the default ratelimits to only allow one failed validation per 24 + // hours before pausing. + txnBuilder, err := ratelimits.NewTransactionBuilder(ratelimits.LimitConfigs{ + ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount.String(): &ratelimits.LimitConfig{ + Burst: 1, + Count: 1, + Period: config.Duration{Duration: time.Hour * 24}}, + }) test.AssertNotError(t, err, "making transaction composer") ra.txnBuilder = txnBuilder @@ -1055,101 +1012,6 @@ func TestCertificateKeyNotEqualAccountKey(t *testing.T) { test.AssertEquals(t, err.Error(), "certificate public key must be different than account key") } -func TestNewOrderRateLimiting(t *testing.T) { - _, _, ra, _, fc, cleanUp := initAuthorities(t) - defer cleanUp() - - // Create a dummy rate limit config that sets a NewOrdersPerAccount rate - // limit with a very low threshold/short window - rateLimitDuration := 5 * time.Minute - ra.rlPolicies = &dummyRateLimitConfig{ - NewOrdersPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: rateLimitDuration}, - }, - } - - orderOne := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"first.example.com"}, - } - orderTwo := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"second.example.com"}, - } - - // To start, it should be possible to create a new order - _, err := ra.NewOrder(ctx, orderOne) - test.AssertNotError(t, err, "NewOrder for orderOne failed") - - // Advance the clock 1s to separate the orders in time - fc.Add(time.Second) - - // Creating an order immediately after the first with different names - // should fail - _, err = ra.NewOrder(ctx, orderTwo) - test.AssertError(t, err, "NewOrder for orderTwo succeeded, should have been ratelimited") - - // Creating the first order again should succeed because of order reuse, no - // new pending order is produced. - _, err = ra.NewOrder(ctx, orderOne) - test.AssertNotError(t, err, "Reuse of orderOne failed") - - // Advancing the clock by 2 * the rate limit duration should allow orderTwo to - // succeed - fc.Add(2 * rateLimitDuration) - _, err = ra.NewOrder(ctx, orderTwo) - test.AssertNotError(t, err, "NewOrder for orderTwo failed after advancing clock") -} - -// 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) - defer cleanUp() - - rateLimitDuration := 5 * time.Minute - - domain := "early-ratelimit-example.com" - - // Set a mock RL policy with a CertificatesPerName threshold for the domain - // name so low if it were enforced it would prevent a new order for any names. - ra.rlPolicies = &dummyRateLimitConfig{ - CertificatesPerNamePolicy: ratelimit.RateLimitPolicy{ - Threshold: 10, - Window: config.Duration{Duration: rateLimitDuration}, - // Setting the Threshold to 0 skips applying the rate limit. Setting an - // override to 0 does the trick. - Overrides: map[string]int64{ - domain: 0, - }, - }, - NewOrdersPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 10, - Window: config.Duration{Duration: rateLimitDuration}, - }, - } - - // Request an order for the test domain - newOrder := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{domain}, - } - - // With the feature flag enabled the NewOrder request should fail because of - // the CertificatesPerNamePolicy. - _, err := ra.NewOrder(ctx, newOrder) - test.AssertError(t, err, "NewOrder did not apply cert rate limits with feature flag enabled") - - var bErr *berrors.BoulderError - test.Assert(t, errors.As(err, &bErr), "NewOrder did not return a boulder error") - test.AssertEquals(t, bErr.RetryAfter, rateLimitDuration) - - // The err should be the expected rate limit error - expected := "too many certificates already issued for \"early-ratelimit-example.com\". Retry after 2020-03-04T05:05:00Z: see https://letsencrypt.org/docs/rate-limits/#new-orders-per-account" - test.AssertEquals(t, bErr.Error(), expected) -} - // mockInvalidAuthorizationsAuthority is a mock which claims that the given // domain has one invalid authorization. type mockInvalidAuthorizationsAuthority struct { @@ -1169,16 +1031,18 @@ func TestAuthzFailedRateLimitingNewOrder(t *testing.T) { _, _, ra, _, _, cleanUp := initAuthorities(t) defer cleanUp() - ra.rlPolicies = &dummyRateLimitConfig{ - InvalidAuthorizationsPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: 1 * time.Hour}, - }, - } + txnBuilder, err := ratelimits.NewTransactionBuilder(ratelimits.LimitConfigs{ + ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount.String(): &ratelimits.LimitConfig{ + Burst: 1, + Count: 1, + Period: config.Duration{Duration: time.Hour * 1}}, + }) + test.AssertNotError(t, err, "making transaction composer") + ra.txnBuilder = txnBuilder limit := ra.rlPolicies.InvalidAuthorizationsPerAccount() ra.SA = &mockInvalidAuthorizationsAuthority{domainWithFailures: "all.i.do.is.lose.com"} - err := ra.checkInvalidAuthorizationLimits(ctx, Registration.Id, + err = ra.checkInvalidAuthorizationLimits(ctx, Registration.Id, []string{"charlie.brown.com", "all.i.do.is.lose.com"}, limit) test.AssertError(t, err, "checkInvalidAuthorizationLimits did not encounter expected rate limit error") test.AssertEquals(t, err.Error(), "too many failed authorizations recently: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account") @@ -1541,8 +1405,7 @@ func (m mockSAWithFQDNSet) FQDNSetTimestampsForWindow(_ context.Context, req *sa } } -// TestExactPublicSuffixCertLimit tests the behaviour of issue #2681 with and -// without the feature flag for the fix enabled. +// TestExactPublicSuffixCertLimit tests the behaviour of issue #2681. // See https://github.com/letsencrypt/boulder/issues/2681 func TestExactPublicSuffixCertLimit(t *testing.T) { _, _, ra, _, fc, cleanUp := initAuthorities(t) @@ -1637,8 +1500,14 @@ func TestDeactivateAuthorization_Pausing(t *testing.T) { features.Set(features.Config{AutomaticallyPauseZombieClients: true}) defer features.Reset() - // Override the default ratelimits to only allow one failed validation. - txnBuilder, err := ratelimits.NewTransactionBuilder("testdata/one-failed-validation-before-pausing.yml", "") + // Set the default ratelimits to only allow one failed validation per 24 + // hours before pausing. + txnBuilder, err := ratelimits.NewTransactionBuilder(ratelimits.LimitConfigs{ + ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount.String(): &ratelimits.LimitConfig{ + Burst: 1, + Count: 1, + Period: config.Duration{Duration: time.Hour * 24}}, + }) test.AssertNotError(t, err, "making transaction composer") ra.txnBuilder = txnBuilder @@ -2392,113 +2261,6 @@ func TestNewOrder_AuthzReuse_NoPending(t *testing.T) { test.AssertNotEquals(t, new.V2Authorizations[0], extant.V2Authorizations[0]) } -// mockSACountPendingFails has a CountPendingAuthorizations2 implementation -// that always returns error -type mockSACountPendingFails struct { - sapb.StorageAuthorityClient -} - -func (mock *mockSACountPendingFails) CountPendingAuthorizations2(ctx context.Context, req *sapb.RegistrationID, _ ...grpc.CallOption) (*sapb.Count, error) { - return nil, errors.New("counting is slow and boring") -} - -// 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) - defer cleanUp() - - ra.rlPolicies = &dummyRateLimitConfig{ - PendingAuthorizationsPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: 24 * time.Hour}, - RegistrationOverrides: map[int64]int64{ - 13: -1, - }, - }, - } - - ra.SA = &mockSACountPendingFails{} - - limit := ra.rlPolicies.PendingAuthorizationsPerAccount() - err := ra.checkPendingAuthorizationLimit(context.Background(), 13, limit) - test.AssertNotError(t, err, "checking pending authorization limit") -} - -// An authority that returns nonzero failures for CountInvalidAuthorizations2, -// and also returns existing authzs for the same domain from GetAuthorizations2 -type mockInvalidPlusValidAuthzAuthority struct { - mockSAWithAuthzs - domainWithFailures string -} - -func (sa *mockInvalidPlusValidAuthzAuthority) CountInvalidAuthorizations2(ctx context.Context, req *sapb.CountInvalidAuthorizationsRequest, _ ...grpc.CallOption) (*sapb.Count, error) { - if req.DnsName == sa.domainWithFailures { - return &sapb.Count{Count: 1}, nil - } else { - return &sapb.Count{}, nil - } -} - -// Test that the failed authorizations limit is checked before authz reuse. -func TestNewOrderCheckFailedAuthorizationsFirst(t *testing.T) { - _, _, ra, _, clk, cleanUp := initAuthorities(t) - defer cleanUp() - - // Create an order (and thus a pending authz) for example.com - ctx := context.Background() - order, err := ra.NewOrder(ctx, &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"example.com"}, - }) - test.AssertNotError(t, err, "adding an initial order for regA") - test.AssertNotNil(t, order.Id, "initial order had a nil ID") - test.AssertEquals(t, numAuthorizations(order), 1) - - // Now treat example.com as if it had a recent failure, but also a valid authz. - expires := clk.Now().Add(24 * time.Hour) - ra.SA = &mockInvalidPlusValidAuthzAuthority{ - mockSAWithAuthzs: mockSAWithAuthzs{ - authzs: []*core.Authorization{ - { - ID: "1", - Identifier: identifier.NewDNS("example.com"), - RegistrationID: Registration.Id, - Expires: &expires, - Status: "valid", - Challenges: []core.Challenge{ - { - Type: core.ChallengeTypeHTTP01, - Status: core.StatusValid, - Token: core.NewToken(), - }, - }, - }, - }, - }, - domainWithFailures: "example.com", - } - - // Set a very restrictive police for invalid authorizations - one failure - // and you're done for a day. - ra.rlPolicies = &dummyRateLimitConfig{ - InvalidAuthorizationsPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: 24 * time.Hour}, - }, - } - - // Creating an order for example.com should error with the "too many failed - // authorizations recently" error. - _, err = ra.NewOrder(ctx, &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"example.com"}, - }) - - test.AssertError(t, err, "expected error for domain with too many failures") - test.AssertEquals(t, err.Error(), "too many failed authorizations recently: see https://letsencrypt.org/docs/rate-limits/#authorization-failures-per-hostname-per-account") -} - // mockSAWithAuthzs has a GetAuthorizations2 method that returns the protobuf // version of its authzs struct member. It also has a fake GetOrderForNames // which always fails, and a fake NewOrderAndAuthzs which always succeeds, to @@ -4622,83 +4384,6 @@ func TestAdministrativelyRevokeCertificate(t *testing.T) { test.AssertError(t, err, "AdministrativelyRevokeCertificate should have failed with just serial for keyCompromise") } -func TestNewOrderRateLimitingExempt(t *testing.T) { - _, _, ra, _, _, cleanUp := initAuthorities(t) - defer cleanUp() - - // Set up a rate limit policy that allows 1 order every 5 minutes. - rateLimitDuration := 5 * time.Minute - ra.rlPolicies = &dummyRateLimitConfig{ - NewOrdersPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: rateLimitDuration}, - }, - } - - exampleOrderOne := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"first.example.com", "second.example.com"}, - } - exampleOrderTwo := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"first.example.com", "third.example.com"}, - } - - // Create an order immediately. - _, err := ra.NewOrder(ctx, exampleOrderOne) - test.AssertNotError(t, err, "orderOne should have succeeded") - - // Create another order immediately. This should fail. - _, err = ra.NewOrder(ctx, exampleOrderTwo) - test.AssertError(t, err, "orderTwo should have failed") - - // Exempt orderTwo from rate limiting. - exampleOrderTwo.IsARIRenewal = true - _, err = ra.NewOrder(ctx, exampleOrderTwo) - test.AssertNotError(t, err, "orderTwo should have succeeded") -} - -func TestNewOrderFailedAuthzRateLimitingExempt(t *testing.T) { - _, _, ra, _, _, cleanUp := initAuthorities(t) - defer cleanUp() - - exampleOrder := &rapb.NewOrderRequest{ - RegistrationID: Registration.Id, - DnsNames: []string{"example.com"}, - } - - // Create an order, and thus a pending authz, for "example.com". - ctx := context.Background() - order, err := ra.NewOrder(ctx, exampleOrder) - test.AssertNotError(t, err, "adding an initial order for regA") - test.AssertNotNil(t, order.Id, "initial order had a nil ID") - test.AssertEquals(t, numAuthorizations(order), 1) - - // Mock SA that has a failed authorization for "example.com". - ra.SA = &mockInvalidPlusValidAuthzAuthority{ - mockSAWithAuthzs{authzs: []*core.Authorization{}}, - "example.com", - } - - // Set up a rate limit policy that allows 1 order every 24 hours. - ra.rlPolicies = &dummyRateLimitConfig{ - InvalidAuthorizationsPerAccountPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: 24 * time.Hour}, - }, - } - - // Requesting a new order for "example.com" should fail due to too many - // failed authorizations. - _, err = ra.NewOrder(ctx, exampleOrder) - test.AssertError(t, err, "expected error for domain with too many failures") - - // Exempt the order from rate limiting. - exampleOrder.IsARIRenewal = true - _, err = ra.NewOrder(ctx, exampleOrder) - test.AssertNotError(t, err, "limit exempt order should have succeeded") -} - // An authority that returns an error from NewOrderAndAuthzs if the // "ReplacesSerial" field of the request is empty. type mockNewOrderMustBeReplacementAuthority struct { diff --git a/ra/testdata/one-failed-validation-before-pausing.yml b/ra/testdata/one-failed-validation-before-pausing.yml deleted file mode 100644 index 57bf710666f..00000000000 --- a/ra/testdata/one-failed-validation-before-pausing.yml +++ /dev/null @@ -1,4 +0,0 @@ -FailedAuthorizationsForPausingPerDomainPerAccount: - count: 1 - burst: 1 - period: 24h diff --git a/ratelimits/gcra.go b/ratelimits/gcra.go index 5a6ff27b8b9..24ae21859ba 100644 --- a/ratelimits/gcra.go +++ b/ratelimits/gcra.go @@ -10,7 +10,7 @@ import ( // returns a Decision struct with the result of the decision and the updated // TAT. The cost must be 0 or greater and <= the burst capacity of the limit. func maybeSpend(clk clock.Clock, txn Transaction, tat time.Time) *Decision { - if txn.cost < 0 || txn.cost > txn.limit.Burst { + if txn.cost < 0 || txn.cost > txn.limit.burst { // The condition above is the union of the conditions checked in Check // and Spend methods of Limiter. If this panic is reached, it means that // the caller has introduced a bug. @@ -67,7 +67,7 @@ func maybeSpend(clk clock.Clock, txn Transaction, tat time.Time) *Decision { // or greater. A cost will only be refunded up to the burst capacity of the // limit. A partial refund is still considered successful. func maybeRefund(clk clock.Clock, txn Transaction, tat time.Time) *Decision { - if txn.cost < 0 || txn.cost > txn.limit.Burst { + if txn.cost < 0 || txn.cost > txn.limit.burst { // The condition above is checked in the Refund method of Limiter. If // this panic is reached, it means that the caller has introduced a bug. panic("invalid cost for maybeRefund") @@ -80,7 +80,7 @@ func maybeRefund(clk clock.Clock, txn Transaction, tat time.Time) *Decision { // The TAT is in the past, therefore the bucket is full. return &Decision{ allowed: false, - remaining: txn.limit.Burst, + remaining: txn.limit.burst, retryIn: time.Duration(0), resetIn: time.Duration(0), newTAT: tat, diff --git a/ratelimits/gcra_test.go b/ratelimits/gcra_test.go index 26b2ca0d082..7f9fb2ca3d2 100644 --- a/ratelimits/gcra_test.go +++ b/ratelimits/gcra_test.go @@ -5,13 +5,14 @@ import ( "time" "github.com/jmhodges/clock" + "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/test" ) func TestDecide(t *testing.T) { clk := clock.NewFake() - limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}} + limit := &limit{burst: 10, count: 1, period: config.Duration{Duration: time.Second}} limit.precompute() // Begin by using 1 of our 10 requests. @@ -138,7 +139,7 @@ func TestDecide(t *testing.T) { func TestMaybeRefund(t *testing.T) { clk := clock.NewFake() - limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}} + limit := &limit{burst: 10, count: 1, period: config.Duration{Duration: time.Second}} limit.precompute() // Begin by using 1 of our 10 requests. diff --git a/ratelimits/limit.go b/ratelimits/limit.go index c6999f1aa83..16dc65ac962 100644 --- a/ratelimits/limit.go +++ b/ratelimits/limit.go @@ -15,11 +15,12 @@ import ( // currently configured. var errLimitDisabled = errors.New("limit disabled") -// limit defines the configuration for a rate limit or a rate limit override. +// LimitConfig defines the exportable configuration for a rate limit or a rate +// limit override, without a `limit`'s internal fields. // -// The zero value of this struct is invalid, because some of the fields must -// be greater than zero. -type limit struct { +// The zero value of this struct is invalid, because some of the fields must be +// greater than zero. +type LimitConfig struct { // Burst specifies maximum concurrent allowed requests at any given time. It // must be greater than zero. Burst int64 @@ -31,6 +32,26 @@ type limit struct { // Period is the duration of time in which the count (of requests) is // allowed. It must be greater than zero. Period config.Duration +} + +type LimitConfigs map[string]*LimitConfig + +// limit defines the configuration for a rate limit or a rate limit override. +// +// The zero value of this struct is invalid, because some of the fields must +// be greater than zero. +type limit struct { + // burst specifies maximum concurrent allowed requests at any given time. It + // must be greater than zero. + burst int64 + + // count is the number of requests allowed per period. It must be greater + // than zero. + count int64 + + // period is the duration of time in which the count (of requests) is + // allowed. It must be greater than zero. + period config.Duration // name is the name of the limit. It must be one of the Name enums defined // in this package. @@ -59,19 +80,19 @@ func (l *limit) isOverride() bool { // precompute calculates the emissionInterval and burstOffset for the limit. func (l *limit) precompute() { - l.emissionInterval = l.Period.Nanoseconds() / l.Count - l.burstOffset = l.emissionInterval * l.Burst + l.emissionInterval = l.period.Nanoseconds() / l.count + l.burstOffset = l.emissionInterval * l.burst } func validateLimit(l *limit) error { - if l.Burst <= 0 { - return fmt.Errorf("invalid burst '%d', must be > 0", l.Burst) + if l.burst <= 0 { + return fmt.Errorf("invalid burst '%d', must be > 0", l.burst) } - if l.Count <= 0 { - return fmt.Errorf("invalid count '%d', must be > 0", l.Count) + if l.count <= 0 { + return fmt.Errorf("invalid count '%d', must be > 0", l.count) } - if l.Period.Duration <= 0 { - return fmt.Errorf("invalid period '%s', must be > 0", l.Period) + if l.period.Duration <= 0 { + return fmt.Errorf("invalid period '%s', must be > 0", l.period) } return nil } @@ -79,8 +100,8 @@ func validateLimit(l *limit) error { type limits map[string]*limit // loadDefaults marshals the defaults YAML file at path into a map of limits. -func loadDefaults(path string) (limits, error) { - lm := make(limits) +func loadDefaults(path string) (LimitConfigs, error) { + lm := make(LimitConfigs) data, err := os.ReadFile(path) if err != nil { return nil, err @@ -93,7 +114,7 @@ func loadDefaults(path string) (limits, error) { } type overrideYAML struct { - limit `yaml:",inline"` + LimitConfig `yaml:",inline"` // Ids is a list of ids that this override applies to. Ids []struct { Id string `yaml:"id"` @@ -142,30 +163,31 @@ func parseOverrideNameId(key string) (Name, string, error) { return name, id, nil } -// loadAndParseOverrideLimits loads override limits from YAML. The YAML file -// must be formatted as a list of maps, where each map has a single key -// representing the limit name and a value that is a map containing the limit -// fields and an additional 'ids' field that is a list of ids that this override -// applies to. -func loadAndParseOverrideLimits(path string) (limits, error) { - fromFile, err := loadOverrides(path) - if err != nil { - return nil, err - } +// parseOverrideLimits validates a YAML list of override limits. It must be +// formatted as a list of maps, where each map has a single key representing the +// limit name and a value that is a map containing the limit fields and an +// additional 'ids' field that is a list of ids that this override applies to. +func parseOverrideLimits(newOverridesYAML overridesYAML) (limits, error) { parsed := make(limits) - for _, ov := range fromFile { + for _, ov := range newOverridesYAML { for k, v := range ov { - limit := &v.limit - err = validateLimit(limit) - if err != nil { - return nil, fmt.Errorf("validating override limit %q: %w", k, err) - } name, ok := stringToName[k] if !ok { return nil, fmt.Errorf("unrecognized name %q in override limit, must be one of %v", k, limitNames) } - v.limit.name = name + + lim := &limit{ + burst: v.Burst, + count: v.Count, + period: v.Period, + name: name, + } + + err := validateLimit(lim) + if err != nil { + return nil, fmt.Errorf("validating override limit %q: %w", k, err) + } for _, entry := range v.Ids { id := entry.Id @@ -174,42 +196,45 @@ func loadAndParseOverrideLimits(path string) (limits, error) { return nil, fmt.Errorf( "validating name %s and id %q for override limit %q: %w", name, id, k, err) } - limit.overrideKey = joinWithColon(name.EnumString(), id) + lim.overrideKey = joinWithColon(name.EnumString(), id) if name == CertificatesPerFQDNSet { // 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, ","))) } - limit.precompute() - parsed[joinWithColon(name.EnumString(), id)] = limit + lim.precompute() + parsed[joinWithColon(name.EnumString(), id)] = lim } } } return parsed, nil } -// loadAndParseDefaultLimits loads default limits from YAML, validates them, and -// parses them into a map of limits keyed by 'Name'. -func loadAndParseDefaultLimits(path string) (limits, error) { - fromFile, err := loadDefaults(path) - if err != nil { - return nil, err - } - parsed := make(limits, len(fromFile)) +// parseDefaultLimits validates a map of default limits and rekeys it by 'Name'. +func parseDefaultLimits(newDefaultLimits LimitConfigs) (limits, error) { + parsed := make(limits) - for k, v := range fromFile { - err := validateLimit(v) - if err != nil { - return nil, fmt.Errorf("parsing default limit %q: %w", k, err) - } + for k, v := range newDefaultLimits { name, ok := stringToName[k] if !ok { return nil, fmt.Errorf("unrecognized name %q in default limit, must be one of %v", k, limitNames) } - v.name = name - v.precompute() - parsed[name.EnumString()] = v + + lim := &limit{ + burst: v.Burst, + count: v.Count, + period: v.Period, + name: name, + } + + err := validateLimit(lim) + if err != nil { + return nil, fmt.Errorf("parsing default limit %q: %w", k, err) + } + + lim.precompute() + parsed[name.EnumString()] = lim } return parsed, nil } @@ -222,26 +247,39 @@ type limitRegistry struct { overrides limits } -func newLimitRegistry(defaults, overrides string) (*limitRegistry, error) { - var err error - registry := &limitRegistry{} - registry.defaults, err = loadAndParseDefaultLimits(defaults) +func newLimitRegistryFromFiles(defaults, overrides string) (*limitRegistry, error) { + defaultsData, err := loadDefaults(defaults) if err != nil { return nil, err } if overrides == "" { - // No overrides specified, initialize an empty map. - registry.overrides = make(limits) - return registry, nil + return newLimitRegistry(defaultsData, nil) + } + + overridesData, err := loadOverrides(overrides) + if err != nil { + return nil, err + } + + return newLimitRegistry(defaultsData, overridesData) +} + +func newLimitRegistry(defaults LimitConfigs, overrides overridesYAML) (*limitRegistry, error) { + regDefaults, err := parseDefaultLimits(defaults) + if err != nil { + return nil, err } - registry.overrides, err = loadAndParseOverrideLimits(overrides) + regOverrides, err := parseOverrideLimits(overrides) if err != nil { return nil, err } - return registry, nil + return &limitRegistry{ + defaults: regDefaults, + overrides: regOverrides, + }, nil } // getLimit returns the limit for the specified by name and bucketKey, name is diff --git a/ratelimits/limit_test.go b/ratelimits/limit_test.go index 805a97e4cca..56cecc37a89 100644 --- a/ratelimits/limit_test.go +++ b/ratelimits/limit_test.go @@ -9,6 +9,32 @@ import ( "github.com/letsencrypt/boulder/test" ) +// loadAndParseDefaultLimits is a helper that calls both loadDefaults and +// parseDefaultLimits to handle a YAML file. +// +// TODO(#7901): Update the tests to test these functions individually. +func loadAndParseDefaultLimits(path string) (limits, error) { + fromFile, err := loadDefaults(path) + if err != nil { + return nil, err + } + + return parseDefaultLimits(fromFile) +} + +// loadAndParseOverrideLimits is a helper that calls both loadOverrides and +// parseOverrideLimits to handle a YAML file. +// +// TODO(#7901): Update the tests to test these functions individually. +func loadAndParseOverrideLimits(path string) (limits, error) { + fromFile, err := loadOverrides(path) + if err != nil { + return nil, err + } + + return parseOverrideLimits(fromFile) +} + func TestParseOverrideNameId(t *testing.T) { // 'enum:ipv4' // Valid IPv4 address. @@ -42,14 +68,14 @@ func TestParseOverrideNameId(t *testing.T) { } func TestValidateLimit(t *testing.T) { - err := validateLimit(&limit{Burst: 1, Count: 1, Period: config.Duration{Duration: time.Second}}) + err := validateLimit(&limit{burst: 1, count: 1, period: config.Duration{Duration: time.Second}}) test.AssertNotError(t, err, "valid limit") // All of the following are invalid. for _, l := range []*limit{ - {Burst: 0, Count: 1, Period: config.Duration{Duration: time.Second}}, - {Burst: 1, Count: 0, Period: config.Duration{Duration: time.Second}}, - {Burst: 1, Count: 1, Period: config.Duration{Duration: 0}}, + {burst: 0, count: 1, period: config.Duration{Duration: time.Second}}, + {burst: 1, count: 0, period: config.Duration{Duration: time.Second}}, + {burst: 1, count: 1, period: config.Duration{Duration: 0}}, } { err = validateLimit(l) test.AssertError(t, err, "limit should be invalid") @@ -61,29 +87,29 @@ func TestLoadAndParseOverrideLimits(t *testing.T) { l, err := loadAndParseOverrideLimits("testdata/working_override.yml") test.AssertNotError(t, err, "valid single override limit") expectKey := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "10.0.0.2") - test.AssertEquals(t, l[expectKey].Burst, int64(40)) - test.AssertEquals(t, l[expectKey].Count, int64(40)) - test.AssertEquals(t, l[expectKey].Period.Duration, time.Second) + test.AssertEquals(t, l[expectKey].burst, int64(40)) + test.AssertEquals(t, l[expectKey].count, int64(40)) + test.AssertEquals(t, l[expectKey].period.Duration, time.Second) // Load single valid override limit with a 'domain' Id. l, err = loadAndParseOverrideLimits("testdata/working_override_regid_domain.yml") test.AssertNotError(t, err, "valid single override limit with Id of regId:domain") expectKey = joinWithColon(CertificatesPerDomain.EnumString(), "example.com") - test.AssertEquals(t, l[expectKey].Burst, int64(40)) - test.AssertEquals(t, l[expectKey].Count, int64(40)) - test.AssertEquals(t, l[expectKey].Period.Duration, time.Second) + test.AssertEquals(t, l[expectKey].burst, int64(40)) + test.AssertEquals(t, l[expectKey].count, int64(40)) + test.AssertEquals(t, l[expectKey].period.Duration, time.Second) // Load multiple valid override limits with 'regId' Ids. l, err = loadAndParseOverrideLimits("testdata/working_overrides.yml") test.AssertNotError(t, err, "multiple valid override limits") expectKey1 := joinWithColon(NewRegistrationsPerIPAddress.EnumString(), "10.0.0.2") - test.AssertEquals(t, l[expectKey1].Burst, int64(40)) - test.AssertEquals(t, l[expectKey1].Count, int64(40)) - test.AssertEquals(t, l[expectKey1].Period.Duration, time.Second) + test.AssertEquals(t, l[expectKey1].burst, int64(40)) + test.AssertEquals(t, l[expectKey1].count, int64(40)) + test.AssertEquals(t, l[expectKey1].period.Duration, time.Second) expectKey2 := joinWithColon(NewRegistrationsPerIPv6Range.EnumString(), "2001:0db8:0000::/48") - test.AssertEquals(t, l[expectKey2].Burst, int64(50)) - test.AssertEquals(t, l[expectKey2].Count, int64(50)) - test.AssertEquals(t, l[expectKey2].Period.Duration, time.Second*2) + test.AssertEquals(t, l[expectKey2].burst, int64(50)) + test.AssertEquals(t, l[expectKey2].count, int64(50)) + test.AssertEquals(t, l[expectKey2].period.Duration, time.Second*2) // Load multiple valid override limits with 'fqdnSet' Ids, as follows: // - CertificatesPerFQDNSet:example.com @@ -97,15 +123,15 @@ func TestLoadAndParseOverrideLimits(t *testing.T) { test.AssertNotError(t, err, "valid fqdnSet with three domains should not fail") l, err = loadAndParseOverrideLimits("testdata/working_overrides_regid_fqdnset.yml") test.AssertNotError(t, err, "multiple valid override limits with 'fqdnSet' Ids") - test.AssertEquals(t, l[firstEntryKey].Burst, int64(40)) - test.AssertEquals(t, l[firstEntryKey].Count, int64(40)) - test.AssertEquals(t, l[firstEntryKey].Period.Duration, time.Second) - test.AssertEquals(t, l[secondEntryKey].Burst, int64(50)) - test.AssertEquals(t, l[secondEntryKey].Count, int64(50)) - test.AssertEquals(t, l[secondEntryKey].Period.Duration, time.Second*2) - test.AssertEquals(t, l[thirdEntryKey].Burst, int64(60)) - test.AssertEquals(t, l[thirdEntryKey].Count, int64(60)) - test.AssertEquals(t, l[thirdEntryKey].Period.Duration, time.Second*3) + test.AssertEquals(t, l[firstEntryKey].burst, int64(40)) + test.AssertEquals(t, l[firstEntryKey].count, int64(40)) + test.AssertEquals(t, l[firstEntryKey].period.Duration, time.Second) + test.AssertEquals(t, l[secondEntryKey].burst, int64(50)) + test.AssertEquals(t, l[secondEntryKey].count, int64(50)) + test.AssertEquals(t, l[secondEntryKey].period.Duration, time.Second*2) + test.AssertEquals(t, l[thirdEntryKey].burst, int64(60)) + test.AssertEquals(t, l[thirdEntryKey].count, int64(60)) + test.AssertEquals(t, l[thirdEntryKey].period.Duration, time.Second*3) // Path is empty string. _, err = loadAndParseOverrideLimits("") @@ -152,19 +178,19 @@ func TestLoadAndParseDefaultLimits(t *testing.T) { // Load a single valid default limit. l, err := loadAndParseDefaultLimits("testdata/working_default.yml") test.AssertNotError(t, err, "valid single default limit") - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Burst, int64(20)) - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Count, int64(20)) - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Period.Duration, time.Second) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].burst, int64(20)) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].count, int64(20)) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].period.Duration, time.Second) // Load multiple valid default limits. l, err = loadAndParseDefaultLimits("testdata/working_defaults.yml") test.AssertNotError(t, err, "multiple valid default limits") - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Burst, int64(20)) - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Count, int64(20)) - test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].Period.Duration, time.Second) - test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Burst, int64(30)) - test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Count, int64(30)) - test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].Period.Duration, time.Second*2) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].burst, int64(20)) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].count, int64(20)) + test.AssertEquals(t, l[NewRegistrationsPerIPAddress.EnumString()].period.Duration, time.Second) + test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].burst, int64(30)) + test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].count, int64(30)) + test.AssertEquals(t, l[NewRegistrationsPerIPv6Range.EnumString()].period.Duration, time.Second*2) // Path is empty string. _, err = loadAndParseDefaultLimits("") diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 0654787b6ec..ef119d1819a 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -117,8 +117,8 @@ func (d *Decision) Result(now time.Time) error { return berrors.RegistrationsPerIPAddressError( retryAfter, "too many new registrations (%d) from this IP address in the last %s, retry after %s", - d.transaction.limit.Burst, - d.transaction.limit.Period.Duration, + d.transaction.limit.burst, + d.transaction.limit.period.Duration, retryAfterTs, ) @@ -126,16 +126,16 @@ func (d *Decision) Result(now time.Time) error { return berrors.RegistrationsPerIPv6RangeError( retryAfter, "too many new registrations (%d) from this /48 subnet of IPv6 addresses in the last %s, retry after %s", - d.transaction.limit.Burst, - d.transaction.limit.Period.Duration, + d.transaction.limit.burst, + d.transaction.limit.period.Duration, retryAfterTs, ) case NewOrdersPerAccount: return berrors.NewOrdersPerAccountError( retryAfter, "too many new orders (%d) from this account in the last %s, retry after %s", - d.transaction.limit.Burst, - d.transaction.limit.Period.Duration, + d.transaction.limit.burst, + d.transaction.limit.period.Duration, retryAfterTs, ) @@ -149,9 +149,9 @@ func (d *Decision) Result(now time.Time) error { return berrors.FailedAuthorizationsPerDomainPerAccountError( retryAfter, "too many failed authorizations (%d) for %q in the last %s, retry after %s", - d.transaction.limit.Burst, + d.transaction.limit.burst, domain, - d.transaction.limit.Period.Duration, + d.transaction.limit.period.Duration, retryAfterTs, ) @@ -165,9 +165,9 @@ func (d *Decision) Result(now time.Time) error { return berrors.CertificatesPerDomainError( retryAfter, "too many certificates (%d) already issued for %q in the last %s, retry after %s", - d.transaction.limit.Burst, + d.transaction.limit.burst, domain, - d.transaction.limit.Period.Duration, + d.transaction.limit.period.Duration, retryAfterTs, ) @@ -175,8 +175,8 @@ func (d *Decision) Result(now time.Time) error { return berrors.CertificatesPerFQDNSetError( retryAfter, "too many certificates (%d) already issued for this exact set of domains in the last %s, retry after %s", - d.transaction.limit.Burst, - d.transaction.limit.Period.Duration, + d.transaction.limit.burst, + d.transaction.limit.period.Duration, retryAfterTs, ) @@ -285,7 +285,7 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision d := maybeSpend(l.clk, txn, storedTAT) if txn.limit.isOverride() { - utilization := float64(txn.limit.Burst-d.remaining) / float64(txn.limit.Burst) + utilization := float64(txn.limit.burst-d.remaining) / float64(txn.limit.burst) l.overrideUsageGauge.WithLabelValues(txn.limit.name.String(), txn.limit.overrideKey).Set(utilization) } diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index cf18fe271f5..902f4c13435 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -32,7 +32,7 @@ func newTestLimiter(t *testing.T, s Source, clk clock.FakeClock) *Limiter { // - 'NewRegistrationsPerIPAddress' burst: 20 count: 20 period: 1s // - 'NewRegistrationsPerIPAddress:10.0.0.2' burst: 40 count: 40 period: 1s func newTestTransactionBuilder(t *testing.T) *TransactionBuilder { - c, err := NewTransactionBuilder("testdata/working_default.yml", "testdata/working_override.yml") + c, err := NewTransactionBuilderFromFiles("testdata/working_default.yml", "testdata/working_override.yml") test.AssertNotError(t, err, "should not error") return c } @@ -484,8 +484,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: NewRegistrationsPerIPAddress, - Burst: 10, - Period: config.Duration{Duration: time.Hour}, + burst: 10, + period: config.Duration{Duration: time.Hour}, }, }, }, @@ -500,8 +500,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: NewRegistrationsPerIPv6Range, - Burst: 5, - Period: config.Duration{Duration: time.Hour}, + burst: 5, + period: config.Duration{Duration: time.Hour}, }, }, }, @@ -516,8 +516,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: NewOrdersPerAccount, - Burst: 2, - Period: config.Duration{Duration: time.Hour}, + burst: 2, + period: config.Duration{Duration: time.Hour}, }, }, }, @@ -532,8 +532,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: FailedAuthorizationsPerDomainPerAccount, - Burst: 7, - Period: config.Duration{Duration: time.Hour}, + burst: 7, + period: config.Duration{Duration: time.Hour}, }, bucketKey: "4:12345:example.com", }, @@ -549,8 +549,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: CertificatesPerDomain, - Burst: 3, - Period: config.Duration{Duration: time.Hour}, + burst: 3, + period: config.Duration{Duration: time.Hour}, }, bucketKey: "5:example.org", }, @@ -566,8 +566,8 @@ func TestRateLimitError(t *testing.T) { transaction: Transaction{ limit: &limit{ name: CertificatesPerDomainPerAccount, - Burst: 3, - Period: config.Duration{Duration: time.Hour}, + burst: 3, + period: config.Duration{Duration: time.Hour}, }, bucketKey: "6:12345678:example.net", }, diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index fc5df72c547..b5fd1653269 100644 --- a/ratelimits/transaction.go +++ b/ratelimits/transaction.go @@ -129,7 +129,7 @@ func validateTransaction(txn Transaction) (Transaction, error) { if txn.cost < 0 { return Transaction{}, ErrInvalidCost } - if txn.limit.Burst == 0 { + if txn.limit.burst == 0 { // This should never happen. If the limit was loaded from a file, // Burst was validated then. If this is a zero-valued Transaction // (that is, an allow-only transaction), then validateTransaction @@ -137,7 +137,7 @@ func validateTransaction(txn Transaction) (Transaction, error) { // valid. return Transaction{}, fmt.Errorf("invalid limit, burst must be > 0") } - if txn.cost > txn.limit.Burst { + if txn.cost > txn.limit.burst { return Transaction{}, ErrInvalidCostOverLimit } return txn, nil @@ -183,12 +183,23 @@ type TransactionBuilder struct { *limitRegistry } +// NewTransactionBuilderFromFiles returns a new *TransactionBuilder. The +// provided defaults and overrides paths are expected to be paths to YAML files +// that contain the default and override limits, respectively. Overrides is +// optional, defaults is required. +func NewTransactionBuilderFromFiles(defaults, overrides string) (*TransactionBuilder, error) { + registry, err := newLimitRegistryFromFiles(defaults, overrides) + if err != nil { + return nil, err + } + return &TransactionBuilder{registry}, nil +} + // NewTransactionBuilder returns a new *TransactionBuilder. The provided -// defaults and overrides paths are expected to be paths to YAML files that -// contain the default and override limits, respectively. Overrides is optional, -// defaults is required. -func NewTransactionBuilder(defaults, overrides string) (*TransactionBuilder, error) { - registry, err := newLimitRegistry(defaults, overrides) +// defaults map is expected to contain default limit data. Overrides are not +// supported. Defaults is required. +func NewTransactionBuilder(defaults LimitConfigs) (*TransactionBuilder, error) { + registry, err := newLimitRegistry(defaults, nil) if err != nil { return nil, err } diff --git a/ratelimits/transaction_test.go b/ratelimits/transaction_test.go index f8003e2f404..8cf0b798a1e 100644 --- a/ratelimits/transaction_test.go +++ b/ratelimits/transaction_test.go @@ -5,17 +5,19 @@ import ( "net" "sort" "testing" + "time" + "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/core" "github.com/letsencrypt/boulder/test" ) -func TestNewTransactionBuilder_WithBadLimitsPath(t *testing.T) { +func TestNewTransactionBuilderFromFiles_WithBadLimitsPath(t *testing.T) { t.Parallel() - _, err := NewTransactionBuilder("testdata/does-not-exist.yml", "") + _, err := NewTransactionBuilderFromFiles("testdata/does-not-exist.yml", "") test.AssertError(t, err, "should error") - _, err = NewTransactionBuilder("testdata/defaults.yml", "testdata/does-not-exist.yml") + _, err = NewTransactionBuilderFromFiles("testdata/defaults.yml", "testdata/does-not-exist.yml") test.AssertError(t, err, "should error") } @@ -29,7 +31,7 @@ func sortTransactions(txns []Transaction) []Transaction { func TestNewRegistrationsPerIPAddressTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "creating TransactionBuilder") // A check-and-spend transaction for the global limit. @@ -42,7 +44,7 @@ func TestNewRegistrationsPerIPAddressTransactions(t *testing.T) { func TestNewRegistrationsPerIPv6AddressTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "creating TransactionBuilder") // A check-and-spend transaction for the global limit. @@ -55,7 +57,7 @@ func TestNewRegistrationsPerIPv6AddressTransactions(t *testing.T) { func TestNewOrdersPerAccountTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "creating TransactionBuilder") // A check-and-spend transaction for the global limit. @@ -68,7 +70,7 @@ func TestNewOrdersPerAccountTransactions(t *testing.T) { func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") + tb, err := NewTransactionBuilderFromFiles("../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. @@ -105,7 +107,7 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(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") + tb, err := NewTransactionBuilderFromFiles("../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. @@ -119,7 +121,7 @@ func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testin func TestCertificatesPerDomainTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "creating TransactionBuilder") // One check-only transaction for the global limit. @@ -140,7 +142,7 @@ func TestCertificatesPerDomainTransactions(t *testing.T) { func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "testdata/working_override_13371338.yml") test.AssertNotError(t, err, "creating TransactionBuilder") // We only expect a single check-only transaction for the per-account limit @@ -191,7 +193,7 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { func TestCertificatesPerFQDNSetTransactions(t *testing.T) { t.Parallel() - tb, err := NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + tb, err := NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "creating TransactionBuilder") // A single check-only transaction for the global limit. @@ -202,3 +204,25 @@ func TestCertificatesPerFQDNSetTransactions(t *testing.T) { test.Assert(t, txn.checkOnly(), "should be check-only") test.Assert(t, !txn.limit.isOverride(), "should not be an override") } + +func TestNewTransactionBuilder(t *testing.T) { + t.Parallel() + + expectedBurst := int64(10000) + expectedCount := int64(10000) + expectedPeriod := config.Duration{Duration: time.Hour * 168} + + tb, err := NewTransactionBuilder(LimitConfigs{ + NewRegistrationsPerIPAddress.String(): &LimitConfig{ + Burst: expectedBurst, + Count: expectedCount, + Period: expectedPeriod}, + }) + test.AssertNotError(t, err, "creating TransactionBuilder") + + newRegDefault, ok := tb.limitRegistry.defaults[NewRegistrationsPerIPAddress.EnumString()] + test.Assert(t, ok, "NewRegistrationsPerIPAddress was not populated in registry") + test.AssertEquals(t, newRegDefault.burst, expectedBurst) + test.AssertEquals(t, newRegDefault.count, expectedCount) + test.AssertEquals(t, newRegDefault.period, expectedPeriod) +} diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 19063b7cd2f..75768ea1e6b 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -2406,14 +2406,17 @@ func (wfe *WebFrontEndImpl) NewOrder( return } - refundLimits, err := wfe.checkNewOrderLimits(ctx, acct.ID, names, isRenewal) - if err != nil && features.Get().UseKvLimitsForNewOrder { - if errors.Is(err, berrors.RateLimit) { - wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err) - return - } else { - wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While checking rate limits"), err) - return + refundLimits := func() {} + if !isARIRenewal { + refundLimits, err = wfe.checkNewOrderLimits(ctx, acct.ID, names, isRenewal || isARIRenewal) + if err != nil && features.Get().UseKvLimitsForNewOrder { + if errors.Is(err, berrors.RateLimit) { + wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err) + return + } else { + wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While checking rate limits"), err) + return + } } } diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index c8229f58a7f..a49a31c915a 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -36,6 +36,7 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/letsencrypt/boulder/cmd" + "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" berrors "github.com/letsencrypt/boulder/errors" @@ -410,7 +411,7 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) { // Setup rate limiting. limiter, err := ratelimits.NewLimiter(fc, ratelimits.NewInmemSource(), stats) test.AssertNotError(t, err, "making limiter") - txnBuilder, err := ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + txnBuilder, err := ratelimits.NewTransactionBuilderFromFiles("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "making transaction composer") unpauseSigner, err := unpause.NewJWTSigner(cmd.HMACKeyConfig{KeyFile: "../test/secrets/sfe_unpause_key"}) @@ -4224,20 +4225,26 @@ func Test_sendErrorInternalServerError(t *testing.T) { test.AssertEquals(t, testResponse.Header().Get("Retry-After"), "60") } -type mockSA struct { +// mockSAForARI provides a mock SA with the methods required for an issuance and +// a renewal with the ARI `Replaces` field. +type mockSAForARI struct { sapb.StorageAuthorityReadOnlyClient cert *corepb.Certificate } +func (sa *mockSAForARI) FQDNSetExists(ctx context.Context, in *sapb.FQDNSetExistsRequest, opts ...grpc.CallOption) (*sapb.Exists, error) { + return &sapb.Exists{Exists: false}, nil +} + // GetCertificate returns the inner certificate if it matches the given serial. -func (sa *mockSA) GetCertificate(ctx context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*corepb.Certificate, error) { +func (sa *mockSAForARI) GetCertificate(ctx context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*corepb.Certificate, error) { if req.Serial == sa.cert.Serial { return sa.cert, nil } return nil, berrors.NotFoundError("certificate with serial %q not found", req.Serial) } -func (sa *mockSA) ReplacementOrderExists(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*sapb.Exists, error) { +func (sa *mockSAForARI) ReplacementOrderExists(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*sapb.Exists, error) { if in.Serial == sa.cert.Serial { return &sapb.Exists{Exists: false}, nil @@ -4245,11 +4252,11 @@ func (sa *mockSA) ReplacementOrderExists(ctx context.Context, in *sapb.Serial, o return &sapb.Exists{Exists: true}, nil } -func (sa *mockSA) IncidentsForSerial(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*sapb.Incidents, error) { +func (sa *mockSAForARI) IncidentsForSerial(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*sapb.Incidents, error) { return &sapb.Incidents{}, nil } -func (sa *mockSA) GetCertificateStatus(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*corepb.CertificateStatus, error) { +func (sa *mockSAForARI) GetCertificateStatus(ctx context.Context, in *sapb.Serial, opts ...grpc.CallOption) (*corepb.CertificateStatus, error) { return &corepb.CertificateStatus{Serial: in.Serial, Status: string(core.OCSPStatusGood)}, nil } @@ -4267,7 +4274,7 @@ func TestOrderMatchesReplacement(t *testing.T) { mockDer, err := x509.CreateCertificate(rand.Reader, &rawCert, &rawCert, &testKey.PublicKey, testKey) test.AssertNotError(t, err, "failed to create test certificate") - wfe.sa = &mockSA{ + wfe.sa = &mockSAForARI{ cert: &corepb.Certificate{ RegistrationID: 1, Serial: expectSerial.String(), @@ -4414,9 +4421,10 @@ func TestCountNewOrderWithReplaces(t *testing.T) { wfe, _, signer := setupWFE(t) expectExpiry := time.Now().AddDate(0, 0, 1) - var expectAKID []byte + // Pick a random issuer to "issue" expectCert. + var issuer *issuance.Certificate for _, v := range wfe.issuerCertificates { - expectAKID = v.SubjectKeyId + issuer = v break } testKey, _ := rsa.GenerateKey(rand.Reader, 1024) @@ -4425,7 +4433,7 @@ func TestCountNewOrderWithReplaces(t *testing.T) { NotAfter: expectExpiry, DNSNames: []string{"example.com"}, SerialNumber: expectSerial, - AuthorityKeyId: expectAKID, + AuthorityKeyId: issuer.SubjectKeyId, } expectCertId, err := makeARICertID(expectCert) test.AssertNotError(t, err, "failed to create test cert id") @@ -4433,7 +4441,7 @@ func TestCountNewOrderWithReplaces(t *testing.T) { test.AssertNotError(t, err, "failed to create test certificate") // MockSA that returns the certificate with the expected serial. - wfe.sa = &mockSA{ + wfe.sa = &mockSAForARI{ cert: &corepb.Certificate{ RegistrationID: 1, Serial: core.SerialToString(expectSerial), @@ -4456,3 +4464,83 @@ func TestCountNewOrderWithReplaces(t *testing.T) { test.AssertEquals(t, responseWriter.Code, http.StatusCreated) test.AssertMetricWithLabelsEquals(t, wfe.stats.ariReplacementOrders, prometheus.Labels{"isReplacement": "true", "limitsExempt": "true"}, 1) } + +func TestNewOrderRateLimits(t *testing.T) { + wfe, fc, signer := setupWFE(t) + + features.Set(features.Config{UseKvLimitsForNewOrder: true}) + defer features.Reset() + + // Set the default ratelimits to only allow one new order per account per 24 + // hours. + txnBuilder, err := ratelimits.NewTransactionBuilder(ratelimits.LimitConfigs{ + ratelimits.NewOrdersPerAccount.String(): &ratelimits.LimitConfig{ + Burst: 1, + Count: 1, + Period: config.Duration{Duration: time.Hour * 24}}, + }) + test.AssertNotError(t, err, "making transaction composer") + wfe.txnBuilder = txnBuilder + + // Pick a random issuer to "issue" extantCert. + var issuer *issuance.Certificate + for _, v := range wfe.issuerCertificates { + issuer = v + break + } + testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + test.AssertNotError(t, err, "failed to create test key") + extantCert := &x509.Certificate{ + NotBefore: fc.Now(), + NotAfter: fc.Now().AddDate(0, 0, 90), + DNSNames: []string{"example.com"}, + SerialNumber: big.NewInt(1337), + AuthorityKeyId: issuer.SubjectKeyId, + } + extantCertId, err := makeARICertID(extantCert) + test.AssertNotError(t, err, "failed to create test cert id") + extantDer, err := x509.CreateCertificate(rand.Reader, extantCert, extantCert, &testKey.PublicKey, testKey) + test.AssertNotError(t, err, "failed to create test certificate") + + // Mock SA that returns the certificate with the expected serial. + wfe.sa = &mockSAForARI{ + cert: &corepb.Certificate{ + RegistrationID: 1, + Serial: core.SerialToString(extantCert.SerialNumber), + Der: extantDer, + Issued: timestamppb.New(extantCert.NotBefore), + Expires: timestamppb.New(extantCert.NotAfter), + }, + } + + // Set the fake clock forward to 1s past the suggested renewal window start + // time. + renewalWindowStart := core.RenewalInfoSimple(extantCert.NotBefore, extantCert.NotAfter).SuggestedWindow.Start + fc.Set(renewalWindowStart.Add(time.Second)) + + mux := wfe.Handler(metrics.NoopRegisterer) + + // Request the certificate for the first time. Because we mocked together + // the certificate, it will have been issued 60 days ago. + r := signAndPost(signer, newOrderPath, "http://localhost"+newOrderPath, + `{"Identifiers": [{"type": "dns", "value": "example.com"}]}`) + responseWriter := httptest.NewRecorder() + mux.ServeHTTP(responseWriter, r) + test.AssertEquals(t, responseWriter.Code, http.StatusCreated) + + // Request another, identical certificate. This should fail for violating + // the NewOrdersPerAccount rate limit. + r = signAndPost(signer, newOrderPath, "http://localhost"+newOrderPath, + `{"Identifiers": [{"type": "dns", "value": "example.com"}]}`) + responseWriter = httptest.NewRecorder() + mux.ServeHTTP(responseWriter, r) + test.AssertEquals(t, responseWriter.Code, http.StatusTooManyRequests) + + // Make a request with the "Replaces" field, which should satisfy ARI checks + // and therefore bypass the rate limit. + r = signAndPost(signer, newOrderPath, "http://localhost"+newOrderPath, + fmt.Sprintf(`{"Identifiers": [{"type": "dns", "value": "example.com"}], "Replaces": %q}`, extantCertId)) + responseWriter = httptest.NewRecorder() + mux.ServeHTTP(responseWriter, r) + test.AssertEquals(t, responseWriter.Code, http.StatusCreated) +}