From 5b80d01b7269e0d8767ae5098ad47459a23099c8 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Mon, 29 Mar 2021 09:59:48 +0800 Subject: [PATCH] Refactor and make client cert controller reusable Signed-off-by: Yang Le --- pkg/clientcert/cert_controller.go | 352 ++++++++++++++++++ .../certificate.go | 112 +++--- .../certificate_test.go | 156 +++++--- .../controller_test.go | 69 ++-- pkg/helpers/testing/testinghelpers.go | 12 +- pkg/spoke/hubclientcert/controller.go | 345 ----------------- pkg/spoke/managedcluster/registration.go | 112 ++++++ pkg/spoke/managedcluster/registration_test.go | 48 +++ .../secret_controller.go | 2 +- .../secret_controller_test.go | 46 ++- pkg/spoke/spokeagent.go | 51 ++- pkg/spoke/spokeagent_test.go | 6 +- test/integration/integration_suite_test.go | 4 +- 13 files changed, 774 insertions(+), 541 deletions(-) create mode 100644 pkg/clientcert/cert_controller.go rename pkg/{spoke/hubclientcert => clientcert}/certificate.go (60%) rename pkg/{spoke/hubclientcert => clientcert}/certificate_test.go (65%) rename pkg/{spoke/hubclientcert => clientcert}/controller_test.go (81%) delete mode 100644 pkg/spoke/hubclientcert/controller.go create mode 100644 pkg/spoke/managedcluster/registration.go create mode 100644 pkg/spoke/managedcluster/registration_test.go rename pkg/spoke/{hubclientcert => managedcluster}/secret_controller.go (99%) rename pkg/spoke/{hubclientcert => managedcluster}/secret_controller_test.go (75%) diff --git a/pkg/clientcert/cert_controller.go b/pkg/clientcert/cert_controller.go new file mode 100644 index 000000000..edaf88a19 --- /dev/null +++ b/pkg/clientcert/cert_controller.go @@ -0,0 +1,352 @@ +package clientcert + +import ( + "context" + "crypto/tls" + "crypto/x509/pkix" + "fmt" + "math/rand" + "reflect" + "time" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + + certificates "k8s.io/api/certificates/v1beta1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1" + corev1informers "k8s.io/client-go/informers/core/v1" + csrclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + certificateslisters "k8s.io/client-go/listers/certificates/v1beta1" + cache "k8s.io/client-go/tools/cache" + certutil "k8s.io/client-go/util/cert" + "k8s.io/client-go/util/keyutil" + "k8s.io/klog/v2" +) + +const ( + // KubeconfigFile is the name of the kubeconfig file in kubeconfigSecret + KubeconfigFile = "kubeconfig" + // TLSKeyFile is the name of tls key file in kubeconfigSecret + TLSKeyFile = "tls.key" + // TLSCertFile is the name of the tls cert file in kubeconfigSecret + TLSCertFile = "tls.crt" + + clusterNameAnnotation = "open-cluster-management.io/cluster-name" + ClusterNameFile = "cluster-name" + AgentNameFile = "agent-name" + + ClusterNameLabel = "open-cluster-management.io/cluster-name" + AddonNameLabel = "open-cluster-management.io/addon-name" + SignerNameLabel = "open-cluster-management.io/signer-name" +) + +// ControllerResyncInterval is exposed so that integration tests can crank up the constroller sync speed. +var ControllerResyncInterval = 5 * time.Minute + +// CSROption includes options that is used to create and monitor csrs +type CSROption struct { + // ObjectMeta is the ObjectMeta shared by all created csrs. It should use GenerateName instead of Name + // to generate random csr names + ObjectMeta metav1.ObjectMeta + // Subject represents the subject of the client certificate used to create csrs + Subject *pkix.Name + // SignerName is the name of the signer specified in the created csrs + SignerName string + + // EventFilterFunc matches csrs created with above options + EventFilterFunc factory.EventFilterFunc +} + +// ClientCertOption includes options that is used to create client certificate +type ClientCertOption struct { + // SecretNamespace is the namespace of the secret containing client certificate. + SecretNamespace string + // 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 + AdditonalSecretData map[string][]byte +} + +// clientCertificateController implements the common logic of hub client certification creation/rotation. It +// creates a client certificate and rotates it before it becomes expired by using csrs. The client +// certificate generated is stored in a specific secret with the keys below: +// 1). tls.key: tls key file +// 2). tls.crt: tls cert file +type clientCertificateController struct { + ClientCertOption + CSROption + + hubCSRLister certificateslisters.CertificateSigningRequestLister + hubCSRClient csrclient.CertificateSigningRequestInterface + spokeCoreClient corev1client.CoreV1Interface + controllerName string + + // csrName is the name of csr created by controller and waiting for approval. + csrName string + + // keyData is the private key data used to created a csr + // csrName and keyData store the internal state of the controller. They are set after controller creates a new csr + // and cleared once the csr is approved and processed by controller. There are 4 combination of their values: + // 1. csrName empty, keyData empty: means we aren't trying to create a new client cert, our current one is valid + // 2. csrName set, keyData empty: there was bug + // 3. csrName set, keyData set: we are waiting for a new cert to be signed. + // 4. csrName empty, keydata set: the CSR failed to create, this shouldn't happen, it's a bug. + keyData []byte +} + +// NewClientCertificateController return an instance of clientCertificateController +func NewClientCertificateController( + clientCertOption ClientCertOption, + csrOption CSROption, + hubCSRInformer certificatesinformers.CertificateSigningRequestInformer, + hubCSRClient csrclient.CertificateSigningRequestInterface, + spokeSecretInformer corev1informers.SecretInformer, + spokeCoreClient corev1client.CoreV1Interface, + recorder events.Recorder, + controllerName string, +) factory.Controller { + c := clientCertificateController{ + ClientCertOption: clientCertOption, + CSROption: csrOption, + hubCSRLister: hubCSRInformer.Lister(), + hubCSRClient: hubCSRClient, + spokeCoreClient: spokeCoreClient, + controllerName: controllerName, + } + + return factory.New(). + WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { + key, _ := cache.MetaNamespaceKeyFunc(obj) + return key + }, func(obj interface{}) bool { + accessor, err := meta.Accessor(obj) + if err != nil { + return false + } + // only enqueue a specific secret + if accessor.GetNamespace() == c.SecretNamespace && accessor.GetName() == c.SecretName { + return true + } + return false + }, spokeSecretInformer.Informer()). + WithFilteredEventsInformersQueueKeyFunc(func(obj runtime.Object) string { + accessor, _ := meta.Accessor(obj) + return accessor.GetName() + }, c.EventFilterFunc, hubCSRInformer.Informer()). + WithSync(c.sync). + ResyncEvery(ControllerResyncInterval). + ToController(controllerName, recorder) +} + +func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + // get secret containing client certificate + secret, err := c.spokeCoreClient.Secrets(c.SecretNamespace).Get(ctx, c.SecretName, metav1.GetOptions{}) + switch { + case errors.IsNotFound(err): + secret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: c.SecretNamespace, + Name: c.SecretName, + }, + } + case err != nil: + return fmt.Errorf("unable to get secret %q: %w", c.SecretNamespace+"/"+c.SecretName, err) + } + + // reconcile pending csr if exists + if c.csrName != "" { + newSecretConfig, err := c.syncCSR(secret) + if err != nil { + c.reset() + return err + } + if len(newSecretConfig) == 0 { + return nil + } + // append additional data into client certificate secret + for k, v := range c.AdditonalSecretData { + newSecretConfig[k] = v + } + secret.Data = newSecretConfig + // save the changes into secret + if err := c.saveSecret(secret); err != nil { + return err + } + syncCtx.Recorder().Eventf("ClientCertificateCreated", "A new client certificate for %s is available", c.controllerName) + return nil + } + + // add additional data into client certificate secret + newSecretConfig := map[string][]byte{} + for k, v := range secret.Data { + newSecretConfig[k] = v + } + for k, v := range c.AdditonalSecretData { + newSecretConfig[k] = v + } + if !reflect.DeepEqual(newSecretConfig, secret.Data) { + secret.Data = newSecretConfig + if err := c.saveSecret(secret); err != nil { + return err + } + } + + // create a csr to request new client certificate if + // a. there is no valid client certificate issued for the current cluster/agent + // b. client certificate exists and has less than a random percentage range from 20% to 25% of its life remaining + if c.hasValidClientCertificate(secret) { + notBefore, notAfter, err := getCertValidityPeriod(secret) + if err != nil { + return err + } + + total := notAfter.Sub(*notBefore) + remaining := notAfter.Sub(time.Now()) + klog.V(4).Infof("Client certificate for %s: time total=%v, remaining=%v, remaining/total=%v", c.controllerName, total, remaining, 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 + klog.V(4).Infof("Client certificate for %s is valid and has more than %.2f%% of its life remaining", c.controllerName, threshold*100) + return nil + } + syncCtx.Recorder().Eventf("CertificateRotationStarted", "The current client certificate for %s expires in %v. Start certificate rotation", c.controllerName, remaining.Round(time.Second)) + } else { + syncCtx.Recorder().Eventf("NoValidCertificateFound", "No valid client certificate for %s is found. Bootstrap is required", c.controllerName) + } + + // create a new private key + c.keyData, err = keyutil.MakeEllipticPrivateKeyPEM() + if err != nil { + return err + } + + // create a csr + c.csrName, err = c.createCSR(ctx) + if err != nil { + c.reset() + return err + } + syncCtx.Recorder().Eventf("CSRCreated", "A csr %q is created for %s", c.csrName, c.controllerName) + return nil +} + +func (c *clientCertificateController) syncCSR(secret *corev1.Secret) (map[string][]byte, error) { + // skip if there is no ongoing csr + if c.csrName == "" { + c.reset() + return nil, nil + } + + // skip if csr no longer exists + csr, err := c.hubCSRLister.Get(c.csrName) + switch { + case errors.IsNotFound(err): + // fallback to fetching csr from hub apiserver in case it is not cached by informer yet + csr, err = c.hubCSRClient.Get(context.Background(), c.csrName, metav1.GetOptions{}) + if errors.IsNotFound(err) { + klog.V(4).Infof("Unable to get csr %q. It might have already been deleted.", c.csrName) + c.reset() + return nil, nil + } + case err != nil: + return nil, err + } + + // skip if csr is not approved yet + if !isCSRApproved(csr) { + return nil, nil + } + + // skip if csr has no certificate in its status yet + if csr.Status.Certificate == nil { + return nil, nil + } + + klog.V(4).Infof("Sync csr %v", c.csrName) + // check if cert in csr status matches with the corresponding private key + if c.keyData == nil { + c.reset() + return nil, fmt.Errorf("No private key found for certificate in csr: %s", c.csrName) + } + _, err = tls.X509KeyPair(csr.Status.Certificate, c.keyData) + if err != nil { + c.reset() + return nil, fmt.Errorf("Private key does not match with the certificate in csr: %s", c.csrName) + } + + data := map[string][]byte{ + TLSCertFile: csr.Status.Certificate, + TLSKeyFile: c.keyData, + } + + // clear the csr name and private key + c.reset() + return data, nil +} + +func (c *clientCertificateController) createCSR(ctx context.Context) (string, error) { + privateKey, err := keyutil.ParsePrivateKeyPEM(c.keyData) + if err != nil { + return "", fmt.Errorf("invalid private key for certificate request: %w", err) + } + csrData, err := certutil.MakeCSR(privateKey, c.Subject, nil, nil) + if err != nil { + return "", fmt.Errorf("unable to generate certificate request: %w", err) + } + + csr := &certificates.CertificateSigningRequest{ + ObjectMeta: c.ObjectMeta, + Spec: certificates.CertificateSigningRequestSpec{ + Request: csrData, + Usages: []certificates.KeyUsage{ + certificates.UsageDigitalSignature, + certificates.UsageKeyEncipherment, + certificates.UsageClientAuth, + }, + SignerName: &c.SignerName, + }, + } + + req, err := c.hubCSRClient.Create(ctx, csr, metav1.CreateOptions{}) + if err != nil { + return "", err + } + return req.Name, nil +} + +func (c *clientCertificateController) saveSecret(secret *corev1.Secret) error { + var err error + if secret.ResourceVersion == "" { + _, err = c.spokeCoreClient.Secrets(c.SecretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) + return err + } + _, err = c.spokeCoreClient.Secrets(c.SecretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) + return err +} + +func (c *clientCertificateController) reset() { + c.csrName = "" + c.keyData = nil +} + +func (c *clientCertificateController) hasValidClientCertificate(secret *corev1.Secret) bool { + if valid, err := IsCertificateValid(secret.Data[TLSCertFile], c.Subject); err == nil { + return valid + } + return false +} + +func jitter(percentage float64, maxFactor float64) float64 { + if maxFactor <= 0.0 { + maxFactor = 1.0 + } + newPercentage := percentage + percentage*rand.Float64()*maxFactor + return newPercentage +} diff --git a/pkg/spoke/hubclientcert/certificate.go b/pkg/clientcert/certificate.go similarity index 60% rename from pkg/spoke/hubclientcert/certificate.go rename to pkg/clientcert/certificate.go index bc666bbfb..189e832fb 100644 --- a/pkg/spoke/hubclientcert/certificate.go +++ b/pkg/clientcert/certificate.go @@ -1,31 +1,30 @@ -package hubclientcert +package clientcert import ( + "crypto/x509/pkix" "errors" "fmt" + "reflect" "strings" "time" - certificates "k8s.io/api/certificates/v1beta1" + certificatesv1beta1 "k8s.io/api/certificates/v1beta1" corev1 "k8s.io/api/core/v1" restclient "k8s.io/client-go/rest" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" certutil "k8s.io/client-go/util/cert" "k8s.io/klog/v2" - - "github.com/open-cluster-management/registration/pkg/hub/user" ) -// HasValidKubeconfig checks if there exists a valid kubeconfig in the given secret -// Returns true if the conditions below are met: -// 1. KubeconfigFile exists +// HasValidClientCertificate 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 not empty, the given commonName matches the common name of the subject in the -// certificate stored in TLSCertFile -func hasValidKubeconfig(secret *corev1.Secret, commonName string) bool { - if secret.Data == nil { - klog.V(4).Infof("No kubeconfig found in secret %q", secret.Namespace+"/"+secret.Name) +// 4. If subject is specified, it matches the subject in the certificate stored in TLSCertFile +func HasValidHubKubeconfig(secret *corev1.Secret, subject *pkix.Name) bool { + if len(secret.Data) == 0 { + klog.V(4).Infof("No data found in secret %q", secret.Namespace+"/"+secret.Name) return false } @@ -45,36 +44,19 @@ func hasValidKubeconfig(secret *corev1.Secret, commonName string) bool { return false } - valid, err := IsCertificateValid(certData) + valid, err := IsCertificateValid(certData, subject) if err != nil { - klog.V(4).Infof("unable to validate certificate in secret %s: %v", secret.Namespace+"/"+secret.Name, err) + klog.V(4).Infof("Unable to validate certificate in secret %s: %v", secret.Namespace+"/"+secret.Name, err) return false } - if len(commonName) == 0 || !valid { - return valid - } - - // check the common name of the subject in certification - certs, err := certutil.ParseCertsPEM(certData) - if err != nil { - klog.V(4).Infof("unable to parse certificate: %v", err) - return false - } - - for _, cert := range certs { - if cert.Subject.CommonName == commonName { - return true - } - } - - klog.V(4).Infof("certificate is not issued for %q", commonName) - return false + return valid } -// IsCertificateValid return true if all certs in client certificate are not expired. -// Otherwise return false -func IsCertificateValid(certData []byte) (bool, error) { +// IsCertificateValid return true if +// 1) All certs in client certificate are not expired. +// 2) At least one cert matches the given subject if specified +func IsCertificateValid(certData []byte, subject *pkix.Name) (bool, error) { certs, err := certutil.ParseCertsPEM(certData) if err != nil { return false, errors.New("unable to parse certificate") @@ -93,7 +75,29 @@ func IsCertificateValid(certData []byte) (bool, error) { } } - return true, nil + if subject == nil { + return true, nil + } + + // check subject of certificates + for _, cert := range certs { + if cert.Subject.CommonName != subject.CommonName { + continue + } + + if !reflect.DeepEqual(cert.Subject.Organization, subject.Organization) { + continue + } + + if !reflect.DeepEqual(cert.Subject.OrganizationalUnit, subject.OrganizationalUnit) { + continue + } + return true, nil + } + + klog.V(4).Infof("Certificate is not issued for subject (cn=%s; o=%s; ou=%s)", + subject.CommonName, strings.Join(subject.Organization, ","), strings.Join(subject.OrganizationalUnit, ",")) + return false, nil } // getCertValidityPeriod returns the validity period of the client certificate in the secret @@ -137,8 +141,8 @@ func getCertValidityPeriod(secret *corev1.Secret) (*time.Time, *time.Time, error return notBefore, notAfter, nil } -// buildKubeconfig builds a kubeconfig based on a rest config template with a cert/key pair -func buildKubeconfig(clientConfig *restclient.Config, certPath, keyPath string) clientcmdapi.Config { +// BuildKubeconfig builds a kubeconfig based on a rest config template with a cert/key pair +func BuildKubeconfig(clientConfig *restclient.Config, certPath, keyPath string) clientcmdapi.Config { // Build kubeconfig. kubeconfig := clientcmdapi.Config{ // Define a cluster stanza based on the bootstrap kubeconfig. @@ -164,38 +168,16 @@ func buildKubeconfig(clientConfig *restclient.Config, certPath, keyPath string) return kubeconfig } -func isCSRApproved(csr *certificates.CertificateSigningRequest) bool { - // TODO: need to make it work in csr v1 as well +// isCSRApproved returns true if the given csr has been approved +func isCSRApproved(csr *certificatesv1beta1.CertificateSigningRequest) bool { approved := false for _, condition := range csr.Status.Conditions { - if condition.Type == certificates.CertificateDenied { + if condition.Type == certificatesv1beta1.CertificateDenied { return false - } else if condition.Type == certificates.CertificateApproved { + } else if condition.Type == certificatesv1beta1.CertificateApproved { approved = true } } return approved } - -// GetClusterAgentNamesFromCertificate returns the cluster name and agent name by parsing -// the common name of the certification -func GetClusterAgentNamesFromCertificate(certData []byte) (clusterName, agentName string, err error) { - certs, err := certutil.ParseCertsPEM(certData) - if err != nil { - return "", "", fmt.Errorf("unable to parse certificate: %w", err) - } - - for _, cert := range certs { - if ok := strings.HasPrefix(cert.Subject.CommonName, user.SubjectPrefix); !ok { - continue - } - names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, user.SubjectPrefix), ":") - if len(names) != 2 { - continue - } - return names[0], names[1], nil - } - - return "", "", nil -} diff --git a/pkg/spoke/hubclientcert/certificate_test.go b/pkg/clientcert/certificate_test.go similarity index 65% rename from pkg/spoke/hubclientcert/certificate_test.go rename to pkg/clientcert/certificate_test.go index 6bbb513ca..3658d1771 100644 --- a/pkg/spoke/hubclientcert/certificate_test.go +++ b/pkg/clientcert/certificate_test.go @@ -1,18 +1,19 @@ -package hubclientcert +package clientcert import ( "crypto/x509" + "crypto/x509/pkix" "testing" "time" - testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" - certificates "k8s.io/api/certificates/v1beta1" corev1 "k8s.io/api/core/v1" certutil "k8s.io/client-go/util/cert" + + testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" ) -func TestCSRApproved(t *testing.T) { +func TestIsCSRApproved(t *testing.T) { cases := []struct { name string csr *certificates.CertificateSigningRequest @@ -42,12 +43,12 @@ func TestCSRApproved(t *testing.T) { } } -func TestValidKubeconfig(t *testing.T) { +func TestHasValidHubKubeconfig(t *testing.T) { cases := []struct { - name string - secret *corev1.Secret - commonName string - isValid bool + name string + secret *corev1.Secret + subject *pkix.Name + isValid bool }{ { name: "no data", @@ -76,24 +77,107 @@ func TestValidKubeconfig(t *testing.T) { }), }, { - name: "unmatched common name", - secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ + name: "expired cert", + secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", -60*time.Second), map[string][]byte{ KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }), - commonName: "wrong-common-name", }, { - name: "valid hub config", + name: "invalid common name", secret: testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }), - commonName: "test", - isValid: true, + 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(nil, nil), + }), + subject: &pkix.Name{ + CommonName: "test", + }, + isValid: true, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - isValid := hasValidKubeconfig(c.secret, c.commonName) + isValid := HasValidHubKubeconfig(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 + testCert *testinghelpers.TestCert + subject *pkix.Name + isValid bool + }{ + { + name: "no cert", + testCert: &testinghelpers.TestCert{}, + }, + { + name: "bad cert", + testCert: &testinghelpers.TestCert{Cert: []byte("bad cert")}, + }, + { + name: "expired cert", + testCert: testinghelpers.NewTestCert("test", -60*time.Second), + }, + { + name: "invalid common name", + testCert: testinghelpers.NewTestCert("test", 60*time.Second), + subject: &pkix.Name{ + CommonName: "wrong-common-name", + }, + }, + { + name: "invalid organization", + testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{ + CommonName: "test", + Organization: []string{"a", "b"}, + }, 60*time.Second), + subject: &pkix.Name{ + CommonName: "test", + Organization: []string{"c"}, + }, + }, + { + name: "invalid organizational unit", + testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{ + CommonName: "test", + OrganizationalUnit: []string{"x"}, + }, 60*time.Second), + subject: &pkix.Name{ + CommonName: "test", + OrganizationalUnit: []string{"y", "z"}, + }, + }, + { + name: "valid cert", + testCert: testinghelpers.NewTestCertWithSubject(pkix.Name{ + CommonName: "test", + Organization: []string{"a", "b"}, + OrganizationalUnit: []string{"x"}, + }, 60*time.Second), + subject: &pkix.Name{ + CommonName: "test", + Organization: []string{"a", "b"}, + OrganizationalUnit: []string{"x"}, + }, + isValid: true, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + isValid, _ := IsCertificateValid(c.testCert.Cert, c.subject) if isValid != c.isValid { t.Errorf("expected %t, but got %t", c.isValid, isValid) } @@ -150,43 +234,3 @@ func TestGetCertValidityPeriod(t *testing.T) { }) } } - -func TestGetClusterAgentNamesFromCertificate(t *testing.T) { - cases := []struct { - name string - certData []byte - expectedClusterName string - expectedAgentName string - expectedErrorPrefix string - }{ - { - name: "cert data is invalid", - certData: []byte("invalid cert"), - expectedErrorPrefix: "unable to parse certificate:", - }, - { - name: "cert with invalid commmon name", - certData: testinghelpers.NewTestCert("test", 60*time.Second).Cert, - }, - { - name: "valid cert with correct common name", - certData: testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second).Cert, - expectedClusterName: "cluster1", - expectedAgentName: "agent1", - }, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - clusterName, agentName, err := GetClusterAgentNamesFromCertificate(c.certData) - testinghelpers.AssertErrorWithPrefix(t, err, c.expectedErrorPrefix) - - if clusterName != c.expectedClusterName { - t.Errorf("expect %v, but got %v", c.expectedClusterName, clusterName) - } - - if agentName != c.expectedAgentName { - t.Errorf("expect %v, but got %v", c.expectedAgentName, agentName) - } - }) - } -} diff --git a/pkg/spoke/hubclientcert/controller_test.go b/pkg/clientcert/controller_test.go similarity index 81% rename from pkg/spoke/hubclientcert/controller_test.go rename to pkg/clientcert/controller_test.go index 1d043fc76..dbdff8529 100644 --- a/pkg/spoke/hubclientcert/controller_test.go +++ b/pkg/clientcert/controller_test.go @@ -1,15 +1,13 @@ -package hubclientcert +package clientcert import ( "context" + "crypto/x509/pkix" "fmt" "reflect" "testing" "time" - testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" - "github.com/open-cluster-management/registration/pkg/hub/user" - certificates "k8s.io/api/certificates/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,7 +15,9 @@ import ( "k8s.io/client-go/informers" kubefake "k8s.io/client-go/kubernetes/fake" clienttesting "k8s.io/client-go/testing" - "k8s.io/client-go/tools/clientcmd" + + testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" + "github.com/open-cluster-management/registration/pkg/hub/user" ) const ( @@ -30,6 +30,10 @@ const ( var commonName = fmt.Sprintf("%s%s:%s", user.SubjectPrefix, testinghelpers.TestManagedClusterName, testAgentName) func TestSync(t *testing.T) { + testSubject := &pkix.Name{ + CommonName: commonName, + } + cases := []struct { name string queueKey string @@ -69,6 +73,7 @@ func TestSync(t *testing.T) { } }, }, + { name: "syc csr after bootstrap", queueKey: testSecretName, @@ -79,13 +84,18 @@ func TestSync(t *testing.T) { }, ), }, - approvedCSRCert: testinghelpers.NewTestCert(testinghelpers.TestManagedClusterName, 10*time.Second), + approvedCSRCert: testinghelpers.NewTestCert(commonName, 10*time.Second), validateActions: func(t *testing.T, hubActions, agentActions []clienttesting.Action) { testinghelpers.AssertActions(t, hubActions, "get") testinghelpers.AssertActions(t, agentActions, "get", "update") actual := agentActions[1].(clienttesting.UpdateActionImpl).Object - if !hasValidKubeconfig(actual.(*corev1.Secret), "") { - t.Error("kubeconfig secret is invalid") + secret := actual.(*corev1.Secret) + valid, err := IsCertificateValid(secret.Data[TLSCertFile], testSubject) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !valid { + t.Error("client certificate is invalid") } }, }, @@ -93,7 +103,7 @@ func TestSync(t *testing.T) { name: "sync a valid hub kubeconfig secret", queueKey: testSecretName, secrets: []runtime.Object{ - testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", testinghelpers.NewTestCert(commonName, 100*time.Second), map[string][]byte{ + testinghelpers.NewHubKubeconfigSecret(testNamespace, testSecretName, "1", testinghelpers.NewTestCert(commonName, 10000*time.Second), map[string][]byte{ ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), AgentNameFile: []byte(testAgentName), KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), @@ -135,6 +145,7 @@ func TestSync(t *testing.T) { csrs = append(csrs, csr) } hubKubeClient := kubefake.NewSimpleClientset(csrs...) + // GenerateName is not working for fake clent, we set the name with prepend reactor hubKubeClient.PrependReactor( "create", @@ -144,30 +155,36 @@ func TestSync(t *testing.T) { }, ) hubInformerFactory := informers.NewSharedInformerFactory(hubKubeClient, 3*time.Minute) - agentKubeClient := kubefake.NewSimpleClientset(c.secrets...) - agentInformerFactory := informers.NewSharedInformerFactory(agentKubeClient, 3*time.Minute) - controller := &ClientCertForHubController{ - clusterName: testinghelpers.TestManagedClusterName, - agentName: testAgentName, - hubKubeconfigSecretNamespace: testNamespace, - hubKubeconfigSecretName: testSecretName, - hubCSRLister: hubInformerFactory.Certificates().V1beta1().CertificateSigningRequests().Lister(), - hubCSRClient: hubKubeClient.CertificatesV1beta1().CertificateSigningRequests(), - spokeSecretLister: agentInformerFactory.Core().V1().Secrets().Lister(), - spokeCoreClient: agentKubeClient.CoreV1(), + clientCertOption := ClientCertOption{ + SecretNamespace: testNamespace, + SecretName: testSecretName, + AdditonalSecretData: map[string][]byte{ + ClusterNameFile: []byte(testinghelpers.TestManagedClusterName), + AgentNameFile: []byte(testAgentName), + }, + } + csrOption := CSROption{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Subject: testSubject, + SignerName: certificates.KubeAPIServerClientSignerName, + } + + controller := &clientCertificateController{ + ClientCertOption: clientCertOption, + CSROption: csrOption, + hubCSRLister: hubInformerFactory.Certificates().V1beta1().CertificateSigningRequests().Lister(), + hubCSRClient: hubKubeClient.CertificatesV1beta1().CertificateSigningRequests(), + spokeCoreClient: agentKubeClient.CoreV1(), + controllerName: "test-agent", } if c.approvedCSRCert != nil { controller.csrName = testCSRName controller.keyData = c.approvedCSRCert.Key - kubeconfig := testinghelpers.NewKubeconfig(c.approvedCSRCert.Key, c.approvedCSRCert.Cert) - clientConfig, err := clientcmd.RESTConfigFromKubeConfig(kubeconfig) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - controller.hubClientConfig = clientConfig } err := controller.sync(context.TODO(), testinghelpers.NewFakeSyncContext(t, c.queueKey)) diff --git a/pkg/helpers/testing/testinghelpers.go b/pkg/helpers/testing/testinghelpers.go index a85ccd05e..b517fef44 100644 --- a/pkg/helpers/testing/testinghelpers.go +++ b/pkg/helpers/testing/testinghelpers.go @@ -372,7 +372,7 @@ func NewHubKubeconfigSecret(namespace, name, resourceVersion string, cert *TestC return secret } -func NewTestCert(commonName string, duration time.Duration) *TestCert { +func NewTestCertWithSubject(subject pkix.Name, duration time.Duration) *TestCert { caKey, err := rsa.GenerateKey(cryptorand.Reader, 2048) if err != nil { panic(err) @@ -391,9 +391,7 @@ func NewTestCert(commonName string, duration time.Duration) *TestCert { certDERBytes, err := x509.CreateCertificate( cryptorand.Reader, &x509.Certificate{ - Subject: pkix.Name{ - CommonName: commonName, - }, + Subject: subject, SerialNumber: big.NewInt(1), NotBefore: caCert.NotBefore, NotAfter: time.Now().Add(duration).UTC(), @@ -425,6 +423,12 @@ func NewTestCert(commonName string, duration time.Duration) *TestCert { } } +func NewTestCert(commonName string, duration time.Duration) *TestCert { + return NewTestCertWithSubject(pkix.Name{ + CommonName: commonName, + }, duration) +} + func WriteFile(filename string, data []byte) { if err := ioutil.WriteFile(filename, data, 0644); err != nil { panic(err) diff --git a/pkg/spoke/hubclientcert/controller.go b/pkg/spoke/hubclientcert/controller.go deleted file mode 100644 index 1b3a5f116..000000000 --- a/pkg/spoke/hubclientcert/controller.go +++ /dev/null @@ -1,345 +0,0 @@ -package hubclientcert - -import ( - "context" - "crypto/tls" - "crypto/x509/pkix" - "fmt" - "reflect" - "time" - - "github.com/openshift/library-go/pkg/controller/factory" - "github.com/openshift/library-go/pkg/operator/events" - - certificates "k8s.io/api/certificates/v1beta1" - corev1 "k8s.io/api/core/v1" - kerrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1" - corev1informers "k8s.io/client-go/informers/core/v1" - csrclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" - corev1client "k8s.io/client-go/kubernetes/typed/core/v1" - certificateslisters "k8s.io/client-go/listers/certificates/v1beta1" - corev1lister "k8s.io/client-go/listers/core/v1" - restclient "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" - certutil "k8s.io/client-go/util/cert" - "k8s.io/client-go/util/keyutil" - "k8s.io/klog/v2" - - "github.com/open-cluster-management/registration/pkg/hub/user" -) - -const ( - // KubeconfigFile is the name of the kubeconfig file in kubeconfigSecret - KubeconfigFile = "kubeconfig" - // TLSKeyFile is the name of tls key file in kubeconfigSecret - TLSKeyFile = "tls.key" - // TLSCertFile is the name of the tls cert file in kubeconfigSecret - TLSCertFile = "tls.crt" - - clusterNameAnnotation = "open-cluster-management.io/cluster-name" - ClusterNameFile = "cluster-name" - AgentNameFile = "agent-name" - - irrelevantSecretKey = "irrelevantSecretKey" -) - -// ControllerSyncInterval is exposed so that integration tests can crank up the constroller sync speed. -var ControllerSyncInterval = 5 * time.Minute - -// ClientCertForHubController maintains the client cert and kubeconfig for hub -type ClientCertForHubController struct { - clusterName string - agentName string - // hubKubeconfigSecretNamespace is the namespace of the hubKubeconfigSecret. - // The secret may contain the keys below: - // 1. kubeconfig: kubeconfig file for hub with references to tls key/cert files in the same directory - // 2. tls.key: tls key file - // 3. tls.crt: tls cert file - // 4. cluster-name: cluster name - // 5. agent-name: agent name - hubKubeconfigSecretNamespace string - hubKubeconfigSecretName string - // hubClientConfig is an anonymous client config used as template to create the real client config for hub. - // It should not be mutated by controller. - hubClientConfig *restclient.Config - hubCSRLister certificateslisters.CertificateSigningRequestLister - hubCSRClient csrclient.CertificateSigningRequestInterface - spokeSecretLister corev1lister.SecretLister - spokeCoreClient corev1client.CoreV1Interface - // csrName is the name of csr created by controller and waiting for approval. - csrName string - // keyData is the private key data used to created a csr - // csrName and keyData store the internal state of the controller. They are set after controller creates a new csr - // and cleared once the csr is approved and processed by controller. There are 4 combination of their values: - // 1. csrName empty, keyData empty: means we aren't trying to create a new client cert, our current one is valid - // 2. csrName set, keyData empty: there was bug - // 3. csrName set, keyData set: we are waiting for a new cert to be signed. - // 4. csrName empty, keydata set: the CSR failed to create, this shouldn't happen, it's a bug. - keyData []byte -} - -// NewClientCertForHubController return a ClientCertForHubController -func NewClientCertForHubController( - clusterName, agentName, hubKubeconfigSecretNamespace, kubeconfigSecretName string, - hubClientConfig *restclient.Config, - spokeCoreClient corev1client.CoreV1Interface, - hubCSRClient csrclient.CertificateSigningRequestInterface, - hubCSRInformer certificatesinformers.CertificateSigningRequestInformer, - spokeSecretInformer corev1informers.SecretInformer, - recorder events.Recorder, controllerName string) factory.Controller { - c := &ClientCertForHubController{ - clusterName: clusterName, - agentName: agentName, - hubKubeconfigSecretNamespace: hubKubeconfigSecretNamespace, - hubKubeconfigSecretName: kubeconfigSecretName, - hubClientConfig: hubClientConfig, - hubCSRLister: hubCSRInformer.Lister(), - hubCSRClient: hubCSRClient, - spokeSecretLister: spokeSecretInformer.Lister(), - spokeCoreClient: spokeCoreClient, - } - - return factory.New(). - WithFilteredEventsInformersQueueKeyFunc( - func(obj runtime.Object) string { - accessor, _ := meta.Accessor(obj) - return accessor.GetName() - }, - func(obj interface{}) bool { - accessor, err := meta.Accessor(obj) - if err != nil { - return false - } - // only enqueue when hub kubeconfig secret is changed - if accessor.GetNamespace() == hubKubeconfigSecretNamespace && accessor.GetName() == kubeconfigSecretName { - return true - } - return false - }, - spokeSecretInformer.Informer()). - WithInformers(hubCSRInformer.Informer()). - WithSync(c.sync). - ResyncEvery(ControllerSyncInterval). - ToController(controllerName, recorder) -} - -func (c *ClientCertForHubController) sync(ctx context.Context, syncCtx factory.SyncContext) error { - // get hubKubeconfigSecret - secret, err := c.spokeCoreClient.Secrets(c.hubKubeconfigSecretNamespace).Get(ctx, c.hubKubeconfigSecretName, metav1.GetOptions{}) - switch { - case kerrors.IsNotFound(err): - secret = &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: c.hubKubeconfigSecretNamespace, - Name: c.hubKubeconfigSecretName, - }, - } - case err != nil: - return fmt.Errorf("unable to get secret %q: %w", c.hubKubeconfigSecretNamespace+"/"+c.hubKubeconfigSecretName, err) - } - - // reconcile pending csr if exists - if c.csrName != "" { - newSecretConfig, err := c.syncCSR(secret) - if err != nil { - c.reset() - return err - } - if len(newSecretConfig) == 0 { - return nil - } - - newSecretConfig[ClusterNameFile] = []byte(c.clusterName) - newSecretConfig[AgentNameFile] = []byte(c.agentName) - secret.Data = newSecretConfig - - // save the changes into secret - if err := c.saveHubKubeconfigSecret(secret); err != nil { - return err - } - syncCtx.Recorder().Event("ClientCertificateCreated", "A new client certificate is available") - return nil - } - - // save the cluster name and agent name into secret if they are not saved yet - newSecretConfig := map[string][]byte{} - for k, v := range secret.Data { - newSecretConfig[k] = v - } - newSecretConfig[ClusterNameFile] = []byte(c.clusterName) - newSecretConfig[AgentNameFile] = []byte(c.agentName) - if !reflect.DeepEqual(newSecretConfig, secret.Data) { - secret.Data = newSecretConfig - if err := c.saveHubKubeconfigSecret(secret); err != nil { - return err - } - } - - // create a csr to request new client certificate if - // a. there is no valid client certificate issued for the current cluster/agent - // b. client certificate exists and has less than 20% of its life remaining - if hasValidKubeconfig(secret, fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName)) { - notBefore, notAfter, err := getCertValidityPeriod(secret) - if err != nil { - return err - } - - total := notAfter.Sub(*notBefore) - remaining := notAfter.Sub(time.Now()) - klog.V(4).Infof("Client certificate time total=%v, remaining=%v, remaining/total=%v", total, remaining, remaining.Seconds()/total.Seconds()) - if remaining.Seconds()/total.Seconds() > 0.2 { - // Do nothing if the client certificate is valid and has more than 20% of its life remaining - klog.V(4).Info("Client certificate is valid and has more than 20% of its life remaining") - return nil - } - syncCtx.Recorder().Eventf("CertificateRotationStarted", "The current client certificate for hub expires in %v. Start certificate rotation", remaining.Round(time.Second)) - } else { - syncCtx.Recorder().Event("NoValidCertificateFound", "No valid client certificate for hub is found. Bootstrap is required") - } - - // create a csr - c.keyData, err = keyutil.MakeEllipticPrivateKeyPEM() - if err != nil { - return err - } - - c.csrName, err = c.createCSR() - if err != nil { - c.reset() - return err - } - syncCtx.Recorder().Eventf("CSRCreated", "A csr %q is created", c.csrName) - return nil -} - -func (c *ClientCertForHubController) syncCSR(secret *corev1.Secret) (map[string][]byte, error) { - // skip if there is no ongoing csr - if c.csrName == "" { - c.reset() - return nil, nil - } - - // skip if csr no longer exists - csr, err := c.hubCSRLister.Get(c.csrName) - if kerrors.IsNotFound(err) { - // fallback to fetching csr from hub apiserver in case it is not cached by informer yet - csr, err = c.hubCSRClient.Get(context.Background(), c.csrName, metav1.GetOptions{}) - if kerrors.IsNotFound(err) { - klog.V(4).Infof("Unable to get csr %q. It might have already been deleted.", c.csrName) - c.reset() - return nil, nil - } - } - if err != nil { - return nil, err - } - - // skip if csr is not approved yet - if !isCSRApproved(csr) { - return nil, nil - } - - // skip if csr has no certificate in its status yet - if csr.Status.Certificate == nil { - return nil, nil - } - - klog.V(4).Infof("Sync csr %v", c.csrName) - // check if cert in csr status matches with the corresponding private key - if c.keyData == nil { - c.reset() - return nil, fmt.Errorf("No private key found for certificate in csr: %s", c.csrName) - } - _, err = tls.X509KeyPair(csr.Status.Certificate, c.keyData) - if err != nil { - c.reset() - return nil, fmt.Errorf("Private key does not match with the certificate in csr: %s", c.csrName) - } - - data := map[string][]byte{ - TLSCertFile: csr.Status.Certificate, - TLSKeyFile: c.keyData, - } - - // create a kubeconfig with references to the key/cert files in kubeconfigSecret if it dose not exists. - // So other components deployed in separated deployments are able to access this kubeconfig for hub as - // well by sharing the secret - kubeconfigData, ok := secret.Data[KubeconfigFile] - if !ok { - kubeconfig := buildKubeconfig(restclient.CopyConfig(c.hubClientConfig), TLSCertFile, TLSKeyFile) - kubeconfigData, err = clientcmd.Write(kubeconfig) - if err != nil { - return nil, err - } - } - data[KubeconfigFile] = kubeconfigData - - // clear the csr name and private key - c.reset() - return data, nil -} - -func (c *ClientCertForHubController) createCSR() (string, error) { - subject := &pkix.Name{ - Organization: []string{ - fmt.Sprintf("%s%s", user.SubjectPrefix, c.clusterName), - user.ManagedClustersGroup, - }, - CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName), - } - - privateKey, err := keyutil.ParsePrivateKeyPEM(c.keyData) - if err != nil { - return "", fmt.Errorf("invalid private key for certificate request: %w", err) - } - csrData, err := certutil.MakeCSR(privateKey, subject, nil, nil) - if err != nil { - return "", fmt.Errorf("unable to generate certificate request: %w", err) - } - - signerName := certificates.KubeAPIServerClientSignerName - - csr := &certificates.CertificateSigningRequest{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: fmt.Sprintf("%s-", c.clusterName), - Labels: map[string]string{ - // the label is only an hint for cluster name. Anyone could set/modify it. - clusterNameAnnotation: c.clusterName, - }, - }, - Spec: certificates.CertificateSigningRequestSpec{ - Request: csrData, - Usages: []certificates.KeyUsage{ - certificates.UsageDigitalSignature, - certificates.UsageKeyEncipherment, - certificates.UsageClientAuth, - }, - SignerName: &signerName, - }, - } - - req, err := c.hubCSRClient.Create(context.TODO(), csr, metav1.CreateOptions{}) - if err != nil { - return "", err - } - return req.Name, nil -} - -func (c *ClientCertForHubController) saveHubKubeconfigSecret(secret *corev1.Secret) error { - var err error - if secret.ResourceVersion == "" { - _, err = c.spokeCoreClient.Secrets(c.hubKubeconfigSecretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) - return err - } - _, err = c.spokeCoreClient.Secrets(c.hubKubeconfigSecretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) - return err -} - -func (c *ClientCertForHubController) reset() { - c.csrName = "" - c.keyData = nil -} diff --git a/pkg/spoke/managedcluster/registration.go b/pkg/spoke/managedcluster/registration.go new file mode 100644 index 000000000..180d876ea --- /dev/null +++ b/pkg/spoke/managedcluster/registration.go @@ -0,0 +1,112 @@ +package managedcluster + +import ( + "crypto/x509/pkix" + "fmt" + "strings" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + certificates "k8s.io/api/certificates/v1beta1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1" + corev1informers "k8s.io/client-go/informers/core/v1" + csrclient "k8s.io/client-go/kubernetes/typed/certificates/v1beta1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + certutil "k8s.io/client-go/util/cert" + + "github.com/open-cluster-management/registration/pkg/clientcert" + "github.com/open-cluster-management/registration/pkg/hub/user" +) + +// NewClientCertForHubController returns a controller to +// 1). Create a new client certificate and build a hub kubeconfig for the registration agent; +// 2). Or rotate the client certificate referenced by the hub kubeconfig before it become expired; +func NewClientCertForHubController( + clusterName string, + agentName string, + clientCertSecretNamespace string, + clientCertSecretName string, + kubeconfigData []byte, + spokeCoreClient corev1client.CoreV1Interface, + hubCSRClient csrclient.CertificateSigningRequestInterface, + hubCSRInformer certificatesinformers.CertificateSigningRequestInformer, + spokeSecretInformer corev1informers.SecretInformer, + recorder events.Recorder, + controllerName string, +) factory.Controller { + clientCertOption := clientcert.ClientCertOption{ + SecretNamespace: clientCertSecretNamespace, + SecretName: clientCertSecretName, + AdditonalSecretData: map[string][]byte{ + clientcert.ClusterNameFile: []byte(clusterName), + clientcert.AgentNameFile: []byte(agentName), + clientcert.KubeconfigFile: kubeconfigData, + }, + } + csrOption := clientcert.CSROption{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: fmt.Sprintf("%s-", clusterName), + Labels: map[string]string{ + // the label is only an hint for cluster name. Anyone could set/modify it. + clientcert.ClusterNameLabel: clusterName, + }, + }, + Subject: &pkix.Name{ + Organization: []string{ + fmt.Sprintf("%s%s", user.SubjectPrefix, clusterName), + user.ManagedClustersGroup, + }, + CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, clusterName, agentName), + }, + SignerName: certificates.KubeAPIServerClientSignerName, + EventFilterFunc: func(obj interface{}) bool { + accessor, err := meta.Accessor(obj) + if err != nil { + return false + } + labels := accessor.GetLabels() + // only enqueue csr from a specific managed cluster + if labels[clientcert.ClusterNameLabel] != clusterName { + return false + } + + // only enqueue csr whose name starts with the cluster name + return strings.HasPrefix(accessor.GetName(), fmt.Sprintf("%s-", clusterName)) + }, + } + + return clientcert.NewClientCertificateController( + clientCertOption, + csrOption, + hubCSRInformer, + hubCSRClient, + spokeSecretInformer, + spokeCoreClient, + recorder, + controllerName, + ) +} + +// GetClusterAgentNamesFromCertificate returns the cluster name and agent name by parsing +// the common name of the certification +func GetClusterAgentNamesFromCertificate(certData []byte) (clusterName, agentName string, err error) { + certs, err := certutil.ParseCertsPEM(certData) + if err != nil { + return "", "", fmt.Errorf("unable to parse certificate: %w", err) + } + + for _, cert := range certs { + if ok := strings.HasPrefix(cert.Subject.CommonName, user.SubjectPrefix); !ok { + continue + } + names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, user.SubjectPrefix), ":") + if len(names) != 2 { + continue + } + return names[0], names[1], nil + } + + return "", "", nil +} diff --git a/pkg/spoke/managedcluster/registration_test.go b/pkg/spoke/managedcluster/registration_test.go new file mode 100644 index 000000000..d62980103 --- /dev/null +++ b/pkg/spoke/managedcluster/registration_test.go @@ -0,0 +1,48 @@ +package managedcluster + +import ( + "testing" + "time" + + testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" +) + +func TestGetClusterAgentNamesFromCertificate(t *testing.T) { + cases := []struct { + name string + certData []byte + expectedClusterName string + expectedAgentName string + expectedErrorPrefix string + }{ + { + name: "cert data is invalid", + certData: []byte("invalid cert"), + expectedErrorPrefix: "unable to parse certificate:", + }, + { + name: "cert with invalid commmon name", + certData: testinghelpers.NewTestCert("test", 60*time.Second).Cert, + }, + { + name: "valid cert with correct common name", + certData: testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second).Cert, + expectedClusterName: "cluster1", + expectedAgentName: "agent1", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + clusterName, agentName, err := GetClusterAgentNamesFromCertificate(c.certData) + testinghelpers.AssertErrorWithPrefix(t, err, c.expectedErrorPrefix) + + if clusterName != c.expectedClusterName { + t.Errorf("expect %v, but got %v", c.expectedClusterName, clusterName) + } + + if agentName != c.expectedAgentName { + t.Errorf("expect %v, but got %v", c.expectedAgentName, agentName) + } + }) + } +} diff --git a/pkg/spoke/hubclientcert/secret_controller.go b/pkg/spoke/managedcluster/secret_controller.go similarity index 99% rename from pkg/spoke/hubclientcert/secret_controller.go rename to pkg/spoke/managedcluster/secret_controller.go index c771a69e2..6eaefbb2f 100644 --- a/pkg/spoke/hubclientcert/secret_controller.go +++ b/pkg/spoke/managedcluster/secret_controller.go @@ -1,4 +1,4 @@ -package hubclientcert +package managedcluster import ( "bytes" diff --git a/pkg/spoke/hubclientcert/secret_controller_test.go b/pkg/spoke/managedcluster/secret_controller_test.go similarity index 75% rename from pkg/spoke/hubclientcert/secret_controller_test.go rename to pkg/spoke/managedcluster/secret_controller_test.go index 51d5b1986..6e594c20f 100644 --- a/pkg/spoke/hubclientcert/secret_controller_test.go +++ b/pkg/spoke/managedcluster/secret_controller_test.go @@ -1,4 +1,4 @@ -package hubclientcert +package managedcluster import ( "context" @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/open-cluster-management/registration/pkg/clientcert" testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" "github.com/openshift/library-go/pkg/operator/events/eventstesting" @@ -17,6 +18,11 @@ import ( kubefake "k8s.io/client-go/kubernetes/fake" ) +const ( + testNamespace = "testns" + testSecretName = "testsecret" +) + func TestDumpSecret(t *testing.T) { testDir, err := ioutil.TempDir("", "dumpsecret") if err != nil { @@ -54,42 +60,42 @@ func TestDumpSecret(t *testing.T) { testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ - ClusterNameFile: []byte("test"), - AgentNameFile: []byte("test"), - KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), + clientcert.ClusterNameFile: []byte("test"), + clientcert.AgentNameFile: []byte("test"), + clientcert.KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }, ), validateFiles: func(t *testing.T, hubKubeconfigDir string) { - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, ClusterNameFile), []byte("test")) - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, AgentNameFile), []byte("test")) - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, KubeconfigFile), kubeConfigFile) - testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, TLSKeyFile)) - testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, TLSCertFile)) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.ClusterNameFile), []byte("test")) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.AgentNameFile), []byte("test")) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.KubeconfigFile), kubeConfigFile) + testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, clientcert.TLSKeyFile)) + testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, clientcert.TLSCertFile)) }, }, { name: "secret is updated", queueKey: testSecretName, oldConfigData: map[string][]byte{ - ClusterNameFile: []byte("test"), - AgentNameFile: []byte("test"), - KubeconfigFile: []byte("test"), + clientcert.ClusterNameFile: []byte("test"), + clientcert.AgentNameFile: []byte("test"), + clientcert.KubeconfigFile: []byte("test"), }, secret: testinghelpers.NewHubKubeconfigSecret( testNamespace, testSecretName, "", testinghelpers.NewTestCert("test", 60*time.Second), map[string][]byte{ - ClusterNameFile: []byte("test1"), - AgentNameFile: []byte("test"), - KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), + clientcert.ClusterNameFile: []byte("test1"), + clientcert.AgentNameFile: []byte("test"), + clientcert.KubeconfigFile: testinghelpers.NewKubeconfig(nil, nil), }, ), validateFiles: func(t *testing.T, hubKubeconfigDir string) { - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, ClusterNameFile), []byte("test1")) - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, AgentNameFile), []byte("test")) - testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, KubeconfigFile), kubeConfigFile) - testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, TLSKeyFile)) - testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, TLSCertFile)) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.ClusterNameFile), []byte("test1")) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.AgentNameFile), []byte("test")) + testinghelpers.AssertFileContent(t, path.Join(hubKubeconfigDir, clientcert.KubeconfigFile), kubeConfigFile) + testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, clientcert.TLSKeyFile)) + testinghelpers.AssertFileExist(t, path.Join(hubKubeconfigDir, clientcert.TLSCertFile)) }, }, } diff --git a/pkg/spoke/spokeagent.go b/pkg/spoke/spokeagent.go index 7215d2751..a332c595c 100644 --- a/pkg/spoke/spokeagent.go +++ b/pkg/spoke/spokeagent.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "github.com/openshift/library-go/pkg/controller/factory" "io/ioutil" "os" "path" @@ -12,12 +11,13 @@ import ( clusterv1client "github.com/open-cluster-management/api/client/cluster/clientset/versioned" clusterv1informers "github.com/open-cluster-management/api/client/cluster/informers/externalversions" + "github.com/open-cluster-management/registration/pkg/clientcert" "github.com/open-cluster-management/registration/pkg/features" "github.com/open-cluster-management/registration/pkg/helpers" - "github.com/open-cluster-management/registration/pkg/spoke/hubclientcert" "github.com/open-cluster-management/registration/pkg/spoke/managedcluster" "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/spf13/pflag" @@ -31,7 +31,6 @@ import ( "k8s.io/client-go/kubernetes" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" - restclient "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" "k8s.io/klog/v2" ) @@ -137,7 +136,7 @@ func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext ) go spokeClusterCreatingController.Run(ctx, 1) - hubKubeconfigSecretController := hubclientcert.NewHubKubeconfigSecretController( + hubKubeconfigSecretController := managedcluster.NewHubKubeconfigSecretController( o.HubKubeconfigDir, o.ComponentNamespace, o.HubKubeconfigSecret, spokeKubeClient.CoreV1(), namespacedSpokeKubeInformerFactory.Core().V1().Secrets(), @@ -160,9 +159,16 @@ func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext // create a ClientCertForHubController for spoke agent bootstrap bootstrapInformerFactory := informers.NewSharedInformerFactory(bootstrapKubeClient, 10*time.Minute) - clientCertForHubController := hubclientcert.NewClientCertForHubController( + // create a kubeconfig with references to the key/cert files in the same secret + kubeconfig := clientcert.BuildKubeconfig(bootstrapClientConfig, clientcert.TLSCertFile, clientcert.TLSKeyFile) + kubeconfigData, err := clientcmd.Write(kubeconfig) + if err != nil { + return err + } + + clientCertForHubController := managedcluster.NewClientCertForHubController( o.ClusterName, o.AgentName, o.ComponentNamespace, o.HubKubeconfigSecret, - restclient.AnonymousClientConfig(bootstrapClientConfig), + kubeconfigData, spokeKubeClient.CoreV1(), bootstrapKubeClient.CertificatesV1beta1().CertificateSigningRequests(), bootstrapInformerFactory.Certificates().V1beta1().CertificateSigningRequests(), @@ -191,7 +197,7 @@ func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext } // create hub clients and shared informer factories from hub kube config - hubClientConfig, err := clientcmd.BuildConfigFromFlags("", path.Join(o.HubKubeconfigDir, hubclientcert.KubeconfigFile)) + hubClientConfig, err := clientcmd.BuildConfigFromFlags("", path.Join(o.HubKubeconfigDir, clientcert.KubeconfigFile)) if err != nil { return err } @@ -218,10 +224,17 @@ func (o *SpokeAgentOptions) RunSpokeAgent(ctx context.Context, controllerContext controllerContext.EventRecorder.Event("HubClientConfigReady", "Client config for hub is ready.") + // create a kubeconfig with references to the key/cert files in the same secret + kubeconfig := clientcert.BuildKubeconfig(hubClientConfig, clientcert.TLSCertFile, clientcert.TLSKeyFile) + kubeconfigData, err := clientcmd.Write(kubeconfig) + if err != nil { + return err + } + // create another ClientCertForHubController for client certificate rotation - clientCertForHubController := hubclientcert.NewClientCertForHubController( + clientCertForHubController := managedcluster.NewClientCertForHubController( o.ClusterName, o.AgentName, o.ComponentNamespace, o.HubKubeconfigSecret, - restclient.AnonymousClientConfig(hubClientConfig), + kubeconfigData, spokeKubeClient.CoreV1(), hubKubeClient.CertificatesV1beta1().CertificateSigningRequests(), hubKubeInformerFactory.Certificates().V1beta1().CertificateSigningRequests(), @@ -354,7 +367,7 @@ func (o *SpokeAgentOptions) Complete(coreV1Client corev1client.CoreV1Interface, } // dump data in hub kubeconfig secret into file system if it exists - err = hubclientcert.DumpSecret(coreV1Client, o.ComponentNamespace, o.HubKubeconfigSecret, + err = managedcluster.DumpSecret(coreV1Client, o.ComponentNamespace, o.HubKubeconfigSecret, o.HubKubeconfigDir, ctx, recorder) if err != nil { return err @@ -386,19 +399,19 @@ func generateAgentName() string { // completes. Changing the name of the cluster will make the existing hub kubeconfig invalid, // because certificate in TLSCertFile is issued to a specific cluster/agent. func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { - kubeconfigPath := path.Join(o.HubKubeconfigDir, hubclientcert.KubeconfigFile) + kubeconfigPath := path.Join(o.HubKubeconfigDir, clientcert.KubeconfigFile) if _, err := os.Stat(kubeconfigPath); os.IsNotExist(err) { klog.V(4).Infof("Kubeconfig file %q not found", kubeconfigPath) return false, nil } - keyPath := path.Join(o.HubKubeconfigDir, hubclientcert.TLSKeyFile) + keyPath := path.Join(o.HubKubeconfigDir, clientcert.TLSKeyFile) if _, err := os.Stat(keyPath); os.IsNotExist(err) { klog.V(4).Infof("TLS key file %q not found", keyPath) return false, nil } - certPath := path.Join(o.HubKubeconfigDir, hubclientcert.TLSCertFile) + certPath := path.Join(o.HubKubeconfigDir, clientcert.TLSCertFile) certData, err := ioutil.ReadFile(path.Clean(certPath)) if err != nil { klog.V(4).Infof("Unable to load TLS cert file %q", certPath) @@ -406,7 +419,7 @@ func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { } // check if the tls certificate is issued for the current cluster/agent - clusterName, agentName, err := hubclientcert.GetClusterAgentNamesFromCertificate(certData) + clusterName, agentName, err := managedcluster.GetClusterAgentNamesFromCertificate(certData) if err != nil { return false, nil } @@ -417,7 +430,7 @@ func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { return false, nil } - return hubclientcert.IsCertificateValid(certData) + return clientcert.IsCertificateValid(certData, nil) } // getOrGenerateClusterAgentNames returns cluster name and agent name. @@ -435,10 +448,10 @@ func (o *SpokeAgentOptions) hasValidHubClientConfig() (bool, error) { func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { // try to load cluster/agent name from tls certification var clusterNameInCert, agentNameInCert string - certPath := path.Join(o.HubKubeconfigDir, hubclientcert.TLSCertFile) + certPath := path.Join(o.HubKubeconfigDir, clientcert.TLSCertFile) certData, certErr := ioutil.ReadFile(path.Clean(certPath)) if certErr == nil { - clusterNameInCert, agentNameInCert, _ = hubclientcert.GetClusterAgentNamesFromCertificate(certData) + clusterNameInCert, agentNameInCert, _ = managedcluster.GetClusterAgentNamesFromCertificate(certData) } clusterName := o.ClusterName @@ -447,7 +460,7 @@ func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { // TODO, read cluster name from openshift struct if the spoke agent is running in an openshift cluster // and then load the cluster name from the mounted secret - clusterNameFilePath := path.Join(o.HubKubeconfigDir, hubclientcert.ClusterNameFile) + clusterNameFilePath := path.Join(o.HubKubeconfigDir, clientcert.ClusterNameFile) clusterNameBytes, err := ioutil.ReadFile(path.Clean(clusterNameFilePath)) switch { case len(clusterNameInCert) > 0: @@ -466,7 +479,7 @@ func (o *SpokeAgentOptions) getOrGenerateClusterAgentNames() (string, string) { } // try to load agent name from the mounted secret - agentNameFilePath := path.Join(o.HubKubeconfigDir, hubclientcert.AgentNameFile) + agentNameFilePath := path.Join(o.HubKubeconfigDir, clientcert.AgentNameFile) agentNameBytes, err := ioutil.ReadFile(path.Clean(agentNameFilePath)) var agentName string switch { diff --git a/pkg/spoke/spokeagent_test.go b/pkg/spoke/spokeagent_test.go index 2a41ded4f..4508d7817 100644 --- a/pkg/spoke/spokeagent_test.go +++ b/pkg/spoke/spokeagent_test.go @@ -9,8 +9,8 @@ import ( "testing" "time" + "github.com/open-cluster-management/registration/pkg/clientcert" testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" - "github.com/open-cluster-management/registration/pkg/spoke/hubclientcert" "github.com/openshift/library-go/pkg/operator/events/eventstesting" corev1 "k8s.io/api/core/v1" @@ -306,8 +306,8 @@ func TestGetOrGenerateClusterAgentNames(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { if c.options.HubKubeconfigDir != "" { - testinghelpers.WriteFile(path.Join(tempDir, hubclientcert.ClusterNameFile), []byte(c.expectedClusterName)) - testinghelpers.WriteFile(path.Join(tempDir, hubclientcert.AgentNameFile), []byte(c.expectedAgentName)) + testinghelpers.WriteFile(path.Join(tempDir, clientcert.ClusterNameFile), []byte(c.expectedClusterName)) + testinghelpers.WriteFile(path.Join(tempDir, clientcert.AgentNameFile), []byte(c.expectedAgentName)) } clusterName, agentName := c.options.getOrGenerateClusterAgentNames() if clusterName != c.expectedClusterName { diff --git a/test/integration/integration_suite_test.go b/test/integration/integration_suite_test.go index c1fd8c2a6..5bc78e45c 100644 --- a/test/integration/integration_suite_test.go +++ b/test/integration/integration_suite_test.go @@ -23,8 +23,8 @@ import ( clusterclientset "github.com/open-cluster-management/api/client/cluster/clientset/versioned" workclientset "github.com/open-cluster-management/api/client/work/clientset/versioned" clusterv1 "github.com/open-cluster-management/api/cluster/v1" + "github.com/open-cluster-management/registration/pkg/clientcert" "github.com/open-cluster-management/registration/pkg/hub" - "github.com/open-cluster-management/registration/pkg/spoke/hubclientcert" "github.com/open-cluster-management/registration/pkg/spoke/managedcluster" "github.com/open-cluster-management/registration/test/integration/util" @@ -65,7 +65,7 @@ var _ = ginkgo.BeforeSuite(func(done ginkgo.Done) { // crank up the sync speed transport.CertCallbackRefreshDuration = 5 * time.Second - hubclientcert.ControllerSyncInterval = 5 * time.Second + clientcert.ControllerResyncInterval = 5 * time.Second managedcluster.CreatingControllerSyncInterval = 1 * time.Second // install cluster CRD and start a local kube-apiserver