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

Proposal: kubelet authentication/authorization #32518

Merged
merged 1 commit into from
Oct 7, 2016

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Sep 12, 2016

Proposal for kubernetes/enhancements#89

This change is Reviewable

@liggitt
Copy link
Member Author

liggitt commented Sep 12, 2016

cc @kubernetes/sig-auth @kubernetes/sig-node @roberthbailey @mikedanese @cjcullen @erictune @kayrus @deads2k

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 12, 2016
@luxas
Copy link
Member

luxas commented Sep 12, 2016

cc @kubernetes/sig-cluster-lifecycle


This proposal assumes the existence of:

* a functioning API server
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a chicken-egg supposition. /cc @mikedanese

Copy link
Member Author

@liggitt liggitt Sep 12, 2016

Choose a reason for hiding this comment

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

Nope, you don't need to call the kubelet API to bootstrap an apiserver with a kubelet

@k8s-bot
Copy link

k8s-bot commented Sep 12, 2016

GCE e2e build/test passed for commit cf6efb2aabdae41e6816eed84299003a4892c5b7.

@philips
Copy link
Contributor

philips commented Sep 12, 2016

This is something we want to get fixed up in v1.5. cc @ericchiang @aaronlevy @yifan-gu @euank @pbx0


### Anonymous

Add a new `--anonymous-auth=[true|false]` option to the kubelet.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is in use in various places, is this the first time this is part of Kube?

Copy link
Member Author

Choose a reason for hiding this comment

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

part of #32386 for the apiserver

@smarterclayton
Copy link
Contributor

Looks fairly similar to what works already in OpenShift - the other flags look consistent with our current uses in apiserver. Bootstrapping may in the future want to play in this mode, although I prefer bootstrapping to be a pre-stage to full kubelet startup and the kubelet to be required to transition from bootstrap into one of these modes explicitly.

k8s-github-robot pushed a commit that referenced this pull request Sep 13, 2016
Automatic merge from submit-queue

Allow webhook authenticator to use TokenReviewsInterface

Refactors the authentication webhook to be able to be fed a kubeconfig file or a TokenReviewsInterface 

Fixes a bug with webhooks not retrying on 429/500 errors

Related to #32518 (comment)
@liggitt liggitt added this to the v1.5 milestone Sep 21, 2016
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 22, 2016
@erictune
Copy link
Member

@alex-mohr @roberthbailey @cjcullen
I think you are interested in kubelet API security.

The master API server can already be started with `--kubelet-client-certificate` and
`--kubelet-client-key` options in order to make authenticated requests to the kubelet.

### Bearer token
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a bearer token method really necessary? Are there many scenarios for users accessing the Kubelet API or can we lock this down even further?

Copy link
Member Author

Choose a reason for hiding this comment

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

in openshift, we've been able to eliminate the unsecured kubelet port, and have heapster gather stats and metrics data using a service account token against the secured kubelet port

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm for this particular part of the proposal staying in. Being able to centrally administer what has access to these endpoints is great. And the tooling around issuance and revocation of service accounts is much more built out than our PKI and CRL infrastructure.

@smarterclayton
Copy link
Contributor

The overall proposal accurately represents most of the security discussions we had when locking the Kubelet down in OpenShift and seems broadly in line with our existing apiserver flags and direction. This proposal LGTM - can folks on this proposal who have concerns still do a pass and raise those soon so we can move forward?


## API Changes

None
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible new resources for the SubjectAccessReview?

Copy link
Member Author

Choose a reason for hiding this comment

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

no API changes are needed to support the SubjectAccessReviews the kubelet would submit (it would express the checks in verb and group/version/resource/subresource/name terms)

@ericchiang
Copy link
Contributor

@smarterclayton I'm +1 on this.

@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 4, 2016

LGTM

@smarterclayton
Copy link
Contributor

@erictune any last comments?

@erictune
Copy link
Member

erictune commented Oct 5, 2016

I'd really like one of @cjcullen @alex-mohr @roberthbailey to take a look at this because this relies on SubjectAccessReview and TokenReviews, which we don't expose on GKE.

@cjcullen
Copy link
Member

cjcullen commented Oct 5, 2016

GKE does not enable the SubjectAccessReview and TokenReview APIs because we don't really want to support those as generic Kubernetes APIs. I'm not completely opposed to providing this as an option for kubelet auth, but it feels fishy to me.

We don't want to expose the TokenReview endpoint on GKE, because it could possibly allow 3rd parties to inspect https://www.googleapis.com/auth/cloud-platform OAuth2 tokens.

I don't think overloading SubjectAccessReview for kubelet authz is a great idea. We would be crossing the namespaces of kubelet resources and kubernetes resources. SubjectAccessReview is currently pretty purpose-built for the user-facing Kubernetes API. Do we see the kubelet API becoming more of a user-facing thing? If not, SubjectAccessReview feels like overkill.

I think the x509 option provides the cleanest way forward for GKE. As a first step, we could mint a cert for kube-apiserver and a cert for heapster with a separate CA than the one we use for the apiserver's certs, and then AllowAll inside the kubelet. This would add another place to worry about cert rotation, and would make a compromised heapster a potential avenue to long-lived access to any pod. To mitigate that a bit, we could make authz give read-write to kube-apiserver and read-only to heapster.

LGTM to the proposal (to unblock any work that is waiting). I don't see the webhook options as viable paths forward for GKE (without rethinking other things), but I think the other options here will get us going the right direction. Thanks @liggitt for putting this together.

@smarterclayton
Copy link
Contributor

I'll say our view is somewhat biased because the vast majority of deployed
clusters we've seen lack anything like central google oauth, and if they
did have one, they'd want Kube to manage the cluster infra auth (i.e. they
want the rules applying to kubelet access to be centrally managed on the
cluster). if you don't have a worldwide auth domain, you want your Kube
cluster to be internally consistent and not have separate authorization
paths.

The kubelet API today is user facing in two directions - administrative
level access (logs and metrics) and to heapster. Heapster having
administrative access to nodes is concerning, so at least some role
granularity is required. I suspect other system agents in the future will
require access (monitoring like Kube-snap).

I don't think it's unreasonable that the authorizer on the kubelet have
multiple paths (which include cert auth and SAR) and different deployments
get to make that choice, just like the masters.

On Oct 4, 2016, at 8:22 PM, Eric Tune notifications@github.com wrote:

I'd really like one of @cjcullen https://github.com/cjcullen @alex-mohr
https://github.com/alex-mohr @roberthbailey
https://github.com/roberthbailey to take a look at this because this
relies on SubjectAccessReview and TokenReviews, which we don't expose on
GKE.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32518 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p4fnkdelS-4XG3XkK4RnNHHmuIQlks5qwu2mgaJpZM4J6_J8
.

@smarterclayton
Copy link
Contributor

If no other comments, LGTM. Thanks

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2016
Copy link
Contributor

@jbeda jbeda left a comment

Choose a reason for hiding this comment

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

Small tweak. Hopefully not disruptive.


### x509 client certificate

Add a new `--client-ca-file=[file]` option to the kubelet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick comment -- any place where we have a CA certificate we should support a bundle or a set of certificates. Otherwise it is almost impossible to do key rotation.

We should also make sure that we plan to have this stuff driven by component configs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. File could be a bundle, just like the API server. Also, we'd hook up the CRL bits here as soon as we have them worked out for the apiserver authenticator.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f93e01d into kubernetes:master Oct 7, 2016
@liggitt liggitt deleted the kubelet-auth-proposal branch October 8, 2016 01:05
k8s-github-robot pushed a commit that referenced this pull request Oct 12, 2016
Automatic merge from submit-queue

Allow webhook authorizer to use SubjectAccessReviewInterface

Refactors the authorization webhook to be able to be fed a kubeconfig file or a SubjectAccessReviewsInterface 

Added tests to exercise retry logic, and ensure the correct serialized version is sent to the remote webhook (I also made sure the new tests passed on the current webhook impl in master)

c.f. #32547
c.f. #32518
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Automatic merge from submit-queue

Proposal: kubelet authentication/authorization

Proposal for kubernetes/enhancements#89
tamalsaha pushed a commit to kmodules/authorizer that referenced this pull request Nov 17, 2021
Automatic merge from submit-queue

Allow webhook authorizer to use SubjectAccessReviewInterface

Refactors the authorization webhook to be able to be fed a kubeconfig file or a SubjectAccessReviewsInterface 

Added tests to exercise retry logic, and ensure the correct serialized version is sent to the remote webhook (I also made sure the new tests passed on the current webhook impl in master)

c.f. kubernetes/kubernetes#32547
c.f. kubernetes/kubernetes#32518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.