Count only failed MFA attempts toward the rate limit

convox.yml

@@ -30,7 +30,7 @@ services:
     # Pinned to an immutable release tag. scripts/bump-version.sh sets it to the new
     # version (:vX.Y.Z); the release workflow publishes that tag when the v* git tag is pushed.
     # :e327add is the v0.1.1 release (identical to :latest when this was pinned).
-    image: docker.io/docspringcom/rack-gateway:v0.1.24
+    image: docker.io/docspringcom/rack-gateway:v0.1.25
     command: ./scripts/start-gateway.sh
     environment:
       - PORT=8080
@@ -54,7 +54,7 @@ services:
 
   admin:
     # Uses same image as gateway service (kept in sync by scripts/bump-version.sh)
-    image: docker.io/docspringcom/rack-gateway:v0.1.24
+    image: docker.io/docspringcom/rack-gateway:v0.1.25
     command: echo "Use this service to run migrations, database admin tasks, etc."
     environment:
       - PORT=8080

docs/src/content/docs/concepts/mfa-security.mdx

@@ -227,7 +227,9 @@ flowchart LR
 
 1. **Rate limit attempts**
 
-   Prevent brute-force attacks on 6-digit codes
+   Prevent brute-force attacks on 6-digit codes. Rack Gateway allows five failed (or still unchecked) attempts with
+   each kind of factor in five minutes, and answers further attempts with `429`. Successful verifications don't
+   count: every `run`, `exec` and deploy approval needs its own MFA proof
 
 2. **Lock out after failures**
 

internal/gateway/auth/mfa/attempts.go

@@ -8,8 +8,8 @@ import (
 )
 
 const (
-	// attemptLimit is the most attempts with one kind of factor (codes, or security keys) a user may make in
-	// attemptWindowMinutes.
+	// attemptLimit is the most failed (or unfinished) attempts with one kind of factor (codes, or security keys) a
+	// user may have in attemptWindowMinutes (see db.ReserveMFAAttempt).
 	attemptLimit         = 5
 	attemptWindowMinutes = 5
 )
@@ -36,7 +36,7 @@ func (s *Service) reserveAttempt(
 	case errors.Is(err, db.ErrUserLocked):
 		return nil, fmt.Errorf("account locked - contact administrator")
 	case errors.Is(err, db.ErrMFAAttemptLimit):
-		return nil, fmt.Errorf("too many attempts - try again in 5 minutes")
+		return nil, fmt.Errorf("%w: try again in %d minutes", err, attemptWindowMinutes)
 	case err != nil:
 		return nil, fmt.Errorf("failed to check rate limit: %w", err)
 	}

internal/gateway/auth/mfa/security_test.go

@@ -1,6 +1,7 @@
 package mfa
 
 import (
+	"errors"
 	"sync"
 	"testing"
 	"time"
@@ -61,7 +62,8 @@ func TestVerifyTOTP_RateLimiting(t *testing.T) {
 	// 6th attempt should be rate limited (not locked, since only 4 failed verifications)
 	if _, err := svc.VerifyTOTP(user, "123456", "1.2.3.4", "test-agent", nil); err == nil {
 		t.Fatal("expected rate limiting to reject attempt")
-	} else if err.Error() != "too many attempts - try again in 5 minutes" {
+	} else if !errors.Is(err, db.ErrMFAAttemptLimit) ||
+		err.Error() != "too many failed MFA attempts: try again in 5 minutes" {
 		t.Fatalf("expected rate limit error, got: %v", err)
 	}
 }

internal/gateway/db/mfa_attempts.go

@@ -78,8 +78,9 @@ const (
 
 var (
 
-	// ErrMFAAttemptLimit is returned by ReserveMFAAttempt when the user has made the most attempts the window allows.
-	ErrMFAAttemptLimit = errors.New("too many MFA attempts")
+	// ErrMFAAttemptLimit is returned by ReserveMFAAttempt when the user has made the most failed attempts the window
+	// allows.
+	ErrMFAAttemptLimit = errors.New("too many failed MFA attempts")
 	// ErrUserLocked is returned by ReserveMFAAttempt for a locked user.
 	ErrUserLocked = errors.New("user is locked")
 )
@@ -94,16 +95,20 @@ type MFAAttemptRequest struct {
 }
 
 // MFAAttempt is an MFA verification attempt recorded by ReserveMFAAttempt. It counts toward the rate limit at once,
-// and toward the automatic lock (CountRecentFailedMFAAttempts) once Finish records that it failed.
+// until Finish records that it succeeded, and toward the automatic lock (CountRecentFailedMFAAttempts) once Finish
+// records that it failed.
 type MFAAttempt struct {
 	db *Database
 	ID int64
 }
 
 // ReserveMFAAttempt records an attempt before its factor is checked. It holds the user's row lock while it checks
-// that they aren't locked (ErrUserLocked) and have made fewer than limit attempts of this kind in the last
-// minutes, so concurrent attempts are counted one at a time and can't pass the limit together. Over the limit it
-// records a rate_limited attempt and returns ErrMFAAttemptLimit.
+// that they aren't locked (ErrUserLocked) and have fewer than limit attempts of this kind in the last minutes that
+// failed or are still being verified, so concurrent attempts are counted one at a time and can't pass the limit
+// together. Successful attempts don't count: every command asks for a fresh MFA proof, and a person running several
+// commands, or approving several deploys, verifies many times in a few minutes. Refused (rate_limited) attempts
+// don't count either, or retries would keep the limit in force. Over the limit it records a rate_limited attempt
+// and returns ErrMFAAttemptLimit.
 func (d *Database) ReserveMFAAttempt(req MFAAttemptRequest, limit, minutes int) (*MFAAttempt, error) {
 	tx, err := d.db.Begin()
 	if err != nil {
@@ -126,7 +131,8 @@ func (d *Database) ReserveMFAAttempt(req MFAAttemptRequest, limit, minutes int)
 
 	var count int
 	err = tx.QueryRow(d.rebind(`SELECT COUNT(*) FROM mfa_attempts
-		WHERE user_id = ? AND method_type = ? AND attempted_at > NOW() - INTERVAL '1 minute' * ?`),
+		WHERE user_id = ? AND method_type = ? AND attempted_at > NOW() - INTERVAL '1 minute' * ?
+		  AND success = FALSE AND COALESCE(failure_reason, '') <> 'rate_limited'`),
 		req.UserID, req.MethodType, minutes).Scan(&count)
 	if err != nil {
 		return nil, fmt.Errorf("failed to count MFA attempts: %w", err)

internal/gateway/db/mfa_attempts_test.go

@@ -33,7 +33,50 @@ func TestCountRecentFailedMFAAttemptsCountsFinishedFailures(t *testing.T) {
 	require.NoError(t, reserve().Finish(nil, true, ""))
 	require.Equal(t, 2, failedCount())
 
-	_, err := database.ReserveMFAAttempt(req, 4, 5)
+	// The rate limit counts the unfinished attempt and the two failures, not the success.
+	_, err := database.ReserveMFAAttempt(req, 3, 5)
 	require.ErrorIs(t, err, gwdb.ErrMFAAttemptLimit)
 	require.Equal(t, 2, failedCount(), "a rate-limited attempt isn't a failed verification")
 }
+
+// The rate limit counts attempts that failed or are still being verified. Successful attempts don't count: every
+// command asks for a fresh proof, so a person running a few commands, or approving a few deploys, verifies many
+// times in five minutes. Refused attempts don't count either, or retrying would keep the limit in force.
+func TestReserveMFAAttemptCountsOnlyFailedAndUnfinishedAttempts(t *testing.T) {
+	database, user := newEnrollmentUser(t)
+	req := gwdb.MFAAttemptRequest{UserID: user.ID, MethodType: gwdb.MFAMethodTypeWebAuthn}
+	const limit = 3
+	reserve := func() (*gwdb.MFAAttempt, error) {
+		t.Helper()
+		return database.ReserveMFAAttempt(req, limit, 5)
+	}
+
+	for range 10 {
+		attempt, err := reserve()
+		require.NoError(t, err, "successful attempts don't count toward the limit")
+		require.NoError(t, attempt.Finish(nil, true, ""))
+	}
+
+	unfinished, err := reserve()
+	require.NoError(t, err)
+	failed, err := reserve()
+	require.NoError(t, err)
+	require.NoError(t, failed.Finish(nil, false, "invalid_signature"))
+	_, err = database.ReserveMFAAttempt(
+		gwdb.MFAAttemptRequest{UserID: user.ID, MethodType: gwdb.MFAMethodTypeTOTP}, 1, 5,
+	)
+	require.NoError(t, err, "each kind of factor has its own limit")
+
+	last, err := reserve()
+	require.NoError(t, err)
+	for range 3 {
+		_, err = reserve()
+		require.ErrorIs(t, err, gwdb.ErrMFAAttemptLimit, "an unfinished and two failed attempts reach the limit")
+	}
+
+	// The refused attempts didn't count: once the unfinished ones succeed, there's room again.
+	require.NoError(t, unfinished.Finish(nil, true, ""))
+	require.NoError(t, last.Finish(nil, true, ""))
+	_, err = reserve()
+	require.NoError(t, err)
+}

internal/gateway/handlers/deploy_approval_requests_mfa_test.go

@@ -433,3 +433,53 @@ func TestCreateAPIToken_AlwaysRequiresMFACode(t *testing.T) {
 		require.NotEmpty(t, response["token"])
 	})
 }
+
+// An approval refused by the MFA rate limit says so (429), rather than calling the proof wrong: it was never checked.
+func TestApproveDeployApprovalRequest_RateLimitedMFA(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	database := dbtest.NewDatabase(t)
+	fixture := setupDeployApprovalMFATest(t, database)
+	req, err := database.CreateDeployApprovalRequest(
+		"Test deploy", "test-app", "abc123def4567890abc123def4567890abc123de", "main", "", []byte("{}"),
+		fixture.admin.ID, nil, fixture.apiToken.ID,
+	)
+	require.NoError(t, err)
+	for range 5 {
+		attempt, err := database.ReserveMFAAttempt(
+			db.MFAAttemptRequest{UserID: fixture.admin.ID, MethodType: db.MFAMethodTypeTOTP}, 5, 5,
+		)
+		require.NoError(t, err)
+		require.NoError(t, attempt.Finish(nil, false, "invalid_code"))
+	}
+
+	_, session, err := fixture.sessionManager.CreateSession(fixture.admin, auth.SessionMetadata{Channel: "web"})
+	require.NoError(t, err)
+	now := time.Now()
+	session.MFAVerifiedAt = &now
+	router := gin.New()
+	router.POST("/api/v1/deploy-approval-requests/:id/approve", func(c *gin.Context) {
+		authUser := &auth.User{
+			Email: fixture.admin.Email, Name: fixture.admin.Name, Roles: fixture.admin.Roles, Session: session,
+			MFAType: "totp", MFAValue: "000000",
+		}
+		c.Request = c.Request.WithContext(context.WithValue(c.Request.Context(), auth.UserContextKey, authUser))
+		c.Set("user_email", fixture.admin.Email)
+		c.Next()
+	}, middleware.EnforceMFARequirements(fixture.mfaService, database, fixture.mfaSettings),
+		fixture.handler.ApproveDeployApprovalRequest)
+
+	w := httptest.NewRecorder()
+	httpReq, _ := http.NewRequest(
+		"POST", "/api/v1/deploy-approval-requests/"+req.PublicID+"/approve", bytes.NewReader([]byte("{}")),
+	)
+	httpReq.Header.Set("Content-Type", "application/json")
+	router.ServeHTTP(w, httpReq)
+
+	require.Equal(t, http.StatusTooManyRequests, w.Code)
+	var response map[string]interface{}
+	require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response))
+	require.Equal(t, "mfa_rate_limited", response["error"])
+	approval, err := database.GetDeployApprovalRequestByPublicID(req.PublicID)
+	require.NoError(t, err)
+	require.Equal(t, "pending", approval.Status)
+}

internal/gateway/middleware/mfa.go

@@ -3,6 +3,7 @@ package middleware
 import (
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"log"
 	"net/http"
 	"time"
@@ -316,7 +317,7 @@ func verifyByType(
 			sessionID,
 		)
 		if err != nil {
-			return handleVerifyError(c, authUser.MFAType)
+			return handleVerifyError(c, authUser.MFAType, err)
 		}
 		if result != nil {
 			log.Printf("Inline MFA verification successful: method_id=%d", result.MethodID)
@@ -332,7 +333,7 @@ func verifyByType(
 			c.GetHeader("User-Agent"),
 			sessionID,
 		); err != nil {
-			return handleVerifyError(c, authUser.MFAType)
+			return handleVerifyError(c, authUser.MFAType, err)
 		}
 		return true
 	default:
@@ -344,7 +345,14 @@ func verifyByType(
 	}
 }
 
-func handleVerifyError(c *gin.Context, mfaType string) bool {
+func handleVerifyError(c *gin.Context, mfaType string, err error) bool {
+	if errors.Is(err, db.ErrMFAAttemptLimit) {
+		c.AbortWithStatusJSON(http.StatusTooManyRequests, gin.H{
+			"error":   "mfa_rate_limited",
+			"message": "Too many failed MFA attempts. Wait 5 minutes and try again.",
+		})
+		return false
+	}
 	message := "Verification code is incorrect or has expired. Please try again."
 	if mfaType == "webauthn" {
 		message = "Security key verification failed. Please try again."

internal/gateway/proxy/mfa_verification.go

@@ -120,6 +120,12 @@ func (h *Handler) requireRequestMFA(
 		)
 	case errors.Is(err, errMFAUnavailable):
 		h.logMFADenial(r, w, authUser, rackConfig, start, "failed to verify MFA", http.StatusInternalServerError, err)
+	case errors.Is(err, db.ErrMFAAttemptLimit):
+		h.logMFADenial(
+			r, w, authUser, rackConfig, start,
+			"Too many failed MFA attempts. Wait 5 minutes and try again.",
+			http.StatusTooManyRequests, err,
+		)
 	default:
 		h.logMFADenial(
 			r, w, authUser, rackConfig, start,

web/package.json

@@ -1,6 +1,6 @@
 {
   "name": "web",
-  "version": "0.1.24",
+  "version": "0.1.25",
   "private": true,
   "type": "module",
   "scripts": {