From b7480369e86875b561d53abdbb9a806fd8bc57df Mon Sep 17 00:00:00 2001 From: JOY <5027251+JOY@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:35:07 +0700 Subject: [PATCH] fix(security): make the client IP trustworthy and scope credential lockout to it Two coupled defects. The second could not be fixed safely before the first. Gin was never told which proxies to trust. Nothing in the repository calls SetTrustedProxies or sets TrustedPlatform, so the engine ran on Gin's own default of 0.0.0.0/0 and ::/0 - every peer is a trusted proxy. validateHeader then walks X-Forwarded-For right to left and stops at the first address that is *not* a trusted proxy; with everything trusted it never stops, and returns the leftmost value, which is the one the caller supplied. Any client could therefore choose the address recorded in t_login_credential_log.client_ip and t_user.last_login_ip by sending one header. X-Real-IP is in Gin's default RemoteIPHeaders too, so it was a second route to the same result. - ServerConfig gains trustedProxies and trustedPlatform. Left unset, trustedProxies falls back to the loopback, RFC1918, IPv6 unique-local and link-local ranges rather than to Gin's trust-everything default. That covers a sidecar tunnel, a compose network and a local nginx, and a client on the public internet cannot present one of those addresses as its direct peer. - trustedPlatform maps "cloudflare", "fly.io" and "google-app-engine" onto Gin's own constants and passes any other value through as a literal header name. It defaults to empty on purpose: an edge that appends rather than overwrites would turn the header back into a client-controlled value. - NewServer applies both before any middleware is registered, and fails startup on an unparseable CIDR rather than quietly dropping the trust boundary. - Documented in config/config.example.yaml, and .env.example sets TRUSTED_PLATFORM=cloudflare because this deployment sits behind a Cloudflare Tunnel. With an address that can be believed, credential lockout stops being a denial of service. isCredentialLocked keyed on the username alone, so anybody who knew a username could lock the real account out for the whole window, from anywhere, as often as they liked - and the window was renewable indefinitely. - The per-account window is now keyed on principal *and* client address, so one source grinding on one account is still stopped. - A second window counts failures from one address across all principals, which is what credential stuffing looks like. auth.maxFailedAttemptsPerIP configures it and defaults to four times maxFailedAttempts; disabling maxFailedAttempts disables both, matching the existing convention. - An address the server could not determine is normalised to a single "unknown" bucket and excluded from the per-address window, so a missing IP cannot pool every such caller into one lockout. - createLoginCredentialLog normalises on write so the read and write sides cannot drift apart. Tests. TestGinDefaultTrustsEveryProxy pins the vulnerable default, so the fix stays load-bearing and a future Gin that changes the default says so instead of silently making the configuration decorative. Three more cover an untrusted peer's forged header being ignored, a trusted proxy's appended address beating client-prepended junk, and the platform header taking precedence over X-Forwarded-For. Two lockout tests cover the denial of service being closed and the per-address window catching a username spray that no single account trips. Four existing lockout tests seeded credential logs without a client address, which is exactly the assumption this removes, so they now seed one. --- .env.example | 11 ++ config/config.example.yaml | 12 ++ internal/bootstrap/server.go | 13 ++ internal/bootstrap/server_clientip_test.go | 148 +++++++++++++++++++++ internal/pkg/config/config.go | 84 +++++++++++- internal/pkg/config/config_test.go | 104 +++++++++++++++ internal/services/auth_service.go | 67 ++++++++-- internal/services/auth_service_test.go | 79 +++++++++++ 8 files changed, 505 insertions(+), 13 deletions(-) create mode 100644 internal/bootstrap/server_clientip_test.go 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 {