From 15fe20f4d43be0eb6e6c4ef7a99af20c01f29a7f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 15:31:07 +0000 Subject: [PATCH] fix(security): stop trusting forged client IPs from the whole private range c.ClientIP() keys every IP-based rate limit and the client_ip recorded in the access log. When the connecting peer is a trusted proxy, Gin takes that value from a client-supplied X-Real-IP / X-Forwarded-For header. The trusted set was all of RFC-1918 plus loopback, and the compose file published the API port -- so traffic reaching the API directly arrived from the Docker bridge, inside 172.16.0.0/12, and the header was honoured verbatim. Verified against a running instance with rate limiting enabled: after the real source was throttled (10 x 429), 40 logins with a rotating X-Real-IP returned 0 x 429, and 30 registrations with rotating IPs all returned 201. A request carrying 'X-Real-IP: 203.0.113.77' was logged with that value as client_ip, so an attacker can also write arbitrary source addresses into the access log. The per-account lockout still capped single-account password guessing and is unaffected; what the IP limits alone guarded -- password spraying, reset and verification email flooding, registration flooding, mass account-lockout DoS -- was bypassable. Changes: - Trusted proxies are configurable via TRUSTED_PROXIES and now default to loopback only. A startup warning names the setting when it is unset. - docker-compose binds the API port to 127.0.0.1 (override with API_BIND), so the API is reached through the nginx ingress rather than directly. This also stops /metrics being reachable off-host. - TRUSTED_PROXIES documented in docker-compose.yml and .env.example. Deployments behind a reverse proxy MUST set TRUSTED_PROXIES to the ingress address, otherwise forwarded client IPs are ignored and all proxied traffic shares one rate-limit bucket. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GukWfyJMY28qv2CJjxFvKF --- .env.example | 9 +++++++ cmd/api/main.go | 47 +++++++++++++++++++++++++++++++++--- cmd/api/trustedproxy_test.go | 38 +++++++++++++++++++++++++++++ docker-compose.yml | 13 +++++++++- 4 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 cmd/api/trustedproxy_test.go diff --git a/.env.example b/.env.example index f86a6aa..331b19b 100644 --- a/.env.example +++ b/.env.example @@ -29,6 +29,15 @@ REFRESH_EXPIRES_IN=7d # ----------------------------------------------------------------------------- CORS_ORIGIN=http://localhost:5173 +# ----------------------------------------------------------------------------- +# Reverse proxy +# ----------------------------------------------------------------------------- +# Comma-separated hosts/CIDRs whose X-Real-IP / X-Forwarded-For headers are +# trusted. Name your ingress ONLY -- any client that can reach the API from +# inside a trusted range can forge its own source IP and bypass rate limiting. +# Defaults to loopback only when unset. +# TRUSTED_PROXIES=172.16.0.0/12 + # ----------------------------------------------------------------------------- # Admin # ----------------------------------------------------------------------------- diff --git a/cmd/api/main.go b/cmd/api/main.go index 88a4758..3d387b8 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -42,6 +42,24 @@ import ( // fatal logs a structured error and exits. Structured attributes (e.g. an // "error" key) may be passed after the message, matching slog's variadic API. // Used for unrecoverable startup failures where the process must fail closed. +// defaultTrustedProxies is deliberately narrow: only the loopback interface. +// Anything wider lets a client that can reach the API from inside that range +// forge X-Real-IP / X-Forwarded-For and bypass every IP-keyed rate limit. +// Deployments behind a reverse proxy must name it via TRUSTED_PROXIES. +var defaultTrustedProxies = []string{"127.0.0.1", "::1"} + +// splitAndTrim splits a comma-separated env value into non-empty trimmed items. +func splitAndTrim(raw string) []string { + parts := strings.Split(raw, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + if p = strings.TrimSpace(p); p != "" { + out = append(out, p) + } + } + return out +} + func fatal(msg string, args ...any) { slog.Error(msg, args...) os.Exit(1) @@ -303,9 +321,32 @@ func main() { // nil → the JSON default logger configured by logging.Setup above. router.Use(middleware.LoggerMiddleware(nil)) - // Trust proxy headers (X-Real-IP, X-Forwarded-For) from nginx - // so that c.ClientIP() returns the real client IP, not the proxy's address. - if err := router.SetTrustedProxies([]string{"127.0.0.1", "::1", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"}); err != nil { + // Trust proxy headers (X-Real-IP, X-Forwarded-For) ONLY from the ingress. + // + // c.ClientIP() is the key for every IP-based rate limit and the client_ip + // recorded in the access log. When the connecting peer is trusted, Gin takes + // that value from a client-supplied header — so trusting a broad range lets + // anyone whose packets arrive from inside it forge their own source IP, + // defeating the auth/admin/sign limiters and poisoning the logs. + // + // The previous default trusted all of RFC-1918. In the shipped compose + // topology the API port is published, so traffic reaching it directly + // arrives from the Docker bridge (172.16.0.0/12) — inside that range — and + // the header was honoured verbatim. + // + // TRUSTED_PROXIES should be set to the ingress address (the nginx container + // or load balancer) in any deployment where the API is not exclusively + // reached through that ingress. + trustedProxies := defaultTrustedProxies + if raw := os.Getenv("TRUSTED_PROXIES"); raw != "" { + trustedProxies = splitAndTrim(raw) + slog.Info("Trusted proxies configured", "proxies", trustedProxies) + } else { + slog.Warn("TRUSTED_PROXIES not set — falling back to loopback only; " + + "set it to your ingress address (e.g. the nginx container IP/CIDR) " + + "so forwarded client IPs are trusted from that host only") + } + if err := router.SetTrustedProxies(trustedProxies); err != nil { fatal("failed to set trusted proxies", "error", err) } router.ForwardedByClientIP = true diff --git a/cmd/api/trustedproxy_test.go b/cmd/api/trustedproxy_test.go new file mode 100644 index 0000000..708499f --- /dev/null +++ b/cmd/api/trustedproxy_test.go @@ -0,0 +1,38 @@ +package main + +import ( + "reflect" + "testing" +) + +func TestSplitAndTrim(t *testing.T) { + tests := []struct { + in string + want []string + }{ + {"172.16.0.0/12", []string{"172.16.0.0/12"}}, + {"10.1.2.3, 10.1.2.4", []string{"10.1.2.3", "10.1.2.4"}}, + {" 127.0.0.1 ,, ::1 ", []string{"127.0.0.1", "::1"}}, + {"", []string{}}, + } + for _, tt := range tests { + if got := splitAndTrim(tt.in); !reflect.DeepEqual(got, tt.want) { + t.Errorf("splitAndTrim(%q) = %v, want %v", tt.in, got, tt.want) + } + } +} + +// The default must NOT include the RFC-1918 ranges. Trusting those meant any +// client reaching the API from inside them (e.g. via the published port on the +// Docker bridge) could forge X-Real-IP and defeat every IP-keyed rate limit. +func TestDefaultTrustedProxies_ExcludesPrivateRanges(t *testing.T) { + for _, p := range defaultTrustedProxies { + switch p { + case "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16": + t.Errorf("default trusted proxies must not include the broad private range %q", p) + } + } + if len(defaultTrustedProxies) == 0 { + t.Error("expected loopback entries in the default trusted proxies") + } +} diff --git a/docker-compose.yml b/docker-compose.yml index 91b959e..a55ad50 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -46,11 +46,22 @@ services: # CORS CORS_ORIGIN: ${CORS_ORIGIN:-http://localhost:5173,http://localhost:80} + + # Hosts whose X-Real-IP / X-Forwarded-For headers are trusted. Must name + # the ingress only. On the default bridge network the nginx container is + # in 172.16.0.0/12; narrow this further if you can pin its address. + TRUSTED_PROXIES: ${TRUSTED_PROXIES:-172.16.0.0/12} # Logging LOG_LEVEL: ${LOG_LEVEL:-info} ports: - - "${API_PORT:-3000}:3000" + # Bound to loopback: the API is reached through the nginx ingress, which + # shares the ninerlog-network. Publishing it on all interfaces exposed + # /metrics unauthenticated and let clients reach the API without nginx -- + # which also meant their packets arrived from the Docker bridge, inside + # the trusted-proxy range, so X-Real-IP could be forged to defeat every + # IP-based rate limit. Override API_BIND to 0.0.0.0 only if you know why. + - "${API_BIND:-127.0.0.1}:${API_PORT:-3000}:3000" networks: - ninerlog-network volumes: