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

allow delegated authorization to have privileged groups #70671

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 5, 2018

In kube, system:masters is a special group that has full API powers. We use this group to be able to have kube-apiservers loopback with full power and to allow management of RBAC using a client cert with that group.

Since DelegatedAuthorization is intended to be consistent with kube recommendations, this updates the default value to include an authorizer that lists system:masters as having full rights. This means that system:masters users will not have to have a remote authz call which improves performance and makes debugging very broken conditions possible.

@smarterclayton
@kubernetes/sig-api-machinery-pr-reviews @kubernetes/sig-auth-pr-reviews

delegated authorization can now allow unrestricted access for `system:masters` like the main kube-apiserver

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 5, 2018
}

func NewDelegatingAuthorizationOptions() *DelegatingAuthorizationOptions {
return &DelegatingAuthorizationOptions{
// very low for responsiveness, but high enough to handle storms
AllowCacheTTL: 10 * time.Second,
DenyCacheTTL: 10 * time.Second,

// in a kube cluster, system:masters has full power. Use this as the default which could be later changed.
AlwaysAllowGroups: []string{user.SystemPrivilegedGroup},
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm on board with defaulting to a value that allows coasting, but I expected a flag wired to this to surface it and allow control

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm on board with defaulting to a value that allows coasting, but I expected a flag wired to this to surface it and allow control

This seems like a setting that individual binaries can choose to control how they like. I think a very small minority (if any) would want to allow an admin to control it and those binaries could choose to wire the value to whatever flag they like.

@mikedanese
Copy link
Member

I'm worried that:

  • This is a leak of an RBAC specific detail into the scope of general authorization
  • Bypasses short-circuit deny authorizers

@liggitt
Copy link
Member

liggitt commented Nov 6, 2018

  • This is a leak of an RBAC specific detail into the scope of general authorization
  • Bypasses short-circuit deny authorizers

the same superuser exists in the kube-apiserver as the pinned first authorizer in the chain, independent of RBAC and prior to any configured authorizers

@mikedanese
Copy link
Member

Ahh... loopback stuff... I have similar concerns with that flow. While we couldn't make the same guarantees around preventing misconfiguration, I would strongly prefer if the loopback authorizer was appended to the authorizer chain rather than prepended. The usecase was outlined here #51862

@mikedanese
Copy link
Member

mikedanese commented Nov 6, 2018

I'm primarily concerned with introducing well known user/group strings that are capable of unilateral bypassing authorization when operators generally have little control over the users and groups of credentials (namely those provisioned by the certificates API).

I would strongly prefer if the loopback authorizer was appended to the authorizer chain rather than prepended.

Alternatively, we could enforce that the loopback authorizer could only authorize request from loopback addresses. Would this approach address the issues raised here? e.g. administrator needs port forward AND a system:master credential to bypass authorization of apiservers running in the cluster and apiservers running outside the cluster can be protected?

@deads2k
Copy link
Contributor Author

deads2k commented Nov 6, 2018

I can make it optional, but I think that prepending is the correct default choice. This is already what the kube-apiserver does, so this is purely an optimization on asking. Again, a user of this API can wire or disable this in any way that they like and removing the default makes it purely opt-in.

@deads2k
Copy link
Contributor Author

deads2k commented Nov 6, 2018

Default removed. If someone requests it, then it will function as desired.

@liggitt
Copy link
Member

liggitt commented Nov 6, 2018

/kind feature
/priority important-soon
/lgtm
/approve

needs release note

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@k8s-ci-robot 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 6, 2018
@mikedanese
Copy link
Member

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants