Skip to content

Commit

Permalink
WFE: Check NewOrder rate limits (#7201)
Browse files Browse the repository at this point in the history
Add non-blocking checks of New Order limits to the WFE using the new
key-value based rate limits package.

Part of #5545
  • Loading branch information
beautifulentropy authored Jan 27, 2024
1 parent 03152aa commit 97a19b1
Show file tree
Hide file tree
Showing 9 changed files with 242 additions and 59 deletions.
13 changes: 13 additions & 0 deletions cmd/boulder-wfe2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.NameID][][]byte{}
issuerCerts := map[issuance.NameID]*issuance.Certificate{}
Expand Down Expand Up @@ -393,6 +405,7 @@ func main() {
accountGetter,
limiter,
txnBuilder,
maxNames,
)
cmd.FailOnError(err, "Unable to create WFE")

Expand Down
6 changes: 5 additions & 1 deletion ratelimits/bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 24 additions & 2 deletions test/config-next/wfe2-ratelimit-defaults.yml
Original file line number Diff line number Diff line change
@@ -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
37 changes: 36 additions & 1 deletion test/config-next/wfe2-ratelimit-overrides.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
50 changes: 50 additions & 0 deletions test/integration/ratelimit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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")
}
}
54 changes: 0 additions & 54 deletions test/rate-limit-policies-b.yml

This file was deleted.

5 changes: 5 additions & 0 deletions test/v2_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]))
Expand Down
Loading

0 comments on commit 97a19b1

Please sign in to comment.