Skip to content

authorize personalSAR based on selfsubjectaccessreviews.authorization.k8s.io#13256

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:auth-04-requestattributes
Mar 10, 2017
Merged

authorize personalSAR based on selfsubjectaccessreviews.authorization.k8s.io#13256
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:auth-04-requestattributes

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 6, 2017

This eliminates our usage of the AttributeRestrictions for policy rules and the matching authorization attributes. It helps us collapse onto the upstream authorizer interface.

Instead of authorizing using the AttributeRestrictions, the requestInfoResolver uses the information to lie about the resource being accessed. When its a personalSAR or personalLSAR, the requestInfo is written to selfsubjectaccessreviews.authorization.k8s.io

@liggitt This is what we spoke about
@enj ptal
@mfojtik @soltysh fyi

@deads2k
Copy link
Contributor Author

deads2k commented Mar 6, 2017

[test]

@deads2k deads2k force-pushed the auth-04-requestattributes branch 2 times, most recently from 1e245e5 to 891ef8b Compare March 6, 2017 19:19
Copy link
Contributor

@enj enj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits.

Opened #13312 to cleanup policy rules.

func (a *personalSARRequestInfoResolver) NewRequestInfo(req *http.Request) (*request.RequestInfo, error) {
requestInfo, err := a.infoFactory.NewRequestInfo(req)
if err != nil {
return requestInfo, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return nil, err?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, The upstream function docs like this: "If error is not nil, RequestInfo holds the information as best it is known before the failure", so that at least some error comes back that might make sense.


default:
return false, fmt.Errorf("unexpected request attributes for checking personal access review: %v", extendedAttributes)
// only match SAR and LSAR requests for personal review
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch?

@enj
Copy link
Contributor

enj commented Mar 8, 2017

Having recently read the pile of tests we have for the SAR stuff, I am quite confident that this is good 😄

@deads2k
Copy link
Contributor Author

deads2k commented Mar 8, 2017

@enj is that like "minor comments, then LGTM"?

@enj
Copy link
Contributor

enj commented Mar 8, 2017

Yup.

@deads2k deads2k force-pushed the auth-04-requestattributes branch from 891ef8b to e3f7aaa Compare March 9, 2017 12:43
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e3f7aaa

@stevekuznetsov
Copy link
Contributor

Removing merge tag -- seeing issues with node dep starting, fix is probably in #13317 so let's let that go first.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/56/) (Base Commit: c9ed4e5)

@stevekuznetsov
Copy link
Contributor

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to e3f7aaa

@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 9, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/23/) (Base Commit: 39ceb75) (Image: devenv-rhel7_6062)

@openshift-bot openshift-bot merged commit a28df2f into openshift:master Mar 10, 2017
@deads2k deads2k deleted the auth-04-requestattributes branch August 3, 2017 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants