Kubelet API Certificate #3125

gambol99 · 2017-08-02T20:31:29Z

A while back options to permit secure kube-apiserver to kubelet api was PR2381 using the server.cert and server.key as testing grounds. This PR formalizes the options and generates a client certificate on their behalf (note, the server{.cert,key} can no longer be used post 1.7 as the certificate usage is checked i.e. it's not using a client cert). The users now only need to add anonymousAuth: false to enable secure api to kubelet. I'd like to make this default to all new builds i'm not sure where to place it.

updated the security.md to reflect the changes
issue a new client kubelet-api certificate used to secure authorize comms between api and kubelet
fixed any formatting issues i came across on the journey

k8s-ci-robot · 2017-08-02T20:31:36Z

Hi @gambol99. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

justinsb · 2017-08-06T20:07:16Z

docs/security.md

+    anonymousAuth: false
+```
+
+**Note** on a existing cluster with 'anonymousAuth' unset you would need to first roll out the masters and then update the pools. 


I think by "pools" you mean "nodes" or "node instance groups" (?)

justinsb · 2017-08-06T20:07:41Z

docs/security.md

 ## Kubernetes API

 (this section is a work in progress)

 Kubernetes has a number of authentication mechanisms:

+## Kubelet API
+
+By default AnonymousAuth on the kubelet is off and so communication between kube-apiserver and kubelet api is not authenticated. In order to switch on authentication;


I think you mean "by default AnonymousAuth on the kubelet is on..."

justinsb · 2017-08-06T20:09:25Z

nodeup/pkg/model/context.go

+
+	// @check if we have anything specific to master kubelet
+	if c.IsMaster {
+		if cluster.MasterKubelet != nil && cluster.MasterKubelet.AnonymousAuth != nil && *cluster.MasterKubelet.AnonymousAuth == true {


I think you mean == false at the end here.

thank you :-) .. and fixed

justinsb · 2017-08-06T20:12:31Z

nodeup/pkg/model/context.go

+
+// UseSecureKubelet checks if the kubelet api should be protected by a client certificate. Note: the settings are be
+// in one of three section, master specific kubelet, cluster wide kubelet or the InstanceGroup. Though arguably is
+// doesn't make much sense to unset this on a per InstanceGroup level, but hey :)


I agree that it doesn't really make a difference, because it doesn't make any sense to mix this setting, but I feel like the logic should be:

if instancegroup anonymousauth != nil, return that value
if master and masterkubelet anonymousauth != nil, return that value
if kubelet anonymousauth != nil, return that value

i.e. most specific to least specific, and you can set anonymousauth explicitly to true on the instancegroup and override (though this is silly in practice)

yep .. that make's a lot sense

justinsb · 2017-08-06T20:14:54Z

nodeup/pkg/model/kubeapiserver.go

@@ -70,6 +71,19 @@ func (b *KubeAPIServerBuilder) Build(c *fi.ModelBuilderContext) error {
 		c.AddTask(t)
 	}

+	// @check if we are using secure client certificates for kubelet and grab the certificates
+	{
+		if b.UseSecureKubelet() {


I am a big fan of using { ... } to group blocks of code, but here I think we can use the natural block defined by if b.UseSecureKubelet()

Oh... and mixing this setting is really not going to go well, if the apiserver doesn't have the cert but the nodes are expecting it :-)

Indeed ... i'm not a fan of bracketed code either, but i wanted to stay consistent and everything else else bracketed, so i felt the peer pressure :-)

justinsb · 2017-08-06T20:17:30Z

nodeup/pkg/model/kubeapiserver.go

 	}
+
+	return fmt.Errorf("Unrecognized authentication config %v", b.Cluster.Spec.Authentication)


BTW the main reason I like not having this as a fall-through is that then it is a compilation error if you forget to return nil in one of the branches, if each of the branches is expected to exit.

i guess the a idiom would to be return early ... i.e. b.Cluster.Spec.Authentication.Kopeio == nil then return the error .. else everything continues

justinsb · 2017-08-06T20:21:11Z

nodeup/pkg/model/kubeapiserver.go

 	kubeAPIServer.BasicAuthFile = filepath.Join(b.PathSrvKubernetes(), "basic_auth.csv")
 	kubeAPIServer.TokenAuthFile = filepath.Join(b.PathSrvKubernetes(), "known_tokens.csv")

+	// @check if we are using secure kubelet client certificates
+	if b.UseSecureKubelet() {
+		// @note we are making assumption we are using the one's created by the pki model, not custom defined ones


Good to call this out. This is tricky actually. In some places we haven't mapped the flag in the model, and we just directly add the flag to the arguments. But that feels sort of hacky in that then we aren't using the flag code we've worked so hard on!

Realistically, eventually with componentconfig in k8s these will move out of flags (either to secrets or to dynamically generated and locally stored certificates, using the CertificateSigningRequest API). But we aren't there yet.

One option could be to glog.Warningf if the values are not empty. But this is fine I think... setting the kubelet / kubeapiserver config flags directly is "expert mode" kops and it is then possible/easy to break things (for typical usage we prefer setting options which set these flags, which is how kops continues to work across k8s versions even if the underlying flags or flag semantics change).

justinsb · 2017-08-06T20:22:48Z

nodeup/pkg/model/kubelet.go

@@ -265,6 +268,12 @@ func (b *KubeletBuilder) buildKubeletConfigSpec() (*kops.KubeletConfigSpec, erro
 		utils.JsonMergeStruct(c, b.Cluster.Spec.Kubelet)
 	}

+	// @check if we are using secure kubelet <-> api settings
+	if b.UseSecureKubelet() {
+		// @TODO these filenames need to be a constant somewhere


One option is to put them into a helper function in NodeupModelContext.

justinsb · 2017-08-06T20:26:07Z

/ok-to-test

Looks great. I think it would be great to figure out a top level cluster option which controls this configuration. One option is just to automatically enable it for k8s 1.8, but it would be nice to have this available for earlier versions as well I suspect. On the other hand, the flags you have right now do work, and so we can continue building these options out and then see if we want an overarching "secureTraffic" mode or similar. In other words, not a blocker to this PR merging :-)

k8s-github-robot · 2017-08-06T21:06:01Z

@gambol99 PR needs rebase

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

justinsb · 2017-08-07T01:40:19Z

LGTM - ready for merge?

gambol99 · 2017-08-07T13:24:37Z

hi @justinsb ... yep, good to merge :-)

k8s-github-robot · 2017-08-08T00:03:23Z

@gambol99 PR needs rebase

A while back options to permit secure kube-apiserver to kubelet api was kubernetes#2831 using the server.cert and server.key as testing grouns. This PR formalizes the options and generates a client certificate on their behalf (note, the server{.cert,key} can no longer be used post 1.7 as the certificate usage is checked i.e. it's not using a client cert). The users now only need to add anonymousAuth: false to enable secure api to kubelet. I'd like to make this default to all new builds i'm not sure where to place it. - updated the security.md to reflect the changes - issue a new client kubelet-api certificate used to secure authorize comms between api and kubelet - fixed any formatting issues i came across on the journey

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

justinsb · 2017-08-11T09:06:33Z

/lgtm

k8s-github-robot · 2017-08-11T09:06:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, justinsb

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-11T09:07:03Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-11T09:41:14Z

Automatic merge from submit-queue

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

…tes#3125

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2017

gambol99 force-pushed the kubelet_api branch 2 times, most recently from 5db7f6e to 578a9c8 Compare August 3, 2017 19:25

gambol99 mentioned this pull request Aug 3, 2017

Customize Kops Platform UKHomeOffice/kops#1

Closed

gambol99 force-pushed the kubelet_api branch from 578a9c8 to e873d86 Compare August 4, 2017 19:45

justinsb reviewed Aug 6, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 6, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2017

gambol99 force-pushed the kubelet_api branch from e873d86 to 5dd063c Compare August 6, 2017 21:37

gambol99 added a commit to gambol99/kops that referenced this pull request Aug 6, 2017

Requested Changes

5dd063c

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

gambol99 force-pushed the kubelet_api branch from 5dd063c to 7d4d85a Compare August 6, 2017 21:50

gambol99 added a commit to gambol99/kops that referenced this pull request Aug 6, 2017

Requested Changes

7d4d85a

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2017

gambol99 force-pushed the kubelet_api branch from 7d4d85a to d4bd38a Compare August 6, 2017 21:58

gambol99 added a commit to gambol99/kops that referenced this pull request Aug 6, 2017

Requested Changes

d4bd38a

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

justinsb self-assigned this Aug 7, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017

gambol99 added 2 commits August 8, 2017 01:11

Requested Changes

9873fc1

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

gambol99 force-pushed the kubelet_api branch from d4bd38a to 9873fc1 Compare August 8, 2017 12:47

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2017

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

k8s-github-robot merged commit b418c30 into kubernetes:master Aug 11, 2017

gambol99 deleted the kubelet_api branch August 11, 2017 14:11

aknuds1 pushed a commit to aknuds1/kops that referenced this pull request Aug 25, 2017

Requested Changes

0540c74

- fixed the various issues highlighted in kubernetes#3125 - changed the docuementation to make more sense - changed the logic of the UseSecureKubelet to return early

justinsb mentioned this pull request Dec 3, 2017

Default kubelet to secure #3998

Closed

audip pushed a commit to audip/kops that referenced this pull request Jan 20, 2018

Update secureKubelet flag to false by default kubernetes#3998 kuberne…

49bfcc1

…tes#3125

audip mentioned this pull request Jan 20, 2018

Update kubelet to be secureKubelet by default #4307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubelet API Certificate #3125

Kubelet API Certificate #3125

gambol99 commented Aug 2, 2017

k8s-ci-robot commented Aug 2, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

gambol99 Aug 6, 2017

justinsb Aug 6, 2017

gambol99 Aug 6, 2017 •

edited

Loading

justinsb Aug 6, 2017

justinsb Aug 6, 2017

gambol99 Aug 6, 2017

justinsb Aug 6, 2017

gambol99 Aug 6, 2017

justinsb Aug 6, 2017

justinsb Aug 6, 2017

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 6, 2017

justinsb commented Aug 7, 2017

gambol99 commented Aug 7, 2017

k8s-github-robot commented Aug 8, 2017

justinsb commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

		}

		return fmt.Errorf("Unrecognized authentication config %v", b.Cluster.Spec.Authentication)

Kubelet API Certificate #3125

Kubelet API Certificate #3125

Conversation

gambol99 commented Aug 2, 2017

k8s-ci-robot commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gambol99 Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 6, 2017

k8s-github-robot commented Aug 6, 2017

justinsb commented Aug 7, 2017

gambol99 commented Aug 7, 2017

k8s-github-robot commented Aug 8, 2017

justinsb commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

k8s-github-robot commented Aug 11, 2017

gambol99 Aug 6, 2017 •

edited

Loading