Use the RealIP middleware also behind a reverse proxy (#2858)

* Use the RealIP middleware only behind a reverse proxy

* Fix proxy ip source in tests

* Fix test for PR#2087

The PR did not update the test after changing the behavior, but the test still
passed because another condition was preventing the user from being created in
the test.

* Use RealIP even without a trusted reverse proxy

* Use own type for context key

* Fix casing to follow go's conventions

* Do not apply RealIP middleware twice

* Fix IP source in logs

The most interesting data point in the log message is the proxy's IP, but
having the client IP too can help identify integration issues.

model/request/request.go

@@ -16,6 +16,7 @@ const (
 	Player         = contextKey("player")
 	Transcoding    = contextKey("transcoding")
 	ClientUniqueId = contextKey("clientUniqueId")
+	ReverseProxyIp = contextKey("reverseProxyIp")
 )
 
 func WithUser(ctx context.Context, u model.User) context.Context {
@@ -46,6 +47,10 @@ func WithClientUniqueId(ctx context.Context, clientUniqueId string) context.Cont
 	return context.WithValue(ctx, ClientUniqueId, clientUniqueId)
 }
 
+func WithReverseProxyIp(ctx context.Context, reverseProxyIp string) context.Context {
+	return context.WithValue(ctx, ReverseProxyIp, reverseProxyIp)
+}
+
 func UserFrom(ctx context.Context) (model.User, bool) {
 	v, ok := ctx.Value(User).(model.User)
 	return v, ok
@@ -80,3 +85,8 @@ func ClientUniqueIdFrom(ctx context.Context) (string, bool) {
 	v, ok := ctx.Value(ClientUniqueId).(string)
 	return v, ok
 }
+
+func ReverseProxyIpFrom(ctx context.Context) (string, bool) {
+	v, ok := ctx.Value(ReverseProxyIp).(string)
+	return v, ok
+}

server/auth.go

@@ -199,8 +199,13 @@ func UsernameFromReverseProxyHeader(r *http.Request) string {
 	if conf.Server.ReverseProxyWhitelist == "" && !strings.HasPrefix(conf.Server.Address, "unix:") {
 		return ""
 	}
-	if !validateIPAgainstList(r.RemoteAddr, conf.Server.ReverseProxyWhitelist) {
-		log.Warn(r.Context(), "IP is not whitelisted for reverse proxy login", "ip", r.RemoteAddr)
+	reverseProxyIp, ok := request.ReverseProxyIpFrom(r.Context())
+	if !ok {
+		log.Error("ReverseProxyWhitelist enabled but no proxy IP found in request context. Please report this error.")
+		return ""
+	}
+	if !validateIPAgainstList(reverseProxyIp, conf.Server.ReverseProxyWhitelist) {
+		log.Warn(r.Context(), "IP is not whitelisted for reverse proxy login", "proxy-ip", reverseProxyIp, "client-ip", r.RemoteAddr)
 		return ""
 	}
 	username := r.Header.Get(conf.Server.ReverseProxyUserHeader)

server/auth_test.go

@@ -11,6 +11,10 @@ import (
 	"strings"
 	"time"
 
+	"github.com/navidrome/navidrome/model/request"
+
+	"github.com/google/uuid"
+
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/core/auth"
@@ -62,6 +66,13 @@ var _ = Describe("Auth", func() {
 		})
 
 		Describe("Login from HTTP headers", func() {
+			const (
+				trustedIpv4   = "192.168.0.42"
+				untrustedIpv4 = "8.8.8.8"
+				trustedIpv6   = "2001:4860:4860:1234:5678:0000:4242:8888"
+				untrustedIpv6 = "5005:0:3003"
+			)
+
 			fs := os.DirFS("tests/fixtures")
 
 			BeforeEach(func() {
@@ -75,7 +86,7 @@ var _ = Describe("Auth", func() {
 			})
 
 			It("sets auth data if IPv4 matches whitelist", func() {
-				req.RemoteAddr = "192.168.0.42:25293"
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), trustedIpv4))
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())
@@ -85,7 +96,7 @@ var _ = Describe("Auth", func() {
 			})
 
 			It("sets no auth data if IPv4 does not match whitelist", func() {
-				req.RemoteAddr = "8.8.8.8:25293"
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), untrustedIpv4))
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())
@@ -93,7 +104,7 @@ var _ = Describe("Auth", func() {
 			})
 
 			It("sets auth data if IPv6 matches whitelist", func() {
-				req.RemoteAddr = "[2001:4860:4860:1234:5678:0000:4242:8888]:25293"
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), trustedIpv6))
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())
@@ -103,23 +114,28 @@ var _ = Describe("Auth", func() {
 			})
 
 			It("sets no auth data if IPv6 does not match whitelist", func() {
-				req.RemoteAddr = "[5005:0:3003]:25293"
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), untrustedIpv6))
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())
 				Expect(config["auth"]).To(BeNil())
 			})
 
-			It("sets no auth data if user does not exist", func() {
-				req.Header.Set("Remote-User", "INVALID_USER")
+			It("creates user and sets auth data if user does not exist", func() {
+				newUser := "NEW_USER_" + uuid.NewString()
+
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), trustedIpv4))
+				req.Header.Set("Remote-User", newUser)
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())
-				Expect(config["auth"]).To(BeNil())
+				parsed := config["auth"].(map[string]interface{})
+
+				Expect(parsed["username"]).To(Equal(newUser))
 			})
 
 			It("sets auth data if user exists", func() {
-				req.RemoteAddr = "192.168.0.42:25293"
+				req = req.WithContext(request.WithReverseProxyIp(req.Context(), trustedIpv4))
 				serveIndex(ds, fs, nil)(resp, req)
 
 				config := extractAppConfig(resp.Body.String())

server/middlewares.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io/fs"
@@ -158,13 +159,35 @@ func clientUniqueIDMiddleware(next http.Handler) http.Handler {
 	})
 }
 
-// realIPMiddleware wraps the middleware.RealIP middleware function, bypassing it if the
-// ReverseProxyWhitelist configuration option is set.
-func realIPMiddleware(h http.Handler) http.Handler {
-	if conf.Server.ReverseProxyWhitelist == "" {
-		return middleware.RealIP(h)
+// realIPMiddleware applies middleware.RealIP, and additionally saves the request's original RemoteAddr to the request's
+// context if navidrome is behind a trusted reverse proxy.
+func realIPMiddleware(next http.Handler) http.Handler {
+	if conf.Server.ReverseProxyWhitelist != "" {
+		return chi.Chain(
+			reqToCtx(request.ReverseProxyIp, func(r *http.Request) any { return r.RemoteAddr }),
+			middleware.RealIP,
+		).Handler(next)
+	}
+
+	// The middleware is applied without a trusted reverse proxy to support other use-cases such as multiple clients
+	// behind a caching proxy. In this case, navidrome only uses the request's RemoteAddr for logging, so the security
+	// impact of reading the headers from untrusted sources is limited.
+	return middleware.RealIP(next)
+}
+
+// reqToCtx creates a middleware that updates the request's context with a value computed from the request. A given key
+// can only be set once.
+func reqToCtx(key any, fn func(req *http.Request) any) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.Context().Value(key) == nil {
+				ctx := context.WithValue(r.Context(), key, fn(r))
+				r = r.WithContext(ctx)
+			}
+
+			next.ServeHTTP(w, r)
+		})
 	}
-	return h
 }
 
 // serverAddressMiddleware is a middleware function that modifies the request object

server/subsonic/middlewares.go

@@ -148,7 +148,7 @@ func getPlayer(players core.Players) func(next http.Handler) http.Handler {
 			userName, _ := request.UsernameFrom(ctx)
 			client, _ := request.ClientFrom(ctx)
 			playerId := playerIDFromCookie(r, userName)
-			ip, _, _ := net.SplitHostPort(realIP(r))
+			ip, _, _ := net.SplitHostPort(r.RemoteAddr)
 			userAgent := canonicalUserAgent(r)
 			player, trc, err := players.Register(ctx, playerId, client, userAgent, ip)
 			if err != nil {
@@ -185,19 +185,6 @@ func canonicalUserAgent(r *http.Request) string {
 	return userAgent
 }
 
-func realIP(r *http.Request) string {
-	if xrip := r.Header.Get("X-Real-IP"); xrip != "" {
-		return xrip
-	} else if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		i := strings.Index(xff, ", ")
-		if i == -1 {
-			i = len(xff)
-		}
-		return xff[:i]
-	}
-	return r.RemoteAddr
-}
-
 func playerIDFromCookie(r *http.Request, userName string) string {
 	cookieName := playerIDCookieName(userName)
 	var playerId string