diff --git a/api/auth_flow_rate_limiter.go b/api/auth_flow_rate_limiter.go index 9acf3bf5..24a1b677 100644 --- a/api/auth_flow_rate_limiter.go +++ b/api/auth_flow_rate_limiter.go @@ -12,6 +12,33 @@ import ( const buildModeTest = "test" +// Auth-flow rate limit values (requests per 60s sliding window). +const ( + // authFlowSessionLimit bounds requests sharing one session identifier + // (OAuth state / SAML RelayState / token code). + authFlowSessionLimit = 100 + + // authFlowUserLimit bounds requests keyed on a single user identifier + // (login_hint, or client_id for the client-credentials grant). It is + // deliberately LOWER than the per-IP limit so the user scope is + // independently enforceable (issue #506): for single-account attacks such + // as password-spray or account enumeration against one victim from a single + // source IP, the user counter now trips before the shared-IP counter, which + // at an equal limit always tripped first and subsumed the user scope. 50/60s + // is far above any legitimate single-account cadence — an interactive OAuth + // flow is a handful of requests, and a client-credentials client with a + // 1-hour token needs roughly one token request per hour — and + // ResetUserRateLimit clears this counter on a successful login, so + // legitimate users are never locked out. + authFlowUserLimit = 50 + + // authFlowDefaultIPLimit bounds requests from a single source IP across the + // auth-flow and token endpoints. Higher than authFlowUserLimit so that + // shared egress IPs (corporate NAT) can carry many distinct users before the + // coarse IP scope engages. + authFlowDefaultIPLimit = 100 +) + // envForceAuthFlowRateLimiting, when set to "true", forces the auth-flow rate // limiter to enforce limits even in build_mode=test. It exists so the // integration suite can exercise the multi-scope limiter against a server that @@ -60,22 +87,26 @@ func (r *AuthFlowRateLimiter) ResetUserRateLimit(ctx context.Context, userIdenti } } -// CheckRateLimit checks all three scopes and returns the most restrictive result -// Scopes: session (100/min), IP (100/min), user identifier (100/min) +// CheckRateLimit checks all three scopes and returns the most restrictive result. +// Scopes are evaluated most-specific-first: session (100/min), user identifier +// (50/min), IP (100/min). // SEM@2ba330fcb59eb085d8f877fe8f75f90af9b69071: check all three auth-flow rate limit scopes and return the most restrictive result (reads DB) func (r *AuthFlowRateLimiter) CheckRateLimit(ctx context.Context, sessionID string, ipAddress string, userIdentifier string) (*RateLimitResult, error) { - return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, 100) + return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, authFlowDefaultIPLimit) } // CheckRateLimitForTokenEndpoint checks rate limits for the token endpoint -// Uses the same 100 requests/minute per IP limit as other auth endpoints +// Uses the same per-IP limit as other auth endpoints // SEM@c70d49ed2d6089c24d05f8bc287ba5711c73abde: check rate limits for the token endpoint using the standard per-IP limit (reads DB) func (r *AuthFlowRateLimiter) CheckRateLimitForTokenEndpoint(ctx context.Context, sessionID string, ipAddress string, userIdentifier string) (*RateLimitResult, error) { - return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, 100) + return r.checkRateLimitWithIPLimit(ctx, sessionID, ipAddress, userIdentifier, authFlowDefaultIPLimit) } -// checkRateLimitWithIPLimit implements multi-scope rate limiting with a configurable IP limit -// SEM@c70d49ed2d6089c24d05f8bc287ba5711c73abde: check session, IP, and user rate limit scopes with a configurable IP limit (reads DB) +// checkRateLimitWithIPLimit implements multi-scope rate limiting with a configurable IP limit. +// Scopes are checked most-specific-first (session -> user -> IP) so that the +// most narrowly-scoped counter is the one attributed when several would trip, +// and so the lower per-user limit engages before the shared per-IP limit. +// SEM@40c0e38339277e6a54d03dc01d30025bc0ef663d: check session, user, then IP rate limit scopes with a configurable IP limit (reads DB) func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, sessionID string, ipAddress string, userIdentifier string, ipLimit int) (*RateLimitResult, error) { logger := slogging.Get() @@ -96,63 +127,64 @@ func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, ses return &RateLimitResult{Allowed: true}, nil } - // Check session scope (100 requests/minute) + // Check session scope (most specific: one OAuth state / SAML relay / code). if sessionID != "" { sessionKey := fmt.Sprintf("auth:ratelimit:session:60s:%s", sessionID) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, sessionKey, 100, 60) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, sessionKey, authFlowSessionLimit, 60) if err != nil { logger.Error("failed to check session rate limit: %v", err) return nil, fmt.Errorf("session rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, sessionKey, 100, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, sessionKey, authFlowSessionLimit, 60) return &RateLimitResult{ Allowed: false, BlockedByScope: "session", RetryAfter: retryAfter, - Limit: 100, + Limit: authFlowSessionLimit, Remaining: remaining, ResetAt: resetAt, }, nil } } - // Check IP scope (configurable requests/minute) - if ipAddress != "" { - ipKey := fmt.Sprintf("auth:ratelimit:ip:60s:%s", ipAddress) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, ipKey, ipLimit, 60) + // Check user identifier scope before IP so the lower per-user limit engages + // independently for single-account attacks from a shared source IP. + if userIdentifier != "" { + userKey := fmt.Sprintf("auth:ratelimit:user:60s:%s", userIdentifier) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, userKey, authFlowUserLimit, 60) if err != nil { - logger.Error("failed to check IP rate limit: %v", err) - return nil, fmt.Errorf("IP rate limit check failed: %w", err) + logger.Error("failed to check user identifier rate limit: %v", err) + return nil, fmt.Errorf("user identifier rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, ipKey, ipLimit, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, userKey, authFlowUserLimit, 60) return &RateLimitResult{ Allowed: false, - BlockedByScope: "ip", + BlockedByScope: "user", RetryAfter: retryAfter, - Limit: ipLimit, + Limit: authFlowUserLimit, Remaining: remaining, ResetAt: resetAt, }, nil } } - // Check user identifier scope (100 attempts/minute) - if userIdentifier != "" { - userKey := fmt.Sprintf("auth:ratelimit:user:60s:%s", userIdentifier) - allowed, retryAfter, err := r.CheckSlidingWindow(ctx, userKey, 100, 60) + // Check IP scope last (coarsest: a shared egress IP may carry many users). + if ipAddress != "" { + ipKey := fmt.Sprintf("auth:ratelimit:ip:60s:%s", ipAddress) + allowed, retryAfter, err := r.CheckSlidingWindow(ctx, ipKey, ipLimit, 60) if err != nil { - logger.Error("failed to check user identifier rate limit: %v", err) - return nil, fmt.Errorf("user identifier rate limit check failed: %w", err) + logger.Error("failed to check IP rate limit: %v", err) + return nil, fmt.Errorf("IP rate limit check failed: %w", err) } if !allowed { - remaining, resetAt, _ := r.GetRateLimitInfo(ctx, userKey, 100, 60) + remaining, resetAt, _ := r.GetRateLimitInfo(ctx, ipKey, ipLimit, 60) return &RateLimitResult{ Allowed: false, - BlockedByScope: "user", + BlockedByScope: "ip", RetryAfter: retryAfter, - Limit: 100, + Limit: ipLimit, Remaining: remaining, ResetAt: resetAt, }, nil @@ -164,15 +196,15 @@ func (r *AuthFlowRateLimiter) checkRateLimitWithIPLimit(ctx context.Context, ses var resetAt int64 if sessionID != "" { sessionKey := fmt.Sprintf("auth:ratelimit:session:60s:%s", sessionID) - remaining, resetAt, _ = r.GetRateLimitInfo(ctx, sessionKey, 100, 60) + remaining, resetAt, _ = r.GetRateLimitInfo(ctx, sessionKey, authFlowSessionLimit, 60) } else { - remaining = 100 + remaining = authFlowSessionLimit resetAt = time.Now().Unix() + 60 } return &RateLimitResult{ Allowed: true, - Limit: 100, + Limit: authFlowSessionLimit, Remaining: remaining, ResetAt: resetAt, }, nil diff --git a/api/rate_limit_middleware_test.go b/api/rate_limit_middleware_test.go index d75c5d64..6e44db2d 100644 --- a/api/rate_limit_middleware_test.go +++ b/api/rate_limit_middleware_test.go @@ -661,9 +661,9 @@ func TestAuthFlowRateLimitMiddleware(t *testing.T) { loginHint := "testuser@example.com" - // Make 100 requests to exhaust user limit (100/min) + // Make authFlowUserLimit requests to exhaust the user limit (50/min) // Use different state AND different IP each time to avoid session/IP limits - for i := range 100 { + for i := range authFlowUserLimit { req := httptest.NewRequest("GET", "/oauth2/authorize?state="+uuid.New().String()+"&login_hint="+loginHint, nil) req.RemoteAddr = fmt.Sprintf("10.%d.%d.%d:12345", i/65536%256, i/256%256, i%256+1) w := httptest.NewRecorder() @@ -671,7 +671,7 @@ func TestAuthFlowRateLimitMiddleware(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code, "Request %d should be allowed", i+1) } - // 101st request should be blocked by user scope + // The next request should be blocked by user scope req := httptest.NewRequest("GET", "/oauth2/authorize?state="+uuid.New().String()+"&login_hint="+loginHint, nil) req.RemoteAddr = "10.99.99.99:12345" w := httptest.NewRecorder() @@ -1067,18 +1067,19 @@ func TestAuthFlowRateLimiter(t *testing.T) { limiter := NewAuthFlowRateLimiter(client) userIdentifier := "testuser@example.com" - // Make 100 requests to exhaust user limit (use different sessions and IPs) - for i := range 100 { + // Make authFlowUserLimit requests to exhaust the user limit (use different sessions and IPs) + for i := range authFlowUserLimit { result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), uuid.New().String(), userIdentifier) require.NoError(t, err) assert.True(t, result.Allowed, "Request %d should be allowed", i+1) } - // 101st request should be blocked by user scope + // The next request should be blocked by user scope result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), uuid.New().String(), userIdentifier) require.NoError(t, err) assert.False(t, result.Allowed) assert.Equal(t, "user", result.BlockedByScope) + assert.Equal(t, authFlowUserLimit, result.Limit) }) } @@ -1250,8 +1251,10 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { limiter := NewAuthFlowRateLimiter(client) targetUser := "admin@example.com" - // Attacker probes one username 100 times from different IPs/sessions - for i := range 100 { + // Attacker probes one username authFlowUserLimit times from different IPs/sessions. + // The per-user limit (50) is lower than the per-IP limit (100), so the user + // scope bites independently even though each request also uses a fresh IP. + for i := range authFlowUserLimit { result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), fmt.Sprintf("10.0.%d.%d", i/256, i%256), @@ -1260,7 +1263,7 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { assert.True(t, result.Allowed, "Request %d should be allowed", i+1) } - // 101st attempt blocked by user scope + // The next attempt is blocked by user scope result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), "172.16.0.1", @@ -1270,6 +1273,35 @@ func TestAuthFlowRateLimiterMultiScopeScenarios(t *testing.T) { assert.Equal(t, "user", result.BlockedByScope) }) + t.Run("single-account attack from one IP is blocked by user scope, not subsumed by IP (issue #506)", func(t *testing.T) { + client, mr := setupTestRedis(t) + defer mr.Close() + defer func() { _ = client.Close() }() + + limiter := NewAuthFlowRateLimiter(client) + targetUser := "victim@example.com" + attackerIP := "203.0.113.77" + + // A real single-IP attacker password-spraying one account: same user and + // same IP on every request (unique session per request so the session + // scope stays out of it). Because the per-user limit (50) is lower than + // the per-IP limit (100), the user scope must trip first — before the IP + // counter reaches its limit. Prior to #506 the equal limits and the + // session->IP->user order let the IP scope always trip first, so the user + // scope could never be the attributed blocker for single-IP traffic. + for i := range authFlowUserLimit { + result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), attackerIP, targetUser) + require.NoError(t, err) + assert.True(t, result.Allowed, "Request %d should be allowed", i+1) + } + + result, err := limiter.CheckRateLimit(context.Background(), uuid.New().String(), attackerIP, targetUser) + require.NoError(t, err) + assert.False(t, result.Allowed) + assert.Equal(t, "user", result.BlockedByScope, "user scope must bite before the shared-IP scope for single-account attacks") + assert.Equal(t, authFlowUserLimit, result.Limit) + }) + t.Run("session replay: same session blocked at 100 regardless of other scopes", func(t *testing.T) { client, mr := setupTestRedis(t) defer mr.Close() diff --git a/test/integration/workflows/auth_flow_rate_limit_test.go b/test/integration/workflows/auth_flow_rate_limit_test.go index 513a0abe..232e40f2 100644 --- a/test/integration/workflows/auth_flow_rate_limit_test.go +++ b/test/integration/workflows/auth_flow_rate_limit_test.go @@ -8,12 +8,18 @@ import ( "github.com/ericfitz/tmi/test/integration/framework" ) -// authFlowLimit is the per-scope auth-flow rate limit (requests / 60s) enforced -// by api/auth_flow_rate_limiter.go for the session, IP, and user scopes. The -// token endpoint uses the same per-IP limit. Keep this in sync with the -// implementation (checkRateLimitWithIPLimit). +// authFlowLimit is the per-session and per-IP auth-flow rate limit (requests / +// 60s) enforced by api/auth_flow_rate_limiter.go. The token endpoint uses the +// same per-IP limit. Keep this in sync with the implementation +// (authFlowSessionLimit / authFlowDefaultIPLimit in checkRateLimitWithIPLimit). const authFlowLimit = 100 +// authFlowUserLimit is the per-user-identifier auth-flow rate limit (requests / +// 60s). It is deliberately lower than authFlowLimit so the user scope is +// independently enforceable for single-account attacks (issue #506). Keep this +// in sync with authFlowUserLimit in api/auth_flow_rate_limiter.go. +const authFlowUserLimit = 50 + // uniqueAuthFlowIP returns a distinct non-loopback test IP for index i. The // integration server runs with no trusted proxies, so it honors X-Forwarded-For // (extractIPAddress's manual path) — letting a test vary the perceived client IP @@ -148,19 +154,20 @@ func TestAuthFlowRateLimiting_MultiScope(t *testing.T) { framework.AssertNoError(t, err, "Failed to create client") // Same login_hint, but a unique state AND a unique IP per request so - // neither the session nor the IP scope accumulates — only the user scope. + // neither the session nor the IP scope accumulates — only the user scope, + // which enforces the lower authFlowUserLimit (issue #506). loginHint := "ratelimit-user-" + framework.UniqueUserID() - for i := 0; i < authFlowLimit; i++ { + for i := 0; i < authFlowUserLimit; i++ { state := fmt.Sprintf("user-state-%s-%d", framework.UniqueUserID(), i) resp := authFlowAuthorize(t, client, state, loginHint, uniqueAuthFlowIP(i)) if resp.StatusCode == 429 { - t.Fatalf("Request %d was rate limited before the limit of %d", i+1, authFlowLimit) + t.Fatalf("Request %d was rate limited before the limit of %d", i+1, authFlowUserLimit) } } - resp := authFlowAuthorize(t, client, "user-final-"+framework.UniqueUserID(), loginHint, uniqueAuthFlowIP(authFlowLimit)) + resp := authFlowAuthorize(t, client, "user-final-"+framework.UniqueUserID(), loginHint, uniqueAuthFlowIP(authFlowUserLimit)) if resp.StatusCode != 429 { - t.Errorf("Expected 429 on request %d with the same login_hint, got %d", authFlowLimit+1, resp.StatusCode) + t.Errorf("Expected 429 on request %d with the same login_hint, got %d", authFlowUserLimit+1, resp.StatusCode) } if scope := resp.Headers.Get("X-RateLimit-Scope"); scope != "user" { t.Errorf("Expected X-RateLimit-Scope=user, got %q", scope)