From 8321d01891a9ba0ee6887eed8a992613f72b2a76 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 13 Jun 2023 10:00:37 -0700 Subject: [PATCH] fix logic --- .../fleetresourcehandler_webhook.go | 32 +++-- pkg/webhook/validation/uservalidation.go | 51 +------- pkg/webhook/validation/uservalidation_test.go | 79 ++---------- test/e2e/framework/cluster.go | 35 +++++- test/e2e/webhook_test.go | 113 ++++++++++++++++-- 5 files changed, 157 insertions(+), 153 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index c6918cd3d..1afbd1396 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -21,7 +21,6 @@ const ( // ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources. ValidationPath = "/validate-v1-fleetresourcehandler" groupMatch = `^[^.]*\.(.*)` - crdGVK = "apiextensions.k8s.io/v1, Kind=CustomResourceDefinition" ) // Add registers the webhook for K8s bulit-in object types. @@ -36,23 +35,22 @@ type fleetResourceValidator struct { decoder *admission.Decoder } -func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response { +func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response { var response admission.Response - klog.V(2).InfoS("GVKs", "request GVK", req.Kind.String(), "crd GVK", crdGVK) if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { switch req.Kind.String() { - case crdGVK: - klog.V(2).InfoS("handling CRD resource", "crdGVK", crdGVK) - response = v.handleCRD(ctx, req) + case retrieveCRDGVK(): + klog.V(2).InfoS("handling CRD resource", "crdGVK", retrieveCRDGVK()) + response = v.handleCRD(req) default: - klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook") + klog.V(2).InfoS(fmt.Sprintf("resource with GVK: %s is not monitored by fleet resource validator webhook", req.Kind.String())) response = admission.Allowed("") } } return response } -func (v *fleetResourceValidator) handleCRD(ctx context.Context, req admission.Request) admission.Response { +func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Response { var crd v1.CustomResourceDefinition if req.Operation == admissionv1.Delete { // req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762. @@ -67,20 +65,20 @@ func (v *fleetResourceValidator) handleCRD(ctx context.Context, req admission.Re } } - // Need to check to see if the user is authenticated to do the operation. - if err := validation.ValidateUser(ctx, v.Client, req.UserInfo); err != nil { - return admission.Denied(fmt.Sprintf("failed to validate user %s in groups: %v to modify CRD", req.UserInfo.Username, req.UserInfo.Groups)) - } - klog.V(2).InfoS("successfully validated the user", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) - group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] - if validation.CheckCRDGroup(group) { - return admission.Denied(fmt.Sprintf("user: %s in groups: %v cannot modify fleet CRD %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) + if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(req.UserInfo) { + return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) } - klog.V(2).InfoS("successfully validated the CRD group", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) return admission.Allowed("") } +func retrieveCRDGVK() string { + var crd v1.CustomResourceDefinition + crd.APIVersion = v1.SchemeGroupVersion.Group + "/" + v1.SchemeGroupVersion.Version + crd.Kind = "CustomResourceDefinition" + return crd.GroupVersionKind().String() +} + func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index a95a2d4f9..cc4e7ec01 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -1,58 +1,17 @@ package validation import ( - "context" - "fmt" - "regexp" - authenticationv1 "k8s.io/api/authentication/v1" - "k8s.io/klog/v2" "k8s.io/utils/strings/slices" - "sigs.k8s.io/controller-runtime/pkg/client" - - fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" ) const ( - authenticatedGroup = "system:authenticated" - mastersGroup = "system:masters" - serviceAccountGroup = "system:serviceaccounts" - bootstrapGroup = "system:bootstrappers" - - serviceAccountUser = "system:serviceaccount" + mastersGroup = "system:masters" ) -// TODO: Get valid user names as flag and check to validate those user names. +// TODO:(Arvindthiru) Get valid usernames as flag and allow those usernames. -// ValidateUser checks to see if user is authenticated to make a request to the hub cluster's api-server. -func ValidateUser(ctx context.Context, client client.Client, userInfo authenticationv1.UserInfo) error { - // special case where users belong to the masters group. - if slices.Contains(userInfo.Groups, mastersGroup) { - return nil - } - if slices.Contains(userInfo.Groups, bootstrapGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { - return nil - } - // this ensures all internal service accounts are validated. - if slices.Contains(userInfo.Groups, serviceAccountGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { - match := regexp.MustCompile(serviceAccountUser).FindStringSubmatch(userInfo.Username)[1] - if match != "" { - return nil - } - } - // list all the member clusters - var memberClusterList fleetv1alpha1.MemberClusterList - if err := client.List(ctx, &memberClusterList); err != nil { - klog.V(2).ErrorS(err, "failed to list member clusters") - return err - } - identities := make([]string, len(memberClusterList.Items)) - for i, memberCluster := range memberClusterList.Items { - identities[i] = memberCluster.Spec.Identity.Name - } - // this ensures will allow all member agents are validated. - if slices.Contains(identities, userInfo.Username) && slices.Contains(userInfo.Groups, authenticatedGroup) { - return nil - } - return fmt.Errorf("failed to validate user %s in groups %v", userInfo.Username, userInfo.Groups) +// ValidateUserForCRD checks to see if user is authenticated to make a request to modify fleet CRDs. +func ValidateUserForCRD(userInfo authenticationv1.UserInfo) bool { + return slices.Contains(userInfo.Groups, mastersGroup) } diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index 8cb59db5a..d21d1a883 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/pkg/webhook/validation/uservalidation_test.go @@ -2,24 +2,21 @@ package validation import ( "context" - "errors" "testing" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/stretchr/testify/assert" v1 "k8s.io/api/authentication/v1" - rbacv1 "k8s.io/api/rbac/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/pkg/utils" ) -func TestValidateUser(t *testing.T) { +func TestValidateUserForCRD(t *testing.T) { testCases := map[string]struct { - client client.Client - userInfo v1.UserInfo - wantErr error + client client.Client + userInfo v1.UserInfo + wantResult bool }{ "allow user in system:masters group": { client: &test.MockClient{ @@ -31,67 +28,7 @@ func TestValidateUser(t *testing.T) { Username: "test-user", Groups: []string{"system:masters"}, }, - wantErr: nil, - }, - "allow user in system:bootstrappers group": { - client: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - return nil - }, - }, - userInfo: v1.UserInfo{ - Username: "test-user", - Groups: []string{"system:bootstrappers", "system:authenticated"}, - }, - wantErr: nil, - }, - "allow user in system:serviceaccounts group, has service account prefix in name": { - client: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - return nil - }, - }, - userInfo: v1.UserInfo{ - Username: "service:account:test-user", - Groups: []string{"system:masters", "system:serviceaccounts"}, - }, - wantErr: nil, - }, - "allow member cluster identity": { - client: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - o := list.(*v1alpha1.MemberClusterList) - *o = v1alpha1.MemberClusterList{ - Items: []v1alpha1.MemberCluster{ - { - Spec: v1alpha1.MemberClusterSpec{ - Identity: rbacv1.Subject{ - Name: "member-cluster-identity", - }, - }, - }, - }, - } - return nil - }, - }, - userInfo: v1.UserInfo{ - Username: "member-cluster-identity", - Groups: []string{"system:authenticated"}, - }, - wantErr: nil, - }, - "fail to list member clusters": { - client: &test.MockClient{ - MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - return errors.New("failed to member clusters") - }, - }, - userInfo: v1.UserInfo{ - Username: "test-user", - Groups: []string{"system:authenticated"}, - }, - wantErr: errors.New("failed to member clusters"), + wantResult: true, }, "fail to validate user with invalid username, groups": { client: &test.MockClient{ @@ -103,14 +40,14 @@ func TestValidateUser(t *testing.T) { Username: "test-user", Groups: []string{"test-group"}, }, - wantErr: errors.New("failed to validate user test-user in groups [test-group]"), + wantResult: false, }, } for testName, testCase := range testCases { t.Run(testName, func(t *testing.T) { - gotErr := ValidateUser(context.Background(), testCase.client, testCase.userInfo) - assert.Equal(t, testCase.wantErr, gotErr, utils.TestCaseMsg, testName) + gotResult := ValidateUserForCRD(testCase.userInfo) + assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName) }) } } diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index b36156f3a..6f2e1f1a7 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/dynamic" "k8s.io/client-go/tools/clientcmd" + "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) @@ -22,12 +23,13 @@ var ( // Cluster object defines the required clients based on the kubeconfig of the test cluster. type Cluster struct { - Scheme *runtime.Scheme - KubeClient client.Client - DynamicClient dynamic.Interface - ClusterName string - HubURL string - RestMapper meta.RESTMapper + Scheme *runtime.Scheme + KubeClient client.Client + ImpersonateKubeClient client.Client + DynamicClient dynamic.Interface + ClusterName string + HubURL string + RestMapper meta.RESTMapper } func NewCluster(name string, scheme *runtime.Scheme) *Cluster { @@ -40,12 +42,18 @@ func NewCluster(name string, scheme *runtime.Scheme) *Cluster { // GetClusterClient returns a Cluster client for the cluster. func GetClusterClient(cluster *Cluster) { clusterConfig := GetClientConfig(cluster) + impersonateClusterConfig := GetImpersonateClientConfig(cluster) restConfig, err := clusterConfig.ClientConfig() if err != nil { gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up rest config") } + impersonateRestConfig, err := impersonateClusterConfig.ClientConfig() + if err != nil { + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up impersonate rest config") + } + cluster.KubeClient, err = client.New(restConfig, client.Options{Scheme: cluster.Scheme}) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") @@ -54,6 +62,9 @@ func GetClusterClient(cluster *Cluster) { cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(restConfig, apiutil.WithLazyDiscovery) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Rest Mapper") + + cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme}) + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") } func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig { @@ -63,3 +74,15 @@ func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig { CurrentContext: cluster.ClusterName, }) } + +func GetImpersonateClientConfig(cluster *Cluster) clientcmd.ClientConfig { + return clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, + &clientcmd.ConfigOverrides{ + CurrentContext: cluster.ClusterName, + AuthInfo: api.AuthInfo{ + Impersonate: "test-user", + ImpersonateGroups: []string{"system:authenticated"}, + }, + }) +} diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index e7d1e0278..419f1b536 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -15,7 +15,8 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -461,49 +462,135 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster", rs.Namespace, rs.Name))) }) }) +}) + +var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { + BeforeEach(func() { + By("create cluster role to modify CRDs") + cr := rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-user-cluster-role", + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"apiextensions.k8s.io"}, + Verbs: []string{"*"}, + Resources: []string{"*"}, + }, + }, + } + err := HubCluster.KubeClient.Create(ctx, &cr) + Expect(err).Should(Succeed()) + + By("create cluster role binding for test-user to modify CRD") + crb := rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-user-cluster-role-binding", + }, + Subjects: []rbacv1.Subject{ + { + APIGroup: rbacv1.GroupName, + Kind: "User", + Name: "test-user", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: "test-user-cluster-role", + }, + } + err = HubCluster.KubeClient.Create(ctx, &crb) + Expect(err).Should(Succeed()) + }) + + AfterEach(func() { + By("remove cluster role binding") + crb := rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-user-cluster-role-binding", + }, + } + err := HubCluster.KubeClient.Delete(ctx, &crb) + Expect(err).Should(Succeed()) + + By("remove cluster role") + cr := rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-user-cluster-role", + }, + } + err = HubCluster.KubeClient.Delete(ctx, &cr) + Expect(err).Should(Succeed()) + }) Context("CRD validation webhook", func() { - It("should deny CREATE operation on Fleet CRD", func() { + It("should deny CREATE operation on Fleet CRD for user not in system:masters group", func() { var crd v1.CustomResourceDefinition err := utils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) Expect(err).Should(Succeed()) By("expecting denial of operation CREATE of CRD") - err = HubCluster.KubeClient.Create(ctx, &crd) + err = HubCluster.ImpersonateKubeClient.Create(ctx, &crd) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create CRD call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) - Expect(statusErr.ErrStatus.Message).Should(Equal(fmt.Sprintf( - `admission webhook "fleet.customresourcedefinition.validating" denied the request: user: kubernetes-admin in groups: [system:masters system:authenticated] cannot modify fleet CRD %s`, crd.Name))) + msg := fmt.Sprintf(`admission webhook "fleet.customresourcedefinition.validating" denied the request: failed to validate user: test-user in groups: [system:authenticated] to modify fleet CRD: %s`, crd.Name) + Expect(statusErr.ErrStatus.Message).Should(Equal(msg)) }) - It("should deny UPDATE operation on Fleet CRD", func() { + It("should deny UPDATE operation on Fleet CRD for user not in system:masters group", func() { var crd v1.CustomResourceDefinition err := HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: "memberclusters.fleet.azure.com"}, &crd) Expect(err).Should(Succeed()) - crd.Labels["new-key"] = "new-value" + + By("update labels in CRD") + labels := crd.GetLabels() + labels["test-key"] = "test-value" + crd.SetLabels(labels) By("expecting denial of operation UPDATE of CRD") - err = HubCluster.KubeClient.Update(ctx, &crd) + err = HubCluster.ImpersonateKubeClient.Update(ctx, &crd) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update CRD call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) Expect(statusErr.ErrStatus.Message).Should(Equal(fmt.Sprintf( - `admission webhook "fleet.customresourcedefinition.validating" denied the request: user: kubernetes-admin in groups: [system:masters system:authenticated] cannot modify fleet CRD %s`, crd.Name))) + `admission webhook "fleet.customresourcedefinition.validating" denied the request: failed to validate user: test-user in groups: [system:authenticated] to modify fleet CRD: %s`, crd.Name))) }) - It("should deny DELETE operation on Fleet CRD", func() { + It("should deny DELETE operation on Fleet CRD for user not in system:masters group", func() { crd := v1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ Name: "works.multicluster.x-k8s.io", }, } By("expecting denial of operation Delete of CRD") - err := HubCluster.KubeClient.Delete(ctx, &crd) + err := HubCluster.ImpersonateKubeClient.Delete(ctx, &crd) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete CRD call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) - Expect(statusErr.ErrStatus.Message).Should(Equal(fmt.Sprintf(`admission webhook "fleet.customresourcedefinition.validating" denied the request: user: kubernetes-admin in groups: [system:masters system:authenticated] cannot modify fleet CRD %s`, crd.Name))) + Expect(statusErr.ErrStatus.Message).Should(Equal(fmt.Sprintf(`admission webhook "fleet.customresourcedefinition.validating" denied the request: failed to validate user: test-user in groups: [system:authenticated] to modify fleet CRD: %s`, crd.Name))) + }) + + It("should allow UPDATE operation on Fleet CRDs if user in system:masters group", func() { + var crd v1.CustomResourceDefinition + err := HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: "memberclusters.fleet.azure.com"}, &crd) + Expect(err).Should(Succeed()) + + By("update labels in CRD") + labels := crd.GetLabels() + labels["test-key"] = "test-value" + crd.SetLabels(labels) + + By("expecting denial of operation UPDATE of CRD") + // The user associated with KubeClient is kubernetes-admin in groups: [system:masters, system:authenticated] + err = HubCluster.KubeClient.Update(ctx, &crd) + Expect(err).To(BeNil()) + + By("remove new label added for test") + labels = crd.GetLabels() + delete(labels, "test-key") + crd.SetLabels(labels) }) - It("should allow CREATE OPERATION on Other CRDs", func() { + It("should allow CREATE operation on Other CRDs", func() { var crd v1.CustomResourceDefinition err := utils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd) Expect(err).Should(Succeed())