KEP: hardened exec requests #1899

tallclair · 2020-07-15T23:53:41Z

Harden exec requests against SSRF vulnerabilities. See kubernetes/kubernetes#92914 for motivation.

/sig node api-machinery
/area security

k8s-ci-robot · 2020-07-15T23:53:44Z

@tallclair: The label(s) area/security cannot be applied, because the repository doesn't have them

In response to this:

Harden exec requests against SSRF vulnerabilities. See kubernetes/kubernetes#92914 for motivation.

For #1898.

/sig node api-machinery
/area security

/cc @liggitt @cjcullen @mtaufen

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.

sftim · 2020-07-23T01:45:22Z

keps/sig-node/1898-hardened-exec/README.md

+The second path format includes the pod's UID, and is never called by the kube-apiserver. We should
+remove these endpoints as they're unused, to reduce code complexity.


Aside: if you wanted to preserve the UID checking behavior, it might be feasible to make up a conditional header If-UID: and fail requests where that's set and not the same as the running Pod's UID.

I don't think it's used anywhere. It would be easy enough to add this approach later if a use case arose.

sftim · 2020-07-23T02:03:19Z

keps/sig-node/1898-hardened-exec/README.md

+endpoints](#2-require-a--request-to-the-streaming-endpoints) and [4. Require the request options to
+be provided in the request body](#4-require-the-request-options-to-be-provided-in-the-request-body).
+
+#### 3. Require the websocket protocol for GET requests.


https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#websockets-and-spdy says:

Clients should use the SPDY protocols if their clients have native support, or WebSockets as a fallback.
…
In the future, an HTTP/2 implementation will be exposed that deprecates SPDY.

Is that still accurate?

I'm not clear on why some clients (browsers?) aren't able to use POST + WebSocket - it'd be nice to have that clarified.

correct. browsers don't allow controlling the http method when opening a websocket

I think I get it. If I'm right, the challenge here is that the client sends a single request to the API server that is both an upgrade request and request for a command execution. But I'm not sure I've understood, even after re-reading several times.

I haven't looked at the API here because I don't think there's documentation on how this works. The upgrade to WebSocket includes the exec request in its query string. So the options could include:

Retain current behavior because it's not vulnerable to this attack (?)

POST requesting an exec, API server returns a WebSocket URI, client send a (GET) Upgrade: to open that WebSocket, continue conversation over WebSocket

Client requests a WebSocket, upgrade happens, and then within the WebSocket stream request an exec

I couldn't be sure I've understood what this section is calling for. With that in mind, would you be willing @tallclair to reword this to clarify?

It is vulnerable to attack, but by locking it down to only websocket requests, it makes it harder to exploit (especially in environments where websockets aren't used).

This is similar to the approach we previously took for the apiserver <-> CRI communications. I like the idea, but it's also a breaking change. We could skip the caching layer that we used in the CRI implementation by just signing the request parameters (i.e. POST request to get the signed query params, follow up with the GET including the signature)

I'm not familiar enough with the websocket protocol to know how complicated this would be. It's certainly feasible, but as it's also a breaking change I'm not sure how it compares with Role-based access control (RBAC) #2

I'll update the KEP with these alternatives.

Updated with these options.

What does a browser do if it is redirected from an https://... endpoint to a wss://... endpoint? Does it open a websocket request?

GET (and other HTTP verbs) don't have any meaning for WebSockets, so I think the browser will error out.
(untested)

I haven't tested it, but the redirected request would be missing the required headers for the opening handshake, so I assume it wouldn't work. https://tools.ietf.org/html/rfc6455#section-1.3

Thinking about this more, I have a strong preference for "alternative 1" (hash of request as a subprotocol). Alternative 2 (exec token request) would require managing a signing key and option 3 (make request over the websocket connection) would be a much larger change. Since headers can't be modified on a redirect, I think the hash approach provides sufficient protection with minimal changes on the client & server side. We just need a well-defined serialized input for the hash function. Then the question is whether we want to tie this to the HardenedExecRequests feature gate.

knight42 · 2020-07-27T12:58:47Z

@tallclair I just noticed this KEP when I am drafting #1908. Would it help if we make kubectl not follow redirects by default?

tallclair · 2020-07-27T21:09:15Z

Yes, I think these 2 approaches are complementary, and we should pursue both.

tallclair · 2020-09-16T19:01:42Z

/assign @liggitt

derekwaynecarr · 2020-09-16T20:36:52Z

/assign

I won’t be able to read in depth until next week.

keps/sig-node/1898-hardened-exec/README.md

liggitt

a few initial questions

keps/sig-node/1898-hardened-exec/README.md

liggitt · 2020-09-23T14:56:12Z

keps/sig-node/1898-hardened-exec/README.md

+
+- If only body options are provided, use those.
+- If only query options are provided, use those.
+- If both body and query options are provided, require that they be identical.


does this match current behavior?

The current behavior completely ignores the request body. Only the query parameters are used.

liggitt · 2020-09-23T15:00:23Z

keps/sig-node/1898-hardened-exec/README.md

+endpoints](#2-require-a--request-to-the-streaming-endpoints) and [4. Require the request options to
+be provided in the request body](#4-require-the-request-options-to-be-provided-in-the-request-body).
+
+#### 3. Require the websocket protocol for GET requests.


What does a browser do if it is redirected from an https://... endpoint to a wss://... endpoint? Does it open a websocket request?

mlbiam · 2020-09-28T17:51:11Z

A few thoughts:

Agree that there shouldn't be any redirects followed.
I really like the idea of using a POST with any API call to initiate the request, even if it's breaking. The current method of using a combination of request parameters and headers is really painful to implement. I spent days looking through the code and test cases to try to decipher it.

i can setup a quick test to answer @liggitt question about what browsers will do on a redirect from a GET to wss://. My guess is it will just follow it.

tallclair · 2020-09-28T18:16:28Z

Thanks @mlbiam !

i can setup a quick test to answer @liggitt question about what browsers will do on a redirect from a GET to wss://. My guess is it will just follow it.

If you get a chance to test this, that would be great! I think a redirect from a https:// URL to a wss:// URL will be rejected because it doesn't have the correct headers, but I'm guessing a redirect from wss:// to wss:// will work.

mlbiam · 2020-09-28T18:20:51Z

@tallclair

I think a redirect from a https:// URL to a wss:// URL will be rejected because it doesn't have the correct headers

That brings up a really good question. Does it matter if a redirect works? A redirect from a GET to wss:// would only ever happen from a js library inside of a browser, right? just a simple 302 from the url bar wouldn't do much to your point.

tallclair · 2020-09-28T18:24:03Z

If the redirect works, it's more important that we get redirect protection on websocket requests. If the redirect doesn't work, we might be able to treat them as their own thing.

derekwaynecarr

I think the open questions are addressable after the alpha phase, and this has sufficient detail to move forward.

/approve
/lgtm

derekwaynecarr · 2020-09-30T18:50:04Z

keps/sig-node/1898-hardened-exec/README.md

+### Non-Goals
+
+- Protecting proxy requests
+- Maintaining backwards compatibility of the Kubelet API, which is only intended for consumption by


I agree with this goal, my fear is that it may not have been honored ;-)

derekwaynecarr · 2020-09-30T18:55:42Z

keps/sig-node/1898-hardened-exec/README.md

+
+The changes to the Kubelet API are:
+
+1. Remove the `/run` endpoint, which is unused in core Kubernetes.


for the history buffs, i think this links back to 2016 when we moved away from it.
kubernetes/kubernetes#24921

derekwaynecarr · 2020-09-30T18:57:57Z

keps/sig-node/1898-hardened-exec/README.md

+The changes to the Kubelet API are:
+
+1. Remove the `/run` endpoint, which is unused in core Kubernetes.
+2. Require a `POST` request for the `/exec`, `/attach` and `/portForward` endpoints.


I agree that POST is also semantically better because a GET should not have changed system state.

derekwaynecarr · 2020-09-30T19:02:27Z

keps/sig-node/1898-hardened-exec/README.md

+request. This endpoint is not used by any Kubernetes components, and should be removed to reduce the
+attack surface.
+
+**Risk:** Although this endpoint is not exposed in the Kubernetes API, it is still reachable via a


I am inclined to just remove it. We gave no version guarantee on the endpoint.

k8s-ci-robot · 2020-09-30T19:21:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, 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:

~~keps/sig-node/OWNERS~~ [derekwaynecarr]

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

liggitt · 2020-10-01T18:00:36Z

2. I really like the idea of using a POST with any API call to initiate the request, even if it's breaking

If that comment refers to the kubelet API, we can do that assuming the kube-apiserver is the only client.

If that comment refers to the kube-apiserver API, then requiring a POST breaks existing browser websocket clients with no recourse.

k8s-ci-robot requested review from cjcullen, liggitt and mtaufen July 15, 2020 23:53

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jul 15, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 15, 2020

tallclair added this to the v1.20 milestone Jul 15, 2020

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Jul 15, 2020

tallclair mentioned this pull request Jul 15, 2020

Allow kubectl (and client-go and other client libs) to not follow redirects kubernetes/kubernetes#93129

Closed

KEP: hardened exec requests

8414610

tallclair force-pushed the hardened-exec branch from 7ab2b2b to 8414610 Compare July 16, 2020 00:06

Fix metadata

9a2239f

sftim reviewed Jul 23, 2020

View reviewed changes

k8s-ci-robot assigned liggitt Sep 16, 2020

k8s-ci-robot assigned derekwaynecarr Sep 16, 2020

tallclair added 2 commits September 17, 2020 11:35

Test plan & websocket alternatives

959d199

Fix typo

45ba01b

tallclair mentioned this pull request Sep 17, 2020

Harden exec requests against SSRF #1898

Closed

13 tasks

sftim reviewed Sep 23, 2020

View reviewed changes

keps/sig-node/1898-hardened-exec/README.md Outdated Show resolved Hide resolved

liggitt reviewed Sep 23, 2020

View reviewed changes

Address review feedback

ef4a0b3

tallclair mentioned this pull request Sep 28, 2020

Support kubectl exec/attach using Websocket kubernetes/kubernetes#89163

Closed

derekwaynecarr approved these changes Sep 30, 2020

View reviewed changes

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

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

k8s-ci-robot merged commit 40f5ac0 into kubernetes:master Sep 30, 2020

github-actions bot mentioned this pull request Oct 6, 2020

Week Ending October 4, 2020 dev-obs/actus#238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP: hardened exec requests #1899

KEP: hardened exec requests #1899

tallclair commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

sftim Jul 23, 2020

tallclair Sep 17, 2020

sftim Jul 23, 2020

sftim Jul 23, 2020

liggitt Jul 23, 2020

sftim Jul 23, 2020 •

edited

Loading

tallclair Sep 17, 2020

tallclair Sep 17, 2020

liggitt Sep 23, 2020

sftim Sep 23, 2020

tallclair Sep 23, 2020

tallclair Sep 28, 2020

knight42 commented Jul 27, 2020

tallclair commented Jul 27, 2020

tallclair commented Sep 16, 2020

derekwaynecarr commented Sep 16, 2020

liggitt left a comment

liggitt Sep 23, 2020

tallclair Sep 23, 2020

liggitt Sep 23, 2020

mlbiam commented Sep 28, 2020

tallclair commented Sep 28, 2020

mlbiam commented Sep 28, 2020

tallclair commented Sep 28, 2020

derekwaynecarr left a comment

derekwaynecarr Sep 30, 2020

derekwaynecarr Sep 30, 2020

derekwaynecarr Sep 30, 2020

derekwaynecarr Sep 30, 2020

k8s-ci-robot commented Sep 30, 2020

liggitt commented Oct 1, 2020

		The second path format includes the pod's UID, and is never called by the kube-apiserver. We should
		remove these endpoints as they're unused, to reduce code complexity.


		The changes to the Kubelet API are:

		1. Remove the `/run` endpoint, which is unused in core Kubernetes.

KEP: hardened exec requests #1899

KEP: hardened exec requests #1899

Conversation

tallclair commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

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

sftim Jul 23, 2020 • 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

knight42 commented Jul 27, 2020

tallclair commented Jul 27, 2020

tallclair commented Sep 16, 2020

derekwaynecarr commented Sep 16, 2020

liggitt 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

mlbiam commented Sep 28, 2020

tallclair commented Sep 28, 2020

mlbiam commented Sep 28, 2020

tallclair commented Sep 28, 2020

derekwaynecarr 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

k8s-ci-robot commented Sep 30, 2020

liggitt commented Oct 1, 2020

sftim Jul 23, 2020 •

edited

Loading