-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Invalidate cached client when TLS cert changes #59370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c73574e
97403f6
2c0988d
7e7510b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||||||
| package clientcache | ||||||||||
|
|
||||||||||
| import ( | ||||||||||
| "bytes" | ||||||||||
| "context" | ||||||||||
| "log/slog" | ||||||||||
| "slices" | ||||||||||
|
|
@@ -36,11 +37,30 @@ type Cache struct { | |||||||||
| cfg Config | ||||||||||
| mu sync.RWMutex | ||||||||||
| // clients keeps a mapping from key (profile name and leaf cluster name) to cluster client. | ||||||||||
| clients map[key]*client.ClusterClient | ||||||||||
| clients map[key]*clientWithCert | ||||||||||
| // group prevents duplicate requests to create clients for a given cluster. | ||||||||||
| group singleflight.Group | ||||||||||
| } | ||||||||||
|
|
||||||||||
| type clientWithCert struct { | ||||||||||
| // client is cluster client. | ||||||||||
| client *client.ClusterClient | ||||||||||
| // coreTLSCert is the cert used in TeleportClient.ConnectToCluster to create the client. | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
or maybe even
Suggested change
|
||||||||||
| coreTLSCert []byte | ||||||||||
| // readCoreTLSCert reads a fresh cert from disk. | ||||||||||
| readCoreTLSCert func() ([]byte, error) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's very Go-like, I assume the more idiomatic way to do it is to create a profile interface with |
||||||||||
| } | ||||||||||
|
|
||||||||||
| func (c *clientWithCert) isCoreTLSCertUnchanged() (bool, error) { | ||||||||||
| tlsCert, err := c.readCoreTLSCert() | ||||||||||
| if err != nil { | ||||||||||
| return false, trace.Wrap(err) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| equal := bytes.Equal(c.coreTLSCert, tlsCert) | ||||||||||
| return equal, nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // NewClientFunc is a function that will return a new [*client.TeleportClient] for a given profile and leaf | ||||||||||
| // cluster. [leafClusterName] may be empty, in which case implementations should return a client for the root cluster. | ||||||||||
| type NewClientFunc func(ctx context.Context, profileName, leafClusterName string) (*client.TeleportClient, error) | ||||||||||
|
|
@@ -89,7 +109,7 @@ func New(c Config) (*Cache, error) { | |||||||||
|
|
||||||||||
| return &Cache{ | ||||||||||
| cfg: c, | ||||||||||
| clients: make(map[key]*client.ClusterClient), | ||||||||||
| clients: make(map[key]*clientWithCert), | ||||||||||
| }, nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -99,8 +119,19 @@ func (c *Cache) Get(ctx context.Context, profileName, leafClusterName string) (* | |||||||||
| k := key{profile: profileName, leafCluster: leafClusterName} | ||||||||||
| groupClt, err, _ := c.group.Do(k.String(), func() (any, error) { | ||||||||||
| if fromCache := c.getFromCache(k); fromCache != nil { | ||||||||||
| c.cfg.Logger.DebugContext(ctx, "Retrieved client from cache", "cluster", k) | ||||||||||
| return fromCache, nil | ||||||||||
| unchanged, err := fromCache.isCoreTLSCertUnchanged() | ||||||||||
| if err != nil { | ||||||||||
| c.cfg.Logger.WarnContext(ctx, "Failed to validate TLS certificate, removing from cache", "cluster", k, "error", err) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } else if unchanged { | ||||||||||
| c.cfg.Logger.DebugContext(ctx, "Retrieved client from cache", "cluster", k) | ||||||||||
| return fromCache.client, nil | ||||||||||
| } else { | ||||||||||
| c.cfg.Logger.DebugContext(ctx, "TLS certificate for cached client has changed, removing from cache", "cluster", k) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| if err := c.clearForKey(k); err != nil { | ||||||||||
| return nil, trace.Wrap(err) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| tc, err := c.cfg.NewClientFunc(ctx, profileName, leafClusterName) | ||||||||||
|
|
@@ -120,8 +151,17 @@ func (c *Cache) Get(ctx context.Context, profileName, leafClusterName string) (* | |||||||||
| return nil, trace.Wrap(err) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| keyRing, err := tc.LocalAgent().GetCoreKeyRing() | ||||||||||
| if err != nil { | ||||||||||
| return nil, trace.Wrap(err) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Save the client in the cache, so we don't have to build a new connection next time. | ||||||||||
| c.addToCache(k, newClient) | ||||||||||
| c.addToCache(k, &clientWithCert{ | ||||||||||
| client: newClient, | ||||||||||
| coreTLSCert: keyRing.TLSCert, | ||||||||||
| readCoreTLSCert: tc.Profile().TLSCert, | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| c.cfg.Logger.InfoContext(ctx, "Added client to cache", "cluster", k) | ||||||||||
|
|
||||||||||
|
|
@@ -151,7 +191,7 @@ func (c *Cache) ClearForRoot(profileName string) error { | |||||||||
|
|
||||||||||
| for k, clt := range c.clients { | ||||||||||
| if k.profile == profileName { | ||||||||||
| if err := clt.Close(); err != nil { | ||||||||||
| if err := clt.client.Close(); err != nil { | ||||||||||
| errors = append(errors, err) | ||||||||||
| } | ||||||||||
| deleted = append(deleted, k.String()) | ||||||||||
|
|
@@ -175,7 +215,7 @@ func (c *Cache) Clear() error { | |||||||||
|
|
||||||||||
| var errors []error | ||||||||||
| for _, clt := range c.clients { | ||||||||||
| if err := clt.Close(); err != nil { | ||||||||||
| if err := clt.client.Close(); err != nil { | ||||||||||
| errors = append(errors, err) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -184,14 +224,28 @@ func (c *Cache) Clear() error { | |||||||||
| return trace.NewAggregate(errors...) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func (c *Cache) addToCache(k key, clusterClient *client.ClusterClient) { | ||||||||||
| func (c *Cache) clearForKey(k key) error { | ||||||||||
| c.mu.Lock() | ||||||||||
| defer c.mu.Unlock() | ||||||||||
|
|
||||||||||
| clt, ok := c.clients[k] | ||||||||||
| delete(c.clients, k) | ||||||||||
| if ok { | ||||||||||
| err := clt.client.Close() | ||||||||||
| return trace.Wrap(err) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func (c *Cache) addToCache(k key, cc *clientWithCert) { | ||||||||||
| c.mu.Lock() | ||||||||||
| defer c.mu.Unlock() | ||||||||||
|
|
||||||||||
| c.clients[k] = clusterClient | ||||||||||
| c.clients[k] = cc | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func (c *Cache) getFromCache(k key) *client.ClusterClient { | ||||||||||
| func (c *Cache) getFromCache(k key) *clientWithCert { | ||||||||||
| c.mu.RLock() | ||||||||||
| defer c.mu.RUnlock() | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,10 +40,6 @@ func (s *Handler) Login(ctx context.Context, req *api.LoginRequest) (*api.EmptyR | |
| return nil, trace.BadParameter("cluster URI must be a root URI") | ||
| } | ||
|
|
||
| if err = s.DaemonService.ClearCachedClientsForRoot(cluster.URI); err != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the removal of these constitute a significant change in behavior? In the previous version, after logging in or assuming a role, all clients created so far would be closed. With the new behavior, no client will be closed after one of those operations until another RPC attempts to get a client from the cache. Does this have any significance? It feels like it mostly could affect leaf clients as those might continue to not be closed well after a relogin / role assumption.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #59760, I tried re-enabling the client cache in tests. Currently there's a test which just straight up fails (#59760 (comment)). I think it's because in the real world, we depend on the cache being cleared during the login RPCs. In tests however we circumvent those RPCs completely (because we don't have access to user credentials so we depend on test helpers to generate new certs on disk). The test passes when I manually clear the cache on relogin (see the diff). I think once this PR is merged and when the cache no longer depends on being manually cleared, then we should be able to re-enable it in integration tests without such workarounds like the one in the diff. Diffdiff --git a/integration/proxy/teleterm_test.go b/integration/proxy/teleterm_test.go
index ee19d22c1d2..02b5112a9cb 100644
--- a/integration/proxy/teleterm_test.go
+++ b/integration/proxy/teleterm_test.go
@@ -264,6 +264,7 @@ func testGatewayCertRenewal(ctx context.Context, t *testing.T, params gatewayCer
t.Cleanup(func() {
daemonService.Stop()
})
+ tshdEventsService.daemonService = daemonService
// Connect the daemon to the tshd events service, like it would
// during normal initialization of the app.
@@ -320,6 +321,7 @@ type mockTSHDEventsService struct {
sendNotificationCallCount atomic.Uint32
promptMFACallCount atomic.Uint32
generateAndSetupUserCreds generateAndSetupUserCredsFunc
+ daemonService *daemon.Service
}
func newMockTSHDEventsServiceServer(t *testing.T, tc *libclient.TeleportClient, generateAndSetupUserCreds generateAndSetupUserCredsFunc) (service *mockTSHDEventsService) {
@@ -360,11 +362,20 @@ func newMockTSHDEventsServiceServer(t *testing.T, tc *libclient.TeleportClient,
// Relogin simulates the act of the user logging in again in the Electron app by replacing the user
// cert on disk with a valid one.
-func (c *mockTSHDEventsService) Relogin(context.Context, *api.ReloginRequest) (*api.ReloginResponse, error) {
+func (c *mockTSHDEventsService) Relogin(ctx context.Context, req *api.ReloginRequest) (*api.ReloginResponse, error) {
c.reloginCallCount.Add(1)
// Generate valid certs with the default TTL.
c.generateAndSetupUserCreds(c.t, c.tc, 0 /* ttl */)
+ if c.daemonService != nil {
+ clusterURI, err := uri.Parse(req.RootClusterUri)
+ if err != nil {
+ return nil, err
+ }
+ if err := c.daemonService.ClearCachedClientsForRoot(clusterURI); err != nil {
+ return nil, err
+ }
+ }
return &api.ReloginResponse{}, nil
}
|
||
| return nil, trace.Wrap(err) | ||
| } | ||
|
|
||
| if req.Params == nil { | ||
| return nil, trace.BadParameter("missing login parameters") | ||
| } | ||
|
|
@@ -91,10 +87,6 @@ func (s *Handler) LoginPasswordless(stream api.TerminalService_LoginPasswordless | |
| // daemon.Service.ResolveClusterURI. | ||
| clusterClient.MFAPromptConstructor = nil | ||
|
|
||
| if err := s.DaemonService.ClearCachedClientsForRoot(cluster.URI); err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| // Start the prompt flow. | ||
| if err := cluster.PasswordlessLogin(stream.Context(), stream); err != nil { | ||
| return trace.Wrap(err) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop this comment hehe.