Merge pull request #120 from elgnay/addon-registration

Refactor and make client cert controller reusable

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

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

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

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

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)

pkg/spoke/hubclientcert/controller.go

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

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

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

pkg/spoke/managedcluster/secret_controller.go

@@ -1,4 +1,4 @@
-package hubclientcert
+package managedcluster
 
 import (
 	"bytes"

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

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 {

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 {

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