Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teleport Connect MFA prompt #34135

Merged
merged 8 commits into from
Dec 1, 2023
Merged

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Nov 1, 2023

Add a tsh daemon MFA prompt to replace the standard CLI-based MFA prompt. By using this prompt in the daemon's TeleportClients, MFA prompts like that for admin actions will issue a tshdEventService message promptMFA instead of just outputting the tap prompt to Stderr. This also allows Connect to support TOTP prompts.

This change is intended to be a no-op (continue using CLI-based prompt) until the client-side is implemented with a Webauth/TOTP prompt modal in a follow up PR.

Relies on #34087

@Joerger Joerger mentioned this pull request Nov 2, 2023
14 tasks
@ravicious ravicious self-requested a review November 2, 2023 09:19
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

The Connect implementation should work. I assume that we're going to open an important modal to ask either for a key tap or for TOTP.

The only scenario where this would not work is if we tried to execute an administrative action from within an important modal. Imagine this convoluted-on-purpose scenario where the user clicks "Change username" and it shows an important modal with an input to enter a new name. Then when they submit the form, tshd prompts for MFA using an important modal. But tshd cannot do that as there's already an important modal that is open! This is a problem similar to #31070.

Fortunately, AFAIK the only administrative actions that can be executed from Connect are access request approvals and setting up Connect My Computer (upsert role + update user + create token) and none of them use a modal.

Comment on lines 59 to 63
return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_TOTP{
TOTP: &proto.TOTPResponse{Code: resp.TotpCode},
},
}, nil
Copy link
Member

Choose a reason for hiding this comment

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

The final version will have to pass Webauthn responses as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it's not necessary. The flow I laid out here is:

sequenceDiagram
    participant tshd as tsh daemon
    participant electron as Electron app
    actor user
    tshd->>electron: PrompMFA req
    electron->>user: show MFA modal
    
    alt user enters otp
        user->>electron: otp code
        electron->>tshd: otp code
    else user closes modal
        user->>electron: close modal
        electron->>tshd: cancel request
    else user completes webauthn
        user->>tshd: user taps webauthn key
        tshd->>electron: close modal (no error)
    end
Loading

Previously I had trouble handling tshd cancelling its request without outputting an error in the Electron App, but I'm going to try again. Otherwise we would need to add another call from Electron -> tshd to handle Webauthn, which is similar to how we handled headless. The extra steps in the headless flow made more sense whereas I think we should try to get away with the short flow here.

Copy link
Member

Choose a reason for hiding this comment

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

Cancel request and close modal is where my subsequent questions would be. How do you plan to implement "close modal"?

Close modal

It seems to me that it might be worthwhile to tie this logic to the request-response cycle. In that sense, closing the modal by tsh could be implemented by cancelling the request. I'm not sure what you mean by cancelling a request without outputting an error in the Electron app, but AFAIR the relogin logic already handles request cancellation.

this.subscribeToTshdEvent('relogin', ({ request, onCancelled }) => {
// The handler for the relogin event should return only after the relogin procedure finishes.
return this.reloginService.relogin(
request as ReloginRequest,
onCancelled
);
});

This is hidden behind some layers of abstraction but essentially when the Go code cancels the request, grpc-js emits the cancelled event on the call object.

const service: apiService.ITshdEventsServiceServer = {
relogin: (call, callback) => {
const request = call.request.toObject();
logger.info('Emitting relogin', request);
const onCancelled = (callback: () => void) => {
call.on('cancelled', callback);
};
emitter.emit('relogin', { request, onCancelled }).then(

Cancel request

Here I suppose the tshd events service could return a response with code ABORTED. I haven't done that anywhere yet, but taking the relogin service just as an example, it'd have to look something like this:

diff --git a/web/packages/teleterm/src/services/tshdEvents/index.ts b/web/packages/teleterm/src/services/tshdEvents/index.ts
index 8c98badf3e..8a1831e890 100644
--- a/web/packages/teleterm/src/services/tshdEvents/index.ts
+++ b/web/packages/teleterm/src/services/tshdEvents/index.ts
@@ -163,6 +163,18 @@ function createService(logger: Logger): {
           callback(null, new api.ReloginResponse());
         },
         error => {
+          // error passes through the context bridge [1] so none of its properties are preserved
+          // other than the message.
+          //
+          // [1] https://www.electronjs.org/docs/latest/api/context-bridge
+          if (error?.message?.includes('aborted')) {
+            const status = new grpc.StatusBuilder()
+              .withCode(grpc.status.ABORTED)
+              .build();
+            callback(status);
+            return;
+          }
+
           callback(error);
         }
       );
diff --git a/web/packages/teleterm/src/ui/services/relogin/reloginService.ts b/web/packages/teleterm/src/ui/services/relogin/reloginService.ts
index 06aa001018..6036f1bcd6 100644
--- a/web/packages/teleterm/src/ui/services/relogin/reloginService.ts
+++ b/web/packages/teleterm/src/ui/services/relogin/reloginService.ts
@@ -60,6 +60,8 @@ export class ReloginService {
           reject(new Error('Login process was canceled by the user')),
       });
 
+      throw new Error('aborted');
+
       onRequestCancelled(closeDialog);
     });
   }

@Joerger
Copy link
Contributor Author

Joerger commented Nov 2, 2023

The Connect implementation should work. I assume that we're going to open an important modal to ask either for a key tap or for TOTP.

👍

The only scenario where this would not work is if we tried to execute an administrative action from within an important modal. Imagine this convoluted-on-purpose scenario where the user clicks "Change username" and it shows an important modal with an input to enter a new name. Then when they submit the form, tshd prompts for MFA using an important modal. But tshd cannot do that as there's already an important modal that is open! This is a problem similar to #31070.

Fortunately, AFAIK the only administrative actions that can be executed from Connect are access request approvals and setting up Connect My Computer (upsert role + update user + create token) and none of them use a modal.

Good point, this is something we'll need to keep an eye out for in the future. For now, I'll make it so the tshd prompt (yubikey blinking) goes through while tshdEventService.promptMFA waits for the importantModalMu. This will essentially allow Webauthn to continue working without the Modal, though TOTP will not (no regression).

@Joerger Joerger force-pushed the joerger/teleport-connect-retry-with-mfa branch 2 times, most recently from dad0cf6 to ebe3f08 Compare November 5, 2023 17:03
@Joerger Joerger marked this pull request as ready for review November 6, 2023 22:08
Copy link

github-actions bot commented Nov 6, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions github-actions bot added desktop-access size/md tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui labels Nov 6, 2023
@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Nov 9, 2023
@Joerger Joerger marked this pull request as draft November 17, 2023 00:49
@Joerger Joerger force-pushed the joerger/teleport-connect-retry-with-mfa branch from ebe3f08 to 938b0ee Compare November 17, 2023 00:51
@Joerger Joerger marked this pull request as ready for review November 17, 2023 00:51
@github-actions github-actions bot requested review from kimlisa and rudream November 17, 2023 00:51
lib/client/api.go Outdated Show resolved Hide resolved
@ravicious ravicious self-requested a review November 21, 2023 14:04
lib/teleterm/daemon/mfaPrompt.go Outdated Show resolved Hide resolved
@Joerger Joerger force-pushed the joerger/teleport-connect-retry-with-mfa branch from 10f039a to b89cc01 Compare November 28, 2023 03:39
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from rudream November 28, 2023 03:39
@Joerger Joerger enabled auto-merge November 28, 2023 03:40
@Joerger Joerger force-pushed the joerger/teleport-connect-retry-with-mfa branch from d2e82ce to 363472d Compare November 28, 2023 21:34
@Joerger Joerger added this pull request to the merge queue Dec 1, 2023
Merged via the queue into master with commit 704f3fc Dec 1, 2023
40 checks passed
@Joerger Joerger deleted the joerger/teleport-connect-retry-with-mfa branch December 1, 2023 23:03
@Joerger Joerger mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop-access no-changelog Indicates that a PR does not require a changelog entry size/md tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants