Rotate kubelet client certificate. #41912

jcbsmpsn · 2017-02-22T18:47:54Z

Changes the kubelet so it bootstraps off the cert/key specified in the
config file and uses those to request new cert/key pairs from the
Certificate Signing Request API, as well as rotating client certificates
when they approach expiration.

Default behavior is for client certificate rotation to be disabled. If enabled
using a command line flag, the kubelet exits each time the certificate is
rotated. I tried to use GetCertificate in tls.Config but it is only called
on the server side of connections. Then I tried GetClientCertificate,
but it is new in 1.8.

Release note

With --feature-gates=RotateKubeletClientCertificate=true set, the kubelet will
request a client 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-02-22T18:48:01Z

This change is

jcbsmpsn · 2017-02-22T18:48:15Z

cc @mikedanese

jcbsmpsn · 2017-02-22T19:14:22Z

cc @liggitt @timstclair

jcbsmpsn · 2017-02-23T02:34:35Z

@k8s-bot gci gce e2e test this
@k8s-bot non-cri e2e test this

jcbsmpsn · 2017-02-24T16:45:30Z

@liggitt, @mikedanese, @timstclair Anyone got some time before the code freeze?

timstclair · 2017-02-24T18:23:06Z

cmd/kubelet/app/options/options.go

@@ -131,6 +131,7 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.TLSPrivateKeyFile, "tls-private-key-file", s.TLSPrivateKeyFile, "File containing x509 private key matching --tls-cert-file.")
 	fs.StringVar(&s.CertDirectory, "cert-dir", s.CertDirectory, "The directory where the TLS certs are located (by default /var/run/kubernetes). "+
 		"If --tls-cert-file and --tls-private-key-file are provided, this flag will be ignored.")
+	fs.Int32Var(&s.CertRotation, "experimental-cert-rotation", s.CertRotation, "<Warning: Experimental feature> The percentage threshold of certificate duration remaining before a new key should be generated and signed using the certificate signing request API on the api server. New cert/key pairs generated by this process will be stored in the directory specified by --cert-dir. If less than 1, rotation is disabled. [default=0]")


How important is it to give the admin this knob? It seems like something that should be tuned on our end, not by the user. If that's the case, I suggest using a feature_gate instead.

+1 for feature gate intially (default to off this release)

Not familiar with feature_gate. This flag sets the amount of remaining duration of the certificate before it gets rotated. Does a feature_gate allow some value to be set? Or hard code this to something?

I think so far they're just binary flags, and let you tag features as alpha/beta. My suggestion above was that we hardcode a value for this, unless you think it's an important knob to provide.

Having this knob is pretty useful for testing, and when restarts are no longer part of rotation we can turn this up and rotate frequently during end to end tests.

timstclair · 2017-02-24T18:46:20Z

cmd/kubelet/app/server.go

+		CertificateRotationPercent: rotation,
+		Rotation: func() {
+			glog.Info("The client certificate was rotated, restarting so changes take effect.")
+			os.Exit(0)


This is too disruptive. My biggest concern about restarting the Kubelet is it would disrupt any open connections (watching logs, exec/attach/portforward connections). It would also stress the runtime, and may have other performance impacts.

Fortunately, the ListenAndServeTLS function is not too complicated, and should be pretty easy to unroll and swap in our own Accept implementation:

ListenAndServeTLS just sets up a tls listener, and calls server.Serve(...) on it.

tls.NewListener just wraps a tls.listener, which simlpy wraps the Conn returned by the inner listener.

I think all we'd need to do is create our own listener wrapper which calls tls.Server with the tls.Config with the latest certs.

+1
I think @sttts already did an implementation to pass SNI certs to apiserver

kubernetes/staging/src/k8s.io/apiserver/pkg/server/serve.go

Line 102 in d74ef65

func runServer(server *http.Server, network string, stopCh <-chan struct{}) (int, error) {

It's more about being able to stop the server with a channel. Now with Go 1.8 we will have an official API for that.

Can we avoid stopping the server in this case? I believe stopping the server will have the same problems mentioned above (or are existing connections kept alive when the server stops?).

I agreed with @timstclair on this. Restarting Kubelet is very disruptive. In addition to the issues pointed out by @timstclair, there are other concerns:

In GKE, we might agree upon O(months) certification rotation, but how we ensure this behavior for OSS users?

Any misbehaviour operation on certification rotation might lead to a node readiness flapping. But from the users' viewpoint, they only see node status is changed between ready and notReady unless they view the log. This adds more debuggability and introspection difficulties.

We encountered a lot of issues with mystery kubelet / docker restarts due to various reasons in prod. This feature only makes things worse.

What about other standalone daemons running on the node, which are talking to API server too, for example, NPD in GKE. Based on this design, are we going to force NPD restart too?

Continue with above comments:
5. The worst concern I had based on this design is that all kubelets in the cluster might start at the same time and pulling the information from API server. Can API server handle that load? Shouldn't this is another potential API server scalability issue?

@gmarek @wojtek-t

Answering 5 from above comment - I think that restating all kubelets at once can overload apiserver for quite some time (in fact I did such experiment in 2k-node cluster few months ago and it recovered from backlog after 5minutes or so IIRC).
I didn't read the PR, but if this is going to restart all kubelets at the same time, it's not only bad from scalability perspective, it just sounds bad to me.
We should try spreading the restarts over some time. If the time is long enough, I wouldn't worry about scalability (I'm not sure what long enough means, but from the top of my head, 1hour/1000nodes should be enough).

[And sorry if it's already spread - as I said, I didn't read the PR, just read the comments above.]

We're not restarting all the kubelets at the same time on purpose, but they will restart at the same time period after they were added to the cluster, so if a lot of kubelets are added at approximately the same time, they could clobber the certificate request signing api at the same time 10 months later. I'll add some jitter to the rotation timing.

It looks like this PR won't make 1.6 code freeze. It also looks like transitioning to Go 1.8 is in progress for Kubernetes 1.7. Once that's done we should be able to rotate client certificates without a restart.

Proposed an adjustment to the certificate manager to spread out the certificate rotation requests over here #42498

timstclair · 2017-02-24T18:47:31Z

hack/verify-flags/known-flags.txt

+log-lines-total
+run-duration
+attach-detach-reconcile-sync-period
+disable-attach-detach-reconcile-sync


Why were all these other flags added? Is it just a rebase issue?

Not my flags. Must be a rebase issue. Will remove.

timstclair · 2017-02-24T18:53:34Z

pkg/kubelet/certificate/certificate_manager.go

 	if err != nil {
 		return nil, err
 	}

-	if certRotationPercent > 100 {
-		certRotationPercent = 100
+	if config.CertificateRotationPercent > 100 {


nit: just return an error

timstclair · 2017-02-24T18:54:40Z

pkg/kubelet/certificate/certificate_manager.go

+func getCurrentCertificateOrBootstrap(
+	store Store,
+	bootstrapCertificatePEM []byte,
+	bootstrapKeyPEM []byte) (*CertKeyData, bool, error) {


nit: name the return values (what is the bool?).

timstclair · 2017-02-24T18:55:31Z

pkg/kubelet/certificate/certificate_manager.go

+	if err != nil {
+		return nil, false, fmt.Errorf("unable to parse certificate data: %v", err)
+	}
+	cert.Leaf = certs[0]


is len(certs) guaranteed to be > 0?

Length of the output will equal the length of the input, so in this case, always 1, as long as there is no err.

timstclair · 2017-02-24T19:00:51Z

pkg/kubelet/certificate/certificate_manager.go

-	notAfter := m.cert.Leaf.NotAfter
-	total := notAfter.Sub(m.cert.Leaf.NotBefore)
+	notAfter := m.certKeyData.Certificate.Leaf.NotAfter
+	total := notAfter.Sub(m.certKeyData.Certificate.Leaf.NotBefore)


Where does the notAfter value come from? Shouldn't we be able to calculate exactly when the next rotation needs to occur, and wait until that moment, removing the need for the sync period polling & shouldRotate()?

notAfter is certificate data. It's the expiration of the certificate. We can use it as you suggest. A follow up PR ok?

follow up is fine.

Issue tracking follow up: #42089

mikedanese · 2017-02-24T17:51:48Z

pkg/kubelet/certificate/certificate_manager.go

+	// certificate, if there is no current certificate available in the
+	// CertificateStore. If there is a current certificate available, this will
+	// be ignored.
+	BootstrapKeyPEM []byte


Can this be a BootstrapClient and a certificatesclient.CertificateSigningRequestInterface? Why shouldn't I be able to use any form of authn to bootstrap my client TLS?

Well, you can use any authn to initialize your TLS client, but initializing your TLS client doesn't happen in the certificate manager. The certificate manager is the consumer of an already initialized client, only responsible for rotating as expiration approaches and returning the current cert/key pair. If you aren't going to use certs and keys, you wouldn't be using the CertificateManager, which is only designed for managing cert/key pairs.

There is already a CertificateSigningRequestInterface in this struct, it's the first member.

I see the comments for these two members are incorrect and may have mislead you. I'll update it. The bootstrap cert/key pair are the data returned from the certificate manager if the Store doesn't currently have any cert/key pairs to return.

mikedanese · 2017-02-24T17:54:43Z

pkg/kubelet/certificate/certificate_manager.go

@@ -46,85 +46,142 @@ 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)


Public setters leave a bad taste in my mouth.

It's an awkward situation alright. Since the manager uses the REST client to do rotations, I need the REST client to initialize the manager, and I need the manager to supply the keys to initialize the REST client.

mikedanese · 2017-02-25T00:20:30Z

cmd/kubelet/app/server.go

+		CertificateRotationPercent: rotation,
+		Rotation: func() {
+			glog.Info("The client certificate was rotated, restarting so changes take effect.")
+			os.Exit(0)


Client GetCertificate is not supported until go1.8. Any suggestions pretaining to how to do this with the kube client? I'd like to point out that this is an experimantal flag and rotations event frequency is very low (O(months)).

jcbsmpsn · 2017-02-27T17:43:17Z

@smarterclayton Do you have some time to approve this?

jcbsmpsn · 2017-02-27T22:46:23Z

@k8s-bot cvm gce e2e test this

ethernetdan · 2017-03-02T00:50:57Z

Removing PR from 1.6 milestone since code freeze has begun:

If this is a bug fix, please label as kind/bug and reapply the milestone.
If this is a feature for 1.6, an exception should be filed for it before 11:00PM PST on 3/2. Note that most exceptions to code freeze are not accepted.
If this is a feature for 1.7, feel free to add the 1.7 milestone to the PR.

/cc @kubernetes/release-team @kubernetes/kubernetes-release-managers

mikedanese · 2017-03-28T17:55:46Z

We are waiting on go 1.8.1

liggitt · 2017-05-31T17:31:16Z

cmd/kubelet/app/server.go

+	if err != nil {
+		return fmt.Errorf("unable to configure TLS for the rest client: %v", err)
+	}
+	tlsConfig.RootCAs = pool


tlsConfig can be nil if no customizations were required. in that case you should initialize to an empty one.

liggitt · 2017-05-31T17:38:49Z

cmd/kubelet/app/server.go

+		}
+		return cert, nil
+	}
+	clientConfig.Transport = utilnet.SetTransportDefaults(&http.Transport{


these are different than what you would have gotten otherwise: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/transport/cache.go#L67

it's hard to tell how important the differences are

add a TODO to reconcile the defaults here with the defaults you'd get from calling restclient.TransportFor(config)

Updated to match.

liggitt · 2017-05-31T17:39:20Z

cmd/kubelet/app/server.go

+	clientConfig.KeyData = nil
+	clientConfig.CertFile = ""
+	clientConfig.KeyFile = ""
+	clientConfig.CAData = nil


empty CAFile as well

liggitt · 2017-05-31T17:46:24Z

cmd/kubelet/app/server.go

@@ -597,6 +622,94 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.KubeletDeps) (err error) {
 	return nil
 }

+func updateTransport(clientConfig *restclient.Config, clientCertificateManager certificate.Manager) error {


if clientConfig already has a Transport set, return an error

liggitt · 2017-05-31T20:43:25Z

cmd/kubelet/app/server.go

+	tlsConfig.GetClientCertificate = func(requestInfo *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+		cert := clientCertificateManager.Current()
+		if cert == nil {
+			return nil, fmt.Errorf("no certificate available")


should return &tls.Certificate{Certificate:nil}, nil in this case, right? we don't want to abort the handshake, we just don't have a client certificate to return

From the godoc:

If GetClientCertificate returns an error, the handshake will be aborted and that error will be returned. Otherwise GetClientCertificate must return a non-nil Certificate. If Certificate.Certificate is empty then no certificate will be sent to the server.

Sounds good to me. Done.

liggitt · 2017-05-31T21:00:51Z

/lgtm

jcbsmpsn · 2017-05-31T21:56:26Z

/assign @smarterclayton

mikedanese · 2017-05-31T22:09:17Z

/approve

smarterclayton · 2017-06-01T16:18:44Z

/approve

k8s-github-robot · 2017-06-01T16:20:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

~~cmd/kubelet/OWNERS~~ [mikedanese,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

Changes the kubelet so it bootstraps off the cert/key specified in the config file and uses those to request new cert/key pairs from the Certificate Signing Request API, as well as rotating client certificates when they approach expiration.

k8s-github-robot · 2017-06-03T04:42:37Z

Automatic merge from submit-queue (batch tested with PRs 46726, 41912, 46695, 46034, 46551)

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

k8s-github-robot assigned smarterclayton Feb 22, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Feb 22, 2017

mikedanese added this to the v1.6 milestone Feb 22, 2017

mikedanese assigned liggitt, mikedanese and timstclair and unassigned smarterclayton Feb 22, 2017

jcbsmpsn force-pushed the rotate-client-certificate branch from e1f4b55 to d64a5a1 Compare February 22, 2017 20:22

jcbsmpsn force-pushed the rotate-client-certificate branch from d64a5a1 to 5782d15 Compare February 24, 2017 16:43

timstclair suggested changes Feb 24, 2017

View reviewed changes

mikedanese reviewed Feb 25, 2017

View reviewed changes

jcbsmpsn mentioned this pull request Feb 25, 2017

Certificate manager rotate on duration instead of periodically. #42089

Closed

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

ethernetdan removed this from the v1.6 milestone Mar 2, 2017

jcbsmpsn mentioned this pull request Mar 3, 2017

Fix the certificate rotation threshold and add jitter. #42498

Merged

ericchiang mentioned this pull request Apr 6, 2017

pkg/util/cert: Add file based CRL implementation #33519

Closed

jcbsmpsn mentioned this pull request Apr 21, 2017

Add bootstrap support to certificate manager. #42486

Merged

jcbsmpsn force-pushed the rotate-client-certificate branch from 1aa5835 to 7961438 Compare April 24, 2017 18:16

liggitt reviewed May 31, 2017

View reviewed changes

jcbsmpsn force-pushed the rotate-client-certificate branch 3 times, most recently from 7f93650 to 3944ba1 Compare May 31, 2017 20:35

liggitt reviewed May 31, 2017

View reviewed changes

jcbsmpsn force-pushed the rotate-client-certificate branch 2 times, most recently from f401f13 to b773ae6 Compare May 31, 2017 20:52

jcbsmpsn mentioned this pull request May 31, 2017

Add certificate rotation support to dynamic kubelet config loader #46730

Closed

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

k8s-ci-robot assigned smarterclayton May 31, 2017

k8s-ci-robot assigned yujuhong and unassigned yujuhong May 31, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 1, 2017

Rotate kubelet client certificate.

1519bb9

Changes the kubelet so it bootstraps off the cert/key specified in the config file and uses those to request new cert/key pairs from the Certificate Signing Request API, as well as rotating client certificates when they approach expiration.

jcbsmpsn force-pushed the rotate-client-certificate branch from b773ae6 to 1519bb9 Compare June 1, 2017 16:29

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 1, 2017

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

liggitt mentioned this pull request Jun 2, 2017

Alpha Dynamic Kubelet Configuration #46254

Merged

k8s-github-robot merged commit 24d0997 into kubernetes:master Jun 3, 2017

Rotate kubelet client certificate. #41912

Rotate kubelet client certificate. #41912

Conversation

jcbsmpsn commented Feb 22, 2017 • edited by timstclair

k8s-reviewable commented Feb 22, 2017

jcbsmpsn commented Feb 22, 2017

jcbsmpsn commented Feb 22, 2017

jcbsmpsn commented Feb 23, 2017

jcbsmpsn commented Feb 24, 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Feb 25, 2017 • edited

Choose a reason for hiding this comment

jcbsmpsn commented Feb 27, 2017

jcbsmpsn commented Feb 27, 2017

ethernetdan commented Mar 2, 2017

mikedanese commented Mar 28, 2017

liggitt May 31, 2017 • edited

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

Choose a reason for hiding this comment

liggitt May 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 31, 2017

jcbsmpsn commented May 31, 2017

mikedanese commented May 31, 2017

smarterclayton commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k8s-github-robot commented Jun 3, 2017

jcbsmpsn commented Feb 22, 2017 •

edited by timstclair

mikedanese Feb 25, 2017 •

edited

liggitt May 31, 2017 •

edited

liggitt May 31, 2017 •

edited