From ae01d248e2947e78edb85c5a22cc1acabcd9f089 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 23 May 2023 12:50:18 -0700 Subject: [PATCH 01/22] Webhook for CRDs --- .../customresourcedefinition_webhook.go | 59 +++++++++++++++++++ pkg/webhook/validation/crdvalidation.go | 11 ++++ pkg/webhook/validation/uservalidation.go | 20 +++++++ 3 files changed, 90 insertions(+) create mode 100644 pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go create mode 100644 pkg/webhook/validation/crdvalidation.go create mode 100644 pkg/webhook/validation/uservalidation.go diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go new file mode 100644 index 000000000..5810b2475 --- /dev/null +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -0,0 +1,59 @@ +package customresourcedefinition + +import ( + "context" + "fmt" + "net/http" + "regexp" + + admissionv1 "k8s.io/api/admission/v1" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/webhook/validation" +) + +const ( + // ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources. + ValidationPath = "/validate-v1-customresourcedefinition" + groupMatch = `^[^.]*\.(.*)` +) + +// Add registers the webhook for K8s bulit-in object types. +func Add(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &customResourceDefintionValidator{Client: mgr.GetClient()}}) + return nil +} + +type customResourceDefintionValidator struct { + Client client.Client + decoder *admission.Decoder +} + +func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { + crd := &v1.CustomResourceDefinition{} + err := v.decoder.Decode(req, crd) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] + if !validation.ValidateObjectGroup(group) { + return admission.Denied(fmt.Sprintf("failed to validate group for CRD %s", crd.Name)) + } + // Need to check to see if the user is authorized to do the operation. + if !validation.ValidateUserGroups(req.UserInfo.Groups) { + return admission.Denied(fmt.Sprintf("failed to validate user %s to modify CRD", req.UserInfo.Username)) + } + } + return admission.Allowed("") +} + +func (v *customResourceDefintionValidator) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/validation/crdvalidation.go b/pkg/webhook/validation/crdvalidation.go new file mode 100644 index 000000000..afdb038b5 --- /dev/null +++ b/pkg/webhook/validation/crdvalidation.go @@ -0,0 +1,11 @@ +package validation + +import "k8s.io/utils/strings/slices" + +var ( + validObjectGroups = []string{"networking.fleet.azure.com", "fleet.azure.com", "multicluster.x-k8s.io"} +) + +func ValidateObjectGroup(group string) bool { + return slices.Contains(validObjectGroups, group) +} diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go new file mode 100644 index 000000000..ff1f9cdec --- /dev/null +++ b/pkg/webhook/validation/uservalidation.go @@ -0,0 +1,20 @@ +package validation + +import ( + "k8s.io/utils/strings/slices" +) + +const ( + authenticatedGroup = "system:authenticated" + mastersGroup = "system:masters" +) + +// TODO: Get valid user names as flag and check to validate those user names. + +// ValidateUserGroups checks to see if user is authorized to make a request to the hub cluster's api-server. +func ValidateUserGroups(groups []string) bool { + if slices.Contains(groups, authenticatedGroup) || slices.Contains(groups, mastersGroup) { + return true + } + return false +} From 4a9844a12b41d72186c9eb24184423a805fd16aa Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 24 May 2023 10:50:37 -0700 Subject: [PATCH 02/22] working webhook --- pkg/webhook/add_customresourcedefinition.go | 10 +++++++ .../customresourcedefinition_webhook.go | 8 +++--- pkg/webhook/validation/crdvalidation.go | 3 ++- pkg/webhook/webhook.go | 27 +++++++++++++++++++ 4 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 pkg/webhook/add_customresourcedefinition.go diff --git a/pkg/webhook/add_customresourcedefinition.go b/pkg/webhook/add_customresourcedefinition.go new file mode 100644 index 000000000..e46ef5be7 --- /dev/null +++ b/pkg/webhook/add_customresourcedefinition.go @@ -0,0 +1,10 @@ +package webhook + +import ( + "go.goms.io/fleet/pkg/webhook/customresourcedefinition" +) + +func init() { + // AddToManagerFuncs is a list of functions to create webhook and add them to a manager. + AddToManagerFuncs = append(AddToManagerFuncs, customresourcedefinition.Add) +} diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go index 5810b2475..97ee396cc 100644 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -41,14 +41,14 @@ func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admis if err != nil { return admission.Errored(http.StatusBadRequest, err) } - group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] - if !validation.ValidateObjectGroup(group) { - return admission.Denied(fmt.Sprintf("failed to validate group for CRD %s", crd.Name)) - } // Need to check to see if the user is authorized to do the operation. if !validation.ValidateUserGroups(req.UserInfo.Groups) { return admission.Denied(fmt.Sprintf("failed to validate user %s to modify CRD", req.UserInfo.Username)) } + group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] + if validation.CheckCRDGroup(group) { + return admission.Denied(fmt.Sprintf("cannot modify fleet CRDs %s", crd.Name)) + } } return admission.Allowed("") } diff --git a/pkg/webhook/validation/crdvalidation.go b/pkg/webhook/validation/crdvalidation.go index afdb038b5..e1fc276c3 100644 --- a/pkg/webhook/validation/crdvalidation.go +++ b/pkg/webhook/validation/crdvalidation.go @@ -6,6 +6,7 @@ var ( validObjectGroups = []string{"networking.fleet.azure.com", "fleet.azure.com", "multicluster.x-k8s.io"} ) -func ValidateObjectGroup(group string) bool { +// CheckCRDGroup checks to see if the input CRD group is a fleet CRD group. +func CheckCRDGroup(group string) bool { return slices.Contains(validObjectGroups, group) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index ea38fa539..da757a718 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -23,6 +23,7 @@ import ( admv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" @@ -33,6 +34,7 @@ import ( fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/cmd/hubagent/options" "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" + "go.goms.io/fleet/pkg/webhook/customresourcedefinition" "go.goms.io/fleet/pkg/webhook/pod" "go.goms.io/fleet/pkg/webhook/replicaset" ) @@ -178,6 +180,28 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { }, }, }, + { + Name: "fleet.customresourcedefinition.validating", + ClientConfig: w.createClientConfig(v1.CustomResourceDefinition{}), + FailurePolicy: &failPolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: []string{"v1", "v1beta1"}, + Rules: []admv1.RuleWithOperations{ + { + Operations: []admv1.OperationType{ + admv1.Create, + admv1.Update, + admv1.Delete, + }, + Rule: admv1.Rule{ + APIGroups: []string{"apiextensions.k8s.io"}, + APIVersions: []string{"v1"}, + Resources: []string{"customresourcedefinitions"}, + Scope: &clusterScope, + }, + }, + }, + }, }, } @@ -225,6 +249,9 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC case appsv1.ReplicaSet: serviceEndpoint = w.serviceURL + replicaset.ValidationPath serviceRef.Path = pointer.String(replicaset.ValidationPath) + case v1.CustomResourceDefinition: + serviceEndpoint = w.serviceURL + customresourcedefinition.ValidationPath + serviceRef.Path = pointer.String(customresourcedefinition.ValidationPath) } config := admv1.WebhookClientConfig{ From 6aa1e01b7da9e790c925f84c3549fcb6e897ec57 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 24 May 2023 12:26:49 -0700 Subject: [PATCH 03/22] improve deny logs --- .../customresourcedefinition_webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go index 97ee396cc..ce0d41e88 100644 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -43,11 +43,11 @@ func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admis } // Need to check to see if the user is authorized to do the operation. if !validation.ValidateUserGroups(req.UserInfo.Groups) { - return admission.Denied(fmt.Sprintf("failed to validate user %s to modify CRD", req.UserInfo.Username)) + return admission.Denied(fmt.Sprintf("failed to authorize user %s in groups: %v to modify CRD", req.UserInfo.Username, req.UserInfo.Groups)) } group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] if validation.CheckCRDGroup(group) { - return admission.Denied(fmt.Sprintf("cannot modify fleet CRDs %s", crd.Name)) + return admission.Denied(fmt.Sprintf("user: %s in groups: %v cannot modify fleet CRD %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) } } return admission.Allowed("") From cae2fc96770e2396d79497d546409d8065358c8e Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 25 May 2023 09:50:31 -0700 Subject: [PATCH 04/22] partial E2E --- pkg/utils/test_util.go | 7 +-- .../customresourcedefinition_webhook.go | 12 +++- test/e2e/webhook_test.go | 60 ++++++++++++++++++- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/pkg/utils/test_util.go b/pkg/utils/test_util.go index 4adb45093..877dcbf48 100644 --- a/pkg/utils/test_util.go +++ b/pkg/utils/test_util.go @@ -10,16 +10,15 @@ import ( "os" "strings" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/apimachinery/pkg/util/yaml" - "k8s.io/client-go/kubernetes/scheme" - "github.com/onsi/gomega/format" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" ) diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go index ce0d41e88..cf4c4d59e 100644 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -37,9 +37,15 @@ type customResourceDefintionValidator struct { func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admission.Request) admission.Response { if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { crd := &v1.CustomResourceDefinition{} - err := v.decoder.Decode(req, crd) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) + if req.Operation == admissionv1.Delete { + // req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762 + if err := v.decoder.DecodeRaw(req.OldObject, crd); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + } else { + if err := v.decoder.Decode(req, crd); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } } // Need to check to see if the user is authorized to do the operation. if !validation.ValidateUserGroups(req.UserInfo.Groups) { diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index db771032c..0895efdc8 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -6,7 +6,7 @@ Licensed under the MIT license. package e2e import ( - errors "errors" + "errors" "fmt" "reflect" "regexp" @@ -15,13 +15,16 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/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" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/pkg/utils" + pkgutils "go.goms.io/fleet/pkg/utils" testUtils "go.goms.io/fleet/test/e2e/utils" ) @@ -459,4 +462,59 @@ 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))) }) }) + + Context("CRD validation webhook", func() { + It("should deny CREATE operation on Fleet CRD", func() { + var crd v1.CustomResourceDefinition + err := pkgutils.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) + 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{}))) + fmt.Println(statusErr.ErrStatus.Message) + 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))) + }) + + It("should deny UPDATE operation on Fleet CRD", 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("expecting denial of operation UPDATE of CRD") + err = HubCluster.KubeClient.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))) + }) + + It("should deny DELETE operation on Fleet CRD", func() { + crd := v1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "works.multicluster.x-k8s.io", + }, + } + err := HubCluster.KubeClient.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{}))) + fmt.Println(statusErr.ErrStatus.Message) + fmt.Println(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: user: kubernetes-admin in groups: [system:masters system:authenticated] cannot modify fleet CRD %s`, crd.Name))) + }) + + It("should allow CREATE OPERATION on Other CRDs", func() { + var crd v1.CustomResourceDefinition + err := pkgutils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd) + Expect(err).Should(Succeed()) + By("expecting error to be nil") + err = HubCluster.KubeClient.Create(ctx, &crd) + Expect(err).To(BeNil()) + By("delete clone set CRD") + err = HubCluster.KubeClient.Delete(ctx, &crd) + Expect(err).To(BeNil()) + }) + }) }) From 79c6fc6f41c2ee8082fece6b2f5977ad0d0ca32d Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 25 May 2023 13:11:30 -0700 Subject: [PATCH 05/22] not working E2E test to not allow user --- test/e2e/framework/cluster.go | 45 ++++++++++++++++++------- test/e2e/webhook_test.go | 62 +++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 11 deletions(-) diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index b36156f3a..ed25bb231 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -5,6 +5,7 @@ Licensed under the MIT license. package framework import ( + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "os" "github.com/onsi/gomega" @@ -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,19 +42,28 @@ 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() + validRestConfig, err := clusterConfig.ClientConfig() if err != nil { - gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up rest config") + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up valid rest config") } - cluster.KubeClient, err = client.New(restConfig, client.Options{Scheme: cluster.Scheme}) + 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(validRestConfig, client.Options{Scheme: cluster.Scheme}) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") - cluster.DynamicClient, err = dynamic.NewForConfig(restConfig) + cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme}) + gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Impersonate Kube Client") + + cluster.DynamicClient, err = dynamic.NewForConfig(validRestConfig) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Dynamic Client") - cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(restConfig, apiutil.WithLazyDiscovery) + cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(validRestConfig, apiutil.WithLazyDiscovery) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Rest Mapper") } @@ -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: clientcmdapi.AuthInfo{ + Impersonate: "bad-user", + ImpersonateGroups: []string{"user:unauthenticated"}, + }, + }) +} diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 0895efdc8..67d4c002e 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -15,6 +15,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/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" @@ -464,6 +465,63 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }) Context("CRD validation webhook", func() { + It("should deny user if they are not part of system:masters/system:authenticated", func() { + var crd v1.CustomResourceDefinition + err := pkgutils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) + Expect(err).Should(Succeed()) + + By("create cluster role for bad-user to modify CRDs") + cr := rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-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 admin cluster role binding for bad-user") + crb := rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-user-cluster-role-binding", + }, + Subjects: []rbacv1.Subject{ + { + APIGroup: rbacv1.GroupName, + Kind: "User", + Name: "bad-user", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: "bad-user-cluster-role", + }, + } + err = HubCluster.KubeClient.Create(ctx, &crb) + Expect(err).Should(Succeed()) + + By("expecting denial of operation CREATE of CRD by un-authenticated user") + 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{}))) + fmt.Println(statusErr.ErrStatus.Message) + + By("delete cluster role binding for bad-user") + err = HubCluster.KubeClient.Delete(ctx, &crb) + Expect(err).Should(Succeed()) + + By("delete cluster role for bad-user") + err = HubCluster.KubeClient.Delete(ctx, &cr) + Expect(err).Should(Succeed()) + }) + It("should deny CREATE operation on Fleet CRD", func() { var crd v1.CustomResourceDefinition err := pkgutils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) @@ -483,6 +541,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { err := HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: "memberclusters.fleet.azure.com"}, &crd) Expect(err).Should(Succeed()) crd.Labels["new-key"] = "new-value" + By("expecting denial of operation UPDATE of CRD") err = HubCluster.KubeClient.Update(ctx, &crd) var statusErr *k8sErrors.StatusError @@ -497,6 +556,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Name: "works.multicluster.x-k8s.io", }, } + By("expecting denial of operation Delete of CRD") err := HubCluster.KubeClient.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{}))) @@ -509,9 +569,11 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { var crd v1.CustomResourceDefinition err := pkgutils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd) Expect(err).Should(Succeed()) + By("expecting error to be nil") err = HubCluster.KubeClient.Create(ctx, &crd) Expect(err).To(BeNil()) + By("delete clone set CRD") err = HubCluster.KubeClient.Delete(ctx, &crd) Expect(err).To(BeNil()) From 2fa5e81167727f9a2a016be78cb3dd8df9629c74 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 25 May 2023 17:18:51 -0700 Subject: [PATCH 06/22] remove user group e2e --- test/e2e/framework/cluster.go | 45 +++++++------------------- test/e2e/webhook_test.go | 61 ----------------------------------- 2 files changed, 11 insertions(+), 95 deletions(-) diff --git a/test/e2e/framework/cluster.go b/test/e2e/framework/cluster.go index ed25bb231..b36156f3a 100644 --- a/test/e2e/framework/cluster.go +++ b/test/e2e/framework/cluster.go @@ -5,7 +5,6 @@ Licensed under the MIT license. package framework import ( - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "os" "github.com/onsi/gomega" @@ -23,13 +22,12 @@ var ( // Cluster object defines the required clients based on the kubeconfig of the test cluster. type Cluster struct { - Scheme *runtime.Scheme - KubeClient client.Client - ImpersonateKubeClient client.Client - DynamicClient dynamic.Interface - ClusterName string - HubURL string - RestMapper meta.RESTMapper + Scheme *runtime.Scheme + KubeClient client.Client + DynamicClient dynamic.Interface + ClusterName string + HubURL string + RestMapper meta.RESTMapper } func NewCluster(name string, scheme *runtime.Scheme) *Cluster { @@ -42,28 +40,19 @@ 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) - validRestConfig, err := clusterConfig.ClientConfig() + restConfig, err := clusterConfig.ClientConfig() if err != nil { - gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up valid rest config") + 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(validRestConfig, client.Options{Scheme: cluster.Scheme}) + cluster.KubeClient, err = client.New(restConfig, client.Options{Scheme: cluster.Scheme}) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") - cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme}) - gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Impersonate Kube Client") - - cluster.DynamicClient, err = dynamic.NewForConfig(validRestConfig) + cluster.DynamicClient, err = dynamic.NewForConfig(restConfig) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Dynamic Client") - cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(validRestConfig, apiutil.WithLazyDiscovery) + cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(restConfig, apiutil.WithLazyDiscovery) gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Rest Mapper") } @@ -74,15 +63,3 @@ 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: clientcmdapi.AuthInfo{ - Impersonate: "bad-user", - ImpersonateGroups: []string{"user:unauthenticated"}, - }, - }) -} diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 67d4c002e..858ba2991 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -15,7 +15,6 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/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" @@ -465,63 +464,6 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }) Context("CRD validation webhook", func() { - It("should deny user if they are not part of system:masters/system:authenticated", func() { - var crd v1.CustomResourceDefinition - err := pkgutils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) - Expect(err).Should(Succeed()) - - By("create cluster role for bad-user to modify CRDs") - cr := rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bad-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 admin cluster role binding for bad-user") - crb := rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bad-user-cluster-role-binding", - }, - Subjects: []rbacv1.Subject{ - { - APIGroup: rbacv1.GroupName, - Kind: "User", - Name: "bad-user", - }, - }, - RoleRef: rbacv1.RoleRef{ - APIGroup: rbacv1.GroupName, - Kind: "ClusterRole", - Name: "bad-user-cluster-role", - }, - } - err = HubCluster.KubeClient.Create(ctx, &crb) - Expect(err).Should(Succeed()) - - By("expecting denial of operation CREATE of CRD by un-authenticated user") - 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{}))) - fmt.Println(statusErr.ErrStatus.Message) - - By("delete cluster role binding for bad-user") - err = HubCluster.KubeClient.Delete(ctx, &crb) - Expect(err).Should(Succeed()) - - By("delete cluster role for bad-user") - err = HubCluster.KubeClient.Delete(ctx, &cr) - Expect(err).Should(Succeed()) - }) - It("should deny CREATE operation on Fleet CRD", func() { var crd v1.CustomResourceDefinition err := pkgutils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) @@ -531,7 +473,6 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { err = HubCluster.KubeClient.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{}))) - fmt.Println(statusErr.ErrStatus.Message) 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))) }) @@ -560,8 +501,6 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { err := HubCluster.KubeClient.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{}))) - fmt.Println(statusErr.ErrStatus.Message) - fmt.Println(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: user: kubernetes-admin in groups: [system:masters system:authenticated] cannot modify fleet CRD %s`, crd.Name))) }) From bd628ef63a68119f063a983db0102edf25c91386 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 25 May 2023 17:23:36 -0700 Subject: [PATCH 07/22] make reviewable --- test/e2e/webhook_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 858ba2991..e7d1e0278 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -15,7 +15,7 @@ import ( . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + 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" @@ -24,7 +24,6 @@ import ( fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/pkg/utils" - pkgutils "go.goms.io/fleet/pkg/utils" testUtils "go.goms.io/fleet/test/e2e/utils" ) @@ -466,7 +465,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Context("CRD validation webhook", func() { It("should deny CREATE operation on Fleet CRD", func() { var crd v1.CustomResourceDefinition - err := pkgutils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd) + 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") @@ -506,7 +505,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { It("should allow CREATE OPERATION on Other CRDs", func() { var crd v1.CustomResourceDefinition - err := pkgutils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd) + err := utils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd) Expect(err).Should(Succeed()) By("expecting error to be nil") From a0cc816107bea318a34d4524edbdaee1a9dc4037 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 25 May 2023 17:36:04 -0700 Subject: [PATCH 08/22] add period on comment --- .../customresourcedefinition_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go index cf4c4d59e..18a0c34b9 100644 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -38,7 +38,7 @@ func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admis if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { crd := &v1.CustomResourceDefinition{} if req.Operation == admissionv1.Delete { - // req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762 + // req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762. if err := v.decoder.DecodeRaw(req.OldObject, crd); err != nil { return admission.Errored(http.StatusBadRequest, err) } From d7e4f03699c7e471c04a819e22c1b2814740fdab Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 30 May 2023 17:38:51 -0700 Subject: [PATCH 09/22] expand user validation --- .../customresourcedefinition_webhook.go | 2 +- pkg/webhook/validation/uservalidation.go | 37 ++++++++++++++++--- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go index 18a0c34b9..3067c72a5 100644 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go @@ -48,7 +48,7 @@ func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admis } } // Need to check to see if the user is authorized to do the operation. - if !validation.ValidateUserGroups(req.UserInfo.Groups) { + if !validation.ValidateUser(ctx, v.Client, req.UserInfo) { return admission.Denied(fmt.Sprintf("failed to authorize user %s in groups: %v to modify CRD", req.UserInfo.Username, req.UserInfo.Groups)) } group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index ff1f9cdec..9804699ef 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -1,20 +1,47 @@ package validation import ( + "context" + "regexp" + + authenticationv1 "k8s.io/api/authentication/v1" "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" + authenticatedGroup = "system:authenticated" + mastersGroup = "system:masters" + serviceAccountGroup = "system:serviceaccounts" + bootstrapGroup = "system:bootstrappers" + + serviceAccountUser = "system:serviceaccount" ) // TODO: Get valid user names as flag and check to validate those user names. -// ValidateUserGroups checks to see if user is authorized to make a request to the hub cluster's api-server. -func ValidateUserGroups(groups []string) bool { - if slices.Contains(groups, authenticatedGroup) || slices.Contains(groups, mastersGroup) { +// 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) bool { + if slices.Contains(userInfo.Groups, mastersGroup) { return true } + if slices.Contains(userInfo.Groups, bootstrapGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { + return true + } + // This ensures all internal service accounts are validated + if slices.Contains(userInfo.Groups, serviceAccountGroup) { + match := regexp.MustCompile(serviceAccountUser).FindStringSubmatch(userInfo.Username)[1] + if match != "" { + return true + } + } + // list all the member clusters + var memberClusterList fleetv1alpha1.MemberClusterList + if err := client.List(ctx, &memberClusterList); err != nil { + // log error + return false + } return false } From 8b9c4657099568eb628a27b11dc2bcf60f4da955 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Jun 2023 12:34:58 -0700 Subject: [PATCH 10/22] generic handler --- pkg/webhook/add_customresourcedefinition.go | 6 +- .../customresourcedefinition_webhook.go | 65 -------------- .../fleetresourcehandler_webhook.go | 90 +++++++++++++++++++ pkg/webhook/validation/uservalidation.go | 14 ++- pkg/webhook/webhook.go | 6 +- 5 files changed, 107 insertions(+), 74 deletions(-) delete mode 100644 pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go create mode 100644 pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go diff --git a/pkg/webhook/add_customresourcedefinition.go b/pkg/webhook/add_customresourcedefinition.go index e46ef5be7..3172bcc19 100644 --- a/pkg/webhook/add_customresourcedefinition.go +++ b/pkg/webhook/add_customresourcedefinition.go @@ -1,10 +1,8 @@ package webhook -import ( - "go.goms.io/fleet/pkg/webhook/customresourcedefinition" -) +import "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" func init() { // AddToManagerFuncs is a list of functions to create webhook and add them to a manager. - AddToManagerFuncs = append(AddToManagerFuncs, customresourcedefinition.Add) + AddToManagerFuncs = append(AddToManagerFuncs, fleetresourcehandler.Add) } diff --git a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go b/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go deleted file mode 100644 index 3067c72a5..000000000 --- a/pkg/webhook/customresourcedefinition/customresourcedefinition_webhook.go +++ /dev/null @@ -1,65 +0,0 @@ -package customresourcedefinition - -import ( - "context" - "fmt" - "net/http" - "regexp" - - admissionv1 "k8s.io/api/admission/v1" - v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "go.goms.io/fleet/pkg/webhook/validation" -) - -const ( - // ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources. - ValidationPath = "/validate-v1-customresourcedefinition" - groupMatch = `^[^.]*\.(.*)` -) - -// Add registers the webhook for K8s bulit-in object types. -func Add(mgr manager.Manager) error { - hookServer := mgr.GetWebhookServer() - hookServer.Register(ValidationPath, &webhook.Admission{Handler: &customResourceDefintionValidator{Client: mgr.GetClient()}}) - return nil -} - -type customResourceDefintionValidator struct { - Client client.Client - decoder *admission.Decoder -} - -func (v *customResourceDefintionValidator) Handle(ctx context.Context, req admission.Request) admission.Response { - if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { - crd := &v1.CustomResourceDefinition{} - if req.Operation == admissionv1.Delete { - // req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762. - if err := v.decoder.DecodeRaw(req.OldObject, crd); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - } else { - if err := v.decoder.Decode(req, crd); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - } - // Need to check to see if the user is authorized to do the operation. - if !validation.ValidateUser(ctx, v.Client, req.UserInfo) { - return admission.Denied(fmt.Sprintf("failed to authorize user %s in groups: %v to modify CRD", req.UserInfo.Username, 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)) - } - } - return admission.Allowed("") -} - -func (v *customResourceDefintionValidator) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil -} diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go new file mode 100644 index 000000000..4b7b3344d --- /dev/null +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -0,0 +1,90 @@ +package fleetresourcehandler + +import ( + "context" + "fmt" + "net/http" + "regexp" + + admissionv1 "k8s.io/api/admission/v1" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/webhook/validation" +) + +const ( + // ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources. + ValidationPath = "/validate-v1-fleetresourcehandler" + groupMatch = `^[^.]*\.(.*)` +) + +// Add registers the webhook for K8s bulit-in object types. +func Add(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &fleetResourceValidator{Client: mgr.GetClient()}}) + return nil +} + +type fleetResourceValidator struct { + Client client.Client + decoder *admission.Decoder +} + +func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + var response admission.Response + if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { + switch req.Kind.String() { + case crdGroupVersionKind(): + response = v.handleCRD(ctx, req) + default: + // we don't care about these resources. + response = admission.Allowed("") + } + } + return response +} + +func (v *fleetResourceValidator) handleCRD(ctx context.Context, 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. + if err := v.decoder.DecodeRaw(req.OldObject, &crd); err != nil { + klog.ErrorS(err, "failed to decode old request object for delete operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Errored(http.StatusBadRequest, err) + } + } else { + if err := v.decoder.Decode(req, &crd); err != nil { + klog.ErrorS(err, "failed to decode request object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Errored(http.StatusBadRequest, err) + } + } + + // Need to check to see if the user is authenticated to do the operation. + if !validation.ValidateUser(ctx, v.Client, req.UserInfo) { + 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)) + } + klog.V(2).InfoS("successfully validated the CRD group", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Allowed("") +} + +func crdGroupVersionKind() string { + var crd v1.CustomResourceDefinition + crdObjectKind := crd.GetObjectKind() + return crdObjectKind.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 9804699ef..18bb5eb7c 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -5,6 +5,7 @@ import ( "regexp" authenticationv1 "k8s.io/api/authentication/v1" + "k8s.io/klog/v2" "k8s.io/utils/strings/slices" "sigs.k8s.io/controller-runtime/pkg/client" @@ -24,13 +25,14 @@ const ( // 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) bool { + // special case where users belong to the masters group. if slices.Contains(userInfo.Groups, mastersGroup) { return true } if slices.Contains(userInfo.Groups, bootstrapGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { return true } - // This ensures all internal service accounts are validated + // this ensures all internal service accounts are validated. if slices.Contains(userInfo.Groups, serviceAccountGroup) { match := regexp.MustCompile(serviceAccountUser).FindStringSubmatch(userInfo.Username)[1] if match != "" { @@ -40,8 +42,16 @@ func ValidateUser(ctx context.Context, client client.Client, userInfo authentica // list all the member clusters var memberClusterList fleetv1alpha1.MemberClusterList if err := client.List(ctx, &memberClusterList); err != nil { - // log error + klog.V(2).ErrorS(err, "failed to list member clusters") return false } + var identities []string + for _, memberCluster := range memberClusterList.Items { + identities = append(identities, memberCluster.Spec.Identity.Name) + } + // this ensures will allow all member agents are validated. + if slices.Contains(identities, userInfo.Username) { + return true + } return false } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index da757a718..beb09ad6d 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -34,7 +34,7 @@ import ( fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/cmd/hubagent/options" "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" - "go.goms.io/fleet/pkg/webhook/customresourcedefinition" + "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" "go.goms.io/fleet/pkg/webhook/pod" "go.goms.io/fleet/pkg/webhook/replicaset" ) @@ -250,8 +250,8 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC serviceEndpoint = w.serviceURL + replicaset.ValidationPath serviceRef.Path = pointer.String(replicaset.ValidationPath) case v1.CustomResourceDefinition: - serviceEndpoint = w.serviceURL + customresourcedefinition.ValidationPath - serviceRef.Path = pointer.String(customresourcedefinition.ValidationPath) + serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath + serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) } config := admv1.WebhookClientConfig{ From d263567d6813c2717284f34ffe8ec76c6dd8bf75 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 2 Jun 2023 16:08:43 -0700 Subject: [PATCH 11/22] crd GVK check --- .../fleetresourcehandler_webhook.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 4b7b3344d..4045f51ed 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -21,6 +21,7 @@ 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. @@ -37,12 +38,14 @@ type fleetResourceValidator struct { func (v *fleetResourceValidator) Handle(ctx 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 crdGroupVersionKind(): + case crdGVK: + klog.V(2).InfoS("handling CRD resource", "crdGVK", crdGVK) response = v.handleCRD(ctx, req) default: - // we don't care about these resources. + klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook") response = admission.Allowed("") } } @@ -78,12 +81,6 @@ func (v *fleetResourceValidator) handleCRD(ctx context.Context, req admission.Re return admission.Allowed("") } -func crdGroupVersionKind() string { - var crd v1.CustomResourceDefinition - crdObjectKind := crd.GetObjectKind() - return crdObjectKind.GroupVersionKind().String() -} - func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil From 1670f533e4771e4e7a127f06e7aca4e96bb99ee2 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 6 Jun 2023 11:18:45 -0700 Subject: [PATCH 12/22] check authenticated group --- pkg/webhook/validation/uservalidation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 18bb5eb7c..4a625a3f4 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -33,7 +33,7 @@ func ValidateUser(ctx context.Context, client client.Client, userInfo authentica return true } // this ensures all internal service accounts are validated. - if slices.Contains(userInfo.Groups, serviceAccountGroup) { + if slices.Contains(userInfo.Groups, serviceAccountGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { match := regexp.MustCompile(serviceAccountUser).FindStringSubmatch(userInfo.Username)[1] if match != "" { return true From 9f4425fe0a8f42ba6f5827eba3007907f95510ba Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 6 Jun 2023 17:33:18 -0700 Subject: [PATCH 13/22] add UTs --- .../fleetresourcehandler_webhook.go | 2 +- pkg/webhook/validation/uservalidation.go | 23 ++-- pkg/webhook/validation/uservalidation_test.go | 116 ++++++++++++++++++ 3 files changed, 129 insertions(+), 12 deletions(-) create mode 100644 pkg/webhook/validation/uservalidation_test.go diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 4045f51ed..c6918cd3d 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -68,7 +68,7 @@ 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 !validation.ValidateUser(ctx, v.Client, req.UserInfo) { + 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) diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 4a625a3f4..a95a2d4f9 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -2,6 +2,7 @@ package validation import ( "context" + "fmt" "regexp" authenticationv1 "k8s.io/api/authentication/v1" @@ -24,34 +25,34 @@ const ( // TODO: Get valid user names as flag and check to validate those user names. // 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) bool { +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 true + return nil } if slices.Contains(userInfo.Groups, bootstrapGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) { - return true + 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 true + 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 false + return err } - var identities []string - for _, memberCluster := range memberClusterList.Items { - identities = append(identities, memberCluster.Spec.Identity.Name) + 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) { - return true + if slices.Contains(identities, userInfo.Username) && slices.Contains(userInfo.Groups, authenticatedGroup) { + return nil } - return false + return fmt.Errorf("failed to validate user %s in groups %v", userInfo.Username, userInfo.Groups) } diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go new file mode 100644 index 000000000..8cb59db5a --- /dev/null +++ b/pkg/webhook/validation/uservalidation_test.go @@ -0,0 +1,116 @@ +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) { + testCases := map[string]struct { + client client.Client + userInfo v1.UserInfo + wantErr error + }{ + "allow user in system:masters 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: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"), + }, + "fail to validate user with invalid username, groups": { + 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{"test-group"}, + }, + wantErr: errors.New("failed to validate user test-user in groups [test-group]"), + }, + } + + 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) + }) + } +} From 0f2c1842de65ed93f68a167c7d0139947e029654 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 7 Jun 2023 09:59:26 -0700 Subject: [PATCH 14/22] lint fix --- pkg/utils/test_util.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/utils/test_util.go b/pkg/utils/test_util.go index 877dcbf48..2b9c8798e 100644 --- a/pkg/utils/test_util.go +++ b/pkg/utils/test_util.go @@ -62,7 +62,6 @@ func GetObjectFromManifest(relativeFilePath string, obj runtime.Object) error { if err != nil { return err } - return GetObjectFromRawExtension(fileRaw, obj) } From 4b4794b1e24b247fa20f920f2a6ab489a85ba964 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 13 Jun 2023 10:00:37 -0700 Subject: [PATCH 15/22] 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()) From fd55d1946ccf9800ee534428dfc64b48780ff0c7 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 13 Jun 2023 13:40:43 -0700 Subject: [PATCH 16/22] fix log --- .../fleetresourcehandler/fleetresourcehandler_webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 1afbd1396..8eafe2258 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -40,10 +40,10 @@ func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { switch req.Kind.String() { case retrieveCRDGVK(): - klog.V(2).InfoS("handling CRD resource", "crdGVK", retrieveCRDGVK()) + klog.V(2).InfoS("handling CRD resource", "GVK", retrieveCRDGVK()) response = v.handleCRD(req) default: - klog.V(2).InfoS(fmt.Sprintf("resource with GVK: %s is not monitored by fleet resource validator webhook", req.Kind.String())) + klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String()) response = admission.Allowed("") } } From 2eb3d9a0cc6c9447ea251eb6abfebe0e742bfe82 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 13 Jun 2023 13:48:30 -0700 Subject: [PATCH 17/22] lint fix --- test/e2e/webhook_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 419f1b536..6019d00f9 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -16,7 +16,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + 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" From 36608e6733e9f98717cc097a20a382246e7250bd Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 14 Jun 2023 17:38:52 -0700 Subject: [PATCH 18/22] address comments --- .../fleetresourcehandler_webhook.go | 17 ++++--- pkg/webhook/webhook.go | 39 +++++++++------- test/e2e/webhook_test.go | 45 +++++++++++-------- 3 files changed, 61 insertions(+), 40 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 8eafe2258..7c6db1833 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -8,6 +8,7 @@ import ( admissionv1 "k8s.io/api/admission/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -21,6 +22,7 @@ const ( // ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources. ValidationPath = "/validate-v1-fleetresourcehandler" groupMatch = `^[^.]*\.(.*)` + crdKind = "CustomResourceDefinition" ) // Add registers the webhook for K8s bulit-in object types. @@ -38,7 +40,7 @@ type fleetResourceValidator struct { func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response { var response admission.Response if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { - switch req.Kind.String() { + switch req.Kind { case retrieveCRDGVK(): klog.V(2).InfoS("handling CRD resource", "GVK", retrieveCRDGVK()) response = v.handleCRD(req) @@ -69,14 +71,15 @@ func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Resp 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)) } - return admission.Allowed("") + return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) } -func retrieveCRDGVK() string { - var crd v1.CustomResourceDefinition - crd.APIVersion = v1.SchemeGroupVersion.Group + "/" + v1.SchemeGroupVersion.Version - crd.Kind = "CustomResourceDefinition" - return crd.GroupVersionKind().String() +func retrieveCRDGVK() metav1.GroupVersionKind { + return metav1.GroupVersionKind{ + Group: v1.SchemeGroupVersion.Group, + Version: v1.SchemeGroupVersion.Version, + Kind: crdKind, + } } func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error { diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index beb09ad6d..97e151ca1 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -21,6 +21,7 @@ import ( "time" admv1 "k8s.io/api/admissionregistration/v1" + admv1beta1 "k8s.io/api/admissionregistration/v1beta1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -44,6 +45,14 @@ const ( FleetWebhookKeyFileName = "tls.key" FleetWebhookCfgName = "fleet-validating-webhook-configuration" FleetWebhookSvcName = "fleetwebhook" + + crdResourceName = "customresourcedefinitions" + replicaSetResourceName = "replicasets" + podResourceName = "pods" +) + +var ( + admissionReviewVersions = []string{admv1.SchemeGroupVersion.Version, admv1beta1.SchemeGroupVersion.Version} ) var AddToManagerFuncs []func(manager.Manager) error @@ -122,7 +131,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { ClientConfig: w.createClientConfig(corev1.Pod{}), FailurePolicy: &failPolicy, SideEffects: &sideEffortsNone, - AdmissionReviewVersions: []string{"v1", "v1beta1"}, + AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { @@ -130,9 +139,9 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { admv1.Create, }, Rule: admv1.Rule{ - APIGroups: []string{""}, - APIVersions: []string{"v1"}, - Resources: []string{"pods"}, + APIGroups: []string{corev1.SchemeGroupVersion.Group}, + APIVersions: []string{corev1.SchemeGroupVersion.Version}, + Resources: []string{podResourceName}, Scope: &namespacedScope, }, }, @@ -143,7 +152,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { ClientConfig: w.createClientConfig(fleetv1alpha1.ClusterResourcePlacement{}), FailurePolicy: &failPolicy, SideEffects: &sideEffortsNone, - AdmissionReviewVersions: []string{"v1", "v1beta1"}, + AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { @@ -152,8 +161,8 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { admv1.Update, }, Rule: admv1.Rule{ - APIGroups: []string{"fleet.azure.com"}, - APIVersions: []string{"v1alpha1"}, + APIGroups: []string{fleetv1alpha1.GroupVersion.Group}, + APIVersions: []string{fleetv1alpha1.GroupVersion.Version}, Resources: []string{fleetv1alpha1.ClusterResourcePlacementResource}, Scope: &clusterScope, }, @@ -165,16 +174,16 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { ClientConfig: w.createClientConfig(appsv1.ReplicaSet{}), FailurePolicy: &failPolicy, SideEffects: &sideEffortsNone, - AdmissionReviewVersions: []string{"v1", "v1beta1"}, + AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { Operations: []admv1.OperationType{ admv1.Create, }, Rule: admv1.Rule{ - APIGroups: []string{"apps"}, - APIVersions: []string{"v1"}, - Resources: []string{"replicasets"}, + APIGroups: []string{appsv1.SchemeGroupVersion.Group}, + APIVersions: []string{appsv1.SchemeGroupVersion.Version}, + Resources: []string{replicaSetResourceName}, Scope: &namespacedScope, }, }, @@ -185,7 +194,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { ClientConfig: w.createClientConfig(v1.CustomResourceDefinition{}), FailurePolicy: &failPolicy, SideEffects: &sideEffortsNone, - AdmissionReviewVersions: []string{"v1", "v1beta1"}, + AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { Operations: []admv1.OperationType{ @@ -194,9 +203,9 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { admv1.Delete, }, Rule: admv1.Rule{ - APIGroups: []string{"apiextensions.k8s.io"}, - APIVersions: []string{"v1"}, - Resources: []string{"customresourcedefinitions"}, + APIGroups: []string{v1.SchemeGroupVersion.Group}, + APIVersions: []string{v1.SchemeGroupVersion.Version}, + Resources: []string{crdResourceName}, Scope: &clusterScope, }, }, diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 6019d00f9..3c435b60f 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -34,6 +34,17 @@ var ( kubeSystemNamespace, memberNamespace, } + testGroups = []string{"system:authenticated"} +) + +const ( + testUser = "test-user" + testKey = "test-key" + testValue = "test-value" + testUserClusterRole = "test-user-cluster-role" + testUserClusterRoleBinding = "test-user-cluster-role-binding" + + crdStatusErrFormat = `failed to validate user: %s in groups: %v to modify fleet CRD: %s` ) var _ = Describe("Fleet's Hub cluster webhook tests", func() { @@ -469,11 +480,11 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("create cluster role to modify CRDs") cr := rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-user-cluster-role", + Name: testUserClusterRole, }, Rules: []rbacv1.PolicyRule{ { - APIGroups: []string{"apiextensions.k8s.io"}, + APIGroups: []string{v1.SchemeGroupVersion.Group}, Verbs: []string{"*"}, Resources: []string{"*"}, }, @@ -485,19 +496,19 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("create cluster role binding for test-user to modify CRD") crb := rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-user-cluster-role-binding", + Name: testUserClusterRoleBinding, }, Subjects: []rbacv1.Subject{ { APIGroup: rbacv1.GroupName, Kind: "User", - Name: "test-user", + Name: testUser, }, }, RoleRef: rbacv1.RoleRef{ APIGroup: rbacv1.GroupName, Kind: "ClusterRole", - Name: "test-user-cluster-role", + Name: testUserClusterRole, }, } err = HubCluster.KubeClient.Create(ctx, &crb) @@ -508,7 +519,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("remove cluster role binding") crb := rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-user-cluster-role-binding", + Name: testUserClusterRoleBinding, }, } err := HubCluster.KubeClient.Delete(ctx, &crb) @@ -517,7 +528,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("remove cluster role") cr := rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-user-cluster-role", + Name: testUserClusterRole, }, } err = HubCluster.KubeClient.Delete(ctx, &cr) @@ -534,8 +545,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { 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{}))) - 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)) + Expect(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf(crdStatusErrFormat, testUser, testGroups, crd.Name))) }) It("should deny UPDATE operation on Fleet CRD for user not in system:masters group", func() { @@ -545,15 +555,14 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("update labels in CRD") labels := crd.GetLabels() - labels["test-key"] = "test-value" + labels[testKey] = testValue crd.SetLabels(labels) By("expecting denial of operation UPDATE of 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: failed to validate user: test-user in groups: [system:authenticated] to modify fleet CRD: %s`, crd.Name))) + Expect(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf(crdStatusErrFormat, testUser, testGroups, crd.Name))) }) It("should deny DELETE operation on Fleet CRD for user not in system:masters group", func() { @@ -566,7 +575,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { 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: failed to validate user: test-user in groups: [system:authenticated] to modify fleet CRD: %s`, crd.Name))) + Expect(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf(crdStatusErrFormat, testUser, testGroups, crd.Name))) }) It("should allow UPDATE operation on Fleet CRDs if user in system:masters group", func() { @@ -576,17 +585,17 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("update labels in CRD") labels := crd.GetLabels() - labels["test-key"] = "test-value" + labels[testKey] = testValue 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()) + Expect(err).To(Succeed()) By("remove new label added for test") labels = crd.GetLabels() - delete(labels, "test-key") + delete(labels, testKey) crd.SetLabels(labels) }) @@ -597,11 +606,11 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { By("expecting error to be nil") err = HubCluster.KubeClient.Create(ctx, &crd) - Expect(err).To(BeNil()) + Expect(err).To(Succeed()) By("delete clone set CRD") err = HubCluster.KubeClient.Delete(ctx, &crd) - Expect(err).To(BeNil()) + Expect(err).To(Succeed()) }) }) }) From e1768eb051fae8bc62a63c717b86c466e619d47e Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 14 Jun 2023 18:05:14 -0700 Subject: [PATCH 19/22] description for allowed --- .../fleetresourcehandler/fleetresourcehandler_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 7c6db1833..69d13289f 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -46,7 +46,7 @@ func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request response = v.handleCRD(req) default: klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String()) - response = admission.Allowed("") + response = admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify resource with GVK: %s", req.UserInfo.Username, req.UserInfo.Groups, req.Kind.String())) } } return response From 0c56a6f6ff6892f716b28c3a08637a44f23b04f9 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 15 Jun 2023 16:41:25 -0700 Subject: [PATCH 20/22] address comments --- ...inition.go => add_fleetresourcehandler.go} | 0 pkg/webhook/validation/crdvalidation.go | 2 +- test/e2e/webhook_test.go | 37 +++++++------------ 3 files changed, 14 insertions(+), 25 deletions(-) rename pkg/webhook/{add_customresourcedefinition.go => add_fleetresourcehandler.go} (100%) diff --git a/pkg/webhook/add_customresourcedefinition.go b/pkg/webhook/add_fleetresourcehandler.go similarity index 100% rename from pkg/webhook/add_customresourcedefinition.go rename to pkg/webhook/add_fleetresourcehandler.go diff --git a/pkg/webhook/validation/crdvalidation.go b/pkg/webhook/validation/crdvalidation.go index e1fc276c3..40826634c 100644 --- a/pkg/webhook/validation/crdvalidation.go +++ b/pkg/webhook/validation/crdvalidation.go @@ -3,7 +3,7 @@ package validation import "k8s.io/utils/strings/slices" var ( - validObjectGroups = []string{"networking.fleet.azure.com", "fleet.azure.com", "multicluster.x-k8s.io"} + validObjectGroups = []string{"networking.fleet.azure.com", "fleet.azure.com", "multicluster.x-k8s.io", "placement.karavel.io"} ) // CheckCRDGroup checks to see if the input CRD group is a fleet CRD group. diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 3c435b60f..9f9046d7f 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -490,8 +490,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { }, }, } - err := HubCluster.KubeClient.Create(ctx, &cr) - Expect(err).Should(Succeed()) + Expect(HubCluster.KubeClient.Create(ctx, &cr)).Should(Succeed()) By("create cluster role binding for test-user to modify CRD") crb := rbacv1.ClusterRoleBinding{ @@ -511,8 +510,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { Name: testUserClusterRole, }, } - err = HubCluster.KubeClient.Create(ctx, &crb) - Expect(err).Should(Succeed()) + Expect(HubCluster.KubeClient.Create(ctx, &crb)).Should(Succeed()) }) AfterEach(func() { @@ -522,8 +520,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { Name: testUserClusterRoleBinding, }, } - err := HubCluster.KubeClient.Delete(ctx, &crb) - Expect(err).Should(Succeed()) + Expect(HubCluster.KubeClient.Delete(ctx, &crb)).Should(Succeed()) By("remove cluster role") cr := rbacv1.ClusterRole{ @@ -531,18 +528,16 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { Name: testUserClusterRole, }, } - err = HubCluster.KubeClient.Delete(ctx, &cr) - Expect(err).Should(Succeed()) + Expect(HubCluster.KubeClient.Delete(ctx, &cr)).Should(Succeed()) }) Context("CRD validation webhook", 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()) + Expect(utils.GetObjectFromManifest("./charts/hub-agent/templates/crds/fleet.azure.com_clusterresourceplacements.yaml", &crd)).Should(Succeed()) By("expecting denial of operation CREATE of CRD") - err = HubCluster.ImpersonateKubeClient.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(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf(crdStatusErrFormat, testUser, testGroups, crd.Name))) @@ -550,8 +545,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", 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()) + Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: "memberclusters.fleet.azure.com"}, &crd)).Should(Succeed()) By("update labels in CRD") labels := crd.GetLabels() @@ -559,7 +553,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { crd.SetLabels(labels) By("expecting denial of operation UPDATE of CRD") - err = HubCluster.ImpersonateKubeClient.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(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf(crdStatusErrFormat, testUser, testGroups, crd.Name))) @@ -580,8 +574,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { 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()) + Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: "memberclusters.fleet.azure.com"}, &crd)).Should(Succeed()) By("update labels in CRD") labels := crd.GetLabels() @@ -590,8 +583,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { 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(Succeed()) + Expect(HubCluster.KubeClient.Update(ctx, &crd)).To(Succeed()) By("remove new label added for test") labels = crd.GetLabels() @@ -601,16 +593,13 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", 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()) + Expect(utils.GetObjectFromManifest("./test/integration/manifests/resources/test_clonesets_crd.yaml", &crd)).Should(Succeed()) By("expecting error to be nil") - err = HubCluster.KubeClient.Create(ctx, &crd) - Expect(err).To(Succeed()) + Expect(HubCluster.KubeClient.Create(ctx, &crd)).To(Succeed()) By("delete clone set CRD") - err = HubCluster.KubeClient.Delete(ctx, &crd) - Expect(err).To(Succeed()) + Expect(HubCluster.KubeClient.Delete(ctx, &crd)).To(Succeed()) }) }) }) From 20b4e7c99179942f69dd9ad535cb383cb4ebb736 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 15 Jun 2023 18:22:11 -0700 Subject: [PATCH 21/22] add comment for group match --- pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 69d13289f..b0d251c99 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -67,6 +67,7 @@ func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Resp } } + // This regex works because every CRD name in kubernetes follows this pattern .. group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1] 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)) From e8ba12e2ed0d0d9a02bd2142a5040a81c3ca0659 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 16 Jun 2023 12:38:52 -0700 Subject: [PATCH 22/22] address comment --- .../fleetresourcehandler/fleetresourcehandler_webhook.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index b0d251c99..99640070c 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -41,8 +41,8 @@ func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request var response admission.Response if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete { switch req.Kind { - case retrieveCRDGVK(): - klog.V(2).InfoS("handling CRD resource", "GVK", retrieveCRDGVK()) + case createCRDGVK(): + klog.V(2).InfoS("handling CRD resource", "GVK", createCRDGVK()) response = v.handleCRD(req) default: klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String()) @@ -75,7 +75,7 @@ func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Resp return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) } -func retrieveCRDGVK() metav1.GroupVersionKind { +func createCRDGVK() metav1.GroupVersionKind { return metav1.GroupVersionKind{ Group: v1.SchemeGroupVersion.Group, Version: v1.SchemeGroupVersion.Version,