Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return reason for allowed rbac authorizations #58531

Merged
merged 1 commit into from Jan 20, 2018

Conversation

@liggitt
Copy link
Member

@liggitt liggitt commented Jan 19, 2018

includes the binding, role, and subject that allowed a request so audit can make use of it

xref #56209 #58083

example reasons

allowed by ClusterRoleBinding "system:controller:cronjob-controller" of ClusterRole "system:controller:cronjob-controller" to ServiceAccount "cronjob-controller/kube-system"

allowed by RoleBinding "bob-viewer/default" of ClusterRole "view" to User "bob"

perf impact

go test ./plugin/pkg/auth/authorizer/rbac/ -run foo -bench . -benchmem

on master:

BenchmarkAuthorize/allow_list_pods-8         	  500000	      2674 ns/op	    1632 B/op	      27 allocs/op
BenchmarkAuthorize/allow_update_pods/status-8         	  500000	      2858 ns/op	    1632 B/op	      27 allocs/op
BenchmarkAuthorize/forbid_educate_dolphins-8          	  500000	      2654 ns/op	    1632 B/op	      27 allocs/op

with this PR:

BenchmarkAuthorize/allow_list_pods-8         	  500000	      2697 ns/op	    1664 B/op	      28 allocs/op
BenchmarkAuthorize/allow_update_pods/status-8         	  500000	      2873 ns/op	    1680 B/op	      29 allocs/op
BenchmarkAuthorize/forbid_educate_dolphins-8          	  500000	      2687 ns/op	    1664 B/op	      28 allocs/op
NONE
@deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 19, 2018

/lgtm

Loading

@liggitt
Copy link
Member Author

@liggitt liggitt commented Jan 19, 2018

/hold

cleaning up the subject printing a little

Loading

@liggitt
Copy link
Member Author

@liggitt liggitt commented Jan 19, 2018

/hold cancel

Loading

@@ -258,7 +258,7 @@ func TestAppliesTo(t *testing.T) {
}

for _, tc := range tests {
got := appliesTo(tc.user, tc.subjects, tc.namespace)
_, got := appliesTo(tc.user, tc.subjects, tc.namespace)
Copy link
Member

@enj enj Jan 19, 2018

Choose a reason for hiding this comment

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

Check the idx in tests?

Loading

@enj
Copy link
Member

@enj enj commented Jan 19, 2018

@liggitt why not use more structured output instead of a giant string?

Loading

includes the binding, role, and subject that allowed a request so audit can make use of it
@liggitt
Copy link
Member Author

@liggitt liggitt commented Jan 19, 2018

@liggitt why not use more structured output instead of a giant string?

  • we already have plumbing for a string reason
  • putting structure inside a string is fragile and makes the interface misleading
  • the same interface needs to be remoteable (via subject access review)
  • the intended consumer of this (audit) is unlikely to nest structured data from arbitrary authorizers. see #58143 for discussion of a way for authorizers/admission plugins to contribute unstructured attributes to an audit event

Loading

@enj
Copy link
Member

@enj enj commented Jan 19, 2018

@liggitt 👍

/lgtm

Loading

@liggitt
Copy link
Member Author

@liggitt liggitt commented Jan 19, 2018

/retest

Loading

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jan 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj, liggitt

Associated issue: #56209

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Loading

@k8s-github-robot
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Jan 20, 2018

Automatic merge from submit-queue (batch tested with PRs 53895, 58013, 58466, 58531, 58535). If you want to cherry-pick this change to another branch, please follow the instructions here.

Loading

@k8s-github-robot k8s-github-robot merged commit c1d8f71 into kubernetes:master Jan 20, 2018
13 checks passed
Loading
@liggitt liggitt deleted the rbac-reason branch Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants