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,17 @@ 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 (
+	// 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"
 
+	authenticationv1 "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"
@@ -41,12 +42,15 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	utilnet "k8s.io/apimachinery/pkg/util/net"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apiserver/pkg/authentication/user"
 	apiserverdiscovery "k8s.io/apiserver/pkg/endpoints/discovery"
 	"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	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 +257,86 @@ func WithRequestIdentity(handler http.Handler) http.Handler {
 	})
 }
 
+type privilege int
+
+const (
+	superPrivileged privilege = iota
+	priviledged
+	unprivileged
+)
+
+var (
+	// specialGroups specify groups with special meaning kcp. Lower privilege (= higher number)
+	// cannot impersonate higher privilege levels.
+	specialGroups = map[string]privilege{
+		authorizationbootstrap.SystemMastersGroup:  superPrivileged,
+		authorizationbootstrap.SystemKcpAdminGroup: priviledged,
+	}
+)
+
+// 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.
+		// Else just pass the request.
+		impersonationUser := req.Header.Get(authenticationv1.ImpersonateUserHeader)
+		var requestor user.Info
+		if len(impersonationUser) != 0 {
+			var exists bool
+			requestor, exists = request.UserFrom(req.Context())
+			if !exists {
+				responsewriters.ErrorNegotiated(
+					apierrors.NewForbidden(schema.GroupResource{}, "", fmt.Errorf("impersonation is invalid for the requestor")),
+					errorCodecs, schema.GroupVersion{}, w, req)
+				return
+			}
+			if validImpersonation(requestor.GetGroups(), req.Header[authenticationv1.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)
+			return
+		}
+		handler.ServeHTTP(w, req)
+	})
+}
+
+// validImpersonation checks if a user with given privilege levels can impersonate requested groups.
+func validImpersonation(userGroups []string, requestedGroups []string) bool {
+	userMaxPrivilege := unprivileged // Start with the lowest privilege
+
+	// Determine the highest privilege the user has
+	for _, userGroup := range userGroups {
+		if priv, exists := specialGroups[userGroup]; exists && priv < userMaxPrivilege {
+			userMaxPrivilege = priv
+		}
+	}
+
+	// Iterate through each requested group and verify if user's privilege allows impersonation
+	for _, requestedGroup := range requestedGroups {
+		requestedPrivilege, exists := specialGroups[requestedGroup]
+		// If requested group is not recognized, treat it as the lowest privilege level
+		if !exists {
+			requestedPrivilege = unprivileged
+		}
+
+		// Check if user's max privilege is less than or equal to the requested group's privilege
+		if userMaxPrivilege > requestedPrivilege {
+			return false // Deny impersonation if user's privilege is not sufficient
+		}
+	}
+
+	// If all checks pass, impersonation is allowed
+	return true
+}
+
 func processResourceIdentity(req *http.Request, requestInfo *request.RequestInfo) (*http.Request, error) {
 	if !requestInfo.IsResourceRequest {
 		return req, nil

pkg/server/handler_test.go

@@ -25,6 +25,8 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/endpoints/request"
+
+	authorizationbootstrap "github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 )
 
 func TestProcessResourceIdentity(t *testing.T) {
@@ -153,3 +155,79 @@ func TestProcessResourceIdentity(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckImpersonation(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
+		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 := validImpersonation(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.
+}