diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 710e90e1d..5eb7a13b9 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -28,11 +28,12 @@ func _() { _ = x[ServeRenewalInfo-17] _ = x[GetAuthzReadOnly-18] _ = x[GetAuthzUseIndex-19] + _ = x[CheckFailedAuthorizationsFirst-20] } -const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationV1DisableNewValidationsStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndex" +const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationV1DisableNewValidationsStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndexCheckFailedAuthorizationsFirst" -var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 125, 138, 152, 170, 188, 207, 230, 246, 265, 289, 300, 316, 332, 348} +var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 125, 138, 152, 170, 188, 207, 230, 246, 265, 289, 300, 316, 332, 348, 378} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index 1a7e40e99..b2fbb4ade 100644 --- a/features/features.go +++ b/features/features.go @@ -59,30 +59,33 @@ const ( // GetAuthzUseIndex causes the SA to use to add a USE INDEX hint when it // queries the authz2 table. GetAuthzUseIndex + // Check the failed authorization limit before doing authz reuse. + CheckFailedAuthorizationsFirst ) // List of features and their default value, protected by fMu var features = map[FeatureFlag]bool{ - unused: false, - CAAValidationMethods: false, - CAAAccountURI: false, - EnforceMultiVA: false, - MultiVAFullResults: false, - MandatoryPOSTAsGET: false, - AllowV1Registration: true, - V1DisableNewValidations: false, - PrecertificateRevocation: false, - StripDefaultSchemePort: false, - StoreIssuerInfo: false, - StoreRevokerInfo: false, - RestrictRSAKeySizes: false, - FasterNewOrdersRateLimit: false, - NonCFSSLSigner: false, - ECDSAForAll: false, - StreamlineOrderAndAuthzs: false, - ServeRenewalInfo: false, - GetAuthzReadOnly: false, - GetAuthzUseIndex: false, + unused: false, + CAAValidationMethods: false, + CAAAccountURI: false, + EnforceMultiVA: false, + MultiVAFullResults: false, + MandatoryPOSTAsGET: false, + AllowV1Registration: true, + V1DisableNewValidations: false, + PrecertificateRevocation: false, + StripDefaultSchemePort: false, + StoreIssuerInfo: false, + StoreRevokerInfo: false, + RestrictRSAKeySizes: false, + FasterNewOrdersRateLimit: false, + NonCFSSLSigner: false, + ECDSAForAll: false, + StreamlineOrderAndAuthzs: false, + ServeRenewalInfo: false, + GetAuthzReadOnly: false, + GetAuthzUseIndex: false, + CheckFailedAuthorizationsFirst: false, } var fMu = new(sync.RWMutex) diff --git a/ra/mock_test.go b/ra/mock_test.go index a23eb840d..1ca8533cf 100644 --- a/ra/mock_test.go +++ b/ra/mock_test.go @@ -2,7 +2,9 @@ package ra import ( "context" + "time" + corepb "github.com/letsencrypt/boulder/core/proto" "github.com/letsencrypt/boulder/mocks" sapb "github.com/letsencrypt/boulder/sa/proto" grpc "google.golang.org/grpc" @@ -30,3 +32,25 @@ func (sa *mockInvalidAuthorizationsAuthority) CountInvalidAuthorizations2(ctx co return &sapb.Count{}, nil } } + +// An authority that returns nonzero failures for CountInvalidAuthorizations2, +// and also returns existing authzs for the same domain from GetAuthorizations2 +type mockInvalidPlusValidAuthzAuthority struct { + mockInvalidAuthorizationsAuthority +} + +func (sa *mockInvalidPlusValidAuthzAuthority) GetAuthorizations2(ctx context.Context, req *sapb.GetAuthorizationsRequest, _ ...grpc.CallOption) (*sapb.Authorizations, error) { + return &sapb.Authorizations{ + Authz: []*sapb.Authorizations_MapElement{ + { + Domain: sa.domainWithFailures, Authz: &corepb.Authorization{ + Id: "1234", + Status: "valid", + Identifier: sa.domainWithFailures, + RegistrationID: 1234, + Expires: time.Date(2101, 12, 3, 0, 0, 0, 0, time.UTC).Unix(), + }, + }, + }, + }, nil +} diff --git a/ra/ra.go b/ra/ra.go index c78578bd5..8f0deaac4 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -2165,6 +2165,12 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New if err := ra.checkLimits(ctx, newOrder.Names, newOrder.RegistrationID); err != nil { return nil, err } + if features.Enabled(features.CheckFailedAuthorizationsFirst) { + err := ra.checkInvalidAuthorizationLimits(ctx, newOrder.RegistrationID, newOrder.Names) + if err != nil { + return nil, err + } + } // An order's lifetime is effectively bound by the shortest remaining lifetime // of its associated authorizations. For that reason it would be Uncool if @@ -2241,11 +2247,15 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New // If the order isn't fully authorized we need to check that the client has // rate limit room for more pending authorizations if len(missingAuthzNames) > 0 { - if err := ra.checkPendingAuthorizationLimit(ctx, newOrder.RegistrationID); err != nil { + err := ra.checkPendingAuthorizationLimit(ctx, newOrder.RegistrationID) + if err != nil { return nil, err } - if err := ra.checkInvalidAuthorizationLimits(ctx, newOrder.RegistrationID, missingAuthzNames); err != nil { - return nil, err + if !features.Enabled(features.CheckFailedAuthorizationsFirst) { + err := ra.checkInvalidAuthorizationLimits(ctx, newOrder.RegistrationID, missingAuthzNames) + if err != nil { + return nil, err + } } } diff --git a/ra/ra_test.go b/ra/ra_test.go index 00326d6e3..c6608f905 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -2470,6 +2470,46 @@ func TestNewOrderReuseInvalidAuthz(t *testing.T) { test.AssertNotEquals(t, secondOrder.V2Authorizations[0], order.V2Authorizations[0]) } +// Test that the failed authorizations limit is checked before authz reuse. +func TestNewOrderCheckFailedAuthorizationsFirst(t *testing.T) { + _, _, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + + _ = features.Set(map[string]bool{"CheckFailedAuthorizationsFirst": true}) + defer features.Reset() + + // Create an order (and thus a pending authz) for example.com + ctx := context.Background() + order, err := ra.NewOrder(ctx, &rapb.NewOrderRequest{ + RegistrationID: Registration.Id, + Names: []string{"example.com"}, + }) + test.AssertNotError(t, err, "adding an initial order for regA") + test.AssertNotNil(t, order.Id, "initial order had a nil ID") + test.AssertEquals(t, numAuthorizations(order), 1) + + // Now treat example.com as if it had a recent failure. + ra.SA = &mockInvalidPlusValidAuthzAuthority{mockInvalidAuthorizationsAuthority{domainWithFailures: "example.com"}} + // Set a very restrictive police for invalid authorizations - one failure + // and you're done for a day. + ra.rlPolicies = &dummyRateLimitConfig{ + InvalidAuthorizationsPerAccountPolicy: ratelimit.RateLimitPolicy{ + Threshold: 1, + Window: cmd.ConfigDuration{Duration: 24 * time.Hour}, + }, + } + + // Creating an order for example.com should error with the "too many failed + // authorizations recently" error. + _, err = ra.NewOrder(ctx, &rapb.NewOrderRequest{ + RegistrationID: Registration.Id, + Names: []string{"example.com"}, + }) + + test.AssertError(t, err, "expected error for domain with too many failures") + test.AssertEquals(t, err.Error(), "too many failed authorizations recently: see https://letsencrypt.org/docs/rate-limits/") +} + // mockSAUnsafeAuthzReuse has a GetAuthorizations implementation that returns // an HTTP-01 validated wildcard authz. type mockSAUnsafeAuthzReuse struct {