sfe: Add per IP rate limits to the overrides request form submissions (#8366)

Author: beautifulentropy

cmd/sfe/main.go

@@ -13,6 +13,8 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	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"
 	"github.com/letsencrypt/boulder/sfe"
 	"github.com/letsencrypt/boulder/sfe/zendesk"
@@ -60,6 +62,20 @@ type Config struct {
 				IPAddress        int64 `validate:"required"`
 			} `validate:"required,dive"`
 		} `validate:"omitempty,dive"`
+
+		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. Failed Authorizations limits passed
+			// in this file must be identical to those in the RA.
+			Defaults string `validate:"required_with=Redis"`
+		}
 		Features features.Config
 	}
 
@@ -139,6 +155,20 @@ func main() {
 		}
 	}
 
+	var limiter *ratelimits.Limiter
+	var txnBuilder *ratelimits.TransactionBuilder
+	var limiterRedis *bredis.Ring
+	if c.SFE.Limiter.Defaults != "" {
+		limiterRedis, err = bredis.NewRingFromConfig(*c.SFE.Limiter.Redis, stats, logger)
+		cmd.FailOnError(err, "Failed to create Redis ring")
+
+		source := ratelimits.NewRedisSource(limiterRedis.Ring, clk, stats)
+		limiter, err = ratelimits.NewLimiter(clk, source, stats)
+		cmd.FailOnError(err, "Failed to create rate limiter")
+		txnBuilder, err = ratelimits.NewTransactionBuilderFromFiles(c.SFE.Limiter.Defaults, "")
+		cmd.FailOnError(err, "Failed to create rate limits transaction builder")
+	}
+
 	sfei, err := sfe.NewSelfServiceFrontEndImpl(
 		stats,
 		clk,
@@ -148,6 +178,8 @@ func main() {
 		sac,
 		unpauseHMACKey,
 		zendeskClient,
+		limiter,
+		txnBuilder,
 	)
 	cmd.FailOnError(err, "Unable to create SFE")
 

errors/errors.go

@@ -258,6 +258,14 @@ func FailedAuthorizationsPerDomainPerAccountError(retryAfter time.Duration, msg
 	}
 }
 
+func LimitOverrideRequestsPerIPAddressError(retryAfter time.Duration, msg string, args ...any) error {
+	return &BoulderError{
+		Type:       RateLimit,
+		Detail:     fmt.Sprintf(msg+": see https://letsencrypt.org/docs/rate-limits/#limit-override-requests-per-ip-address", args...),
+		RetryAfter: retryAfter,
+	}
+}
+
 func RejectedIdentifierError(msg string, args ...any) error {
 	return newf(RejectedIdentifier, msg, args...)
 }

ratelimits/limiter.go

@@ -172,6 +172,15 @@ func (d *Decision) Result(now time.Time) error {
 			retryAfterTs,
 		)
 
+	case LimitOverrideRequestsPerIPAddress:
+		return berrors.LimitOverrideRequestsPerIPAddressError(
+			retryAfter,
+			"too many override request form submissions (%d) from this IP address in the last %s, retry after %s",
+			d.transaction.limit.Burst,
+			d.transaction.limit.Period.Duration,
+			retryAfterTs,
+		)
+
 	default:
 		return berrors.InternalServerError("cannot generate error for unknown rate limit")
 	}

ratelimits/limiter_test.go

@@ -557,6 +557,22 @@ func TestRateLimitError(t *testing.T) {
 			expectedErr:     "too many certificates (3) already issued for \"example.net\" in the last 1h0m0s, retry after 1970-01-01 00:00:20 UTC: see https://letsencrypt.org/docs/rate-limits/#new-certificates-per-registered-domain",
 			expectedErrType: berrors.RateLimit,
 		},
+		{
+			name: "LimitOverrideRequestsPerIPAddress limit reached",
+			decision: &Decision{
+				allowed: false,
+				retryIn: 20 * time.Second,
+				transaction: Transaction{
+					limit: &Limit{
+						Name:   LimitOverrideRequestsPerIPAddress,
+						Burst:  3,
+						Period: config.Duration{Duration: time.Hour},
+					},
+				},
+			},
+			expectedErr:     "too many override request form submissions (3) from this IP address in the last 1h0m0s, retry after 1970-01-01 00:00:20 UTC: see https://letsencrypt.org/docs/rate-limits/#limit-override-requests-per-ip-address",
+			expectedErrType: berrors.RateLimit,
+		},
 		{
 			name: "Unknown rate limit name",
 			decision: &Decision{

ratelimits/names.go

@@ -18,8 +18,8 @@ import (
 // IMPORTANT: If you add or remove a limit Name, you MUST update:
 //   - the string representation of the Name in nameToString,
 //   - the validators for that name in validateIdForName(),
-//   - the transaction constructors for that name in bucket.go
-//   - the Subscriber facing error message in ErrForDecision(), and
+//   - the transaction constructors for that name in transaction.go
+//   - the Subscriber facing error message in Decision.Result(), and
 //   - the case in BuildBucketKey() for that name.
 type Name int
 
@@ -99,6 +99,11 @@ const (
 	//    the account and identValue is the value of an identifier in the
 	//    certificate.
 	FailedAuthorizationsForPausingPerDomainPerAccount
+
+	// LimitOverrideRequestsPerIPAddress is used to limit the number of requests
+	// to the rate limit override request endpoint per IP address. It uses
+	// bucket key 'enum:ipAddress'.
+	LimitOverrideRequestsPerIPAddress
 )
 
 // nameToString is a map of Name values to string names.
@@ -112,6 +117,7 @@ var nameToString = map[Name]string{
 	CertificatesPerDomainPerAccount:                   "CertificatesPerDomainPerAccount",
 	CertificatesPerFQDNSet:                            "CertificatesPerFQDNSet",
 	FailedAuthorizationsForPausingPerDomainPerAccount: "FailedAuthorizationsForPausingPerDomainPerAccount",
+	LimitOverrideRequestsPerIPAddress:                 "LimitOverrideRequestsPerIPAddress",
 }
 
 // isValid returns true if the Name is a valid rate limit name.
@@ -278,7 +284,7 @@ func validateFQDNSet(id string) error {
 
 func validateIdForName(name Name, id string) error {
 	switch name {
-	case NewRegistrationsPerIPAddress:
+	case NewRegistrationsPerIPAddress, LimitOverrideRequestsPerIPAddress:
 		// 'enum:ipaddress'
 		return validIPAddress(id)
 
@@ -361,7 +367,7 @@ func BuildBucketKey(name Name, regId int64, singleIdent identifier.ACMEIdentifie
 	}
 
 	switch name {
-	case NewRegistrationsPerIPAddress:
+	case NewRegistrationsPerIPAddress, LimitOverrideRequestsPerIPAddress:
 		if !subscriberIP.IsValid() {
 			return "", makeMissingErr("subscriberIP")
 		}

ratelimits/transaction.go

@@ -572,3 +572,18 @@ func (builder *TransactionBuilder) NewAccountLimitTransactions(ip netip.Addr) ([
 	}
 	return append(transactions, txn), nil
 }
+
+// LimitOverrideRequestsPerIPAddressTransaction returns a Transaction for the
+// LimitOverrideRequestsPerIPAddress limit for the provided IP address. This
+// limit is used to rate limit requests to the SFE override request endpoint.
+func (builder *TransactionBuilder) LimitOverrideRequestsPerIPAddressTransaction(ip netip.Addr) (Transaction, error) {
+	bucketKey := newIPAddressBucketKey(LimitOverrideRequestsPerIPAddress, ip)
+	limit, err := builder.getLimit(LimitOverrideRequestsPerIPAddress, bucketKey)
+	if err != nil {
+		if errors.Is(err, errLimitDisabled) {
+			return newAllowOnlyTransaction(), nil
+		}
+		return Transaction{}, err
+	}
+	return newTransaction(limit, bucketKey, 1)
+}

sfe/overrides.go

@@ -2,6 +2,7 @@ package sfe
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"html/template"
 	"net/http"
@@ -10,11 +11,13 @@ import (
 	"strconv"
 	"strings"
 
+	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/iana"
 	"github.com/letsencrypt/boulder/policy"
 	rl "github.com/letsencrypt/boulder/ratelimits"
 	"github.com/letsencrypt/boulder/sfe/forms"
 	"github.com/letsencrypt/boulder/sfe/zendesk"
+	"github.com/letsencrypt/boulder/web"
 )
 
 const (
@@ -556,7 +559,54 @@ type overrideRequest struct {
 // either the form logic is flawed or the requester has bypassed the form and
 // submitting (malformed) requests directly to this endpoint.
 func (sfe *SelfServiceFrontEndImpl) submitOverrideRequestHandler(w http.ResponseWriter, r *http.Request) {
-	// TODO(#8359): Check per-IP rate limits for this endpoint.
+	var refundLimits func()
+	var submissionSuccess bool
+	if sfe.limiter != nil && sfe.txnBuilder != nil {
+		requesterIP, err := web.ExtractRequesterIP(r)
+		if err != nil {
+			sfe.log.Errf("determining requester IP address: %s", err)
+			http.Error(w, "failed to determine the IP address of the requester", http.StatusInternalServerError)
+			return
+		}
+
+		txns, err := sfe.txnBuilder.LimitOverrideRequestsPerIPAddressTransaction(requesterIP)
+		if err != nil {
+			sfe.log.Errf("building transaction for override request form limits: %s", err)
+			http.Error(w, "failed to build transaction for override request form limits", http.StatusInternalServerError)
+			return
+		}
+
+		d, err := sfe.limiter.Spend(r.Context(), txns)
+		if err != nil {
+			sfe.log.Errf("spending transaction for override request form limits: %s", err)
+			http.Error(w, "failed to spend transaction for override request form limits", http.StatusInternalServerError)
+			return
+		}
+
+		err = d.Result(sfe.clk.Now())
+		if err != nil {
+			var bErr *berrors.BoulderError
+			if errors.As(err, &bErr) && bErr.Type == berrors.RateLimit {
+				http.Error(w, bErr.Detail, http.StatusTooManyRequests)
+				return
+			}
+			sfe.log.Errf("determining result of override request form limits transaction: %s", err)
+			http.Error(w, "failed to determine result of override request form limits transaction", http.StatusInternalServerError)
+			return
+		}
+
+		refundLimits = func() {
+			_, err := sfe.limiter.Refund(r.Context(), txns)
+			if err != nil {
+				sfe.log.Errf("refunding transaction for override request form limits: %s", err)
+			}
+		}
+	}
+	defer func() {
+		if !submissionSuccess && refundLimits != nil {
+			refundLimits()
+		}
+	}()
 
 	var req overrideRequest
 	err := json.NewDecoder(http.MaxBytesReader(w, r.Body, submitOverrideRequestBodyLimit)).Decode(&req)
@@ -710,5 +760,6 @@ func (sfe *SelfServiceFrontEndImpl) submitOverrideRequestHandler(w http.Response
 	// TODO(#8362): If FundraisingFieldName value is true, use the Salesforce
 	// API to create a new Lead record with the provided information.
 
+	submissionSuccess = true
 	w.WriteHeader(http.StatusOK)
 }

sfe/overrides_test.go

@@ -411,3 +411,49 @@ func TestValidateOverrideRequestField(t *testing.T) {
 		})
 	}
 }
+
+func TestSubmitOverrideRequestHandlerRateLimited(t *testing.T) {
+	t.Parallel()
+
+	sfe, _ := setupSFE(t)
+	sfe.templatePages = minimalTemplates(t)
+	client := createFakeZendeskClientServer(t)
+	sfe.zendeskClient = client
+
+	for attempt := range 101 {
+		reqObj := overrideRequest{
+			RateLimit: rl.CertificatesPerDomainPerAccount.String(),
+			Fields: map[string]string{
+				subscriberAgreementFieldName: "true",
+				privacyPolicyFieldName:       "true",
+				mailingListFieldName:         "false",
+				fundraisingFieldName:         FundraisingOptions[0],
+				emailAddressFieldName:        "[email protected]",
+				OrganizationFieldName:        "Big Host Inc.",
+				useCaseFieldName:             strings.Repeat("x", 60),
+				TierFieldName:                certificatesPerDomainPerAccountTierOptions[0],
+				AccountURIFieldName:          "https://acme-v02.api.letsencrypt.org/acme/acct/67890",
+			},
+		}
+		body, err := json.Marshal(reqObj)
+		if err != nil {
+			t.Errorf("Unexpected failure to marshal JSON overrideRequest: %s", err)
+		}
+		rec := httptest.NewRecorder()
+		req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body))
+
+		sfe.submitOverrideRequestHandler(rec, req)
+		if attempt < 100 {
+			if rec.Code != http.StatusOK {
+				t.Errorf("Unexpected status=%d, expected status=200", rec.Code)
+			}
+		} else {
+			if rec.Code != http.StatusTooManyRequests {
+				t.Errorf("Unexpected status=%d, expected status=429", rec.Code)
+			}
+			if !strings.Contains(rec.Body.String(), "too many override request form submissions (100)") {
+				t.Errorf("Expected rate limit error message, got: %s", rec.Body.String())
+			}
+		}
+	}
+}

sfe/sfe.go

@@ -67,6 +67,9 @@ type SelfServiceFrontEndImpl struct {
 
 	templatePages *template.Template
 	cop           *http.CrossOriginProtection
+
+	limiter    *rl.Limiter
+	txnBuilder *rl.TransactionBuilder
 }
 
 // NewSelfServiceFrontEndImpl constructs a web service for Boulder
@@ -79,6 +82,8 @@ func NewSelfServiceFrontEndImpl(
 	sac sapb.StorageAuthorityReadOnlyClient,
 	unpauseHMACKey []byte,
 	zendeskClient *zendesk.Client,
+	limiter *rl.Limiter,
+	txnBuilder *rl.TransactionBuilder,
 ) (SelfServiceFrontEndImpl, error) {
 
 	// Parse the files once at startup to avoid each request causing the server
@@ -99,6 +104,8 @@ func NewSelfServiceFrontEndImpl(
 		zendeskClient:  zendeskClient,
 		templatePages:  tmplPages,
 		cop:            http.NewCrossOriginProtection(),
+		limiter:        limiter,
+		txnBuilder:     txnBuilder,
 	}
 
 	return sfe, nil

sfe/sfe_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/mocks"
 	"github.com/letsencrypt/boulder/must"
+	"github.com/letsencrypt/boulder/ratelimits"
 	"github.com/letsencrypt/boulder/test"
 	"github.com/letsencrypt/boulder/unpause"
 
@@ -51,6 +52,11 @@ func setupSFE(t *testing.T) (SelfServiceFrontEndImpl, clock.FakeClock) {
 	key, err := hmacKey.Load()
 	test.AssertNotError(t, err, "Unable to load HMAC key")
 
+	limiter, err := ratelimits.NewLimiter(fc, ratelimits.NewInmemSource(), stats)
+	test.AssertNotError(t, err, "making limiter")
+	txnBuilder, err := ratelimits.NewTransactionBuilderFromFiles("../test/config-next/sfe-ratelimit-defaults.yml", "")
+	test.AssertNotError(t, err, "making transaction composer")
+
 	sfe, err := NewSelfServiceFrontEndImpl(
 		stats,
 		fc,
@@ -60,6 +66,8 @@ func setupSFE(t *testing.T) (SelfServiceFrontEndImpl, clock.FakeClock) {
 		mockSA,
 		key,
 		nil,
+		limiter,
+		txnBuilder,
 	)
 	test.AssertNotError(t, err, "Unable to create SFE")