From a14450aa9346782a75bc4d102379715b3810740c Mon Sep 17 00:00:00 2001 From: Yang Le Date: Wed, 26 Jun 2024 10:32:52 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=8C=B1=20make=20additional=20secret=20dat?= =?UTF-8?q?a=20always=20sensitive=20(#525)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yang Le --- .../clientcert/cert_controller.go | 15 +++++------- .../clientcert/controller_test.go | 23 ++++++++---------- .../spoke/addon/registration_controller.go | 11 ++++----- pkg/registration/spoke/spokeagent.go | 18 +++++++------- .../spokeagent_rebootstrap_test.go | 24 ++++++++++++++++--- 5 files changed, 50 insertions(+), 41 deletions(-) diff --git a/pkg/registration/clientcert/cert_controller.go b/pkg/registration/clientcert/cert_controller.go index f9421a0ac..666f6fea1 100644 --- a/pkg/registration/clientcert/cert_controller.go +++ b/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] diff --git a/pkg/registration/clientcert/controller_test.go b/pkg/registration/clientcert/controller_test.go index 7f5611814..abf3c9c34 100644 --- a/pkg/registration/clientcert/controller_test.go +++ b/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{ diff --git a/pkg/registration/spoke/addon/registration_controller.go b/pkg/registration/spoke/addon/registration_controller.go index 89e4ab0a0..be9bfc0e3 100644 --- a/pkg/registration/spoke/addon/registration_controller.go +++ b/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{ diff --git a/pkg/registration/spoke/spokeagent.go b/pkg/registration/spoke/spokeagent.go index d6d38ed1f..c776af03e 100644 --- a/pkg/registration/spoke/spokeagent.go +++ b/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 diff --git a/test/integration/registration/spokeagent_rebootstrap_test.go b/test/integration/registration/spokeagent_rebootstrap_test.go index 1437a0418..2232ba317 100644 --- a/test/integration/registration/spokeagent_rebootstrap_test.go +++ b/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")