Allow cluster admin revoke other intercepts #4027

yuyuvn · 2026-01-08T07:08:53Z

Description

Checklist

I made sure to update ./CHANGELOG.yml (our release notes are generated from this file).
I made sure to add any documentation changes required for my change.
My change is adequately tested.
I updated CONTRIBUTING.md with any special dev tricks I had to use to work on this code efficiently.
Once my PR is ready to have integration tests ran, I posted the PR in #telepresence-oss channel on the
CNCF Slack so that the "ok to test" label can be applied.

thallgren · 2026-01-08T13:10:27Z

I don't see anything here preventing a client from revoking an intercept. You'll need to provide a test that proves that only an admin account can revoke an intercept.

yuyuvn · 2026-01-09T06:45:01Z

sorry, after I commit DCO, the change related to token review was disapeared...

I added it and add unit test, too

thallgren

This code looks great overall. I made some improvement suggestions.

I'm missing one major part here though. The client side calling this new API endpoint. The PR won't be much use to an end user without that. So please also provide the new telepresence revoke command (or similar), where the user passes a token (preferably from stdin).

It would also be very valuable if you could add a section to the docs explaining how to add a user with the correct group affinity to the cluster and obtain the necessary token.

thallgren · 2026-01-09T10:34:01Z

pkg/k8sapi/tokenreview.go

+
+// VerifyToken verifies a bearer token using Kubernetes TokenReview API
+// and returns information about the authenticated user including their groups.
+func VerifyToken(ctx context.Context, token string) *VerifyTokenResult {


I'm not a fan of returning an error as part of the result. I think it's more in line with other API's if you remove the Error field from the VerifyTokenResult and instead return (VerifyTokenResult, error).

Maybe also consider renaming the function and the struct. The function doesn't just verify a token, it actually obtains user information from that token. So perhaps something like:

func UserInfoForToken(ctx context.Context, token string) (UserInfo, error) ?

resolved in 80274bd

thallgren · 2026-01-09T10:35:08Z

pkg/k8sapi/tokenreview.go

+}
+
+// IsMemberOfGroup checks if a user belongs to a specific group.
+func IsMemberOfGroup(groups []string, targetGroup string) bool {


Maybe this could be func (u *UserInfo) IsMemberOfGroup(group string) bool ?

resolved in 80274bd

thallgren · 2026-01-09T10:39:01Z

pkg/k8sapi/tokenreview.go

+	for _, group := range groups {
+		if group == targetGroup {
+			return true
+		}
+	}
+	return false


Suggested change

for _, group := range groups {

if group == targetGroup {

return true

}

}

return false

return slices.Contains(groups, targetGroup)

resolved in 80274bd

thallgren · 2026-01-09T10:42:58Z

cmd/traffic/cmd/manager/service.go

+		return nil, status.Errorf(codes.PermissionDenied, "user must be a member of telepresence:admin or system:masters group")
+	}
+
+	dlog.Infof(ctx, "User %s authorized to revoke intercepts", tokenResult.Username)


Please don't include the user name in the log. It might be considered sensitive information and prevent logs from being shared.

resolved in 80274bd

yuyuvn · 2026-01-13T08:29:26Z

I refactored to use AGENT_K8S_ADMIN_GROUPS env to control allowed admin groups because it's hard to test in kind cluster for harded code group (with a dynamic group, it can also allow service account token to be used)

Can quickly test by creating a service account in ambassador namespace and add this env to manager deployment

          - name: AGENT_K8S_ADMIN_GROUPS
            value: "system:masters system:serviceaccounts:ambassador"

with token created from

kubectl create token test-admin -n ambassador --duration=1h

it also works with OIDC token from aws eks get-token

Signed-off-by: Phan Duc <[email protected]>

yuyuvn · 2026-01-14T03:15:46Z

Fixed lint 🙏

thallgren

A general question. Can we control this somehow using RBAC? Telepresence is often used in situations where it's limited to a specific set of namespaces. It would be great if the permission to revoke an intercept was associated with a role/rolebinding or clusterrole/clusterrolebinding. Which one that is in use depends on scope determined by the namespace selector. This way, any user could be permitted to do this by only associating this binding with an existing user account.

thallgren · 2026-01-14T10:09:34Z

pkg/client/cli/cmd/revoke.go

+		Use:  "revoke <token> <intercept_id>",
+		Args: cobra.ExactArgs(2),
+
+		Short: "Revoke an intercept by intercept ID. The intercept ID must be in the format <session_id>:<intercept_name>",


I see two problems with exposing and using the intercept ID like this.

This ID is supposed to be opaque. The fact that it's composed by the client's session-id and a name isn't necessarily something that we want to expose as API and then stick with it.

How is a user supposed to find another users session_id? AFAIK, the only place it's visible today is if you run the traffic-manager with log level debug or trace and then scan the logs.

I can fetch it from sidecar logs (that's how I plan to use in my environment)

I think that depends on what type of conflict you're encountering. If the traffic-manager detects that you have a --http-header based conflict, then you'll get an error similar to:

"intercept in error state AGENT_ERROR: Conflicts with the currently-served intercept "ae29c3db-0e14-4517-a538-7953e19fd155:echo-easy": header filters form a subset

but if the intercept is global, the user will instead see:

container port 8080 is already intercepted by root@3029256df965, intercept echo-easy

I think this is unfortunate. It would be far better, and also consistent with your approach, if both errors displayed the intercept ID. It would also remove the need to look into logs to find the ID. Do you think it would be possible for you to fix so that these errors both report the intercept ID?

yuyuvn · 2026-01-14T12:53:38Z

A general question. Can we control this somehow using RBAC? Telepresence is often used in situations where it's limited to a specific set of namespaces.

I can try if SubjectAccessReview can work without define custom resource for intercept.
it would be more k8s native if intercept state is stored in k8s storage with crd for intercept, but I feel like it would be a big refactor. that also resolve problem how to get list other's intercept

yuyuvn force-pushed the support-revoke-intercept branch from a8ba8f0 to 6518075 Compare January 8, 2026 07:21

yuyuvn force-pushed the support-revoke-intercept branch 5 times, most recently from 5d1489f to 4ed3287 Compare January 9, 2026 06:43

yuyuvn force-pushed the support-revoke-intercept branch 2 times, most recently from 3e50e55 to 448d483 Compare January 9, 2026 06:46

thallgren reviewed Jan 9, 2026

View reviewed changes

yuyuvn force-pushed the support-revoke-intercept branch 3 times, most recently from 479017a to 74327a1 Compare January 13, 2026 08:25

yuyuvn force-pushed the support-revoke-intercept branch 7 times, most recently from d5e663f to bac5c2f Compare January 14, 2026 03:01

yuyuvn added 5 commits January 14, 2026 12:01

Support revoke intercept

e9ef51c

Signed-off-by: Phan Duc <[email protected]>

Fix comments

2b6203b

Signed-off-by: Phan Duc <[email protected]>

Add revoke to client

58dedcb

Signed-off-by: Phan Duc <[email protected]>

Support cli revoke command and allow config admin groups via env

4323edc

Signed-off-by: Phan Duc <[email protected]>

Fix lint

8611806

Signed-off-by: Phan Duc <[email protected]>

yuyuvn force-pushed the support-revoke-intercept branch from bac5c2f to 8611806 Compare January 14, 2026 03:01

thallgren reviewed Jan 14, 2026

View reviewed changes

Uh oh!

Allow cluster admin revoke other intercepts #4027

Are you sure you want to change the base?

Allow cluster admin revoke other intercepts #4027

Uh oh!

Conversation

yuyuvn commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

thallgren commented Jan 8, 2026

Uh oh!

yuyuvn commented Jan 9, 2026

Uh oh!

thallgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuyuvn commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuyuvn commented Jan 14, 2026

Uh oh!

thallgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuyuvn commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yuyuvn commented Jan 8, 2026 •

edited

Loading

yuyuvn commented Jan 13, 2026 •

edited

Loading

yuyuvn commented Jan 14, 2026 •

edited

Loading