[Browser MFA] Add Browser MFA to Connect#64887
Conversation
a862d28 to
c7db1e2
Compare
There was a problem hiding this comment.
💡 Codex Review
teleport/lib/teleterm/daemon/mfaprompt.go
Lines 128 to 135 in c7db1e2
When Connect receives both WebauthnChallenge and BrowserMFAChallenge for the same user—which is the normal browser-passkey case in lib/auth/auth.go—this code still launches the native WebAuthn goroutine immediately. That means users who choose Browser MFA because their credential only exists in the browser (for example Touch ID/password-manager passkeys) can still lose the whole ceremony to a native WebAuthn failure: HandleMFAPromptGoroutines treats wancli.ErrUsingNonRegisteredDevice as terminal in lib/client/mfa/prompt.go:114-116, so the browser flow never gets a chance to succeed. In practice, the new Browser MFA option stays broken for exactly the users it is meant to unblock.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
7dabf4b to
c9a119c
Compare
|
@danielashare can you ping me after this one is rebased and ready for re-review? |
c7db1e2 to
170cf79
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170cf79940
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@zmb3 rebased and ready for review, thanks |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 523b2234b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| go func() { | ||
| defer wg.Done() | ||
|
|
||
| resp, err := p.promptMfa(ctx, chal) |
There was a problem hiding this comment.
Pass Browser-only challenge to Browser MFA ceremony
This Browser MFA branch forwards the full chal into promptMfa, but MFACeremony.Run gives priority to SSOChallenge when both are present. In environments where a challenge includes both SSO and Browser MFA options, selecting Browser MFA in Connect will still execute the SSO path (expecting an SSO token) and can fail instead of returning a browser WebAuthn response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We need to have p.promptSSO and p.promptBrowser separate, like the CLI prompt
| if promptBrowserMfa { | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
Avoid deadlock when adding Browser MFA goroutine
Introducing this additional goroutine can increase concurrent MFA responders to four (app prompt, WebAuthn, SSO, Browser), but HandleMFAPromptGoroutines only has a 2-slot response buffer and returns after the first success before wg.Wait(). In mixed-method challenges (notably when WebAuthn, SSO, and Browser are all available), one sender can block forever on respC <- ..., causing the wait to hang and the login flow to stall.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The comments that Codex left seem valid, no?
There was a problem hiding this comment.
Yeah, this looks like a pre-existing issue. Opened a separate PR to address it. If you want to just bump the buffer up to 4 for the sake of this PR, that seems sufficient.
| if promptBrowserMfa { | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
The comments that Codex left seem valid, no?
There was a problem hiding this comment.
Is there any way we can test it? Grzegorz added e2e tests for headless auth, perhaps something similar could be utilized here? Seeing how browser MFA utilizes both Connect and the Web UI too.
There was a problem hiding this comment.
An e2e test could be a follow-up to this PR (so that it's not blocked on this), but it'd definitely be great to have it.
| go func() { | ||
| defer wg.Done() | ||
|
|
||
| resp, err := p.promptMfa(ctx, chal) |
There was a problem hiding this comment.
We need to have p.promptSSO and p.promptBrowser separate, like the CLI prompt
| if promptBrowserMfa { | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
Yeah, this looks like a pre-existing issue. Opened a separate PR to address it. If you want to just bump the buffer up to 4 for the sake of this PR, that seems sufficient.
f9fa433 to
bdcba07
Compare
523b223 to
5e60f4e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd1a673600
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e16dcca98
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Fire Browser MFA goroutine. | ||
| if promptBrowserMfa { | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
Avoid running SSO and Browser MFA ceremonies concurrently
When a challenge includes both SSOChallenge and BrowserMFAChallenge, this path now starts both callback-based ceremonies in parallel against the same c.cfg.MFACeremony instance. Both Run calls ultimately wait on the same redirector callback channel, so whichever goroutine reads the single callback first may treat it as the wrong response type and fail, while the intended goroutine then waits until timeout. In that mixed-method scenario, users can see nondeterministic login stalls/failures despite completing MFA successfully in the browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8a3aa84da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ravicious
left a comment
There was a problem hiding this comment.
LGTM once Codex feedback is evaluated.
There was a problem hiding this comment.
An e2e test could be a follow-up to this PR (so that it's not blocked on this), but it'd definitely be great to have it.
| // Fire Browser MFA goroutine. | ||
| if promptBrowserMfa { | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
One edge case of starting these in parallel is that if I actually have a hardware key plugged in but for whatever reason I want to use browser MFA (like now when I'm testing the implementation 😏), then Connect makes my YubiKey blink. So when I select browser MFA and I want to auth in browser with my YubiKey, I cannot use it there. I have to cancel the dialog in the browser and then try again for it to effectively "steal" YubiKey from Connect.
The current implementation should work fine for its intended purpose, that is when the user has no MFA available outside of the browser. I'm just noting it down in case we need a more elaborate implementation in the future, e.g. selecting the browser MFA option shuts down the goroutine that prompts hardware keys.
There was a problem hiding this comment.
This is outside of scope of this PR, but while testing I've noticed that neither tsh nor Connect react to hardware key insertion. Connect even says "Insert and press your key". But if there was no key at the time when the prompt was shown, inserting a key is going to have no effect. I'll create an issue for this on Monday.
5ff3627 to
a12fb73
Compare
689863c to
5b19ccc
Compare
|
@danielashare See the table below for backport results.
|
[Browser MFA] Add protobuf and config (#63831) [Browser MFA] Add proto for Browser MFA feature (#64048) [Browser MFA] Add CompleteBrowserMFAChallenge gRPC (#63873) [Browser MFA] Rename browser mfa config name (#64980) [Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (#63945) [Browser MFA] Add Browser MFA to challenge request flow (#63936) [Browser MFA] Add initial requests for browser MFA process to client tools (#64301) [Browser MFA] Add tsh callback handling for webauthn response (#64461) [Browser MFA] Add Browser MFA to presence checks (#65052) [Browser MFA] Add browser MFA path to MFA finish flow (#64523) [Browser MFA] Add Browser MFA to Connect (#64887) [Browser MFA] Add Browser MFA UI (#64692) [Browser MFA] Fix formatting in moderated sessions (#65236) [Browser MFA] Add Browser MFA ceremony tests
[Browser MFA] Add protobuf and config (gravitational#63831) [Browser MFA] Add proto for Browser MFA feature (gravitational#64048) [Browser MFA] Add CompleteBrowserMFAChallenge gRPC (gravitational#63873) [Browser MFA] Rename browser mfa config name (gravitational#64980) [Browser MFA] Add BrowserMFARequestID to CreateAuthenticateChallenge (gravitational#63945) [Browser MFA] Add Browser MFA to challenge request flow (gravitational#63936) [Browser MFA] Add initial requests for browser MFA process to client tools (gravitational#64301) [Browser MFA] Add tsh callback handling for webauthn response (gravitational#64461) [Browser MFA] Add Browser MFA to presence checks (gravitational#65052) [Browser MFA] Add browser MFA path to MFA finish flow (gravitational#64523) [Browser MFA] Add Browser MFA to Connect (gravitational#64887) [Browser MFA] Add Browser MFA UI (gravitational#64692) [Browser MFA] Fix formatting in moderated sessions (gravitational#65236) [Browser MFA] Add Browser MFA ceremony tests
This PR adds Browser MFA support to Connect and renames more SSOMFA* specific types/funcs to be more generic now they're used by Browser and SSO MFA. The RFD for this feature can be found here. Tracking issue #63987.
Manual Test Plan
Test Environment
Running Teleport locally from a PoC branch (danielashare/tsh-browser-mfa-sso) because this branch doesn't have the web UI changes. SSO tested with keycloak running locally.
Test Cases