From a13f3780cd23d923a4da1d19d61fad2ea48e2fca Mon Sep 17 00:00:00 2001 From: yue9944882 <291271447@qq.com> Date: Tue, 26 Jan 2021 16:56:34 +0800 Subject: [PATCH 1/2] defing a common user-group to match all spoke-clusters Signed-off-by: yue9944882 <291271447@qq.com> --- pkg/hub/csr/controller.go | 15 ++++++++++----- pkg/hub/user/doc.go | 2 ++ pkg/hub/user/identity.go | 8 ++++++++ pkg/spoke/hubclientcert/certificate.go | 6 ++++-- pkg/spoke/hubclientcert/controller.go | 12 ++++++++---- pkg/spoke/hubclientcert/controller_test.go | 3 ++- 6 files changed, 34 insertions(+), 12 deletions(-) create mode 100644 pkg/hub/user/doc.go create mode 100644 pkg/hub/user/identity.go diff --git a/pkg/hub/csr/controller.go b/pkg/hub/csr/controller.go index be3deed24..33ab4eaa3 100644 --- a/pkg/hub/csr/controller.go +++ b/pkg/hub/csr/controller.go @@ -15,16 +15,17 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1" "k8s.io/client-go/kubernetes" certificateslisters "k8s.io/client-go/listers/certificates/v1beta1" "k8s.io/klog/v2" "github.com/open-cluster-management/registration/pkg/helpers" + "github.com/open-cluster-management/registration/pkg/hub/user" ) const ( - subjectPrefix = "system:open-cluster-management:" spokeClusterNameLabel = "open-cluster-management.io/cluster-name" ) @@ -158,16 +159,20 @@ func isSpokeClusterClientCertRenewal(csr *certificatesv1beta1.CertificateSigning return false } - if len(x509cr.Subject.Organization) != 1 { + requestingOrgs := sets.NewString(x509cr.Subject.Organization...) + if requestingOrgs.Has(user.SpokeClustersGroup) { // optional common group for backward-compatibility + requestingOrgs.Delete(user.SpokeClustersGroup) + } + if requestingOrgs.Len() != 1 { return false } - organization := x509cr.Subject.Organization[0] - if organization != fmt.Sprintf("%s%s", subjectPrefix, spokeClusterName) { + expectedPerClusterOrg := fmt.Sprintf("%s%s", user.SubjectPrefix, spokeClusterName) + if !requestingOrgs.Has(expectedPerClusterOrg) { return false } - if !strings.HasPrefix(x509cr.Subject.CommonName, organization) { + if !strings.HasPrefix(x509cr.Subject.CommonName, expectedPerClusterOrg) { return false } diff --git a/pkg/hub/user/doc.go b/pkg/hub/user/doc.go new file mode 100644 index 000000000..5a21d1046 --- /dev/null +++ b/pkg/hub/user/doc.go @@ -0,0 +1,2 @@ +// Package user contains common definition works for kubernetes certificates +package user diff --git a/pkg/hub/user/identity.go b/pkg/hub/user/identity.go new file mode 100644 index 000000000..964e75b5b --- /dev/null +++ b/pkg/hub/user/identity.go @@ -0,0 +1,8 @@ +package user + +const ( + // SubjectPrefix is a prefix for marking open-cluster-management users + SubjectPrefix = "system:open-cluster-management:" + // SpokeClustersGroup is a common group for all spoke clusters + SpokeClustersGroup = SubjectPrefix + "spoke-clusters" +) diff --git a/pkg/spoke/hubclientcert/certificate.go b/pkg/spoke/hubclientcert/certificate.go index 2b581cfde..bc666bbfb 100644 --- a/pkg/spoke/hubclientcert/certificate.go +++ b/pkg/spoke/hubclientcert/certificate.go @@ -12,6 +12,8 @@ import ( 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 @@ -185,10 +187,10 @@ func GetClusterAgentNamesFromCertificate(certData []byte) (clusterName, agentNam } for _, cert := range certs { - if ok := strings.HasPrefix(cert.Subject.CommonName, subjectPrefix); !ok { + if ok := strings.HasPrefix(cert.Subject.CommonName, user.SubjectPrefix); !ok { continue } - names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, subjectPrefix), ":") + names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, user.SubjectPrefix), ":") if len(names) != 2 { continue } diff --git a/pkg/spoke/hubclientcert/controller.go b/pkg/spoke/hubclientcert/controller.go index 3d681f74a..79dff3e55 100644 --- a/pkg/spoke/hubclientcert/controller.go +++ b/pkg/spoke/hubclientcert/controller.go @@ -28,6 +28,8 @@ import ( 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 ( @@ -38,7 +40,6 @@ const ( // TLSCertFile is the name of the tls cert file in kubeconfigSecret TLSCertFile = "tls.crt" - subjectPrefix = "system:open-cluster-management:" clusterNameAnnotation = "open-cluster-management.io/cluster-name" ClusterNameFile = "cluster-name" AgentNameFile = "agent-name" @@ -181,7 +182,7 @@ func (c *ClientCertForHubController) sync(ctx context.Context, syncCtx factory.S // 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", subjectPrefix, c.clusterName, c.agentName)) { + if hasValidKubeconfig(secret, fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName)) { notBefore, notAfter, err := getCertValidityPeriod(secret) if err != nil { return err @@ -284,8 +285,11 @@ func (c *ClientCertForHubController) syncCSR(secret *corev1.Secret) (map[string] func (c *ClientCertForHubController) createCSR() (string, error) { subject := &pkix.Name{ - Organization: []string{fmt.Sprintf("%s%s", subjectPrefix, c.clusterName)}, - CommonName: fmt.Sprintf("%s%s:%s", subjectPrefix, c.clusterName, c.agentName), + Organization: []string{ + fmt.Sprintf("%s%s", user.SubjectPrefix, c.clusterName), + user.SpokeClustersGroup, + }, + CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName), } privateKey, err := keyutil.ParsePrivateKeyPEM(c.keyData) diff --git a/pkg/spoke/hubclientcert/controller_test.go b/pkg/spoke/hubclientcert/controller_test.go index 672ff3c10..1d043fc76 100644 --- a/pkg/spoke/hubclientcert/controller_test.go +++ b/pkg/spoke/hubclientcert/controller_test.go @@ -8,6 +8,7 @@ import ( "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" @@ -26,7 +27,7 @@ const ( testCSRName = "testcsr" ) -var commonName = fmt.Sprintf("%s%s:%s", subjectPrefix, testinghelpers.TestManagedClusterName, testAgentName) +var commonName = fmt.Sprintf("%s%s:%s", user.SubjectPrefix, testinghelpers.TestManagedClusterName, testAgentName) func TestSync(t *testing.T) { cases := []struct { From 842d4bb15e48952cc52afd290c324cc135440510 Mon Sep 17 00:00:00 2001 From: yue9944882 <291271447@qq.com> Date: Wed, 27 Jan 2021 20:37:06 +0800 Subject: [PATCH 2/2] addressing review comments Signed-off-by: yue9944882 <291271447@qq.com> --- pkg/hub/csr/controller.go | 4 ++-- pkg/hub/csr/controller_test.go | 31 ++++++++++++++++++++++++--- pkg/hub/user/identity.go | 4 ++-- pkg/spoke/hubclientcert/controller.go | 2 +- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/pkg/hub/csr/controller.go b/pkg/hub/csr/controller.go index 33ab4eaa3..e547789e5 100644 --- a/pkg/hub/csr/controller.go +++ b/pkg/hub/csr/controller.go @@ -160,8 +160,8 @@ func isSpokeClusterClientCertRenewal(csr *certificatesv1beta1.CertificateSigning } requestingOrgs := sets.NewString(x509cr.Subject.Organization...) - if requestingOrgs.Has(user.SpokeClustersGroup) { // optional common group for backward-compatibility - requestingOrgs.Delete(user.SpokeClustersGroup) + if requestingOrgs.Has(user.ManagedClustersGroup) { // optional common group for backward-compatibility + requestingOrgs.Delete(user.ManagedClustersGroup) } if requestingOrgs.Len() != 1 { return false diff --git a/pkg/hub/csr/controller_test.go b/pkg/hub/csr/controller_test.go index e417ba3f0..990072264 100644 --- a/pkg/hub/csr/controller_test.go +++ b/pkg/hub/csr/controller_test.go @@ -6,12 +6,14 @@ import ( "time" testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing" + "github.com/open-cluster-management/registration/pkg/hub/user" "github.com/openshift/library-go/pkg/operator/events/eventstesting" authorizationv1 "k8s.io/api/authorization/v1" certificatesv1beta1 "k8s.io/api/certificates/v1beta1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/informers" kubefake "k8s.io/client-go/kubernetes/fake" clienttesting "k8s.io/client-go/testing" @@ -23,9 +25,9 @@ var ( Name: "testcsr", Labels: map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"}, SignerName: &signerName, - CN: "system:open-cluster-management:managedcluster1:spokeagent1", - Orgs: []string{"system:open-cluster-management:managedcluster1"}, - Username: "system:open-cluster-management:managedcluster1:spokeagent1", + CN: user.SubjectPrefix + "managedcluster1:spokeagent1", + Orgs: []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup}, + Username: user.SubjectPrefix + "managedcluster1:spokeagent1", ReqBlockType: "CERTIFICATE REQUEST", } ) @@ -96,6 +98,29 @@ func TestSync(t *testing.T) { testinghelpers.AssertCSRCondition(t, actual.(*certificatesv1beta1.CertificateSigningRequest).Status.Conditions, expectedCondition) }, }, + { + name: "allow an auto approving csr w/o ManagedClusterGroup for backward-compatibility", + startingCSRs: []runtime.Object{testinghelpers.NewCSR(testinghelpers.CSRHolder{ + Name: validCSR.Name, + Labels: validCSR.Labels, + SignerName: validCSR.SignerName, + CN: validCSR.CN, + Orgs: sets.NewString(validCSR.Orgs...).Delete(user.ManagedClustersGroup).List(), + Username: validCSR.Username, + ReqBlockType: validCSR.ReqBlockType, + })}, + autoApprovingAllowed: true, + validateActions: func(t *testing.T, actions []clienttesting.Action) { + expectedCondition := certificatesv1beta1.CertificateSigningRequestCondition{ + Type: certificatesv1beta1.CertificateApproved, + Reason: "AutoApprovedByHubCSRApprovingController", + Message: "Auto approving Managed cluster agent certificate after SubjectAccessReview.", + } + testinghelpers.AssertActions(t, actions, "create", "update") + actual := actions[1].(clienttesting.UpdateActionImpl).Object + testinghelpers.AssertCSRCondition(t, actual.(*certificatesv1beta1.CertificateSigningRequest).Status.Conditions, expectedCondition) + }, + }, } for _, c := range cases { diff --git a/pkg/hub/user/identity.go b/pkg/hub/user/identity.go index 964e75b5b..83f6554c7 100644 --- a/pkg/hub/user/identity.go +++ b/pkg/hub/user/identity.go @@ -3,6 +3,6 @@ package user const ( // SubjectPrefix is a prefix for marking open-cluster-management users SubjectPrefix = "system:open-cluster-management:" - // SpokeClustersGroup is a common group for all spoke clusters - SpokeClustersGroup = SubjectPrefix + "spoke-clusters" + // ManagedClustersGroup is a common group for all spoke clusters + ManagedClustersGroup = SubjectPrefix + "managed-clusters" ) diff --git a/pkg/spoke/hubclientcert/controller.go b/pkg/spoke/hubclientcert/controller.go index 79dff3e55..1b3a5f116 100644 --- a/pkg/spoke/hubclientcert/controller.go +++ b/pkg/spoke/hubclientcert/controller.go @@ -287,7 +287,7 @@ func (c *ClientCertForHubController) createCSR() (string, error) { subject := &pkix.Name{ Organization: []string{ fmt.Sprintf("%s%s", user.SubjectPrefix, c.clusterName), - user.SpokeClustersGroup, + user.ManagedClustersGroup, }, CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName), }