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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 13, 2025
Merged

Fix Per-session MFA for desktops #50793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
596f27b
Add sendChallengeResponse implementation for desktop sessions.
Joerger Jan 6, 2025
d6b0c75
Rename useMfaTty to useMfaEmitter.
Joerger Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import AuthnDialog from 'teleport/components/AuthnDialog';
import Document from 'teleport/Console/Document';
import { Terminal, TerminalRef } from 'teleport/Console/DocumentSsh/Terminal';
import * as stores from 'teleport/Console/stores/types';
import { useMfaTty } from 'teleport/lib/useMfa';
import { useMfaEmitter } from 'teleport/lib/useMfa';

import { ConnectDialog } from './ConnectDialog';
import { useDbSession } from './useDbSession';
Expand All @@ -37,7 +37,7 @@ type Props = {
export function DocumentDb({ doc, visible }: Props) {
const terminalRef = useRef<TerminalRef>();
const { tty, status, closeDocument, sendDbConnectData } = useDbSession(doc);
const mfa = useMfaTty(tty);
const mfa = useMfaEmitter(tty);
useEffect(() => {
// when switching tabs or closing tabs, focus on visible terminal
terminalRef.current?.focus();
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import Document from 'teleport/Console/Document';
import useKubeExecSession from 'teleport/Console/DocumentKubeExec/useKubeExecSession';
import { Terminal, TerminalRef } from 'teleport/Console/DocumentSsh/Terminal';
import * as stores from 'teleport/Console/stores/types';
import { useMfaTty } from 'teleport/lib/useMfa';
import { useMfaEmitter } from 'teleport/lib/useMfa';

import KubeExecData from './KubeExecDataDialog';

Expand All @@ -38,7 +38,7 @@ export default function DocumentKubeExec({ doc, visible }: Props) {
const terminalRef = useRef<TerminalRef>();
const { tty, status, closeDocument, sendKubeExecData } =
useKubeExecSession(doc);
const mfa = useMfaTty(tty);
const mfa = useMfaEmitter(tty);
useEffect(() => {
// when switching tabs or closing tabs, focus on visible terminal
terminalRef.current?.focus();
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { TerminalSearch } from 'shared/components/TerminalSearch';

import AuthnDialog from 'teleport/components/AuthnDialog';
import * as stores from 'teleport/Console/stores';
import { useMfa, useMfaTty } from 'teleport/lib/useMfa';
import { useMfa, useMfaEmitter } from 'teleport/lib/useMfa';
import { MfaChallengeScope } from 'teleport/services/auth/auth';

import { useConsoleContext } from '../consoleContextProvider';
Expand All @@ -54,7 +54,7 @@ function DocumentSsh({ doc, visible }: PropTypes) {
const { tty, status, closeDocument, session } = useSshSession(doc);
const [showSearch, setShowSearch] = useState(false);

const ttyMfa = useMfaTty(tty);
const ttyMfa = useMfaEmitter(tty);
const ftMfa = useMfa({
isMfaRequired: ttyMfa.required,
req: {
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleport/src/DesktopSession/useDesktopSession.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import useAttempt from 'shared/hooks/useAttemptNext';

import type { UrlDesktopParams } from 'teleport/config';
import { ButtonState } from 'teleport/lib/tdp';
import { useMfaTty } from 'teleport/lib/useMfa';
import { useMfaEmitter } from 'teleport/lib/useMfa';
import desktopService from 'teleport/services/desktops';
import userService from 'teleport/services/user';

Expand Down Expand Up @@ -129,7 +129,7 @@ export default function useDesktopSession() {
});
const tdpClient = clientCanvasProps.tdpClient;

const mfa = useMfaTty(tdpClient);
const mfa = useMfaEmitter(tdpClient);

const onShareDirectory = () => {
try {
Expand Down
9 changes: 9 additions & 0 deletions web/packages/teleport/src/lib/tdp/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import init, {
import { AuthenticatedWebSocket } from 'teleport/lib/AuthenticatedWebSocket';
import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender';
import { TermEvent, WebsocketCloseCode } from 'teleport/lib/term/enums';
import { MfaChallengeResponse } from 'teleport/services/mfa';

import Codec, {
FileType,
Expand Down Expand Up @@ -619,6 +620,14 @@ export default class Client extends EventEmitterMfaSender {
this.send(this.codec.encodeClipboardData(clipboardData));
}

sendChallengeResponse(data: MfaChallengeResponse) {
const msg = this.codec.encodeMfaJson({
mfaType: 'n',
jsonString: JSON.stringify(data),
});
this.send(msg);
}

addSharedDirectory(sharedDirectory: FileSystemDirectoryHandle) {
try {
this.sdManager.add(sharedDirectory);
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/lib/useMfa.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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

const [mfaRequired, setMfaRequired] = useState(false);

const mfa = useMfa({ isMfaRequired: mfaRequired });
Expand Down
Loading