pkg/endpoints: forward Impersonate headers in ProxyRequest

[gpaul]

Changes

Forward the Impersonate-User, Impersonate-Group and Authorization headers when proxying requests to the apiserver. See https://kubernetes.io/docs/reference/access-authn-authz/authentication/#user-impersonation

This partially addresses the privilege escalation where users with limited privileges, who can view the Tekton dashboard, essentially have the same privileges as the dashboard itself via the /proxy endpoint.

There remain endpoints that still amount to privilege escalation: a user with read-only permissions to a given namespace can still re-trigger pipelines, as just one example.

This PR assumes that the Tekton dashboard will be fronted by an authenticating reverse proxy that forwards end-users' OIDC email and group claims in the Impersonate-User and Impersonate-Group headers, respectively.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide
for more details.

[tekton-robot]

Hi @gpaul. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[AlanGreene]

Thanks for the PR @gpaul, this certainly looks useful and fits with a lot of the work currently in progress to provide better granularity in the permissions required / exposed.

I've added a comment below for a couple of the other maintainers' attention. We used to proxy all request headers but this was disabled ~10 months ago, so I'd like to make sure there's no conflict with something else here.

[AlanGreene]

@steveodonovan @a-roberts thoughts on this? Personally I think I'd prefer to copy all headers rather than cherry-picking, but looks like we used to do that and removed it for some reason.

Steve, can you remember why you removed this? Was it just the issue with the logs and the Accept header or was there more to it? 911a816#diff-13330ca6ce530de93565ddc93380b446L41

Would it be safe to copy all headers (with the possible exception of Accept)?

[AlanGreene]

Spoke with Steve about this and tested out proxying all headers. The log issue is still present when sending the Accept: text/plain header:
{"level":"error","msg":"Error: the server was unable to respond with a content type that the client supports"}

Omitting this seems fine and we could do this in the client, or send Accept: text/plain,application/json to preserve the intent and satisfy the API server.

We also need to verify the default behaviour without an OIDC authenticator configured on the cluster and ensure any impersonation requests are rejected.

[eddycharly]

Isn’t there a risk that it facilitates privilege escalation too ?

Does it require that all users exist in the cluster to be able to use the dashboard ?

[gpaul]

@AlanGreene

We also need to verify the default behaviour without an OIDC authenticator configured on the cluster and ensure any impersonation requests are rejected.

Indeed - I believe a simple CLI flag --proxy-impersonate-headers, or a more generic but less user-friendly --proxy-header="Impersonate-Group" --proxy-header="Impersonate-User" --proxy-header="Authorization" slice CLI option would do.

@eddycharly

Isn’t there a risk that it facilitates privilege escalation too ?

Definitely, if the dashboard is not deployed behind a reverse proxy that manages the Impersonate-* headers, then those headers should not be trusted and not forwarded.

Does it require that all users exist in the cluster to be able to use the dashboard ?

With the current design, if the headers aren't specified, then no impersonation is performed and the request is served using the tekton-dashboard serviceaccount's privileges.

If, however, the dashboard is hosted behind an authenticating reverse proxy that sets the Impersonate-* headers on the request to the dashboard, then the user's identity (i.e., sub or email claim), or a group that it belongs to (i.e., groups claim) would need to be granted explicit RBAC permissions in the cluster.

[eddycharly]

A regular user could push a task run that could call the dashboard service directly (not going through the reverse proxy because the pod is running inside the cluster) impersonating an admin ?

Can we ensure all incoming traffic is actually going through the reverse proxy ?

[AlanGreene]

From the docs:

• A user makes an API call with their credentials and impersonation headers.
• API server authenticates the user.
• API server ensures the authenticated users have impersonation privileges.

So the user must already have permissions to impersonate for the desired user/group/resource, and must be authenticated to the API server. How could this be abused to achieve privilege escalation?

I would expect the API server to reject any impersonation attempt if the user cannot be authenticated (whether missing credentials, or if no suitable authenticator is configured), or if they have not already been granted the required privileges for impersonation.

[gpaul]

A regular user could push a task run that could call the dashboard service directly (not going through the reverse proxy because the pod is running inside the cluster) impersonating an admin ?

Correct, a user who is allowed to launch tasks could do that.

Can we ensure all incoming traffic is actually going through the reverse proxy ?

One might use a NetworkPolicy to restrict incoming connections to the tekton-dashboard to those that originated at the reverse proxy. https://kubernetes.io/docs/concepts/services-networking/network-policies/

[gpaul]

@AlanGreene the attack vector is as follows:

By default, a pod in the cluster can reach any service in the cluster via cluster DNS, e.g., https://tekton-dashboard.default/

That would not go through the ingress controller / reverse proxy, as the request is originating inside the cluster.

If a user is allowed to launch tasks, e.g., pods, then that user can send HTTP requests to http://tekton-dashboard.default/...proxy/...secrets/ and set the Imporsonate-User: admin HTTP header on their request.

The tekton dashboard will receive the request, notice the Impersonate-User header, and simply forward it to the kube-apiserver, which will honour it and respond with the privileged data.

[AlanGreene]

I think I'm still missing some piece of the puzzle here.

The authenticated user would be the dashboard service account in that case, and it does not have impersonation privileges so I would expect the API server to reject the request.

If the API server is blindly trusting any Impersonate-* header it receives then it's acting contrary to the documentation and there are much bigger problems to worry about even without the dashboard in the picture at all.

[gpaul]

Ah yes - the dashboard would need impersonation privileges.

[gpaul]

If the dashboard is the one making the request to the apiserver, it doesn't help that the end user has impersonation privileges. The dashboard needs to have them.

[gpaul]

Actually, the reverse proxy should use its service account token to also set the Authorization header on the request to the dashboard. Then the dashboard forwards the Authorization header along with the Impersonate-* headers.

That way, the impersonating user is not the dashboard, but the reverse proxy. The dashboard does not use its own serviceaccount to perform the forwarded request against the apiserver.

In the case of a malicious user active inside the cluster, any malicious pod would have to run as a serviceaccount that has impersonation privileges, otherwise the apiserver forbids the request.

[sebbrandt87]

❤️

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sebbrandt87
To complete the pull request process, please assign dibbles
You can assign the PR to them by writing /assign @dibbles in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[gpaul]

I've updated the PR:

• I've threaded an "Impersonate" option through from main().
• I've made the Authorization header required when Impersonation is enabled, as per pkg/endpoints: forward Impersonate headers in ProxyRequest #1416 (comment)

[AlanGreene]

Do we need the new config flag? Is there a scenario where the Authorization + Impersonate-* headers are provided that we wouldn't want to proxy them?

[gpaul]

There's the case of a malicious pod in the cluster not providing any of the headers, in which case the request is served using the dashboard serviceaccount which results in escalated privilege.

[AlanGreene]

I wonder if a network policy would be more suitable here to prevent untrusted access to the dashboard, so only the auth proxy would be allowed.

One of my concerns is that the more configuration options we add to the dashboard the more complex the test story becomes, and also the harder it becomes to ensure we're not inadvertently creating new gaps. There are already well defined and understood kubernetes approaches to limiting access within the cluster and we should consider if some of those would be more appropriate and perhaps more flexible here rather than trying to implement something custom in the dashboard itself.

I'm curious what others think about this, especially in light of some of the other work in progress around limiting dashboard access / functionality.

[eddycharly]

I have a few concerns too.

I feel like it will be hard to make a good user experience from this, mainly because we don't know what a user can or cannot do, so we cannot hide actions and menu items that are not allowed etc...

It probably won't work well with the backend controllers, there will be no consistency between the impersonated role and the role used by the controllers, the web socket will not be inline with the current user permissions.

Another concern is that it adds quite a bit of complexity, if we want this to make it, it should be well documented and possible misconfigurations should be explained in details so that end users don't make mistakes when enabling impersonation.

One more concern i have is that the proxy exposes the api server on an external network, it's not directly related to this PR but exposing the control plane is something i'm not comfortable with from the beginning. Adding the possibility to use the exposed proxy combined with impersonation gives me cold sweats.

Read-only and single namespace modes are not a perfect solution but at least it should help to limit the blast radius in a controlled and consistent way.

For sure i would love to see authn/authz supported in the dashboard, i really find the attempt here interesting but i feel like it's a big feature that will go beyond just enabling impersonation headers and setting up a reverse proxy.

It's just my personal opinions, very curious about what others think and quite happy that the topic gets more discussions.

[AlanGreene]

Closing this PR, but happy to review / discuss further in a design proposal that addresses the concerns above. I do think the user impersonation approach is worth exploring, and a solution would likely overlap with some of the front end work required for #1388 as well as some enhancements to our recent changes for single namespace visibility.