impersonator: always authorize every request

This change updates the impersonator to always authorize every
request instead of relying on the Kuberentes API server to perform
the check on the impersonated request.  This protects us from
scenarios where we fail to correctly impersonate the user due to
some bug in our proxy logic.  We still rely completely on the API
server to perform admission checks on the impersonated requests.

Signed-off-by: Monis Khan <mok@vmware.com>

internal/concierge/impersonator/doc.go

@@ -27,9 +27,12 @@ in that Pinniped itself provides the Token Credential Request API which is used
 specifically by anonymous users to retrieve credentials.  This API is the single
 API that will remain available even when anonymous authentication is disabled.
 
-In terms of authorization, we rely mostly on the Kubernetes API server.  Since we
+In terms of authorization, in addition to the regular checks that the Kubernetes
-impersonate the user, the proxied request will be authorized against that user.
+API server will make for the impersonated user, we perform the same authorization
-Thus for all regular REST verbs, we perform no authorization checks.
+checks via subject access review calls.  This protects us from scenarios where
+we fail to correctly impersonate the user due to some bug in our proxy logic.
+We rely completely on the Kubernetes API server to perform admission checks on
+the impersonated requests.
 
 Nested impersonation is handled by performing the same authorization checks the
 Kubernetes API server would (we get this mostly for free by using the aggregated

internal/concierge/impersonator/impersonator.go

@@ -70,7 +70,7 @@ func New(
 	dynamicCertProvider dynamiccert.Private,
 	impersonationProxySignerCA dynamiccert.Public,
 ) (func(stopCh <-chan struct{}) error, error) {
-	return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil)
+	return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil, nil)
 }
 
 func newInternal( //nolint:funlen // yeah, it's kind of long.
@@ -79,6 +79,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 	impersonationProxySignerCA dynamiccert.Public,
 	clientOpts []kubeclient.Option, // for unit testing, should always be nil in production
 	recOpts func(*genericoptions.RecommendedOptions), // for unit testing, should always be nil in production
+	recConfig func(*genericapiserver.RecommendedConfig), // for unit testing, should always be nil in production
 ) (func(stopCh <-chan struct{}) error, error) {
 	var listener net.Listener
 
@@ -256,33 +257,38 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 		serverConfig.Authentication.Authenticator = blockAnonymousAuthenticator
 
 		delegatingAuthorizer := serverConfig.Authorization.Authorizer
-		nestedImpersonationAuthorizer := &comparableAuthorizer{
+		customReasonAuthorizer := &comparableAuthorizer{
 			authorizerFunc: func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
+				const baseReason = "decision made by impersonation-proxy.concierge.pinniped.dev"
 				switch a.GetVerb() {
 				case "":
 					// Empty string is disallowed because request info has had bugs in the past where it would leave it empty.
-					return authorizer.DecisionDeny, "invalid verb", nil
+					return authorizer.DecisionDeny, "invalid verb, " + baseReason, nil
-				case "create",
-					"update",
-					"delete",
-					"deletecollection",
-					"get",
-					"list",
-					"watch",
-					"patch",
-					"proxy":
-					// we know these verbs are from the request info parsing which is safe to delegate to KAS
-					return authorizer.DecisionAllow, "deferring standard verb authorization to kube API server", nil
 				default:
-					// assume everything else is internal SAR checks that we need to run against the requesting user
+					// Since we authenticate the requesting user, we are in the best position to correctly authorize them.
-					// because when KAS does the check, it may run the check against our service account and not the
+					// When KAS does the check, it may run the check against our service account and not the requesting user
-					// requesting user.  This also handles the impersonate verb to allow for nested impersonation.
+					// (due to a bug in the code or any other internal SAR checks that the request processing does).
-					return delegatingAuthorizer.Authorize(ctx, a)
+					// This also handles the impersonate verb to allow for nested impersonation.
+					decision, reason, err := delegatingAuthorizer.Authorize(ctx, a)
+
+					// make it easier to detect when the impersonation proxy is authorizing a request vs KAS
+					switch len(reason) {
+					case 0:
+						reason = baseReason
+					default:
+						reason = reason + ", " + baseReason
+					}
+
+					return decision, reason, err
 				}
 			},
 		}
 		// Set our custom authorizer before calling Compete(), which will use it.
-		serverConfig.Authorization.Authorizer = nestedImpersonationAuthorizer
+		serverConfig.Authorization.Authorizer = customReasonAuthorizer
+
+		if recConfig != nil {
+			recConfig(serverConfig)
+		}
 
 		completedConfig := serverConfig.Complete()
 		impersonationProxyServer, err := completedConfig.New("impersonation-proxy", genericapiserver.NewEmptyDelegate())
@@ -298,7 +304,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 		}
 
 		// Sanity check. Make sure that our custom authorizer is still in place and did not get changed or wrapped.
-		if preparedRun.Authorizer != nestedImpersonationAuthorizer {
+		if preparedRun.Authorizer != customReasonAuthorizer {
 			return nil, fmt.Errorf("invalid mutation of impersonation authorizer detected: %#v", preparedRun.Authorizer)
 		}
 

internal/concierge/impersonator/impersonator_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"strconv"
+	"sync"
 	"testing"
 	"time"
 
@@ -29,6 +30,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/request/bearertoken"
 	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/features"
 	genericapiserver "k8s.io/apiserver/pkg/server"
@@ -84,6 +86,7 @@ func TestImpersonator(t *testing.T) {
 		wantKubeAPIServerRequestHeaders    http.Header
 		wantError                          string
 		wantConstructionError              string
+		wantAuthorizerAttributes           []authorizer.AttributesRecord
 	}{
 		{
 			name:                               "happy path",
@@ -98,6 +101,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "happy path with forbidden healthz",
@@ -116,6 +125,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "happy path with unauthorized healthz",
@@ -135,6 +150,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "happy path with upgrade",
@@ -160,6 +181,12 @@ func TestImpersonator(t *testing.T) {
 				"Connection":        {"Upgrade"},
 				"Upgrade":           {"spdy/3.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username2", UID: "", Groups: []string{"test-group3", "test-group4", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "happy path ignores forwarded header",
@@ -177,6 +204,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username2", UID: "", Groups: []string{"test-group3", "test-group4", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "happy path ignores forwarded header canonicalization",
@@ -194,6 +227,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username2", UID: "", Groups: []string{"test-group3", "test-group4", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "user is authenticated but the kube API request returns an error",
@@ -210,6 +249,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "when there is no client cert on request, it is an anonymous request",
@@ -224,6 +269,12 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding":   {"gzip"},
 				"X-Forwarded-For":   {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:       "when there is no client cert on request but it has basic auth, it is still an anonymous request",
@@ -244,12 +295,19 @@ func TestImpersonator(t *testing.T) {
 				"X-Forwarded-For":   {"127.0.0.1"},
 				"Test":              {"val"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                               "failed client cert authentication",
 			clientCert:                         newClientCert(t, unrelatedCA, "test-username", []string{"test-group1"}),
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Unauthorized",
+			wantAuthorizerAttributes:           nil,
 		},
 		{
 			name:                               "nested impersonation by regular users calls delegating authorizer",
@@ -258,7 +316,14 @@ func TestImpersonator(t *testing.T) {
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			// this fails because the delegating authorizer in this test only allows system:masters and fails everything else
 			wantError: `users "some-other-username" is forbidden: User "test-username" ` +
-				`cannot impersonate resource "users" in API group "" at the cluster scope`,
+				`cannot impersonate resource "users" in API group "" at the cluster scope: ` +
+				`decision made by impersonation-proxy.concierge.pinniped.dev`,
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "some-other-username", ResourceRequest: true, Path: "",
+				},
+			},
 		},
 		{
 			name:       "nested impersonation by admin users calls delegating authorizer",
@@ -304,6 +369,96 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding": {"gzip"},
 				"X-Forwarded-For": {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "fire", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "groups", Subresource: "", Name: "elements", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "iam.gke.io/user-assertion", Name: "good", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "iam.gke.io/user-assertion", Name: "stuff", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/roles", Name: "a-role1", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/roles", Name: "a-role2", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "user-assertion.cloud.google.com", Name: "smaller", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "user-assertion.cloud.google.com", Name: "things", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "colors", Name: "red", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "colors", Name: "orange", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "colors", Name: "blue", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "scopes.authorization.openshift.io", Name: "user:info", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "scopes.authorization.openshift.io", Name: "user:full", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "scopes.authorization.openshift.io", Name: "user:check-access", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/project/name", Name: "a-project-name", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/user/domain/id", Name: "a-domain-id", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/user/domain/name", Name: "a-domain-name", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "alpha.kubernetes.io/identity/project/id", Name: "a-project-id", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "fire", UID: "", Groups: []string{"elements", "system:authenticated"},
+						Extra: map[string][]string{
+							"alpha.kubernetes.io/identity/project/id":       {"a-project-id"},
+							"alpha.kubernetes.io/identity/project/name":     {"a-project-name"},
+							"alpha.kubernetes.io/identity/roles":            {"a-role1", "a-role2"},
+							"alpha.kubernetes.io/identity/user/domain/id":   {"a-domain-id"},
+							"alpha.kubernetes.io/identity/user/domain/name": {"a-domain-name"},
+							"colors":                            {"red", "orange", "blue"},
+							"iam.gke.io/user-assertion":         {"good", "stuff"},
+							"scopes.authorization.openshift.io": {"user:info", "user:full", "user:check-access"},
+							"user-assertion.cloud.google.com":   {"smaller", "things"},
+						},
+					},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                  "nested impersonation by admin users cannot impersonate UID",
@@ -314,6 +469,16 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: invalid impersonation",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "some-other-username", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "some-other-username", UID: "", Groups: []string{"system:authenticated"}, Extra: map[string][]string{}},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                  "nested impersonation by admin users cannot impersonate UID header canonicalization",
@@ -324,6 +489,16 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: invalid impersonation",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "some-other-username", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "some-other-username", UID: "", Groups: []string{"system:authenticated"}, Extra: map[string][]string{}},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:       "nested impersonation by admin users cannot use reserved key",
@@ -338,6 +513,33 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: unimplemented functionality - unable to act as current user",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "other-user-to-impersonate", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "groups", Subresource: "", Name: "other-peeps", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "something.impersonation-proxy.concierge.pinniped.dev", Name: "bad data", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "key", Name: "good", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "other-user-to-impersonate", UID: "", Groups: []string{"other-peeps", "system:authenticated"},
+						Extra: map[string][]string{
+							"key": {"good"},
+							"something.impersonation-proxy.concierge.pinniped.dev": {"bad data"},
+						},
+					},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:       "nested impersonation by admin users cannot use invalid key",
@@ -351,6 +553,24 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: unimplemented functionality - unable to act as current user",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "panda", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "groups", Subresource: "", Name: "other-peeps", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "party~~time", Name: "danger", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "panda", UID: "", Groups: []string{"other-peeps", "system:authenticated"}, Extra: map[string][]string{"party~~time": {"danger"}}},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:       "nested impersonation by admin users can use uppercase key because impersonation is lossy",
@@ -374,10 +594,29 @@ func TestImpersonator(t *testing.T) {
 				"Accept-Encoding": {"gzip"},
 				"X-Forwarded-For": {"127.0.0.1"},
 			},
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "panda", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "groups", Subresource: "", Name: "other-peeps", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "test-admin", UID: "", Groups: []string{"test-group2", "system:masters", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "authentication.k8s.io", APIVersion: "v1", Resource: "userextras", Subresource: "roar", Name: "tiger", ResourceRequest: true, Path: "",
+				},
+				{
+					User: &user.DefaultInfo{Name: "panda", UID: "", Groups: []string{"other-peeps", "system:authenticated"}, Extra: map[string][]string{"roar": {"tiger"}}},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:                     "no bearer token file in Kube API server client config",
 			wantConstructionError:    "invalid impersonator loopback rest config has wrong bearer token semantics",
+			wantAuthorizerAttributes: nil,
 		},
 		{
 			name: "unexpected healthz response",
@@ -386,6 +625,7 @@ func TestImpersonator(t *testing.T) {
 				_, _ = w.Write([]byte("broken"))
 			}),
 			wantConstructionError:    `could not detect if anonymous authentication is enabled: an error on the server ("broken") has prevented the request from succeeding`,
+			wantAuthorizerAttributes: nil,
 		},
 		{
 			name:       "header canonicalization user header",
@@ -395,7 +635,14 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError: `users "PANDA" is forbidden: User "test-username" ` +
-				`cannot impersonate resource "users" in API group "" at the cluster scope`,
+				`cannot impersonate resource "users" in API group "" at the cluster scope: ` +
+				`decision made by impersonation-proxy.concierge.pinniped.dev`,
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "impersonate", Namespace: "", APIGroup: "", APIVersion: "", Resource: "users", Subresource: "", Name: "PANDA", ResourceRequest: true, Path: "",
+				},
+			},
 		},
 		{
 			name:       "header canonicalization future UID header",
@@ -405,6 +652,12 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: invalid impersonation",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 		{
 			name:       "future UID header",
@@ -414,6 +667,12 @@ func TestImpersonator(t *testing.T) {
 			},
 			kubeAPIServerClientBearerTokenFile: "required-to-be-set",
 			wantError:                          "Internal error occurred: invalid impersonation",
+			wantAuthorizerAttributes: []authorizer.AttributesRecord{
+				{
+					User: &user.DefaultInfo{Name: "test-username", UID: "", Groups: []string{"test-group1", "test-group2", "system:authenticated"}, Extra: nil},
+					Verb: "list", Namespace: "", APIGroup: "", APIVersion: "v1", Resource: "namespaces", Subresource: "", Name: "", ResourceRequest: true, Path: "/api/v1/namespaces",
+				},
+			},
 		},
 	}
 	for _, tt := range tests {
@@ -548,8 +807,29 @@ func TestImpersonator(t *testing.T) {
 				options.SecureServing.Listener = listener // use our listener with the dynamic port
 			}
 
+			recorder := &attributeRecorder{}
+			defer func() {
+				require.ElementsMatch(t, tt.wantAuthorizerAttributes, recorder.attributes)
+				require.Len(t, recorder.attributes, len(tt.wantAuthorizerAttributes))
+			}()
+
+			// Allow standard REST verbs to be authorized so that tests pass without invasive changes
+			recConfig := func(config *genericapiserver.RecommendedConfig) {
+				authz := config.Authorization.Authorizer.(*comparableAuthorizer)
+				delegate := authz.authorizerFunc
+				authz.authorizerFunc = func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
+					recorder.record(a)
+					switch a.GetVerb() {
+					case "create", "get", "list":
+						return authorizer.DecisionAllow, "standard verbs are allowed in tests", nil
+					default:
+						return delegate(ctx, a)
+					}
+				}
+			}
+
 			// Create an impersonator.  Use an invalid port number to make sure our listener override works.
-			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, clientOpts, recOpts)
+			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, clientOpts, recOpts, recConfig)
 			if len(tt.wantConstructionError) > 0 {
 				require.EqualError(t, constructionErr, tt.wantConstructionError)
 				require.Nil(t, runner)
@@ -620,6 +900,34 @@ func TestImpersonator(t *testing.T) {
 			// of the original request mutated by the impersonator.  Otherwise the headers should be nil.
 			require.Equal(t, tt.wantKubeAPIServerRequestHeaders, testKubeAPIServerSawHeaders)
 
+			// these authorization checks are caused by the anonymous auth checks below
+			tt.wantAuthorizerAttributes = append(tt.wantAuthorizerAttributes,
+				authorizer.AttributesRecord{
+					User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+					Verb: "create", Namespace: "", APIGroup: "login.concierge.pinniped.dev", APIVersion: "v1alpha1", Resource: "tokencredentialrequests", Subresource: "", Name: "", ResourceRequest: true, Path: "/apis/login.concierge.pinniped.dev/v1alpha1/tokencredentialrequests",
+				},
+				authorizer.AttributesRecord{
+					User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+					Verb: "create", Namespace: "", APIGroup: "login.concierge.walrus.tld", APIVersion: "v1alpha1", Resource: "tokencredentialrequests", Subresource: "", Name: "", ResourceRequest: true, Path: "/apis/login.concierge.walrus.tld/v1alpha1/tokencredentialrequests",
+				},
+			)
+			if !tt.anonymousAuthDisabled {
+				tt.wantAuthorizerAttributes = append(tt.wantAuthorizerAttributes,
+					authorizer.AttributesRecord{
+						User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+						Verb: "get", Namespace: "", APIGroup: "", APIVersion: "", Resource: "", Subresource: "", Name: "", ResourceRequest: false, Path: "/probe",
+					},
+					authorizer.AttributesRecord{
+						User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+						Verb: "list", Namespace: "", APIGroup: "not-concierge.walrus.tld", APIVersion: "v1", Resource: "tokencredentialrequests", Subresource: "", Name: "", ResourceRequest: true, Path: "/apis/not-concierge.walrus.tld/v1/tokencredentialrequests",
+					},
+					authorizer.AttributesRecord{
+						User: &user.DefaultInfo{Name: "system:anonymous", UID: "", Groups: []string{"system:unauthenticated"}, Extra: nil},
+						Verb: "list", Namespace: "", APIGroup: "not-concierge.walrus.tld", APIVersion: "v1", Resource: "ducks", Subresource: "", Name: "", ResourceRequest: true, Path: "/apis/not-concierge.walrus.tld/v1/ducks",
+					},
+				)
+			}
+
 			// anonymous TCR should always work
 
 			tcrRegGroup, err := kubeclient.New(kubeclient.WithConfig(rest.AnonymousClientConfig(clientKubeconfig)))
@@ -1705,3 +2013,14 @@ func Test_withBearerTokenPreservation(t *testing.T) {
 		})
 	}
 }
+
+type attributeRecorder struct {
+	lock       sync.Mutex
+	attributes []authorizer.AttributesRecord
+}
+
+func (r *attributeRecorder) record(attributes authorizer.Attributes) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+	r.attributes = append(r.attributes, *attributes.(*authorizer.AttributesRecord))
+}

test/integration/concierge_impersonation_proxy_test.go

@@ -566,7 +566,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			require.True(t, k8serrors.IsForbidden(err), err)
 			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`,
+					`User "%s" cannot impersonate resource "users" in API group "" at the cluster scope: `+
+					`decision made by impersonation-proxy.concierge.pinniped.dev`,
 				env.TestUser.ExpectedUsername))
 
 			// impersonate the GC service account instead which can read anything (the binding to edit allows this)
@@ -614,7 +615,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			require.True(t, k8serrors.IsForbidden(err), err)
 			require.EqualError(t, err, fmt.Sprintf(
 				`userextras.authentication.k8s.io "with a dangerous value" is forbidden: `+
-					`User "%s" cannot impersonate resource "userextras/some-fancy-key" in API group "authentication.k8s.io" at the cluster scope`,
+					`User "%s" cannot impersonate resource "userextras/some-fancy-key" in API group "authentication.k8s.io" at the cluster scope: `+
+					`decision made by impersonation-proxy.concierge.pinniped.dev`,
 				env.TestUser.ExpectedUsername))
 		})
 
@@ -672,7 +674,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			// the impersonated user lacks the RBAC to perform this call
 			require.True(t, k8serrors.IsForbidden(err), err)
 			require.EqualError(t, err, fmt.Sprintf(
-				`secrets "%s" is forbidden: User "other-user-to-impersonate" cannot get resource "secrets" in API group "" in the namespace "%s"`,
+				`secrets "%s" is forbidden: User "other-user-to-impersonate" cannot get resource "secrets" in API group "" in the namespace "%s": `+
+					`decision made by impersonation-proxy.concierge.pinniped.dev`,
 				impersonationProxyTLSSecretName(env), env.ConciergeNamespace,
 			))
 
@@ -702,7 +705,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 				clusterAdminCredentials, impersonationProxyURL, impersonationProxyCACertPEM,
 				&rest.ImpersonationConfig{
 					UserName: "other-user-to-impersonate",
-					Groups:   []string{"other-group-1", "other-group-2"},
+					Groups:   []string{"other-group-1", "other-group-2", "system:masters"}, // impersonate system:masters so we get past authorization checks
 					Extra: map[string][]string{
 						"this-good-key": {"to this good value"},
 						"something.impersonation-proxy.concierge.pinniped.dev": {"super sneaky value"},
@@ -744,7 +747,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 			require.True(t, k8serrors.IsForbidden(err), err)
 			require.EqualError(t, err, fmt.Sprintf(
 				`serviceaccounts "root-ca-cert-publisher" is forbidden: `+
-					`User "%s" cannot impersonate resource "serviceaccounts" in API group "" in the namespace "kube-system"`,
+					`User "%s" cannot impersonate resource "serviceaccounts" in API group "" in the namespace "kube-system": `+
+					`decision made by impersonation-proxy.concierge.pinniped.dev`,
 				serviceaccount.MakeUsername(namespaceName, saName)))
 
 			// webhook authorizer deny cache TTL is 10 seconds so we need to wait long enough for it to drain
@@ -1271,7 +1275,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 					healthzLog, errHealthzLog := impersonationProxyAdminRestClientAsAnonymous.Get().AbsPath("/healthz/log").DoRaw(ctx)
 					require.True(t, k8serrors.IsForbidden(errHealthzLog), "%s\n%s", library.Sdump(errHealthzLog), string(healthzLog))
-					require.Equal(t, `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"forbidden: User \"system:anonymous\" cannot get path \"/healthz/log\"","reason":"Forbidden","details":{},"code":403}`+"\n", string(healthzLog))
+					require.Equal(t, `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"forbidden: User \"system:anonymous\" cannot get path \"/healthz/log\": decision made by impersonation-proxy.concierge.pinniped.dev","reason":"Forbidden","details":{},"code":403}`+"\n", string(healthzLog))
 				})
 			})
 
@@ -1302,7 +1306,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 					pod, err := impersonationProxyAnonymousClient.Kubernetes.CoreV1().Pods(metav1.NamespaceSystem).
 						Get(ctx, "does-not-matter", metav1.GetOptions{})
 					require.True(t, k8serrors.IsForbidden(err), library.Sdump(err))
-					require.EqualError(t, err, `pods "does-not-matter" is forbidden: User "system:anonymous" cannot get resource "pods" in API group "" in the namespace "kube-system"`, library.Sdump(err))
+					require.EqualError(t, err, `pods "does-not-matter" is forbidden: User "system:anonymous" cannot get resource "pods" in API group "" in the namespace "kube-system": `+
+						`decision made by impersonation-proxy.concierge.pinniped.dev`, library.Sdump(err))
 					require.Equal(t, &corev1.Pod{}, pod)
 				})
 
@@ -1373,15 +1378,18 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 	})
 
 	t.Run("assert correct impersonator service account is being used", func(t *testing.T) {
-		impersonationProxyNodesClient := impersonationProxyKubeClient(t).CoreV1().Nodes() // pick some resource the test user cannot access
+		// pick an API that everyone can access but always make invalid requests to it
+		// we can tell that the request is reaching KAS because only it has the validation logic
+		impersonationProxySSRRClient := impersonationProxyKubeClient(t).AuthorizationV1().SelfSubjectRulesReviews()
 		crbClient := adminClient.RbacV1().ClusterRoleBindings()
 		impersonationProxyName := env.ConciergeAppName + "-impersonation-proxy"
 		saFullName := serviceaccount.MakeUsername(env.ConciergeNamespace, impersonationProxyName)
+		invalidSSRR := &authorizationv1.SelfSubjectRulesReview{}
 
 		// sanity check default expected error message
-		_, err := impersonationProxyNodesClient.List(ctx, metav1.ListOptions{})
+		_, err := impersonationProxySSRRClient.Create(ctx, invalidSSRR, metav1.CreateOptions{})
-		require.True(t, k8serrors.IsForbidden(err), library.Sdump(err))
+		require.True(t, k8serrors.IsBadRequest(err), library.Sdump(err))
-		require.EqualError(t, err, `nodes is forbidden: User "`+env.TestUser.ExpectedUsername+`" cannot list resource "nodes" in API group "" at the cluster scope`)
+		require.EqualError(t, err, "no namespace on request")
 
 		// remove the impersonation proxy SA's permissions
 		crb, err := crbClient.Get(ctx, impersonationProxyName, metav1.GetOptions{})
@@ -1418,26 +1426,23 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 
 		// assert that the impersonation proxy stops working when we remove its permissions
 		library.RequireEventuallyWithoutError(t, func() (bool, error) {
-			_, errList := impersonationProxyNodesClient.List(ctx, metav1.ListOptions{})
+			_, errCreate := impersonationProxySSRRClient.Create(ctx, invalidSSRR, metav1.CreateOptions{})
-			if errList == nil {
-				return false, fmt.Errorf("unexpected nil error for test user node list")
-			}
 
-			if !k8serrors.IsForbidden(errList) {
+			switch {
-				return false, fmt.Errorf("unexpected error for test user node list: %w", errList)
+			case errCreate == nil:
-			}
+				return false, fmt.Errorf("unexpected nil error for test user create invalid SSRR")
 
-			switch errList.Error() {
+			case k8serrors.IsBadRequest(errCreate) && errCreate.Error() == "no namespace on request":
-			case `nodes is forbidden: User "` + env.TestUser.ExpectedUsername + `" cannot list resource "nodes" in API group "" at the cluster scope`:
 				t.Log("waiting for impersonation proxy service account to lose impersonate permissions")
 				return false, nil // RBAC change has not rolled out yet
 
-			case `users "` + env.TestUser.ExpectedUsername + `" is forbidden: User "` + saFullName +
+			case k8serrors.IsForbidden(errCreate) && errCreate.Error() ==
+				`users "`+env.TestUser.ExpectedUsername+`" is forbidden: User "`+saFullName+
 					`" cannot impersonate resource "users" in API group "" at the cluster scope`:
 				return true, nil // expected RBAC error
 
 			default:
-				return false, fmt.Errorf("unexpected forbidden error for test user node list: %w", errList)
+				return false, fmt.Errorf("unexpected error for test user create invalid SSRR: %w", errCreate)
 			}
 		}, time.Minute, time.Second)
 	})