diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 8e921caaf..1bde2c85a 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1" fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" ) @@ -36,6 +37,8 @@ const ( var ( fleetCRDGroups = []string{"networking.fleet.azure.com", "fleet.azure.com", "multicluster.x-k8s.io", "cluster.kubernetes-fleet.io", "placement.kubernetes-fleet.io"} + imcGVK = metav1.GroupVersionKind{Group: clusterv1beta1.GroupVersion.Group, Version: clusterv1beta1.GroupVersion.Version, Kind: "InternalMemberCluster"} + v1Alpha1IMCGVK = metav1.GroupVersionKind{Group: fleetv1alpha1.GroupVersion.Group, Version: fleetv1alpha1.GroupVersion.Version, Kind: "InternalMemberCluster"} ) // ValidateUserForFleetCRD checks to see if user is not allowed to modify fleet CRDs. @@ -163,17 +166,31 @@ func checkCRDGroup(group string) bool { // ValidateMCIdentity returns admission allowed/denied based on the member cluster's identity. func ValidateMCIdentity(ctx context.Context, client client.Client, req admission.Request, mcName string) admission.Response { + var identity string namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace} userInfo := req.UserInfo - var mc fleetv1alpha1.MemberCluster - if err := client.Get(ctx, types.NamespacedName{Name: mcName}, &mc); err != nil { - // fail open, if the webhook cannot get member cluster resources we don't block the request. - klog.V(2).ErrorS(err, fmt.Sprintf("failed to get member cluster resource for request to modify %+v/%s, allowing request to be handled by api server", req.RequestKind, req.SubResource), - "user", userInfo.Username, "groups", userInfo.Groups, "namespacedName", namespacedName) - return admission.Allowed(fmt.Sprintf(resourceAllowedGetMCFailed, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName)) + if *req.RequestKind == v1Alpha1IMCGVK { + var mc fleetv1alpha1.MemberCluster + if err := client.Get(ctx, types.NamespacedName{Name: mcName}, &mc); err != nil { + // fail open, if the webhook cannot get member cluster resources we don't block the request. + klog.V(2).ErrorS(err, fmt.Sprintf("failed to get member cluster resource for request to modify %+v/%s, allowing request to be handled by api server", req.RequestKind, req.SubResource), + "user", userInfo.Username, "groups", userInfo.Groups, "namespacedName", namespacedName) + return admission.Allowed(fmt.Sprintf(resourceAllowedGetMCFailed, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName)) + } + identity = mc.Spec.Identity.Name + } else if *req.RequestKind == imcGVK { + var mc clusterv1beta1.MemberCluster + if err := client.Get(ctx, types.NamespacedName{Name: mcName}, &mc); err != nil { + // fail open, if the webhook cannot get member cluster resources we don't block the request. + klog.V(2).ErrorS(err, fmt.Sprintf("failed to get member cluster resource for request to modify %+v/%s, allowing request to be handled by api server", req.RequestKind, req.SubResource), + "user", userInfo.Username, "groups", userInfo.Groups, "namespacedName", namespacedName) + return admission.Allowed(fmt.Sprintf(resourceAllowedGetMCFailed, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName)) + } + identity = mc.Spec.Identity.Name } + // For the upstream E2E we use hub agent service account's token which allows member agent to modify Work status, hence we use serviceAccountFmt to make the check. - if mc.Spec.Identity.Name == userInfo.Username || fmt.Sprintf(serviceAccountFmt, mc.Spec.Identity.Name) == userInfo.Username { + if identity == userInfo.Username || fmt.Sprintf(serviceAccountFmt, identity) == userInfo.Username { klog.V(2).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) return admission.Allowed(fmt.Sprintf(resourceAllowedFormat, userInfo.Username, userInfo.Groups, req.Operation, req.RequestKind, req.SubResource, namespacedName)) } diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index 3871188d4..c462ec21e 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -12,12 +12,14 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" admissionv1 "k8s.io/api/admission/v1" + corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1" + placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils" ) @@ -147,7 +149,26 @@ var _ = Describe("fleet guard rail tests for allow/deny MC UPDATE, DELETE operat }) var _ = Describe("fleet guard rail tests for deny IMC CREATE operations", func() { - mcName := fmt.Sprintf(mcNameTemplate, GinkgoParallelProcess()) + mcName := "test-mc-123" + + BeforeEach(func() { + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(utils.NamespaceNameFormat, mcName), + Labels: map[string]string{placementv1beta1.FleetResourceLabelKey: "true"}, + }, + } + Expect(hubClient.Create(ctx, &ns)).Should(Succeed()) + }) + + AfterEach(func() { + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(utils.NamespaceNameFormat, mcName), + }, + } + Expect(hubClient.Delete(ctx, &ns)).Should(Succeed()) + }) It("should deny CREATE operation on internal member cluster CR for user not in MC identity in fleet member namespace", func() { imcNamespace := fmt.Sprintf(utils.NamespaceNameFormat, mcName) @@ -164,7 +185,7 @@ var _ = Describe("fleet guard rail tests for deny IMC CREATE operations", func() By(fmt.Sprintf("expecting denial of operation CREATE of Internal Member Cluster %s/%s", mcName, imcNamespace)) err := impersonateHubClient.Create(ctx, &imc) - By(fmt.Sprintf(err.Error())) + fmt.Println(err.Error()) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create internal 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(resourceDeniedFormat, testUser, testGroups, admissionv1.Create, &imcGVK, "", types.NamespacedName{Name: imc.Name, Namespace: imc.Namespace})))