wfe/features: Deprecate UseKvLimitsForNewOrder (#7765)

Default code paths that depended on this flag to be true.

Part of #5545

Author: beautifulentropy

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.

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 {

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:[email protected]"
-	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()

test/config-next/ra.json

@@ -130,8 +130,7 @@
 		},
 		"features": {
 			"AsyncFinalize": true,
-			"UseKvLimitsForNewOrder": true,
-			"UseKvLimitsForNewAccount": true
+			"UseKvLimitsForNewOrder": true
 		},
 		"ctLogs": {
 			"stagger": "500ms",

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",

test/config/ra.json

@@ -107,7 +107,8 @@
 			}
 		},
 		"features": {
-			"CheckRenewalExemptionAtWFE": true
+			"CheckRenewalExemptionAtWFE": true,
+			"UseKvLimitsForNewAccount": true
 		},
 		"ctLogs": {
 			"stagger": "500ms",

test/config/wfe2.json

@@ -106,7 +106,8 @@
 		"pendingAuthorizationLifetimeDays": 7,
 		"features": {
 			"ServeRenewalInfo": true,
-			"CheckRenewalExemptionAtWFE": true
+			"CheckRenewalExemptionAtWFE": true,
+			"UseKvLimitsForNewAccount": true
 		}
 	},
 	"syslog": {

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