Skip to content

lib/teleterm: Do not reissue user certs before issuing gateway cert #62096

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.

Sign up for GitHub

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

Merged
ravicious merged 2 commits into from
Dec 10, 2025
Merged

lib/teleterm: Do not reissue user certs before issuing gateway cert #62096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b838fd9
lib/teleterm: Do not ReissueUserCerts before issuing gateway cert
ravicious Dec 8, 2025
047dbc3
Explain why clearing stale clients needs to be done before check for …
ravicious Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lib/teleterm/clusters/cluster_apps.go
View file
Open in desktop
Copy link
Copy Markdown
Member Author

@ravicious ravicious Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gzdunek Do you remember why we clear stale clients before checking if access has changed rather than after this check? Off the top of my head I cannot tell what situation this would address.

teleport/web/packages/teleterm/src/mainProcess/clusterLifecycleManager/clusterLifecycleManager.ts

Lines 247 to 260 in 9ad1f18

// Only clear clients with outdated certificates.
// The watcher 'changed' event may be emitted right after the user logs in
// or assumes a role via Connect (which already closes all clients
// for the profile), so we avoid closing them again if they're already up to date.
await client.clearStaleClusterClients({ rootClusterUri: next.uri });
await this.syncOrUpdateCluster(next);
if (!hasAccessChanged(previous.loggedInUser, next.loggedInUser)) {
return;
}
await this.rendererEventHandler.send({
op: 'did-change-access',
uri: next.uri,
});

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clients should also be cleared when the user logs in again (via tsh), otherwise the auth server might reject requests coming from a stale client. In that case, only the validUntil field would be different.

My intent here was to clear the clients when any property of the profile changes, and then run a more detailed check to detect potential access changes.

Copy link
Copy Markdown
Member Author

@ravicious ravicious Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reject requests from a stale client as in a client with a stale cert in case disconnect_expired_cert is enabled?

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Copy Markdown
Member Author

@ravicious ravicious Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this explanation to the comment in code.

Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,6 @@ func GetApp(ctx context.Context, authClient authclient.ClientI, appName string)

// ReissueAppCert issue new certificates for the app and saves them to disk.
func (c *Cluster) ReissueAppCert(ctx context.Context, clusterClient *client.ClusterClient, routeToApp proto.RouteToApp) (tls.Certificate, error) {
// Refresh the certs to account for clusterClient.SiteName pointing at a leaf cluster.
err := clusterClient.ReissueUserCerts(ctx, client.CertCacheKeep, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
AccessRequests: c.status.ActiveRequests,
})
if err != nil {
return tls.Certificate{}, trace.Wrap(err)
}

result, err := clusterClient.IssueUserCertsWithMFA(ctx, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
RouteToApp: routeToApp,
Expand Down
9 changes: 0 additions & 9 deletions lib/teleterm/clusters/cluster_databases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,6 @@ func (c *Cluster) reissueDBCerts(ctx context.Context, clusterClient *client.Clus
return tls.Certificate{}, trace.BadParameter("the username must be present")
}

// Refresh the certs to account for clusterClient.SiteName pointing at a leaf cluster.
err := clusterClient.ReissueUserCerts(ctx, client.CertCacheKeep, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
AccessRequests: c.status.ActiveRequests,
})
if err != nil {
return tls.Certificate{}, trace.Wrap(err)
}

result, err := clusterClient.IssueUserCertsWithMFA(ctx, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
RouteToDatabase: client.RouteToDatabaseToProto(routeToDatabase),
Expand Down
9 changes: 0 additions & 9 deletions lib/teleterm/clusters/cluster_kubes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,6 @@ type KubeServer struct {

// reissueKubeCert issue new certificates for kube cluster and saves them to disk.
func (c *Cluster) reissueKubeCert(ctx context.Context, clusterClient *client.ClusterClient, kubeCluster string) (tls.Certificate, error) {
// Refresh the certs to account for clusterClient.SiteName pointing at a leaf cluster.
err := clusterClient.ReissueUserCerts(ctx, client.CertCacheKeep, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
AccessRequests: c.status.ActiveRequests,
})
if err != nil {
return tls.Certificate{}, trace.Wrap(err)
}

result, err := clusterClient.IssueUserCertsWithMFA(
ctx, client.ReissueParams{
RouteToCluster: c.clusterClient.SiteName,
Expand Down
3 changes: 3 additions & 0 deletions web/packages/teleterm/src/mainProcess/clusterLifecycleManager/clusterLifecycleManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ export class ClusterLifecycleManager {
// The watcher 'changed' event may be emitted right after the user logs in
// or assumes a role via Connect (which already closes all clients
// for the profile), so we avoid closing them again if they're already up to date.
//
// This needs to be done before the check for changed access. It handles a scenario where the
// user logged in again via tsh and the auth server has disconnect_expired_cert enabled.
await client.clearStaleClusterClients({ rootClusterUri: next.uri });
await this.syncOrUpdateCluster(next);

Expand Down
Loading