From 459440ce425d199806bfb4a08430b6a7381ac20f Mon Sep 17 00:00:00 2001 From: Samantha Date: Fri, 16 Aug 2024 09:47:19 -0400 Subject: [PATCH] ratelimits: Add a feature-flag which makes key-value implementation authoritative --- features/features.go | 10 ++ ra/ra.go | 21 ++--- test/config-next/ra.json | 4 +- test/config-next/wfe2-ratelimit-overrides.yml | 4 + test/config-next/wfe2.json | 4 +- test/integration/ratelimit_test.go | 52 +---------- wfe2/wfe.go | 93 +++++++++++-------- 7 files changed, 89 insertions(+), 99 deletions(-) diff --git a/features/features.go b/features/features.go index 892d29f9931a..706d8e1daf1d 100644 --- a/features/features.go +++ b/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) diff --git a/ra/ra.go b/ra/ra.go index dc4d01959da2..45a6d2d84bc1 100644 --- a/ra/ra.go +++ b/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. @@ -1994,13 +1997,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 } @@ -2572,7 +2569,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 { @@ -2651,7 +2648,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 diff --git a/test/config-next/ra.json b/test/config-next/ra.json index c71fd27cdfad..c53cf297379e 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -130,7 +130,9 @@ }, "features": { "AsyncFinalize": true, - "CheckRenewalExemptionAtWFE": true + "CheckRenewalExemptionAtWFE": true, + "NewRateLimitsNewOrderAuthoritative": true, + "NewRateLimitsNewAccountAuthoritative": true }, "ctLogs": { "stagger": "500ms", diff --git a/test/config-next/wfe2-ratelimit-overrides.yml b/test/config-next/wfe2-ratelimit-overrides.yml index 95303173dc8c..716ccb3aa1f0 100644 --- a/test/config-next/wfe2-ratelimit-overrides.yml +++ b/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 diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json index 55b28980ae24..af87708ca99f 100644 --- a/test/config-next/wfe2.json +++ b/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", diff --git a/test/integration/ratelimit_test.go b/test/integration/ratelimit_test.go index 5527f52d2b05..a13707ed08b4 100644 --- a/test/integration/ratelimit_test.go +++ b/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}, 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") } } diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 005d5059f180..89d64ba701d9 100644 --- a/wfe2/wfe.go +++ b/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.Warningf("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.Warningf("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, isRenewal) if err != nil { - wfe.log.Warningf("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.Warningf("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.Warningf("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()) + } } }()