ratelimits: Fix transaction building for Failed Authorizations Limit

ratelimits/bucket.go

@@ -235,39 +235,84 @@ func (builder *TransactionBuilder) OrdersPerAccountTransaction(regId int64) (Tra
 	return newTransaction(limit, bucketKey, 1)
 }
 
-// FailedAuthorizationsPerAccountCheckOnlyTransaction returns a check-only
-// Transaction for the provided ACME registration Id for the
-// FailedAuthorizationsPerAccount limit.
-func (builder *TransactionBuilder) FailedAuthorizationsPerAccountCheckOnlyTransaction(regId int64) (Transaction, error) {
-	bucketKey, err := newRegIdBucketKey(FailedAuthorizationsPerAccount, regId)
-	if err != nil {
-		return Transaction{}, err
+// FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions 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 authorizations to be created.
+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)
 	}
-	limit, err := builder.getLimit(FailedAuthorizationsPerAccount, bucketKey)
+
+	// FailedAuthorizationsPerDomainPerAccount limit uses the 'enum:regId'
+	// bucket key format for overrides.
+	perAccountBucketKey, err := newRegIdBucketKey(FailedAuthorizationsPerDomainPerAccount, regId)
 	if err != nil {
-		if errors.Is(err, errLimitDisabled) {
-			return newAllowOnlyTransaction()
+		return nil, err
+	}
+	limit, err := builder.getLimit(FailedAuthorizationsPerDomainPerAccount, perAccountBucketKey)
+	if err != nil && !errors.Is(err, errLimitDisabled) {
+		return nil, err
+	}
+
+	var txns []Transaction
+	for _, name := range DomainsForRateLimiting(orderDomains) {
+		// FailedAuthorizationsPerDomainPerAccount limit uses the
+		// 'enum:regId:domain' bucket key format for transactions.
+		perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, name)
+		if err != nil {
+			return nil, err
 		}
-		return Transaction{}, err
+
+		// 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)
+		if err != nil {
+			return nil, err
+		}
+		txns = append(txns, txn)
 	}
-	return newCheckOnlyTransaction(limit, bucketKey, 1)
+	return txns, nil
 }
 
-// FailedAuthorizationsPerAccountTransaction returns a Transaction for the
-// FailedAuthorizationsPerAccount limit for the provided ACME registration Id.
-func (builder *TransactionBuilder) FailedAuthorizationsPerAccountTransaction(regId int64) (Transaction, error) {
-	bucketKey, err := newRegIdBucketKey(FailedAuthorizationsPerAccount, regId)
-	if err != nil {
-		return Transaction{}, err
+// FailedAuthorizationsPerDomainPerAccountSpendOnlyTransactions 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
+// spending capacity, as a result of a failed authorization.
+func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendOnlyTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
+	if len(orderDomains) > maxNames {
+		return nil, fmt.Errorf("order contains more than %d DNS names", maxNames)
 	}
-	limit, err := builder.getLimit(FailedAuthorizationsPerAccount, bucketKey)
+
+	// FailedAuthorizationsPerDomainPerAccount limit uses the 'enum:regId'
+	// bucket key format for overrides.
+	perAccountBucketKey, err := newRegIdBucketKey(FailedAuthorizationsPerDomainPerAccount, regId)
 	if err != nil {
-		if errors.Is(err, errLimitDisabled) {
-			return newAllowOnlyTransaction()
+		return nil, err
+	}
+	limit, err := builder.getLimit(FailedAuthorizationsPerDomainPerAccount, perAccountBucketKey)
+	if err != nil && !errors.Is(err, errLimitDisabled) {
+		return nil, err
+	}
+
+	var txns []Transaction
+	for _, name := range DomainsForRateLimiting(orderDomains) {
+		// FailedAuthorizationsPerDomainPerAccount limit uses the
+		// 'enum:regId:domain' bucket key format for transactions.
+		perDomainPerAccountBucketKey, err := newRegIdDomainBucketKey(FailedAuthorizationsPerDomainPerAccount, regId, name)
+		if err != nil {
+			return nil, err
 		}
-		return Transaction{}, err
+
+		// Add an allow-only transaction for each per domain per account bucket.
+		txn, err := newSpendOnlyTransaction(limit, perDomainPerAccountBucketKey, 1)
+		if err != nil {
+			return nil, err
+		}
+		txns = append(txns, txn)
 	}
-	return newTransaction(limit, bucketKey, 1)
+	return txns, nil
 }
 
 // CertificatesPerDomainTransactions returns a slice of Transactions for the

ratelimits/names.go

@@ -40,11 +40,14 @@ const (
 	// NewOrdersPerAccount uses bucket key 'enum:regId'.
 	NewOrdersPerAccount
 
-	// FailedAuthorizationsPerAccount uses bucket key 'enum:regId', where regId
-	// is the ACME registration Id of the account. Cost MUST be consumed from
-	// this bucket only when the authorization is considered "failed". It SHOULD
-	// be checked before new authorizations are created.
-	FailedAuthorizationsPerAccount
+	// FailedAuthorizationsPerDomainPerAccount uses two different bucket keys
+	// depending on the context:
+	//  - When referenced in an overrides file: uses bucket key 'enum:regId',
+	//    where regId is the ACME registration Id of the account.
+	//  - When referenced in a transaction: uses bucket key 'enum:regId:domain',
+	//    where regId is the ACME registration Id of the account and domain is a
+	//    domain name in the certificate.
+	FailedAuthorizationsPerDomainPerAccount
 
 	// CertificatesPerDomain uses bucket key 'enum:domain', where domain is a
 	// domain name in the certificate.
@@ -96,14 +99,14 @@ func (n Name) EnumString() string {
 
 // nameToString is a map of Name values to string names.
 var nameToString = map[Name]string{
-	Unknown:                         "Unknown",
-	NewRegistrationsPerIPAddress:    "NewRegistrationsPerIPAddress",
-	NewRegistrationsPerIPv6Range:    "NewRegistrationsPerIPv6Range",
-	NewOrdersPerAccount:             "NewOrdersPerAccount",
-	FailedAuthorizationsPerAccount:  "FailedAuthorizationsPerAccount",
-	CertificatesPerDomain:           "CertificatesPerDomain",
-	CertificatesPerDomainPerAccount: "CertificatesPerDomainPerAccount",
-	CertificatesPerFQDNSet:          "CertificatesPerFQDNSet",
+	Unknown:                                 "Unknown",
+	NewRegistrationsPerIPAddress:            "NewRegistrationsPerIPAddress",
+	NewRegistrationsPerIPv6Range:            "NewRegistrationsPerIPv6Range",
+	NewOrdersPerAccount:                     "NewOrdersPerAccount",
+	FailedAuthorizationsPerDomainPerAccount: "FailedAuthorizationsPerDomainPerAccount",
+	CertificatesPerDomain:                   "CertificatesPerDomain",
+	CertificatesPerDomainPerAccount:         "CertificatesPerDomainPerAccount",
+	CertificatesPerFQDNSet:                  "CertificatesPerFQDNSet",
 }
 
 // validIPAddress validates that the provided string is a valid IP address.
@@ -202,10 +205,19 @@ func validateIdForName(name Name, id string) error {
 		// 'enum:ipv6rangeCIDR'
 		return validIPv6RangeCIDR(id)
 
-	case NewOrdersPerAccount, FailedAuthorizationsPerAccount:
+	case NewOrdersPerAccount:
 		// 'enum:regId'
 		return validateRegId(id)
 
+	case FailedAuthorizationsPerDomainPerAccount:
+		// 'enum:regId:domain' for transaction
+		err := validateRegIdDomain(id)
+		if err == nil {
+			return nil
+		}
+		// 'enum:regId' for overrides
+		return validateRegId(id)
+
 	case CertificatesPerDomainPerAccount:
 		// 'enum:regId:domain' for transaction
 		err := validateRegIdDomain(id)

ratelimits/testdata/working_overrides.yml

@@ -8,3 +8,9 @@
     count: 50
     period: 2s
     ids: [2001:0db8:0000::/48]
+- FailedAuthorizationsPerDomainPerAccount:
+    burst: 60
+    count: 60
+    period: 3s
+    ids: [1234, 5678]
+

test/config-next/wfe2-ratelimit-defaults.yml

@@ -10,7 +10,7 @@ CertificatesPerDomain:
   count: 2
   burst: 2
   period: 2160h
-FailedAuthorizationsPerAccount:
+FailedAuthorizationsPerDomainPerAccount:
   count: 3
   burst: 3
   period: 5m

wfe2/wfe.go

@@ -2064,19 +2064,19 @@ func (wfe *WebFrontEndImpl) newNewOrderLimitTransactions(regId int64, names []st
 	}
 	transactions = append(transactions, txn)
 
-	txn, err = wfe.txnBuilder.FailedAuthorizationsPerAccountCheckOnlyTransaction(regId)
+	failedAuthzTxns, err := wfe.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names, wfe.maxNames)
 	if err != nil {
-		logTxnErr(err, ratelimits.FailedAuthorizationsPerAccount)
+		logTxnErr(err, ratelimits.FailedAuthorizationsPerDomainPerAccount)
 		return nil
 	}
-	transactions = append(transactions, txn)
+	transactions = append(transactions, failedAuthzTxns...)
 
-	txns, err := wfe.txnBuilder.CertificatesPerDomainTransactions(regId, names, wfe.maxNames)
+	certsPerDomainTxns, err := wfe.txnBuilder.CertificatesPerDomainTransactions(regId, names, wfe.maxNames)
 	if err != nil {
 		logTxnErr(err, ratelimits.CertificatesPerDomain)
 		return nil
 	}
-	transactions = append(transactions, txns...)
+	transactions = append(transactions, certsPerDomainTxns...)
 
 	txn, err = wfe.txnBuilder.CertificatesPerFQDNSetTransaction(names)
 	if err != nil {