fix(security): use trusted-proxy-aware IP extraction in logging and remove CORS from proxy
- commit
- 372a0c55bdaa4b1584df2b5f0bebc7e3fbcd9356
- author
- bnema · · 2026-02-01 14:32 UTC
- parent
- 4d0f66f
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +26 -38
M internal/adapters/in/http/middleware/logging.go +14 -30
| @@ -3,6 +3,7 @@ package middleware | |||
| 3 | 3 | ||
| 4 | 4 | import ( | |
| 5 | 5 | "encoding/json" | |
| 6 | + | "net" | |
| 6 | 7 | "net/http" | |
| 7 | 8 | "time" | |
| 8 | 9 | ||
| @@ -51,7 +52,13 @@ func (rw *ResponseWriter) BytesWritten() int { | |||
| 51 | 52 | ||
| 52 | 53 | // RequestLogger is a middleware that logs HTTP requests using zerowrap. | |
| 53 | 54 | // It also attaches the logger to the request context for downstream handlers. | |
| 54 | - | func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | |
| 55 | + | // The trustedNets parameter controls which proxy headers are trusted for IP extraction. | |
| 56 | + | // Pass nil to only use RemoteAddr (safest default when not behind a trusted proxy). | |
| 57 | + | func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.Handler) http.Handler { | |
| 58 | + | var nets []*net.IPNet | |
| 59 | + | if len(trustedNets) > 0 { | |
| 60 | + | nets = trustedNets[0] | |
| 61 | + | } | |
| 55 | 62 | return func(next http.Handler) http.Handler { | |
| 56 | 63 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 57 | 64 | start := time.Now() | |
| @@ -59,8 +66,9 @@ func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | |||
| 59 | 66 | // Wrap the response writer to capture status and bytes | |
| 60 | 67 | rw := NewResponseWriter(w) | |
| 61 | 68 | ||
| 62 | - | // Get client IP (handle X-Forwarded-For and X-Real-IP headers) | |
| 63 | - | clientIP := getClientIP(r) | |
| 69 | + | // SECURITY: Use trusted-proxy-aware IP extraction to prevent | |
| 70 | + | // IP spoofing via X-Forwarded-For from untrusted sources. | |
| 71 | + | clientIP := GetClientIP(r, nets) | |
| 64 | 72 | ||
| 65 | 73 | // Attach the logger to the request context for downstream handlers | |
| 66 | 74 | ctx := zerowrap.WithCtx(r.Context(), log) | |
| @@ -92,32 +100,6 @@ func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | |||
| 92 | 100 | } | |
| 93 | 101 | } | |
| 94 | 102 | ||
| 95 | - | // getClientIP extracts the client IP from various headers. | |
| 96 | - | func getClientIP(r *http.Request) string { | |
| 97 | - | // Check X-Forwarded-For header (comma-separated list, first is original client) | |
| 98 | - | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | |
| 99 | - | // Take the first IP (original client) | |
| 100 | - | for idx := 0; idx < len(xff); idx++ { | |
| 101 | - | if xff[idx] == ',' { | |
| 102 | - | return xff[:idx] | |
| 103 | - | } | |
| 104 | - | } | |
| 105 | - | return xff | |
| 106 | - | } | |
| 107 | - | ||
| 108 | - | // Check X-Real-IP header | |
| 109 | - | if xri := r.Header.Get("X-Real-IP"); xri != "" { | |
| 110 | - | return xri | |
| 111 | - | } | |
| 112 | - | ||
| 113 | - | // Check X-Forwarded header (less common) | |
| 114 | - | if xf := r.Header.Get("X-Forwarded"); xf != "" { | |
| 115 | - | return xf | |
| 116 | - | } | |
| 117 | - | ||
| 118 | - | // Fallback to RemoteAddr | |
| 119 | - | return r.RemoteAddr | |
| 120 | - | } | |
| 121 | 103 | ||
| 122 | 104 | // PanicRecovery middleware recovers from panics and logs them. | |
| 123 | 105 | func PanicRecovery(log zerowrap.Logger) func(http.Handler) http.Handler { | |
| @@ -145,7 +127,9 @@ func PanicRecovery(log zerowrap.Logger) func(http.Handler) http.Handler { | |||
| 145 | 127 | } | |
| 146 | 128 | } | |
| 147 | 129 | ||
| 148 | - | // CORS middleware adds CORS headers. | |
| 130 | + | // CORS middleware adds permissive CORS headers. | |
| 131 | + | // NOTE: This is intentionally NOT used on the proxy chain (backends control their own CORS). | |
| 132 | + | // Available for use on specific endpoints (e.g., registry API) where CORS is needed. | |
| 149 | 133 | func CORS(next http.Handler) http.Handler { | |
| 150 | 134 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 151 | 135 | w.Header().Set("Access-Control-Allow-Origin", "*") | |
| @@ -3,6 +3,7 @@ package middleware | |||||
| 3 | 3 | ||||
| 4 | import ( | 4 | import ( | ||
| 5 | "encoding/json" | 5 | "encoding/json" | ||
| 6 | + | "net" | |||
| 6 | "net/http" | 7 | "net/http" | ||
| 7 | "time" | 8 | "time" | ||
| 8 | 9 | ||||
| @@ -51,7 +52,13 @@ func (rw *ResponseWriter) BytesWritten() int { | |||||
| 51 | 52 | ||||
| 52 | // RequestLogger is a middleware that logs HTTP requests using zerowrap. | 53 | // RequestLogger is a middleware that logs HTTP requests using zerowrap. | ||
| 53 | // It also attaches the logger to the request context for downstream handlers. | 54 | // It also attaches the logger to the request context for downstream handlers. | ||
| 54 | - | func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | 55 | + | // The trustedNets parameter controls which proxy headers are trusted for IP extraction. |
| 56 | + | // Pass nil to only use RemoteAddr (safest default when not behind a trusted proxy). | |||
| 57 | + | func RequestLogger(log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.Handler) http.Handler { | |||
| 58 | + | var nets []*net.IPNet | |||
| 59 | + | if len(trustedNets) > 0 { | |||
| 60 | + | nets = trustedNets[0] | |||
| 61 | + | } | |||
| 55 | return func(next http.Handler) http.Handler { | 62 | return func(next http.Handler) http.Handler { | ||
| 56 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 63 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 57 | start := time.Now() | 64 | start := time.Now() | ||
| @@ -59,8 +66,9 @@ func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | |||||
| 59 | // Wrap the response writer to capture status and bytes | 66 | // Wrap the response writer to capture status and bytes | ||
| 60 | rw := NewResponseWriter(w) | 67 | rw := NewResponseWriter(w) | ||
| 61 | 68 | ||||
| 62 | - | // Get client IP (handle X-Forwarded-For and X-Real-IP headers) | 69 | + | // SECURITY: Use trusted-proxy-aware IP extraction to prevent |
| 63 | - | clientIP := getClientIP(r) | 70 | + | // IP spoofing via X-Forwarded-For from untrusted sources. |
| 71 | + | clientIP := GetClientIP(r, nets) | |||
| 64 | 72 | ||||
| 65 | // Attach the logger to the request context for downstream handlers | 73 | // Attach the logger to the request context for downstream handlers | ||
| 66 | ctx := zerowrap.WithCtx(r.Context(), log) | 74 | ctx := zerowrap.WithCtx(r.Context(), log) | ||
| @@ -92,32 +100,6 @@ func RequestLogger(log zerowrap.Logger) func(http.Handler) http.Handler { | |||||
| 92 | } | 100 | } | ||
| 93 | } | 101 | } | ||
| 94 | 102 | ||||
| 95 | - | // getClientIP extracts the client IP from various headers. | |||
| 96 | - | func getClientIP(r *http.Request) string { | |||
| 97 | - | // Check X-Forwarded-For header (comma-separated list, first is original client) | |||
| 98 | - | if xff := r.Header.Get("X-Forwarded-For"); xff != "" { | |||
| 99 | - | // Take the first IP (original client) | |||
| 100 | - | for idx := 0; idx < len(xff); idx++ { | |||
| 101 | - | if xff[idx] == ',' { | |||
| 102 | - | return xff[:idx] | |||
| 103 | - | } | |||
| 104 | - | } | |||
| 105 | - | return xff | |||
| 106 | - | } | |||
| 107 | - | ||||
| 108 | - | // Check X-Real-IP header | |||
| 109 | - | if xri := r.Header.Get("X-Real-IP"); xri != "" { | |||
| 110 | - | return xri | |||
| 111 | - | } | |||
| 112 | - | ||||
| 113 | - | // Check X-Forwarded header (less common) | |||
| 114 | - | if xf := r.Header.Get("X-Forwarded"); xf != "" { | |||
| 115 | - | return xf | |||
| 116 | - | } | |||
| 117 | - | ||||
| 118 | - | // Fallback to RemoteAddr | |||
| 119 | - | return r.RemoteAddr | |||
| 120 | - | } | |||
| 121 | 103 | ||||
| 122 | // PanicRecovery middleware recovers from panics and logs them. | 104 | // PanicRecovery middleware recovers from panics and logs them. | ||
| 123 | func PanicRecovery(log zerowrap.Logger) func(http.Handler) http.Handler { | 105 | func PanicRecovery(log zerowrap.Logger) func(http.Handler) http.Handler { | ||
| @@ -145,7 +127,9 @@ func PanicRecovery(log zerowrap.Logger) func(http.Handler) http.Handler { | |||||
| 145 | } | 127 | } | ||
| 146 | } | 128 | } | ||
| 147 | 129 | ||||
| 148 | - | // CORS middleware adds CORS headers. | 130 | + | // CORS middleware adds permissive CORS headers. |
| 131 | + | // NOTE: This is intentionally NOT used on the proxy chain (backends control their own CORS). | |||
| 132 | + | // Available for use on specific endpoints (e.g., registry API) where CORS is needed. | |||
| 149 | func CORS(next http.Handler) http.Handler { | 133 | func CORS(next http.Handler) http.Handler { | ||
| 150 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 134 | return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 151 | w.Header().Set("Access-Control-Allow-Origin", "*") | 135 | w.Header().Set("Access-Control-Allow-Origin", "*") | ||
M internal/app/run.go +12 -8
| @@ -7,7 +7,6 @@ import ( | |||
| 7 | 7 | "encoding/hex" | |
| 8 | 8 | "encoding/json" | |
| 9 | 9 | "fmt" | |
| 10 | - | "net" | |
| 11 | 10 | "net/http" | |
| 12 | 11 | "os" | |
| 13 | 12 | "os/signal" | |
| @@ -948,11 +947,15 @@ func syncAndAutoStart(ctx context.Context, svc *services, log zerowrap.Logger) { | |||
| 948 | 947 | ||
| 949 | 948 | // createHTTPHandlers creates HTTP handlers with middleware. | |
| 950 | 949 | func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Handler, http.Handler) { | |
| 950 | + | // Parse trusted proxies once for all middleware chains. | |
| 951 | + | // This ensures consistent IP extraction across logging, rate limiting, and auth. | |
| 952 | + | trustedNets := middleware.ParseTrustedProxies(cfg.API.RateLimit.TrustedProxies) | |
| 953 | + | ||
| 951 | 954 | registryHandler := registry.NewHandler(svc.registrySvc, log) | |
| 952 | 955 | ||
| 953 | 956 | registryMiddlewares := []func(http.Handler) http.Handler{ | |
| 954 | 957 | middleware.PanicRecovery(log), | |
| 955 | - | middleware.RequestLogger(log), | |
| 958 | + | middleware.RequestLogger(log, trustedNets), | |
| 956 | 959 | middleware.SecurityHeaders, | |
| 957 | 960 | } | |
| 958 | 961 | ||
| @@ -993,7 +996,7 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 993 | 996 | // but still need rate limiting to prevent brute force attacks | |
| 994 | 997 | authWithMiddleware := middleware.Chain( | |
| 995 | 998 | middleware.PanicRecovery(log), | |
| 996 | - | middleware.RequestLogger(log), | |
| 999 | + | middleware.RequestLogger(log, trustedNets), | |
| 997 | 1000 | middleware.SecurityHeaders, | |
| 998 | 1001 | rateLimitMiddleware, | |
| 999 | 1002 | )(svc.authHandler) | |
| @@ -1005,18 +1008,16 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 1005 | 1008 | if svc.adminHandler != nil { | |
| 1006 | 1009 | adminMiddlewares := []func(http.Handler) http.Handler{ | |
| 1007 | 1010 | middleware.PanicRecovery(log), | |
| 1008 | - | middleware.RequestLogger(log), | |
| 1011 | + | middleware.RequestLogger(log, trustedNets), | |
| 1009 | 1012 | middleware.SecurityHeaders, | |
| 1010 | 1013 | } | |
| 1011 | 1014 | // Add admin auth middleware if auth is enabled | |
| 1012 | 1015 | if svc.authSvc != nil { | |
| 1013 | 1016 | // Create rate limiters for admin API - uses same config as registry | |
| 1014 | 1017 | var globalLimiter, ipLimiter out.RateLimiter | |
| 1015 | - | var trustedNets []*net.IPNet | |
| 1016 | 1018 | if cfg.API.RateLimit.Enabled { | |
| 1017 | 1019 | globalLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.GlobalRPS, cfg.API.RateLimit.Burst, log) | |
| 1018 | 1020 | ipLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.PerIPRPS, cfg.API.RateLimit.Burst, log) | |
| 1019 | - | trustedNets = middleware.ParseTrustedProxies(cfg.API.RateLimit.TrustedProxies) | |
| 1020 | 1021 | } | |
| 1021 | 1022 | adminMiddlewares = append(adminMiddlewares, admin.AuthMiddleware(svc.authSvc, globalLimiter, ipLimiter, trustedNets, log)) | |
| 1022 | 1023 | } | |
| @@ -1024,11 +1025,14 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||
| 1024 | 1025 | registryMux.Handle("/admin/", adminWithMiddleware) | |
| 1025 | 1026 | } | |
| 1026 | 1027 | ||
| 1028 | + | // SECURITY: No CORS middleware on the proxy chain. Backend applications | |
| 1029 | + | // should control their own CORS policies. A blanket Access-Control-Allow-Origin: * | |
| 1030 | + | // would override backend CORS settings and allow any website to make | |
| 1031 | + | // cross-origin authenticated requests to proxied applications. | |
| 1027 | 1032 | proxyMiddlewares := []func(http.Handler) http.Handler{ | |
| 1028 | 1033 | middleware.PanicRecovery(log), | |
| 1029 | - | middleware.RequestLogger(log), | |
| 1034 | + | middleware.RequestLogger(log, trustedNets), | |
| 1030 | 1035 | middleware.SecurityHeaders, | |
| 1031 | - | middleware.CORS, | |
| 1032 | 1036 | } | |
| 1033 | 1037 | ||
| 1034 | 1038 | proxyWithMiddleware := middleware.Chain(proxyMiddlewares...)(svc.proxySvc) | |
| @@ -7,7 +7,6 @@ import ( | |||||
| 7 | "encoding/hex" | 7 | "encoding/hex" | ||
| 8 | "encoding/json" | 8 | "encoding/json" | ||
| 9 | "fmt" | 9 | "fmt" | ||
| 10 | - | "net" | |||
| 11 | "net/http" | 10 | "net/http" | ||
| 12 | "os" | 11 | "os" | ||
| 13 | "os/signal" | 12 | "os/signal" | ||
| @@ -948,11 +947,15 @@ func syncAndAutoStart(ctx context.Context, svc *services, log zerowrap.Logger) { | |||||
| 948 | 947 | ||||
| 949 | // createHTTPHandlers creates HTTP handlers with middleware. | 948 | // createHTTPHandlers creates HTTP handlers with middleware. | ||
| 950 | func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Handler, http.Handler) { | 949 | func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Handler, http.Handler) { | ||
| 950 | + | // Parse trusted proxies once for all middleware chains. | |||
| 951 | + | // This ensures consistent IP extraction across logging, rate limiting, and auth. | |||
| 952 | + | trustedNets := middleware.ParseTrustedProxies(cfg.API.RateLimit.TrustedProxies) | |||
| 953 | + | ||||
| 951 | registryHandler := registry.NewHandler(svc.registrySvc, log) | 954 | registryHandler := registry.NewHandler(svc.registrySvc, log) | ||
| 952 | 955 | ||||
| 953 | registryMiddlewares := []func(http.Handler) http.Handler{ | 956 | registryMiddlewares := []func(http.Handler) http.Handler{ | ||
| 954 | middleware.PanicRecovery(log), | 957 | middleware.PanicRecovery(log), | ||
| 955 | - | middleware.RequestLogger(log), | 958 | + | middleware.RequestLogger(log, trustedNets), |
| 956 | middleware.SecurityHeaders, | 959 | middleware.SecurityHeaders, | ||
| 957 | } | 960 | } | ||
| 958 | 961 | ||||
| @@ -993,7 +996,7 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 993 | // but still need rate limiting to prevent brute force attacks | 996 | // but still need rate limiting to prevent brute force attacks | ||
| 994 | authWithMiddleware := middleware.Chain( | 997 | authWithMiddleware := middleware.Chain( | ||
| 995 | middleware.PanicRecovery(log), | 998 | middleware.PanicRecovery(log), | ||
| 996 | - | middleware.RequestLogger(log), | 999 | + | middleware.RequestLogger(log, trustedNets), |
| 997 | middleware.SecurityHeaders, | 1000 | middleware.SecurityHeaders, | ||
| 998 | rateLimitMiddleware, | 1001 | rateLimitMiddleware, | ||
| 999 | )(svc.authHandler) | 1002 | )(svc.authHandler) | ||
| @@ -1005,18 +1008,16 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 1005 | if svc.adminHandler != nil { | 1008 | if svc.adminHandler != nil { | ||
| 1006 | adminMiddlewares := []func(http.Handler) http.Handler{ | 1009 | adminMiddlewares := []func(http.Handler) http.Handler{ | ||
| 1007 | middleware.PanicRecovery(log), | 1010 | middleware.PanicRecovery(log), | ||
| 1008 | - | middleware.RequestLogger(log), | 1011 | + | middleware.RequestLogger(log, trustedNets), |
| 1009 | middleware.SecurityHeaders, | 1012 | middleware.SecurityHeaders, | ||
| 1010 | } | 1013 | } | ||
| 1011 | // Add admin auth middleware if auth is enabled | 1014 | // Add admin auth middleware if auth is enabled | ||
| 1012 | if svc.authSvc != nil { | 1015 | if svc.authSvc != nil { | ||
| 1013 | // Create rate limiters for admin API - uses same config as registry | 1016 | // Create rate limiters for admin API - uses same config as registry | ||
| 1014 | var globalLimiter, ipLimiter out.RateLimiter | 1017 | var globalLimiter, ipLimiter out.RateLimiter | ||
| 1015 | - | var trustedNets []*net.IPNet | |||
| 1016 | if cfg.API.RateLimit.Enabled { | 1018 | if cfg.API.RateLimit.Enabled { | ||
| 1017 | globalLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.GlobalRPS, cfg.API.RateLimit.Burst, log) | 1019 | globalLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.GlobalRPS, cfg.API.RateLimit.Burst, log) | ||
| 1018 | ipLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.PerIPRPS, cfg.API.RateLimit.Burst, log) | 1020 | ipLimiter = ratelimit.NewMemoryStore(cfg.API.RateLimit.PerIPRPS, cfg.API.RateLimit.Burst, log) | ||
| 1019 | - | trustedNets = middleware.ParseTrustedProxies(cfg.API.RateLimit.TrustedProxies) | |||
| 1020 | } | 1021 | } | ||
| 1021 | adminMiddlewares = append(adminMiddlewares, admin.AuthMiddleware(svc.authSvc, globalLimiter, ipLimiter, trustedNets, log)) | 1022 | adminMiddlewares = append(adminMiddlewares, admin.AuthMiddleware(svc.authSvc, globalLimiter, ipLimiter, trustedNets, log)) | ||
| 1022 | } | 1023 | } | ||
| @@ -1024,11 +1025,14 @@ func createHTTPHandlers(svc *services, cfg Config, log zerowrap.Logger) (http.Ha | |||||
| 1024 | registryMux.Handle("/admin/", adminWithMiddleware) | 1025 | registryMux.Handle("/admin/", adminWithMiddleware) | ||
| 1025 | } | 1026 | } | ||
| 1026 | 1027 | ||||
| 1028 | + | // SECURITY: No CORS middleware on the proxy chain. Backend applications | |||
| 1029 | + | // should control their own CORS policies. A blanket Access-Control-Allow-Origin: * | |||
| 1030 | + | // would override backend CORS settings and allow any website to make | |||
| 1031 | + | // cross-origin authenticated requests to proxied applications. | |||
| 1027 | proxyMiddlewares := []func(http.Handler) http.Handler{ | 1032 | proxyMiddlewares := []func(http.Handler) http.Handler{ | ||
| 1028 | middleware.PanicRecovery(log), | 1033 | middleware.PanicRecovery(log), | ||
| 1029 | - | middleware.RequestLogger(log), | 1034 | + | middleware.RequestLogger(log, trustedNets), |
| 1030 | middleware.SecurityHeaders, | 1035 | middleware.SecurityHeaders, | ||
| 1031 | - | middleware.CORS, | |||
| 1032 | } | 1036 | } | ||
| 1033 | 1037 | ||||
| 1034 | proxyWithMiddleware := middleware.Chain(proxyMiddlewares...)(svc.proxySvc) | 1038 | proxyWithMiddleware := middleware.Chain(proxyMiddlewares...)(svc.proxySvc) | ||