Update Covers to handle AttributeRestrictions#13415
Update Covers to handle AttributeRestrictions#13415openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
|
looks reasonable. |
|
did we remove all rules with attribute restrictions from bootstrap policy already? |
There was a todo somewhere. @enj this is a reasonable pull to do it in. |
|
it has to be done here, otherwise reconciliation will keep re-adding those rules, since they'll show up as uncovered |
The unit tests I wrote should fail if that was the case =\ |
The rule doesn't do anything. It's always covered because it's a rule that grants no permissions. A rule with attributerestrictions is completely ignored by the authorizer. |
|
ah, do we breakdown both existing and expected rules? |
His update here doesn't allow a rule with attributerestrictions to cover anything and when it breaks down a rule with attributestrictions, it returns an empty slice of rules. |
enj
force-pushed
the
enj/i/covers_attribute_restrictions
branch
from
6eeac4e to
607e970
Compare
|
@deads2k I removed all the rules with |
|
lgtm[merge] |
openshift-bot
added
the
needs-rebase
Signed-off-by: Monis Khan <mkhan@redhat.com>
enj
force-pushed
the
enj/i/covers_attribute_restrictions
branch
from
607e970 to
8c66c8d
Compare
enj
removed
the
needs-rebase
|
[merge] |
|
Flake #13426 |
|
re[merge] |
|
re[test] |
|
Evaluated for origin test up to 8c66c8d |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/292/) (Base Commit: 9e19032) |
|
Evaluated for origin merge up to 8c66c8d |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/133/) (Base Commit: bd0bf10) (Image: devenv-rhel7_6086) |
| case UserAccessCheck: | ||
| return []authorizationapi.PolicyRule{ | ||
| authorizationapi.NewRule("create").Groups(kauthorizationapi.GroupName).Resources("selfsubjectaccessreviews").RuleOrDie(), | ||
| {Verbs: sets.NewString("create"), APIGroups: []string{authorizationapi.GroupName, authorizationapi.LegacyGroupName}, Resources: sets.NewString("subjectaccessreviews", "localsubjectaccessreviews"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}}, |
There was a problem hiding this comment.
This change broke the ability of a client to request a subject access review without scopes. Scopes aren't in upstream Kube, and as a result, you can no longer use a token that only has "user:info user:check-access" scopes to do a SAR that tells you what the user can do. Which breaks the ability to use this as a proxy.
If I can't do SAR and take scopes into account, then we are regressing functionality and shouldn't remove this until we have an equivalent behavior in Kube. If there's a workaround I don't see it - couldn't find anything that allowed a separate channel for passing scopes.
There was a problem hiding this comment.
to use as a proxy, without granting a token that gives you the ability to act as the full user.
There was a problem hiding this comment.
Upstream has a dedicated resource for SelfSubjectAccessReview… no attribute restrictions needed
There was a problem hiding this comment.
I can't pass empty scopes to check permissions on it like i can today.
|
Ah. All the existing openshift APIs should still be usable and work identically. If they don't, that's a bug we have to fix. The goal of this was not to change that but to remove dependence on AttributeRestrictions. The special case of IsPersonalSubjectAccessReview got shifted from being embedded in the role attribute restrictions to being treated by the authorizer as a SelfSubjectAccessReview resource for authorization purposes. That should all be internal impl details… if anything leaked or broke, we'll need to resolve it. |
|
I think the gap is that a user:check-access user should be able to do the
equivalent of
POST /apis/authorization.openshift.io/subjectaccessreviews
{"scopes": [], "namespace": "foo", "resource": "pods", "verb": "get"}
as themselves. Without an OpenShift "selfsubjectaccessreview", or the
authorizer allowing "subjectaccessreview" only for user unspecified
(meaning current user), there's no way to make the previous call.
…On Tue, Apr 11, 2017 at 4:19 AM, Jordan Liggitt ***@***.***> wrote:
Ah. All the existing openshift APIs should still be usable and work
identically. If they don't, that's a bug we have to fix.
The goal of this was not to change that but to remove dependence on
AttributeRestrictions. The special case of IsPersonalSubjectAccessReview
got shifted from being embedded in the role attribute restrictions to being
treated by the authorizer as a SelfSubjectAccessReview resource for
authorization purposes. That should all be internal impl details… if
anything leaked or broke, we'll need to resolve it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13415 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p1236Nc08CzEuobuiCeE98xf-wJ5ks5ruuMvgaJpZM4MfrT1>
.
|
|
This changed the way the permission is expressed, but did not change the way the API works. The openshift self-SAR endpoint is still available and accessible. The permission used for the check is the kube self-sar, not the openshift self-sar, but the old endpoint is still there. |
This is something that the upstream self-SAR can't do, but this pull did not remove the ability to use the old openshift API endpoint (or it wasn't intended to at any rate). The openshift endpoint checks the kube self-SAR permission. |
|
The openshift endpoint isn't in the check-access scope, which this PR
removed on this line. Or at least, I was unable to run it and got a
scope denial error.
|
|
Fixed in #13715 |
5 participants
Fixes #13312
Needed to finish #13334
cc @liggitt
[test]
Signed-off-by: Monis Khan mkhan@redhat.com