From e3537418c07069e50c0095b631fca300f4db7d13 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 21 Nov 2024 11:45:44 -0800 Subject: [PATCH 01/20] Include sso channel ID in web mfa challenges. --- lib/web/apiserver.go | 2 +- lib/web/mfa.go | 25 +++++++++++++++++++++---- lib/web/password.go | 2 +- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 597360e64df8a..3edae1da2db3e 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -2763,7 +2763,7 @@ func (h *Handler) mfaLoginBegin(w http.ResponseWriter, r *http.Request, p httpro return nil, trace.AccessDenied("invalid credentials") } - return makeAuthenticateChallenge(mfaChallenge), nil + return makeAuthenticateChallenge(mfaChallenge, "" /*channelID*/), nil } // mfaLoginFinish completes the MFA login ceremony, returning a new SSH diff --git a/lib/web/mfa.go b/lib/web/mfa.go index 485a4eff460bc..c53c66ff5d9d2 100644 --- a/lib/web/mfa.go +++ b/lib/web/mfa.go @@ -21,8 +21,10 @@ package web import ( "context" "net/http" + "net/url" "strings" + "github.com/google/uuid" "github.com/gravitational/trace" "github.com/julienschmidt/httprouter" @@ -201,6 +203,20 @@ func (h *Handler) createAuthenticateChallengeHandle(w http.ResponseWriter, r *ht allowReuse = mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES } + // Prepare an sso client redirect URL in case the user has an SSO MFA device. + ssoClientRedirectURL, err := url.Parse(sso.WebMFARedirect) + if err != nil { + return nil, trace.Wrap(err) + } + + // id is used by the front end to differentiate between separate ongoing SSO challenges. + id, err := uuid.NewRandom() + if err != nil { + return nil, trace.Wrap(err) + } + channelID := id.String() + ssoClientRedirectURL.RawQuery = url.Values{"channel_id": {channelID}}.Encode() + chal, err := clt.CreateAuthenticateChallenge(r.Context(), &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ ContextUser: &proto.ContextUser{}, @@ -211,13 +227,13 @@ func (h *Handler) createAuthenticateChallengeHandle(w http.ResponseWriter, r *ht AllowReuse: allowReuse, UserVerificationRequirement: req.UserVerificationRequirement, }, - SSOClientRedirectURL: sso.WebMFARedirect, + SSOClientRedirectURL: ssoClientRedirectURL.String(), }) if err != nil { return nil, trace.Wrap(err) } - return makeAuthenticateChallenge(chal), nil + return makeAuthenticateChallenge(chal, channelID), nil } // createAuthenticateChallengeWithTokenHandle creates and returns MFA authenticate challenges for the user defined in token. @@ -235,7 +251,7 @@ func (h *Handler) createAuthenticateChallengeWithTokenHandle(w http.ResponseWrit return nil, trace.Wrap(err) } - return makeAuthenticateChallenge(chal), nil + return makeAuthenticateChallenge(chal, "" /*channelID*/), nil } type createRegisterChallengeWithTokenRequest struct { @@ -581,7 +597,7 @@ func (h *Handler) checkMFARequired(ctx context.Context, req *isMFARequiredReques } // makeAuthenticateChallenge converts proto to JSON format. -func makeAuthenticateChallenge(protoChal *proto.MFAAuthenticateChallenge) *client.MFAAuthenticateChallenge { +func makeAuthenticateChallenge(protoChal *proto.MFAAuthenticateChallenge, ssoChannelID string) *client.MFAAuthenticateChallenge { chal := &client.MFAAuthenticateChallenge{ TOTPChallenge: protoChal.GetTOTP() != nil, } @@ -590,6 +606,7 @@ func makeAuthenticateChallenge(protoChal *proto.MFAAuthenticateChallenge) *clien } if protoChal.GetSSOChallenge() != nil { chal.SSOChallenge = client.SSOChallengeFromProto(protoChal.GetSSOChallenge()) + chal.SSOChallenge.ChannelID = ssoChannelID } return chal } diff --git a/lib/web/password.go b/lib/web/password.go index 6ae5923787d7e..824c8b00ecb5a 100644 --- a/lib/web/password.go +++ b/lib/web/password.go @@ -108,5 +108,5 @@ func (h *Handler) createAuthenticateChallengeWithPassword(w http.ResponseWriter, return nil, trace.Wrap(err) } - return makeAuthenticateChallenge(chal), nil + return makeAuthenticateChallenge(chal, "" /*channelID*/), nil } From c5c7d11de49b194b688d168f424984aa3e5a9d2b Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 21 Nov 2024 11:27:27 -0800 Subject: [PATCH 02/20] Handle SSO MFA challenges. --- .../wizards/AddAuthDeviceWizard.test.tsx | 60 +++++++++++++-- .../wizards/DeleteAuthDeviceWizard.test.tsx | 46 +++++++++-- web/packages/teleport/src/lib/useMfa.ts | 57 +++----------- .../teleport/src/services/auth/auth.ts | 76 +++++++++++++++++++ .../teleport/src/services/mfa/mfaOptions.ts | 4 +- .../teleport/src/services/mfa/types.ts | 6 +- 6 files changed, 187 insertions(+), 62 deletions(-) diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx index 96a2fc05a404f..b4fb5a0303fe2 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx @@ -23,7 +23,7 @@ import { userEvent, UserEvent } from '@testing-library/user-event'; import { ContextProvider } from 'teleport'; import auth from 'teleport/services/auth'; -import MfaService from 'teleport/services/mfa'; +import MfaService, { SsoChallenge } from 'teleport/services/mfa'; import TeleportContext from 'teleport/teleportContext'; import { AddAuthDeviceWizardStepProps } from './AddAuthDeviceWizard'; @@ -170,11 +170,16 @@ describe('flow without reauthentication', () => { }); describe('flow with reauthentication', () => { + const dummyMfaChallenge = { + totpChallenge: true, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + ssoChallenge: {} as SsoChallenge, + }; + beforeEach(() => { - jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({ - totpChallenge: true, - webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, - }); + jest + .spyOn(auth, 'getMfaChallenge') + .mockResolvedValueOnce(dummyMfaChallenge); jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest .spyOn(auth, 'createPrivilegeToken') @@ -194,6 +199,11 @@ describe('flow with reauthentication', () => { expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); await user.click(screen.getByRole('button', { name: 'Create a passkey' })); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'webauthn', + '' + ); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'passwordless', @@ -228,6 +238,46 @@ describe('flow with reauthentication', () => { expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); await user.click(screen.getByRole('button', { name: 'Create a passkey' })); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'totp', + '654987' + ); + expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ + tokenId: 'privilege-token', + deviceUsage: 'passwordless', + }); + + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); + await user.click(screen.getByRole('button', { name: 'Save the Passkey' })); + expect(ctx.mfaService.saveNewWebAuthnDevice).toHaveBeenCalledWith({ + credential: dummyCredential, + addRequest: { + deviceName: 'new-passkey', + deviceUsage: 'passwordless', + tokenId: 'privilege-token', + }, + }); + expect(onSuccess).toHaveBeenCalled(); + }); + + test('adds a passkey with SSO reauthentication', async () => { + render(); + + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); + }); + await user.click(screen.getByText('SSO')); + await user.click(screen.getByText('Verify my identity')); + + expect(screen.getByTestId('create-step')).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: 'Create a passkey' })); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'sso', + '' + ); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'passwordless', diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx index dd780c4f3996f..c4e77e1365df7 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx @@ -23,7 +23,7 @@ import { userEvent, UserEvent } from '@testing-library/user-event'; import TeleportContext from 'teleport/teleportContext'; import { ContextProvider } from 'teleport'; -import MfaService from 'teleport/services/mfa'; +import MfaService, { SsoChallenge } from 'teleport/services/mfa'; import auth from 'teleport/services/auth'; import { DeleteAuthDeviceWizardStepProps } from './DeleteAuthDeviceWizard'; @@ -36,15 +36,18 @@ let ctx: TeleportContext; let user: UserEvent; let onSuccess: jest.Mock; +const dummyMfaChallenge = { + totpChallenge: true, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + ssoChallenge: {} as SsoChallenge, +}; + beforeEach(() => { ctx = new TeleportContext(); user = userEvent.setup(); onSuccess = jest.fn(); - jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({ - totpChallenge: true, - webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, - }); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(dummyMfaChallenge); jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest .spyOn(auth, 'createPrivilegeToken') @@ -80,6 +83,11 @@ test('deletes a device with WebAuthn reauthentication', async () => { expect(screen.getByTestId('delete-step')).toBeInTheDocument(); await user.click(screen.getByRole('button', { name: 'Delete' })); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'webauthn', + '' + ); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( 'privilege-token', 'TouchID' @@ -100,6 +108,34 @@ test('deletes a device with OTP reauthentication', async () => { expect(screen.getByTestId('delete-step')).toBeInTheDocument(); await user.click(screen.getByRole('button', { name: 'Delete' })); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'totp', + '654987' + ); + expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( + 'privilege-token', + 'TouchID' + ); +}); + +test('deletes a device with SSO reauthentication', async () => { + render(); + + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); + }); + await user.click(screen.getByText('SSO')); + await user.click(screen.getByText('Verify my identity')); + + expect(screen.getByTestId('delete-step')).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: 'Delete' })); + + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + dummyMfaChallenge, + 'sso', + '' + ); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( 'privilege-token', 'TouchID' diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 664016790e002..29ab598d5af39 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -23,7 +23,7 @@ import { TermEvent } from 'teleport/lib/term/enums'; import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa'; import { MfaAuthenticateChallengeJson, - SSOChallenge, + SsoChallenge, } from 'teleport/services/mfa'; import auth from 'teleport/services/auth/auth'; @@ -32,7 +32,7 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { errorText: string; addMfaToScpUrls: boolean; webauthnPublicKey: PublicKeyCredentialRequestOptions; - ssoChallenge: SSOChallenge; + ssoChallenge: SsoChallenge; totpChallenge: boolean; }>({ addMfaToScpUrls: false, @@ -60,15 +60,7 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { return; } - // try to center the screen - const width = 1045; - const height = 550; - const left = (screen.width - width) / 2; - const top = (screen.height - height) / 2; - - // these params will open a tiny window. - const params = `width=${width},height=${height},left=${left},top=${top}`; - window.open(state.ssoChallenge.redirectUrl, '_blank', params); + auth.openSsoChallengeRedirect(state.ssoChallenge); } function onWebauthnAuthenticate() { @@ -103,26 +95,20 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { const waitForSsoChallengeResponse = useCallback( async ( - ssoChallenge: SSOChallenge, + ssoChallenge: SsoChallenge, abortSignal: AbortSignal ): Promise => { - const channel = new BroadcastChannel(ssoChallenge.channelId); - try { - const event = await waitForMessage(channel, abortSignal); - emitterSender.sendChallengeResponse({ - sso_response: { - requestId: ssoChallenge.requestId, - token: event.data.mfaToken, - }, - }); + const resp = await auth.waitForSsoChallengeResponse( + ssoChallenge, + abortSignal + ); + emitterSender.sendChallengeResponse(resp); clearChallenges(); } catch (error) { if (error.name !== 'AbortError') { throw error; } - } finally { - channel.close(); } }, [emitterSender] @@ -147,7 +133,6 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { })); if (ssoChallenge) { - ssoChallengeAbortController?.abort(); ssoChallengeAbortController = new AbortController(); void waitForSsoChallengeResponse( ssoChallenge, @@ -195,7 +180,7 @@ export type MfaState = { requested: boolean; addMfaToScpUrls: boolean; webauthnPublicKey: PublicKeyCredentialRequestOptions; - ssoChallenge: SSOChallenge; + ssoChallenge: SsoChallenge; }; // used for testing @@ -211,25 +196,3 @@ export function makeDefaultMfaState(): MfaState { ssoChallenge: null, }; } - -function waitForMessage( - channel: BroadcastChannel, - abortSignal: AbortSignal -): Promise { - return new Promise((resolve, reject) => { - // Create the event listener - function eventHandler(e: MessageEvent) { - // Remove the event listener after it triggers - channel.removeEventListener('message', eventHandler); - // Resolve the promise with the event object - resolve(e); - } - - // Add the event listener - channel.addEventListener('message', eventHandler); - abortSignal.onabort = e => { - channel.removeEventListener('message', eventHandler); - reject(e); - }; - }); -} diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index 3724f1dc8b056..6480e41931ff2 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -23,6 +23,7 @@ import { DeviceUsage, MfaAuthenticateChallenge, MfaChallengeResponse, + SsoChallenge, } from 'teleport/services/mfa'; import { CaptureEvent, userEventService } from 'teleport/services/userEvent'; @@ -289,6 +290,8 @@ const auth = { mfaType = 'totp'; } else if (challenge.webauthnPublicKey) { mfaType = 'webauthn'; + } else if (challenge.ssoChallenge) { + mfaType = 'sso'; } } @@ -296,6 +299,10 @@ const auth = { return auth.getWebAuthnChallengeResponse(challenge.webauthnPublicKey); } + if (mfaType === 'sso') { + return auth.getSsoChallengeResponse(challenge.ssoChallenge); + } + if (mfaType === 'totp') { return { totp_code: totpCode, @@ -333,6 +340,51 @@ const auth = { }); }, + // TODO(Joerger): Delete once no longer used by /e + async getSsoChallengeResponse( + challenge: SsoChallenge + ): Promise { + const abortController = new AbortController(); + + auth.openSsoChallengeRedirect(challenge, abortController); + return await auth.waitForSsoChallengeResponse( + challenge, + abortController.signal + ); + }, + + openSsoChallengeRedirect( + { redirectUrl }: SsoChallenge, + abortController?: AbortController + ) { + // try to center the screen + const width = 1045; + const height = 550; + const left = (screen.width - width) / 2; + const top = (screen.height - height) / 2; + + // these params will open a tiny window. + const params = `width=${width},height=${height},left=${left},top=${top}`; + const w = window.open(redirectUrl, '_blank', params); + + // If the redirect URL window is closed prematurely, abort. + w.onclose = abortController?.abort; + }, + + async waitForSsoChallengeResponse( + { channelId, requestId }: SsoChallenge, + abortSignal: AbortSignal + ): Promise { + const channel = new BroadcastChannel(channelId); + const msg = await waitForMessage(channel, abortSignal); + return { + sso_response: { + requestId, + token: msg.data.mfaToken, + }, + }; + }, + // TODO(Joerger): Delete once no longer used by /e createPrivilegeTokenWithWebauthn() { return auth @@ -430,6 +482,30 @@ function base64EncodeUnicode(str: string) { ); } +function waitForMessage( + channel: BroadcastChannel, + abortSignal: AbortSignal +): Promise { + return new Promise((resolve, reject) => { + // Create the event listener + function eventHandler(e: MessageEvent) { + // Remove the event listener after it triggers + channel.removeEventListener('message', eventHandler); + // Resolve the promise with the event object + resolve(e); + } + + // Add the event listener + channel.addEventListener('message', eventHandler); + + // Close the event listener early if aborted. + abortSignal.onabort = e => { + channel.removeEventListener('message', eventHandler); + reject(e); + }; + }); +} + export default auth; export type IsMfaRequiredRequest = diff --git a/web/packages/teleport/src/services/mfa/mfaOptions.ts b/web/packages/teleport/src/services/mfa/mfaOptions.ts index 96510d31e668f..283feb83eb71f 100644 --- a/web/packages/teleport/src/services/mfa/mfaOptions.ts +++ b/web/packages/teleport/src/services/mfa/mfaOptions.ts @@ -18,7 +18,7 @@ import { Auth2faType } from 'shared/services'; -import { DeviceType, MfaAuthenticateChallenge, SSOChallenge } from './types'; +import { DeviceType, MfaAuthenticateChallenge, SsoChallenge } from './types'; // returns mfa challenge options in order of preferences: WebAuthn > SSO > TOTP. export function getMfaChallengeOptions(mfaChallenge: MfaAuthenticateChallenge) { @@ -74,7 +74,7 @@ export const MFA_OPTION_SSO_DEFAULT: MfaOption = { label: 'SSO', }; -const getSsoMfaOption = (ssoChallenge: SSOChallenge): MfaOption => { +const getSsoMfaOption = (ssoChallenge: SsoChallenge): MfaOption => { return { value: 'sso', label: diff --git a/web/packages/teleport/src/services/mfa/types.ts b/web/packages/teleport/src/services/mfa/types.ts index 382d7831f82fe..f8c0787544d08 100644 --- a/web/packages/teleport/src/services/mfa/types.ts +++ b/web/packages/teleport/src/services/mfa/types.ts @@ -51,7 +51,7 @@ export type SaveNewHardwareDeviceRequest = { }; export type MfaAuthenticateChallengeJson = { - sso_challenge?: SSOChallenge; + sso_challenge?: SsoChallenge; totp_challenge?: boolean; webauthn_challenge?: { publicKey: PublicKeyCredentialRequestOptionsJSON; @@ -59,12 +59,12 @@ export type MfaAuthenticateChallengeJson = { }; export type MfaAuthenticateChallenge = { - ssoChallenge?: SSOChallenge; + ssoChallenge?: SsoChallenge; totpChallenge?: boolean; webauthnPublicKey?: PublicKeyCredentialRequestOptions; }; -export type SSOChallenge = { +export type SsoChallenge = { channelId: string; redirectUrl: string; requestId: string; From 4edc5b8a145a9067dec2d97ca86b42b67be1da73 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 3 Dec 2024 18:37:18 -0800 Subject: [PATCH 03/20] Handle sso response in backend. --- lib/client/weblogin.go | 11 ++++++++++- lib/web/mfajson/mfajson.go | 27 ++++----------------------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 7edf946c0e39f..31978248faf89 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -124,7 +124,7 @@ type SSOResponse struct { // GetOptionalMFAResponseProtoReq converts response to a type proto.MFAAuthenticateResponse, // if there were any responses set. Otherwise returns nil. func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthenticateResponse, error) { - if r.TOTPCode != "" && r.WebauthnResponse != nil { + if r.TOTPCode != "" && r.WebauthnResponse != nil && r.SSOResponse != nil { return nil, trace.BadParameter("only one MFA response field can be set") } @@ -140,6 +140,15 @@ func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthe }}, nil } + if r.SSOResponse != nil { + return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: r.SSOResponse.RequestID, + Token: r.SSOResponse.Token, + }, + }}, nil + } + return nil, nil } diff --git a/lib/web/mfajson/mfajson.go b/lib/web/mfajson/mfajson.go index 70abb8ecfec32..fcbd95bfa2096 100644 --- a/lib/web/mfajson/mfajson.go +++ b/lib/web/mfajson/mfajson.go @@ -45,33 +45,14 @@ func Decode(b []byte, typ string) (*authproto.MFAAuthenticateResponse, error) { return nil, trace.Wrap(err) } - switch { - case resp.CredentialAssertionResponse != nil: + // TODO(Joerger): DELETE in v18.0.0, client.MFAChallengeResponse is be used instead. + if resp.CredentialAssertionResponse != nil { return &authproto.MFAAuthenticateResponse{ Response: &authproto.MFAAuthenticateResponse_Webauthn{ Webauthn: wantypes.CredentialAssertionResponseToProto(resp.CredentialAssertionResponse), }, }, nil - case resp.WebauthnResponse != nil: - return &authproto.MFAAuthenticateResponse{ - Response: &authproto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(resp.WebauthnResponse), - }, - }, nil - case resp.SSOResponse != nil: - return &authproto.MFAAuthenticateResponse{ - Response: &authproto.MFAAuthenticateResponse_SSO{ - SSO: &authproto.SSOResponse{ - RequestId: resp.SSOResponse.RequestID, - Token: resp.SSOResponse.Token, - }, - }, - }, nil - case resp.TOTPCode != "": - // Note: we can support TOTP through the websocket if desired, we just need to add - // a TOTP prompt modal and flip the switch here. - return nil, trace.BadParameter("totp is not supported in the WebUI") - default: - return nil, trace.BadParameter("invalid MFA response from web") } + + return resp.GetOptionalMFAResponseProtoReq() } From afad48daf3f4ad4a51bb7fce05041fe0b74c4f77 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 21 Nov 2024 16:20:09 -0800 Subject: [PATCH 04/20] Handle non-webauthn mfa response for file transfer, admin actions, and app session. --- lib/client/weblogin.go | 32 +++++++++++--- lib/web/apiserver.go | 10 ++--- lib/web/apiserver_test.go | 8 ++-- lib/web/apps.go | 29 ++++++------ lib/web/files.go | 44 ++++++++----------- lib/web/mfajson/mfajson.go | 21 +++++---- .../src/Console/DocumentSsh/useGetScpUrl.ts | 7 +-- web/packages/teleport/src/config.ts | 19 ++++---- .../teleport/src/lib/EventEmitterMfaSender.ts | 9 ---- web/packages/teleport/src/lib/tdp/client.ts | 8 ---- web/packages/teleport/src/lib/term/tty.ts | 12 +---- web/packages/teleport/src/lib/useMfa.ts | 2 +- .../teleport/src/services/api/api.test.ts | 6 +-- web/packages/teleport/src/services/api/api.ts | 8 ++-- .../teleport/src/services/apps/apps.ts | 10 +++-- 15 files changed, 108 insertions(+), 117 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 31978248faf89..cd5d70d9251af 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -113,6 +113,8 @@ type MFAChallengeResponse struct { WebauthnResponse *wantypes.CredentialAssertionResponse `json:"webauthn_response,omitempty"` // SSOResponse is a response from an SSO MFA flow. SSOResponse *SSOResponse `json:"sso_response"` + // TODO(Joerger): DELETE IN v19.0.0, WebauthnResponse used instead. + WebauthnAssertionResponse *wantypes.CredentialAssertionResponse `json:"webauthnAssertionResponse"` } // SSOResponse is a json compatible [proto.SSOResponse]. @@ -128,15 +130,25 @@ func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthe return nil, trace.BadParameter("only one MFA response field can be set") } - if r.TOTPCode != "" { + switch { + case r.WebauthnResponse != nil: + return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_Webauthn{ + Webauthn: wantypes.CredentialAssertionResponseToProto(r.WebauthnResponse), + }}, nil + case r.SSOResponse != nil: + return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: r.SSOResponse.RequestID, + Token: r.SSOResponse.Token, + }, + }}, nil + case r.TOTPCode != "": return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_TOTP{ TOTP: &proto.TOTPResponse{Code: r.TOTPCode}, }}, nil - } - - if r.WebauthnResponse != nil { + case r.WebauthnAssertionResponse != nil: return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(r.WebauthnResponse), + Webauthn: wantypes.CredentialAssertionResponseToProto(r.WebauthnAssertionResponse), }}, nil } @@ -152,6 +164,16 @@ func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthe return nil, nil } +func ParseMFAChallengeResponse(mfaResponseJSON []byte) (*proto.MFAAuthenticateResponse, error) { + var resp MFAChallengeResponse + if err := json.Unmarshal(mfaResponseJSON, &resp); err != nil { + return nil, trace.Wrap(err) + } + + protoResp, err := resp.GetOptionalMFAResponseProtoReq() + return protoResp, trace.Wrap(err) +} + // CreateSSHCertReq is passed by tsh to authenticate a local user without MFA // and receive short-lived certificates. type CreateSSHCertReq struct { diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 3edae1da2db3e..81538c40df642 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -4847,16 +4847,12 @@ func parseMFAResponseFromRequest(r *http.Request) error { // context and returned. func contextWithMFAResponseFromRequestHeader(ctx context.Context, requestHeader http.Header) (context.Context, error) { if mfaResponseJSON := requestHeader.Get("Teleport-MFA-Response"); mfaResponseJSON != "" { - var resp mfaResponse - if err := json.Unmarshal([]byte(mfaResponseJSON), &resp); err != nil { + mfaResp, err := client.ParseMFAChallengeResponse([]byte(mfaResponseJSON)) + if err != nil { return nil, trace.Wrap(err) } - return mfa.ContextWithMFAResponse(ctx, &proto.MFAAuthenticateResponse{ - Response: &proto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(resp.WebauthnAssertionResponse), - }, - }), nil + return mfa.ContextWithMFAResponse(ctx, mfaResp), nil } return ctx, nil diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 48a9ff61179a5..3ac6f34e64d73 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -5573,10 +5573,6 @@ func TestCreateAppSession_RequireSessionMFA(t *testing.T) { require.NoError(t, err) mfaResp, err := webauthnDev.SolveAuthn(chal) require.NoError(t, err) - mfaRespJSON, err := json.Marshal(mfaResponse{ - WebauthnAssertionResponse: wantypes.CredentialAssertionResponseFromProto(mfaResp.GetWebauthn()), - }) - require.NoError(t, err) // Extract the session ID and bearer token for the current session. rawCookie := *pack.cookies[0] @@ -5610,7 +5606,9 @@ func TestCreateAppSession_RequireSessionMFA(t *testing.T) { PublicAddr: "panel.example.com", ClusterName: "localhost", }, - MFAResponse: string(mfaRespJSON), + MFAResponse: client.MFAChallengeResponse{ + WebauthnAssertionResponse: wantypes.CredentialAssertionResponseFromProto(mfaResp.GetWebauthn()), + }, }, expectMFAVerified: true, }, diff --git a/lib/web/apps.go b/lib/web/apps.go index 5e809d2df29e1..7dc431ff22f77 100644 --- a/lib/web/apps.go +++ b/lib/web/apps.go @@ -22,7 +22,6 @@ package web import ( "context" - "encoding/json" "net/http" "sort" @@ -33,7 +32,7 @@ import ( "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" - wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" + "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/httplib" "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/utils" @@ -191,7 +190,10 @@ type CreateAppSessionRequest struct { // AWSRole is the AWS role ARN when accessing AWS management console. AWSRole string `json:"arn,omitempty"` // MFAResponse is an optional MFA response used to create an MFA verified app session. - MFAResponse string `json:"mfa_response"` + MFAResponse client.MFAChallengeResponse `json:"mfaResponse"` + // TODO(Joerger): DELETE IN v19.0.0 + // Backwards compatible version of MFAResponse + MFAResponseJSON string `json:"mfa_response"` } // CreateAppSessionResponse is a response to POST /v1/webapi/sessions/app @@ -230,17 +232,16 @@ func (h *Handler) createAppSession(w http.ResponseWriter, r *http.Request, p htt } } - var mfaProtoResponse *proto.MFAAuthenticateResponse - if req.MFAResponse != "" { - var resp mfaResponse - if err := json.Unmarshal([]byte(req.MFAResponse), &resp); err != nil { - return nil, trace.Wrap(err) - } + mfaResponse, err := req.MFAResponse.GetOptionalMFAResponseProtoReq() + if err != nil { + return nil, trace.Wrap(err) + } - mfaProtoResponse = &proto.MFAAuthenticateResponse{ - Response: &proto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(resp.WebauthnAssertionResponse), - }, + // Fallback to backwards compatible mfa response. + if mfaResponse == nil && req.MFAResponseJSON != "" { + mfaResponse, err = client.ParseMFAChallengeResponse([]byte(req.MFAResponseJSON)) + if err != nil { + return nil, trace.Wrap(err) } } @@ -263,7 +264,7 @@ func (h *Handler) createAppSession(w http.ResponseWriter, r *http.Request, p htt PublicAddr: result.App.GetPublicAddr(), ClusterName: result.ClusterName, AWSRoleARN: req.AWSRole, - MFAResponse: mfaProtoResponse, + MFAResponse: mfaResponse, AppName: result.App.GetName(), URI: result.App.GetURI(), ClientAddr: r.RemoteAddr, diff --git a/lib/web/files.go b/lib/web/files.go index 53248258dd034..62b3913760313 100644 --- a/lib/web/files.go +++ b/lib/web/files.go @@ -20,7 +20,6 @@ package web import ( "context" - "encoding/json" "errors" "net/http" "time" @@ -35,7 +34,6 @@ import ( "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auth/authclient" - wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/multiplexer" "github.com/gravitational/teleport/lib/reversetunnelclient" @@ -56,8 +54,8 @@ type fileTransferRequest struct { remoteLocation string // filename is a file name filename string - // webauthn is an optional parameter that contains a webauthn response string used to issue single use certs - webauthn string + // mfaResponse is an optional parameter that contains an mfa response string used to issue single use certs + mfaResponse string // fileTransferRequestID is used to find a FileTransferRequest on a session fileTransferRequestID string // moderatedSessonID is an ID of a moderated session that has completed a @@ -74,11 +72,22 @@ func (h *Handler) transferFile(w http.ResponseWriter, r *http.Request, p httprou remoteLocation: query.Get("location"), filename: query.Get("filename"), namespace: defaults.Namespace, - webauthn: query.Get("webauthn"), + mfaResponse: query.Get("mfaResponse"), fileTransferRequestID: query.Get("fileTransferRequestId"), moderatedSessionID: query.Get("moderatedSessionId"), } + // Check for old query parameter, uses the same data structure. + // TODO(Joerger): DELETE IN v19.0.0 + if req.mfaResponse == "" { + req.mfaResponse = query.Get("webauthn") + } + + mfaResponse, err := client.ParseMFAChallengeResponse([]byte(req.mfaResponse)) + if err != nil { + return nil, trace.Wrap(err) + } + // Send an error if only one of these params has been sent. Both should exist or not exist together if (req.fileTransferRequestID != "") != (req.moderatedSessionID != "") { return nil, trace.BadParameter("fileTransferRequestId and moderatedSessionId must both be included in the same request.") @@ -107,7 +116,7 @@ func (h *Handler) transferFile(w http.ResponseWriter, r *http.Request, p httprou return nil, trace.Wrap(err) } - if mfaReq.Required && query.Get("webauthn") == "" { + if mfaReq.Required && mfaResponse == nil { return nil, trace.AccessDenied("MFA required for file transfer") } @@ -135,8 +144,8 @@ func (h *Handler) transferFile(w http.ResponseWriter, r *http.Request, p httprou return nil, trace.Wrap(err) } - if req.webauthn != "" { - err = ft.issueSingleUseCert(req.webauthn, r, tc) + if req.mfaResponse != "" { + err = ft.issueSingleUseCert(mfaResponse, r, tc) if err != nil { return nil, trace.Wrap(err) } @@ -216,21 +225,10 @@ func (f *fileTransfer) createClient(req fileTransferRequest, httpReq *http.Reque return tc, nil } -type mfaResponse struct { - // WebauthnResponse is the response from authenticators. - WebauthnAssertionResponse *wantypes.CredentialAssertionResponse `json:"webauthnAssertionResponse"` -} - // issueSingleUseCert will take an assertion response sent from a solved challenge in the web UI // and use that to generate a cert. This cert is added to the Teleport Client as an authmethod that // can be used to connect to a node. -func (f *fileTransfer) issueSingleUseCert(webauthn string, httpReq *http.Request, tc *client.TeleportClient) error { - var mfaResp mfaResponse - err := json.Unmarshal([]byte(webauthn), &mfaResp) - if err != nil { - return trace.Wrap(err) - } - +func (f *fileTransfer) issueSingleUseCert(mfaResponse *proto.MFAAuthenticateResponse, httpReq *http.Request, tc *client.TeleportClient) error { pk, err := keys.ParsePrivateKey(f.sctx.cfg.Session.GetSSHPriv()) if err != nil { return trace.Wrap(err) @@ -241,11 +239,7 @@ func (f *fileTransfer) issueSingleUseCert(webauthn string, httpReq *http.Request SSHPublicKey: pk.MarshalSSHPublicKey(), Username: f.sctx.GetUser(), Expires: time.Now().Add(time.Minute).UTC(), - MFAResponse: &proto.MFAAuthenticateResponse{ - Response: &proto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(mfaResp.WebauthnAssertionResponse), - }, - }, + MFAResponse: mfaResponse, }) if err != nil { return trace.Wrap(err) diff --git a/lib/web/mfajson/mfajson.go b/lib/web/mfajson/mfajson.go index fcbd95bfa2096..2105b0178b3a9 100644 --- a/lib/web/mfajson/mfajson.go +++ b/lib/web/mfajson/mfajson.go @@ -28,7 +28,7 @@ import ( "github.com/gravitational/teleport/lib/client" ) -// TODO(Joerger): DELETE IN v18.0.0 and use client.MFAChallengeResponse instead. +// TODO(Joerger): DELETE IN v19.0.0 and use client.MFAChallengeResponse instead. // Before v17, the WebUI sends a flattened webauthn response instead of a full // MFA challenge response. Newer WebUI versions v17+ will send both for // backwards compatibility. @@ -45,14 +45,17 @@ func Decode(b []byte, typ string) (*authproto.MFAAuthenticateResponse, error) { return nil, trace.Wrap(err) } - // TODO(Joerger): DELETE in v18.0.0, client.MFAChallengeResponse is be used instead. - if resp.CredentialAssertionResponse != nil { - return &authproto.MFAAuthenticateResponse{ - Response: &authproto.MFAAuthenticateResponse_Webauthn{ - Webauthn: wantypes.CredentialAssertionResponseToProto(resp.CredentialAssertionResponse), - }, - }, nil + // Move flattened webauthn response into resp. + resp.MFAChallengeResponse.WebauthnAssertionResponse = resp.CredentialAssertionResponse + + protoResp, err := resp.GetOptionalMFAResponseProtoReq() + if err != nil { + return nil, trace.Wrap(err) + } + + if protoResp == nil { + return nil, trace.BadParameter("invalid MFA response from web") } - return resp.GetOptionalMFAResponseProtoReq() + return protoResp, trace.Wrap(err) } diff --git a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts b/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts index 478ccbcc5fa59..ac12807524d9c 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts +++ b/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts @@ -39,17 +39,14 @@ export default function useGetScpUrl(addMfaToScpUrls: boolean) { scope: MfaChallengeScope.USER_SESSION, }); - const response = await auth.getMfaChallengeResponse( - challenge, - 'webauthn' - ); + const mfaResponse = await auth.getMfaChallengeResponse(challenge); setAttempt({ status: 'success', statusText: '', }); return cfg.getScpUrl({ - webauthn: response.webauthn_response, + mfaResponse, ...params, }); } catch (error) { diff --git a/web/packages/teleport/src/config.ts b/web/packages/teleport/src/config.ts index f69a468f121fe..52f394b9e08ee 100644 --- a/web/packages/teleport/src/config.ts +++ b/web/packages/teleport/src/config.ts @@ -17,9 +17,7 @@ */ import { generatePath } from 'react-router'; - import { IncludedResourceMode } from 'shared/components/UnifiedResources'; - import { mergeDeep } from 'shared/utils/highbar'; import { @@ -43,10 +41,10 @@ import type { import type { SortType } from 'teleport/services/agents'; import type { KubeResourceKind } from 'teleport/services/kube/types'; -import type { WebauthnAssertionResponse } from 'teleport/services/mfa'; import type { RecordingType } from 'teleport/services/recordings'; import type { ParticipantMode } from 'teleport/services/session'; import type { YamlSupportedResourceKind } from 'teleport/services/yaml/types'; +import type { MfaChallengeResponse } from './services/mfa'; const cfg = { /** @deprecated Use cfg.edition instead. */ @@ -885,20 +883,25 @@ const cfg = { }); }, - getScpUrl({ webauthn, ...params }: UrlScpParams) { + getScpUrl({ mfaResponse, ...params }: UrlScpParams) { let path = generatePath(cfg.api.scp, { ...params, }); - if (!webauthn) { + if (!mfaResponse) { return path; } // non-required MFA will mean this param is undefined and generatePath doesn't like undefined // or optional params. So we append it ourselves here. Its ok to be undefined when sent to the server // as the existence of this param is what will issue certs - return `${path}&webauthn=${JSON.stringify({ - webauthnAssertionResponse: webauthn, + + // TODO(Joerger): DELETE IN v19.0.0 + // We include webauthn for backwards compatibility. + path = `${path}&webauthn=${JSON.stringify({ + webauthnAssertionResponse: mfaResponse.webauthn_response, })}`; + + return `${path}&mfaResponse=${JSON.stringify(mfaResponse)}`; }, getRenewTokenUrl() { @@ -1254,7 +1257,7 @@ export interface UrlScpParams { filename: string; moderatedSessionId?: string; fileTransferRequestId?: string; - webauthn?: WebauthnAssertionResponse; + mfaResponse?: MfaChallengeResponse; } export interface UrlSshParams { diff --git a/web/packages/teleport/src/lib/EventEmitterMfaSender.ts b/web/packages/teleport/src/lib/EventEmitterMfaSender.ts index da30f1201e0c9..656ac29dc77fe 100644 --- a/web/packages/teleport/src/lib/EventEmitterMfaSender.ts +++ b/web/packages/teleport/src/lib/EventEmitterMfaSender.ts @@ -32,15 +32,6 @@ class EventEmitterMfaSender extends EventEmitter { sendChallengeResponse(data: MfaChallengeResponse) { throw new Error('Not implemented'); } - - // TODO (avatus) DELETE IN 18 - /** - * @deprecated Use sendChallengeResponse instead. - */ - // eslint-disable-next-line @typescript-eslint/no-unused-vars - sendWebAuthn(data: WebauthnAssertionResponse) { - throw new Error('Not implemented'); - } } export { EventEmitterMfaSender }; diff --git a/web/packages/teleport/src/lib/tdp/client.ts b/web/packages/teleport/src/lib/tdp/client.ts index ca18c58744124..ef44f0cce0f3a 100644 --- a/web/packages/teleport/src/lib/tdp/client.ts +++ b/web/packages/teleport/src/lib/tdp/client.ts @@ -624,14 +624,6 @@ export default class Client extends EventEmitterMfaSender { this.send(this.codec.encodeClipboardData(clipboardData)); } - sendWebAuthn(data: WebauthnAssertionResponse) { - const msg = this.codec.encodeMfaJson({ - mfaType: 'n', - jsonString: JSON.stringify(data), - }); - this.send(msg); - } - addSharedDirectory(sharedDirectory: FileSystemDirectoryHandle) { try { this.sdManager.add(sharedDirectory); diff --git a/web/packages/teleport/src/lib/term/tty.ts b/web/packages/teleport/src/lib/term/tty.ts index 3e924ff466f3f..22311cb8f5ff7 100644 --- a/web/packages/teleport/src/lib/term/tty.ts +++ b/web/packages/teleport/src/lib/term/tty.ts @@ -88,7 +88,7 @@ class Tty extends EventEmitterMfaSender { // but to be backward compatible, we need to still spread the existing webauthn only fields // as "top level" fields so old proxies can still respond to webauthn challenges. // in 19, we can just pass "data" without this extra step - // TODO (avatus): DELETE IN 18 + // TODO (avatus): DELETE IN 19.0.0 const backwardCompatibleData = { ...data.webauthn_response, ...data, @@ -100,16 +100,6 @@ class Tty extends EventEmitterMfaSender { this.socket.send(bytearray); } - // TODO (avatus) DELETE IN 18 - /** - * @deprecated Use sendChallengeResponse instead. - */ - sendWebAuthn(data: WebauthnAssertionResponse) { - const encoded = this._proto.encodeChallengeResponse(JSON.stringify(data)); - const bytearray = new Uint8Array(encoded); - this.socket.send(bytearray); - } - sendKubeExecData(data: KubeExecData) { const encoded = this._proto.encodeKubeExecData(JSON.stringify(data)); const bytearray = new Uint8Array(encoded); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 29ab598d5af39..e7970d00f34d7 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -86,7 +86,7 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { errorText: '', webauthnPublicKey: null, })); - emitterSender.sendWebAuthn(res.webauthn_response); + emitterSender.sendChallengeResponse(res); }) .catch((err: Error) => { setErrorText(err.message); diff --git a/web/packages/teleport/src/services/api/api.test.ts b/web/packages/teleport/src/services/api/api.test.ts index b9689eeb4210b..af362e602bc4e 100644 --- a/web/packages/teleport/src/services/api/api.test.ts +++ b/web/packages/teleport/src/services/api/api.test.ts @@ -16,8 +16,6 @@ * along with this program. If not, see . */ -import { MfaChallengeResponse } from '../mfa'; - import api, { MFA_HEADER, defaultRequestOptions, @@ -28,7 +26,7 @@ import api, { describe('api.fetch', () => { const mockedFetch = jest.spyOn(global, 'fetch').mockResolvedValue({} as any); // we don't care about response - const mfaResp: MfaChallengeResponse = { + const mfaResp = { webauthn_response: { id: 'some-id', type: 'some-type', @@ -104,6 +102,7 @@ describe('api.fetch', () => { ...defaultRequestOptions.headers, ...getAuthHeaders(), [MFA_HEADER]: JSON.stringify({ + ...mfaResp, webauthnAssertionResponse: mfaResp.webauthn_response, }), }, @@ -124,6 +123,7 @@ describe('api.fetch', () => { ...customOpts.headers, ...getAuthHeaders(), [MFA_HEADER]: JSON.stringify({ + ...mfaResp, webauthnAssertionResponse: mfaResp.webauthn_response, }), }, diff --git a/web/packages/teleport/src/services/api/api.ts b/web/packages/teleport/src/services/api/api.ts index 1048c3333e11c..02f1c4ffbb21c 100644 --- a/web/packages/teleport/src/services/api/api.ts +++ b/web/packages/teleport/src/services/api/api.ts @@ -237,8 +237,8 @@ const api = { * If customOptions field is not provided, only fields defined in * `defaultRequestOptions` will be used. * - * @param webauthnResponse if defined (eg: `fetchJsonWithMfaAuthnRetry`) - * will add a custom MFA header field that will hold the webauthn response. + * @param mfaResponse if defined (eg: `fetchJsonWithMfaAuthnRetry`) + * will add a custom MFA header field that will hold the mfaResponse. */ fetch( url: string, @@ -258,7 +258,9 @@ const api = { if (mfaResponse) { options.headers[MFA_HEADER] = JSON.stringify({ - // TODO(Joerger): Handle non-webauthn response. + ...mfaResponse, + // TODO(Joerger): DELETE IN v19.0.0. + // We include webauthnAssertionResponse for backwards compatibility. webauthnAssertionResponse: mfaResponse.webauthn_response, }); } diff --git a/web/packages/teleport/src/services/apps/apps.ts b/web/packages/teleport/src/services/apps/apps.ts index d64f37414a872..a8ec75c1e2eb3 100644 --- a/web/packages/teleport/src/services/apps/apps.ts +++ b/web/packages/teleport/src/services/apps/apps.ts @@ -57,15 +57,17 @@ const service = { }, }); - const resp = await auth.getMfaChallengeResponse(challenge); + const mfaResponse = await auth.getMfaChallengeResponse(challenge); const createAppSession = { ...resolveApp, arn: params.arn, - // TODO(Joerger): Handle non-webauthn response. - mfa_response: resp + mfaResponse, + // TODO(Joerger): DELETE IN v19.0.0. + // We include a string version of the MFA response for backwards compatibility. + mfa_response: mfaResponse ? JSON.stringify({ - webauthnAssertionResponse: resp.webauthn_response, + webauthnAssertionResponse: mfaResponse.webauthn_response, }) : null, }; From 7d751a02190e5a7deaf56e64fd2c8a885475e07f Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 4 Dec 2024 13:02:25 -0800 Subject: [PATCH 05/20] Simplify useMfa with new helpers. --- .../DocumentKubeExec/DocumentKubeExec.tsx | 4 +- .../src/Console/DocumentSsh/DocumentSsh.tsx | 6 +- .../src/DesktopSession/DesktopSession.tsx | 8 +- .../AuthnDialog/AuthnDialog.story.tsx | 35 ++-- .../AuthnDialog/AuthnDialog.test.tsx | 46 +++-- .../components/AuthnDialog/AuthnDialog.tsx | 31 +-- .../ReAuthenticate/useReAuthenticate.ts | 9 +- web/packages/teleport/src/lib/useMfa.ts | 186 ++++-------------- 8 files changed, 120 insertions(+), 205 deletions(-) diff --git a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx index 30f3f45cae668..b67ee03167efd 100644 --- a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx +++ b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx @@ -43,7 +43,7 @@ export default function DocumentKubeExec({ doc, visible }: Props) { useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.requested]); + }, [visible, mfa.mfaChallenge]); const theme = useTheme(); const terminal = ( @@ -63,7 +63,7 @@ export default function DocumentKubeExec({ doc, visible }: Props) { )} - {mfa.requested && } + {mfa.mfaChallenge && } {status === 'waiting-for-exec-data' && ( diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index c378216dd66fb..34b8ce90fa4e5 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -62,7 +62,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { getDownloader, getUploader, fileTransferRequests, - } = useFileTransfer(tty, session, doc, mfa.addMfaToScpUrls); + } = useFileTransfer(tty, session, doc, mfa.mfaRequired); const theme = useTheme(); function handleCloseFileTransfer() { @@ -76,7 +76,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.requested]); + }, [visible, mfa.mfaChallenge]); const onSearchClose = useCallback(() => { setShowSearch(false); @@ -143,7 +143,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - {mfa.requested && } + {mfa.mfaChallenge && } {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index f5105f7d0246e..04a7f3485f9df 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -186,9 +186,9 @@ const MfaDialog = ({ mfa }: { mfa: MfaState }) => { { - mfa.setErrorText( - 'This session requires multi factor authentication to continue. Please hit "Retry" and follow the prompts given by your browser to complete authentication.' - ); + mfa.submitAttempt.status = 'failed'; + mfa.submitAttempt.statusText = + 'This session requires multi factor authentication to continue. Please hit "Retry" and follow the prompts given by your browser to complete authentication.'; }} /> ); @@ -294,7 +294,7 @@ const nextScreenState = ( // Otherwise, calculate a new screen state. const showAnotherSessionActive = showAnotherSessionActiveDialog; - const showMfa = webauthn.requested; + const showMfa = webauthn.mfaChallenge; const showAlert = fetchAttempt.status === 'failed' || // Fetch attempt failed tdpConnection.status === 'failed' || // TDP connection failed diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx index 0e493d383efb4..a73971043c5b3 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx @@ -29,18 +29,20 @@ export const LoadedWithMultipleOptions = () => { ...defaultProps, mfa: { ...defaultProps.mfa, - ssoChallenge: { - redirectUrl: 'hi', - requestId: '123', - channelId: '123', - device: { - connectorId: '123', - connectorType: 'saml', - displayName: 'Okta', + mfaChallenge: { + ssoChallenge: { + redirectUrl: 'hi', + requestId: '123', + channelId: '123', + device: { + connectorId: '123', + connectorType: 'saml', + displayName: 'Okta', + }, + }, + webauthnPublicKey: { + challenge: new ArrayBuffer(1), }, - }, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), }, }, }; @@ -52,8 +54,10 @@ export const LoadedWithSingleOption = () => { ...defaultProps, mfa: { ...defaultProps.mfa, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), + mfaChallenge: { + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, }, }, }; @@ -65,7 +69,10 @@ export const Error = () => { ...defaultProps, mfa: { ...defaultProps.mfa, - errorText: 'Something went wrong', + submitAttempt: { + status: 'failed', + statusText: 'Something went wrong', + }, }, }; return ; diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx index 516c021c8d452..d7577b2af823f 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx @@ -16,15 +16,15 @@ * along with this program. If not, see . */ -import { render, screen, fireEvent } from 'design/utils/testing'; +import { fireEvent, render, screen } from 'design/utils/testing'; import { makeDefaultMfaState, MfaState } from 'teleport/lib/useMfa'; -import { SSOChallenge } from 'teleport/services/mfa'; +import { SsoChallenge } from 'teleport/services/mfa'; import AuthnDialog from './AuthnDialog'; -const mockSsoChallenge: SSOChallenge = { +const mockSsoChallenge: SsoChallenge = { redirectUrl: 'url', requestId: '123', device: { @@ -51,7 +51,11 @@ describe('AuthnDialog', () => { }); test('renders single option dialog', () => { - const mfa = makeMockState({ ssoChallenge: mockSsoChallenge }); + const mfa = makeMockState({ + mfaChallenge: { + ssoChallenge: mockSsoChallenge, + }, + }); render(); expect(screen.getByText('Verify Your Identity')).toBeInTheDocument(); @@ -64,9 +68,11 @@ describe('AuthnDialog', () => { test('renders multi option dialog', () => { const mfa = makeMockState({ - ssoChallenge: mockSsoChallenge, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), + mfaChallenge: { + ssoChallenge: mockSsoChallenge, + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, }, }); render(); @@ -83,7 +89,15 @@ describe('AuthnDialog', () => { test('displays error text when provided', () => { const errorText = 'Authentication failed'; - const mfa = makeMockState({ errorText }); + const mfa = makeMockState({ + mfaChallenge: {}, + submitAttempt: { + status: 'error', + statusText: errorText, + data: null, + error: new Error(errorText), + }, + }); render(); expect(screen.getByTestId('danger-alert')).toBeInTheDocument(); @@ -92,23 +106,27 @@ describe('AuthnDialog', () => { test('sso button renders with callback', async () => { const mfa = makeMockState({ - ssoChallenge: mockSsoChallenge, - onSsoAuthenticate: jest.fn(), + mfaChallenge: { + ssoChallenge: mockSsoChallenge, + }, + submitMfa: jest.fn(), }); render(); const ssoButton = screen.getByText('Okta'); fireEvent.click(ssoButton); - expect(mfa.onSsoAuthenticate).toHaveBeenCalledTimes(1); + expect(mfa.submitMfa).toHaveBeenCalledTimes(1); }); test('webauthn button renders with callback', async () => { const mfa = makeMockState({ - webauthnPublicKey: { challenge: new ArrayBuffer(0) }, - onWebauthnAuthenticate: jest.fn(), + mfaChallenge: { + webauthnPublicKey: { challenge: new ArrayBuffer(0) }, + }, + submitMfa: jest.fn(), }); render(); const webauthn = screen.getByText('Passkey or MFA Device'); fireEvent.click(webauthn); - expect(mfa.onWebauthnAuthenticate).toHaveBeenCalledTimes(1); + expect(mfa.submitMfa).toHaveBeenCalledTimes(1); }); }); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index 0a301b1f16c43..f867c976d5f8e 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -16,11 +16,11 @@ * along with this program. If not, see . */ -import Dialog, { DialogContent } from 'design/Dialog'; import { Danger } from 'design/Alert'; -import { FingerprintSimple, Cross } from 'design/Icon'; +import Dialog, { DialogContent } from 'design/Dialog'; +import { Cross, FingerprintSimple } from 'design/Icon'; -import { Text, ButtonSecondary, Flex, ButtonIcon, H2 } from 'design'; +import { ButtonIcon, ButtonSecondary, Flex, H2, Text } from 'design'; import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; @@ -28,7 +28,8 @@ import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; import { MfaState } from 'teleport/lib/useMfa'; export default function AuthnDialog({ mfa, onCancel }: Props) { - let hasMultipleOptions = mfa.ssoChallenge && mfa.webauthnPublicKey; + let hasMultipleOptions = + mfa.mfaChallenge.ssoChallenge && mfa.mfaChallenge.webauthnPublicKey; return ( ({ width: '400px' })} open={true}> @@ -39,9 +40,9 @@ export default function AuthnDialog({ mfa, onCancel }: Props) { - {mfa.errorText && ( + {mfa.submitAttempt.status === 'error' && ( - {mfa.errorText} + {mfa.submitAttempt.statusText} )} @@ -51,28 +52,28 @@ export default function AuthnDialog({ mfa, onCancel }: Props) { - {mfa.ssoChallenge && ( + {mfa.mfaChallenge.ssoChallenge && ( mfa.submitMfa('sso')} gap={2} block > - {mfa.ssoChallenge.device.displayName || - mfa.ssoChallenge.device.connectorId} + {mfa.mfaChallenge.ssoChallenge.device.displayName || + mfa.mfaChallenge.ssoChallenge.device.connectorId} )} - {mfa.webauthnPublicKey && ( + {mfa.mfaChallenge.webauthnPublicKey && ( mfa.submitMfa('webauthn')} gap={2} block > diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index ed8c73f3fe6da..62291b5a1f6a7 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -38,12 +38,15 @@ export default function useReAuthenticate({ const [mfaOptions, setMfaOptions] = useState(); const [challengeState, setChallengeState] = useState(); + function setMfaChallenge(challenge: MfaAuthenticateChallenge) { + setChallengeState({ challenge, deviceUsage: 'mfa' }); + } + const [initAttempt, init] = useAsync(async () => { const challenge = await auth.getMfaChallenge({ scope: challengeScope, }); - - setChallengeState({ challenge, deviceUsage: 'mfa' }); + setMfaChallenge(challenge); setMfaOptions(getMfaChallengeOptions(challenge)); }); @@ -112,6 +115,7 @@ export default function useReAuthenticate({ return { initAttempt, mfaOptions, + setMfaChallenge, submitWithMfa, submitAttempt, clearSubmitAttempt, @@ -126,6 +130,7 @@ export type ReauthProps = { export type ReauthState = { initAttempt: Attempt; mfaOptions: MfaOption[]; + setMfaChallenge: (challenge: MfaAuthenticateChallenge) => void; submitWithMfa: ( mfaType?: DeviceType, deviceUsage?: DeviceUsage, diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index e7970d00f34d7..69971b36621a6 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -16,183 +16,67 @@ * along with this program. If not, see . */ -import { useState, useEffect, useCallback } from 'react'; +import { useCallback, useEffect, useState } from 'react'; +import { Attempt, makeEmptyAttempt, useAsync } from 'shared/hooks/useAsync'; import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender'; import { TermEvent } from 'teleport/lib/term/enums'; -import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa'; -import { - MfaAuthenticateChallengeJson, - SsoChallenge, -} from 'teleport/services/mfa'; import auth from 'teleport/services/auth/auth'; +import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; +import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa'; export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { - const [state, setState] = useState<{ - errorText: string; - addMfaToScpUrls: boolean; - webauthnPublicKey: PublicKeyCredentialRequestOptions; - ssoChallenge: SsoChallenge; - totpChallenge: boolean; - }>({ - addMfaToScpUrls: false, - errorText: '', - webauthnPublicKey: null, - ssoChallenge: null, - totpChallenge: false, - }); - - function clearChallenges() { - setState(prevState => ({ - ...prevState, - totpChallenge: false, - webauthnPublicKey: null, - ssoChallenge: null, - })); - } - - function onSsoAuthenticate() { - if (!state.ssoChallenge) { - setState(prevState => ({ - ...prevState, - errorText: 'Invalid or missing SSO challenge', - })); - return; - } - - auth.openSsoChallengeRedirect(state.ssoChallenge); - } - - function onWebauthnAuthenticate() { - if (!window.PublicKeyCredential) { - const errorText = - 'This browser does not support WebAuthn required for hardware tokens, \ - please try the latest version of Chrome, Firefox or Safari.'; - - setState({ - ...state, - errorText, - }); - return; - } - - auth - .getMfaChallengeResponse({ - webauthnPublicKey: state.webauthnPublicKey, - }) - .then(res => { - setState(prevState => ({ - ...prevState, - errorText: '', - webauthnPublicKey: null, - })); - emitterSender.sendChallengeResponse(res); - }) - .catch((err: Error) => { - setErrorText(err.message); - }); - } - - const waitForSsoChallengeResponse = useCallback( - async ( - ssoChallenge: SsoChallenge, - abortSignal: AbortSignal - ): Promise => { - try { - const resp = await auth.waitForSsoChallengeResponse( - ssoChallenge, - abortSignal - ); - emitterSender.sendChallengeResponse(resp); - clearChallenges(); - } catch (error) { - if (error.name !== 'AbortError') { - throw error; - } - } - }, - [emitterSender] + const [mfaChallenge, setMfaChallenge] = useState(); + const [mfaRequired, setMfaRequired] = useState(false); + + const [submitAttempt, submitMfa] = useAsync( + useCallback( + async (mfaType: DeviceType) => { + if (!mfaChallenge) + throw new Error('expected non empty mfa challenge in state'); + + return auth + .getMfaChallengeResponse(mfaChallenge, mfaType) + .then(res => emitterSender.sendChallengeResponse(res)); + }, + [mfaChallenge] + ) ); useEffect(() => { - let ssoChallengeAbortController: AbortController | undefined; const challengeHandler = (challengeJson: string) => { - const challenge = JSON.parse( - challengeJson - ) as MfaAuthenticateChallengeJson; - - const { webauthnPublicKey, ssoChallenge, totpChallenge } = - parseMfaChallenge(challenge); - - setState(prevState => ({ - ...prevState, - addMfaToScpUrls: true, - ssoChallenge, - webauthnPublicKey, - totpChallenge, - })); - - if (ssoChallenge) { - ssoChallengeAbortController = new AbortController(); - void waitForSsoChallengeResponse( - ssoChallenge, - ssoChallengeAbortController.signal - ); - } + const challenge = JSON.parse(challengeJson); + setMfaChallenge(parseMfaChallenge(challenge)); + setMfaRequired(true); }; emitterSender?.on(TermEvent.MFA_CHALLENGE, challengeHandler); - return () => { - ssoChallengeAbortController?.abort(); emitterSender?.removeListener(TermEvent.MFA_CHALLENGE, challengeHandler); }; - }, [emitterSender, waitForSsoChallengeResponse]); - - function setErrorText(newErrorText: string) { - setState(prevState => ({ ...prevState, errorText: newErrorText })); - } - - // if any challenge exists, requested is true - const requested = !!( - state.webauthnPublicKey || - state.totpChallenge || - state.ssoChallenge - ); + }, [emitterSender]); return { - requested, - onWebauthnAuthenticate, - onSsoAuthenticate, - addMfaToScpUrls: state.addMfaToScpUrls, - setErrorText, - errorText: state.errorText, - webauthnPublicKey: state.webauthnPublicKey, - ssoChallenge: state.ssoChallenge, + mfaChallenge, + submitAttempt, + submitMfa, + mfaRequired, }; } export type MfaState = { - onWebauthnAuthenticate: () => void; - onSsoAuthenticate: () => void; - setErrorText: (errorText: string) => void; - errorText: string; - requested: boolean; - addMfaToScpUrls: boolean; - webauthnPublicKey: PublicKeyCredentialRequestOptions; - ssoChallenge: SsoChallenge; + mfaChallenge: MfaAuthenticateChallenge; + submitMfa: (mfaType?: DeviceType) => Promise; + submitAttempt: Attempt; + mfaRequired: boolean; }; // used for testing export function makeDefaultMfaState(): MfaState { return { - onWebauthnAuthenticate: () => null, - onSsoAuthenticate: () => null, - setErrorText: () => null, - errorText: '', - requested: false, - addMfaToScpUrls: false, - webauthnPublicKey: null, - ssoChallenge: null, + mfaChallenge: null, + submitAttempt: makeEmptyAttempt(), + submitMfa: () => null, + mfaRequired: false, }; } From 62e581d49a5c50a125987b1ec8aff2fb9ea92960 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 17:18:01 -0800 Subject: [PATCH 06/20] Fix lint. --- web/packages/teleport/src/lib/EventEmitterMfaSender.ts | 5 +---- web/packages/teleport/src/lib/tdp/client.ts | 1 - web/packages/teleport/src/lib/term/tty.ts | 1 - web/packages/teleport/src/lib/useMfa.ts | 2 +- 4 files changed, 2 insertions(+), 7 deletions(-) diff --git a/web/packages/teleport/src/lib/EventEmitterMfaSender.ts b/web/packages/teleport/src/lib/EventEmitterMfaSender.ts index 656ac29dc77fe..2753251121061 100644 --- a/web/packages/teleport/src/lib/EventEmitterMfaSender.ts +++ b/web/packages/teleport/src/lib/EventEmitterMfaSender.ts @@ -18,10 +18,7 @@ import { EventEmitter } from 'events'; -import { - MfaChallengeResponse, - WebauthnAssertionResponse, -} from 'teleport/services/mfa'; +import { MfaChallengeResponse } from 'teleport/services/mfa'; class EventEmitterMfaSender extends EventEmitter { constructor() { diff --git a/web/packages/teleport/src/lib/tdp/client.ts b/web/packages/teleport/src/lib/tdp/client.ts index ef44f0cce0f3a..b6ab1264b185d 100644 --- a/web/packages/teleport/src/lib/tdp/client.ts +++ b/web/packages/teleport/src/lib/tdp/client.ts @@ -57,7 +57,6 @@ import type { SyncKeys, SharedDirectoryTruncateResponse, } from './codec'; -import type { WebauthnAssertionResponse } from 'teleport/services/mfa'; export enum TdpClientEvent { TDP_CLIENT_SCREEN_SPEC = 'tdp client screen spec', diff --git a/web/packages/teleport/src/lib/term/tty.ts b/web/packages/teleport/src/lib/term/tty.ts index 22311cb8f5ff7..a78fafb1ebd0d 100644 --- a/web/packages/teleport/src/lib/term/tty.ts +++ b/web/packages/teleport/src/lib/term/tty.ts @@ -19,7 +19,6 @@ import Logger from 'shared/libs/logger'; import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender'; -import { WebauthnAssertionResponse } from 'teleport/services/mfa'; import { AuthenticatedWebSocket } from 'teleport/lib/AuthenticatedWebSocket'; import { MfaChallengeResponse } from 'teleport/services/mfa'; diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 69971b36621a6..4b62b1968672d 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -77,6 +77,6 @@ export function makeDefaultMfaState(): MfaState { mfaChallenge: null, submitAttempt: makeEmptyAttempt(), submitMfa: () => null, - mfaRequired: false, + mfaRequired: true, }; } From 781023896a9f19f6a6c0ece1257da9f5cedcba62 Mon Sep 17 00:00:00 2001 From: joerger Date: Sun, 15 Dec 2024 19:05:46 -0800 Subject: [PATCH 07/20] Use AuthnDialog for file transfers; Fix json backend logic for file transfers. --- lib/web/files.go | 9 +- .../src/Console/DocumentSsh/DocumentSsh.tsx | 37 ++++---- .../Console/DocumentSsh/useFileTransfer.ts | 88 ++++++++++++++++--- .../src/Console/DocumentSsh/useGetScpUrl.ts | 63 ------------- .../components/AuthnDialog/AuthnDialog.tsx | 40 +++++---- web/packages/teleport/src/lib/useMfa.ts | 3 +- 6 files changed, 128 insertions(+), 112 deletions(-) delete mode 100644 web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts diff --git a/lib/web/files.go b/lib/web/files.go index 62b3913760313..1c48dbf4f745e 100644 --- a/lib/web/files.go +++ b/lib/web/files.go @@ -83,9 +83,12 @@ func (h *Handler) transferFile(w http.ResponseWriter, r *http.Request, p httprou req.mfaResponse = query.Get("webauthn") } - mfaResponse, err := client.ParseMFAChallengeResponse([]byte(req.mfaResponse)) - if err != nil { - return nil, trace.Wrap(err) + var mfaResponse *proto.MFAAuthenticateResponse + if req.mfaResponse != "" { + var err error + if mfaResponse, err = client.ParseMFAChallengeResponse([]byte(req.mfaResponse)); err != nil { + return nil, trace.Wrap(err) + } } // Send an error if only one of these params has been sent. Both should exist or not exist together diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index 34b8ce90fa4e5..4ea1b934e076c 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -16,16 +16,16 @@ * along with this program. If not, see . */ -import { useRef, useEffect, useState, useCallback } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { useTheme } from 'styled-components'; -import { Indicator, Box } from 'design'; +import { Box, Indicator } from 'design'; import { - FileTransferActionBar, FileTransfer, - FileTransferRequests, + FileTransferActionBar, FileTransferContextProvider, + FileTransferRequests, } from 'shared/components/FileTransfer'; import { TerminalSearch } from 'shared/components/TerminalSearch'; @@ -39,8 +39,8 @@ import Document from '../Document'; import { useConsoleContext } from '../consoleContextProvider'; import { Terminal, TerminalRef } from './Terminal'; -import useSshSession from './useSshSession'; import { useFileTransfer } from './useFileTransfer'; +import useSshSession from './useSshSession'; export default function DocumentSshWrapper(props: PropTypes) { return ( @@ -57,12 +57,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { const { tty, status, closeDocument, session } = useSshSession(doc); const [showSearch, setShowSearch] = useState(false); const mfa = useMfa(tty); - const { - getMfaResponseAttempt, - getDownloader, - getUploader, - fileTransferRequests, - } = useFileTransfer(tty, session, doc, mfa.mfaRequired); + const ft = useFileTransfer(tty, session, doc, mfa.mfaRequired); const theme = useTheme(); function handleCloseFileTransfer() { @@ -110,21 +105,19 @@ function DocumentSsh({ doc, visible }: PropTypes) { } beforeClose={() => window.confirm('Are you sure you want to cancel file transfers?') } errorText={ - getMfaResponseAttempt.status === 'failed' - ? getMfaResponseAttempt.statusText - : null + ft.mfaAttempt.status === 'error' ? ft.mfaAttempt.statusText : null } afterClose={handleCloseFileTransfer} transferHandlers={{ - getDownloader, - getUploader, + getDownloader: ft.getDownloader, + getUploader: ft.getUploader, }} /> @@ -143,7 +136,15 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - {mfa.mfaChallenge && } + {mfa.mfaChallenge && } + {ft.mfaChallenge && ( + + )} {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts index 90c3625a902cf..0439574200501 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts +++ b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts @@ -16,18 +16,26 @@ * along with this program. If not, see . */ -import { useEffect, useState, useCallback } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { useFileTransferContext } from 'shared/components/FileTransfer'; -import Tty from 'teleport/lib/term/tty'; +import { DocumentSsh } from 'teleport/Console/stores'; import { EventType } from 'teleport/lib/term/enums'; +import Tty from 'teleport/lib/term/tty'; import { Session } from 'teleport/services/session'; -import { DocumentSsh } from 'teleport/Console/stores'; + +import { useAsync } from 'shared/hooks/useAsync'; +import cfg from 'teleport/config'; +import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; +import { + DeviceType, + MfaAuthenticateChallenge, + MfaChallengeResponse, +} from 'teleport/services/mfa'; import { useConsoleContext } from '../consoleContextProvider'; import { getHttpFileTransferHandlers } from './httpFileTransferHandlers'; -import useGetScpUrl from './useGetScpUrl'; export type FileTransferRequest = { sid: string; @@ -60,17 +68,63 @@ export const useFileTransfer = ( const [fileTransferRequests, setFileTransferRequests] = useState< FileTransferRequest[] >([]); - const { getScpUrl, attempt: getMfaResponseAttempt } = - useGetScpUrl(addMfaToScpUrls); const { clusterId, serverId, login } = currentDoc; + const [mfaChallenge, setMfaChallenge] = useState(); + const mfaResponsePromise = + useRef>(); + + const clearMfaChallenge = () => { + setMfaChallenge(null); + }; + + const [mfaAttempt, getMfaResponse] = useAsync( + useCallback(async () => { + const challenge = await auth.getMfaChallenge({ + scope: MfaChallengeScope.USER_SESSION, + isMfaRequiredRequest: { + node: { + node_name: serverId, + login: login, + }, + }, + }); + if (!challenge) return; + + setMfaChallenge(challenge); + mfaResponsePromise.current = Promise.withResolvers(); + const mfaResponse = await mfaResponsePromise.current.promise; + return mfaResponse; + }, [setMfaChallenge, clearMfaChallenge, mfaResponsePromise]) + ); + + const [submitMfaAttempt, submitMfa] = useAsync( + useCallback( + async (mfaType?: DeviceType) => { + try { + const resp = auth.getMfaChallengeResponse(mfaChallenge, mfaType); + mfaResponsePromise.current.resolve(resp); + } catch (err) { + mfaResponsePromise.current.reject(err); + throw err; + } + }, + [mfaChallenge, mfaResponsePromise] + ) + ); + const download = useCallback( async ( location: string, abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - const url = await getScpUrl({ + let mfaResponse: MfaChallengeResponse; + if (addMfaToScpUrls) { + [mfaResponse] = await getMfaResponse(); + } + + const url = cfg.getScpUrl({ location, clusterId, serverId, @@ -78,7 +132,9 @@ export const useFileTransfer = ( filename: location, moderatedSessionId: moderatedSessionParams?.moderatedSessionId, fileTransferRequestId: moderatedSessionParams?.fileRequestId, + mfaResponse, }); + if (!url) { // if we return nothing here, the file transfer will not be added to the // file transfer list. If we add it to the list, the file will continue to @@ -88,7 +144,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().download(url, abortController); }, - [clusterId, login, serverId, getScpUrl] + [clusterId, login, serverId, addMfaToScpUrls] ); const upload = useCallback( @@ -98,7 +154,12 @@ export const useFileTransfer = ( abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - const url = await getScpUrl({ + let mfaResponse: MfaChallengeResponse; + if (addMfaToScpUrls) { + [mfaResponse] = await getMfaResponse(); + } + + const url = cfg.getScpUrl({ location, clusterId, serverId, @@ -106,6 +167,7 @@ export const useFileTransfer = ( filename: file.name, moderatedSessionId: moderatedSessionParams?.moderatedSessionId, fileTransferRequestId: moderatedSessionParams?.fileRequestId, + mfaResponse, }); if (!url) { // if we return nothing here, the file transfer will not be added to the @@ -116,7 +178,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().upload(url, file, abortController); }, - [clusterId, serverId, login, getScpUrl] + [clusterId, serverId, login, addMfaToScpUrls] ); /* @@ -255,8 +317,12 @@ export const useFileTransfer = ( } return { + mfaAttempt, + mfaChallenge, + clearMfaChallenge, + submitMfa, + submitMfaAttempt, fileTransferRequests, - getMfaResponseAttempt, getUploader, getDownloader, }; diff --git a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts b/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts deleted file mode 100644 index ac12807524d9c..0000000000000 --- a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts +++ /dev/null @@ -1,63 +0,0 @@ -/** - * Teleport - * Copyright (C) 2023 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import { useCallback } from 'react'; -import useAttempt from 'shared/hooks/useAttemptNext'; - -import cfg, { UrlScpParams } from 'teleport/config'; -import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; - -export default function useGetScpUrl(addMfaToScpUrls: boolean) { - const { setAttempt, attempt, handleError } = useAttempt(''); - - const getScpUrl = useCallback( - async (params: UrlScpParams) => { - setAttempt({ - status: 'processing', - statusText: '', - }); - if (!addMfaToScpUrls) { - return cfg.getScpUrl(params); - } - try { - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.USER_SESSION, - }); - - const mfaResponse = await auth.getMfaChallengeResponse(challenge); - - setAttempt({ - status: 'success', - statusText: '', - }); - return cfg.getScpUrl({ - mfaResponse, - ...params, - }); - } catch (error) { - handleError(error); - } - }, - [addMfaToScpUrls, handleError, setAttempt] - ); - - return { - getScpUrl, - attempt, - }; -} diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index f867c976d5f8e..b67a5a7f5788b 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -25,11 +25,17 @@ import { ButtonIcon, ButtonSecondary, Flex, H2, Text } from 'design'; import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; -import { MfaState } from 'teleport/lib/useMfa'; +import { Attempt } from 'shared/hooks/useAsync'; +import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; -export default function AuthnDialog({ mfa, onCancel }: Props) { - let hasMultipleOptions = - mfa.mfaChallenge.ssoChallenge && mfa.mfaChallenge.webauthnPublicKey; +export default function AuthnDialog({ + mfaChallenge, + submitMfa, + submitAttempt, + onCancel, +}: Props) { + const hasMultipleOptions = + mfaChallenge.ssoChallenge && mfaChallenge.webauthnPublicKey; return ( ({ width: '400px' })} open={true}> @@ -40,9 +46,9 @@ export default function AuthnDialog({ mfa, onCancel }: Props) { - {mfa.submitAttempt.status === 'error' && ( + {submitAttempt.status === 'error' && ( - {mfa.submitAttempt.statusText} + {submitAttempt.statusText} )} @@ -52,28 +58,28 @@ export default function AuthnDialog({ mfa, onCancel }: Props) { - {mfa.mfaChallenge.ssoChallenge && ( + {mfaChallenge.ssoChallenge && ( mfa.submitMfa('sso')} + onClick={() => submitMfa('sso')} gap={2} block > - {mfa.mfaChallenge.ssoChallenge.device.displayName || - mfa.mfaChallenge.ssoChallenge.device.connectorId} + {mfaChallenge.ssoChallenge.device.displayName || + mfaChallenge.ssoChallenge.device.connectorId} )} - {mfa.mfaChallenge.webauthnPublicKey && ( + {mfaChallenge.webauthnPublicKey && ( mfa.submitMfa('webauthn')} + onClick={() => submitMfa('webauthn')} gap={2} block > @@ -87,6 +93,8 @@ export default function AuthnDialog({ mfa, onCancel }: Props) { } export type Props = { - mfa: MfaState; + mfaChallenge: MfaAuthenticateChallenge; + submitMfa: (mfaType?: DeviceType) => Promise; + submitAttempt: Attempt; onCancel: () => void; }; diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 4b62b1968672d..71e65f219f18c 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -37,7 +37,8 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { return auth .getMfaChallengeResponse(mfaChallenge, mfaType) - .then(res => emitterSender.sendChallengeResponse(res)); + .then(res => emitterSender.sendChallengeResponse(res)) + .then(() => setMfaChallenge(null)); }, [mfaChallenge] ) From 0ccf73d0dfb77adc45d3d0740ea1297d4b98150f Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 12:55:54 -0800 Subject: [PATCH 08/20] Make useMfa and AuthnDialog more reusable and error proof. --- .../DocumentKubeExec/DocumentKubeExec.tsx | 14 +- .../src/Console/DocumentSsh/DocumentSsh.tsx | 39 +++-- .../Console/DocumentSsh/useFileTransfer.ts | 76 +------- .../src/DesktopSession/DesktopSession.tsx | 13 +- .../src/DesktopSession/useDesktopSession.tsx | 4 +- .../AuthnDialog/AuthnDialog.test.tsx | 18 +- .../components/AuthnDialog/AuthnDialog.tsx | 103 ++++++----- web/packages/teleport/src/lib/useMfa.ts | 164 ++++++++++++++---- 8 files changed, 248 insertions(+), 183 deletions(-) diff --git a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx index b67ee03167efd..94fded2d6c49f 100644 --- a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx +++ b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx @@ -22,7 +22,7 @@ import { Box, Indicator } from 'design'; import * as stores from 'teleport/Console/stores/types'; import { Terminal, TerminalRef } from 'teleport/Console/DocumentSsh/Terminal'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfaTty } from 'teleport/lib/useMfa'; import useKubeExecSession from 'teleport/Console/DocumentKubeExec/useKubeExecSession'; import Document from 'teleport/Console/Document'; @@ -39,11 +39,11 @@ export default function DocumentKubeExec({ doc, visible }: Props) { const terminalRef = useRef(); const { tty, status, closeDocument, sendKubeExecData } = useKubeExecSession(doc); - const mfa = useMfa(tty); + const mfa = useMfaTty(tty); useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.mfaChallenge]); + }, [visible, mfa.challenge]); const theme = useTheme(); const terminal = ( @@ -63,7 +63,13 @@ export default function DocumentKubeExec({ doc, visible }: Props) { )} - {mfa.mfaChallenge && } + { + mfa.resetAttempt(); + closeDocument(); + }} + /> {status === 'waiting-for-exec-data' && ( diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index 4ea1b934e076c..4ea493ecfea42 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -32,7 +32,8 @@ import { TerminalSearch } from 'shared/components/TerminalSearch'; import * as stores from 'teleport/Console/stores'; import AuthnDialog from 'teleport/components/AuthnDialog'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfa, useMfaTty } from 'teleport/lib/useMfa'; +import { MfaChallengeScope } from 'teleport/services/auth/auth'; import Document from '../Document'; @@ -56,8 +57,15 @@ function DocumentSsh({ doc, visible }: PropTypes) { const terminalRef = useRef(); const { tty, status, closeDocument, session } = useSshSession(doc); const [showSearch, setShowSearch] = useState(false); - const mfa = useMfa(tty); - const ft = useFileTransfer(tty, session, doc, mfa.mfaRequired); + + const ttyMfa = useMfaTty(tty); + const ftMfa = useMfa({ + isMfaRequired: ttyMfa.required, + req: { + scope: MfaChallengeScope.USER_SESSION, + }, + }); + const ft = useFileTransfer(tty, session, doc, ftMfa); const theme = useTheme(); function handleCloseFileTransfer() { @@ -71,7 +79,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.mfaChallenge]); + }, [visible, ftMfa.challenge]); const onSearchClose = useCallback(() => { setShowSearch(false); @@ -111,13 +119,9 @@ function DocumentSsh({ doc, visible }: PropTypes) { beforeClose={() => window.confirm('Are you sure you want to cancel file transfers?') } - errorText={ - ft.mfaAttempt.status === 'error' ? ft.mfaAttempt.statusText : null - } afterClose={handleCloseFileTransfer} transferHandlers={{ - getDownloader: ft.getDownloader, - getUploader: ft.getUploader, + ...ft, }} /> @@ -136,15 +140,14 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - {mfa.mfaChallenge && } - {ft.mfaChallenge && ( - - )} + { + ttyMfa.resetAttempt(); + closeDocument(); + }} + /> + {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts index 0439574200501..92c4c9976a198 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts +++ b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -import { useCallback, useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useState } from 'react'; import { useFileTransferContext } from 'shared/components/FileTransfer'; import { DocumentSsh } from 'teleport/Console/stores'; @@ -24,14 +24,9 @@ import { EventType } from 'teleport/lib/term/enums'; import Tty from 'teleport/lib/term/tty'; import { Session } from 'teleport/services/session'; -import { useAsync } from 'shared/hooks/useAsync'; import cfg from 'teleport/config'; -import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; -import { - DeviceType, - MfaAuthenticateChallenge, - MfaChallengeResponse, -} from 'teleport/services/mfa'; + +import { MfaState } from 'teleport/lib/useMfa'; import { useConsoleContext } from '../consoleContextProvider'; @@ -59,7 +54,7 @@ export const useFileTransfer = ( tty: Tty, session: Session, currentDoc: DocumentSsh, - addMfaToScpUrls: boolean + mfa: MfaState ) => { const { filesStore } = useFileTransferContext(); const startTransfer = filesStore.start; @@ -70,60 +65,13 @@ export const useFileTransfer = ( >([]); const { clusterId, serverId, login } = currentDoc; - const [mfaChallenge, setMfaChallenge] = useState(); - const mfaResponsePromise = - useRef>(); - - const clearMfaChallenge = () => { - setMfaChallenge(null); - }; - - const [mfaAttempt, getMfaResponse] = useAsync( - useCallback(async () => { - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.USER_SESSION, - isMfaRequiredRequest: { - node: { - node_name: serverId, - login: login, - }, - }, - }); - if (!challenge) return; - - setMfaChallenge(challenge); - mfaResponsePromise.current = Promise.withResolvers(); - const mfaResponse = await mfaResponsePromise.current.promise; - return mfaResponse; - }, [setMfaChallenge, clearMfaChallenge, mfaResponsePromise]) - ); - - const [submitMfaAttempt, submitMfa] = useAsync( - useCallback( - async (mfaType?: DeviceType) => { - try { - const resp = auth.getMfaChallengeResponse(mfaChallenge, mfaType); - mfaResponsePromise.current.resolve(resp); - } catch (err) { - mfaResponsePromise.current.reject(err); - throw err; - } - }, - [mfaChallenge, mfaResponsePromise] - ) - ); - const download = useCallback( async ( location: string, abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - let mfaResponse: MfaChallengeResponse; - if (addMfaToScpUrls) { - [mfaResponse] = await getMfaResponse(); - } - + const mfaResponse = await mfa.getChallengeResponse(); const url = cfg.getScpUrl({ location, clusterId, @@ -144,7 +92,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().download(url, abortController); }, - [clusterId, login, serverId, addMfaToScpUrls] + [clusterId, login, serverId, mfa] ); const upload = useCallback( @@ -154,10 +102,7 @@ export const useFileTransfer = ( abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - let mfaResponse: MfaChallengeResponse; - if (addMfaToScpUrls) { - [mfaResponse] = await getMfaResponse(); - } + const mfaResponse = await mfa.getChallengeResponse(); const url = cfg.getScpUrl({ location, @@ -178,7 +123,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().upload(url, file, abortController); }, - [clusterId, serverId, login, addMfaToScpUrls] + [clusterId, serverId, login, mfa] ); /* @@ -317,11 +262,6 @@ export const useFileTransfer = ( } return { - mfaAttempt, - mfaChallenge, - clearMfaChallenge, - submitMfa, - submitMfaAttempt, fileTransferRequests, getUploader, getDownloader, diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index 04a7f3485f9df..42bf5ceda4018 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -184,11 +184,12 @@ export function DesktopSession(props: State) { const MfaDialog = ({ mfa }: { mfa: MfaState }) => { return ( { - mfa.submitAttempt.status = 'failed'; - mfa.submitAttempt.statusText = - 'This session requires multi factor authentication to continue. Please hit "Retry" and follow the prompts given by your browser to complete authentication.'; + {...mfa} + replaceErrorText={ + 'This session requires multi factor authentication to continue. Please hit try again and follow the prompts given by your browser to complete authentication.' + } + resetAttempt={() => { + mfa.resetAttempt(); }} /> ); @@ -294,7 +295,7 @@ const nextScreenState = ( // Otherwise, calculate a new screen state. const showAnotherSessionActive = showAnotherSessionActiveDialog; - const showMfa = webauthn.mfaChallenge; + const showMfa = webauthn.challenge; const showAlert = fetchAttempt.status === 'failed' || // Fetch attempt failed tdpConnection.status === 'failed' || // TDP connection failed diff --git a/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx b/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx index 1f642d38d8d96..f14482669f471 100644 --- a/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx @@ -22,7 +22,7 @@ import { useParams } from 'react-router'; import useAttempt from 'shared/hooks/useAttemptNext'; import { ButtonState } from 'teleport/lib/tdp'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfaTty } from 'teleport/lib/useMfa'; import desktopService from 'teleport/services/desktops'; import userService from 'teleport/services/user'; @@ -130,7 +130,7 @@ export default function useDesktopSession() { }); const tdpClient = clientCanvasProps.tdpClient; - const mfa = useMfa(tdpClient); + const mfa = useMfaTty(tdpClient); const onShareDirectory = () => { try { diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx index d7577b2af823f..3d146dd9fd96a 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx @@ -52,7 +52,7 @@ describe('AuthnDialog', () => { test('renders single option dialog', () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, }, }); @@ -68,7 +68,7 @@ describe('AuthnDialog', () => { test('renders multi option dialog', () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, webauthnPublicKey: { challenge: new ArrayBuffer(1), @@ -90,7 +90,7 @@ describe('AuthnDialog', () => { test('displays error text when provided', () => { const errorText = 'Authentication failed'; const mfa = makeMockState({ - mfaChallenge: {}, + challenge: {}, submitAttempt: { status: 'error', statusText: errorText, @@ -106,27 +106,27 @@ describe('AuthnDialog', () => { test('sso button renders with callback', async () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, }, - submitMfa: jest.fn(), + submit: jest.fn(), }); render(); const ssoButton = screen.getByText('Okta'); fireEvent.click(ssoButton); - expect(mfa.submitMfa).toHaveBeenCalledTimes(1); + expect(mfa.submit).toHaveBeenCalledTimes(1); }); test('webauthn button renders with callback', async () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { webauthnPublicKey: { challenge: new ArrayBuffer(0) }, }, - submitMfa: jest.fn(), + submit: jest.fn(), }); render(); const webauthn = screen.getByText('Passkey or MFA Device'); fireEvent.click(webauthn); - expect(mfa.submitMfa).toHaveBeenCalledTimes(1); + expect(mfa.submit).toHaveBeenCalledTimes(1); }); }); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index b67a5a7f5788b..3bb5ea4442838 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -26,68 +26,87 @@ import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; import { Attempt } from 'shared/hooks/useAsync'; +import { MfaState } from 'teleport/lib/useMfa'; import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; +type AuthnDialogProps = MfaState & { + replaceErrorText?: string; +}; + export default function AuthnDialog({ - mfaChallenge, - submitMfa, - submitAttempt, - onCancel, -}: Props) { - const hasMultipleOptions = - mfaChallenge.ssoChallenge && mfaChallenge.webauthnPublicKey; + options, + challenge, + submit, + attempt, + resetAttempt, + replaceErrorText, +}: AuthnDialogProps) { + // Only show the dialog when we are in a processing or error state. + if (attempt.status === '' || attempt.status === 'success') return; + + // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. + const onlyTotpAvailable = options?.length === 1 && challenge?.totpChallenge; return ( ({ width: '400px' })} open={true}>

Verify Your Identity

- + - {submitAttempt.status === 'error' && ( + {onlyTotpAvailable && ( - {submitAttempt.statusText} + { + 'totp is not currently supported for this action, please register a security key to continue' + } + + )} + {attempt.status === 'error' && ( + + {replaceErrorText || attempt.statusText} )} - {hasMultipleOptions + {options?.length > 1 ? 'Select one of the following methods to verify your identity:' : 'Select the method below to verify your identity:'} - - {mfaChallenge.ssoChallenge && ( - submitMfa('sso')} - gap={2} - block - > - - {mfaChallenge.ssoChallenge.device.displayName || - mfaChallenge.ssoChallenge.device.connectorId} - - )} - {mfaChallenge.webauthnPublicKey && ( - submitMfa('webauthn')} - gap={2} - block - > - - Passkey or MFA Device - - )} - + {challenge && ( + + {challenge.ssoChallenge && ( + submit('sso')} + gap={2} + block + > + + {challenge.ssoChallenge.device.displayName || + challenge.ssoChallenge.device.connectorId} + + )} + {challenge.webauthnPublicKey && ( + submit('webauthn')} + gap={2} + block + > + + Passkey or MFA Device + + )} + + )} ); } diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 71e65f219f18c..704767d81f10d 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -16,68 +16,164 @@ * along with this program. If not, see . */ -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { Attempt, makeEmptyAttempt, useAsync } from 'shared/hooks/useAsync'; import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender'; import { TermEvent } from 'teleport/lib/term/enums'; +import { + CreateAuthenticateChallengeRequest, + parseMfaChallengeJson, +} from 'teleport/services/auth'; import auth from 'teleport/services/auth/auth'; -import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; -import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa'; +import { + DeviceType, + getMfaChallengeOptions, + MfaAuthenticateChallenge, + MfaChallengeResponse, + MfaOption, +} from 'teleport/services/mfa'; -export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { - const [mfaChallenge, setMfaChallenge] = useState(); - const [mfaRequired, setMfaRequired] = useState(false); +export type MfaProps = { + req?: CreateAuthenticateChallengeRequest; + isMfaRequired?: boolean | null; +}; + +// useMfa prepares an MFA state designed for use with AuthnDialog. +// 1. Call getMfaChallengeResponse() where MFA may be required. +// If MFA is not required, it should be a noop, returning null. +// 2. Setup AuthnDialog using the MFA state, to show a dialog when +// mfaAttempt.state === 'processing'. +export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { + const [required, setMfaRequired] = useState(null); + const [options, setMfaOptions] = useState(); - const [submitAttempt, submitMfa] = useAsync( + const [challenge, setMfaChallenge] = useState(); + const mfaResponsePromise = + useRef>(); + + const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( - async (mfaType: DeviceType) => { - if (!mfaChallenge) - throw new Error('expected non empty mfa challenge in state'); - - return auth - .getMfaChallengeResponse(mfaChallenge, mfaType) - .then(res => emitterSender.sendChallengeResponse(res)) - .then(() => setMfaChallenge(null)); + async (challenge?: MfaAuthenticateChallenge) => { + if (!challenge) { + if (isMfaRequired === false || required === false) return; + + challenge = await auth.getMfaChallenge(req); + if (!challenge) { + setMfaRequired(false); + return; + } + } + + // Set mfa requirement and options after we get a challenge for the first time. + if (!required) setMfaRequired(true); + if (!options) setMfaOptions(getMfaChallengeOptions(challenge)); + + mfaResponsePromise.current = Promise.withResolvers(); + setMfaChallenge(challenge); + try { + return await mfaResponsePromise.current.promise; + } finally { + mfaResponsePromise.current = null; + setMfaChallenge(null); + } }, - [mfaChallenge] + [req, mfaResponsePromise, options, isMfaRequired, required] ) ); + const resetAttempt = () => { + if (mfaResponsePromise.current) mfaResponsePromise.current.reject(); + mfaResponsePromise.current = null; + setMfaChallenge(null); + setMfaAttempt(makeEmptyAttempt()); + }; + + const getChallengeResponse = async (challenge?: MfaAuthenticateChallenge) => { + const [resp, err] = await getResponse(challenge); + if (err) throw err; + return resp; + }; + + const submit = useCallback( + async (mfaType?: DeviceType, totpCode?: string) => { + try { + const resp = await auth.getMfaChallengeResponse( + challenge, + mfaType, + totpCode + ); + mfaResponsePromise.current.resolve(resp); + } catch (err) { + setMfaAttempt({ + data: null, + status: 'error', + statusText: err.message, + error: err, + }); + } + }, + [challenge, mfaResponsePromise, setMfaAttempt] + ); + + return { + required, + options, + challenge, + getChallengeResponse, + submit, + attempt, + resetAttempt, + }; +} + +export function useMfaTty(emitterSender: EventEmitterMfaSender): MfaState { + const [mfaRequired, setMfaRequired] = useState(false); + + const mfa = useMfa({ isMfaRequired: mfaRequired }); + useEffect(() => { - const challengeHandler = (challengeJson: string) => { - const challenge = JSON.parse(challengeJson); - setMfaChallenge(parseMfaChallenge(challenge)); + const challengeHandler = async (challengeJson: string) => { + // set Mfa required for other uses of this MfaState (e.g. file transfers) setMfaRequired(true); + + const challenge = parseMfaChallengeJson(JSON.parse(challengeJson)); + const resp = await mfa.getChallengeResponse(challenge); + emitterSender.sendChallengeResponse(resp); }; emitterSender?.on(TermEvent.MFA_CHALLENGE, challengeHandler); return () => { emitterSender?.removeListener(TermEvent.MFA_CHALLENGE, challengeHandler); }; - }, [emitterSender]); + }, [mfa, emitterSender]); - return { - mfaChallenge, - submitAttempt, - submitMfa, - mfaRequired, - }; + return mfa; } export type MfaState = { - mfaChallenge: MfaAuthenticateChallenge; - submitMfa: (mfaType?: DeviceType) => Promise; - submitAttempt: Attempt; - mfaRequired: boolean; + required: boolean; + options: MfaOption[]; + challenge: MfaAuthenticateChallenge; + // Generally you wouldn't pass in a challenge, unless you already + // have one handy, e.g. from a terminal websocket message. + getChallengeResponse: ( + challenge?: MfaAuthenticateChallenge + ) => Promise; + submit: (mfaType?: DeviceType, totpCode?: string) => Promise; + attempt: Attempt; + resetAttempt: () => void; }; // used for testing export function makeDefaultMfaState(): MfaState { return { - mfaChallenge: null, - submitAttempt: makeEmptyAttempt(), - submitMfa: () => null, - mfaRequired: true, + required: true, + options: null, + challenge: null, + getChallengeResponse: async () => null, + submit: () => null, + attempt: makeEmptyAttempt(), + resetAttempt: () => null, }; } From f86afc0291a75dcfccbfaab7519be4346b73cf7c Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 14:05:01 -0800 Subject: [PATCH 09/20] Use AuthnDialog for App sessions. --- .../teleport/src/AppLauncher/AppLauncher.tsx | 40 +++++++++++++++---- .../teleport/src/services/apps/apps.ts | 29 ++++---------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index 97d3559bb6365..36fba9e0248fb 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -26,8 +26,11 @@ import { AccessDenied } from 'design/CardError'; import useAttempt from 'shared/hooks/useAttemptNext'; +import AuthnDialog from 'teleport/components/AuthnDialog'; import { UrlLauncherParams } from 'teleport/config'; +import { useMfa } from 'teleport/lib/useMfa'; import service from 'teleport/services/apps'; +import { MfaChallengeScope } from 'teleport/services/auth/auth'; export function AppLauncher() { const { attempt, setAttempt } = useAttempt('processing'); @@ -37,6 +40,20 @@ export function AppLauncher() { const queryParams = new URLSearchParams(search); const isRedirectFlow = queryParams.get('required-apps'); + const mfa = useMfa({ + req: { + scope: MfaChallengeScope.USER_SESSION, + allowReuse: false, + isMfaRequiredRequest: { + app: { + fqdn: pathParams.fqdn, + cluster_name: pathParams.clusterId, + public_addr: pathParams.publicAddr, + }, + }, + }, + }); + const createAppSession = useCallback(async (params: UrlLauncherParams) => { let fqdn = params.fqdn; const port = location.port ? `:${location.port}` : ''; @@ -101,7 +118,11 @@ export function AppLauncher() { if (params.arn) { params.arn = decodeURIComponent(params.arn); } - const session = await service.createAppSession(params); + + // Prompt for MFA if per-session MFA is required for this app. + const mfaResponse = await mfa.getChallengeResponse(); + + const session = await service.createAppSession(params, mfaResponse); // Set all the fields expected by server to validate request. const url = getXTeleportAuthUrl({ fqdn, port }); @@ -140,13 +161,18 @@ export function AppLauncher() { useEffect(() => { createAppSession(pathParams); - }, [pathParams]); - - if (attempt.status === 'failed') { - return ; - } + }, []); - return ; + return ( +
+ {attempt.status === 'failed' ? ( + + ) : ( + + )} + +
+ ); } export function AppLauncherProcessing() { diff --git a/web/packages/teleport/src/services/apps/apps.ts b/web/packages/teleport/src/services/apps/apps.ts index a8ec75c1e2eb3..aaa4316f276e2 100644 --- a/web/packages/teleport/src/services/apps/apps.ts +++ b/web/packages/teleport/src/services/apps/apps.ts @@ -16,11 +16,11 @@ * along with this program. If not, see . */ -import api from 'teleport/services/api'; import cfg, { UrlAppParams, UrlResourcesParams } from 'teleport/config'; import { ResourcesResponse } from 'teleport/services/agents'; +import api from 'teleport/services/api'; -import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; +import { MfaChallengeResponse } from '../mfa'; import makeApp from './makeApps'; import { App } from './types'; @@ -41,27 +41,12 @@ const service = { }); }, - async createAppSession(params: UrlAppParams) { - const resolveApp = { - fqdn: params.fqdn, - cluster_name: params.clusterId, - public_addr: params.publicAddr, - }; - - // Prompt for MFA if per-session MFA is required for this app. - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.USER_SESSION, - allowReuse: false, - isMfaRequiredRequest: { - app: resolveApp, - }, - }); - - const mfaResponse = await auth.getMfaChallengeResponse(challenge); - + async createAppSession( + params: UrlAppParams, + mfaResponse: MfaChallengeResponse + ) { const createAppSession = { - ...resolveApp, - arn: params.arn, + ...params, mfaResponse, // TODO(Joerger): DELETE IN v19.0.0. // We include a string version of the MFA response for backwards compatibility. From 3fabcd10285f5fab02b2be7aec4884bffa51ba6d Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 14:33:49 -0800 Subject: [PATCH 10/20] Resolve comments. --- lib/client/weblogin.go | 13 ++++++++++++- lib/web/mfa.go | 4 +++- .../src/Console/DocumentSsh/DocumentSsh.tsx | 9 +++++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index cd5d70d9251af..9821250d5b8b7 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -126,7 +126,18 @@ type SSOResponse struct { // GetOptionalMFAResponseProtoReq converts response to a type proto.MFAAuthenticateResponse, // if there were any responses set. Otherwise returns nil. func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthenticateResponse, error) { - if r.TOTPCode != "" && r.WebauthnResponse != nil && r.SSOResponse != nil { + var availableResponses int + if r.TOTPCode != "" { + availableResponses++ + } + if r.WebauthnResponse != nil { + availableResponses++ + } + if r.SSOResponse != nil { + availableResponses++ + } + + if availableResponses > 1 { return nil, trace.BadParameter("only one MFA response field can be set") } diff --git a/lib/web/mfa.go b/lib/web/mfa.go index c53c66ff5d9d2..c59b0ae10cbd7 100644 --- a/lib/web/mfa.go +++ b/lib/web/mfa.go @@ -215,7 +215,9 @@ func (h *Handler) createAuthenticateChallengeHandle(w http.ResponseWriter, r *ht return nil, trace.Wrap(err) } channelID := id.String() - ssoClientRedirectURL.RawQuery = url.Values{"channel_id": {channelID}}.Encode() + query := ssoClientRedirectURL.Query() + query.Set("channel_id", channelID) + ssoClientRedirectURL.RawQuery = query.Encode() chal, err := clt.CreateAuthenticateChallenge(r.Context(), &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index 4ea493ecfea42..7d3c42cec8a23 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -78,8 +78,13 @@ function DocumentSsh({ doc, visible }: PropTypes) { useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal - terminalRef.current?.focus(); - }, [visible, ftMfa.challenge]); + if ( + ttyMfa.attempt.status === 'processing' || + ftMfa.attempt.status === 'processing' + ) { + terminalRef.current?.focus(); + } + }, [visible, ttyMfa.attempt.status, ftMfa.attempt.status]); const onSearchClose = useCallback(() => { setShowSearch(false); From 2de795560d0a85aa4356552a37501a1879e6e86e Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 19:35:25 -0800 Subject: [PATCH 11/20] Fix broken app launcher; improve mfaRequired logic in useMfa. --- .../src/AppLauncher/AppLauncher.test.tsx | 4 ++-- .../teleport/src/AppLauncher/AppLauncher.tsx | 12 +++++------- web/packages/teleport/src/config.ts | 8 ++++++++ web/packages/teleport/src/lib/useMfa.ts | 18 +++++++++++++----- .../teleport/src/services/apps/apps.ts | 18 ++++++++---------- 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx index ae561d4950532..6b9e7fdf3400b 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.test.tsx @@ -16,13 +16,13 @@ * along with this program. If not, see . */ -import { render, waitFor, screen } from 'design/utils/testing'; +import { render, screen, waitFor } from 'design/utils/testing'; import { createMemoryHistory } from 'history'; import { Router } from 'react-router'; import { Route } from 'teleport/components/Router'; -import api from 'teleport/services/api'; import cfg from 'teleport/config'; +import api from 'teleport/services/api'; import service from 'teleport/services/apps'; import { AppLauncher } from './AppLauncher'; diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index 36fba9e0248fb..39f2911dd241f 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -27,7 +27,7 @@ import { AccessDenied } from 'design/CardError'; import useAttempt from 'shared/hooks/useAttemptNext'; import AuthnDialog from 'teleport/components/AuthnDialog'; -import { UrlLauncherParams } from 'teleport/config'; +import { CreateAppSessionParams, UrlLauncherParams } from 'teleport/config'; import { useMfa } from 'teleport/lib/useMfa'; import service from 'teleport/services/apps'; import { MfaChallengeScope } from 'teleport/services/auth/auth'; @@ -43,7 +43,6 @@ export function AppLauncher() { const mfa = useMfa({ req: { scope: MfaChallengeScope.USER_SESSION, - allowReuse: false, isMfaRequiredRequest: { app: { fqdn: pathParams.fqdn, @@ -119,10 +118,9 @@ export function AppLauncher() { params.arn = decodeURIComponent(params.arn); } - // Prompt for MFA if per-session MFA is required for this app. - const mfaResponse = await mfa.getChallengeResponse(); - - const session = await service.createAppSession(params, mfaResponse); + const createAppSessionParams = params as CreateAppSessionParams; + createAppSessionParams.mfaResponse = await mfa.getChallengeResponse(); + const session = await service.createAppSession(createAppSessionParams); // Set all the fields expected by server to validate request. const url = getXTeleportAuthUrl({ fqdn, port }); @@ -161,7 +159,7 @@ export function AppLauncher() { useEffect(() => { createAppSession(pathParams); - }, []); + }, [pathParams]); return (
diff --git a/web/packages/teleport/src/config.ts b/web/packages/teleport/src/config.ts index 52f394b9e08ee..5d1792b2abb3d 100644 --- a/web/packages/teleport/src/config.ts +++ b/web/packages/teleport/src/config.ts @@ -1249,6 +1249,14 @@ export interface UrlAppParams { arn?: string; } +export interface CreateAppSessionParams { + fqdn: string; + clusterId?: string; + publicAddr?: string; + arn?: string; + mfaResponse?: MfaChallengeResponse; +} + export interface UrlScpParams { clusterId: string; serverId: string; diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 704767d81f10d..d335608dc5b51 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -45,18 +45,26 @@ export type MfaProps = { // 2. Setup AuthnDialog using the MFA state, to show a dialog when // mfaAttempt.state === 'processing'. export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { - const [required, setMfaRequired] = useState(null); + const [mfaRequired, setMfaRequired] = useState(); const [options, setMfaOptions] = useState(); const [challenge, setMfaChallenge] = useState(); const mfaResponsePromise = useRef>(); + useEffect(() => { + setMfaRequired(isMfaRequired); + }, [isMfaRequired]); + + useEffect(() => { + setMfaRequired(null); + }, [req?.isMfaRequiredRequest]); + const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { if (!challenge) { - if (isMfaRequired === false || required === false) return; + if (mfaRequired === false) return; challenge = await auth.getMfaChallenge(req); if (!challenge) { @@ -66,7 +74,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { } // Set mfa requirement and options after we get a challenge for the first time. - if (!required) setMfaRequired(true); + if (!mfaRequired) setMfaRequired(true); if (!options) setMfaOptions(getMfaChallengeOptions(challenge)); mfaResponsePromise.current = Promise.withResolvers(); @@ -78,7 +86,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { setMfaChallenge(null); } }, - [req, mfaResponsePromise, options, isMfaRequired, required] + [req, mfaResponsePromise, options, mfaRequired] ) ); @@ -117,7 +125,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { ); return { - required, + required: mfaRequired, options, challenge, getChallengeResponse, diff --git a/web/packages/teleport/src/services/apps/apps.ts b/web/packages/teleport/src/services/apps/apps.ts index aaa4316f276e2..268a48915aa2b 100644 --- a/web/packages/teleport/src/services/apps/apps.ts +++ b/web/packages/teleport/src/services/apps/apps.ts @@ -16,12 +16,14 @@ * along with this program. If not, see . */ -import cfg, { UrlAppParams, UrlResourcesParams } from 'teleport/config'; +import cfg, { + CreateAppSessionParams, + UrlAppParams, + UrlResourcesParams, +} from 'teleport/config'; import { ResourcesResponse } from 'teleport/services/agents'; import api from 'teleport/services/api'; -import { MfaChallengeResponse } from '../mfa'; - import makeApp from './makeApps'; import { App } from './types'; @@ -41,18 +43,14 @@ const service = { }); }, - async createAppSession( - params: UrlAppParams, - mfaResponse: MfaChallengeResponse - ) { + async createAppSession(params: CreateAppSessionParams) { const createAppSession = { ...params, - mfaResponse, // TODO(Joerger): DELETE IN v19.0.0. // We include a string version of the MFA response for backwards compatibility. - mfa_response: mfaResponse + mfa_response: params.mfaResponse ? JSON.stringify({ - webauthnAssertionResponse: mfaResponse.webauthn_response, + webauthnAssertionResponse: params.mfaResponse.webauthn_response, }) : null, }; From 817dc3e0e5f05dd9012223c8a40a0df2e0305e7b Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 19:46:35 -0800 Subject: [PATCH 12/20] Fix AuthnDialog test. --- .../AuthnDialog/AuthnDialog.test.tsx | 46 ++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx index 3d146dd9fd96a..88df5a52991bf 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx @@ -20,7 +20,7 @@ import { fireEvent, render, screen } from 'design/utils/testing'; import { makeDefaultMfaState, MfaState } from 'teleport/lib/useMfa'; -import { SsoChallenge } from 'teleport/services/mfa'; +import { getMfaChallengeOptions, SsoChallenge } from 'teleport/services/mfa'; import AuthnDialog from './AuthnDialog'; @@ -55,8 +55,13 @@ describe('AuthnDialog', () => { challenge: { ssoChallenge: mockSsoChallenge, }, + attempt: { + status: 'processing', + statusText: '', + data: null, + }, }); - render(); + render(); expect(screen.getByText('Verify Your Identity')).toBeInTheDocument(); expect( @@ -67,15 +72,22 @@ describe('AuthnDialog', () => { }); test('renders multi option dialog', () => { + const challenge = { + ssoChallenge: mockSsoChallenge, + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, + }; const mfa = makeMockState({ - challenge: { - ssoChallenge: mockSsoChallenge, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), - }, + options: getMfaChallengeOptions(challenge), + challenge, + attempt: { + status: 'processing', + statusText: '', + data: null, }, }); - render(); + render(); expect(screen.getByText('Verify Your Identity')).toBeInTheDocument(); expect( @@ -91,14 +103,14 @@ describe('AuthnDialog', () => { const errorText = 'Authentication failed'; const mfa = makeMockState({ challenge: {}, - submitAttempt: { + attempt: { status: 'error', statusText: errorText, data: null, error: new Error(errorText), }, }); - render(); + render(); expect(screen.getByTestId('danger-alert')).toBeInTheDocument(); expect(screen.getByText(errorText)).toBeInTheDocument(); @@ -109,9 +121,14 @@ describe('AuthnDialog', () => { challenge: { ssoChallenge: mockSsoChallenge, }, + attempt: { + status: 'processing', + statusText: '', + data: null, + }, submit: jest.fn(), }); - render(); + render(); const ssoButton = screen.getByText('Okta'); fireEvent.click(ssoButton); expect(mfa.submit).toHaveBeenCalledTimes(1); @@ -122,9 +139,14 @@ describe('AuthnDialog', () => { challenge: { webauthnPublicKey: { challenge: new ArrayBuffer(0) }, }, + attempt: { + status: 'processing', + statusText: '', + data: null, + }, submit: jest.fn(), }); - render(); + render(); const webauthn = screen.getByText('Passkey or MFA Device'); fireEvent.click(webauthn); expect(mfa.submit).toHaveBeenCalledTimes(1); From 702578163997e5eacac30ae15920e9764fea68cc Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 17 Dec 2024 10:20:50 -0800 Subject: [PATCH 13/20] Fix merge conflict with Db web access. --- .../src/Console/DocumentDb/DocumentDb.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx b/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx index 0d6d333141b2a..5f166c8570dfb 100644 --- a/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx +++ b/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx @@ -15,20 +15,20 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ -import { useRef, useEffect } from 'react'; +import { useEffect, useRef } from 'react'; -import { useTheme } from 'styled-components'; import { Box, Indicator } from 'design'; +import { useTheme } from 'styled-components'; -import * as stores from 'teleport/Console/stores/types'; import { Terminal, TerminalRef } from 'teleport/Console/DocumentSsh/Terminal'; -import { useMfa } from 'teleport/lib/useMfa'; +import * as stores from 'teleport/Console/stores/types'; +import { useMfaTty } from 'teleport/lib/useMfa'; import Document from 'teleport/Console/Document'; import AuthnDialog from 'teleport/components/AuthnDialog'; -import { useDbSession } from './useDbSession'; import { ConnectDialog } from './ConnectDialog'; +import { useDbSession } from './useDbSession'; type Props = { visible: boolean; @@ -38,11 +38,11 @@ type Props = { export function DocumentDb({ doc, visible }: Props) { const terminalRef = useRef(); const { tty, status, closeDocument, sendDbConnectData } = useDbSession(doc); - const mfa = useMfa(tty); + const mfa = useMfaTty(tty); useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.requested, status]); + }, [visible, mfa, status]); const theme = useTheme(); return ( @@ -52,7 +52,7 @@ export function DocumentDb({ doc, visible }: Props) { )} - {mfa.requested && } + {status === 'waiting' && ( Date: Tue, 17 Dec 2024 11:13:16 -0800 Subject: [PATCH 14/20] fix stories. --- .../DesktopSession/DesktopSession.story.tsx | 23 ++++--- .../AuthnDialog/AuthnDialog.story.tsx | 65 ++++++++++--------- .../components/AuthnDialog/AuthnDialog.tsx | 13 +--- 3 files changed, 48 insertions(+), 53 deletions(-) diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.story.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.story.tsx index 97606b1ea3b86..e401ab43de9f1 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.story.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.story.tsx @@ -16,16 +16,16 @@ * along with this program. If not, see . */ -import { useState } from 'react'; import { ButtonPrimary } from 'design/Button'; +import { useState } from 'react'; import { NotificationItem } from 'shared/components/Notification'; import { throttle } from 'shared/utils/highbar'; import { TdpClient, TdpClientEvent } from 'teleport/lib/tdp'; import { makeDefaultMfaState } from 'teleport/lib/useMfa'; -import { State } from './useDesktopSession'; import { DesktopSession } from './DesktopSession'; +import { State } from './useDesktopSession'; export default { title: 'Teleport/DesktopSession', @@ -261,14 +261,17 @@ export const WebAuthnPrompt = () => ( }} wsConnection={{ status: 'open' }} mfa={{ - errorText: '', - requested: true, - setErrorText: () => null, - addMfaToScpUrls: false, - onWebauthnAuthenticate: () => null, - onSsoAuthenticate: () => null, - webauthnPublicKey: null, - ssoChallenge: null, + ...makeDefaultMfaState(), + attempt: { + status: 'processing', + statusText: '', + data: null, + }, + challenge: { + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, + }, }} /> ); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx index a73971043c5b3..fa83076935274 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx @@ -27,23 +27,25 @@ export default { export const LoadedWithMultipleOptions = () => { const props: Props = { ...defaultProps, - mfa: { - ...defaultProps.mfa, - mfaChallenge: { - ssoChallenge: { - redirectUrl: 'hi', - requestId: '123', - channelId: '123', - device: { - connectorId: '123', - connectorType: 'saml', - displayName: 'Okta', - }, - }, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), + attempt: { + status: 'processing', + statusText: '', + data: null, + }, + challenge: { + ssoChallenge: { + redirectUrl: 'hi', + requestId: '123', + channelId: '123', + device: { + connectorId: '123', + connectorType: 'saml', + displayName: 'Okta', }, }, + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, }, }; return ; @@ -52,33 +54,32 @@ export const LoadedWithMultipleOptions = () => { export const LoadedWithSingleOption = () => { const props: Props = { ...defaultProps, - mfa: { - ...defaultProps.mfa, - mfaChallenge: { - webauthnPublicKey: { - challenge: new ArrayBuffer(1), - }, + attempt: { + status: 'processing', + statusText: '', + data: null, + }, + challenge: { + webauthnPublicKey: { + challenge: new ArrayBuffer(1), }, }, }; return ; }; -export const Error = () => { +export const LoadedWithError = () => { + const err = new Error('Something went wrong'); const props: Props = { ...defaultProps, - mfa: { - ...defaultProps.mfa, - submitAttempt: { - status: 'failed', - statusText: 'Something went wrong', - }, + attempt: { + status: 'error', + statusText: err.message, + error: err, + data: null, }, }; return ; }; -const defaultProps: Props = { - mfa: makeDefaultMfaState(), - onCancel: () => null, -}; +const defaultProps: Props = makeDefaultMfaState(); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index 3bb5ea4442838..3b28751f46af8 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -25,11 +25,9 @@ import { ButtonIcon, ButtonSecondary, Flex, H2, Text } from 'design'; import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; -import { Attempt } from 'shared/hooks/useAsync'; import { MfaState } from 'teleport/lib/useMfa'; -import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; -type AuthnDialogProps = MfaState & { +export type Props = MfaState & { replaceErrorText?: string; }; @@ -40,7 +38,7 @@ export default function AuthnDialog({ attempt, resetAttempt, replaceErrorText, -}: AuthnDialogProps) { +}: Props) { // Only show the dialog when we are in a processing or error state. if (attempt.status === '' || attempt.status === 'success') return; @@ -110,10 +108,3 @@ export default function AuthnDialog({
); } - -export type Props = { - mfaChallenge: MfaAuthenticateChallenge; - submitMfa: (mfaType?: DeviceType) => Promise; - submitAttempt: Attempt; - onCancel: () => void; -}; From 1921b953f2d7c3830224f2a98002acc2a1e142b0 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 17 Dec 2024 12:39:56 -0800 Subject: [PATCH 15/20] Refactor mfa required logic. --- web/packages/teleport/src/lib/useMfa.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index d335608dc5b51..7a5107192f6b2 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -63,14 +63,12 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { - if (!challenge) { - if (mfaRequired === false) return; + if (mfaRequired === false) return; - challenge = await auth.getMfaChallenge(req); - if (!challenge) { - setMfaRequired(false); - return; - } + challenge = challenge ? challenge : await auth.getMfaChallenge(req); + if (!challenge) { + setMfaRequired(false); + return; } // Set mfa requirement and options after we get a challenge for the first time. From c96e18b414d73dec404ddb3bc4084d9e250cb07d Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 18 Dec 2024 12:45:53 -0800 Subject: [PATCH 16/20] Address bl-nero's comments. --- lib/client/weblogin.go | 9 -------- .../components/AuthnDialog/AuthnDialog.tsx | 9 ++++---- web/packages/teleport/src/lib/useMfa.ts | 23 +++++++++++++++---- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 9821250d5b8b7..6eb16df268b9c 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -163,15 +163,6 @@ func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthe }}, nil } - if r.SSOResponse != nil { - return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_SSO{ - SSO: &proto.SSOResponse{ - RequestId: r.SSOResponse.RequestID, - Token: r.SSOResponse.Token, - }, - }}, nil - } - return nil, nil } diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index 3b28751f46af8..c43e9e5c68040 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -26,6 +26,7 @@ import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; import { MfaState } from 'teleport/lib/useMfa'; +import { MFA_OPTION_TOTP } from 'teleport/services/mfa'; export type Props = MfaState & { replaceErrorText?: string; @@ -39,11 +40,11 @@ export default function AuthnDialog({ resetAttempt, replaceErrorText, }: Props) { - // Only show the dialog when we are in a processing or error state. - if (attempt.status === '' || attempt.status === 'success') return; + if (!challenge && attempt.status !== 'error') return; // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. - const onlyTotpAvailable = options?.length === 1 && challenge?.totpChallenge; + const onlyTotpAvailable = + options?.length === 1 && options[0] === MFA_OPTION_TOTP; return ( ({ width: '400px' })} open={true}> @@ -57,7 +58,7 @@ export default function AuthnDialog({ {onlyTotpAvailable && ( { - 'totp is not currently supported for this action, please register a security key to continue' + 'Authenticator app is not currently supported for this action, please register a passkey or a security key to continue.' } )} diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 7a5107192f6b2..39c999df3c162 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -39,11 +39,12 @@ export type MfaProps = { isMfaRequired?: boolean | null; }; -// useMfa prepares an MFA state designed for use with AuthnDialog. -// 1. Call getMfaChallengeResponse() where MFA may be required. -// If MFA is not required, it should be a noop, returning null. -// 2. Setup AuthnDialog using the MFA state, to show a dialog when -// mfaAttempt.state === 'processing'. +/** + * Use the returned object to request MFA checks with a shared state. + * When MFA authentication is in progress, the object's properties can + * be used to display options to the user and prompt for them to complete + * the MFA check. + */ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [mfaRequired, setMfaRequired] = useState(); const [options, setMfaOptions] = useState(); @@ -60,9 +61,19 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { setMfaRequired(null); }, [req?.isMfaRequiredRequest]); + // getResponse is used to initiate MFA authentication. + // 1. Check if MFA is required by getting a new MFA challenge + // 2. If MFA is required, set the challenge in the MFA state and wait for it to + // be resolved by the caller. + // 3. The caller sees the mfa challenge set in state and submits an mfa response + // request with arguments provided by the user (mfa type, otp code). + // 4. Receive the mfa response through the mfaResponsePromise ref and return it. + // + // The caller should also display errors seen in attempt. const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { + // If a previous call determined that MFA is not required, this is a noop. if (mfaRequired === false) return; challenge = challenge ? challenge : await auth.getMfaChallenge(req); @@ -75,6 +86,8 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { if (!mfaRequired) setMfaRequired(true); if (!options) setMfaOptions(getMfaChallengeOptions(challenge)); + // Prepare a new promise to collect the mfa response retrieved + // through the submit function. mfaResponsePromise.current = Promise.withResolvers(); setMfaChallenge(challenge); try { From c1438ba2025e06eff78ecea67de9d8ef17ab43d6 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 18 Dec 2024 13:29:19 -0800 Subject: [PATCH 17/20] Address Ryan's comments. --- lib/client/weblogin.go | 1 + .../teleport/src/AppLauncher/AppLauncher.tsx | 2 +- .../src/Console/DocumentDb/DocumentDb.tsx | 2 +- .../DocumentKubeExec/DocumentKubeExec.tsx | 8 +------- .../src/Console/DocumentSsh/DocumentSsh.tsx | 10 ++-------- .../src/DesktopSession/DesktopSession.tsx | 5 +---- .../AuthnDialog/AuthnDialog.test.tsx | 10 +++++----- .../components/AuthnDialog/AuthnDialog.tsx | 19 ++++++++++++------- web/packages/teleport/src/lib/useMfa.ts | 13 ++++++++----- 9 files changed, 32 insertions(+), 38 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 6eb16df268b9c..c3415e340417d 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -166,6 +166,7 @@ func (r *MFAChallengeResponse) GetOptionalMFAResponseProtoReq() (*proto.MFAAuthe return nil, nil } +// ParseMFAChallengeResponse parses [MFAChallengeResponse] from JSON and returns it as a [proto.MFAAuthenticateResponse]. func ParseMFAChallengeResponse(mfaResponseJSON []byte) (*proto.MFAAuthenticateResponse, error) { var resp MFAChallengeResponse if err := json.Unmarshal(mfaResponseJSON, &resp); err != nil { diff --git a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx index 39f2911dd241f..78db3d6733f2d 100644 --- a/web/packages/teleport/src/AppLauncher/AppLauncher.tsx +++ b/web/packages/teleport/src/AppLauncher/AppLauncher.tsx @@ -168,7 +168,7 @@ export function AppLauncher() { ) : ( )} - + ); } diff --git a/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx b/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx index 5f166c8570dfb..6c024edfe7331 100644 --- a/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx +++ b/web/packages/teleport/src/Console/DocumentDb/DocumentDb.tsx @@ -52,7 +52,7 @@ export function DocumentDb({ doc, visible }: Props) { )} - + {status === 'waiting' && ( )} - { - mfa.resetAttempt(); - closeDocument(); - }} - /> + {status === 'waiting-for-exec-data' && ( diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index 7d3c42cec8a23..4902d90845bf1 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -145,14 +145,8 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - { - ttyMfa.resetAttempt(); - closeDocument(); - }} - /> - + + {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index 42bf5ceda4018..851c72b769fe4 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -184,13 +184,10 @@ export function DesktopSession(props: State) { const MfaDialog = ({ mfa }: { mfa: MfaState }) => { return ( { - mfa.resetAttempt(); - }} /> ); }; diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx index 88df5a52991bf..34be98660bc39 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx @@ -61,7 +61,7 @@ describe('AuthnDialog', () => { data: null, }, }); - render(); + render(); expect(screen.getByText('Verify Your Identity')).toBeInTheDocument(); expect( @@ -87,7 +87,7 @@ describe('AuthnDialog', () => { data: null, }, }); - render(); + render(); expect(screen.getByText('Verify Your Identity')).toBeInTheDocument(); expect( @@ -110,7 +110,7 @@ describe('AuthnDialog', () => { error: new Error(errorText), }, }); - render(); + render(); expect(screen.getByTestId('danger-alert')).toBeInTheDocument(); expect(screen.getByText(errorText)).toBeInTheDocument(); @@ -128,7 +128,7 @@ describe('AuthnDialog', () => { }, submit: jest.fn(), }); - render(); + render(); const ssoButton = screen.getByText('Okta'); fireEvent.click(ssoButton); expect(mfa.submit).toHaveBeenCalledTimes(1); @@ -146,7 +146,7 @@ describe('AuthnDialog', () => { }, submit: jest.fn(), }); - render(); + render(); const webauthn = screen.getByText('Passkey or MFA Device'); fireEvent.click(webauthn); expect(mfa.submit).toHaveBeenCalledTimes(1); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index c43e9e5c68040..1b862601beaf1 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -28,17 +28,16 @@ import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; import { MfaState } from 'teleport/lib/useMfa'; import { MFA_OPTION_TOTP } from 'teleport/services/mfa'; -export type Props = MfaState & { +export type Props = { + mfaState: MfaState; replaceErrorText?: string; + onClose?: () => void; }; export default function AuthnDialog({ - options, - challenge, - submit, - attempt, - resetAttempt, + mfaState: { options, challenge, submit, attempt, resetAttempt }, replaceErrorText, + onClose, }: Props) { if (!challenge && attempt.status !== 'error') return; @@ -50,7 +49,13 @@ export default function AuthnDialog({ ({ width: '400px' })} open={true}>

Verify Your Identity

- + { + resetAttempt(); + onClose(); + }} + > diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 39c999df3c162..ed0b2bf530161 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -108,11 +108,14 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { setMfaAttempt(makeEmptyAttempt()); }; - const getChallengeResponse = async (challenge?: MfaAuthenticateChallenge) => { - const [resp, err] = await getResponse(challenge); - if (err) throw err; - return resp; - }; + const getChallengeResponse = useCallback( + async (challenge?: MfaAuthenticateChallenge) => { + const [resp, err] = await getResponse(challenge); + if (err) throw err; + return resp; + }, + [getResponse] + ); const submit = useCallback( async (mfaType?: DeviceType, totpCode?: string) => { From d3d473dec421f640cdb02ee6df2acb7c3fff82e0 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 18 Dec 2024 18:26:41 -0800 Subject: [PATCH 18/20] Add useMfa unit test. --- web/packages/teleport/src/lib/useMfa.test.tsx | 205 ++++++++++++++++++ web/packages/teleport/src/lib/useMfa.ts | 7 +- 2 files changed, 207 insertions(+), 5 deletions(-) create mode 100644 web/packages/teleport/src/lib/useMfa.test.tsx diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx new file mode 100644 index 0000000000000..9d2516808f107 --- /dev/null +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -0,0 +1,205 @@ +/** + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; + +import { renderHook, waitFor } from '@testing-library/react'; +import { useState } from 'react'; + +import { CreateAuthenticateChallengeRequest } from 'teleport/services/auth'; +import { + MFA_OPTION_WEBAUTHN, + MfaAuthenticateChallenge, + MfaChallengeResponse, +} from 'teleport/services/mfa'; + +import { useMfa } from './useMfa'; + +const mockChallenge: MfaAuthenticateChallenge = { + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, +}; + +const mockResponse: MfaChallengeResponse = { + webauthn_response: { + id: 'cred-id', + type: 'public-key', + extensions: { + appid: true, + }, + rawId: 'rawId', + response: { + authenticatorData: 'authenticatorData', + clientDataJSON: 'clientDataJSON', + signature: 'signature', + userHandle: 'userHandle', + }, + }, +}; + +const mockChallengeReq: CreateAuthenticateChallengeRequest = { + scope: MfaChallengeScope.USER_SESSION, + isMfaRequiredRequest: { + node: { + node_name: 'node', + login: 'login', + }, + }, +}; + +describe('useMfa', () => { + beforeEach(() => jest.spyOn(console, 'error').mockImplementation()); + + test('mfa required', async () => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge); + jest + .spyOn(auth, 'getMfaChallengeResponse') + .mockResolvedValueOnce(mockResponse); + const { result: mfa } = renderHook(() => + useMfa({ + req: mockChallengeReq, + }) + ); + + const respPromise = mfa.current.getChallengeResponse(); + await waitFor(() => { + expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + }); + + expect(mfa.current.options).toEqual([MFA_OPTION_WEBAUTHN]); + expect(mfa.current.required).toEqual(true); + expect(mfa.current.challenge).toEqual(mockChallenge); + expect(mfa.current.attempt.status).toEqual('processing'); + + await mfa.current.submit('webauthn'); + await waitFor(() => { + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + mockChallenge, + 'webauthn', + undefined + ); + }); + + const resp = await respPromise; + expect(resp).toEqual(mockResponse); + expect(mfa.current.challenge).toEqual(null); + expect(mfa.current.attempt.status).toEqual('success'); + }); + + test('mfa not required', async () => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue(null); + + const { result: mfa } = renderHook(() => + useMfa({ + req: mockChallengeReq, + }) + ); + + // If a challenge is not returned, an empty mfa response should be returned + // early and the requirement changed to false for future calls. + const resp = await mfa.current.getChallengeResponse(); + expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + expect(resp).toEqual(undefined); + await waitFor(() => expect(mfa.current.required).toEqual(false)); + }); + + test('adaptable mfa requirement state', async () => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue(null); + + let isMfaRequired: boolean; + let setMfaRequired: (b: boolean) => void; + + let req: CreateAuthenticateChallengeRequest; + let setReq: (r: CreateAuthenticateChallengeRequest) => void; + + const { result: mfa } = renderHook(() => { + [isMfaRequired, setMfaRequired] = useState(null); + [req, setReq] = + useState(mockChallengeReq); + + return useMfa({ + req: req, + isMfaRequired: isMfaRequired, + }); + }); + + // mfaRequired should change when the isMfaRequired arg changes, allowing + // callers to propagate mfa required late (e.g. per-session MFA for file transfers) + setMfaRequired(false); + await waitFor(() => expect(mfa.current.required).toEqual(false)); + + setMfaRequired(true); + await waitFor(() => expect(mfa.current.required).toEqual(true)); + + setMfaRequired(null); + await waitFor(() => expect(mfa.current.required).toEqual(null)); + + // If isMfaRequiredRequest changes, the mfaRequired value should be reset. + setReq({ + ...mockChallengeReq, + isMfaRequiredRequest: { + admin_action: {}, + }, + }); + await waitFor(() => expect(mfa.current.required).toEqual(null)); + }); + + test('mfa challenge error', async () => { + const err = new Error('an error has occurred'); + jest.spyOn(auth, 'getMfaChallenge').mockImplementation(() => { + throw err; + }); + + const { result: mfa } = renderHook(() => useMfa({})); + + await expect(mfa.current.getChallengeResponse).rejects.toThrow(err); + await waitFor(() => { + expect(mfa.current.attempt).toEqual({ + status: 'error', + statusText: err.message, + error: err, + data: null, + }); + }); + }); + + test('mfa response error', async () => { + const err = new Error('an error has occurred'); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge); + jest.spyOn(auth, 'getMfaChallengeResponse').mockImplementation(async () => { + throw err; + }); + + const { result: mfa } = renderHook(() => useMfa({})); + + const respPromise = mfa.current.getChallengeResponse(); + await waitFor(() => { + expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + }); + await mfa.current.submit('webauthn'); + + await expect(respPromise).rejects.toThrow(err); + await waitFor(() => { + expect(mfa.current.attempt).toEqual({ + status: 'error', + statusText: err.message, + error: err, + data: null, + }); + }); + }); +}); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index ed0b2bf530161..91c080a34e3bd 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -120,12 +120,9 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const submit = useCallback( async (mfaType?: DeviceType, totpCode?: string) => { try { - const resp = await auth.getMfaChallengeResponse( - challenge, - mfaType, - totpCode + await mfaResponsePromise.current.resolve( + auth.getMfaChallengeResponse(challenge, mfaType, totpCode) ); - mfaResponsePromise.current.resolve(resp); } catch (err) { setMfaAttempt({ data: null, From 353ffe983a8f5a082785cb701e9fe17c1e4760cd Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 18 Dec 2024 18:59:59 -0800 Subject: [PATCH 19/20] Fix story lint. --- .../AuthnDialog/AuthnDialog.story.tsx | 74 ++++++++++--------- web/packages/teleport/src/lib/useMfa.ts | 2 +- 2 files changed, 40 insertions(+), 36 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx index fa83076935274..7137b983a4d23 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.story.tsx @@ -26,25 +26,27 @@ export default { export const LoadedWithMultipleOptions = () => { const props: Props = { - ...defaultProps, - attempt: { - status: 'processing', - statusText: '', - data: null, - }, - challenge: { - ssoChallenge: { - redirectUrl: 'hi', - requestId: '123', - channelId: '123', - device: { - connectorId: '123', - connectorType: 'saml', - displayName: 'Okta', - }, + mfaState: { + ...makeDefaultMfaState(), + attempt: { + status: 'processing', + statusText: '', + data: null, }, - webauthnPublicKey: { - challenge: new ArrayBuffer(1), + challenge: { + ssoChallenge: { + redirectUrl: 'hi', + requestId: '123', + channelId: '123', + device: { + connectorId: '123', + connectorType: 'saml', + displayName: 'Okta', + }, + }, + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, }, }, }; @@ -53,15 +55,17 @@ export const LoadedWithMultipleOptions = () => { export const LoadedWithSingleOption = () => { const props: Props = { - ...defaultProps, - attempt: { - status: 'processing', - statusText: '', - data: null, - }, - challenge: { - webauthnPublicKey: { - challenge: new ArrayBuffer(1), + mfaState: { + ...makeDefaultMfaState(), + attempt: { + status: 'processing', + statusText: '', + data: null, + }, + challenge: { + webauthnPublicKey: { + challenge: new ArrayBuffer(1), + }, }, }, }; @@ -71,15 +75,15 @@ export const LoadedWithSingleOption = () => { export const LoadedWithError = () => { const err = new Error('Something went wrong'); const props: Props = { - ...defaultProps, - attempt: { - status: 'error', - statusText: err.message, - error: err, - data: null, + mfaState: { + ...makeDefaultMfaState(), + attempt: { + status: 'error', + statusText: err.message, + error: err, + data: null, + }, }, }; return ; }; - -const defaultProps: Props = makeDefaultMfaState(); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 91c080a34e3bd..262b9eb9ac10a 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -121,7 +121,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { async (mfaType?: DeviceType, totpCode?: string) => { try { await mfaResponsePromise.current.resolve( - auth.getMfaChallengeResponse(challenge, mfaType, totpCode) + await auth.getMfaChallengeResponse(challenge, mfaType, totpCode) ); } catch (err) { setMfaAttempt({ From e103a1fd0f718323ad1df74ee3a9d6e6b293e324 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 19 Dec 2024 12:25:04 -0800 Subject: [PATCH 20/20] Replace Promise.withResolvers for compatiblity with older browers; Fix bug where MFA couldn't be retried after a failed attempt; Add extra tests. --- web/packages/teleport/src/lib/useMfa.test.tsx | 47 +++++++++++++++++-- web/packages/teleport/src/lib/useMfa.ts | 47 ++++++++++++++----- 2 files changed, 80 insertions(+), 14 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index 9d2516808f107..886fbff25a662 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -62,7 +62,13 @@ const mockChallengeReq: CreateAuthenticateChallengeRequest = { }; describe('useMfa', () => { - beforeEach(() => jest.spyOn(console, 'error').mockImplementation()); + beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); test('mfa required', async () => { jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge); @@ -184,7 +190,11 @@ describe('useMfa', () => { throw err; }); - const { result: mfa } = renderHook(() => useMfa({})); + const { result: mfa } = renderHook(() => + useMfa({ + req: mockChallengeReq, + }) + ); const respPromise = mfa.current.getChallengeResponse(); await waitFor(() => { @@ -192,7 +202,6 @@ describe('useMfa', () => { }); await mfa.current.submit('webauthn'); - await expect(respPromise).rejects.toThrow(err); await waitFor(() => { expect(mfa.current.attempt).toEqual({ status: 'error', @@ -201,5 +210,37 @@ describe('useMfa', () => { data: null, }); }); + + // After an error, the mfa response promise remains in an unresolved state, + // allowing for retries. + jest + .spyOn(auth, 'getMfaChallengeResponse') + .mockResolvedValueOnce(mockResponse); + await mfa.current.submit('webauthn'); + expect(await respPromise).toEqual(mockResponse); + }); + + test('reset mfa attempt', async () => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue(mockChallenge); + const { result: mfa } = renderHook(() => + useMfa({ + req: mockChallengeReq, + }) + ); + + const respPromise = mfa.current.getChallengeResponse(); + await waitFor(() => { + expect(auth.getMfaChallenge).toHaveBeenCalled(); + }); + + mfa.current.resetAttempt(); + + await expect(respPromise).rejects.toThrow( + new Error('MFA attempt cancelled by user') + ); + + await waitFor(() => { + expect(mfa.current.attempt.status).toEqual('error'); + }); }); }); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 262b9eb9ac10a..50f852c3768e3 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -39,6 +39,12 @@ export type MfaProps = { isMfaRequired?: boolean | null; }; +type mfaResponsePromiseWithResolvers = { + promise: Promise; + resolve: (v: MfaChallengeResponse) => void; + reject: (v?: any) => void; +}; + /** * Use the returned object to request MFA checks with a shared state. * When MFA authentication is in progress, the object's properties can @@ -48,10 +54,10 @@ export type MfaProps = { export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [mfaRequired, setMfaRequired] = useState(); const [options, setMfaOptions] = useState(); - const [challenge, setMfaChallenge] = useState(); - const mfaResponsePromise = - useRef>(); + + const mfaResponsePromiseWithResolvers = + useRef(); useEffect(() => { setMfaRequired(isMfaRequired); @@ -70,6 +76,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { // 4. Receive the mfa response through the mfaResponsePromise ref and return it. // // The caller should also display errors seen in attempt. + const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { @@ -88,22 +95,36 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { // Prepare a new promise to collect the mfa response retrieved // through the submit function. - mfaResponsePromise.current = Promise.withResolvers(); + let resolve, reject; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + + mfaResponsePromiseWithResolvers.current = { + promise, + resolve, + reject, + }; + setMfaChallenge(challenge); try { - return await mfaResponsePromise.current.promise; + return await promise; } finally { - mfaResponsePromise.current = null; + mfaResponsePromiseWithResolvers.current = null; setMfaChallenge(null); } }, - [req, mfaResponsePromise, options, mfaRequired] + [req, mfaResponsePromiseWithResolvers, options, mfaRequired] ) ); const resetAttempt = () => { - if (mfaResponsePromise.current) mfaResponsePromise.current.reject(); - mfaResponsePromise.current = null; + if (mfaResponsePromiseWithResolvers.current) + mfaResponsePromiseWithResolvers.current.reject( + new Error('MFA attempt cancelled by user') + ); + mfaResponsePromiseWithResolvers.current = null; setMfaChallenge(null); setMfaAttempt(makeEmptyAttempt()); }; @@ -119,8 +140,12 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const submit = useCallback( async (mfaType?: DeviceType, totpCode?: string) => { + if (!mfaResponsePromiseWithResolvers.current) { + throw new Error('submit called without an in flight MFA attempt'); + } + try { - await mfaResponsePromise.current.resolve( + await mfaResponsePromiseWithResolvers.current.resolve( await auth.getMfaChallengeResponse(challenge, mfaType, totpCode) ); } catch (err) { @@ -132,7 +157,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { }); } }, - [challenge, mfaResponsePromise, setMfaAttempt] + [challenge, mfaResponsePromiseWithResolvers, setMfaAttempt] ); return {