Merge pull request #159 from bnema/fix/preserve-x-forwarded-proto
fix(proxy): preserve X-Forwarded-Proto from trusted upstream proxies
2 files · +124 -2
M internal/adapters/in/http/proxy/forwarder.go +28 -2
| @@ -15,6 +15,7 @@ import ( | |||
| 15 | 15 | ||
| 16 | 16 | "github.com/bnema/zerowrap" | |
| 17 | 17 | ||
| 18 | + | "github.com/bnema/gordon/internal/adapters/in/http/middleware" | |
| 18 | 19 | "github.com/bnema/gordon/internal/domain" | |
| 19 | 20 | ) | |
| 20 | 21 | ||
| @@ -95,7 +96,7 @@ func (h *Handler) forwardToTarget(w http.ResponseWriter, r *http.Request, target | |||
| 95 | 96 | ||
| 96 | 97 | transport := h.transportForTarget(target) | |
| 97 | 98 | ||
| 98 | - | proxy := newReverseProxy(targetURL, originalHost, transport, | |
| 99 | + | proxy := newReverseProxy(targetURL, originalHost, transport, r, h.trustedNets, | |
| 99 | 100 | func(w http.ResponseWriter, _ *http.Request, proxyErr error) { | |
| 100 | 101 | var maxBytesErr *http.MaxBytesError | |
| 101 | 102 | if errors.As(proxyErr, &maxBytesErr) { | |
| @@ -183,9 +184,13 @@ func (h *Handler) forwardToRegistry(w http.ResponseWriter, r *http.Request, regi | |||
| 183 | 184 | // originalHost, when non-empty, overrides the Host header sent to the backend. | |
| 184 | 185 | // This is used for DNS-pinned targets where the proxy dials an IP but needs to | |
| 185 | 186 | // send the original hostname for virtual-hosted upstreams. | |
| 186 | - | func newReverseProxy(targetURL *url.URL, originalHost string, transport http.RoundTripper, errorHandler func(http.ResponseWriter, *http.Request, error), modifyResp func(*http.Response) error) *httputil.ReverseProxy { | |
| 187 | + | // | |
| 188 | + | // incomingReq and trustedNets are used to preserve X-Forwarded-Proto when the | |
| 189 | + | // request originates from a trusted upstream proxy. | |
| 190 | + | func newReverseProxy(targetURL *url.URL, originalHost string, transport http.RoundTripper, incomingReq *http.Request, trustedNets []*net.IPNet, errorHandler func(http.ResponseWriter, *http.Request, error), modifyResp func(*http.Response) error) *httputil.ReverseProxy { | |
| 187 | 191 | return &httputil.ReverseProxy{ | |
| 188 | 192 | Rewrite: func(pr *httputil.ProxyRequest) { | |
| 193 | + | existingProto := pr.In.Header.Get("X-Forwarded-Proto") | |
| 189 | 194 | pr.SetURL(targetURL) | |
| 190 | 195 | pr.SetXForwarded() | |
| 191 | 196 | if originalHost != "" { | |
| @@ -193,6 +198,13 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou | |||
| 193 | 198 | } else { | |
| 194 | 199 | pr.Out.Host = targetURL.Host | |
| 195 | 200 | } | |
| 201 | + | // Preserve X-Forwarded-Proto from trusted upstream proxies. | |
| 202 | + | // SetXForwarded() unconditionally sets it based on the incoming scheme | |
| 203 | + | // (HTTP between proxies), but when a trusted proxy already sent the | |
| 204 | + | // real client proto, we must honor it. | |
| 205 | + | if existingProto != "" && isTrustedSource(incomingReq, trustedNets) { | |
| 206 | + | pr.Out.Header.Set("X-Forwarded-Proto", existingProto) | |
| 207 | + | } | |
| 196 | 208 | }, | |
| 197 | 209 | Transport: transport, | |
| 198 | 210 | ErrorHandler: errorHandler, | |
| @@ -200,6 +212,20 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou | |||
| 200 | 212 | } | |
| 201 | 213 | } | |
| 202 | 214 | ||
| 215 | + | // isTrustedSource reports whether the request's remote address is in trustedNets. | |
| 216 | + | func isTrustedSource(r *http.Request, trustedNets []*net.IPNet) bool { | |
| 217 | + | if len(trustedNets) == 0 { | |
| 218 | + | return false | |
| 219 | + | } | |
| 220 | + | host, _, err := net.SplitHostPort(r.RemoteAddr) | |
| 221 | + | if err != nil { | |
| 222 | + | // RemoteAddr may be a bare IP without a port (e.g. from Unix sockets | |
| 223 | + | // or certain proxy setups). Fall back to using it directly. | |
| 224 | + | host = strings.TrimSuffix(strings.TrimPrefix(r.RemoteAddr, "["), "]") | |
| 225 | + | } | |
| 226 | + | return middleware.IsTrustedProxy(host, trustedNets) | |
| 227 | + | } | |
| 228 | + | ||
| 203 | 229 | // modifyResponse returns a function that adds proxy headers and enforces response size limits. | |
| 204 | 230 | func modifyResponse(maxResponseSize int64) func(*http.Response) error { | |
| 205 | 231 | return func(resp *http.Response) error { | |
| @@ -15,6 +15,7 @@ import ( | |||||
| 15 | 15 | ||||
| 16 | "github.com/bnema/zerowrap" | 16 | "github.com/bnema/zerowrap" | ||
| 17 | 17 | ||||
| 18 | + | "github.com/bnema/gordon/internal/adapters/in/http/middleware" | |||
| 18 | "github.com/bnema/gordon/internal/domain" | 19 | "github.com/bnema/gordon/internal/domain" | ||
| 19 | ) | 20 | ) | ||
| 20 | 21 | ||||
| @@ -95,7 +96,7 @@ func (h *Handler) forwardToTarget(w http.ResponseWriter, r *http.Request, target | |||||
| 95 | 96 | ||||
| 96 | transport := h.transportForTarget(target) | 97 | transport := h.transportForTarget(target) | ||
| 97 | 98 | ||||
| 98 | - | proxy := newReverseProxy(targetURL, originalHost, transport, | 99 | + | proxy := newReverseProxy(targetURL, originalHost, transport, r, h.trustedNets, |
| 99 | func(w http.ResponseWriter, _ *http.Request, proxyErr error) { | 100 | func(w http.ResponseWriter, _ *http.Request, proxyErr error) { | ||
| 100 | var maxBytesErr *http.MaxBytesError | 101 | var maxBytesErr *http.MaxBytesError | ||
| 101 | if errors.As(proxyErr, &maxBytesErr) { | 102 | if errors.As(proxyErr, &maxBytesErr) { | ||
| @@ -183,9 +184,13 @@ func (h *Handler) forwardToRegistry(w http.ResponseWriter, r *http.Request, regi | |||||
| 183 | // originalHost, when non-empty, overrides the Host header sent to the backend. | 184 | // originalHost, when non-empty, overrides the Host header sent to the backend. | ||
| 184 | // This is used for DNS-pinned targets where the proxy dials an IP but needs to | 185 | // This is used for DNS-pinned targets where the proxy dials an IP but needs to | ||
| 185 | // send the original hostname for virtual-hosted upstreams. | 186 | // send the original hostname for virtual-hosted upstreams. | ||
| 186 | - | func newReverseProxy(targetURL *url.URL, originalHost string, transport http.RoundTripper, errorHandler func(http.ResponseWriter, *http.Request, error), modifyResp func(*http.Response) error) *httputil.ReverseProxy { | 187 | + | // |
| 188 | + | // incomingReq and trustedNets are used to preserve X-Forwarded-Proto when the | |||
| 189 | + | // request originates from a trusted upstream proxy. | |||
| 190 | + | func newReverseProxy(targetURL *url.URL, originalHost string, transport http.RoundTripper, incomingReq *http.Request, trustedNets []*net.IPNet, errorHandler func(http.ResponseWriter, *http.Request, error), modifyResp func(*http.Response) error) *httputil.ReverseProxy { | |||
| 187 | return &httputil.ReverseProxy{ | 191 | return &httputil.ReverseProxy{ | ||
| 188 | Rewrite: func(pr *httputil.ProxyRequest) { | 192 | Rewrite: func(pr *httputil.ProxyRequest) { | ||
| 193 | + | existingProto := pr.In.Header.Get("X-Forwarded-Proto") | |||
| 189 | pr.SetURL(targetURL) | 194 | pr.SetURL(targetURL) | ||
| 190 | pr.SetXForwarded() | 195 | pr.SetXForwarded() | ||
| 191 | if originalHost != "" { | 196 | if originalHost != "" { | ||
| @@ -193,6 +198,13 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou | |||||
| 193 | } else { | 198 | } else { | ||
| 194 | pr.Out.Host = targetURL.Host | 199 | pr.Out.Host = targetURL.Host | ||
| 195 | } | 200 | } | ||
| 201 | + | // Preserve X-Forwarded-Proto from trusted upstream proxies. | |||
| 202 | + | // SetXForwarded() unconditionally sets it based on the incoming scheme | |||
| 203 | + | // (HTTP between proxies), but when a trusted proxy already sent the | |||
| 204 | + | // real client proto, we must honor it. | |||
| 205 | + | if existingProto != "" && isTrustedSource(incomingReq, trustedNets) { | |||
| 206 | + | pr.Out.Header.Set("X-Forwarded-Proto", existingProto) | |||
| 207 | + | } | |||
| 196 | }, | 208 | }, | ||
| 197 | Transport: transport, | 209 | Transport: transport, | ||
| 198 | ErrorHandler: errorHandler, | 210 | ErrorHandler: errorHandler, | ||
| @@ -200,6 +212,20 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou | |||||
| 200 | } | 212 | } | ||
| 201 | } | 213 | } | ||
| 202 | 214 | ||||
| 215 | + | // isTrustedSource reports whether the request's remote address is in trustedNets. | |||
| 216 | + | func isTrustedSource(r *http.Request, trustedNets []*net.IPNet) bool { | |||
| 217 | + | if len(trustedNets) == 0 { | |||
| 218 | + | return false | |||
| 219 | + | } | |||
| 220 | + | host, _, err := net.SplitHostPort(r.RemoteAddr) | |||
| 221 | + | if err != nil { | |||
| 222 | + | // RemoteAddr may be a bare IP without a port (e.g. from Unix sockets | |||
| 223 | + | // or certain proxy setups). Fall back to using it directly. | |||
| 224 | + | host = strings.TrimSuffix(strings.TrimPrefix(r.RemoteAddr, "["), "]") | |||
| 225 | + | } | |||
| 226 | + | return middleware.IsTrustedProxy(host, trustedNets) | |||
| 227 | + | } | |||
| 228 | + | ||||
| 203 | // modifyResponse returns a function that adds proxy headers and enforces response size limits. | 229 | // modifyResponse returns a function that adds proxy headers and enforces response size limits. | ||
| 204 | func modifyResponse(maxResponseSize int64) func(*http.Response) error { | 230 | func modifyResponse(maxResponseSize int64) func(*http.Response) error { | ||
| 205 | return func(resp *http.Response) error { | 231 | return func(resp *http.Response) error { | ||
M internal/adapters/in/http/proxy/forwarder_test.go +96 -0
| @@ -5,6 +5,7 @@ import ( | |||
| 5 | 5 | "net" | |
| 6 | 6 | "net/http" | |
| 7 | 7 | "net/http/httptest" | |
| 8 | + | "net/url" | |
| 8 | 9 | "strconv" | |
| 9 | 10 | "testing" | |
| 10 | 11 | ||
| @@ -13,6 +14,7 @@ import ( | |||
| 13 | 14 | "github.com/stretchr/testify/mock" | |
| 14 | 15 | "github.com/stretchr/testify/require" | |
| 15 | 16 | ||
| 17 | + | "github.com/bnema/gordon/internal/adapters/in/http/middleware" | |
| 16 | 18 | "github.com/bnema/gordon/internal/boundaries/in" | |
| 17 | 19 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | |
| 18 | 20 | "github.com/bnema/gordon/internal/domain" | |
| @@ -198,3 +200,97 @@ func TestForwardToTarget_ProxyHeaderSet(t *testing.T) { | |||
| 198 | 200 | ||
| 199 | 201 | assert.Equal(t, http.StatusOK, w.Code) | |
| 200 | 202 | } | |
| 203 | + | func TestNewReverseProxy_XForwardedProto(t *testing.T) { | |
| 204 | + | trustedNets := middleware.ParseTrustedProxies([]string{"10.0.0.0/8"}) | |
| 205 | + | ||
| 206 | + | tests := []struct { | |
| 207 | + | name string | |
| 208 | + | remoteAddr string | |
| 209 | + | incomingProto string // X-Forwarded-Proto on incoming request, empty = not set | |
| 210 | + | wantProto string // expected X-Forwarded-Proto at backend | |
| 211 | + | nets []*net.IPNet | |
| 212 | + | }{ | |
| 213 | + | { | |
| 214 | + | name: "trusted proxy with https preserved", | |
| 215 | + | remoteAddr: "10.0.0.1:12345", | |
| 216 | + | incomingProto: "https", | |
| 217 | + | wantProto: "https", | |
| 218 | + | nets: trustedNets, | |
| 219 | + | }, | |
| 220 | + | { | |
| 221 | + | name: "untrusted source proto overwritten", | |
| 222 | + | remoteAddr: "1.2.3.4:12345", | |
| 223 | + | incomingProto: "https", | |
| 224 | + | wantProto: "http", | |
| 225 | + | nets: trustedNets, | |
| 226 | + | }, | |
| 227 | + | { | |
| 228 | + | name: "trusted proxy without proto set normally", | |
| 229 | + | remoteAddr: "10.0.0.1:12345", | |
| 230 | + | incomingProto: "", | |
| 231 | + | wantProto: "http", | |
| 232 | + | nets: trustedNets, | |
| 233 | + | }, | |
| 234 | + | { | |
| 235 | + | name: "no trusted nets proto not preserved", | |
| 236 | + | remoteAddr: "10.0.0.1:12345", | |
| 237 | + | incomingProto: "https", | |
| 238 | + | wantProto: "http", | |
| 239 | + | nets: nil, | |
| 240 | + | }, | |
| 241 | + | { | |
| 242 | + | name: "bare IPv4 no port trusted source preserves proto", | |
| 243 | + | remoteAddr: "10.0.0.1", | |
| 244 | + | incomingProto: "https", | |
| 245 | + | wantProto: "https", | |
| 246 | + | nets: trustedNets, | |
| 247 | + | }, | |
| 248 | + | { | |
| 249 | + | name: "bare IPv6 with brackets trusted source preserves proto", | |
| 250 | + | remoteAddr: "[::1]", | |
| 251 | + | incomingProto: "https", | |
| 252 | + | wantProto: "https", | |
| 253 | + | nets: middleware.ParseTrustedProxies([]string{"::1/128"}), | |
| 254 | + | }, | |
| 255 | + | { | |
| 256 | + | name: "bare IPv4 no port untrusted source strips proto", | |
| 257 | + | remoteAddr: "1.2.3.4", | |
| 258 | + | incomingProto: "https", | |
| 259 | + | wantProto: "http", | |
| 260 | + | nets: trustedNets, | |
| 261 | + | }, | |
| 262 | + | } | |
| 263 | + | ||
| 264 | + | for _, tt := range tests { | |
| 265 | + | t.Run(tt.name, func(t *testing.T) { | |
| 266 | + | var gotProto string | |
| 267 | + | backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 268 | + | gotProto = r.Header.Get("X-Forwarded-Proto") | |
| 269 | + | w.WriteHeader(http.StatusOK) | |
| 270 | + | })) | |
| 271 | + | defer backend.Close() | |
| 272 | + | ||
| 273 | + | backendURL, err := url.Parse(backend.URL) | |
| 274 | + | require.NoError(t, err) | |
| 275 | + | ||
| 276 | + | // Build incoming request (http scheme, no TLS) | |
| 277 | + | incomingReq := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) | |
| 278 | + | incomingReq.RemoteAddr = tt.remoteAddr | |
| 279 | + | if tt.incomingProto != "" { | |
| 280 | + | incomingReq.Header.Set("X-Forwarded-Proto", tt.incomingProto) | |
| 281 | + | } | |
| 282 | + | ||
| 283 | + | proxy := newReverseProxy(backendURL, "", http.DefaultTransport, incomingReq, tt.nets, | |
| 284 | + | func(w http.ResponseWriter, _ *http.Request, err error) { | |
| 285 | + | http.Error(w, err.Error(), http.StatusBadGateway) | |
| 286 | + | }, | |
| 287 | + | nil, | |
| 288 | + | ) | |
| 289 | + | ||
| 290 | + | rec := httptest.NewRecorder() | |
| 291 | + | proxy.ServeHTTP(rec, incomingReq) | |
| 292 | + | ||
| 293 | + | assert.Equal(t, tt.wantProto, gotProto) | |
| 294 | + | }) | |
| 295 | + | } | |
| 296 | + | } | |
| @@ -5,6 +5,7 @@ import ( | |||||
| 5 | "net" | 5 | "net" | ||
| 6 | "net/http" | 6 | "net/http" | ||
| 7 | "net/http/httptest" | 7 | "net/http/httptest" | ||
| 8 | + | "net/url" | |||
| 8 | "strconv" | 9 | "strconv" | ||
| 9 | "testing" | 10 | "testing" | ||
| 10 | 11 | ||||
| @@ -13,6 +14,7 @@ import ( | |||||
| 13 | "github.com/stretchr/testify/mock" | 14 | "github.com/stretchr/testify/mock" | ||
| 14 | "github.com/stretchr/testify/require" | 15 | "github.com/stretchr/testify/require" | ||
| 15 | 16 | ||||
| 17 | + | "github.com/bnema/gordon/internal/adapters/in/http/middleware" | |||
| 16 | "github.com/bnema/gordon/internal/boundaries/in" | 18 | "github.com/bnema/gordon/internal/boundaries/in" | ||
| 17 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | 19 | inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks" | ||
| 18 | "github.com/bnema/gordon/internal/domain" | 20 | "github.com/bnema/gordon/internal/domain" | ||
| @@ -198,3 +200,97 @@ func TestForwardToTarget_ProxyHeaderSet(t *testing.T) { | |||||
| 198 | 200 | ||||
| 199 | assert.Equal(t, http.StatusOK, w.Code) | 201 | assert.Equal(t, http.StatusOK, w.Code) | ||
| 200 | } | 202 | } | ||
| 203 | + | func TestNewReverseProxy_XForwardedProto(t *testing.T) { | |||
| 204 | + | trustedNets := middleware.ParseTrustedProxies([]string{"10.0.0.0/8"}) | |||
| 205 | + | ||||
| 206 | + | tests := []struct { | |||
| 207 | + | name string | |||
| 208 | + | remoteAddr string | |||
| 209 | + | incomingProto string // X-Forwarded-Proto on incoming request, empty = not set | |||
| 210 | + | wantProto string // expected X-Forwarded-Proto at backend | |||
| 211 | + | nets []*net.IPNet | |||
| 212 | + | }{ | |||
| 213 | + | { | |||
| 214 | + | name: "trusted proxy with https preserved", | |||
| 215 | + | remoteAddr: "10.0.0.1:12345", | |||
| 216 | + | incomingProto: "https", | |||
| 217 | + | wantProto: "https", | |||
| 218 | + | nets: trustedNets, | |||
| 219 | + | }, | |||
| 220 | + | { | |||
| 221 | + | name: "untrusted source proto overwritten", | |||
| 222 | + | remoteAddr: "1.2.3.4:12345", | |||
| 223 | + | incomingProto: "https", | |||
| 224 | + | wantProto: "http", | |||
| 225 | + | nets: trustedNets, | |||
| 226 | + | }, | |||
| 227 | + | { | |||
| 228 | + | name: "trusted proxy without proto set normally", | |||
| 229 | + | remoteAddr: "10.0.0.1:12345", | |||
| 230 | + | incomingProto: "", | |||
| 231 | + | wantProto: "http", | |||
| 232 | + | nets: trustedNets, | |||
| 233 | + | }, | |||
| 234 | + | { | |||
| 235 | + | name: "no trusted nets proto not preserved", | |||
| 236 | + | remoteAddr: "10.0.0.1:12345", | |||
| 237 | + | incomingProto: "https", | |||
| 238 | + | wantProto: "http", | |||
| 239 | + | nets: nil, | |||
| 240 | + | }, | |||
| 241 | + | { | |||
| 242 | + | name: "bare IPv4 no port trusted source preserves proto", | |||
| 243 | + | remoteAddr: "10.0.0.1", | |||
| 244 | + | incomingProto: "https", | |||
| 245 | + | wantProto: "https", | |||
| 246 | + | nets: trustedNets, | |||
| 247 | + | }, | |||
| 248 | + | { | |||
| 249 | + | name: "bare IPv6 with brackets trusted source preserves proto", | |||
| 250 | + | remoteAddr: "[::1]", | |||
| 251 | + | incomingProto: "https", | |||
| 252 | + | wantProto: "https", | |||
| 253 | + | nets: middleware.ParseTrustedProxies([]string{"::1/128"}), | |||
| 254 | + | }, | |||
| 255 | + | { | |||
| 256 | + | name: "bare IPv4 no port untrusted source strips proto", | |||
| 257 | + | remoteAddr: "1.2.3.4", | |||
| 258 | + | incomingProto: "https", | |||
| 259 | + | wantProto: "http", | |||
| 260 | + | nets: trustedNets, | |||
| 261 | + | }, | |||
| 262 | + | } | |||
| 263 | + | ||||
| 264 | + | for _, tt := range tests { | |||
| 265 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 266 | + | var gotProto string | |||
| 267 | + | backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 268 | + | gotProto = r.Header.Get("X-Forwarded-Proto") | |||
| 269 | + | w.WriteHeader(http.StatusOK) | |||
| 270 | + | })) | |||
| 271 | + | defer backend.Close() | |||
| 272 | + | ||||
| 273 | + | backendURL, err := url.Parse(backend.URL) | |||
| 274 | + | require.NoError(t, err) | |||
| 275 | + | ||||
| 276 | + | // Build incoming request (http scheme, no TLS) | |||
| 277 | + | incomingReq := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) | |||
| 278 | + | incomingReq.RemoteAddr = tt.remoteAddr | |||
| 279 | + | if tt.incomingProto != "" { | |||
| 280 | + | incomingReq.Header.Set("X-Forwarded-Proto", tt.incomingProto) | |||
| 281 | + | } | |||
| 282 | + | ||||
| 283 | + | proxy := newReverseProxy(backendURL, "", http.DefaultTransport, incomingReq, tt.nets, | |||
| 284 | + | func(w http.ResponseWriter, _ *http.Request, err error) { | |||
| 285 | + | http.Error(w, err.Error(), http.StatusBadGateway) | |||
| 286 | + | }, | |||
| 287 | + | nil, | |||
| 288 | + | ) | |||
| 289 | + | ||||
| 290 | + | rec := httptest.NewRecorder() | |||
| 291 | + | proxy.ServeHTTP(rec, incomingReq) | |||
| 292 | + | ||||
| 293 | + | assert.Equal(t, tt.wantProto, gotProto) | |||
| 294 | + | }) | |||
| 295 | + | } | |||
| 296 | + | } | |||