Fix unrecoverable ssh cert errors in tsh/Connect (#61322)

* Initialize default Username/HostLogin only in tsh

* Move `Username()` from `api.go` to `tsh.go`

* Remove wrong `Profile.SiteName` default

* Remove resetting `SiteName`

Not sure why it was needed. Perhaps to clear the default that we just removed? But even if add the default back and remove this fix, everything works.

* Gracefully handle missing SSH/TLS certs

* Remove unused `TeleportClient.LoadKeyForClusterWithReissue`

* Revert "Move `Username()` from `api.go` to `tsh.go`"

This reverts commit f7ff0ff6d1.

* Revert "Initialize default Username/HostLogin only in tsh"

This reverts commit ed38babf5e.

* When any of SSH/TLS cert is missing, return partial profile

* Only log non-nil errors

* Revert "Remove wrong `Profile.SiteName` default"

* Revert "Remove resetting `SiteName`"

This reverts commit f54ab3f22a.

* Set `SiteName` when adding cluster

* Improve comments

* Add test

* Fix test

* Add myself to TODO

* Add test for logging out with missing SSH cert

* Lint

api/profile/profile.go

@@ -392,6 +392,10 @@ func profileFromFile(filePath string) (*Profile, error) {
 	// Older versions of tsh did not always store the cluster name in the
 	// profile. If no cluster name is found, fallback to the name of the profile
 	// for backward compatibility.
+	//
+	// TODO(gzdunek): A profile name is not the same thing as a site name, and they differ when the proxy hostname is different
+	// from the cluster name.
+	// Instead, tsh should be able to handle an empty site name, or this default should be changed.
 	if p.SiteName == "" {
 		p.SiteName = p.Name()
 	}

integration/teleterm_test.go

@@ -145,6 +145,11 @@ func TestTeleterm(t *testing.T) {
 		testLogout(t, pack, creds)
 	})
 
+	t.Run("setting site name", func(t *testing.T) {
+		t.Parallel()
+		testSettingSiteName(t, pack, creds)
+	})
+
 	t.Run("ListDatabaseUsers", func(t *testing.T) {
 		// ListDatabaseUsers cannot be run in parallel as it modifies the default roles of users set up
 		// through the test pack.
@@ -673,6 +678,59 @@ func testLogout(t *testing.T, pack *dbhelpers.DatabasePack, creds *helpers.UserC
 	require.NoError(t, err)
 }
 
+func testSettingSiteName(t *testing.T, pack *dbhelpers.DatabasePack, creds *helpers.UserCreds) {
+	ctx := context.Background()
+
+	tc, err := pack.Root.Cluster.NewClient(helpers.ClientConfig{
+		TeleportUser: pack.Root.User.GetName(),
+		Cluster:      "root.example.com",
+	})
+	require.NoError(t, err)
+
+	storageFakeClock := clockwork.NewFakeClockAt(time.Now())
+
+	storage, err := clusters.NewStorage(clusters.Config{
+		ClientStore:        tc.ClientStore,
+		Clock:              storageFakeClock,
+		InsecureSkipVerify: tc.InsecureSkipVerify,
+	})
+	require.NoError(t, err)
+
+	// Add a cluster.
+	cluster, clusterClient, err := storage.Add(ctx, tc.WebProxyAddr)
+	require.NoError(t, err)
+	require.Equal(t, "root.example.com", clusterClient.SiteName)
+	require.Equal(t, "root.example.com", cluster.Name)
+	// Adding a cluster should set the site name in the profile.
+	profile, err := clusterClient.GetProfile(tc.WebProxyAddr)
+	require.NoError(t, err)
+	require.Equal(t, "root.example.com", profile.SiteName)
+
+	// Simulate logging into a leaf cluster, which changes the profile's site name.
+	clusterClient.SiteName = "leaf.example.com"
+	err = clusterClient.SaveProfile(false)
+	require.NoError(t, err)
+
+	// The URI should always resolve to the target cluster, even if the profile's site name points to a different cluster.
+	cluster, clusterClient, err = storage.ResolveCluster(cluster.URI)
+	require.NoError(t, err)
+	// These are empty because the user is not logged in, so there's no cert to retrieve the root cluster name.
+	require.Empty(t, clusterClient.SiteName)
+	require.Empty(t, cluster.Name)
+	// SiteName in the profile should still point to the leaf.
+	profile, err = clusterClient.GetProfile(tc.WebProxyAddr)
+	require.NoError(t, err)
+	require.Equal(t, "leaf.example.com", profile.SiteName)
+
+	// Saving the profile with SaveProfileAndPreserveSiteName doesn't overwrite the profile
+	// with the current clusterClient.SiteName.
+	err = clusters.SaveProfileAndPreserveSiteName(clusterClient, false)
+	require.NoError(t, err)
+	profile, err = clusterClient.GetProfile(tc.WebProxyAddr)
+	require.NoError(t, err)
+	require.Equal(t, "leaf.example.com", profile.SiteName)
+}
+
 func testCreateConnectMyComputerRole(t *testing.T, pack *dbhelpers.DatabasePack) {
 	systemUser, err := user.Current()
 	require.NoError(t, err)

lib/client/api.go

@@ -1396,38 +1396,12 @@ func (tc *TeleportClient) LoadKeyForCluster(ctx context.Context, clusterName str
 	return nil
 }
 
-// LoadKeyForClusterWithReissue fetches a cluster-specific SSH key and loads it into the
-// SSH agent.  If the key is not found, it is requested to be reissued.
-func (tc *TeleportClient) LoadKeyForClusterWithReissue(ctx context.Context, clusterName string) error {
-	ctx, span := tc.Tracer.Start(
-		ctx,
-		"teleportClient/LoadKeyForClusterWithReissue",
-		oteltrace.WithSpanKind(oteltrace.SpanKindClient),
-		oteltrace.WithAttributes(attribute.String("cluster", clusterName)),
-	)
-	defer span.End()
-
-	err := tc.LoadKeyForCluster(ctx, clusterName)
-	if err == nil {
-		return nil
-	}
-	if !trace.IsNotFound(err) {
-		return trace.Wrap(err)
-	}
-	// Reissuing also loads the new key.
-	err = tc.ReissueUserCerts(ctx, CertCacheKeep, ReissueParams{RouteToCluster: clusterName})
-	if err != nil {
-		return trace.Wrap(err)
-	}
-	return nil
-}
-
 // SignersForClusterWithReissue fetches cluster-specific signers from stored certificates.
 // If the cluster certificates are not found, it is requested to be reissued.
 func (tc *TeleportClient) SignersForClusterWithReissue(ctx context.Context, clusterName string) ([]ssh.Signer, error) {
 	ctx, span := tc.Tracer.Start(
 		ctx,
-		"teleportClient/LoadKeyForClusterWithReissue",
+		"teleportClient/SignersForClusterWithReissue",
 		oteltrace.WithSpanKind(oteltrace.SpanKindClient),
 		oteltrace.WithAttributes(attribute.String("cluster", clusterName)),
 	)

lib/client/client_store.go

@@ -266,35 +266,38 @@ func (s *Store) ReadProfileStatus(proxyAddressOrProfile string) (*ProfileStatus,
 		ClusterName: profile.SiteName,
 		Username:    profile.Username,
 	}
+
+	// If we can't find a keyRing to match the profile, connect to the keyRing (hardware key),
+	// or read the full profile status, return a partial status.
+	// This is used for some superficial functions `tsh logout` and `tsh status`.
+	partialStatus := &ProfileStatus{
+		Name: profileName,
+		Dir:  profile.Dir,
+		ProxyURL: url.URL{
+			Scheme: "https",
+			Host:   profile.WebProxyAddr,
+		},
+		Username:    profile.Username,
+		Cluster:     profile.SiteName,
+		KubeEnabled: profile.KubeProxyAddr != "",
+		// Set ValidUntil to now and GetKeyRingError to show that the keys are not available.
+		ValidUntil:              time.Now(),
+		SAMLSingleLogoutEnabled: profile.SAMLSingleLogoutEnabled,
+		SSOHost:                 profile.SSOHost,
+	}
+
 	keyRing, err := s.GetKeyRing(idx, WithAllCerts...)
 	if err != nil {
 		if trace.IsNotFound(err) || trace.IsConnectionProblem(err) {
-			// If we can't find a keyRing to match the profile, or can't connect to
-			// the keyRing (hardware key), return a partial status. This is used for
-			// some superficial functions `tsh logout` and `tsh status`.
-			return &ProfileStatus{
-				Name: profileName,
-				Dir:  profile.Dir,
-				ProxyURL: url.URL{
-					Scheme: "https",
-					Host:   profile.WebProxyAddr,
-				},
-				Username:    profile.Username,
-				Cluster:     profile.SiteName,
-				KubeEnabled: profile.KubeProxyAddr != "",
-				// Set ValidUntil to now and GetKeyRingError to show that the keys are not available.
-				ValidUntil:              time.Now(),
-				GetKeyRingError:         err,
-				SAMLSingleLogoutEnabled: profile.SAMLSingleLogoutEnabled,
-				SSOHost:                 profile.SSOHost,
-			}, nil
+			partialStatus.GetKeyRingError = err
+			return partialStatus, nil
 		}
 		return nil, trace.Wrap(err)
 	}
 
 	_, onDisk := s.KeyStore.(*FSKeyStore)
 
-	return profileStatusFromKeyRing(keyRing, profileOptions{
+	profileStatus, err := profileStatusFromKeyRing(keyRing, profileOptions{
 		ProfileName:             profileName,
 		ProfileDir:              profile.Dir,
 		WebProxyAddr:            profile.WebProxyAddr,
@@ -308,6 +311,12 @@ func (s *Store) ReadProfileStatus(proxyAddressOrProfile string) (*ProfileStatus,
 		IsVirtual:               !onDisk,
 		TLSRoutingEnabled:       profile.TLSRoutingEnabled,
 	})
+	if trace.IsNotFound(err) {
+		return partialStatus, nil
+	} else if err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return profileStatus, nil
 }
 
 // FullProfileStatus returns the status of the active profile along with the

lib/teleterm/clusters/cluster.go

@@ -401,3 +401,21 @@ type Server struct {
 
 	types.Server
 }
+
+// SaveProfileAndPreserveSiteName wraps [client.TeleportClient.SaveProfile]. It restores the original site name before
+// saving the profile.
+//
+// It's needed because teleterm/clusters.Storage.loadProfileStatusAndClusterKey overwrites the profile's site name so that
+// the cluster client is created for the cluster specified in the URI rather than the one stored in the profile.
+// This adjustment is only relevant for Connect and should not be persisted.
+func SaveProfileAndPreserveSiteName(clusterClient *client.TeleportClient, makeCurrent bool) error {
+	profile, err := clusterClient.GetProfile(clusterClient.WebProxyAddr)
+	if err != nil && !trace.IsNotFound(err) {
+		return trace.Wrap(err)
+	}
+	if profile != nil {
+		clusterClient.SiteName = profile.SiteName
+	}
+	err = clusterClient.SaveProfile(makeCurrent)
+	return trace.Wrap(err)
+}

lib/teleterm/clusters/cluster_access_requests.go

@@ -234,7 +234,7 @@ func (c *Cluster) AssumeRole(ctx context.Context, rootClient *client.ClusterClie
 		return trace.Wrap(err)
 	}
 
-	err = c.clusterClient.SaveProfile(true)
+	err = SaveProfileAndPreserveSiteName(c.clusterClient, true)
 	return trace.Wrap(err)
 }
 

lib/teleterm/clusters/cluster_auth.go

@@ -53,7 +53,7 @@ func (c *Cluster) SyncAuthPreference(ctx context.Context) (*webclient.WebConfigA
 		c.Logger.DebugContext(ctx, "Got ping response", "response", string(pingResponseJSON))
 	}
 
-	if err := c.clusterClient.SaveProfile(false); err != nil {
+	if err := SaveProfileAndPreserveSiteName(c.clusterClient, false); err != nil {
 		return nil, nil, trace.Wrap(err)
 	}
 

lib/teleterm/clusters/storage.go

@@ -171,6 +171,13 @@ func (s *Storage) addCluster(ctx context.Context, webProxyAddress string) (*Clus
 		return nil, nil, trace.Wrap(err)
 	}
 
+	// There's an incorrect default in api/profile.profileFromFile - an empty SiteName is replaced with the profile name.
+	// A profile name is not the same thing as a site name, and they differ when the proxy hostname is different
+	// from the cluster name.
+	// Using this incorrect site name causes login failures in `tsh`, so we proactively set SiteName to the root cluster
+	// name instead.
+	clusterClient.SiteName = pingResponse.ClusterName
+
 	clusterLog := s.Logger.With("cluster", clusterURI)
 
 	pingResponseJSON, err := json.Marshal(pingResponse)
@@ -274,15 +281,38 @@ func (s *Storage) loadProfileStatusAndClusterKey(clusterClient *client.TeleportC
 		clusterClient.SiteName = rootClusterName
 	}
 
-	if err == nil && clusterClient.Username != "" {
-		status, err = clusterClient.ProfileStatus()
-		if err != nil {
-			return nil, trace.Wrap(err)
-		}
+	// TODO(gzdunek): If the key doesn't exist, we should still try to read
+	// the profile status.
+	// This creates an inconsistency in how the profile is interpreted after running
+	//`tsh logout --proxy=... --user=...`  by `tsh status` versus Connect.
+	//
+	// tsh will still show a profile that includes the username, while Connect
+	// receives an empty profile status and therefore has no username.
+	// Fixing this requires updating how ClusterLifecycleManager detects logouts.
+	// Right now it assumes that a logout results in an empty username.
+	// After the fix, the username would still be present, so we'll need to rely on
+	// a different field of LoggedInUser (or introduce a new one) to determine logout
+	// state reliably.
+	if err != nil || clusterClient.Username == "" {
+		return status, nil
+	}
 
-		if err := clusterClient.LoadKeyForCluster(context.Background(), status.Cluster); err != nil {
+	status, err = clusterClient.ProfileStatus()
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	// Load SSH key for the cluster indicated in the profile.
+	// Skip if the profile is empty, the key cannot be found, or the key isn't supported as an agent key.
+	err = clusterClient.LoadKeyForCluster(context.Background(), status.Cluster)
+	if err != nil {
+		if !trace.IsNotFound(err) && !trace.IsConnectionProblem(err) && !trace.IsCompareFailed(err) {
 			return nil, trace.Wrap(err)
 		}
+		s.Logger.InfoContext(context.Background(), "Could not load key for cluster into the local agent",
+			"cluster", status.Cluster,
+			"error", err,
+		)
 	}
 
 	return status, nil

tool/tsh/common/tsh_test.go

@@ -87,6 +87,7 @@ import (
 	"github.com/gravitational/teleport/lib/cryptosuites"
 	"github.com/gravitational/teleport/lib/cryptosuites/cryptosuitestest"
 	"github.com/gravitational/teleport/lib/defaults"
+	"github.com/gravitational/teleport/lib/fixtures"
 	"github.com/gravitational/teleport/lib/kube/kubeconfig"
 	"github.com/gravitational/teleport/lib/modules"
 	"github.com/gravitational/teleport/lib/modules/modulestest"
@@ -6078,6 +6079,17 @@ func TestLogout(t *testing.T) {
 				require.NoError(t, os.Remove(privKeyPath))
 			},
 		},
+		{
+			name: "TLS cert is present but SSH cert is missing",
+			modifyKeyDir: func(t *testing.T, homePath string) {
+				tlsCertPath := keypaths.TLSCertPath(homePath, clientKeyRing.ProxyHost, clientKeyRing.Username)
+				err := os.WriteFile(tlsCertPath, []byte(fixtures.TLSCACertPEM), 0o600)
+				require.NoError(t, err)
+				sshCertPath := keypaths.SSHCertPath(homePath, clientKeyRing.ProxyHost, clientKeyRing.Username, clientKeyRing.ClusterName)
+				_, err = os.ReadFile(sshCertPath)
+				require.ErrorIs(t, err, os.ErrNotExist)
+			},
+		},
 		{
 			name: "TLS private key missing",
 			modifyKeyDir: func(t *testing.T, homePath string) {