diff --git a/cmd/boulder-wfe2/main.go b/cmd/boulder-wfe2/main.go index 2fc1524bc07..6612e61e0e6 100644 --- a/cmd/boulder-wfe2/main.go +++ b/cmd/boulder-wfe2/main.go @@ -158,6 +158,13 @@ type Config struct { // default rate limits. Overrides string } + + // MaxNames is the maximum number of subjectAltNames in a single cert. + // The value supplied SHOULD be greater than 0 and no more than 100, + // defaults to 100. These limits are per section 7.1 of our combined + // CP/CPS, under "DV-SSL Subscriber Certificate". The value must match + // the CA and RA configurations. + MaxNames int `validate:"min=0,max=100"` } Syslog cmd.SyslogConfig @@ -299,6 +306,11 @@ func main() { if *debugAddr != "" { c.WFE.DebugAddr = *debugAddr } + maxNames := c.WFE.MaxNames + if maxNames == 0 { + // Default to 100 names per cert. + maxNames = 100 + } certChains := map[issuance.IssuerNameID][][]byte{} issuerCerts := map[issuance.IssuerNameID]*issuance.Certificate{} @@ -396,6 +408,7 @@ func main() { accountGetter, limiter, txnBuilder, + maxNames, ) cmd.FailOnError(err, "Unable to create WFE") diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index f4909c50297..deda7ca4276 100644 --- a/ratelimits/bucket.go +++ b/ratelimits/bucket.go @@ -282,7 +282,11 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerAccountTransaction(reg // // When a CertificatesPerDomainPerAccount override is not configured, a check- // and-spend Transaction is returned for each per domain bucket. -func (builder *TransactionBuilder) CertificatesPerDomainTransactions(regId int64, orderDomains []string) ([]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) + } + perAccountLimitBucketKey, err := newRegIdBucketKey(CertificatesPerDomainPerAccount, regId) if err != nil { return nil, err diff --git a/test/config-next/wfe2-ratelimit-defaults.yml b/test/config-next/wfe2-ratelimit-defaults.yml index 70551e9d9df..f10b5966d30 100644 --- a/test/config-next/wfe2-ratelimit-defaults.yml +++ b/test/config-next/wfe2-ratelimit-defaults.yml @@ -1,2 +1,24 @@ -NewRegistrationsPerIPAddress: { burst: 10000, count: 10000, period: 168h } -NewRegistrationsPerIPv6Range: { burst: 99999, count: 99999, period: 168h } +NewRegistrationsPerIPAddress: + count: 10000 + burst: 10000 + period: 168h +NewRegistrationsPerIPv6Range: + count: 99999 + burst: 99999 + period: 168h +CertificatesPerDomain: + count: 2 + burst: 2 + period: 2160h +FailedAuthorizationsPerAccount: + count: 3 + burst: 3 + period: 5m +NewOrdersPerAccount: + count: 1500 + burst: 1500 + period: 3h +CertificatesPerFQDNSet: + count: 6 + burst: 6 + period: 168h diff --git a/test/config-next/wfe2-ratelimit-overrides.yml b/test/config-next/wfe2-ratelimit-overrides.yml index 9d80ffaf464..4f45ba82667 100644 --- a/test/config-next/wfe2-ratelimit-overrides.yml +++ b/test/config-next/wfe2-ratelimit-overrides.yml @@ -2,4 +2,39 @@ burst: 1000000 count: 1000000 period: 168h - ids: [127.0.0.1] + ids: + - 127.0.0.1 +- CertificatesPerDomain: + burst: 1 + count: 1 + period: 2160h + ids: + - ratelimit.me +- CertificatesPerDomain: + burst: 10000 + count: 10000 + period: 2160h + ids: + - le.wtf + - le1.wtf + - le2.wtf + - le3.wtf + - nginx.wtf + - good-caa-reserved.com + - bad-caa-reserved.com + - ecdsa.le.wtf + - must-staple.le.wtf +- CertificatesPerFQDNSet: + burst: 10000 + count: 10000 + period: 168h + ids: + - le.wtf + - le1.wtf + - le2.wtf + - le3.wtf + - le.wtf,le1.wtf + - good-caa-reserved.com + - nginx.wtf + - ecdsa.le.wtf + - must-staple.le.wtf diff --git a/test/integration/ratelimit_test.go b/test/integration/ratelimit_test.go index 67543c713dd..cc5b402928f 100644 --- a/test/integration/ratelimit_test.go +++ b/test/integration/ratelimit_test.go @@ -3,8 +3,17 @@ package integration import ( + "context" + "os" + "strings" "testing" + "github.com/jmhodges/clock" + "github.com/letsencrypt/boulder/cmd" + blog "github.com/letsencrypt/boulder/log" + "github.com/letsencrypt/boulder/metrics" + "github.com/letsencrypt/boulder/ratelimits" + bredis "github.com/letsencrypt/boulder/redis" "github.com/letsencrypt/boulder/test" ) @@ -20,4 +29,45 @@ func TestDuplicateFQDNRateLimit(t *testing.T) { _, err = authAndIssue(nil, nil, []string{domain}, true) test.AssertError(t, err, "Somehow managed to issue third certificate") + + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + // Setup rate limiting. + rc := bredis.Config{ + Username: "unittest-rw", + TLS: cmd.TLSConfig{ + CACertFile: "test/redis-tls/minica.pem", + CertFile: "test/redis-tls/boulder/cert.pem", + KeyFile: "test/redis-tls/boulder/key.pem", + }, + Lookups: []cmd.ServiceDomain{ + { + Service: "redisratelimits", + Domain: "service.consul", + }, + }, + LookupDNSAuthority: "consul.service.consul", + } + rc.PasswordConfig = cmd.PasswordConfig{ + PasswordFile: "test/secrets/ratelimits_redis_password", + } + + fc := clock.NewFake() + stats := metrics.NoopRegisterer + log := blog.NewMock() + ring, err := bredis.NewRingFromConfig(rc, stats, log) + test.AssertNotError(t, err, "making redis ring client") + source := ratelimits.NewRedisSource(ring.Ring, fc, stats) + test.AssertNotNil(t, source, "source should not be nil") + limiter, err := ratelimits.NewLimiter(fc, source, stats) + test.AssertNotError(t, err, "making limiter") + txnBuilder, err := ratelimits.NewTransactionBuilder("test/config-next/wfe2-ratelimit-defaults.yml", "") + test.AssertNotError(t, err, "making transaction composer") + + // Check that the CertificatesPerFQDNSet limit is reached. + txn, err := txnBuilder.CertificatesPerFQDNSetTransaction([]string{domain}) + test.AssertNotError(t, err, "making transaction") + result, err := limiter.Check(context.Background(), txn) + test.AssertNotError(t, err, "checking transaction") + test.Assert(t, !result.Allowed, "should not be allowed") + } } diff --git a/test/rate-limit-policies-b.yml b/test/rate-limit-policies-b.yml deleted file mode 100644 index 2582c3a6ec2..00000000000 --- a/test/rate-limit-policies-b.yml +++ /dev/null @@ -1,54 +0,0 @@ -# See cmd/shell.go for definitions of these rate limits. -certificatesPerName: - window: 2160h - threshold: 99 - overrides: - ratelimit.me: 1 - lim.it: 0 - # Hostnames used by the letsencrypt client integration test. - le.wtf: 9999 - le1.wtf: 9999 - le2.wtf: 9999 - le3.wtf: 9999 - le4.wtf: 9999 - nginx.wtf: 9999 - good-caa-reserved.com: 9999 - bad-caa-reserved.com: 9999 - ecdsa.le.wtf: 9999 - must-staple.le.wtf: 9999 - registrationOverrides: - 101: 1000 -registrationsPerIP: - window: 168h # 1 week - threshold: 9999 - overrides: - 127.0.0.1: 999990 -registrationsPerIPRange: - window: 168h # 1 week - threshold: 99999 - overrides: - 127.0.0.1: 1000000 -pendingAuthorizationsPerAccount: - window: 168h # 1 week, should match pending authorization lifetime. - threshold: 999 -newOrdersPerAccount: - window: 3h - threshold: 9999 -certificatesPerFQDNSet: - window: 168h - threshold: 99999 - overrides: - le.wtf: 9999 - le1.wtf: 9999 - le2.wtf: 9999 - le3.wtf: 9999 - le.wtf,le1.wtf: 9999 - good-caa-reserved.com: 9999 - nginx.wtf: 9999 - ecdsa.le.wtf: 9999 - must-staple.le.wtf: 9999 -certificatesPerFQDNSetFast: - window: 2h - threshold: 20 - overrides: - le.wtf: 9 diff --git a/test/v2_integration.py b/test/v2_integration.py index d20318ecabe..93355e72a28 100644 --- a/test/v2_integration.py +++ b/test/v2_integration.py @@ -1565,6 +1565,11 @@ def test_renewal_exemption(): chisel2.expect_problem("urn:ietf:params:acme:error:rateLimited", lambda: chisel2.auth_and_issue(["mail." + base_domain])) +# TODO(#5545) +# - Phase 2: Once the new rate limits are authoritative in config-next, ensure +# that this test only runs in config. +# - Phase 3: Once the new rate limits are authoritative in config, remove this +# test entirely. def test_certificates_per_name(): chisel2.expect_problem("urn:ietf:params:acme:error:rateLimited", lambda: chisel2.auth_and_issue([random_domain() + ".lim.it"])) diff --git a/wfe2/wfe.go b/wfe2/wfe.go index b300fabc0df..e9b77037666 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -170,6 +170,7 @@ type WebFrontEndImpl struct { pendingAuthorizationLifetime time.Duration limiter *ratelimits.Limiter txnBuilder *ratelimits.TransactionBuilder + maxNames int } // NewWebFrontEndImpl constructs a web service for Boulder @@ -193,6 +194,7 @@ func NewWebFrontEndImpl( accountGetter AccountGetter, limiter *ratelimits.Limiter, txnBuilder *ratelimits.TransactionBuilder, + maxNames int, ) (WebFrontEndImpl, error) { if len(issuerCertificates) == 0 { return WebFrontEndImpl{}, errors.New("must provide at least one issuer certificate") @@ -231,6 +233,7 @@ func NewWebFrontEndImpl( accountGetter: accountGetter, limiter: limiter, txnBuilder: txnBuilder, + maxNames: maxNames, } return wfe, nil @@ -2056,6 +2059,81 @@ func (wfe *WebFrontEndImpl) orderToOrderJSON(request *http.Request, order *corep return respObj } +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.Errf("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) + + txn, err = wfe.txnBuilder.FailedAuthorizationsPerAccountCheckOnlyTransaction(regId) + if err != nil { + logTxnErr(err, ratelimits.FailedAuthorizationsPerAccount) + return nil + } + transactions = append(transactions, txn) + + txns, err := wfe.txnBuilder.CertificatesPerDomainTransactions(regId, names, wfe.maxNames) + if err != nil { + logTxnErr(err, ratelimits.CertificatesPerDomain) + return nil + } + transactions = append(transactions, txns...) + + 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. +// +// 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) { + if wfe.limiter == nil && wfe.txnBuilder == nil { + // Limiter is disabled. + return + } + + _, err := wfe.limiter.BatchSpend(ctx, transactions) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return + } + 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 + } + + _, err := wfe.limiter.BatchRefund(ctx, transactions) + if err != nil { + wfe.log.Errf("refunding newOrder limits: %s", err) + } +} + // NewOrder is used by clients to create a new order object and a set of // authorizations to fulfill for issuance. func (wfe *WebFrontEndImpl) NewOrder( @@ -2130,6 +2208,24 @@ func (wfe *WebFrontEndImpl) NewOrder( logEvent.DNSNames = names + // 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.newNewOrderLimitTransactions(acct.ID, names) + go wfe.checkNewOrderLimits(ctx, txns) + + var newOrderSuccessful bool + var errIsRateLimit bool + defer func() { + 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) + } + }() + order, err := wfe.ra.NewOrder(ctx, &rapb.NewOrderRequest{ RegistrationID: acct.ID, Names: names, @@ -2137,6 +2233,15 @@ func (wfe *WebFrontEndImpl) NewOrder( // TODO(#7153): Check each value via core.IsAnyNilOrZero if err != nil || order == nil || order.Id == 0 || order.RegistrationID == 0 || len(order.Names) == 0 || core.IsAnyNilOrZero(order.Created, order.Expires) { wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Error creating new order"), err) + 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 + } return } logEvent.Created = fmt.Sprintf("%d", order.Id) @@ -2151,6 +2256,7 @@ func (wfe *WebFrontEndImpl) NewOrder( wfe.sendError(response, logEvent, probs.ServerInternal("Error marshaling order"), err) return } + newOrderSuccessful = true } // GetOrder is used to retrieve a existing order object diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 7cd56ac99f3..9ad66dbf082 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -425,7 +425,9 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) { rncKey, mockSA, limiter, - txnBuilder) + txnBuilder, + 100, + ) test.AssertNotError(t, err, "Unable to create WFE") wfe.SubscriberAgreementURL = agreementURL