Switch RBAC escalation check to use active authorizer by liggitt · Pull Request #56358 · kubernetes/kubernetes

liggitt · 2017-11-24T22:57:02Z

RBAC: all configured authorizers are now checked to determine if an RBAC role or clusterrole escalation (setting permissions the user does not currently have via RBAC) is allowed. This is done by checking if the user is authorized to perform the "escalate" verb on the "roles" or "clusterroles" resource.

liggitt · 2017-11-24T22:59:25Z

cc @kubernetes/sig-auth-pr-reviews

liggitt · 2017-11-24T22:59:43Z

cc @cjcullen

jhorwit2 · 2017-11-25T04:01:23Z

pkg/registry/rbac/escalation_check.go

Should these errors be logged?

liggitt · 2017-11-25T08:46:08Z

/retest

liggitt · 2017-11-25T14:21:52Z

/retest

deads2k · 2017-11-27T13:08:55Z

/approve

deads2k · 2017-11-27T13:14:48Z

lgtm. I have no preference about logging since its a wholely internal request that attempts a secondary path on errors, but I'll leave it to you.

enj · 2017-11-27T13:26:02Z

LGTM

enj · 2017-11-27T13:29:30Z

A GKE test showing this works would be nice.

liggitt · 2017-11-27T16:46:16Z

/retest

liggitt · 2017-11-27T16:47:19Z

A GKE test showing this works would be nice.

@cjcullen thoughts on how to write this? as far as I can tell, the e2e user is given superuser permissions by the cluster setup, and service accounts don't have GKE IAM permissions

cjcullen · 2018-05-02T20:44:07Z

pkg/registry/rbac/escalation_check.go

Do these return false lines merit a log (at least an Info.V4)? If we can't pull a user/requestInfo off the context in this call, something is broken, right?

cjcullen · 2018-05-02T20:49:08Z

pkg/registry/rbac/escalation_check.go

Do we care about the reason?

cjcullen · 2018-05-02T20:57:12Z

pkg/registry/rbac/clusterrole/policybased/storage_test.go

Might help the upkeep of these tests to add a little meat to the comments (like if we decide to switch the order of the escalation checks or something).

// Superuser: Can create/update role w/o escalation, so authorizer is not called, and create/update call should succeed. // Unauthorized: Bob must escalate to create/update the role. When he doesn't have "escalate" permission, the calls should fail. // Authorized: Bob must escalate to create/update the role. When he does have "escalate permission, the calls should succeed. // Implicitly authorized: Alice ...

Actually, I wasn't sure why the Alice thing worked the way it did. I'll take another look in a little bit.

made it a table test, added more descriptions

Awesome. That's even better.

cjcullen · 2018-05-05T00:00:14Z

Pretty much LGTM. I still want to bikeshed on where the verb should live (and if "escalate" is the right name). I think we should bring it up at the next sig-auth.

cjcullen · 2018-06-05T18:42:01Z

/lgtm

fejta-bot · 2018-06-05T22:11:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T00:59:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T03:47:45Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T06:35:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T09:44:20Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T12:53:18Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T15:41:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-06T18:29:18Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

…ion checks

cjcullen · 2018-06-08T23:23:04Z

/lgtm

k8s-ci-robot · 2018-06-08T23:23:12Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [deads2k,liggitt]
~~pkg/kubectl/OWNERS~~ [deads2k,liggitt]
~~pkg/registry/OWNERS~~ [deads2k,liggitt]

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

fejta-bot · 2018-06-11T10:49:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-06-20T03:13:56Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@cjcullen @deads2k @ericchiang @liggitt

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.12 milestone.

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

k8s-github-robot · 2018-06-20T12:48:21Z

Automatic merge from submit-queue (batch tested with PRs 64688, 64451, 64504, 64506, 56358). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned enj and deads2k Nov 24, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 24, 2017

liggitt added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 24, 2017

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Nov 24, 2017

liggitt assigned ericchiang and unassigned enj Nov 24, 2017

liggitt force-pushed the rbac-alternate-authorizer branch 2 times, most recently from f961f68 to cab5f75 Compare November 25, 2017 03:04

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2017

jhorwit2 reviewed Nov 25, 2017

View reviewed changes

pkg/registry/rbac/escalation_check.go Outdated

Copy link
Copy Markdown

Contributor

jhorwit2 Nov 25, 2017

Choose a reason for hiding this comment

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

Should these errors be logged?

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2017

liggitt force-pushed the rbac-alternate-authorizer branch from cab5f75 to 30728d3 Compare November 27, 2017 15:03

liggitt added this to the v1.10 milestone Nov 28, 2017

liggitt assigned cjcullen Dec 14, 2017

liggitt force-pushed the rbac-alternate-authorizer branch from e3b4b73 to 2719b6a Compare May 2, 2018 20:40

cjcullen reviewed May 2, 2018

View reviewed changes

liggitt force-pushed the rbac-alternate-authorizer branch from 2719b6a to 1790611 Compare May 3, 2018 03:14

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 3, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

Allow non-RBAC authorizers to participate in role/clusterrole escalat…

1034efd

…ion checks

liggitt force-pushed the rbac-alternate-authorizer branch from 1790611 to 1034efd Compare June 6, 2018 19:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2018

liggitt added this to the v1.12 milestone Jun 20, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 20, 2018

k8s-github-robot merged commit d9272f8 into kubernetes:master Jun 20, 2018

liggitt deleted the rbac-alternate-authorizer branch June 28, 2018 20:34

liggitt mentioned this pull request Aug 21, 2018

Document cross-authorizer permissions for creating RBAC roles kubernetes/website#10015

Merged

Conversation

liggitt commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Nov 24, 2017

Uh oh!

liggitt commented Nov 24, 2017

Uh oh!

jhorwit2 Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt commented Nov 25, 2017

Uh oh!

liggitt commented Nov 25, 2017

Uh oh!

deads2k commented Nov 27, 2017

Uh oh!

deads2k commented Nov 27, 2017

Uh oh!

enj commented Nov 27, 2017

Uh oh!

enj commented Nov 27, 2017

Uh oh!

liggitt commented Nov 27, 2017

Uh oh!

liggitt commented Nov 27, 2017

Uh oh!

cjcullen May 2, 2018

Choose a reason for hiding this comment

Uh oh!

cjcullen May 2, 2018

Choose a reason for hiding this comment

Uh oh!

cjcullen May 2, 2018

Choose a reason for hiding this comment

Uh oh!

liggitt May 3, 2018

Choose a reason for hiding this comment

Uh oh!

cjcullen May 4, 2018

Choose a reason for hiding this comment

Uh oh!

cjcullen commented May 5, 2018

Uh oh!

cjcullen commented Jun 5, 2018

Uh oh!

fejta-bot commented Jun 5, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

fejta-bot commented Jun 6, 2018

Uh oh!

cjcullen commented Jun 8, 2018

Uh oh!

k8s-ci-robot commented Jun 8, 2018

Uh oh!

fejta-bot commented Jun 11, 2018

Uh oh!

k8s-github-robot commented Jun 20, 2018

Uh oh!

k8s-github-robot commented Jun 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

liggitt commented Nov 24, 2017 •

edited

Loading