From 4b02e029b5d26118a2470adea32e11c8ce50bce8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20L=C3=A1zni=C4=8Dka?= Date: Thu, 14 Nov 2024 11:20:29 +0100 Subject: [PATCH] reverse proxy: use Rewrite instead of Director This is to mitigate Golang's behavior with `ReverseProxy.Director` when, after the outgoing request is mutated, any headers specified by "Connection" would get rmoved from the mutated request. `ReverseProxy.Rewrite()` is only called once the headers specified in "Connection" were already removed from the outgoing-to-be request. --- cmd/kube-rbac-proxy/app/kube-rbac-proxy.go | 63 +++++-- .../app/kube-rbac-proxy_test.go | 162 ++++++++++++++++++ 2 files changed, 208 insertions(+), 17 deletions(-) create mode 100644 cmd/kube-rbac-proxy/app/kube-rbac-proxy_test.go diff --git a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go index 514d9ba0a..a6754eca8 100644 --- a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go +++ b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go @@ -212,23 +212,7 @@ func Run(cfg *server.KubeRBACProxyConfig) error { return fmt.Errorf("failed to setup an authorizer: %v", err) } - proxy := httputil.NewSingleHostReverseProxy(cfg.KubeRBACProxyInfo.UpstreamURL) - proxy.Transport = cfg.KubeRBACProxyInfo.UpstreamTransport - - if cfg.KubeRBACProxyInfo.UpstreamForceH2C { - // Force http/2 for connections to the upstream i.e. do not start with HTTP1.1 UPGRADE req to - // initialize http/2 session. - // See https://github.com/golang/go/issues/14141#issuecomment-219212895 for more context - proxy.Transport = &http2.Transport{ - // Allow http schema. This doesn't automatically disable TLS - AllowHTTP: true, - // Do disable TLS. - // In combination with the schema check above. We could enforce h2c against the upstream server - DialTLS: func(netw, addr string, cfg *tls.Config) (net.Conn, error) { - return net.Dial(netw, addr) - }, - } - } + proxy := setupProxyHandler(cfg.KubeRBACProxyInfo) handler := identityheaders.WithAuthHeaders(proxy, cfg.KubeRBACProxyInfo.UpstreamHeaders) handler = kubefilters.WithAuthorization(handler, authz, scheme.Codecs) @@ -354,3 +338,48 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco return rewritingAuthorizer, nil } + +func setupProxyHandler(cfg *server.KubeRBACProxyInfo) http.Handler { + proxy := &httputil.ReverseProxy{ + Rewrite: func(pr *httputil.ProxyRequest) { + target := cfg.UpstreamURL + pr.SetURL(target) + pr.Out.Host = target.Host + copyHeaderIfSet(pr.In, pr.Out, "X-Forwarded-For") + pr.SetXForwarded() + }, + } + proxy.Transport = cfg.UpstreamTransport + + if cfg.UpstreamForceH2C { + // Force http/2 for connections to the upstream i.e. do not start with HTTP1.1 UPGRADE req to + // initialize http/2 session. + // See https://github.com/golang/go/issues/14141#issuecomment-219212895 for more context + proxy.Transport = &http2.Transport{ + // Allow http schema. This doesn't automatically disable TLS + AllowHTTP: true, + // Do disable TLS. + // In combination with the schema check above. We could enforce h2c against the upstream server + DialTLS: func(netw, addr string, cfg *tls.Config) (net.Conn, error) { + return net.Dial(netw, addr) + }, + } + } + + return proxy +} + +func copyHeaderIfSet(inReq *http.Request, outReq *http.Request, headerKey string) { + src := inReq.Header.Values(headerKey) + if src == nil { + return + } + + if outReq.Header == nil { + outReq.Header = http.Header{} + } + + for _, v := range src { + outReq.Header.Add(headerKey, v) + } +} diff --git a/cmd/kube-rbac-proxy/app/kube-rbac-proxy_test.go b/cmd/kube-rbac-proxy/app/kube-rbac-proxy_test.go new file mode 100644 index 000000000..943c52123 --- /dev/null +++ b/cmd/kube-rbac-proxy/app/kube-rbac-proxy_test.go @@ -0,0 +1,162 @@ +/* +Copyright 2022 the kube-rbac-proxy maintainers. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package app + +import ( + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "testing" + "time" + + "github.com/brancz/kube-rbac-proxy/pkg/server" + "github.com/google/go-cmp/cmp" +) + +func Test_copyHeaderIfSet(t *testing.T) { + tests := []struct { + name string + headerKey string + inHeader http.Header + outHeader http.Header + expectedValues []string + }{ + { + name: "src exists, dist does not", + headerKey: "NONCanon", + inHeader: http.Header{ + "Noncanon": []string{"here"}, + }, + expectedValues: []string{"here"}, + }, + { + name: "src exists, dist does too", + headerKey: "NONCanon", + inHeader: http.Header{ + "Noncanon": []string{"here"}, + }, + outHeader: http.Header{ + "Noncanon": []string{"there"}, + }, + expectedValues: []string{"there", "here"}, + }, + { + name: "src does not exist, dist does", + headerKey: "nonCanon", + outHeader: http.Header{ + "Noncanon": []string{"there"}, + }, + expectedValues: []string{"there"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + inReq := http.Request{ + Header: tt.inHeader, + } + outReq := http.Request{ + Header: tt.outHeader, + } + + copyHeaderIfSet(&inReq, &outReq, tt.headerKey) + if gotVals := outReq.Header.Values(tt.headerKey); !reflect.DeepEqual(tt.expectedValues, gotVals) { + t.Errorf("expected values: %v, got: %v", tt.expectedValues, gotVals) + } + }) + } +} + +func TestProxyHandler(t *testing.T) { + reqChan := make(chan http.Header, 1) + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + reqChan <- req.Header + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(testServer.Close) + + testServerURL, err := url.Parse(testServer.URL) + if err != nil { + t.Fatalf("failed to parse testserver URL") + } + + config := &server.KubeRBACProxyInfo{ + UpstreamURL: testServerURL, + } + testHandler := setupProxyHandler(config) + + // the Golang implementation of an HTTP server passes the remote address of an + // incoming connection into the HTTP request, we'll emulate this in the tests + const ( + testRemoteIP = "10.0.0.1" + testRemoteAddr = testRemoteIP + ":10354" + ) + + tests := []struct { + name string + header http.Header + wantHeader http.Header + }{ + { + name: "no extra headers", + header: make(http.Header), + wantHeader: http.Header{ + "X-Forwarded-For": []string{testRemoteIP}, + "X-Forwarded-Host": []string{testServerURL.Host}, + "X-Forwarded-Proto": []string{"http"}, + }, + }, + { + name: "X-Forwarded-For is set", + header: http.Header{ + "X-Forwarded-For": []string{"10.0.0.2"}, + }, + wantHeader: http.Header{ + "X-Forwarded-For": []string{"10.0.0.2, " + testRemoteIP}, + "X-Forwarded-Host": []string{testServerURL.Host}, + "X-Forwarded-Proto": []string{"http"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testWriter := httptest.NewRecorder() + req, err := http.NewRequest(http.MethodGet, testServer.URL, nil) + if err != nil { + t.Fatalf("failed to create an http request: %v", err) + } + req.Header = tt.header + req.RemoteAddr = testRemoteAddr + testHandler.ServeHTTP(testWriter, req) + + var gotHeaders http.Header + select { + case gotHeaders = <-reqChan: + case <-time.After(5 * time.Second): + t.Fatal("timeout: did not receive any response") + } + + gotHeaders.Del("Content-Length") + gotHeaders.Del("Accept-Encoding") + gotHeaders.Del("Date") + + if !reflect.DeepEqual(gotHeaders, tt.wantHeader) { + t.Errorf("got different headers than expected: %s", cmp.Diff(tt.wantHeader, gotHeaders)) + } + }) + } +}