From e5edb7077fc19ad6be7e25e30e807f7c85beebab Mon Sep 17 00:00:00 2001 From: Samantha Frank Date: Wed, 23 Oct 2024 18:13:24 -0400 Subject: [PATCH] wfe/features: Deprecate UseKvLimitsForNewOrder (#7765) Default code paths that depended on this flag to be true. Part of #5545 --- features/features.go | 6 +-- ra/ra.go | 66 ------------------------------ ra/ra_test.go | 84 -------------------------------------- test/config-next/ra.json | 3 +- test/config-next/wfe2.json | 3 +- test/config/ra.json | 3 +- test/config/wfe2.json | 3 +- wfe2/wfe.go | 6 +-- 8 files changed, 9 insertions(+), 165 deletions(-) diff --git a/features/features.go b/features/features.go index 25b3ba83d..46204c529 100644 --- a/features/features.go +++ b/features/features.go @@ -29,6 +29,7 @@ type Config struct { CertCheckerRequiresCorrespondence bool ECDSAForAll bool CheckRenewalExemptionAtWFE bool + UseKvLimitsForNewAccount bool // ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for // GET requests. WARNING: This feature is a draft and highly unstable. @@ -104,11 +105,6 @@ type Config struct { // fqdnSets tables at Finalize time. UseKvLimitsForNewOrder bool - // UseKvLimitsForNewAccount 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. - UseKvLimitsForNewAccount bool - // DisableLegacyLimitWrites when enabled, disables writes to: // - the newOrdersRL table at new-order time, and // - the certificatesPerName table at finalize time. diff --git a/ra/ra.go b/ra/ra.go index 62486f515..b368b9885 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -415,58 +415,6 @@ func (ra *RegistrationAuthorityImpl) checkRegistrationIPLimit(ctx context.Contex return nil } -// checkRegistrationLimits enforces the RegistrationsPerIP and -// RegistrationsPerIPRange limits -func (ra *RegistrationAuthorityImpl) checkRegistrationLimits(ctx context.Context, ip net.IP) error { - // Check the registrations per IP limit using the CountRegistrationsByIP SA - // function that matches IP addresses exactly - exactRegLimit := ra.rlPolicies.RegistrationsPerIP() - if exactRegLimit.Enabled() { - started := ra.clk.Now() - err := ra.checkRegistrationIPLimit(ctx, exactRegLimit, ip, ra.SA.CountRegistrationsByIP) - elapsed := ra.clk.Since(started) - if err != nil { - if errors.Is(err, berrors.RateLimit) { - ra.rlCheckLatency.WithLabelValues(ratelimit.RegistrationsPerIP, ratelimits.Denied).Observe(elapsed.Seconds()) - ra.log.Infof("Rate limit exceeded, RegistrationsPerIP, by IP: %q", ip) - } - return err - } - ra.rlCheckLatency.WithLabelValues(ratelimit.RegistrationsPerIP, ratelimits.Allowed).Observe(elapsed.Seconds()) - } - - // We only apply the fuzzy reg limit to IPv6 addresses. - // Per https://golang.org/pkg/net/#IP.To4 "If ip is not an IPv4 address, To4 - // returns nil" - if ip.To4() != nil { - return nil - } - - // Check the registrations per IP range limit using the - // CountRegistrationsByIPRange SA function that fuzzy-matches IPv6 addresses - // within a larger address range - fuzzyRegLimit := ra.rlPolicies.RegistrationsPerIPRange() - if fuzzyRegLimit.Enabled() { - started := ra.clk.Now() - err := ra.checkRegistrationIPLimit(ctx, fuzzyRegLimit, ip, ra.SA.CountRegistrationsByIPRange) - elapsed := ra.clk.Since(started) - if err != nil { - if errors.Is(err, berrors.RateLimit) { - ra.rlCheckLatency.WithLabelValues(ratelimit.RegistrationsPerIPRange, ratelimits.Denied).Observe(elapsed.Seconds()) - ra.log.Infof("Rate limit exceeded, RegistrationsByIPRange, IP: %q", ip) - - // For the fuzzyRegLimit we use a new error message that specifically - // mentions that the limit being exceeded is applied to a *range* of IPs - return berrors.RateLimitError(0, "too many registrations for this IP range") - } - return err - } - ra.rlCheckLatency.WithLabelValues(ratelimit.RegistrationsPerIPRange, ratelimits.Allowed).Observe(elapsed.Seconds()) - } - - return nil -} - // NewRegistration constructs a new Registration from a request. func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, request *corepb.Registration) (*corepb.Registration, error) { // Error if the request is nil, there is no account key or IP address @@ -485,20 +433,6 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, reques return nil, berrors.MalformedError("invalid public key: %s", err.Error()) } - // Check IP address rate limits. - var ipAddr net.IP - err = ipAddr.UnmarshalText(request.InitialIP) - if err != nil { - return nil, berrors.InternalServerError("failed to unmarshal ip address: %s", err.Error()) - } - - if !features.Get().UseKvLimitsForNewAccount { - err = ra.checkRegistrationLimits(ctx, ipAddr) - if err != nil { - return nil, err - } - } - // Check that contacts conform to our expectations. err = validateContactsPresent(request.Contact, request.ContactsPresent) if err != nil { diff --git a/ra/ra_test.go b/ra/ra_test.go index a6b555f07..6f7681b24 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -628,90 +628,6 @@ func TestNewRegistrationBadKey(t *testing.T) { test.AssertError(t, err, "Should have rejected authorization with short key") } -func TestNewRegistrationRateLimit(t *testing.T) { - _, _, ra, _, cleanUp := initAuthorities(t) - defer cleanUp() - - // Specify a dummy rate limit policy that allows 1 registration per exact IP - // match, and 2 per range. - ra.rlPolicies = &dummyRateLimitConfig{ - RegistrationsPerIPPolicy: ratelimit.RateLimitPolicy{ - Threshold: 1, - Window: config.Duration{Duration: 24 * 90 * time.Hour}, - }, - RegistrationsPerIPRangePolicy: ratelimit.RateLimitPolicy{ - Threshold: 2, - Window: config.Duration{Duration: 24 * 90 * time.Hour}, - }, - } - - // Create one registration for an IPv4 address - mailto := "mailto:foo@letsencrypt.org" - reg := &corepb.Registration{ - Contact: []string{mailto}, - ContactsPresent: true, - Key: newAcctKey(t), - InitialIP: parseAndMarshalIP(t, "7.6.6.5"), - } - // There should be no errors - it is within the RegistrationsPerIP rate limit - _, err := ra.NewRegistration(ctx, reg) - test.AssertNotError(t, err, "Unexpected error adding new IPv4 registration") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIP, "decision": ratelimits.Allowed}, 1) - // There are no overrides for this IP, so the override usage gauge should - // contain 0 entries with labels matching it. - test.AssertMetricWithLabelsEquals(t, ra.rlOverrideUsageGauge, prometheus.Labels{"limit": ratelimit.RegistrationsPerIP, "override_key": "7.6.6.5"}, 0) - - // Create another registration for the same IPv4 address by changing the key - reg.Key = newAcctKey(t) - - // There should be an error since a 2nd registration will exceed the - // RegistrationsPerIP rate limit - _, err = ra.NewRegistration(ctx, reg) - test.AssertError(t, err, "No error adding duplicate IPv4 registration") - test.AssertEquals(t, err.Error(), "too many registrations for this IP: see https://letsencrypt.org/docs/too-many-registrations-for-this-ip/") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIP, "decision": ratelimits.Denied}, 1) - - // Create a registration for an IPv6 address - reg.Key = newAcctKey(t) - reg.InitialIP = parseAndMarshalIP(t, "2001:cdba:1234:5678:9101:1121:3257:9652") - - // There should be no errors - it is within the RegistrationsPerIP rate limit - _, err = ra.NewRegistration(ctx, reg) - test.AssertNotError(t, err, "Unexpected error adding a new IPv6 registration") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIP, "decision": ratelimits.Allowed}, 2) - - // Create a 2nd registration for the IPv6 address by changing the key - reg.Key = newAcctKey(t) - - // There should be an error since a 2nd reg for the same IPv6 address will - // exceed the RegistrationsPerIP rate limit - _, err = ra.NewRegistration(ctx, reg) - test.AssertError(t, err, "No error adding duplicate IPv6 registration") - test.AssertEquals(t, err.Error(), "too many registrations for this IP: see https://letsencrypt.org/docs/too-many-registrations-for-this-ip/") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIP, "decision": ratelimits.Denied}, 2) - - // Create a registration for an IPv6 address in the same /48 - reg.Key = newAcctKey(t) - reg.InitialIP = parseAndMarshalIP(t, "2001:cdba:1234:5678:9101:1121:3257:9653") - - // There should be no errors since two IPv6 addresses in the same /48 is - // within the RegistrationsPerIPRange limit - _, err = ra.NewRegistration(ctx, reg) - test.AssertNotError(t, err, "Unexpected error adding second IPv6 registration in the same /48") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIPRange, "decision": ratelimits.Allowed}, 2) - - // Create a registration for yet another IPv6 address in the same /48 - reg.Key = newAcctKey(t) - reg.InitialIP = parseAndMarshalIP(t, "2001:cdba:1234:5678:9101:1121:3257:9654") - - // There should be an error since three registrations within the same IPv6 - // /48 is outside of the RegistrationsPerIPRange limit - _, err = ra.NewRegistration(ctx, reg) - test.AssertError(t, err, "No error adding a third IPv6 registration in the same /48") - test.AssertEquals(t, err.Error(), "too many registrations for this IP range: see https://letsencrypt.org/docs/rate-limits/") - test.AssertMetricWithLabelsEquals(t, ra.rlCheckLatency, prometheus.Labels{"limit": ratelimit.RegistrationsPerIPRange, "decision": ratelimits.Denied}, 1) -} - func TestRegistrationsPerIPOverrideUsage(t *testing.T) { _, _, ra, _, cleanUp := initAuthorities(t) defer cleanUp() diff --git a/test/config-next/ra.json b/test/config-next/ra.json index 45d03529e..ad320cab7 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -130,8 +130,7 @@ }, "features": { "AsyncFinalize": true, - "UseKvLimitsForNewOrder": true, - "UseKvLimitsForNewAccount": true + "UseKvLimitsForNewOrder": true }, "ctLogs": { "stagger": "500ms", diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json index 3403f2cc0..25ab2e796 100644 --- a/test/config-next/wfe2.json +++ b/test/config-next/wfe2.json @@ -130,8 +130,7 @@ "ServeRenewalInfo": true, "TrackReplacementCertificatesARI": true, "CheckIdentifiersPaused": true, - "UseKvLimitsForNewOrder": true, - "UseKvLimitsForNewAccount": true + "UseKvLimitsForNewOrder": true }, "certProfiles": { "legacy": "The normal profile you know and love", diff --git a/test/config/ra.json b/test/config/ra.json index 3b20313d8..7efd19676 100644 --- a/test/config/ra.json +++ b/test/config/ra.json @@ -107,7 +107,8 @@ } }, "features": { - "CheckRenewalExemptionAtWFE": true + "CheckRenewalExemptionAtWFE": true, + "UseKvLimitsForNewAccount": true }, "ctLogs": { "stagger": "500ms", diff --git a/test/config/wfe2.json b/test/config/wfe2.json index 625ee175c..a34804c83 100644 --- a/test/config/wfe2.json +++ b/test/config/wfe2.json @@ -106,7 +106,8 @@ "pendingAuthorizationLifetimeDays": 7, "features": { "ServeRenewalInfo": true, - "CheckRenewalExemptionAtWFE": true + "CheckRenewalExemptionAtWFE": true, + "UseKvLimitsForNewAccount": true } }, "syslog": { diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 0a5e8483a..3fe5e5dd5 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -792,10 +792,8 @@ func (wfe *WebFrontEndImpl) NewAccount( refundLimits, err := wfe.checkNewAccountLimits(ctx, ip) if err != nil { if errors.Is(err, berrors.RateLimit) { - if features.Get().UseKvLimitsForNewAccount { - wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err) - return - } + wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err) + return } else { wfe.log.Warning(err.Error()) }