Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WFE: Two changes to NewRegistration key-value rate limits #7258

Merged
merged 4 commits into from
Jan 23, 2024
Merged

WFE: Two changes to NewRegistration key-value rate limits #7258

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e61140e
WFE: Only refund NewRegistration key-value limits after check completion
beautifulentropy Jan 11, 2024
7ad61fd
Address comments.
beautifulentropy Jan 16, 2024
d01ae82
Merge branch 'main' into refund-after-check-completion
beautifulentropy Jan 16, 2024
5f32b90
Use simpler racy approach and adopt the batched spending.
beautifulentropy Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 48 additions & 62 deletions wfe2/wfe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,102 +619,71 @@ func link(url, relation string) string {
return fmt.Sprintf("<%s>;rel=\"%s\"", url, relation)
}

// checkNewAccountLimits checks whether sufficient limit quota exists for the
// creation of a new account from the given IP address. If so, that quota is
// spent. If an error is encountered during the check, it is logged but not
// returned.
//
// 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, ip net.IP) {
func (wfe *WebFrontEndImpl) newNewAccountLimitTransactions(ip net.IP) []ratelimits.Transaction {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
return nil
}

warn := func(err error, limit ratelimits.Name) {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return
}
// 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
return nil
}
transactions = append(transactions, txn)

decision, err := wfe.limiter.Spend(ctx, txn)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPAddress)
return
}
if !decision.Allowed || ip.To4() != nil {
// This requester is being limited or the request was made from an IPv4
// address.
return
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.
//
// 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) {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
}

_, err = wfe.limiter.Spend(ctx, txn)
_, err := wfe.limiter.BatchSpend(ctx, transactions)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPv6Range)
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, ip net.IP) {
func (wfe *WebFrontEndImpl) refundNewAccountLimits(ctx context.Context, transactions []ratelimits.Transaction) {
if wfe.limiter == nil && wfe.txnBuilder == nil {
// Limiter is disabled.
return
}

warn := func(err error, limit ratelimits.Name) {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return
}
// TODO(#5545): Once key-value rate limits are authoritative this log
// line should be removed in favor of returning the error.
wfe.log.Warningf("refunding %s rate limit: %s", limit, err)
}

txn, err := wfe.txnBuilder.RegistrationsPerIPAddressTransaction(ip)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPAddress)
return
}

_, err = wfe.limiter.Refund(ctx, txn)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPAddress)
return
}
if ip.To4() != nil {
// Request was made from an IPv4 address.
return
}

txn, err = wfe.txnBuilder.RegistrationsPerIPv6RangeTransaction(ip)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPv6Range)
return
}

_, err = wfe.limiter.Refund(ctx, txn)
_, err := wfe.limiter.BatchRefund(ctx, transactions)
if err != nil {
warn(err, ratelimits.NewRegistrationsPerIPv6Range)
wfe.log.Errf("refunding newAccount limits: %s", err)
}
}

Expand Down Expand Up @@ -842,17 +811,34 @@ func (wfe *WebFrontEndImpl) NewAccount(

// TODO(#5545): Spending and Refunding can be async until these rate limits
// are authoritative. This saves us from adding latency to each request.
go wfe.checkNewAccountLimits(ctx, ip)
// 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)

var newRegistrationSuccessful bool
var errIsRateLimit bool
defer func() {
if !newRegistrationSuccessful {
go wfe.refundNewAccountLimits(ctx, ip)
if !newRegistrationSuccessful && !errIsRateLimit {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't touch the newRegistrationSuccessful bool in this PR, so feel free to ignore me, but could this become

if err != nil && !errors.Is(err, berrors.RateLimit) {

? Rather than carrying around these two bools, I think the conditional can be computed here from variables we already have access to.

// 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)
}
}()

// Send the registration to the RA via grpc
acctPB, err := wfe.ra.NewRegistration(ctx, &reg)
if err != nil {
if errors.Is(err, berrors.RateLimit) {
// Request was denied by a legacy rate limit. In this error case we
// do not want to refund the quota consumed by the request because
// repeated requests would result in unearned refunds.
//
// TODO(#5545): Once key-value rate limits are authoritative this
// can be removed.
errIsRateLimit = true
beautifulentropy marked this conversation as resolved.
Show resolved Hide resolved
}
if errors.Is(err, berrors.Duplicate) {
existingAcct, err := wfe.sa.GetRegistrationByKey(ctx, &sapb.JSONWebKey{Jwk: keyBytes})
if err == nil {
Expand Down