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

kubelet authn/authz #34381

Merged
merged 5 commits into from
Oct 26, 2016
Merged

kubelet authn/authz #34381

merged 5 commits into from
Oct 26, 2016

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Oct 8, 2016

Implements https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/kubelet-auth.md

Part of Authenticated/Authorized access to kubelet API feature

Access to the Kubelet HTTPS port can now be authenticated using X509 client certificates or API bearer tokens, and anonymous access can be disallowed. Access to the Kubelet HTTPS port can also be authorized using the same authorization configured for the API server.

This change is Reviewable

@liggitt liggitt added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Oct 8, 2016
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Oct 8, 2016
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 10, 2016
@liggitt liggitt changed the title WIP - kubelet authn/authz kubelet authn/authz Oct 10, 2016
@liggitt
Copy link
Member Author

liggitt commented Oct 10, 2016

initial implementation of Authenticated/Authorized access to kubelet API feature

cc @kubernetes/sig-auth @kubernetes/sig-node

@deads2k
Copy link
Contributor

deads2k commented Oct 10, 2016

Can you split out "Allow webhook authorizer to use SubjectAccessReviewInterface" and we can get it in separately?

@liggitt
Copy link
Member Author

liggitt commented Oct 10, 2016

Can you split out "Allow webhook authorizer to use SubjectAccessReviewInterface" and we can get it in separately?

Already in #34071, as noted

ClientCAFile string `json:"clientCAFile"`

// enableWebhookToken enables bearer token authentication using the tokenreviews.authentication.k8s.io API
EnableWebhookToken bool `json:"enableWebhookToken"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Group the two webhook related fields into a substruct to keep the associated. Also, add indications that this functions against the API server.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

attrs.Subresource = "metrics"
case isSubpath(requestPath, logsPath):
attrs.Verb = apiVerb
attrs.Subresource = "log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading through this I am confused why this is log vs logs. stats above matches the resource /stats/

Copy link
Member Author

Choose a reason for hiding this comment

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

trying to match existing log subresources (pods/log), to try to avoid typos in policy that controls both

@philips
Copy link
Contributor

philips commented Oct 13, 2016

overall looks fine. the subresource == log vs path == logs is confusing but if correct deserves a comment.

// This allows subdividing access to the kubelet API
switch {
case isSubpath(requestPath, statsPath):
attrs.Verb = apiVerb

Choose a reason for hiding this comment

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

nit: Verb is already initialized to apiVerb above

@@ -96,6 +96,29 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
fs.Var(componentconfig.IPVar{Val: &s.Address}, "address", "The IP address for the Kubelet to serve on (set to 0.0.0.0 for all interfaces)")
fs.Int32Var(&s.Port, "port", s.Port, "The port for the Kubelet to serve on.")
fs.Int32Var(&s.ReadOnlyPort, "read-only-port", s.ReadOnlyPort, "The read-only port for the Kubelet to serve on with no authentication/authorization (set to 0 to disable)")

// Authentication
fs.BoolVar(&s.Authentication.Anonymous.Enabled, "anonymous-auth", s.Authentication.Anonymous.Enabled, ""+
Copy link
Contributor

Choose a reason for hiding this comment

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

This defaults on?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

"Enables anonymous requests to the Kubelet server. Requests that are not rejected by another "+
"authentication method are treated as anonymous requests. Anonymous requests have a username "+
"of system:anonymous, and a group name of system:unauthenticated.")
fs.BoolVar(&s.Authentication.Webhook.Enabled, "authentication-token-webhook", s.Authentication.Webhook.Enabled, ""+
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this default-on too. One level release skew should be safe since we supported the endpoint in 1.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

we shouldn't default this on if the corresponding API isn't defaulted on

Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't default this on if the corresponding API isn't defaulted on

beta API that I thought I turned on by default. It's not?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, the backing API is on. However, the kubelet isn't guaranteed to have an API client. I can look into dynamically enabling it as a follow up, but the stateless default has to be off, I think

// KubeletAuthorizationModeAlwaysAllow authorizes all authenticated requests
KubeletAuthorizationModeAlwaysAllow KubeletAuthorizationMode = "AlwaysAllow"
// KubeletAuthorizationModeWebhook uses the SubjectAccessReview API to determine authorization
KubeletAuthorizationModeWebhook KubeletAuthorizationMode = "Webhook"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems more, "ClusterBased" or something right? As opposed to a generic webhook they get to choose a destination on.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems more, "ClusterBased" or something right? As opposed to a generic webhook they get to choose a destination on.

the remote authz check runs against the kubelet's apiserver by default, but some deployments will not enable that API. I want to retain the future ability to point this at the same webhook the apiserver supports without duplicating all the associated cache flags.

// x509 contains settings related to x509 client certificate authentication
X509 KubeletX509Authentication `json:"x509"`
// webhook contains settings related to webhook bearer token authentication
Webhook KubeletWebhookAuthentication `json:"webhook"`
Copy link
Contributor

Choose a reason for hiding this comment

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

apiserver token based as opposed to generic webhook, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

apiserver token based as opposed to generic webhook, isn't it?

same reasoning as the remote authorizer

@deads2k
Copy link
Contributor

deads2k commented Oct 13, 2016

relatively minor comments. Core code looks good.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 1e87267b2b3e575fd0ede772b588e5d2f0a3b8bf. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2016
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 20, 2016
@liggitt liggitt added this to the v1.5 milestone Oct 25, 2016
@liggitt liggitt force-pushed the kubelet-auth branch 2 times, most recently from 05698df to 0b4c46b Compare October 25, 2016 06:15
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 0b4c46bfdd2d6abf5ce2448fe9bc81911c7d87ca. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 137a304f5256ab420875050d90910f97e5d22c73. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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

deads2k commented Oct 25, 2016

lgtm

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a898f3d into kubernetes:master Oct 26, 2016
@liggitt liggitt deleted the kubelet-auth branch October 27, 2016 05:37
@erictune
Copy link
Member

erictune commented Nov 2, 2016

@cjcullen did you see this.

@erictune
Copy link
Member

erictune commented Nov 2, 2016

@alex-mohr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants