From 08c063a8760a77faf7086642bd753dfede379b1e Mon Sep 17 00:00:00 2001 From: Iggy Jackson Date: Wed, 16 Sep 2026 18:08:56 -0700 Subject: [PATCH] feat: add setting to show real client IP instead of proxy IP in audit logs Add support for allowing gomodel to show the real client IP from x-forwarded-for headers via a trusted cidrs setting. --- .env.template | 8 + config/config.example.yaml | 7 + config/config.go | 3 + config/config_test.go | 2 +- config/logging.go | 88 ++++++++++- config/logging_test.go | 87 +++++++++++ docs/advanced/configuration.mdx | 18 +++ internal/app/app.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 +- tests/e2e/auditlog_test.go | 60 ++++++++ 14 files changed, 630 insertions(+), 3 deletions(-) create mode 100644 internal/auditlog/clientip.go create mode 100644 internal/auditlog/clientip_test.go diff --git a/.env.template b/.env.template index 32323840f..3d34b48e9 100644 --- a/.env.template +++ b/.env.template @@ -451,6 +451,14 @@ # 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/config.example.yaml b/config/config.example.yaml index c16eccf9f..e8f77bc2f 100644 --- a/config/config.example.yaml +++ b/config/config.example.yaml @@ -217,6 +217,13 @@ 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 5d0b4be5f..538149c46 100644 --- a/config/config.go +++ b/config/config.go @@ -338,6 +338,9 @@ 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 { + return nil, err + } return &LoadResult{ Config: cfg, diff --git a/config/config_test.go b/config/config_test.go index c56b9bd25..ff7982bec 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -79,7 +79,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_BUFFER_SIZE", + "LOGGING_ONLY_MODEL_INTERACTIONS", "LOGGING_TRUSTED_PROXY_CIDRS", "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 0586523a3..a96e92419 100644 --- a/config/logging.go +++ b/config/logging.go @@ -1,6 +1,10 @@ package config -import "strings" +import ( + "fmt" + "net" + "strings" +) // LogConfig holds audit logging configuration type LogConfig struct { @@ -84,6 +88,88 @@ 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 ef3f16619..c51b176f9 100644 --- a/config/logging_test.go +++ b/config/logging_test.go @@ -57,3 +57,90 @@ 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/docs/advanced/configuration.mdx b/docs/advanced/configuration.mdx index 51bcb9182..2a9c7dc26 100644 --- a/docs/advanced/configuration.mdx +++ b/docs/advanced/configuration.mdx @@ -163,6 +163,7 @@ 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)_ | Realtime dashboard previews and persisted audit logs are separate features. With `DASHBOARD_LIVE_LOGS_ENABLED=true` and `LOGGING_ENABLED=false`, requests @@ -179,6 +180,23 @@ 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 98b7046da..01c342931 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -404,6 +404,7 @@ 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/auditlog/auditlog.go b/internal/auditlog/auditlog.go index dc8fadbe5..fab552133 100644 --- a/internal/auditlog/auditlog.go +++ b/internal/auditlog/auditlog.go @@ -543,4 +543,9 @@ 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 new file mode 100644 index 000000000..49a4c4ae9 --- /dev/null +++ b/internal/auditlog/clientip.go @@ -0,0 +1,124 @@ +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 new file mode 100644 index 000000000..0df1d3514 --- /dev/null +++ b/internal/auditlog/clientip_test.go @@ -0,0 +1,227 @@ +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 43a9a6fcf..06b82472c 100644 --- a/internal/auditlog/factory.go +++ b/internal/auditlog/factory.go @@ -83,6 +83,7 @@ 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 01ead0eef..e4c345003 100644 --- a/internal/auditlog/middleware.go +++ b/internal/auditlog/middleware.go @@ -67,7 +67,7 @@ func Middleware(logger LoggerInterface) echo.MiddlewareFunc { ID: uuid.NewString(), Timestamp: start, RequestID: requestID, - ClientIP: c.RealIP(), + ClientIP: cfg.TrustedProxies.ClientIP(req, c.RealIP()), Method: req.Method, Path: req.URL.Path, UserPath: userPath, diff --git a/tests/e2e/auditlog_test.go b/tests/e2e/auditlog_test.go index d1a6dbfb6..99320152e 100644 --- a/tests/e2e/auditlog_test.go +++ b/tests/e2e/auditlog_test.go @@ -859,3 +859,63 @@ func TestAuditLogOnlyModelInteractions(t *testing.T) { assert.Equal(t, "/v1/chat/completions", entries[0].Path) }) } + +// TestAuditLogClientIP checks that audit entries keep recording the +// 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) + + 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") + }) + + 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"}), + } + + 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", "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) + + entries := store.WaitForAPIEntries(1, 2*time.Second) + require.Len(t, entries, 1) + assert.Equal(t, "198.51.100.23", entries[0].ClientIP) + }) +}