WIP

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.

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)
+			}
+		})
+	}
+}

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)

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

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")
 	}
 }