🌱 make additional secret data always sensitive (#525)

Signed-off-by: Yang Le <yangle@redhat.com>

pkg/registration/clientcert/cert_controller.go

@@ -79,11 +79,9 @@ type ClientCertOption struct {
 	// SecretName is the name of the secret containing client certificate. The secret will be created if
 	// it does not exist.
 	SecretName string
-	// AdditonalSecretData contains data that will be added into client certificate secret besides tls.key/tls.crt
+	// AdditionalSecretData contains data that will be added into client certificate secret besides tls.key/tls.crt
+	// Once AdditionalSecretData changes, the client cert will be recreated.
 	AdditionalSecretData map[string][]byte
-	// AdditonalSecretDataSensitive is true indicates the client cert is sensitive to the AdditonalSecretData.
-	// That means once AdditonalSecretData changes, the client cert will be recreated.
-	AdditionalSecretDataSensitive bool
 }
 
 type StatusUpdateFunc func(ctx context.Context, cond metav1.Condition) error
@@ -293,7 +291,6 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory.
 		secret,
 		syncCtx.Recorder(),
 		c.Subject,
-		c.AdditionalSecretDataSensitive,
 		c.AdditionalSecretData)
 	if err != nil {
 		return err
@@ -375,14 +372,14 @@ func shouldCreateCSR(
 	secret *corev1.Secret,
 	recorder events.Recorder,
 	subject *pkix.Name,
-	additionalSecretDataSensitive bool,
 	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)
-	case additionalSecretDataSensitive && !hasAdditionalSecretData(additionalSecretData, secret):
-		recorder.Eventf("AdditonalSecretDataChanged",
+	// 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)
@@ -408,7 +405,7 @@ func shouldCreateCSR(
 	return true, nil
 }
 
-// hasAdditonalSecretData checks if the secret includes the expected additional secret data.
+// hasAdditionalSecretData checks if the secret includes the expected additional secret data.
 func hasAdditionalSecretData(additionalSecretData map[string][]byte, secret *corev1.Secret) bool {
 	for k, v := range additionalSecretData {
 		value, ok := secret.Data[k]

pkg/registration/clientcert/controller_test.go

@@ -39,15 +39,14 @@ func TestSync(t *testing.T) {
 	}
 
 	cases := []struct {
-		name                         string
-		queueKey                     string
-		secrets                      []runtime.Object
-		approvedCSRCert              *testinghelpers.TestCert
-		keyDataExpected              bool
-		csrNameExpected              bool
-		additonalSecretDataSensitive bool
-		expectedCondition            *metav1.Condition
-		validateActions              func(t *testing.T, hubActions, agentActions []clienttesting.Action)
+		name              string
+		queueKey          string
+		secrets           []runtime.Object
+		approvedCSRCert   *testinghelpers.TestCert
+		keyDataExpected   bool
+		csrNameExpected   bool
+		expectedCondition *metav1.Condition
+		validateActions   func(t *testing.T, hubActions, agentActions []clienttesting.Action)
 	}{
 		{
 			name:            "agent bootstrap",
@@ -139,9 +138,8 @@ func TestSync(t *testing.T) {
 					AgentNameFile:   []byte("invalid-name"),
 				}),
 			},
-			keyDataExpected:              true,
-			csrNameExpected:              true,
-			additonalSecretDataSensitive: true,
+			keyDataExpected: true,
+			csrNameExpected: true,
 			validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) {
 				testingcommon.AssertActions(t, hubActions, "create")
 				actual := hubActions[0].(clienttesting.CreateActionImpl).Object
@@ -183,7 +181,6 @@ func TestSync(t *testing.T) {
 					ClusterNameFile: []byte(testinghelpers.TestManagedClusterName),
 					AgentNameFile:   []byte(testAgentName),
 				},
-				AdditionalSecretDataSensitive: c.additonalSecretDataSensitive,
 			}
 			csrOption := CSROption{
 				ObjectMeta: metav1.ObjectMeta{

pkg/registration/spoke/addon/registration_controller.go

@@ -210,17 +210,16 @@ func (c *addOnRegistrationController) startRegistration(ctx context.Context, con
 	kubeInformerFactory := informers.NewSharedInformerFactoryWithOptions(
 		kubeClient, 10*time.Minute, informers.WithNamespace(config.InstallationNamespace))
 
-	additonalSecretData := map[string][]byte{}
+	additionalSecretData := map[string][]byte{}
 	if config.registration.SignerName == certificatesv1.KubeAPIServerClientSignerName {
-		additonalSecretData[clientcert.KubeconfigFile] = c.kubeconfigData
+		additionalSecretData[clientcert.KubeconfigFile] = c.kubeconfigData
 	}
 
 	// build and start a client cert controller
 	clientCertOption := clientcert.ClientCertOption{
-		SecretNamespace:               config.InstallationNamespace,
-		SecretName:                    config.secretName,
-		AdditionalSecretData:          additonalSecretData,
-		AdditionalSecretDataSensitive: true,
+		SecretNamespace:      config.InstallationNamespace,
+		SecretName:           config.secretName,
+		AdditionalSecretData: additionalSecretData,
 	}
 
 	csrOption := clientcert.CSROption{

pkg/registration/spoke/spokeagent.go

@@ -11,7 +11,6 @@ import (
 	"github.com/openshift/library-go/pkg/controller/controllercmd"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -244,12 +243,6 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context,
 	// in scenario #2 and #3, which results in an error message in log: 'Observed a panic: timeout waiting for
 	// informer cache'
 	if !ok {
-		// delete the hub kubeconfig secret if exits
-		if err := managementKubeClient.CoreV1().Secrets(o.agentOptions.ComponentNamespace).Delete(ctx,
-			o.registrationOption.HubKubeconfigSecret, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
-			return err
-		}
-
 		// create a ClientCertForHubController for spoke agent bootstrap
 		// the bootstrap informers are supposed to be terminated after completing the bootstrap process.
 		bootstrapInformerFactory := informers.NewSharedInformerFactory(bootstrapKubeClient, 10*time.Minute)
@@ -294,12 +287,17 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context,
 
 		go clientCertForHubController.Run(bootstrapCtx, 1)
 
-		// wait for the hub client config is ready.
-		logger.Info("Waiting for hub client config and managed cluster to be ready")
+		// wait until a valid hub kubeconfig is in place.
+		// If no hub kube kubeconfig exits, the controller will create the kubeconfig and the client cert; otherwise there
+		// exits an invalid hub kube kubeconfig, the controller will update both the kubeconfig and the referenced client
+		// cert with correct content.
+		// It's difficult to set a timeout for the waiting because a manual approval (approve the CSR) is required on the
+		// hub cluster to create a client cert with the bootstrap hub kubeconfig.
+		logger.Info("Waiting for hub client config for managed cluster to be ready")
 		if err := wait.PollUntilContextCancel(bootstrapCtx, 1*time.Second, true, o.HasValidHubClientConfig); err != nil {
 			// TODO need run the bootstrap CSR forever to re-establish the client-cert if it is ever lost.
 			stopBootstrap()
-			return fmt.Errorf("failed to wait for hub client config and managed cluster to be ready: %w", err)
+			return fmt.Errorf("failed to wait for hub client config for managed cluster to be ready: %w", err)
 		}
 
 		// stop the clientCertForHubController for bootstrap once the hub client config is ready

test/integration/registration/spokeagent_rebootstrap_test.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
+	certutil "k8s.io/client-go/util/cert"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
 	addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
@@ -518,10 +519,27 @@ var _ = ginkgo.Describe("Rebootstrap", func() {
 			ginkgo.By("stop the hub")
 			stopHub()
 
-			ginkgo.By("the hub kubeconfig secret should be deleted once client cert expired")
+			ginkgo.By("wait until the client cert expires")
 			gomega.Eventually(func() bool {
-				_, err := kubeClient.CoreV1().Secrets(testNamespace).Get(context.Background(), hubKubeconfigSecret, metav1.GetOptions{})
-				return apierrors.IsNotFound(err)
+				secret, err := kubeClient.CoreV1().Secrets(testNamespace).Get(context.Background(), hubKubeconfigSecret, metav1.GetOptions{})
+				if err != nil {
+					return false
+				}
+				data, ok := secret.Data[clientcert.TLSCertFile]
+				if !ok {
+					return false
+				}
+				certs, err := certutil.ParseCertsPEM(data)
+				if err != nil {
+					return false
+				}
+				now := time.Now()
+				for _, cert := range certs {
+					if now.After(cert.NotAfter) {
+						return true
+					}
+				}
+				return false
 			}, eventuallyTimeout, eventuallyInterval).Should(gomega.BeTrue())
 
 			ginkgo.By("start the hub again")