diff --git a/integration/teleterm_test.go b/integration/teleterm_test.go index 7ae8f668f0d..38c65e99191 100644 --- a/integration/teleterm_test.go +++ b/integration/teleterm_test.go @@ -558,10 +558,17 @@ func testClientCache(t *testing.T, pack *dbhelpers.DatabasePack, creds *helpers. require.NoError(t, err) require.Equal(t, concurrentCallsForClient[0], secondCallForClient) - // Let's remove the client from the cache. - // The call to GetCachedClient will - // connect to proxy and return a new client. - err = daemonService.ClearCachedClientsForRoot(cluster.URI) + // Reissue user certs by assuming a role with a bogus ID in DropAccessRequests. + // This makes the cached client stale. + accessRequest := &api.AssumeRoleRequest{ + RootClusterUri: cluster.URI.String(), + DropRequestIds: []string{"does-not-matter"}, + } + err = cluster.AssumeRole(ctx, secondCallForClient, accessRequest) + require.NoError(t, err) + + // Clearing stale clients should delete the stale client and force a new one. + err = daemonService.ClearStaleCachedClientsForRoot(cluster.URI) require.NoError(t, err) thirdCallForClient, err := daemonService.GetCachedClient(ctx, cluster.URI) require.NoError(t, err) diff --git a/lib/client/clientcache/clientcache.go b/lib/client/clientcache/clientcache.go index c8f92b97180..78e4bb93cc0 100644 --- a/lib/client/clientcache/clientcache.go +++ b/lib/client/clientcache/clientcache.go @@ -186,25 +186,9 @@ func (c *Cache) Get(ctx context.Context, profileName, leafClusterName string) (* return clt, nil } -// ClearOption configures ClearForRoot behavior. -type ClearOption func(*clearConfig) - -type clearConfig struct { - onlyClearClientsWithStaleCert bool -} - -// WithClearingOnlyClientsWithStaleCert closes only clients that use outdated certs. -func WithClearingOnlyClientsWithStaleCert() ClearOption { - return func(c *clearConfig) { c.onlyClearClientsWithStaleCert = true } -} - -// ClearForRoot closes and removes clients from the cache for the root cluster and its leaf clusters. -func (c *Cache) ClearForRoot(profileName string, opts ...ClearOption) error { - cfg := &clearConfig{} - for _, o := range opts { - o(cfg) - } - +// ClearStaleClientsForRoot closes and removes clients from the cache +// for the root cluster and its leaf clusters, if their cert is outdated. +func (c *Cache) ClearStaleClientsForRoot(profileName string) error { c.mu.Lock() defer c.mu.Unlock() @@ -217,16 +201,14 @@ func (c *Cache) ClearForRoot(profileName string, opts ...ClearOption) error { if k.profile != profileName { continue } - if cfg.onlyClearClientsWithStaleCert { - stale, err := clt.isTLSCertStale() - // If an error occurs, close the client as well. - if err != nil { - errors = append(errors, err) - } else if !stale { - continue - } + stale, err := clt.isTLSCertStale() + // If an error occurs, close the client as well. + if err != nil { + errors = append(errors, err) + } else if !stale { + continue } - if err := clt.client.Close(); err != nil { + if err = clt.client.Close(); err != nil { errors = append(errors, err) } deleted = append(deleted, k.String()) @@ -275,7 +257,8 @@ func (c *Cache) getFromCache(k key) *clientWithMetadata { // NoCache is a client cache implementation that returns a new client // on each call to Get. // -// ClearForRoot and Clear still work as expected. +// Clear works as expected. +// ClearStaleClientsForRoot always clears clients for the root cluster. type NoCache struct { mu sync.Mutex newClientFunc NewClientFunc @@ -314,7 +297,7 @@ func (c *NoCache) Get(ctx context.Context, profileName, leafClusterName string) return newClient, nil } -func (c *NoCache) ClearForRoot(profileName string, _ ...ClearOption) error { +func (c *NoCache) ClearStaleClientsForRoot(profileName string) error { c.mu.Lock() defer c.mu.Unlock() diff --git a/lib/client/clientcache/clientcache_test.go b/lib/client/clientcache/clientcache_test.go index 6e883dd92a4..b1a0dc1b2c7 100644 --- a/lib/client/clientcache/clientcache_test.go +++ b/lib/client/clientcache/clientcache_test.go @@ -67,12 +67,13 @@ func TestClearingClientsWithStaleCert(t *testing.T) { cache, err := New(Config{ NewClientFunc: func(ctx context.Context, profileName, leafClusterName string) (*client.TeleportClient, error) { config := &client.Config{ - ClientStore: clientStore, - SSHProxyAddr: "localhost:3080", - WebProxyAddr: "localhost:3080", - Username: "testuser", - Tracer: tracing.NoopProvider().Tracer("test"), - SiteName: "root", + ClientStore: clientStore, + SSHProxyAddr: "localhost:3080", + WebProxyAddr: "localhost:3080", + Username: "testuser", + Tracer: tracing.NoopProvider().Tracer("test"), + SiteName: "root", + AddKeysToAgent: client.AddKeysToAgentNo, } if leafClusterName != "" { config.SiteName = leafClusterName @@ -106,7 +107,7 @@ func TestClearingClientsWithStaleCert(t *testing.T) { require.NoError(t, err) // Clear stale clients. - err = cache.ClearForRoot("root", WithClearingOnlyClientsWithStaleCert()) + err = cache.ClearStaleClientsForRoot("root") require.NoError(t, err) newRootClient, err := cache.Get(t.Context(), "root", "") @@ -120,6 +121,15 @@ func TestClearingClientsWithStaleCert(t *testing.T) { require.NotEqual(t, newLeaf1Client, leaf1Client) // The client opened after updating the cert should be untouched. require.Equal(t, newLeaf2Client, leaf2Client) + + // Verify that the client is considered "stale" after logging out. + err = clientStore.DeleteKeyRing(keyRing.KeyRingIndex) + require.NoError(t, err) + err = cache.ClearStaleClientsForRoot("root") + require.NoError(t, err) + _, err = cache.Get(t.Context(), "root", "") + // Getting the client should return an error because we are logged out. + require.ErrorContains(t, err, "are you logged in?") } // makeCerts makes TSL and SSH certs. diff --git a/lib/teleterm/apiserver/handler/handler_auth.go b/lib/teleterm/apiserver/handler/handler_auth.go index dc0cc7bdcf2..eeb2a6fda22 100644 --- a/lib/teleterm/apiserver/handler/handler_auth.go +++ b/lib/teleterm/apiserver/handler/handler_auth.go @@ -61,7 +61,7 @@ func (s *Handler) Login(ctx context.Context, req *api.LoginRequest) (*api.EmptyR // Clear the cache after login, not before. // During a re-login, another thread might try to retrieve a client from the cache. // Because the cache is empty, it could initialize a new client using the previous certificate. - if err = s.DaemonService.ClearCachedClientsForRoot(cluster.URI); err != nil { + if err = s.DaemonService.ClearStaleCachedClientsForRoot(cluster.URI); err != nil { return nil, trace.Wrap(err) } @@ -103,7 +103,7 @@ func (s *Handler) LoginPasswordless(stream api.TerminalService_LoginPasswordless // Clear the cache after login, not before. // During a re-login, another thread might try to retrieve a client from the cache. // Because the cache is empty, it could initialize a new client using the previous certificate. - err = s.DaemonService.ClearCachedClientsForRoot(cluster.URI) + err = s.DaemonService.ClearStaleCachedClientsForRoot(cluster.URI) return trace.Wrap(err) } diff --git a/lib/teleterm/daemon/config.go b/lib/teleterm/daemon/config.go index ee758d72e8c..9764074b236 100644 --- a/lib/teleterm/daemon/config.go +++ b/lib/teleterm/daemon/config.go @@ -93,9 +93,9 @@ type ClientCache interface { // otherwise it dials the remote server. // The caller should not close the returned client. Get(ctx context.Context, profileName, leafClusterName string) (*client.ClusterClient, error) - // ClearForRoot closes and removes clients from the cache - // for the root cluster and its leaf clusters. - ClearForRoot(profileName string, opts ...clientcache.ClearOption) error + // ClearStaleClientsForRoot closes and removes clients from the cache + // for the root cluster and its leaf clusters, if their cert is outdated. + ClearStaleClientsForRoot(profileName string) error // Clear closes and removes all clients. Clear() error } diff --git a/lib/teleterm/daemon/daemon.go b/lib/teleterm/daemon/daemon.go index 13e4ca77f33..bcc7e197c04 100644 --- a/lib/teleterm/daemon/daemon.go +++ b/lib/teleterm/daemon/daemon.go @@ -40,7 +40,6 @@ import ( api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/client" - "github.com/gravitational/teleport/lib/client/clientcache" "github.com/gravitational/teleport/lib/client/sso" dtauthn "github.com/gravitational/teleport/lib/devicetrust/authn" "github.com/gravitational/teleport/lib/teleterm/api/uri" @@ -353,7 +352,7 @@ func (s *Service) ClusterLogout(ctx context.Context, uri uri.ResourceURI, remove return trace.Wrap(err) } - return trace.Wrap(s.ClearCachedClientsForRoot(uri)) + return trace.Wrap(s.ClearStaleCachedClientsForRoot(uri)) } // CreateGateway creates a gateway to given targetURI @@ -875,7 +874,7 @@ func (s *Service) AssumeRole(ctx context.Context, req *api.AssumeRoleRequest) er } // We have to reconnect using the updated cert. - return trace.Wrap(s.ClearCachedClientsForRoot(cluster.URI)) + return trace.Wrap(s.ClearStaleCachedClientsForRoot(cluster.URI)) } // ListKubernetesResourcesRequest defines a request to retrieve kube resources paginated. @@ -1277,18 +1276,11 @@ func (s *Service) GetCachedClient(ctx context.Context, resourceURI uri.ResourceU return clt, trace.Wrap(err) } -// ClearCachedClientsForRoot closes and removes clients from the cache -// for the root cluster and its leaf clusters. -func (s *Service) ClearCachedClientsForRoot(clusterURI uri.ResourceURI) error { - profileName := clusterURI.GetProfileName() - return trace.Wrap(s.clientCache.ClearForRoot(profileName)) -} - // ClearStaleCachedClientsForRoot closes and removes clients from the cache // for the root cluster and its leaf clusters, if their cert is outdated. func (s *Service) ClearStaleCachedClientsForRoot(clusterURI uri.ResourceURI) error { profileName := clusterURI.GetProfileName() - err := s.clientCache.ClearForRoot(profileName, clientcache.WithClearingOnlyClientsWithStaleCert()) + err := s.clientCache.ClearStaleClientsForRoot(profileName) return trace.Wrap(err) } diff --git a/tool/tsh/common/vnet_client_application.go b/tool/tsh/common/vnet_client_application.go index a4e8c9a8724..3a3688616a9 100644 --- a/tool/tsh/common/vnet_client_application.go +++ b/tool/tsh/common/vnet_client_application.go @@ -201,7 +201,7 @@ func (p *vnetClientApplication) retryWithRelogin(ctx context.Context, tc *client return nil }), client.WithAfterLoginHook(func() error { - return trace.Wrap(p.clientCache.ClearForRoot(profileName), "clearing client cache after relogin") + return trace.Wrap(p.clientCache.ClearStaleClientsForRoot(profileName), "clearing client cache after relogin") }), client.WithMakeCurrentProfile(false), )