ratelimits: Check at NewOrder and SpendOnly later (letsencrypt#7669)

- Check `CertificatesPerDomain` at newOrder and spend at Finalize time.
- Check `CertificatesPerAccountPerDomain` at newOrder and spend at
Finalize time.
- Check `CertificatesPerFQDNSet` at newOrder and spend at Finalize time.
- Fix a bug
in`FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction()` which
results in failed authorizations being spent for the exact FQDN, not the
eTLD+1.
- Remove redundant "max names" check at transaction construction time
- Enable key-value rate limits in the RA

ra/ra.go

@@ -1298,6 +1298,38 @@ func (ra *RegistrationAuthorityImpl) issueCertificateOuter(
 	return order, err
 }
 
+// countCertificateIssued increments the certificates (per domain and per
+// account) and duplicate certificate rate limits. There is no reason to surface
+// errors from this function to the Subscriber, spends against these limit are
+// best effort.
+func (ra *RegistrationAuthorityImpl) countCertificateIssued(ctx context.Context, regId int64, orderDomains []string) {
+	if ra.limiter == nil || ra.txnBuilder == nil {
+		// Limiter is disabled.
+		return
+	}
+
+	var transactions []ratelimits.Transaction
+	txns, err := ra.txnBuilder.CertificatesPerDomainSpendOnlyTransactions(regId, orderDomains)
+	if err != nil {
+		ra.log.Warningf("building rate limit transactions at finalize: %s", err)
+	}
+	transactions = append(transactions, txns...)
+
+	txn, err := ra.txnBuilder.CertificatesPerFQDNSetSpendOnlyTransaction(orderDomains)
+	if err != nil {
+		ra.log.Warningf("building rate limit transaction at finalize: %s", err)
+	}
+	transactions = append(transactions, txn)
+
+	_, err = ra.limiter.BatchSpend(ctx, transactions)
+	if err != nil {
+		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+			return
+		}
+		ra.log.Warningf("spending against rate limits at finalize: %s", err)
+	}
+}
+
 // certProfileID contains the name and hash of a certificate profile returned by
 // a CA.
 type certProfileID struct {
@@ -1386,6 +1418,8 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 		return nil, nil, wrapError(err, "parsing final certificate")
 	}
 
+	ra.countCertificateIssued(ctx, int64(acctID), parsedCertificate.DNSNames)
+
 	// Asynchronously submit the final certificate to any configured logs
 	go ra.ctpolicy.SubmitFinalCert(cert.Der, parsedCertificate.NotAfter)
 
@@ -1816,6 +1850,9 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI
 	return err
 }
 
+// countFailedValidation increments the failed authorizations per domain per
+// account rate limit. There is no reason to surface errors from this function
+// to the Subscriber, spends against this limit are best effort.
 func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, name string) {
 	if ra.limiter == nil || ra.txnBuilder == nil {
 		// Limiter is disabled.
@@ -1824,15 +1861,15 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context,
 
 	txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, name)
 	if err != nil {
-		ra.log.Errf("constructing rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err)
+		ra.log.Warningf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err)
 	}
 
 	_, err = ra.limiter.Spend(ctx, txn)
 	if err != nil {
 		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
 			return
 		}
-		ra.log.Errf("checking the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err)
+		ra.log.Warningf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err)
 	}
 }
 

ratelimits/bucket.go

@@ -243,11 +243,7 @@ func (builder *TransactionBuilder) ordersPerAccountTransaction(regId int64) (Tra
 //
 // Precondition: orderDomains must all pass policy.WellFormedDomainNames.
 // Precondition: len(orderDomains) < maxNames.
-func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
-	if len(orderDomains) > maxNames {
-		return nil, fmt.Errorf("order contains more than %d DNS names", maxNames)
-	}
-
+func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId int64, orderDomains []string) ([]Transaction, error) {
 	// FailedAuthorizationsPerDomainPerAccount limit uses the 'enum:regId'
 	// bucket key format for overrides.
 	perAccountBucketKey, err := newRegIdBucketKey(FailedAuthorizationsPerDomainPerAccount, regId)
@@ -271,7 +267,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckO
 		// Add a check-only transaction for each per domain per account bucket.
 		// The cost is 0, as we are only checking that the account and domain
 		// pair aren't already over the limit.
-		txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 0)
+		txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 1)
 		if err != nil {
 			return nil, err
 		}
@@ -284,6 +280,8 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckO
 // only Transaction for the provided order domain name. An error is returned if
 // the order domain name is invalid. This method should be used for spending
 // capacity, as a result of a failed authorization.
+//
+// Precondition: orderDomain must pass policy.WellFormedDomainNames.
 func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId int64, orderDomain string) (Transaction, error) {
 	// FailedAuthorizationsPerDomainPerAccount limit uses the 'enum:regId'
 	// bucket key format for overrides.
@@ -296,9 +294,14 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO
 		return Transaction{}, err
 	}
 
+	orderDomains := DomainsForRateLimiting([]string{orderDomain})
+	if len(orderDomains) != 1 {
+		return Transaction{}, fmt.Errorf("expected 1 valid domain name, got %q", orderDomain)
+	}
+
 	// FailedAuthorizationsPerDomainPerAccount limit uses the
 	// 'enum:regId:domain' bucket key format for transactions.
-	perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, orderDomain)
+	perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, orderDomains[0])
 	if err != nil {
 		return Transaction{}, err
 	}
@@ -310,26 +313,83 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO
 	return txn, nil
 }
 
-// certificatesPerDomainTransactions returns a slice of Transactions for the
-// provided order domain names. An error is returned if any of the order domain
-// names are invalid. When a CertificatesPerDomainPerAccount override is
-// configured, two types of Transactions are returned:
-//   - A spend-only Transaction for each per domain bucket. Spend-only transactions
-//     will not be denied if the bucket lacks the capacity to satisfy the cost.
-//   - A check-and-spend Transaction for each per account per domain bucket. Check-
-//     and-spend transactions will be denied if the bucket lacks the capacity to
-//     satisfy the cost.
+// certificatesPerDomainCheckOnlyTransactions returns a slice of Transactions
+// for the provided order domain names. An error is returned if any of the order
+// domain names are invalid. This method should be used for checking capacity,
+// before allowing more orders to be created. If a CertificatesPerDomainPerAccount
+// override is active, a check-only Transaction is created for each per account
+// per domain bucket. Otherwise, a check-only Transaction is generated for each
+// global per domain bucket. This method should be used for checking capacity,
+// before allowing more orders to be created.
 //
-// When a CertificatesPerDomainPerAccount override is not configured, a check-
-// and-spend Transaction is returned for each per domain bucket.
-//
-// Precondition: orderDomains must all pass policy.WellFormedDomainNames.
-// Precondition: len(orderDomains) < maxNames.
-func (builder *TransactionBuilder) certificatesPerDomainTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
-	if len(orderDomains) > maxNames {
-		return nil, fmt.Errorf("order contains more than %d DNS names", maxNames)
+// Precondition: All orderDomains must comply with policy.WellFormedDomainNames.
+func (builder *TransactionBuilder) certificatesPerDomainCheckOnlyTransactions(regId int64, orderDomains []string) ([]Transaction, error) {
+	perAccountLimitBucketKey, err := newRegIdBucketKey(CertificatesPerDomainPerAccount, regId)
+	if err != nil {
+		return nil, err
+	}
+	perAccountLimit, err := builder.getLimit(CertificatesPerDomainPerAccount, perAccountLimitBucketKey)
+	if err != nil && !errors.Is(err, errLimitDisabled) {
+		return nil, err
 	}
 
+	var txns []Transaction
+	for _, name := range DomainsForRateLimiting(orderDomains) {
+		perDomainBucketKey, err := newDomainBucketKey(CertificatesPerDomain, name)
+		if err != nil {
+			return nil, err
+		}
+		if perAccountLimit.isOverride() {
+			// An override is configured for the CertificatesPerDomainPerAccount
+			// limit.
+			perAccountPerDomainKey, err := newRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name)
+			if err != nil {
+				return nil, err
+			}
+			// Add a check-only transaction for each per account per domain
+			// bucket.
+			txn, err := newCheckOnlyTransaction(perAccountLimit, perAccountPerDomainKey, 1)
+			if err != nil {
+				return nil, err
+			}
+			txns = append(txns, txn)
+		} else {
+			// Use the per domain bucket key when no per account per domain override
+			// is configured.
+			perDomainLimit, err := builder.getLimit(CertificatesPerDomain, perDomainBucketKey)
+			if errors.Is(err, errLimitDisabled) {
+				// Skip disabled limit.
+				continue
+			}
+			if err != nil {
+				return nil, err
+			}
+			// Add a check-only transaction for each per domain bucket.
+			txn, err := newCheckOnlyTransaction(perDomainLimit, perDomainBucketKey, 1)
+			if err != nil {
+				return nil, err
+			}
+			txns = append(txns, txn)
+		}
+	}
+	return txns, nil
+}
+
+// CertificatesPerDomainSpendOnlyTransactions returns a slice of Transactions
+// for the specified order domain names. It returns an error if any domain names
+// are invalid. If a CertificatesPerDomainPerAccount override is configured, it
+// generates two types of Transactions:
+//   - A spend-only Transaction for each per-account, per-domain bucket, which
+//     enforces the limit on certificates issued per domain for each account.
+//   - A spend-only Transaction for each per-domain bucket, which enforces the
+//     global limit on certificates issued per domain.
+//
+// If no CertificatesPerDomainPerAccount override is present, it returns a
+// spend-only Transaction for each global per-domain bucket. This method should
+// be used for spending capacity, when a certificate is issued.
+//
+// Precondition: orderDomains must all pass policy.WellFormedDomainNames.
+func (builder *TransactionBuilder) CertificatesPerDomainSpendOnlyTransactions(regId int64, orderDomains []string) ([]Transaction, error) {
 	perAccountLimitBucketKey, err := newRegIdBucketKey(CertificatesPerDomainPerAccount, regId)
 	if err != nil {
 		return nil, err
@@ -352,9 +412,9 @@ func (builder *TransactionBuilder) certificatesPerDomainTransactions(regId int64
 			if err != nil {
 				return nil, err
 			}
-			// Add a check-and-spend transaction for each per account per domain
+			// Add a spend-only transaction for each per account per domain
 			// bucket.
-			txn, err := newTransaction(perAccountLimit, perAccountPerDomainKey, 1)
+			txn, err := newSpendOnlyTransaction(perAccountLimit, perAccountPerDomainKey, 1)
 			if err != nil {
 				return nil, err
 			}
@@ -376,8 +436,8 @@ func (builder *TransactionBuilder) certificatesPerDomainTransactions(regId int64
 			}
 			txns = append(txns, txn)
 		} else {
-			// Use the per domain bucket key when no per account per domain override
-			// is configured.
+			// Use the per domain bucket key when no per account per domain
+			// override is configured.
 			perDomainLimit, err := builder.getLimit(CertificatesPerDomain, perDomainBucketKey)
 			if errors.Is(err, errLimitDisabled) {
 				// Skip disabled limit.
@@ -386,8 +446,8 @@ func (builder *TransactionBuilder) certificatesPerDomainTransactions(regId int64
 			if err != nil {
 				return nil, err
 			}
-			// Add a check-and-spend transaction for each per domain bucket.
-			txn, err := newTransaction(perDomainLimit, perDomainBucketKey, 1)
+			// Add a spend-only transaction for each per domain bucket.
+			txn, err := newSpendOnlyTransaction(perDomainLimit, perDomainBucketKey, 1)
 			if err != nil {
 				return nil, err
 			}
@@ -397,9 +457,10 @@ func (builder *TransactionBuilder) certificatesPerDomainTransactions(regId int64
 	return txns, nil
 }
 
-// certificatesPerFQDNSetTransaction returns a Transaction for the provided
-// order domain names.
-func (builder *TransactionBuilder) certificatesPerFQDNSetTransaction(orderNames []string) (Transaction, error) {
+// certificatesPerFQDNSetCheckOnlyTransaction returns a check-only Transaction
+// for the provided order domain names. This method should only be used for
+// checking capacity, before allowing more orders to be created.
+func (builder *TransactionBuilder) certificatesPerFQDNSetCheckOnlyTransaction(orderNames []string) (Transaction, error) {
 	bucketKey, err := newFQDNSetBucketKey(CertificatesPerFQDNSet, orderNames)
 	if err != nil {
 		return Transaction{}, err
@@ -411,16 +472,34 @@ func (builder *TransactionBuilder) certificatesPerFQDNSetTransaction(orderNames
 		}
 		return Transaction{}, err
 	}
-	return newTransaction(limit, bucketKey, 1)
+	return newCheckOnlyTransaction(limit, bucketKey, 1)
+}
+
+// CertificatesPerFQDNSetSpendOnlyTransaction returns a spend-only Transaction
+// for the provided order domain names. This method should only be used for
+// spending capacity, when a certificate is issued.
+func (builder *TransactionBuilder) CertificatesPerFQDNSetSpendOnlyTransaction(orderNames []string) (Transaction, error) {
+	bucketKey, err := newFQDNSetBucketKey(CertificatesPerFQDNSet, orderNames)
+	if err != nil {
+		return Transaction{}, err
+	}
+	limit, err := builder.getLimit(CertificatesPerFQDNSet, bucketKey)
+	if err != nil {
+		if errors.Is(err, errLimitDisabled) {
+			return newAllowOnlyTransaction()
+		}
+		return Transaction{}, err
+	}
+	return newSpendOnlyTransaction(limit, bucketKey, 1)
 }
 
-// NewOrderLimitTransactions takes in values from a new-order request and and
+// NewOrderLimitTransactions takes in values from a new-order request and
 // returns the set of rate limit transactions that should be evaluated before
 // allowing the request to proceed.
 //
 // Precondition: names must be a list of DNS names that all pass
 // policy.WellFormedDomainNames.
-func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names []string, maxNames int, isRenewal bool) ([]Transaction, error) {
+func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names []string, isRenewal bool) ([]Transaction, error) {
 	makeTxnError := func(err error, limit Name) error {
 		return fmt.Errorf("error constructing rate limit transaction for %s rate limit: %w", limit, err)
 	}
@@ -435,22 +514,22 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names
 		transactions = append(transactions, txn)
 	}
 
-	txns, err := builder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names, maxNames)
+	txns, err := builder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names)
 	if err != nil {
 		return nil, makeTxnError(err, FailedAuthorizationsPerDomainPerAccount)
 	}
 	transactions = append(transactions, txns...)
 
 	// TODO(#7511) Remove this feature flag check.
 	if features.Get().CheckRenewalExemptionAtWFE && !isRenewal {
-		txns, err := builder.certificatesPerDomainTransactions(regId, names, maxNames)
+		txns, err := builder.certificatesPerDomainCheckOnlyTransactions(regId, names)
 		if err != nil {
 			return nil, makeTxnError(err, CertificatesPerDomain)
 		}
 		transactions = append(transactions, txns...)
 	}
 
-	txn, err := builder.certificatesPerFQDNSetTransaction(names)
+	txn, err := builder.certificatesPerFQDNSetCheckOnlyTransaction(names)
 	if err != nil {
 		return nil, makeTxnError(err, CertificatesPerFQDNSet)
 	}

test/config-next/ra.json

@@ -1,6 +1,30 @@
 {
 	"ra": {
 		"rateLimitPoliciesFilename": "test/rate-limit-policies.yml",
+		"limiter": {
+			"redis": {
+				"username": "boulder-wfe",
+				"passwordFile": "test/secrets/wfe_ratelimits_redis_password",
+				"lookups": [
+					{
+						"Service": "redisratelimits",
+						"Domain": "service.consul"
+					}
+				],
+				"lookupDNSAuthority": "consul.service.consul",
+				"readTimeout": "250ms",
+				"writeTimeout": "250ms",
+				"poolSize": 100,
+				"routeRandomly": true,
+				"tls": {
+					"caCertFile": "test/certs/ipki/minica.pem",
+					"certFile": "test/certs/ipki/wfe.boulder/cert.pem",
+					"keyFile": "test/certs/ipki/wfe.boulder/key.pem"
+				}
+			},
+			"Defaults": "test/config-next/wfe2-ratelimit-defaults.yml",
+			"Overrides": "test/config-next/wfe2-ratelimit-overrides.yml"
+		},
 		"maxContactsPerRegistration": 3,
 		"hostnamePolicyFile": "test/hostname-policy.yaml",
 		"maxNames": 100,

test/integration/ratelimit_test.go

@@ -66,7 +66,7 @@ func TestDuplicateFQDNRateLimit(t *testing.T) {
 		test.AssertNotError(t, err, "making transaction composer")
 
 		// Check that the CertificatesPerFQDNSet limit is reached.
-		txns, err := txnBuilder.NewOrderLimitTransactions(1, []string{domain}, 100, false)
+		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")