✨ PoC: Implement user warrants #3156

sttts · 2024-08-22T11:57:37Z

Summary

From Slack:

🧵 long ago @sur presented a problem (if I remember right) in our current auth model of serviceaccounts only being valid locally, and with that the inability to create workspaces through an APIExport virtual workspace. The steps are these:

a user U creates a workspace W2 in logicalcluster L1 through an APIExport virtual workspace (that has a permission claim on Workspaces).
the VW forwards the requests by impersonating its system:masters user with a fake system:kcp:admin serviceaccount (S1) for L1 named system:serviceaccount:default:rest.
the forwarded requests hits kcp and kcp authorizes the workspace creation with that S1 serviceaccount. It uses that identity to authorize the use of workspace types. By giving use permission to everybody, this goes through.
workspace admission sticks S1 as owner on the workspace object, to be used by initialization later.
the workspace scheduler creates a logicalclusters L2 for W2 and gives S1 admin permissions through a clusterrole+binding.
the initializers kick in and access the initializer VW for L2 where W2 has been scheduled to. Their requests are impersonated as S1 by the VW.
these requests hit kcp and the authorizer denies access because S1 is a foreign service account 💥

Now assume a modified system that

understands serviceaccounts from other workspaces, but does not give them permissions by default.
does not use a fake serviceaccount S1 identity to pass through the APIExport VW requests, but preserves the actual controller user and only attaches a warrant that gives the requires access of the claim.

The steps from above would turn into:

a user U creates a workspace W2 in logicalcluster L1 through an APIExport virtual workspace (that has a permission claim on Workspaces).
the VW forwards the requests by impersonating its system:masters user as U with a warrant for a fake system:kcp:admin serviceaccount (S1) for L1 named system:serviceaccount:default:rest.
the forwarded requests hits kcp and kcp authorizes the workspace creation with that warrant S1 serviceaccount because U is not enough. It uses the U identity to authorize the use of workspace types. ~~By giving use permission to everybody, t~~This goes through.
workspace admission sticks U with warrant S1 as owner on the workspace object, to be used by initialization later.
the workspace scheduler creates a logicalclusters L2 for W2 and gives U with warrant S1 admin permissions through a clusterrole+binding.
the initializers kick in and access the initializer VW for L2 where W2 has been scheduled to. Their requests are impersonated as U with warrant S1 by the VW.
these requests hit kcp and the authorizer ~~denies access because S1 is a foreign service account~~ grants access because U with warrant S1 is admin in L2.

Now assume in addition that U is actually a controller serviceaccount S0 in workspace root. Step (7) would 💥 because S0 is foreign for L2. We further modify the system to authenticate serviceaccount as system:kcp:serviceaccount:<logicalcluster>:<ns>:<name> with a warrant for system:serviceaccount:<ns>:<name> restricted to their defining logicalcluster.

Now, U becomes system:kcp:serviceaccount:root:default:controller with warrant system:serviceaccount:default:controller restricted to root. With that (4) becomes

workspace admission sticks system:kcp:serviceaccount:root:default:controller with warrant system:serviceaccount:default:controller restricted to root with warrant S1 as owner on the workspace object, to be used by initialization later.

I.e. the user has two warrants. With that (7) becomes:

these requests hit kcp and the authorizer grants access because system:kcp:serviceaccount:root:default:controller with the TWO warrants including S1 is admin in L2.

Related issue(s)

Fixes #

Release Notes

Pass through original identity of controllers accessing a logical cluster through the APIExport virtual workspace. To get the required permissions, a warrant mechanism is added through user extra fields that attaches secondary user identities purely used for authorization.

kcp-ci-bot · 2024-08-22T12:33:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sttts. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

docs/content/concepts/authorization.md

pkg/authorization/decorator.go

mjudeikis · 2024-11-23T09:54:57Z

pkg/authorization/decorator.go

+			logger.V(4).Info("authorization step",
+				"reason", reason,


Should this not be auditReasonMsg ? We add this into annotations but remove from logs? Intentional?

mjudeikis · 2024-11-23T10:09:09Z

pkg/server/options/authorization.go

+
+// withWarrantsInChainCallingDelegateWithOriginalUser could be named shorter, but then it would
+// look innocent which it isn't. It produces one level of the chain with the wrapper called
+// for the base and all warrents. The delegate though is called only once with the original user.


s/warrents/warrants

mjudeikis · 2024-11-23T10:10:14Z

pkg/virtual/apiexport/builder/build.go

-					Groups:   []string{bootstrap.SystemKcpAdminGroup},
+				// Add a warrant of a fake local service account giving full access
+				warrant := validation.Warrant{
+					User:   "system:serviceaccount:default:rest",


Just of interest, what is somebody creates an account like this?

totally possible. Same as today: you can create any kind of client cert faking a service account or oidc user or whatever you like. In other words, control over an authn provider is always critical.

But don't think you can impersonate this in any way.

Impersonation is a good point. Please double check that this is properly e2e tested. Escalating permissions due to impersonation in a workspace must not be possible.

deal. will try to spin some tests scenarios for this manually too.

mjudeikis · 2024-11-23T10:13:04Z

In general just a questions. looks good,

mjudeikis · 2024-11-23T10:42:54Z

pkg/authorization/maximal_permission_policy_authorizer.go

@@ -44,7 +44,7 @@ const (

 // NewMaximalPermissionPolicyAuthorizer returns an authorizer that first checks if the request is for a
 // bound resource or not. If the resource is bound it checks the maximal permission policy of the underlying API export.
-func NewMaximalPermissionPolicyAuthorizer(kubeInformers, globalKubeInformers kcpkubernetesinformers.SharedInformerFactory, kcpInformers, globalKcpInformers kcpinformers.SharedInformerFactory, delegate authorizer.Authorizer) authorizer.Authorizer {
+func NewMaximalPermissionPolicyAuthorizer(kubeInformers, globalKubeInformers kcpkubernetesinformers.SharedInformerFactory, kcpInformers, globalKcpInformers kcpinformers.SharedInformerFactory) func(delegate authorizer.Authorizer) authorizer.Authorizer {


Just mental note:

func NewMaximalPermissionPolicyAuthorizer( kubeInformers, globalKubeInformers kcpkubernetesinformers.SharedInformerFactory, kcpInformers, globalKcpInformers kcpinformers.SharedInformerFactory, ) func(delegate authorizer.Authorizer) authorizer.Authorizer {

easier to read. Took me a second to absorb the signature.

mjudeikis · 2024-12-03T15:20:23Z

test/e2e/authorizer/impersonation_test.go

+	impersonatedClient, err := kcpkubernetesclientset.NewForConfig(impersonationConfig)
+	require.NoError(t, err)
+
+	t.Log("As user-2 with system:masters, we should be able to read secrets")


As user-1 with impersonation of user-2 with system:masters, we should be able to read secrets ?

mjudeikis · 2024-12-03T15:20:55Z

test/e2e/authorizer/impersonation_test.go

+
+	t.Logf("Check SelfSubjectReview as user-2 with system:masters")
+	req := &authenticationv1.SelfSubjectReview{}
+	resp, err := impersonatedClient.Cluster(wsPath).AuthenticationV1().SelfSubjectReviews().Create(ctx, req, metav1.CreateOptions{})


Would impersonation not return system:masters group? Why its not expecting it?

mjudeikis · 2024-12-03T15:39:34Z

pkg/authorization/workspace_content_authorizer.go

@@ -44,25 +44,27 @@ const (
 	WorkspaceAccessNotPermittedReason = "workspace access not permitted"
 )

-func NewWorkspaceContentAuthorizer(localInformers, globalInformers kcpkubernetesinformers.SharedInformerFactory, localLogicalClusterLister, globalLogicalClusterLister corev1alpha1listers.LogicalClusterClusterLister, delegate authorizer.Authorizer) authorizer.Authorizer {


Im starting to wonder if we should duplicate this code (2 content authorizer implementations) and make all this warrant code feature-flag gated? But might be a lot of if-else statements, not sure if this worth the effort.
My inner me - says - lets just go.
But OSS me - says - better safe than sorry.

test/e2e/authorizer/impersonation_test.go

sttts · 2024-12-04T12:27:59Z

pkg/server/options/authorization.go


 	// protect status updates to apiexport and apibinding
-	systemCRDAuth := authz.NewSystemCRDAuthorizer(maxPermissionPolicyAuth)
-	systemCRDAuth = authz.NewDecorator("03-systemcrd", systemCRDAuth).AddAuditLogging().AddAnonymization().AddReasonAnnotation()
+	chain = withWarrantsInChainCallingDelegateWithOriginalUser(authz.NewSystemCRDAuthorizer, chain)


@mjudeikis to feature gate it, we could have a noop withWarrantsInChainCallingDelegateWithOriginalUser.

mjudeikis

maybe service accounts could go into separate pr too?

mjudeikis · 2024-12-20T11:02:34Z

docs/content/concepts/authorization/authorizers.md

+Service accounts from foreign workspaces are authenticated as user `system:kcp:serviceaccount:<logicalcluster>:<ns>:<serviceaccount>` 
+and under that name can be granted access to workspaces and other permissions. The original service account name is 
+appended as a warrant (see next section) in order to match the same (cluster) role bindings:


mental note:
this could be nice test too in e2e stack.

mjudeikis · 2024-12-20T11:15:00Z

pkg/server/handler.go

+// WithServiceAccountRewrite replaces the user info of the context for service
+// accounts by representing them as globally valid kcp service account representation
+// with a warrant in the old format (to match kube-like (cluster) role bindings).
+func WithServiceAccountRewrite(handler http.Handler) http.Handler {


Can service accounts be split too to separate PR? Just enable service account for cross-workspaces?

Will try. Have started the rebase now.

mjudeikis · 2024-12-20T11:15:53Z

pkg/authorization/workspace_content_authorizer.go

-	case !isUser && !isServiceAccountFromCluster:
-		// service accounts from other workspaces cannot access
+	case isServiceAccount && isForeign:
+		// Service accounts from other workspaces might conflict with local service accounts by name.


Is this still true if we rewrite service account names to be globally unique?

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

…for the target workspace Signed-off-by: Dr. Stefan Schimanski <[email protected]>

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

kcp-ci-bot · 2024-12-21T16:02:39Z

@sttts: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-verify	`d853530`	link	true	`/test pull-kcp-verify`
pull-kcp-verify-codegen	`d853530`	link	true	`/test pull-kcp-verify-codegen`
pull-kcp-test-e2e-shared	`d853530`	link	true	`/test pull-kcp-test-e2e-shared`
pull-kcp-lint	`d853530`	link	true	`/test pull-kcp-lint`
pull-kcp-test-e2e-sharded	`d853530`	link	true	`/test pull-kcp-test-e2e-sharded`
pull-kcp-test-e2e	`d853530`	link	true	`/test pull-kcp-test-e2e`
pull-kcp-test-e2e-multiple-runs	`d853530`	link	true	`/test pull-kcp-test-e2e-multiple-runs`

Full PR test history

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. I understand the commands that are listed here.

sttts changed the title ~~✨ PoC: Implement user warrents~~ ✨ PoC: Implement user warrants Aug 22, 2024

sttts force-pushed the sttts-warrants branch from 0de78d6 to 12b4ee2 Compare August 22, 2024 12:30

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2024

sttts force-pushed the sttts-warrants branch from 12b4ee2 to 00dd36e Compare August 22, 2024 12:33

sttts force-pushed the sttts-warrants branch from 00dd36e to ebb5114 Compare August 22, 2024 13:00

mjudeikis reviewed Aug 22, 2024

View reviewed changes

docs/content/concepts/authorization.md Outdated Show resolved Hide resolved

docs/content/concepts/authorization.md Outdated Show resolved Hide resolved

docs/content/concepts/authorization.md Outdated Show resolved Hide resolved

sttts force-pushed the sttts-warrants branch from ebb5114 to 76d5096 Compare August 23, 2024 06:33

kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 23, 2024

sttts mentioned this pull request Aug 23, 2024

✨ Add original user/group as extra to the impersonating client used by virtual workspace #3155

Merged

sttts force-pushed the sttts-warrants branch 4 times, most recently from 4b188ca to 2ca96c3 Compare August 26, 2024 09:29

kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2024

xrstf mentioned this pull request Nov 13, 2024

UPSTREAM: <carry>: include cluster name in authz SubjectAccessReview in webhooks kcp-dev/kubernetes#151

Merged

mjudeikis reviewed Nov 23, 2024

View reviewed changes

pkg/authorization/decorator.go Show resolved Hide resolved

mjudeikis reviewed Nov 23, 2024

View reviewed changes

sttts force-pushed the sttts-warrants branch from 2ca96c3 to d5fb3da Compare December 2, 2024 13:07

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2024

sttts force-pushed the sttts-warrants branch from d5fb3da to 9109d79 Compare December 2, 2024 13:29

mjudeikis reviewed Dec 3, 2024

View reviewed changes

test/e2e/authorizer/impersonation_test.go Show resolved Hide resolved

sttts commented Dec 4, 2024

View reviewed changes

kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2024

sttts force-pushed the sttts-warrants branch from 59654d8 to 8ad6d50 Compare December 12, 2024 15:38

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2024

sttts force-pushed the sttts-warrants branch from 8ad6d50 to cca5a21 Compare December 13, 2024 12:34

kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2024

mjudeikis reviewed Dec 20, 2024

View reviewed changes

sttts added 11 commits December 21, 2024 16:34

authz: add user scoping

afd37ff

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

authz: add warrants

33e9eb9

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

authn: rewrite service account to globally valid kcp sa with warrant

aed9c96

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

apis: typo

c8e2dd7

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

virtual/apiexport: pass through request user with additional warrent …

febdcdd

…for the target workspace Signed-off-by: Dr. Stefan Schimanski <[email protected]>

authz: make step logs useful

fdbf0df

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

e2e: add serviceaccount test accessing other workspaces

2110b5e

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

e2e: add warrant and scope tests

03af854

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

DEVEL: Bump kube

667fac8

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

Make linter happy

51a65fa

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

WIP: e2e: add impersonation tests

d853530

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

sttts force-pushed the sttts-warrants branch from cca5a21 to d853530 Compare December 21, 2024 15:36

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2024

sttts mentioned this pull request Dec 21, 2024

✨ PoC: Implement user scopes #3235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ PoC: Implement user warrants #3156

✨ PoC: Implement user warrants #3156

sttts commented Aug 22, 2024 •

edited

Loading

kcp-ci-bot commented Aug 22, 2024

mjudeikis Nov 23, 2024

mjudeikis Nov 23, 2024

mjudeikis Nov 23, 2024

sttts Nov 23, 2024

mjudeikis Nov 23, 2024

sttts Nov 25, 2024

mjudeikis Dec 2, 2024

mjudeikis commented Nov 23, 2024

mjudeikis Nov 23, 2024

mjudeikis Dec 3, 2024

mjudeikis Dec 3, 2024

mjudeikis Dec 3, 2024

sttts Dec 4, 2024

mjudeikis left a comment

mjudeikis Dec 20, 2024

mjudeikis Dec 20, 2024

sttts Dec 21, 2024

mjudeikis Dec 20, 2024

kcp-ci-bot commented Dec 21, 2024

✨ PoC: Implement user warrants #3156

Are you sure you want to change the base?

✨ PoC: Implement user warrants #3156

Conversation

sttts commented Aug 22, 2024 • edited Loading

Summary

Related issue(s)

Release Notes

kcp-ci-bot commented Aug 22, 2024

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

mjudeikis commented Nov 23, 2024

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

mjudeikis 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

kcp-ci-bot commented Dec 21, 2024

sttts commented Aug 22, 2024 •

edited

Loading