Skip to content
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.

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

Fix Per-session MFA for desktops #50793

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Jan 6, 2025

Restore EventEmitterMfaSender implementation for desktop access mistakenly removed in #49794.

This will be backported in #50529

Closes #50557

@@ -172,7 +172,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState {
};
}

export function useMfaTty(emitterSender: EventEmitterMfaSender): MfaState {
export function useMfaEmitter(emitterSender: EventEmitterMfaSender): MfaState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something still feels off about this.

Are you sure desktop code should be calling this? AFAIK, the TermEvent enum is a bunch of events that our web SSH console depends on. I wouldn't expect the desktop player to depend on any term events since it is not a terminal.

If your intent is some refactoring so that SSH and desktop MFA share more code I think that's a fine goal, but I think the abstraction is probably not quite right. I don't think desktop code should depend on TermEvent.

Copy link
Contributor Author

@Joerger Joerger Jan 7, 2025

Choose a reason for hiding this comment

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

I didn't make any significant refactor to the Desktop session MFA code, it was already using useMfa, which took Tty or EventEmitterMfaSender as an argument (tdpClient implements EventEmitterMfaSender). I renamed it to useMfaTty or useMfaEmitter so that I could make useMfa more generally usable.

EventEmitterMfaSender just represents the websocket used in WebUI sessions to send MFA challenges from the proxy and retrieve a challenge response from the client. Tty extends that for Terminal purposes. I can take a stab at renaming to make this more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avatus Can you help provide any more clarification? It is a dense section of the code base.

Copy link
Contributor

@avatus avatus Jan 7, 2025

Choose a reason for hiding this comment

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

I think this is a result of poor naming/abstractions from the past. desktop and ssh docs both implemented the old EventWebAuthnEmitter or whatever it was called, which had a single non-implemented sendWebauthn method that tty and tdpClient had to implement themselves. And we've since updated it from useWebauthn to useMfa but that weird abstraction layer should probably just go imo, we don't need it. Outside of this PR probably

@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Jan 7, 2025
Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

MFA works for me locally on windows desktop now

@Joerger
Copy link
Contributor Author

Joerger commented Jan 8, 2025

@zmb3 @gzdunek @ryanclark Friendly reminder to review, this should go into v17.2

@Joerger Joerger requested a review from zmb3 January 8, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per-session MFA for desktops is broken
3 participants