diff --git a/lb/gateway.go b/lb/gateway.go index b047af5b..d04e46f2 100644 --- a/lb/gateway.go +++ b/lb/gateway.go @@ -47,6 +47,42 @@ const ( pathPrefix = "PathPrefix" ) +// forwardingHeaders are the request headers the proxy sets itself, from the connection: who the client is and what it +// asked for, over what. No route filter may change them, or whoever can write a route to a backend could tell that +// backend any client address. +var forwardingHeaders = []string{"Forwarded", "X-Forwarded-For", "X-Forwarded-Host", "X-Forwarded-Proto", "X-Real-IP"} + +// forwardingHeader is the forwarding header a name denotes, if it denotes one (header names are case-insensitive). +func forwardingHeader(name string) (string, bool) { + for _, h := range forwardingHeaders { + if strings.EqualFold(name, h) { + return h, true + } + } + return "", false +} + +// touchesForwarding is why a request header filter cannot be served: it sets, adds or removes a forwarding header. +func touchesForwarding(f *HTTPHeaderFilter) string { + if f == nil { + return "" + } + var names []string + for _, h := range f.Set { + names = append(names, h.Name) + } + for _, h := range f.Add { + names = append(names, h.Name) + } + names = append(names, f.Remove...) + for _, n := range names { + if h, ok := forwardingHeader(n); ok { + return fmt.Sprintf("a request header filter may not change %s: the gateway sets it from the connection", h) + } + } + return "" +} + // GatewayInput is what the API shows that the gateway policy reads. type GatewayInput struct { Classes []*GatewayClass @@ -638,7 +674,11 @@ func unsupportedHTTP(r *HTTPRoute) string { okFilter := func(fs []HTTPRouteFilter) string { for _, f := range fs { switch f.Type { - case filterRequestHeaderModifier, filterResponseHeaderModifier, filterRequestRedirect, filterURLRewrite: + case filterRequestHeaderModifier: + if msg := touchesForwarding(f.RequestHeaderModifier); msg != "" { + return msg + } + case filterResponseHeaderModifier, filterRequestRedirect, filterURLRewrite: default: return fmt.Sprintf("filter %s is not supported", f.Type) } diff --git a/lb/gateway_proxy.go b/lb/gateway_proxy.go index d21889eb..34b7ed95 100644 --- a/lb/gateway_proxy.go +++ b/lb/gateway_proxy.go @@ -293,11 +293,6 @@ func (p *Proxy) serve(s *portServer, w http.ResponseWriter, r *http.Request) { Rewrite: func(pr *httputil.ProxyRequest) { pr.SetURL(&url.URL{Scheme: "http", Host: ep.String()}) pr.Out.Host = pr.In.Host // forwarded unmodified unless a filter says otherwise - // SetXForwarded replaces whatever the client sent: only this hop's view is trusted - pr.SetXForwarded() - if ip, _, err := net.SplitHostPort(pr.In.RemoteAddr); err == nil { - pr.Out.Header.Set("X-Real-IP", ip) - } for _, f := range filters { switch { case f.Type == filterRequestHeaderModifier && f.RequestHeaderModifier != nil: @@ -312,6 +307,16 @@ func (p *Proxy) serve(s *portServer, w http.ResponseWriter, r *http.Request) { } } } + // After the filters, so none can change these (the model already refuses routes that try): only this + // hop's view of the connection is passed on. Cleared first, as SetXForwarded appends to an + // X-Forwarded-For already there. + for _, h := range forwardingHeaders { + pr.Out.Header.Del(h) + } + pr.SetXForwarded() + if ip, _, err := net.SplitHostPort(pr.In.RemoteAddr); err == nil { + pr.Out.Header.Set("X-Real-IP", ip) + } }, ModifyResponse: func(resp *http.Response) error { for _, f := range filters { diff --git a/lb/gateway_proxy_test.go b/lb/gateway_proxy_test.go index 20d63796..cad00dbc 100644 --- a/lb/gateway_proxy_test.go +++ b/lb/gateway_proxy_test.go @@ -53,7 +53,7 @@ func named(t *testing.T, name string) (*httptest.Server, Endpoint) { w.Header().Set("X-Backend", name) n, _ := io.Copy(io.Discard, r.Body) fmt.Fprintf(w, "%s path=%s host=%s xff=%s proto=%s xhost=%s realip=%s added=%s removed=%s body=%d", - name, r.URL.Path, r.Host, r.Header.Get("X-Forwarded-For"), r.Header.Get("X-Forwarded-Proto"), + name, r.URL.Path, r.Host, strings.Join(r.Header.Values("X-Forwarded-For"), ","), r.Header.Get("X-Forwarded-Proto"), r.Header.Get("X-Forwarded-Host"), r.Header.Get("X-Real-IP"), strings.Join(r.Header.Values("X-Added"), ","), r.Header.Get("X-Remove-Me"), n) })) @@ -258,6 +258,25 @@ func TestProxyRedirectsAndForwards(t *testing.T) { } } +// The model refuses routes whose filters touch the forwarding headers; the proxy holds the line anyway. +func TestProxyForwardingHeadersSurviveFilters(t *testing.T) { + tp := newTestProxy(t) + _, a := named(t, "A") + routes := []ServeRoute{{Key: "ns/r", Rules: []ServeRule{{Matches: []HTTPRouteMatch{prefix("/")}, Backends: backend("A", a), + Filters: []HTTPRouteFilter{{Type: filterRequestHeaderModifier, RequestHeaderModifier: &HTTPHeaderFilter{ + Set: []HTTPHeader{{Name: "X-Real-IP", Value: "6.6.6.6"}, {Name: "x-forwarded-host", Value: "evil.example"}}, + Add: []HTTPHeader{{Name: "X-Forwarded-For", Value: "6.6.6.6"}}, + Remove: []string{"X-Forwarded-Proto"}, + }}}}}}} + tp.Apply([]ServeListener{{Gateway: "ns/gw", Name: "http", Address: net.ParseIP("192.0.2.1"), Port: 80, Routes: routes}}) + _, body, _ := get(t, "http://"+tp.at(80)+"/", "git.levk.dev", map[string]string{"X-Forwarded-For": "7.7.7.7"}) + for _, want := range []string{"xff=127.0.0.1 ", "proto=http ", "xhost=git.levk.dev ", "realip=127.0.0.1 "} { + if !strings.Contains(body, want) { + t.Fatalf("a filter changed the forwarding headers: %q (want %q)", body, want) + } + } +} + func TestProxyAnswersWhatItCannotRoute(t *testing.T) { tp := newTestProxy(t) routes := []ServeRoute{{Key: "ns/r", Rules: []ServeRule{ diff --git a/lb/gateway_test.go b/lb/gateway_test.go index e1176ef1..767ec79d 100644 --- a/lb/gateway_test.go +++ b/lb/gateway_test.go @@ -397,6 +397,18 @@ func TestRouteAttachment(t *testing.T) { "regular expression path": {func(in *GatewayInput) { in.HTTPRoutes[0].Spec.Rules[0].Matches = []HTTPRouteMatch{{Path: &HTTPPathMatch{Type: ptr("RegularExpression"), Value: ptr("/.*")}}} }, "gitea", "UnsupportedValue"}, + "filter sets X-Forwarded-For": {func(in *GatewayInput) { + in.HTTPRoutes[0].Spec.Rules[0].Filters = []HTTPRouteFilter{{Type: filterRequestHeaderModifier, + RequestHeaderModifier: &HTTPHeaderFilter{Set: []HTTPHeader{{Name: "X-Forwarded-For", Value: "6.6.6.6"}}}}} + }, "gitea", "UnsupportedValue"}, + "backend filter adds x-real-ip": {func(in *GatewayInput) { + in.HTTPRoutes[0].Spec.Rules[0].BackendRefs[0].Filters = []HTTPRouteFilter{{Type: filterRequestHeaderModifier, + RequestHeaderModifier: &HTTPHeaderFilter{Add: []HTTPHeader{{Name: "x-real-ip", Value: "6.6.6.6"}}}}} + }, "gitea", "UnsupportedValue"}, + "filter removes Forwarded": {func(in *GatewayInput) { + in.HTTPRoutes[0].Spec.Rules[0].Filters = []HTTPRouteFilter{{Type: filterRequestHeaderModifier, + RequestHeaderModifier: &HTTPHeaderFilter{Remove: []string{"FORWARDED"}}}} + }, "gitea", "UnsupportedValue"}, "wrong port": {func(in *GatewayInput) { in.TCPRoutes[0].Spec.ParentRefs[0].Port = ptr(int32(2222)) }, "gitea-ssh", "NoMatchingParent"}, } for name, c := range cases { @@ -428,6 +440,29 @@ func TestRouteAttachment(t *testing.T) { } } +func TestForwardingHeadersAreTheGateways(t *testing.T) { + accepted := func(name string, value string) metav1.Condition { + in := giteaInput(t) + in.HTTPRoutes[0].Spec.Rules[0].Filters = []HTTPRouteFilter{{Type: filterRequestHeaderModifier, + RequestHeaderModifier: &HTTPHeaderFilter{Set: []HTTPHeader{{Name: name, Value: value}}}}} + for _, r := range ComputeGateways("cp", gwNodes(), in).Routes { + if r.Name == "gitea" { + return condOf(r.Status.Parents[0].Conditions, condAccepted) + } + } + t.Fatalf("no status for route gitea") + return metav1.Condition{} + } + a := accepted("x-forwarded-proto", "https") + if a.Status != metav1.ConditionFalse || a.Reason != "UnsupportedValue" || !strings.Contains(a.Message, "X-Forwarded-Proto") { + t.Fatalf("a filter on X-Forwarded-Proto: %+v", a) + } + // a header that only looks like one of them is the route's to set + if a := accepted("X-Forwarded-Prefix", "/git"); a.Status != metav1.ConditionTrue { + t.Fatalf("a filter on X-Forwarded-Prefix: %+v", a) + } +} + func TestBackendNotFoundBreaksTheRule(t *testing.T) { in := giteaInput(t) in.HTTPRoutes[0].Spec.Rules[0].BackendRefs[0].Name = "nope" diff --git a/reweave/test/gateway/gateway-test.yaml b/reweave/test/gateway/gateway-test.yaml index f39e4cb2..2369b33b 100644 --- a/reweave/test/gateway/gateway-test.yaml +++ b/reweave/test/gateway/gateway-test.yaml @@ -93,6 +93,20 @@ spec: - matches: [{path: {type: PathPrefix, value: /broken}}] backendRefs: [{name: missing, port: 80}] --- +# refused: the gateway sets the forwarding headers itself, so no route may change them +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: {name: forge, namespace: gwtest} +spec: + parentRefs: [{name: gwtest, namespace: gateway-system, sectionName: https}] + hostnames: [gw.test] + rules: + - matches: [{path: {type: PathPrefix, value: /forge}}] + filters: + - type: RequestHeaderModifier + requestHeaderModifier: {set: [{name: X-Forwarded-For, value: 6.6.6.6}]} + backendRefs: [{name: echo-a, port: 80}] +--- apiVersion: gateway.networking.k8s.io/v1 kind: HTTPRoute metadata: {name: redirect, namespace: gwtest} diff --git a/reweave/test/gateway/run.sh b/reweave/test/gateway/run.sh index 25a1ad84..feb48d4d 100755 --- a/reweave/test/gateway/run.sh +++ b/reweave/test/gateway/run.sh @@ -7,7 +7,8 @@ # The machine running this script is the outside client. Asserts, in order: # G0 GatewayClass reweave is accepted # G1 the Gateway is accepted and programmed at ADDR, its listeners with their routes attached -# G2 route statuses: accepted and resolved, the broken one ResolvedRefs False (BackendNotFound) +# G2 route statuses: accepted and resolved, the broken one ResolvedRefs False (BackendNotFound), and the one whose +# filter sets X-Forwarded-For refused (Accepted False, UnsupportedValue) # G3 TCP listener: forwarded in the kernel to a pod on another node, which sees this client's own address # G4 HTTPS from outside: the listener's certificate by SNI, HTTP/2, routing by exact path, by prefix with a # rewrite, and by default; the backend sees this client (the same address as G3) in X-Forwarded-For @@ -84,7 +85,8 @@ for r in "httproute main" "httproute redirect" "tcproute raw"; do route_is $r ResolvedRefs "True/ResolvedRefs" || fail "G2: $r resolved: $(route $r ResolvedRefs)" done route_is httproute broken ResolvedRefs "False/BackendNotFound" || fail "G2: broken: $(route httproute broken ResolvedRefs)" -ok "G2. routes accepted and resolved; the broken one ResolvedRefs False (BackendNotFound)" +eventually 30 route_is httproute forge Accepted "False/UnsupportedValue" || fail "G2: forge: $(route httproute forge Accepted)" +ok "G2. routes accepted and resolved; the broken one ResolvedRefs False (BackendNotFound); the forging one refused" tcp_hostname() { [ "$(curl -s --max-time 5 "http://$ADDR:18022/hostname")" = echo-b ]; } eventually 30 tcp_hostname || fail "G3: TCP listener did not reach echo-b"