diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 50b7d7ca60e..429dffbbc50 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -5,9 +5,13 @@ import ( "errors" "fmt" "math" + "math/rand/v2" "slices" + "strings" "time" + berrors "github.com/letsencrypt/boulder/errors" + "github.com/jmhodges/clock" "github.com/prometheus/client_golang/prometheus" ) @@ -83,6 +87,96 @@ type Decision struct { // theoretical arrival time (TAT) of next request. It must be no more than // (burst * (period / count)) in the future at any single point in time. newTAT time.Time + + // transaction is the Transaction that resulted in this Decision. It is + // included for the production of verbose Subscriber-facing errors. It is + // set by the Limiter before returning the Decision. + transaction Transaction +} + +// RateLimitError returns a verbose Subscriber-facing error for denied +// *Decisions. If the *Decision is allowed, it returns nil. +func (d *Decision) RateLimitError(clk clock.Clock) error { + if d.Allowed { + return nil + } + + // Add 0-3% jitter to the RetryIn duration to prevent thundering herd. + jitter := time.Duration(float64(d.RetryIn) * 0.03 * rand.Float64()) + retryAfter := d.RetryIn + jitter + retryAfterTs := clk.Now().Add(retryAfter).Format(time.RFC3339) + + switch d.transaction.limit.name { + case NewRegistrationsPerIPAddress: + return berrors.RegistrationsPerIPError( + 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, + retryAfterTs, + ) + + case NewRegistrationsPerIPv6Range: + return berrors.RateLimitError( + retryAfter, + "too many new registrations (%d) from this /48 block of IPv6 addresses in the last %s, retry after %s", + d.transaction.limit.Burst, + d.transaction.limit.Period.Duration, + retryAfterTs, + ) + case NewOrdersPerAccount: + return berrors.RateLimitError( + 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, + retryAfterTs, + ) + + case FailedAuthorizationsPerDomainPerAccount: + // Uses bucket key 'enum:regId:domain'. + idx := strings.LastIndex(d.transaction.bucketKey, ":") + if idx == -1 { + return berrors.InternalServerError("empty bucket key while generating error") + } + domain := d.transaction.bucketKey[idx+1:] + return berrors.FailedValidationError( + retryAfter, + "too many failed authorizations (%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, ":") + if idx == -1 { + return berrors.InternalServerError("empty bucket key while generating error") + } + domain := d.transaction.bucketKey[idx+1:] + return berrors.RateLimitError( + retryAfter, + "too many certificates (%d) already issued for %q in the last %s, retry after %s", + d.transaction.limit.Burst, + domain, + d.transaction.limit.Period.Duration, + retryAfterTs, + ) + + case CertificatesPerFQDNSet: + return berrors.DuplicateCertificateError( + 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, + retryAfterTs, + ) + + default: + return berrors.InternalServerError("cannot generate error for unknown rate limit") + } } // Check DOES NOT deduct the cost of the request from the provided bucket's @@ -105,9 +199,13 @@ func (l *Limiter) Check(ctx context.Context, txn Transaction) (*Decision, error) // First request from this client. No need to initialize the bucket // because this is a check, not a spend. A TAT of "now" is equivalent to // a full bucket. - return maybeSpend(l.clk, txn.limit, l.clk.Now(), txn.cost), nil + d := maybeSpend(l.clk, txn.limit, l.clk.Now(), txn.cost) + d.transaction = txn + return d, nil } - return maybeSpend(l.clk, txn.limit, tat, txn.cost), nil + d := maybeSpend(l.clk, txn.limit, tat, txn.cost) + d.transaction = txn + return d, nil } // Spend attempts to deduct the cost from the provided bucket's capacity. The @@ -153,10 +251,11 @@ func newBatchDecision() *batchDecision { func (d *batchDecision) merge(in *Decision) { d.Allowed = d.Allowed && in.Allowed d.Remaining = min(d.Remaining, in.Remaining) - d.RetryIn = max(d.RetryIn, in.RetryIn) d.ResetIn = max(d.ResetIn, in.ResetIn) if in.newTAT.After(d.newTAT) { d.newTAT = in.newTAT + d.RetryIn = in.RetryIn + d.transaction = in.transaction } } @@ -201,6 +300,7 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision } d := maybeSpend(l.clk, txn.limit, tat, txn.cost) + d.transaction = txn if txn.limit.isOverride() { utilization := float64(txn.limit.Burst-d.Remaining) / float64(txn.limit.Burst) @@ -297,6 +397,7 @@ func (l *Limiter) BatchRefund(ctx context.Context, txns []Transaction) (*Decisio cost = txn.cost } d := maybeRefund(l.clk, txn.limit, tat, cost) + d.transaction = txn batchDecision.merge(d) if d.Allowed && tat != d.newTAT { // New bucket state should be persisted. diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index efec4543224..daa3d566408 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -10,6 +10,8 @@ import ( "github.com/jmhodges/clock" "github.com/prometheus/client_golang/prometheus" + "github.com/letsencrypt/boulder/config" + berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/metrics" "github.com/letsencrypt/boulder/test" ) @@ -457,3 +459,133 @@ func TestLimiter_RefundAndReset(t *testing.T) { }) } } + +func TestRateLimitError(t *testing.T) { + t.Parallel() + clk := clock.NewFake() + + testCases := []struct { + name string + decision *Decision + expectedErr string + expectedErrType berrors.ErrorType + }{ + { + name: "Allowed decision", + decision: &Decision{ + Allowed: true, + }, + }, + { + name: "RegistrationsPerIP limit reached", + decision: &Decision{ + Allowed: false, + RetryIn: 5 * time.Second, + transaction: Transaction{ + limit: limit{ + name: NewRegistrationsPerIPAddress, + Burst: 10, + Period: config.Duration{Duration: time.Hour}, + }, + }, + }, + expectedErr: "too many new registrations (10) from this IP address in the last 1h0m0s, retry after 1970-01-01T00:00:05Z", + expectedErrType: berrors.RateLimit, + }, + { + name: "RegistrationsPerIPv6Range limit reached", + decision: &Decision{ + Allowed: false, + RetryIn: 10 * time.Second, + transaction: Transaction{ + limit: limit{ + name: NewRegistrationsPerIPv6Range, + Burst: 5, + Period: config.Duration{Duration: time.Hour}, + }, + }, + }, + expectedErr: "too many new registrations (5) from this /48 block of IPv6 addresses in the last 1h0m0s, retry after 1970-01-01T00:00:10Z", + expectedErrType: berrors.RateLimit, + }, + { + name: "FailedAuthorizationsPerDomainPerAccount limit reached", + decision: &Decision{ + Allowed: false, + RetryIn: 15 * time.Second, + transaction: Transaction{ + limit: limit{ + name: FailedAuthorizationsPerDomainPerAccount, + 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-01T00:00:15Z", + expectedErrType: berrors.RateLimit, + }, + { + name: "CertificatesPerDomain limit reached", + decision: &Decision{ + Allowed: false, + RetryIn: 20 * time.Second, + transaction: Transaction{ + limit: limit{ + name: CertificatesPerDomain, + Burst: 3, + Period: config.Duration{Duration: time.Hour}, + }, + bucketKey: "5:example.org", + }, + }, + expectedErr: "too many certificates (3) already issued for \"example.org\" in the last 1h0m0s, retry after 1970-01-01T00:00:20Z", + expectedErrType: berrors.RateLimit, + }, + { + name: "CertificatesPerDomainPerAccount limit reached", + decision: &Decision{ + Allowed: false, + RetryIn: 20 * time.Second, + transaction: Transaction{ + limit: limit{ + name: CertificatesPerDomainPerAccount, + Burst: 3, + Period: config.Duration{Duration: time.Hour}, + }, + bucketKey: "6:12345678:example.net", + }, + }, + expectedErr: "too many certificates (3) already issued for \"example.net\" in the last 1h0m0s, retry after 1970-01-01T00:00:20Z", + expectedErrType: berrors.RateLimit, + }, + { + name: "Unknown rate limit name", + decision: &Decision{ + Allowed: false, + RetryIn: 30 * time.Second, + transaction: Transaction{ + limit: limit{ + name: 9999999, + }, + }, + }, + expectedErr: "cannot generate error for unknown rate limit", + expectedErrType: berrors.InternalServer, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := tc.decision.RateLimitError(clk) + if tc.expectedErr == "" { + test.AssertNotError(t, err, "expected no error") + } else { + test.AssertError(t, err, "expected an error") + test.AssertContains(t, err.Error(), tc.expectedErr) + test.AssertErrorIs(t, err, tc.expectedErrType) + } + }) + } +} diff --git a/ratelimits/names.go b/ratelimits/names.go index fdfd8e81e06..c70f3953678 100644 --- a/ratelimits/names.go +++ b/ratelimits/names.go @@ -13,10 +13,11 @@ import ( // limit names as strings and to provide a type-safe way to refer to rate // limits. // -// IMPORTANT: If you add a new limit Name, you MUST add: -// - it to the nameToString mapping, -// - an entry for it in the validateIdForName(), and -// - provide the appropriate constructors in bucket.go. +// IMPORTANT: If you add or remove a limit Name, you MUST update: +// - the string representation of the Name in nameToString, +// - the validators for that name in validateIdForName(), +// - the transaction constructors for that name in bucket.go, and +// - the Subscriber facing error message in ErrForDecision(). type Name int const ( @@ -77,6 +78,18 @@ const ( CertificatesPerFQDNSet ) +// 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", +} + // isValid returns true if the Name is a valid rate limit name. func (n Name) isValid() bool { return n > Unknown && n < Name(len(nameToString)) @@ -99,18 +112,6 @@ func (n Name) EnumString() string { return strconv.Itoa(int(n)) } -// 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", -} - // validIPAddress validates that the provided string is a valid IP address. func validIPAddress(id string) error { ip := net.ParseIP(id) diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index 0192c4bb340..c24e854c2b3 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -19,6 +19,6 @@ NewOrdersPerAccount: burst: 1500 period: 3h CertificatesPerFQDNSet: - count: 6 - burst: 6 - period: 168h + count: 2 + burst: 2 + period: 3h diff --git a/test/integration/ratelimit_test.go b/test/integration/ratelimit_test.go index 88600af2a44..71e88a68110 100644 --- a/test/integration/ratelimit_test.go +++ b/test/integration/ratelimit_test.go @@ -11,6 +11,7 @@ import ( "github.com/jmhodges/clock" "github.com/letsencrypt/boulder/cmd" + berrors "github.com/letsencrypt/boulder/errors" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/metrics" "github.com/letsencrypt/boulder/ratelimits" @@ -52,7 +53,7 @@ func TestDuplicateFQDNRateLimit(t *testing.T) { PasswordFile: "test/secrets/ratelimits_redis_password", } - fc := clock.NewFake() + fc := clock.New() stats := metrics.NoopRegisterer log := blog.NewMock() ring, err := bredis.NewRingFromConfig(rc, stats, log) @@ -67,8 +68,11 @@ func TestDuplicateFQDNRateLimit(t *testing.T) { // Check that the CertificatesPerFQDNSet limit is reached. txns, err := txnBuilder.NewOrderLimitTransactions(1, []string{domain}, 100, false) test.AssertNotError(t, err, "making transaction") - result, err := limiter.BatchSpend(context.Background(), txns) + decision, err := limiter.BatchSpend(context.Background(), txns) test.AssertNotError(t, err, "checking transaction") - test.Assert(t, !result.Allowed, "should not be allowed") + test.Assert(t, !decision.Allowed, "should not be allowed") + err = decision.RateLimitError(fc) + test.AssertErrorIs(t, err, berrors.RateLimit) + test.AssertContains(t, err.Error(), "too many certificates (2) already issued for this exact set of domains in the last 3h0m0s") } }