Certificate rotation for kubelet server certs. #45059

jcbsmpsn · 2017-04-28T01:02:21Z

Replaces the current kubelet server side self signed certs with certs signed by
the Certificate Request Signing API on the API server. Also renews expiring
kubelet server certs as expiration approaches.

Two Points:

With --feature-gates=RotateKubeletServerCertificate=true set, the kubelet will
request a certificate during the boot cycle and pause waiting for the request to
be satisfied.
In order to have the kubelet's certificate signing request auto approved,
--insecure-experimental-approve-all-kubelet-csrs-for-group= must be set on
the cluster controller manager. There is an improved mechanism for auto
approval proposed.

Release note:

With `--feature-gates=RotateKubeletServerCertificate=true` set, the kubelet will
request a server certificate from the API server during the boot cycle and pause
waiting for the request to be satisfied. It will continually refresh the certificate as
the certificates expiration approaches.

k8s-reviewable · 2017-04-28T01:02:29Z

This change is

jcbsmpsn · 2017-04-28T01:04:04Z

@ericchiang Initial bootstrapping of the server cert by requesting a signed cert from the certificate request signing API on the API server is in here.

ericchiang

Looks like the Go 1.8.1 is getting rolled back so we might have to wait a bit for the CI :( #38228 (comment)

ericchiang · 2017-04-28T17:09:01Z

pkg/controller/certificates/groupapprove.go

@@ -72,7 +72,7 @@ func (cc *groupApprover) AutoApprove(csr *certificates.CertificateSigningRequest
 	if len(x509cr.DNSNames)+len(x509cr.EmailAddresses)+len(x509cr.IPAddresses) != 0 {
 		return csr, nil
 	}
-	if !hasExactUsages(csr, kubeletClientUsages) {
+	if !hasExactUsages(csr, kubeletClientUsages) && !hasExactUsages(csr, kubeletServerUsages) {


For the server cert, I'd expect some matching of the user that requested it (system:node:(nodename)) and the common name requested.

What check are you thinking on the common name?

During bootstrapping, the client cert contains the common name system:node:( nodename ). When it requests a serving cert from the certificates API, it'll be authenticated as the username system:node:( nodename ). When approving these requests, I'd expect that a node would only be able to request serving certs for it's nodename, and not others.

Actually, after the groupapprover is used to grant the client cert, the kubelet can use that cert and wont be part of the bootstrapping group after. This was brought up by @deads2k in the original client cert bootstrapping PR: #30094 (comment)

cc @kubernetes/sig-auth-pr-reviews

Agree. There are three distinct autoapproval cases we want to detect (see #45030)

CSR shaped like a kubelet client cert, requested by the kubelet itself

CSR shaped like a kubelet serving cert, requested by the kubelet itself

CSR shaped like a kubelet client cert, requested by a different user (like a shared bootstrap user)

This seems beyond the scope of the group approver. Maybe a different PR?

It is my hope to put some bare minimum into this PR and move on to #45030 next, after getting some feedback on that issue. @mikedanese says he thinks it would be a relatively small change, but it will depend on feedback.

If it seems reasonable to proceed here with some bare minimum, would addressing @ericchiang's comment be enough?

the checks this function does wouldn't match the shape of a node's serving cert... it would have IP and DNS SANs, at least. I'd expect to start to see the refactor into isNodeClientCSR(CSR) and isNodeServingCSR(CSR) here.

the way we eventually determine which IP and DNS names a node can ask for a serving cert for are will be interesting... should a kubelet have to register its Node API object before it can ask for a serving cert?

ericchiang

fyi @liggitt the controller manager approver changes have been removed from this PR.

ericchiang · 2017-05-04T23:56:38Z

pkg/kubelet/certificate/certificate_manager.go

@@ -46,6 +46,9 @@ const (
 // manager. In the background it communicates with the API server to get new
 // certificates for certificates about to expire.
 type Manager interface {
+	// CertificateSigningRequestClient sets the client interface that is used for
+	// signing new certificates generated as part of rotation.
+	CertificateSigningRequestClient(certificatesclient.CertificateSigningRequestInterface)


I'd expect this to be part of the initializer. Any reason this was added to the interface? It doesn't actually look like it's being used outside of tests.

It's not used in this PR, but it's used in #41912. It's specifically to work around the one case where the API client needs the certificate from the certificate manager to make a connection, and the certificate manager needs the API client to request certificate changes. The sequence of steps is:

Initialize certificate manager with local certs (either bootstrap or previously requested and stored cert). No client is passed in yet, so the certificate manager can't rotate/update yet.

Initialize the API client with the certificate manager

.Start() the certificate manager

certificate manager uses the client to request an updated certificate (if it needs too, either because of bootstrapping or expiration)

jcbsmpsn · 2017-05-08T18:12:03Z

/assign @dchen1107

jcbsmpsn · 2017-05-08T22:40:30Z

/assign @timstclair

jcbsmpsn · 2017-05-08T22:41:05Z

/unassign @dchen1107

jcbsmpsn · 2017-05-09T19:50:07Z

@k8s-bot gce etcd3 e2e test this

liggitt · 2017-05-27T18:48:02Z

pkg/kubelet/kubelet.go

+func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg *componentconfig.KubeletConfiguration, nodeName types.NodeName, cloudIPs []net.IP, hostnames []string) (certificate.Manager, error) {
+	var certSigningRequestClient clientcertificates.CertificateSigningRequestInterface
+	if kubeClient != nil && kubeClient.Certificates() != nil {
+		certSigningRequestClient = kubeClient.Certificates().CertificateSigningRequests()


what are the implications if we don't have a client here? is it even possible for the cert manager to do anything?

The certificate manager can serve whatever local certificates it has, either received through the command line parameters, config file, or existing files on disk.

liggitt · 2017-05-27T18:51:10Z

pkg/kubelet/kubelet.go

+			DNSNames:    hostnames,
+			IPAddresses: ips,
+		},
+		Usages: []certificates.KeyUsage{


nit: might be worth having a helper in the certificates api package that returns usages for a TLS client cert, and one that returns usages for a TLS serving cert.

Will follow on with this change.

liggitt · 2017-05-27T18:52:36Z

pkg/kubelet/kubelet.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to initialize certificate store: %v", err)
+	}
+	ips, err := allLocalIPsWithoutLoopback()


hoist IP detection out of this function, this function should just take the lists of dns and ip SANs we want

liggitt · 2017-05-27T18:55:38Z

pkg/kubelet/kubelet.go

+	})
+}
+
+func allLocalIPsWithoutLoopback() ([]net.IP, error) {


the kubelet takes an --address option that lets you bind to a specific interface. if that is set to a specific IP, we should just use that (we should only iterate over all interfaces if it is binding to 0.0.0.0)

liggitt · 2017-05-27T20:21:52Z

looking good, just a couple questions and a couple structural changes around where/how we determine local ips. (needs a release note in the description as well)

timstclair

+1 for adding a more detailed release note.

timstclair · 2017-05-29T16:16:42Z

pkg/kubelet/kubelet.go

+		if err != nil {
+			return nil, err
+		}
+		ips = append(ips, cloudIPs...)


Is it possible for allLocalIPsWithoutLoopback to include the external IPs? If so, I think you should probably de-dupe this list?

timstclair · 2017-05-29T16:19:06Z

pkg/features/kube_features.go

+	// Gets a server certificate for the kubelet from the Certificate Signing
+	// Request API instead of generating one self signed and auto rotates the
+	// certificate as expiration approaches.
+	RotateKubeletServerCertificate utilfeature.Feature = "RotateKubeletServerCertificate"


I don't think feature gates should be a long-term option. If we want something to toggle this after it goes to GA, it should be a separate flag.

jcbsmpsn · 2017-05-29T19:21:12Z

Added a release note.

Replaces the current kubelet server side self signed certs with certs signed by the Certificate Request Signing API on the API server. Also renews expiring kubelet server certs as expiration approaches.

timstclair · 2017-05-30T19:15:30Z

/lgtm

jcbsmpsn · 2017-05-30T19:44:20Z

/assign @smarterclayton

smarterclayton · 2017-05-30T19:58:31Z

/approve

as per reviewers

k8s-github-robot · 2017-05-30T19:59:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcbsmpsn, smarterclayton, timstclair

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

~~cmd/kubelet/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]

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-05-31T02:49:01Z

Automatic merge from submit-queue (batch tested with PRs 46635, 45619, 46637, 45059, 46415)

smarterclayton · 2017-05-31T19:48:45Z

This broke hack/local-up-cluster - while this will be disabled by default in hack local up cluster, we need a fix to it that allows someone to locally test this change.

liggitt · 2017-05-31T21:09:52Z

PR to disable alpha gates in local up cluster is already open. Would be easy to create a CA for the controller manager to enable signing.

smarterclayton · 2017-07-05T21:58:00Z

pkg/kubelet/kubelet.go

@@ -1037,6 +1094,62 @@ type Kubelet struct {
 	dockerLegacyService dockershim.DockerLegacyService
 }

+func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg *componentconfig.KubeletConfiguration, nodeName types.NodeName, ips []net.IP, hostnames []string) (certificate.Manager, error) {


I'd like to move this into its own package (since it's not dependent on anything in the kubelet) and import it, primarily to separate it from this mega package, but also to make it easier to consume from other contexts (like tools that want to provide their own certificate management in a similar form).

Objections to a small refactor? I also want to do that for the client code.

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

k8s-github-robot assigned mtaufen and sjpotter Apr 28, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Apr 28, 2017

ericchiang reviewed Apr 28, 2017

View reviewed changes

luxas 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 Apr 29, 2017

jcbsmpsn force-pushed the rotate-server-certificate branch from fc62cd1 to 8c462c2 Compare May 2, 2017 00:51

ericchiang mentioned this pull request May 4, 2017

breakup the certificates controller #45302

Closed

jcbsmpsn force-pushed the rotate-server-certificate branch from 8c462c2 to f5c6484 Compare May 4, 2017 22:37

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 4, 2017

jcbsmpsn force-pushed the rotate-server-certificate branch from f5c6484 to 7dc7cf3 Compare May 4, 2017 23:24

ericchiang reviewed May 5, 2017

View reviewed changes

k8s-ci-robot assigned dchen1107 May 8, 2017

k8s-ci-robot assigned timstclair May 8, 2017

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

k8s-ci-robot unassigned dchen1107 May 8, 2017

jcbsmpsn force-pushed the rotate-server-certificate branch from 7dc7cf3 to b3530ac Compare May 8, 2017 22:42

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

jcbsmpsn force-pushed the rotate-server-certificate branch from b3530ac to f0bbe84 Compare May 9, 2017 17:26

jcbsmpsn force-pushed the rotate-server-certificate branch from f0bbe84 to 9a9d3c1 Compare May 13, 2017 10:09

k8s-github-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 13, 2017

liggitt reviewed May 27, 2017

View reviewed changes

liggitt added sig/auth Categorizes an issue or PR as relevant to SIG Auth. area/security labels May 27, 2017

liggitt added the sig/node Categorizes an issue or PR as relevant to SIG Node. label May 27, 2017

jcbsmpsn force-pushed the rotate-server-certificate branch from dfca6bc to 2b4b578 Compare May 29, 2017 13:43

timstclair reviewed May 29, 2017

View reviewed changes

jcbsmpsn force-pushed the rotate-server-certificate branch 2 times, most recently from d2febc8 to 92a9d07 Compare May 29, 2017 18:56

Certificate rotation for kubelet server certs.

4c22e6b

Replaces the current kubelet server side self signed certs with certs signed by the Certificate Request Signing API on the API server. Also renews expiring kubelet server certs as expiration approaches.

jcbsmpsn force-pushed the rotate-server-certificate branch from 92a9d07 to 4c22e6b Compare May 29, 2017 19:28

jcbsmpsn mentioned this pull request May 30, 2017

Support certificate rotation kubernetes/kubeadm#206

Closed

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

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

k8s-github-robot merged commit f2074ba into kubernetes:master May 31, 2017

wanghaoran1988 mentioned this pull request May 31, 2017

No nodes resources in local cluster. #46691

Closed

smarterclayton reviewed Jul 5, 2017

View reviewed changes

jamiehannaford mentioned this pull request Oct 10, 2017

Key/Cert rotation for Kubernetes clients. #4672

Closed

scfast mentioned this pull request May 4, 2023

1.3.6 Ensure that the RotateKubeletServerCertificate argument is set to true sandevistan-server-hardening/CIS_Kubernetes_Benchmark_v1.7.0#45

Open

Certificate rotation for kubelet server certs. #45059

Certificate rotation for kubelet server certs. #45059

Conversation

jcbsmpsn commented Apr 28, 2017 • edited by timstclair Loading

k8s-reviewable commented Apr 28, 2017

jcbsmpsn commented Apr 28, 2017

ericchiang left a 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

Choose a reason for hiding this comment

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbsmpsn commented May 8, 2017

jcbsmpsn commented May 8, 2017

jcbsmpsn commented May 8, 2017

jcbsmpsn commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt May 27, 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

liggitt commented May 27, 2017

timstclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbsmpsn commented May 29, 2017

timstclair commented May 30, 2017

jcbsmpsn commented May 30, 2017

smarterclayton commented May 30, 2017

k8s-github-robot commented May 30, 2017

k8s-github-robot commented May 31, 2017

smarterclayton commented May 31, 2017

liggitt commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbsmpsn commented Apr 28, 2017 •

edited by timstclair

Loading

liggitt May 27, 2017 •

edited

Loading

liggitt May 27, 2017 •

edited

Loading