ra: check failed authorizations limit before attempting authz reuse (#5827)

Part of #5826

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) {

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)

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
+}

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
+			}
 		}
 	}
 

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 {