diff --git a/.github/workflows/validate.yaml b/.github/workflows/validate.yaml index ea4b54c199..eb02754cc0 100644 --- a/.github/workflows/validate.yaml +++ b/.github/workflows/validate.yaml @@ -72,11 +72,6 @@ jobs: make generate git diff --exit-code - - name: go mod tidy - run: | - go mod tidy - git diff --exit-code - - name: make generate-crd run: | make generate-crd diff --git a/docs/content/migrate/v3.md b/docs/content/migrate/v3.md index 7c6aee5454..97bd7a6831 100644 --- a/docs/content/migrate/v3.md +++ b/docs/content/migrate/v3.md @@ -9,6 +9,27 @@ This guide provides detailed migration steps for upgrading between different Tra --- +## v3.6.14 + +### Kubernetes CRD: Chain middleware and `allowCrossNamespace` + +In `v3.6.14`, the `Chain` middleware now honors the Kubernetes CRD provider's `allowCrossNamespace` option. +Previously, a `Chain` could reference middlewares in other namespaces regardless of the `allowCrossNamespace` configuration. + +If `allowCrossNamespace` is set to `false` (the default) and a `Chain` middleware references a middleware in a different namespace from its own, +the whole `Chain` is now rejected and an error is logged. + +### ForwardAuth middleware: `trustForwardHeader` + +In `v3.6.14`, when `trustForwardHeader` is not explicitly set, Traefik logs a warning as its behavior is inconsistent: +some `X-Forwarded-*` headers (e.g. `X-Forwarded-For`, `X-Forwarded-Proto`) are removed while others (e.g. `X-Forwarded-Prefix`) are forwarded untouched. + +To silence the warning and avoid security concerns, explicitly set `trustForwardHeader` to `true` or `false` in your ForwardAuth middleware configuration. + +Please check out the [ForwardAuth](../reference/routing-configuration/http/middlewares/forwardauth.md##opt-trustforwardheader) middleware documentation for more details. + +--- + ## v3.6.9 ### `maxResponseBodySize` configuration on ForwardAuth middleware diff --git a/docs/content/reference/routing-configuration/http/middlewares/forwardauth.md b/docs/content/reference/routing-configuration/http/middlewares/forwardauth.md index dd2f1aad56..d463197d50 100644 --- a/docs/content/reference/routing-configuration/http/middlewares/forwardauth.md +++ b/docs/content/reference/routing-configuration/http/middlewares/forwardauth.md @@ -56,7 +56,7 @@ spec: | Field | Description | Default | Required | |:-----------|:--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------|:---------| | `address` | Authentication server address. | "" | Yes | -| `trustForwardHeader` | Trust all `X-Forwarded-*` headers. | false | No | +| `trustForwardHeader` | Trust all `X-Forwarded-*` headers.
More information [here](#trustforwardheader)| false | No | | `authResponseHeaders` | List of headers to copy from the authentication server response and set on forwarded request, replacing any existing conflicting headers. | [] | No | | `authResponseHeadersRegex` | Regex to match by the headers to copy from the authentication server response and set on forwarded request, after stripping all headers that match the regex.
More information [here](#authresponseheadersregex). | "" | No | | `authRequestHeaders` | List of the headers to copy from the request to the authentication server.
It allows filtering headers that should not be passed to the authentication server.
If not set or empty, then all request headers are passed. | [] | No | @@ -127,6 +127,28 @@ If left unset, the request body size is unrestricted which can have performance It is strongly recommended to set this option to a suitable value. Not setting it (or setting it to `-1`) allows unlimited response body sizes which can lead to DoS attacks and memory exhaustion. +### trustforwardheader + +### `trustForwardHeader` + +!!! warning + + If `trustForwardHeader` is not explicitly set, Traefik will log a warning at startup and use a legacy behavior where some `X-Forwarded-*` headers (e.g. `X-Forwarded-For`, `X-Forwarded-Proto`) are removed but others (e.g. `X-Forwarded-Prefix`) are forwarded untouched. + To silence this warning, explicitly set `trustForwardHeader` to `true` or `false`. + +!!! tip "Recommended configuration" + + The recommended approach is to configure trusted IPs at the [EntryPoint level](../../../install-configuration/entrypoints.md#forwarded-headers) using `forwardedHeaders.trustedIPs`, and set `trustForwardHeader: true` on this middleware. + + With this setup, the EntryPoint is responsible for sanitizing incoming `X-Forwarded-*` headers: + it strips any such headers sent by untrusted clients and only preserves those coming from trusted upstream proxies. + By the time the ForwardAuth middleware processes the request, all `X-Forwarded-*` headers are guaranteed to be trustworthy, + including those intentionally added by other middlewares in the chain — for example, the `X-Forwarded-Prefix` header set by the [StripPrefix](stripprefix.md) middleware. + + Setting `trustForwardHeader: true` on this middleware then simply tells ForwardAuth to forward all those (already sanitized) headers to the authentication server. + +Set the `trustForwardHeader` option to `true` to trust all `X-Forwarded-*` headers. + ## Forward-Request Headers The following request properties are provided to the forward-auth target endpoint as `X-Forwarded-` headers. diff --git a/pkg/config/dynamic/middlewares.go b/pkg/config/dynamic/middlewares.go index a04f99713a..f7fb4b4a83 100644 --- a/pkg/config/dynamic/middlewares.go +++ b/pkg/config/dynamic/middlewares.go @@ -249,7 +249,7 @@ type ForwardAuth struct { // TLS defines the configuration used to secure the connection to the authentication server. TLS *ClientTLS `json:"tls,omitempty" toml:"tls,omitempty" yaml:"tls,omitempty" export:"true"` // TrustForwardHeader defines whether to trust (ie: forward) all X-Forwarded-* headers. - TrustForwardHeader bool `json:"trustForwardHeader,omitempty" toml:"trustForwardHeader,omitempty" yaml:"trustForwardHeader,omitempty" export:"true"` + TrustForwardHeader *bool `json:"trustForwardHeader,omitempty" toml:"trustForwardHeader,omitempty" yaml:"trustForwardHeader,omitempty" export:"true"` // AuthResponseHeaders defines the list of headers to copy from the authentication server response and set on forwarded request, replacing any existing conflicting headers. AuthResponseHeaders []string `json:"authResponseHeaders,omitempty" toml:"authResponseHeaders,omitempty" yaml:"authResponseHeaders,omitempty" export:"true"` // AuthResponseHeadersRegex defines the regex to match headers to copy from the authentication server response and set on forwarded request, after stripping all headers that match the regex. diff --git a/pkg/config/dynamic/zz_generated.deepcopy.go b/pkg/config/dynamic/zz_generated.deepcopy.go index 88a956e18e..a14c1e97cb 100644 --- a/pkg/config/dynamic/zz_generated.deepcopy.go +++ b/pkg/config/dynamic/zz_generated.deepcopy.go @@ -363,6 +363,11 @@ func (in *ForwardAuth) DeepCopyInto(out *ForwardAuth) { *out = new(ClientTLS) (*in).DeepCopyInto(*out) } + if in.TrustForwardHeader != nil { + in, out := &in.TrustForwardHeader, &out.TrustForwardHeader + *out = new(bool) + **out = **in + } if in.AuthResponseHeaders != nil { in, out := &in.AuthResponseHeaders, &out.AuthResponseHeaders *out = make([]string, len(*in)) diff --git a/pkg/config/label/label_test.go b/pkg/config/label/label_test.go index 903aa1a097..b85001f1c7 100644 --- a/pkg/config/label/label_test.go +++ b/pkg/config/label/label_test.go @@ -576,7 +576,7 @@ func TestDecodeConfiguration(t *testing.T) { InsecureSkipVerify: true, CAOptional: pointer(true), }, - TrustForwardHeader: true, + TrustForwardHeader: pointer(true), AuthResponseHeaders: []string{ "foobar", "fiibar", @@ -1131,7 +1131,7 @@ func TestEncodeConfiguration(t *testing.T) { InsecureSkipVerify: true, CAOptional: pointer(true), }, - TrustForwardHeader: true, + TrustForwardHeader: pointer(true), AuthResponseHeaders: []string{ "foobar", "fiibar", diff --git a/pkg/middlewares/accesslog/logger.go b/pkg/middlewares/accesslog/logger.go index 239d0708d0..2290cd4c06 100644 --- a/pkg/middlewares/accesslog/logger.go +++ b/pkg/middlewares/accesslog/logger.go @@ -458,8 +458,8 @@ func usernameIfPresent(theURL *url.URL) string { return "-" } -var requestCounter uint64 // Request ID +var requestCounter atomic.Uint64 // Request ID func nextRequestCount() uint64 { - return atomic.AddUint64(&requestCounter, 1) + return requestCounter.Add(1) } diff --git a/pkg/middlewares/auth/basic_auth.go b/pkg/middlewares/auth/basic_auth.go index a6b99b45e8..5c7ef27171 100644 --- a/pkg/middlewares/auth/basic_auth.go +++ b/pkg/middlewares/auth/basic_auth.go @@ -46,7 +46,7 @@ func NewBasic(ctx context.Context, next http.Handler, authConfig dynamic.BasicAu // To prevent timing attacks, we need to compute a hash even if the user is not found. // We assume it to be safe only when the users hashes are all from the same algorithm, // so we can pick the first one as a random hash to compute. - notFoundSecret := users[slices.Collect(maps.Values(users))[0]] + notFoundSecret := slices.Collect(maps.Values(users))[0] ba := &basicAuth{ next: next, diff --git a/pkg/middlewares/auth/basic_auth_test.go b/pkg/middlewares/auth/basic_auth_test.go index 83a56390e3..8133461679 100644 --- a/pkg/middlewares/auth/basic_auth_test.go +++ b/pkg/middlewares/auth/basic_auth_test.go @@ -16,6 +16,17 @@ import ( "github.com/traefik/traefik/v3/pkg/testhelpers" ) +func TestNewBasicNotFoundSecretIsSet(t *testing.T) { + auth := dynamic.BasicAuth{ + Users: []string{"test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/"}, + } + middleware, err := NewBasic(t.Context(), nil, auth, "authName") + require.NoError(t, err) + + ba := middleware.(*basicAuth) + assert.Equal(t, "$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/", ba.notFoundSecret) +} + func TestBasicAuthFail(t *testing.T) { next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintln(w, "traefik") diff --git a/pkg/middlewares/auth/forward.go b/pkg/middlewares/auth/forward.go index bbd2a3196a..f34e778877 100644 --- a/pkg/middlewares/auth/forward.go +++ b/pkg/middlewares/auth/forward.go @@ -6,16 +6,19 @@ import ( "errors" "fmt" "io" + "maps" "net" "net/http" "net/url" "regexp" + "slices" "strings" "time" "github.com/traefik/traefik/v3/pkg/config/dynamic" "github.com/traefik/traefik/v3/pkg/middlewares" "github.com/traefik/traefik/v3/pkg/middlewares/accesslog" + "github.com/traefik/traefik/v3/pkg/middlewares/forwardedheaders" "github.com/traefik/traefik/v3/pkg/middlewares/observability" "github.com/traefik/traefik/v3/pkg/observability/tracing" "github.com/traefik/traefik/v3/pkg/proxy/httputil" @@ -27,10 +30,7 @@ import ( const typeNameForward = "ForwardAuth" -const ( - xForwardedURI = "X-Forwarded-Uri" - xForwardedMethod = "X-Forwarded-Method" -) +var errResponseBodyTooLarge = errors.New("response body too large") // hopHeaders Hop-by-hop headers to be removed in the authentication request. // http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html @@ -53,7 +53,7 @@ type forwardAuth struct { next http.Handler name string client http.Client - trustForwardHeader bool + trustForwardHeader *bool authRequestHeaders []string maxResponseBodySize int64 addAuthCookiesToResponse map[string]struct{} @@ -140,6 +140,12 @@ func NewForward(ctx context.Context, next http.Handler, config dynamic.ForwardAu fa.authResponseHeadersRegex = re } + if config.TrustForwardHeader == nil { + logger.Warn().Msg("TrustForwardHeader is not set: this creates an inconsistent security behavior where some X-Forwarded headers (e.g. X-Forwarded-For, X-Forwarded-Proto) are removed but others (e.g. X-Forwarded-Prefix) are forwarded untouched. Set it to false to remove all X-Forwarded headers, or true to trust them all.") + } else if *config.TrustForwardHeader && len(fa.authRequestHeaders) > 0 { + fa.authRequestHeaders = append(fa.authRequestHeaders, slices.Collect(maps.Keys(forwardedheaders.XHeadersSet))...) + } + return fa, nil } @@ -188,7 +194,11 @@ func (fa *forwardAuth) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } } - writeHeader(req, forwardReq, fa.trustForwardHeader, fa.authRequestHeaders) + if fa.trustForwardHeader != nil { + writeHeader(req, forwardReq, *fa.trustForwardHeader, fa.authRequestHeaders) + } else { + oldWriteHeader(req, forwardReq, fa.authRequestHeaders) + } var forwardSpan trace.Span var tracer *tracing.Tracer @@ -369,8 +379,6 @@ func (fa *forwardAuth) readBodyBytes(req *http.Request) ([]byte, error) { return nil, errBodyTooLarge } -var errResponseBodyTooLarge = errors.New("response body too large") - func (fa *forwardAuth) readResponseBodyBytes(res *http.Response) ([]byte, error) { if fa.maxResponseBodySize < 0 { return io.ReadAll(res.Body) @@ -396,6 +404,10 @@ func writeHeader(req, forwardReq *http.Request, trustForwardHeader bool, allowed RemoveConnectionHeaders(forwardReq) utils.RemoveHeaders(forwardReq.Header, hopHeaders...) + if !trustForwardHeader { + forwardedheaders.DeleteXForwardedHeaders(forwardReq.Header) + } + if _, ok := req.Header[userAgentHeader]; !ok { // If the incoming request doesn't have a User-Agent header set, // don't send the default Go HTTP client User-Agent for the forwarded request. @@ -405,59 +417,61 @@ func writeHeader(req, forwardReq *http.Request, trustForwardHeader bool, allowed forwardReq.Header = filterForwardRequestHeaders(forwardReq.Header, allowedHeaders) if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil { - if trustForwardHeader { - if prior, ok := req.Header[forward.XForwardedFor]; ok { - clientIP = strings.Join(prior, ", ") + ", " + clientIP - } + if prior, ok := forwardReq.Header[forwardedheaders.XForwardedFor]; ok { + clientIP = strings.Join(prior, ", ") + ", " + clientIP } - forwardReq.Header.Set(forward.XForwardedFor, clientIP) + forwardReq.Header.Set(forwardedheaders.XForwardedFor, clientIP) } - xMethod := req.Header.Get(xForwardedMethod) - switch { - case xMethod != "" && trustForwardHeader: - forwardReq.Header.Set(xForwardedMethod, xMethod) - case req.Method != "": - forwardReq.Header.Set(xForwardedMethod, req.Method) - default: - forwardReq.Header.Del(xForwardedMethod) + if _, ok := forwardReq.Header[forwardedheaders.XForwardedMethod]; !ok { + forwardReq.Header.Set(forwardedheaders.XForwardedMethod, req.Method) } - xfp := req.Header.Get(forward.XForwardedProto) - switch { - case xfp != "" && trustForwardHeader: - forwardReq.Header.Set(forward.XForwardedProto, xfp) - case req.TLS != nil: - forwardReq.Header.Set(forward.XForwardedProto, "https") - default: - forwardReq.Header.Set(forward.XForwardedProto, "http") + if _, ok := forwardReq.Header[forwardedheaders.XForwardedProto]; !ok { + forwardReq.Header.Set(forwardedheaders.XForwardedProto, "http") + if req.TLS != nil { + forwardReq.Header.Set(forwardedheaders.XForwardedProto, "https") + } } - if xfp := req.Header.Get(forward.XForwardedPort); xfp != "" && trustForwardHeader { - forwardReq.Header.Set(forward.XForwardedPort, xfp) + if _, ok := forwardReq.Header[forwardedheaders.XForwardedPort]; !ok { + forwardReq.Header.Set(forwardedheaders.XForwardedPort, forwardedPort(req)) } - xfh := req.Header.Get(forward.XForwardedHost) - switch { - case xfh != "" && trustForwardHeader: - forwardReq.Header.Set(forward.XForwardedHost, xfh) - case req.Host != "": - forwardReq.Header.Set(forward.XForwardedHost, req.Host) - default: - forwardReq.Header.Del(forward.XForwardedHost) + if _, ok := forwardReq.Header[forwardedheaders.XForwardedHost]; !ok { + forwardReq.Header.Set(forwardedheaders.XForwardedHost, req.Host) } - xfURI := req.Header.Get(xForwardedURI) - switch { - case xfURI != "" && trustForwardHeader: - forwardReq.Header.Set(xForwardedURI, xfURI) - case req.URL.RequestURI() != "": - forwardReq.Header.Set(xForwardedURI, req.URL.RequestURI()) - default: - forwardReq.Header.Del(xForwardedURI) + if _, ok := forwardReq.Header[forwardedheaders.XForwardedURI]; !ok { + forwardReq.Header.Set(forwardedheaders.XForwardedURI, req.URL.RequestURI()) } } +// oldWriteHeader is the legacy implementation of writeHeader, which is used when TrustForwardHeader is not set (old false behavior). +// It is kept to avoid breaking existing configurations that rely on the previous behavior. +func oldWriteHeader(req, forwardReq *http.Request, allowedHeaders []string) { + utils.CopyHeaders(forwardReq.Header, req.Header) + + RemoveConnectionHeaders(forwardReq) + utils.RemoveHeaders(forwardReq.Header, hopHeaders...) + + forwardReq.Header = filterForwardRequestHeaders(forwardReq.Header, allowedHeaders) + + if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil { + forwardReq.Header.Set(forwardedheaders.XForwardedFor, clientIP) + } + + proto := "http" + if req.TLS != nil { + proto = "https" + } + forwardReq.Header.Set(forwardedheaders.XForwardedProto, proto) + + forwardReq.Header.Set(forwardedheaders.XForwardedMethod, req.Method) + forwardReq.Header.Set(forwardedheaders.XForwardedHost, req.Host) + forwardReq.Header.Set(forwardedheaders.XForwardedURI, req.URL.RequestURI()) +} + func filterForwardRequestHeaders(forwardRequestHeaders http.Header, allowedHeaders []string) http.Header { if len(allowedHeaders) == 0 { return forwardRequestHeaders @@ -465,11 +479,30 @@ func filterForwardRequestHeaders(forwardRequestHeaders http.Header, allowedHeade filteredHeaders := http.Header{} for _, headerName := range allowedHeaders { - values := forwardRequestHeaders.Values(headerName) - if len(values) > 0 { - filteredHeaders[http.CanonicalHeaderKey(headerName)] = append([]string(nil), values...) + if values := forwardRequestHeaders.Values(headerName); len(values) > 0 { + filteredHeaders[http.CanonicalHeaderKey(headerName)] = values } } return filteredHeaders } + +func forwardedPort(req *http.Request) string { + if req == nil { + return "" + } + + if _, port, err := net.SplitHostPort(req.Host); err == nil && port != "" { + return port + } + + if req.Header.Get(forwardedheaders.XForwardedProto) == "https" || req.Header.Get(forwardedheaders.XForwardedProto) == "wss" { + return "443" + } + + if req.TLS != nil { + return "443" + } + + return "80" +} diff --git a/pkg/middlewares/auth/forward_test.go b/pkg/middlewares/auth/forward_test.go index da3d03dd15..fba4aa41ee 100644 --- a/pkg/middlewares/auth/forward_test.go +++ b/pkg/middlewares/auth/forward_test.go @@ -480,9 +480,9 @@ func TestForwardAuthForwardError(t *testing.T) { assert.Equal(t, http.StatusInternalServerError, recorder.Result().StatusCode) } -func Test_writeHeader(t *testing.T) { +func TestForwardAuth_writeHeader(t *testing.T) { testCases := []struct { - name string + desc string headers map[string]string authRequestHeaders []string trustForwardHeader bool @@ -491,7 +491,7 @@ func Test_writeHeader(t *testing.T) { checkForUnexpectedHeaders bool }{ { - name: "trust Forward Header", + desc: "trust forward header", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -503,7 +503,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "not trust Forward Header", + desc: "not trust forward header", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -515,7 +515,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "trust Forward Header with empty Host", + desc: "trust forward header with empty Host", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -528,7 +528,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "not trust Forward Header with empty Host", + desc: "not trust forward header with empty Host", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -540,7 +540,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "trust Forward Header with forwarded URI", + desc: "trust forward header with forwarded URI", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -554,7 +554,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "not trust Forward Header with forward requested URI", + desc: "not trust forward header with forward requested URI", headers: map[string]string{ "Accept": "application/json", "X-Forwarded-Host": "fii.bir", @@ -568,7 +568,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "trust Forward Header with forwarded request Method", + desc: "trust forward header with forwarded request Method", headers: map[string]string{ "X-Forwarded-Method": "OPTIONS", }, @@ -578,7 +578,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "not trust Forward Header with forward request Method", + desc: "not trust forward header with forward request Method", headers: map[string]string{ "X-Forwarded-Method": "OPTIONS", }, @@ -588,7 +588,7 @@ func Test_writeHeader(t *testing.T) { }, }, { - name: "remove hop-by-hop headers", + desc: "remove hop-by-hop headers", headers: map[string]string{ forward.Connection: "Connection", forward.KeepAlive: "KeepAlive", @@ -607,6 +607,7 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", forward.ProxyAuthenticate: "ProxyAuthenticate", forward.ProxyAuthorization: "ProxyAuthorization", "User-Agent": "", @@ -614,7 +615,7 @@ func Test_writeHeader(t *testing.T) { checkForUnexpectedHeaders: true, }, { - name: "filter forward request headers", + desc: "filter forward request headers", headers: map[string]string{ "X-CustomHeader": "CustomHeader", "Content-Type": "multipart/form-data; boundary=---123456", @@ -629,11 +630,12 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", }, checkForUnexpectedHeaders: true, }, { - name: "filter forward request headers doesn't add new headers", + desc: "filter forward request headers doesn't add new headers", headers: map[string]string{ "X-CustomHeader": "CustomHeader", "Content-Type": "multipart/form-data; boundary=---123456", @@ -649,11 +651,12 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", }, checkForUnexpectedHeaders: true, }, { - name: "set empty User-Agent header if header is allowed but missing", + desc: "set empty User-Agent header if header is allowed but missing", headers: map[string]string{ "X-CustomHeader": "CustomHeader", "Accept": "application/json", @@ -670,12 +673,13 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", "User-Agent": "", }, checkForUnexpectedHeaders: true, }, { - name: "ignore User-Agent header if header is not allowed and missing", + desc: "ignore User-Agent header if header is not allowed and missing", headers: map[string]string{ "X-CustomHeader": "CustomHeader", "Accept": "application/json", @@ -691,11 +695,12 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", }, checkForUnexpectedHeaders: true, }, { - name: "set empty User-Agent header if header is missing", + desc: "set empty User-Agent header if header is missing", headers: map[string]string{ "X-CustomHeader": "CustomHeader", "Accept": "application/json", @@ -707,14 +712,64 @@ func Test_writeHeader(t *testing.T) { "X-Forwarded-Host": "foo.bar", "X-Forwarded-Uri": "/path?q=1", "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", "User-Agent": "", }, checkForUnexpectedHeaders: true, }, + { + desc: "authRequestHeaders and XForwarded are kept if trusted", + headers: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Uri": "/path?q=2", + }, + authRequestHeaders: []string{ + "X-CustomHeader", + }, + trustForwardHeader: true, + expectedHeaders: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=2", + "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", + }, + checkForUnexpectedHeaders: true, + }, + { + desc: "X-Forwarded and X_forwarded headers are removed when not trusted", + headers: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X_forwarded_for": "127.0.0.1", + "X-Forwarded-Proto": "xxx", + }, + trustForwardHeader: false, + expectedHeaders: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + "X-Forwarded-Method": "GET", + "X-Forwarded-Port": "80", + }, + }, } for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + cfg := dynamic.ForwardAuth{ + TrustForwardHeader: &test.trustForwardHeader, + AuthRequestHeaders: test.authRequestHeaders, + } + hdl, err := NewForward(t.Context(), nil, cfg, "test") + require.NoError(t, err) + + fwdAuth, ok := hdl.(*forwardAuth) + require.True(t, ok) + req := testhelpers.MustNewRequest(http.MethodGet, "http://foo.bar/path?q=1", nil) for key, value := range test.headers { req.Header.Set(key, value) @@ -726,7 +781,7 @@ func Test_writeHeader(t *testing.T) { forwardReq := testhelpers.MustNewRequest(http.MethodGet, "http://foo.bar/path?q=1", nil) - writeHeader(req, forwardReq, test.trustForwardHeader, test.authRequestHeaders) + writeHeader(req, forwardReq, *fwdAuth.trustForwardHeader, fwdAuth.authRequestHeaders) actualHeaders := forwardReq.Header @@ -735,13 +790,184 @@ func Test_writeHeader(t *testing.T) { _, headerExists := actualHeaders[http.CanonicalHeaderKey(key)] assert.True(t, headerExists, "Expected header %s not found", key) - assert.Equal(t, value, actualHeaders.Get(key)) + assert.Equal(t, value, forwardReq.Header.Get(key)) - actualHeaders.Del(key) + forwardReq.Header.Del(key) } if test.checkForUnexpectedHeaders { - for key := range actualHeaders { + for key := range forwardReq.Header { + assert.Fail(t, "Unexpected header found", key) + } + } + }) + } +} + +func TestForwardAuth_oldWriteHeader(t *testing.T) { + testCases := []struct { + desc string + headers map[string]string + authRequestHeaders []string + emptyHost bool + expectedHeaders map[string]string + checkForUnexpectedHeaders bool + }{ + { + desc: "not trust forward header", + headers: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "fii.bir", + }, + expectedHeaders: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "foo.bar", + }, + }, + { + desc: "not trust forward header with empty Host", + headers: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "fii.bir", + }, + emptyHost: true, + expectedHeaders: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "", + }, + }, + { + desc: "not trust forward header with forward requested URI", + headers: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "fii.bir", + "X-Forwarded-Uri": "/forward?q=1", + }, + expectedHeaders: map[string]string{ + "Accept": "application/json", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + }, + }, + { + desc: "not trust forward header with forward request Method", + headers: map[string]string{ + "X-Forwarded-Method": "OPTIONS", + }, + expectedHeaders: map[string]string{ + "X-Forwarded-Method": "GET", + }, + }, + { + desc: "remove hop-by-hop headers", + headers: map[string]string{ + forward.Connection: "Connection", + forward.KeepAlive: "KeepAlive", + forward.ProxyAuthenticate: "ProxyAuthenticate", + forward.ProxyAuthorization: "ProxyAuthorization", + forward.Te: "Te", + forward.Trailers: "Trailers", + forward.TransferEncoding: "TransferEncoding", + forward.Upgrade: "Upgrade", + "X-CustomHeader": "CustomHeader", + }, + expectedHeaders: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + "X-Forwarded-Method": "GET", + forward.ProxyAuthenticate: "ProxyAuthenticate", + forward.ProxyAuthorization: "ProxyAuthorization", + }, + checkForUnexpectedHeaders: true, + }, + { + desc: "filter forward request headers", + headers: map[string]string{ + "X-CustomHeader": "CustomHeader", + "Content-Type": "multipart/form-data; boundary=---123456", + }, + authRequestHeaders: []string{ + "X-CustomHeader", + }, + expectedHeaders: map[string]string{ + "x-customHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + "X-Forwarded-Method": "GET", + }, + checkForUnexpectedHeaders: true, + }, + { + desc: "filter forward request headers doesn't add new headers", + headers: map[string]string{ + "X-CustomHeader": "CustomHeader", + "Content-Type": "multipart/form-data; boundary=---123456", + }, + authRequestHeaders: []string{ + "X-CustomHeader", + "X-Non-Exists-Header", + }, + expectedHeaders: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + "X-Forwarded-Method": "GET", + }, + checkForUnexpectedHeaders: true, + }, + + { + desc: "X-Forwarded-Prefix is kept for non-breaking behavior", + headers: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Prefix": "foo.bar", + }, + expectedHeaders: map[string]string{ + "X-CustomHeader": "CustomHeader", + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "foo.bar", + "X-Forwarded-Uri": "/path?q=1", + "X-Forwarded-Method": "GET", + "X-Forwarded-Prefix": "foo.bar", + }, + checkForUnexpectedHeaders: true, + }, + } + + for _, test := range testCases { + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + hdl, err := NewForward(t.Context(), nil, dynamic.ForwardAuth{AuthRequestHeaders: test.authRequestHeaders}, "test") + require.NoError(t, err) + + fwdAuth, ok := hdl.(*forwardAuth) + require.True(t, ok) + + req := testhelpers.MustNewRequest(http.MethodGet, "http://foo.bar/path?q=1", nil) + for key, value := range test.headers { + req.Header.Set(key, value) + } + + if test.emptyHost { + req.Host = "" + } + + forwardReq := testhelpers.MustNewRequest(http.MethodGet, "http://foo.bar/path?q=1", nil) + + oldWriteHeader(req, forwardReq, fwdAuth.authRequestHeaders) + + expectedHeaders := test.expectedHeaders + for key, value := range expectedHeaders { + assert.Equal(t, value, forwardReq.Header.Get(key)) + forwardReq.Header.Del(key) + } + if test.checkForUnexpectedHeaders { + for key := range forwardReq.Header { assert.Fail(t, "Unexpected header found", key) } } @@ -936,9 +1162,9 @@ func TestForwardAuthPreserveRequestMethod(t *testing.T) { } } -func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { +func TestForwardAuthMaxResponseBodySize(t *testing.T) { testCases := []struct { - name string + desc string maxResponseBodySize int64 status int body string @@ -946,7 +1172,7 @@ func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { expectedBody string }{ { - name: "auth failure, unlimited response body", + desc: "auth failure, unlimited response body", maxResponseBodySize: -1, status: http.StatusForbidden, body: "Forbidden", @@ -954,7 +1180,7 @@ func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { expectedBody: "Forbidden", }, { - name: "auth failure, response body exceeds the limit", + desc: "auth failure, response body exceeds the limit", maxResponseBodySize: 1, status: http.StatusForbidden, body: "Forbidden", @@ -962,7 +1188,7 @@ func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { expectedBody: "", }, { - name: "auth success within limit", + desc: "auth success within limit", maxResponseBodySize: 100, status: http.StatusOK, body: "ok", @@ -970,7 +1196,7 @@ func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { expectedBody: "traefik\n", }, { - name: "auth success body exceeds limit", + desc: "auth success body exceeds limit", maxResponseBodySize: 1, status: http.StatusOK, body: "large auth response", @@ -980,7 +1206,7 @@ func Test_ForwardAuthMaxResponseBodySize(t *testing.T) { } for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { + t.Run(test.desc, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(test.status) fmt.Fprint(w, test.body) diff --git a/pkg/middlewares/forwardedheaders/forwarded_header.go b/pkg/middlewares/forwardedheaders/forwarded_header.go index 9ba697d57c..a2278d4ca3 100644 --- a/pkg/middlewares/forwardedheaders/forwarded_header.go +++ b/pkg/middlewares/forwardedheaders/forwarded_header.go @@ -13,14 +13,14 @@ import ( ) const ( - xForwardedProto = "X-Forwarded-Proto" - xForwardedFor = "X-Forwarded-For" - xForwardedHost = "X-Forwarded-Host" - xForwardedPort = "X-Forwarded-Port" + XForwardedProto = "X-Forwarded-Proto" + XForwardedFor = "X-Forwarded-For" + XForwardedHost = "X-Forwarded-Host" + XForwardedPort = "X-Forwarded-Port" xForwardedServer = "X-Forwarded-Server" - xForwardedURI = "X-Forwarded-Uri" - xForwardedMethod = "X-Forwarded-Method" - xForwardedPrefix = "X-Forwarded-Prefix" + XForwardedURI = "X-Forwarded-Uri" + XForwardedMethod = "X-Forwarded-Method" + XForwardedPrefix = "X-Forwarded-Prefix" xForwardedTLSClientCert = "X-Forwarded-Tls-Client-Cert" xForwardedTLSClientCertInfo = "X-Forwarded-Tls-Client-Cert-Info" xRealIP = "X-Real-Ip" @@ -28,18 +28,40 @@ const ( upgrade = "Upgrade" ) -var xHeaders = []string{ - xForwardedProto, - xForwardedFor, - xForwardedHost, - xForwardedPort, - xForwardedServer, - xForwardedURI, - xForwardedMethod, - xForwardedPrefix, - xForwardedTLSClientCert, - xForwardedTLSClientCertInfo, - xRealIP, +// XHeadersSet contains the canonical X-headers managed by Traefik. Used by +// isManagedXHeader to detect both the canonical form and underscore variants +// that Go's HTTP server preserves (e.g. X_Forwarded_Proto). +var XHeadersSet = map[string]struct{}{ + XForwardedProto: {}, + XForwardedFor: {}, + XForwardedHost: {}, + XForwardedPort: {}, + xForwardedServer: {}, + XForwardedURI: {}, + XForwardedMethod: {}, + XForwardedPrefix: {}, + xForwardedTLSClientCert: {}, + xForwardedTLSClientCertInfo: {}, + xRealIP: {}, +} + +// isManagedXHeader reports whether key matches one of Traefik's X-headers, +// treating '_' as '-'. Every managed header starts with 'X', so a byte check +// skips most headers without any map work; the underscore branch is only +// reached for the rare attacker-injected variants. +func isManagedXHeader(key string) bool { + if len(key) == 0 || key[0] != 'X' { + return false + } + if _, ok := XHeadersSet[key]; ok { + return true + } + if strings.IndexByte(key, '_') < 0 { + return false + } + canonical := http.CanonicalHeaderKey(strings.ReplaceAll(key, "_", "-")) + _, ok := XHeadersSet[canonical] + return ok } // XForwarded is an HTTP handler wrapper that sets the X-Forwarded headers, @@ -86,6 +108,128 @@ func NewXForwarded(insecure bool, trustedIPs []string, connectionHeaders []strin }, nil } +// ServeHTTP implements http.Handler. +func (x *XForwarded) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if !x.insecure && !x.isTrustedIP(r.RemoteAddr) { + DeleteXForwardedHeaders(r.Header) + } + + x.rewrite(r) + + x.removeConnectionHeaders(r) + + x.next.ServeHTTP(w, r) +} + +func (x *XForwarded) isTrustedIP(ip string) bool { + if x.ipChecker == nil { + return false + } + return x.ipChecker.IsAuthorized(ip) == nil +} + +func (x *XForwarded) rewrite(outreq *http.Request) { + if clientIP, _, err := net.SplitHostPort(outreq.RemoteAddr); err == nil { + clientIP = removeIPv6Zone(clientIP) + + if unsafeHeader(outreq.Header).Get(xRealIP) == "" { + unsafeHeader(outreq.Header).Set(xRealIP, clientIP) + } + } + + xfProto := unsafeHeader(outreq.Header).Get(XForwardedProto) + if xfProto == "" { + // TODO: is this expected to set the X-Forwarded-Proto header value to + // ws(s) as the underlying request used to upgrade the connection is + // made over HTTP(S)? + if isWebsocketRequest(outreq) { + if outreq.TLS != nil { + unsafeHeader(outreq.Header).Set(XForwardedProto, "wss") + } else { + unsafeHeader(outreq.Header).Set(XForwardedProto, "ws") + } + } else { + if outreq.TLS != nil { + unsafeHeader(outreq.Header).Set(XForwardedProto, "https") + } else { + unsafeHeader(outreq.Header).Set(XForwardedProto, "http") + } + } + } + + if xfPort := unsafeHeader(outreq.Header).Get(XForwardedPort); xfPort == "" { + unsafeHeader(outreq.Header).Set(XForwardedPort, forwardedPort(outreq)) + } + + if xfHost := unsafeHeader(outreq.Header).Get(XForwardedHost); xfHost == "" && outreq.Host != "" { + unsafeHeader(outreq.Header).Set(XForwardedHost, outreq.Host) + } + + // Per https://www.rfc-editor.org/rfc/rfc2616#section-4.2, the Forwarded IPs list is in + // the same order as the values in the X-Forwarded-For header(s). + if xffs := unsafeHeader(outreq.Header).Values(XForwardedFor); len(xffs) > 0 { + unsafeHeader(outreq.Header).Set(XForwardedFor, strings.Join(xffs, ", ")) + } + + if x.hostname != "" { + unsafeHeader(outreq.Header).Set(xForwardedServer, x.hostname) + } +} + +func (x *XForwarded) removeConnectionHeaders(req *http.Request) { + var reqUpType string + if httpguts.HeaderValuesContainsToken(req.Header[connection], upgrade) { + reqUpType = unsafeHeader(req.Header).Get(upgrade) + } + + var connectionHopByHopHeaders []string + for _, f := range req.Header[connection] { + for sf := range strings.SplitSeq(f, ",") { + if sf = textproto.TrimString(sf); sf != "" { + key := http.CanonicalHeaderKey(sf) + // Connection header cannot dictate to remove X- headers managed by Traefik, + // as per rfc7230 https://datatracker.ietf.org/doc/html/rfc7230#section-6.1, + // A proxy or gateway MUST ... and then remove the Connection header field itself + // (or replace it with the intermediary's own connection options for the forwarded message). + if isManagedXHeader(key) { + continue + } + + // Keep headers allowed through the middleware chain. + if slices.Contains(x.connectionHeaders, key) { + connectionHopByHopHeaders = append(connectionHopByHopHeaders, key) + continue + } + + // Apply Connection header option. + delete(req.Header, key) + } + } + } + + if reqUpType != "" { + connectionHopByHopHeaders = append(connectionHopByHopHeaders, upgrade) + unsafeHeader(req.Header).Set(upgrade, reqUpType) + } + if len(connectionHopByHopHeaders) > 0 { + unsafeHeader(req.Header).Set(connection, strings.Join(connectionHopByHopHeaders, ",")) + return + } + + unsafeHeader(req.Header).Del(connection) +} + +// DeleteXForwardedHeaders Strip X-Forwarded headers and their underscore variants +// (e.g. X_Forwarded_Proto), which Go's HTTP server preserves +// alongside the canonical dash form. +func DeleteXForwardedHeaders(headers http.Header) { + for key := range headers { + if isManagedXHeader(key) { + delete(headers, key) + } + } +} + // removeIPv6Zone removes the zone if the given IP is an ipv6 address and it has {zone} information in it, // like "[fe80::d806:a55d:eb1b:49cc%vEthernet (vmxnet3 Ethernet Adapter - Virtual Switch)]:64692". func removeIPv6Zone(clientIP string) string { @@ -123,7 +267,7 @@ func forwardedPort(req *http.Request) string { return port } - if unsafeHeader(req.Header).Get(xForwardedProto) == "https" || unsafeHeader(req.Header).Get(xForwardedProto) == "wss" { + if unsafeHeader(req.Header).Get(XForwardedProto) == "https" || unsafeHeader(req.Header).Get(XForwardedProto) == "wss" { return "443" } @@ -134,119 +278,6 @@ func forwardedPort(req *http.Request) string { return "80" } -// ServeHTTP implements http.Handler. -func (x *XForwarded) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if !x.insecure && !x.isTrustedIP(r.RemoteAddr) { - for _, h := range xHeaders { - unsafeHeader(r.Header).Del(h) - } - } - - x.rewrite(r) - - x.removeConnectionHeaders(r) - - x.next.ServeHTTP(w, r) -} - -func (x *XForwarded) isTrustedIP(ip string) bool { - if x.ipChecker == nil { - return false - } - return x.ipChecker.IsAuthorized(ip) == nil -} - -func (x *XForwarded) rewrite(outreq *http.Request) { - if clientIP, _, err := net.SplitHostPort(outreq.RemoteAddr); err == nil { - clientIP = removeIPv6Zone(clientIP) - - if unsafeHeader(outreq.Header).Get(xRealIP) == "" { - unsafeHeader(outreq.Header).Set(xRealIP, clientIP) - } - } - - xfProto := unsafeHeader(outreq.Header).Get(xForwardedProto) - if xfProto == "" { - // TODO: is this expected to set the X-Forwarded-Proto header value to - // ws(s) as the underlying request used to upgrade the connection is - // made over HTTP(S)? - if isWebsocketRequest(outreq) { - if outreq.TLS != nil { - unsafeHeader(outreq.Header).Set(xForwardedProto, "wss") - } else { - unsafeHeader(outreq.Header).Set(xForwardedProto, "ws") - } - } else { - if outreq.TLS != nil { - unsafeHeader(outreq.Header).Set(xForwardedProto, "https") - } else { - unsafeHeader(outreq.Header).Set(xForwardedProto, "http") - } - } - } - - if xfPort := unsafeHeader(outreq.Header).Get(xForwardedPort); xfPort == "" { - unsafeHeader(outreq.Header).Set(xForwardedPort, forwardedPort(outreq)) - } - - if xfHost := unsafeHeader(outreq.Header).Get(xForwardedHost); xfHost == "" && outreq.Host != "" { - unsafeHeader(outreq.Header).Set(xForwardedHost, outreq.Host) - } - - // Per https://www.rfc-editor.org/rfc/rfc2616#section-4.2, the Forwarded IPs list is in - // the same order as the values in the X-Forwarded-For header(s). - if xffs := unsafeHeader(outreq.Header).Values(xForwardedFor); len(xffs) > 0 { - unsafeHeader(outreq.Header).Set(xForwardedFor, strings.Join(xffs, ", ")) - } - - if x.hostname != "" { - unsafeHeader(outreq.Header).Set(xForwardedServer, x.hostname) - } -} - -func (x *XForwarded) removeConnectionHeaders(req *http.Request) { - var reqUpType string - if httpguts.HeaderValuesContainsToken(req.Header[connection], upgrade) { - reqUpType = unsafeHeader(req.Header).Get(upgrade) - } - - var connectionHopByHopHeaders []string - for _, f := range req.Header[connection] { - for sf := range strings.SplitSeq(f, ",") { - if sf = textproto.TrimString(sf); sf != "" { - key := http.CanonicalHeaderKey(sf) - // Connection header cannot dictate to remove X- headers managed by Traefik, - // as per rfc7230 https://datatracker.ietf.org/doc/html/rfc7230#section-6.1, - // A proxy or gateway MUST ... and then remove the Connection header field itself - // (or replace it with the intermediary's own connection options for the forwarded message). - if slices.Contains(xHeaders, key) { - continue - } - - // Keep headers allowed through the middleware chain. - if slices.Contains(x.connectionHeaders, key) { - connectionHopByHopHeaders = append(connectionHopByHopHeaders, key) - continue - } - - // Apply Connection header option. - delete(req.Header, key) - } - } - } - - if reqUpType != "" { - connectionHopByHopHeaders = append(connectionHopByHopHeaders, upgrade) - unsafeHeader(req.Header).Set(upgrade, reqUpType) - } - if len(connectionHopByHopHeaders) > 0 { - unsafeHeader(req.Header).Set(connection, strings.Join(connectionHopByHopHeaders, ",")) - return - } - - unsafeHeader(req.Header).Del(connection) -} - // unsafeHeader allows to manage Header values. // Must be used only when the header name is already a canonical key. type unsafeHeader map[string][]string diff --git a/pkg/middlewares/forwardedheaders/forwarded_header_test.go b/pkg/middlewares/forwardedheaders/forwarded_header_test.go index 505349781c..50b734bf80 100644 --- a/pkg/middlewares/forwardedheaders/forwarded_header_test.go +++ b/pkg/middlewares/forwardedheaders/forwarded_header_test.go @@ -31,9 +31,9 @@ func TestServeHTTP(t *testing.T) { remoteAddr: "", incomingHeaders: map[string][]string{}, expectedHeaders: map[string]string{ - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", }, @@ -44,20 +44,24 @@ func TestServeHTTP(t *testing.T) { trustedIps: nil, remoteAddr: "", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, + "X_forwarded_proto": {"https"}, + "X_forwarded_for": {"10.0.0.1"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "10.0.1.0, 10.0.1.12", - xForwardedURI: "/bar", - xForwardedMethod: "GET", + XForwardedFor: "10.0.1.0, 10.0.1.12", + XForwardedURI: "/bar", + XForwardedMethod: "GET", xForwardedTLSClientCert: "Cert", xForwardedTLSClientCertInfo: "CertInfo", - xForwardedPrefix: "/prefix", + XForwardedPrefix: "/prefix", + "X_forwarded_proto": "https", + "X_forwarded_for": "10.0.0.1", }, }, { @@ -66,20 +70,26 @@ func TestServeHTTP(t *testing.T) { trustedIps: nil, remoteAddr: "", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, + "X_forwarded_proto": {"https"}, + "X_forwarded_for": {"10.0.0.1"}, + "X_forwarded_host": {"evil.example"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", - xForwardedPrefix: "", + XForwardedPrefix: "", + "X_forwarded_proto": "", + "X_forwarded_for": "", + "X_forwarded_host": "", }, }, { @@ -88,20 +98,24 @@ func TestServeHTTP(t *testing.T) { trustedIps: []string{"10.0.1.100"}, remoteAddr: "10.0.1.100:80", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, + "X_forwarded_proto": {"https"}, + "X_forwarded_for": {"10.0.0.1"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "10.0.1.0, 10.0.1.12", - xForwardedURI: "/bar", - xForwardedMethod: "GET", + XForwardedFor: "10.0.1.0, 10.0.1.12", + XForwardedURI: "/bar", + XForwardedMethod: "GET", xForwardedTLSClientCert: "Cert", xForwardedTLSClientCertInfo: "CertInfo", - xForwardedPrefix: "/prefix", + XForwardedPrefix: "/prefix", + "X_forwarded_proto": "https", + "X_forwarded_for": "10.0.0.1", }, }, { @@ -110,20 +124,20 @@ func TestServeHTTP(t *testing.T) { trustedIps: []string{"10.0.1.100"}, remoteAddr: "10.0.1.101:80", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", - xForwardedPrefix: "", + XForwardedPrefix: "", }, }, { @@ -132,20 +146,20 @@ func TestServeHTTP(t *testing.T) { trustedIps: []string{"1.2.3.4/24"}, remoteAddr: "1.2.3.156:80", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "10.0.1.0, 10.0.1.12", - xForwardedURI: "/bar", - xForwardedMethod: "GET", + XForwardedFor: "10.0.1.0, 10.0.1.12", + XForwardedURI: "/bar", + XForwardedMethod: "GET", xForwardedTLSClientCert: "Cert", xForwardedTLSClientCertInfo: "CertInfo", - xForwardedPrefix: "/prefix", + XForwardedPrefix: "/prefix", }, }, { @@ -154,34 +168,34 @@ func TestServeHTTP(t *testing.T) { trustedIps: []string{"1.2.3.4/24"}, remoteAddr: "10.0.1.101:80", incomingHeaders: map[string][]string{ - xForwardedFor: {"10.0.1.0, 10.0.1.12"}, - xForwardedURI: {"/bar"}, - xForwardedMethod: {"GET"}, + XForwardedFor: {"10.0.1.0, 10.0.1.12"}, + XForwardedURI: {"/bar"}, + XForwardedMethod: {"GET"}, xForwardedTLSClientCert: {"Cert"}, xForwardedTLSClientCertInfo: {"CertInfo"}, - xForwardedPrefix: {"/prefix"}, + XForwardedPrefix: {"/prefix"}, }, expectedHeaders: map[string]string{ - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", - xForwardedPrefix: "", + XForwardedPrefix: "", }, }, { desc: "xForwardedFor with multiple header(s) values", insecure: true, incomingHeaders: map[string][]string{ - xForwardedFor: { + XForwardedFor: { "10.0.0.4, 10.0.0.3", "10.0.0.2, 10.0.0.1", "10.0.0.0", }, }, expectedHeaders: map[string]string{ - xForwardedFor: "10.0.0.4, 10.0.0.3, 10.0.0.2, 10.0.0.1, 10.0.0.0", + XForwardedFor: "10.0.0.4, 10.0.0.3, 10.0.0.2, 10.0.0.1, 10.0.0.0", }, }, { @@ -206,14 +220,14 @@ func TestServeHTTP(t *testing.T) { desc: "xForwardedProto with no tls", tls: false, expectedHeaders: map[string]string{ - xForwardedProto: "http", + XForwardedProto: "http", }, }, { desc: "xForwardedProto with tls", tls: true, expectedHeaders: map[string]string{ - xForwardedProto: "https", + XForwardedProto: "https", }, }, { @@ -221,7 +235,7 @@ func TestServeHTTP(t *testing.T) { tls: false, websocket: true, expectedHeaders: map[string]string{ - xForwardedProto: "ws", + XForwardedProto: "ws", }, }, { @@ -229,7 +243,7 @@ func TestServeHTTP(t *testing.T) { tls: true, websocket: true, expectedHeaders: map[string]string{ - xForwardedProto: "wss", + XForwardedProto: "wss", }, }, { @@ -237,17 +251,17 @@ func TestServeHTTP(t *testing.T) { tls: true, websocket: true, incomingHeaders: map[string][]string{ - xForwardedProto: {"wss"}, + XForwardedProto: {"wss"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "wss", + XForwardedProto: "wss", }, }, { desc: "xForwardedPort with explicit port", host: "foo.com:8080", expectedHeaders: map[string]string{ - xForwardedPort: "8080", + XForwardedPort: "8080", }, }, { @@ -255,25 +269,25 @@ func TestServeHTTP(t *testing.T) { // setting insecure just so our initial xForwardedProto does not get cleaned insecure: true, incomingHeaders: map[string][]string{ - xForwardedProto: {"https"}, + XForwardedProto: {"https"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "https", - xForwardedPort: "443", + XForwardedProto: "https", + XForwardedPort: "443", }, }, { desc: "xForwardedPort with implicit tls port from TLS in req", tls: true, expectedHeaders: map[string]string{ - xForwardedPort: "443", + XForwardedPort: "443", }, }, { desc: "xForwardedHost from req host", host: "foo.com:8080", expectedHeaders: map[string]string{ - xForwardedHost: "foo.com:8080", + XForwardedHost: "foo.com:8080", }, }, { @@ -288,39 +302,42 @@ func TestServeHTTP(t *testing.T) { insecure: false, incomingHeaders: map[string][]string{ connection: { - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, + "X_forwarded_proto", }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, + "X_forwarded_proto": {"spoofed"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "http", - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", - xForwardedHost: "", - xForwardedPort: "80", + XForwardedProto: "http", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", + XForwardedHost: "", + XForwardedPort: "80", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", - xForwardedPrefix: "", + XForwardedPrefix: "", xRealIP: "", + "X_forwarded_proto": "", connection: "", }, }, @@ -329,38 +346,38 @@ func TestServeHTTP(t *testing.T) { insecure: true, incomingHeaders: map[string][]string{ connection: { - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "foo", - xForwardedFor: "foo", - xForwardedURI: "foo", - xForwardedMethod: "foo", - xForwardedHost: "foo", - xForwardedPort: "foo", + XForwardedProto: "foo", + XForwardedFor: "foo", + XForwardedURI: "foo", + XForwardedMethod: "foo", + XForwardedHost: "foo", + XForwardedPort: "foo", xForwardedTLSClientCert: "foo", xForwardedTLSClientCertInfo: "foo", - xForwardedPrefix: "foo", + XForwardedPrefix: "foo", xRealIP: "foo", connection: "", }, @@ -369,51 +386,51 @@ func TestServeHTTP(t *testing.T) { desc: "Untrusted and Connection: Connection header has no effect on X- forwarded headers", insecure: false, connectionHeaders: []string{ - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, incomingHeaders: map[string][]string{ connection: { - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "http", - xForwardedFor: "", - xForwardedURI: "", - xForwardedMethod: "", - xForwardedHost: "", - xForwardedPort: "80", + XForwardedProto: "http", + XForwardedFor: "", + XForwardedURI: "", + XForwardedMethod: "", + XForwardedHost: "", + XForwardedPort: "80", xForwardedTLSClientCert: "", xForwardedTLSClientCertInfo: "", - xForwardedPrefix: "", + XForwardedPrefix: "", xRealIP: "", connection: "", }, @@ -422,51 +439,51 @@ func TestServeHTTP(t *testing.T) { desc: "Trusted (insecure) and Connection: Connection header has no effect on X- forwarded headers", insecure: true, connectionHeaders: []string{ - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, incomingHeaders: map[string][]string{ connection: { - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "foo", - xForwardedFor: "foo", - xForwardedURI: "foo", - xForwardedMethod: "foo", - xForwardedHost: "foo", - xForwardedPort: "foo", + XForwardedProto: "foo", + XForwardedFor: "foo", + XForwardedURI: "foo", + XForwardedMethod: "foo", + XForwardedHost: "foo", + XForwardedPort: "foo", xForwardedTLSClientCert: "foo", xForwardedTLSClientCertInfo: "foo", - xForwardedPrefix: "foo", + XForwardedPrefix: "foo", xRealIP: "foo", connection: "", }, @@ -475,51 +492,51 @@ func TestServeHTTP(t *testing.T) { desc: "Trusted (insecure) and Connection: Testing case sensitivity on connection Headers param", insecure: true, connectionHeaders: []string{ - strings.ToLower(xForwardedProto), - strings.ToLower(xForwardedFor), - strings.ToLower(xForwardedURI), - strings.ToLower(xForwardedMethod), - strings.ToLower(xForwardedHost), - strings.ToLower(xForwardedPort), + strings.ToLower(XForwardedProto), + strings.ToLower(XForwardedFor), + strings.ToLower(XForwardedURI), + strings.ToLower(XForwardedMethod), + strings.ToLower(XForwardedHost), + strings.ToLower(XForwardedPort), strings.ToLower(xForwardedTLSClientCert), strings.ToLower(xForwardedTLSClientCertInfo), - strings.ToLower(xForwardedPrefix), + strings.ToLower(XForwardedPrefix), strings.ToLower(xRealIP), }, incomingHeaders: map[string][]string{ connection: { - xForwardedProto, - xForwardedFor, - xForwardedURI, - xForwardedMethod, - xForwardedHost, - xForwardedPort, + XForwardedProto, + XForwardedFor, + XForwardedURI, + XForwardedMethod, + XForwardedHost, + XForwardedPort, xForwardedTLSClientCert, xForwardedTLSClientCertInfo, - xForwardedPrefix, + XForwardedPrefix, xRealIP, }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "foo", - xForwardedFor: "foo", - xForwardedURI: "foo", - xForwardedMethod: "foo", - xForwardedHost: "foo", - xForwardedPort: "foo", + XForwardedProto: "foo", + XForwardedFor: "foo", + XForwardedURI: "foo", + XForwardedMethod: "foo", + XForwardedHost: "foo", + XForwardedPort: "foo", xForwardedTLSClientCert: "foo", xForwardedTLSClientCertInfo: "foo", - xForwardedPrefix: "foo", + XForwardedPrefix: "foo", xRealIP: "foo", connection: "", }, @@ -529,38 +546,38 @@ func TestServeHTTP(t *testing.T) { insecure: true, incomingHeaders: map[string][]string{ connection: { - strings.ToLower(xForwardedProto), - strings.ToLower(xForwardedFor), - strings.ToLower(xForwardedURI), - strings.ToLower(xForwardedMethod), - strings.ToLower(xForwardedHost), - strings.ToLower(xForwardedPort), + strings.ToLower(XForwardedProto), + strings.ToLower(XForwardedFor), + strings.ToLower(XForwardedURI), + strings.ToLower(XForwardedMethod), + strings.ToLower(XForwardedHost), + strings.ToLower(XForwardedPort), strings.ToLower(xForwardedTLSClientCert), strings.ToLower(xForwardedTLSClientCertInfo), - strings.ToLower(xForwardedPrefix), + strings.ToLower(XForwardedPrefix), strings.ToLower(xRealIP), }, - xForwardedProto: {"foo"}, - xForwardedFor: {"foo"}, - xForwardedURI: {"foo"}, - xForwardedMethod: {"foo"}, - xForwardedHost: {"foo"}, - xForwardedPort: {"foo"}, + XForwardedProto: {"foo"}, + XForwardedFor: {"foo"}, + XForwardedURI: {"foo"}, + XForwardedMethod: {"foo"}, + XForwardedHost: {"foo"}, + XForwardedPort: {"foo"}, xForwardedTLSClientCert: {"foo"}, xForwardedTLSClientCertInfo: {"foo"}, - xForwardedPrefix: {"foo"}, + XForwardedPrefix: {"foo"}, xRealIP: {"foo"}, }, expectedHeaders: map[string]string{ - xForwardedProto: "foo", - xForwardedFor: "foo", - xForwardedURI: "foo", - xForwardedMethod: "foo", - xForwardedHost: "foo", - xForwardedPort: "foo", + XForwardedProto: "foo", + XForwardedFor: "foo", + XForwardedURI: "foo", + XForwardedMethod: "foo", + XForwardedHost: "foo", + XForwardedPort: "foo", xForwardedTLSClientCert: "foo", xForwardedTLSClientCertInfo: "foo", - xForwardedPrefix: "foo", + XForwardedPrefix: "foo", xRealIP: "foo", connection: "", }, @@ -583,6 +600,19 @@ func TestServeHTTP(t *testing.T) { "Foo": "bar", }, }, + { + desc: "insecure false preserves non-matching underscore headers", + insecure: false, + remoteAddr: "10.0.1.101:80", + incomingHeaders: map[string][]string{ + "X_custom_header": {"value"}, + "X_forwarded_proto": {"spoofed"}, + }, + expectedHeaders: map[string]string{ + "X_custom_header": "value", + "X_forwarded_proto": "", + }, + }, } for _, test := range testCases { diff --git a/pkg/middlewares/stripprefix/strip_prefix.go b/pkg/middlewares/stripprefix/strip_prefix.go index f4cd8e9d7b..cc8779ce5f 100644 --- a/pkg/middlewares/stripprefix/strip_prefix.go +++ b/pkg/middlewares/stripprefix/strip_prefix.go @@ -36,8 +36,13 @@ func New(ctx context.Context, next http.Handler, config dynamic.StripPrefix, nam // Handle default value (here because of deprecation and the removal of setDefault). forceSlash := config.ForceSlash != nil && *config.ForceSlash + prefixes := make([]string, len(config.Prefixes)) + for i, p := range config.Prefixes { + prefixes[i] = strings.TrimSpace(p) + } + return &stripPrefix{ - prefixes: config.Prefixes, + prefixes: prefixes, next: next, name: name, forceSlash: forceSlash, @@ -55,19 +60,22 @@ func (s *stripPrefix) ServeHTTP(rw http.ResponseWriter, req *http.Request) { if req.URL.RawPath != "" { req.URL.RawPath = s.getRawPathStripped(req.URL.RawPath, prefix) } - s.serveRequest(rw, req, strings.TrimSpace(prefix)) - return + + // Here we are sanitizing the URL when the path is not empty, + // as the JoinPath method is adding a leading slash if the path is empty + // to be aligned with ensureLeadingSlash behavior. + if req.URL.Path != "" { + req.URL = req.URL.JoinPath() + } + + req.Header.Add(ForwardedPrefixHeader, prefix) + req.RequestURI = req.URL.RequestURI() + break } } s.next.ServeHTTP(rw, req) } -func (s *stripPrefix) serveRequest(rw http.ResponseWriter, req *http.Request, prefix string) { - req.Header.Add(ForwardedPrefixHeader, prefix) - req.RequestURI = req.URL.RequestURI() - s.next.ServeHTTP(rw, req) -} - func (s *stripPrefix) getPathStripped(urlPath, prefix string) string { if s.forceSlash { // Only for compatibility reason with the previous behavior, diff --git a/pkg/middlewares/stripprefix/strip_prefix_test.go b/pkg/middlewares/stripprefix/strip_prefix_test.go index 6615b32e32..7078342c30 100644 --- a/pkg/middlewares/stripprefix/strip_prefix_test.go +++ b/pkg/middlewares/stripprefix/strip_prefix_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/traefik/traefik/v3/pkg/config/dynamic" "github.com/traefik/traefik/v3/pkg/testhelpers" + "k8s.io/utils/ptr" ) func TestStripPrefix(t *testing.T) { @@ -141,6 +142,40 @@ func TestStripPrefix(t *testing.T) { expectedRawPath: "/a%2Fb", expectedHeader: "/api/", }, + { + desc: "dot in the path not stripped by the prefix", + config: dynamic.StripPrefix{ + Prefixes: []string{"/api"}, + }, + path: "/api./foo", + expectedStatusCode: http.StatusOK, + expectedPath: "/foo", + expectedRawPath: "", + expectedHeader: "/api", + }, + { + desc: "multiple dots in the path not stripped by the prefix", + config: dynamic.StripPrefix{ + Prefixes: []string{"/api"}, + }, + path: "/api../foo", + expectedStatusCode: http.StatusOK, + expectedPath: "/foo", + expectedRawPath: "", + expectedHeader: "/api", + }, + { + desc: "multiple dots in the path not stripped by the prefix with forceSlash", + config: dynamic.StripPrefix{ + Prefixes: []string{"/api"}, + ForceSlash: ptr.To(true), + }, + path: "/api../foo", + expectedStatusCode: http.StatusOK, + expectedPath: "/foo", + expectedRawPath: "", + expectedHeader: "/api", + }, } for _, test := range testCases { diff --git a/pkg/middlewares/stripprefixregex/strip_prefix_regex.go b/pkg/middlewares/stripprefixregex/strip_prefix_regex.go index 15e34c3538..f1ee622305 100644 --- a/pkg/middlewares/stripprefixregex/strip_prefix_regex.go +++ b/pkg/middlewares/stripprefixregex/strip_prefix_regex.go @@ -62,9 +62,15 @@ func (s *stripPrefixRegex) ServeHTTP(rw http.ResponseWriter, req *http.Request) req.URL.RawPath = ensureLeadingSlash(req.URL.RawPath[encodedPrefixLen(req.URL.RawPath, prefix):]) } + // Here we are sanitizing the URL when the path is not empty, + // as the JoinPath method is adding a leading slash if the path is empty + // to be aligned with ensureLeadingSlash behavior. + if req.URL.Path != "" { + req.URL = req.URL.JoinPath() + } + req.RequestURI = req.URL.RequestURI() - s.next.ServeHTTP(rw, req) - return + break } } diff --git a/pkg/middlewares/stripprefixregex/strip_prefix_regex_test.go b/pkg/middlewares/stripprefixregex/strip_prefix_regex_test.go index 472670e3e0..97bc8269a3 100644 --- a/pkg/middlewares/stripprefixregex/strip_prefix_regex_test.go +++ b/pkg/middlewares/stripprefixregex/strip_prefix_regex_test.go @@ -153,7 +153,7 @@ func TestStripPrefixRegex(t *testing.T) { path: "/b/ap%69/test", expectedStatusCode: http.StatusOK, expectedPath: "/test", - expectedRawPath: "/test", + expectedRawPath: "", expectedRequestURI: "/test", expectedHeader: "/b/api/", }, @@ -197,6 +197,26 @@ func TestStripPrefixRegex(t *testing.T) { expectedRequestURI: "/a%2Fb", expectedHeader: "/t /test", }, + { + desc: "/api./foo", + config: dynamic.StripPrefixRegex{Regex: []string{"/api"}}, + path: "/api./foo", + expectedStatusCode: http.StatusOK, + expectedPath: "/foo", + expectedRawPath: "", + expectedRequestURI: "/foo", + expectedHeader: "/api", + }, + { + desc: "/api../foo", + config: dynamic.StripPrefixRegex{Regex: []string{"/api"}}, + path: "/api../foo", + expectedStatusCode: http.StatusOK, + expectedPath: "/foo", + expectedRawPath: "", + expectedRequestURI: "/foo", + expectedHeader: "/api", + }, } for _, test := range testCases { diff --git a/pkg/provider/kubernetes/crd/fixtures/with_middleware_cross_namespace.yml b/pkg/provider/kubernetes/crd/fixtures/with_middleware_cross_namespace.yml index 3c1e899562..faf9e953fa 100644 --- a/pkg/provider/kubernetes/crd/fixtures/with_middleware_cross_namespace.yml +++ b/pkg/provider/kubernetes/crd/fixtures/with_middleware_cross_namespace.yml @@ -37,6 +37,16 @@ spec: port: 80 middlewares: - name: cross-ns-stripprefix@kubernetescrd + - match: Host(`foo.com`) && PathPrefix(`/chain`) + kind: Rule + priority: 12 + services: + - name: whoami + namespace: default + port: 80 + middlewares: + - name: test-chain + - name: test-chain-cross-provider --- apiVersion: traefik.io/v1alpha1 @@ -50,6 +60,31 @@ spec: prefixes: - /stripit +--- +apiVersion: traefik.io/v1alpha1 +kind: Middleware +metadata: + name: test-chain + namespace: default + +spec: + chain: + middlewares: + - name: stripprefix + namespace: cross-ns + +--- +apiVersion: traefik.io/v1alpha1 +kind: Middleware +metadata: + name: test-chain-cross-provider + namespace: default + +spec: + chain: + middlewares: + - name: other-middleware@kubernetescrd + --- apiVersion: traefik.io/v1alpha1 kind: Middleware diff --git a/pkg/provider/kubernetes/crd/kubernetes.go b/pkg/provider/kubernetes/crd/kubernetes.go index f83a8d1a08..d820c412eb 100644 --- a/pkg/provider/kubernetes/crd/kubernetes.go +++ b/pkg/provider/kubernetes/crd/kubernetes.go @@ -302,13 +302,19 @@ func (p *Provider) loadConfigurationFromCRD(ctx context.Context, client Client) continue } + chain, err := createChainMiddleware(ctxMid, middleware.Namespace, middleware.Spec.Chain, p.AllowCrossNamespace) + if err != nil { + logger.Error().Err(err).Msg("Error while reading chain middleware") + continue + } + conf.HTTP.Middlewares[id] = &dynamic.Middleware{ AddPrefix: middleware.Spec.AddPrefix, StripPrefix: middleware.Spec.StripPrefix, StripPrefixRegex: middleware.Spec.StripPrefixRegex, ReplacePath: middleware.Spec.ReplacePath, ReplacePathRegex: middleware.Spec.ReplacePathRegex, - Chain: createChainMiddleware(ctxMid, middleware.Namespace, middleware.Spec.Chain), + Chain: chain, IPWhiteList: middleware.Spec.IPWhiteList, IPAllowList: middleware.Spec.IPAllowList, Headers: middleware.Spec.Headers, @@ -1194,13 +1200,20 @@ func loadAuthCredentials(secret *corev1.Secret) ([]string, error) { return credentials, nil } -func createChainMiddleware(ctx context.Context, namespace string, chain *traefikv1alpha1.Chain) *dynamic.Chain { +func createChainMiddleware(ctx context.Context, parentNamespace string, chain *traefikv1alpha1.Chain, allowCrossNamespace bool) (*dynamic.Chain, error) { if chain == nil { - return nil + return nil, nil } var mds []string for _, mi := range chain.Middlewares { + if !allowCrossNamespace && strings.HasSuffix(mi.Name, providerNamespaceSeparator+providerName) { + // Since we are not able to know if another namespace is in the name (namespace-name@kubernetescrd), + // if the provider namespace kubernetescrd is used, + // we don't allow this format to avoid cross-namespace references. + return nil, fmt.Errorf("invalid reference to middleware %s: when allowCrossNamespace is disabled @kubernetescrd provider references are disallowed", mi.Name) + } + if strings.Contains(mi.Name, providerNamespaceSeparator) { if len(mi.Namespace) > 0 { log.Ctx(ctx).Warn().Msgf("namespace %q is ignored in cross-provider context", mi.Namespace) @@ -1209,13 +1222,19 @@ func createChainMiddleware(ctx context.Context, namespace string, chain *traefik continue } - ns := mi.Namespace - if len(ns) == 0 { - ns = namespace + ns := parentNamespace + if len(mi.Namespace) > 0 { + if !isNamespaceAllowed(allowCrossNamespace, parentNamespace, mi.Namespace) { + return nil, fmt.Errorf("middleware %s/%s is not in the chain namespace %s", mi.Namespace, mi.Name, parentNamespace) + } + + ns = mi.Namespace } + mds = append(mds, makeID(ns, mi.Name)) } - return &dynamic.Chain{Middlewares: mds} + + return &dynamic.Chain{Middlewares: mds}, nil } func buildTLSOptions(ctx context.Context, client Client) map[string]tls.Options { diff --git a/pkg/provider/kubernetes/crd/kubernetes_http.go b/pkg/provider/kubernetes/crd/kubernetes_http.go index be9490ad67..653fb981b6 100644 --- a/pkg/provider/kubernetes/crd/kubernetes_http.go +++ b/pkg/provider/kubernetes/crd/kubernetes_http.go @@ -178,8 +178,8 @@ func (p *Provider) makeMiddlewareKeys(ctx context.Context, ingRouteNamespace str if !p.AllowCrossNamespace && strings.HasSuffix(mi.Name, providerNamespaceSeparator+providerName) { // Since we are not able to know if another namespace is in the name (namespace-name@kubernetescrd), // if the provider namespace kubernetescrd is used, - // we don't allow this format to avoid cross namespace references. - return nil, fmt.Errorf("invalid reference to middleware %s: with crossnamespace disallowed, the namespace field needs to be explicitly specified", mi.Name) + // we don't allow this format to avoid cross-namespace references. + return nil, fmt.Errorf("invalid reference to middleware %s: when allowCrossNamespace is disabled @kubernetescrd provider references are disallowed", mi.Name) } if strings.Contains(name, providerNamespaceSeparator) { diff --git a/pkg/provider/kubernetes/crd/kubernetes_test.go b/pkg/provider/kubernetes/crd/kubernetes_test.go index 7845f1d4a2..0a2ca954df 100644 --- a/pkg/provider/kubernetes/crd/kubernetes_test.go +++ b/pkg/provider/kubernetes/crd/kubernetes_test.go @@ -6696,6 +6696,16 @@ func TestCrossNamespace(t *testing.T) { Priority: 12, Middlewares: []string{"default-test-errorpage"}, }, + "default-test-crossnamespace-route-4932ffbbcd99474df323": { + EntryPoints: []string{"foo"}, + Service: "default-test-crossnamespace-route-4932ffbbcd99474df323", + Rule: "Host(`foo.com`) && PathPrefix(`/chain`)", + Priority: 12, + Middlewares: []string{ + "default-test-chain", + "default-test-chain-cross-provider", + }, + }, }, Middlewares: map[string]*dynamic.Middleware{ "cross-ns-stripprefix": { @@ -6722,6 +6732,23 @@ func TestCrossNamespace(t *testing.T) { }, }, }, + "default-test-crossnamespace-route-4932ffbbcd99474df323": { + LoadBalancer: &dynamic.ServersLoadBalancer{ + Strategy: dynamic.BalancerStrategyWRR, + Servers: []dynamic.Server{ + { + URL: "http://10.10.0.1:80", + }, + { + URL: "http://10.10.0.2:80", + }, + }, + PassHostHeader: pointer(true), + ResponseForwarding: &dynamic.ResponseForwarding{ + FlushInterval: ptypes.Duration(100 * time.Millisecond), + }, + }, + }, }, ServersTransports: map[string]*dynamic.ServersTransport{}, }, @@ -6768,6 +6795,16 @@ func TestCrossNamespace(t *testing.T) { Priority: 12, Middlewares: []string{"cross-ns-stripprefix@kubernetescrd"}, }, + "default-test-crossnamespace-route-4932ffbbcd99474df323": { + EntryPoints: []string{"foo"}, + Service: "default-test-crossnamespace-route-4932ffbbcd99474df323", + Rule: "Host(`foo.com`) && PathPrefix(`/chain`)", + Priority: 12, + Middlewares: []string{ + "default-test-chain", + "default-test-chain-cross-provider", + }, + }, }, Middlewares: map[string]*dynamic.Middleware{ "cross-ns-stripprefix": { @@ -6782,6 +6819,16 @@ func TestCrossNamespace(t *testing.T) { Query: "/{status}.html", }, }, + "default-test-chain": { + Chain: &dynamic.Chain{ + Middlewares: []string{"cross-ns-stripprefix"}, + }, + }, + "default-test-chain-cross-provider": { + Chain: &dynamic.Chain{ + Middlewares: []string{"other-middleware@kubernetescrd"}, + }, + }, }, Services: map[string]*dynamic.Service{ "default-test-crossnamespace-route-6b204d94623b3df4370c": { @@ -6852,6 +6899,23 @@ func TestCrossNamespace(t *testing.T) { }, }, }, + "default-test-crossnamespace-route-4932ffbbcd99474df323": { + LoadBalancer: &dynamic.ServersLoadBalancer{ + Strategy: dynamic.BalancerStrategyWRR, + Servers: []dynamic.Server{ + { + URL: "http://10.10.0.1:80", + }, + { + URL: "http://10.10.0.2:80", + }, + }, + PassHostHeader: pointer(true), + ResponseForwarding: &dynamic.ResponseForwarding{ + FlushInterval: ptypes.Duration(100 * time.Millisecond), + }, + }, + }, }, ServersTransports: map[string]*dynamic.ServersTransport{}, }, diff --git a/pkg/provider/kubernetes/crd/traefikio/v1alpha1/middleware.go b/pkg/provider/kubernetes/crd/traefikio/v1alpha1/middleware.go index 891f9c1a8e..bffba83a20 100644 --- a/pkg/provider/kubernetes/crd/traefikio/v1alpha1/middleware.go +++ b/pkg/provider/kubernetes/crd/traefikio/v1alpha1/middleware.go @@ -160,7 +160,7 @@ type ForwardAuth struct { // Address defines the authentication server address. Address string `json:"address,omitempty"` // TrustForwardHeader defines whether to trust (ie: forward) all X-Forwarded-* headers. - TrustForwardHeader bool `json:"trustForwardHeader,omitempty"` + TrustForwardHeader *bool `json:"trustForwardHeader,omitempty"` // AuthResponseHeaders defines the list of headers to copy from the authentication server response and set on forwarded request, replacing any existing conflicting headers. AuthResponseHeaders []string `json:"authResponseHeaders,omitempty"` // AuthResponseHeadersRegex defines the regex to match headers to copy from the authentication server response and set on forwarded request, after stripping all headers that match the regex. diff --git a/pkg/provider/kubernetes/crd/traefikio/v1alpha1/zz_generated.deepcopy.go b/pkg/provider/kubernetes/crd/traefikio/v1alpha1/zz_generated.deepcopy.go index 38cbbe9639..54a17c03d1 100644 --- a/pkg/provider/kubernetes/crd/traefikio/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/provider/kubernetes/crd/traefikio/v1alpha1/zz_generated.deepcopy.go @@ -270,6 +270,11 @@ func (in *ErrorPage) DeepCopy() *ErrorPage { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ForwardAuth) DeepCopyInto(out *ForwardAuth) { *out = *in + if in.TrustForwardHeader != nil { + in, out := &in.TrustForwardHeader, &out.TrustForwardHeader + *out = new(bool) + **out = **in + } if in.AuthResponseHeaders != nil { in, out := &in.AuthResponseHeaders, &out.AuthResponseHeaders *out = make([]string, len(*in)) diff --git a/pkg/provider/kv/kv_test.go b/pkg/provider/kv/kv_test.go index 1dd995ab84..6fdc759f00 100644 --- a/pkg/provider/kv/kv_test.go +++ b/pkg/provider/kv/kv_test.go @@ -437,7 +437,7 @@ func Test_buildConfiguration(t *testing.T) { InsecureSkipVerify: true, CAOptional: pointer(true), }, - TrustForwardHeader: true, + TrustForwardHeader: pointer(true), AuthResponseHeaders: []string{ "foobar", "foobar", diff --git a/pkg/redactor/redactor_config_test.go b/pkg/redactor/redactor_config_test.go index 3fc4f344a7..6b79b51eff 100644 --- a/pkg/redactor/redactor_config_test.go +++ b/pkg/redactor/redactor_config_test.go @@ -275,7 +275,7 @@ func init() { Key: "cert.pem", InsecureSkipVerify: true, }, - TrustForwardHeader: true, + TrustForwardHeader: pointer(true), AuthResponseHeaders: []string{"foo"}, AuthResponseHeadersRegex: "foo", AuthRequestHeaders: []string{"foo"}, diff --git a/pkg/server/service/loadbalancer/mirror/mirror_test.go b/pkg/server/service/loadbalancer/mirror/mirror_test.go index 515bafdb77..33282b23c5 100644 --- a/pkg/server/service/loadbalancer/mirror/mirror_test.go +++ b/pkg/server/service/loadbalancer/mirror/mirror_test.go @@ -139,7 +139,7 @@ func TestMirroringWithBody(t *testing.T) { const numMirrors = 10 var ( - countMirror int32 + countMirror atomic.Int32 body = []byte(`body`) ) @@ -161,7 +161,7 @@ func TestMirroringWithBody(t *testing.T) { bb, err := io.ReadAll(r.Body) assert.NoError(t, err) assert.Equal(t, body, bb) - atomic.AddInt32(&countMirror, 1) + countMirror.Add(1) }), 100) assert.NoError(t, err) } @@ -172,7 +172,7 @@ func TestMirroringWithBody(t *testing.T) { pool.Stop() - val := atomic.LoadInt32(&countMirror) + val := countMirror.Load() assert.Equal(t, numMirrors, int(val)) } @@ -180,7 +180,7 @@ func TestMirroringWithIgnoredBody(t *testing.T) { const numMirrors = 10 var ( - countMirror int32 + countMirror atomic.Int32 body = []byte(`body`) emptyBody = []byte(``) ) @@ -203,7 +203,7 @@ func TestMirroringWithIgnoredBody(t *testing.T) { bb, err := io.ReadAll(r.Body) assert.NoError(t, err) assert.Equal(t, emptyBody, bb) - atomic.AddInt32(&countMirror, 1) + countMirror.Add(1) }), 100) assert.NoError(t, err) } @@ -214,7 +214,7 @@ func TestMirroringWithIgnoredBody(t *testing.T) { pool.Stop() - val := atomic.LoadInt32(&countMirror) + val := countMirror.Load() assert.Equal(t, numMirrors, int(val)) }