letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

ratelimits: Move transaction construction out of the WFE (#7557) 

- Shrink the number of public `ratelimits` methods by relocating two
sizeable transaction constructors. Simplify the spend and refund
call-sites in the WFE.
- Spend calls now block instead of being called asynchronously.
This commit is contained in:
Samantha Frank 2024-06-26 11:49:28 -04:00 committed by GitHub
parent 9207669755
commit a38ed99341
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 124 additions and 151 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

84
ratelimits/bucket.go
View File

@ -183,9 +183,9 @@ func NewTransactionBuilder(defaults, overrides string) (*TransactionBuilder, err
return &TransactionBuilder{registry}, nil
}
// RegistrationsPerIPAddressTransaction returns a Transaction for the
// registrationsPerIPAddressTransaction returns a Transaction for the
// NewRegistrationsPerIPAddress limit for the provided IP address.
func (builder *TransactionBuilder) RegistrationsPerIPAddressTransaction(ip net.IP) (Transaction, error) {
func (builder *TransactionBuilder) registrationsPerIPAddressTransaction(ip net.IP) (Transaction, error) {
bucketKey, err := newIPAddressBucketKey(NewRegistrationsPerIPAddress, ip)
if err != nil {
return Transaction{}, err
@ -200,10 +200,10 @@ func (builder *TransactionBuilder) RegistrationsPerIPAddressTransaction(ip net.I
return newTransaction(limit, bucketKey, 1)
}
// RegistrationsPerIPv6RangeTransaction returns a Transaction for the
// registrationsPerIPv6RangeTransaction returns a Transaction for the
// NewRegistrationsPerIPv6Range limit for the /48 IPv6 range which contains the
// provided IPv6 address.
func (builder *TransactionBuilder) RegistrationsPerIPv6RangeTransaction(ip net.IP) (Transaction, error) {
func (builder *TransactionBuilder) registrationsPerIPv6RangeTransaction(ip net.IP) (Transaction, error) {
bucketKey, err := newIPv6RangeCIDRBucketKey(NewRegistrationsPerIPv6Range, ip)
if err != nil {
return Transaction{}, err
@ -218,9 +218,9 @@ func (builder *TransactionBuilder) RegistrationsPerIPv6RangeTransaction(ip net.I
return newTransaction(limit, bucketKey, 1)
}
// OrdersPerAccountTransaction returns a Transaction for the NewOrdersPerAccount
// ordersPerAccountTransaction returns a Transaction for the NewOrdersPerAccount
// limit for the provided ACME registration Id.
func (builder *TransactionBuilder) OrdersPerAccountTransaction(regId int64) (Transaction, error) {
func (builder *TransactionBuilder) ordersPerAccountTransaction(regId int64) (Transaction, error) {
bucketKey, err := newRegIdBucketKey(NewOrdersPerAccount, regId)
if err != nil {
return Transaction{}, err
@ -309,7 +309,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO
return txn, nil
}
// CertificatesPerDomainTransactions returns a slice of Transactions for the
// 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:
@ -324,7 +324,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountSpendO
//
// Precondition: orderDomains must all pass policy.WellFormedDomainNames.
// Precondition: len(orderDomains) < maxNames.
func (builder *TransactionBuilder) CertificatesPerDomainTransactions(regId int64, orderDomains []string, maxNames int) ([]Transaction, error) {
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)
}
@ -396,9 +396,9 @@ func (builder *TransactionBuilder) CertificatesPerDomainTransactions(regId int64
return txns, nil
}
// CertificatesPerFQDNSetTransaction returns a Transaction for the provided
// certificatesPerFQDNSetTransaction returns a Transaction for the provided
// order domain names.
func (builder *TransactionBuilder) CertificatesPerFQDNSetTransaction(orderNames []string) (Transaction, error) {
func (builder *TransactionBuilder) certificatesPerFQDNSetTransaction(orderNames []string) (Transaction, error) {
bucketKey, err := newFQDNSetBucketKey(CertificatesPerFQDNSet, orderNames)
if err != nil {
return Transaction{}, err
@ -412,3 +412,67 @@ func (builder *TransactionBuilder) CertificatesPerFQDNSetTransaction(orderNames
}
return newTransaction(limit, bucketKey, 1)
}
// NewOrderLimitTransactions takes in values from a new-order request and 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) ([]Transaction, error) {
makeTxnError := func(err error, limit Name) error {
return fmt.Errorf("error constructing rate limit transaction for %s rate limit: %w", limit, err)
}
var transactions []Transaction
txn, err := builder.ordersPerAccountTransaction(regId)
if err != nil {
return nil, makeTxnError(err, NewOrdersPerAccount)
}
transactions = append(transactions, txn)
failedAuthzTxns, err := builder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names, maxNames)
if err != nil {
return nil, makeTxnError(err, FailedAuthorizationsPerDomainPerAccount)
}
transactions = append(transactions, failedAuthzTxns...)
certsPerDomainTxns, err := builder.certificatesPerDomainTransactions(regId, names, maxNames)
if err != nil {
return nil, makeTxnError(err, CertificatesPerDomain)
}
transactions = append(transactions, certsPerDomainTxns...)
txn, err = builder.certificatesPerFQDNSetTransaction(names)
if err != nil {
return nil, makeTxnError(err, CertificatesPerFQDNSet)
}
return append(transactions, txn), nil
}
// NewAccountLimitTransactions takes in an IP address from a new-account request
// and returns the set of rate limit transactions that should be evaluated
// before allowing the request to proceed.
func (builder *TransactionBuilder) NewAccountLimitTransactions(ip net.IP) ([]Transaction, error) {
makeTxnError := func(err error, limit Name) error {
return fmt.Errorf("error constructing rate limit transaction for %s rate limit: %w", limit, err)
}
var transactions []Transaction
txn, err := builder.registrationsPerIPAddressTransaction(ip)
if err != nil {
return nil, makeTxnError(err, NewRegistrationsPerIPAddress)
}
transactions = append(transactions, txn)
if ip.To4() != nil {
// This request was made from an IPv4 address.
return transactions, nil
}
txn, err = builder.registrationsPerIPv6RangeTransaction(ip)
if err != nil {
return nil, makeTxnError(err, NewRegistrationsPerIPv6Range)
}
return append(transactions, txn), nil
}

4
test/integration/ratelimit_test.go
View File

@ -65,9 +65,9 @@ func TestDuplicateFQDNRateLimit(t *testing.T) {
test.AssertNotError(t, err, "making transaction composer")
// Check that the CertificatesPerFQDNSet limit is reached.
txn, err := txnBuilder.CertificatesPerFQDNSetTransaction([]string{domain})
txns, err := txnBuilder.NewOrderLimitTransactions(1, []string{domain}, 100)
test.AssertNotError(t, err, "making transaction")
result, err := limiter.Check(context.Background(), txn)
result, err := limiter.BatchSpend(context.Background(), txns)
test.AssertNotError(t, err, "checking transaction")
test.Assert(t, !result.Allowed, "should not be allowed")
}

187
wfe2/wfe.go
View File

@ -620,71 +620,41 @@ func link(url, relation string) string {
return fmt.Sprintf("<%s>;rel=\"%s\"", url, relation)
}
func (wfe *WebFrontEndImpl) newNewAccountLimitTransactions(ip net.IP) []ratelimits.Transaction {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return nil
}
warn := func(err error, limit ratelimits.Name) {
// TODO(#5545): Once key-value rate limits are authoritative this log
// line should be removed in favor of returning the error.
wfe.log.Warningf("checking %s rate limit: %s", limit, err)
}
var transactions []ratelimits.Transaction
txn, err := wfe.txnBuilder.RegistrationsPerIPAddressTransaction(ip)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPAddress)
return nil
}
transactions = append(transactions, txn)
if ip.To4() != nil {
// This request was made from an IPv4 address.
return transactions
}
txn, err = wfe.txnBuilder.RegistrationsPerIPv6RangeTransaction(ip)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPv6Range)
return nil
}
return append(transactions, txn)
}
// checkNewAccountLimits checks whether sufficient limit quota exists for the
// creation of a new account. If so, that quota is spent. If an error is
// encountered during the check, it is logged but not returned.
// encountered during the check, it is logged but not returned. A refund
// function is returned that can be called to refund the quota if the account
// creation fails, the func will be nil if any error was encountered during the
// check.
//
// TODO(#5545): For now we're simply exercising the new rate limiter codepath.
// This should eventually return a berrors.RateLimit error containing the retry
// after duration among other information available in the ratelimits.Decision.
func (wfe *WebFrontEndImpl) checkNewAccountLimits(ctx context.Context, transactions []ratelimits.Transaction) {
func (wfe *WebFrontEndImpl) checkNewAccountLimits(ctx context.Context, ip net.IP) func() {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
// Key-value rate limiting is disabled.
return nil
}
_, err := wfe.limiter.BatchSpend(ctx, transactions)
txns, err := wfe.txnBuilder.NewAccountLimitTransactions(ip)
if err != nil {
// TODO(#5545): Once key-value rate limits are authoritative this
// log line should be removed in favor of returning the error.
wfe.log.Infof("building new account limit transactions: %v", err)
return nil
}
_, err = wfe.limiter.BatchSpend(ctx, txns)
if err != nil {
wfe.log.Errf("checking newAccount limits: %s", err)
}
}
// refundNewAccountLimits is typically called when a new account creation fails.
// It refunds the limit quota consumed by the request, allowing the caller to
// retry immediately. If an error is encountered during the refund, it is logged
// but not returned.
func (wfe *WebFrontEndImpl) refundNewAccountLimits(ctx context.Context, transactions []ratelimits.Transaction) {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
return nil
}
_, err := wfe.limiter.BatchRefund(ctx, transactions)
if err != nil {
wfe.log.Errf("refunding newAccount limits: %s", err)
return func() {
_, err := wfe.limiter.BatchRefund(ctx, txns)
if err != nil {
wfe.log.Errf("refunding newAccount limits: %s", err)
}
}
}
@ -810,21 +780,13 @@ func (wfe *WebFrontEndImpl) NewAccount(
InitialIP: ipBytes,
}
// TODO(#5545): Spending and Refunding can be async until these rate limits
// are authoritative. This saves us from adding latency to each request.
// Goroutines spun out below will respect a context deadline set by the
// ratelimits package and cannot be prematurely canceled by the requester.
txns := wfe.newNewAccountLimitTransactions(ip)
go wfe.checkNewAccountLimits(ctx, txns)
refundLimits := wfe.checkNewAccountLimits(ctx, ip)
var newRegistrationSuccessful bool
var errIsRateLimit bool
defer func() {
if !newRegistrationSuccessful && !errIsRateLimit {
// This can be a little racy, but we're not going to worry about it
// for now. If the check hasn't completed yet, we can pretty safely
// assume that the refund will be similarly delayed.
go wfe.refundNewAccountLimits(ctx, txns)
if !newRegistrationSuccessful && !errIsRateLimit && refundLimits != nil {
go refundLimits()
}
}()
@ -2045,83 +2007,41 @@ func (wfe *WebFrontEndImpl) orderToOrderJSON(request *http.Request, order *corep
return respObj
}
// newNewOrderLimitTransactions constructs a set of rate limit transactions to
// evaluate for a new-order request.
//
// Precondition: names must be a list of DNS names that all pass
// policy.WellFormedDomainNames.
func (wfe *WebFrontEndImpl) newNewOrderLimitTransactions(regId int64, names []string) []ratelimits.Transaction {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return nil
}
logTxnErr := func(err error, limit ratelimits.Name) {
// TODO(#5545): Once key-value rate limits are authoritative this log
// line should be removed in favor of returning the error.
wfe.log.Infof("error constructing rate limit transaction for %s rate limit: %s", limit, err)
}
var transactions []ratelimits.Transaction
txn, err := wfe.txnBuilder.OrdersPerAccountTransaction(regId)
if err != nil {
logTxnErr(err, ratelimits.NewOrdersPerAccount)
return nil
}
transactions = append(transactions, txn)
failedAuthzTxns, err := wfe.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(regId, names, wfe.maxNames)
if err != nil {
logTxnErr(err, ratelimits.FailedAuthorizationsPerDomainPerAccount)
return nil
}
transactions = append(transactions, failedAuthzTxns...)
certsPerDomainTxns, err := wfe.txnBuilder.CertificatesPerDomainTransactions(regId, names, wfe.maxNames)
if err != nil {
logTxnErr(err, ratelimits.CertificatesPerDomain)
return nil
}
transactions = append(transactions, certsPerDomainTxns...)
txn, err = wfe.txnBuilder.CertificatesPerFQDNSetTransaction(names)
if err != nil {
logTxnErr(err, ratelimits.CertificatesPerFQDNSet)
return nil
}
return append(transactions, txn)
}
// checkNewOrderLimits checks whether sufficient limit quota exists for the
// creation of a new order. If so, that quota is spent. If an error is
// encountered during the check, it is logged but not returned.
// encountered during the check, it is logged but not returned. A refund
// function is returned that can be used to refund the quota if the order is not
// created, the func will be nil if any error was encountered during the check.
//
// TODO(#5545): For now we're simply exercising the new rate limiter codepath.
// This should eventually return a berrors.RateLimit error containing the retry
// after duration among other information available in the ratelimits.Decision.
func (wfe *WebFrontEndImpl) checkNewOrderLimits(ctx context.Context, transactions []ratelimits.Transaction) {
func (wfe *WebFrontEndImpl) checkNewOrderLimits(ctx context.Context, regId int64, names []string) func() {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
// Key-value rate limiting is disabled.
return nil
}
_, err := wfe.limiter.BatchSpend(ctx, transactions)
txns, err := wfe.txnBuilder.NewOrderLimitTransactions(regId, names, wfe.maxNames)
if err != nil {
wfe.log.Errf("building new order limit transactions: %v", err)
return nil
}
_, err = wfe.limiter.BatchSpend(ctx, txns)
if err != nil {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return
return nil
}
wfe.log.Errf("checking newOrder limits: %s", err)
}
}
func (wfe *WebFrontEndImpl) refundNewOrderLimits(ctx context.Context, transactions []ratelimits.Transaction) {
if wfe.limiter == nil || wfe.txnBuilder == nil {
return
return nil
}
_, err := wfe.limiter.BatchRefund(ctx, transactions)
if err != nil {
wfe.log.Errf("refunding newOrder limits: %s", err)
return func() {
_, err := wfe.limiter.BatchRefund(ctx, txns)
if err != nil {
wfe.log.Errf("refunding newOrder limits: %s", err)
}
}
}
@ -2367,15 +2287,7 @@ func (wfe *WebFrontEndImpl) NewOrder(
return
}
// TODO(#5545): Spending and Refunding can be async until these rate limits
// are authoritative. This saves us from adding latency to each request.
// Goroutines spun out below will respect a context deadline set by the
// ratelimits package and cannot be prematurely canceled by the requester.
var txns []ratelimits.Transaction
if !limitsExempt {
txns = wfe.newNewOrderLimitTransactions(acct.ID, names)
go wfe.checkNewOrderLimits(ctx, txns)
}
refundLimits := wfe.checkNewOrderLimits(ctx, acct.ID, names)
var newOrderSuccessful bool
var errIsRateLimit bool
@ -2387,11 +2299,8 @@ func (wfe *WebFrontEndImpl) NewOrder(
}).Inc()
}
if !newOrderSuccessful && !errIsRateLimit {
// This can be a little racy, but we're not going to worry about it
// for now. If the check hasn't completed yet, we can pretty safely
// assume that the refund will be similarly delayed.
go wfe.refundNewOrderLimits(ctx, txns)
if !newOrderSuccessful && !errIsRateLimit && refundLimits != nil {
go refundLimits()
}
}()