Cleanup Connection headers before passing the middleware chain

Co-authored-by: Romain <rtribotte@users.noreply.github.com>

pkg/middlewares/auth/connectionheader.go

@@ -1,4 +1,4 @@
-package connectionheader
+package auth
 
 import (
 	"net/http"

pkg/middlewares/auth/connectionheader_test.go

@@ -1,4 +1,4 @@
-package connectionheader
+package auth
 
 import (
 	"net/http"

pkg/middlewares/auth/forward.go

@@ -15,7 +15,6 @@ import (
 	"github.com/traefik/traefik/v2/pkg/config/dynamic"
 	"github.com/traefik/traefik/v2/pkg/log"
 	"github.com/traefik/traefik/v2/pkg/middlewares"
-	"github.com/traefik/traefik/v2/pkg/middlewares/connectionheader"
 	"github.com/traefik/traefik/v2/pkg/tracing"
 	"github.com/vulcand/oxy/v2/forward"
 	"github.com/vulcand/oxy/v2/utils"
@@ -90,7 +89,7 @@ func NewForward(ctx context.Context, next http.Handler, config dynamic.ForwardAu
 		fa.authResponseHeadersRegex = re
 	}
 
-	return connectionheader.Remover(fa), nil
+	return Remover(fa), nil
 }
 
 func (fa *forwardAuth) GetTracingInformation() (string, ext.SpanKindEnum) {

pkg/middlewares/forwardedheaders/forwarded_header.go

@@ -3,10 +3,13 @@ package forwardedheaders
 import (
 	"net"
 	"net/http"
+	"net/textproto"
 	"os"
+	"slices"
 	"strings"
 
 	"github.com/traefik/traefik/v2/pkg/ip"
+	"golang.org/x/net/http/httpguts"
 )
 
 const (
@@ -42,19 +45,20 @@ var xHeaders = []string{
 // Unless insecure is set,
 // it first removes all the existing values for those headers if the remote address is not one of the trusted ones.
 type XForwarded struct {
-	insecure   bool
-	trustedIps []string
-	ipChecker  *ip.Checker
-	next       http.Handler
-	hostname   string
+	insecure          bool
+	trustedIPs        []string
+	connectionHeaders []string
+	ipChecker         *ip.Checker
+	next              http.Handler
+	hostname          string
 }
 
 // NewXForwarded creates a new XForwarded.
-func NewXForwarded(insecure bool, trustedIps []string, next http.Handler) (*XForwarded, error) {
+func NewXForwarded(insecure bool, trustedIPs []string, connectionHeaders []string, next http.Handler) (*XForwarded, error) {
 	var ipChecker *ip.Checker
-	if len(trustedIps) > 0 {
+	if len(trustedIPs) > 0 {
 		var err error
-		ipChecker, err = ip.NewChecker(trustedIps)
+		ipChecker, err = ip.NewChecker(trustedIPs)
 		if err != nil {
 			return nil, err
 		}
@@ -66,11 +70,12 @@ func NewXForwarded(insecure bool, trustedIps []string, next http.Handler) (*XFor
 	}
 
 	return &XForwarded{
-		insecure:   insecure,
-		trustedIps: trustedIps,
-		ipChecker:  ipChecker,
-		next:       next,
-		hostname:   hostname,
+		insecure:          insecure,
+		trustedIPs:        trustedIPs,
+		connectionHeaders: connectionHeaders,
+		ipChecker:         ipChecker,
+		next:              next,
+		hostname:          hostname,
 	}, nil
 }
 
@@ -189,9 +194,53 @@ func (x *XForwarded) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	x.rewrite(r)
 
+	x.removeConnectionHeaders(r)
+
 	x.next.ServeHTTP(w, r)
 }
 
+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.Split(f, ",") {
+			if sf = textproto.TrimString(sf); 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, sf) {
+					continue
+				}
+
+				// Keep headers allowed through the middleware chain.
+				if slices.Contains(x.connectionHeaders, sf) {
+					connectionHopByHopHeaders = append(connectionHopByHopHeaders, sf)
+					continue
+				}
+
+				// Apply Connection header option.
+				req.Header.Del(sf)
+			}
+		}
+	}
+
+	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

pkg/middlewares/forwardedheaders/forwarded_header_test.go

@@ -12,15 +12,16 @@ import (
 
 func TestServeHTTP(t *testing.T) {
 	testCases := []struct {
-		desc            string
-		insecure        bool
-		trustedIps      []string
-		incomingHeaders map[string][]string
-		remoteAddr      string
-		expectedHeaders map[string]string
-		tls             bool
-		websocket       bool
-		host            string
+		desc              string
+		insecure          bool
+		trustedIps        []string
+		connectionHeaders []string
+		incomingHeaders   map[string][]string
+		remoteAddr        string
+		expectedHeaders   map[string]string
+		tls               bool
+		websocket         bool
+		host              string
 	}{
 		{
 			desc:            "all Empty",
@@ -269,6 +270,196 @@ func TestServeHTTP(t *testing.T) {
 				xForwardedServer: "foo.com:8080",
 			},
 		},
+		{
+			desc:     "Untrusted: Connection header has no effect on X- forwarded headers",
+			insecure: false,
+			incomingHeaders: map[string][]string{
+				connection: {
+					xForwardedProto,
+					xForwardedFor,
+					xForwardedURI,
+					xForwardedMethod,
+					xForwardedHost,
+					xForwardedPort,
+					xForwardedTLSClientCert,
+					xForwardedTLSClientCertInfo,
+					xRealIP,
+				},
+				xForwardedProto:             {"foo"},
+				xForwardedFor:               {"foo"},
+				xForwardedURI:               {"foo"},
+				xForwardedMethod:            {"foo"},
+				xForwardedHost:              {"foo"},
+				xForwardedPort:              {"foo"},
+				xForwardedTLSClientCert:     {"foo"},
+				xForwardedTLSClientCertInfo: {"foo"},
+				xRealIP:                     {"foo"},
+			},
+			expectedHeaders: map[string]string{
+				xForwardedProto:             "http",
+				xForwardedFor:               "",
+				xForwardedURI:               "",
+				xForwardedMethod:            "",
+				xForwardedHost:              "",
+				xForwardedPort:              "80",
+				xForwardedTLSClientCert:     "",
+				xForwardedTLSClientCertInfo: "",
+				xRealIP:                     "",
+				connection:                  "",
+			},
+		},
+		{
+			desc:     "Trusted (insecure): Connection header has no effect on X- forwarded headers",
+			insecure: true,
+			incomingHeaders: map[string][]string{
+				connection: {
+					xForwardedProto,
+					xForwardedFor,
+					xForwardedURI,
+					xForwardedMethod,
+					xForwardedHost,
+					xForwardedPort,
+					xForwardedTLSClientCert,
+					xForwardedTLSClientCertInfo,
+					xRealIP,
+				},
+				xForwardedProto:             {"foo"},
+				xForwardedFor:               {"foo"},
+				xForwardedURI:               {"foo"},
+				xForwardedMethod:            {"foo"},
+				xForwardedHost:              {"foo"},
+				xForwardedPort:              {"foo"},
+				xForwardedTLSClientCert:     {"foo"},
+				xForwardedTLSClientCertInfo: {"foo"},
+				xRealIP:                     {"foo"},
+			},
+			expectedHeaders: map[string]string{
+				xForwardedProto:             "foo",
+				xForwardedFor:               "foo",
+				xForwardedURI:               "foo",
+				xForwardedMethod:            "foo",
+				xForwardedHost:              "foo",
+				xForwardedPort:              "foo",
+				xForwardedTLSClientCert:     "foo",
+				xForwardedTLSClientCertInfo: "foo",
+				xRealIP:                     "foo",
+				connection:                  "",
+			},
+		},
+		{
+			desc:     "Untrusted and Connection: Connection header has no effect on X- forwarded headers",
+			insecure: false,
+			connectionHeaders: []string{
+				xForwardedProto,
+				xForwardedFor,
+				xForwardedURI,
+				xForwardedMethod,
+				xForwardedHost,
+				xForwardedPort,
+				xForwardedTLSClientCert,
+				xForwardedTLSClientCertInfo,
+				xRealIP,
+			},
+			incomingHeaders: map[string][]string{
+				connection: {
+					xForwardedProto,
+					xForwardedFor,
+					xForwardedURI,
+					xForwardedMethod,
+					xForwardedHost,
+					xForwardedPort,
+					xForwardedTLSClientCert,
+					xForwardedTLSClientCertInfo,
+					xRealIP,
+				},
+				xForwardedProto:             {"foo"},
+				xForwardedFor:               {"foo"},
+				xForwardedURI:               {"foo"},
+				xForwardedMethod:            {"foo"},
+				xForwardedHost:              {"foo"},
+				xForwardedPort:              {"foo"},
+				xForwardedTLSClientCert:     {"foo"},
+				xForwardedTLSClientCertInfo: {"foo"},
+				xRealIP:                     {"foo"},
+			},
+			expectedHeaders: map[string]string{
+				xForwardedProto:             "http",
+				xForwardedFor:               "",
+				xForwardedURI:               "",
+				xForwardedMethod:            "",
+				xForwardedHost:              "",
+				xForwardedPort:              "80",
+				xForwardedTLSClientCert:     "",
+				xForwardedTLSClientCertInfo: "",
+				xRealIP:                     "",
+				connection:                  "",
+			},
+		},
+		{
+			desc:     "Trusted (insecure) and Connection: Connection header has no effect on X- forwarded headers",
+			insecure: true,
+			connectionHeaders: []string{
+				xForwardedProto,
+				xForwardedFor,
+				xForwardedURI,
+				xForwardedMethod,
+				xForwardedHost,
+				xForwardedPort,
+				xForwardedTLSClientCert,
+				xForwardedTLSClientCertInfo,
+				xRealIP,
+			},
+			incomingHeaders: map[string][]string{
+				connection: {
+					xForwardedProto,
+					xForwardedFor,
+					xForwardedURI,
+					xForwardedMethod,
+					xForwardedHost,
+					xForwardedPort,
+					xForwardedTLSClientCert,
+					xForwardedTLSClientCertInfo,
+					xRealIP,
+				},
+				xForwardedProto:             {"foo"},
+				xForwardedFor:               {"foo"},
+				xForwardedURI:               {"foo"},
+				xForwardedMethod:            {"foo"},
+				xForwardedHost:              {"foo"},
+				xForwardedPort:              {"foo"},
+				xForwardedTLSClientCert:     {"foo"},
+				xForwardedTLSClientCertInfo: {"foo"},
+				xRealIP:                     {"foo"},
+			},
+			expectedHeaders: map[string]string{
+				xForwardedProto:             "foo",
+				xForwardedFor:               "foo",
+				xForwardedURI:               "foo",
+				xForwardedMethod:            "foo",
+				xForwardedHost:              "foo",
+				xForwardedPort:              "foo",
+				xForwardedTLSClientCert:     "foo",
+				xForwardedTLSClientCertInfo: "foo",
+				xRealIP:                     "foo",
+				connection:                  "",
+			},
+		},
+		{
+			desc: "Connection: one remove, and one passthrough header",
+			connectionHeaders: []string{
+				"foo",
+			},
+			incomingHeaders: map[string][]string{
+				connection: {
+					"foo",
+				},
+				"Foo": {"bar"},
+				"Bar": {"foo"},
+			},
+			expectedHeaders: map[string]string{
+				"Bar": "foo",
+			},
+		},
 	}
 
 	for _, test := range testCases {
@@ -299,7 +490,7 @@ func TestServeHTTP(t *testing.T) {
 				}
 			}
 
-			m, err := NewXForwarded(test.insecure, test.trustedIps,
+			m, err := NewXForwarded(test.insecure, test.trustedIps, test.connectionHeaders,
 				http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
 			require.NoError(t, err)
 
@@ -382,3 +573,74 @@ func Test_isWebsocketRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestConnection(t *testing.T) {
+	testCases := []struct {
+		desc              string
+		reqHeaders        map[string]string
+		connectionHeaders []string
+		expected          http.Header
+	}{
+		{
+			desc: "simple remove",
+			reqHeaders: map[string]string{
+				"Foo":      "bar",
+				connection: "foo",
+			},
+			expected: http.Header{},
+		},
+		{
+			desc: "remove and upgrade",
+			reqHeaders: map[string]string{
+				upgrade:    "test",
+				"Foo":      "bar",
+				connection: "upgrade,foo",
+			},
+			expected: http.Header{
+				upgrade:    []string{"test"},
+				connection: []string{"Upgrade"},
+			},
+		},
+		{
+			desc: "no remove",
+			reqHeaders: map[string]string{
+				"Foo":      "bar",
+				connection: "fii",
+			},
+			expected: http.Header{
+				"Foo": []string{"bar"},
+			},
+		},
+		{
+			desc: "no remove because connection header pass through",
+			reqHeaders: map[string]string{
+				"Foo":      "bar",
+				connection: "Foo",
+			},
+			connectionHeaders: []string{"Foo"},
+			expected: http.Header{
+				"Foo":      []string{"bar"},
+				connection: []string{"Foo"},
+			},
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			forwarded, err := NewXForwarded(true, nil, test.connectionHeaders, nil)
+			require.NoError(t, err)
+
+			req := httptest.NewRequest(http.MethodGet, "https://localhost", nil)
+
+			for k, v := range test.reqHeaders {
+				req.Header.Set(k, v)
+			}
+
+			forwarded.removeConnectionHeaders(req)
+
+			assert.Equal(t, test.expected, req.Header)
+		})
+	}
+}

pkg/middlewares/headers/headers.go

@@ -10,7 +10,6 @@ import (
 	"github.com/traefik/traefik/v2/pkg/config/dynamic"
 	"github.com/traefik/traefik/v2/pkg/log"
 	"github.com/traefik/traefik/v2/pkg/middlewares"
-	"github.com/traefik/traefik/v2/pkg/middlewares/connectionheader"
 	"github.com/traefik/traefik/v2/pkg/tracing"
 )
 
@@ -69,12 +68,11 @@ func New(ctx context.Context, next http.Handler, cfg dynamic.Headers, name strin
 
 	if hasCustomHeaders || hasCorsHeaders {
 		logger.Debugf("Setting up customHeaders/Cors from %v", cfg)
-		h, err := NewHeader(nextHandler, cfg)
+		var err error
+		handler, err = NewHeader(nextHandler, cfg)
 		if err != nil {
 			return nil, err
 		}
-
-		handler = connectionheader.Remover(h)
 	}
 
 	return &headers{