Merge pull request #3267 from sttts/sttts-impersonation-guard-complete

🐛 impersonation: add missing privileged groups

Author: kcp-ci-bot

pkg/server/filters/impersonation.go

@@ -46,9 +46,12 @@ var (
 	// specialGroups specify groups with special meaning kcp. Lower privilege (= lower number)
 	// cannot impersonate higher privilege levels.
 	specialGroups = map[string]privilege{
-		authorizationbootstrap.SystemMastersGroup:  superPrivileged,
-		authorizationbootstrap.SystemKcpAdminGroup: priviledged,
-		user.AllAuthenticated:                      authenticated,
+		authorizationbootstrap.SystemMastersGroup:                superPrivileged,
+		authorizationbootstrap.SystemLogicalClusterAdmin:         priviledged,
+		authorizationbootstrap.SystemExternalLogicalClusterAdmin: priviledged,
+		authorizationbootstrap.SystemKcpWorkspaceBootstrapper:    priviledged,
+		authorizationbootstrap.SystemKcpAdminGroup:               priviledged,
+		user.AllAuthenticated:                                    authenticated,
 	}
 )
 
@@ -177,33 +180,20 @@ func WithImpersonationScoping(handler http.Handler) http.Handler {
 	})
 }
 
-// maxUserPrivilege returns the highest privilege level found among the user's groups.
-func maxUserPrivilege(userGroups []string) privilege {
-	max := unprivileged
-	for _, g := range userGroups {
-		if p, found := specialGroups[g]; found && p > max {
-			max = p
+// validImpersonation checks if a user can impersonate all requested groups.
+func validImpersonation(existingGroups, requestedGroups []string) bool {
+	for _, g := range existingGroups {
+		if g == authorizationbootstrap.SystemMastersGroup {
+			return true
 		}
 	}
-	return max
-}
 
-// validImpersonation checks if a user can impersonate all requested groups.
-func validImpersonation(userGroups, requestedGroups []string) bool {
-	userMax := maxUserPrivilege(userGroups)
-
-	// Case 1: User is requesting to impersonate a group with higher privilege.
+	existing := sets.New(existingGroups...)
 	for _, g := range requestedGroups {
-		if userMax < specialGroups[g] {
-			return false
+		if specialGroups[g] != unprivileged && !existing.Has(g) {
+			return false // only impersonate non-unprivileged groups the user already has.
 		}
 	}
-	// Case 2: User is requesting to impersonate a `system:authenticated` group without having the group itself and not being privileged.
-	// This is very much academic, as all users reaching this point will have the `system:authenticated` group or be privileged.
-	if sets.New(requestedGroups...).Has(user.AllAuthenticated) &&
-		!(sets.New(userGroups...).HasAny(user.AllAuthenticated, authorizationbootstrap.SystemMastersGroup, authorizationbootstrap.SystemKcpAdminGroup)) {
-		return false
-	}
 
 	return true
 }

pkg/server/filters/impersonation_test.go

@@ -33,14 +33,9 @@ import (
 	authorizationbootstrap "github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 )
 
-func TestCheckImpersonation(t *testing.T) {
+func TestValidImpersonation(t *testing.T) {
 	var systemUserGroup = "system:user:group"
 	nonExistingGroup := "non-existing-group"
-	specialGroups = map[string]privilege{
-		authorizationbootstrap.SystemMastersGroup:  superPrivileged,
-		authorizationbootstrap.SystemKcpAdminGroup: priviledged,
-		systemUserGroup: unprivileged,
-	}
 
 	tests := []struct {
 		name            string
@@ -79,7 +74,7 @@ func TestCheckImpersonation(t *testing.T) {
 			expectedResult:  false,
 		},
 		{
-			name:            "system:autenticated is beyond unprivileged",
+			name:            "system:authenticated is beyond unprivileged",
 			userGroups:      []string{systemUserGroup},
 			requestedGroups: []string{user.AllAuthenticated},
 			expectedResult:  false,