[WIP] Begin KEP for fine-grained authz #3617
Conversation
lavalamp
commented
Oct 12, 2022
•
lavalamp
commented
- One-line PR description: Initial draft of KEP
- Issue link: fine-grained authz #3737
- Other comments: This is complete enough to give the gist, but there are some missing sections still -- feel free to send PRs to my branch if you want to help.
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
kind/kep
k8s-ci-robot
added
sig/api-machinery
| * .metadata.ownerReferences (maybe) | ||
| * .status | ||
| * .status.conditions (many objects but not all; parameterized by specific type) | ||
| * .spec |
There was a problem hiding this comment.
I need to add .spec.containers[].resources...
|
|
||
| The authz system will see the verb "granular:labels". | ||
|
|
||
| #### Specific Labels |
There was a problem hiding this comment.
@apelisse can you look at this section and see if you can think of a better system? (you may need to read some prior sections to understand this)
| request with a forbidden error. | ||
| 3. Compute the change (patch logic, SSA logic, defaulting etc). Compute a list | ||
| of fields which changed. (SSA logic makes this easy.) | ||
| 4. Pre- or post- (DECISION NEEDED) webhooks, check the list of fields: |
There was a problem hiding this comment.
Probably post-mutating webhooks, no? I guess you could also run it both before and after.
There was a problem hiding this comment.
Kind of a trade off -- doing it post webhook is safer in terms of what gets committed but breaks webhooks that e.g. look at nodeName and keep a label up to date. pre-webhook is safer in terms of not exposing side-effect-having webhooks to malicious payloads.
There was a problem hiding this comment.
I prefer post-mutating webhooks as well. I'd like to check on the actual object being stored.
Co-authored-by: Han Kang <hankang@google.com>
| Therefore, __modifying a single authz system can't solve the problem__. | ||
|
|
||
| An authz check can be thought of as sending structured data to an opaque system | ||
| which answers yes or no. Essentially we can't change things about the system |
There was a problem hiding this comment.
yes, no, or no opinion. I think the distinction between no and no opinion is significant.
There was a problem hiding this comment.
I think that matters for individual authorizors but not really for the authz stack as a whole, which eventually either admits or rejects.
| (TODO: I'm sure sig auth will want to correct my wording here) | ||
|
|
||
| * The resource type (group, version, resource) | ||
| * The object's locator (namespace if any, name, uid) |
|
|
||
| * The resource type (group, version, resource) | ||
| * The object's locator (namespace if any, name, uid) | ||
| * The actor's identity (username, group) |
There was a problem hiding this comment.
user.Extra and user.UID are provided. See user.Info.
| in the order of most general to most specific. When we describe the | ||
| representation in the schema it will be clear what that means. The reason for | ||
| this is that multiple fields can be covered by a more general permission, | ||
| greatly reducing the number of checks needed in the worst case (as long as the |
There was a problem hiding this comment.
Should we expand the authorization interface to have a slice option to pass more than one to evaluate a time? We could expand the webhook using another version and non-participating webhooks could be shimmed to answer one at a time in parallel
There was a problem hiding this comment.
that's funny, I literally had that thought this morning
There was a problem hiding this comment.
that would already be nice for impersonation
There was a problem hiding this comment.
I have no objection to that but it sounds like an optimization that can be done whether or not the KEP goes through.
There was a problem hiding this comment.
I have no objection to that but it sounds like an optimization that can be done whether or not the KEP goes through.
I'm not completely sure of that. There are two "obvious" ways to handle parameterization of something like label values.
- One way is to describe in the schema a manner in which shrink the keyspace (you've done this).
- Another way is to send all the label keys to the authorizer as distinct verbs and let the authorizer decide how best to let a user/cluster-admin express rules.
I think if we have a bulk authorization interface, it becomes more practical to lookup every key at once.
There was a problem hiding this comment.
Sorry it says you sent this 7 days ago but this is the first I saw it -- if you look at my comment here I think my current plan doesn't require a bulk lookup. My backup plan also doesn't require a bulk lookup.
There was a problem hiding this comment.
The latest revision doesn't require bulk lookup, either.
|
Can you add a section that describes your ideal schema for describing policy rules? Is it just current RBAC with wildcard prefixes on verb? |
| A risk is that it would be possible to configure permissions which result in a | ||
| large number of authz checks. Specifically CRD authors could craft a CRD with | ||
| many checks required to do anything. |
There was a problem hiding this comment.
We restrict resource use by CRDs' validation functions. Perhaps a similar restriction could apply here?
There was a problem hiding this comment.
Oops, feedback is too early. Sorry about that.
| (The exact permission system will be described below because we're not done with | ||
| the requirements yet, keep reading!) | ||
|
|
||
| #### Uncovered verbs |
There was a problem hiding this comment.
@deads2k I think moving this concept out of the schema gets you what you want.
|
@lavalamp: The following test failed, say
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. |
|
|
||
| "fine-grained" means giving the ability to certain actors (users, groups, | ||
| controllers, etc) to edit only particular fields, instead of entire objects. | ||
| Additionally, it involves adding the ability to restrict existing board |
There was a problem hiding this comment.
Overall I am thrilled to see this. I am probably not close enought o find all the nitpicks needed, but I very much appreciate that you listed me. As always, your KEPs are a joy to read.
A few comments inline, one more general:
How do we default fields which are uncovered and unsatisfied? Or do we just NOT?
| Ordinary users must not be able to set this to true. Only admins may set it to | ||
| true. | ||
|
|
||
| #### List of known fields wanting these features |
There was a problem hiding this comment.
We have also had CVEs in the past which would have been better solved by a field-level mechanism, I think, such as Service.spec.externalIPs
| we have an alpha or beta behind us, hence we're not ready to specify that in | ||
| this KEP yet. | ||
|
|
||
| (The exact permission system will be described below because we're not done with |
There was a problem hiding this comment.
I just want to say that I am only this far in, and already I think it's a great KEP.
| Example: | ||
|
|
||
| ``` | ||
| apiVersion: permissionconfig.k8s.io/v1 |
There was a problem hiding this comment.
IMHO "config" adds no value in this string.
| There's one more piece of information we need to satisfy our second goal, and it | ||
| can't go in the schema because it may vary from cluster to cluster. | ||
|
|
||
| This concept is whether a permission verb is "covered" by enclosing permissions, |
There was a problem hiding this comment.
I'm not in love with "covered" and "unconvered" as terms - are they standard concepts in authz realm?
If not, maybe we can solicit ideas for names. e.g. Inherited vs Explicit or something
|
|
||
| ##### Everything in spec | ||
|
|
||
| On types: |
There was a problem hiding this comment.
Why both? Every type is linked from some field and we know that (sadly) we sometimes reuse types. Is there a driving case where defining it by type is the best solution?
| #### Pod's nodeName | ||
|
|
||
| Given where this appears in Pod, this permission will only be checked for pods | ||
| if the agent doesn't have the "specification" permission. |
There was a problem hiding this comment.
If they have PATCH/PUT it is also satisifed, right? Because this is a convered permission?
|
|
||
| ```go | ||
| ... | ||
| // +permission-verb:"nodeAssignment" |
There was a problem hiding this comment.
curiously, all your "verbs" are nouns - is it worth keeping the word "verb" in this? e.g +permission:"nodeAssignment" is just as clear. Or +permission-verb:"assignNode" ?
| ##### Everything in metadata | ||
|
|
||
| Configured the same as everything in spec, but the verb will be (CHOICE NEEDED) | ||
| "metadata" or "objectmeta". |
|
|
||
| DECISION NEEDED: | ||
|
|
||
| Option A: we will put a registry file in the kubernetes repository so that 3rd |
| status: provisional | ||
| creation-date: 2022-10-07 | ||
| reviewers: | ||
| - "@thockin" |
| DECISION NEEDED: | ||
|
|
||
| Option A: we will put a registry file in the kubernetes repository so that 3rd | ||
| party extension authors can register their custom permissions and dedup/reuse | ||
| rather than create new ones, if possible. | ||
|
|
||
| Option B: we will require 3rd party authors to prefix their custom permissions | ||
| with the string "ext" (TODO: "mycompany.example.com" if there is likely to be | ||
| space). |
There was a problem hiding this comment.
| DECISION NEEDED: | |
| Option A: we will put a registry file in the kubernetes repository so that 3rd | |
| party extension authors can register their custom permissions and dedup/reuse | |
| rather than create new ones, if possible. | |
| Option B: we will require 3rd party authors to prefix their custom permissions | |
| with the string "ext" (TODO: "mycompany.example.com" if there is likely to be | |
| space). | |
| <<[UNRESOLVED decision needed]>> | |
| Option A: we will put a registry file in the k/kubernetes repository so that 3rd | |
| party extension authors can register their custom permissions and dedup/reuse | |
| rather than create new ones, if possible. | |
| Option B: we will require 3rd party authors to prefix their custom permissions | |
| with the string "ext" (TODO: "mycompany.example.com" if there is likely to be | |
| space). | |
| <<[/UNRESOLVED]>> |
My option C would be to copy the idea from labels and use kubernetes.io/whatever for permissions defined by Kubernetes. Prefixes need to be valid DNS subdomains and should be associated with the party that defines them. Etc.
| DECISION NEEDED: would it be better to use a CEL expression to process the | ||
| parameter? |
There was a problem hiding this comment.
| DECISION NEEDED: would it be better to use a CEL expression to process the | |
| parameter? | |
| <<[UNRESOLVED decision needed]>> | |
| Would it be better to use a CEL expression to process the parameter? | |
| <<[/UNRESOLVED]>> |
|
/milestone v1.28 |
|
|
||
| 1. Check the "PUT" / "PATCH" / "CREATE" permission. If the actor has this, | ||
| place a marker=regular in the request context and proceed to step 3. | ||
| 2. Otherwise, check the "granular" permission. If yes, place a |
There was a problem hiding this comment.
Does a granular check make sense in the context of a create? It seems reasonable to me that it would only apply to update/patch requests.
There was a problem hiding this comment.
You might let people make a Pod, provided they don't set schedulingGates, and then have exceptions - maybe one principal can set this particular schedulingGate but not an arbitrary one.
Similar idea to setting a finalizer at create time. If we want to have fine grained control, I think it could be frustrating to have to duplicate the logic / whatever so that we restrict both creates and updates.
There was a problem hiding this comment.
Yeah the intention is that the per-field permissions are the same for both, otherwise I have to duplicate this all to handle both paths.
The result is you won't be able to create things if you lack a granular permission on a required, non-defaulted field.
There was a problem hiding this comment.
Does this check apply before or after static defaults?
There was a problem hiding this comment.
I think after? And probably some extra work not described in the KEP needs to be done to make it work like I just said.
I hope to make another pass on this KEP soon
There was a problem hiding this comment.
So if a field needs an uncovered permission, it cannot have a default value (or it has to be defaulted in registry, not static) - is that the right implication? That seems plausible to me.
|
|
||
| On any mutating call, | ||
|
|
||
| 1. Check the "PUT" / "PATCH" / "CREATE" permission. If the actor has this, |
There was a problem hiding this comment.
nit: permissions should be "updated" / "patch" / "create"
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/close Closing this since Daniel has stepped away from Kube and there isn't anyone to drive this currently. We can always reopen if someone steps up. |
|
@enj: Closed this PR. In response to this:
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. |
