Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ PoC: Implement user warrants #3156

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

sttts
Copy link
Member

@sttts sttts commented Aug 22, 2024

Summary

Needs kcp-dev/kubernetes#145.

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:

  1. a user U creates a workspace W2 in logicalcluster L1 through an APIExport virtual workspace (that has a permission claim on Workspaces).
  2. 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.
  3. 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.
  4. workspace admission sticks S1 as owner on the workspace object, to be used by initialization later.
  5. the workspace scheduler creates a logicalclusters L2 for W2 and gives S1 admin permissions through a clusterrole+binding.
  6. 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.
  7. 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:

  1. a user U creates a workspace W2 in logicalcluster L1 through an APIExport virtual workspace (that has a permission claim on Workspaces).
  2. 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.
  3. 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, tThis goes through.
  4. workspace admission sticks U with warrant S1 as owner on the workspace object, to be used by initialization later.
  5. the workspace scheduler creates a logicalclusters L2 for W2 and gives U with warrant S1 admin permissions through a clusterrole+binding.
  6. 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.
  7. 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

  1. 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:

  1. 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 kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2024
@sttts sttts changed the title ✨ PoC: Implement user warrents ✨ PoC: Implement user warrants Aug 22, 2024
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2024
@kcp-ci-bot
Copy link
Contributor

[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

user: user1
groups: ["group1"]
extra:
authentication.kcp.io/scope: cluster:logical-cluster-1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a scopes as it can contain multiple?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, extra is map[string][]string. But the keys are traditionally singular.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

user: user1
groups: ["group1"]
extra:
authorization.kcp.io/warrant: |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorization.kcp.io/warrants as It can the list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines +259 to +277
This warrant allows `user1` to act under the permissions of `user2` in
`logical-cluster-1` if `user1` is not allowed to act as `user2` in the first place.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talking here about local user? This is not a problem if external/oidc identity is used? What is user type here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any user. You can use scopes to scope it down to a workspace.

@kcp-ci-bot 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
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
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]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
@kcp-ci-bot
Copy link
Contributor

@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 2ca96c3 link true /test pull-kcp-verify
pull-kcp-verify-codegen 2ca96c3 link true /test pull-kcp-verify-codegen
pull-kcp-test-e2e-shared 2ca96c3 link true /test pull-kcp-test-e2e-shared
pull-kcp-lint 2ca96c3 link true /test pull-kcp-lint
pull-kcp-test-e2e-sharded 2ca96c3 link true /test pull-kcp-test-e2e-sharded
pull-kcp-test-e2e 2ca96c3 link true /test pull-kcp-test-e2e
pull-kcp-test-e2e-multiple-runs 2ca96c3 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.

@kcp-ci-bot
Copy link
Contributor

PR needs rebase.

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.

@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants