From 6875e953782076237a0c20facc05eeb5d49aa161 Mon Sep 17 00:00:00 2001 From: "Tim St. Clair" Date: Mon, 22 May 2017 12:22:37 -0700 Subject: [PATCH] Append X-Forwarded-For in proxy handler --- .../k8s.io/apimachinery/pkg/util/net/http.go | 15 +++++++++++ .../apimachinery/pkg/util/net/http_test.go | 26 ++++++++++++++++++ .../apiserver/pkg/endpoints/handlers/proxy.go | 20 +++++++------- .../pkg/registry/generic/rest/proxy.go | 22 +++++++-------- .../kube-aggregator/pkg/apiserver/BUILD | 1 + .../pkg/apiserver/handler_proxy.go | 27 +++++-------------- .../pkg/apiserver/handler_proxy_test.go | 1 + 7 files changed, 69 insertions(+), 43 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go index ce8113ce65..adb80813be 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http.go @@ -209,6 +209,21 @@ func GetClientIP(req *http.Request) net.IP { return ips[0] } +// Prepares the X-Forwarded-For header for another forwarding hop by appending the previous sender's +// IP address to the X-Forwarded-For chain. +func AppendForwardedForHeader(req *http.Request) { + // Copied from net/http/httputil/reverseproxy.go: + if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil { + // If we aren't the first proxy retain prior + // X-Forwarded-For information as a comma+space + // separated list and fold multiple headers into one. + if prior, ok := req.Header["X-Forwarded-For"]; ok { + clientIP = strings.Join(prior, ", ") + ", " + clientIP + } + req.Header.Set("X-Forwarded-For", clientIP) + } +} + var defaultProxyFuncPointer = fmt.Sprintf("%p", http.ProxyFromEnvironment) // isDefault checks to see if the transportProxierFunc is pointing to the default one diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go index 54906c2ab2..2d41eda49d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go @@ -108,6 +108,32 @@ func TestGetClientIP(t *testing.T) { } } +func TestAppendForwardedForHeader(t *testing.T) { + testCases := []struct { + addr, forwarded, expected string + }{ + {"1.2.3.4:8000", "", "1.2.3.4"}, + {"1.2.3.4:8000", "8.8.8.8", "8.8.8.8, 1.2.3.4"}, + {"1.2.3.4:8000", "8.8.8.8, 1.2.3.4", "8.8.8.8, 1.2.3.4, 1.2.3.4"}, + {"1.2.3.4:8000", "foo,bar", "foo,bar, 1.2.3.4"}, + } + for i, test := range testCases { + req := &http.Request{ + RemoteAddr: test.addr, + Header: make(http.Header), + } + if test.forwarded != "" { + req.Header.Set("X-Forwarded-For", test.forwarded) + } + + AppendForwardedForHeader(req) + actual := req.Header.Get("X-Forwarded-For") + if actual != test.expected { + t.Errorf("[%d] Expected %q, Got %q", i, test.expected, actual) + } + } +} + func TestProxierWithNoProxyCIDR(t *testing.T) { testCases := []struct { name string diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go index d4bbc4e053..11853b4eec 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go @@ -17,6 +17,7 @@ limitations under the License. package handlers import ( + "context" "errors" "io" "math/rand" @@ -156,17 +157,10 @@ func (r *ProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { } location.RawQuery = values.Encode() - newReq, err := http.NewRequest(req.Method, location.String(), req.Body) - if err != nil { - httpCode = responsewriters.ErrorNegotiated(ctx, err, r.Serializer, gv, w, req) - return - } - httpCode = http.StatusOK - newReq.Header = req.Header - newReq.ContentLength = req.ContentLength - // Copy the TransferEncoding is for future-proofing. Currently Go only supports "chunked" and - // it can determine the TransferEncoding based on ContentLength and the Body. - newReq.TransferEncoding = req.TransferEncoding + // WithContext creates a shallow clone of the request with the new context. + newReq := req.WithContext(context.Background()) + newReq.Header = net.CloneHeader(req.Header) + newReq.URL = location // TODO convert this entire proxy to an UpgradeAwareProxy similar to // https://github.com/openshift/origin/blob/master/pkg/util/httpproxy/upgradeawareproxy.go. @@ -224,6 +218,10 @@ func (r *ProxyHandler) tryUpgrade(ctx request.Context, w http.ResponseWriter, re if !httpstream.IsUpgradeRequest(req) { return false } + // Only append X-Forwarded-For in the upgrade path, since httputil.NewSingleHostReverseProxy + // handles this in the non-upgrade path. + net.AppendForwardedForHeader(newReq) + backendConn, err := proxyutil.DialURL(location, transport) if err != nil { responsewriters.ErrorNegotiated(ctx, err, r.Serializer, gv, w, req) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go index 8286597c62..e6ef8cb924 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go @@ -17,6 +17,7 @@ limitations under the License. package rest import ( + "context" "fmt" "io" "net" @@ -116,16 +117,10 @@ func (h *UpgradeAwareProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Re h.Transport = h.defaultProxyTransport(req.URL, h.Transport) } - newReq, err := http.NewRequest(req.Method, loc.String(), req.Body) - if err != nil { - h.Responder.Error(err) - return - } - newReq.Header = req.Header - newReq.ContentLength = req.ContentLength - // Copy the TransferEncoding is for future-proofing. Currently Go only supports "chunked" and - // it can determine the TransferEncoding based on ContentLength and the Body. - newReq.TransferEncoding = req.TransferEncoding + // WithContext creates a shallow clone of the request with the new context. + newReq := req.WithContext(context.Background()) + newReq.Header = utilnet.CloneHeader(req.Header) + newReq.URL = &loc proxy := httputil.NewSingleHostReverseProxy(&url.URL{Scheme: h.Location.Scheme, Host: h.Location.Host}) proxy.Transport = h.Transport @@ -145,10 +140,13 @@ func (h *UpgradeAwareProxyHandler) tryUpgrade(w http.ResponseWriter, req *http.R err error ) + clone := utilnet.CloneRequest(req) + // Only append X-Forwarded-For in the upgrade path, since httputil.NewSingleHostReverseProxy + // handles this in the non-upgrade path. + utilnet.AppendForwardedForHeader(clone) if h.InterceptRedirects && utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StreamingProxyRedirects) { - backendConn, rawResponse, err = utilnet.ConnectWithRedirects(req.Method, h.Location, req.Header, req.Body, h) + backendConn, rawResponse, err = utilnet.ConnectWithRedirects(req.Method, h.Location, clone.Header, req.Body, h) } else { - clone := utilnet.CloneRequest(req) clone.URL = h.Location backendConn, err = h.Dial(clone) } diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/BUILD b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/BUILD index 6fef5ff258..26c6515b15 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/BUILD +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/BUILD @@ -54,6 +54,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/httpstream:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go index 08e30a9582..9820a6281f 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go @@ -17,6 +17,7 @@ limitations under the License. package apiserver import ( + "context" "net/http" "net/url" "sync/atomic" @@ -24,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/httpstream" "k8s.io/apimachinery/pkg/util/httpstream/spdy" + utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" genericfeatures "k8s.io/apiserver/pkg/features" @@ -110,21 +112,14 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { location.Path = req.URL.Path location.RawQuery = req.URL.Query().Encode() - // make a new request object with the updated location and the body we already have - newReq, err := http.NewRequest(req.Method, location.String(), req.Body) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - mergeHeader(newReq.Header, req.Header) - newReq.ContentLength = req.ContentLength - // Copy the TransferEncoding is for future-proofing. Currently Go only supports "chunked" and - // it can determine the TransferEncoding based on ContentLength and the Body. - newReq.TransferEncoding = req.TransferEncoding + // WithContext creates a shallow clone of the request with the new context. + newReq := req.WithContext(context.Background()) + newReq.Header = utilnet.CloneHeader(req.Header) + newReq.URL = location upgrade := false // we need to wrap the roundtripper in another roundtripper which will apply the front proxy headers - proxyRoundTripper, upgrade, err = maybeWrapForConnectionUpgrades(handlingInfo.restConfig, proxyRoundTripper, req) + proxyRoundTripper, upgrade, err := maybeWrapForConnectionUpgrades(handlingInfo.restConfig, proxyRoundTripper, req) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -163,14 +158,6 @@ func maybeWrapForConnectionUpgrades(restConfig *restclient.Config, rt http.Round return wrappedRT, true, nil } -func mergeHeader(dst, src http.Header) { - for k, vv := range src { - for _, v := range vv { - dst.Add(k, v) - } - } -} - // responder implements rest.Responder for assisting a connector in writing objects or errors. type responder struct { w http.ResponseWriter diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go index 9c24dd4b02..046e0c9f45 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go @@ -128,6 +128,7 @@ func TestProxyHandler(t *testing.T) { expectedHeaders: map[string][]string{ "X-Forwarded-Proto": {"https"}, "X-Forwarded-Uri": {"/request/path"}, + "X-Forwarded-For": {"127.0.0.1"}, "X-Remote-User": {"username"}, "User-Agent": {"Go-http-client/1.1"}, "Accept-Encoding": {"gzip"},