From dea3513125d68d54c9d18e971049fcbce5056866 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 30 Nov 2023 15:55:27 -0800 Subject: [PATCH] Add test for when no SA token is cached in impersonator_test.go --- .../impersonator/impersonator_test.go | 46 +++++++++++++++---- .../concierge/impersonator/roundtripper.go | 11 ++++- .../impersonator/roundtripper_test.go | 2 +- .../concierge_impersonation_proxy_test.go | 1 - test/integration/concierge_whoami_test.go | 1 - 5 files changed, 47 insertions(+), 14 deletions(-) diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 344241515..2164dda31 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -58,7 +58,6 @@ import ( "go.pinniped.dev/internal/tokenclient" ) -// TODO: add a test without a token? func TestImpersonator(t *testing.T) { const ( priorityLevelConfigurationsVersion = "v1beta3" @@ -94,6 +93,7 @@ func TestImpersonator(t *testing.T) { kubeAPIServerStatusCode int kubeAPIServerHealthz http.Handler anonymousAuthDisabled bool + noServiceAcctTokenInCache bool // when true, no available service account token for the impersonator to use wantKubeAPIServerRequestHeaders http.Header wantError string wantConstructionError string @@ -658,6 +658,19 @@ func TestImpersonator(t *testing.T) { wantError: `an error on the server ("Internal Server Error: \"/api/v1/namespaces\": requested [{UID 008 authentication.k8s.io/v1 }] without impersonating a user") has prevented the request from succeeding (get namespaces)`, wantAuthorizerAttributes: []authorizer.AttributesRecord{}, }, + { + name: "when there is no service account token cached for the impersonator to use to call the KAS", + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), + noServiceAcctTokenInCache: true, + wantKubeAPIServerRequestHeaders: nil, // no request should have been made to the KAS on behalf of the user + wantError: `an error on the server ("") has prevented the request from succeeding (get namespaces)`, + 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 { tt := tt @@ -673,7 +686,9 @@ func TestImpersonator(t *testing.T) { require.NoError(t, err) // After failing to start and after shutdown, the impersonator port should be available again. - defer requireCanBindToPort(t, port) + t.Cleanup(func() { + requireCanBindToPort(t, port) + }) if tt.kubeAPIServerStatusCode == 0 { tt.kubeAPIServerStatusCode = http.StatusOK @@ -845,10 +860,13 @@ func TestImpersonator(t *testing.T) { return kubeclient.Secure(config) } + serviceTokenCache := tokenclient.NewExpiringSingletonTokenCache() + if !tt.noServiceAcctTokenInCache { + serviceTokenCache.Set("some-service-account-token", 1*time.Hour) + } + // Create an impersonator. Use an invalid port number to make sure our listener override works. - cache := tokenclient.NewExpiringSingletonTokenCache() - cache.Set("some-service-account-token", 1*time.Hour) - runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, cache, &testKubeAPIServerKubeconfig, recOpts, recConfig) + runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, serviceTokenCache, &testKubeAPIServerKubeconfig, recOpts, recConfig) if len(tt.wantConstructionError) > 0 { require.EqualError(t, constructionErr, tt.wantConstructionError) require.Nil(t, runner) @@ -866,6 +884,13 @@ func TestImpersonator(t *testing.T) { errCh <- stopErr }() + // Stop the impersonator server at the end of the test, even if it fails. + t.Cleanup(func() { + close(stopCh) + exitErr := <-errCh + require.NoError(t, exitErr) + }) + // Create a kubeconfig to talk to the impersonator as a client. clientKubeconfig := &rest.Config{ Host: "https://127.0.0.1:" + strconv.Itoa(port), @@ -919,6 +944,12 @@ 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) + // The rest of the test doesn't make sense for when there is no service account token available in the cache. + // In this case, the impersonator cannot make any calls to the Kube API server on behalf of any user. + if tt.noServiceAcctTokenInCache { + return + } + // these authorization checks are caused by the anonymous auth checks below tt.wantAuthorizerAttributes = append(tt.wantAuthorizerAttributes, authorizer.AttributesRecord{ @@ -1021,11 +1052,6 @@ func TestImpersonator(t *testing.T) { _, errBadCert := tcrBadCert.PinnipedConcierge.LoginV1alpha1().TokenCredentialRequests().Create(ctx, &loginv1alpha1.TokenCredentialRequest{}, metav1.CreateOptions{}) require.True(t, errors.IsUnauthorized(errBadCert), errBadCert) require.EqualError(t, errBadCert, "Unauthorized") - - // Stop the impersonator server. - close(stopCh) - exitErr := <-errCh - require.NoError(t, exitErr) }) } } diff --git a/internal/concierge/impersonator/roundtripper.go b/internal/concierge/impersonator/roundtripper.go index a028b7f75..fc3e89d2a 100644 --- a/internal/concierge/impersonator/roundtripper.go +++ b/internal/concierge/impersonator/roundtripper.go @@ -4,11 +4,13 @@ package impersonator import ( + "errors" "fmt" "net/http" utilnet "k8s.io/apimachinery/pkg/util/net" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/tokenclient" ) @@ -25,10 +27,17 @@ func (rt *authorizationRoundTripper) WrappedRoundTripper() http.RoundTripper { func (rt *authorizationRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { req = utilnet.CloneRequest(req) + token := rt.cache.Get() + if token == "" { - return nil, fmt.Errorf("no token available") + plog.Error("could not RoundTrip impersonation proxy request to API server", + errors.New("no service account token available in in-memory cache")) + + return nil, fmt.Errorf("no impersonator service account token available") } + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + return rt.base.RoundTrip(req) } diff --git a/internal/concierge/impersonator/roundtripper_test.go b/internal/concierge/impersonator/roundtripper_test.go index 34b07c0c1..0c8fc0e67 100644 --- a/internal/concierge/impersonator/roundtripper_test.go +++ b/internal/concierge/impersonator/roundtripper_test.go @@ -69,7 +69,7 @@ func TestRoundTrip(t *testing.T) { { name: "no token available", token: "", // since the cache always returns a non-pointer string, this indicates empty - wantError: "no token available", + wantError: "no impersonator service account token available", }, } { tt := tt diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 409b913a1..2143a2fe8 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -2050,7 +2050,6 @@ func createServiceAccountToken(ctx context.Context, t *testing.T, adminClient ku Delete(context.Background(), serviceAccount.Name, metav1.DeleteOptions{})) }) - // TODO: What is this used for? secret, err := adminClient.CoreV1().Secrets(namespaceName).Create(ctx, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "int-test-service-account-token-", diff --git a/test/integration/concierge_whoami_test.go b/test/integration/concierge_whoami_test.go index f1dce6f85..7fb5a9092 100644 --- a/test/integration/concierge_whoami_test.go +++ b/test/integration/concierge_whoami_test.go @@ -89,7 +89,6 @@ func TestWhoAmI_ServiceAccount_Legacy_Parallel(t *testing.T) { }, metav1.CreateOptions{}) require.NoError(t, err) - // TODO: What is this used for? secret, err := kubeClient.Secrets(ns.Name).Create(ctx, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-whoami-",