From 2e03de57964678753fe2ba8bd9db6e53a2e6c141 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Wed, 5 Mar 2025 13:09:31 +0100 Subject: [PATCH 1/6] Do not try to send an empty MFA challenge in a desktop session --- lib/web/desktop.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 124035bf3c58f..ddbac8691dd85 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -385,6 +385,9 @@ func (h *Handler) performSessionMFACeremony( return mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { codec := tdpMFACodec{} + if chal.WebauthnChallenge == nil { + return nil, trace.AccessDenied("Desktop Access requires WebAuthn MFA, please register a WebAuthn device to connect") + } // Send the challenge over the socket. msg, err := codec.Encode( &client.MFAAuthenticateChallenge{ From bbfc95cf96ede4eafc2338080db6048de31d85c0 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Wed, 5 Mar 2025 13:12:12 +0100 Subject: [PATCH 2/6] Prevent a runtime error when neither a challenge nor a request is present --- web/packages/teleport/src/lib/useMfa.test.tsx | 9 +++++++++ web/packages/teleport/src/lib/useMfa.ts | 10 +++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index 38e8aafdfe139..303046e5b2673 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -118,6 +118,15 @@ describe('useMfa', () => { await waitFor(() => expect(mfa.current.mfaRequired).toEqual(false)); }); + test('returns empty challenge response when mfa challenge and request are absent', async () => { + const { result: mfa } = renderHook(() => useMfa()); + + const resp = await act(() => mfa.current.getChallengeResponse()); + + expect(resp).toBeUndefined(); + expect(mfa.current.mfaRequired).toEqual(false); + }); + test('adaptable mfa requirement', async () => { jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge); jest diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 73fcffb947cca..b1a99278d960e 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -85,12 +85,12 @@ export function useMfa(props?: MfaProps): MfaState { // Caller didn't pass a challenge and the mfa required is true or unknown. if (!challenge) { - let req = props.req; + if (props?.req) { + // We already know MFA is required, skip the extra check. + if (mfaRequired === true) props.req.isMfaRequiredRequest = null; - // We already know MFA is required, skip the extra check. - if (mfaRequired === true) req.isMfaRequiredRequest = null; - - challenge = await auth.getMfaChallenge(props.req); + challenge = await auth.getMfaChallenge(props.req); + } // An empty challenge means either mfa is not required, or the user has no mfa devices. if (!challenge) { From 1c6b8b83e0c898752cf3dc2d45293f1032f06a54 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Wed, 5 Mar 2025 13:30:34 +0100 Subject: [PATCH 3/6] Do not overwrite MFA errors with a custom message, show an error when the MFA dialog is closed --- .../src/DesktopSession/DesktopSession.tsx | 44 ++++++++++--------- .../components/AuthnDialog/AuthnDialog.tsx | 11 ++--- web/packages/teleport/src/lib/useMfa.ts | 11 +++++ 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index c14c4f35a77e5..b8391575e5e07 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -26,6 +26,7 @@ import Dialog, { DialogHeader, DialogTitle, } from 'design/Dialog'; +import { Attempt as AsyncAttempt } from 'shared/hooks/useAsync'; import { Attempt } from 'shared/hooks/useAttemptNext'; import AuthnDialog from 'teleport/components/AuthnDialog'; @@ -34,7 +35,7 @@ import { TdpClientCanvasRef } from 'teleport/components/TdpClientCanvas/TdpClien import { TdpClientEvent } from 'teleport/lib/tdp'; import { BitmapFrame } from 'teleport/lib/tdp/client'; import { ClientScreenSpec, PngFrame } from 'teleport/lib/tdp/codec'; -import type { MfaState } from 'teleport/lib/useMfa'; +import { MfaState, shouldShowMfaPrompt } from 'teleport/lib/useMfa'; import TopBar from './TopBar'; import useDesktopSession, { @@ -320,7 +321,7 @@ export function DesktopSession(props: State) { {screenState.screen === 'anotherSessionActive' && ( )} - {screenState.screen === 'mfa' && } + {screenState.screen === 'mfa' && } {screenState.screen === 'alert dialog' && ( )} @@ -345,17 +346,6 @@ export function DesktopSession(props: State) { ); } -const MfaDialog = ({ mfa }: { mfa: MfaState }) => { - return ( - - ); -}; - const AlertDialog = ({ screenState }: { screenState: ScreenState }) => ( ({ width: '484px' })} open={true}> @@ -363,9 +353,13 @@ const AlertDialog = ({ screenState }: { screenState: ScreenState }) => ( <> - {screenState.alertMessage || invalidStateMessage}} - /> + {typeof screenState.alertMessage === 'object' ? ( + + {screenState.alertMessage.title} + + ) : ( + {screenState.alertMessage} + )} Refresh the page to reconnect. @@ -439,7 +433,7 @@ const nextScreenState = ( tdpConnection: Attempt, wsConnection: WebsocketAttempt, showAnotherSessionActiveDialog: boolean, - webauthn: MfaState + mfa: MfaState ): ScreenState => { // We always want to show the user the first alert that caused the session to fail/end, // so if we're already showing an alert, don't change the screen. @@ -456,11 +450,12 @@ const nextScreenState = ( // Otherwise, calculate a new screen state. const showAnotherSessionActive = showAnotherSessionActiveDialog; - const showMfa = webauthn.challenge; + const showMfa = shouldShowMfaPrompt(mfa); const showAlert = fetchAttempt.status === 'failed' || // Fetch attempt failed tdpConnection.status === 'failed' || // TDP connection failed tdpConnection.status === '' || // TDP connection ended gracefully server-side + mfa.attempt.status === 'error' || // MFA was canceled wsConnection.status === 'closed'; // Websocket closed (could mean client side graceful close or unexpected close, the message will tell us which) const atLeastOneAttemptProcessing = @@ -495,6 +490,7 @@ const nextScreenState = ( tdpConnection, wsConnection, showAnotherSessionActiveDialog, + mfa.attempt, prevState ), canvasState: { shouldConnect: false, shouldDisplay: false }, @@ -525,9 +521,17 @@ const calculateAlertMessage = ( tdpConnection: Attempt, wsConnection: WebsocketAttempt, showAnotherSessionActiveDialog: boolean, + mfaAttempt: AsyncAttempt, prevState: ScreenState -): string => { +) => { let message = ''; + // Errors, except for dialog cancellations, are handled within the MFA dialog. + if (mfaAttempt.status === 'error') { + return { + title: 'This session requires multi factor authentication', + message: mfaAttempt.statusText, + }; + } if (fetchAttempt.status === 'failed') { message = fetchAttempt.statusText || 'fetch attempt failed'; } else if (tdpConnection.status === 'failed') { @@ -560,7 +564,7 @@ type ScreenState = { | 'processing' | 'canvas'; - alertMessage?: string; + alertMessage?: string | { title: string; message: string }; canvasState: { shouldConnect: boolean; shouldDisplay: boolean; diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index e8a53eb3365a4..740f3a995820e 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -23,12 +23,11 @@ import { Cross, FingerprintSimple } from 'design/Icon'; import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; -import { MfaCanceledError, MfaState } from 'teleport/lib/useMfa'; +import { MfaState, shouldShowMfaPrompt } from 'teleport/lib/useMfa'; import { MFA_OPTION_TOTP } from 'teleport/services/mfa'; export type Props = { mfaState: MfaState; - replaceErrorText?: string; // onClose is an optional function to perform additional operations // upon closing the dialog. e.g. close a shell session onClose?: () => void; @@ -36,13 +35,9 @@ export type Props = { export default function AuthnDialog({ mfaState: { options, challenge, submit, attempt, cancelAttempt }, - replaceErrorText, onClose = () => {}, }: Props) { - const showError = - attempt.status === 'error' && !(attempt.error instanceof MfaCanceledError); - - if (!challenge && !showError) return; + if (!shouldShowMfaPrompt({ challenge, attempt })) return; // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. const onlyTotpAvailable = @@ -72,7 +67,7 @@ export default function AuthnDialog({ )} {attempt.status === 'error' && ( - {replaceErrorText || attempt.statusText} + {attempt.statusText} )} diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index b1a99278d960e..4e05795d9dd31 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -218,6 +218,17 @@ export type MfaState = { cancelAttempt: () => void; }; +/** Indicates if an MFA dialog should be visible. */ +export function shouldShowMfaPrompt( + mfa: Pick +): boolean { + return ( + !!mfa.challenge || + (mfa.attempt.status === 'error' && + !(mfa.attempt.error instanceof MfaCanceledError)) + ); +} + // used for testing export function makeDefaultMfaState(): MfaState { return { From 877b72f6ce63a972de0cbc85700f0125b72c1fd3 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Wed, 5 Mar 2025 15:49:55 +0100 Subject: [PATCH 4/6] Fix a test so that either a request or a challenge is passed --- web/packages/teleport/src/lib/useMfa.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index 303046e5b2673..afe86ca77aae4 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -184,7 +184,7 @@ describe('useMfa', () => { throw err; }); - const { result: mfa } = renderHook(() => useMfa({})); + const { result: mfa } = renderHook(() => useMfa({ req: mockChallengeReq })); await act(async () => { await expect(mfa.current.getChallengeResponse()).rejects.toThrow(err); From 9e1ccf2c33e9e4f13692b82696ca723f46122328 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 6 Mar 2025 19:35:33 +0100 Subject: [PATCH 5/6] Desktop Access -> Desktop access --- lib/web/desktop.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/desktop.go b/lib/web/desktop.go index ddbac8691dd85..2dbd576cef5f8 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -386,7 +386,7 @@ func (h *Handler) performSessionMFACeremony( codec := tdpMFACodec{} if chal.WebauthnChallenge == nil { - return nil, trace.AccessDenied("Desktop Access requires WebAuthn MFA, please register a WebAuthn device to connect") + return nil, trace.AccessDenied("Desktop access requires WebAuthn MFA, please register a WebAuthn device to connect") } // Send the challenge over the socket. msg, err := codec.Encode( From a726a533a5c8dc26d547847dcbc15045984cba3f Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 6 Mar 2025 19:35:40 +0100 Subject: [PATCH 6/6] Reduce nesting --- web/packages/teleport/src/lib/useMfa.ts | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 4e05795d9dd31..9e14e40c319dd 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -84,23 +84,20 @@ export function useMfa(props?: MfaProps): MfaState { } // Caller didn't pass a challenge and the mfa required is true or unknown. - if (!challenge) { - if (props?.req) { - // We already know MFA is required, skip the extra check. - if (mfaRequired === true) props.req.isMfaRequiredRequest = null; - - challenge = await auth.getMfaChallenge(props.req); - } - - // An empty challenge means either mfa is not required, or the user has no mfa devices. - if (!challenge) { - setMfaRequired(false); - return; - } + if (!challenge && props?.req) { + // We already know MFA is required, skip the extra check. + if (mfaRequired === true) props.req.isMfaRequiredRequest = null; + challenge = await auth.getMfaChallenge(props.req); + } - setMfaRequired(true); + // An empty challenge means either mfa is not required, or the user has no mfa devices. + if (!challenge) { + setMfaRequired(false); + return; } + setMfaRequired(true); + // Prepare a new promise to collect the mfa response retrieved // through the submit function. let resolve: (value: MfaChallengeResponse) => void;