WFE/RA: Default codepaths to CheckRenewalExemptionAtWFE: true (letsen…

…crypt#7745)

Also, remove redundant renewal checks in
`RA.checkNewOrdersPerAccountLimit()` and
`RA.checkCertificatesPerNameLimit()`.

Part of letsencrypt#7511

features/features.go

@@ -28,6 +28,7 @@ type Config struct {
 	MultiVAFullResults                bool
 	CertCheckerRequiresCorrespondence bool
 	ECDSAForAll                       bool
+	CheckRenewalExemptionAtWFE        bool
 
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
@@ -89,16 +90,6 @@ type Config struct {
 	//     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 limits.
-	//  - RA.NewOrderAndAuthzs: skips checks for legacy NewOrdersPerAccount and
-	//    NewOrdersPerDomain limits if the WFE indicates that the order is a
-	//    renewal.
-	//
-	// TODO(#7511): Remove this feature flag.
-	CheckRenewalExemptionAtWFE bool
-
 	// CheckIdentifiersPaused checks if any of the identifiers in the order are
 	// currently paused at NewOrder time. If any are paused, an error is
 	// returned to the Subscriber indicating that the order cannot be processed

ra/ra.go

@@ -676,20 +676,7 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context.
 // checkNewOrdersPerAccountLimit enforces the rlPolicies `NewOrdersPerAccount`
 // rate limit. This rate limit ensures a client can not create more than the
 // specified threshold of new orders within the specified time window.
-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{DnsNames: names})
-	if err != nil {
-		return fmt.Errorf("checking renewal exemption for %q: %s", names, err)
-	}
-	if exists.Exists {
-		return nil
-	}
-
+func (ra *RegistrationAuthorityImpl) checkNewOrdersPerAccountLimit(ctx context.Context, acctID int64, limit ratelimit.RateLimitPolicy) error {
 	now := ra.clk.Now()
 	count, err := ra.SA.CountOrders(ctx, &sapb.CountOrdersRequest{
 		AccountID: acctID,
@@ -1533,20 +1520,6 @@ func (ra *RegistrationAuthorityImpl) enforceNameCounts(ctx context.Context, name
 }
 
 func (ra *RegistrationAuthorityImpl) checkCertificatesPerNameLimit(ctx context.Context, names []string, limit ratelimit.RateLimitPolicy, regID int64) error {
-	// 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{DnsNames: names})
-	if err != nil {
-		return fmt.Errorf("checking renewal exemption for %q: %s", names, err)
-	}
-	if exists.Exists {
-		return nil
-	}
-
 	tldNames := ratelimits.FQDNsToETLDsPlusOne(names)
 	namesOutOfLimit, earliest, err := ra.enforceNameCounts(ctx, tldNames, limit, regID)
 	if err != nil {
@@ -1638,11 +1611,9 @@ func (ra *RegistrationAuthorityImpl) checkCertificatesPerFQDNSetLimit(ctx contex
 
 func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, names []string, regID int64, isRenewal bool) error {
 	newOrdersPerAccountLimits := ra.rlPolicies.NewOrdersPerAccount()
-	// TODO(#7511): Remove the feature flag check.
-	skipCheck := features.Get().CheckRenewalExemptionAtWFE && isRenewal
-	if newOrdersPerAccountLimits.Enabled() && !skipCheck {
+	if newOrdersPerAccountLimits.Enabled() && !isRenewal {
 		started := ra.clk.Now()
-		err := ra.checkNewOrdersPerAccountLimit(ctx, regID, names, newOrdersPerAccountLimits)
+		err := ra.checkNewOrdersPerAccountLimit(ctx, regID, newOrdersPerAccountLimits)
 		elapsed := ra.clk.Since(started)
 		if err != nil {
 			if errors.Is(err, berrors.RateLimit) {
@@ -1654,7 +1625,7 @@ func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, na
 	}
 
 	certNameLimits := ra.rlPolicies.CertificatesPerName()
-	if certNameLimits.Enabled() && !skipCheck {
+	if certNameLimits.Enabled() && !isRenewal {
 		started := ra.clk.Now()
 		err := ra.checkCertificatesPerNameLimit(ctx, names, certNameLimits, regID)
 		elapsed := ra.clk.Since(started)

ra/ra_test.go

@@ -1004,7 +1004,7 @@ func TestCertificateKeyNotEqualAccountKey(t *testing.T) {
 }
 
 func TestNewOrderRateLimiting(t *testing.T) {
-	_, sa, ra, fc, cleanUp := initAuthorities(t)
+	_, _, ra, fc, cleanUp := initAuthorities(t)
 	defer cleanUp()
 
 	ra.orderLifetime = 5 * 24 * time.Hour
@@ -1045,33 +1045,6 @@ func TestNewOrderRateLimiting(t *testing.T) {
 	_, err = ra.NewOrder(ctx, orderOne)
 	test.AssertNotError(t, err, "Reuse of orderOne failed")
 
-	// Insert a specific certificate into the database, then create an order for
-	// the same set of names. This order should succeed because it's a renewal.
-	testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	test.AssertNotError(t, err, "generating test key")
-	fakeCert := &x509.Certificate{
-		SerialNumber: big.NewInt(1),
-		DNSNames:     []string{"renewing.example.com"},
-		NotBefore:    fc.Now().Add(-time.Hour),
-		NotAfter:     fc.Now().Add(time.Hour),
-		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
-	}
-	certDER, err := x509.CreateCertificate(rand.Reader, fakeCert, fakeCert, testKey.Public(), testKey)
-	test.AssertNotError(t, err, "generating test certificate")
-	_, err = sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
-		Der:          certDER,
-		RegID:        Registration.Id,
-		Issued:       timestamppb.New(fc.Now().Add(-time.Hour)),
-		IssuerNameID: 1,
-	})
-	test.AssertNotError(t, err, "Adding test certificate")
-
-	_, err = ra.NewOrder(ctx, &rapb.NewOrderRequest{
-		RegistrationID: Registration.Id,
-		DnsNames:       []string{"renewing.example.com"},
-	})
-	test.AssertNotError(t, err, "Renewal of orderRenewal failed")
-
 	// Advancing the clock by 2 * the rate limit duration should allow orderTwo to
 	// succeed
 	fc.Add(2 * rateLimitDuration)
@@ -1490,12 +1463,6 @@ func (m mockSAWithFQDNSet) hashNames(names []string) string {
 	return string(hash[:])
 }
 
-// Add a set of domain names to the FQDN set
-func (m mockSAWithFQDNSet) addFQDNSet(names []string) {
-	hash := m.hashNames(names)
-	m.fqdnSet[hash] = true
-}
-
 // Search for a set of domain names in the FQDN set map
 func (m mockSAWithFQDNSet) FQDNSetExists(_ context.Context, req *sapb.FQDNSetExistsRequest, _ ...grpc.CallOption) (*sapb.Exists, error) {
 	hash := m.hashNames(req.DnsNames)
@@ -1536,51 +1503,6 @@ func (m mockSAWithFQDNSet) FQDNSetTimestampsForWindow(_ context.Context, req *sa
 	}
 }
 
-// Tests for boulder issue 1925[0] - that the `checkCertificatesPerNameLimit`
-// properly honours the FQDNSet exemption. E.g. that if a set of domains has
-// reached the certificates per name rate limit policy threshold but the exact
-// same set of FQDN's was previously issued, then it should not be considered
-// over the certificates per name limit.
-//
-// [0] https://github.com/letsencrypt/boulder/issues/1925
-func TestCheckFQDNSetRateLimitOverride(t *testing.T) {
-	_, _, ra, _, cleanUp := initAuthorities(t)
-	defer cleanUp()
-
-	// Simple policy that only allows 1 certificate per name.
-	certsPerNamePolicy := ratelimit.RateLimitPolicy{
-		Threshold: 1,
-		Window:    config.Duration{Duration: 24 * time.Hour},
-	}
-
-	// Create a mock SA that has both name counts and an FQDN set
-	ts := timestamppb.New(ra.clk.Now())
-	mockSA := &mockSAWithFQDNSet{
-		issuanceTimestamps: map[string]*sapb.Timestamps{
-			"example.com": {Timestamps: []*timestamppb.Timestamp{ts, ts}},
-			"zombo.com":   {Timestamps: []*timestamppb.Timestamp{ts, ts}},
-		},
-		fqdnSet: map[string]bool{},
-		t:       t,
-	}
-	ra.SA = mockSA
-
-	// First check that without a pre-existing FQDN set that the provided set of
-	// names is rate limited due to being over the certificates per name limit for
-	// "example.com" and "zombo.com"
-	err := ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com", "www.zombo.com"}, certsPerNamePolicy, 99)
-	test.AssertError(t, err, "certificate per name rate limit not applied correctly")
-
-	// Now add a FQDN set entry for these domains
-	mockSA.addFQDNSet([]string{"www.example.com", "example.com", "www.zombo.com"})
-
-	// A subsequent check against the certificates per name limit should now be OK
-	// - there exists a FQDN set and so the exemption to this particular limit
-	// comes into effect.
-	err = ra.checkCertificatesPerNameLimit(ctx, []string{"www.example.com", "example.com", "www.zombo.com"}, certsPerNamePolicy, 99)
-	test.AssertNotError(t, err, "FQDN set certificate per name exemption not applied correctly")
-}
-
 // TestExactPublicSuffixCertLimit tests the behaviour of issue #2681 with and
 // without the feature flag for the fix enabled.
 // See https://github.com/letsencrypt/boulder/issues/2681

ratelimits/bucket.go

@@ -8,7 +8,6 @@ import (
 	"strings"
 
 	"github.com/letsencrypt/boulder/core"
-	"github.com/letsencrypt/boulder/features"
 )
 
 // ErrInvalidCost indicates that the cost specified was < 0.
@@ -497,8 +496,7 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names
 	}
 
 	var transactions []Transaction
-	// TODO(#7511) Remove this feature flag check.
-	if features.Get().CheckRenewalExemptionAtWFE && !isRenewal {
+	if !isRenewal {
 		txn, err := builder.ordersPerAccountTransaction(regId)
 		if err != nil {
 			return nil, makeTxnError(err, NewOrdersPerAccount)
@@ -512,8 +510,7 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names
 	}
 	transactions = append(transactions, txns...)
 
-	// TODO(#7511) Remove this feature flag check.
-	if features.Get().CheckRenewalExemptionAtWFE && !isRenewal {
+	if !isRenewal {
 		txns, err := builder.certificatesPerDomainCheckOnlyTransactions(regId, names)
 		if err != nil {
 			return nil, makeTxnError(err, CertificatesPerDomain)

test/config-next/ra.json

@@ -130,7 +130,6 @@
 		},
 		"features": {
 			"AsyncFinalize": true,
-			"CheckRenewalExemptionAtWFE": true,
 			"UseKvLimitsForNewOrder": true,
 			"UseKvLimitsForNewAccount": true
 		},

test/config-next/wfe2.json

@@ -129,7 +129,6 @@
 		"features": {
 			"ServeRenewalInfo": true,
 			"TrackReplacementCertificatesARI": true,
-			"CheckRenewalExemptionAtWFE": true,
 			"CheckIdentifiersPaused": true,
 			"UseKvLimitsForNewOrder": true,
 			"UseKvLimitsForNewAccount": true

wfe2/wfe.go

@@ -2359,8 +2359,7 @@ func (wfe *WebFrontEndImpl) NewOrder(
 	}
 
 	var isRenewal bool
-	// TODO(#7511) Remove this feature flag check.
-	if features.Get().CheckRenewalExemptionAtWFE && !isARIRenewal {
+	if !isARIRenewal {
 		// The Subscriber does not have an ARI exemption. However, we can check
 		// if the order is a renewal, and thus exempt from the NewOrdersPerAccount
 		// and CertificatesPerDomain limits.

wfe2/wfe_test.go

@@ -437,7 +437,6 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) {
 	var unpauseLifetime time.Duration
 	var unpauseURL string
 	if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" {
-		features.Set(features.Config{CheckRenewalExemptionAtWFE: true})
 		unpauseSigner, err = unpause.NewJWTSigner(cmd.HMACKeyConfig{KeyFile: "../test/secrets/sfe_unpause_key"})
 		test.AssertNotError(t, err, "making unpause signer")
 		unpauseLifetime = time.Hour * 24 * 14