ratelimits: Exempt renewals from NewOrdersPerAccount and Certificates…

…PerDomain
letsencrypt · Jun 18, 2024 · 596a850 · 596a850
1 parent a69ba99
commit 596a850
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 86 deletions.
diff --git a/features/features.go b/features/features.go
@@ -92,6 +92,16 @@ type Config struct {
 	//     `orders.certificateProfileName` column. Values in this column are
 	//     allowed to be empty.
 	MultipleCertificateProfiles bool
+
+	// CheckRenewalExemptionAtWFE when enabled, triggers the following behavior:
+	//  - WFE.NewOrder: checks if the order is a renewal, and if so:
+	//    - skips checks for NewOrdersPerAccount and NewOrdersPerDomain, also
+	//    - passes the isRenewal value in the NewOrderRequest to the RA.
+	//  - RA.NewOrderAndAuthzs: skips checks for legacy NewOrdersPerAccount
+	//    and NewOrdersPerDomain.
+	//
+	// TODO(#7511): Remove this feature flag.
+	CheckRenewalExemptionAtWFE bool
 }
 
 var fMu = new(sync.RWMutex)

diff --git a/ra/proto/ra.pb.go b/ra/proto/ra.pb.go
diff --git a/ra/proto/ra.proto b/ra/proto/ra.proto
@@ -68,12 +68,15 @@ message AdministrativelyRevokeCertificateRequest {
 }
 
 message NewOrderRequest {
-  // Next unused field number: 6
+  // Next unused field number: 7
   int64 registrationID = 1;
   repeated string names = 2;
   string replacesSerial = 3;
+  // TODO(#7512): Remove this field.
   bool limitsExempt = 4;
   string certificateProfileName = 5;
+  // TODO(#7512): Remove this field.
+  bool isRenewal = 6;
 }
 
 message FinalizeOrderRequest {

diff --git a/ra/ra.go b/ra/ra.go
@@ -683,6 +683,9 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context.
 func (ra *RegistrationAuthorityImpl) checkNewOrdersPerAccountLimit(ctx context.Context, acctID int64, names []string, limit ratelimit.RateLimitPolicy) error {
 	// Check if there is already an existing certificate for the exact name set we
 	// are issuing for. If so bypass the newOrders limit.
+	//
+	// TODO(#7511): This check and early return should be removed, it's
+	// being performed at the WFE.
 	exists, err := ra.SA.FQDNSetExists(ctx, &sapb.FQDNSetExistsRequest{Domains: names})
 	if err != nil {
 		return fmt.Errorf("checking renewal exemption for %q: %s", names, err)
@@ -1480,6 +1483,9 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerNameLimit(ctx context.C
 	// check if there is already an existing certificate for
 	// the exact name set we are issuing for. If so bypass the
 	// the certificatesPerName limit.
+	//
+	// TODO(#7511): This check and early return should be removed, it's
+	// being performed, once, at the WFE.
 	exists, err := ra.SA.FQDNSetExists(ctx, &sapb.FQDNSetExistsRequest{Domains: names})
 	if err != nil {
 		return fmt.Errorf("checking renewal exemption for %q: %s", names, err)
@@ -1577,9 +1583,11 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerFQDNSetLimit(ctx contex
 	}
 }
 
-func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, names []string, regID int64) error {
+func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, names []string, regID int64, isRenewal bool) error {
 	newOrdersPerAccountLimits := ra.rlPolicies.NewOrdersPerAccount()
-	if newOrdersPerAccountLimits.Enabled() {
+	// TODO(#7511): Remove the feature flag check.
+	skipCheck := features.Get().CheckRenewalExemptionAtWFE && isRenewal
+	if newOrdersPerAccountLimits.Enabled() && !skipCheck {
 		started := ra.clk.Now()
 		err := ra.checkNewOrdersPerAccountLimit(ctx, regID, names, newOrdersPerAccountLimits)
 		elapsed := ra.clk.Since(started)
@@ -1593,7 +1601,7 @@ func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, na
 	}
 
 	certNameLimits := ra.rlPolicies.CertificatesPerName()
-	if certNameLimits.Enabled() {
+	if certNameLimits.Enabled() && !skipCheck {
 		started := ra.clk.Now()
 		err := ra.checkCertificatesPerNameLimit(ctx, names, certNameLimits, regID)
 		elapsed := ra.clk.Since(started)
@@ -2517,7 +2525,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
 	if !req.LimitsExempt {
 
 		// Check if there is rate limit space for issuing a certificate.
-		err = ra.checkNewOrderLimits(ctx, newOrder.Names, newOrder.RegistrationID)
+		err = ra.checkNewOrderLimits(ctx, newOrder.Names, newOrder.RegistrationID, req.IsRenewal)
 		if err != nil {
 			return nil, err
 		}

diff --git a/test/config-next/ra.json b/test/config-next/ra.json
@@ -105,7 +105,8 @@
 			}
 		},
 		"features": {
-			"AsyncFinalize": true
+			"AsyncFinalize": true,
+			"CheckRenewalExemptionAtWFE": true
 		},
 		"ctLogs": {
 			"stagger": "500ms",

diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json
@@ -128,7 +128,8 @@
 		},
 		"features": {
 			"ServeRenewalInfo": true,
-			"TrackReplacementCertificatesARI": true
+			"TrackReplacementCertificatesARI": true,
+			"CheckRenewalExemptionAtWFE": true
 		},
 		"certificateProfileNames": [
 			"defaultBoulderCertificateProfile"