ratelimits: Add a feature-flag which makes key-value implementation a…

…uthoritative

features/features.go

@@ -104,6 +104,16 @@ type Config struct {
 	// returned to the Subscriber indicating that the order cannot be processed
 	// until the paused identifiers are unpaused and the order is resubmitted.
 	CheckIdentifiersPaused bool
+
+	// NewRateLimitsNewOrderAuthoritative when enabled, causes the key-value
+	// rate limiter to be the authoritative source of rate limiting information
+	// for new-order callers and disables the legacy rate limiting checks.
+	NewRateLimitsNewOrderAuthoritative bool
+
+	// NewRateLimitsNewAccountAuthoritative when enabled, causes the key-value
+	// rate limiter to be the authoritative source of rate limiting information
+	// for new-account callers and disables the legacy rate limiting checks.
+	NewRateLimitsNewAccountAuthoritative bool
 }
 
 var fMu = new(sync.RWMutex)

ra/ra.go

@@ -491,9 +491,12 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, reques
 	if err != nil {
 		return nil, berrors.InternalServerError("failed to unmarshal ip address: %s", err.Error())
 	}
-	err = ra.checkRegistrationLimits(ctx, ipAddr)
-	if err != nil {
-		return nil, err
+
+	if !features.Get().NewRateLimitsNewAccountAuthoritative {
+		err = ra.checkRegistrationLimits(ctx, ipAddr)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// Check that contacts conform to our expectations.
@@ -1816,6 +1819,9 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI
 	return err
 }
 
+// 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) {
 	if ra.limiter == nil || ra.txnBuilder == nil {
 		// Limiter is disabled.
@@ -1957,13 +1963,7 @@ func (ra *RegistrationAuthorityImpl) PerformValidation(
 		if prob != nil {
 			challenge.Status = core.StatusInvalid
 			challenge.Error = prob
-
-			// TODO(#5545): Spending can be async until key-value rate limits
-			// are authoritative. This saves us from adding latency to each
-			// request. Goroutines spun out below will respect a context
-			// deadline set by the ratelimits package and cannot be prematurely
-			// canceled by the requester.
-			go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier.Value)
+			ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier.Value)
 		} else {
 			challenge.Status = core.StatusValid
 		}
@@ -2535,7 +2535,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 	}
 
 	// Renewal orders, indicated by ARI, are exempt from NewOrder rate limits.
-	if !req.IsARIRenewal {
+	if !req.IsARIRenewal && !features.Get().NewRateLimitsNewOrderAuthoritative {
 		// Check if there is rate limit space for issuing a certificate.
 		err = ra.checkNewOrderLimits(ctx, newOrder.DnsNames, newOrder.RegistrationID, req.IsRenewal)
 		if err != nil {
@@ -2614,7 +2614,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 	}
 
 	// Renewal orders, indicated by ARI, are exempt from NewOrder rate limits.
-	if len(missingAuthzIdents) > 0 && !req.IsARIRenewal {
+	if len(missingAuthzIdents) > 0 && !req.IsARIRenewal && !features.Get().NewRateLimitsNewOrderAuthoritative {
 		pendingAuthzLimits := ra.rlPolicies.PendingAuthorizationsPerAccount()
 		if pendingAuthzLimits.Enabled() {
 			// The order isn't fully authorized we need to check that the client

ratelimits/limiter.go

@@ -105,8 +105,6 @@ func (d *Decision) Result(now time.Time) error {
 		return nil
 	}
 
-	fmt.Printf("\n\n%#v\n\n", d.transaction)
-
 	// 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

test/config-next/ra.json

@@ -106,7 +106,9 @@
 		},
 		"features": {
 			"AsyncFinalize": true,
-			"CheckRenewalExemptionAtWFE": true
+			"CheckRenewalExemptionAtWFE": true,
+			"NewRateLimitsNewOrderAuthoritative": true,
+			"NewRateLimitsNewAccountAuthoritative": true
 		},
 		"ctLogs": {
 			"stagger": "500ms",

test/config-next/wfe2-ratelimit-overrides.yml

@@ -5,6 +5,10 @@
     ids:
       - id: 127.0.0.1
         comment: localhost
+      - id: 10.77.77.77
+        comment: test
+      - id: 10.88.88.88
+        comment: test
 - CertificatesPerDomain:
     burst: 1
     count: 1

test/config-next/wfe2.json

@@ -130,7 +130,9 @@
 			"ServeRenewalInfo": true,
 			"TrackReplacementCertificatesARI": true,
 			"CheckRenewalExemptionAtWFE": true,
-			"CheckIdentifiersPaused": true
+			"CheckIdentifiersPaused": true,
+			"NewRateLimitsNewOrderAuthoritative": true,
+			"NewRateLimitsNewAccountAuthoritative": true
 		},
 		"certProfiles": {
 			"legacy": "The normal profile you know and love",

test/integration/ratelimit_test.go

@@ -3,19 +3,10 @@
 package integration
 
 import (
-	"context"
 	"os"
 	"strings"
 	"testing"
 
-	"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"
-	bredis "github.com/letsencrypt/boulder/redis"
 	"github.com/letsencrypt/boulder/test"
 )
 
@@ -33,45 +24,10 @@ func TestDuplicateFQDNRateLimit(t *testing.T) {
 	test.AssertError(t, err, "Somehow managed to issue third certificate")
 
 	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		// Setup rate limiting.
-		rc := bredis.Config{
-			Username: "unittest-rw",
-			TLS: cmd.TLSConfig{
-				CACertFile: "test/certs/ipki/minica.pem",
-				CertFile:   "test/certs/ipki/localhost/cert.pem",
-				KeyFile:    "test/certs/ipki/localhost/key.pem",
-			},
-			Lookups: []cmd.ServiceDomain{
-				{
-					Service: "redisratelimits",
-					Domain:  "service.consul",
-				},
-			},
-			LookupDNSAuthority: "consul.service.consul",
-		}
-		rc.PasswordConfig = cmd.PasswordConfig{
-			PasswordFile: "test/secrets/ratelimits_redis_password",
-		}
-
-		fc := clock.New()
-		stats := metrics.NoopRegisterer
-		log := blog.NewMock()
-		ring, err := bredis.NewRingFromConfig(rc, stats, log)
-		test.AssertNotError(t, err, "making redis ring client")
-		source := ratelimits.NewRedisSource(ring.Ring, fc, stats)
-		test.AssertNotNil(t, source, "source should not be nil")
-		limiter, err := ratelimits.NewLimiter(fc, source, stats)
-		test.AssertNotError(t, err, "making limiter")
-		txnBuilder, err := ratelimits.NewTransactionBuilder("test/config-next/wfe2-ratelimit-defaults.yml", "")
-		test.AssertNotError(t, err, "making transaction composer")
-
-		// Check that the CertificatesPerFQDNSet limit is reached.
-		txns, err := txnBuilder.NewOrderLimitTransactions(1, []string{domain}, 100, false)
-		test.AssertNotError(t, err, "making transaction")
-		decision, err := limiter.BatchSpend(context.Background(), txns)
-		test.AssertNotError(t, err, "checking transaction")
-		err = decision.Result(fc.Now())
-		test.AssertErrorIs(t, err, berrors.RateLimit)
+		// Error should be served from key-value rate limits implementation.
 		test.AssertContains(t, err.Error(), "too many certificates (2) already issued for this exact set of domains in the last 3h0m0s")
+	} else {
+		// Error should be served from legacy rate limits implementation.
+		test.AssertContains(t, err.Error(), "too many certificates (2) already issued for this exact set of domains in the last 3 hours")
 	}
 }

wfe2/wfe.go

@@ -642,36 +642,34 @@ func link(url, relation string) string {
 // function is returned that can be called to refund the quota if the account
 // creation fails, the func will be nil if any error was encountered during the
 // check.
-//
-// TODO(#5545): For now we're simply exercising the new rate limiter codepath.
-// This should eventually return a berrors.RateLimit error containing the retry
-// after duration among other information available in the ratelimits.Decision.
-func (wfe *WebFrontEndImpl) checkNewAccountLimits(ctx context.Context, ip net.IP) func() {
+func (wfe *WebFrontEndImpl) checkNewAccountLimits(ctx context.Context, ip net.IP) (func() error, error) {
 	if wfe.limiter == nil && wfe.txnBuilder == nil {
 		// Key-value rate limiting is disabled.
-		return nil
+		return nil, nil
 	}
 
 	txns, err := wfe.txnBuilder.NewAccountLimitTransactions(ip)
 	if err != nil {
-		// TODO(#5545): Once key-value rate limits are authoritative this
-		// log line should be removed in favor of returning the error.
-		wfe.log.Infof("building new account limit transactions: %v", err)
-		return nil
+		return nil, fmt.Errorf("building new account limit transactions: %w", err)
 	}
 
-	_, err = wfe.limiter.BatchSpend(ctx, txns)
+	d, err := wfe.limiter.BatchSpend(ctx, txns)
 	if err != nil {
-		wfe.log.Errf("checking newAccount limits: %s", err)
-		return nil
+		return nil, fmt.Errorf("spending new account limits: %w", err)
 	}
 
-	return func() {
+	err = d.Result(wfe.clk.Now())
+	if err != nil {
+		return nil, err
+	}
+
+	return func() error {
 		_, err := wfe.limiter.BatchRefund(ctx, txns)
 		if err != nil {
-			wfe.log.Errf("refunding newAccount limits: %s", err)
+			return fmt.Errorf("refunding new account limits: %w", err)
 		}
-	}
+		return nil
+	}, nil
 }
 
 // NewAccount is used by clients to submit a new account
@@ -796,13 +794,25 @@ func (wfe *WebFrontEndImpl) NewAccount(
 		InitialIP:       ipBytes,
 	}
 
-	refundLimits := wfe.checkNewAccountLimits(ctx, ip)
+	refundLimits, err := wfe.checkNewAccountLimits(ctx, ip)
+	if err != nil {
+		if errors.Is(err, berrors.RateLimit) {
+			if features.Get().NewRateLimitsNewAccountAuthoritative {
+				wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err)
+				return
+			}
+		}
+		wfe.log.Errf(err.Error())
+	}
 
 	var newRegistrationSuccessful bool
 	var errIsRateLimit bool
 	defer func() {
 		if !newRegistrationSuccessful && !errIsRateLimit && refundLimits != nil {
-			go refundLimits()
+			err := refundLimits()
+			if err != nil {
+				wfe.log.Errf(err.Error())
+			}
 		}
 	}()
 
@@ -2026,37 +2036,34 @@ func (wfe *WebFrontEndImpl) orderToOrderJSON(request *http.Request, order *corep
 // encountered during the check, it is logged but not returned. A refund
 // function is returned that can be used to refund the quota if the order is not
 // created, the func will be nil if any error was encountered during the check.
-//
-// TODO(#5545): For now we're simply exercising the new rate limiter codepath.
-// This should eventually return a berrors.RateLimit error containing the retry
-// after duration among other information available in the ratelimits.Decision.
-func (wfe *WebFrontEndImpl) checkNewOrderLimits(ctx context.Context, regId int64, names []string, isRenewal bool) func() {
+func (wfe *WebFrontEndImpl) checkNewOrderLimits(ctx context.Context, regId int64, names []string, isRenewal bool) (func() error, error) {
 	if wfe.limiter == nil && wfe.txnBuilder == nil {
 		// Key-value rate limiting is disabled.
-		return nil
+		return nil, nil
 	}
 
 	txns, err := wfe.txnBuilder.NewOrderLimitTransactions(regId, names, wfe.maxNames, isRenewal)
 	if err != nil {
-		wfe.log.Errf("building new order limit transactions: %v", err)
-		return nil
+		return nil, fmt.Errorf("building new order limit transactions: %w", err)
 	}
 
-	_, err = wfe.limiter.BatchSpend(ctx, txns)
+	d, err := wfe.limiter.BatchSpend(ctx, txns)
 	if err != nil {
-		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
-			return nil
-		}
-		wfe.log.Errf("checking newOrder limits: %s", err)
-		return nil
+		return nil, fmt.Errorf("spending new order limits: %w", err)
 	}
 
-	return func() {
+	err = d.Result(wfe.clk.Now())
+	if err != nil {
+		return nil, err
+	}
+
+	return func() error {
 		_, err := wfe.limiter.BatchRefund(ctx, txns)
 		if err != nil {
-			wfe.log.Errf("refunding newOrder limits: %s", err)
+			return fmt.Errorf("refunding new order limits: %w", err)
 		}
-	}
+		return nil
+	}, nil
 }
 
 // orderMatchesReplacement checks if the order matches the provided certificate
@@ -2367,7 +2374,16 @@ func (wfe *WebFrontEndImpl) NewOrder(
 		return
 	}
 
-	refundLimits := wfe.checkNewOrderLimits(ctx, acct.ID, names, isRenewal)
+	refundLimits, err := wfe.checkNewOrderLimits(ctx, acct.ID, names, isRenewal)
+	if err != nil {
+		if errors.Is(err, berrors.RateLimit) {
+			if features.Get().NewRateLimitsNewOrderAuthoritative {
+				wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err)
+				return
+			}
+		}
+		wfe.log.Errf(err.Error())
+	}
 
 	var newOrderSuccessful bool
 	var errIsRateLimit bool
@@ -2380,7 +2396,10 @@ func (wfe *WebFrontEndImpl) NewOrder(
 		}
 
 		if !newOrderSuccessful && !errIsRateLimit && refundLimits != nil {
-			go refundLimits()
+			err := refundLimits()
+			if err != nil {
+				wfe.log.Errf(err.Error())
+			}
 		}
 	}()