Fix impersonation for non-system users

On-behalf-of: @SAP mangirdas.judeikis@sap.com
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>

pkg/admission/reservednames/admission.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"io"
 
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/admission"
@@ -80,6 +81,11 @@ func NewReservedNames() *ReservedNames {
 				tenancyv1alpha1.Kind("WorkspaceType"),
 				tenancyv1alpha1.WorkspaceTypeReservedNames()...,
 			),
+			newReservedNameFn(
+				corev1.Resource("serviceaccounts"),
+				corev1.SchemeGroupVersion.WithKind("ServiceAccount").GroupKind(),
+				"kcp-rest",
+			),
 		},
 	}
 }

pkg/authorization/bootstrap/policy.go

@@ -46,6 +46,21 @@ const (
 	SystemKcpWorkspaceAccessGroup = "system:kcp:workspace:access"
 )
 
+const (
+	// SystemMastersGroup is the group inherited from k8s codebase - all powerful, all knowing!
+	// Users should not be added to this group.
+	SystemMastersGroup = "system:masters"
+)
+
+const (
+	// SystemKcpAdminUser is the user that has all permissions across all workspaces.
+	SystemKcpAdminUser = "kcp-admin"
+	// SystemShardAdmin is the user that has all permissions across all workspaces.
+	SystemShardAdmin = "shard-admin"
+	// SystemServiceAccountDefaultRest is the default service account for fake rest client.
+	SystemServiceAccountDefaultRest = "system:serviceaccount:default:kcp-rest"
+)
+
 // ClusterRoleBindings return default rolebindings to the default roles.
 func clusterRoleBindings() []rbacv1.ClusterRoleBinding {
 	return []rbacv1.ClusterRoleBinding{

pkg/proxy/server.go

@@ -69,11 +69,13 @@ func NewServer(ctx context.Context, c CompletedConfig) (*Server, error) {
 			return shardClient, nil
 		},
 	)
-
 	handler, err := NewHandler(ctx, s.CompletedConfig.Options, s.IndexController)
 	if err != nil {
 		return s, err
 	}
+	// This must run before WithImpersonation upstream handler to
+	// catch this before user data is lost.
+	handler = server.WithImpersonationGatekeeper(handler)
 
 	failedHandler := frontproxyfilters.NewUnauthorizedHandler()
 	handler = frontproxyfilters.WithOptionalAuthentication(

pkg/server/handler.go

@@ -30,6 +30,7 @@ import (
 	"github.com/emicklei/go-restful/v3"
 	jwt2 "gopkg.in/square/go-jose.v2/jwt"
 
+	authenticationapi "k8s.io/api/authentication/v1"
 	apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"
 	"k8s.io/apiextensions-apiserver/pkg/kcp"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -47,6 +48,8 @@ import (
 	clientgotransport "k8s.io/client-go/transport"
 	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/pkg/controlplane/apiserver/miniaggregator"
+
+	authorizationbootstrap "github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 )
 
 var (
@@ -253,6 +256,86 @@ func WithRequestIdentity(handler http.Handler) http.Handler {
 	})
 }
 
+var (
+	// impersonationGroups is a set of groups that are not allowed to impersonate
+	// if the user does not have a specific group or lower ID (lower = higher permissions).
+	impersonationGroups = map[string]int{
+		authorizationbootstrap.SystemMastersGroup:  0,
+		authorizationbootstrap.SystemKcpAdminGroup: 1,
+	}
+)
+
+// WithImpersonationGatekeeper checks the request for impersonations and validates them,
+// if they are valid. If they are not, will return a 403.
+// We check for impersonation in the request headers, early to avoid it being propagated to
+// the backend services.
+func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		// Impersonation check is only done when impersonation is requested.
+		// And impersonations is only allowed for the users, who have metadata in the ctx.
+		impersonationUser := req.Header.Get(authenticationapi.ImpersonateUserHeader)
+		if len(impersonationUser) != 0 {
+			requestor, exists := request.UserFrom(req.Context())
+			if !exists {
+				responsewriters.ErrorNegotiated(
+					apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("impersonation is not allowed for the requestor")),
+					errorCodecs, schema.GroupVersion{}, w, req)
+			}
+
+			if checkImpersonation(requestor.GetGroups(), req.Header[authenticationapi.ImpersonateGroupHeader]) {
+				handler.ServeHTTP(w, req)
+				return
+			}
+		}
+
+		responsewriters.ErrorNegotiated(
+			apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("impersonation is not allowed for the requestor")),
+			errorCodecs, schema.GroupVersion{}, w, req)
+	})
+}
+
+func checkImpersonation(userGroups []string, requestedGroups []string) bool {
+	// Convert user groups to their hierarchy levels
+	userLevels := []int{}
+	for _, userGroup := range userGroups {
+		if level, exists := impersonationGroups[userGroup]; exists {
+			userLevels = append(userLevels, level)
+		}
+	}
+
+	// If no valid user groups found, deny impersonation
+	if len(userLevels) == 0 {
+		return false
+	}
+
+	// Iterate through each requested group and verify permissions
+	for _, requestedGroup := range requestedGroups {
+		requestedLevel, exists := impersonationGroups[requestedGroup]
+
+		// If requested group is not known, treat it as the lowest level
+		if !exists {
+			requestedLevel = int(^uint(0) >> 1) // Max int value (lowest level)
+		}
+
+		// Check if any user group level allows impersonation of the requested group
+		allowed := false
+		for _, userLevel := range userLevels {
+			if userLevel <= requestedLevel {
+				allowed = true
+				break
+			}
+		}
+
+		// If no user group allows impersonation of this requested group, deny impersonation
+		if !allowed {
+			return false
+		}
+	}
+
+	// If all requested groups are allowed, return true
+	return true
+}
+
 func processResourceIdentity(req *http.Request, requestInfo *request.RequestInfo) (*http.Request, error) {
 	if !requestInfo.IsResourceRequest {
 		return req, nil

pkg/server/handler_test.go

@@ -21,6 +21,7 @@ import (
 	"net/url"
 	"testing"
 
+	authorizationbootstrap "github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 	"github.com/stretchr/testify/require"
 
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -153,3 +154,79 @@ func TestProcessResourceIdentity(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckImpersonation(t *testing.T) {
+	var systemUserGroup = "system:user:group"
+	nonExistingGroup := "non-existing-group"
+	impersonationGroups = map[string]int{
+		authorizationbootstrap.SystemMastersGroup:  0,
+		authorizationbootstrap.SystemKcpAdminGroup: 1,
+		systemUserGroup: 2,
+	}
+
+	tests := []struct {
+		name            string
+		userGroups      []string
+		requestedGroups []string
+		expectedResult  bool
+	}{
+		{
+			name:            "Single group - allowed",
+			userGroups:      []string{authorizationbootstrap.SystemMastersGroup},
+			requestedGroups: []string{authorizationbootstrap.SystemKcpAdminGroup},
+			expectedResult:  true,
+		},
+		{
+			name:            "Multiple groups - allowed",
+			userGroups:      []string{authorizationbootstrap.SystemMastersGroup},
+			requestedGroups: []string{authorizationbootstrap.SystemKcpAdminGroup, systemUserGroup},
+			expectedResult:  true,
+		},
+		{
+			name:            "Single group - not allowed",
+			userGroups:      []string{authorizationbootstrap.SystemKcpAdminGroup},
+			requestedGroups: []string{authorizationbootstrap.SystemMastersGroup},
+			expectedResult:  false,
+		},
+		{
+			name:            "Multiple groups - mixed permissions",
+			userGroups:      []string{authorizationbootstrap.SystemKcpAdminGroup},
+			requestedGroups: []string{authorizationbootstrap.SystemMastersGroup, systemUserGroup},
+			expectedResult:  false,
+		},
+		{
+			name:            "Multiple groups - lower permissions only",
+			userGroups:      []string{systemUserGroup},
+			requestedGroups: []string{authorizationbootstrap.SystemKcpAdminGroup, authorizationbootstrap.SystemMastersGroup},
+			expectedResult:  false,
+		},
+		{
+			name:            "Empty user groups",
+			userGroups:      []string{},
+			requestedGroups: []string{authorizationbootstrap.SystemKcpAdminGroup},
+			expectedResult:  false,
+		},
+		{
+			name:            "Empty requested groups",
+			userGroups:      []string{authorizationbootstrap.SystemMastersGroup},
+			requestedGroups: []string{},
+			expectedResult:  true,
+		},
+		{
+			name:            "Unknown requested group",
+			userGroups:      []string{authorizationbootstrap.SystemMastersGroup},
+			requestedGroups: []string{nonExistingGroup},
+			expectedResult:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := checkImpersonation(tt.userGroups, tt.requestedGroups)
+			if result != tt.expectedResult {
+				t.Errorf("checkImpersonation(%v, %v) = %v; want %v",
+					tt.userGroups, tt.requestedGroups, result, tt.expectedResult)
+			}
+		})
+	}
+}

pkg/virtual/apiexport/builder/build.go

@@ -36,7 +36,7 @@ import (
 	"k8s.io/klog/v2"
 
 	"github.com/kcp-dev/kcp/pkg/authorization"
-	"github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
+	authorizationbootstrap "github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 	virtualapiexportauth "github.com/kcp-dev/kcp/pkg/virtual/apiexport/authorizer"
 	"github.com/kcp-dev/kcp/pkg/virtual/apiexport/controllers/apireconciler"
 	"github.com/kcp-dev/kcp/pkg/virtual/apiexport/schemas"
@@ -116,8 +116,8 @@ func BuildVirtualWorkspace(
 
 				impersonationConfig := rest.CopyConfig(cfg)
 				impersonationConfig.Impersonate = rest.ImpersonationConfig{
-					UserName: "system:serviceaccount:default:rest",
-					Groups:   []string{bootstrap.SystemKcpAdminGroup},
+					UserName: authorizationbootstrap.SystemServiceAccountDefaultRest,
+					Groups:   []string{authorizationbootstrap.SystemKcpAdminGroup},
 					Extra: map[string][]string{
 						serviceaccount.ClusterNameKey: {cluster.Name.Path().String()},
 					},

test/e2e/authorizer/impersonate_test.go

@@ -0,0 +1,112 @@
+/*
+Copyright 2024 The KCP Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package authorizer
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"
+	"github.com/stretchr/testify/require"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/rest"
+
+	"github.com/kcp-dev/kcp/test/e2e/framework"
+)
+
+func TestImpersonation(t *testing.T) {
+	t.Parallel()
+	framework.Suite(t, "control-plane")
+
+	ctx, cancelFn := context.WithCancel(context.Background())
+	t.Cleanup(cancelFn)
+
+	server := framework.SharedKcpServer(t)
+	cfg := server.BaseConfig(t)
+	user1workspace, _ := framework.NewOrganizationFixture(t, server)
+	user2workspace, _ := framework.NewOrganizationFixture(t, server)
+
+	kubeClusterClient, err := kcpkubernetesclientset.NewForConfig(cfg)
+	require.NoError(t, err)
+
+	t.Log("Make user-1 an admin of the workspace1")
+	framework.AdmitWorkspaceAccess(ctx, t, kubeClusterClient, user1workspace, []string{"user-1"}, nil, true)
+	user1Cfg := framework.StaticTokenUserConfig("user-1", cfg)
+	user1KubeClusterClient, err := kcpkubernetesclientset.NewForConfig(user1Cfg)
+	require.NoError(t, err)
+
+	t.Log("User-1 should be able to read secrets")
+	require.Eventually(t, func() bool {
+		_, err := user1KubeClusterClient.Cluster(user1workspace).CoreV1().Secrets("default").List(ctx, metav1.ListOptions{})
+		return err == nil
+	}, wait.ForeverTestTimeout, time.Millisecond*100, "user-1 should be able to read secrets")
+
+	t.Log("Make user-2 an admin of the worksapce2")
+	framework.AdmitWorkspaceAccess(ctx, t, kubeClusterClient, user2workspace, []string{"user-2"}, nil, true)
+	user2Cfg := framework.StaticTokenUserConfig("user-2", cfg)
+	user2KubeClusterClient, err := kcpkubernetesclientset.NewForConfig(user2Cfg)
+	require.NoError(t, err)
+
+	t.Log("User-2 should be able to read secrets")
+	require.Eventually(t, func() bool {
+		_, err := user2KubeClusterClient.Cluster(user2workspace).CoreV1().Secrets("default").List(ctx, metav1.ListOptions{})
+		return err == nil
+	}, wait.ForeverTestTimeout, time.Millisecond*100, "user-2 should be able to read secrets")
+
+	t.Log("User-1 should NOT be able to read secrets in user-2 workspace")
+	require.Eventually(t, func() bool {
+		_, err := user1KubeClusterClient.Cluster(user2workspace).CoreV1().Secrets("default").List(ctx, metav1.ListOptions{})
+		return apierrors.IsForbidden(err)
+	}, wait.ForeverTestTimeout, time.Millisecond*100, "User-1 should NOT be able to read secrets in user-2 workspace")
+
+	t.Log("Make user-1 impersonate user-2 as system:masters")
+	impersonationConfig := rest.CopyConfig(user1Cfg)
+	impersonationConfig.Impersonate = rest.ImpersonationConfig{
+		UserName: "user-2",
+		Groups:   []string{"system:masters"},
+	}
+	impersonatedClient, err := kcpkubernetesclientset.NewForConfig(impersonationConfig)
+	require.NoError(t, err)
+
+	t.Log("As user-1 with system:masters, we should NOT be able to read secrets in user 2 workspace")
+	require.Eventually(t, func() bool {
+		_, err := impersonatedClient.Cluster(user2workspace).CoreV1().Secrets("default").List(ctx, metav1.ListOptions{})
+		return apierrors.IsForbidden(err)
+	}, wait.ForeverTestTimeout, time.Millisecond*100, "as user-1 with system:masters, we should NOT be able to read secrets")
+
+	t.Log("Make user-1 impersonate system:serviceaccount:default:kcp-rest as system:kcp:admin")
+	impersonationKCPAdminConfig := rest.CopyConfig(user1Cfg)
+	impersonationKCPAdminConfig.Impersonate = rest.ImpersonationConfig{
+		UserName: "system:serviceaccount:default:kcp-rest",
+		Groups:   []string{"system:kcp:admin"},
+	}
+	impersonatedKCPAdminClient, err := kcpkubernetesclientset.NewForConfig(impersonationKCPAdminConfig)
+	require.NoError(t, err)
+
+	t.Log("As user-1 with kcp-admin:system:kcp:admin, we should NOT be able to read secrets in user1 workspace")
+	require.Eventually(t, func() bool {
+		_, err := impersonatedKCPAdminClient.Cluster(user2workspace).CoreV1().Secrets("default").List(ctx, metav1.ListOptions{})
+		return apierrors.IsForbidden(err)
+	}, wait.ForeverTestTimeout, time.Millisecond*100, "as user-1 with kcp-admin:system:kcp:admin, we should NOT be able to read secrets")
+
+	// TODO: Add test to check for warrant impersonation once https://github.com/kcp-dev/kcp/pull/3156/
+	// is merged and the feature is available in the API server.
+}