diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 00063c1..b945778 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -1232,14 +1232,12 @@ func setupRouter(server *handlers.Server, stockholmHandler *stockholm.Handler, w // every downstream middleware and the recorder see the cleaned path. r.Use(middleware.CleanPath) - // TrustedRealIP must run before any handler that reads r.RemoteAddr — + // ClientIPMiddleware must run before any handler that reads the client IP — // SnapshotMiddleware captures the request, and several handlers - // (HandleMargePowerOn, etc.) inspect the source IP. The middleware is - // gated on Settings.TrustForwardedHeaders; when off (the safe default), - // it returns nil and we skip Use'ing it entirely. - if mw := server.TrustedRealIPMiddleware(); mw != nil { - r.Use(mw) - } + // (HandleMargePowerOn, etc.) inspect the source IP via middleware.GetClientIP. + // Always wired: at minimum the socket peer is recorded; when + // TrustForwardedHeaders is on and the peer is trusted, XFF is resolved. + r.Use(server.ClientIPMiddleware()) r.Use(server.SnapshotMiddleware) r.Use(server.OriginMiddleware) diff --git a/pkg/service/handlers/middleware_clientip.go b/pkg/service/handlers/middleware_clientip.go new file mode 100644 index 0000000..2c4d265 --- /dev/null +++ b/pkg/service/handlers/middleware_clientip.go @@ -0,0 +1,139 @@ +package handlers + +import ( + "fmt" + "log" + "net" + "net/http" + "net/netip" + + "github.com/go-chi/chi/v5/middleware" +) + +// defaultTrustedProxyCIDRs is the safe-by-default list applied when +// Settings.TrustedProxyCIDRs is empty. Only loopback addresses are trusted — +// i.e. a reverse proxy on the same host. Anyone deploying behind a proxy on a +// different host must override this in settings.json. +var defaultTrustedProxyCIDRs = []string{ + "127.0.0.0/8", + "::1/128", +} + +// clientIPMiddleware resolves the client IP into the request context (read via +// middleware.GetClientIP). The socket peer is always recorded. When +// trustForwardedHeaders is set AND the immediate TCP peer is one of trustedPeers, +// the X-Forwarded-For chain is consulted (chi walks it right-to-left, skipping +// trustedCIDRStrings, taking the first untrusted entry). On a flat LAN a +// non-trusted peer's XFF is ignored, so a malicious speaker can't spoof its IP. +func clientIPMiddleware(trustForwardedHeaders bool, trustedPeers []*net.IPNet, trustedCIDRStrings []string) func(http.Handler) http.Handler { + base := middleware.ClientIPFromRemoteAddr + + if !trustForwardedHeaders || len(trustedPeers) == 0 { + return func(next http.Handler) http.Handler { return base(next) } + } + + xff := middleware.ClientIPFromXFF(trustedCIDRStrings...) + + return func(next http.Handler) http.Handler { + trusted := base(xff(next)) // peer set first, XFF overrides when found + untrusted := base(next) // peer only + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if isFromTrustedPeer(r.RemoteAddr, trustedPeers) { + trusted.ServeHTTP(w, r) + + return + } + + untrusted.ServeHTTP(w, r) + }) + } +} + +// isFromTrustedPeer reports whether remoteAddr (in the host:port shape that +// net/http populates) is contained in any of the supplied CIDR blocks. +func isFromTrustedPeer(remoteAddr string, trustedPeers []*net.IPNet) bool { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + host = remoteAddr + } + + ip := net.ParseIP(host) + if ip == nil { + return false + } + + for _, n := range trustedPeers { + if n.Contains(ip) { + return true + } + } + + return false +} + +// ParseTrustedProxyCIDRs converts string CIDRs into *net.IPNet values, falling +// back to defaultTrustedProxyCIDRs when the input is empty. An invalid CIDR +// in the input list is reported as an error and stops parsing — better to +// fail loud than silently fall back. +func ParseTrustedProxyCIDRs(cidrs []string) ([]*net.IPNet, error) { + if len(cidrs) == 0 { + cidrs = defaultTrustedProxyCIDRs + } + + out := make([]*net.IPNet, 0, len(cidrs)) + + for _, c := range cidrs { + _, n, err := net.ParseCIDR(c) + if err != nil { + return nil, fmt.Errorf("invalid trusted proxy CIDR %q: %w", c, err) + } + + out = append(out, n) + } + + return out, nil +} + +// validateCIDRStringsForXFF checks that every CIDR string can be parsed by +// netip.ParsePrefix, which is what middleware.ClientIPFromXFF uses internally +// (it calls netip.MustParsePrefix and panics on failure). Returns an error +// listing the first bad entry, so the caller can fall back to peer-only mode +// rather than panicking at startup. +func validateCIDRStringsForXFF(cidrs []string) error { + for _, c := range cidrs { + if _, err := netip.ParsePrefix(c); err != nil { + return fmt.Errorf("CIDR %q is not valid for ClientIPFromXFF: %w", c, err) + } + } + + return nil +} + +// buildClientIPMiddleware is the server-level helper that reads settings and +// returns a ready-to-use chi middleware. It is split out of ClientIPMiddleware +// so tests can drive the logic without a full Server. +func buildClientIPMiddleware(trustForwardedHeaders bool, cidrStrings []string) func(http.Handler) http.Handler { + if !trustForwardedHeaders { + return clientIPMiddleware(false, nil, nil) + } + + if len(cidrStrings) == 0 { + cidrStrings = defaultTrustedProxyCIDRs + } + + // Validate for netip.MustParsePrefix (panic guard). + if err := validateCIDRStringsForXFF(cidrStrings); err != nil { + log.Printf("[ClientIP] invalid trusted_proxy_cidrs: %v — falling back to peer-only", err) + return clientIPMiddleware(false, nil, nil) + } + + // Parse for the peer gate (net.IPNet). + cidrs, err := ParseTrustedProxyCIDRs(cidrStrings) + if err != nil { + log.Printf("[ClientIP] invalid trusted_proxy_cidrs: %v — falling back to peer-only", err) + return clientIPMiddleware(false, nil, nil) + } + + return clientIPMiddleware(true, cidrs, cidrStrings) +} diff --git a/pkg/service/handlers/middleware_clientip_test.go b/pkg/service/handlers/middleware_clientip_test.go new file mode 100644 index 0000000..ed74d9a --- /dev/null +++ b/pkg/service/handlers/middleware_clientip_test.go @@ -0,0 +1,187 @@ +package handlers + +import ( + "net" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5/middleware" +) + +// captureClientIP is a tiny handler that records the resolved client IP from +// the request context (as set by clientIPMiddleware). +func captureClientIP(got *string) http.Handler { + return http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + *got = middleware.GetClientIP(r.Context()) + }) +} + +func TestClientIPMiddleware(t *testing.T) { + defaultCIDRs := []string{"127.0.0.0/8", "::1/128"} + + defaultPeers, err := ParseTrustedProxyCIDRs(defaultCIDRs) + if err != nil { + t.Fatalf("ParseTrustedProxyCIDRs: %v", err) + } + + cases := []struct { + name string + trustForwardedHeaders bool + trustedPeers []*net.IPNet + trustedCIDRStrings []string + remoteAddr string + xForwardedFor string + wantClientIP string + }{ + { + name: "trust disabled: peer recorded, XFF ignored", + trustForwardedHeaders: false, + remoteAddr: "1.2.3.4:5555", + xForwardedFor: "9.9.9.9", + wantClientIP: "1.2.3.4", + }, + { + name: "trust enabled, trusted loopback peer, XFF honoured", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "127.0.0.1:5555", + xForwardedFor: "9.9.9.9", + wantClientIP: "9.9.9.9", + }, + { + name: "trust enabled, untrusted peer, XFF ignored (peer gate)", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "8.8.8.8:5555", + xForwardedFor: "9.9.9.9", + wantClientIP: "8.8.8.8", + }, + { + name: "trust enabled, trusted peer, XFF chain rightmost-untrusted wins", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "127.0.0.1:5555", + xForwardedFor: "9.9.9.9, 127.0.0.2", + wantClientIP: "9.9.9.9", + }, + { + name: "trust enabled, trusted peer, garbage XFF falls back to peer", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "127.0.0.1:5555", + xForwardedFor: "not-an-ip", + wantClientIP: "127.0.0.1", + }, + { + name: "trust enabled, trusted IPv6 loopback peer, XFF honoured", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "[::1]:5555", + xForwardedFor: "9.9.9.9", + wantClientIP: "9.9.9.9", + }, + { + name: "trust enabled, trusted peer, no XFF header: peer recorded", + trustForwardedHeaders: true, + trustedPeers: defaultPeers, + trustedCIDRStrings: defaultCIDRs, + remoteAddr: "127.0.0.1:5555", + xForwardedFor: "", + wantClientIP: "127.0.0.1", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var got string + + mw := clientIPMiddleware(tc.trustForwardedHeaders, tc.trustedPeers, tc.trustedCIDRStrings) + h := mw(captureClientIP(&got)) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = tc.remoteAddr + + if tc.xForwardedFor != "" { + req.Header.Set("X-Forwarded-For", tc.xForwardedFor) + } + + h.ServeHTTP(httptest.NewRecorder(), req) + + if got != tc.wantClientIP { + t.Errorf("GetClientIP = %q, want %q", got, tc.wantClientIP) + } + }) + } +} + +func TestClientIPMiddleware_AlwaysPopulated(t *testing.T) { + // Regardless of trust settings, GetClientIP must always return a non-empty + // string after clientIPMiddleware has run (the socket peer is the fallback). + mw := clientIPMiddleware(false, nil, nil) + + var got string + + h := mw(captureClientIP(&got)) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.1:1234" + + h.ServeHTTP(httptest.NewRecorder(), req) + + if got == "" { + t.Error("GetClientIP must not be empty after clientIPMiddleware runs") + } +} + +func TestParseTrustedProxyCIDRs(t *testing.T) { + t.Run("empty input yields loopback default", func(t *testing.T) { + got, err := ParseTrustedProxyCIDRs(nil) + if err != nil { + t.Fatalf("ParseTrustedProxyCIDRs: %v", err) + } + + if len(got) != 2 { + t.Fatalf("default CIDR count = %d, want 2 (127/8 + ::1/128)", len(got)) + } + + // Should contain 127.0.0.1 and ::1. + if !isFromTrustedPeer("127.0.0.1:1", got) { + t.Error("default CIDRs should include 127.0.0.1") + } + + if !isFromTrustedPeer("[::1]:1", got) { + t.Error("default CIDRs should include ::1") + } + }) + + t.Run("custom CIDRs override defaults", func(t *testing.T) { + got, err := ParseTrustedProxyCIDRs([]string{"10.0.0.0/8"}) + if err != nil { + t.Fatalf("ParseTrustedProxyCIDRs: %v", err) + } + + if len(got) != 1 { + t.Errorf("custom CIDR count = %d, want 1", len(got)) + } + + if !isFromTrustedPeer("10.1.2.3:1", got) { + t.Error("10.1.2.3 should be in 10.0.0.0/8") + } + + if isFromTrustedPeer("127.0.0.1:1", got) { + t.Error("127.0.0.1 should NOT match when default is overridden") + } + }) + + t.Run("invalid CIDR returns error", func(t *testing.T) { + _, err := ParseTrustedProxyCIDRs([]string{"not-a-cidr"}) + if err == nil { + t.Fatal("expected error on invalid CIDR") + } + }) +} diff --git a/pkg/service/handlers/middleware_peer_observer.go b/pkg/service/handlers/middleware_peer_observer.go index 14f9083..93014f9 100644 --- a/pkg/service/handlers/middleware_peer_observer.go +++ b/pkg/service/handlers/middleware_peer_observer.go @@ -12,8 +12,8 @@ import ( // the handler runs, so passive reachability probes can register a device // IP and learn whether any inbound landed in their wait window. // -// Placement: after TrustedRealIPMiddleware (so r.RemoteAddr reflects the -// trusted client IP) and after Recoverer (so any panic inside this +// Placement: after ClientIPMiddleware (so the resolved client IP is available +// via middleware.GetClientIP) and after Recoverer (so any panic inside this // middleware is contained). Before any short-circuiting middleware // would be unnecessary — Signal runs before next.ServeHTTP, so the // observation lands regardless of how later middleware handles the diff --git a/pkg/service/handlers/middleware_realip.go b/pkg/service/handlers/middleware_realip.go deleted file mode 100644 index 32244cd..0000000 --- a/pkg/service/handlers/middleware_realip.go +++ /dev/null @@ -1,95 +0,0 @@ -package handlers - -import ( - "fmt" - "net" - "net/http" - - "github.com/go-chi/chi/v5/middleware" -) - -// defaultTrustedProxyCIDRs is the safe-by-default list applied when -// Settings.TrustedProxyCIDRs is empty. Only loopback addresses are trusted — -// i.e. a reverse proxy on the same host. Anyone deploying behind a proxy on a -// different host must override this in settings.json. -var defaultTrustedProxyCIDRs = []string{ - "127.0.0.0/8", - "::1/128", -} - -// TrustedRealIP returns a middleware that delegates to chi's RealIP — which -// rewrites r.RemoteAddr from True-Client-IP / X-Real-IP / X-Forwarded-For -// headers — but only when the immediate TCP peer is in `trustedPeers`. For -// any request whose peer is *not* trusted (i.e. anything other than the -// configured reverse proxy), the headers are ignored and r.RemoteAddr stays -// as-is. -// -// This avoids the standard X-Forwarded-* spoofing pitfall: on a flat LAN -// where a malicious speaker could send the headers itself, we won't honour -// them; behind a reverse proxy we will. -// -// Returns nil if trustedPeers is empty — caller should not Use a nil mw. -func TrustedRealIP(trustedPeers []*net.IPNet) func(http.Handler) http.Handler { - if len(trustedPeers) == 0 { - return nil - } - - delegate := middleware.RealIP - - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if isFromTrustedPeer(r.RemoteAddr, trustedPeers) { - delegate(next).ServeHTTP(w, r) - - return - } - - next.ServeHTTP(w, r) - }) - } -} - -// isFromTrustedPeer reports whether remoteAddr (in the host:port shape that -// net/http populates) is contained in any of the supplied CIDR blocks. -func isFromTrustedPeer(remoteAddr string, trustedPeers []*net.IPNet) bool { - host, _, err := net.SplitHostPort(remoteAddr) - if err != nil { - host = remoteAddr - } - - ip := net.ParseIP(host) - if ip == nil { - return false - } - - for _, n := range trustedPeers { - if n.Contains(ip) { - return true - } - } - - return false -} - -// ParseTrustedProxyCIDRs converts string CIDRs into *net.IPNet values, falling -// back to defaultTrustedProxyCIDRs when the input is empty. An invalid CIDR -// in the input list is reported as an error and stops parsing — better to -// fail loud than silently fall back. -func ParseTrustedProxyCIDRs(cidrs []string) ([]*net.IPNet, error) { - if len(cidrs) == 0 { - cidrs = defaultTrustedProxyCIDRs - } - - out := make([]*net.IPNet, 0, len(cidrs)) - - for _, c := range cidrs { - _, n, err := net.ParseCIDR(c) - if err != nil { - return nil, fmt.Errorf("invalid trusted proxy CIDR %q: %w", c, err) - } - - out = append(out, n) - } - - return out, nil -} diff --git a/pkg/service/handlers/middleware_realip_test.go b/pkg/service/handlers/middleware_realip_test.go deleted file mode 100644 index 41f206b..0000000 --- a/pkg/service/handlers/middleware_realip_test.go +++ /dev/null @@ -1,155 +0,0 @@ -package handlers - -import ( - "net" - "net/http" - "net/http/httptest" - "testing" -) - -func TestTrustedRealIP(t *testing.T) { - cidrs, err := ParseTrustedProxyCIDRs([]string{"127.0.0.0/8", "::1/128"}) - if err != nil { - t.Fatalf("ParseTrustedProxyCIDRs: %v", err) - } - - mw := TrustedRealIP(cidrs) - if mw == nil { - t.Fatal("TrustedRealIP returned nil for non-empty trustedPeers") - } - - cases := []struct { - name string - remoteAddr string - xRealIP string - xForwardedFor string - wantRemoteAddr string - }{ - { - name: "trusted peer with X-Real-IP is honoured", - remoteAddr: "127.0.0.1:54321", - xRealIP: "192.0.2.10", - wantRemoteAddr: "192.0.2.10", - }, - { - name: "trusted peer with X-Forwarded-For is honoured", - remoteAddr: "127.0.0.1:54321", - xForwardedFor: "192.0.2.20, 10.0.0.1", - wantRemoteAddr: "192.0.2.20", - }, - { - name: "trusted peer with no headers leaves RemoteAddr alone", - remoteAddr: "127.0.0.1:54321", - wantRemoteAddr: "127.0.0.1:54321", - }, - { - name: "untrusted peer's X-Real-IP is ignored", - remoteAddr: "192.0.2.99:54321", - xRealIP: "1.2.3.4", - wantRemoteAddr: "192.0.2.99:54321", - }, - { - name: "untrusted peer's X-Forwarded-For is ignored", - remoteAddr: "192.0.2.99:54321", - xForwardedFor: "1.2.3.4", - wantRemoteAddr: "192.0.2.99:54321", - }, - { - name: "trusted peer with garbage X-Real-IP leaves RemoteAddr alone", - remoteAddr: "127.0.0.1:54321", - xRealIP: "not-an-ip", - wantRemoteAddr: "127.0.0.1:54321", - }, - { - name: "trusted IPv6 loopback peer is honoured", - remoteAddr: "[::1]:54321", - xRealIP: "fe80::1", - wantRemoteAddr: "fe80::1", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - var got string - - h := mw(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - got = r.RemoteAddr - })) - - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = tc.remoteAddr - - if tc.xRealIP != "" { - req.Header.Set("X-Real-IP", tc.xRealIP) - } - - if tc.xForwardedFor != "" { - req.Header.Set("X-Forwarded-For", tc.xForwardedFor) - } - - h.ServeHTTP(httptest.NewRecorder(), req) - - if got != tc.wantRemoteAddr { - t.Errorf("RemoteAddr = %q, want %q", got, tc.wantRemoteAddr) - } - }) - } -} - -func TestTrustedRealIP_NilForEmptyPeers(t *testing.T) { - if mw := TrustedRealIP(nil); mw != nil { - t.Error("TrustedRealIP(nil) returned non-nil; expected nil so caller can skip Use()") - } - - if mw := TrustedRealIP([]*net.IPNet{}); mw != nil { - t.Error("TrustedRealIP([]) returned non-nil; expected nil so caller can skip Use()") - } -} - -func TestParseTrustedProxyCIDRs(t *testing.T) { - t.Run("empty input yields loopback default", func(t *testing.T) { - got, err := ParseTrustedProxyCIDRs(nil) - if err != nil { - t.Fatalf("ParseTrustedProxyCIDRs: %v", err) - } - - if len(got) != 2 { - t.Fatalf("default CIDR count = %d, want 2 (127/8 + ::1/128)", len(got)) - } - - // Should contain 127.0.0.1 and ::1. - if !isFromTrustedPeer("127.0.0.1:1", got) { - t.Error("default CIDRs should include 127.0.0.1") - } - - if !isFromTrustedPeer("[::1]:1", got) { - t.Error("default CIDRs should include ::1") - } - }) - - t.Run("custom CIDRs override defaults", func(t *testing.T) { - got, err := ParseTrustedProxyCIDRs([]string{"10.0.0.0/8"}) - if err != nil { - t.Fatalf("ParseTrustedProxyCIDRs: %v", err) - } - - if len(got) != 1 { - t.Errorf("custom CIDR count = %d, want 1", len(got)) - } - - if !isFromTrustedPeer("10.1.2.3:1", got) { - t.Error("10.1.2.3 should be in 10.0.0.0/8") - } - - if isFromTrustedPeer("127.0.0.1:1", got) { - t.Error("127.0.0.1 should NOT match when default is overridden") - } - }) - - t.Run("invalid CIDR returns error", func(t *testing.T) { - _, err := ParseTrustedProxyCIDRs([]string{"not-a-cidr"}) - if err == nil { - t.Fatal("expected error on invalid CIDR") - } - }) -} diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 5b48841..7361c66 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -485,33 +485,26 @@ func (s *Server) loadOwnCACert() *x509.Certificate { return s.ownCACache.cert } -// TrustedRealIPMiddleware returns a chi middleware that rewrites -// r.RemoteAddr from X-Real-IP / X-Forwarded-For / True-Client-IP, but only -// when the immediate TCP peer is in the configured trusted-proxy list. -// Returns nil when Settings.TrustForwardedHeaders is false (the safe -// default), so the caller can skip wiring the middleware entirely. +// ClientIPMiddleware returns a chi middleware that resolves the client IP into +// the request context (read via middleware.GetClientIP). Always returns a +// non-nil middleware: at minimum, the socket peer is recorded. +// +// When Settings.TrustForwardedHeaders is true and the immediate TCP peer is in +// the configured trusted-proxy list, the X-Forwarded-For header is also +// consulted: chi walks the chain right-to-left, skipping entries that fall +// within the trusted CIDRs, and stores the first untrusted entry as the client. // // The trusted-peer gate prevents the typical X-Forwarded-* spoofing surface: // on a flat LAN where a malicious speaker could send the headers itself, we // won't honour them; behind a documented reverse proxy on loopback we will. -func (s *Server) TrustedRealIPMiddleware() func(http.Handler) http.Handler { +func (s *Server) ClientIPMiddleware() func(http.Handler) http.Handler { settings, err := s.ds.GetSettings() if err != nil { - log.Printf("[RealIP] failed to load settings: %v — skipping forwarded-header trust", err) - return nil + log.Printf("[ClientIP] failed to load settings: %v - falling back to peer-only", err) + return clientIPMiddleware(false, nil, nil) } - if !settings.TrustForwardedHeaders { - return nil - } - - cidrs, err := ParseTrustedProxyCIDRs(settings.TrustedProxyCIDRs) - if err != nil { - log.Printf("[RealIP] invalid trusted_proxy_cidrs: %v — skipping forwarded-header trust", err) - return nil - } - - return TrustedRealIP(cidrs) + return buildClientIPMiddleware(settings.TrustForwardedHeaders, settings.TrustedProxyCIDRs) } // SetVersionInfo sets the version information for the server.