From 315ab1a54e90ee809d22e3bfe33939d8e57c2bf8 Mon Sep 17 00:00:00 2001 From: Samantha Date: Thu, 15 Aug 2024 15:48:53 -0400 Subject: [PATCH] ratelimits: Check at NewOrder and SpendOnly later --- ra/ra.go | 41 +++++++- ratelimits/bucket.go | 155 ++++++++++++++++++++++------- test/integration/ratelimit_test.go | 2 +- wfe2/wfe.go | 2 +- 4 files changed, 158 insertions(+), 42 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 070ec3d5f1a1..369e18d44575 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1298,6 +1298,38 @@ func (ra *RegistrationAuthorityImpl) issueCertificateOuter( return order, err } +// countCertificateIssued increments the certficates (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.Errf("building rate limit transactions at finalize: %s", err) + } + transactions = append(transactions, txns...) + + txn, err := ra.txnBuilder.CertificatesPerFQDNSetSpendOnlyTransaction(orderDomains) + if err != nil { + ra.log.Errf("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.Errf("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,7 +1861,7 @@ 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.Errf("building rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } _, err = ra.limiter.Spend(ctx, txn) @@ -1832,7 +1869,7 @@ func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, 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.Errf("spending against the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) } } diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 2cc7a75afc60..a8337f12cc4a 100644 --- a/ratelimits/bucket.go +++ b/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) @@ -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,7 +514,7 @@ 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) } @@ -443,14 +522,14 @@ func (builder *TransactionBuilder) NewOrderLimitTransactions(regId int64, names // 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) } diff --git a/test/integration/ratelimit_test.go b/test/integration/ratelimit_test.go index 63bf0b57b0d4..5527f52d2b05 100644 --- a/test/integration/ratelimit_test.go +++ b/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") diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 2f4cfbe8841e..20298f18fa59 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -2036,7 +2036,7 @@ func (wfe *WebFrontEndImpl) checkNewOrderLimits(ctx context.Context, regId int64 return nil } - txns, err := wfe.txnBuilder.NewOrderLimitTransactions(regId, names, wfe.maxNames, isRenewal) + txns, err := wfe.txnBuilder.NewOrderLimitTransactions(regId, names, isRenewal) if err != nil { wfe.log.Errf("building new order limit transactions: %v", err) return nil