Add verification to apiserver redirect following #66516

tallclair · 2018-07-23T20:23:38Z

What this PR does / why we need it:
Only allow the apiserver to follow redirects to the same host.

Release note:

Restrict redirect following from the apiserver to same-host redirects, and ignore redirects in some cases.

tallclair · 2018-07-23T20:24:13Z

/sig api-machinery

liggitt · 2018-07-23T20:29:24Z

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

@@ -343,7 +343,7 @@ func ConnectWithRedirects(originalMethod string, originalLocation *url.URL, head

 redirectLoop:
 	for redirects := 0; ; redirects++ {
-		if redirects > maxRedirects {
+		if redirects >= maxRedirects {


first request isn't a redirect, so I think > is correct... maxRedirects=1 would let you go through the loop twice

It should fail on the 10th redirect, not allow 10 redirects, at least to be consistent with http.Client. I changed this because the test caught this inconsistency.

I would expect maxRedirects=10 to allow 10 and fail on 11. I don't think it really matters in practice either way as long as we don't set maxRedirects to 1

I'd like to keep this consistent with the default http redirect checker, so I reverted the logic but changed maxRedirects to 9. (Also fixed the spdy test with the new max)

liggitt · 2018-07-23T20:30:06Z

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

@@ -404,6 +404,11 @@ redirectLoop:
 		if err != nil {
 			return nil, nil, fmt.Errorf("malformed Location header: %v", err)
 		}
+
+		// Only allow redirects to the same host.
+		if location.Host != originalLocation.Host {


compare scheme as well?

I considered that, but I think at least http -> https should be allowed. And if an attacker controls the endpoint enough to send an https redirect response, then they don't gain anything from a downgrade attack.

Makes sense

liggitt · 2018-07-23T20:30:52Z

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

+	}
+	// Only allow redirects to the same host.
+	newHost := req.URL.Host
+	originalHost := via[0].URL.Host


is len(via) always greater than 0?

Yes, a redirect always has an original request. See https://github.com/golang/go/blob/214f7ec554888042a7a54fdca9cba19aee8ebaf1/src/net/http/client.go#L551-L581

tallclair · 2018-07-23T22:12:04Z

@mrunalp - For CRI-o
@Random-Liu - For containerd

This will break setups where the apiserver sends the initial request on one IP address to the node, and the CRI streaming response sends a redirect to a different interface (e.g. internal vs. external address). This isn't an issue with dockershim, because it always uses a relative redirect. Unfortunately that doesn't appear to be the case with CRI-o (not sure about contianerd). How is CRI-o and contianerd streaming typically configured?

tallclair · 2018-07-23T22:12:22Z

@kubernetes/sig-node-pr-reviews

liggitt · 2018-07-23T23:24:40Z

This will break setups where the apiserver sends the initial request on one IP address to the node, and the CRI streaming response sends a redirect to a different interface (e.g. internal vs. external address)

Or external DNS name vs external IP, etc

tallclair · 2018-07-24T23:40:23Z

OK, updated this PR with a few changes:

Allow same-host-different-port redirects. This is required for out-of-process CRIs, which need to listen on a different port than the Kubelet.
Blanket disable redirects in the LocationStreamer. This streamer is only used by the log pod subresource, which is served by the Kubelet and should never redirect.
Add a feature gate to enable same-host redirect validation, defaulted to disabled. I think it's too risky to push this by default, at least for past releases, for the reasons discussed above (CRI could respond with a different hostname / interface). I think most clusters should be able to safely enable this, so we can consider making it the default for 1.12.

@liggitt PTAL

tallclair · 2018-07-24T23:42:16Z

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

 	//
 	// StreamingProxyRedirects controls whether the apiserver should intercept (and follow)
 	// redirects from the backend (Kubelet) for streaming requests (exec/attach/port-forward).
 	StreamingProxyRedirects utilfeature.Feature = "StreamingProxyRedirects"

+	// owner: @tallclair
+	// alpha: v1.9


BTW - I put 1.9 here because I think we should cherrypick this back to 1.9

philips · 2018-07-25T17:38:52Z

/lgtm

I would suggest a comment on why log never can redirect around here: https://github.com/kubernetes/kubernetes/pull/66516/files#diff-02939fae98eaa89403de3ea191772176R89

tallclair · 2018-07-30T18:35:47Z

Add comment to LocationStreamer about redirect change
Change LocationStreamer to forward the redirect back to the client, rather than outright failing
Added release note.

fedebongio · 2018-07-30T22:07:18Z

/cc @roycaihw

philips · 2018-07-30T22:12:15Z

/lgtm

roycaihw · 2018-07-31T00:34:11Z

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

+		Location: loc,
+	}
+	_, _, _, err = streamer.InputStream("", "")
+	assert.Equal(t, http.ErrUseLastResponse, err, "Expected error from redirect")


If CheckRedirect returns ErrUseLastResponse, then the most recent response is returned with its body unclosed, along with a nil error: https://github.com/golang/go/blob/214f7ec554888042a7a54fdca9cba19aee8ebaf1/src/net/http/client.go#L583-L588

Oops, thanks. On closer inspection, forwarding the redirect doesn't make sense here since only the request body is returned. I reverted the change back to outright rejecting redirects.

tallclair · 2018-09-21T01:56:49Z

Finally got back around to this. PTAL.

cjcullen · 2018-09-21T17:25:07Z

/lgtm

tallclair · 2018-09-21T23:48:25Z

Fixed breakage caused by rebase & squashed commits.

philips · 2018-09-22T00:20:24Z

/lgtm

(very brief scroll through because of amount of review so far)

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

lavalamp · 2018-09-25T17:44:31Z

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

+
+		// Only follow redirects to the same host. Otherwise, propogate the redirect response back.
+		if validateRedirects && location.Hostname() != originalLocation.Hostname() {
+			break redirectLoop


Shouldn't you be returning an error?

No, this returns the redirect response back to the client. See the discussion on this comment thread: #66516 (comment)

Do you think code calling this function is prepared to get a redirection response?

(not to argue with @liggitt's comment-like in that thread, but... given what you told me in person I think it's probably right to go ahead and return the error, and just explicitly say that hostname to IP redirections (or vice versa) are not supported even if they happen to refer to the same thing.)

If the CRI returned a different internal host address, then returning that to the client would be meaningless, and an error response would make more sense. However, if the pod returns a redirect to an external service (trivial example: a pod that just returns a redirect response to a different external web address), then the expected behavior would be that the client can follow that redirect.

Do you think code calling this function is prepared to get a redirection response?

Looking at all the call sites, yes - it shouldn't be a problem. Also, this method only handles 302 redirects, so all other redirects were already being returned to the client.

OK, sounds great :)

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

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

tallclair

Thanks @lavalamp. PTAL

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

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

tallclair · 2018-09-25T19:47:11Z

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

+
+		// Only follow redirects to the same host. Otherwise, propogate the redirect response back.
+		if validateRedirects && location.Hostname() != originalLocation.Hostname() {
+			break redirectLoop


No, this returns the redirect response back to the client. See the discussion on this comment thread: #66516 (comment)

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

lavalamp · 2018-09-25T20:27:27Z

please squash, thanks!

tallclair · 2018-09-26T00:19:12Z

Squashed. Thanks!

lavalamp · 2018-09-26T16:52:58Z

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

@@ -28,6 +28,8 @@ import (
 	"reflect"
 	"testing"

+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"


not going to ask for another change at this point but technically 3rd party deps get their own section...

lavalamp · 2018-09-26T16:53:12Z

/approve
/lgtm

k8s-ci-robot · 2018-09-26T16:53:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, lavalamp, philips, rphillips, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [lavalamp,tallclair]
~~pkg/registry/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-09-26T19:21:50Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

…516-upstream-release-1.12-1539730092 Automated cherry-pick of #66516 to v1.12

tallclair assigned liggitt Jul 23, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 23, 2018

k8s-ci-robot requested review from caesarxuchao and saad-ali July 23, 2018 20:23

tallclair added the kind/bug Categorizes issue or PR as related to a bug. label Jul 23, 2018

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 23, 2018

liggitt reviewed Jul 23, 2018

View reviewed changes

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 23, 2018

tallclair commented Jul 24, 2018

View reviewed changes

k8s-ci-robot assigned philips Jul 25, 2018

k8s-ci-robot requested a review from roycaihw July 30, 2018 22:07

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2018

roycaihw reviewed Jul 31, 2018

View reviewed changes

k8s-ci-robot added the area/kubelet label Sep 21, 2018

tallclair force-pushed the redirect branch from dbb45de to b82fad3 Compare September 21, 2018 01:56

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2018

k8s-ci-robot assigned cjcullen Sep 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2018

tallclair force-pushed the redirect branch from b82fad3 to 302e57c Compare September 21, 2018 23:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2018

lavalamp reviewed Sep 25, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2018

tallclair commented Sep 25, 2018

View reviewed changes

tallclair force-pushed the redirect branch from 6a0b29d to f4065af Compare September 26, 2018 00:18

Only allow apiserver to follow redriects to the same host

4ee9f00

tallclair force-pushed the redirect branch from f4065af to 4ee9f00 Compare September 26, 2018 00:19

lavalamp reviewed Sep 26, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2018

k8s-ci-robot merged commit 109b67c into kubernetes:master Sep 26, 2018

k8s-ci-robot added a commit that referenced this pull request Oct 22, 2018

Merge pull request #69942 from tallclair/automated-cherry-pick-of-#66…

687c04c

…516-upstream-release-1.12-1539730092 Automated cherry-pick of #66516 to v1.12

tallclair mentioned this pull request Dec 3, 2019

CVE-2018-1002102: Unvalidated redirect #85867

Closed

DmitriyLewen mentioned this pull request Oct 2, 2023

Update upstream/CVE-2018-1002102.json aquasecurity/vuln-list-k8s#62

Merged

Add verification to apiserver redirect following #66516

Add verification to apiserver redirect following #66516

Conversation

tallclair commented Jul 23, 2018 • edited Loading

tallclair commented Jul 23, 2018 • edited Loading

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

tallclair commented Jul 23, 2018

tallclair commented Jul 23, 2018

liggitt commented Jul 23, 2018

tallclair commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philips commented Jul 25, 2018

tallclair commented Jul 30, 2018

fedebongio commented Jul 30, 2018

philips commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Sep 21, 2018

cjcullen commented Sep 21, 2018

tallclair commented Sep 21, 2018

philips commented Sep 22, 2018

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

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Sep 25, 2018

tallclair commented Sep 26, 2018

Choose a reason for hiding this comment

lavalamp commented Sep 26, 2018

k8s-ci-robot commented Sep 26, 2018

fejta-bot commented Sep 26, 2018

tallclair commented Jul 23, 2018 •

edited

Loading

tallclair commented Jul 23, 2018 •

edited

Loading