From ae6ee96b36d0f3ed1a53bf23b218e300c5ac0baf Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 25 Sep 2017 23:18:57 -0400 Subject: [PATCH 1/9] Verify the bootstrap client cert before using it Before the bootstrap client is used, check a number of conditions that ensure it can be safely loaded by the server. If any of those conditions are invalid, re-bootstrap the node. This is primarily to force bootstrapping without human intervention when a certificate is expired, but also handles partial file corruption. --- pkg/kubelet/certificate/bootstrap/BUILD | 2 + .../certificate/bootstrap/bootstrap.go | 64 ++++++++++++++++--- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/pkg/kubelet/certificate/bootstrap/BUILD b/pkg/kubelet/certificate/bootstrap/BUILD index e4aeb0e97fa..3d12e166a41 100644 --- a/pkg/kubelet/certificate/bootstrap/BUILD +++ b/pkg/kubelet/certificate/bootstrap/BUILD @@ -25,10 +25,12 @@ go_library( "//pkg/kubelet/util/csr:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/rest:go_default_library", "//vendor/k8s.io/client-go/tools/clientcmd:go_default_library", "//vendor/k8s.io/client-go/tools/clientcmd/api:go_default_library", + "//vendor/k8s.io/client-go/transport:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", ], ) diff --git a/pkg/kubelet/certificate/bootstrap/bootstrap.go b/pkg/kubelet/certificate/bootstrap/bootstrap.go index 2952ba0bd62..ab13a9de84b 100644 --- a/pkg/kubelet/certificate/bootstrap/bootstrap.go +++ b/pkg/kubelet/certificate/bootstrap/bootstrap.go @@ -20,14 +20,17 @@ import ( "fmt" "os" "path/filepath" + "time" "github.com/golang/glog" "k8s.io/apimachinery/pkg/types" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" certificates "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + "k8s.io/client-go/transport" certutil "k8s.io/client-go/util/cert" "k8s.io/kubernetes/pkg/kubelet/util/csr" ) @@ -42,17 +45,15 @@ const ( // On success, a kubeconfig file referencing the generated key and obtained certificate is written to kubeconfigPath. // The certificate and key file are stored in certDir. func LoadClientCert(kubeconfigPath string, bootstrapPath string, certDir string, nodeName types.NodeName) error { - // Short-circuit if the kubeconfig file already exists. - // TODO: inspect the kubeconfig, ensure a rest client can be built from it, verify client cert expiration, etc. - _, err := os.Stat(kubeconfigPath) - if err == nil { - glog.V(2).Infof("Kubeconfig %s exists, skipping bootstrap", kubeconfigPath) - return nil - } - if !os.IsNotExist(err) { - glog.Errorf("Error reading kubeconfig %s, skipping bootstrap: %v", kubeconfigPath, err) + // Short-circuit if the kubeconfig file exists and is valid. + ok, err := verifyBootstrapClientConfig(kubeconfigPath) + if err != nil { return err } + if ok { + glog.V(2).Infof("Kubeconfig %s exists and is valid, skipping bootstrap", kubeconfigPath) + return nil + } glog.V(2).Info("Using bootstrap kubeconfig to generate TLS client cert, key and kubeconfig file") @@ -150,3 +151,48 @@ func loadRESTClientConfig(kubeconfig string) (*restclient.Config, error) { loader, ).ClientConfig() } + +// verifyBootstrapClientConfig checks the provided kubeconfig to see if it has a valid +// client certificate. It returns true if the kubeconfig is valid, or an error if bootstrapping +// should stop immediately. +func verifyBootstrapClientConfig(kubeconfigPath string) (bool, error) { + _, err := os.Stat(kubeconfigPath) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("error reading existing bootstrap kubeconfig %s: %v", kubeconfigPath, err) + } + bootstrapClientConfig, err := loadRESTClientConfig(kubeconfigPath) + if err != nil { + utilruntime.HandleError(fmt.Errorf("Unable to read existing bootstrap client config: %v", err)) + return false, nil + } + transportConfig, err := bootstrapClientConfig.TransportConfig() + if err != nil { + utilruntime.HandleError(fmt.Errorf("Unable to load transport configuration from existing bootstrap client config: %v", err)) + return false, nil + } + // has side effect of populating transport config data fields + if _, err := transport.TLSConfigFor(transportConfig); err != nil { + utilruntime.HandleError(fmt.Errorf("Unable to load TLS configuration from existing bootstrap client config: %v", err)) + return false, nil + } + certs, err := certutil.ParseCertsPEM(transportConfig.TLS.CertData) + if err != nil { + utilruntime.HandleError(fmt.Errorf("Unable to load TLS certificates from existing bootstrap client config: %v", err)) + return false, nil + } + if len(certs) == 0 { + utilruntime.HandleError(fmt.Errorf("Unable to read TLS certificates from existing bootstrap client config: %v", err)) + return false, nil + } + now := time.Now() + for _, cert := range certs { + if now.After(cert.NotAfter) { + utilruntime.HandleError(fmt.Errorf("Part of the existing bootstrap client certificate is expired: %s", cert.NotAfter)) + return false, nil + } + } + return true, nil +} From 74a0abb6992c44e1e0191def21627a350847bd7f Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 25 Sep 2017 23:49:05 -0400 Subject: [PATCH 2/9] An expired certificate is not compatible If the certificate in the CSR is expired, it's no good to the code. Error out with the correct message. --- pkg/kubelet/util/csr/csr.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/kubelet/util/csr/csr.go b/pkg/kubelet/util/csr/csr.go index 316de1abe5f..5c2fb8549f7 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/pkg/kubelet/util/csr/csr.go @@ -203,5 +203,17 @@ func ensureCompatible(new, orig *certificates.CertificateSigningRequest, private if err := newCsr.CheckSignature(); err != nil { return fmt.Errorf("error validating signature new CSR against old key: %v", err) } + if len(new.Status.Certificate) > 0 { + certs, err := certutil.ParseCertsPEM(new.Status.Certificate) + if err != nil { + return fmt.Errorf("error parsing signed certificate for CSR: %v", err) + } + now := time.Now() + for _, cert := range certs { + if now.After(cert.NotAfter) { + return fmt.Errorf("one of the certificates for the CSR has expired: %s", cert.NotAfter) + } + } + } return nil } From 710dfb3427aec9eb9f323ec7b0be8b9728b8c3e3 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 28 Sep 2017 23:50:07 -0400 Subject: [PATCH 3/9] Delete the private key for the bootstrap client cert on failure Ensures that in a crash loop state we can make forward progress by generating a new key and hence new CSR. If we do not delete the key, an expired CSR may block startup. Also more aggressively delete a bad cert path --- .../certificate/bootstrap/bootstrap.go | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/pkg/kubelet/certificate/bootstrap/bootstrap.go b/pkg/kubelet/certificate/bootstrap/bootstrap.go index ab13a9de84b..2a954332d27 100644 --- a/pkg/kubelet/certificate/bootstrap/bootstrap.go +++ b/pkg/kubelet/certificate/bootstrap/bootstrap.go @@ -73,6 +73,17 @@ func LoadClientCert(kubeconfigPath string, bootstrapPath string, certDir string, if err != nil { return fmt.Errorf("unable to build bootstrap key path: %v", err) } + // If we are unable to generate a CSR, we remove our key file and start fresh. + // This method is used before enabling client rotation and so we must ensure we + // can make forward progress if we crash and exit when a CSR exists but the cert + // it is signed for has expired. + defer func() { + if !success { + if err := os.Remove(keyPath); err != nil && !os.IsNotExist(err) { + glog.Warningf("Cannot clean up the key file %q: %v", keyPath, err) + } + } + }() keyData, _, err := certutil.LoadOrGenerateKeyFile(keyPath) if err != nil { return err @@ -83,6 +94,13 @@ func LoadClientCert(kubeconfigPath string, bootstrapPath string, certDir string, if err != nil { return fmt.Errorf("unable to build bootstrap client cert path: %v", err) } + defer func() { + if !success { + if err := os.Remove(certPath); err != nil && !os.IsNotExist(err) { + glog.Warningf("Cannot clean up the cert file %q: %v", certPath, err) + } + } + }() certData, err := csr.RequestNodeCertificate(bootstrapClient.CertificateSigningRequests(), keyData, nodeName) if err != nil { return err @@ -90,13 +108,6 @@ func LoadClientCert(kubeconfigPath string, bootstrapPath string, certDir string, if err := certutil.WriteCert(certPath, certData); err != nil { return err } - defer func() { - if !success { - if err := os.Remove(certPath); err != nil { - glog.Warningf("Cannot clean up the cert file %q: %v", certPath, err) - } - } - }() // Get the CA data from the bootstrap client config. caFile, caData := bootstrapClientConfig.CAFile, []byte{} From de3d7d188165a039395de1599d48cc4f40e41566 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sat, 30 Sep 2017 17:24:57 -0400 Subject: [PATCH 4/9] If CSR is deleted, exit immediately No point in waiting --- pkg/kubelet/util/csr/csr.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/kubelet/util/csr/csr.go b/pkg/kubelet/util/csr/csr.go index 5c2fb8549f7..7ccf581b31f 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/pkg/kubelet/util/csr/csr.go @@ -124,6 +124,8 @@ func requestCertificate(client certificatesclient.CertificateSigningRequestInter func(event watch.Event) (bool, error) { switch event.Type { case watch.Modified, watch.Added: + case watch.Deleted: + return false, fmt.Errorf("csr %q was deleted", csr.Name) default: return false, nil } From c3bea24ab6242b084e7d09073c8906c778611243 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 1 Oct 2017 16:10:51 -0400 Subject: [PATCH 5/9] Collapse duplicate code into pkg/util/csr There is no reason to duplicate this code into two places. --- pkg/kubelet/util/csr/csr.go | 12 +-- .../k8s.io/client-go/util/certificate/BUILD | 4 +- .../util/certificate/certificate_manager.go | 81 ++----------------- .../certificate/certificate_manager_test.go | 12 +++ 4 files changed, 28 insertions(+), 81 deletions(-) diff --git a/pkg/kubelet/util/csr/csr.go b/pkg/kubelet/util/csr/csr.go index 7ccf581b31f..281295a2611 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/pkg/kubelet/util/csr/csr.go @@ -68,16 +68,16 @@ func RequestNodeCertificate(client certificatesclient.CertificateSigningRequestI certificates.UsageClientAuth, } name := digestedName(privateKeyData, subject, usages) - return requestCertificate(client, csrData, name, usages, privateKey) + return RequestCertificate(client, csrData, name, usages, privateKey) } -// requestCertificate will either use an existing (if this process has run +// RequestCertificate will either use an existing (if this process has run // before but not to completion) or create a certificate signing request using the // PEM encoded CSR and send it to API server, then it will watch the object's // status, once approved by API server, it will return the API server's issued // certificate (pem-encoded). If there is any errors, or the watch timeouts, it // will return an error. -func requestCertificate(client certificatesclient.CertificateSigningRequestInterface, csrData []byte, name string, usages []certificates.KeyUsage, privateKey interface{}) (certData []byte, err error) { +func RequestCertificate(client certificatesclient.CertificateSigningRequestInterface, csrData []byte, name string, usages []certificates.KeyUsage, privateKey interface{}) (certData []byte, err error) { csr := &certificates.CertificateSigningRequest{ // Username, UID, Groups will be injected by API server. TypeMeta: metav1.TypeMeta{Kind: "CertificateSigningRequest"}, @@ -89,11 +89,14 @@ func requestCertificate(client certificatesclient.CertificateSigningRequestInter Usages: usages, }, } + if len(csr.Name) == 0 { + csr.GenerateName = "csr-" + } req, err := client.Create(csr) switch { case err == nil: - case errors.IsAlreadyExists(err): + case errors.IsAlreadyExists(err) && len(name) > 0: glog.Infof("csr for this node already exists, reusing") req, err = client.Get(name, metav1.GetOptions{}) if err != nil { @@ -149,7 +152,6 @@ func requestCertificate(client certificatesclient.CertificateSigningRequestInter } return event.Object.(*certificates.CertificateSigningRequest).Status.Certificate, nil - } // This digest should include all the relevant pieces of the CSR we care about. diff --git a/staging/src/k8s.io/client-go/util/certificate/BUILD b/staging/src/k8s.io/client-go/util/certificate/BUILD index 87f88964cd1..c0e32aaaf46 100644 --- a/staging/src/k8s.io/client-go/util/certificate/BUILD +++ b/staging/src/k8s.io/client-go/util/certificate/BUILD @@ -35,12 +35,10 @@ go_library( importpath = "k8s.io/client-go/util/certificate", tags = ["automanaged"], deps = [ + "//pkg/kubelet/util/csr:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", ], diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go index 08363491b81..7eb4aaa74a0 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go @@ -30,12 +30,10 @@ import ( "github.com/golang/glog" certificates "k8s.io/api/certificates/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/apimachinery/pkg/watch" certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" "k8s.io/client-go/util/cert" + "k8s.io/kubernetes/pkg/kubelet/util/csr" ) // Manager maintains and updates the certificates in use by this certificate @@ -278,7 +276,7 @@ func (m *manager) shouldRotate() bool { func (m *manager) rotateCerts() (bool, error) { glog.V(2).Infof("Rotating certificates") - csrPEM, keyPEM, err := m.generateCSR() + csrPEM, keyPEM, privateKey, err := m.generateCSR() if err != nil { glog.Errorf("Unable to generate a certificate signing request: %v", err) return false, nil @@ -286,7 +284,7 @@ func (m *manager) rotateCerts() (bool, error) { // Call the Certificate Signing Request API to get a certificate for the // new private key. - crtPEM, err := requestCertificate(m.certSigningRequestClient, csrPEM, m.usages) + crtPEM, err := csr.RequestCertificate(m.certSigningRequestClient, csrPEM, "", m.usages, privateKey) if err != nil { glog.Errorf("Failed while requesting a signed certificate from the master: %v", err) return false, nil @@ -343,85 +341,22 @@ func (m *manager) updateCached(cert *tls.Certificate) { m.cert = cert } -func (m *manager) generateCSR() (csrPEM []byte, keyPEM []byte, err error) { +func (m *manager) generateCSR() (csrPEM []byte, keyPEM []byte, key interface{}, err error) { // Generate a new private key. privateKey, err := ecdsa.GenerateKey(elliptic.P256(), cryptorand.Reader) if err != nil { - return nil, nil, fmt.Errorf("unable to generate a new private key: %v", err) + return nil, nil, nil, fmt.Errorf("unable to generate a new private key: %v", err) } der, err := x509.MarshalECPrivateKey(privateKey) if err != nil { - return nil, nil, fmt.Errorf("unable to marshal the new key to DER: %v", err) + return nil, nil, nil, fmt.Errorf("unable to marshal the new key to DER: %v", err) } keyPEM = pem.EncodeToMemory(&pem.Block{Type: cert.ECPrivateKeyBlockType, Bytes: der}) csrPEM, err = cert.MakeCSRFromTemplate(privateKey, m.template) if err != nil { - return nil, nil, fmt.Errorf("unable to create a csr from the private key: %v", err) + return nil, nil, nil, fmt.Errorf("unable to create a csr from the private key: %v", err) } - return csrPEM, keyPEM, nil -} - -// requestCertificate will create a certificate signing request using the PEM -// encoded CSR and send it to API server, then it will watch the object's -// status, once approved by API server, it will return the API server's issued -// certificate (pem-encoded). If there is any errors, or the watch timeouts, it -// will return an error. -// -// NOTE This is a copy of a function with the same name in -// k8s.io/kubernetes/pkg/kubelet/util/csr/csr.go, changing only the package that -// CertificateSigningRequestInterface and KeyUsage are imported from. -func requestCertificate(client certificatesclient.CertificateSigningRequestInterface, csrData []byte, usages []certificates.KeyUsage) (certData []byte, err error) { - glog.Infof("Requesting new certificate.") - req, err := client.Create(&certificates.CertificateSigningRequest{ - // Username, UID, Groups will be injected by API server. - TypeMeta: metav1.TypeMeta{Kind: "CertificateSigningRequest"}, - ObjectMeta: metav1.ObjectMeta{GenerateName: "csr-"}, - - Spec: certificates.CertificateSigningRequestSpec{ - Request: csrData, - Usages: usages, - }, - }) - if err != nil { - return nil, fmt.Errorf("cannot create certificate signing request: %v", err) - } - - // Make a default timeout = 3600s. - var defaultTimeoutSeconds int64 = 3600 - certWatch, err := client.Watch(metav1.ListOptions{ - Watch: true, - TimeoutSeconds: &defaultTimeoutSeconds, - FieldSelector: fields.OneTermEqualSelector("metadata.name", req.Name).String(), - }) - if err != nil { - return nil, fmt.Errorf("cannot watch on the certificate signing request: %v", err) - } - defer certWatch.Stop() - ch := certWatch.ResultChan() - - for { - event, ok := <-ch - if !ok { - break - } - - if event.Type == watch.Modified || event.Type == watch.Added { - if event.Object.(*certificates.CertificateSigningRequest).UID != req.UID { - continue - } - status := event.Object.(*certificates.CertificateSigningRequest).Status - for _, c := range status.Conditions { - if c.Type == certificates.CertificateDenied { - return nil, fmt.Errorf("certificate signing request is not approved, reason: %v, message: %v", c.Reason, c.Message) - } - if c.Type == certificates.CertificateApproved && status.Certificate != nil { - return status.Certificate, nil - } - } - } - } - - return nil, fmt.Errorf("watch channel closed") + return csrPEM, keyPEM, privateKey, nil } diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go b/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go index 6a77f99bf52..3df131e7d2b 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go @@ -643,6 +643,18 @@ type fakeClient struct { certificatePEM []byte } +func (c fakeClient) List(opts v1.ListOptions) (*certificates.CertificateSigningRequestList, error) { + if c.failureType == watchError { + return nil, fmt.Errorf("Watch error") + } + csrReply := certificates.CertificateSigningRequestList{ + Items: []certificates.CertificateSigningRequest{ + {ObjectMeta: v1.ObjectMeta{UID: "fake-uid"}}, + }, + } + return &csrReply, nil +} + func (c fakeClient) Create(*certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) { if c.failureType == createError { return nil, fmt.Errorf("Create error") From 7555dec82ed0afcff7fef5f7a6df36cebabecc68 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Sun, 1 Oct 2017 16:52:07 -0400 Subject: [PATCH 6/9] Kubelet should exit if the current client cert has expired The client cert manager uses the most recent cert to request new certificates. If that certificate is expired, it will be unable to complete new CSR requests. This commit alters the manager to force process exit if no further client cert rotation is possible, which is expected to trigger a restart of the kubelet and either a re-bootstrap from the bootstrap kubeconfig or a re-read of the current disk state (assuming that some other agent is managing the bootstrap configuration). This prevents the Kubelet from wedging in a state where it cannot make API calls. --- cmd/kubelet/app/server.go | 4 +++- pkg/kubelet/certificate/transport.go | 9 ++++++--- pkg/kubelet/certificate/transport_test.go | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go index 099b5f50da3..66badd8f2d6 100644 --- a/cmd/kubelet/app/server.go +++ b/cmd/kubelet/app/server.go @@ -342,7 +342,9 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies) (err error) { if err != nil { return err } - if err := kubeletcertificate.UpdateTransport(wait.NeverStop, clientConfig, clientCertificateManager); err != nil { + // we set exitIfExpired to true because we use this client configuration to request new certs - if we are unable + // to request new certs, we will be unable to continue normal operation + if err := kubeletcertificate.UpdateTransport(wait.NeverStop, clientConfig, clientCertificateManager, true); err != nil { return err } } diff --git a/pkg/kubelet/certificate/transport.go b/pkg/kubelet/certificate/transport.go index 49c089b44a9..d206f23cdeb 100644 --- a/pkg/kubelet/certificate/transport.go +++ b/pkg/kubelet/certificate/transport.go @@ -45,13 +45,13 @@ import ( // // stopCh should be used to indicate when the transport is unused and doesn't need // to continue checking the manager. -func UpdateTransport(stopCh <-chan struct{}, clientConfig *restclient.Config, clientCertificateManager certificate.Manager) error { - return updateTransport(stopCh, 10*time.Second, clientConfig, clientCertificateManager) +func UpdateTransport(stopCh <-chan struct{}, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitIfExpired bool) error { + return updateTransport(stopCh, 10*time.Second, clientConfig, clientCertificateManager, exitIfExpired) } // updateTransport is an internal method that exposes how often this method checks that the // client cert has changed. Intended for testing. -func updateTransport(stopCh <-chan struct{}, period time.Duration, clientConfig *restclient.Config, clientCertificateManager certificate.Manager) error { +func updateTransport(stopCh <-chan struct{}, period time.Duration, clientConfig *restclient.Config, clientCertificateManager certificate.Manager, exitIfExpired bool) error { if clientConfig.Transport != nil { return fmt.Errorf("there is already a transport configured") } @@ -80,6 +80,9 @@ func updateTransport(stopCh <-chan struct{}, period time.Duration, clientConfig lastCert := clientCertificateManager.Current() go wait.Until(func() { curr := clientCertificateManager.Current() + if exitIfExpired && curr != nil && time.Now().After(curr.Leaf.NotAfter) { + glog.Fatalf("The currently active client certificate has expired, exiting.") + } if curr == nil || lastCert == curr { // Cert hasn't been rotated. return diff --git a/pkg/kubelet/certificate/transport_test.go b/pkg/kubelet/certificate/transport_test.go index 306c3c18a03..5dce3eafb4c 100644 --- a/pkg/kubelet/certificate/transport_test.go +++ b/pkg/kubelet/certificate/transport_test.go @@ -184,7 +184,7 @@ func TestRotateShutsDownConnections(t *testing.T) { } // Check for a new cert every 10 milliseconds - if err := updateTransport(stop, 10*time.Millisecond, c, m); err != nil { + if err := updateTransport(stop, 10*time.Millisecond, c, m, false); err != nil { t.Fatal(err) } From cbecf177274e6f6924d6ca756eccf0a55e2933c0 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 5 Oct 2017 18:51:57 -0400 Subject: [PATCH 7/9] cache.ListWatchUntil should return err.ErrWaitTimeout Clients shouldn't have to know about watch.ErrWatchClosed, which is typically a server side decision to close and always means "Timeout" in this conetxt. --- pkg/kubelet/util/csr/csr.go | 4 ++++ staging/src/k8s.io/client-go/tools/cache/listwatch.go | 10 +++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/kubelet/util/csr/csr.go b/pkg/kubelet/util/csr/csr.go index 281295a2611..9bbb6a7bc90 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/pkg/kubelet/util/csr/csr.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" "k8s.io/client-go/tools/cache" @@ -147,6 +148,9 @@ func RequestCertificate(client certificatesclient.CertificateSigningRequestInter return false, nil }, ) + if err == wait.ErrWaitTimeout { + return nil, wait.ErrWaitTimeout + } if err != nil { return nil, fmt.Errorf("cannot watch on the certificate signing request: %v", err) } diff --git a/staging/src/k8s.io/client-go/tools/cache/listwatch.go b/staging/src/k8s.io/client-go/tools/cache/listwatch.go index 454d50aadc6..db2329c55a2 100644 --- a/staging/src/k8s.io/client-go/tools/cache/listwatch.go +++ b/staging/src/k8s.io/client-go/tools/cache/listwatch.go @@ -25,6 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/pager" @@ -103,6 +104,8 @@ func (lw *ListWatch) Watch(options metav1.ListOptions) (watch.Interface, error) return lw.WatchFunc(options) } +// ListWatchUntil checks the provided conditions against the items returned by the list watcher, returning wait.ErrWaitTimeout +// if timeout is exceeded without all conditions returning true, or an error if an error occurs. // TODO: check for watch expired error and retry watch from latest point? Same issue exists for Until. func ListWatchUntil(timeout time.Duration, lw ListerWatcher, conditions ...watch.ConditionFunc) (*watch.Event, error) { if len(conditions) == 0 { @@ -166,5 +169,10 @@ func ListWatchUntil(timeout time.Duration, lw ListerWatcher, conditions ...watch return nil, err } - return watch.Until(timeout, watchInterface, remainingConditions...) + evt, err := watch.Until(timeout, watchInterface, remainingConditions...) + if err == watch.ErrWatchClosed { + // present a consistent error interface to callers + err = wait.ErrWaitTimeout + } + return evt, err } From b3a11aa635022761637090f4fc8d5cb57f3f0010 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 5 Oct 2017 18:57:53 -0400 Subject: [PATCH 8/9] Have the certificate manager decide if the server is healthy Prevent a Kubelet from shutting down when the server isn't responding to us but we cannot get a new certificate. This allows a cluster to coast if the master is unresponsive or a node is partitioned and their client cert expires. --- pkg/kubelet/certificate/transport.go | 6 +- pkg/kubelet/certificate/transport_test.go | 5 +- pkg/kubelet/util/csr/BUILD | 1 + pkg/kubelet/util/csr/csr.go | 35 +++- .../k8s.io/client-go/util/certificate/BUILD | 5 + .../util/certificate/certificate_manager.go | 94 ++++++++- .../certificate/certificate_manager_test.go | 179 +++++++++++++++++- 7 files changed, 304 insertions(+), 21 deletions(-) diff --git a/pkg/kubelet/certificate/transport.go b/pkg/kubelet/certificate/transport.go index d206f23cdeb..1683ad09735 100644 --- a/pkg/kubelet/certificate/transport.go +++ b/pkg/kubelet/certificate/transport.go @@ -81,7 +81,11 @@ func updateTransport(stopCh <-chan struct{}, period time.Duration, clientConfig go wait.Until(func() { curr := clientCertificateManager.Current() if exitIfExpired && curr != nil && time.Now().After(curr.Leaf.NotAfter) { - glog.Fatalf("The currently active client certificate has expired, exiting.") + if clientCertificateManager.ServerHealthy() { + glog.Fatalf("The currently active client certificate has expired and the server is responsive, exiting.") + } else { + glog.Errorf("The currently active client certificate has expired, but the server is not responsive. A restart may be necessary to retrieve new initial credentials.") + } } if curr == nil || lastCert == curr { // Cert hasn't been rotated. diff --git a/pkg/kubelet/certificate/transport_test.go b/pkg/kubelet/certificate/transport_test.go index 5dce3eafb4c..6e476c3b803 100644 --- a/pkg/kubelet/certificate/transport_test.go +++ b/pkg/kubelet/certificate/transport_test.go @@ -114,13 +114,16 @@ func newCertificateData(certificatePEM string, keyPEM string) *certificateData { } type fakeManager struct { - cert atomic.Value // Always a *tls.Certificate + cert atomic.Value // Always a *tls.Certificate + healthy bool } func (f *fakeManager) SetCertificateSigningRequestClient(certificatesclient.CertificateSigningRequestInterface) error { return nil } +func (f *fakeManager) ServerHealthy() bool { return f.healthy } + func (f *fakeManager) Start() {} func (f *fakeManager) Current() *tls.Certificate { diff --git a/pkg/kubelet/util/csr/BUILD b/pkg/kubelet/util/csr/BUILD index 3414983f2cf..78c435528b5 100644 --- a/pkg/kubelet/util/csr/BUILD +++ b/pkg/kubelet/util/csr/BUILD @@ -19,6 +19,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/tools/cache:go_default_library", diff --git a/pkg/kubelet/util/csr/csr.go b/pkg/kubelet/util/csr/csr.go index 9bbb6a7bc90..53b67f8650e 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/pkg/kubelet/util/csr/csr.go @@ -69,7 +69,11 @@ func RequestNodeCertificate(client certificatesclient.CertificateSigningRequestI certificates.UsageClientAuth, } name := digestedName(privateKeyData, subject, usages) - return RequestCertificate(client, csrData, name, usages, privateKey) + req, err := RequestCertificate(client, csrData, name, usages, privateKey) + if err != nil { + return nil, err + } + return WaitForCertificate(client, req, 3600*time.Second) } // RequestCertificate will either use an existing (if this process has run @@ -78,7 +82,7 @@ func RequestNodeCertificate(client certificatesclient.CertificateSigningRequestI // status, once approved by API server, it will return the API server's issued // certificate (pem-encoded). If there is any errors, or the watch timeouts, it // will return an error. -func RequestCertificate(client certificatesclient.CertificateSigningRequestInterface, csrData []byte, name string, usages []certificates.KeyUsage, privateKey interface{}) (certData []byte, err error) { +func RequestCertificate(client certificatesclient.CertificateSigningRequestInterface, csrData []byte, name string, usages []certificates.KeyUsage, privateKey interface{}) (req *certificates.CertificateSigningRequest, err error) { csr := &certificates.CertificateSigningRequest{ // Username, UID, Groups will be injected by API server. TypeMeta: metav1.TypeMeta{Kind: "CertificateSigningRequest"}, @@ -94,27 +98,31 @@ func RequestCertificate(client certificatesclient.CertificateSigningRequestInter csr.GenerateName = "csr-" } - req, err := client.Create(csr) + req, err = client.Create(csr) switch { case err == nil: case errors.IsAlreadyExists(err) && len(name) > 0: glog.Infof("csr for this node already exists, reusing") req, err = client.Get(name, metav1.GetOptions{}) if err != nil { - return nil, fmt.Errorf("cannot retrieve certificate signing request: %v", err) + return nil, formatError("cannot retrieve certificate signing request: %v", err) } if err := ensureCompatible(req, csr, privateKey); err != nil { return nil, fmt.Errorf("retrieved csr is not compatible: %v", err) } glog.Infof("csr for this node is still valid") default: - return nil, fmt.Errorf("cannot create certificate signing request: %v", err) + return nil, formatError("cannot create certificate signing request: %v", err) } + return req, nil +} +// WaitForCertificate waits for a certificate to be issued until timeout, or returns an error. +func WaitForCertificate(client certificatesclient.CertificateSigningRequestInterface, req *certificates.CertificateSigningRequest, timeout time.Duration) (certData []byte, err error) { fieldSelector := fields.OneTermEqualSelector("metadata.name", req.Name).String() event, err := cache.ListWatchUntil( - 3600*time.Second, + timeout, &cache.ListWatch{ ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { options.FieldSelector = fieldSelector @@ -129,7 +137,7 @@ func RequestCertificate(client certificatesclient.CertificateSigningRequestInter switch event.Type { case watch.Modified, watch.Added: case watch.Deleted: - return false, fmt.Errorf("csr %q was deleted", csr.Name) + return false, fmt.Errorf("csr %q was deleted", req.Name) default: return false, nil } @@ -152,7 +160,7 @@ func RequestCertificate(client certificatesclient.CertificateSigningRequestInter return nil, wait.ErrWaitTimeout } if err != nil { - return nil, fmt.Errorf("cannot watch on the certificate signing request: %v", err) + return nil, formatError("cannot watch on the certificate signing request: %v", err) } return event.Object.(*certificates.CertificateSigningRequest).Status.Certificate, nil @@ -225,3 +233,14 @@ func ensureCompatible(new, orig *certificates.CertificateSigningRequest, private } return nil } + +// formatError preserves the type of an API message but alters the message. Expects +// a single argument format string, and returns the wrapped error. +func formatError(format string, err error) error { + if s, ok := err.(errors.APIStatus); ok { + se := &errors.StatusError{ErrStatus: s.Status()} + se.ErrStatus.Message = fmt.Sprintf(format, se.ErrStatus.Message) + return se + } + return fmt.Errorf(format, err) +} diff --git a/staging/src/k8s.io/client-go/util/certificate/BUILD b/staging/src/k8s.io/client-go/util/certificate/BUILD index c0e32aaaf46..6903afd3bb2 100644 --- a/staging/src/k8s.io/client-go/util/certificate/BUILD +++ b/staging/src/k8s.io/client-go/util/certificate/BUILD @@ -19,7 +19,10 @@ go_test( tags = ["automanaged"], deps = [ "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", @@ -38,6 +41,8 @@ go_library( "//pkg/kubelet/util/csr:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go index 7eb4aaa74a0..22b14f363d3 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go @@ -30,12 +30,18 @@ import ( "github.com/golang/glog" certificates "k8s.io/api/certificates/v1beta1" + "k8s.io/apimachinery/pkg/api/errors" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" "k8s.io/client-go/util/cert" "k8s.io/kubernetes/pkg/kubelet/util/csr" ) +// certificateWaitBackoff controls the amount and timing of retries when the +// watch for certificate approval is interrupted. +var certificateWaitBackoff = wait.Backoff{Duration: 30 * time.Second, Steps: 4, Factor: 1.5, Jitter: 0.1} + // Manager maintains and updates the certificates in use by this certificate // manager. In the background it communicates with the API server to get new // certificates for certificates about to expire. @@ -49,6 +55,12 @@ type Manager interface { // certificate manager, as well as the associated certificate and key data // in PEM format. Current() *tls.Certificate + // ServerHealthy returns true if the manager is able to communicate with + // the server. This allows a caller to determine whether the cert manager + // thinks it can potentially talk to the API server. The cert manager may + // be very conservative and only return true if recent communication has + // occurred with the server. + ServerHealthy() bool } // Config is the set of configuration parameters available for a new Manager. @@ -132,6 +144,7 @@ type manager struct { rotationDeadline time.Time forceRotation bool certificateExpiration Gauge + serverHealth bool } // NewManager returns a new certificate manager. A certificate manager is @@ -169,6 +182,14 @@ func (m *manager) Current() *tls.Certificate { return m.cert } +// ServerHealthy returns true if the cert manager believes the server +// is currently alive. +func (m *manager) ServerHealthy() bool { + m.certAccessLock.RLock() + defer m.certAccessLock.RUnlock() + return m.serverHealth +} + // SetCertificateSigningRequestClient sets the client interface that is used // for signing new certificates generated as part of rotation. It must be // called before Start() and can not be used to change the @@ -203,9 +224,8 @@ func (m *manager) Start() { // doesn't have a certificate at all yet. if m.shouldRotate() { glog.V(1).Infof("shouldRotate() is true, forcing immediate rotation") - _, err := m.rotateCerts() - if err != nil { - glog.Errorf("Could not rotate certificates: %v", err) + if _, err := m.rotateCerts(); err != nil { + utilruntime.HandleError(fmt.Errorf("Could not rotate certificates: %v", err)) } } backoff := wait.Backoff{ @@ -219,7 +239,7 @@ func (m *manager) Start() { glog.V(2).Infof("Waiting %v for next certificate rotation", sleepInterval) time.Sleep(sleepInterval) if err := wait.ExponentialBackoff(backoff, m.rotateCerts); err != nil { - glog.Errorf("Reached backoff limit, still unable to rotate certs: %v", err) + utilruntime.HandleError(fmt.Errorf("Reached backoff limit, still unable to rotate certs: %v", err)) wait.PollInfinite(128*time.Second, m.rotateCerts) } }, 0) @@ -273,26 +293,58 @@ func (m *manager) shouldRotate() bool { return time.Now().After(m.rotationDeadline) } +// rotateCerts attempts to request a client cert from the server, wait a reasonable +// period of time for it to be signed, and then update the cert on disk. If it cannot +// retrieve a cert, it will return false. It will only return error in exceptional cases. +// This method also keeps track of "server health" by interpreting the responses it gets +// from the server on the various calls it makes. func (m *manager) rotateCerts() (bool, error) { glog.V(2).Infof("Rotating certificates") csrPEM, keyPEM, privateKey, err := m.generateCSR() if err != nil { - glog.Errorf("Unable to generate a certificate signing request: %v", err) + utilruntime.HandleError(fmt.Errorf("Unable to generate a certificate signing request: %v", err)) return false, nil } // Call the Certificate Signing Request API to get a certificate for the // new private key. - crtPEM, err := csr.RequestCertificate(m.certSigningRequestClient, csrPEM, "", m.usages, privateKey) + req, err := csr.RequestCertificate(m.certSigningRequestClient, csrPEM, "", m.usages, privateKey) if err != nil { - glog.Errorf("Failed while requesting a signed certificate from the master: %v", err) + utilruntime.HandleError(fmt.Errorf("Failed while requesting a signed certificate from the master: %v", err)) + return false, m.updateServerError(err) + } + + // Wait for the certificate to be signed. Instead of one long watch, we retry with slighly longer + // intervals each time in order to tolerate failures from the server AND to preserve the liveliness + // of the cert manager loop. This creates slightly more traffic against the API server in return + // for bounding the amount of time we wait when a certificate expires. + var crtPEM []byte + watchDuration := time.Minute + if err := wait.ExponentialBackoff(certificateWaitBackoff, func() (bool, error) { + data, err := csr.WaitForCertificate(m.certSigningRequestClient, req, watchDuration) + switch { + case err == nil: + crtPEM = data + return true, nil + case err == wait.ErrWaitTimeout: + watchDuration += time.Minute + if watchDuration > 5*time.Minute { + watchDuration = 5 * time.Minute + } + return false, nil + default: + utilruntime.HandleError(fmt.Errorf("Unable to check certificate signing status: %v", err)) + return false, m.updateServerError(err) + } + }); err != nil { + utilruntime.HandleError(fmt.Errorf("Certificate request was not signed: %v", err)) return false, nil } cert, err := m.certStore.Update(crtPEM, keyPEM) if err != nil { - glog.Errorf("Unable to store the new cert/key pair: %v", err) + utilruntime.HandleError(fmt.Errorf("Unable to store the new cert/key pair: %v", err)) return false, nil } @@ -335,12 +387,38 @@ var jitteryDuration = func(totalDuration float64) time.Duration { return wait.Jitter(time.Duration(totalDuration), 0.2) - time.Duration(totalDuration*0.3) } +// updateCached sets the most recent retrieved cert. It also sets the server +// as assumed healthy. func (m *manager) updateCached(cert *tls.Certificate) { m.certAccessLock.Lock() defer m.certAccessLock.Unlock() + m.serverHealth = true m.cert = cert } +// updateServerError takes an error returned by the server and infers +// the health of the server based on the error. It will return nil if +// the error does not require immediate termination of any wait loops, +// and otherwise it will return the error. +func (m *manager) updateServerError(err error) error { + m.certAccessLock.Lock() + defer m.certAccessLock.Unlock() + switch { + case errors.IsUnauthorized(err): + // SSL terminating proxies may report this error instead of the master + m.serverHealth = true + case errors.IsUnexpectedServerError(err): + // generally indicates a proxy or other load balancer problem, rather than a problem coming + // from the master + m.serverHealth = false + default: + // Identify known errors that could be expected for a cert request that + // indicate everything is working normally + m.serverHealth = errors.IsNotFound(err) || errors.IsForbidden(err) + } + return nil +} + func (m *manager) generateCSR() (csrPEM []byte, keyPEM []byte, key interface{}, err error) { // Generate a new private key. privateKey, err := ecdsa.GenerateKey(elliptic.P256(), cryptorand.Reader) diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go b/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go index 3df131e7d2b..ab19e37b12d 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go @@ -27,7 +27,10 @@ import ( "time" certificates "k8s.io/api/certificates/v1beta1" + "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" watch "k8s.io/apimachinery/pkg/watch" certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" ) @@ -288,6 +291,7 @@ func TestRotateCertWaitingForResultError(t *testing.T) { }, } + certificateWaitBackoff = wait.Backoff{Steps: 1} if success, err := m.rotateCerts(); success { t.Errorf("Got success from 'rotateCerts', wanted failure.") } else if err != nil { @@ -612,11 +616,170 @@ func TestInitializeOtherRESTClients(t *testing.T) { } else { m.setRotationDeadline() if m.shouldRotate() { - if success, err := certificateManager.(*manager).rotateCerts(); !success { - t.Errorf("Got failure from 'rotateCerts', expected success") - } else if err != nil { + success, err := certificateManager.(*manager).rotateCerts() + if err != nil { t.Errorf("Got error %v, expected none.", err) + return } + if !success { + t.Errorf("Unexpected response 'rotateCerts': %t", success) + return + } + } + } + + certificate = certificateManager.Current() + if !certificatesEqual(certificate, tc.expectedCertAfterStart.certificate) { + t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertAfterStart.certificate)) + } + }) + } +} + +func TestServerHealth(t *testing.T) { + type certs struct { + storeCert *certificateData + bootstrapCert *certificateData + apiCert *certificateData + expectedCertBeforeStart *certificateData + expectedCertAfterStart *certificateData + } + + updatedCerts := certs{ + storeCert: storeCertData, + bootstrapCert: bootstrapCertData, + apiCert: apiServerCertData, + expectedCertBeforeStart: storeCertData, + expectedCertAfterStart: apiServerCertData, + } + + currentCerts := certs{ + storeCert: storeCertData, + bootstrapCert: bootstrapCertData, + apiCert: apiServerCertData, + expectedCertBeforeStart: storeCertData, + expectedCertAfterStart: storeCertData, + } + + testCases := []struct { + description string + certs + + failureType fakeClientFailureType + clientErr error + + expectRotateFail bool + expectHealthy bool + }{ + { + description: "Current certificate, bootstrap certificate", + certs: updatedCerts, + expectHealthy: true, + }, + { + description: "Generic error on create", + certs: currentCerts, + + failureType: createError, + expectRotateFail: true, + }, + { + description: "Unauthorized error on create", + certs: currentCerts, + + failureType: createError, + clientErr: errors.NewUnauthorized("unauthorized"), + expectRotateFail: true, + expectHealthy: true, + }, + { + description: "Generic unauthorized error on create", + certs: currentCerts, + + failureType: createError, + clientErr: errors.NewGenericServerResponse(401, "POST", schema.GroupResource{}, "", "", 0, true), + expectRotateFail: true, + expectHealthy: true, + }, + { + description: "Generic not found error on create", + certs: currentCerts, + + failureType: createError, + clientErr: errors.NewGenericServerResponse(404, "POST", schema.GroupResource{}, "", "", 0, true), + expectRotateFail: true, + expectHealthy: false, + }, + { + description: "Not found error on create", + certs: currentCerts, + + failureType: createError, + clientErr: errors.NewGenericServerResponse(404, "POST", schema.GroupResource{}, "", "", 0, false), + expectRotateFail: true, + expectHealthy: true, + }, + { + description: "Conflict error on watch", + certs: currentCerts, + + failureType: watchError, + clientErr: errors.NewGenericServerResponse(409, "POST", schema.GroupResource{}, "", "", 0, false), + expectRotateFail: true, + expectHealthy: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + certificateStore := &fakeStore{ + cert: tc.storeCert.certificate, + } + + certificateManager, err := NewManager(&Config{ + Template: &x509.CertificateRequest{ + Subject: pkix.Name{ + Organization: []string{"system:nodes"}, + CommonName: "system:node:fake-node-name", + }, + }, + Usages: []certificates.KeyUsage{ + certificates.UsageDigitalSignature, + certificates.UsageKeyEncipherment, + certificates.UsageClientAuth, + }, + CertificateStore: certificateStore, + BootstrapCertificatePEM: tc.bootstrapCert.certificatePEM, + BootstrapKeyPEM: tc.bootstrapCert.keyPEM, + CertificateSigningRequestClient: &fakeClient{ + certificatePEM: tc.apiCert.certificatePEM, + failureType: tc.failureType, + err: tc.clientErr, + }, + }) + if err != nil { + t.Errorf("Got %v, wanted no error.", err) + } + + certificate := certificateManager.Current() + if !certificatesEqual(certificate, tc.expectedCertBeforeStart.certificate) { + t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertBeforeStart.certificate)) + } + + if _, ok := certificateManager.(*manager); !ok { + t.Errorf("Expected a '*manager' from 'NewManager'") + } else { + success, err := certificateManager.(*manager).rotateCerts() + if err != nil { + t.Errorf("Got error %v, expected none.", err) + return + } + if !success != tc.expectRotateFail { + t.Errorf("Unexpected response 'rotateCerts': %t", success) + return + } + if actual := certificateManager.(*manager).ServerHealthy(); actual != tc.expectHealthy { + t.Errorf("Unexpected manager server health: %t", actual) } } @@ -641,10 +804,14 @@ type fakeClient struct { certificatesclient.CertificateSigningRequestInterface failureType fakeClientFailureType certificatePEM []byte + err error } func (c fakeClient) List(opts v1.ListOptions) (*certificates.CertificateSigningRequestList, error) { if c.failureType == watchError { + if c.err != nil { + return nil, c.err + } return nil, fmt.Errorf("Watch error") } csrReply := certificates.CertificateSigningRequestList{ @@ -657,6 +824,9 @@ func (c fakeClient) List(opts v1.ListOptions) (*certificates.CertificateSigningR func (c fakeClient) Create(*certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) { if c.failureType == createError { + if c.err != nil { + return nil, c.err + } return nil, fmt.Errorf("Create error") } csrReply := certificates.CertificateSigningRequest{} @@ -666,6 +836,9 @@ func (c fakeClient) Create(*certificates.CertificateSigningRequest) (*certificat func (c fakeClient) Watch(opts v1.ListOptions) (watch.Interface, error) { if c.failureType == watchError { + if c.err != nil { + return nil, c.err + } return nil, fmt.Errorf("Watch error") } return &fakeWatch{ From 5649f9a578f4f130f61579d77d5609fbdaf82a1f Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 10 Oct 2017 20:15:03 -0400 Subject: [PATCH 9/9] Move pkg/kubelet/util/csr into client-go Everything else it depends on was already there, and now we have a somewhat consistent code chain. --- pkg/kubelet/certificate/bootstrap/BUILD | 2 +- .../certificate/bootstrap/bootstrap.go | 2 +- pkg/kubelet/util/BUILD | 1 - .../k8s.io/client-go/util/certificate/BUILD | 7 ++++-- .../util/certificate/certificate_manager.go | 2 +- .../client-go/util/certificate}/csr/BUILD | 5 ++-- .../client-go/util/certificate}/csr/csr.go | 25 +++++++++++++++---- .../util/certificate}/csr/csr_test.go | 0 8 files changed, 30 insertions(+), 14 deletions(-) rename {pkg/kubelet/util => staging/src/k8s.io/client-go/util/certificate}/csr/BUILD (91%) rename {pkg/kubelet/util => staging/src/k8s.io/client-go/util/certificate}/csr/csr.go (93%) rename {pkg/kubelet/util => staging/src/k8s.io/client-go/util/certificate}/csr/csr_test.go (100%) diff --git a/pkg/kubelet/certificate/bootstrap/BUILD b/pkg/kubelet/certificate/bootstrap/BUILD index 3d12e166a41..05d24e0c0f4 100644 --- a/pkg/kubelet/certificate/bootstrap/BUILD +++ b/pkg/kubelet/certificate/bootstrap/BUILD @@ -22,7 +22,6 @@ go_library( srcs = ["bootstrap.go"], importpath = "k8s.io/kubernetes/pkg/kubelet/certificate/bootstrap", deps = [ - "//pkg/kubelet/util/csr:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", @@ -32,6 +31,7 @@ go_library( "//vendor/k8s.io/client-go/tools/clientcmd/api:go_default_library", "//vendor/k8s.io/client-go/transport:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", + "//vendor/k8s.io/client-go/util/certificate/csr:go_default_library", ], ) diff --git a/pkg/kubelet/certificate/bootstrap/bootstrap.go b/pkg/kubelet/certificate/bootstrap/bootstrap.go index 2a954332d27..d123c0beee5 100644 --- a/pkg/kubelet/certificate/bootstrap/bootstrap.go +++ b/pkg/kubelet/certificate/bootstrap/bootstrap.go @@ -32,7 +32,7 @@ import ( clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/client-go/transport" certutil "k8s.io/client-go/util/cert" - "k8s.io/kubernetes/pkg/kubelet/util/csr" + "k8s.io/client-go/util/certificate/csr" ) const ( diff --git a/pkg/kubelet/util/BUILD b/pkg/kubelet/util/BUILD index a51fc6daea0..fdb78bddfba 100644 --- a/pkg/kubelet/util/BUILD +++ b/pkg/kubelet/util/BUILD @@ -53,7 +53,6 @@ filegroup( srcs = [ ":package-srcs", "//pkg/kubelet/util/cache:all-srcs", - "//pkg/kubelet/util/csr:all-srcs", "//pkg/kubelet/util/format:all-srcs", "//pkg/kubelet/util/ioutils:all-srcs", "//pkg/kubelet/util/queue:all-srcs", diff --git a/staging/src/k8s.io/client-go/util/certificate/BUILD b/staging/src/k8s.io/client-go/util/certificate/BUILD index 6903afd3bb2..f10a2d9e21e 100644 --- a/staging/src/k8s.io/client-go/util/certificate/BUILD +++ b/staging/src/k8s.io/client-go/util/certificate/BUILD @@ -38,7 +38,6 @@ go_library( importpath = "k8s.io/client-go/util/certificate", tags = ["automanaged"], deps = [ - "//pkg/kubelet/util/csr:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", @@ -46,6 +45,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//vendor/k8s.io/client-go/kubernetes/typed/certificates/v1beta1:go_default_library", "//vendor/k8s.io/client-go/util/cert:go_default_library", + "//vendor/k8s.io/client-go/util/certificate/csr:go_default_library", ], ) @@ -58,7 +58,10 @@ filegroup( filegroup( name = "all-srcs", - srcs = [":package-srcs"], + srcs = [ + ":package-srcs", + "//staging/src/k8s.io/client-go/util/certificate/csr:all-srcs", + ], tags = ["automanaged"], visibility = ["//visibility:public"], ) diff --git a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go index 22b14f363d3..e27966f5e1b 100644 --- a/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go +++ b/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go @@ -35,7 +35,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" "k8s.io/client-go/util/cert" - "k8s.io/kubernetes/pkg/kubelet/util/csr" + "k8s.io/client-go/util/certificate/csr" ) // certificateWaitBackoff controls the amount and timing of retries when the diff --git a/pkg/kubelet/util/csr/BUILD b/staging/src/k8s.io/client-go/util/certificate/csr/BUILD similarity index 91% rename from pkg/kubelet/util/csr/BUILD rename to staging/src/k8s.io/client-go/util/certificate/csr/BUILD index 78c435528b5..c6def5bbf0c 100644 --- a/pkg/kubelet/util/csr/BUILD +++ b/staging/src/k8s.io/client-go/util/certificate/csr/BUILD @@ -9,9 +9,8 @@ load( go_library( name = "go_default_library", srcs = ["csr.go"], - importpath = "k8s.io/kubernetes/pkg/kubelet/util/csr", + importpath = "k8s.io/client-go/util/certificate/csr", deps = [ - "//pkg/apis/certificates/v1beta1:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", @@ -43,7 +42,7 @@ filegroup( go_test( name = "go_default_test", srcs = ["csr_test.go"], - importpath = "k8s.io/kubernetes/pkg/kubelet/util/csr", + importpath = "k8s.io/client-go/util/certificate/csr", library = ":go_default_library", deps = [ "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", diff --git a/pkg/kubelet/util/csr/csr.go b/staging/src/k8s.io/client-go/util/certificate/csr/csr.go similarity index 93% rename from pkg/kubelet/util/csr/csr.go rename to staging/src/k8s.io/client-go/util/certificate/csr/csr.go index 53b67f8650e..22112a5b5b6 100644 --- a/pkg/kubelet/util/csr/csr.go +++ b/staging/src/k8s.io/client-go/util/certificate/csr/csr.go @@ -19,14 +19,15 @@ package csr import ( "crypto" "crypto/sha512" + "crypto/x509" "crypto/x509/pkix" "encoding/base64" + "encoding/pem" "fmt" + "github.com/golang/glog" "reflect" "time" - "github.com/golang/glog" - certificates "k8s.io/api/certificates/v1beta1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,7 +39,6 @@ import ( certificatesclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" "k8s.io/client-go/tools/cache" certutil "k8s.io/client-go/util/cert" - certhelper "k8s.io/kubernetes/pkg/apis/certificates/v1beta1" ) // RequestNodeCertificate will create a certificate signing request for a node @@ -200,11 +200,11 @@ func digestedName(privateKeyData []byte, subject *pkix.Name, usages []certificat // ensureCompatible ensures that a CSR object is compatible with an original CSR func ensureCompatible(new, orig *certificates.CertificateSigningRequest, privateKey interface{}) error { - newCsr, err := certhelper.ParseCSR(new) + newCsr, err := ParseCSR(new) if err != nil { return fmt.Errorf("unable to parse new csr: %v", err) } - origCsr, err := certhelper.ParseCSR(orig) + origCsr, err := ParseCSR(orig) if err != nil { return fmt.Errorf("unable to parse original csr: %v", err) } @@ -244,3 +244,18 @@ func formatError(format string, err error) error { } return fmt.Errorf(format, err) } + +// ParseCSR extracts the CSR from the API object and decodes it. +func ParseCSR(obj *certificates.CertificateSigningRequest) (*x509.CertificateRequest, error) { + // extract PEM from request object + pemBytes := obj.Spec.Request + block, _ := pem.Decode(pemBytes) + if block == nil || block.Type != "CERTIFICATE REQUEST" { + return nil, fmt.Errorf("PEM block type must be CERTIFICATE REQUEST") + } + csr, err := x509.ParseCertificateRequest(block.Bytes) + if err != nil { + return nil, err + } + return csr, nil +} diff --git a/pkg/kubelet/util/csr/csr_test.go b/staging/src/k8s.io/client-go/util/certificate/csr/csr_test.go similarity index 100% rename from pkg/kubelet/util/csr/csr_test.go rename to staging/src/k8s.io/client-go/util/certificate/csr/csr_test.go