enhancements icon indicating copy to clipboard operation
enhancements copied to clipboard

Changing ReferenceGrant KEP to broader Referential Authorization KEP

Open robscott opened this issue 1 year ago • 9 comments

  • One-line PR description: This significantly refactors the ReferenceGrant into a broader Referential Authorization KEP in response to conversations with SIG Auth leadership.

  • Issue link: #3766

  • Other comments: This is a follow up from a rough proposal doc and corresponding POC to ensure it all works

/cc @deads2k @enj @liggitt @msau42 @thockin @ttakahashi21 @youngnick @stealthybox @sftim

robscott avatar Jan 06 '24 01:01 robscott

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robscott Once this PR has been reviewed and has the lgtm label, please ask for approval from enj. 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

k8s-ci-robot avatar Jan 27 '24 02:01 k8s-ci-robot

/triage accepted

stlaz avatar Jan 29 '24 17:01 stlaz

Thanks to everyone for the great feedback here! I've pushed some updates that should resolve most/all feedback. PTAL.

robscott avatar Jan 31 '24 04:01 robscott

I'd still advocate for splitting ClusterReferenceGrant into two objects:

  • one for the API author, defining how the referring API is shaped
  • one for the (cluster) admin, saying that all within-namespace references are ok

What if "one for the (cluster) admin, saying that all within-namespace references are ok" actually just means we move this part to ClusterReferenceConsumer? So then admins can define if they want to grant a consumer access to all same-namespace references.

robscott avatar Feb 29 '24 16:02 robscott

@robscott: The following test 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-enhancements-verify 0815231cf511dd8adb1edddfce3350a1f382e341 link true /test pull-enhancements-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-ci-robot avatar Mar 27 '24 03:03 k8s-ci-robot

One thing we discussed/realized at Kubernetes Contributor Summit:

This proposal has three possible uses:

  • Allow cross-namespace references
  • Limit the scope of what operators can do (avoid confused deputy in controllers, do not allow e.g. cluster-wide read access to gateways)
  • Possibly require referencegrants for in-namespace references as well

It struck me that we don't get any more isolation than before, although we remove the RBAC role (e.g. get secrets cluster wide), because:

  • operators in general need to add finalizers, ownerreferences, labels etc.
  • for this, the operator needs update access for the custom resource
  • but in order to authorize updates on metadata, you implicitly also give update on the spec
  • => the operator can change the spec, where the reference is, to make it able to access essentially any referred resource (e.g. secrets), even though the authorizer is in kube apiserver

So we need a dedicated myresource/metadata subresource, so users can give access to metadata, but not spec, new KEP 😄

luxas avatar Apr 17 '24 15:04 luxas