From 18143fa5a16e44f02aff4202118e2d54dc384222 Mon Sep 17 00:00:00 2001 From: crazygolem Date: Fri, 26 Apr 2024 02:43:58 +0200 Subject: [PATCH] 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 | 10 ++++++++++ server/auth.go | 9 +++++++-- server/auth_test.go | 32 +++++++++++++++++++++++-------- server/middlewares.go | 35 ++++++++++++++++++++++++++++------ server/subsonic/middlewares.go | 15 +-------------- 5 files changed, 71 insertions(+), 30 deletions(-) diff --git a/model/request/request.go b/model/request/request.go index eead56d54..c62a2f3eb 100644 --- a/model/request/request.go +++ b/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 +} diff --git a/server/auth.go b/server/auth.go index 2d7c6b68b..784c17645 100644 --- a/server/auth.go +++ b/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) diff --git a/server/auth_test.go b/server/auth_test.go index 511535855..6b2686541 100644 --- a/server/auth_test.go +++ b/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()) diff --git a/server/middlewares.go b/server/middlewares.go index 858cc48ca..7bbfbd0d6 100644 --- a/server/middlewares.go +++ b/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 diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index a5f368328..1efaf6157 100644 --- a/server/subsonic/middlewares.go +++ b/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