feat: block member label modification through DP

cmd/hubagent/main.go

@@ -171,7 +171,7 @@ func main() {
 
 	if opts.EnableWebhook {
 		whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",")
-		if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs); err != nil {
+		if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels); err != nil {
 			klog.ErrorS(err, "unable to set up webhook")
 			exitWithErrorFunc()
 		}
@@ -203,9 +203,9 @@ func main() {
 }
 
 // SetupWebhook generates the webhook cert and then set up the webhook configurator.
-func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool) error {
+func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error {
 	// Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start.
-	w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail)
+	w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail, denyModifyMemberClusterLabels)
 	if err != nil {
 		klog.ErrorS(err, "fail to generate WebhookConfig")
 		return err
@@ -214,7 +214,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho
 		klog.ErrorS(err, "unable to add WebhookConfig")
 		return err
 	}
-	if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API); err != nil {
+	if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API, denyModifyMemberClusterLabels); err != nil {
 		klog.ErrorS(err, "unable to register webhooks to the manager")
 		return err
 	}

cmd/hubagent/options/options.go

@@ -98,6 +98,8 @@ type Options struct {
 	EnableStagedUpdateRunAPIs bool
 	// EnableEvictionAPIs enables to agents to watch the eviction and placement disruption budget CRs.
 	EnableEvictionAPIs bool
+	// DenyModifyMemberClusterLabels indicates if the member cluster labels cannot be modified by groups (excluding system:masters)
+	DenyModifyMemberClusterLabels bool
 }
 
 // NewOptions builds an empty options.
@@ -158,6 +160,7 @@ func (o *Options) AddFlags(flags *flag.FlagSet) {
 	flags.DurationVar(&o.ForceDeleteWaitTime.Duration, "force-delete-wait-time", 15*time.Minute, "The duration the hub agent waits before force deleting a member cluster.")
 	flags.BoolVar(&o.EnableStagedUpdateRunAPIs, "enable-staged-update-run-apis", true, "If set, the agents will watch for the ClusterStagedUpdateRun APIs.")
 	flags.BoolVar(&o.EnableEvictionAPIs, "enable-eviction-apis", true, "If set, the agents will watch for the Eviction and PlacementDisruptionBudget APIs.")
+	flags.BoolVar(&o.DenyModifyMemberClusterLabels, "deny-modify-member-cluster-labels", false, "If set, users not in the system:masters cannot modify member cluster labels.")
 
 	o.RateLimiterOpts.AddFlags(flags)
 }

cmd/hubagent/options/validation_test.go

@@ -17,10 +17,12 @@ limitations under the License.
 package options
 
 import (
+	"flag"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
@@ -102,3 +104,13 @@ func TestValidateControllerManagerConfiguration(t *testing.T) {
 		})
 	}
 }
+
+func TestAddFlags(t *testing.T) {
+	g := gomega.NewWithT(t)
+	opts := NewOptions()
+
+	flags := flag.NewFlagSet("deny-modify-member-cluster-labels", flag.ExitOnError)
+	opts.AddFlags(flags)
+
+	g.Expect(opts.DenyModifyMemberClusterLabels).To(gomega.BeFalse(), "deny-modify-member-cluster-labels should be false by default")
+}

pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go

@@ -35,23 +35,25 @@ const (
 )
 
 // Add registers the webhook for K8s built-in object types.
-func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool) error {
+func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error {
 	hookServer := mgr.GetWebhookServer()
 	handler := &fleetResourceValidator{
-		client:            mgr.GetClient(),
-		whiteListedUsers:  whiteListedUsers,
-		isFleetV1Beta1API: isFleetV1Beta1API,
-		decoder:           admission.NewDecoder(mgr.GetScheme()),
+		client:                        mgr.GetClient(),
+		whiteListedUsers:              whiteListedUsers,
+		isFleetV1Beta1API:             isFleetV1Beta1API,
+		decoder:                       admission.NewDecoder(mgr.GetScheme()),
+		denyModifyMemberClusterLabels: denyModifyMemberClusterLabels,
 	}
 	hookServer.Register(ValidationPath, &webhook.Admission{Handler: handler})
 	return nil
 }
 
 type fleetResourceValidator struct {
-	client            client.Client
-	whiteListedUsers  []string
-	isFleetV1Beta1API bool
-	decoder           webhook.AdmissionDecoder
+	client                        client.Client
+	whiteListedUsers              []string
+	isFleetV1Beta1API             bool
+	decoder                       webhook.AdmissionDecoder
+	denyModifyMemberClusterLabels bool
 }
 
 // Handle receives the request then allows/denies the request to modify fleet resources.
@@ -133,7 +135,7 @@ func (v *fleetResourceValidator) handleMemberCluster(req admission.Request) admi
 		}
 		isFleetMC := utils.IsFleetAnnotationPresent(oldMC.Annotations)
 		if isFleetMC {
-			return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers)
+			return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers, v.denyModifyMemberClusterLabels)
 		}
 		return validation.ValidatedUpstreamMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers)
 	}

pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go

@@ -634,6 +634,189 @@ func TestHandleMemberCluster(t *testing.T) {
 			},
 			wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "aks-support", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Delete, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})),
 		},
+
+		"allow label modification by system:masters when flag is set to true": {
+			req: admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Name: "test-mc",
+					Object: runtime.RawExtension{
+						Raw: func() []byte {
+							updatedMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value1"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(updatedMC)
+							return raw
+						}(),
+					},
+					OldObject: runtime.RawExtension{
+						Raw: fleetMCObjectBytes,
+					},
+					UserInfo: authenticationv1.UserInfo{
+						Username: "mastersUser",
+						Groups:   []string{"system:masters"},
+					},
+					RequestKind: &utils.MCMetaGVK,
+					Operation:   admissionv1.Update,
+				},
+			},
+			resourceValidator: fleetResourceValidator{
+				decoder:                       decoder,
+				denyModifyMemberClusterLabels: true,
+			},
+			wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "mastersUser", utils.GenerateGroupString([]string{"system:masters"}), admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})),
+		},
+		"deny label modification by non system:masters user when flag is set to true": {
+			req: admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Name: "test-mc",
+					Object: runtime.RawExtension{
+						Raw: func() []byte {
+							updatedMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value1"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(updatedMC)
+							return raw
+						}(),
+					},
+					OldObject: runtime.RawExtension{
+						Raw: func() []byte {
+							oldMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value2"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(oldMC)
+							return raw
+						}(),
+					},
+					UserInfo: authenticationv1.UserInfo{
+						Username: "nonSystemMastersUser",
+						Groups:   []string{"system:authenticated"},
+					},
+					RequestKind: &utils.MCMetaGVK,
+					Operation:   admissionv1.Update,
+				},
+			},
+			resourceValidator: fleetResourceValidator{
+				decoder:                       decoder,
+				denyModifyMemberClusterLabels: true,
+			},
+			wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)),
+		},
+		"deny label modification by fleet agent when flag is set to true": {
+			req: admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Name: "test-mc",
+					Object: runtime.RawExtension{
+						Raw: func() []byte {
+							updatedMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value1"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(updatedMC)
+							return raw
+						}(),
+					},
+					OldObject: runtime.RawExtension{
+						Raw: func() []byte {
+							oldMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value2"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(oldMC)
+							return raw
+						}(),
+					},
+					UserInfo: authenticationv1.UserInfo{
+						Username: "system:serviceaccount:fleet-system:hub-agent-sa",
+						Groups:   []string{"system:serviceaccounts"},
+					},
+					RequestKind: &utils.MCMetaGVK,
+					Operation:   admissionv1.Update,
+				},
+			},
+			resourceValidator: fleetResourceValidator{
+				decoder:                       decoder,
+				denyModifyMemberClusterLabels: true,
+			},
+			wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)),
+		},
+		"allow label modification by non system:masters resources when flag is set to false": {
+			req: admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Name: "test-mc",
+					Object: runtime.RawExtension{
+						Raw: func() []byte {
+							updatedMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value1"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(updatedMC)
+							return raw
+						}(),
+					},
+					OldObject: runtime.RawExtension{
+						Raw: func() []byte {
+							oldMC := &clusterv1beta1.MemberCluster{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:   "test-mc",
+									Labels: map[string]string{"key1": "value2"},
+									Annotations: map[string]string{
+										"fleet.azure.com/cluster-resource-id": "test-cluster-resource-id",
+									},
+								},
+							}
+							raw, _ := json.Marshal(oldMC)
+							return raw
+						}(),
+					},
+					UserInfo: authenticationv1.UserInfo{
+						Username: "nonMastersUser",
+						Groups:   []string{"system:authenticated"},
+					},
+					RequestKind: &utils.MCMetaGVK,
+					Operation:   admissionv1.Update,
+				},
+			},
+			resourceValidator: fleetResourceValidator{
+				decoder:                       decoder,
+				denyModifyMemberClusterLabels: false,
+			},
+			wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat,
+				"nonMastersUser", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &utils.MCMetaGVK, "",
+				types.NamespacedName{Name: "test-mc"})),
+		},
 	}
 
 	for testName, testCase := range testCases {

pkg/webhook/validation/uservalidation.go

@@ -32,10 +32,11 @@ const (
 	aksSupportUser            = "aks-support"
 	serviceAccountFmt         = "system:serviceaccount:fleet-system:%s"
 
-	allowedModifyResource       = "user in groups is allowed to modify resource"
-	deniedModifyResource        = "user in groups is not allowed to modify resource"
-	deniedAddFleetAnnotation    = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster"
-	deniedRemoveFleetAnnotation = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster"
+	allowedModifyResource           = "user in groups is allowed to modify resource"
+	deniedModifyResource            = "user in groups is not allowed to modify resource"
+	deniedAddFleetAnnotation        = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster"
+	deniedRemoveFleetAnnotation     = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster"
+	DeniedModifyMemberClusterLabels = "users are not allowed to modify labels through hub cluster directly"
 
 	ResourceAllowedFormat      = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v"
 	ResourceDeniedFormat       = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v"
@@ -62,7 +63,7 @@ func ValidateUserForFleetCRD(req admission.Request, whiteListedUsers []string, g
 func ValidateUserForResource(req admission.Request, whiteListedUsers []string) admission.Response {
 	namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace}
 	userInfo := req.UserInfo
-	if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isNodeGroupUser(userInfo) || isAKSSupportUser(userInfo) {
+	if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isUserInGroup(userInfo, nodeGroup) || isAKSSupportUser(userInfo) {
 		klog.V(3).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, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName))
 	}
@@ -93,7 +94,7 @@ func ValidateV1Alpha1MemberClusterUpdate(currentMC, oldMC fleetv1alpha1.MemberCl
 }
 
 // ValidateFleetMemberClusterUpdate checks to see if user had updated the fleet member cluster resource and allows/denies the request.
-func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string) admission.Response {
+func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string, denyModifyMemberClusterLabels bool) admission.Response {
 	namespacedName := types.NamespacedName{Name: currentMC.GetName()}
 	userInfo := req.UserInfo
 	if areAllFleetAnnotationsRemoved(currentMC.Annotations, oldMC.Annotations) {
@@ -107,6 +108,17 @@ func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberClus
 	if err != nil {
 		return admission.Denied(err.Error())
 	}
+
+	// Users are no longer allowed to modify labels of fleet member cluster through webhook.
+	// This will be disabled until member labels are accessible through CLI
+	if denyModifyMemberClusterLabels {
+		isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels())
+		if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) {
+			klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
+			return admission.Denied(DeniedModifyMemberClusterLabels)
+		}
+	}
+
 	isAnnotationUpdated := isFleetAnnotationUpdated(currentMC.Annotations, oldMC.Annotations)
 	if isObjUpdated || isAnnotationUpdated {
 		return ValidateUserForResource(req, whiteListedUsers)
@@ -162,9 +174,9 @@ func isAKSSupportUser(userInfo authenticationv1.UserInfo) bool {
 	return userInfo.Username == aksSupportUser
 }
 
-// isNodeGroupUser returns true if user belongs to system:nodes group.
-func isNodeGroupUser(userInfo authenticationv1.UserInfo) bool {
-	return slices.Contains(userInfo.Groups, nodeGroup)
+// isUserInGroup returns true if user belongs to the specified groupName.
+func isUserInGroup(userInfo authenticationv1.UserInfo, groupName string) bool {
+	return slices.Contains(userInfo.Groups, groupName)
 }
 
 // isMemberClusterMapFieldUpdated return true if member cluster label is updated.