From 9758f1f5f82368204198518a6f597806bfc468c2 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Tue, 20 Jun 2023 23:51:02 -0700 Subject: [PATCH 1/4] Webhook for member cluster --- charts/hub-agent/templates/deployment.yaml | 1 + cmd/hubagent/main.go | 8 +- cmd/hubagent/options/options.go | 3 + pkg/webhook/add_clusterresourceplacement.go | 15 -- pkg/webhook/add_fleetresourcehandler.go | 8 -- pkg/webhook/add_handler.go | 16 +++ pkg/webhook/add_pod.go | 14 -- pkg/webhook/add_replicaset.go | 14 -- .../clusterresourceplacement_webhook.go | 2 +- .../fleetresourcehandler_webhook.go | 79 ++++++++--- pkg/webhook/pod/pod_validating_webhook.go | 2 +- .../replicaset_validating_webhook.go | 2 +- pkg/webhook/validation/uservalidation.go | 27 ++++ pkg/webhook/webhook.go | 38 ++++- test/e2e/webhook_test.go | 133 ++++++++++++++++++ 15 files changed, 277 insertions(+), 85 deletions(-) delete mode 100644 pkg/webhook/add_clusterresourceplacement.go delete mode 100644 pkg/webhook/add_fleetresourcehandler.go create mode 100644 pkg/webhook/add_handler.go delete mode 100644 pkg/webhook/add_pod.go delete mode 100644 pkg/webhook/add_replicaset.go diff --git a/charts/hub-agent/templates/deployment.yaml b/charts/hub-agent/templates/deployment.yaml index e7b238ba1..fd2102000 100644 --- a/charts/hub-agent/templates/deployment.yaml +++ b/charts/hub-agent/templates/deployment.yaml @@ -22,6 +22,7 @@ spec: args: - --leader-elect=true - --enable-webhook={{ .Values.enableWebhook }} + - --whitelisted-users=system:serviceaccount:fleet-system:hub-agent-sa - --webhook-client-connection-type={{.Values.webhookClientConnectionType}} - --v={{ .Values.logVerbosity }} - -add_dir_header diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index 6df887790..9254993c2 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -8,6 +8,7 @@ package main import ( "flag" "os" + "strings" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -109,7 +110,8 @@ func main() { } if opts.EnableWebhook { - if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType)); err != nil { + whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",") + if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), whiteListedUsers); err != nil { klog.ErrorS(err, "unable to set up webhook") exitWithErrorFunc() } @@ -130,7 +132,7 @@ func main() { } // SetupWebhook generates the webhook cert and then set up the webhook configurator. -func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType) error { +func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, whiteListedUsers []string) error { // Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start. w, err := webhook.NewWebhookConfig(mgr, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir) if err != nil { @@ -141,7 +143,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho klog.ErrorS(err, "unable to add WebhookConfig") return err } - if err = webhook.AddToManager(mgr); err != nil { + if err = webhook.AddToManager(mgr, whiteListedUsers); err != nil { klog.ErrorS(err, "unable to register webhooks to the manager") return err } diff --git a/cmd/hubagent/options/options.go b/cmd/hubagent/options/options.go index 7a5039577..c91852de5 100644 --- a/cmd/hubagent/options/options.go +++ b/cmd/hubagent/options/options.go @@ -35,6 +35,8 @@ type Options struct { MetricsBindAddress string // EnableWebhook indicates if we will run a webhook EnableWebhook bool + // WhiteListedUsers indicates the list of user who are allowed to modify fleet resources + WhiteListedUsers string // Sets the connection type for the webhook. WebhookClientConnectionType string // NetworkingAgentsEnabled indicates if we enable network agents @@ -91,6 +93,7 @@ func (o *Options) AddFlags(flags *flag.FlagSet) { flags.DurationVar(&o.LeaderElection.LeaseDuration.Duration, "leader-lease-duration", 15*time.Second, "This is effectively the maximum duration that a leader can be stopped before someone else will replace it.") flag.StringVar(&o.LeaderElection.ResourceNamespace, "leader-election-namespace", utils.FleetSystemNamespace, "The namespace in which the leader election resource will be created.") flag.BoolVar(&o.EnableWebhook, "enable-webhook", true, "If set, the fleet webhook is enabled.") + flag.StringVar(&o.WhiteListedUsers, "whitelisted-users", "", "If set, white listed users can modify fleet related resources.") flag.StringVar(&o.WebhookClientConnectionType, "webhook-client-connection-type", "url", "Sets the connection type used by the webhook client. Only URL or Service is valid.") flag.BoolVar(&o.NetworkingAgentsEnabled, "networking-agents-enabled", false, "Whether the networking agents are enabled or not.") flags.DurationVar(&o.ClusterUnhealthyThreshold.Duration, "cluster-unhealthy-threshold", 60*time.Second, "The duration for a member cluster to be in a degraded state before considered unhealthy.") diff --git a/pkg/webhook/add_clusterresourceplacement.go b/pkg/webhook/add_clusterresourceplacement.go deleted file mode 100644 index b9cc50737..000000000 --- a/pkg/webhook/add_clusterresourceplacement.go +++ /dev/null @@ -1,15 +0,0 @@ -/* -Copyright (c) Microsoft Corporation. -Licensed under the MIT license. -*/ - -package webhook - -import ( - "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" -) - -func init() { - // AddToManagerFuncs is a list of functions to create webhook and add them to a manager. - AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add) -} diff --git a/pkg/webhook/add_fleetresourcehandler.go b/pkg/webhook/add_fleetresourcehandler.go deleted file mode 100644 index 3172bcc19..000000000 --- a/pkg/webhook/add_fleetresourcehandler.go +++ /dev/null @@ -1,8 +0,0 @@ -package webhook - -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, fleetresourcehandler.Add) -} diff --git a/pkg/webhook/add_handler.go b/pkg/webhook/add_handler.go new file mode 100644 index 000000000..b135606b5 --- /dev/null +++ b/pkg/webhook/add_handler.go @@ -0,0 +1,16 @@ +package webhook + +import ( + "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" + "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" + "go.goms.io/fleet/pkg/webhook/pod" + "go.goms.io/fleet/pkg/webhook/replicaset" +) + +func init() { + // AddToManagerFuncs is a list of functions to create webhook and add them to a manager. + AddToManagerFuncs = append(AddToManagerFuncs, fleetresourcehandler.Add) + AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add) + AddToManagerFuncs = append(AddToManagerFuncs, pod.Add) + AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add) +} diff --git a/pkg/webhook/add_pod.go b/pkg/webhook/add_pod.go deleted file mode 100644 index c5a82fa86..000000000 --- a/pkg/webhook/add_pod.go +++ /dev/null @@ -1,14 +0,0 @@ -/* -Copyright (c) Microsoft Corporation. -Licensed under the MIT license. -*/ - -package webhook - -import ( - "go.goms.io/fleet/pkg/webhook/pod" -) - -func init() { - AddToManagerFuncs = append(AddToManagerFuncs, pod.Add) -} diff --git a/pkg/webhook/add_replicaset.go b/pkg/webhook/add_replicaset.go deleted file mode 100644 index 9d1e37e90..000000000 --- a/pkg/webhook/add_replicaset.go +++ /dev/null @@ -1,14 +0,0 @@ -/* -Copyright (c) Microsoft Corporation. -Licensed under the MIT license. -*/ - -package webhook - -import ( - "go.goms.io/fleet/pkg/webhook/replicaset" -) - -func init() { - AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add) -} diff --git a/pkg/webhook/clusterresourceplacement/clusterresourceplacement_webhook.go b/pkg/webhook/clusterresourceplacement/clusterresourceplacement_webhook.go index 5e1a2a4b6..15fbe500f 100644 --- a/pkg/webhook/clusterresourceplacement/clusterresourceplacement_webhook.go +++ b/pkg/webhook/clusterresourceplacement/clusterresourceplacement_webhook.go @@ -16,6 +16,6 @@ const ( ValidationPath = "/validate-fleet-azure-com-v1alpha1-clusterresourceplacement" ) -func Add(mgr manager.Manager) error { +func Add(mgr manager.Manager, _ []string) error { return (&fleetv1alpha1.ClusterResourcePlacement{}).SetupWebhookWithManager(mgr) } diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 99640070c..4cafdbaa0 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -9,41 +9,48 @@ 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/apimachinery/pkg/runtime" "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" + fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "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 = `^[^.]*\.(.*)` - crdKind = "CustomResourceDefinition" + ValidationPath = "/validate-v1-fleetresourcehandler" + groupMatch = `^[^.]*\.(.*)` + crdKind = "CustomResourceDefinition" + memberClusterKind = "MemberCluster" ) // Add registers the webhook for K8s bulit-in object types. -func Add(mgr manager.Manager) error { +func Add(mgr manager.Manager, whiteListedUsers []string) error { hookServer := mgr.GetWebhookServer() - hookServer.Register(ValidationPath, &webhook.Admission{Handler: &fleetResourceValidator{Client: mgr.GetClient()}}) + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &FleetResourceValidator{Client: mgr.GetClient(), whiteListedUsers: whiteListedUsers}}) return nil } -type fleetResourceValidator struct { - Client client.Client - decoder *admission.Decoder +type FleetResourceValidator struct { + Client client.Client + whiteListedUsers []string + decoder *admission.Decoder } -func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response { +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 { case createCRDGVK(): klog.V(2).InfoS("handling CRD resource", "GVK", createCRDGVK()) response = v.handleCRD(req) + case createMemberClusterGVK(): + klog.V(2).InfoS("handling Member cluster resource", "GVK", createMemberClusterGVK()) + response = v.handleMemberCluster(ctx, req) default: klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String()) 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())) @@ -52,19 +59,10 @@ func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request return response } -func (v *fleetResourceValidator) handleCRD(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. - 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) - } + if err := v.decodeRequestObject(req, &crd); err != nil { + return admission.Errored(http.StatusBadRequest, err) } // This regex works because every CRD name in kubernetes follows this pattern .. @@ -75,6 +73,35 @@ 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 (v *FleetResourceValidator) handleMemberCluster(ctx context.Context, req admission.Request) admission.Response { + var mc fleetv1alpha1.MemberCluster + if err := v.decodeRequestObject(req, &mc); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + if !validation.ValidateUserForFleetCR(ctx, v.Client, v.whiteListedUsers, req.UserInfo) { + return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify member cluster CR: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name)) + } + klog.V(2).InfoS("user in groups is allowed to modify member cluster CR", "user", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify member cluster: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name)) +} + +func (v *FleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error { + 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, obj); err != nil { + klog.ErrorS(err, "failed to decode old request object for delete operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return err + } + } else { + if err := v.decoder.Decode(req, obj); err != nil { + klog.ErrorS(err, "failed to decode request object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return err + } + } + return nil +} + func createCRDGVK() metav1.GroupVersionKind { return metav1.GroupVersionKind{ Group: v1.SchemeGroupVersion.Group, @@ -83,7 +110,15 @@ func createCRDGVK() metav1.GroupVersionKind { } } -func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error { +func createMemberClusterGVK() metav1.GroupVersionKind { + return metav1.GroupVersionKind{ + Group: fleetv1alpha1.GroupVersion.Group, + Version: fleetv1alpha1.GroupVersion.Version, + Kind: memberClusterKind, + } +} + +func (v *FleetResourceValidator) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil } diff --git a/pkg/webhook/pod/pod_validating_webhook.go b/pkg/webhook/pod/pod_validating_webhook.go index 347cf92aa..2cc937540 100644 --- a/pkg/webhook/pod/pod_validating_webhook.go +++ b/pkg/webhook/pod/pod_validating_webhook.go @@ -26,7 +26,7 @@ const ( ) // Add registers the webhook for K8s bulit-in object types. -func Add(mgr manager.Manager) error { +func Add(mgr manager.Manager, _ []string) error { hookServer := mgr.GetWebhookServer() hookServer.Register(ValidationPath, &webhook.Admission{Handler: &podValidator{Client: mgr.GetClient()}}) return nil diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index c5b92110f..1c747dc74 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -31,7 +31,7 @@ type replicaSetValidator struct { } // Add registers the webhook for K8s bulit-in object types. -func Add(mgr manager.Manager) error { +func Add(mgr manager.Manager, _ []string) error { hookServer := mgr.GetWebhookServer() hookServer.Register(ValidationPath, &webhook.Admission{Handler: &replicaSetValidator{Client: mgr.GetClient()}}) return nil diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index cc4e7ec01..138c06185 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -1,8 +1,14 @@ package validation import ( + "context" + 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 ( @@ -15,3 +21,24 @@ const ( func ValidateUserForCRD(userInfo authenticationv1.UserInfo) bool { return slices.Contains(userInfo.Groups, mastersGroup) } + +// ValidateUserForFleetCR checks to see if user is authenticated to make a request to modify Fleet CRs. +func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { + if slices.Contains(userInfo.Groups, mastersGroup) { + return true + } + if slices.Contains(whiteListedUsers, userInfo.Username) { + return true + } + var memberClusterList fleetv1alpha1.MemberClusterList + if err := client.List(ctx, &memberClusterList); err != nil { + klog.V(2).ErrorS(err, "failed to list member clusters") + return false + } + var identities []string + for i, _ := range memberClusterList.Items { + identities = append(identities, memberClusterList.Items[i].Spec.Identity.Name) + } + // this ensures will allow all member agents are validated. + return slices.Contains(identities, userInfo.Username) +} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 97e151ca1..dd52de149 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -46,21 +46,22 @@ const ( FleetWebhookCfgName = "fleet-validating-webhook-configuration" FleetWebhookSvcName = "fleetwebhook" - crdResourceName = "customresourcedefinitions" - replicaSetResourceName = "replicasets" - podResourceName = "pods" + crdResourceName = "customresourcedefinitions" + memberClusterResourceName = "memberclusters" + replicaSetResourceName = "replicasets" + podResourceName = "pods" ) var ( admissionReviewVersions = []string{admv1.SchemeGroupVersion.Version, admv1beta1.SchemeGroupVersion.Version} ) -var AddToManagerFuncs []func(manager.Manager) error +var AddToManagerFuncs []func(manager.Manager, []string) error // AddToManager adds all Controllers to the Manager -func AddToManager(m manager.Manager) error { +func AddToManager(m manager.Manager, whiteListedUsers []string) error { for _, f := range AddToManagerFuncs { - if err := f(m); err != nil { + if err := f(m, whiteListedUsers); err != nil { return err } } @@ -211,6 +212,28 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { }, }, }, + { + Name: "fleet.membercluster.validating", + ClientConfig: w.createClientConfig(fleetv1alpha1.MemberCluster{}), + FailurePolicy: &failPolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: admissionReviewVersions, + Rules: []admv1.RuleWithOperations{ + { + Operations: []admv1.OperationType{ + admv1.Create, + admv1.Update, + admv1.Delete, + }, + Rule: admv1.Rule{ + APIGroups: []string{fleetv1alpha1.GroupVersion.Group}, + APIVersions: []string{fleetv1alpha1.GroupVersion.Version}, + Resources: []string{memberClusterResourceName}, + Scope: &clusterScope, + }, + }, + }, + }, }, } @@ -261,6 +284,9 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC case v1.CustomResourceDefinition: serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) + case fleetv1alpha1.MemberCluster: + serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath + serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) } config := admv1.WebhookClientConfig{ diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 9f9046d7f..b2958fe87 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -45,6 +45,7 @@ const ( testUserClusterRoleBinding = "test-user-cluster-role-binding" crdStatusErrFormat = `failed to validate user: %s in groups: %v to modify fleet CRD: %s` + mcStatusErrFormat = `failed to validate user: %s in groups: %v to modify member cluster CR: %s` ) var _ = Describe("Fleet's Hub cluster webhook tests", func() { @@ -603,3 +604,135 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { }) }) }) + +var _ = Describe("Fleet's CR Resource Handler webhook tests", func() { + BeforeEach(func() { + By("create cluster role to modify Fleet CRs") + cr := rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: testUserClusterRole, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{fleetv1alpha1.GroupVersion.Group}, + Verbs: []string{"*"}, + Resources: []string{"*"}, + }, + }, + } + Expect(HubCluster.KubeClient.Create(ctx, &cr)).Should(Succeed()) + + By("create cluster role binding for test-user to modify fleet CRs") + crb := rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: testUserClusterRoleBinding, + }, + Subjects: []rbacv1.Subject{ + { + APIGroup: rbacv1.GroupName, + Kind: "User", + Name: testUser, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: testUserClusterRole, + }, + } + Expect(HubCluster.KubeClient.Create(ctx, &crb)).Should(Succeed()) + }) + + AfterEach(func() { + By("remove cluster role binding") + crb := rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: testUserClusterRoleBinding, + }, + } + Expect(HubCluster.KubeClient.Delete(ctx, &crb)).Should(Succeed()) + + By("remove cluster role") + cr := rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: testUserClusterRole, + }, + } + Expect(HubCluster.KubeClient.Delete(ctx, &cr)).Should(Succeed()) + }) + + Context("CR validation webhook", func() { + It("should deny CREATE operation on Fleet Member cluster CR for user not in system:masters group", func() { + mc := fleetv1alpha1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-member-cluster", + }, + Spec: fleetv1alpha1.MemberClusterSpec{ + State: fleetv1alpha1.ClusterStateJoin, + Identity: rbacv1.Subject{ + Kind: "User", + APIGroup: "", + Name: "test-subject", + Namespace: "fleet-system", + }, + }, + } + + By("expecting denial of operation CREATE of member cluster") + err := HubCluster.ImpersonateKubeClient.Create(ctx, &mc) + fmt.Println(err) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create member cluster 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(mcStatusErrFormat, testUser, testGroups, mc.Name))) + }) + + It("should deny UPDATE operation on member cluster CR for user not in system:masters group", func() { + var mc fleetv1alpha1.MemberCluster + Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: MemberCluster.ClusterName}, &mc)).Should(Succeed()) + + By("update member cluster spec") + mc.Spec.State = fleetv1alpha1.ClusterStateLeave + + By("expecting denial of operation UPDATE of member cluster") + err := HubCluster.ImpersonateKubeClient.Update(ctx, &mc) + var statusErr *k8sErrors.StatusError + fmt.Println(err.Error()) + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update member cluster 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(mcStatusErrFormat, testUser, testGroups, mc.Name))) + }) + + It("should deny DELETE operation on member cluster CR for user not in system:masters group", func() { + mc := fleetv1alpha1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: MemberCluster.ClusterName, + }, + } + + By("expecting denial of operation Delete of member cluster") + err := HubCluster.ImpersonateKubeClient.Delete(ctx, &mc) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete member cluster 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(mcStatusErrFormat, testUser, testGroups, mc.Name))) + }) + + It("should allow update operation on member cluster if user in system:masters group", func() { + var mc fleetv1alpha1.MemberCluster + Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: MemberCluster.ClusterName}, &mc)).Should(Succeed()) + + By("update labels in CRD") + labels := mc.GetLabels() + labels = make(map[string]string) + labels[testKey] = testValue + mc.SetLabels(labels) + + By("expecting denial of operation UPDATE of CRD") + // The user associated with KubeClient is kubernetes-admin in groups: [system:masters, system:authenticated] + Expect(HubCluster.KubeClient.Update(ctx, &mc)).To(Succeed()) + + By("remove new label added for test") + labels = mc.GetLabels() + delete(labels, testKey) + mc.SetLabels(labels) + }) + }) +}) From 6459bef3fe1a08157f0deea56bf546a167c9e3e8 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 21 Jun 2023 00:26:44 -0700 Subject: [PATCH 2/4] add UTs --- pkg/webhook/validation/uservalidation.go | 6 +- pkg/webhook/validation/uservalidation_test.go | 69 +++++++++++++++++-- test/e2e/webhook_test.go | 3 +- 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 138c06185..89b11ff9a 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -15,8 +15,6 @@ const ( mastersGroup = "system:masters" ) -// TODO:(Arvindthiru) Get valid usernames as flag and allow those usernames. - // 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) @@ -35,8 +33,8 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList klog.V(2).ErrorS(err, "failed to list member clusters") return false } - var identities []string - for i, _ := range memberClusterList.Items { + identities := make([]string, len(memberClusterList.Items)) + for i := range memberClusterList.Items { identities = append(identities, memberClusterList.Items[i].Spec.Identity.Name) } // this ensures will allow all member agents are validated. diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index d21d1a883..c87fc94fc 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/pkg/webhook/validation/uservalidation_test.go @@ -7,26 +7,87 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/stretchr/testify/assert" v1 "k8s.io/api/authentication/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/pkg/utils" ) func TestValidateUserForCRD(t *testing.T) { testCases := map[string]struct { - client client.Client userInfo v1.UserInfo wantResult bool }{ "allow user in system:masters group": { + userInfo: v1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + wantResult: true, + }, + "fail to validate user with invalid username, groups": { + userInfo: v1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + wantResult: false, + }, + } + + for testName, testCase := range testCases { + t.Run(testName, func(t *testing.T) { + gotResult := ValidateUserForCRD(testCase.userInfo) + assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName) + }) + } +} + +func TestValidateUserForFleetCR(t *testing.T) { + testCases := map[string]struct { + client client.Client + whiteListedUsers []string + userInfo v1.UserInfo + wantResult bool + }{ + "allow use in system:masters group": { + userInfo: v1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + wantResult: true, + }, + "allow white listed user not in system:masters group": { + userInfo: v1.UserInfo{ + Username: "test-user", + }, + whiteListedUsers: []string{"test-user"}, + wantResult: true, + }, + "allow member cluster identity": { client: &test.MockClient{ MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + o := list.(*fleetv1alpha1.MemberClusterList) + *o = fleetv1alpha1.MemberClusterList{ + Items: []fleetv1alpha1.MemberCluster{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-member-cluster", + }, + Spec: fleetv1alpha1.MemberClusterSpec{ + Identity: rbacv1.Subject{ + Name: "member-cluster-identity", + }, + }, + }, + }, + } return nil }, }, userInfo: v1.UserInfo{ - Username: "test-user", - Groups: []string{"system:masters"}, + Username: "member-cluster-identity", }, wantResult: true, }, @@ -46,7 +107,7 @@ func TestValidateUserForCRD(t *testing.T) { for testName, testCase := range testCases { t.Run(testName, func(t *testing.T) { - gotResult := ValidateUserForCRD(testCase.userInfo) + gotResult := ValidateUserForFleetCR(context.Background(), testCase.client, testCase.whiteListedUsers, testCase.userInfo) assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName) }) } diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index b2958fe87..d140537d6 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -720,8 +720,7 @@ var _ = Describe("Fleet's CR Resource Handler webhook tests", func() { Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: MemberCluster.ClusterName}, &mc)).Should(Succeed()) By("update labels in CRD") - labels := mc.GetLabels() - labels = make(map[string]string) + labels := make(map[string]string) labels[testKey] = testValue mc.SetLabels(labels) From ee084c3ca3885b011197de2a16e8a7e881f17415 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 21 Jun 2023 00:33:38 -0700 Subject: [PATCH 3/4] change test messages --- test/e2e/webhook_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index d140537d6..c12a1131e 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -607,7 +607,7 @@ var _ = Describe("Fleet's CRD Resource Handler webhook tests", func() { var _ = Describe("Fleet's CR Resource Handler webhook tests", func() { BeforeEach(func() { - By("create cluster role to modify Fleet CRs") + By("create cluster role to modify fleet CRs") cr := rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ Name: testUserClusterRole, @@ -662,7 +662,7 @@ var _ = Describe("Fleet's CR Resource Handler webhook tests", func() { }) Context("CR validation webhook", func() { - It("should deny CREATE operation on Fleet Member cluster CR for user not in system:masters group", func() { + It("should deny CREATE operation on member cluster CR for user not in system:masters group", func() { mc := fleetv1alpha1.MemberCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "test-member-cluster", @@ -708,14 +708,14 @@ var _ = Describe("Fleet's CR Resource Handler webhook tests", func() { }, } - By("expecting denial of operation Delete of member cluster") + By("expecting denial of operation DELETE of member cluster") err := HubCluster.ImpersonateKubeClient.Delete(ctx, &mc) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete member cluster 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(mcStatusErrFormat, testUser, testGroups, mc.Name))) }) - It("should allow update operation on member cluster if user in system:masters group", func() { + It("should allow update operation on member cluster CR for user in system:masters group", func() { var mc fleetv1alpha1.MemberCluster Expect(HubCluster.KubeClient.Get(ctx, types.NamespacedName{Name: MemberCluster.ClusterName}, &mc)).Should(Succeed()) From 5c37df739dcea77022501689a3ec4bbc34026fcc Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 22 Jun 2023 09:24:50 -0700 Subject: [PATCH 4/4] address comments --- .../fleetresourcehandler_webhook.go | 28 +++++++++---------- pkg/webhook/validation/uservalidation.go | 13 +++++---- pkg/webhook/validation/uservalidation_test.go | 14 ++++++++-- pkg/webhook/webhook.go | 2 +- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 4cafdbaa0..bc9650b03 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -31,17 +31,17 @@ const ( // Add registers the webhook for K8s bulit-in object types. func Add(mgr manager.Manager, whiteListedUsers []string) error { hookServer := mgr.GetWebhookServer() - hookServer.Register(ValidationPath, &webhook.Admission{Handler: &FleetResourceValidator{Client: mgr.GetClient(), whiteListedUsers: whiteListedUsers}}) + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &fleetResourceValidator{client: mgr.GetClient(), whiteListedUsers: whiteListedUsers}}) return nil } -type FleetResourceValidator struct { - Client client.Client +type fleetResourceValidator struct { + client client.Client whiteListedUsers []string decoder *admission.Decoder } -func (v *FleetResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response { +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 { @@ -59,7 +59,7 @@ func (v *FleetResourceValidator) Handle(ctx context.Context, req admission.Reque return response } -func (v *FleetResourceValidator) handleCRD(req admission.Request) admission.Response { +func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Response { var crd v1.CustomResourceDefinition if err := v.decodeRequestObject(req, &crd); err != nil { return admission.Errored(http.StatusBadRequest, err) @@ -67,26 +67,26 @@ 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) { + if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(v.whiteListedUsers, 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(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name)) } -func (v *FleetResourceValidator) handleMemberCluster(ctx context.Context, req admission.Request) admission.Response { +func (v *fleetResourceValidator) handleMemberCluster(ctx context.Context, req admission.Request) admission.Response { var mc fleetv1alpha1.MemberCluster if err := v.decodeRequestObject(req, &mc); err != nil { return admission.Errored(http.StatusBadRequest, err) } - if !validation.ValidateUserForFleetCR(ctx, v.Client, v.whiteListedUsers, req.UserInfo) { + if !validation.ValidateUserForFleetCR(ctx, v.client, v.whiteListedUsers, req.UserInfo) { return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify member cluster CR: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name)) } klog.V(2).InfoS("user in groups is allowed to modify member cluster CR", "user", req.UserInfo.Username, "groups", req.UserInfo.Groups) return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify member cluster: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name)) } -func (v *FleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error { +func (v *fleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error { 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, obj); err != nil { @@ -102,6 +102,11 @@ func (v *FleetResourceValidator) decodeRequestObject(req admission.Request, obj return nil } +func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} + func createCRDGVK() metav1.GroupVersionKind { return metav1.GroupVersionKind{ Group: v1.SchemeGroupVersion.Group, @@ -117,8 +122,3 @@ func createMemberClusterGVK() metav1.GroupVersionKind { Kind: memberClusterKind, } } - -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 89b11ff9a..41a88d99d 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -16,16 +16,13 @@ const ( ) // 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) +func ValidateUserForCRD(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { + return isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) } // ValidateUserForFleetCR checks to see if user is authenticated to make a request to modify Fleet CRs. func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { - if slices.Contains(userInfo.Groups, mastersGroup) { - return true - } - if slices.Contains(whiteListedUsers, userInfo.Username) { + if isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) { return true } var memberClusterList fleetv1alpha1.MemberClusterList @@ -40,3 +37,7 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList // this ensures will allow all member agents are validated. return slices.Contains(identities, userInfo.Username) } + +func isMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { + return slices.Contains(whiteListedUsers, userInfo.Username) || slices.Contains(userInfo.Groups, mastersGroup) +} diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index c87fc94fc..0ecaf05ab 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/pkg/webhook/validation/uservalidation_test.go @@ -17,8 +17,9 @@ import ( func TestValidateUserForCRD(t *testing.T) { testCases := map[string]struct { - userInfo v1.UserInfo - wantResult bool + userInfo v1.UserInfo + whiteListedUsers []string + wantResult bool }{ "allow user in system:masters group": { userInfo: v1.UserInfo{ @@ -27,6 +28,13 @@ func TestValidateUserForCRD(t *testing.T) { }, wantResult: true, }, + "allow white listed user not in system:masters group": { + userInfo: v1.UserInfo{ + Username: "test-user", + }, + whiteListedUsers: []string{"test-user"}, + wantResult: true, + }, "fail to validate user with invalid username, groups": { userInfo: v1.UserInfo{ Username: "test-user", @@ -38,7 +46,7 @@ func TestValidateUserForCRD(t *testing.T) { for testName, testCase := range testCases { t.Run(testName, func(t *testing.T) { - gotResult := ValidateUserForCRD(testCase.userInfo) + gotResult := ValidateUserForCRD(testCase.whiteListedUsers, testCase.userInfo) assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName) }) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index dd52de149..ee99349dc 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -114,7 +114,7 @@ func (w *Config) Start(ctx context.Context) error { // createFleetWebhookConfiguration creates the ValidatingWebhookConfiguration object for the webhook. func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { - failPolicy := admv1.Fail + failPolicy := admv1.Ignore sideEffortsNone := admv1.SideEffectClassNone namespacedScope := admv1.NamespacedScope clusterScope := admv1.ClusterScope