diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 0af86ed..9d19eb1 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -852,6 +852,16 @@ func startDeviceDiscovery(server *handlers.Server) { func setupRouter(server *handlers.Server) *chi.Mux { r := chi.NewRouter() + + // TrustedRealIP must run before any handler that reads r.RemoteAddr — + // 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) + } + r.Use(server.SnapshotMiddleware) r.Use(server.OriginMiddleware) r.Use(middleware.Recoverer) diff --git a/docs/guides/HTTPS-SETUP.md b/docs/guides/HTTPS-SETUP.md index f1957de..b67c6d5 100644 --- a/docs/guides/HTTPS-SETUP.md +++ b/docs/guides/HTTPS-SETUP.md @@ -70,6 +70,21 @@ server { } ``` +> **Tell the service to honour `X-Real-IP`/`X-Forwarded-For`.** When deploying +> behind a reverse proxy on the same host as above, set +> `"trust_forwarded_headers": true` in `data/settings.json`. With that flag +> on, the service rewrites `r.RemoteAddr` from the proxy-supplied headers, +> so handlers that act on the source IP (e.g. the Spotify priming triggered +> by `/marge/streaming/support/power_on`) see the speaker's real address +> instead of the proxy's loopback peer. +> +> By default only `127.0.0.0/8` and `::1/128` are trusted to set those +> headers. If your reverse proxy lives on a different host, list its CIDR(s) +> in `"trusted_proxy_cidrs"` (e.g. `["10.0.0.0/8"]`). Do **not** enable +> `trust_forwarded_headers` on a flat LAN deployment without a proxy: a +> malicious speaker on the LAN can send the headers itself and spoof its +> source IP. + --- ## Manual CA injection (advanced) diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index 971dfc6..15b1af7 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -1806,6 +1806,22 @@ type Settings struct { // keeps verification on; opt in only when the upstream certificate chain is // broken (post end-of-service) and a temporary unblock is required. AllowInsecureUpstreamTLS bool `json:"allow_insecure_upstream_tls,omitempty"` + + // TrustForwardedHeaders enables proxy-aware client IP resolution: when the + // immediate TCP peer is one of the TrustedProxyCIDRs, the X-Real-IP / + // X-Forwarded-For / True-Client-IP headers are honoured and replace + // r.RemoteAddr. Required when the service is fronted by nginx, Caddy, or + // any other reverse proxy. Default false — direct LAN deployments must + // not enable this, otherwise a malicious LAN-resident client could spoof + // its source IP via these headers. + TrustForwardedHeaders bool `json:"trust_forwarded_headers,omitempty"` + + // TrustedProxyCIDRs is the list of CIDR blocks whose immediate TCP peers + // are allowed to set X-Forwarded-* headers when TrustForwardedHeaders is + // true. Defaults to loopback (127.0.0.0/8 and ::1/128) — i.e. only a + // reverse proxy on the same host. Override only if the proxy lives on a + // different host within a known-good private subnet. + TrustedProxyCIDRs []string `json:"trusted_proxy_cidrs,omitempty"` } // GetSettings retrieves the global service settings. diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index e5f4a94..c7162ee 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -289,13 +289,32 @@ func (s *Server) HandleMargePowerOn(w http.ResponseWriter, r *http.Request) { } } - if deviceIP != "" { - go s.PrimeDeviceWithSpotify(deviceIP) - } else { - // Fallback to remote address if IP is missing from XML - if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { - go s.PrimeDeviceWithSpotify(host) - } + // Prefer the TCP source address over the body's self-reported IP for + // any outbound credential push. The body field is attacker-controllable + // (a malicious LAN-resident speaker can set it to any value), while + // r.RemoteAddr is the actual peer — and if the service runs behind a + // trusted reverse proxy, the TrustedRealIP middleware has already + // rewritten it from X-Real-IP / X-Forwarded-For. We log when the two + // disagree so the discrepancy is investigable but never trust the body. + remoteHost := "" + if h, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + remoteHost = h + } + + if deviceIP != "" && remoteHost != "" && deviceIP != remoteHost { + log.Printf("[Marge] power_on body IP %q differs from TCP source %q for device %s — using TCP source for credential push", + deviceIP, remoteHost, deviceID) + } + + target := remoteHost + if target == "" { + // RemoteAddr was unparseable (shouldn't happen under net/http) — + // fall back to the body so we don't silently skip the push. + target = deviceIP + } + + if target != "" { + go s.PrimeDeviceWithSpotify(target) } w.WriteHeader(http.StatusOK) diff --git a/pkg/service/handlers/middleware_realip.go b/pkg/service/handlers/middleware_realip.go new file mode 100644 index 0000000..32244cd --- /dev/null +++ b/pkg/service/handlers/middleware_realip.go @@ -0,0 +1,95 @@ +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 new file mode 100644 index 0000000..b322094 --- /dev/null +++ b/pkg/service/handlers/middleware_realip_test.go @@ -0,0 +1,155 @@ +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.168.1.10", + wantRemoteAddr: "192.168.1.10", + }, + { + name: "trusted peer with X-Forwarded-For is honoured", + remoteAddr: "127.0.0.1:54321", + xForwardedFor: "192.168.1.20, 10.0.0.1", + wantRemoteAddr: "192.168.1.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.168.1.99:54321", + xRealIP: "1.2.3.4", + wantRemoteAddr: "192.168.1.99:54321", + }, + { + name: "untrusted peer's X-Forwarded-For is ignored", + remoteAddr: "192.168.1.99:54321", + xForwardedFor: "1.2.3.4", + wantRemoteAddr: "192.168.1.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 e475ec1..817a6f3 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -100,6 +100,35 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, pro return s } +// 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. +// +// 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 { + settings, err := s.ds.GetSettings() + if err != nil { + log.Printf("[RealIP] failed to load settings: %v — skipping forwarded-header trust", err) + return 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) +} + // SetVersionInfo sets the version information for the server. func (s *Server) SetVersionInfo(version, commit, date, repoURL string) { s.mu.Lock()