Add TLS auto-approval for kubelet. #663

diegs · 2017-07-27T19:15:53Z

No description provided.

ericchiang · 2017-07-27T20:11:06Z

pkg/asset/internal/templates.go

+- apiGroups: ["certificates.k8s.io"]
+  resources: ["certificatesigningrequests/nodeclient"]
+  verbs: ["create"]
+- apiGroups: ["certificates.k8s.io"]


note for later:

There's actually a system:node-bootstrapper role we can just bind to https://github.com/kubernetes/kubernetes/blob/3a0d8f8fea75140e4a38f5165a75e93bcd28c60c/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml#L724-L742

ericchiang · 2017-07-27T20:13:26Z

pkg/tlsutil/tlsutil.go

@@ -127,3 +128,21 @@ func NewSignedCertificate(cfg CertConfig, key *rsa.PrivateKey, caCert *x509.Cert
 	}
 	return x509.ParseCertificate(certDERBytes)
 }
+
+// NewRandomToken generates an n-byte random hexadecimal token. The output will have length n*2.
+func NewRandomToken(n int) ([]byte, error) {


nit: there's actually a defined format for another authenticator. We might just want to copy that for now: https://kubernetes.io/docs/admin/bootstrap-tokens/#token-format

ericchiang · 2017-07-27T20:14:05Z

pkg/asset/tls.go

 	// TLS organizations map to Kubernetes groups, and "system:masters"
 	// is a well-known Kubernetes group that gives a user admin power.
 	//
-	// For now, put the kubelets in this group. Later we can restrict
-	// their credentials, likely with the help of TLS bootstrapping.
+	// For now, put the admins in this group.


We can remove this comment. We're not going to change this group ever.

ericchiang · 2017-07-27T22:40:32Z

Seeing some issues with only using the RotateKubeletClientCertificate to perform the first CSR (and not the bootstrap flag). Reported issue is that it requires a kubelet restart after doing the CSR to start authenticating as a node.

I think the kubelet is opening a connection to the API server with the bootstrapping credentials to perform its CSR, then keeping that connection alive even though it's got a client cert. The kubelet never closes its existing connection to the API server, so it never re-performs a TLS handshake to authenticate using its client cert.

Only workaround I see is to have systemd restart the kubelet once it detects the kubelet's written its first certificate to disk. Blah.

jcbsmpsn · 2017-07-27T22:49:16Z

@ericchiang Yes, I can see how that would happen. The underlying connection is only re-established if it is disrupted for some reason. The rotated cert is set immediately, and will be used when the connection is re-established.

liggitt · 2017-07-28T00:49:50Z

I think the kubelet is opening a connection to the API server with the bootstrapping credentials to perform its CSR, then keeping that connection alive even though it's got a client cert.

That's concerning... the client and transport built from the bootstrapping credentials shouldn't be reused beyond the bootstrap phase. Once it gets the first client cert, the input to building the API client should be different, so it should get a different tls config, with a new set of keepalive connections.

liggitt · 2017-07-28T00:50:48Z

oh, by bootstrapping, you don't actually mean --bootstrap... you mean the initial content of --kubeconfig

ericchiang · 2017-07-28T01:04:06Z

@jcbsmpsn because the API server authenticates x509 certs at the time of the request, and not on the handshake[1] I think we'll hit the same issue when the kubelet cert expires. E.g. once the kubelet's cert expires, it'll keep the connection open, but the requests will start getting forbidden errors.

I think the kubelet's rotation code should shut down its connections once it successfully rotates a cert. That would let it immediately start using the new certs and prevent these kind of issues with long lived connections and old credentials.

[1] https://github.com/kubernetes/kubernetes/blob/v1.7.2/staging/src/k8s.io/apiserver/pkg/authentication/request/x509/x509.go

jcbsmpsn · 2017-07-28T17:27:10Z

@ericchiang That would be good. I do not think that will be easy.

jcbsmpsn · 2017-08-02T17:42:13Z

kubernetes/kubernetes#49899 addresses closing existing connections when the certificate changes.

diegs · 2017-08-02T17:45:12Z

@jcbsmpsn yes, I added a temporary commit that switches the kubelet to use a hyperkube with that PR to test.

@jcbsmpsn

Automatic merge from submit-queue (batch tested with PRs 49237, 49656, 49980, 49841, 49899) certificate manager: close existing client conns once cert rotates After the kubelet rotates its client cert, it will keep connections to the API server open indefinitely, causing it to use its old credentials instead of the new certs. Because the API server authenticates client certs at the time of the request, and not the handshake, this could cause the kubelet to start hitting auth failures even if it rotated its certificate to a new, valid one. When the kubelet rotates its cert, close down existing connections to force a new TLS handshake. Ref kubernetes/enhancements#266 Updates kubernetes-retired/bootkube#663 ```release-note After a kubelet rotates its client cert, it now closes its connections to the API server to force a handshake using the new cert. Previously, the kubelet could keep its existing connection open, even if the cert used for that connection was expired and rejected by the API server. ``` /cc @kubernetes/sig-auth-bugs /assign @jcbsmpsn @mikedanese

ericchiang · 2017-11-07T23:41:12Z

@diegs this PR has been updated with 1.8 features. It also now enables the node authorizer and admission controller.

https://kubernetes.io/docs/admin/authorization/node/
https://kubernetes.io/docs/admin/admission-controllers/#noderestriction

Everything works now except the checkpointer. I think we want to run the checkpointer with a service account and ideally its own RBAC role. Since it's running as a static pod, and wont have access to KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT, we probably want to do something similar the kube-proxy-kubeconfig in this PR.

cc @dghubble @colemickens --- in case either of you want to take a look.

edit: am trying to debug the test failures. hack/multi-node does work.

ericchiang · 2017-11-09T00:48:02Z

pkg/asset/internal/templates.go

@@ -56,7 +112,7 @@ spec:
      - name: kubelet
        image: {{ .Images.Hyperkube }}
        command:
-        - ./hyperkube
+        - /hyperkube


woops, need to remove this. think its a rebase artifact.

@jcbsmpsn

…ce-backoff Automatic merge from submit-queue (batch tested with PRs 54773, 52523, 47497, 55356, 49429). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. certificate manager: reduce max backoff from 128s to 32s For TLS bootstrapping in bootkube we run a kubelet with a control plane run through static pods. That static control plane has an API server and controller manager that approve the kubelet's CSR. Since the kubelet has to wait for the static control plane to come up to be approved, we hit this backoff every time and it actually adds a notable overhead to startup times. kubernetes-retired/bootkube#663 If this choice is somewhat arbitrary, I'd like to see it lowered for 1.9. /assign @jcbsmpsn @mikedanese ```release-note NONE ```

@jcbsmpsn

Automatic merge from submit-queue (batch tested with PRs 54773, 52523, 47497, 55356, 49429). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. certificate manager: reduce max backoff from 128s to 32s For TLS bootstrapping in bootkube we run a kubelet with a control plane run through static pods. That static control plane has an API server and controller manager that approve the kubelet's CSR. Since the kubelet has to wait for the static control plane to come up to be approved, we hit this backoff every time and it actually adds a notable overhead to startup times. kubernetes-retired/bootkube#663 If this choice is somewhat arbitrary, I'd like to see it lowered for 1.9. /assign @jcbsmpsn @mikedanese ```release-note NONE ``` Kubernetes-commit: 0ff21718d127b9fc14bdfc068624e82fb84e99c2

rothgar · 2017-11-15T18:01:23Z

hack/multi-node/user-data.sample

        ExecStartPre=/bin/mkdir -p /etc/kubernetes/cni/net.d
        ExecStartPre=/bin/mkdir -p /etc/kubernetes/checkpoint-secrets
        ExecStartPre=/bin/mkdir -p /etc/kubernetes/inactive-manifests
+        ExecStartPre=/bin/mkdir -p /etc/kubernetes/kubelet/tls


Default directory for kubeadm is /etc/kubernetes/pki. Does bootkube care to be the same? Just might help people who use both systems but not a real concern for functionality.

@rothgar I think that's a good suggestion, but tls is used in a few places in bootkube. Mind opening an issue to suggest aligning with kubeadm? We can address in a separate PR.

@jcbsmpsn

Automatic merge from submit-queue (batch tested with PRs 54773, 52523, 47497, 55356, 49429). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. certificate manager: reduce max backoff from 128s to 32s For TLS bootstrapping in bootkube we run a kubelet with a control plane run through static pods. That static control plane has an API server and controller manager that approve the kubelet's CSR. Since the kubelet has to wait for the static control plane to come up to be approved, we hit this backoff every time and it actually adds a notable overhead to startup times. kubernetes-retired/bootkube#663 If this choice is somewhat arbitrary, I'd like to see it lowered for 1.9. /assign @jcbsmpsn @mikedanese ```release-note NONE ``` Kubernetes-commit: 0ff21718d127b9fc14bdfc068624e82fb84e99c2

@jcbsmpsn

Automatic merge from submit-queue (batch tested with PRs 54773, 52523, 47497, 55356, 49429). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. certificate manager: reduce max backoff from 128s to 32s For TLS bootstrapping in bootkube we run a kubelet with a control plane run through static pods. That static control plane has an API server and controller manager that approve the kubelet's CSR. Since the kubelet has to wait for the static control plane to come up to be approved, we hit this backoff every time and it actually adds a notable overhead to startup times. kubernetes-retired/bootkube#663 If this choice is somewhat arbitrary, I'd like to see it lowered for 1.9. /assign @jcbsmpsn @mikedanese ```release-note NONE ``` Kubernetes-commit: 0ff21718d127b9fc14bdfc068624e82fb84e99c2

redbaron · 2017-12-11T08:33:56Z

Checkpointer is running in a dedicated service account now, is there anything left [I can help with], which stops this PR from being merged?

diegs · 2017-12-11T18:57:01Z

@redbaron I am wrapping up #784 which makes the tests better and changes the checkpointer to only look at its own namespace, which relaxes the required roll bindings. Hoping to finish that up in the next day or two.

ericchiang · 2017-12-11T18:59:57Z

Checkpointer is running in a dedicated service account now, is there anything left [I can help with], which stops this PR from being merged?

This is actually already done, even without @diegs changes. The changes were manifest only and didn't require any changes to the checkpointer itself.

I think we still need to add an approve all rule for rotation[1 (different from bootstrap approval) as well as docs. Will update this when I get the chance.

[1] https://kubernetes.io/docs/admin/kubelet-tls-bootstrapping/#kube-controller-manager-configuration

redbaron · 2017-12-12T08:15:54Z

hack/single-node/user-data.sample

@@ -38,7 +42,8 @@ coreos:
          --lock-file=/var/run/lock/kubelet.lock \
          --network-plugin=cni \
          --node-labels=node-role.kubernetes.io/master \
-          --pod-manifest-path=/etc/kubernetes/manifests
+          --pod-manifest-path=/etc/kubernetes/manifests \
+          --rotate-certificates


shouldn't it be --bootstrap-kubeconfig and --require-kubeconfig as per https://kubernetes.io/docs/admin/kubelet-tls-bootstrapping/#kubelet-configuration ?

This is actually intentional. We want to start static pods even if the rotation hasn't completed. --bootstrap-kubeconfig will block, but --rotate-certificates wont, even though they both have the same effect.

ericchiang · 2017-12-13T21:26:18Z

Still waiting for e2e. If this passes then I think the code bits are done.

We still need to do some rotation testing (e.g. if we run a long lived cluster the rotation works) and add some docs.

aaronlevy · 2018-01-05T20:40:52Z

@ericchiang any updates on this?

ericchiang · 2018-01-05T21:01:38Z

No I haven't had time to do the short rotation time period testing.

ericchiang · 2018-01-08T20:57:34Z

Found a bug in the rotation code. It's fixed now.

Tested rotation with the following diff. We probably want to do something similar during the e2e tests. Maybe as a follow up?

--- a/pkg/asset/internal/templates.go               
+++ b/pkg/asset/internal/templates.go               
@@ -503,6 +503,7 @@ spec:                           
         - --leader-elect=true                      
         - --root-ca-file=/etc/kubernetes/secrets/ca.crt                                                 
         - --service-account-private-key-file=/etc/kubernetes/secrets/service-account.key                
+        - --experimental-cluster-signing-duration=15m                                                   
         livenessProbe:                             
           httpGet:                                 
             path: /healthz                                       
@@ -581,6 +581,7 @@ spec:                           
     - --leader-elect=true                          
     - --root-ca-file=/etc/kubernetes/secrets/ca.crt                                                     
     - --service-account-private-key-file=/etc/kubernetes/secrets/service-account.key                    
+    - --experimental-cluster-signing-duration=15m  
     volumeMounts:                                  
     - name: secrets                                
       mountPath: /etc/kubernetes/secrets

diegs · 2018-01-08T20:58:53Z

@ericchiang sure, that sounds good, except does that require waiting 15m for the certs to rotate?

ericchiang · 2018-01-08T20:59:47Z

@diegs yes. Maybe during conformance instead?

diegs · 2018-01-08T21:01:41Z

@ericchiang SG. also code LGTM but I can't LGTM "my own" PR.

ericchiang · 2018-01-08T21:54:16Z

coreosbot run e2e

ericchiang

adding approval on behalf of @diegs

diegs · 2018-01-08T23:23:21Z

Needs another approval from a member, cc @rphillips @aaronlevy

aaronlevy

Two really small nits that are fine to leave in.

aaronlevy · 2018-01-09T01:09:32Z

pkg/asset/k8s.go

+	}
+
+	for i, b := range token {
+		token[i] = validBootstrapTokenChars[int(uint8(b))%len(validBootstrapTokenChars)]


nit: don't think the uint8() necessary

aaronlevy · 2018-01-09T01:10:12Z

pkg/asset/k8s.go

+// newBootstrapToken constructs a bootstrap token in conformance with the following format:
+// https://kubernetes.io/docs/admin/bootstrap-tokens/#token-format
+func newBootstrapToken() (id string, secret string, err error) {
+	token := make([]byte, 6+16)


nit: not immediately clear what 6+16 is (id+secret).

Added comment.

@mattmoyer

Automatic merge from submit-queue (batch tested with PRs 56290, 57984). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubeadm: more random tokens The strategy of hex encoding a random byte array only uses the following characters: 0123456789abcdef Instead of the entire bootstrapping token character set: 0123456789abcdefghijklmnopqrstuvwxyz Update the token generation to use the entire character set. This increases the token secret from 48 bits of entropy to ~82 bits. 256^8 (1.8e+19) vs. 36^16 (7.9e+24). Noticed this writing kubernetes-retired/bootkube#663 ```release-note NONE ``` /cc @mattmoyer @luxas

diegs self-assigned this Jul 27, 2017

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

ericchiang reviewed Jul 27, 2017

View reviewed changes

ericchiang mentioned this pull request Jul 31, 2017

certificate manager: close existing client conns once cert rotates kubernetes/kubernetes#49899

Merged

diegs force-pushed the tls branch 3 times, most recently from 3e2f0d2 to cd0da11 Compare August 2, 2017 04:04

diegs force-pushed the tls branch from cd0da11 to 0a4181e Compare August 2, 2017 18:46

diegs force-pushed the tls branch from 9cd0179 to 3d85734 Compare August 22, 2017 23:27

ericchiang force-pushed the tls branch from 3d85734 to d0106e4 Compare November 7, 2017 23:01

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 7, 2017

ericchiang force-pushed the tls branch 2 times, most recently from e0c4ff9 to e5bcfe8 Compare November 8, 2017 23:41

ericchiang mentioned this pull request Nov 9, 2017

certificate manager: reduce max backoff from 128s to 32s kubernetes/kubernetes#55356

Merged

ericchiang reviewed Nov 9, 2017

View reviewed changes

ericchiang mentioned this pull request Nov 9, 2017

pkg/asset: use service accounts for checkpointer and kube-proxy creds #767

Merged

rothgar reviewed Nov 15, 2017

View reviewed changes

rothgar mentioned this pull request Nov 15, 2017

Use same certificate folder as kubeadm #773

Closed

redbaron reviewed Dec 12, 2017

View reviewed changes

ericchiang force-pushed the tls branch from 1e259db to d7f71f3 Compare December 13, 2017 21:00

diegs changed the title ~~WIP: Add TLS auto-approval for kubelet.~~ Add TLS auto-approval for kubelet. Dec 18, 2017

ericchiang force-pushed the tls branch from d7f71f3 to 55b928e Compare January 8, 2018 20:55

ericchiang approved these changes Jan 8, 2018

View reviewed changes

aaronlevy approved these changes Jan 9, 2018

View reviewed changes

ericchiang mentioned this pull request Jan 9, 2018

kubeadm: more random tokens kubernetes/kubernetes#57984

Merged

ericchiang added 2 commits January 8, 2018 18:37

pkg: add CSR auto-approval manifests

9aa8cf5

hack: kubelet host changes for CSR auto-approval

b1af653

diegs force-pushed the tls branch from 55b928e to b1af653 Compare January 9, 2018 02:41

diegs merged commit 38a5af6 into kubernetes-retired:master Jan 9, 2018

diegs deleted the tls branch January 9, 2018 03:37

ericchiang mentioned this pull request Jan 19, 2018

conformance: use admin kubeconfig #845

Merged

abhinavdahiya mentioned this pull request Jan 24, 2018

Kubelet tls bootstrap coreos/tectonic-installer#2809

Merged

enj mentioned this pull request Feb 10, 2023

scheduler/extender: collapse client wiring onto standard approach kubernetes/kubernetes#115687

Closed

Add TLS auto-approval for kubelet. #663

Add TLS auto-approval for kubelet. #663

Conversation

diegs commented Jul 27, 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

ericchiang commented Jul 27, 2017

jcbsmpsn commented Jul 27, 2017

liggitt commented Jul 28, 2017

liggitt commented Jul 28, 2017

ericchiang commented Jul 28, 2017

jcbsmpsn commented Jul 28, 2017

jcbsmpsn commented Aug 2, 2017

diegs commented Aug 2, 2017

ericchiang commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbaron commented Dec 11, 2017 • edited Loading

diegs commented Dec 11, 2017

ericchiang commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Dec 13, 2017

aaronlevy commented Jan 5, 2018

ericchiang commented Jan 5, 2018

ericchiang commented Jan 8, 2018 • edited Loading

diegs commented Jan 8, 2018

ericchiang commented Jan 8, 2018

diegs commented Jan 8, 2018

ericchiang commented Jan 8, 2018

ericchiang left a comment

Choose a reason for hiding this comment

diegs commented Jan 8, 2018

aaronlevy left a comment

Choose a reason for hiding this comment

aaronlevy Jan 9, 2018 • 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

ericchiang commented Nov 7, 2017 •

edited

Loading

redbaron commented Dec 11, 2017 •

edited

Loading

ericchiang commented Jan 8, 2018 •

edited

Loading

aaronlevy Jan 9, 2018 •

edited

Loading