From e8490c024493f1b3a623dd2ff4f135ec2a0cb216 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 20 Oct 2023 15:28:53 -0500 Subject: [PATCH] Do not use long-lived service account tokens in secrets --- deploy/concierge/deployment.yaml | 29 +- deploy/concierge/rbac.yaml | 9 +- internal/backoff/infinitebackoff.go | 44 +++ internal/backoff/infinitebackoff_test.go | 94 ++++++ internal/backoff/stepping.go | 56 ++++ internal/backoff/stepping_test.go | 124 ++++++++ internal/concierge/apiserver/apiserver.go | 20 ++ .../concierge/impersonator/impersonator.go | 60 ++-- .../impersonator/impersonator_test.go | 127 +++----- .../concierge/impersonator/roundtripper.go | 34 ++ .../impersonator/roundtripper_test.go | 107 +++++++ internal/concierge/server/server.go | 22 ++ internal/config/concierge/config.go | 6 + internal/config/concierge/config_test.go | 73 ++++- internal/config/concierge/types.go | 2 + .../impersonatorconfig/impersonator_config.go | 6 + .../impersonator_config_test.go | 6 +- internal/controller/secrets/secrets.go | 92 ++++++ internal/controller/secrets/secrets_test.go | 296 ++++++++++++++++++ .../controllermanager/prepare_controllers.go | 17 + internal/tokenclient/expiringtokencache.go | 50 +++ .../tokenclient/expiringtokencache_test.go | 35 +++ internal/tokenclient/tokenclient.go | 146 +++++++++ internal/tokenclient/tokenclient_test.go | 202 ++++++++++++ .../concierge_impersonation_proxy_test.go | 1 + test/integration/concierge_whoami_test.go | 1 + 26 files changed, 1524 insertions(+), 135 deletions(-) create mode 100644 internal/backoff/infinitebackoff.go create mode 100644 internal/backoff/infinitebackoff_test.go create mode 100644 internal/backoff/stepping.go create mode 100644 internal/backoff/stepping_test.go create mode 100644 internal/concierge/impersonator/roundtripper.go create mode 100644 internal/concierge/impersonator/roundtripper_test.go create mode 100644 internal/controller/secrets/secrets.go create mode 100644 internal/controller/secrets/secrets_test.go create mode 100644 internal/tokenclient/expiringtokencache.go create mode 100644 internal/tokenclient/expiringtokencache_test.go create mode 100644 internal/tokenclient/tokenclient.go create mode 100644 internal/tokenclient/tokenclient_test.go diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index 9b3c344b..3ad7a439 100644 --- a/deploy/concierge/deployment.yaml +++ b/deploy/concierge/deployment.yaml @@ -45,8 +45,9 @@ metadata: annotations: #! we need to create this service account before we create the secret kapp.k14s.io/change-group: "impersonation-proxy.concierge.pinniped.dev/serviceaccount" -secrets: #! make sure the token controller does not create any other secrets -- name: #@ defaultResourceNameWithSuffix("impersonation-proxy") + kubernetes.io/enforce-mountable-secrets: "true" +secrets: [] #! make sure the token controller does not create any secrets +automountServiceAccountToken: false --- apiVersion: v1 kind: ConfigMap @@ -77,6 +78,8 @@ data: impersonationCACertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-ca-certificate") @) impersonationSignerSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-signer-ca-certificate") @) agentServiceAccount: (@= defaultResourceNameWithSuffix("kube-cert-agent") @) + impersonationProxyServiceAccount: (@= defaultResourceNameWithSuffix("impersonation-proxy") @) + impersonationProxyLegacySecret: (@= defaultResourceNameWithSuffix("impersonation-proxy") @) labels: (@= json.encode(labels()).rstrip() @) kubeCertAgent: namePrefix: (@= defaultResourceNameWithSuffix("kube-cert-agent-") @) @@ -182,9 +185,6 @@ spec: - name: podinfo mountPath: /etc/podinfo readOnly: true - - name: impersonation-proxy - mountPath: /var/run/secrets/impersonation-proxy.concierge.pinniped.dev/serviceaccount - readOnly: true env: #@ if data.values.https_proxy: - name: HTTPS_PROXY @@ -220,12 +220,6 @@ spec: - name: config-volume configMap: name: #@ defaultResourceNameWithSuffix("config") - - name: impersonation-proxy - secret: - secretName: #@ defaultResourceNameWithSuffix("impersonation-proxy") - items: #! make sure our pod does not start until the token controller has a chance to populate the secret - - key: token - path: token - name: podinfo downwardAPI: items: @@ -348,16 +342,3 @@ spec: loadBalancerIP: #@ data.values.impersonation_proxy_spec.service.load_balancer_ip #@ end annotations: #@ data.values.impersonation_proxy_spec.service.annotations ---- -apiVersion: v1 -kind: Secret -metadata: - name: #@ defaultResourceNameWithSuffix("impersonation-proxy") - namespace: #@ namespace() - labels: #@ labels() - annotations: - #! wait until the SA exists to create this secret so that the token controller does not delete it - #! we have this secret at the end so that kubectl will create the service account first - kapp.k14s.io/change-rule: "upsert after upserting impersonation-proxy.concierge.pinniped.dev/serviceaccount" - kubernetes.io/service-account.name: #@ defaultResourceNameWithSuffix("impersonation-proxy") -type: kubernetes.io/service-account-token diff --git a/deploy/concierge/rbac.yaml b/deploy/concierge/rbac.yaml index 61ffa57b..eade914a 100644 --- a/deploy/concierge/rbac.yaml +++ b/deploy/concierge/rbac.yaml @@ -1,4 +1,4 @@ -#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +#! Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. #! SPDX-License-Identifier: Apache-2.0 #@ load("@ytt:data", "data") @@ -156,6 +156,13 @@ rules: - apiGroups: [ coordination.k8s.io ] resources: [ leases ] verbs: [ create, get, update ] + #! We need to be able to get service accounts and create serviceaccounts/tokens so that we can create short-lived tokens for the impersonation proxy + - apiGroups: [""] + resources: [ serviceaccounts ] + verbs: [ get ] + - apiGroups: [""] + resources: [ serviceaccounts/token ] + verbs: [ create ] --- kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/internal/backoff/infinitebackoff.go b/internal/backoff/infinitebackoff.go new file mode 100644 index 00000000..f03cca5c --- /dev/null +++ b/internal/backoff/infinitebackoff.go @@ -0,0 +1,44 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package backoff + +import ( + "math" + "time" +) + +type InfiniteBackoff struct { + // The initial duration. + Duration time.Duration + + // Duration is multiplied by Factor each iteration, if the limit imposed by MaxDuration has not been + // reached. + // + Factor float64 + + // A limit on step size. Once reached, this value will be used as the interval. + MaxDuration time.Duration + + hasStepped bool +} + +// Step returns the next duration in the backoff sequence. +// It modifies the receiver and is not thread-safe. +func (b *InfiniteBackoff) Step() time.Duration { + if !b.hasStepped { + b.hasStepped = true + return b.Duration + } + + var next time.Duration + b.Factor = math.Max(1, b.Factor) + // Grow by the factor (which could be 1). + next = time.Duration(float64(b.Duration) * b.Factor) + // Stop growing the intervals once we exceed the max duration. + if b.MaxDuration > 0 && next > b.MaxDuration { + next = b.MaxDuration + } + b.Duration = next + return next +} diff --git a/internal/backoff/infinitebackoff_test.go b/internal/backoff/infinitebackoff_test.go new file mode 100644 index 00000000..04757dfb --- /dev/null +++ b/internal/backoff/infinitebackoff_test.go @@ -0,0 +1,94 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package backoff + +import ( + "math" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestInfiniteBackoff(t *testing.T) { + tests := []struct { + name string + stepper Stepper + expectedSequence []time.Duration + }{ + { + name: "zero initialization results in 0ns steps", + stepper: &InfiniteBackoff{}, + expectedSequence: func() []time.Duration { + results := make([]time.Duration, 1000) + for i := 0; i < 1000; i++ { + results[i] = time.Duration(0) + } + return results + }(), + }, + { + name: "double 5 ns forever", + stepper: &InfiniteBackoff{ + Duration: 5 * time.Nanosecond, + Factor: 2, + }, + expectedSequence: func() []time.Duration { + // limit to 60 to prevent int64 overflow + results := make([]time.Duration, 60) + results[0] = 5 * time.Nanosecond + for i := 1; i < 60; i++ { + results[i] = 2 * results[i-1] + } + return results + }(), + }, + { + name: "grows slowly until limit", + stepper: &InfiniteBackoff{ + Duration: 20 * time.Nanosecond, + Factor: 1.1, + MaxDuration: 40 * time.Nanosecond, + }, + expectedSequence: func() []time.Duration { + results := make([]time.Duration, 1000) + results[0] = 20 * time.Nanosecond + for i := 1; i < 1000; i++ { + nanoseconds := 1.1 * float64(results[i-1]) + results[i] = time.Duration(math.Min(nanoseconds, 40)) + } + return results + }(), + }, + { + name: "factor less than 1.0 is replaced with 1.0", + stepper: &InfiniteBackoff{ + Duration: 20 * time.Nanosecond, + Factor: 0.9, + }, + expectedSequence: func() []time.Duration { + results := make([]time.Duration, 1000) + for i := 0; i < 1000; i++ { + results[i] = 20 * time.Nanosecond + } + return results + }(), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + require.NotEmpty(t, tt.expectedSequence) + for i, expected := range tt.expectedSequence { + actual := tt.stepper.Step() + require.Equalf(t, expected, actual, "incorrect result on step #%d, previous steps are %v", i, tt.expectedSequence[:i]) + } + + backoff, ok := tt.stepper.(*InfiniteBackoff) + require.True(t, ok) + require.NotNil(t, backoff) + require.GreaterOrEqual(t, backoff.Factor, 1.0) + }) + } +} diff --git a/internal/backoff/stepping.go b/internal/backoff/stepping.go new file mode 100644 index 00000000..d0f5f554 --- /dev/null +++ b/internal/backoff/stepping.go @@ -0,0 +1,56 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package backoff + +import ( + "context" + "time" + + "k8s.io/apimachinery/pkg/util/wait" +) + +type Stepper interface { + Step() time.Duration +} + +func wrapConditionWithNoPanics(ctx context.Context, condition wait.ConditionWithContextFunc) (done bool, err error) { + defer func() { + if r := recover(); r != nil { + if err2, ok := r.(error); ok { + err = err2 + return + } + } + }() + + return condition(ctx) +} + +func WithContext(ctx context.Context, backoff Stepper, condition wait.ConditionWithContextFunc) error { + // Loop forever, unless we reach one of the return statements below. + for { + // Stop if the context is done. + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + + // Stop trying unless the condition function returns false. + // Allow cancellation during the attempt if the condition function respects the ctx. + if ok, err := wrapConditionWithNoPanics(ctx, condition); err != nil || ok { + return err + } + + // Calculate how long to wait before the next step. + waitBeforeRetry := backoff.Step() + + // Wait before running again, allowing cancellation during the wait. + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(waitBeforeRetry): + } + } +} diff --git a/internal/backoff/stepping_test.go b/internal/backoff/stepping_test.go new file mode 100644 index 00000000..432f8900 --- /dev/null +++ b/internal/backoff/stepping_test.go @@ -0,0 +1,124 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package backoff + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/wait" +) + +type MockStepper struct { + steps []time.Duration + currentStep int +} + +func (m *MockStepper) Step() time.Duration { + result := m.steps[m.currentStep] + m.currentStep++ + return result +} + +func TestWithContext(t *testing.T) { + tests := []struct { + name string + steps []time.Duration + finalCondition wait.ConditionWithContextFunc + expectedErr error + }{ + { + name: "cancelling results in cancellation error", + steps: []time.Duration{ + time.Duration(0), + time.Duration(0), + time.Duration(0), + }, + finalCondition: func(ctx context.Context) (done bool, err error) { + return false, nil + }, + expectedErr: context.Canceled, + }, + { + name: "when condition is done, exit early", + steps: []time.Duration{ + time.Duration(0), + time.Duration(0), + time.Duration(0), + }, + finalCondition: func(ctx context.Context) (done bool, err error) { + return true, nil + }, + expectedErr: nil, + }, + { + name: "when condition returns error, exit early", + steps: []time.Duration{ + time.Duration(0), + time.Duration(0), + time.Duration(0), + time.Duration(0), + time.Duration(0), + }, + finalCondition: func(ctx context.Context) (done bool, err error) { + return false, errors.New("error from condition") + }, + expectedErr: errors.New("error from condition"), + }, + { + name: "when condition panics, cover and exit", + steps: []time.Duration{ + time.Duration(0), + }, + finalCondition: func(ctx context.Context) (done bool, err error) { + panic(errors.New("panic error")) + }, + expectedErr: errors.New("panic error"), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + testContext, cancel := context.WithCancel(context.Background()) + backoff := &MockStepper{ + steps: tt.steps, + } + actualConditionCalls := 0 + + err := WithContext(testContext, backoff, func(ctx context.Context) (done bool, err error) { + actualConditionCalls++ + + if backoff.currentStep >= (len(backoff.steps) - 1) { + cancel() + return tt.finalCondition(ctx) + } + + return false, nil + }) + require.Equal(t, tt.expectedErr, err) + require.Equal(t, len(backoff.steps), actualConditionCalls) + }) + } + + t.Run("does not invoke any functions when run with a cancelled context", func(t *testing.T) { + testContext, cancel := context.WithCancel(context.Background()) + cancel() + + stepper := &MockStepper{} + + conditionCalls := 0 + condition := func(context.Context) (done bool, err error) { + conditionCalls++ + return false, nil + } + + err := WithContext(testContext, stepper, condition) + require.Equal(t, context.Canceled, err) + require.Equal(t, 0, conditionCalls) + require.Equal(t, 0, stepper.currentStep) + }) +} diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index 56f7a8a8..37aafff7 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -21,6 +21,7 @@ import ( "go.pinniped.dev/internal/pversion" "go.pinniped.dev/internal/registry/credentialrequest" "go.pinniped.dev/internal/registry/whoamirequest" + "go.pinniped.dev/internal/tokenclient" ) type Config struct { @@ -36,6 +37,7 @@ type ExtraConfig struct { NegotiatedSerializer runtime.NegotiatedSerializer LoginConciergeGroupVersion schema.GroupVersion IdentityConciergeGroupVersion schema.GroupVersion + TokenClient *tokenclient.TokenClient } type PinnipedServer struct { @@ -134,6 +136,24 @@ func (c completedConfig) New() (*PinnipedServer, error) { }, ) + s.GenericAPIServer.AddPostStartHookOrDie("fetch-impersonation-proxy-tokens", + func(postStartContext genericapiserver.PostStartHookContext) error { + plog.Debug("fetch-impersonation-proxy-tokens start hook starting") + defer plog.Debug("fetch-impersonation-proxy-tokens start hook completed") + + controllersShutdownWaitGroup.Add(1) + go func() { + defer controllersShutdownWaitGroup.Done() + + // Start the token client + c.ExtraConfig.TokenClient.Start(controllersCtx) + plog.Debug("fetch-impersonation-proxy-tokens start hook's background goroutine has finished") + }() + + return nil + }, + ) + s.GenericAPIServer.AddPreShutdownHookOrDie("stop-controllers", func() error { plog.Debug("stop-controllers pre shutdown hook starting") diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index bce80fd2..f5091057 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -12,7 +12,6 @@ import ( "net/http" "net/http/httputil" "net/url" - "os" "reflect" "regexp" "strings" @@ -56,6 +55,7 @@ import ( "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/tokenclient" "go.pinniped.dev/internal/valuelesscontext" ) @@ -68,26 +68,38 @@ type FactoryFunc func( port int, dynamicCertProvider dynamiccert.Private, impersonationProxySignerCA dynamiccert.Public, + impersonationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet, ) (func(stopCh <-chan struct{}) error, error) func New( port int, dynamicCertProvider dynamiccert.Private, impersonationProxySignerCA dynamiccert.Public, + impersonationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet, ) (func(stopCh <-chan struct{}) error, error) { - return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, kubeclient.Secure, nil, nil, nil) + return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, kubeclient.Secure, impersonationProxyTokenCache, nil, nil, nil) } -func newInternal( //nolint:funlen // yeah, it's kind of long. +//nolint:funlen // It is definitely too complicated. New calls newInternal, which makes another function. +func newInternal( port int, dynamicCertProvider dynamiccert.Private, impersonationProxySignerCA dynamiccert.Public, restConfigFunc ptls.RestConfigFunc, // for unit testing, should always be kubeclient.Secure in production - clientOpts []kubeclient.Option, // for unit testing, should always be nil in production + cache tokenclient.ExpiringSingletonTokenCacheGet, + baseConfig *rest.Config, // 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 + var err error + + if baseConfig == nil { + baseConfig, err = rest.InClusterConfig() + if err != nil { + return nil, err + } + } constructServer := func() (func(stopCh <-chan struct{}) error, error) { // Bare minimum server side scheme to allow for status messages to be encoded. @@ -117,7 +129,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. // along with the Kube API server's CA. // Note: any changes to the Authentication stack need to be kept in sync with any assumptions made // by getTransportForUser, especially if we ever update the TCR API to start returning bearer tokens. - kubeClientUnsafeForProxying, err := kubeclient.New(clientOpts...) + kubeClientUnsafeForProxying, err := kubeclient.New(kubeclient.WithConfig(baseConfig)) if err != nil { return nil, err } @@ -168,7 +180,8 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. ) // use the custom impersonation proxy service account credentials when reverse proxying to the API server - kubeClientForProxy, err := getReverseProxyClient(clientOpts) + + kubeClientForProxy, err := getReverseProxyClient(baseConfig, cache) if err != nil { return nil, fmt.Errorf("failed to build reverse proxy client: %w", err) } @@ -321,11 +334,6 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. return nil, fmt.Errorf("invalid mutation of impersonation authorizer detected: %#v", preparedRun.Authorizer) } - // Sanity check. Assert that we have a functioning token file to use and no bearer token. - if len(preparedRun.LoopbackClientConfig.BearerToken) != 0 || len(preparedRun.LoopbackClientConfig.BearerTokenFile) == 0 { - return nil, constable.Error("invalid impersonator loopback rest config has wrong bearer token semantics") - } - return preparedRun.Run, nil } @@ -341,28 +349,16 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. return result, nil } -func getReverseProxyClient(clientOpts []kubeclient.Option) (*kubeclient.Client, error) { - // just use the overrides given during unit tests - if len(clientOpts) != 0 { - return kubeclient.New(clientOpts...) +func getReverseProxyClient(baseConfig *rest.Config, cache tokenclient.ExpiringSingletonTokenCacheGet) (*kubeclient.Client, error) { + impersonationProxyRestConfig := kubeclient.SecureAnonymousClientConfig(baseConfig) + + authRoundTripper := func(base http.RoundTripper) http.RoundTripper { + return &authorizationRoundTripper{ + cache: cache, + base: base, + } } - - // this is the magic path where the impersonation proxy SA token is mounted - const tokenFile = "/var/run/secrets/impersonation-proxy.concierge.pinniped.dev/serviceaccount/token" //nolint:gosec // this is not a credential - - // make sure the token file we need exists before trying to use it - if _, err := os.Stat(tokenFile); err != nil { - return nil, err - } - - // build an in cluster config that uses the impersonation proxy token file - impersonationProxyRestConfig, err := rest.InClusterConfig() - if err != nil { - return nil, err - } - impersonationProxyRestConfig = kubeclient.SecureAnonymousClientConfig(impersonationProxyRestConfig) - impersonationProxyRestConfig.BearerTokenFile = tokenFile - + impersonationProxyRestConfig.Wrap(authRoundTripper) return kubeclient.New(kubeclient.WithConfig(impersonationProxyRestConfig)) } diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 81c1167f..34424151 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -55,8 +55,10 @@ import ( "go.pinniped.dev/internal/httputil/roundtripper" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil/tlsserver" + "go.pinniped.dev/internal/tokenclient" ) +// TODO: add a test without a token? func TestImpersonator(t *testing.T) { const ( priorityLevelConfigurationsVersion = "v1beta3" @@ -84,24 +86,22 @@ func TestImpersonator(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.APIPriorityAndFairness, false)() tests := []struct { - name string - clientCert *clientCert - clientImpersonateUser rest.ImpersonationConfig - clientMutateHeaders func(http.Header) - clientNextProtos []string - kubeAPIServerClientBearerTokenFile string - kubeAPIServerStatusCode int - kubeAPIServerHealthz http.Handler - anonymousAuthDisabled bool - wantKubeAPIServerRequestHeaders http.Header - wantError string - wantConstructionError string - wantAuthorizerAttributes []authorizer.AttributesRecord + name string + clientCert *clientCert + clientImpersonateUser rest.ImpersonationConfig + clientMutateHeaders func(http.Header) + clientNextProtos []string + kubeAPIServerStatusCode int + kubeAPIServerHealthz http.Handler + anonymousAuthDisabled bool + wantKubeAPIServerRequestHeaders http.Header + wantError string + wantConstructionError string + wantAuthorizerAttributes []authorizer.AttributesRecord }{ { - name: "happy path", - clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path", + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"test-username"}, "Impersonate-Group": {"test-group1", "test-group2", "system:authenticated"}, @@ -119,9 +119,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "happy path with forbidden healthz", - clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path with forbidden healthz", + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), kubeAPIServerHealthz: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte("no healthz for you")) @@ -143,9 +142,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "happy path with unauthorized healthz", - clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path with unauthorized healthz", + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), kubeAPIServerHealthz: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusUnauthorized) _, _ = w.Write([]byte("no healthz for you")) @@ -168,9 +166,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "happy path with upgrade", - clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path with upgrade", + clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), clientMutateHeaders: func(header http.Header) { header.Add("Connection", "Upgrade") header.Add("Upgrade", "spdy/3.1") @@ -199,9 +196,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "happy path ignores forwarded header", - clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path ignores forwarded header", + clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), clientMutateHeaders: func(header http.Header) { header.Add("X-Forwarded-For", "example.com") }, @@ -222,9 +218,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "happy path ignores forwarded header canonicalization", - clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "happy path ignores forwarded header canonicalization", + clientCert: newClientCert(t, ca, "test-username2", []string{"test-group3", "test-group4"}), clientMutateHeaders: func(header http.Header) { header["x-FORWARDED-for"] = append(header["x-FORWARDED-for"], "example.com") }, @@ -245,11 +240,10 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "user is authenticated but the kube API request returns an error", - kubeAPIServerStatusCode: http.StatusNotFound, - clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: `the server could not find the requested resource (get namespaces)`, + name: "user is authenticated but the kube API request returns an error", + kubeAPIServerStatusCode: http.StatusNotFound, + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), + wantError: `the server could not find the requested resource (get namespaces)`, wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"test-username"}, "Impersonate-Group": {"test-group1", "test-group2", "system:authenticated"}, @@ -267,9 +261,8 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "when there is no client cert on request, it is an anonymous request", - clientCert: &clientCert{}, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "when there is no client cert on request, it is an anonymous request", + clientCert: &clientCert{}, wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"system:anonymous"}, "Impersonate-Group": {"system:unauthenticated"}, @@ -294,7 +287,6 @@ func TestImpersonator(t *testing.T) { req := &http.Request{Header: header} req.SetBasicAuth("foo", "bar") }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"system:anonymous"}, "Impersonate-Group": {"system:unauthenticated"}, @@ -313,17 +305,15 @@ func TestImpersonator(t *testing.T) { }, }, { - name: "failed client cert authentication", - clientCert: newClientCert(t, unrelatedCA, "test-username", []string{"test-group1"}), - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: "Unauthorized", - wantAuthorizerAttributes: nil, + name: "failed client cert authentication", + clientCert: newClientCert(t, unrelatedCA, "test-username", []string{"test-group1"}), + wantError: "Unauthorized", + wantAuthorizerAttributes: nil, }, { - name: "nested impersonation by regular users calls delegating authorizer", - clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), - clientImpersonateUser: rest.ImpersonationConfig{UserName: "some-other-username"}, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", + name: "nested impersonation by regular users calls delegating authorizer", + clientCert: newClientCert(t, ca, "test-username", []string{"test-group1", "test-group2"}), + clientImpersonateUser: rest.ImpersonationConfig{UserName: "some-other-username"}, // 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: ` + @@ -359,7 +349,6 @@ func TestImpersonator(t *testing.T) { "alpha.kubernetes.io/identity/user/domain/name": {"a-domain-name"}, }, }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"fire"}, "Impersonate-Group": {"elements", "system:authenticated"}, @@ -477,8 +466,7 @@ func TestImpersonator(t *testing.T) { clientMutateHeaders: func(header http.Header) { header["Impersonate-Uid"] = []string{"root"} }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: "Internal error occurred: unimplemented functionality - unable to act as current user", + 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}, @@ -501,8 +489,7 @@ func TestImpersonator(t *testing.T) { clientMutateHeaders: func(header http.Header) { header["imPerSoNaTE-uid"] = []string{"magic"} }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: "Internal error occurred: unimplemented functionality - unable to act as current user", + 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}, @@ -529,8 +516,7 @@ func TestImpersonator(t *testing.T) { "something.impersonation-proxy.concierge.pinniped.dev": {"bad data"}, }, }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: "Internal error occurred: unimplemented functionality - unable to act as current user", + 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}, @@ -569,8 +555,7 @@ func TestImpersonator(t *testing.T) { "party~~time": {"danger"}, }, }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: "Internal error occurred: unimplemented functionality - unable to act as current user", + 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}, @@ -600,7 +585,6 @@ func TestImpersonator(t *testing.T) { "ROAR": {"tiger"}, // by the time our code sees this key, it is lowercased to "roar" }, }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", wantKubeAPIServerRequestHeaders: http.Header{ "Impersonate-User": {"panda"}, "Impersonate-Group": {"other-peeps", "system:authenticated"}, @@ -631,11 +615,6 @@ func TestImpersonator(t *testing.T) { }, }, }, - { - 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", kubeAPIServerHealthz: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -651,7 +630,6 @@ func TestImpersonator(t *testing.T) { clientMutateHeaders: func(header http.Header) { header["imPerSonaTE-USer"] = []string{"PANDA"} }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", wantError: `users "PANDA" is forbidden: User "test-username" ` + `cannot impersonate resource "users" in API group "" at the cluster scope: ` + `decision made by impersonation-proxy.concierge.pinniped.dev`, @@ -668,9 +646,8 @@ func TestImpersonator(t *testing.T) { clientMutateHeaders: func(header http.Header) { header["imPerSonaTE-uid"] = []string{"007"} }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - wantError: `an error on the server ("Internal Server Error: \"/api/v1/namespaces\": requested [{UID 007 authentication.k8s.io/v1 }] without impersonating a user") has prevented the request from succeeding (get namespaces)`, - wantAuthorizerAttributes: []authorizer.AttributesRecord{}, + wantError: `an error on the server ("Internal Server Error: \"/api/v1/namespaces\": requested [{UID 007 authentication.k8s.io/v1 }] without impersonating a user") has prevented the request from succeeding (get namespaces)`, + wantAuthorizerAttributes: []authorizer.AttributesRecord{}, }, { name: "future UID header", // no longer future as it exists in Kube v1.22 @@ -678,9 +655,8 @@ func TestImpersonator(t *testing.T) { clientMutateHeaders: func(header http.Header) { header["Impersonate-Uid"] = []string{"008"} }, - kubeAPIServerClientBearerTokenFile: "required-to-be-set", - 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{}, + 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{}, }, } for _, tt := range tests { @@ -830,11 +806,8 @@ func TestImpersonator(t *testing.T) { // Create the client config that the impersonation server should use to talk to the Kube API server. testKubeAPIServerKubeconfig := rest.Config{ Host: testKubeAPIServer.URL, - BearerToken: "some-service-account-token", TLSClientConfig: rest.TLSClientConfig{CAData: tlsserver.TLSTestServerCA(testKubeAPIServer)}, - BearerTokenFile: tt.kubeAPIServerClientBearerTokenFile, } - clientOpts := []kubeclient.Option{kubeclient.WithConfig(&testKubeAPIServerKubeconfig)} // Punch out just enough stuff to make New actually run without error. recOpts := func(options *genericoptions.RecommendedOptions) { @@ -873,7 +846,9 @@ func TestImpersonator(t *testing.T) { } // Create an impersonator. Use an invalid port number to make sure our listener override works. - runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, clientOpts, recOpts, recConfig) + cache := tokenclient.NewExpiringSingletonTokenCache() + cache.Set("some-service-account-token", 1*time.Hour) + runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, cache, &testKubeAPIServerKubeconfig, recOpts, recConfig) if len(tt.wantConstructionError) > 0 { require.EqualError(t, constructionErr, tt.wantConstructionError) require.Nil(t, runner) @@ -922,7 +897,7 @@ func TestImpersonator(t *testing.T) { client, err := kubeclient.New(kubeclient.WithConfig(clientKubeconfig)) require.NoError(t, err) - // The fake Kube API server knows how to to list namespaces, so make that request using the client + // The fake Kube API server knows how to list namespaces, so make that request using the client // through the impersonator. listResponse, err := client.Kubernetes.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) if len(tt.wantError) > 0 { diff --git a/internal/concierge/impersonator/roundtripper.go b/internal/concierge/impersonator/roundtripper.go new file mode 100644 index 00000000..a028b7f7 --- /dev/null +++ b/internal/concierge/impersonator/roundtripper.go @@ -0,0 +1,34 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package impersonator + +import ( + "fmt" + "net/http" + + utilnet "k8s.io/apimachinery/pkg/util/net" + + "go.pinniped.dev/internal/tokenclient" +) + +type authorizationRoundTripper struct { + cache tokenclient.ExpiringSingletonTokenCacheGet + base http.RoundTripper +} + +var _ utilnet.RoundTripperWrapper = (*authorizationRoundTripper)(nil) + +func (rt *authorizationRoundTripper) WrappedRoundTripper() http.RoundTripper { + return rt.base +} + +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") + } + 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 new file mode 100644 index 00000000..75e22215 --- /dev/null +++ b/internal/concierge/impersonator/roundtripper_test.go @@ -0,0 +1,107 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package impersonator + +import ( + "errors" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "go.pinniped.dev/internal/tokenclient" +) + +func TestWrappedRoundTripper(t *testing.T) { + var base = new(oauth2.Transport) + + roundTripper := authorizationRoundTripper{ + base: base, + } + + require.Equal(t, base, roundTripper.WrappedRoundTripper()) +} + +type fakeRoundTripper struct { + request *http.Request + response *http.Response + err error +} + +func (t *fakeRoundTripper) RoundTrip(request *http.Request) (*http.Response, error) { + t.request = request + return t.response, t.err +} + +var _ http.RoundTripper = (*fakeRoundTripper)(nil) + +type fakeCache struct { + token string +} + +func (c *fakeCache) Get() string { + return c.token +} + +var _ tokenclient.ExpiringSingletonTokenCacheGet = (*fakeCache)(nil) + +func TestRoundTrip(t *testing.T) { + fakeResponse := new(http.Response) + for _, tt := range []struct { + name string + token string + baseResponse *http.Response + baseError string + wantResponse *http.Response + wantError string + }{ + { + name: "happy path", + token: "token", + baseResponse: fakeResponse, + baseError: "error from base", + wantResponse: fakeResponse, + wantError: "error from base", + }, + { + name: "no token available", + token: "", // since the cache always returns a non-pointer string, this indicates empty + wantError: "no token available", + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + base := &fakeRoundTripper{ + response: new(http.Response), + err: errors.New(tt.baseError), + } + + cache := &fakeCache{ + token: tt.token, + } + + roundTripper := &authorizationRoundTripper{ + cache: cache, + base: base, + } + + //nolint:noctx // this is test code + request, err := http.NewRequest("GET", "https://example.com", nil) + require.NoError(t, err) + defer request.Body.Close() + + response, err := roundTripper.RoundTrip(request) + require.Equal(t, tt.wantResponse, response) + require.ErrorContains(t, err, tt.wantError) + defer response.Body.Close() + + if tt.token != "" { + require.Equal(t, "Bearer "+tt.token, base.request.Header.Get("Authorization")) + } else { + require.Empty(t, base.request) + } + }) + } +} diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index 4a04f39f..42830392 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -12,6 +12,8 @@ import ( "time" "github.com/spf13/cobra" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -38,6 +40,7 @@ import ( "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/pversion" "go.pinniped.dev/internal/registry/credentialrequest" + "go.pinniped.dev/internal/tokenclient" ) // App is an object that represents the pinniped-concierge application. @@ -136,6 +139,8 @@ func (a *App) runServer(ctx context.Context) error { // injected suffix). scheme, loginGV, identityGV := conciergescheme.New(*cfg.APIGroupSuffix) + impersonationProxyTokenCache := tokenclient.NewExpiringSingletonTokenCache() + // Prepare to start the controllers, but defer actually starting them until the // post start hook of the aggregated API server. buildControllers, err := controllermanager.PrepareControllers( @@ -154,6 +159,7 @@ func (a *App) runServer(ctx context.Context) error { AuthenticatorCache: authenticators, // This port should be safe to cast because the config reader already validated it. ImpersonationProxyServerPort: int(*cfg.ImpersonationProxyServerPort), + ImpersonationProxyTokenCache: impersonationProxyTokenCache, }, ) if err != nil { @@ -181,6 +187,22 @@ func (a *App) runServer(ctx context.Context) error { return fmt.Errorf("could not configure aggregated API server: %w", err) } + oneDayInSeconds := int64(24 * 60 * 60) + k8sClient, err := kubeclient.New() + if err != nil { + return fmt.Errorf("could not create default kubernetes client: %w", err) + } + aggregatedAPIServerConfig.ExtraConfig.TokenClient = tokenclient.New( + podInfo.Namespace, + cfg.NamesConfig.ImpersonationProxyServiceAccount, + k8sClient.Kubernetes, + func(tokenRequestStatus authenticationv1.TokenRequestStatus, ttl metav1.Duration) error { + impersonationProxyTokenCache.Set(tokenRequestStatus.Token, ttl.Duration) + return nil + }, + plog.New(), + tokenclient.WithExpirationSeconds(oneDayInSeconds)) + // Complete the aggregated API server config and make a server instance. server, err := aggregatedAPIServerConfig.Complete().New() if err != nil { diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index bcaf6cdd..ee4c97e1 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -161,6 +161,12 @@ func validateNames(names *NamesConfigSpec) error { if names.AgentServiceAccount == "" { missingNames = append(missingNames, "agentServiceAccount") } + if names.ImpersonationProxyServiceAccount == "" { + missingNames = append(missingNames, "impersonationProxyServiceAccount") + } + if names.ImpersonationProxyLegacySecret == "" { + missingNames = append(missingNames, "impersonationProxyLegacySecret") + } if len(missingNames) > 0 { return constable.Error("missing required names: " + strings.Join(missingNames, ", ")) } diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index fda72af8..61b14516 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -47,6 +47,8 @@ func TestFromPath(t *testing.T) { impersonationSignerSecret: impersonationSignerSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value extraName: extraName-value labels: myLabelKey1: myLabelValue1 @@ -80,6 +82,8 @@ func TestFromPath(t *testing.T) { ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", AgentServiceAccount: "agentServiceAccount-value", + ImpersonationProxyServiceAccount: "impersonationProxyServiceAccount-value", + ImpersonationProxyLegacySecret: "impersonationProxyLegacySecret-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -121,6 +125,8 @@ func TestFromPath(t *testing.T) { impersonationSignerSecret: impersonationSignerSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value extraName: extraName-value labels: myLabelKey1: myLabelValue1 @@ -156,6 +162,8 @@ func TestFromPath(t *testing.T) { ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", AgentServiceAccount: "agentServiceAccount-value", + ImpersonationProxyServiceAccount: "impersonationProxyServiceAccount-value", + ImpersonationProxyLegacySecret: "impersonationProxyLegacySecret-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -197,6 +205,8 @@ func TestFromPath(t *testing.T) { impersonationSignerSecret: impersonationSignerSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value extraName: extraName-value labels: myLabelKey1: myLabelValue1 @@ -233,6 +243,8 @@ func TestFromPath(t *testing.T) { ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", AgentServiceAccount: "agentServiceAccount-value", + ImpersonationProxyServiceAccount: "impersonationProxyServiceAccount-value", + ImpersonationProxyLegacySecret: "impersonationProxyLegacySecret-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -264,6 +276,7 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value log: level: all format: snorlax @@ -284,6 +297,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -308,6 +323,8 @@ func TestFromPath(t *testing.T) { ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", AgentServiceAccount: "agentServiceAccount-value", + ImpersonationProxyServiceAccount: "impersonationProxyServiceAccount-value", + ImpersonationProxyLegacySecret: "impersonationProxyLegacySecret-value", }, Labels: map[string]string{}, KubeCertAgentConfig: KubeCertAgentSpec{ @@ -322,7 +339,7 @@ func TestFromPath(t *testing.T) { wantError: "validate names: missing required names: servingCertificateSecret, credentialIssuer, " + "apiService, impersonationLoadBalancerService, " + "impersonationClusterIPService, impersonationTLSCertificateSecret, impersonationCACertificateSecret, " + - "impersonationSignerSecret, agentServiceAccount", + "impersonationSignerSecret, agentServiceAccount, impersonationProxyServiceAccount, impersonationProxyLegacySecret", }, { name: "Missing apiService name", @@ -337,6 +354,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: apiService", }, @@ -353,6 +372,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: credentialIssuer", }, @@ -369,6 +390,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: servingCertificateSecret", }, @@ -385,6 +408,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: impersonationLoadBalancerService", }, @@ -401,6 +426,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: impersonationClusterIPService", }, @@ -417,6 +444,8 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: impersonationTLSCertificateSecret", }, @@ -433,6 +462,8 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: impersonationCACertificateSecret", }, @@ -449,9 +480,47 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: impersonationSignerSecret", }, + { + name: "Missing impersonationProxyServiceAccount name", + yaml: here.Doc(` + --- + names: + servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate + credentialIssuer: pinniped-config + apiService: pinniped-api + impersonationLoadBalancerService: impersonationLoadBalancerService-value + impersonationClusterIPService: impersonationClusterIPService-value + impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value + impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value + agentServiceAccount: agentServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value + `), + wantError: "validate names: missing required names: impersonationProxyServiceAccount", + }, + { + name: "Missing impersonationProxyLegacySecret name", + yaml: here.Doc(` + --- + names: + servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate + credentialIssuer: pinniped-config + apiService: pinniped-api + impersonationLoadBalancerService: impersonationLoadBalancerService-value + impersonationClusterIPService: impersonationClusterIPService-value + impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value + impersonationCACertificateSecret: impersonationCACertificateSecret-value + impersonationSignerSecret: impersonationSignerSecret-value + agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + `), + wantError: "validate names: missing required names: impersonationProxyLegacySecret", + }, { name: "Missing several required names", yaml: here.Doc(` @@ -464,6 +533,8 @@ func TestFromPath(t *testing.T) { impersonationClusterIPService: impersonationClusterIPService-value impersonationSignerSecret: impersonationSignerSecret-value agentServiceAccount: agentServiceAccount-value + impersonationProxyServiceAccount: impersonationProxyServiceAccount-value + impersonationProxyLegacySecret: impersonationProxyLegacySecret-value `), wantError: "validate names: missing required names: " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret", diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index 6e818704..967a3878 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -45,6 +45,8 @@ type NamesConfigSpec struct { ImpersonationCACertificateSecret string `json:"impersonationCACertificateSecret"` ImpersonationSignerSecret string `json:"impersonationSignerSecret"` AgentServiceAccount string `json:"agentServiceAccount"` + ImpersonationProxyServiceAccount string `json:"impersonationProxyServiceAccount"` + ImpersonationProxyLegacySecret string `json:"impersonationProxyLegacySecret"` } // ServingCertificateConfigSpec contains the configuration knobs for the API's diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 99f99c3c..9cf38c76 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -47,6 +47,7 @@ import ( "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/tokenclient" ) const ( @@ -87,6 +88,8 @@ type impersonatorConfigController struct { tlsServingCertDynamicCertProvider dynamiccert.Private infoLog logr.Logger debugLog logr.Logger + + impersonationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet } func NewImpersonatorConfigController( @@ -109,6 +112,7 @@ func NewImpersonatorConfigController( impersonationSignerSecretName string, impersonationSigningCertProvider dynamiccert.Provider, log logr.Logger, + impersonationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet, ) controllerlib.Controller { secretNames := sets.NewString(tlsSecretName, caSecretName, impersonationSignerSecretName) log = log.WithName("impersonator-config-controller") @@ -136,6 +140,7 @@ func NewImpersonatorConfigController( tlsServingCertDynamicCertProvider: dynamiccert.NewServingCert("impersonation-proxy-serving-cert"), infoLog: log.V(plog.KlogLevelInfo), debugLog: log.V(plog.KlogLevelDebug), + impersonationProxyTokenCache: impersonationProxyTokenCache, }, }, withInformer(credentialIssuerInformer, @@ -488,6 +493,7 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx contr c.impersonationProxyPort, c.tlsServingCertDynamicCertProvider, c.impersonationSigningCertProvider, + c.impersonationProxyTokenCache, ) if err != nil { return err diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 8ec62f59..ac43b69a 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -45,6 +45,7 @@ import ( "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/tokenclient" ) func TestImpersonatorConfigControllerOptions(t *testing.T) { @@ -93,6 +94,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { caSignerName, nil, plog.Logr(), //nolint:staticcheck // old test with no log assertions + nil, ) credIssuerInformerFilter = observableWithInformerOption.GetFilterForInformer(credIssuerInformer) servicesInformerFilter = observableWithInformerOption.GetFilterForInformer(servicesInformer) @@ -308,6 +310,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { port int, dynamicCertProvider dynamiccert.Private, impersonationProxySignerCAProvider dynamiccert.Public, + impersationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet, ) (func(stopCh <-chan struct{}) error, error) { impersonatorFuncWasCalled++ r.Equal(8444, port) @@ -580,7 +583,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { impersonatorFunc, mTLSClientCertCASecretName, mTLSClientCertProvider, - plog.Logr(), //nolint:staticcheck // old test with no log assertions + plog.Logr(), //nolint:staticcheck // old test with no log assertions, + nil, ) controllerlib.TestWrap(t, subject, func(syncer controllerlib.Syncer) controllerlib.Syncer { tlsServingCertDynamicCertProvider = syncer.(*impersonatorConfigController).tlsServingCertDynamicCertProvider diff --git a/internal/controller/secrets/secrets.go b/internal/controller/secrets/secrets.go new file mode 100644 index 00000000..132ffa2b --- /dev/null +++ b/internal/controller/secrets/secrets.go @@ -0,0 +1,92 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package secrets + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" +) + +func NewServiceAccountTokenCleanupController( + namespace string, + legacySecretName string, + k8sClient kubernetes.Interface, + secretInformer corev1informers.SecretInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, + logger plog.Logger, +) controllerlib.Controller { + name := "service-account-token-cleanup-controller" + return controllerlib.New(controllerlib.Config{ + Name: name, + Syncer: &serviceAccountTokenCleanupController{ + name: name, + namespace: namespace, + legacySecretName: legacySecretName, + k8sClient: k8sClient, + secretInformer: secretInformer, + logger: logger.WithName(name), + }, + }, + withInformer( + secretInformer, + pinnipedcontroller.SimpleFilterWithSingletonQueue(func(obj metav1.Object) bool { + secret, ok := obj.(*corev1.Secret) + + return obj.GetNamespace() == namespace && + obj.GetName() == legacySecretName && + ok && + secret.Type == corev1.SecretTypeServiceAccountToken + }), + controllerlib.InformerOption{}, + )) +} + +type serviceAccountTokenCleanupController struct { + name string + namespace string + legacySecretName string + k8sClient kubernetes.Interface + secretInformer corev1informers.SecretInformer + logger plog.Logger +} + +func (c serviceAccountTokenCleanupController) Sync(syncCtx controllerlib.Context) error { + secrets, err := c.secretInformer.Lister().Secrets(c.namespace).List(labels.Everything()) + if err != nil { + return fmt.Errorf("unable to list all secrets in namespace %s: %w", c.namespace, err) + } + c.logger.Info(fmt.Sprintf("You have now arrived in the %s controller, found %d secrets", c.name, len(secrets))) + + foundSecret := false + for _, secret := range secrets { + if secret.Name == c.legacySecretName && secret.Type == corev1.SecretTypeServiceAccountToken { + foundSecret = true + } + } + + c.logger.Info(fmt.Sprintf( + "The %s controller has found a secret of name %s to delete with type %s", + c.name, + c.legacySecretName, + corev1.SecretTypeServiceAccountToken, + )) + + if foundSecret { + err = c.k8sClient.CoreV1().Secrets(c.namespace).Delete(syncCtx.Context, c.legacySecretName, metav1.DeleteOptions{}) + if err != nil { + return fmt.Errorf("unable to delete secret %s in namespace %s: %w", c.legacySecretName, c.namespace, err) + } + } + + return nil +} diff --git a/internal/controller/secrets/secrets_test.go b/internal/controller/secrets/secrets_test.go new file mode 100644 index 00000000..acf66605 --- /dev/null +++ b/internal/controller/secrets/secrets_test.go @@ -0,0 +1,296 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package secrets + +import ( + "bytes" + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/testutil" +) + +func TestNewServiceAccountTokenCleanupController(t *testing.T) { + namespace := "a-namespace" + legacySecretName := "a-secret" + observableWithInformerOption := testutil.NewObservableWithInformerOption() + secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() + + var log bytes.Buffer + _ = NewServiceAccountTokenCleanupController( + namespace, + legacySecretName, + nil, // not needed for this test + secretsInformer, + observableWithInformerOption.WithInformer, + plog.TestLogger(t, &log), + ) + + secretsInformerFilter := observableWithInformerOption.GetFilterForInformer(secretsInformer) + + legacySecret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: legacySecretName, Namespace: namespace}, Type: corev1.SecretTypeServiceAccountToken} + wrongName := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrongName", Namespace: namespace}, Type: corev1.SecretTypeServiceAccountToken} + wrongType := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrongType", Namespace: namespace}, Type: "other-type"} + wrongNamespace := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "wrongNamespace", Namespace: "wrong-namespace"}, Type: corev1.SecretTypeServiceAccountToken} + wrongObject := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "config-map", Namespace: namespace}} + + require.False(t, secretsInformerFilter.Add(wrongName)) + require.False(t, secretsInformerFilter.Update(wrongName, wrongNamespace)) + require.False(t, secretsInformerFilter.Update(wrongNamespace, wrongName)) + require.False(t, secretsInformerFilter.Delete(wrongName)) + + require.False(t, secretsInformerFilter.Add(wrongObject)) + require.False(t, secretsInformerFilter.Update(wrongObject, wrongNamespace)) + require.False(t, secretsInformerFilter.Update(wrongNamespace, wrongObject)) + require.False(t, secretsInformerFilter.Delete(wrongObject)) + + require.False(t, secretsInformerFilter.Add(wrongNamespace)) + require.False(t, secretsInformerFilter.Update(wrongNamespace, wrongObject)) + require.False(t, secretsInformerFilter.Update(wrongObject, wrongNamespace)) + require.False(t, secretsInformerFilter.Delete(wrongNamespace)) + + require.False(t, secretsInformerFilter.Add(wrongType)) + require.False(t, secretsInformerFilter.Update(wrongType, wrongNamespace)) + require.False(t, secretsInformerFilter.Update(wrongNamespace, wrongType)) + require.False(t, secretsInformerFilter.Delete(wrongType)) + + require.True(t, secretsInformerFilter.Add(legacySecret)) + require.True(t, secretsInformerFilter.Update(legacySecret, wrongNamespace)) + require.True(t, secretsInformerFilter.Update(wrongNamespace, legacySecret)) + require.True(t, secretsInformerFilter.Delete(legacySecret)) +} + +func TestSync(t *testing.T) { + kubeAPIClient := kubernetesfake.NewSimpleClientset() + kubeInformerClient := kubernetesfake.NewSimpleClientset() + + kubeInformers := kubeinformers.NewSharedInformerFactory( + kubeInformerClient, + 0, + ) + + namespace := "some-namespace" + secretToDelete := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-to-delete", + Namespace: namespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + secretWithWrongName := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrong-name", + Namespace: namespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + secretWithWrongType := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-to-leave-alone", + Namespace: namespace, + }, + } + secretWithWrongNamespace := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-to-delete", + Namespace: "other", + }, + Type: corev1.SecretTypeServiceAccountToken, + } + + require.NoError(t, kubeAPIClient.Tracker().Add(secretToDelete)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretToDelete)) + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongName)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongName)) + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongType)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongType)) + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongNamespace)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongNamespace)) + + var log bytes.Buffer + + controller := NewServiceAccountTokenCleanupController( + namespace, + "secret-to-delete", + kubeAPIClient, + kubeInformers.Core().V1().Secrets(), + controllerlib.WithInformer, + plog.TestLogger(t, &log), + ) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Must start informers before calling TestRunSynchronously(). + kubeInformers.Start(ctx.Done()) + + controllerlib.TestRunSynchronously(t, controller) + + err := controllerlib.TestSync(t, controller, controllerlib.Context{ + Context: ctx, + }) + require.NoError(t, err) + t.Log(log.String()) + + expectedActions := []kubetesting.Action{kubetesting.NewDeleteAction( + schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", + }, + namespace, + "secret-to-delete", + )} + actualActions := kubeAPIClient.Actions() + require.Equal(t, expectedActions, actualActions) +} + +func TestSync_NoSecretToDelete(t *testing.T) { + kubeAPIClient := kubernetesfake.NewSimpleClientset() + kubeInformerClient := kubernetesfake.NewSimpleClientset() + + kubeInformers := kubeinformers.NewSharedInformerFactory( + kubeInformerClient, + 0, + ) + + namespace := "some-namespace" + secretWithWrongName := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrong-name", + Namespace: namespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + secretWithWrongType := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-to-leave-alone", + Namespace: namespace, + }, + } + secretWithWrongNamespace := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-to-delete", + Namespace: "other", + }, + Type: corev1.SecretTypeServiceAccountToken, + } + + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongName)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongName)) + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongType)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongType)) + require.NoError(t, kubeAPIClient.Tracker().Add(secretWithWrongNamespace)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretWithWrongNamespace)) + + var log bytes.Buffer + + controller := NewServiceAccountTokenCleanupController( + namespace, + "secret-to-delete", + kubeAPIClient, + kubeInformers.Core().V1().Secrets(), + controllerlib.WithInformer, + plog.TestLogger(t, &log), + ) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Must start informers before calling TestRunSynchronously(). + kubeInformers.Start(ctx.Done()) + + controllerlib.TestRunSynchronously(t, controller) + + err := controllerlib.TestSync(t, controller, controllerlib.Context{ + Context: ctx, + }) + require.NoError(t, err) + t.Log(log.String()) + + actualActions := kubeAPIClient.Actions() + require.Empty(t, actualActions) +} + +func TestSync_ReturnsAPIErrors(t *testing.T) { + kubeAPIClient := kubernetesfake.NewSimpleClientset() + kubeInformerClient := kubernetesfake.NewSimpleClientset() + + kubeInformers := kubeinformers.NewSharedInformerFactory( + kubeInformerClient, + 0, + ) + + errorMessage := "error from API client" + kubeAPIClient.PrependReactor( + "delete", + "secrets", + func(a kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New(errorMessage) + }, + ) + namespace := "some-namespace" + + var log bytes.Buffer + + legacySecretName := "secret-to-delete" + secretToDelete := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: legacySecretName, + Namespace: namespace, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + + require.NoError(t, kubeAPIClient.Tracker().Add(secretToDelete)) + require.NoError(t, kubeInformerClient.Tracker().Add(secretToDelete)) + + controller := NewServiceAccountTokenCleanupController( + namespace, + legacySecretName, + kubeAPIClient, + kubeInformers.Core().V1().Secrets(), + controllerlib.WithInformer, + plog.TestLogger(t, &log), + ) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Must start informers before calling TestRunSynchronously(). + kubeInformers.Start(ctx.Done()) + + controllerlib.TestRunSynchronously(t, controller) + + err := controllerlib.TestSync(t, controller, controllerlib.Context{ + Context: ctx, + }) + require.ErrorContains(t, err, fmt.Sprintf("unable to delete secret %s in namespace %s: %s", legacySecretName, namespace, errorMessage)) + t.Log(log.String()) + + expectedActions := []kubetesting.Action{kubetesting.NewDeleteAction( + schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", + }, + namespace, + legacySecretName, + )} + actualActions := kubeAPIClient.Actions() + require.Equal(t, expectedActions, actualActions) +} diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 92dc1e1e..fd196535 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -25,6 +25,7 @@ import ( "go.pinniped.dev/internal/controller/authenticator/webhookcachefiller" "go.pinniped.dev/internal/controller/impersonatorconfig" "go.pinniped.dev/internal/controller/kubecertagent" + "go.pinniped.dev/internal/controller/secrets" "go.pinniped.dev/internal/controllerinit" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/deploymentref" @@ -34,6 +35,7 @@ import ( "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/leaderelection" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/tokenclient" ) const ( @@ -81,6 +83,9 @@ type Config struct { // (Note that the impersonation proxy also accepts client certs signed by the Kube API server's cert.) ImpersonationSigningCertProvider dynamiccert.Provider + // ImpersonationProxyTokenCache holds short-lived tokens for the impersonation proxy service account. + ImpersonationProxyTokenCache tokenclient.ExpiringSingletonTokenCacheGet + // ServingCertDuration is the validity period, in seconds, of the API serving certificate. ServingCertDuration time.Duration @@ -276,6 +281,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol c.NamesConfig.ImpersonationSignerSecret, c.ImpersonationSigningCertProvider, plog.Logr(), //nolint:staticcheck // old controller with lots of log statements + c.ImpersonationProxyTokenCache, ), singletonWorker, ). @@ -306,6 +312,17 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol plog.New(), ), singletonWorker, + ). + WithController( + secrets.NewServiceAccountTokenCleanupController( + c.ServerInstallationInfo.Namespace, + c.NamesConfig.ImpersonationProxyLegacySecret, + client.Kubernetes, + informers.installationNamespaceK8s.Core().V1().Secrets(), + controllerlib.WithInformer, + plog.New(), + ), + singletonWorker, ) return controllerinit.Prepare(controllerManager.Start, leaderElector, diff --git a/internal/tokenclient/expiringtokencache.go b/internal/tokenclient/expiringtokencache.go new file mode 100644 index 00000000..58b379c2 --- /dev/null +++ b/internal/tokenclient/expiringtokencache.go @@ -0,0 +1,50 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tokenclient + +import ( + "time" + + "k8s.io/apimachinery/pkg/util/cache" +) + +const tokenCacheKey = "token" + +type ExpiringSingletonTokenCacheGet interface { + Get() string +} + +type ExpiringSingletonTokenCache interface { + ExpiringSingletonTokenCacheGet + Set(token string, ttl time.Duration) +} + +type expiringCacheImpl struct { + cache *cache.Expiring +} + +var _ ExpiringSingletonTokenCacheGet = &expiringCacheImpl{} +var _ ExpiringSingletonTokenCache = &expiringCacheImpl{} + +func NewExpiringSingletonTokenCache() ExpiringSingletonTokenCache { + return &expiringCacheImpl{cache: cache.NewExpiring()} +} + +func (e *expiringCacheImpl) Get() string { + maybeToken, ok := e.cache.Get(tokenCacheKey) + if !ok { + return "" + } + + token, ok := maybeToken.(string) + if !ok { + return "" + } + + return token +} + +func (e *expiringCacheImpl) Set(token string, ttl time.Duration) { + e.cache.Set(tokenCacheKey, token, ttl) +} diff --git a/internal/tokenclient/expiringtokencache_test.go b/internal/tokenclient/expiringtokencache_test.go new file mode 100644 index 00000000..3ccb1c00 --- /dev/null +++ b/internal/tokenclient/expiringtokencache_test.go @@ -0,0 +1,35 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tokenclient + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + k8sCache "k8s.io/apimachinery/pkg/util/cache" +) + +func TestExpiringSingletonTokenCache(t *testing.T) { + cache := NewExpiringSingletonTokenCache() + require.NotNil(t, cache) + require.Empty(t, cache.Get()) + + cache.Set("i am a 12 hour token", 12*time.Hour) + require.Equal(t, "i am a 12 hour token", cache.Get()) + + cache.Set("i am a 0-TTL token", time.Duration(0)) + time.Sleep(1 * time.Millisecond) + require.Empty(t, cache.Get()) + + cache.Set("i am a very short token", 1*time.Millisecond) + time.Sleep(2 * time.Millisecond) + require.Empty(t, cache.Get()) +} + +func TestExpiringSingletonTokenCache_WithNonString(t *testing.T) { + cache := &expiringCacheImpl{cache: k8sCache.NewExpiring()} + cache.cache.Set(tokenCacheKey, true, 1*time.Hour) + require.Empty(t, cache.Get()) +} diff --git a/internal/tokenclient/tokenclient.go b/internal/tokenclient/tokenclient.go new file mode 100644 index 00000000..bfcbacfc --- /dev/null +++ b/internal/tokenclient/tokenclient.go @@ -0,0 +1,146 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tokenclient + +import ( + "context" + "fmt" + "time" + + "github.com/pkg/errors" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/utils/clock" + + "go.pinniped.dev/internal/backoff" + "go.pinniped.dev/internal/plog" +) + +type WhatToDoWithTokenFunc func(authenticationv1.TokenRequestStatus, metav1.Duration) error + +type TokenClient struct { + namespace string + serviceAccountName string + k8sClient kubernetes.Interface + whatToDoWithToken WhatToDoWithTokenFunc + expirationSeconds int64 + clock clock.Clock + logger plog.Logger +} + +type Opt func(client *TokenClient) + +func WithExpirationSeconds(expirationSeconds int64) Opt { + return func(client *TokenClient) { + client.expirationSeconds = expirationSeconds + } +} + +func New( + namespace string, + serviceAccountName string, + k8sClient kubernetes.Interface, + whatToDoWithToken WhatToDoWithTokenFunc, + logger plog.Logger, + opts ...Opt, +) *TokenClient { + client := &TokenClient{ + namespace: namespace, + serviceAccountName: serviceAccountName, + k8sClient: k8sClient, + whatToDoWithToken: whatToDoWithToken, + expirationSeconds: 600, + clock: clock.RealClock{}, + logger: logger, + } + for _, opt := range opts { + opt(client) + } + return client +} + +type howToFetchTokenFromAPIServer func(tokenRequest *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) + +func (tokenClient TokenClient) Start(ctx context.Context) { + sleeper := make(chan time.Time, 1) + // Make sure that the <-sleeper below gets run once immediately. + sleeper <- time.Now() + for { + select { + case <-ctx.Done(): + tokenClient.logger.Info("TokenClient was cancelled and is stopping") + return + case <-sleeper: + var tokenTTL metav1.Duration + err := backoff.WithContext(ctx, &backoff.InfiniteBackoff{ + Duration: 10 * time.Millisecond, + MaxDuration: 5 * time.Second, + Factor: 2.0, + }, func(ctx context.Context) (bool, error) { + var err error + var tokenRequestStatus authenticationv1.TokenRequestStatus + tokenRequestStatus, tokenTTL, err = tokenClient.fetchToken( + func(tokenRequest *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) { + return tokenClient.k8sClient.CoreV1().ServiceAccounts(tokenClient.namespace).CreateToken( + ctx, + tokenClient.serviceAccountName, + tokenRequest, + metav1.CreateOptions{}) + }, + ) + + if err != nil { + tokenClient.logger.Warning(fmt.Sprintf("Could not fetch token: %s\n", err)) + // We got an error. Swallow it and ask for retry. + return false, nil + } + + err = tokenClient.whatToDoWithToken(tokenRequestStatus, tokenTTL) + if err != nil { + tokenClient.logger.Warning(fmt.Sprintf("unable to pass token to the caller: %s\n", err)) + // We got an error. Swallow it and ask for retry. + return false, nil + } + // We got a token. Stop backing off. + return true, nil + }) + if err != nil { + // We were cancelled during our WithContext. We know it was not due to some other + // error because our last argument to WithContext above never returns any errors. + return + } + // Schedule ourselves to wake up in the future. + time.AfterFunc(tokenTTL.Duration*4/5, func() { + sleeper <- time.Now() + }) + } + } +} + +func (tokenClient TokenClient) fetchToken( + howToFetchTokenFromAPIServer howToFetchTokenFromAPIServer, +) (authenticationv1.TokenRequestStatus, metav1.Duration, error) { + tokenClient.logger.Debug(fmt.Sprintf("refreshing cache at time=%s\n", tokenClient.clock.Now().Format(time.RFC3339))) + + tokenRequestInput := &authenticationv1.TokenRequest{ + Spec: authenticationv1.TokenRequestSpec{ + ExpirationSeconds: &tokenClient.expirationSeconds, + }, + } + + tokenRequest, err := howToFetchTokenFromAPIServer(tokenRequestInput) + + emptyMetav1Duration := metav1.Duration{Duration: 0} + if err != nil { + return authenticationv1.TokenRequestStatus{}, emptyMetav1Duration, errors.Wrap(err, "error creating token") + } + + if tokenRequest == nil { + return authenticationv1.TokenRequestStatus{}, emptyMetav1Duration, errors.New("tokenRequest is nil after request") + } + + ttl := metav1.Duration{Duration: tokenRequest.Status.ExpirationTimestamp.Sub(tokenClient.clock.Now())} + return tokenRequest.Status, ttl, nil +} diff --git a/internal/tokenclient/tokenclient_test.go b/internal/tokenclient/tokenclient_test.go new file mode 100644 index 00000000..88b346a7 --- /dev/null +++ b/internal/tokenclient/tokenclient_test.go @@ -0,0 +1,202 @@ +// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tokenclient + +import ( + "bytes" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/utils/clock" + clocktesting "k8s.io/utils/clock/testing" + + "go.pinniped.dev/internal/plog" +) + +func TestNew(t *testing.T) { + mockWhatToDoWithTokenFunc := *new(WhatToDoWithTokenFunc) + mockClient := new(kubernetes.Clientset) + mockTime := time.Now() + mockClock := clocktesting.NewFakeClock(mockTime) + var log bytes.Buffer + testLogger := plog.TestLogger(t, &log) + + type args struct { + namespace string + serviceAccountName string + k8sClient *kubernetes.Clientset + whatToDoWithToken WhatToDoWithTokenFunc + logger plog.Logger + opts []Opt + } + + tests := []struct { + name string + args args + expected *TokenClient + }{ + { + name: "defaults", + args: args{ + namespace: "namespace", + serviceAccountName: "serviceAccountName", + k8sClient: mockClient, + whatToDoWithToken: mockWhatToDoWithTokenFunc, + logger: testLogger, + }, + expected: &TokenClient{ + namespace: "namespace", + serviceAccountName: "serviceAccountName", + k8sClient: mockClient, + whatToDoWithToken: mockWhatToDoWithTokenFunc, + expirationSeconds: 600, + clock: clock.RealClock{}, + logger: testLogger, + }, + }, + { + name: "with all opts", + args: args{ + namespace: "custom-namespace", + serviceAccountName: "custom-serviceAccountName", + k8sClient: mockClient, + whatToDoWithToken: mockWhatToDoWithTokenFunc, + logger: testLogger, + opts: []Opt{ + WithExpirationSeconds(777), + withClock(mockClock), + }, + }, + expected: &TokenClient{ + namespace: "custom-namespace", + serviceAccountName: "custom-serviceAccountName", + k8sClient: mockClient, + whatToDoWithToken: mockWhatToDoWithTokenFunc, + expirationSeconds: 777, + clock: mockClock, + logger: testLogger, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + actual := New( + tt.args.namespace, + tt.args.serviceAccountName, + tt.args.k8sClient, + tt.args.whatToDoWithToken, + tt.args.logger, + tt.args.opts..., + ) + + require.Equal(t, tt.expected, actual) + }) + } +} + +// withClock should only be used for testing. +func withClock(clock clock.Clock) Opt { + return func(client *TokenClient) { + client.clock = clock + } +} + +func TestFetchToken(t *testing.T) { + mockTime := metav1.Now() + + type expected struct { + tokenRequestStatus authenticationv1.TokenRequestStatus + ttl metav1.Duration + errMessage string + } + + tests := []struct { + name string + expirationSeconds int64 + howToFetchTokenFromAPIServer howToFetchTokenFromAPIServer + expected expected + }{ + { + name: "happy path", + expirationSeconds: 555, + howToFetchTokenFromAPIServer: func(_ *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) { + tokenRequest := authenticationv1.TokenRequest{ + Status: authenticationv1.TokenRequestStatus{ + Token: "token value", + ExpirationTimestamp: metav1.NewTime(mockTime.Add(25 * time.Minute)), + }, + } + return &tokenRequest, nil + }, + expected: expected{ + tokenRequestStatus: authenticationv1.TokenRequestStatus{ + Token: "token value", + ExpirationTimestamp: metav1.NewTime(mockTime.Add(25 * time.Minute)), + }, + ttl: metav1.Duration{ + Duration: 25 * time.Minute, + }, + }, + }, + { + name: "returns errors from howToFetchTokenFromAPIServer", + expirationSeconds: 444, + howToFetchTokenFromAPIServer: func(_ *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) { + return nil, errors.New("has an error") + }, + expected: expected{ + errMessage: "error creating token: has an error", + }, + }, + { + name: "errors when howToFetchTokenFromAPIServer returns nil", + expirationSeconds: 333, + howToFetchTokenFromAPIServer: func(_ *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) { + return nil, nil + }, + expected: expected{ + errMessage: "tokenRequest is nil after request", + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + wrappedFunc := func(tokenRequest *authenticationv1.TokenRequest) (*authenticationv1.TokenRequest, error) { + require.NotNil(t, tokenRequest) + require.Equal(t, tt.expirationSeconds, *tokenRequest.Spec.ExpirationSeconds) + require.Empty(t, tokenRequest.Spec.Audiences) + require.Empty(t, tokenRequest.Spec.BoundObjectRef) + return tt.howToFetchTokenFromAPIServer(tokenRequest) + } + + mockClock := clocktesting.NewFakeClock(mockTime.Time) + var log bytes.Buffer + + tokenClient := TokenClient{ + expirationSeconds: tt.expirationSeconds, + clock: mockClock, + logger: plog.TestLogger(t, &log), + } + + tokenRequestStatus, ttl, err := tokenClient.fetchToken( + wrappedFunc, + ) + + if tt.expected.errMessage != "" { + require.ErrorContains(t, err, tt.expected.errMessage) + } else { + require.Equal(t, tt.expected.tokenRequestStatus, tokenRequestStatus) + require.Equal(t, tt.expected.ttl, ttl) + } + }) + } +} diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index dd1dadb3..f8509854 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -2051,6 +2051,7 @@ 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 7fb5a909..f1dce6f8 100644 --- a/test/integration/concierge_whoami_test.go +++ b/test/integration/concierge_whoami_test.go @@ -89,6 +89,7 @@ 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-",