From 5d9ac4c997162281cbb524fe5c75cfab7234df2c Mon Sep 17 00:00:00 2001 From: Samantha Date: Thu, 29 Feb 2024 16:49:54 -0500 Subject: [PATCH] RA: Count failed authorizations using key-value ratelimits --- cmd/admin-revoker/main_test.go | 2 + cmd/boulder-ra/main.go | 46 ++++++++++++++++++++ cmd/boulder-wfe2/main.go | 7 +++- ra/ra.go | 33 +++++++++++++++ ra/ra_test.go | 76 +++++++++++++++++++++++++++++++++- ratelimits/bucket.go | 2 +- 6 files changed, 162 insertions(+), 4 deletions(-) diff --git a/cmd/admin-revoker/main_test.go b/cmd/admin-revoker/main_test.go index 48a46d66fc38..10e71fd7fb60 100644 --- a/cmd/admin-revoker/main_test.go +++ b/cmd/admin-revoker/main_test.go @@ -476,6 +476,8 @@ func setup(t *testing.T) testCtx { metrics.NoopRegisterer, 1, goodkey.KeyPolicy{}, + nil, + nil, 100, 300*24*time.Hour, 7*24*time.Hour, diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go index c63d7a3a0950..d844fb4a4e50 100644 --- a/cmd/boulder-ra/main.go +++ b/cmd/boulder-ra/main.go @@ -22,6 +22,8 @@ import ( pubpb "github.com/letsencrypt/boulder/publisher/proto" "github.com/letsencrypt/boulder/ra" rapb "github.com/letsencrypt/boulder/ra/proto" + "github.com/letsencrypt/boulder/ratelimits" + bredis "github.com/letsencrypt/boulder/redis" sapb "github.com/letsencrypt/boulder/sa/proto" vapb "github.com/letsencrypt/boulder/va/proto" ) @@ -42,6 +44,33 @@ type Config struct { PublisherService *cmd.GRPCClientConfig AkamaiPurgerService *cmd.GRPCClientConfig + Limiter struct { + // Redis contains the configuration necessary to connect to Redis + // for rate limiting. This field is required to enable rate + // limiting. + Redis *bredis.Config `validate:"required_with=Defaults"` + + // Defaults is a path to a YAML file containing default rate limits. + // See: ratelimits/README.md for details. This field is required to + // enable rate limiting. If any individual rate limit is not set, + // that limit will be disabled. Limits passed in this file must be + // identical to those in the WFE. + // + // Note: At this time, only the Failed Authorizations rate limit is + // necessary in the RA. + Defaults string `validate:"required_with=Redis"` + + // Overrides is a path to a YAML file containing overrides for the + // default rate limits. See: ratelimits/README.md for details. If + // this field is not set, all requesters will be subject to the + // default rate limits. Overrides passed in this file must be + // identical to those in the WFE. + // + // Note: At this time, only the Failed Authorizations overrides are + // necessary in the RA. + Overrides string + } + // MaxNames is the maximum number of subjectAltNames in a single cert. // The value supplied MUST be greater than 0 and no more than 100. These // limits are per section 7.1 of our combined CP/CPS, under "DV-SSL @@ -232,12 +261,29 @@ func main() { cmd.Fail("Error in RA config: MaxNames must not be 0") } + var limiter *ratelimits.Limiter + var txnBuilder *ratelimits.TransactionBuilder + var limiterRedis *bredis.Ring + if c.RA.Limiter.Defaults != "" { + // Setup rate limiting. + limiterRedis, err = bredis.NewRingFromConfig(*c.RA.Limiter.Redis, scope, logger) + cmd.FailOnError(err, "Failed to create Redis ring") + + source := ratelimits.NewRedisSource(limiterRedis.Ring, clk, scope) + limiter, err = ratelimits.NewLimiter(clk, source, scope) + cmd.FailOnError(err, "Failed to create rate limiter") + txnBuilder, err = ratelimits.NewTransactionBuilder(c.RA.Limiter.Defaults, c.RA.Limiter.Overrides) + cmd.FailOnError(err, "Failed to create rate limits transaction builder") + } + rai := ra.NewRegistrationAuthorityImpl( clk, logger, scope, c.RA.MaxContactsPerRegistration, kp, + limiter, + txnBuilder, c.RA.MaxNames, authorizationLifetime, pendingAuthorizationLifetime, diff --git a/cmd/boulder-wfe2/main.go b/cmd/boulder-wfe2/main.go index 3da9928e965f..ae6d7f94b78d 100644 --- a/cmd/boulder-wfe2/main.go +++ b/cmd/boulder-wfe2/main.go @@ -149,13 +149,16 @@ type Config struct { // Defaults is a path to a YAML file containing default rate limits. // See: ratelimits/README.md for details. This field is required to // enable rate limiting. If any individual rate limit is not set, - // that limit will be disabled. + // that limit will be disabled. Failed Authorizations limits passed + // in this file must be identical to those in the RA. Defaults string `validate:"required_with=Redis"` // Overrides is a path to a YAML file containing overrides for the // default rate limits. See: ratelimits/README.md for details. If // this field is not set, all requesters will be subject to the - // default rate limits. + // default rate limits. Overrides for the Failed Authorizations + // overrides passed in this file must be identical to those in the + // RA. Overrides string } diff --git a/ra/ra.go b/ra/ra.go index b046e6c26adb..9929a9bfe2ac 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -96,6 +96,8 @@ type RegistrationAuthorityImpl struct { pendingAuthorizationLifetime time.Duration rlPolicies ratelimit.Limits maxContactsPerReg int + limiter *ratelimits.Limiter + txnBuilder *ratelimits.TransactionBuilder maxNames int orderLifetime time.Duration finalizeTimeout time.Duration @@ -127,6 +129,8 @@ func NewRegistrationAuthorityImpl( stats prometheus.Registerer, maxContactsPerReg int, keyPolicy goodkey.KeyPolicy, + limiter *ratelimits.Limiter, + txnBuilder *ratelimits.TransactionBuilder, maxNames int, authorizationLifetime time.Duration, pendingAuthorizationLifetime time.Duration, @@ -246,6 +250,8 @@ func NewRegistrationAuthorityImpl( rlPolicies: ratelimit.New(), maxContactsPerReg: maxContactsPerReg, keyPolicy: keyPolicy, + limiter: limiter, + txnBuilder: txnBuilder, maxNames: maxNames, publisher: pubc, caa: caaClient, @@ -1756,6 +1762,26 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI return err } +func (ra *RegistrationAuthorityImpl) countFailedValidation(ctx context.Context, regId int64, name string) { + if ra.limiter == nil && ra.txnBuilder == nil { + // Limiter is disabled. + return + } + + txns, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransactions(regId, []string{name}, ra.maxNames) + if err != nil { + ra.log.Errf("constructing rate limit transaction for the %s rate limit: %s", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) + } + + _, err = ra.limiter.BatchSpend(ctx, txns) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return + } + ra.log.Errf("checking the %s limit", ratelimits.FailedAuthorizationsPerDomainPerAccount, err) + } +} + // PerformValidation initiates validation for a specific challenge associated // with the given base authorization. The authorization and challenge are // updated based on the results. @@ -1884,6 +1910,13 @@ func (ra *RegistrationAuthorityImpl) PerformValidation( if prob != nil { challenge.Status = core.StatusInvalid challenge.Error = prob + + // TODO(#5545): Spending can be async until key-value 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. + go ra.countFailedValidation(vaCtx, authz.RegistrationID, authz.Identifier.Value) } else { challenge.Status = core.StatusValid } diff --git a/ra/ra_test.go b/ra/ra_test.go index 101799256c9b..2f272622669a 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -17,6 +17,7 @@ import ( "math/big" mrand "math/rand" "net" + "os" "regexp" "strconv" "strings" @@ -38,6 +39,7 @@ import ( akamaipb "github.com/letsencrypt/boulder/akamai/proto" capb "github.com/letsencrypt/boulder/ca/proto" + "github.com/letsencrypt/boulder/cmd" "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" @@ -56,6 +58,7 @@ import ( rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/ratelimit" "github.com/letsencrypt/boulder/ratelimits" + bredis "github.com/letsencrypt/boulder/redis" "github.com/letsencrypt/boulder/sa" sapb "github.com/letsencrypt/boulder/sa/proto" "github.com/letsencrypt/boulder/test" @@ -365,9 +368,40 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho }, }, nil, nil, 0, log, metrics.NoopRegisterer) + var limiter *ratelimits.Limiter + var txnBuilder *ratelimits.TransactionBuilder + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + 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", + } + 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") + } + ra := NewRegistrationAuthorityImpl( fc, log, stats, - 1, testKeyPolicy, 100, + 1, testKeyPolicy, limiter, txnBuilder, 100, 300*24*time.Hour, 7*24*time.Hour, nil, noopCAA{}, 0, 5*time.Minute, @@ -857,6 +891,19 @@ func TestPerformValidationSuccess(t *testing.T) { Problems: nil, } + var remainingFailedValidations int64 + var rlTxns []ratelimits.Transaction + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + // Gather a baseline for the rate limit. + var err error + rlTxns, err = ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(authzPB.RegistrationID, []string{Identifier}, 100) + test.AssertNotError(t, err, "FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions failed") + + d, err := ra.limiter.BatchSpend(ctx, rlTxns) + test.AssertNotError(t, err, "BatchSpend failed") + remainingFailedValidations = d.Remaining + } + now := fc.Now() challIdx := dnsChallIdx(t, authzPB.Challenges) authzPB, err := ra.PerformValidation(ctx, &rapb.PerformValidationRequest{ @@ -898,6 +945,13 @@ func TestPerformValidationSuccess(t *testing.T) { // Check that validated timestamp was recorded, stored, and retrieved expectedValidated := fc.Now() test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") + + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + // The failed validations bucket should be identical to the baseline. + d, err := ra.limiter.BatchSpend(ctx, rlTxns) + test.AssertNotError(t, err, "BatchSpend failed") + test.AssertEquals(t, d.Remaining, remainingFailedValidations) + } } func TestPerformValidationVAError(t *testing.T) { @@ -906,6 +960,19 @@ func TestPerformValidationVAError(t *testing.T) { authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour)) + var remainingFailedValidations int64 + var rlTxns []ratelimits.Transaction + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + // Gather a baseline for the rate limit. + var err error + rlTxns, err = ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(authzPB.RegistrationID, []string{Identifier}, 100) + test.AssertNotError(t, err, "FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions failed") + + d, err := ra.limiter.BatchSpend(ctx, rlTxns) + test.AssertNotError(t, err, "BatchSpend failed") + remainingFailedValidations = d.Remaining + } + va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong") challIdx := dnsChallIdx(t, authzPB.Challenges) @@ -945,6 +1012,13 @@ func TestPerformValidationVAError(t *testing.T) { // Check that validated timestamp was recorded, stored, and retrieved expectedValidated := fc.Now() test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing") + + if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { + // The failed validations bucket should have been decremented by 1. + d, err := ra.limiter.BatchSpend(ctx, rlTxns) + test.AssertNotError(t, err, "BatchSpend failed") + test.AssertEquals(t, d.Remaining, remainingFailedValidations-1) + } } func TestCertificateKeyNotEqualAccountKey(t *testing.T) { diff --git a/ratelimits/bucket.go b/ratelimits/bucket.go index 75b54fa05a31..fff8d9727d06 100644 --- a/ratelimits/bucket.go +++ b/ratelimits/bucket.go @@ -265,7 +265,7 @@ func (builder *TransactionBuilder) FailedAuthorizationsPerDomainPerAccountCheckO } // Add a check-only transaction for each per domain per account bucket. - txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 1) + txn, err := newCheckOnlyTransaction(limit, perDomainPerAccountBucketKey, 0) if err != nil { return nil, err }