Skip to content

Refactor web UI file transfers to use sftp package#24260

Merged
capnspacehook merged 1 commit intomasterfrom
capnspacehook/web-ui-sftp
Apr 12, 2023
Merged

Refactor web UI file transfers to use sftp package#24260
capnspacehook merged 1 commit intomasterfrom
capnspacehook/web-ui-sftp

Conversation

@capnspacehook
Copy link
Copy Markdown
Contributor

@capnspacehook capnspacehook commented Apr 7, 2023

The sftp package is where modern file transfer logic lives and is being maintained. Make the web UI use this package to unify how we transfer files.

Closes #23595.

@capnspacehook capnspacehook added the sftp Issues related to Teleport's SFTP implementation label Apr 7, 2023
@github-actions github-actions Bot requested review from lxea and strideynet April 7, 2023 02:58
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log size/md labels Apr 7, 2023
@capnspacehook capnspacehook changed the base branch from master to capnspacehook/tsh-scp-globbing April 7, 2023 03:06
@capnspacehook capnspacehook force-pushed the capnspacehook/web-ui-sftp branch from 38dcbaa to 6fcc338 Compare April 7, 2023 03:17
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Apr 7, 2023

Please make sure file transfers still work when per session MFA is enabled.

Cc @avatus

Comment thread lib/sshutils/sftp/http.go Outdated
Comment thread lib/sshutils/sftp/parse_test.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp.go Outdated
Comment thread lib/sshutils/sftp/sftp_test.go Outdated
Comment thread lib/web/files.go Outdated
@avatus
Copy link
Copy Markdown
Contributor

avatus commented Apr 7, 2023

Please make sure file transfers still work when per session MFA is enabled.

Cc @avatus

Tested locally and the current iteration has per-session-mfa still working. I also love that this doesn't change anything on the frontend so, 👍 from me

@avatus
Copy link
Copy Markdown
Contributor

avatus commented Apr 7, 2023

A couple notes from local testing.

  1. file transfers are way faster now, super sweet!!
  2. I recently merged a PR to let this through if the request includes some env vars related to a moderated session. You have included the env vars in this PR which is great thank you!
  3. Downloading file transfers aren't blocked on the web after this change. tsh tcp was fixed here by adding in the CheckSFTPAllowed but I think we are skipping this now on the web with the c.IsHTTPResponse block. I think we'll have to do some check similar to my linked PR above where we prevent downloads from a user that requires moderation unless those envvars exist/are valid. Let me know what you think!

@capnspacehook capnspacehook force-pushed the capnspacehook/web-ui-sftp branch 2 times, most recently from 9b8c49f to b7282b8 Compare April 10, 2023 18:40
@capnspacehook capnspacehook requested a review from zmb3 April 10, 2023 18:40
@capnspacehook
Copy link
Copy Markdown
Contributor Author

@avatus thanks for manually testing! I think I fixed the file transfer check, can you take a look and make sure everything looks good?

@capnspacehook capnspacehook requested a review from avatus April 10, 2023 18:41
Base automatically changed from capnspacehook/tsh-scp-globbing to master April 11, 2023 15:11
@capnspacehook capnspacehook force-pushed the capnspacehook/web-ui-sftp branch 2 times, most recently from 228035a to 2b4617e Compare April 11, 2023 18:52
Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Looks and works great!

@capnspacehook capnspacehook force-pushed the capnspacehook/web-ui-sftp branch 2 times, most recently from b02964a to 2344c81 Compare April 11, 2023 19:56
Comment thread lib/web/files.go Outdated
@capnspacehook
Copy link
Copy Markdown
Contributor Author

Friendly ping @strideynet

Comment thread lib/srv/ctx.go Outdated
return nil
}
if registry == nil {
return errCannotStartUnattendedSession
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these errors be wrapped so we get a trace to within this function ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually was thinking about that and forgot to do something about it... These should probably be normal errors constructed from errors.New and wrapped when returned, yeah.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually since trace.AccessDenied takes strings, I guess errCannotStartUnattendedSession needs to be a string constant. You think I should drop the err prefix though since it's not an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I opted to just wrap the existing errors, though imo that's not ideal as the trace will include the var block the errors are defined in. Maybe we need to add functions to the trace package to wrap existing errors with a flavor, ie AccessDenied or something.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from lxea April 12, 2023 13:24
The sftp package is where modern file transfer logic lives and is
being maintained. Make the web UI use this package to unify
how we transfer files.
@capnspacehook capnspacehook force-pushed the capnspacehook/web-ui-sftp branch from 200e6f2 to d284e81 Compare April 12, 2023 13:32
@capnspacehook capnspacehook enabled auto-merge April 12, 2023 13:33
@capnspacehook capnspacehook added this pull request to the merge queue Apr 12, 2023
Merged via the queue into master with commit 268c3c8 Apr 12, 2023
@capnspacehook capnspacehook deleted the capnspacehook/web-ui-sftp branch April 12, 2023 14:05
This was referenced Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-log Issues related to Teleports Audit Log sftp Issues related to Teleport's SFTP implementation size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebUI should use SFTP instead of SCP

5 participants