KEP-4601: authorize with field and label selectors

[deads2k]

Still in progress, but the bones are here.

/assign @liggitt
/assign @micahhausler
/cc @enj

• One-line PR description:

• Issue link:

• Other comments:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

• keps/prod-readiness/OWNERS [deads2k]
• keps/sig-auth/OWNERS [deads2k]

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

[deads2k]

@jpbetz @sttts If we add field and label selectors, I'd like to provide advice to the authorization webhook authors about what the semantics will be. This is not a proposal to make those changes.

My intent is that if a field and/or label selector are specified on a mutation request, the object being mutated (both old and new) are guaranteed to match the selectors. If the object (old or new) does not match the selector, the storage layer rejects the request.

[liggitt]

Do we need to define meaning of selectors if/when included for subresources in the future?

The main question I'd have is whether a distinction is made for subresources which do and do not see the full object in question (admission defines objectSelector to only work for subresources which see the full object)

• examples of ones which see the full object: pods/status
• examples of ones which do not see the full object: pods/{exec,attach,log,proxy,binding}, deployments/scale

[jpbetz]

My intent is that if a field and/or label selector are specified on a mutation request, the object being mutated (both old and new) are guaranteed to match the selectors. If the object (old or new) does not match the selector, the storage layer rejects the request.

This intuitively makes sense to me. This implies that updating a label or field matching the selector is only possible if there is another matching selector for the new label or field value? Am I reading this right that it will be possible to authorize multiple field and label selectors?

[deads2k]

The main question I'd have is whether a distinction is made for subresources which do and do not see the full object in question (admission defines objectSelector to only work for subresources which see the full object)

I'm nearly in agreement. But I think that the storage layer of deployments/scale has the entire object. Versus admission which does not have the entire object. store *genericregistry.Store appears to be shared between spec, status, and scale.

Subresources that are not stored into etcd would not be able to enforce as uniformally. We could select specific subresources that we know end up in storage like status and scale. pods/binding and eviction would both be useful, but far narrower.

[liggitt]

update looks pretty good to me, a couple more questions / some maybe-stale docs

[liggitt]

Suggested change

	* For Update, this means that the finalNewObject and oldObject must match the field and label selector.
	* For Update/Patch, this means that the finalNewObject and oldObject must match the field and label selector.

[liggitt]

Suggested change

	In the future, the kube-apiserver may add field and label selectors to Create, Update, and Delete.
	In the future, the kube-apiserver may add field and label selectors to Create, Update, Patch, and Delete.

[liggitt]

Do we need to define meaning of selectors if/when included for subresources in the future?

The main question I'd have is whether a distinction is made for subresources which do and do not see the full object in question (admission defines objectSelector to only work for subresources which see the full object)

• examples of ones which see the full object: pods/status
• examples of ones which do not see the full object: pods/{exec,attach,log,proxy,binding}, deployments/scale

[liggitt]

would these go under ResourceAttributes rather than directly under the SubjectAccessReviewSpec?

[liggitt]

// Webhook implementations must handle malformed rawSelectors.

Must they? A lot of the docs seem leftover from the first draft where the fields were not mutually exclusive

I'd encourage general webhook authors to:

1. ensure rawSelector and requirements are not both set
2. consider the requirements field if set
3. not try to parse or consider the rawSelector field if set

similar questions on the other docs and other selector type

[deads2k]

Must they? A lot of the docs seem leftover from the first draft where the fields were not mutually exclusive

I'd encourage general webhook authors to:

how would you like to phrase: "clients can send you data that is invalid that a kube-apiserver will never send you. Think about it advance and don't crash"?

[liggitt]

I think this covers it?

I'd encourage general webhook authors to:

• ensure rawSelector and requirements are not both set
• consider the requirements field if set
• not try to parse or consider the rawSelector field if set

[deads2k]

ok, willing to update.

Trying to decide how much carries to actual API documentation that appears on the SAR endpoint. I haven't checked docs to see if we have a separate place yet.

[liggitt]

Suggested change

	// rawSelector is the serialization of a field selector that would be included in a List query parameter.
	// rawSelector is the serialization of a field selector that would be included in a query parameter.

[liggitt]

Suggested change

	// requirements is the parsed interpretation of the rawSelector.
	// requirements is the parsed interpretation of a field selector.

[liggitt]

could these be simplified to:

* If `rawSelector` and `requirements` are both present, the request is invalid.
* For kube-apiserver: if `rawSelector` is present, the rawSelector will be parsed and honored if the parsing succeeds and the verb supports selectors.
* For kube-apiserver and webhooks: if requirements are present, the requirements should be honored if the verb supports selectors.

[deads2k]

the requirements should be honored if the verb supports selectors.

How and why would an authorizer make a decision about whether the client that called it supports selectors for a particular verb? If we make this a webhook responsibility, would this mean that the webhook needs to predict when the kube-apiserver will honor selectors for verb and subresource tuples?

[deads2k]

I find the fully separation much easier to find what I'm looking for. Your feel strongly about simplifying? Do you want the change the to webhook semantics from what I proposed? I don't see how it's possible.

[liggitt]

since this is essentially an enum field, should describe how clients should handle unrecognized operators if we ever want the ability to expand this in the future

easiest way is to ignore entries with unrecognized operators and assume they don't limit the request

[jpbetz]

If, in the future, we add field/label selectors to Create/Update/Patch/Delete, is Get the only remaining unsupported CRUD operation? Should we support it in the future too?

[jpbetz]

Thinking about this more? Is an explicit selector ever needed for single resource operations? Is the actual value of the labels and fields sufficient for any authz checks that would need to be made?

[liggitt]

authz doesn't have access to the body of the incoming object or the persisted content of the existing object

The query here would be like a self-attesting precondition the writer could optionally add that would be visible to authz and would have to be verified/blocking at the storage layer for the write to succeed.

The opt-in nature and the duplication with the body content is… a little weird, so I'm not sure if this is a direction we'd ever go, but defining how the precondition would be treated if we did is important.

[jpbetz]

OK, this explains it. I was wondering if this might be the motivation. Including this background in the KEP about why we could consider passing a field selector alongside a single resource operation that redundantly states information already in the resource seems valuable here.

[luxas]

Thanks @deads2k for writing this!

Very excited to get this going, happy to help out on some of the implementation as well 👍

I'm concerned about the sprawl of having two almost-identical representations, but not really in the SAR body. I'm all for making good, canonical client-/server-side libraries instead that wrap the complexity, and contain it there. Just such a thing as anding all requirements together into something that is equivalent but easily comparable/understandable is non-trivial unless one thinks a little bit deeper about the set theoretics.

[luxas]

Suggested change

	List, Watch, and Deletecollection.
	List, Watch, and DeleteCollection.

sorry for the nit, but this would please my eye 😄

[luxas]

Not LocalSubjectAccessReview? They share the same https://kubernetes.io/docs/reference/kubernetes-api/authorization-resources/subject-access-review-v1/#SubjectAccessReviewSpec

[luxas]

Are we concerned that if we add these two methods to the authorizer.Attributes interface now, implementers of the interface won't satisfy it on a go.mod update without manually adding those methods? We don't make promises AFAIK for stuff like that, but just asking.

Another alternative is to make another interface that can only be casted to if label/field selectors are available, but I don't like that alternative so much either.

[deads2k]

Are we concerned that if we add these two methods to the authorizer.Attributes interface now, implementers of the interface won't satisfy it on a go.mod update without manually adding those methods? We don't make promises AFAIK for stuff like that, but just asking.

I'm ok breaking people that integrated at this level. They should be aware of the semantic change and this is an easy way to force it.

[luxas]

what do you mean with "expand" here?

[deads2k]

clarified

[luxas]

Is parsing the URI into requirements really that bad that we have to add two modes, and make the matrix of parsing the SAR much much harder? I would think the opposite, if we have a canonical way (in a Go library) to encode/decode the Requirements into a string, and define strong semantics for the Requirements (e.g. how they can be simplified and such), I would believe that makes it easier for the SAR client to introspect what is happening.

That reduces the burden of the API server and webhooks to understand/parse arbitrary strings. That complexity has to live somewhere, but it seems most fair to put that where it is ultimately needed, the SAR client which serves the http request.

I would really really like to avoid having two modes that expand the matrix significantly.

If we have a good codec for Requirements <-> string, that could even over time start supporting (opt-in) new, canonical functionality, such as set In/NotIn semantics instead of only Equal/NotEqual we have today, without the SAR client even noticing.

We anyways need that codec easily accessible for webhooks, and other places. Why expand the surface unnecessarily? And isn't adding rawSelector something that is a net-add in the future, if we later saw it was absolutely necessary?

[deads2k]

Is parsing the URI into requirements really that bad that we have to add two modes, and make the matrix of parsing the SAR much much harder? I would think the opposite

It's non-trivial and not all webhook implementations are in go, so providing a golang library (note that we already include parsers in apimachinery) falls well short.

SAR to the kube-apiserver will parse the RawSelector for convenience.

[enj]

@luxas #4600 (comment)

[luxas]

What format is this? JSONPath? I know it exists already but how strictly is it defined today, and do we feel that is a strong and secure enough set of semantics. How does this react to e.g. sth like .spec.containers[*].name? We now have CEL as well, which we did not have in k8s infancy when labelSelectors were created.

[deads2k]

What format is this?

Arbitrary string with no semantic meaning attached. That's the meaning today and I don't propose to change it. CRs are slightly more strict.

[luxas]

See my comment above. Someone always have to parse the rawQuery at the end of the day, and I'd like to do that in the place where it doesn't blow up the complexity for others, i.e. in this SAR client. (with well-defined Go libraries to handle this easily, of course).

I expect the SAR client to be interested in what it is forwarding as well, and maybe even through introspection be able to deny or allow certain requests without sending the SAR altogether.

[deads2k]

Someone always have to parse the rawQuery at the end of the day,

In this proposal, that someone is the kube-apiserver (or aggregated apiserver).

[luxas]

Suggested change

	Remember to update these places in existing code:

[luxas]

I believe we should provide a function to "simplify" the requirements into a "canonical representation". For example, if there are multiple rules for the same key, one can apply the following rules:

1. Simplify Equals/NotEquals to In/NotIn (this already happens in the rawSelector parsing layer essentially)
   Equals foo => In [foo]
   NotEquals foo => NotIn [foo]

2. Simplify In/NotIn separately

In [a,b] && In [b,c] => In [b] (intersection)
NotIn [a,b] && NotIn [b,c] => NotIn [a,b,c] (union)

By this stage, we have at most one In and one NotIn requirement per key (plus any possible Exists/NotExist, handled later)

3. Simplify In/NotIn together

In [a,b] && NotIn [b,c] => In [a] (subtraction)

Note: this must be done last (as far as I can see), as the previous union/intersection operations are commutative, but subtraction is not.

In the end of this process we have at most one requirement with operator In or NotIn (not both), plus any possible Exists/NotExist, handled later.

4. Simplify Exists/NotExists

Exists && Exists => Exists
NotExists && NotExists => NotExists
Exists && NotExists => ∅ (a label cannot both be defined and not for the same object, unsatisfiable)

At this point, we have at most one (In or NotIn) and one (Exists or NotExists).
Of course, we can short-circuit earlier if we get an empty set before, as then the requirements are unsatisfiable by any object.

5. Combine one (In or NotIn) and one (Exists or NotExists)

In [a,b] && Exists => In [a,b] (if foo=a or foo=b is true, foo does exist)
NotIn [a,b] && Exists => NotIn [a,b] && Exists (no simplification possible)
In [a,b] && NotExists => ∅ (if the label must both be set and not set, req is unsatisfiable)
NotIn [a,b] && NotExists => NotExists (a label not existing certainly is not set to a given value)

At this point, of all possible requirements, we have at most the following requirements for each key:
In [a,b]
NotIn [a,b]
Exists
NotExists
∅ (unsatisfiable, which makes the whole requirements list for any object unsatisfiable)
NotIn [a,b] && Exists

This "simplified/parsed" requirements set could provide a dedicated interface, which can answer e.g. ObjectMatches(obj), Unsatisfiable(), Equal(otherRequirements), GreaterThan(otherRequirements, etc., such that one can semantically compare varying requirements sets with each other (a use case that will be very much needed).

[deads2k]

I believe we should provide a function to "simplify" the requirements into a "canonical representation". For example, if there are multiple rules for the same key, one can apply the following rules:

I don't see a reason to do this at the moment.

[liggitt]

Those are interesting ideas. Simplifying / making canonical could be useful, but I agree it isn't needed for the bits proposed by this KEP.

Places where it could be useful in the future:

1. Optimizing the thing satisfying the list / watch (no point in querying for an unsatisfiable filter, collapsing multiple requirements to a single requirement could make list evaluation more efficient)
2. Determining "covers" checks to see if one selector covers another

A transformer / wrapper that does those simplifications could be implemented as standalone (even out-of-tree) functions if you wanted to do that.

[luxas]

Talking about RawSelector, at the moment we only support Equals/NotEquals/Exists/NotExists in the url encoding, right? As we're looking into this, how do we feel about adding In/NotIn semantics like labelSelector=foo=a|b|c? I guess it's kind of impossible at this point, because we don't limit the content set on label values, so we cannot choose a character that can act as the separator.

Another thing I was thinking is labelSelector=foo=a,foo=b,foo=c, but that ends up being three different requirements, which ANDed together becomes an unsatisfiable requirements set (see the logic below).

I don't know, but it feels like if we start expanding the scope of these selectors, that's a practical issue that instantly arises for consumers.

[deads2k]

Currently every discrete requirement is AND'd when evaluated server-side. I don't see a need to change that behavior.

[liggitt]

Currently every discrete requirement is AND'd when evaluated server-side. I don't see a need to change that behavior.

Agreed. In fact if we do change it from AND we break the ability to expand operators in the future safely.

With AND semantics, a webhook can check for restrictions they understand, short-circuit when they find a limiting one they are satisfied by, ignore ones with operators they don't recognize and treat them as non-limiting, and fail in a safe direction in the presence of unrecognized operators.

it feels like if we start expanding the scope of these selectors, that's a practical issue that instantly arises for consumers.

Given selectors are already part of our API, both in string and structured form, I don't think plumbing the current semantics into authz really expands their scope, especially if we give explicit instructions for how to treat unrecognized operators that makes it safe to accommodate operator expansion in the future.

[enj]

What if a CEL authz call wants to check list with selectors?

[deads2k]

What if a CEL authz call wants to check list with selectors?

Interesting thought. I'll have a think on it.

[deads2k]

I agree this should be addressed and that it's an API change, so it must be done during the alpha stage. As I recall, CEL library modifications take two releases to roll out.

[enj]

Does it actually have to be thread safe?

[deads2k]

Does it actually have to be thread safe?

Yes. I don't want to constrain future due to not adding a sync.Once.

[enj]

Are we sure this is good enough?

[deads2k]

Are we sure this is good enough?

I can't think of what else Get is beyond a single-item list.

[enj]

How do I, as a cluster admin/operator, know exactly what SAR a request was authorized for?

[deads2k]

How do I, as a cluster admin/operator, know exactly what SAR a request was authorized for?

given a request URI, you know the field and label selectors used.

[enj]

We should mention that this is to avoid another CVE-2022-2880 (i.e. getting different systems to agree on how exactly to parse a query is not something we want), see https://www.oxeye.io/resources/golang-parameter-smuggling-attack for more details.

[enj]

?

Suggested change

	// LabelSelectorAttributes indicates a field limited access.
	// LabelSelectorAttributes indicates a label limited access.

[enj]

I feel like this doc should include more details on the node authz part.

[k8s-ci-robot]

@deads2k: 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	936e914	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-sigs/prow repository. I understand the commands that are listed here.