Implement support for references in permission claims #3670
+632
−61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kcp-ci-bot
added
kind/feature
|
Skipping CI for Draft Pull Request. |
kcp-ci-bot
added
the
size/M
The old code would loop through all bound claims and for each matching (group/resource) claim it would try to find the first (!) matching exported claim. And then check the verbs. If no exported claim is found, the set of allowed verbs is empty, so the HasAny() check can never succeed. Hence if no exported claim exists, we do not even have to check the verbs. This commit improves the loop by comparing all bound claims against all (!) exported claims (with the matching GRI) and skipping any guaranteed-false checks if no exported claim is found. This loop now is ready to be extended to check for references, too. On-behalf-of: @SAP [email protected]
…dynrestmapper and dynclusterclient The DDSIF is metadata-only and cannot be used to get informers for bound resources (i.e. those resources that would be allowed to be named in permissionClaim.selector.references[].group/resource (this validation has not been implemented yet in this branch)), so as a last resort, this approach will just fetch the objects directly. This commit is a WIP since the remaining code for the reference matching hasn't been done yet. On-behalf-of: @SAP [email protected]
xrstf
force-pushed
the
permclaim-references
branch
from
bdd0a7c to
5255489
Compare
kcp-ci-bot
added
the
kind/api-change
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
kcp-ci-bot
added
size/XL
|
I pushed my branch into this repository, but cannot change this PR and am loo tazy to create a new PR just to instantly close it. 😁 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR attempts to implement reference-based access to claimed resources in the apiexport virtual workspace. Where instead of the APIBinding directly naming the (for example) allowed Secrets to access, it would instead say "allow all secrets that are mentioned in this JSONPath in the following other resources". Think of allowing all Secrets that are mentioned in
spec.secretNamein Certificate objects.The idea is to handle most of the access control by extending the permissionclaim labeler, which will calculate for each and every claimed object, whether that object should be included in an APIExport's virtual workspace. The resullt ofthat calculation is then stored as a label and this label is silently injected into requests to the apiexport virtual workspace, thereby filtering the objects down to the desired set.
With
matchAll, this is easy, just label all the claimed objects. For label selectors this is also still somewhat easy because for calcualting if one object should be labeled, only that one object needs to be looked at.However for references, suddenly to calculate the label for objects of resource X, we need to list all objects of resource Y (in full, not just their metadata) and evaluate JSONPaths against all of them to determine the set of allowed object names. This means the labels will need to be recalculated for all X objects whenever any Y object changes.
In effect that means kcp would need to either (like in this primitive unfinished PR) list all the Y objects dynamically or keep runtime informers around for basically all bound resources. And changes to one object can suddenly balloon into label changes on many objects (this was already true with APIBindings, but those change much less frequently than the "interesting" workload resources that users bound into their workspaces), and also changes to a claimed object (resource X) will force kcp to re-evaluate all Y objects.
None of this touches yet on the authorization that would need to happen for when objects are created (I am not sure when a new object is labeled and when authz for that action happens).
Since we currently have no strong usecase for this feature and fear the implications in terms of complexity and performance, we abandoned this approach for now.
What Type of PR Is This?
/kind feature
Related Issue(s)
Fixes #3409
Release Notes