Repository navigation
feat(flyteadmin): optional authorization for execution terminate - #8254
Open
ezandamela-lila wants to merge 1 commit into
Open
ezandamela-lila wants to merge 1 commit into
ezandamela-lila wants to merge 1 commit into
Conversation
TerminateExecution authorizes nothing beyond the project/domain scope every authenticated caller already holds. Add an opt-in terminateAuthz config block that restricts it to the execution's launcher, named admin identities, members of a configured group claim, and named system identities. Off by default. Signed-off-by: Edson Zandamela <ezandamela@lila.ai>
Member
|
As someone who wrote an RBAC implementation that never landed due to lack of upstream support I don't think it makes much sense to make targeting authorization point fixes like this imo. TerminateExecution is very specific. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
Any authenticated user can terminate any other user's execution.
TerminateExecutionchecks nothing beyond the project/domain scope every caller already has, even though flyteadmin already records who launched each execution.What changes were proposed in this pull request?
Adds an opt-in
terminateAuthzconfig block. When it's enabled, a terminate is allowed if:exemptLaunchers, for shared service accountssystemIdentities, for flytepropeller's cascading abortsadminIdentities, or has one ofadminGroupsin thegroupsClaimclaimAnything else gets
PermissionDenied. It's off by default, so nothing changes until you turn it on.This is for Flyte v1 (
master). It needs flyteadmin auth turned on with an identity provider, so the access token carries asub, plus the group claim if you useadminGroups.How was this patch tested?
Unit tests in
auth/authz_test.go, andTestAuthorizeTerminateinexecution_manager_test.gocovering each allowed and denied case above. They pass on currentmaster.We've also been running it in production since September, in this setup:
groupsClaim: groupsfrom Keycloak foradminGroups, and flytepropeller's client id insystemIdentities, since propeller calls flyteadmin with its own credentials from the other clustersWe haven't tested it with any other identity provider.
To try it you need flyteadmin auth on and two users:
terminateAuthz.enabled: true, and add propeller's client id tosystemIdentitiesif propeller authenticates to flyteadmin.flytectl delete execution -p <project> -d <domain> <name>. It fails withPermissionDenied: only the user who launched this execution may terminate it.Labels
Check all the applicable boxes