[web] Add mfa check to web scp and perform ceremony by avatus · Pull Request #22938 · gravitational/teleport

avatus · 2023-03-10T22:41:12Z

Backend buddy #22864

This will check if MFA is required when making a file transfer request and, if required, challenge and send the assertion with the actual file transfer request.

zmb3 · 2023-03-13T21:50:59Z

-              getHttpFileTransferHandlers().download(
+            getDownloader: async (location, abortController) => {
+              const wanResponse = await auth.getAssertionResponseIfRequired({
+                node: { node_name: doc.clusterId, login: doc.login },


Setting the node name field to a variable called "cluster ID" seems suspicious - are you sure that's what we want?

Good catch on this one. I was hitting the first check of "MFA check always" and so it wasn't getting to the node check on the backend. Changed to serverId and updated my local config to reflect this check and working as intended again.

ravicious

I tested this on Firefox with a yubikey, on Chrome with a yubikey and TouchID and on Safari with a yubikey. It works as expected except on Safari when I'm trying to upload something.

Other than that I submitted a couple of nits and a design suggestion which might be too big to take care of at the moment. You might have considered it already but I didn't follow the discussion about this feature. I don't like making the users pay the extra cost of an additional request even if they're not using the feature. 😶‍🌫️

ravicious · 2023-03-14T14:17:19Z

I can't get upload to work on Safari. I noticed that on Safari I also cannot edit the path and then tab to the file input and press Enter like in other browsers, I wonder if this has something to do with the error that's being printed out in the console.

safari-upload.mov

I didn't get to this comment until after my other changes. After making my changes I tried to reproduce this and I could not. Safari works fine for me. Can you try again with the new commits? (not sure why they would fix this but worth a shot?) https://gyazo.com/e06a01f46ba0b89c9d9f128abfd2de89

I still get it but in a different form. I'm on Safari 16.3 (18614.4.6.1.6).

I remember that Isaiah was also encountering this error with directory sharing back in the day.

SharedDirectoryAnnounce webapps#960

SharedDirectoryWriteResponse webapps#1008 (comment)

safari-upload-2.mov

I can't reproduce still, even with exact path. Can someone else give it a shot on safari? Seems the conversations in the linked PRs sort of go no where regarding this issue

i checked this branch out and ran locally, and upload works for me on safari. typing a file path and pressing enter also works for me (opens file browser for me to select files)

Version 16.3 (18614.4.6.1.6), mac m1

ravicious · 2023-03-14T14:23:09Z

+    if (isMfaRequired.required === true) {
+      return auth.getWebauthnResponse();
+    }
+    return;


Nit:

Suggested change

if (isMfaRequired.required === true) {

return auth.getWebauthnResponse();

}

return;

if (isMfaRequired.required) {

return auth.getWebauthnResponse();

}

The second return is not necessary, is it?

ravicious · 2023-03-14T14:29:12Z

-            getDownloader: async (location, abortController) =>
-              getHttpFileTransferHandlers().download(
+            getDownloader: async (location, abortController) => {
+              const wanResponse = await auth.getAssertionResponseIfRequired({


It might be too late to suggest this, but this is going to send an additional request every time, even if I never intend to use per-session MFA, right?

I wonder if it would be possible to get the information about MFA in some other way, perhaps as a return value to the function call which starts the SSH session and then pass it down to places which need to check whether MFA is required or not.

there is another place where we use similar technique (for testing connections to a resource). i thought about this too and mentioned it but no one seemed to mind (no replies), so i figured maybe it was a pre-optimization question?

that said, what do you think of this approach (it's similar to the way we handle it for web terminal):

try to connect

if an error returns, if the trace error is of type "access denied", check if MFA is required

if MFA is required, send back a 200 response with some flag set that says we need users mfa creds

if not, send back an error

For this specific PR, I check if MFA is required and if they've sent an assertion response here. I could just send back an access denied + challenge to solve if thats the case. Not sure how it'd work with discovery

I believe @rosstimothy did something similar for SSH - assume MFA is not required, if you get access denied, then check whether MFA is required and try again.

This way, most attempts don't incur any extra latency when MFA is not required.

https://github.com/gravitational/teleport/blob/master/rfd/0093-offline-access.md

I wonder if it would be possible to get the information about MFA in some other way, perhaps as a return value to the function call which starts the SSH session and then pass it down to places which need to check whether MFA is required or not.

After doing some tinkering, I think I prefer the original suggestion from @ravicious . I think the best place to make this happen is from the useWebauthn hook. There is a listener for TermEvent.WEBAUTHN_CHALLENGE which would happen on the SSH session init. We could set key in state like mfaRequired which would be set to true when we get that message and rely on that. The only issue I can see is if somehow a resource doesnt require MFA anymore (I don't think this could happen) and we send a challenge assertion with a request that doesn't need it. It'll be skipped anyway on the backend if not required so this doesn't prevent anything. Plus this is a turbo edge case that I'm not even sure is an edge case.

If we already knew that mfa was required on init, then we just make the decision of sending an assertion param on the client

Proposed flow:
No MFA required = file request
MFA required = challenge request -> file request

the "double check" flow
no MFA required = file request
MFA required = file request -> challenge request -> file request

My first thought was "just send a challenge in an access denied response body" but our api server doesn't give us access to the body. so I think the proposed solution is best.

I'm not an expert on this but this flow looks better to me than the previous one.

ravicious · 2023-03-15T13:59:44Z

+    // as the existence of this param is what will issue certs
+    return `${path}&webauthn=${JSON.stringify({
+      webauthnAssertionResponse: webauthn,
+    })}`;


Sorry, I missed this completely when I was reviewing the PR earlier. Instead of doing this by hand we could append webauthn to the params passed to generatePath only when webauthn is not undefined, couldn't we?

If we did it that way, we'd have to remove any extra string information in the original scp string template, as we aren't just passing the param here, but in a specific shape to help unmarshal on the backend. I think either way we'd get a bit messy and this way make it clearer imo

ravicious · 2023-03-15T14:01:20Z

+  }
+
+  return {
+    mfaRequired,


As far as I can tell, this returned property is not used anywhere and is accepted as an argument to useMfaFileTransfer, which suggests that even if it was used somewhere it could've been provided from a different source without having to pass through this hook.

This doesn't need to be exported. removed.

ravicious · 2023-03-15T14:04:08Z

      xhr.send();
      return eventEmitter;
    },
+    async getWebauthnResponse(


getWebauthnResponse doesn't seem to be used anywhere.

Vestigial, and removed

ravicious · 2023-03-15T14:07:10Z

  const refTerminal = useRef<Terminal>();
  const { tty, status, closeDocument } = useSshSession(doc);
  const webauthn = useWebAuthn(tty);
+  const { getUrlWithMfa, attempt } = useMfaFileTransfer(webauthn.mfaRequired);


I'd rename attempt either inside useMfaFileTransfer or here locally to something more specific as a variable called attempt inside the DocumentSsh component doesn't give a clear picture as to what this attempt is for.

ravicious · 2023-03-15T14:15:45Z

  const refTerminal = useRef<Terminal>();
  const { tty, status, closeDocument } = useSshSession(doc);
  const webauthn = useWebAuthn(tty);
+  const { getUrlWithMfa, attempt } = useMfaFileTransfer(webauthn.mfaRequired);


If you aim to have a single getUrl* function at the callsite replacing the old cfg.getScpUrl, as opposed to having two separate functions and deciding which one to use depending on whether mfa is required or not, then IMHO it's perfectly acceptable to name this hook useFileTransfer and not mention mfa at all. Same with the getUrlWithMfa function, it could be named getScpUrl I suppose.

This way you truly abstract away the details behind handling file transfer with per-session MFA enabled.

If I didn't know much about file transfer and started reading DocumentSSH, I think I'd start wondering why we always call functions such as getUrlWithMfa even if per-session MFA is not used.

ravicious · 2023-03-15T14:23:46Z

-            getDownloader: async (location, abortController) =>
-              getHttpFileTransferHandlers().download(
+            getDownloader: async (location, abortController) => {
+              const wanResponse = await auth.getAssertionResponseIfRequired({


I'm not an expert on this but this flow looks better to me than the previous one.

ravicious · 2023-03-15T14:25:32Z


 export default function useWebAuthn(emitterSender: EventEmitterWebAuthnSender) {
  const [state, setState] = useState({
+    mfaRequired: false,


I'd document what mfaRequired is for and what the drawbacks are that you stated in your comment. #22938 (comment)

As far as I understand, MFA file transfer piggybacks on the fact that it's necessary to solve the MFA challenge when connecting to a server which might not be clear from just skimming through DocumentSsh.

I think the mfaRequired field is actually so specific to file transfer that I'd even go as far as giving it a name closely related to file transfer. The rest of DocumentSsh doesn't do anything with the knowledge behind mfaRequired. DocumentSsh handles MFA in a completely different way by showing the dialog when requested is true (if I understand it correctly). Yet the name mfaRequired suggest as if it was the source of truth behind something more grand than it really is.

What we seem to do with mfaRequired boils down to "If an MFA challenge was sent, pass the webauthn param for scp URLs".

I'll change to name to addMfaToScpUrls.

ravicious

LGTM once someone confirms that the problem with Safari is just on my machine.

kimlisa · 2023-03-15T20:29:08Z

+import {
+  DeviceType,
+  DeviceUsage,
+  IsMfaRequiredRequest,


i'd move IsMfaRequiredRequest typings to auth/types.ts, since the check is done in auth.ts

kimlisa · 2023-03-15T20:32:00Z

+import cfg, { UrlScpParams } from 'teleport/config';
+import auth from 'teleport/services/auth/auth';
+
+export default function useMfaFileTransfer(addMfaToScrpUrls: boolean) {


i think the filename also meant to be renamed to this hooks name?

kimlisa · 2023-03-15T20:35:26Z

    setState({
      ...state,
      requested: true,
+      addMfaToScpUrls: true,


ohh i see what you did here, that's smart

kimlisa · 2023-03-15T20:36:16Z

+      status: 'processing',
+      statusText: '',
+    });
+    if (!addMfaToScrpUrls) {


Suggested change

if (!addMfaToScrpUrls) {

if (!addMfaToScpUrls) {

kimlisa · 2023-03-15T20:49:57Z

i checked this branch out and ran locally, and upload works for me on safari. typing a file path and pressing enter also works for me (opens file browser for me to select files)

Version 16.3 (18614.4.6.1.6), mac m1

avatus added 3 commits March 10, 2023 15:58

Add mfa check to web scp and perform ceremony

d88d76c

Abstract getAuthResponse from DocumentSsh

1bc53ec

Clean up types

8731301

github-actions Bot requested review from kimlisa and ravicious March 10, 2023 22:41

github-actions Bot added size/sm ui labels Mar 10, 2023

avatus changed the title ~~Add mfa check to web scp and perform ceremony~~ [web] Add mfa check to web scp and perform ceremony Mar 10, 2023

zmb3 approved these changes Mar 13, 2023

View reviewed changes

Update mfa check node_name param to serverId

5df4323

kimlisa reviewed Mar 13, 2023

View reviewed changes

Comment thread web/packages/teleport/src/services/auth/auth.ts Outdated

Comment thread web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx Outdated

Comment thread web/packages/teleport/src/services/auth/auth.ts Outdated

Address PR feedback

d83268a

ravicious reviewed Mar 14, 2023

View reviewed changes

avatus added 6 commits March 14, 2023 15:25

Add mfaRequired key to useWebauthn hook

80e807c

Remove unneeded return

c17adf0

Use mfaRequired during scp url create

d5f46d7

Remove unused check func

46c074a

Use abort controller for errors

6b330e3

Add useMfaFileTransfer hook

84fb17c

ravicious reviewed Mar 15, 2023

View reviewed changes

Change hook name

e13fa47

ravicious approved these changes Mar 15, 2023

View reviewed changes

kimlisa approved these changes Mar 15, 2023

View reviewed changes

Fix typos and move around types

41efe13

avatus merged this pull request into michaelmyers/web_scp_with_mfa Mar 15, 2023

avatus mentioned this pull request Apr 28, 2023

[web] Moderated Session file transfers #24583

Merged

avatus deleted the michaelmyers/web/web_scp_with_mfa branch August 21, 2023 18:36

Conversation

avatus commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Mar 14, 2023 •

edited

Loading

kimlisa Mar 15, 2023 •

edited

Loading

avatus Mar 14, 2023 •

edited

Loading

kimlisa Mar 15, 2023 •

edited

Loading