Allow displaying hardware keys prompts when relogin is in progress #48813
Conversation
…ortant dialogs and get rid of `DialogNone`
gzdunek
added
no-changelog
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| // Since we cache the keys per slot, if two clusters specify the same one, | ||
| // the user will always see the prompt for the same cluster URI. For example, if you are logged into both | ||
| // cluster-a and cluster-b, the prompt will always say "Unlock hardware key to access cluster-b." |
There was a problem hiding this comment.
I think that having a prompt per cluster was a mistake, since one prompt can be used for multiple clusters (I didn't realize that back then). While displaying a card name will require too much work, maybe we should just not display a cluster name in the dialog? I think a scenario where a user logs in with the same key to two clusters is more likely than the one where separate keys are used for different clusters.
lib/teleterm/daemon/daemon.go
Outdated
| // | ||
| // We use a semaphore instead of a mutex in order to cancel important modals that | ||
| // are no longer relevant before acquisition. | ||
| // | ||
| // We use a waitSemaphore in order to make sure there is a clear transition between modals. | ||
| importantModalSemaphore *waitSemaphore | ||
| singleImportantModalSemaphore *waitSemaphore |
There was a problem hiding this comment.
Is this importantModalSemaphore even necessary at all anymore? If we are allowing multiple important modals now, what makes hardware key pin prompts a special case?
There was a problem hiding this comment.
Honestly, I'm a little afraid that we might run into some UX issues. For example, if you are in a middle of a relogin action, you wouldn't want to get an MFA prompt or something like that.
The difference between the semaphore here and the modal list on the fronted is that here a new modal waits for the previous one to complete, while on the fronted a new modal is placed on top of the existing one.
The special thing about hardware key prompts is that it's the only case where we need to complete an action started in one modal in another modal.
There was a problem hiding this comment.
This is what I asked about in #48121 (comment).
With VNet + per-session MFA it's possible to issue two MFA prompts at the same time. Not only that, issuing a prompt immediately starts the Webauthn goroutine which prompts for key touch on hardware level. IIRC it's technically possible to have two concurrent touch prompts on hardware level, but we cannot really race it on the UI level.
For example, say you have two MFA prompts, 1 and 2. They get sent to the hardware key and to the Electron app in order. The Electron app would show a modal for prompt 2, while (AFAIR) the first touch of the key would resolve prompt 1.
That being said, I think the comment above singleImportantModalSemaphore needs a more detailed explanation.
However, we should use this feature only when necessary (for example, for hardware key prompts while relogin is in progress). In any other case, we should open one modal at a time.
At the moment, it's clear to us why it's necessary to allow two important modals for hardware key prompts while relogin is in progress. But the comment doesn't explain that and the next person reading this will have no idea about it. This could use what Grzegorz said in the last sentence of his comment above.
We could also bring up the example I gave with VNet and per-session MFA as to why singleImportantModalSemaphore is still needed.
There was a problem hiding this comment.
Thanks for providing the case with VNet + per-session MFA. I wasn't aware of it. I updated the comment with more detailed explanation. I hope it's more clear now.
EDIT: there's actually a problem with relogin + per-session MFA, see #48813 (comment). We will need there changes anyway.
There was a problem hiding this comment.
I will move the discussion from #48813 (comment) here.
So it looks like we need to remove the importantModalSempahore from the MFA prompt too. Instead we should have a separate one, just for MFA.
After removing importantModalSempahore from the MFA prompt, we will only have it in the relogin and headless auth prompts. This will effectively only prevent from showing a headless auth dialog when relogin is opened and vice-versa. Relogin also has its own mutex so it's not possible to have more than one relogin at a time.
Given all of that into account, maybe we should get rid of importantModalSempahore and only have a mutex/sempahore per prompt kind? Something like reloginMu, mfaMu, headlessWaitSemaphore. This should save us from conflicts between prompts of the same type.
Theoretically, we could get rid of a semaphore for the headless auth, but I think it's a nice feature that we show the prompts in the order of creation.
There was a problem hiding this comment.
Single mutex per prompt kind sounds reasonable for now, I guess.
There was a problem hiding this comment.
Done.
I still left waitSemaphore for headless auth. I remember that we wanted the user to see that a new headless request was received by having a slight delay between them. I'm not sure if this still makes sense, but I thought I'd leave it as is.
|
|
||
| /** | ||
| * Renders an important dialog. | ||
| * Each dialog must implement a `hidden` prop which visually hides the dialog |
There was a problem hiding this comment.
I wonder if it wouldn't make sense to treat all dialogs uniformly, as in require all of them to use the hidden prop. This avoids having separate rules for regular or important dialog.
Think from the perspective of someone who doesn't contribute to Connect that often and wants to add a new modal. Asking them to understand the difference between having to add a hidden dialog or not having to do so is a big ask. They might not remember to do so, which creates a bug.
If all modals had hidden, we could in theory hide the regular modal if an important modal is shown. It should also be possible to somehow enforce this on the type level.
Does that sound reasonable? Would it be something that doesn't require too much work?
There was a problem hiding this comment.
Yeaaah, we could just add have hidden everywhere. While I'm not sure if currently we can enforce it by types, I believe it's less likely that someone will introduce a bug by omitting hidden.
If all modals had hidden, we could in theory hide the regular modal if an important modal is shown.
I did it too. Although sometimes you can see a blink of a regular dialog between important dialogs, overall it seems more "fluent" than with two layers of dialogs.
lib/teleterm/daemon/daemon.go
Outdated
| // | ||
| // We use a semaphore instead of a mutex in order to cancel important modals that | ||
| // are no longer relevant before acquisition. | ||
| // | ||
| // We use a waitSemaphore in order to make sure there is a clear transition between modals. | ||
| importantModalSemaphore *waitSemaphore | ||
| singleImportantModalSemaphore *waitSemaphore |
There was a problem hiding this comment.
This is what I asked about in #48121 (comment).
With VNet + per-session MFA it's possible to issue two MFA prompts at the same time. Not only that, issuing a prompt immediately starts the Webauthn goroutine which prompts for key touch on hardware level. IIRC it's technically possible to have two concurrent touch prompts on hardware level, but we cannot really race it on the UI level.
For example, say you have two MFA prompts, 1 and 2. They get sent to the hardware key and to the Electron app in order. The Electron app would show a modal for prompt 2, while (AFAIR) the first touch of the key would resolve prompt 1.
That being said, I think the comment above singleImportantModalSemaphore needs a more detailed explanation.
However, we should use this feature only when necessary (for example, for hardware key prompts while relogin is in progress). In any other case, we should open one modal at a time.
At the moment, it's clear to us why it's necessary to allow two important modals for hardware key prompts while relogin is in progress. But the comment doesn't explain that and the next person reading this will have no idea about it. This could use what Grzegorz said in the last sentence of his comment above.
We could also bring up the example I gave with VNet and per-session MFA as to why singleImportantModalSemaphore is still needed.
lib/teleterm/daemon/mfaprompt.go
Outdated
There was a problem hiding this comment.
Something is not right. After the cert expires and I submit my credentials through the form, the hardware key starts to blink, but I don't see the prompt in the app. I can touch the key and then continue with the PIV prompts. But I don't see the original MFA prompt for the key touch. I do see the MFA prompt during regular login in an OSS cluster.
This is with require_session_mfa: hardware_key_touch_and_pin
piv-modals.mov
There was a problem hiding this comment.
Guess what, the MFA modal can't be opened because relogin is in progress...
This works during normal login, because in this case the login modal is a regular modal, when relogin is initiated from tshd, it is an important modal).
I believe the bug comes from #47153 (cc @Joerger).
I didn't catch it because I'm mostly using passwordless :(
So it looks like we need to remove the importantModalSempahore from the MFA prompt too. Instead we should have a separate one, just for MFA. WDYT?
There was a problem hiding this comment.
Hmm I'm still getting this actually, let me take a look to see if I can understand the cause.
There was a problem hiding this comment.
Wait I think I just forgot to pull...
…n` to all of them, hide regular modal when important one is visible
…ive each prompt its own mutex/semaphore
There was a problem hiding this comment.
Ok, I tried it along with a couple of different login and relogin methods and I think this is going to work. At least until we add more modals that can be opened from tshd.
Though it all seems just a bit too fragile and I can't escape the feeling that there must be a better way to organize tshd <=> Electron communication in those cases to avoid this class of problems.
| // We use a waitSemaphore to make sure there is a clear transition between modals. | ||
| // We allow a single headless auth prompt at a time. | ||
| headlessAuthSemaphore *waitSemaphore | ||
| // mfaMu prevents concurrent MFA prompts. These can happen when using VNet with per-session MFA. | ||
| // Issuing an MFA prompt starts the Webauthn goroutine which prompts for key touch on hardware level. | ||
| // Webauthn does not support concurrent prompts, so without this semaphore, one of the prompts would fail immediately. |
There was a problem hiding this comment.
I was going to ask if you have a proof for this, but I just quickly tested this by trying to log in through tsh and Connect at the same time and it seems to be true.
I did try this a looong time ago and back then it seemed to work, as in you'd submit two requests for touch and first touch would resolve the first request and the second touch would resolve the second one. But I think at the time I was still using U2F without knowing this.
Yeah, I agree. I think the main problem is that we don't really have a centralized place on the Electron side that manages these prompts. As I said in another comment, tshd should probably only send events, and Electron should be responsible for organizing them into dialogs, ensure that there are smooth transitions, decide what can be shown at the moment. |
…48813) * Convert `DialogConfirmation` to TS * Allow `DialogConfirmation` and `Modal` to be hidden using CSS while closed * Allow hiding all dialogs that are displayed as important * Allow displaying multiple important dialogs, separate regular and important dialogs and get rid of `DialogNone` * Pass `hidden` prop to important dialogs * Rename `importantModalSemaphore` to `singleImportantModalSemaphore` * Remove semaphores from hardware key prompts * `keepMounted` -> `keepInDOMAfterClose` * Remove the explicit value from `keepInDOMAfterClose` * Revert splitting dialogs into regular and important ones, pass `hidden` to all of them, hide regular modal when important one is visible * Use random id as a modal key * Improve `singleImportantModalSemaphore` comment * Improve `NewHardwareKeyPromptConstructor` comment * Do not acquire important modal semaphore in MFA prompt and relogin, give each prompt its own mutex/semaphore (cherry picked from commit 7428128)
…48813) * Convert `DialogConfirmation` to TS * Allow `DialogConfirmation` and `Modal` to be hidden using CSS while closed * Allow hiding all dialogs that are displayed as important * Allow displaying multiple important dialogs, separate regular and important dialogs and get rid of `DialogNone` * Pass `hidden` prop to important dialogs * Rename `importantModalSemaphore` to `singleImportantModalSemaphore` * Remove semaphores from hardware key prompts * `keepMounted` -> `keepInDOMAfterClose` * Remove the explicit value from `keepInDOMAfterClose` * Revert splitting dialogs into regular and important ones, pass `hidden` to all of them, hide regular modal when important one is visible * Use random id as a modal key * Improve `singleImportantModalSemaphore` comment * Improve `NewHardwareKeyPromptConstructor` comment * Do not acquire important modal semaphore in MFA prompt and relogin, give each prompt its own mutex/semaphore (cherry picked from commit 7428128)
…ess (#49702) * Allow displaying hardware keys prompts when relogin is in progress (#48813) * Convert `DialogConfirmation` to TS * Allow `DialogConfirmation` and `Modal` to be hidden using CSS while closed * Allow hiding all dialogs that are displayed as important * Allow displaying multiple important dialogs, separate regular and important dialogs and get rid of `DialogNone` * Pass `hidden` prop to important dialogs * Rename `importantModalSemaphore` to `singleImportantModalSemaphore` * Remove semaphores from hardware key prompts * `keepMounted` -> `keepInDOMAfterClose` * Remove the explicit value from `keepInDOMAfterClose` * Revert splitting dialogs into regular and important ones, pass `hidden` to all of them, hide regular modal when important one is visible * Use random id as a modal key * Improve `singleImportantModalSemaphore` comment * Improve `NewHardwareKeyPromptConstructor` comment * Do not acquire important modal semaphore in MFA prompt and relogin, give each prompt its own mutex/semaphore (cherry picked from commit 7428128) * Re-add `keepInDOMAfterClose` to `Modal.jsx` * Fix a mistake causing document reopen dialog to be always hidden (#49061) (cherry picked from commit 7b520cc) * Change `dialogCss` type to `any` since there is no `StyleFunction` available in `styled-components` in branch/v16
Fixes #48121
To solve the problem of not being able to prompt for the hardware key when an important dialog for relogin is already open, I enabled support for multiple important modals on the Electron side. Dialogs are hidden using CSS to preserve their state.
This allows unblocking the first important modal in the second important modal and then coming back to the first one (sounds like a line from Inception).
The main problem with hiding the dialogs is that I don't have access to parent elements for them. This means that I can't easily hide them from the
ModalsHostlevel. Instead, each important dialog must handle its own hiding (by passing a flag toDialogConfirmation). To make this easier, I separated regular modals from important ones and added a JSDoc comment.When it comes to
importantModalSemaphore, I renamed it tosingleImportantModalSemaphoreto signal that it allows only one modal at a time. Hardware keys prompts don't use it.Note to reviewers: best to review commit by commit and this is needed for 17.0.