RFD: Moderated File Transfers (web) by avatus · Pull Request #23546 · gravitational/teleport

avatus · 2023-03-23T22:35:13Z

Rendered view here
Issue #16008

xacrimon

first pass

xacrimon · 2023-03-24T02:48:22Z

+
+## What
+
+Approval process to allow file transfers during a web moderated session


nit: start off with a verb instead of a noun here, example rephrashing

Suggested change

Approval process to allow file transfers during a web moderated session

Extend web file transfers to work during moderated web sessions using an approval process.

xacrimon · 2023-03-24T02:49:17Z

+
+## Why
+
+Users have no ability to transfer files if their role requires a moderated session for the specified server, even if they are in an active and "valid" session currently. Originally, we would allow file transfers regardless of moderated sessions which, while good for UX, goes against the purpose of moderated sessions in the first place. We have disabled the functionality but would like to reintroduce it in a secure, moderated, and auditable way. 


nit: consider line splitting at sentence boundaries or ;'s where it makes sense. It makes it easier for reviewers to make suggestions or select a specific bit of text.

xacrimon · 2023-03-24T02:49:44Z

+
+## Why
+
+Users have no ability to transfer files if their role requires a moderated session for the specified server, even if they are in an active and "valid" session currently. Originally, we would allow file transfers regardless of moderated sessions which, while good for UX, goes against the purpose of moderated sessions in the first place. We have disabled the functionality but would like to reintroduce it in a secure, moderated, and auditable way. 


Suggested change

Users have no ability to transfer files if their role requires a moderated session for the specified server, even if they are in an active and "valid" session currently. Originally, we would allow file transfers regardless of moderated sessions which, while good for UX, goes against the purpose of moderated sessions in the first place. We have disabled the functionality but would like to reintroduce it in a secure, moderated, and auditable way.

Users have no ability to transfer files if their role requires a moderated session for the specified server, even if they are in an active and launched session currently. Originally, we would allow file transfers regardless of moderated sessions which, while good for UX, goes against the purpose of moderated sessions in the first place. We have disabled the functionality but would like to reintroduce it in a secure, moderated, and auditable way.

xacrimon · 2023-03-24T02:52:47Z

+The proposed flow would look like this (simplified).
+```mermaid
+sequenceDiagram
+Requester->>SSH Server: Send FileTransferRequest over websocket connection


FileTransferRequest over websocket to SSH? How does this make it to the SSH node from the proxy? Maybe add a proxy entity even to this simplified diagram to help illustrate this communication.
i.e requester -> proxy over ws
proxy -> ssh node over async ssh channel req

Even better, I just replaced the "api server" with proxy since thats what it is anyway. Moved it closer to the requester entity as well.

xacrimon · 2023-03-24T02:53:41Z

+```go
+type FileTransferRequest {
+    id string // UUID
+    user string


os user, teleport user? whom is this in the session context?

In this context, I would use scx.Identity.TeleportUser... however, maybe a more contextual name would be requestingTeleportUser. Just so it's very clear its the requester and what "kind" of user.

Yep, a more contextual name is what I wanted. TBH, calling it requester is fine and avoids annoying name size blowup, just throw a comment on the field saying it's a Teleport user so it's obvious on hover.

xacrimon · 2023-03-24T02:54:48Z

+    user string
+    sessionID string
+    filePath string
+    approvers []*party


can we shortly explain what a party is in this context? maybe a participant of peer or moderator type?

Will update soon. I'm not sure we need to store the entire party yet, but yes, it would be "anyone who is peer or moderator"

xacrimon · 2023-03-24T02:57:32Z

+
+The benefit of storing it on the session, and not in an access-request-like object, is that once the session is gone, so is the approved request. Keeping these approvals as ephemeral as possible is ideal. 
+
+### the fileTransferChannel 


Maybe?

Suggested change

### the fileTransferChannel

### Transfer request communication

xacrimon · 2023-03-24T02:59:50Z

+
+### Updated file transfer api handler
+
+Not much will change here. The only difference is adding the `requestID` and `sessionID` as env vars in the `serverContext`. This will allow us to extend our `SessionRegistry.OpenExecSession` method with a new check like `isApprovedRequest`


What are these env vars actually named? Constant values? RequestID feels a bit generic as an env var name, maybe something like commandRequestID?

xacrimon · 2023-03-24T03:00:22Z

+## Security Considerations
+Originally, an idea was thrown around to create a "signed" url with all the `FileTransferRequest` information encoded in it but this seemed unnecessary. Because the current flow stores the original request in the session, we aren't giving them full access to open any exec session, just one that matches the exact request that was asked for. Also, having the request stored in the session means that once the session is gone, there isn't a way to "re-request" it. 


Suggested change

## Security Considerations

Originally, an idea was thrown around to create a "signed" url with all the `FileTransferRequest` information encoded in it but this seemed unnecessary. Because the current flow stores the original request in the session, we aren't giving them full access to open any exec session, just one that matches the exact request that was asked for. Also, having the request stored in the session means that once the session is gone, there isn't a way to "re-request" it.

## Security Considerations

Originally, an idea was thrown around to create a "signed" url with all the `FileTransferRequest` information encoded in it but this seemed unnecessary. Because the current flow stores the original request in the session, we aren't giving them full access to open any exec session, just one that matches the exact request that was asked for. Also, having the request stored in the session means that once the session is gone, there isn't a way to "re-request" it.

zmb3 · 2023-03-24T14:01:56Z

+
+## Scope
+
+This RFD will cover how to implement for web SCP specifically, but may be expanded to SFTP for tsh, although my research has been mostly for web scp. If at anytime there is ambiguity in this RFD for "which type of file transfers does this refer to", default to web scp only. 


Are we leaving tsh out of scope because it requires a different solution or because we just haven't thought about it yet?

From a design perspective, I think it's important to understand why we're proposing a solution that's only valid for web.

I believe the backend portion of this solution could be reused for tsh quite easily. I don't have the context yet to decide what the UI/UX would look like for tsh. I can do more research and revisit.

Commenting a few more notes/thinking out loud.

With the web, the request is outside of the shell and handled via the UI, as well as the approval process. This means the shell isn't blocked by any file transfer requests.

For cli, we can maybe inform the approvers in the cli that someone is requesting a file transfer and implement an approve/deny command, similar to the force-terminate command. I'm not sure how this would scale will multiple file transfer requests at once, but its a thread to pull on.

I think the tsh scp command could function after approval similar to the new file transfer http request optional params by adding two new flags, the --sessionId and --commandRequestId.

I'll continue to ponder.

zmb3 · 2023-03-24T14:06:08Z

+
+#### The FileTransferRequest 
+
+The general idea of the solution is to provide a way for users to "request-a-request" that can be approved by moderators. An example of a file transfer specific struct could look like


Will this new request only be used for moderated sessions or will it apply any time there is a web based file transfer?

If it won't apply every time, how will you detect whether or not you need to use it?

Only for moderated sessions. If it isn't a moderated session, no extra parameters are needed to be sent and will just pass the request without them added. I speak about it here but I see its a bit unclear now. Will update.

The web console will determine if we need to start this new process, and the api handler knowing when to add the environment variables based on the existence of the optional parameters sent. Sort of similar to how we issue certs when a webauthn response is included, we would be able to set the environment variables needed for OpenExecSession.

Also, OpenExecSession knows if the soon-to-be-opened session is moderated or not, so we can have a check "if moderated, see if they've sent through a commandRequestID".

zmb3 · 2023-03-24T14:08:18Z

+Not much will change here. The only difference is adding the `requestID` and `sessionID` as env vars in the `serverContext`. This will allow us to extend our `SessionRegistry.OpenExecSession` method with a new check like `isApprovedRequest`
+
+```go
+func (s *SessionRegistry) isApprovedRequest(ctx context.Context, scx *ServerContext) (bool error) {


request is a vague term and makes me think of access requests here. Maybe isApprovedFileTransfer or something along those lines would be a better name.

isApprovedFileTransfer will work great. If we want, we can even abstract it early (and in other places in the code) to isApprovedCommand? its only being implemented with file transfers right now, this process of requesting an command, approving a command, and running that command seems like it can be related to anything we want in the future. Thoughts?

Let's go with the name that is most clear now. We can always change the name if we expand the scope of the feature in the future, but for now I don't want the code to read as if we support approving a specific command.

zmb3 · 2023-03-26T16:09:47Z

+}
+```
+
+### UX/UI


I think it's worth discussing the UX for tsh-based file transfers. Will it work? Will it fail with a confusing error? Can we detect it and prompt the user to initiate the transfer via the web UI?

Part of #23546 This will add a fileTransferRequest to a session and allow environment variables to be passed from the webUI in order to validate a request that happens "outside" the moderated session (via HTTP request).

zmb3 · 2023-04-03T21:21:26Z

Hey @xacrimon and @capnspacehook - I think all of your comments have been addressed so far.

There's still a few corner cases we're experimenting with, so we'll probably keep this open until we decide on a a path forward, but I wanted to confirm with you two if there's anything else you'd like to see addressed before we start pushing more PRs.

capnspacehook · 2023-04-03T22:09:12Z

LGTM as long as the flow with tsh scp gets addressed eventually

zmb3 · 2023-04-03T23:58:06Z

LGTM as long as the flow with tsh scp gets addressed eventually

To be honest, we might not ever support tsh scp, as the whole idea is you have a moderated session, where moderators are already present and watching. Technically an SCP is a separate session, but in the web UI we can make it appear as part of an existing session. We are unsure how this could work with tsh-initiated transfers.

@avatus it would be worth considering whether we can allow moderators to approve file transfers if they joined via tsh join, so long as the user is in a web UI session. Let's add a bit more info about this to the RFD.

xacrimon · 2023-04-04T00:31:15Z

Hey, everything to me looks fine so far and I'd be happy to see it move forward. Feel free to hit Request Review next to my name once the changes @zmb3 mentioned are added.

Part of #23546 This will add a fileTransferRequest to a session and allow environment variables to be passed from the webUI in order to validate a request that happens "outside" the moderated session (via HTTP request).

zmb3 · 2023-04-07T19:47:12Z

In the future, please try to follow the branch naming conventions for RFDs:

teleport/rfd/0000-rfds.md

Lines 68 to 70 in 785fa04

    
           1. make a branch off of `master` called `rfd/$number-your-title` 
        
              In our example, it'll be branch `rfd/0018-irc-access`.

avatus · 2023-05-10T16:51:41Z

Updated to implemented as well as the RFD number to what I believe was the next free code. Will follow the branch naming conventions next time 👍

avatus · 2023-05-15T16:56:55Z

Friendly ping @zmb3 @capnspacehook @xacrimon

…derated session

avatus marked this pull request as ready for review March 23, 2023 22:42

avatus requested review from jakule, xacrimon and zmb3 March 23, 2023 22:42

github-actions Bot requested review from lxea and ryanclark March 23, 2023 22:43

github-actions Bot added rfd Request for Discussion size/md labels Mar 23, 2023

capnspacehook reviewed Mar 23, 2023

View reviewed changes

Comment thread rfd/0113-moderated-file-transfers.md Outdated

Comment thread rfd/0113-moderated-file-transfers.md Outdated

Comment thread rfd/0113-moderated-file-transfers.md Outdated

avatus requested a review from klizhentas March 24, 2023 00:06

xacrimon reviewed Mar 24, 2023

View reviewed changes

zmb3 reviewed Mar 24, 2023

View reviewed changes

zmb3 reviewed Mar 26, 2023

View reviewed changes

avatus mentioned this pull request Mar 30, 2023

Add FileTransferRequests to session for moderated scp #23875

Merged

avatus mentioned this pull request Apr 6, 2023

[web] Add isModeratedSession flag to web ssh session #24238

Merged

This was referenced Apr 13, 2023

Add FileTransferRequestEvent #24558

Merged

Add sshserver file transfer handlers #24580

Merged

Add web terminal handlers for file transfer requests #24581

Merged

[web] Moderated Session file transfers #24583

Merged

avatus requested review from capnspacehook and zmb3 and removed request for klizhentas May 10, 2023 16:50

avatus removed request for lxea and ryanclark May 10, 2023 16:50

avatus mentioned this pull request May 10, 2023

[docs] Include File Transfers in moderated sessions docs #26032

Merged

avatus requested a review from xacrimon May 12, 2023 18:33

zmb3 approved these changes May 17, 2023

View reviewed changes

Comment thread rfd/0122-moderated-file-transfers.md Outdated

russjones approved these changes May 18, 2023

View reviewed changes

avatus enabled auto-merge May 18, 2023 16:47

avatus added this pull request to the merge queue May 18, 2023

avatus removed this pull request from the merge queue due to a manual request May 18, 2023

Add the RFD for implementing file transfers in the Web UI during a mo…

87269c7

…derated session

avatus force-pushed the michaelmyers/rfd-moderated-file-transfers branch from d178468 to 87269c7 Compare May 18, 2023 16:53

avatus enabled auto-merge May 18, 2023 16:53

avatus added this pull request to the merge queue May 18, 2023

Merged via the queue into master with commit da6b319 May 18, 2023

avatus deleted the michaelmyers/rfd-moderated-file-transfers branch May 18, 2023 17:14


		## What

		Approval process to allow file transfers during a web moderated session

	Approval process to allow file transfers during a web moderated session
	Extend web file transfers to work during moderated web sessions using an approval process.


		## Why

		Users have no ability to transfer files if their role requires a moderated session for the specified server, even if they are in an active and "valid" session currently. Originally, we would allow file transfers regardless of moderated sessions which, while good for UX, goes against the purpose of moderated sessions in the first place. We have disabled the functionality but would like to reintroduce it in a secure, moderated, and auditable way.


		The benefit of storing it on the session, and not in an access-request-like object, is that once the session is gone, so is the approved request. Keeping these approvals as ephemeral as possible is ideal.

		### the fileTransferChannel

	### the fileTransferChannel
	### Transfer request communication


		### Updated file transfer api handler

		Not much will change here. The only difference is adding the `requestID` and `sessionID` as env vars in the `serverContext`. This will allow us to extend our `SessionRegistry.OpenExecSession` method with a new check like `isApprovedRequest`

		## Security Considerations
		Originally, an idea was thrown around to create a "signed" url with all the `FileTransferRequest` information encoded in it but this seemed unnecessary. Because the current flow stores the original request in the session, we aren't giving them full access to open any exec session, just one that matches the exact request that was asked for. Also, having the request stored in the session means that once the session is gone, there isn't a way to "re-request" it.


		## Scope

		This RFD will cover how to implement for web SCP specifically, but may be expanded to SFTP for tsh, although my research has been mostly for web scp. If at anytime there is ambiguity in this RFD for "which type of file transfers does this refer to", default to web scp only.


		#### The FileTransferRequest

		The general idea of the solution is to provide a way for users to "request-a-request" that can be approved by moderators. An example of a file transfer specific struct could look like

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xacrimon 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmb3 commented Apr 3, 2023

Uh oh!

capnspacehook commented Apr 3, 2023

Uh oh!

zmb3 commented Apr 3, 2023

Uh oh!

xacrimon commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmb3 commented Apr 7, 2023

Uh oh!

avatus commented May 10, 2023

Uh oh!

avatus commented May 15, 2023

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

avatus commented Mar 23, 2023 •

edited

Loading

xacrimon commented Apr 4, 2023 •

edited

Loading