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/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, 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": {