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

Switch RBAC escalation check to use active authorizer #56358

Merged
merged 1 commit into from Jun 20, 2018

Conversation

@liggitt
Copy link
Member

liggitt commented Nov 24, 2017

Closes #43409

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

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 24, 2017

@liggitt liggitt assigned ericchiang and unassigned enj Nov 24, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 24, 2017

@liggitt liggitt force-pushed the liggitt:rbac-alternate-authorizer branch 2 times, most recently from f961f68 to cab5f75 Nov 24, 2017

Verb: "*",
Path: "*",
})
if err != nil || decision != authorizer.DecisionAllow {

This comment has been minimized.

@jhorwit2

jhorwit2 Nov 25, 2017

Member

Should these errors be logged?

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 25, 2017

/retest

1 similar comment
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 25, 2017

/retest

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Nov 27, 2017

/approve

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Nov 27, 2017

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

This comment has been minimized.

Copy link
Member

enj commented Nov 27, 2017

LGTM

@enj

This comment has been minimized.

Copy link
Member

enj commented Nov 27, 2017

A GKE test showing this works would be nice.

@liggitt liggitt force-pushed the liggitt:rbac-alternate-authorizer branch from cab5f75 to 30728d3 Nov 27, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 27, 2017

/retest

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 27, 2017

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

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


user, ok := genericapirequest.UserFrom(ctx)
if !ok {
return false

This comment has been minimized.

@cjcullen

cjcullen May 2, 2018

Member

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?

ResourceRequest: true,
}

decision, _, err := a.Authorize(attrs)

This comment has been minimized.

@cjcullen

cjcullen May 2, 2018

Member

Do we care about the reason?


s := NewStorage(fakeStorage, fakeAuthorizer, fakeRuleResolver)

// Superuser

This comment has been minimized.

@cjcullen

cjcullen May 2, 2018

Member

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.

This comment has been minimized.

@liggitt

liggitt May 3, 2018

Author Member

made it a table test, added more descriptions

This comment has been minimized.

@cjcullen

cjcullen May 4, 2018

Member

Awesome. That's even better.

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

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XL labels May 3, 2018

@cjcullen

This comment has been minimized.

Copy link
Member

cjcullen commented May 5, 2018

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

This comment has been minimized.

Copy link
Member

cjcullen commented Jun 5, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 5, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 5, 2018

/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.

7 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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

This comment has been minimized.

Copy link

fejta-bot commented Jun 6, 2018

/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.

@liggitt liggitt force-pushed the liggitt:rbac-alternate-authorizer branch from 1790611 to 1034efd Jun 6, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 6, 2018

@cjcullen

This comment has been minimized.

Copy link
Member

cjcullen commented Jun 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 8, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 8, 2018

[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

Needs approval from an approver in each of these files:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 11, 2018

/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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 20, 2018

[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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 20, 2018

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 k8s-github-robot merged commit d9272f8 into kubernetes:master Jun 20, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@liggitt liggitt deleted the liggitt:rbac-alternate-authorizer branch Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.