Append X-Forwarded-For in proxy handler #46126

timstclair · 2017-05-19T19:01:51Z

Append the request sender's IP to the X-Forwarded-For header chain when proxying requests. This is important for audit logging (kubernetes/enhancements#22) in order to capture the client IP (specifically in the case of federation or kube-aggregator).

/cc @liggitt @deads2k @ericchiang @ihmccreery @soltysh

timstclair · 2017-05-19T19:02:50Z

staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go

@@ -78,6 +78,8 @@ func NewUpgradeAwareProxyHandler(location *url.URL, transport http.RoundTripper,

 // ServeHTTP handles the proxy request
 func (h *UpgradeAwareProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+	utilnet.PrepareForwardedFor(req)


Question to reviewers:

Is it safe to modify the request headers here? Or do we need to copy the request first?

It seems like these modifications should be applied to newReq not req. Even if modifying req is currently safe I don't think you can rely on it remaining safe.

timstclair · 2017-05-19T19:03:00Z

/cc @lavalamp

liggitt · 2017-05-19T19:26:48Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

+	}
+
+	// Try the X-Real-Ip header.
+	hdrRealIp := hdr.Get("X-Real-Ip")


this is the first I've heard of this header...

how is it determined and trusted?

It seems odd to both append it to X-Forwarded-For and not strip the header when forwarding. If other proxies did what we're doing here, wouldn't the X-Real-IP value get interleaved in X-Forward-For multiple times? edit: maybe not... we only insert this into X-Forwarded-For when there is no existing X-Forwarded-For header?

which should take priority, the remote IP obtained from GetRemoteIP() or this header?

how is it determined and trusted?

Some proxies set it. I don't think it's very well defined... It can't be trusted. That's why we still append the RemoteAddr IP below.

we only insert this into X-Forwarded-For when there is no existing X-Forwarded-For header?

Right. See above comment.

which should take priority, the remote IP obtained from GetRemoteIP() or this header?

The logic in PrepareForwardedFor is based off the existing logic in GetClientIP, which gives preference to the X-Real-IP header. Note that for audit logging, we're going to log the full X-Forwarded-For chain, since there's nothing stopping a client from adding bogus IPs to the front of the chain, or spoofing X-Real-IP.

I don't believe any originating IP address can be trusted. That having been said X-Real-Ip is meant to be the originating IP. I don't believe its clear whether GetRemoteIP() is the originating IP, the last IP in the proxy chain or something in between. This is exacerbated by systems line HAProxy and NGINX which are configurable to do either. X-Forwarded-For is nice because while its no more trust worthy it should have all but the proxy IP in its list (assuming that no one has improperly mucked with it). Of course it assumes that the last proxy IP should be the result of calling GetRemoteIP().

liggitt · 2017-05-19T19:27:11Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

+	// First check the X-Forwarded-For header for requests via proxy.
+	hdrForwardedFor := hdr.Get("X-Forwarded-For")
+	if hdrForwardedFor != "" {
+		// Append the remote IP if necessary.


we're ignoring X-Real-IP in this case?

Yeah, for the reason you described below. I couldn't find much on the X-Real-IP header, but it looks like most (all?) proxies that set it also set X-Forwarded-For

lavalamp · 2017-05-19T20:20:02Z

staging/src/k8s.io/apimachinery/pkg/util/net/http_test.go

+		{"1.2.3.4:8000", "1.2.3.5", "", "1.2.3.5,1.2.3.4"},
+		{"1.2.3.4:8000", "1.2.3.5", "8.8.8.8", "8.8.8.8,1.2.3.4"},
+		{"1.2.3.4:8000", "1.2.3.5", "8.8.8.8,", "8.8.8.8,1.2.3.4"},
+		{"1.2.3.4:8000", "1.2.3.5", "foo,bar", "foo,bar,1.2.3.4"},


this never tests preexisting multiple IPs in forwarded?

cheftako · 2017-05-19T20:09:26Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

+			}
+			parts = append(parts[:i+1], remoteIP.String())
+			hdr.Set("X-Forwarded-For", strings.Join(parts, ","))
+			return


Returning at the end of the main for loop seems a but clumsy. Can we find a nicer way to strip trailing commas?

cheftako · 2017-05-19T20:23:00Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

+	}
+
+	// Try the X-Real-Ip header.
+	hdrRealIp := hdr.Get("X-Real-Ip")


I don't believe any originating IP address can be trusted. That having been said X-Real-Ip is meant to be the originating IP. I don't believe its clear whether GetRemoteIP() is the originating IP, the last IP in the proxy chain or something in between. This is exacerbated by systems line HAProxy and NGINX which are configurable to do either. X-Forwarded-For is nice because while its no more trust worthy it should have all but the proxy IP in its list (assuming that no one has improperly mucked with it). Of course it assumes that the last proxy IP should be the result of calling GetRemoteIP().

cheftako · 2017-05-19T20:25:38Z

staging/src/k8s.io/apiserver/pkg/registry/generic/rest/proxy.go

@@ -78,6 +78,8 @@ func NewUpgradeAwareProxyHandler(location *url.URL, transport http.RoundTripper,

 // ServeHTTP handles the proxy request
 func (h *UpgradeAwareProxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+	utilnet.PrepareForwardedFor(req)


It seems like these modifications should be applied to newReq not req. Even if modifying req is currently safe I don't think you can rely on it remaining safe.

timstclair · 2017-05-19T21:22:09Z

Thansk, addressed all feedback. PTAL.

timstclair · 2017-05-19T23:21:45Z

So actually it turns out the go's reverse proxy already does this for us (https://golang.org/src/net/http/httputil/reverseproxy.go?s=5526:5923) but we weren't plumbing the client IP through correctly, due to the way we create request copies. This PR needs an overhaul to account for this, I'll ping when it's ready.

timstclair · 2017-05-20T02:02:04Z

Ok, updated. Changes include:

Perform a more complete copy of requests to proxy, so that the client IP is included
Don't append X-Forwarded-For in the non-upgrade path, since go's ReverseProxy does it for us
Convert PrepareForwardedFor to use go's simpler (and more compliant) implementation

I also noticed #46156 while debugging this...

PTAL

soltysh

lgtm

sttts · 2017-05-22T08:43:17Z

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

@@ -189,6 +189,21 @@ func GetClientIP(req *http.Request) net.IP {
 	return net.ParseIP(req.RemoteAddr)
 }

+// 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 PrepareForwardedFor(req *http.Request) {


s/PrepareForwardedFor/AppendRemoteAddrToForwardedFor/

How about AppendForwardedForHeader?

sttts · 2017-05-22T08:51:49Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/proxy.go

@@ -224,6 +218,8 @@ func (r *ProxyHandler) tryUpgrade(w http.ResponseWriter, req, newReq *http.Reque
 	if !httpstream.IsUpgradeRequest(req) {
 		return false
 	}
+	net.PrepareForwardedFor(newReq)


shouldn't this happen when newReq is created above in ServeHTTP? Here it's only the upgrade code path, isn't it? If httputil.NewSingleHostReverseProxy does this already internally, please add comment here and/or before httputil.NewSingleHostReverseProxy.

sttts · 2017-05-22T08:53:10Z

Some nits, otherwise lgtm.

timstclair · 2017-05-22T18:56:37Z

Addressed feedback, rebased & squashed.

timstclair · 2017-05-22T23:37:27Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

timstclair · 2017-05-22T23:37:35Z

@k8s-bot gce etcd3 e2e test this

sttts · 2017-05-23T07:37:26Z

Lgtm. Only the common federation flake is biting us.

sttts · 2017-05-23T14:37:44Z

/lgtm

sttts · 2017-05-23T14:37:54Z

/approve

sttts · 2017-05-23T14:39:05Z

@deads2k approved? (kube-aggregator)

deads2k · 2017-05-23T14:48:21Z

/approve

k8s-github-robot · 2017-05-23T14:51:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts, timstclair

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

timstclair · 2017-05-23T19:40:33Z

Trivial rebase. Reapplying LGTM.

k8s-ci-robot · 2017-05-23T20:45:34Z

@timstclair: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins Bazel Build	b84cabbc6553f43b60fb3c62f3b749bdc66414a5	link	`@k8s-bot bazel test this`
Jenkins verification	b84cabbc6553f43b60fb3c62f3b749bdc66414a5	link	`@k8s-bot verify test this`
Jenkins kops AWS e2e	b84cabbc6553f43b60fb3c62f3b749bdc66414a5	link	`@k8s-bot kops aws e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

timstclair · 2017-05-23T21:37:20Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-github-robot · 2017-05-24T02:43:00Z

Automatic merge from submit-queue (batch tested with PRs 42042, 46139, 46126, 46258, 46312)

timstclair added the release-note-none Denotes a PR that doesn't merit a release note. label May 19, 2017

timstclair assigned sttts and cheftako May 19, 2017

k8s-ci-robot requested review from ikehz, soltysh, liggitt, ericchiang and deads2k May 19, 2017 19:01

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 19, 2017

timstclair commented May 19, 2017

View reviewed changes

k8s-ci-robot requested a review from lavalamp May 19, 2017 19:03

liggitt reviewed May 19, 2017

View reviewed changes

lavalamp reviewed May 19, 2017

View reviewed changes

cheftako reviewed May 19, 2017

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 20, 2017

soltysh approved these changes May 21, 2017

View reviewed changes

sttts reviewed May 22, 2017

View reviewed changes

timstclair force-pushed the forward-ip branch from b84cabb to 617c15a Compare May 22, 2017 18:55

timstclair force-pushed the forward-ip branch from 617c15a to 7a9884f Compare May 22, 2017 19:22

timstclair force-pushed the forward-ip branch from 7a9884f to 84ea657 Compare May 22, 2017 23:55

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017

Append X-Forwarded-For in proxy handler

6875e95

timstclair force-pushed the forward-ip branch from 84ea657 to 6875e95 Compare May 23, 2017 19:40

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017

timstclair added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2017

timstclair added this to the v1.7 milestone May 24, 2017

k8s-github-robot merged commit 2b1b7f9 into kubernetes:master May 24, 2017

Append X-Forwarded-For in proxy handler #46126

Append X-Forwarded-For in proxy handler #46126

Conversation

timstclair commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair commented May 19, 2017

liggitt May 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair commented May 19, 2017

timstclair commented May 19, 2017

timstclair commented May 20, 2017

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented May 22, 2017

timstclair commented May 22, 2017

timstclair commented May 22, 2017

timstclair commented May 22, 2017

sttts commented May 23, 2017

sttts commented May 23, 2017

sttts commented May 23, 2017

sttts commented May 23, 2017

deads2k commented May 23, 2017

k8s-github-robot commented May 23, 2017

timstclair commented May 23, 2017

k8s-ci-robot commented May 23, 2017 • edited

timstclair commented May 23, 2017

k8s-github-robot commented May 24, 2017

liggitt May 19, 2017 •

edited

k8s-ci-robot commented May 23, 2017 •

edited