Test double impersonation as the cluster admin

internal/concierge/impersonator/impersonator.go

@@ -61,12 +61,12 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 	var listener net.Listener
 
 	constructServer := func() (func(stopCh <-chan struct{}) error, error) {
-		// bare minimum server side scheme to allow for status messages to be encoded
+		// Bare minimum server side scheme to allow for status messages to be encoded.
 		scheme := runtime.NewScheme()
 		metav1.AddToGroupVersion(scheme, metav1.Unversioned)
 		codecs := serializer.NewCodecFactory(scheme)
 
-		// this is unused for now but it is a safe value that we could use in the future
+		// This is unused for now but it is a safe value that we could use in the future.
 		defaultEtcdPathPrefix := "/pinniped-impersonation-proxy-registry"
 
 		recommendedOptions := genericoptions.NewRecommendedOptions(
@@ -77,18 +77,22 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 		recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider // serving certs (end user facing)
 		recommendedOptions.SecureServing.BindPort = port
 
-		// wire up the impersonation proxy signer CA as a valid authenticator for client cert auth
+		// Wire up the impersonation proxy signer CA as another valid authenticator for client cert auth,
-		// TODO fix comments
+		// along with the Kube API server's CA.
 		kubeClient, err := kubeclient.New(clientOpts...)
 		if err != nil {
 			return nil, err
 		}
-		kubeClientCA, err := dynamiccertificates.NewDynamicCAFromConfigMapController("client-ca", metav1.NamespaceSystem, "extension-apiserver-authentication", "client-ca-file", kubeClient.Kubernetes)
+		kubeClientCA, err := dynamiccertificates.NewDynamicCAFromConfigMapController(
+			"client-ca", metav1.NamespaceSystem, "extension-apiserver-authentication", "client-ca-file", kubeClient.Kubernetes,
+		)
 		if err != nil {
 			return nil, err
 		}
 		recommendedOptions.Authentication.ClientCert.ClientCA = "---irrelevant-but-needs-to-be-non-empty---" // drop when we pick up https://github.com/kubernetes/kubernetes/pull/100055
-		recommendedOptions.Authentication.ClientCert.CAContentProvider = dynamiccertificates.NewUnionCAContentProvider(impersonationProxySignerCA, kubeClientCA)
+		recommendedOptions.Authentication.ClientCert.CAContentProvider = dynamiccertificates.NewUnionCAContentProvider(
+			impersonationProxySignerCA, kubeClientCA,
+		)
 
 		if recOpts != nil {
 			recOpts(recommendedOptions)
@@ -108,16 +112,16 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 			return nil, err
 		}
 
-		// loopback authentication to this server does not really make sense since we just proxy everything to KAS
+		// Loopback authentication to this server does not really make sense since we just proxy everything to
-		// thus we replace loopback connection config with one that does direct connections to KAS
+		// the Kube API server, thus we replace loopback connection config with one that does direct connections
-		// loopback config is mainly used by post start hooks, so this is mostly future proofing
+		// the Kube API server. Loopback config is mainly used by post start hooks, so this is mostly future proofing.
 		serverConfig.LoopbackClientConfig = rest.CopyConfig(kubeClient.ProtoConfig) // assume proto is safe (hooks can override)
-		// remove the bearer token so our authorizer does not get stomped on by AuthorizeClientBearerToken
+		// Remove the bearer token so our authorizer does not get stomped on by AuthorizeClientBearerToken.
-		// see sanity checks at the end of this function
+		// See sanity checks at the end of this function.
 		serverConfig.LoopbackClientConfig.BearerToken = ""
 
-		// assume proto config is safe because transport level configs do not use rest.ContentConfig
+		// Assume proto config is safe because transport level configs do not use rest.ContentConfig.
-		// thus if we are interacting with actual APIs, they should be using pre-built clients
+		// Thus if we are interacting with actual APIs, they should be using pre-built clients.
 		impersonationProxy, err := newImpersonationReverseProxy(rest.CopyConfig(kubeClient.ProtoConfig))
 		if err != nil {
 			return nil, err
@@ -125,23 +129,22 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 
 		defaultBuildHandlerChainFunc := serverConfig.BuildHandlerChainFunc
 		serverConfig.BuildHandlerChainFunc = func(_ http.Handler, c *genericapiserver.Config) http.Handler {
-			// we ignore the passed in handler because we never have any REST APIs to delegate to
+			// We ignore the passed in handler because we never have any REST APIs to delegate to.
 			handler := defaultBuildHandlerChainFunc(impersonationProxy, c)
 			handler = securityheader.Wrap(handler)
 			return handler
 		}
 
-		// TODO integration test this authorizer logic with system:masters + double impersonation
+		// Overwrite the delegating authorizer with one that only cares about impersonation.
-		// overwrite the delegating authorizer with one that only cares about impersonation
+		// Empty string is disallowed because request info has had bugs in the past where it would leave it empty.
-		// empty string is disallowed because request info has had bugs in the past where it would leave it empty
 		disallowedVerbs := sets.NewString("", "impersonate")
 		noImpersonationAuthorizer := &comparableAuthorizer{
 			AuthorizerFunc: func(a authorizer.Attributes) (authorizer.Decision, string, error) {
-				// supporting impersonation is not hard, it would just require a bunch of testing
+				// Supporting impersonation is not hard, it would just require a bunch of testing
-				// and configuring the audit layer (to preserve the caller) which we can do later
+				// and configuring the audit layer (to preserve the caller) which we can do later.
-				// we would also want to delete the incoming impersonation headers
+				// We would also want to delete the incoming impersonation headers
 				// instead of overwriting the delegating authorizer, we would
-				// actually use it to make the impersonation authorization checks
+				// actually use it to make the impersonation authorization checks.
 				if disallowedVerbs.Has(a.GetVerb()) {
 					return authorizer.DecisionDeny, "impersonation is not allowed or invalid verb", nil
 				}
@@ -149,7 +152,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 				return authorizer.DecisionAllow, "deferring authorization to kube API server", nil
 			},
 		}
-		// TODO write a big comment explaining wth this is doing
+		// Set our custom authorizer before calling Compete(), which will use it.
 		serverConfig.Authorization.Authorizer = noImpersonationAuthorizer
 
 		impersonationProxyServer, err := serverConfig.Complete().New("impersonation-proxy", genericapiserver.NewEmptyDelegate())
@@ -159,20 +162,16 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 
 		preparedRun := impersonationProxyServer.PrepareRun()
 
-		// wait until the very end to do sanity checks
+		// Sanity check. Make sure that our custom authorizer is still in place and did not get changed or wrapped.
-
 		if preparedRun.Authorizer != noImpersonationAuthorizer {
 			return nil, constable.Error("invalid mutation of impersonation authorizer detected")
 		}
 
-		// assert that we have a functioning token file to use and no bearer token
+		// Sanity check. Assert that we have a functioning token file to use and no bearer token.
 		if len(preparedRun.LoopbackClientConfig.BearerToken) != 0 || len(preparedRun.LoopbackClientConfig.BearerTokenFile) == 0 {
 			return nil, constable.Error("invalid impersonator loopback rest config has wrong bearer token semantics")
 		}
 
-		// TODO make sure this is closed on error
-		_ = preparedRun.SecureServingInfo.Listener
-
 		return preparedRun.Run, nil
 	}
 

test/integration/concierge_impersonation_proxy_test.go

@@ -370,7 +370,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		require.Len(t, listResult.Items, 0)
 	})
 
-	t.Run("double impersonation is blocked", func(t *testing.T) {
+	t.Run("double impersonation as a regular user is blocked", func(t *testing.T) {
 		// Create an RBAC rule to allow this user to read/write everything.
 		library.CreateTestClusterRoleBinding(t,
 			rbacv1.Subject{Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: env.TestUser.ExpectedUsername},
@@ -401,6 +401,38 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			env.TestUser.ExpectedUsername))
 	})
 
+	// This is a separate test from the above double impersonation test because the cluster admin user gets special
+	// authorization treatment from the Kube API server code that we are using, and we want to ensure that we are blocking
+	// double impersonation even for the cluster admin.
+	t.Run("double impersonation as a cluster admin user is blocked", func(t *testing.T) {
+		// Copy the admin credentials from the admin kubeconfig.
+		adminClientRestConfig := library.NewClientConfig(t)
+
+		if adminClientRestConfig.BearerToken == "" && adminClientRestConfig.CertData == nil && adminClientRestConfig.KeyData == nil {
+			t.Skip("The admin kubeconfig does not include credentials, so skipping this test.")
+		}
+
+		clusterAdminCredentials := &loginv1alpha1.ClusterCredential{
+			Token:                 adminClientRestConfig.BearerToken,
+			ClientCertificateData: string(adminClientRestConfig.CertData),
+			ClientKeyData:         string(adminClientRestConfig.KeyData),
+		}
+
+		// Make a client using the admin credentials which will send requests through the impersonation proxy
+		// and will also add impersonate headers to the request.
+		doubleImpersonationKubeClient := newImpersonationProxyClientWithCredentials(
+			clusterAdminCredentials, impersonationProxyURL, impersonationProxyCACertPEM, "other-user-to-impersonate",
+		).Kubernetes
+
+		_, err = doubleImpersonationKubeClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{})
+		// Double impersonation is not supported yet, so we should get an error.
+		require.EqualError(t, err, fmt.Sprintf(
+			`users "other-user-to-impersonate" is forbidden: `+
+				`User "%s" cannot impersonate resource "users" in API group "" at the cluster scope: `+
+				`impersonation is not allowed or invalid verb`,
+			"kubernetes-admin"))
+	})
+
 	t.Run("WhoAmIRequests and different kinds of authentication through the impersonation proxy", func(t *testing.T) {
 		// Test using the TokenCredentialRequest for authentication.
 		impersonationProxyPinnipedConciergeClient := newImpersonationProxyClient(