ratelimits: Fix transaction building for Failed Authorizations Limit

ratelimits/bucket.go

@@ -235,39 +235,77 @@ 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.
+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.
+		txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 1)
+		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
+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)

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

test/integration/ratelimit_test.go

@@ -4,6 +4,7 @@ package integration
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
@@ -65,8 +66,10 @@ func TestDuplicateFQDNRateLimit(t *testing.T) {
 
 		// Check that the CertificatesPerFQDNSet limit is reached.
 		txn, err := txnBuilder.CertificatesPerFQDNSetTransaction([]string{domain})
+		fmt.Printf("\ntxn: %#v\n", txn)
 		test.AssertNotError(t, err, "making transaction")
 		result, err := limiter.Check(context.Background(), txn)
+		fmt.Printf("\nresult: %#v\n", result)
 		test.AssertNotError(t, err, "checking transaction")
 		test.Assert(t, !result.Allowed, "should not be allowed")
 	}

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 {