From 269db6b7c2934345894c402d9c5a5d8ccc8df4a7 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 11 Jun 2021 14:03:18 -0400 Subject: [PATCH] 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 --- internal/concierge/impersonator/doc.go | 9 +- .../concierge/impersonator/impersonator.go | 46 +-- .../impersonator/impersonator_test.go | 331 +++++++++++++++++- .../concierge_impersonation_proxy_test.go | 51 +-- 4 files changed, 385 insertions(+), 52 deletions(-) diff --git a/internal/concierge/impersonator/doc.go b/internal/concierge/impersonator/doc.go index af91e2d3..6ad3f941 100644 --- a/internal/concierge/impersonator/doc.go +++ b/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 -impersonate the user, the proxied request will be authorized against that user. -Thus for all regular REST verbs, we perform no authorization checks. +In terms of authorization, in addition to the regular checks that the Kubernetes +API server will make for the impersonated user, we perform the same authorization +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 diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index 1762e8a2..067fcfac 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/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 - 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 + return authorizer.DecisionDeny, "invalid verb, " + baseReason, nil default: - // assume everything else is internal SAR checks that we need to run against the requesting user - // because when KAS does the check, it may run the check against our service account and not the - // requesting user. This also handles the impersonate verb to allow for nested impersonation. - return delegatingAuthorizer.Authorize(ctx, a) + // Since we authenticate the requesting user, we are in the best position to correctly authorize them. + // When KAS does the check, it may run the check against our service account and not the requesting user + // (due to a bug in the code or any other internal SAR checks that the request processing does). + // 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) } diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 182d3047..a3b743bb 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/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", + 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", @@ -385,7 +624,8 @@ func TestImpersonator(t *testing.T) { w.WriteHeader(http.StatusInternalServerError) _, _ = 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`, + 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)) +} diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index c5d57884..717a52d1 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/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{}) - require.True(t, k8serrors.IsForbidden(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`) + _, err := impersonationProxySSRRClient.Create(ctx, invalidSSRR, metav1.CreateOptions{}) + require.True(t, k8serrors.IsBadRequest(err), library.Sdump(err)) + 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{}) - if errList == nil { - return false, fmt.Errorf("unexpected nil error for test user node list") - } + _, errCreate := impersonationProxySSRRClient.Create(ctx, invalidSSRR, metav1.CreateOptions{}) - if !k8serrors.IsForbidden(errList) { - return false, fmt.Errorf("unexpected error for test user node list: %w", errList) - } + switch { + case errCreate == nil: + return false, fmt.Errorf("unexpected nil error for test user create invalid SSRR") - switch errList.Error() { - case `nodes is forbidden: User "` + env.TestUser.ExpectedUsername + `" cannot list resource "nodes" in API group "" at the cluster scope`: + case k8serrors.IsBadRequest(errCreate) && errCreate.Error() == "no namespace on request": 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 + - `" cannot impersonate resource "users" in API group "" at the cluster scope`: + 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) })