From 1a0c7be4e224751ef826e53faee7dd27b2273ad5 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Sun, 20 Sep 2026 12:12:27 +0200 Subject: [PATCH 1/2] feat(server): resolve client addresses from configurable trusted proxies Replaces LOGGING_TRUSTED_PROXY_CIDRS (unreleased, added in #1049) with SERVER_TRUSTED_PROXIES, SERVER_CLIENT_IP_HEADER, and SERVER_TRUSTED_HOPS. The old key is not accepted. --- .env.template | 32 ++- config/clientip.go | 309 ++++++++++++++++++++ config/clientip_test.go | 410 +++++++++++++++++++++++++++ config/config.example.yaml | 16 +- config/config.go | 2 +- config/config_test.go | 3 +- config/logging.go | 88 +----- config/logging_test.go | 87 ------ config/server.go | 35 +++ docs/advanced/configuration.mdx | 77 +++-- internal/app/app.go | 11 +- internal/app/init_server.go | 1 + internal/auditlog/auditlog.go | 5 - internal/auditlog/clientip.go | 124 -------- internal/auditlog/clientip_test.go | 227 --------------- internal/auditlog/factory.go | 1 - internal/auditlog/middleware.go | 2 +- internal/auditlog/middleware_test.go | 48 ++++ internal/echotest/echotest.go | 28 +- internal/server/clientip.go | 18 ++ internal/server/clientip_test.go | 46 +++ tests/e2e/auditlog_test.go | 119 ++++---- 22 files changed, 1067 insertions(+), 622 deletions(-) create mode 100644 config/clientip.go create mode 100644 config/clientip_test.go delete mode 100644 internal/auditlog/clientip.go delete mode 100644 internal/auditlog/clientip_test.go create mode 100644 internal/server/clientip.go create mode 100644 internal/server/clientip_test.go diff --git a/.env.template b/.env.template index 6269af501..b7ac5f999 100644 --- a/.env.template +++ b/.env.template @@ -11,6 +11,30 @@ # pause reading a stream for over a minute. # STREAM_STALL_TIMEOUT=60 +# Networks your own proxies sit on, comma-separated (default: empty, headers ignored). +# Until this is set, every client address GoModel reports (audit entries, rate limit +# keys, logs) is the socket peer, and forwarding headers are ignored. Entries are CIDR +# networks, bare addresses (a single host), or the presets "loopback" and "private" +# (RFC 1918 + ULA). Nothing is trusted implicitly: list the hops between your clients +# and the gateway. Prefer the one subnet your proxy sits on over "private", which +# trusts every private client to set its own forwarded address. +# SERVER_TRUSTED_PROXIES=10.42.0.0/16,loopback + +# Header a trusted proxy passes the client address in (default: X-Forwarded-For). +# X-Forwarded-For carries a chain and is resolved hop by hop; any other header +# (X-Real-IP, CF-Connecting-IP, True-Client-IP) carries one address and is taken +# verbatim, which is what you want behind Cloudflare or a cloud load balancer that +# writes the client itself. Requires SERVER_TRUSTED_PROXIES. RFC 7239 Forwarded is +# not supported. +# SERVER_CLIENT_IP_HEADER=CF-Connecting-IP + +# How many proxies sit between the client and the gateway (default: 0). With 0, the +# X-Forwarded-For chain is scanned for the nearest hop outside SERVER_TRUSTED_PROXIES. +# Set it when your edge is at a fixed depth but rotates through addresses you cannot +# enumerate: the entry at that depth is taken, the socket peer must still be trusted, +# and a shorter chain falls back to the socket peer. +# SERVER_TRUSTED_HOPS=1 + # Where the running gateway records its process id so `gomodel --reload` can find it # (default: data/gomodel.pid next to a ./data directory, otherwise the per-user data # directory). Set it per instance when several gateways share a host. Leaving this @@ -456,14 +480,6 @@ # Log only model interactions, skip /health, /metrics, /admin endpoints (default: true) # LOGGING_ONLY_MODEL_INTERACTIONS=true -# Networks your own proxies sit on, comma-separated (default: empty, headers ignored) -# With this set, an audit entry records the nearest X-Forwarded-For hop that is not -# one of these networks (the last address your infrastructure wrote, which a client -# cannot overwrite). Requests that do not arrive from a listed network keep their -# socket address. Loopback and private ranges are NOT trusted implicitly: list every -# proxy hop between clients and the gateway. Bare addresses are single-host networks. -# LOGGING_TRUSTED_PROXY_CIDRS=10.0.0.0/8,127.0.0.1 - # In-memory audit log queue capacity in entries/rows, not bytes (default: 1000) # If the queue is full, new audit log entries are dropped with a warning # LOGGING_BUFFER_SIZE=1000 diff --git a/config/clientip.go b/config/clientip.go new file mode 100644 index 000000000..e66e9f9e2 --- /dev/null +++ b/config/clientip.go @@ -0,0 +1,309 @@ +package config + +import ( + "fmt" + "net" + "net/http" + "net/netip" + "slices" + "strings" +) + +// ClientIPHeaderForwardedFor is the default header GoModel reads the client +// address from. It carries a chain of hops rather than a single address, so it +// is the only header resolved by walking the chain. +const ClientIPHeaderForwardedFor = "X-Forwarded-For" + +// clientIPPresets expand a shorthand name into the networks it stands for. +// They are opt-in: nothing here is trusted unless it is named in the +// configuration, so a gateway that faces clients directly keeps ignoring every +// forwarding header. +var clientIPPresets = map[string][]string{ + // loopback covers a proxy sharing the host with the gateway. + "loopback": {"127.0.0.0/8", "::1/128"}, + // private covers RFC 1918 and RFC 4193 (ULA) space, the usual home for a + // container network or a cluster ingress whose address is not fixed. + // Prefer listing the one subnet your proxy sits on when you know it: this + // preset trusts every private client to set its own forwarded address. + "private": {"10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "fc00::/7"}, +} + +// ClientIPPolicy is the resolved form of the server's client-address settings: +// which peers may speak about the client, which header they speak through, and +// how to read it. The zero value trusts nothing, which makes every request +// report the address of the socket that reached the gateway. +type ClientIPPolicy struct { + // Prefixes are the networks whose requests may carry a client address. + Prefixes []netip.Prefix + // Header carries the client address. Empty when no proxy is trusted. + Header string + // Hops is the number of proxies between the client and the gateway. Zero + // selects chain-scanning by Prefixes instead of a fixed depth. + Hops int + // chain reports whether Header carries a comma-separated list of hops + // rather than a single address. + chain bool +} + +// Enabled reports whether any peer is trusted to speak about the client. +func (p ClientIPPolicy) Enabled() bool { return len(p.Prefixes) > 0 } + +// Trusts reports whether an address belongs to a configured proxy network. +func (p ClientIPPolicy) Trusts(addr netip.Addr) bool { + if !addr.IsValid() { + return false + } + addr = canonicalAddr(addr) + for _, prefix := range p.Prefixes { + if prefix.Contains(addr) { + return true + } + } + return false +} + +// Resolve returns the address to attribute a request to. Forwarding headers +// are read only when the request arrived from a trusted network; anything the +// policy cannot vouch for falls back to the address of the socket peer, which +// no client can forge. +func (p ClientIPPolicy) Resolve(req *http.Request) string { + direct := socketAddr(req) + if !p.Enabled() || !p.Trusts(direct) { + return addrString(direct, req) + } + if !p.chain { + return p.singleHeaderIP(req, direct) + } + return p.chainIP(req, direct) +} + +// singleHeaderIP reads a header that carries exactly one address, such as +// CF-Connecting-IP or X-Real-IP. Its value is taken verbatim: the edge that +// wrote it already decided who the client is, and a chain scan would be the +// wrong question to ask of it. +func (p ClientIPPolicy) singleHeaderIP(req *http.Request, direct netip.Addr) string { + addr, err := netip.ParseAddr(trimIPBrackets(strings.TrimSpace(req.Header.Get(p.Header)))) + if err != nil { + return addrString(direct, req) + } + return canonicalAddr(addr).String() +} + +// chainIP reads X-Forwarded-For, where each hop appends the address it saw. +// +// With Hops set, the client sits exactly that many proxies out, so the entry +// at that depth is the answer regardless of the addresses in between — which +// is what you want when your edge rotates through addresses you cannot +// enumerate. Otherwise the nearest hop that is not itself a trusted proxy is +// reported: that is the last address the operator's own infrastructure wrote +// and the furthest one a client cannot overwrite. +func (p ClientIPPolicy) chainIP(req *http.Request, direct netip.Addr) string { + values := req.Header.Values(p.Header) + if len(values) == 0 { + return addrString(direct, req) + } + var hops []netip.Addr + for _, value := range values { + for candidate := range strings.SplitSeq(value, ",") { + addr, err := netip.ParseAddr(trimIPBrackets(strings.TrimSpace(candidate))) + if err != nil { + // A chain GoModel cannot fully read is not evidence about the + // client, so keep the address of the socket peer instead. + return addrString(direct, req) + } + hops = append(hops, canonicalAddr(addr)) + } + } + if p.Hops > 0 { + index := len(hops) - p.Hops + if index < 0 { + // A chain shorter than the configured depth did not come through + // the expected path, so it says nothing about the client. + return addrString(direct, req) + } + return hops[index].String() + } + for _, hop := range slices.Backward(hops) { + if !p.Trusts(hop) { + return hop.String() + } + } + // Every hop is a trusted proxy, so no hop was written by anything the + // gateway does not already trust: the leftmost address is a claim, not + // evidence, and an internal client could invent it. Record the address of + // the socket peer instead of attributing the request to a forgeable value. + return addrString(direct, req) +} + +// ResolveClientIPPolicy validates the server's client-address settings and +// compiles them into ServerConfig.ClientIP, rewriting TrustedProxies as the +// networks it resolved so logs and config dumps show what is actually trusted. +func ResolveClientIPPolicy(cfg *ServerConfig) error { + if cfg == nil { + return nil + } + prefixes, err := parseTrustedProxies(cfg.TrustedProxies) + if err != nil { + return err + } + header, chain, err := clientIPHeader(cfg.ClientIPHeader) + if err != nil { + return err + } + if cfg.TrustedHops < 0 { + return fmt.Errorf("server.trusted_hops must be 0 or a positive number of proxies; got %d", cfg.TrustedHops) + } + if cfg.TrustedHops > 0 && !chain { + return fmt.Errorf("server.trusted_hops counts hops in a %s chain and cannot be used with server.client_ip_header %q, which carries a single address", ClientIPHeaderForwardedFor, header) + } + if len(prefixes) == 0 { + if strings.TrimSpace(cfg.ClientIPHeader) != "" { + return fmt.Errorf("server.client_ip_header is set but server.trusted_proxies is empty; list the networks your proxies sit on or the header is ignored") + } + if cfg.TrustedHops > 0 { + return fmt.Errorf("server.trusted_hops is set but server.trusted_proxies is empty; list the networks your proxies sit on or the header is ignored") + } + cfg.TrustedProxies = nil + cfg.ClientIP = ClientIPPolicy{} + return nil + } + + normalized := make([]string, 0, len(prefixes)) + for _, prefix := range prefixes { + normalized = append(normalized, prefix.String()) + } + cfg.TrustedProxies = normalized + cfg.ClientIP = ClientIPPolicy{ + Prefixes: prefixes, + Header: header, + Hops: cfg.TrustedHops, + chain: chain, + } + return nil +} + +// clientIPHeader canonicalizes the configured header and reports whether it +// carries a chain of hops. RFC 7239 Forwarded is rejected rather than read as +// a single address: its value is a list of key-value pairs, so treating it as +// an address would silently resolve nothing. +func clientIPHeader(value string) (string, bool, error) { + header, err := NormalizeHeaderName(value, ClientIPHeaderForwardedFor) + if err != nil { + return "", false, fmt.Errorf("server.client_ip_header: %w", err) + } + if strings.EqualFold(header, "Forwarded") { + return "", false, fmt.Errorf("server.client_ip_header: the RFC 7239 %q header is not supported; use %s or a single-address header such as X-Real-IP or CF-Connecting-IP", header, ClientIPHeaderForwardedFor) + } + return header, strings.EqualFold(header, ClientIPHeaderForwardedFor), nil +} + +// parseTrustedProxies expands presets, accepts bare addresses as single-host +// networks, drops duplicates and blanks, and rejects anything that is neither. +func parseTrustedProxies(values []string) ([]netip.Prefix, error) { + prefixes := make([]netip.Prefix, 0, len(values)) + seen := make(map[netip.Prefix]struct{}, len(values)) + for _, raw := range values { + value := strings.TrimSpace(raw) + if value == "" { + continue + } + expanded, isPreset := clientIPPresets[strings.ToLower(value)] + if !isPreset { + expanded = []string{value} + } + for _, entry := range expanded { + prefix, err := parseTrustedProxy(entry, raw) + if err != nil { + return nil, err + } + if _, duplicate := seen[prefix]; duplicate { + continue + } + seen[prefix] = struct{}{} + prefixes = append(prefixes, prefix) + } + } + if len(prefixes) == 0 { + return nil, nil + } + return prefixes, nil +} + +// parseTrustedProxy reads one network or bare address. An IPv4-mapped network +// is rewritten as the IPv4 network it stands for, so "::ffff:10.0.0.0/104" +// matches the same requests as "10.0.0.0/8"; one whose prefix reaches past the +// embedded address matches no IPv4 request at all and is rejected instead of +// silently trusting nothing. +func parseTrustedProxy(entry, raw string) (netip.Prefix, error) { + if prefix, err := netip.ParsePrefix(entry); err == nil { + addr := prefix.Addr() + if addr.Is4In6() { + if prefix.Bits() < 96 { + return netip.Prefix{}, fmt.Errorf("server.trusted_proxies: %q is an IPv4-mapped network with a prefix shorter than /96, which matches no IPv4 address; write the IPv4 form (for example 10.0.0.0/8)", entry) + } + prefix = netip.PrefixFrom(addr.Unmap(), prefix.Bits()-96) + } + return prefix.Masked(), nil + } + if addr, err := netip.ParseAddr(entry); err == nil { + addr = canonicalAddr(addr) + return netip.PrefixFrom(addr, addr.BitLen()), nil + } + return netip.Prefix{}, fmt.Errorf("server.trusted_proxies: %q is not a valid IP address, CIDR network, or preset (%s)", raw, strings.Join(clientIPPresetNames(), ", ")) +} + +// clientIPPresetNames lists the preset names in a stable order for messages. +func clientIPPresetNames() []string { + names := make([]string, 0, len(clientIPPresets)) + for name := range clientIPPresets { + names = append(names, name) + } + slices.Sort(names) + return names +} + +// canonicalAddr reduces an address to the form networks are compared in: an +// IPv4-mapped address becomes its IPv4 form, and the zone of a link-local +// address is dropped because it names an interface rather than the peer. +func canonicalAddr(addr netip.Addr) netip.Addr { + return addr.Unmap().WithZone("") +} + +// socketAddr is the peer address of the connection, independent of any header. +func socketAddr(req *http.Request) netip.Addr { + if req == nil { + return netip.Addr{} + } + remote := req.RemoteAddr + if host, _, err := net.SplitHostPort(remote); err == nil { + remote = host + } + addr, err := netip.ParseAddr(trimIPBrackets(remote)) + if err != nil { + return netip.Addr{} + } + return canonicalAddr(addr) +} + +// addrString renders the socket peer, falling back to the raw RemoteAddr when +// it is not an address at all (a unix socket, say) so the caller still gets +// whatever the server knows about the peer. +func addrString(addr netip.Addr, req *http.Request) string { + if addr.IsValid() { + return addr.String() + } + if req == nil { + return "" + } + if host, _, err := net.SplitHostPort(req.RemoteAddr); err == nil { + return host + } + return req.RemoteAddr +} + +// trimIPBrackets removes the brackets a proxy may put around an IPv6 address +// inside a forwarded-for list, so "2001:db8::1, [2001:db8::2]" reads as two +// addresses rather than one unparseable value. +func trimIPBrackets(value string) string { + return strings.TrimSuffix(strings.TrimPrefix(value, "["), "]") +} diff --git a/config/clientip_test.go b/config/clientip_test.go new file mode 100644 index 000000000..5dff00f32 --- /dev/null +++ b/config/clientip_test.go @@ -0,0 +1,410 @@ +package config + +import ( + "net/http" + "net/http/httptest" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveClientIPPolicy covers the operator-facing shape of the settings: +// off unless configured, normalized when set, presets expanded, and a startup +// error on a combination that cannot describe a deployment. +func TestResolveClientIPPolicy(t *testing.T) { + tests := []struct { + name string + cfg ServerConfig + wantNets []string + wantHeader string + wantHops int + wantErr string + }{ + { + name: "unset trusts nothing", + cfg: ServerConfig{}, + wantNets: nil, + }, + { + name: "networks are normalized and deduplicated", + cfg: ServerConfig{TrustedProxies: []string{" 10.0.0.0/8 ", "127.0.0.1", "10.0.0.0/8", ""}}, + wantNets: []string{"10.0.0.0/8", "127.0.0.1/32"}, + wantHeader: ClientIPHeaderForwardedFor, + }, + { + name: "a bare IPv6 address becomes a single host", + cfg: ServerConfig{TrustedProxies: []string{"2001:db8::5"}}, + wantNets: []string{"2001:db8::5/128"}, + wantHeader: ClientIPHeaderForwardedFor, + }, + { + name: "a network is masked to its base address", + cfg: ServerConfig{TrustedProxies: []string{"10.42.7.9/16"}}, + wantNets: []string{"10.42.0.0/16"}, + wantHeader: ClientIPHeaderForwardedFor, + }, + { + name: "presets expand and compose with explicit networks", + cfg: ServerConfig{TrustedProxies: []string{"Loopback", "10.42.0.0/16", "private"}}, + wantNets: []string{"127.0.0.0/8", "::1/128", "10.42.0.0/16", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "fc00::/7"}, + wantHeader: ClientIPHeaderForwardedFor, + }, + { + name: "a single-address header is canonicalized", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, ClientIPHeader: "cf-connecting-ip"}, + wantNets: []string{"10.0.0.0/8"}, + wantHeader: "Cf-Connecting-Ip", + }, + { + name: "hops compose with the network list", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, TrustedHops: 2}, + wantNets: []string{"10.0.0.0/8"}, + wantHeader: ClientIPHeaderForwardedFor, + wantHops: 2, + }, + { + name: "an unusable network is rejected", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/33"}}, + wantErr: "trusted_proxies", + }, + { + name: "a header without trusted networks is rejected", + cfg: ServerConfig{ClientIPHeader: "X-Real-IP"}, + wantErr: "server.trusted_proxies is empty", + }, + { + name: "hops without trusted networks are rejected", + cfg: ServerConfig{TrustedHops: 1}, + wantErr: "server.trusted_proxies is empty", + }, + { + name: "hops on a single-address header are rejected", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, ClientIPHeader: "X-Real-IP", TrustedHops: 1}, + wantErr: "carries a single address", + }, + { + name: "negative hops are rejected", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, TrustedHops: -1}, + wantErr: "trusted_hops must be 0", + }, + { + name: "the RFC 7239 header is rejected rather than misread", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, ClientIPHeader: "forwarded"}, + wantErr: "not supported", + }, + { + name: "an invalid header name is rejected", + cfg: ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}, ClientIPHeader: "X Real IP"}, + wantErr: "client_ip_header", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := tt.cfg + err := ResolveClientIPPolicy(&cfg) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantNets, cfg.TrustedProxies) + assert.Equal(t, tt.wantHeader, cfg.ClientIP.Header) + assert.Equal(t, tt.wantHops, cfg.ClientIP.Hops) + assert.Equal(t, len(tt.wantNets) > 0, cfg.ClientIP.Enabled()) + }) + } +} + +// TestResolveClientIPPolicyMappedIPv6 pins the handling of IPv4-mapped +// networks, which are the ones an operator can write believing a proxy network +// is trusted while it silently matches nothing. +func TestResolveClientIPPolicyMappedIPv6(t *testing.T) { + tests := []struct { + name string + in []string + want []string + wantErr string + }{ + { + name: "mapped prefix inside the embedded address becomes the IPv4 network", + in: []string{"::ffff:10.0.0.0/120"}, + want: []string{"10.0.0.0/24"}, + }, + { + name: "mapped /96 is the whole IPv4 space", + in: []string{"::ffff:0:0/96"}, + want: []string{"0.0.0.0/0"}, + }, + { + name: "a mapped bare address becomes an IPv4 host", + in: []string{"::ffff:10.0.0.1"}, + want: []string{"10.0.0.1/32"}, + }, + { + name: "mapped prefix below /96 reaches past the embedded address", + in: []string{"::ffff:10.0.0.0/8"}, + wantErr: "shorter than /96", + }, + { + name: "plain IPv4 and IPv6 networks are untouched", + in: []string{"10.0.0.0/8", "2001:db8::/32", "::1"}, + want: []string{"10.0.0.0/8", "2001:db8::/32", "::1/128"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := ServerConfig{TrustedProxies: tt.in} + err := ResolveClientIPPolicy(&cfg) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, cfg.TrustedProxies) + }) + } +} + +// TestClientIPPolicyResolve pins which address a request is attributed to, +// given the socket peer and whatever the configured header carries, so a value +// a client controls is never recorded in place of the address the operator's +// proxies observed. +func TestClientIPPolicyResolve(t *testing.T) { + tests := []struct { + name string + proxies []string + header string + hops int + remote string + values []string + wantIP string + }{ + { + name: "no trusted proxies ignores the header", + remote: "203.0.113.7:4321", + values: []string{"198.51.100.8"}, + wantIP: "203.0.113.7", + }, + { + name: "untrusted socket peer ignores the header", + proxies: []string{"10.0.0.0/8"}, + remote: "203.0.113.7:4321", + values: []string{"198.51.100.8"}, + wantIP: "203.0.113.7", + }, + { + name: "trusted peer without header keeps socket IP", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + wantIP: "127.0.0.1", + }, + { + name: "nearest hop written by the operator wins", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"203.0.113.9, 198.51.100.7"}, + wantIP: "198.51.100.7", + }, + { + name: "split header values are read in order", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"203.0.113.9", "198.51.100.7"}, + wantIP: "198.51.100.7", + }, + { + name: "a client cannot overwrite the chain it sends", + proxies: []string{"10.2.0.0/16"}, + remote: "10.2.0.5:4321", + values: []string{"198.51.100.99, 203.0.113.9, 10.2.0.5"}, + wantIP: "203.0.113.9", + }, + { + name: "an all-trusted chain cannot forge the recorded client", + proxies: []string{"10.0.0.0/8"}, + remote: "10.0.0.9:4321", + values: []string{"10.66.66.66, 10.2.2.2"}, + wantIP: "10.0.0.9", + }, + { + name: "an internal client behind a narrowly listed proxy keeps its real address", + proxies: []string{"10.0.0.0/24"}, + remote: "10.0.0.9:4321", + values: []string{"10.20.30.40, 10.0.0.9"}, + wantIP: "10.20.30.40", + }, + { + name: "a home lab trusts its cluster without trusting its clients", + proxies: []string{"10.42.0.0/16"}, + remote: "10.42.0.7:4321", + values: []string{"192.168.1.50, 10.42.0.7"}, + wantIP: "192.168.1.50", + }, + { + name: "unparseable hop falls back to the socket IP", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"203.0.113.9, spoofed"}, + wantIP: "127.0.0.1", + }, + { + name: "bracketed IPv6 is normalised", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"2001:db8::1, [2001:db8::2]"}, + wantIP: "2001:db8::2", + }, + { + name: "an IPv4-mapped hop is reported in its IPv4 form", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"::ffff:198.51.100.7"}, + wantIP: "198.51.100.7", + }, + { + name: "loopback proxy hop is only trusted when listed", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + values: []string{"203.0.113.9, 127.0.0.1"}, + wantIP: "203.0.113.9", + }, + { + name: "a zone-bearing link-local proxy is still recognized", + proxies: []string{"fe80::/10"}, + remote: "[fe80::1%eth0]:4321", + values: []string{"203.0.113.9, fe80::1"}, + wantIP: "203.0.113.9", + }, + { + name: "socket address without port is accepted", + proxies: []string{"10.0.0.0/8"}, + remote: "10.0.0.3", + values: []string{"198.51.100.4"}, + wantIP: "198.51.100.4", + }, + { + name: "the private preset covers an unpredictable ingress address", + proxies: []string{"private"}, + remote: "172.20.4.9:4321", + values: []string{"203.0.113.9"}, + wantIP: "203.0.113.9", + }, + { + name: "a single-address header is taken verbatim", + proxies: []string{"10.0.0.0/8"}, + header: "CF-Connecting-IP", + remote: "10.0.0.3:4321", + values: []string{"203.0.113.9"}, + wantIP: "203.0.113.9", + }, + { + name: "a single-address header inside the trusted network is still the client", + proxies: []string{"10.0.0.0/8"}, + header: "X-Real-IP", + remote: "10.0.0.3:4321", + values: []string{"10.9.9.9"}, + wantIP: "10.9.9.9", + }, + { + name: "a missing single-address header falls back to the socket IP", + proxies: []string{"10.0.0.0/8"}, + header: "X-Real-IP", + remote: "10.0.0.3:4321", + wantIP: "10.0.0.3", + }, + { + name: "one hop selects the rightmost entry", + proxies: []string{"10.0.0.0/8"}, + hops: 1, + remote: "10.0.0.3:4321", + values: []string{"198.51.100.99, 203.0.113.9"}, + wantIP: "203.0.113.9", + }, + { + name: "two hops reach past a rotating edge inside the trusted range", + proxies: []string{"10.0.0.0/8"}, + hops: 2, + remote: "10.0.0.3:4321", + values: []string{"203.0.113.9, 10.7.7.7"}, + wantIP: "203.0.113.9", + }, + { + name: "a chain shorter than the configured depth falls back to the socket IP", + proxies: []string{"10.0.0.0/8"}, + hops: 3, + remote: "10.0.0.3:4321", + values: []string{"203.0.113.9, 10.7.7.7"}, + wantIP: "10.0.0.3", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := ServerConfig{TrustedProxies: tt.proxies, ClientIPHeader: tt.header, TrustedHops: tt.hops} + require.NoError(t, ResolveClientIPPolicy(&cfg)) + + header := tt.header + if header == "" { + header = ClientIPHeaderForwardedFor + } + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = tt.remote + for _, value := range tt.values { + req.Header.Add(header, value) + } + + assert.Equal(t, tt.wantIP, cfg.ClientIP.Resolve(req)) + }) + } +} + +// TestClientIPPolicyGuards checks the policy reports an address rather than +// panicking when asked about a request it cannot read, which is what an +// unconfigured deployment or a peer that is not an IP amounts to. +func TestClientIPPolicyGuards(t *testing.T) { + var unset ClientIPPolicy + assert.False(t, unset.Enabled()) + assert.False(t, unset.Trusts(netip.MustParseAddr("10.0.0.1"))) + assert.False(t, unset.Trusts(netip.Addr{})) + assert.Empty(t, unset.Resolve(nil)) + + cfg := ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}} + require.NoError(t, ResolveClientIPPolicy(&cfg)) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = "@" + req.Header.Set(ClientIPHeaderForwardedFor, "203.0.113.9") + assert.Equal(t, "@", cfg.ClientIP.Resolve(req), "a peer that is not an address cannot be trusted with a header") +} + +// TestLoadClientIPSettings checks the settings reach the resolved policy +// through the environment, since that is how a container deployment sets them. +func TestLoadClientIPSettings(t *testing.T) { + clearAllConfigEnvVars(t) + + withTempDir(t, func(string) { + result, err := Load() + require.NoError(t, err) + require.Empty(t, result.Config.Server.TrustedProxies, "forwarding headers must be ignored unless the operator lists proxy networks") + require.False(t, result.Config.Server.ClientIP.Enabled()) + + t.Setenv("SERVER_TRUSTED_PROXIES", " 10.42.0.0/16 , loopback ") + t.Setenv("SERVER_CLIENT_IP_HEADER", "CF-Connecting-IP") + result, err = Load() + require.NoError(t, err) + require.Equal(t, []string{"10.42.0.0/16", "127.0.0.0/8", "::1/128"}, result.Config.Server.TrustedProxies) + require.Equal(t, "Cf-Connecting-Ip", result.Config.Server.ClientIP.Header) + + t.Setenv("SERVER_TRUSTED_PROXIES", "not-a-network") + _, err = Load() + require.Error(t, err) + require.Contains(t, err.Error(), "trusted_proxies") + }) +} diff --git a/config/config.example.yaml b/config/config.example.yaml index 1e78c4983..14af426da 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -17,6 +17,15 @@ server: enabled_passthrough_providers: ["openai", "anthropic", "cohere", "openrouter", "kilo", "zai", "sglang", "vllm", "llmd", "deepseek", "bailian"] # providers enabled on /p/{provider}/... auth_verify_enabled: false # env: AUTH_VERIFY_ENABLED; expose GET /v1/auth/verify, which reports whether the API key a request carries authenticates against this gateway (401 when it does not; 200 with {"valid":false,"method":"none"} when the gateway has no authentication configured). Outside /admin, so it works with the admin API disabled realtime_enabled: true # env: REALTIME_ENABLED; expose the /v1/realtime and /v1/realtime/translations websockets, their calls/client_secrets siblings, and /p/{provider}/v1/realtime upgrades + # Client address resolution. Leave trusted_proxies empty (default) and every + # client address GoModel reports is the socket peer, with forwarding headers + # ignored. Entries are CIDR networks, bare addresses, or the presets + # "loopback" and "private" (RFC 1918 + ULA); prefer the one subnet your proxy + # sits on over "private", which trusts every private client to set its own + # forwarded address. + # trusted_proxies: ["10.42.0.0/16", "loopback"] + # client_ip_header: "CF-Connecting-IP" # env: SERVER_CLIENT_IP_HEADER; default X-Forwarded-For, resolved hop by hop. Any other header carries one address and is taken verbatim, which is correct behind an edge like Cloudflare. Requires trusted_proxies; RFC 7239 Forwarded is not supported + # trusted_hops: 1 # env: SERVER_TRUSTED_HOPS; proxies between the client and the gateway. Takes the X-Forwarded-For entry at that depth instead of scanning the chain, for an edge at a fixed depth with addresses you cannot enumerate. Requires trusted_proxies pid_file: "data/gomodel.pid" # env: PID_FILE; where the running gateway records its process id so `gomodel --reload` can find it. Set per instance when several gateways share a host; empty writes no pid file and disables --reload; changing it needs a restart, not a reload models: @@ -217,13 +226,6 @@ logging: flush_interval: 5 # seconds retention_days: 30 # 0 = keep forever only_model_interactions: true - # Networks your own proxies sit on. With this set, an audit entry records the - # nearest X-Forwarded-For hop that is not one of these networks (the last - # address your infrastructure wrote, which a client cannot overwrite). - # Requests that do not arrive from a listed network keep their socket address. - # Loopback and private ranges are not trusted implicitly. Leave empty to - # ignore forwarding headers entirely (default). - # trusted_proxy_cidrs: ["10.0.0.0/8", "127.0.0.1"] usage: # Usage actions require USAGE_ENABLED=true (or usage.enabled: true) and a supported diff --git a/config/config.go b/config/config.go index ea670c7b8..44085432a 100644 --- a/config/config.go +++ b/config/config.go @@ -339,7 +339,7 @@ func Load() (*LoadResult, error) { if !cfg.Logging.LogImageBodiesScope.Valid() { return nil, fmt.Errorf("logging.log_image_bodies_scope must be one of: all, input, output; got %q", cfg.Logging.LogImageBodiesScope) } - if err := NormalizeTrustedProxyCIDRs(&cfg.Logging); err != nil { + if err := ResolveClientIPPolicy(&cfg.Server); err != nil { return nil, err } diff --git a/config/config_test.go b/config/config_test.go index 6b52aa90c..07e1d1d44 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -64,6 +64,7 @@ func clearAllConfigEnvVars(t *testing.T) { for _, key := range []string{ "CONFIG_STRICT", "PORT", "BASE_PATH", "GOMODEL_MASTER_KEY", "BODY_SIZE_LIMIT", "STREAM_STALL_TIMEOUT", "SWAGGER_ENABLED", "PPROF_ENABLED", "ENABLE_PASSTHROUGH_ROUTES", "ALLOW_PASSTHROUGH_V1_ALIAS", "USER_PATH_HEADER", "ENABLED_PASSTHROUGH_PROVIDERS", + "SERVER_TRUSTED_PROXIES", "SERVER_CLIENT_IP_HEADER", "SERVER_TRUSTED_HOPS", "GOMODEL_CACHE_DIR", "CACHE_REFRESH_INTERVAL", "MODEL_LIST_URL", "GOMODEL_OFFLINE", "GOMODEL_VERSION_CHECK_ENABLED", "REDIS_URL", "REDIS_KEY_MODELS", "REDIS_KEY_RESPONSES", "REDIS_TTL_MODELS", "REDIS_TTL_RESPONSES", "RESPONSE_CACHE_SIMPLE_ENABLED", @@ -79,7 +80,7 @@ func clearAllConfigEnvVars(t *testing.T) { "METRICS_ENABLED", "METRICS_ENDPOINT", "LOGGING_ENABLED", "LOGGING_LOG_BODIES", "LOGGING_LOG_REVISION_BODIES", "LOGGING_LOG_GUARDRAIL_STEPS", "LOGGING_LOG_HEADERS", "LOGGING_LOG_AUDIO_BODIES", "LOGGING_LOG_IMAGE_BODIES", "LOGGING_LOG_IMAGE_BODIES_SCOPE", - "LOGGING_ONLY_MODEL_INTERACTIONS", "LOGGING_TRUSTED_PROXY_CIDRS", "LOGGING_BUFFER_SIZE", + "LOGGING_ONLY_MODEL_INTERACTIONS", "LOGGING_BUFFER_SIZE", "LOGGING_FLUSH_INTERVAL", "LOGGING_RETENTION_DAYS", "USAGE_ENABLED", "ENFORCE_RETURNING_USAGE_DATA", "USAGE_PRICING_RECALCULATION_ENABLED", diff --git a/config/logging.go b/config/logging.go index a96e92419..0586523a3 100644 --- a/config/logging.go +++ b/config/logging.go @@ -1,10 +1,6 @@ package config -import ( - "fmt" - "net" - "strings" -) +import "strings" // LogConfig holds audit logging configuration type LogConfig struct { @@ -88,88 +84,6 @@ type LogConfig struct { // Endpoints like /health, /metrics, /admin, /v1/models are skipped // Default: true OnlyModelInteractions bool `yaml:"only_model_interactions" env:"LOGGING_ONLY_MODEL_INTERACTIONS"` - - // TrustedProxyCIDRs lists the networks your own proxies sit on, enabling - // X-Forwarded-For based client IPs in audit entries. A bare address is - // treated as a single host. - // - // When empty (default), audit entries record the address of the socket - // peer that connected to the gateway, and forwarding headers are ignored. - // When set, an entry records the nearest hop in the X-Forwarded-For chain - // that is not one of these networks (the last address your own proxy - // wrote, which a client cannot overwrite). Requests that do not arrive from - // a listed network keep their socket peer address. - // - // Loopback and private ranges are not trusted implicitly: list every proxy - // hop between clients and the gateway, or the header chain is ignored. - // Example: ["10.0.0.0/8", "127.0.0.1"]. - // Default: empty (forwarding headers ignored) - TrustedProxyCIDRs []string `yaml:"trusted_proxy_cidrs" env:"LOGGING_TRUSTED_PROXY_CIDRS"` -} - -// NormalizeTrustedProxyCIDRs trims the configured networks, accepts bare -// addresses as single-host CIDRs, drops duplicates and blanks, and rejects -// anything that is neither a valid address nor a valid network. -func NormalizeTrustedProxyCIDRs(cfg *LogConfig) error { - if cfg == nil { - return nil - } - if len(cfg.TrustedProxyCIDRs) == 0 { - cfg.TrustedProxyCIDRs = nil - return nil - } - - normalized := make([]string, 0, len(cfg.TrustedProxyCIDRs)) - seen := make(map[string]struct{}, len(cfg.TrustedProxyCIDRs)) - for _, raw := range cfg.TrustedProxyCIDRs { - value := strings.TrimSpace(raw) - if value == "" { - continue - } - ip, ipnet, err := net.ParseCIDR(value) - if err == nil { - value, err = canonicalProxyCIDR(ip, ipnet) - if err != nil { - return err - } - } else if addr := net.ParseIP(value); addr != nil { - value = singleHostCIDR(addr) - } else { - return fmt.Errorf("logging.trusted_proxy_cidrs: %q is not a valid IP address or CIDR network", raw) - } - if _, duplicate := seen[value]; duplicate { - continue - } - seen[value] = struct{}{} - normalized = append(normalized, value) - } - cfg.TrustedProxyCIDRs = normalized - return nil -} - -// canonicalProxyCIDR renders a parsed network in the form that actually matches -// requests, rejecting the ones that cannot. An IPv4-mapped network whose prefix -// is shorter than /96 ("::ffff:10.0.0.0/8") reaches past the embedded address: -// Go masks it to a network such as "::/8" that no IPv4 request belongs to, so -// the operator would believe a proxy network was trusted while nothing matched -// it. Narrower mapped networks ("::ffff:10.0.0.0/120") are rendered by net.IPNet -// as the IPv4 network they stand for ("10.0.0.0/24") and are kept. -func canonicalProxyCIDR(addr net.IP, ipnet *net.IPNet) (string, error) { - ones, bits := ipnet.Mask.Size() - if bits == 128 && addr.To4() != nil && ones < 96 { - return "", fmt.Errorf("logging.trusted_proxy_cidrs: %q is an IPv4-mapped network with a prefix shorter than /96, which matches no IPv4 address; write the IPv4 form (for example 10.0.0.0/8)", ipnet.String()) - } - return ipnet.String(), nil -} - -// singleHostCIDR renders an address as the network containing only it, so an -// IPv4-mapped address such as "::ffff:10.0.0.1" normalizes to "10.0.0.1/32" -// instead of a /128 no IPv4 request will ever match. -func singleHostCIDR(ip net.IP) string { - if v4 := ip.To4(); v4 != nil { - return v4.String() + "/32" - } - return ip.String() + "/128" } // ImageBodyScope selects which image bytes the audit log embeds when diff --git a/config/logging_test.go b/config/logging_test.go index c51b176f9..ef3f16619 100644 --- a/config/logging_test.go +++ b/config/logging_test.go @@ -57,90 +57,3 @@ func TestLoadImageBodyLoggingEnv(t *testing.T) { require.Contains(t, err.Error(), "log_image_bodies_scope") }) } - -// TestLoadTrustedProxyCIDRs covers the operator-facing shape of the setting: -// off unless configured, normalized when set, and a startup error on a value -// that cannot describe a network. -func TestLoadTrustedProxyCIDRs(t *testing.T) { - clearAllConfigEnvVars(t) - - withTempDir(t, func(string) { - result, err := Load() - require.NoError(t, err) - require.Empty(t, result.Config.Logging.TrustedProxyCIDRs, "forwarding headers must be ignored unless the operator lists proxy networks") - - t.Setenv("LOGGING_TRUSTED_PROXY_CIDRS", " 10.0.0.0/8 , 127.0.0.1, 10.0.0.0/8 ,") - result, err = Load() - require.NoError(t, err) - require.Equal(t, []string{"10.0.0.0/8", "127.0.0.1/32"}, result.Config.Logging.TrustedProxyCIDRs) - - t.Setenv("LOGGING_TRUSTED_PROXY_CIDRS", "2001:db8::5") - result, err = Load() - require.NoError(t, err) - require.Equal(t, []string{"2001:db8::5/128"}, result.Config.Logging.TrustedProxyCIDRs) - - t.Setenv("LOGGING_TRUSTED_PROXY_CIDRS", "10.0.0.0/33") - _, err = Load() - require.Error(t, err) - require.Contains(t, err.Error(), "trusted_proxy_cidrs") - }) -} - -// TestNormalizeTrustedProxyCIDRsMappedIPv6 pins the handling of IPv4-mapped -// networks, which are the ones an operator can write believing a proxy network -// is trusted while it silently matches nothing. -func TestNormalizeTrustedProxyCIDRsMappedIPv6(t *testing.T) { - tests := []struct { - name string - in []string - want []string - wantErr string - }{ - { - name: "mapped prefix inside the embedded address becomes the IPv4 network", - in: []string{"::ffff:10.0.0.0/120"}, - want: []string{"10.0.0.0/24"}, - }, - { - name: "mapped /96 is the whole IPv4 space", - in: []string{"::ffff:0:0/96"}, - want: []string{"0.0.0.0/0"}, - }, - { - name: "mapped prefix below /96 reaches past the embedded address", - in: []string{"::ffff:10.0.0.0/8"}, - wantErr: "shorter than /96", - }, - { - name: "plain IPv4 and IPv6 networks are untouched", - in: []string{"10.0.0.0/8", "2001:db8::/32", "::1"}, - want: []string{"10.0.0.0/8", "2001:db8::/32", "::1/128"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cfg := &LogConfig{TrustedProxyCIDRs: tt.in} - err := NormalizeTrustedProxyCIDRs(cfg) - - if tt.wantErr != "" { - require.Error(t, err) - require.Contains(t, err.Error(), tt.wantErr) - return - } - require.NoError(t, err) - require.Equal(t, tt.want, cfg.TrustedProxyCIDRs) - }) - } -} - -// TestNormalizeTrustedProxyCIDRsDegenerateInput covers the shapes a config file -// can hold that environment parsing would have filtered out already: no -// configuration at all, and entries that carry no value. -func TestNormalizeTrustedProxyCIDRsDegenerateInput(t *testing.T) { - require.NoError(t, NormalizeTrustedProxyCIDRs(nil)) - - cfg := &LogConfig{TrustedProxyCIDRs: []string{" ", ""}} - require.NoError(t, NormalizeTrustedProxyCIDRs(cfg)) - require.Empty(t, cfg.TrustedProxyCIDRs) -} diff --git a/config/server.go b/config/server.go index 15fe26067..46afa764f 100644 --- a/config/server.go +++ b/config/server.go @@ -69,6 +69,41 @@ type ServerConfig struct { // provider connection until the provider side times out. // Default: 60 (DefaultStreamStallTimeoutSeconds). 0 disables the limit. StreamStallTimeout int `yaml:"stream_stall_timeout" env:"STREAM_STALL_TIMEOUT"` + + // TrustedProxies lists the networks your own proxies sit on, enabling + // header-based client addresses everywhere the gateway reports one (audit + // entries, rate limit keys, logs). Entries are CIDR networks, bare + // addresses (a single host), or the presets "loopback" and "private". + // + // When empty (default), the gateway records the address of the socket peer + // that connected to it and ignores every forwarding header. Nothing is + // trusted implicitly, not even loopback: list the hops between your + // clients and the gateway, or name the preset that covers them. + // Example: ["10.42.0.0/16", "loopback"]. + // Default: empty (forwarding headers ignored) + TrustedProxies []string `yaml:"trusted_proxies" env:"SERVER_TRUSTED_PROXIES"` + + // ClientIPHeader is the header a trusted proxy passes the client address + // in. X-Forwarded-For (the default) carries a chain and is resolved hop by + // hop; any other header (X-Real-IP, CF-Connecting-IP, True-Client-IP) + // carries one address and is taken verbatim, which is the correct reading + // behind an edge like Cloudflare that writes the client itself. Requires + // TrustedProxies. RFC 7239 Forwarded is not supported. + // Default: X-Forwarded-For + ClientIPHeader string `yaml:"client_ip_header" env:"SERVER_CLIENT_IP_HEADER"` + + // TrustedHops is how many proxies sit between the client and the gateway, + // selecting the X-Forwarded-For entry at that depth instead of scanning the + // chain against TrustedProxies. Use it when your edge is at a fixed depth + // but rotates through addresses you cannot enumerate; the socket peer must + // still be in TrustedProxies. A chain shorter than this depth falls back to + // the socket peer. Requires TrustedProxies and the default header. + // Default: 0 (scan the chain against TrustedProxies) + TrustedHops int `yaml:"trusted_hops" env:"SERVER_TRUSTED_HOPS"` + + // ClientIP is the compiled form of the three settings above, resolved once + // at load. It is not configured directly. + ClientIP ClientIPPolicy `yaml:"-"` } // DefaultStreamStallTimeoutSeconds is the default ServerConfig.StreamStallTimeout. diff --git a/docs/advanced/configuration.mdx b/docs/advanced/configuration.mdx index b8a29c095..50bf455d1 100644 --- a/docs/advanced/configuration.mdx +++ b/docs/advanced/configuration.mdx @@ -48,6 +48,9 @@ The most common way to configure GoModel. Set any of the variables below to over | `BODY_SIZE_LIMIT` | Max request body size (e.g., `10M`, `1024K`, `500KB`) | _(no limit)_ | | `STREAM_STALL_TIMEOUT` | Seconds one response write on a model route may wait for the client to read it; `0` disables | `60` | | `USER_PATH_HEADER` | Header used to read/write request `user_path` values | `X-GoModel-User-Path` | +| `SERVER_TRUSTED_PROXIES` | Networks your own proxies sit on; enables header-based client addresses | _(empty)_ | +| `SERVER_CLIENT_IP_HEADER` | Header a trusted proxy passes the client address in | `X-Forwarded-For` | +| `SERVER_TRUSTED_HOPS` | Proxies between the client and the gateway; `0` scans the chain instead | `0` | | `SWAGGER_ENABLED` | Expose [Swagger UI](/advanced/swagger-ui); needs a `-tags=swagger` build | `false` | | `AUTH_VERIFY_ENABLED` | Expose [`GET /v1/auth/verify`](/advanced/api-endpoints#key-verification), which reports whether the API key a request carries is valid | `false` | | `PID_FILE` | Where the running gateway records its process id for `gomodel --reload`. Changing it needs a restart | `data/gomodel.pid` next to a `./data` directory, otherwise the per-user data directory | @@ -61,6 +64,57 @@ cancelled, and the audit entry records `client_stalled`, also when the stream was served from the response cache. Raise it for clients that legitimately pause reading a stream for over a minute; set `0` to disable. +#### Client addresses behind a proxy + +Every client address GoModel reports — audit entries, rate limit keys, logs — +is the address of the connection that reached the gateway, so behind a reverse +proxy they all show the proxy. Set `SERVER_TRUSTED_PROXIES` to the networks +your own proxies sit on and GoModel reads the forwarded client instead. +Nothing is trusted until you list it, not even loopback, and changing these +settings needs a restart. + +```bash +SERVER_TRUSTED_PROXIES=10.42.0.0/16,loopback +``` + +Entries are CIDR networks, bare addresses (a single host), or a preset: + +| Preset | Expands to | +| ---------- | ---------------------------------------------------- | +| `loopback` | `127.0.0.0/8`, `::1/128` | +| `private` | `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`, `fc00::/7` | + +Prefer the one subnet your proxy sits on. `private` trusts every private +client to set its own forwarded address, which is the wrong answer when your +clients are on private addresses too — a home lab whose clients live on +`192.168.1.0/24` and whose cluster runs on `10.42.0.0/16` should list +`10.42.0.0/16` alone. + +By default GoModel reads `X-Forwarded-For`, where each hop appends the address +it saw, and records the nearest hop that is not one of your trusted networks: +the last address your own infrastructure wrote, and the furthest one a client +cannot overwrite. A request from outside the listed networks, a chain it cannot +parse, and a chain made entirely of trusted networks all keep the connection +address — in that last case every entry is a claim an internal client could +have invented. + +Two settings change how the header is read: + +- `SERVER_CLIENT_IP_HEADER` names a different header. Anything other than + `X-Forwarded-For` carries a single address and is taken verbatim, which is + the correct reading behind an edge that resolves the client itself: + `SERVER_CLIENT_IP_HEADER=CF-Connecting-IP` behind Cloudflare, or `X-Real-IP` + behind an nginx that sets it. RFC 7239 `Forwarded` is not supported. +- `SERVER_TRUSTED_HOPS` takes the `X-Forwarded-For` entry at a fixed depth + instead of scanning the chain — `1` is the rightmost entry, `2` the one + before it. Use it when your edge sits at a known depth but rotates through + addresses you cannot enumerate. The connection must still come from a trusted + network, and a chain shorter than the configured depth falls back to the + connection address. + +Both require `SERVER_TRUSTED_PROXIES`; on their own they are a startup error +rather than a setting that silently does nothing. + Set `GOMODEL_DEMO_MODE=true` for a public or shared demonstration instance. GoModel logs a warning at startup and every five minutes, renders a persistent warning at the top of the dashboard, and exposes `DEMO_MODE=on` through the @@ -163,7 +217,11 @@ Storage is shared by audit logging, usage tracking, and future features like IAM | `LOGGING_BUFFER_SIZE` | In-memory buffer before flush | `1000` | | `LOGGING_FLUSH_INTERVAL` | Flush interval in seconds | `5` | | `LOGGING_RETENTION_DAYS` | Auto-delete after N days (0 = forever) | `30` | -| `LOGGING_TRUSTED_PROXY_CIDRS` | Networks your proxies sit on; enables X-Forwarded-For client IPs in audit entries | _(empty)_ | + +Audit entries record the client address the gateway resolved. Behind a reverse +proxy that is the proxy until you list its network in +`SERVER_TRUSTED_PROXIES` — see [client addresses behind a +proxy](#client-addresses-behind-a-proxy). Realtime dashboard previews and persisted audit logs are separate features. With `DASHBOARD_LIVE_LOGS_ENABLED=true` and `LOGGING_ENABLED=false`, requests @@ -180,23 +238,6 @@ to `LOGGING_FLUSH_INTERVAL` seconds to appear through the stored-log API. restart GoModel. - - Audit entries record the address of the connection that reached the gateway, - so behind a proxy they all show the proxy. Set - `LOGGING_TRUSTED_PROXY_CIDRS` (or `logging.trusted_proxy_cidrs`) to the - networks your own proxies sit on to record the forwarded client instead: each - entry then carries the nearest `X-Forwarded-For` hop that is not one of those - networks, which is the last address your infrastructure wrote and the one a - client cannot overwrite. Requests arriving from outside the listed networks, - requests whose header cannot be parsed, and requests whose whole chain is made - of listed networks keep their connection address — in that last case no hop - was written by anything the gateway does not already trust, so the chain is a - claim an internal client could invent rather than evidence. Loopback and - private ranges are not trusted implicitly, so list every hop between clients - and the gateway, for example `10.0.0.0/8,127.0.0.1`. Changing it requires a - restart. - - When `LOGGING_LOG_BODIES` is enabled, request and response bodies are stored in full. These may contain sensitive data such as PII or API keys embedded in diff --git a/internal/app/app.go b/internal/app/app.go index 01c342931..7f389497f 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -381,6 +381,16 @@ func (a *App) logStartupInfo() { "recommendation", "list the specific origins you serve an MCP web client from instead of \"*\"") } + // Client address resolution. Reported whenever a proxy is trusted, because + // it decides which address every audit entry, rate limit key, and log line + // attributes a request to. + if policy := cfg.Server.ClientIP; policy.Enabled() { + slog.Info("trusted proxies configured", + "networks", cfg.Server.TrustedProxies, + "client_ip_header", policy.Header, + "trusted_hops", policy.Hops) + } + // Metrics configuration if cfg.Metrics.Enabled { slog.Info("prometheus metrics enabled", "endpoint", cfg.Metrics.Endpoint) @@ -404,7 +414,6 @@ func (a *App) logStartupInfo() { "log_image_bodies_scope", cfg.Logging.LogImageBodiesScope, "log_headers", cfg.Logging.LogHeaders, "retention_days", cfg.Logging.RetentionDays, - "trusted_proxy_cidrs", cfg.Logging.TrustedProxyCIDRs, ) } else { slog.Info("audit logging disabled") diff --git a/internal/app/init_server.go b/internal/app/init_server.go index 25785ca6e..cc53e192b 100644 --- a/internal/app/init_server.go +++ b/internal/app/init_server.go @@ -156,6 +156,7 @@ func (b *bootstrap) initServerConfig() error { Tagging: app.tagging.Service, SessionDetector: session.NewDetectorFromConfig(appCfg.Session), MCPEnabled: appCfg.MCP.Enabled, + IPExtractor: server.ClientIPExtractor(appCfg.Server.ClientIP), VersionChecker: app.versionCheck, } if app.mcpGateway != nil { diff --git a/internal/auditlog/auditlog.go b/internal/auditlog/auditlog.go index fab552133..dc8fadbe5 100644 --- a/internal/auditlog/auditlog.go +++ b/internal/auditlog/auditlog.go @@ -543,9 +543,4 @@ type Config struct { // OnlyModelInteractions limits logging to AI model endpoints only // When true, only /v1/chat/completions, /v1/responses, /v1/embeddings, /v1/files, and /v1/batches are logged OnlyModelInteractions bool - - // TrustedProxies resolves audit client IPs from X-Forwarded-For when a - // request arrives from one of the operator's proxy networks. Nil (the - // default) records the socket peer address and ignores forwarding headers. - TrustedProxies *TrustedProxies } diff --git a/internal/auditlog/clientip.go b/internal/auditlog/clientip.go deleted file mode 100644 index 49a4c4ae9..000000000 --- a/internal/auditlog/clientip.go +++ /dev/null @@ -1,124 +0,0 @@ -package auditlog - -import ( - "net" - "net/http" - "slices" - "strings" -) - -// TrustedProxies resolves the client address of a request that arrived through -// operator-owned proxies. A nil *TrustedProxies means the gateway faces clients -// directly, so forwarding headers are never consulted. -type TrustedProxies struct { - nets []*net.IPNet -} - -// ParseTrustedProxies compiles CIDR networks for client IP resolution. Entries -// that are neither a valid address nor a valid network are skipped: the -// configuration loader rejects them with a startup error, so a value reaching -// here is either usable or already reported. -func ParseTrustedProxies(cidrs []string) *TrustedProxies { - var nets []*net.IPNet - for _, raw := range cidrs { - value := strings.TrimSpace(raw) - if value == "" { - continue - } - if _, ipNet, err := net.ParseCIDR(value); err == nil { - nets = append(nets, ipNet) - continue - } - if ip := net.ParseIP(value); ip != nil { - bits := 32 - if ip.To4() == nil { - bits = 128 - } - nets = append(nets, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) - } - } - if len(nets) == 0 { - return nil - } - return &TrustedProxies{nets: nets} -} - -// Contains reports whether an address belongs to a trusted proxy network. -func (t *TrustedProxies) Contains(ip net.IP) bool { - if t == nil || ip == nil { - return false - } - for _, ipNet := range t.nets { - if ipNet.Contains(ip) { - return true - } - } - return false -} - -// ClientIP returns the address to tag an audit entry with, given the address -// resolved by the HTTP server (directIP). Forwarded-for headers are consulted -// only when the request arrived from a trusted proxy; the nearest hop that is -// not itself a trusted proxy is reported, because that is the last address the -// operator's own infrastructure wrote and the furthest one a client cannot -// overwrite. Requests with no usable header, or with a header GoModel cannot -// trust, keep directIP. -func (t *TrustedProxies) ClientIP(req *http.Request, directIP string) string { - if t == nil || req == nil { - return directIP - } - if !t.Contains(socketIP(req)) { - return directIP - } - values := req.Header.Values("X-Forwarded-For") - if len(values) == 0 { - return directIP - } - var hops []net.IP - for _, value := range values { - for candidate := range strings.SplitSeq(value, ",") { - ip := net.ParseIP(trimIPBrackets(strings.TrimSpace(candidate))) - if ip == nil { - // A chain GoModel cannot fully read is not evidence about the - // client, so keep the address of the socket peer instead. - return directIP - } - hops = append(hops, ip) - } - } - // Reading from the hop nearest the gateway inward finds the last address - // the operator's own infrastructure wrote, which a client cannot overwrite. - for _, hop := range slices.Backward(hops) { - if !t.Contains(hop) { - return hop.String() - } - } - // Every hop is a trusted proxy, so no hop was written by anything the - // gateway does not already trust: the leftmost address is a claim, not - // evidence, and an internal client could invent it. Record the address of - // the socket peer instead of attributing the request to a forgeable value. - return directIP -} - -// socketIP is the peer address of the connection, independent of any header. -// A link-local IPv6 peer names its zone ("fe80::1%eth0"); the zone identifies -// an interface rather than part of the address, and net.ParseIP rejects it, so -// it is dropped before parsing to keep a zone-bearing proxy recognizable. -func socketIP(req *http.Request) net.IP { - remote := req.RemoteAddr - host, _, err := net.SplitHostPort(remote) - if err != nil { - host = remote - } - if zone := strings.LastIndexByte(host, '%'); zone >= 0 { - host = host[:zone] - } - return net.ParseIP(host) -} - -// trimIPBrackets removes the brackets a proxy may put around an IPv6 address -// inside a forwarded-for list, so "2001:db8::1, [2001:db8::2]" reads as two -// addresses rather than one unparseable value. -func trimIPBrackets(value string) string { - return strings.TrimSuffix(strings.TrimPrefix(value, "["), "]") -} diff --git a/internal/auditlog/clientip_test.go b/internal/auditlog/clientip_test.go deleted file mode 100644 index 0df1d3514..000000000 --- a/internal/auditlog/clientip_test.go +++ /dev/null @@ -1,227 +0,0 @@ -package auditlog - -import ( - "net" - "net/http" - "net/http/httptest" - "testing" - - "github.com/labstack/echo/v5" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestParseTrustedProxies covers how configured proxy networks are compiled, -// including the single-host form and the skipping of values the config loader -// is expected to have rejected already. -func TestParseTrustedProxies(t *testing.T) { - t.Run("empty config trusts nothing", func(t *testing.T) { - assert.Nil(t, ParseTrustedProxies(nil)) - assert.Nil(t, ParseTrustedProxies([]string{" ", ""})) - }) - - t.Run("bare addresses become single-host networks", func(t *testing.T) { - proxies := ParseTrustedProxies([]string{"10.0.0.5", " 2001:db8::5 "}) - require.NotNil(t, proxies) - assert.True(t, proxies.Contains(parseTestIP(t, "10.0.0.5"))) - assert.False(t, proxies.Contains(parseTestIP(t, "10.0.0.6"))) - assert.True(t, proxies.Contains(parseTestIP(t, "2001:db8::5"))) - }) - - t.Run("invalid entries are skipped", func(t *testing.T) { - proxies := ParseTrustedProxies([]string{"not-an-ip", "10.0.0.0/8"}) - require.NotNil(t, proxies) - assert.True(t, proxies.Contains(parseTestIP(t, "10.1.2.3"))) - }) -} - -// TestTrustedProxiesClientIP pins which address an audit entry is tagged with, -// given the socket peer and any X-Forwarded-For chain, so a value a client -// controls is never recorded in place of the address the operator's proxies -// observed. -func TestTrustedProxiesClientIP(t *testing.T) { - tests := []struct { - name string - cidrs []string - remote string - xff []string - wantIP string - }{ - { - name: "no trusted proxies ignores the header", - remote: "203.0.113.7:4321", - xff: []string{"198.51.100.8"}, - wantIP: "203.0.113.7", - }, - { - name: "untrusted socket peer ignores the header", - cidrs: []string{"10.0.0.0/8"}, - remote: "203.0.113.7:4321", - xff: []string{"198.51.100.8"}, - wantIP: "203.0.113.7", - }, - { - name: "trusted peer without header keeps socket IP", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - wantIP: "127.0.0.1", - }, - { - name: "nearest hop written by the operator wins", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: []string{"203.0.113.9, 198.51.100.7"}, - wantIP: "198.51.100.7", - }, - { - name: "split header values are read in order", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: []string{"203.0.113.9", "198.51.100.7"}, - wantIP: "198.51.100.7", - }, - { - name: "a client cannot overwrite the chain it sends", - cidrs: []string{"10.2.0.0/16"}, - remote: "10.2.0.5:4321", - xff: []string{"198.51.100.99, 203.0.113.9, 10.2.0.5"}, - wantIP: "203.0.113.9", - }, - { - name: "an all-trusted chain cannot forge the recorded client", - cidrs: []string{"10.0.0.0/8"}, - remote: "10.0.0.9:4321", - xff: []string{"10.66.66.66, 10.2.2.2"}, - wantIP: "10.0.0.9", - }, - { - name: "an internal client behind a narrowly listed proxy keeps its real address", - cidrs: []string{"10.0.0.0/24"}, - remote: "10.0.0.9:4321", - xff: []string{"10.20.30.40, 10.0.0.9"}, - wantIP: "10.20.30.40", - }, - { - name: "unparseable hop falls back to the socket IP", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: []string{"203.0.113.9, spoofed"}, - wantIP: "127.0.0.1", - }, - { - name: "bracketed IPv6 is normalised", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: []string{"2001:db8::1, [2001:db8::2]"}, - wantIP: "2001:db8::2", - }, - { - name: "loopback proxy hop is only trusted when listed", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: []string{"203.0.113.9, 127.0.0.1"}, - wantIP: "203.0.113.9", - }, - { - name: "a zone-bearing link-local proxy is still recognized", - cidrs: []string{"fe80::/10"}, - remote: "[fe80::1%eth0]:4321", - xff: []string{"203.0.113.9, fe80::1"}, - wantIP: "203.0.113.9", - }, - { - name: "socket address without port is accepted", - cidrs: []string{"10.0.0.0/8"}, - remote: "10.0.0.3", - xff: []string{"198.51.100.4"}, - wantIP: "198.51.100.4", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - proxies := ParseTrustedProxies(tt.cidrs) - req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) - req.RemoteAddr = tt.remote - for _, value := range tt.xff { - req.Header.Add("X-Forwarded-For", value) - } - - assert.Equal(t, tt.wantIP, proxies.ClientIP(req, socketIP(req).String())) - }) - } -} - -// TestMiddlewareClientIPTagging checks the resolver is actually wired into the -// audit middleware, on both a directly exposed gateway and one behind a trusted -// proxy network. -func TestMiddlewareClientIPTagging(t *testing.T) { - tests := []struct { - name string - cidrs []string - remote string - xff string - wantIP string - }{ - { - name: "direct exposure records the socket peer", - remote: "203.0.113.7:4321", - xff: "198.51.100.8", - wantIP: "203.0.113.7", - }, - { - name: "behind a trusted proxy records the forwarded client", - cidrs: []string{"127.0.0.0/8"}, - remote: "127.0.0.1:4321", - xff: "203.0.113.9, 198.51.100.7", - wantIP: "198.51.100.7", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - logger := &capturingLogger{cfg: Config{ - Enabled: true, - OnlyModelInteractions: true, - TrustedProxies: ParseTrustedProxies(tt.cidrs), - }} - - handler := Middleware(logger)(func(*echo.Context) error { - return nil - }) - - e := echo.New() - req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) - req.RemoteAddr = tt.remote - if tt.xff != "" { - req.Header.Set("X-Forwarded-For", tt.xff) - } - c := e.NewContext(req, httptest.NewRecorder()) - - require.NoError(t, handler(c)) - require.Len(t, logger.entries, 1) - assert.Equal(t, tt.wantIP, logger.entries[0].ClientIP) - }) - } -} - -// parseTestIP turns a literal address into the form a trusted proxy network is -// checked against. -// TestTrustedProxiesContainsGuards checks the resolver reports "not trusted" -// rather than panicking when asked about an address it cannot have, which is -// what an unconfigured deployment or an unparseable peer amounts to. -func TestTrustedProxiesContainsGuards(t *testing.T) { - var unset *TrustedProxies - assert.False(t, unset.Contains(parseTestIP(t, "10.0.0.1"))) - - configured := ParseTrustedProxies([]string{"10.0.0.0/8"}) - require.NotNil(t, configured) - assert.False(t, configured.Contains(nil)) -} - -func parseTestIP(t *testing.T, value string) net.IP { - t.Helper() - ip := net.ParseIP(value) - require.NotNil(t, ip) - return ip -} diff --git a/internal/auditlog/factory.go b/internal/auditlog/factory.go index 06b82472c..43a9a6fcf 100644 --- a/internal/auditlog/factory.go +++ b/internal/auditlog/factory.go @@ -83,7 +83,6 @@ func buildLoggerConfig(logCfg config.LogConfig) Config { FlushInterval: time.Duration(logCfg.FlushInterval) * time.Second, RetentionDays: logCfg.RetentionDays, OnlyModelInteractions: logCfg.OnlyModelInteractions, - TrustedProxies: ParseTrustedProxies(logCfg.TrustedProxyCIDRs), } // Apply defaults diff --git a/internal/auditlog/middleware.go b/internal/auditlog/middleware.go index 6143529a4..4761c2ba9 100644 --- a/internal/auditlog/middleware.go +++ b/internal/auditlog/middleware.go @@ -66,7 +66,7 @@ func Middleware(logger LoggerInterface) echo.MiddlewareFunc { ID: uuid.NewString(), Timestamp: start, RequestID: requestID, - ClientIP: cfg.TrustedProxies.ClientIP(req, c.RealIP()), + ClientIP: c.RealIP(), Method: req.Method, Path: req.URL.Path, UserPath: userPath, diff --git a/internal/auditlog/middleware_test.go b/internal/auditlog/middleware_test.go index 58e570e27..17a707058 100644 --- a/internal/auditlog/middleware_test.go +++ b/internal/auditlog/middleware_test.go @@ -7,8 +7,10 @@ import ( "testing" "github.com/labstack/echo/v5" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/enterpilot/gomodel/config" "github.com/enterpilot/gomodel/ext" "github.com/enterpilot/gomodel/internal/core" "github.com/enterpilot/gomodel/internal/echotest" @@ -312,3 +314,49 @@ func TestMiddlewarePublishesRemovalWhenHandlerPanics(t *testing.T) { require.Equal(t, LiveEventAuditStarted, logger.events[0].eventType) require.Equal(t, LiveEventAuditRemoved, logger.events[1].eventType) } + +// TestMiddlewareClientIPTagging checks the audit entry carries whatever address +// the server resolved for the request, on both a directly exposed gateway and +// one behind a trusted proxy network. +func TestMiddlewareClientIPTagging(t *testing.T) { + tests := []struct { + name string + proxies []string + remote string + xff string + wantIP string + }{ + { + name: "direct exposure records the socket peer", + remote: "203.0.113.7:4321", + xff: "198.51.100.8", + wantIP: "203.0.113.7", + }, + { + name: "behind a trusted proxy records the forwarded client", + proxies: []string{"127.0.0.0/8"}, + remote: "127.0.0.1:4321", + xff: "203.0.113.9, 198.51.100.7", + wantIP: "198.51.100.7", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serverCfg := config.ServerConfig{TrustedProxies: tt.proxies} + require.NoError(t, config.ResolveClientIPPolicy(&serverCfg)) + + logger := &capturingLogger{cfg: Config{Enabled: true, OnlyModelInteractions: true}} + handler := Middleware(logger)(func(*echo.Context) error { return nil }) + + c, _ := echotest.Post(t, "/v1/chat/completions", nil, + echotest.WithRemoteAddr(tt.remote), + echotest.WithHeader("X-Forwarded-For", tt.xff), + echotest.WithIPExtractor(serverCfg.ClientIP.Resolve)) + + require.NoError(t, handler(c)) + require.Len(t, logger.entries, 1) + assert.Equal(t, tt.wantIP, logger.entries[0].ClientIP) + }) + } +} diff --git a/internal/echotest/echotest.go b/internal/echotest/echotest.go index e9178355f..0d10c3035 100644 --- a/internal/echotest/echotest.go +++ b/internal/echotest/echotest.go @@ -25,6 +25,8 @@ type settings struct { path string values map[string]any contentType string + remoteAddr string + ipExtractor echo.IPExtractor } // WithHeader sets a request header. @@ -50,6 +52,19 @@ func WithValue(key string, value any) Option { return func(s *settings) { s.values[key] = value } } +// WithRemoteAddr sets the address of the connection the request arrived on, +// for handlers that distinguish the socket peer from a forwarded client. The +// value may carry a port ("203.0.113.7:4321") or not. +func WithRemoteAddr(addr string) Option { + return func(s *settings) { s.remoteAddr = addr } +} + +// WithIPExtractor installs the client address strategy the server would apply, +// so c.RealIP() reads forwarding headers the way the deployment does. +func WithIPExtractor(extractor echo.IPExtractor) Option { + return func(s *settings) { s.ipExtractor = extractor } +} + // WithContentType overrides the Content-Type set for a non-nil body. func WithContentType(contentType string) Option { return func(s *settings) { s.contentType = contentType } @@ -72,9 +87,20 @@ func Request(t testing.TB, method, target string, body any, opts ...Option) (*ec req.Header.Set(echo.HeaderContentType, s.contentType) } maps.Copy(req.Header, s.headers) + if s.remoteAddr != "" { + req.RemoteAddr = s.remoteAddr + } rec := httptest.NewRecorder() - c := echo.New().NewContext(req, rec) + e := echo.New() + // Echo's zero value consults forwarding headers; the server pins direct + // extraction unless a deployment opts in, so tests start from the same + // baseline. + e.IPExtractor = echo.ExtractIPDirect() + if s.ipExtractor != nil { + e.IPExtractor = s.ipExtractor + } + c := e.NewContext(req, rec) if s.path != "" { c.SetPath(s.path) } diff --git a/internal/server/clientip.go b/internal/server/clientip.go new file mode 100644 index 000000000..e508d0fc2 --- /dev/null +++ b/internal/server/clientip.go @@ -0,0 +1,18 @@ +package server + +import ( + "github.com/labstack/echo/v5" + + "github.com/enterpilot/gomodel/config" +) + +// ClientIPExtractor turns a resolved client-address policy into the strategy +// echo uses for c.RealIP(), so every consumer — audit entries, rate limit +// keys, request logs — reports the same address. A policy that trusts no +// proxy returns nil, leaving the direct extraction the server defaults to. +func ClientIPExtractor(policy config.ClientIPPolicy) echo.IPExtractor { + if !policy.Enabled() { + return nil + } + return policy.Resolve +} diff --git a/internal/server/clientip_test.go b/internal/server/clientip_test.go new file mode 100644 index 000000000..c430b9c1a --- /dev/null +++ b/internal/server/clientip_test.go @@ -0,0 +1,46 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/enterpilot/gomodel/config" +) + +// TestClientIPExtractor checks the server keeps direct extraction until a +// deployment lists its proxies, and then reports the forwarded client +// everywhere c.RealIP() is read. +func TestClientIPExtractor(t *testing.T) { + assert.Nil(t, ClientIPExtractor(config.ClientIPPolicy{}), "an unconfigured gateway must keep echo's direct extraction") + + cfg := config.ServerConfig{TrustedProxies: []string{"127.0.0.0/8"}} + require.NoError(t, config.ResolveClientIPPolicy(&cfg)) + + extractor := ClientIPExtractor(cfg.ClientIP) + require.NotNil(t, extractor) + + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = "127.0.0.1:4321" + req.Header.Set("X-Forwarded-For", "203.0.113.9, 198.51.100.7") + assert.Equal(t, "198.51.100.7", extractor(req)) +} + +// TestNewUsesConfiguredIPExtractor checks the server installs the strategy it +// was handed rather than leaving echo's default in place. +func TestNewUsesConfiguredIPExtractor(t *testing.T) { + cfg := config.ServerConfig{TrustedProxies: []string{"127.0.0.0/8"}} + require.NoError(t, config.ResolveClientIPPolicy(&cfg)) + + srv := New(nil, &Config{IPExtractor: ClientIPExtractor(cfg.ClientIP)}) + require.NotNil(t, srv) + + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) + req.RemoteAddr = "127.0.0.1:4321" + req.Header.Set("X-Forwarded-For", "203.0.113.9") + c := srv.echo.NewContext(req, httptest.NewRecorder()) + assert.Equal(t, "203.0.113.9", c.RealIP()) +} diff --git a/tests/e2e/auditlog_test.go b/tests/e2e/auditlog_test.go index 2fe0a3c8b..a6335bbb0 100644 --- a/tests/e2e/auditlog_test.go +++ b/tests/e2e/auditlog_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/enterpilot/gomodel/config" "github.com/enterpilot/gomodel/internal/auditlog" "github.com/enterpilot/gomodel/internal/core" "github.com/enterpilot/gomodel/internal/providers" @@ -97,7 +98,11 @@ func (m *mockLogStore) WaitForAPIEntries(count int, timeout time.Duration) []*au } // setupAuditLogTestServer creates a test server with audit logging enabled -func setupAuditLogTestServer(t *testing.T, cfg auditlog.Config, store *mockLogStore) (string, func()) { +// serverOption adjusts the server configuration an audit log test runs +// against, for behavior that is decided outside the audit logger. +type serverOption func(*server.Config) + +func setupAuditLogTestServer(t *testing.T, cfg auditlog.Config, store *mockLogStore, opts ...serverOption) (string, func()) { t.Helper() // Reserve a loopback listener up front so the port cannot be stolen before @@ -120,9 +125,11 @@ func setupAuditLogTestServer(t *testing.T, cfg auditlog.Config, store *mockLogSt logger := auditlog.NewLogger(store, cfg) // Create server with audit logging - srv := server.New(router, &server.Config{ - AuditLogger: logger, - }) + serverCfg := &server.Config{AuditLogger: logger} + for _, opt := range opts { + opt(serverCfg) + } + srv := server.New(router, serverCfg) // Start server (bind to loopback only) serverURL := "http://" + listener.Addr().String() @@ -864,58 +871,64 @@ func TestAuditLogOnlyModelInteractions(t *testing.T) { // connection address by default, and record the forwarded client instead once // the gateway's own proxy network is listed as trusted. func TestAuditLogClientIP(t *testing.T) { - t.Run("records the connection address when no proxies are trusted", func(t *testing.T) { - store := newMockLogStore() - cfg := auditlog.Config{ - Enabled: true, - BufferSize: 100, - FlushInterval: 100 * time.Millisecond, - OnlyModelInteractions: true, - } - - serverURL, cleanup := setupAuditLogTestServer(t, cfg, store) - defer cleanup() - - body, _ := json.Marshal(defaultChatReq("Hello")) - req, err := http.NewRequest(http.MethodPost, serverURL+"/v1/chat/completions", bytes.NewReader(body)) - require.NoError(t, err) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-Forwarded-For", "198.51.100.23") - resp, err := http.DefaultClient.Do(req) - require.NoError(t, err) - defer closeBody(resp) - require.Equal(t, http.StatusOK, resp.StatusCode) + tests := []struct { + name string + proxies []string + header string + xffValue string + wantIP string + }{ + { + name: "records the connection address when no proxies are trusted", + xffValue: "198.51.100.23", + wantIP: "127.0.0.1", + }, + { + name: "records the forwarded client behind a trusted proxy network", + proxies: []string{"127.0.0.0/8"}, + xffValue: "203.0.113.9, 198.51.100.23", + wantIP: "198.51.100.23", + }, + { + name: "the loopback preset covers a proxy on the gateway host", + proxies: []string{"loopback"}, + xffValue: "198.51.100.23", + wantIP: "198.51.100.23", + }, + } - entries := store.WaitForAPIEntries(1, 2*time.Second) - require.Len(t, entries, 1) - assert.Equal(t, "127.0.0.1", entries[0].ClientIP, "forwarding headers must not be trusted by default") - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serverCfg := config.ServerConfig{TrustedProxies: tt.proxies, ClientIPHeader: tt.header} + require.NoError(t, config.ResolveClientIPPolicy(&serverCfg)) - t.Run("records the forwarded client behind a trusted proxy network", func(t *testing.T) { - store := newMockLogStore() - cfg := auditlog.Config{ - Enabled: true, - BufferSize: 100, - FlushInterval: 100 * time.Millisecond, - OnlyModelInteractions: true, - TrustedProxies: auditlog.ParseTrustedProxies([]string{"127.0.0.0/8"}), - } + store := newMockLogStore() + cfg := auditlog.Config{ + Enabled: true, + BufferSize: 100, + FlushInterval: 100 * time.Millisecond, + OnlyModelInteractions: true, + } - serverURL, cleanup := setupAuditLogTestServer(t, cfg, store) - defer cleanup() + serverURL, cleanup := setupAuditLogTestServer(t, cfg, store, func(c *server.Config) { + c.IPExtractor = server.ClientIPExtractor(serverCfg.ClientIP) + }) + defer cleanup() - body, _ := json.Marshal(defaultChatReq("Hello")) - req, err := http.NewRequest(http.MethodPost, serverURL+"/v1/chat/completions", bytes.NewReader(body)) - require.NoError(t, err) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-Forwarded-For", "203.0.113.9, 198.51.100.23") - resp, err := http.DefaultClient.Do(req) - require.NoError(t, err) - defer closeBody(resp) - require.Equal(t, http.StatusOK, resp.StatusCode) + body, err := json.Marshal(defaultChatReq("Hello")) + require.NoError(t, err) + req, err := http.NewRequest(http.MethodPost, serverURL+"/v1/chat/completions", bytes.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Forwarded-For", tt.xffValue) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer closeBody(resp) + require.Equal(t, http.StatusOK, resp.StatusCode) - entries := store.WaitForAPIEntries(1, 2*time.Second) - require.Len(t, entries, 1) - assert.Equal(t, "198.51.100.23", entries[0].ClientIP) - }) + entries := store.WaitForAPIEntries(1, 2*time.Second) + require.Len(t, entries, 1) + assert.Equal(t, tt.wantIP, entries[0].ClientIP) + }) + } } From 0845d5510cd08d490e5fbd38757f714f9b7f0a53 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Sun, 20 Sep 2026 12:29:09 +0200 Subject: [PATCH 2/2] test(server): exercise the client IP wiring through the extractor and a single-address header --- internal/server/clientip_test.go | 6 +++-- tests/e2e/auditlog_test.go | 46 ++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/internal/server/clientip_test.go b/internal/server/clientip_test.go index c430b9c1a..e535ffe9a 100644 --- a/internal/server/clientip_test.go +++ b/internal/server/clientip_test.go @@ -37,10 +37,12 @@ func TestNewUsesConfiguredIPExtractor(t *testing.T) { srv := New(nil, &Config{IPExtractor: ClientIPExtractor(cfg.ClientIP)}) require.NotNil(t, srv) + require.NotNil(t, srv.echo.IPExtractor) req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil) req.RemoteAddr = "127.0.0.1:4321" req.Header.Set("X-Forwarded-For", "203.0.113.9") - c := srv.echo.NewContext(req, httptest.NewRecorder()) - assert.Equal(t, "203.0.113.9", c.RealIP()) + // The extractor is what every c.RealIP() in the server runs, so calling it + // as echo would is the whole of the wiring under test. + assert.Equal(t, "203.0.113.9", srv.echo.IPExtractor(req)) } diff --git a/tests/e2e/auditlog_test.go b/tests/e2e/auditlog_test.go index a6335bbb0..372c4c22d 100644 --- a/tests/e2e/auditlog_test.go +++ b/tests/e2e/auditlog_test.go @@ -872,28 +872,38 @@ func TestAuditLogOnlyModelInteractions(t *testing.T) { // the gateway's own proxy network is listed as trusted. func TestAuditLogClientIP(t *testing.T) { tests := []struct { - name string - proxies []string - header string - xffValue string - wantIP string + name string + proxies []string + header string + headers map[string]string + wantIP string }{ { - name: "records the connection address when no proxies are trusted", - xffValue: "198.51.100.23", - wantIP: "127.0.0.1", + name: "records the connection address when no proxies are trusted", + headers: map[string]string{"X-Forwarded-For": "198.51.100.23"}, + wantIP: "127.0.0.1", }, { - name: "records the forwarded client behind a trusted proxy network", - proxies: []string{"127.0.0.0/8"}, - xffValue: "203.0.113.9, 198.51.100.23", - wantIP: "198.51.100.23", + name: "records the forwarded client behind a trusted proxy network", + proxies: []string{"127.0.0.0/8"}, + headers: map[string]string{"X-Forwarded-For": "203.0.113.9, 198.51.100.23"}, + wantIP: "198.51.100.23", }, { - name: "the loopback preset covers a proxy on the gateway host", - proxies: []string{"loopback"}, - xffValue: "198.51.100.23", - wantIP: "198.51.100.23", + name: "the loopback preset covers a proxy on the gateway host", + proxies: []string{"loopback"}, + headers: map[string]string{"X-Forwarded-For": "198.51.100.23"}, + wantIP: "198.51.100.23", + }, + { + name: "a configured single-address header wins over the chain", + proxies: []string{"loopback"}, + header: "X-Real-IP", + headers: map[string]string{ + "X-Real-IP": "198.51.100.23", + "X-Forwarded-For": "203.0.113.9", + }, + wantIP: "198.51.100.23", }, } @@ -920,7 +930,9 @@ func TestAuditLogClientIP(t *testing.T) { req, err := http.NewRequest(http.MethodPost, serverURL+"/v1/chat/completions", bytes.NewReader(body)) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-Forwarded-For", tt.xffValue) + for name, value := range tt.headers { + req.Header.Set(name, value) + } resp, err := http.DefaultClient.Do(req) require.NoError(t, err) defer closeBody(resp)