Refactor client certificates. (#522)

Signed-off-by: xuezhaojun <zxue@redhat.com>

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
-}

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

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