diff --git a/.env.example b/.env.example index 6127653f..2bcd02ab 100644 --- a/.env.example +++ b/.env.example @@ -9,6 +9,17 @@ PORT=8083 # COMPANY_LOGO_URL="/images/logo.svg" # AGENT_DESK_SERVER_CORS_ALLOWEDORIGINS="http://localhost:3000,http://127.0.0.1:8083" +# Client IP resolution. +# Gin defaults to trusting every proxy, which makes X-Forwarded-For authoritative: +# any caller could then choose the IP recorded in the login credential log and in +# user.last_login_ip, and any IP-keyed rate limit or lockout would be bypassable. +# Unset, trustedProxies falls back to the loopback / RFC1918 / IPv6 unique-local / +# link-local ranges, which already covers a cloudflared sidecar on a compose +# network. Set trustedPlatform when an edge overwrites the header instead of +# appending to it - Cloudflare does, and CF-Connecting-IP is not client-forgeable. +# TRUSTED_PROXIES="10.0.0.0/8,172.16.0.0/12" +TRUSTED_PLATFORM=cloudflare + # Database Configuration # Driver options: sqlite, mysql, postgres # Supabase PostgreSQL (DOS): diff --git a/config/config.example.yaml b/config/config.example.yaml index c1bb70ca..29b76021 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -5,6 +5,18 @@ server: # Custom platform / instance branding (optional) companyName: "" companyLogoUrl: "" + # Reverse proxies in front of the application, as CIDR blocks. Gin's own default + # trusts every peer, which makes X-Forwarded-For authoritative and lets any + # caller choose the client address the server records in the login credential + # log and in user.last_login_ip. Left empty this falls back to the loopback, + # RFC1918, IPv6 unique-local and link-local ranges, which covers a sidecar + # tunnel, a compose network and a local nginx. Set it explicitly when the proxy + # sits on a public address, and never set it to 0.0.0.0/0. + trustedProxies: [] + # Set to "cloudflare" (also accepted: "fly.io", "google-app-engine", or a literal + # header name) when an edge overwrites rather than appends the real client + # address. When set it takes precedence over X-Forwarded-For entirely. + trustedPlatform: "" cors: # Browser CORS allowlist. In production, replace this with the actual frontend or embedded-site domains, such as https://support.example.com. # Leave it empty to reject cross-origin browser requests. Same-origin and non-browser calls are still supported. diff --git a/internal/bootstrap/server.go b/internal/bootstrap/server.go index 389076dc..f750266a 100644 --- a/internal/bootstrap/server.go +++ b/internal/bootstrap/server.go @@ -1,6 +1,7 @@ package bootstrap import ( + "fmt" "log/slog" "net/http" "path" @@ -35,6 +36,18 @@ func NewServer() (*gin.Engine, error) { printBanner() app := gin.New() + + // Gin defaults to trusting every proxy, which makes ClientIP() return the + // leftmost X-Forwarded-For value - a header any caller can set. Everything + // keyed on a client address depends on this being settled first: the login + // credential log, the user's last login IP, and any abuse control. + if platform := cfg.Server.TrustedPlatformHeader(); platform != "" { + app.TrustedPlatform = platform + } + if err := app.SetTrustedProxies(cfg.Server.TrustedProxiesOrDefault()); err != nil { + return nil, fmt.Errorf("invalid server.trustedProxies: %w", err) + } + app.Use(requestIDMiddleware()) app.Use(corsMiddleware()) app.Use(gin.Recovery()) diff --git a/internal/bootstrap/server_clientip_test.go b/internal/bootstrap/server_clientip_test.go new file mode 100644 index 00000000..2711eb24 --- /dev/null +++ b/internal/bootstrap/server_clientip_test.go @@ -0,0 +1,148 @@ +package bootstrap + +import ( + "bytes" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "agent-desk/internal/pkg/config" + + "github.com/gin-gonic/gin" +) + +// captureRequestLog swaps the slog default for one writing into buf, because +// requestLogMiddleware records the address Gin resolved. That is the only place +// the server exposes its ClientIP() decision, and it is the value that ends up in +// t_login_credential_log and t_user.last_login_ip. +func captureRequestLog(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + return &buf +} + +func setTestServerConfig(server config.ServerConfig) { + config.SetCurrent(&config.Config{ + Server: server, + Storage: config.StorageConfig{Local: config.LocalStorageConfig{Root: "storage", BaseURL: "/storage"}}, + }) +} + +// TestGinDefaultTrustsEveryProxy pins the vulnerability the configuration above +// exists to close. A bare Gin engine - which is what NewServer used to build - +// trusts 0.0.0.0/0 and ::/0, so validateHeader walks X-Forwarded-For right to +// left, finds no untrusted proxy to stop at, and returns the leftmost value the +// caller chose. If this assertion ever starts failing, Gin's default has changed +// and the trusted-proxy configuration should be revisited rather than assumed +// necessary. +func TestGinDefaultTrustsEveryProxy(t *testing.T) { + app := gin.New() + var resolved string + app.GET("/probe", func(ctx *gin.Context) { resolved = ctx.ClientIP() }) + + req := httptest.NewRequest(http.MethodGet, "/probe", nil) + req.RemoteAddr = "203.0.113.7:52000" + req.Header.Set("X-Forwarded-For", "198.51.100.9") + app.ServeHTTP(httptest.NewRecorder(), req) + + if resolved != "198.51.100.9" { + t.Fatalf("a bare engine resolved ClientIP to %q, expected the forged 198.51.100.9", resolved) + } +} + +func TestNewServerIgnoresForgedForwardedForFromAnUntrustedPeer(t *testing.T) { + buf := captureRequestLog(t) + setTestServerConfig(config.ServerConfig{}) + + app, err := NewServer() + if err != nil { + t.Fatalf("NewServer() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + // A public peer is outside every default trusted range, so nothing it claims + // about the originating address may be believed. + req.RemoteAddr = "203.0.113.7:52000" + req.Header.Set("X-Forwarded-For", "198.51.100.9") + req.Header.Set("X-Real-IP", "198.51.100.9") + app.ServeHTTP(rec, req) + + logged := buf.String() + if !strings.Contains(logged, "clientIp=203.0.113.7") { + t.Fatalf("expected the real peer address to be logged, got: %s", logged) + } + if strings.Contains(logged, "198.51.100.9") { + t.Fatalf("a forged forwarding header was trusted: %s", logged) + } +} + +func TestNewServerReadsForwardedForFromATrustedProxy(t *testing.T) { + buf := captureRequestLog(t) + setTestServerConfig(config.ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}}) + + app, err := NewServer() + if err != nil { + t.Fatalf("NewServer() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + req.RemoteAddr = "10.0.0.5:52000" + // A hostile client prepends a value it chose; the proxy appends the address it + // actually saw. Gin walks the list right to left and stops at the first + // address that is not itself a trusted proxy, which is the appended one. + req.Header.Set("X-Forwarded-For", "198.51.100.9, 203.0.113.7") + app.ServeHTTP(rec, req) + + logged := buf.String() + if !strings.Contains(logged, "clientIp=203.0.113.7") { + t.Fatalf("expected the proxy-appended address, got: %s", logged) + } + if strings.Contains(logged, "198.51.100.9") { + t.Fatalf("the client-prepended address was trusted: %s", logged) + } +} + +func TestNewServerPrefersTheConfiguredTrustedPlatformHeader(t *testing.T) { + buf := captureRequestLog(t) + setTestServerConfig(config.ServerConfig{TrustedPlatform: "cloudflare"}) + + app, err := NewServer() + if err != nil { + t.Fatalf("NewServer() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + req.RemoteAddr = "10.0.0.5:52000" + req.Header.Set("X-Forwarded-For", "198.51.100.9") + // The edge overwrites this header rather than appending to it, which is what + // makes it usable as an identity at all. + req.Header.Set("CF-Connecting-IP", "203.0.113.7") + app.ServeHTTP(rec, req) + + logged := buf.String() + if !strings.Contains(logged, "clientIp=203.0.113.7") { + t.Fatalf("expected the platform header to win, got: %s", logged) + } + if strings.Contains(logged, "198.51.100.9") { + t.Fatalf("X-Forwarded-For was consulted despite a trusted platform: %s", logged) + } +} + +func TestNewServerRejectsAnInvalidTrustedProxyCIDR(t *testing.T) { + captureRequestLog(t) + setTestServerConfig(config.ServerConfig{TrustedProxies: []string{"10.0.0.0/8", "not-a-cidr"}}) + + if _, err := NewServer(); err == nil { + t.Fatal("NewServer() accepted an unparseable CIDR; a typo here would silently disable the trust boundary") + } else if !strings.Contains(err.Error(), "server.trustedProxies") { + t.Fatalf("NewServer() error = %v, expected it to name server.trustedProxies", err) + } +} diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index bd5c3902..827af873 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -61,6 +61,65 @@ type ServerConfig struct { CompanyLogoURL string `yaml:"companyLogoUrl"` CompanyFaviconURL string `yaml:"companyFaviconUrl"` CORS CORSConfig `yaml:"cors"` + // TrustedProxies are the CIDR blocks of the reverse proxies that sit in front + // of the application. Gin's own default is 0.0.0.0/0 and ::/0, which trusts + // every peer and makes ClientIP() return the leftmost X-Forwarded-For value - + // a header any caller can set. + TrustedProxies []string `yaml:"trustedProxies"` + // TrustedPlatform names an edge that overwrites rather than appends the real + // client address, for example "cloudflare". When set it takes precedence over + // X-Forwarded-For entirely. + TrustedPlatform string `yaml:"trustedPlatform"` +} + +// defaultTrustedProxies covers loopback, RFC1918, IPv6 unique-local and +// link-local ranges. That is the shape of almost every real deployment - a +// sidecar tunnel, a compose network, a local nginx - and a client on the public +// internet cannot present one of these addresses as its direct peer, so +// X-Forwarded-For stays honest. When the app is exposed directly the peer is a +// public address, is not trusted, and Gin falls back to it. +var defaultTrustedProxies = []string{ + "127.0.0.0/8", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "::1/128", + "fc00::/7", + "fe80::/10", +} + +func (s ServerConfig) TrustedProxiesOrDefault() []string { + proxies := make([]string, 0, len(s.TrustedProxies)) + for _, proxy := range s.TrustedProxies { + if proxy = strings.TrimSpace(proxy); proxy != "" { + proxies = append(proxies, proxy) + } + } + if len(proxies) == 0 { + return defaultTrustedProxies + } + return proxies +} + +// TrustedPlatformHeader resolves the configured platform name to the header Gin +// should read the client address from. Recognised names map to Gin's own +// constants; any other non-empty value is passed through as a literal header +// name, which is what Gin's TrustedPlatform field expects. +func (s ServerConfig) TrustedPlatformHeader() string { + platform := strings.TrimSpace(s.TrustedPlatform) + if platform == "" { + return "" + } + switch strings.ToLower(platform) { + case "cloudflare", "cf": + return "CF-Connecting-IP" + case "fly.io", "flyio", "fly-io": + return "Fly-Client-IP" + case "google-app-engine", "appengine", "gae": + return "X-Appengine-Remote-Addr" + default: + return platform + } } func (s ServerConfig) Address() string { @@ -107,7 +166,25 @@ type AuthConfig struct { PasswordLoginEnabled *bool `yaml:"passwordLoginEnabled"` TokenTTLHours int `yaml:"tokenTTLHours"` MaxFailedAttempts int `yaml:"maxFailedAttempts"` - CredentialLockMinute int `yaml:"credentialLockMinute"` + // MaxFailedAttemptsPerIP bounds failures from one client address across every + // username, which is what credential stuffing looks like. Zero or unset + // derives four times MaxFailedAttempts; it is disabled when MaxFailedAttempts + // is disabled. + MaxFailedAttemptsPerIP int `yaml:"maxFailedAttemptsPerIP"` + CredentialLockMinute int `yaml:"credentialLockMinute"` +} + +// MaxFailedAttemptsPerIPOrDefault derives the per-address threshold from the +// per-account one so that a deployment which only tunes MaxFailedAttempts still +// gets a coherent pair of limits. +func (a AuthConfig) MaxFailedAttemptsPerIPOrDefault() int { + if a.MaxFailedAttemptsPerIP > 0 { + return a.MaxFailedAttemptsPerIP + } + if a.MaxFailedAttempts <= 0 { + return 0 + } + return a.MaxFailedAttempts * 4 } func (a AuthConfig) IsPasswordLoginEnabled() bool { @@ -431,6 +508,8 @@ func bindConfigDefaults(v *viper.Viper) { v.SetDefault("server.companyLogoUrl", "") v.SetDefault("server.companyFaviconUrl", "") v.SetDefault("server.cors.allowedOrigins", []string{}) + v.SetDefault("server.trustedProxies", []string{}) + v.SetDefault("server.trustedPlatform", "") v.SetDefault("db.type", "sqlite") v.SetDefault("db.dsn", "file:./data/app.db?_busy_timeout=5000") v.SetDefault("db.maxIdleConns", 5) @@ -442,6 +521,7 @@ func bindConfigDefaults(v *viper.Viper) { v.SetDefault("logger.addSource", false) v.SetDefault("auth.tokenTTLHours", 12) v.SetDefault("auth.maxFailedAttempts", 5) + v.SetDefault("auth.maxFailedAttemptsPerIP", 0) v.SetDefault("auth.credentialLockMinute", 15) v.SetDefault("customerSession.ttlMinutes", 120) v.SetDefault("customerSession.refreshThresholdMinutes", 30) @@ -499,6 +579,8 @@ func bindEnvironmentAliases(v *viper.Viper) { _ = v.BindEnv("server.companyName", "AGENT_DESK_SERVER_COMPANYNAME", "COMPANY_NAME", "NEXT_PUBLIC_COMPANY_NAME", "BRAND_NAME", "BRAND_COMPANY_NAME") _ = v.BindEnv("server.companyLogoUrl", "AGENT_DESK_SERVER_COMPANYLOGOURL", "COMPANY_LOGO_URL", "NEXT_PUBLIC_COMPANY_LOGO_URL", "BRAND_LOGO_URL") _ = v.BindEnv("server.companyFaviconUrl", "AGENT_DESK_SERVER_COMPANYFAVICONURL", "COMPANY_FAVICON_URL", "NEXT_PUBLIC_COMPANY_FAVICON_URL", "BRAND_FAVICON_URL", "FAVICON_URL") + _ = v.BindEnv("server.trustedProxies", "AGENT_DESK_SERVER_TRUSTEDPROXIES", "TRUSTED_PROXIES") + _ = v.BindEnv("server.trustedPlatform", "AGENT_DESK_SERVER_TRUSTEDPLATFORM", "TRUSTED_PLATFORM") _ = v.BindEnv("db.type", "AGENT_DESK_DB_TYPE", "DATABASE_TYPE", "DB_TYPE") _ = v.BindEnv("db.dsn", "AGENT_DESK_DB_DSN", "DATABASE_URL", "DB_DSN") _ = v.BindEnv("auth.passwordLoginEnabled", "AGENT_DESK_AUTH_PASSWORDLOGINENABLED", "PASSWORD_LOGIN_ENABLED") diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index b05068b3..5e600d9c 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -426,3 +426,107 @@ func TestResolveMetaAppWithoutLoadedConfig(t *testing.T) { t.Errorf("ResolveWhatsAppApp() = %+v, want the channel-level credentials", creds) } } + +func TestAuthConfigMaxFailedAttemptsPerIPOrDefault(t *testing.T) { + cases := []struct { + name string + cfg AuthConfig + want int + }{ + {"explicit value wins", AuthConfig{MaxFailedAttempts: 5, MaxFailedAttemptsPerIP: 30}, 30}, + {"unset derives four times the per-account limit", AuthConfig{MaxFailedAttempts: 5}, 20}, + {"disabled per-account limit disables both", AuthConfig{MaxFailedAttempts: 0}, 0}, + {"negative per-account limit disables both", AuthConfig{MaxFailedAttempts: -1}, 0}, + } + for _, tc := range cases { + if got := tc.cfg.MaxFailedAttemptsPerIPOrDefault(); got != tc.want { + t.Errorf("%s: MaxFailedAttemptsPerIPOrDefault() = %d want %d", tc.name, got, tc.want) + } + } +} + +func TestServerConfigTrustedProxiesOrDefault(t *testing.T) { + // An unset list must not fall through to Gin's own default of 0.0.0.0/0 and + // ::/0, which trusts every peer and makes X-Forwarded-For authoritative. + got := ServerConfig{}.TrustedProxiesOrDefault() + for _, want := range []string{"127.0.0.0/8", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "::1/128", "fc00::/7", "fe80::/10"} { + found := false + for _, entry := range got { + if entry == want { + found = true + } + } + if !found { + t.Errorf("TrustedProxiesOrDefault() = %v, missing %s", got, want) + } + } + for _, entry := range got { + if entry == "0.0.0.0/0" || entry == "::/0" { + t.Errorf("TrustedProxiesOrDefault() includes %s, which trusts every peer", entry) + } + } + + // Blank entries from a comma-separated environment variable must not become + // empty CIDRs, which Gin would reject at startup. + got = ServerConfig{TrustedProxies: []string{" 10.1.0.0/16 ", "", " "}}.TrustedProxiesOrDefault() + if len(got) != 1 || got[0] != "10.1.0.0/16" { + t.Errorf("TrustedProxiesOrDefault() = %v want [10.1.0.0/16]", got) + } +} + +func TestServerConfigTrustedPlatformHeader(t *testing.T) { + cases := []struct { + platform string + want string + }{ + {"", ""}, + {"cloudflare", "CF-Connecting-IP"}, + {"Cloudflare", "CF-Connecting-IP"}, + {" CF ", "CF-Connecting-IP"}, + {"fly.io", "Fly-Client-IP"}, + {"google-app-engine", "X-Appengine-Remote-Addr"}, + // Anything unrecognised is a literal header name, which is what Gin's + // TrustedPlatform field expects. + {"X-CDN-IP", "X-CDN-IP"}, + } + for _, tc := range cases { + if got := (ServerConfig{TrustedPlatform: tc.platform}).TrustedPlatformHeader(); got != tc.want { + t.Errorf("TrustedPlatformHeader(%q) = %q want %q", tc.platform, got, tc.want) + } + } +} + +// TestLoadReadsTrustedProxySettings covers the environment spelling, including +// the comma-separated list. Gin rejects an unparseable CIDR at startup, so a +// value that arrives as one string instead of a slice would take the process +// down rather than degrade quietly. +func TestLoadReadsTrustedProxySettings(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte("server:\n port: 8083\n"), 0600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + t.Setenv("ENV_FILE", os.DevNull) + t.Setenv("AGENT_DESK_ENV_FILE", os.DevNull) + t.Setenv("TRUSTED_PROXIES", "10.0.0.0/8,172.16.0.0/12") + t.Setenv("TRUSTED_PLATFORM", "cloudflare") + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + want := []string{"10.0.0.0/8", "172.16.0.0/12"} + got := cfg.Server.TrustedProxies + if len(got) != len(want) { + t.Fatalf("TrustedProxies = %v want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("TrustedProxies[%d] = %q want %q", i, got[i], want[i]) + } + } + if cfg.Server.TrustedPlatformHeader() != "CF-Connecting-IP" { + t.Errorf("TrustedPlatformHeader() = %q want CF-Connecting-IP", cfg.Server.TrustedPlatformHeader()) + } +} diff --git a/internal/services/auth_service.go b/internal/services/auth_service.go index 5765768c..4b259459 100644 --- a/internal/services/auth_service.go +++ b/internal/services/auth_service.go @@ -90,7 +90,7 @@ func (s *authService) Login(req request.LoginRequest, authCfg config.AuthConfig, return nil, errorsx.InvalidParamI18n("error.e0258") } - if s.isCredentialLocked(principal, authCfg) { + if s.isCredentialLocked(principal, clientIP, authCfg) { _ = s.createLoginCredentialLog(principal, 0, false, clientIP, userAgent, "credential locked") return nil, errorsx.CredentialLockedI18n("error.e0270") } @@ -455,28 +455,71 @@ func (s *authService) createLoginCredentialLog(principal string, userID int64, s Principal: principal, UserID: userID, Success: success, - ClientIP: clientIP, + ClientIP: normalizeClientIP(clientIP), UserAgent: userAgent, Reason: reason, CreatedAt: time.Now(), }) } -func (s *authService) isCredentialLocked(principal string, authCfg config.AuthConfig) bool { - maxFailedAttempts := authCfg.MaxFailedAttempts - if maxFailedAttempts <= 0 { - return false - } +// isCredentialLocked reports whether this attempt should be refused before the +// password is even checked. +// +// Both windows are keyed on the client address as well as the username. Keying on +// the username alone - which is what this used to do - means anyone who knows a +// username can lock the real account out for the whole window, from anywhere, as +// often as they like: that is a denial of service dressed up as a protection. The +// principal-and-address window still stops one source grinding on one account, and +// the address-only window across all principals still stops credential stuffing, +// but a legitimate user arriving from a different address is no longer locked out +// by somebody else's failures. +// +// This is only sound because the address itself is trustworthy; see the trusted +// proxy configuration in bootstrap.NewServer. +func (s *authService) isCredentialLocked(principal, clientIP string, authCfg config.AuthConfig) bool { lockMinute := authCfg.CredentialLockMinute if lockMinute <= 0 { lockMinute = 15 } since := time.Now().Add(-time.Duration(lockMinute) * time.Minute) - return LoginCredentialLogService.Count(sqls.NewCnd(). - Eq("principal", normalizeLoginPrincipal(principal)). - Eq("success", false). - NotEq("reason", "credential locked"). - Where("created_at >= ?", since)) >= int64(maxFailedAttempts) + ip := normalizeClientIP(clientIP) + + base := func() *sqls.Cnd { + return sqls.NewCnd(). + Eq("client_ip", ip). + Eq("success", false). + NotEq("reason", "credential locked"). + Where("created_at >= ?", since) + } + + if maxFailedAttempts := authCfg.MaxFailedAttempts; maxFailedAttempts > 0 { + count := LoginCredentialLogService.Count(base().Eq("principal", normalizeLoginPrincipal(principal))) + if count >= int64(maxFailedAttempts) { + return true + } + } + + // An undetermined address would otherwise pool every such caller into one + // bucket and lock them all out together. + if maxPerIP := authCfg.MaxFailedAttemptsPerIPOrDefault(); maxPerIP > 0 && ip != unknownClientIP { + if count := LoginCredentialLogService.Count(base()); count >= int64(maxPerIP) { + return true + } + } + + return false +} + +// unknownClientIP stands in for an address the server could not determine, so +// those attempts share one bucket on the principal window instead of matching +// nothing at all. +const unknownClientIP = "unknown" + +func normalizeClientIP(clientIP string) string { + if clientIP = strings.TrimSpace(clientIP); clientIP == "" { + return unknownClientIP + } + return clientIP } func normalizeLoginPrincipal(principal string) string { diff --git a/internal/services/auth_service_test.go b/internal/services/auth_service_test.go index a7f5fa11..596e3b6f 100644 --- a/internal/services/auth_service_test.go +++ b/internal/services/auth_service_test.go @@ -112,6 +112,7 @@ func TestAuthServiceLoginCredentialLockout(t *testing.T) { Principal: "admin", UserID: user.ID, Success: false, + ClientIP: "127.0.0.1", Reason: "password mismatch", CreatedAt: now.Add(-time.Duration(i+1) * time.Minute), }).Error; err != nil { @@ -122,6 +123,7 @@ func TestAuthServiceLoginCredentialLockout(t *testing.T) { Principal: "admin", UserID: user.ID, Success: false, + ClientIP: "127.0.0.1", Reason: "password mismatch", CreatedAt: now.Add(-30 * time.Minute), }).Error; err != nil { @@ -164,6 +166,7 @@ func TestAuthServiceCredentialLockoutDoesNotExtendWhileLocked(t *testing.T) { Principal: "admin", UserID: 1, Success: false, + ClientIP: "127.0.0.1", Reason: "password mismatch", CreatedAt: now.Add(-2 * time.Minute), }, @@ -171,6 +174,7 @@ func TestAuthServiceCredentialLockoutDoesNotExtendWhileLocked(t *testing.T) { Principal: "admin", UserID: 0, Success: false, + ClientIP: "127.0.0.1", Reason: "credential locked", CreatedAt: now.Add(-1 * time.Minute), }, @@ -199,6 +203,7 @@ func TestAuthServiceCredentialLockoutNormalizesPrincipalCase(t *testing.T) { Principal: "admin", UserID: 1, Success: false, + ClientIP: "127.0.0.1", Reason: "password mismatch", CreatedAt: time.Now().Add(-time.Minute), }).Error; err != nil { @@ -223,6 +228,79 @@ func TestAuthServiceCredentialLockoutNormalizesPrincipalCase(t *testing.T) { } } +// TestAuthServiceCredentialLockoutIsScopedToTheClientAddress is the regression +// test for the denial of service. Keyed on the username alone, anybody who knew a +// username could lock the real account out for the whole window, from anywhere, +// as often as they liked. +func TestAuthServiceCredentialLockoutIsScopedToTheClientAddress(t *testing.T) { + db := setupAuthServiceTestDB(t) + user := createAuthTestUser(t, db, "admin", "secret") + now := time.Now() + for i := 0; i < 5; i++ { + if err := db.Create(&models.LoginCredentialLog{ + Principal: "admin", + UserID: user.ID, + Success: false, + ClientIP: "203.0.113.66", + Reason: "password mismatch", + CreatedAt: now.Add(-time.Duration(i+1) * time.Minute), + }).Error; err != nil { + t.Fatalf("seed credential log: %v", err) + } + } + + authCfg := config.AuthConfig{TokenTTLHours: 2, MaxFailedAttempts: 3, CredentialLockMinute: 15} + + if _, err := newAuthService().Login(request.LoginRequest{Username: "admin", Password: "secret"}, authCfg, "203.0.113.66", "go-test"); !hasCode(err, errorsx.CodeAuthCredentialLocked) { + t.Fatalf("expected the attacking address to be locked, got %v", err) + } + + ret, err := newAuthService().Login(request.LoginRequest{Username: "admin", Password: "secret"}, authCfg, "198.51.100.7", "go-test") + if err != nil { + t.Fatalf("the legitimate owner was locked out by somebody else's failures: %v", err) + } + if ret == nil || ret.AccessToken == "" { + t.Fatalf("expected a session for the legitimate owner, got %+v", ret) + } +} + +// TestAuthServiceCredentialLockoutByAddressAcrossPrincipals covers the window that +// replaces the removed account-wide lock: one address spraying many usernames. +func TestAuthServiceCredentialLockoutByAddressAcrossPrincipals(t *testing.T) { + db := setupAuthServiceTestDB(t) + createAuthTestUser(t, db, "admin", "secret") + now := time.Now() + for i, principal := range []string{"admin", "root", "operator", "support"} { + if err := db.Create(&models.LoginCredentialLog{ + Principal: principal, + UserID: 0, + Success: false, + ClientIP: "203.0.113.66", + Reason: "user not found", + CreatedAt: now.Add(-time.Duration(i+1) * time.Minute), + }).Error; err != nil { + t.Fatalf("seed credential log: %v", err) + } + } + + authCfg := config.AuthConfig{ + TokenTTLHours: 2, + MaxFailedAttempts: 10, + MaxFailedAttemptsPerIP: 4, + CredentialLockMinute: 15, + } + + // No single username has reached MaxFailedAttempts, so only the per-address + // window can catch this. + if _, err := newAuthService().Login(request.LoginRequest{Username: "admin", Password: "secret"}, authCfg, "203.0.113.66", "go-test"); !hasCode(err, errorsx.CodeAuthCredentialLocked) { + t.Fatalf("expected credential stuffing from one address to be locked, got %v", err) + } + + if _, err := newAuthService().Login(request.LoginRequest{Username: "admin", Password: "secret"}, authCfg, "198.51.100.7", "go-test"); err != nil { + t.Fatalf("an unrelated address was locked by another address's failures: %v", err) + } +} + func TestAuthServiceCredentialLockoutDisabledWhenMaxAttemptsNonPositive(t *testing.T) { db := setupAuthServiceTestDB(t) user := createAuthTestUser(t, db, "admin", "secret") @@ -232,6 +310,7 @@ func TestAuthServiceCredentialLockoutDisabledWhenMaxAttemptsNonPositive(t *testi Principal: "admin", UserID: user.ID, Success: false, + ClientIP: "127.0.0.1", Reason: "credential locked", CreatedAt: now.Add(-time.Duration(i+1) * time.Minute), }).Error; err != nil {