From 9f83f899a417aac97c707df6164fb7897ec94297 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Fri, 2 Oct 2026 13:47:45 +1000 Subject: [PATCH] refactor: move client address handling to ingress --- spinifex/gateway/admin.go | 4 +- spinifex/gateway/auth.go | 4 +- spinifex/gateway/gateway.go | 3 +- spinifex/gateway/requestaudit.go | 4 +- spinifex/ingress/http/client_ip.go | 35 ++++++++++++ spinifex/ingress/http/client_ip_test.go | 66 ++++++++++++++++++++++ spinifex/runtime/roles/spinifexui/proxy.go | 4 +- spinifex/utils/utils.go | 27 --------- spinifex/utils/utils_test.go | 55 ------------------ 9 files changed, 111 insertions(+), 91 deletions(-) create mode 100644 spinifex/ingress/http/client_ip.go create mode 100644 spinifex/ingress/http/client_ip_test.go diff --git a/spinifex/gateway/admin.go b/spinifex/gateway/admin.go index c06e93e31..350f55b12 100644 --- a/spinifex/gateway/admin.go +++ b/spinifex/gateway/admin.go @@ -14,7 +14,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/mulgadc/spinifex/spinifex/admin" "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" - "github.com/mulgadc/spinifex/spinifex/utils" + ingresshttp "github.com/mulgadc/spinifex/spinifex/ingress/http" ) // adminMethods lists the callable /admin/ names. A method absent here @@ -168,7 +168,7 @@ func (gw *GatewayConfig) authorizeAdmin(r *http.Request, method string) error { deny := func(reason string) error { slog.Warn("Admin: access denied", "method", method, "reason", reason, "service", service, "accountID", accountID, "identity", identity, - "principalType", principalType, "sourceIP", utils.ClientIP(r.RemoteAddr)) + "principalType", principalType, "sourceIP", ingresshttp.ClientIP(r.RemoteAddr)) return errors.New(awserrors.ErrorAccessDenied) } diff --git a/spinifex/gateway/auth.go b/spinifex/gateway/auth.go index 6ad59d3dd..408312b47 100644 --- a/spinifex/gateway/auth.go +++ b/spinifex/gateway/auth.go @@ -18,7 +18,7 @@ import ( gateway_eks "github.com/mulgadc/spinifex/spinifex/gateway/eks" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" - "github.com/mulgadc/spinifex/spinifex/utils" + ingresshttp "github.com/mulgadc/spinifex/spinifex/ingress/http" ) // AKID prefixes. Prefix-first dispatch prevents a misfiled record from being @@ -38,7 +38,7 @@ func (gw *GatewayConfig) SigV4AuthMiddleware() func(http.Handler) http.Handler { } return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - clientIP := utils.ClientIP(r.RemoteAddr) + clientIP := ingresshttp.ClientIP(r.RemoteAddr) if errCode := gw.RateLimiter.CheckIP(clientIP); errCode != "" { gw.writeSigV4Error(w, r, errCode, "") return diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index fb7948125..fe3202747 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -41,6 +41,7 @@ import ( handlers_ochrevector "github.com/mulgadc/spinifex/spinifex/handlers/ochrevector" handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" + ingresshttp "github.com/mulgadc/spinifex/spinifex/ingress/http" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" @@ -686,7 +687,7 @@ func requestConditionKeys(r *http.Request, principal principalContext) iampolicy // Derived from the request rather than read from the context: the OCI // registry chain never runs SigV4AuthMiddleware, so a context-carried // address would be absent there and every aws:SourceIp condition inert. - if ip := utils.ClientIP(r.RemoteAddr); ip != "" { + if ip := ingresshttp.ClientIP(r.RemoteAddr); ip != "" { keys[iampolicy.KeySourceIP] = ip } // Resolved from the credential record, never from anything the caller diff --git a/spinifex/gateway/requestaudit.go b/spinifex/gateway/requestaudit.go index 1fe2cf4de..2dd32fdf4 100644 --- a/spinifex/gateway/requestaudit.go +++ b/spinifex/gateway/requestaudit.go @@ -12,7 +12,7 @@ import ( "go.opentelemetry.io/otel/trace" "github.com/mulgadc/spinifex/spinifex/foundation/telemetry" - "github.com/mulgadc/spinifex/spinifex/utils" + ingresshttp "github.com/mulgadc/spinifex/spinifex/ingress/http" ) // ctxAudit carries the per-request audit record. It is a pointer, unlike every @@ -43,7 +43,7 @@ type requestAudit struct { // logging happens to be enabled. func requestAuditMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - audit := &requestAudit{clientIP: utils.RequestClientIP(r), requestID: requestIDFrom(r)} + audit := &requestAudit{clientIP: ingresshttp.RequestClientIP(r), requestID: requestIDFrom(r)} ctx := context.WithValue(r.Context(), ctxAudit, audit) next.ServeHTTP(w, r.WithContext(ctx)) audit.annotate(ctx) diff --git a/spinifex/ingress/http/client_ip.go b/spinifex/ingress/http/client_ip.go new file mode 100644 index 000000000..95a2ff98a --- /dev/null +++ b/spinifex/ingress/http/client_ip.go @@ -0,0 +1,35 @@ +// Package http provides generic HTTP ingress helpers. +package http + +import ( + "net" + "net/http" + "net/netip" + "strings" +) + +// RequestClientIP is the caller's address: X-Real-IP when the connection is +// from loopback, where only a local proxy (the nginx edge or console) can have +// set it, and RemoteAddr otherwise, so a direct client cannot choose what is +// logged. +func RequestClientIP(r *http.Request) string { + ip := ClientIP(r.RemoteAddr) + if addr, err := netip.ParseAddr(ip); err != nil || !addr.IsLoopback() { + return ip + } + realIP, err := netip.ParseAddr(strings.TrimSpace(r.Header.Get("X-Real-IP"))) + if err != nil { + return ip + } + return realIP.String() +} + +// ClientIP returns the IP from a RemoteAddr, stripping the port. Handles both +// IPv4 and IPv6, and tolerates a RemoteAddr that is already a bare IP (no port). +func ClientIP(remoteAddr string) string { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + return remoteAddr + } + return host +} diff --git a/spinifex/ingress/http/client_ip_test.go b/spinifex/ingress/http/client_ip_test.go new file mode 100644 index 000000000..3864a09c1 --- /dev/null +++ b/spinifex/ingress/http/client_ip_test.go @@ -0,0 +1,66 @@ +package http + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestClientIP(t *testing.T) { + tests := []struct { + name string + remoteAddr string + expected string + }{ + {"IPv4 with port", "192.168.1.1:12345", "192.168.1.1"}, + {"IPv6 with port", "[::1]:12345", "::1"}, + {"IPv4 bare", "192.168.1.1", "192.168.1.1"}, + {"IPv6 full with port", "[2001:db8::1]:443", "2001:db8::1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ClientIP(tt.remoteAddr); got != tt.expected { + t.Errorf("ClientIP(%q) = %q, want %q", tt.remoteAddr, got, tt.expected) + } + }) + } +} + +// X-Real-IP is honoured only on a loopback connection, where the local edge set +// it. From anywhere else it, and X-Forwarded-For, are the client's own claim. +func TestRequestClientIP(t *testing.T) { + tests := []struct { + name string + remoteAddr string + realIP string + xff string + expected string + }{ + {"loopback trusts X-Real-IP", "127.0.0.1:40000", "198.51.100.7", "", "198.51.100.7"}, + {"IPv6 loopback trusts X-Real-IP", "[::1]:40000", "2001:db8::7", "", "2001:db8::7"}, + {"4in6 loopback trusts X-Real-IP", "[::ffff:127.0.0.1]:40000", "198.51.100.7", "", "198.51.100.7"}, + {"loopback without header", "127.0.0.1:40000", "", "", "127.0.0.1"}, + {"loopback with garbage header", "127.0.0.1:40000", "not-an-ip", "", "127.0.0.1"}, + {"loopback ignores X-Forwarded-For", "127.0.0.1:40000", "", "203.0.113.1", "127.0.0.1"}, + {"spoofed X-Real-IP from public client", "203.0.113.9:5555", "10.0.0.1", "", "203.0.113.9"}, + {"spoofed X-Forwarded-For from public client", "203.0.113.9:5555", "", "127.0.0.1", "203.0.113.9"}, + {"spoofed both from private client", "10.2.0.3:5555", "127.0.0.1", "127.0.0.1", "10.2.0.3"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = tt.remoteAddr + if tt.realIP != "" { + r.Header.Set("X-Real-IP", tt.realIP) + } + if tt.xff != "" { + r.Header.Set("X-Forwarded-For", tt.xff) + } + if got := RequestClientIP(r); got != tt.expected { + t.Errorf("RequestClientIP() = %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/spinifex/runtime/roles/spinifexui/proxy.go b/spinifex/runtime/roles/spinifexui/proxy.go index 05f887bb4..eceea4ac1 100644 --- a/spinifex/runtime/roles/spinifexui/proxy.go +++ b/spinifex/runtime/roles/spinifexui/proxy.go @@ -14,7 +14,7 @@ import ( "time" "github.com/mulgadc/bluebottle/pkg/tlsconfig" - "github.com/mulgadc/spinifex/spinifex/utils" + ingresshttp "github.com/mulgadc/spinifex/spinifex/ingress/http" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) @@ -63,7 +63,7 @@ func newReverseProxy(backendHost, pathPrefix string, transport http.RoundTripper // The backend trusts X-Real-IP from loopback, which this hop always is. // The UI also listens publicly, so re-derive it rather than forward a // client's own claim. X-Request-ID passes through unchanged. - pr.Out.Header.Set("X-Real-IP", utils.RequestClientIP(pr.In)) + pr.Out.Header.Set("X-Real-IP", ingresshttp.RequestClientIP(pr.In)) // Strip the prefix from both path forms. RawPath must survive: a // client that percent-encodes a segment (a ':' in a bedrock diff --git a/spinifex/utils/utils.go b/spinifex/utils/utils.go index 1d77b865b..55e5b43a4 100644 --- a/spinifex/utils/utils.go +++ b/spinifex/utils/utils.go @@ -14,12 +14,10 @@ import ( "log/slog" "net" "net/http" - "net/netip" "os" "os/signal" "reflect" "slices" - "strings" "time" "github.com/aws/aws-sdk-go/aws" @@ -415,31 +413,6 @@ func HashMAC(id string) string { return net.HardwareAddr(b).String() } -// RequestClientIP is the caller's address: X-Real-IP when the connection is from -// loopback, where only a local proxy (the nginx edge, the console) can have set -// it, and RemoteAddr otherwise, so a direct client cannot choose what is logged. -func RequestClientIP(r *http.Request) string { - ip := ClientIP(r.RemoteAddr) - if addr, err := netip.ParseAddr(ip); err != nil || !addr.IsLoopback() { - return ip - } - realIP, err := netip.ParseAddr(strings.TrimSpace(r.Header.Get("X-Real-IP"))) - if err != nil { - return ip - } - return realIP.String() -} - -// ClientIP returns the IP from a RemoteAddr, stripping the port. Handles both -// IPv4 and IPv6, and tolerates a RemoteAddr that is already a bare IP (no port). -func ClientIP(remoteAddr string) string { - host, _, err := net.SplitHostPort(remoteAddr) - if err != nil { - return remoteAddr - } - return host -} - func dirExists(path string) bool { info, err := os.Stat(path) if os.IsNotExist(err) { diff --git a/spinifex/utils/utils_test.go b/spinifex/utils/utils_test.go index 86a5aeb17..ccc973e17 100644 --- a/spinifex/utils/utils_test.go +++ b/spinifex/utils/utils_test.go @@ -1775,25 +1775,6 @@ func TestDistroFamily(t *testing.T) { } } -func TestClientIP(t *testing.T) { - tests := []struct { - name string - remoteAddr string - expected string - }{ - {"IPv4 with port", "192.168.1.1:12345", "192.168.1.1"}, - {"IPv6 with port", "[::1]:12345", "::1"}, - {"IPv4 bare", "192.168.1.1", "192.168.1.1"}, - {"IPv6 full with port", "[2001:db8::1]:443", "2001:db8::1"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, ClientIP(tt.remoteAddr)) - }) - } -} - func TestAvailableImages_ECSNodeEntry(t *testing.T) { img, ok := AvailableImages["spinifex-ecs-node"] require.True(t, ok, "spinifex-ecs-node must be in the system image catalog") @@ -1899,39 +1880,3 @@ func TestWithRequestID_PointerToStruct(t *testing.T) { require.Equal(t, "req-456", v.FieldByName("RequestId").String()) require.Equal(t, []string{"b"}, v.FieldByName("Names").Interface()) } - -// X-Real-IP is honoured only on a loopback connection, where the local edge set -// it. From anywhere else it, and X-Forwarded-For, are the client's own claim. -func TestRequestClientIP(t *testing.T) { - tests := []struct { - name string - remoteAddr string - realIP string - xff string - expected string - }{ - {"loopback trusts X-Real-IP", "127.0.0.1:40000", "198.51.100.7", "", "198.51.100.7"}, - {"IPv6 loopback trusts X-Real-IP", "[::1]:40000", "2001:db8::7", "", "2001:db8::7"}, - {"4in6 loopback trusts X-Real-IP", "[::ffff:127.0.0.1]:40000", "198.51.100.7", "", "198.51.100.7"}, - {"loopback without header", "127.0.0.1:40000", "", "", "127.0.0.1"}, - {"loopback with garbage header", "127.0.0.1:40000", "not-an-ip", "", "127.0.0.1"}, - {"loopback ignores X-Forwarded-For", "127.0.0.1:40000", "", "203.0.113.1", "127.0.0.1"}, - {"spoofed X-Real-IP from public client", "203.0.113.9:5555", "10.0.0.1", "", "203.0.113.9"}, - {"spoofed X-Forwarded-For from public client", "203.0.113.9:5555", "", "127.0.0.1", "203.0.113.9"}, - {"spoofed both from private client", "10.2.0.3:5555", "127.0.0.1", "127.0.0.1", "10.2.0.3"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := httptest.NewRequest(http.MethodGet, "/", nil) - r.RemoteAddr = tt.remoteAddr - if tt.realIP != "" { - r.Header.Set("X-Real-IP", tt.realIP) - } - if tt.xff != "" { - r.Header.Set("X-Forwarded-For", tt.xff) - } - assert.Equal(t, tt.expected, RequestClientIP(r)) - }) - } -}