diff --git a/pkg/registration/clientcert/cert_controller.go b/pkg/registration/clientcert/cert_controller.go index 666f6fea1..22899e0eb 100644 --- a/pkg/registration/clientcert/cert_controller.go +++ b/pkg/registration/clientcert/cert_controller.go @@ -373,51 +373,60 @@ func shouldCreateCSR( recorder events.Recorder, subject *pkix.Name, additionalSecretData map[string][]byte) (bool, error) { - switch { - case !hasValidClientCertificate(logger, subject, secret): - recorder.Eventf("NoValidCertificateFound", - "No valid client certificate for %s is found. Bootstrap is required", controllerName) - // return true to create a CSR for a new client cert once the additional secret data changes. - case !hasAdditionalSecretData(additionalSecretData, secret): - recorder.Eventf("AdditionalSecretDataChanged", - "The additional secret data is changed. Re-create the client certificate for %s", controllerName) - default: - notBefore, notAfter, err := getCertValidityPeriod(secret) - if err != nil { - return false, err - } + // create a csr to request new client certificate if + // a.there is no valid client certificate issued for the current cluster/agent + valid, err := IsCertificateValid(logger, secret.Data[TLSCertFile], subject) + if err != nil { + recorder.Eventf("CertificateValidationFailed", "Failed to validate client certificate for %s: %v", controllerName, err) + return true, nil + } + if !valid { + recorder.Eventf("NoValidCertificateFound", "No valid client certificate for %s is found. Bootstrap is required", controllerName) + return true, nil + } - total := notAfter.Sub(*notBefore) - remaining := time.Until(*notAfter) + // b.client certificate is sensitive to the additional secret data and the data changes + if err := hasAdditionalSecretData(additionalSecretData, secret); err != nil { + recorder.Eventf("AdditonalSecretDataChanged", "The additional secret data is changed for %v. Re-create the client certificate for %s", err, controllerName) + return true, nil + } + + // c.client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining + notBefore, notAfter, err := getCertValidityPeriod(secret) + if err != nil { + return false, err + } + total := notAfter.Sub(*notBefore) + remaining := time.Until(*notAfter) + logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, + "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) + threshold := jitter(0.2, 0.25) + if remaining.Seconds()/total.Seconds() > threshold { + // Do nothing if the client certificate is valid and has more than a random percentage range from 20% to 25% of its life remaining logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) - threshold := jitter(0.2, 0.25) - if remaining.Seconds()/total.Seconds() > threshold { - // Do nothing if the client certificate is valid and has more than a random percentage range from 20% to 25% of its life remaining - logger.V(4).Info("Client certificate for:", "name", controllerName, "time total", total, - "remaining", remaining, "remaining/total", remaining.Seconds()/total.Seconds()) - return false, nil - } - recorder.Eventf("CertificateRotationStarted", - "The current client certificate for %s expires in %v. Start certificate rotation", - controllerName, remaining.Round(time.Second)) + return false, nil } + recorder.Eventf("CertificateRotationStarted", + "The current client certificate for %s expires in %v. Start certificate rotation", + controllerName, remaining.Round(time.Second)) return true, nil } -// hasAdditionalSecretData checks if the secret includes the expected additional secret data. -func hasAdditionalSecretData(additionalSecretData map[string][]byte, secret *corev1.Secret) bool { +// hasAdditonalSecretData checks if the secret includes the expected additional secret data. +func hasAdditionalSecretData(additionalSecretData map[string][]byte, secret *corev1.Secret) error { for k, v := range additionalSecretData { value, ok := secret.Data[k] if !ok { - return false + return fmt.Errorf("key %q not found in secret %q", k, secret.Namespace+"/"+secret.Name) } if !reflect.DeepEqual(v, value) { - return false + return fmt.Errorf("key %q in secret %q does not match the expected value", + k, secret.Namespace+"/"+secret.Name) } } - return true + return nil } func jitter(percentage float64, maxFactor float64) float64 { @@ -427,10 +436,3 @@ func jitter(percentage float64, maxFactor float64) float64 { newPercentage := percentage + percentage*rand.Float64()*maxFactor //#nosec G404 return newPercentage } - -func hasValidClientCertificate(logger klog.Logger, subject *pkix.Name, secret *corev1.Secret) bool { - if valid, err := IsCertificateValid(logger, secret.Data[TLSCertFile], subject); err == nil { - return valid - } - return false -} diff --git a/pkg/registration/clientcert/certificate.go b/pkg/registration/clientcert/certificate.go index dfaa0aa95..7c69a75f4 100644 --- a/pkg/registration/clientcert/certificate.go +++ b/pkg/registration/clientcert/certificate.go @@ -30,43 +30,6 @@ import ( "open-cluster-management.io/ocm/pkg/registration/helpers" ) -// HasValidHubKubeconfig checks if there exists a valid client certificate in the given secret -// Returns true if all the conditions below are met: -// 1. KubeconfigFile exists when hasKubeconfig is true -// 2. TLSKeyFile exists -// 3. TLSCertFile exists and the certificate is not expired -// 4. If subject is specified, it matches the subject in the certificate stored in TLSCertFile -func HasValidHubKubeconfig(logger klog.Logger, secret *corev1.Secret, subject *pkix.Name) bool { - if len(secret.Data) == 0 { - logger.V(4).Info("No data found in secret", "secret", klog.KObj(secret)) - return false - } - - if _, ok := secret.Data[KubeconfigFile]; !ok { - logger.V(4).Info("No specific file found in secret", "file", KubeconfigFile, "secret", klog.KObj(secret)) - return false - } - - if _, ok := secret.Data[TLSKeyFile]; !ok { - logger.V(4).Info("No specific key file found in secret", "keyFile", TLSKeyFile, "secret", klog.KObj(secret)) - return false - } - - certData, ok := secret.Data[TLSCertFile] - if !ok { - logger.V(4).Info("No specific cert file found in secret", "certFile", TLSCertFile, "secret", klog.KObj(secret)) - return false - } - - valid, err := IsCertificateValid(logger, certData, subject) - if err != nil { - logger.V(4).Error(err, "Unable to validate certificate in secret", "secret", klog.KObj(secret)) - return false - } - - return valid -} - // IsCertificateValid return true if // 1) All certs in client certificate are not expired. // 2) At least one cert matches the given subject if specified diff --git a/pkg/registration/clientcert/certificate_test.go b/pkg/registration/clientcert/certificate_test.go index 700449612..9ea31ecad 100644 --- a/pkg/registration/clientcert/certificate_test.go +++ b/pkg/registration/clientcert/certificate_test.go @@ -61,80 +61,6 @@ func TestIsCSRApproved(t *testing.T) { } } -func TestHasValidHubKubeconfig(t *testing.T) { - cases := []struct { - name string - secret *corev1.Secret - subject *pkix.Name - isValid bool - }{ - { - name: "no data", - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", nil, nil), - }, - { - name: "no kubeconfig", - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", nil, map[string][]byte{}), - }, - { - name: "no key", - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", nil, map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - { - name: "no cert", - secret: testinghelpers.NewHubKubeconfigSecret( - testNamespace, testSecretName, "", &testinghelpers.TestCert{Key: []byte("key")}, map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - { - name: "bad cert", - secret: testinghelpers.NewHubKubeconfigSecret( - testNamespace, testSecretName, "", &testinghelpers.TestCert{Key: []byte("key"), Cert: []byte("bad cert")}, map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - { - name: "expired cert", - secret: testinghelpers.NewHubKubeconfigSecret( - testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", -60*time.Second), map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - }, - { - name: "invalid common name", - secret: testinghelpers.NewHubKubeconfigSecret( - testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - subject: &pkix.Name{ - CommonName: "wrong-common-name", - }, - }, - { - name: "valid kubeconfig", - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ - KubeconfigFile: testinghelpers.NewKubeconfig("c1", "https://127.0.0.1:6001", "", nil, nil, nil), - }), - subject: &pkix.Name{ - CommonName: "test", - }, - isValid: true, - }, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - logger, _ := ktesting.NewTestContext(t) - isValid := HasValidHubKubeconfig(logger, c.secret, c.subject) - if isValid != c.isValid { - t.Errorf("expected %t, but got %t", c.isValid, isValid) - } - }) - } -} - func TestIsCertificateValid(t *testing.T) { cases := []struct { name string