gravitational / teleport

The easiest, and most secure way to access and protect all of your infrastructure.
https://goteleport.com
GNU Affero General Public License v3.0
17.42k stars 1.74k forks source link

Connect tries to login to previous session instead of logging in to the DeviceWebToken's session #40962

Open codingllama opened 5 months ago

codingllama commented 5 months ago

This issue is within the context of device trust web. A small explanation: when doing device trust web, the Web UI attempts to open a teleport:// URL. That URL has most of the information required to start a login, namely the username and the proxy address.

Let's say Connect has an expired session for "cluster1", but doesn't know about "cluster2". We are running device web authentication against "cluster2", thus opening Connect via the corresponding "teleport://" URL.

Expected behavior:

Connect is expected to ignore the expired session for "cluster1" and attempt to login directly to "cluster2", as informed by the teleport:// URL.

Current behavior:

Connect attempts to re-connect to the expired session for "cluster1" first. After that is completed or canceled, then it shows the popup to authenticate for "cluster2".

To repro, do the following:

  1. Remove all clusters from Connect
  2. Login to "cluster1"
  3. Close connect
  4. Delete all keys for "cluster1" to simulate an expired credential (eg, rm -fr ~/Library/Application\ Support/Teleport\ Connect/tsh/keys/cluster1/)
  5. Open a device authentication URL for "cluster2" (eg, open 'teleport://alpaca@cluster2:443/authenticate_web_device?id=4fbab9eb-568e-437b-ab89-ca45b843e8aa&token=YqkvcRfkZPsRHg4qiFz9UayN1zMJ97kcTwjjpiABEBQ')

Connect opens and immediately attempts to connect to "cluster1" (zarq in the screenshot):

image

After canceled (ESC), it shows the popup to connect to "cluster2":

image

Bug details:

codingllama commented 5 months ago

As @avatus put it, in much fewer words:

  1. Connect opens and tries to open any sessions
  2. Sees the expired session and tries to reauth
  3. Now that its done "starting", it opens the deep link
codingllama commented 5 months ago

It's probably the same, but we should also consider what happens with distinct users for the same cluster.

ravicious commented 5 months ago

Launching a deep link waits until the frontend app is ready:

https://github.com/gravitational/teleport/blob/606bcb1d417cfe51f79cb32eb8a4090e5204a3a2/web/packages/teleterm/src/mainProcess/windowsManager.ts#L180-L184

Currently the readiness is signaled after WorkspacesService.prototype.restorePersistedState resolves, which includes a call to restore the previous workspace. This is what triggers the login modal for the previous cluster and waits until that modal is closed.

https://github.com/gravitational/teleport/blob/606bcb1d417cfe51f79cb32eb8a4090e5204a3a2/web/packages/teleterm/src/ui/AppInitializer/AppInitializer.tsx#L40-L43

https://github.com/gravitational/teleport/blob/606bcb1d417cfe51f79cb32eb8a4090e5204a3a2/web/packages/teleterm/src/ui/AppInitializer/showStartupModalsAndNotifications.ts#L28-L51

The solution would be to:

  1. Move this.setActiveWorkspace out of WorkspacesService.prototype.restorePersistedState into showStartupModalsAndNotifications. (restorePersistedState is called only once in the app).
  2. Signal readiness after restorePersistedState but before setActiveWorkspace.
    • Similarly, isInitialized of WorkspacesService should be set to true after restorePersistedState but before setActiveWorkspace. This affects VNet only though, not deep links.
  3. Make setActiveWorkspace accept a signal.
  4. WindowsManagerIpc.SignalUserInterfaceReadiness should return an AbortController that aborts the signal passed to setActiveWorkspace in showStartupModalsAndNotifications.
  5. WindowsManager.prototype.launchDeepLink should pass the abort controller to DeepLinksService.prototype.launchDeepLink in the render process.
    • So far, each deep link handler already sets the correct workspace by calling loginAndSetActiveWorkspace. That method should abort the abort controller and show the modal for the right workspace. We cannot always abort the controller, because in the future there might be deep links that are not related to a particular workspace.

Instead of passing an abort controller back and forth between processes through WindowsManagerIpc.SignalUserInterfaceReadiness, perhaps we can create a single abort controller in AppContext (called, idk, restoreWorkspaceAbortController) and then pass it to both DeepLinksService and showStartupModalsAndNotifications.

ravicious commented 3 weeks ago

It's probably the same, but we should also consider what happens with distinct users for the same cluster.

I believe https://github.com/gravitational/teleport/issues/45714 is now tracking this.