[v0.8] Introduce username validation (#1023) (#1026)

* Add update validation for User.UserName (#943)

* Add update validation for User.UserName

* Fix unit test

* Move username validation to happen before manage-users check (#1016)

Author: JonCrowther

docs.md

@@ -356,6 +356,12 @@ When a user is updated or deleted, a check occurs to ensure that the user making
 
 If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
 
+#### Invalid Fields - Update
+
+Users can update the following fields if they had not been set. But after getting initial values, the fields cannot be changed:
+
+- UserName
+
 ## UserAttribute 
 
 ### Validation Checks

pkg/resources/management.cattle.io/v3/users/User.md

@@ -4,4 +4,10 @@
 
 When a user is updated or deleted, a check occurs to ensure that the user making the request has permissions greater than or equal to the user being updated or deleted. To get the user's groups, the user's UserAttributes are checked. This is best effort, because UserAttributes are only updated when a User logs in, so it may not be perfectly up to date.
 
-If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
+If the user making the request has the verb `manage-users` for the resource `users`, then it is allowed to bypass the check. Note that the wildcard `*` includes the `manage-users` verb.
+
+### Invalid Fields - Update
+
+Users can update the following fields if they had not been set. But after getting initial values, the fields cannot be changed:
+
+- UserName

pkg/resources/management.cattle.io/v3/users/validator.go

@@ -13,6 +13,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
 	"k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -71,6 +72,19 @@ func (v *Validator) Admitters() []admission.Admitter {
 }
 
 func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
+	oldUser, newUser, err := objectsv3.UserOldAndNewFromRequest(&request.AdmissionRequest)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get current User from request: %w", err)
+	}
+
+	// Validate update fields
+	fieldPath := field.NewPath("user")
+	if request.Operation == admissionv1.Update {
+		if err := validateUpdateFields(oldUser, newUser, fieldPath); err != nil {
+			return admission.ResponseBadRequest(err.Error()), nil
+		}
+	}
+
 	// Check if requester has manage-user verb
 	hasManageUsers, err := auth.RequestUserHasVerb(request, gvr, a.sar, manageUsersVerb, "", "")
 	if err != nil {
@@ -80,26 +94,22 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	if hasManageUsers {
 		return &admissionv1.AdmissionResponse{Allowed: true}, nil
 	}
-	userObj, err := objectsv3.UserFromRequest(&request.AdmissionRequest)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get current User from request: %w", err)
-	}
 
 	// Need the UserAttribute to find the groups
-	userAttribute, err := a.userAttributeCache.Get(userObj.Name)
+	userAttribute, err := a.userAttributeCache.Get(oldUser.Name)
 	if err != nil && !apierrors.IsNotFound(err) {
-		return nil, fmt.Errorf("failed to get UserAttribute for %s: %w", userObj.Name, err)
+		return nil, fmt.Errorf("failed to get UserAttribute for %s: %w", oldUser.Name, err)
 	}
 
 	userInfo := &user.DefaultInfo{
-		Name:   userObj.Name,
+		Name:   oldUser.Name,
 		Groups: getGroupsFromUserAttribute(userAttribute),
 	}
 
 	// Get all rules for the user being modified
 	rules, err := a.resolver.RulesFor(userInfo, "")
 	if err != nil {
-		return nil, fmt.Errorf("failed to get rules for user %v: %w", userObj, err)
+		return nil, fmt.Errorf("failed to get rules for user %v: %w", oldUser, err)
 	}
 
 	// Ensure that rules of the user being modified aren't greater than the rules of the user making the request
@@ -132,3 +142,12 @@ func getGroupsFromUserAttribute(userAttribute *v3.UserAttribute) []string {
 	}
 	return result
 }
+
+// validateUpdateFields validates fields during an update. The manage-users verb does not apply to these validations.
+func validateUpdateFields(oldUser, newUser *v3.User, fieldPath *field.Path) error {
+	const reason = "field is immutable"
+	if oldUser.Username != "" && oldUser.Username != newUser.Username {
+		return field.Invalid(fieldPath.Child("username"), newUser.Username, reason)
+	}
+	return nil
+}

pkg/resources/management.cattle.io/v3/users/validator_test.go

@@ -35,6 +35,7 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name: defaultUserName,
 		},
+		Username: defaultUserName,
 	}
 	getPods = []rbacv1.PolicyRule{
 		{
@@ -114,12 +115,12 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
-					return getPods, nil
-				} else if s == defaultUserName {
+				switch s {
+				case requesterUserName, defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -129,12 +130,12 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
-					return getPods, nil
-				} else if s == defaultUserName {
+				switch s {
+				case requesterUserName, defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -143,12 +144,14 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return starPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -158,12 +161,14 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return starPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return getPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: true,
 		},
@@ -172,12 +177,14 @@ func Test_Admit(t *testing.T) {
 			oldUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return getPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return starPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: false,
 		},
@@ -187,15 +194,54 @@ func Test_Admit(t *testing.T) {
 			newUser:         defaultUser.DeepCopy(),
 			requestUserName: requesterUserName,
 			resolverRulesFor: func(s string) ([]rbacv1.PolicyRule, error) {
-				if s == requesterUserName {
+				switch s {
+				case requesterUserName:
 					return getPods, nil
-				} else if s == defaultUserName {
+				case defaultUserName:
 					return starPods, nil
+				default:
+					return nil, fmt.Errorf("unexpected error")
 				}
-				return nil, fmt.Errorf("unexpected error")
 			},
 			allowed: false,
 		},
+		{
+			name:    "changing the username not allowed",
+			oldUser: defaultUser.DeepCopy(),
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: defaultUserName,
+				},
+				Username: "new-username",
+			},
+			requestUserName: requesterUserName,
+			allowed:         false,
+		},
+		{
+			name: "adding a new username allowed",
+			oldUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: defaultUserName,
+				},
+			},
+			newUser:         defaultUser.DeepCopy(),
+			requestUserName: requesterUserName,
+			resolverRulesFor: func(string) ([]rbacv1.PolicyRule, error) {
+				return getPods, nil
+			},
+			allowed: true,
+		},
+		{
+			name:    "removing username not allowed",
+			oldUser: defaultUser.DeepCopy(),
+			newUser: &v3.User{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: defaultUserName,
+				},
+			},
+			requestUserName: requesterUserName,
+			allowed:         false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {