From 82101bfbf56a6e162754f1a2129511389f06eee8 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 14:32:32 +1300 Subject: [PATCH 1/2] fix(gateway): count only failed MFA attempts toward the rate limit The limit of five attempts per kind of factor in five minutes counted every attempt, successful and refused ones included. Every run, exec and deploy approval needs its own MFA proof, so a person running a few commands, or approving deploys for a few apps, went past five in five minutes, and each refused retry kept the window full. Their commands failed after a good PIN and touch, with "Security key verification failed" (or "websocket: bad handshake" for an exec). Only attempts that failed or are still being checked count now. A refused attempt is still recorded, and is answered with 429 "Too many failed MFA attempts" instead of a verification failure. --- .../content/docs/concepts/mfa-security.mdx | 4 +- internal/gateway/auth/mfa/attempts.go | 6 +-- internal/gateway/auth/mfa/security_test.go | 4 +- internal/gateway/db/mfa_attempts.go | 20 +++++--- internal/gateway/db/mfa_attempts_test.go | 45 ++++++++++++++++- .../deploy_approval_requests_mfa_test.go | 50 +++++++++++++++++++ internal/gateway/middleware/mfa.go | 14 ++++-- internal/gateway/proxy/mfa_verification.go | 6 +++ 8 files changed, 133 insertions(+), 16 deletions(-) diff --git a/docs/src/content/docs/concepts/mfa-security.mdx b/docs/src/content/docs/concepts/mfa-security.mdx index f094ad04..0bce0d3a 100644 --- a/docs/src/content/docs/concepts/mfa-security.mdx +++ b/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** diff --git a/internal/gateway/auth/mfa/attempts.go b/internal/gateway/auth/mfa/attempts.go index a53f2710..3939da45 100644 --- a/internal/gateway/auth/mfa/attempts.go +++ b/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) } diff --git a/internal/gateway/auth/mfa/security_test.go b/internal/gateway/auth/mfa/security_test.go index ac7daf10..be5f38ec 100644 --- a/internal/gateway/auth/mfa/security_test.go +++ b/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) } } diff --git a/internal/gateway/db/mfa_attempts.go b/internal/gateway/db/mfa_attempts.go index 7ee931de..ebc37fc5 100644 --- a/internal/gateway/db/mfa_attempts.go +++ b/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) diff --git a/internal/gateway/db/mfa_attempts_test.go b/internal/gateway/db/mfa_attempts_test.go index bde94120..628608c0 100644 --- a/internal/gateway/db/mfa_attempts_test.go +++ b/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) +} diff --git a/internal/gateway/handlers/deploy_approval_requests_mfa_test.go b/internal/gateway/handlers/deploy_approval_requests_mfa_test.go index 1bf7b1b5..9bf16efc 100644 --- a/internal/gateway/handlers/deploy_approval_requests_mfa_test.go +++ b/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) +} diff --git a/internal/gateway/middleware/mfa.go b/internal/gateway/middleware/mfa.go index c17c75a6..2ecd41b2 100644 --- a/internal/gateway/middleware/mfa.go +++ b/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." diff --git a/internal/gateway/proxy/mfa_verification.go b/internal/gateway/proxy/mfa_verification.go index 0bf87e00..7becd682 100644 --- a/internal/gateway/proxy/mfa_verification.go +++ b/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, From 501f8b4a8d01d08c7d31029d2ca8e4fa518e1aeb Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Mon, 26 Oct 2026 14:33:22 +1300 Subject: [PATCH 2/2] chore: bump version to v0.1.25 --- convox.yml | 4 ++-- web/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/convox.yml b/convox.yml index fabf758d..eeca5e39 100644 --- a/convox.yml +++ b/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 diff --git a/web/package.json b/web/package.json index 5803d87d..ca11fefa 100644 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "web", - "version": "0.1.24", + "version": "0.1.25", "private": true, "type": "module", "scripts": {