From 10eab440e75deccbb1ea52bc79eb24ab9308f3e8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 10:25:41 +0200 Subject: [PATCH] Resolve real client IP behind trusted proxies (closes #94) RFC1918 ranges are the default trusted proxy set on an omitted key; an explicit list replaces the default; an explicit empty list trusts no one; unparseable values abort startup; forwarded headers honored only from trusted peers. Independent review passed: https://git.eeqj.de/sneak/pixa/pulls/127#issuecomment-98737 model: claude-opus-4-8 (implementation and review); merged by claude-fable-5 --- README.md | 12 ++ TODO.md | 13 ++ config.example.yml | 15 ++ internal/clientip/clientip.go | 119 +++++++++++ internal/clientip/clientip_test.go | 189 ++++++++++++++++++ internal/config/config.go | 80 ++++++-- .../config/trusted_proxies_internal_test.go | 85 ++++++++ internal/handlers/auth.go | 7 +- .../handlers/auth_clientip_internal_test.go | 41 ++++ .../middleware/client_ip_internal_test.go | 118 +++++++++++ internal/middleware/middleware.go | 35 ++-- internal/server/routes.go | 1 + 12 files changed, 678 insertions(+), 37 deletions(-) create mode 100644 internal/clientip/clientip.go create mode 100644 internal/clientip/clientip_test.go create mode 100644 internal/config/trusted_proxies_internal_test.go create mode 100644 internal/handlers/auth_clientip_internal_test.go create mode 100644 internal/middleware/client_ip_internal_test.go diff --git a/README.md b/README.md index ac27ec4..fe32320 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,18 @@ Configured via YAML file (`--config`). Key settings: added to the always-enforced built-in ranges (loopback, private, link-local, CGNAT, benchmark, NAT64, and the like); an invalid CIDR aborts startup +- `trusted_proxies` — list of CIDR ranges of the reverse proxies in front + of pixa. `X-Forwarded-For` is believed only when the direct peer falls + inside one of these ranges; the logged and login-recorded client + address is then the rightmost forwarded entry that is not itself a + trusted proxy. Otherwise the direct peer address is used and the header + is ignored, so a client connecting directly cannot spoof its address. + An omitted key defaults to the RFC 1918 private ranges (`10.0.0.0/8`, + `172.16.0.0/12`, `192.168.0.0/16`), since pixa is deployed behind a + proxy on a private network; an explicitly empty list (`[]`) trusts no + one, and an explicit list replaces the default. An invalid CIDR aborts + startup. Set this to your proxy's address range if it is not already + covered by the defaults - `upstream_fetch_timeout` — timeout for origin requests - `upstream_max_response_size` — max origin response size - `downstream_timeout` — client response timeout diff --git a/TODO.md b/TODO.md index c345355..715ad1b 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,19 @@ exhaustion # Completed Steps +- 2026-09-21 trusted-proxy client IP resolution (closes #94): a + `trusted_proxies` config key taking a list of CIDRs, parsed by the same + `net/netip` list parser as `blocked_networks` (an invalid entry aborts + startup naming the key and value; an omitted key defaults to the RFC 1918 + private ranges, an explicitly empty list trusts no one, and an explicit + list replaces the default); a new + `internal/clientip` package resolves the client address by honoring + `X-Forwarded-For` only when the direct peer is a trusted proxy, walking + the chain right-to-left to the rightmost non-proxy entry, so a client + connecting directly cannot spoof its address; the resolved address is + stored in the request context by a new middleware and used by the + request-logging middleware and the login-attempt logs in place of the + raw peer address; documented in `README.md` and `config.example.yml`. - 2026-09-21 blocked networks configuration extending SSRF protection: a `blocked_networks` config key taking a list of CIDRs (parsed with `net/netip`, an invalid entry aborts startup naming the key and value), diff --git a/config.example.yml b/config.example.yml index e3fbd4a..09f415f 100644 --- a/config.example.yml +++ b/config.example.yml @@ -31,6 +31,21 @@ allowlist_hosts: # - 100.64.0.0/10 # - 2001:db8::/32 +# CIDR ranges of the reverse proxies in front of pixa. X-Forwarded-For +# is believed only when the direct peer is inside one of these ranges; +# the client address in the access log and login records is then the +# rightmost forwarded entry that is not itself a trusted proxy. A client +# connecting directly (peer outside these ranges) cannot spoof its +# address: the header is ignored and the peer address is used. When +# omitted, this defaults to the RFC 1918 private ranges (10.0.0.0/8, +# 172.16.0.0/12, 192.168.0.0/16), since pixa is deployed behind a proxy on +# a private network. An explicitly empty list ([]) trusts no one; an +# explicit list replaces the default. An invalid CIDR aborts startup. +# Uncomment to override the defaults with your proxy's address range. +# trusted_proxies: +# - 10.0.0.0/8 +# - 2001:db8::/32 + # Allow HTTP upstream (only for testing, always use HTTPS in production) allow_http: false diff --git a/internal/clientip/clientip.go b/internal/clientip/clientip.go new file mode 100644 index 0000000..49c2ea7 --- /dev/null +++ b/internal/clientip/clientip.go @@ -0,0 +1,119 @@ +// Package clientip resolves the real client IP address of an HTTP request +// when pixa runs behind a reverse proxy. Forwarding headers are believed +// only when the immediate peer is a configured trusted proxy, so an +// untrusted client cannot spoof its address by sending the header. +package clientip + +import ( + "context" + "net" + "net/netip" + "slices" + "strings" +) + +// ForwardedForHeader is the request header carrying the proxy chain. It is +// honored only when the immediate peer is a trusted proxy. +const ForwardedForHeader = "X-Forwarded-For" + +// Resolver determines the client IP of a request against a fixed set of +// trusted proxy networks. +type Resolver struct { + trusted []netip.Prefix +} + +// NewResolver returns a Resolver that trusts forwarding headers only from +// peers inside the given CIDR ranges. A nil or empty list trusts no one, +// so the peer address is always used. +func NewResolver(trusted []netip.Prefix) *Resolver { + return &Resolver{trusted: trusted} +} + +// Resolve returns the client IP for a request whose direct peer is +// remoteAddr (a "host:port" string as in http.Request.RemoteAddr) and +// whose X-Forwarded-For header lines are forwardedFor (as returned by +// http.Header.Values). When the peer is not a trusted proxy, the peer +// address is returned and the header is ignored entirely. When the peer is +// trusted, the header is walked right to left and the first address that is +// not itself a trusted proxy is returned; this is the client the outermost +// trusted proxy observed, and entries an untrusted client may have prepended +// sit to its left and are never reached. +func (r *Resolver) Resolve(remoteAddr string, forwardedFor []string) string { + peer := hostOnly(remoteAddr) + + peerAddr, err := netip.ParseAddr(peer) + if err != nil || !r.isTrusted(peerAddr) { + return peer + } + + for _, hop := range slices.Backward(forwardedForChain(forwardedFor)) { + hopAddr, err := netip.ParseAddr(hop) + if err != nil || r.isTrusted(hopAddr) { + continue + } + + return hopAddr.String() + } + + return peerAddr.String() +} + +// isTrusted reports whether addr falls inside one of the trusted proxy +// ranges. Addresses are unmapped first so an IPv4-mapped IPv6 form matches +// an IPv4 range, matching the fetcher's blocklist comparison. +func (r *Resolver) isTrusted(addr netip.Addr) bool { + if !addr.IsValid() { + return false + } + + unmapped := addr.Unmap() + + return slices.ContainsFunc(r.trusted, func(prefix netip.Prefix) bool { + return prefix.Contains(unmapped) + }) +} + +// hostOnly strips the port from a "host:port" address. A value without a +// port (already a bare host) is returned unchanged. +func hostOnly(remoteAddr string) string { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + return remoteAddr + } + + return host +} + +// forwardedForChain flattens the comma-separated entries of every +// X-Forwarded-For header line into a single ordered, trimmed list. +func forwardedForChain(values []string) []string { + var chain []string + + for _, value := range values { + for part := range strings.SplitSeq(value, ",") { + trimmed := strings.TrimSpace(part) + if trimmed != "" { + chain = append(chain, trimmed) + } + } + } + + return chain +} + +// contextKey is the private key type under which the resolved client IP is +// stored in a request context. +type contextKey struct{} + +// WithClientIP returns a copy of ctx carrying the resolved client IP. +func WithClientIP(ctx context.Context, ip string) context.Context { + return context.WithValue(ctx, contextKey{}, ip) +} + +// FromContext returns the resolved client IP stored in ctx, or an empty +// string if none was set. +func FromContext(ctx context.Context) string { + ip, _ := ctx.Value(contextKey{}).(string) + + return ip +} diff --git a/internal/clientip/clientip_test.go b/internal/clientip/clientip_test.go new file mode 100644 index 0000000..ef3a839 --- /dev/null +++ b/internal/clientip/clientip_test.go @@ -0,0 +1,189 @@ +package clientip_test + +import ( + "net/netip" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" +) + +// Addresses reused across the resolver cases. +const ( + trustedRangeV4 = "10.0.0.0/8" + forwardedV4 = "203.0.113.7" + untrustedV4 = "198.51.100.9" + trustedPeer = "10.0.0.1:5000" +) + +// mustPrefixes parses CIDR strings into prefixes for building a resolver. +func mustPrefixes(t *testing.T, cidrs ...string) []netip.Prefix { + t.Helper() + + prefixes := make([]netip.Prefix, 0, len(cidrs)) + + for _, c := range cidrs { + p, err := netip.ParsePrefix(c) + if err != nil { + t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err) + } + + prefixes = append(prefixes, p) + } + + return prefixes +} + +type resolveCase struct { + name string + trusted []string + remoteAddr string + forwardedFor []string + want string +} + +// runResolveCases runs each case against a resolver built from its trusted +// list and checks the resolved address. +func runResolveCases(t *testing.T, cases []resolveCase) { + t.Helper() + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := clientip.NewResolver(mustPrefixes(t, tt.trusted...)) + + got := r.Resolve(tt.remoteAddr, tt.forwardedFor) + if got != tt.want { + t.Errorf("Resolve(%q, %v) = %q, want %q", + tt.remoteAddr, tt.forwardedFor, got, tt.want) + } + }) + } +} + +// TestResolvePeerTrust covers the trust decision on the direct peer: a +// forwarded header is believed only from a trusted peer, and a client +// connecting directly cannot spoof its address. +func TestResolvePeerTrust(t *testing.T) { + t.Parallel() + + runResolveCases(t, []resolveCase{ + { + name: "trusted peer honors forwarded client", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + { + name: "untrusted peer ignores forwarded header", + trusted: []string{trustedRangeV4}, + remoteAddr: untrustedV4 + ":33333", + forwardedFor: []string{forwardedV4}, + want: untrustedV4, + }, + { + name: "spoofed chain from untrusted peer cannot influence result", + trusted: []string{trustedRangeV4}, + remoteAddr: untrustedV4 + ":33333", + forwardedFor: []string{"1.2.3.4, 10.9.9.9, 127.0.0.1"}, + want: untrustedV4, + }, + { + name: "empty trusted list always uses peer", + trusted: nil, + remoteAddr: forwardedV4 + ":80", + forwardedFor: []string{"10.0.0.5"}, + want: forwardedV4, + }, + { + name: "trusted peer with no forwarded header uses peer", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: nil, + want: "10.0.0.1", + }, + { + name: "unparseable peer is returned unchanged", + trusted: []string{trustedRangeV4}, + remoteAddr: "garbage", + forwardedFor: []string{forwardedV4}, + want: "garbage", + }, + }) +} + +// TestResolveChainWalk covers walking the X-Forwarded-For chain from a +// trusted peer to the rightmost entry that is not itself a trusted proxy. +func TestResolveChainWalk(t *testing.T) { + t.Parallel() + + runResolveCases(t, []resolveCase{ + { + name: "rightmost untrusted entry across a mixed chain", + trusted: []string{trustedRangeV4, "192.168.0.0/16"}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4 + ", 192.168.1.1, 10.0.0.2"}, + want: forwardedV4, + }, + { + name: "spoofed client behind a trusted proxy is not believed", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{"1.2.3.4, " + untrustedV4}, + want: untrustedV4, + }, + { + name: "chain split across multiple header lines", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4, "10.0.0.2"}, + want: forwardedV4, + }, + { + name: "garbage entries are skipped", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{forwardedV4 + ", not-an-ip"}, + want: forwardedV4, + }, + { + name: "all-trusted chain falls back to peer", + trusted: []string{trustedRangeV4}, + remoteAddr: trustedPeer, + forwardedFor: []string{"10.0.0.9, 10.0.0.2"}, + want: "10.0.0.1", + }, + { + name: "trusted IPv6 peer honors forwarded client", + trusted: []string{"2001:db8::/32"}, + remoteAddr: "[2001:db8::1]:9000", + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + { + name: "IPv4-mapped peer matches IPv4 trusted range", + trusted: []string{trustedRangeV4}, + remoteAddr: "[::ffff:10.0.0.1]:5000", + forwardedFor: []string{forwardedV4}, + want: forwardedV4, + }, + }) +} + +func TestContextRoundTrip(t *testing.T) { + t.Parallel() + + ctx := clientip.WithClientIP(t.Context(), forwardedV4) + if got := clientip.FromContext(ctx); got != forwardedV4 { + t.Errorf("FromContext = %q, want %q", got, forwardedV4) + } +} + +func TestFromContextAbsent(t *testing.T) { + t.Parallel() + + if got := clientip.FromContext(t.Context()); got != "" { + t.Errorf("FromContext with no value = %q, want empty", got) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index c8c1d60..ea15bf7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -44,6 +44,7 @@ const ( keyUpstreamConnectionsPerHost = "upstream_connections_per_host" keyCacheMaxBytes = "cache_max_bytes" keyBlockedNetworks = "blocked_networks" + keyTrustedProxies = "trusted_proxies" ) // placeholderSigningKey is the dummy signing_key shipped in @@ -117,6 +118,17 @@ type Config struct { // fetcher's dialer; the built-in ranges always apply. BlockedNetworks []netip.Prefix + // TrustedProxies are the CIDR ranges of reverse proxies whose + // forwarding headers may be believed. Forwarded headers are honored + // only when the immediate peer falls inside one of these ranges; + // otherwise the peer address is used and the headers are ignored, so + // an untrusted client cannot spoof its address. An omitted key + // defaults to the RFC 1918 private ranges (see defaultTrustedProxies), + // since pixa is deployed behind a proxy on a private network; an + // explicitly empty list trusts nothing and always uses the peer + // address, and an explicit list replaces the default. + TrustedProxies []netip.Prefix + // CacheMaxBytes is the disk cache size limit in bytes. Zero // disables the disk cache entirely. When cache_max_bytes is // omitted from the configuration, this holds the computed default @@ -185,11 +197,24 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { } } - blockedNetworks, err := getBlockedNetworks(sc) + blockedNetworks, err := parseCIDRList(sc, keyBlockedNetworks) if err != nil { return nil, err } + trustedProxies, err := parseCIDRList(sc, keyTrustedProxies) + if err != nil { + return nil, err + } + + // parseCIDRList returns a nil slice only when the key is absent; an + // explicitly empty list ([]) comes back non-nil and empty. An omitted + // key takes the RFC 1918 default, while an explicit empty list is left + // as trust-nothing. + if trustedProxies == nil { + trustedProxies = defaultTrustedProxies() + } + loader := &strictLoader{sc: sc} c := &Config{ @@ -207,6 +232,7 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), BlockedNetworks: blockedNetworks, + TrustedProxies: trustedProxies, } // The computed default for cache_max_bytes needs a validated @@ -322,7 +348,8 @@ func isKnownConfigKey(key string) bool { switch key { case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, - keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, "env": + keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, + keyTrustedProxies, "env": return true } @@ -817,27 +844,42 @@ func getStringSlice(sc *smartconfig.Config) []string { return nil } -// getBlockedNetworks parses the blocked_networks value into CIDR prefixes, -// or returns nil if the key is omitted. It accepts a YAML list of strings -// or a comma-separated string. An explicitly null value, a wrong type, an -// empty entry, a non-string entry, or an unparseable CIDR aborts startup -// naming the key and the offending value; a default (the built-in -// blocklist alone) applies only to an omitted key. -func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) { +// defaultTrustedProxies returns the trusted_proxies default: the three RFC +// 1918 private ranges. pixa is always deployed behind a TLS-terminating +// reverse proxy, which in practice sits on a private network, so its +// forwarding headers are believed unless the operator says otherwise. +// Loopback is deliberately excluded: it is not an RFC 1918 range, and no +// deployment reaches pixa over it. A fresh slice is returned on each call so +// callers may hold it without aliasing shared state. +func defaultTrustedProxies() []netip.Prefix { + return []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/8"), + netip.MustParsePrefix("172.16.0.0/12"), + netip.MustParsePrefix("192.168.0.0/16"), + } +} + +// parseCIDRList parses the value of the named config key into CIDR +// prefixes, or returns nil if the key is omitted. It accepts a YAML list +// of strings or a comma-separated string. An explicitly null value, a +// wrong type, an empty entry, a non-string entry, or an unparseable CIDR +// aborts startup naming the key and the offending value; the default +// (an empty list) applies only to an omitted key. +func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { if sc == nil { return nil, nil } - raw, ok := sc.Get(keyBlockedNetworks) + raw, ok := sc.Get(key) if !ok { return nil, nil } if raw == nil { - return nil, errNullConfigValue(keyBlockedNetworks) + return nil, errNullConfigValue(key) } - entries, err := blockedNetworkEntries(raw) + entries, err := cidrListEntries(raw, key) if err != nil { return nil, err } @@ -848,7 +890,7 @@ func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) { prefix, err := netip.ParsePrefix(entry) if err != nil { return nil, fmt.Errorf("config key %q: value %q is %w", - keyBlockedNetworks, entry, errNotAValidCIDR) + key, entry, errNotAValidCIDR) } prefixes = append(prefixes, prefix) @@ -857,10 +899,10 @@ func getBlockedNetworks(sc *smartconfig.Config) ([]netip.Prefix, error) { return prefixes, nil } -// blockedNetworkEntries extracts the raw blocked_networks entries as +// cidrListEntries extracts the raw entries of the named CIDR-list key as // trimmed, non-empty strings, from either a YAML list of strings or a // comma-separated string. Any other shape is a configuration error. -func blockedNetworkEntries(raw any) ([]string, error) { +func cidrListEntries(raw any, key string) ([]string, error) { switch val := raw.(type) { case []any: entries := make([]string, 0, len(val)) @@ -869,12 +911,12 @@ func blockedNetworkEntries(raw any) ([]string, error) { str, ok := item.(string) if !ok { return nil, fmt.Errorf("config key %q: list entry %v (%T) is %w", - keyBlockedNetworks, item, item, errNotAString) + key, item, item, errNotAString) } if strings.TrimSpace(str) == "" { return nil, fmt.Errorf("config key %q: %w", - keyBlockedNetworks, errEmptyListEntry) + key, errEmptyListEntry) } entries = append(entries, strings.TrimSpace(str)) @@ -888,7 +930,7 @@ func blockedNetworkEntries(raw any) ([]string, error) { trimmed := strings.TrimSpace(part) if trimmed == "" { return nil, fmt.Errorf("config key %q: value %q %w", - keyBlockedNetworks, val, errEmptyEntry) + key, val, errEmptyEntry) } entries = append(entries, trimmed) @@ -897,6 +939,6 @@ func blockedNetworkEntries(raw any) ([]string, error) { return entries, nil default: return nil, fmt.Errorf("config key %q: value %v (%T) is %w", - keyBlockedNetworks, raw, raw, errNotAStringList) + key, raw, raw, errNotAStringList) } } diff --git a/internal/config/trusted_proxies_internal_test.go b/internal/config/trusted_proxies_internal_test.go new file mode 100644 index 0000000..ae61bfd --- /dev/null +++ b/internal/config/trusted_proxies_internal_test.go @@ -0,0 +1,85 @@ +package config + +import ( + "strings" + "testing" +) + +// TestTrustedProxiesConfig checks the trusted_proxies key wiring: an +// explicit CIDR list lands in TrustedProxies in order and replaces the +// default, an omitted key falls back to the RFC 1918 private ranges, and an +// explicitly empty list trusts no one. The list parser itself is shared with +// blocked_networks and is exercised in depth by that key's tests. +func TestTrustedProxiesConfig(t *testing.T) { + t.Parallel() + + t.Run("explicit list replaces the default in order", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, + signingKeyLine+`trusted_proxies: ["10.0.0.0/8", "2001:db8::/32"]`+"\n") + if err != nil { + t.Fatalf("valid trusted_proxies should load: %v", err) + } + + got := make([]string, len(c.TrustedProxies)) + for i, p := range c.TrustedProxies { + got[i] = p.String() + } + + if joined := strings.Join(got, ","); joined != "10.0.0.0/8,2001:db8::/32" { + t.Errorf("TrustedProxies = %v, want the two ranges in order", got) + } + }) + + t.Run("omitted key defaults to the RFC 1918 ranges", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("minimal config should load: %v", err) + } + + got := make([]string, len(c.TrustedProxies)) + for i, p := range c.TrustedProxies { + got[i] = p.String() + } + + want := "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" + if joined := strings.Join(got, ","); joined != want { + t.Errorf("TrustedProxies = %v, want the RFC 1918 ranges %q", got, want) + } + }) + + t.Run("explicitly empty list trusts no one", func(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+"trusted_proxies: []\n") + if err != nil { + t.Fatalf("empty trusted_proxies should load: %v", err) + } + + if len(c.TrustedProxies) != 0 { + t.Errorf("TrustedProxies = %v, want empty", c.TrustedProxies) + } + }) +} + +// TestTrustedProxiesInvalidAbortsStartup checks that an invalid or null +// value aborts startup with an error naming the key and the offending value. +func TestTrustedProxiesInvalidAbortsStartup(t *testing.T) { + t.Parallel() + + runAbortCases(t, []abortCase{ + { + name: "invalid cidr", + yaml: signingKeyLine + `trusted_proxies: ["999.0.0.0/8"]` + "\n", + wantErrSubstrings: []string{keyTrustedProxies, "999.0.0.0/8"}, + }, + { + name: "null value", + yaml: signingKeyLine + "trusted_proxies:\n", + wantErrSubstrings: []string{keyTrustedProxies, nullValueText}, + }, + }) +} diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 5e74e1f..5ea2a32 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -8,6 +8,7 @@ import ( "strconv" "time" + "sneak.berlin/go/pixa/internal/clientip" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/templates" @@ -47,7 +48,8 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { // Constant-time comparison to prevent timing attacks if subtle.ConstantTimeCompare([]byte(submittedKey), []byte(s.config.SigningKey)) != 1 { - s.log.Warn("failed login attempt", "remote_addr", r.RemoteAddr) + s.log.Warn("failed login attempt", + "remote_addr", clientip.FromContext(r.Context())) s.renderLogin(w, r, "Invalid signing key") return @@ -62,7 +64,8 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) { return } - s.log.Info("successful login", "remote_addr", r.RemoteAddr) + s.log.Info("successful login", + "remote_addr", clientip.FromContext(r.Context())) // Redirect to generator page http.Redirect(w, r, "/", http.StatusSeeOther) diff --git a/internal/handlers/auth_clientip_internal_test.go b/internal/handlers/auth_clientip_internal_test.go new file mode 100644 index 0000000..91d3fd2 --- /dev/null +++ b/internal/handlers/auth_clientip_internal_test.go @@ -0,0 +1,41 @@ +package handlers + +import ( + "bytes" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" + "sneak.berlin/go/pixa/internal/config" +) + +// TestFailedLoginLogsResolvedClientIP verifies the failed-login record +// carries the resolved client IP from the request context, not the raw +// proxy peer address. +func TestFailedLoginLogsResolvedClientIP(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + h := &Handlers{ + log: slog.New(slog.NewJSONHandler(&buf, nil)), + config: &config.Config{SigningKey: testSigningKey}, + } + + form := url.Values{loginKeyField: {"wrong-key"}} + req := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/", + strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(clientip.WithClientIP(req.Context(), "203.0.113.7")) + + h.handleLoginPost(httptest.NewRecorder(), req) + + if !strings.Contains(buf.String(), `"remote_addr":"203.0.113.7"`) { + t.Errorf("failed-login log missing resolved client IP; got %q", buf.String()) + } +} diff --git a/internal/middleware/client_ip_internal_test.go b/internal/middleware/client_ip_internal_test.go new file mode 100644 index 0000000..7200f46 --- /dev/null +++ b/internal/middleware/client_ip_internal_test.go @@ -0,0 +1,118 @@ +package middleware + +import ( + "bytes" + "log/slog" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/clientip" + "sneak.berlin/go/pixa/internal/config" +) + +// testForwardedClient is the client address the proxy forwards. +const testForwardedClient = "203.0.113.7" + +// newTestMiddleware builds a Middleware whose resolver trusts the given +// CIDRs and whose logger writes JSON to buf. +func newTestMiddleware(t *testing.T, buf *bytes.Buffer, trusted ...string) *Middleware { + t.Helper() + + prefixes := make([]netip.Prefix, 0, len(trusted)) + + for _, c := range trusted { + p, err := netip.ParsePrefix(c) + if err != nil { + t.Fatalf("netip.ParsePrefix(%q) error = %v", c, err) + } + + prefixes = append(prefixes, p) + } + + return &Middleware{ + log: slog.New(slog.NewJSONHandler(buf, nil)), + config: &config.Config{TrustedProxies: prefixes}, + clientIP: clientip.NewResolver(prefixes), + } +} + +// TestClientIPMiddlewareStoresResolvedIP verifies the ClientIP middleware +// puts the resolved address into the request context for a trusted and an +// untrusted peer. +func TestClientIPMiddlewareStoresResolvedIP(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + remoteAddr string + forwarded string + want string + }{ + { + name: "trusted peer honors forwarded client", + remoteAddr: "10.0.0.1:5000", + forwarded: testForwardedClient, + want: testForwardedClient, + }, + { + name: "untrusted peer ignores forwarded header", + remoteAddr: "198.51.100.9:5000", + forwarded: testForwardedClient, + want: "198.51.100.9", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mw := newTestMiddleware(t, &bytes.Buffer{}, "10.0.0.0/8") + + var got string + + handler := mw.ClientIP()(http.HandlerFunc( + func(_ http.ResponseWriter, r *http.Request) { + got = clientip.FromContext(r.Context()) + })) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil) + req.RemoteAddr = tt.remoteAddr + req.Header.Set("X-Forwarded-For", tt.forwarded) + + handler.ServeHTTP(httptest.NewRecorder(), req) + + if got != tt.want { + t.Errorf("client IP in context = %q, want %q", got, tt.want) + } + }) + } +} + +// TestLoggingUsesResolvedClientIP verifies the logging middleware records +// the resolved forwarded client IP rather than the proxy peer address. +func TestLoggingUsesResolvedClientIP(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + mw := newTestMiddleware(t, &buf, "10.0.0.0/8") + + handler := mw.ClientIP()(mw.Logging()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }))) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.1:5000" + req.Header.Set("X-Forwarded-For", testForwardedClient) + + handler.ServeHTTP(httptest.NewRecorder(), req) + + if !strings.Contains(buf.String(), `"remoteIP":"`+testForwardedClient+`"`) { + t.Errorf("log output missing resolved client IP; got %q", buf.String()) + } +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 00d59f7..916e3c8 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -3,7 +3,6 @@ package middleware import ( "log/slog" - "net" "net/http" "time" @@ -14,6 +13,7 @@ import ( ghmm "github.com/slok/go-http-metrics/middleware" "github.com/slok/go-http-metrics/middleware/std" "go.uber.org/fx" + "sneak.berlin/go/pixa/internal/clientip" "sneak.berlin/go/pixa/internal/config" "sneak.berlin/go/pixa/internal/logger" ) @@ -58,31 +58,34 @@ type Params struct { // Middleware provides HTTP middleware functions. type Middleware struct { - log *slog.Logger - config *config.Config + log *slog.Logger + config *config.Config + clientIP *clientip.Resolver } // New creates a new Middleware instance. func New(_ fx.Lifecycle, params Params) (*Middleware, error) { s := &Middleware{ - log: params.Logger.Get(), - config: params.Config, + log: params.Logger.Get(), + config: params.Config, + clientIP: clientip.NewResolver(params.Config.TrustedProxies), } return s, nil } -func ipFromHostPort(hp string) string { - h, _, err := net.SplitHostPort(hp) - if err != nil { - return "" +// ClientIP returns a middleware that resolves the real client IP, +// honoring X-Forwarded-For only from trusted proxies, and stores it in +// the request context for the logging middleware and handlers to read. +func (s *Middleware) ClientIP() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ip := s.clientIP.Resolve( + r.RemoteAddr, r.Header.Values(clientip.ForwardedForHeader)) + ctx := clientip.WithClientIP(r.Context(), ip) + next.ServeHTTP(w, r.WithContext(ctx)) + }) } - - if len(h) > 0 && h[0] == '[' { - return h[1 : len(h)-1] - } - - return h } type loggingResponseWriter struct { @@ -127,7 +130,7 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler { "request_id", reqID, "referer", r.Referer(), "proto", r.Proto, - "remoteIP", ipFromHostPort(r.RemoteAddr), + "remoteIP", clientip.FromContext(ctx), "status", lrw.statusCode, "response_bytes", lrw.bytesWritten, "latency_ms", latency.Milliseconds(), diff --git a/internal/server/routes.go b/internal/server/routes.go index 3b7dfc5..8e41fd7 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -18,6 +18,7 @@ func (s *Server) SetupRoutes() { s.router.Use(middleware.Recoverer) s.router.Use(middleware.RequestID) + s.router.Use(s.mw.ClientIP()) s.router.Use(s.mw.SecurityHeaders()) s.router.Use(s.mw.Logging())