Add web terminal handlers for file transfer requests by avatus · Pull Request #24581 · gravitational/teleport

avatus · 2023-04-13T22:37:34Z

Part of #23546
Relies on #24580

web buddy: #24583

This PR adds the fileTransferC channel to our web terminal to listen to websocket events dealing with file transfer requests. The two events added are for requesting a new file transfer, and responding to an existing file transfer.

tobiaszheller · 2023-04-14T07:34:06Z

 		if err != nil {
 			return trace.Wrap(err)
 		}
-		t.SessionRegistry.NotifyFileTransferRequest(req, FileTransferApproved, scx)


why this notify is missing?

This is just some base branch shenanigans. Both of the Notifys happen inside the approve/deny methods now but it'll be updated here when I get the base branch updated/merged. Ignore for now!

tobiaszheller · 2023-04-14T08:11:27Z

+		if err != nil {
+			return 0, trace.Wrap(err)
+		}
+		approved, ok := e["approved"].(bool)


i haven't seen that part in RFD. Some audit event will be extended to have this field or does it already have it? can you link it?

This approved is handling the file transfer response. Is the response "Approved" or "Denied" basically.

tobiaszheller · 2023-04-14T08:14:39Z

 		}

+		return 0, nil
+	case defaults.WebsocketFileTransferRequestResponse:


i saw that in resizeC we check for nil channel at top, do we need it here as well?

ravicious · 2023-04-14T14:40:32Z

+		select {
+		case <-t.terminalContext.Done():
+			return
+		case transferRequest := <-fileTransferC:


Don't we have to handle the channel getting closed, just as handleWindowResize does?

teleport/lib/web/terminal.go

Lines 894 to 898 in 959073c

case params := <-resizeC:

// nil params indicates the channel was closed

if params == nil {

return

}

Shouldn't the new version of handleFileTransfer account for individual channels getting closed?

ravicious · 2023-04-14T14:47:21Z

+			return
+		case transferRequest := <-fileTransferC:
+			// if not response, this is a new file transfer request
+			if !transferRequest.Response {


Did you consider representing those two different request types as two separate structs or something akin to a discriminated union in TypeScript?

Packing every possible field in a single type and then differentiating between them based on the assumed shape doesn't sound like a sound long-term strategy to me. I can't help here much unfortunately without doing a deeper dive into the code. I'd ask someone who knows Go better than me if it'd be possible to represent those two different types of requests as different structs.

Yeah, this grew from "eh only 1 field is needed" to a few, and I didn't cut them apart. It's best to do that now and make it more clear.

After splitting the structs, I made a new chan, one to handle the requests, and one to handle the responses. The original reason to have this struct be the same was to reuse the same chan and just make decisions based on what info came through but I think thats a bit lazy now. Although I have a separate channel, I kept the "setting" of the channels in the same method (just with the two channels now instead of one) and also, have both cases in the same handler as well. Came out cleaner and easier to parse around throughout the app. Thanks for keeping me accountable

ravicious · 2023-04-14T14:54:55Z

 	EnvsJSON []byte `json:"envs"`
 }

+type FileTransferReq struct {


Looking at other definitions in this file, shouldn't these two have a godoc as well?

ravicious · 2023-04-14T15:00:32Z

+func (s *Session) FileTransferRequestResponse(ctx context.Context, req FileTransferResponseReq) error {
+	const request = "file-transfer-request-response"
+	config := tracing.NewConfig(s.wrapper.opts)
+	ctx, span := config.TracerProvider.Tracer(instrumentationName).Start(


Do we need to set up all of this tracing stuff even though in the end we end up just calling s.Session.SendRequest which does that again? I don't really know much about how we do tracing, I'm looking at SetEnv which ends up calling SendRequest too, but SetEnv actually does some extra stuff.

s.Session.SendRequest doesn't have the tracing as that uses the wrapped ssh.Session, but if I instead just used s.SendRequest, then yeah, I can ignore the tracing as that SendRequest does set it up. Good call

Ah, I didn't notice the difference between the two. Still, I'd recommend asking on #dev-teleport if one way of doing this is preferred over the other.

If you want any custom attributes to be included in the span you will need to create one explicitly. The underlying ssh request tracing will simply tell you that a request named foo was sent.

avatus · 2023-04-16T21:32:19Z

Didn't mean to rebase this early and didn't realize after I pushed, sorry!

…chaelmyers/web_terminal_file_request_handlers

ravicious

Some minor adjustments are needed but looks ok otherwise.

ravicious · 2023-04-17T12:48:39Z

+		download, ok := e["download"].(bool)
+		if !ok {
+			return 0, trace.BadParameter("Unable to find approved status on response")


The error message doesn't match the check that's being performed here.

ravicious · 2023-04-17T12:52:08Z

+
+	// WebsocketFileTransferRequestResponse is received when a response (approve/deny) has been
+	// made for an existing file transfer request
+	WebsocketFileTransferRequestResponse = "t"


It'd be nice to add some tests for that based on TestResizeTerminal.

@avatus can we add some test coverage for this?

ravicious · 2023-04-17T13:05:36Z

+
+// FileTransferRequestApprove sends a "file-transfer-request-response" ssh request
+// The response will contain an Approve bool which will approve or deny a requested file transfer
+func (s *Session) FileTransferRequestResponse(ctx context.Context, req FileTransferResponseReq) error {


When I was reviewing the web PR, I realized that this might need a better name. The other method names are verb based while this one feels a bit odd compared to them.

I tried thinking of something that would make sense for both like ResponseToFileTransferRequest but, idk I didn't like it. So I just split them into to, ApproveFileTransferRequest and DenyFileTransferRequest. They both take just the requestID now as a string, and the req is created inside the method and sets Approved appropriately. Maybe a bit verbose but much more readable and understandable

rosstimothy · 2023-04-17T13:09:28Z

+func (s *Session) FileTransferRequestResponse(ctx context.Context, req FileTransferResponseReq) error {
+	const request = "file-transfer-request-response"
+	config := tracing.NewConfig(s.wrapper.opts)
+	ctx, span := config.TracerProvider.Tracer(instrumentationName).Start(


If you want any custom attributes to be included in the span you will need to create one explicitly. The underlying ssh request tracing will simply tell you that a request named foo was sent.

…chaelmyers/web_terminal_file_request_handlers

rosstimothy · 2023-04-18T13:50:51Z

+
+	// WebsocketFileTransferRequestResponse is received when a response (approve/deny) has been
+	// made for an existing file transfer request
+	WebsocketFileTransferRequestResponse = "t"


…chaelmyers/web_terminal_file_request_handlers

* Add file transfer handlers to web terminal * remove unused keys * Add FileTransferContainer This PR separates the absolute positioned container from the FileTransfer component. We will be adding FileTransferRequests in the same spot, and this will allow them to be visible together without css shenanigans trying to absolutely place them together. FileTransferRequests are separate from FileTransfers in that they interact with the tty. * Add useFileTransfer hook * Use useFileTransfer hook inside useSshSession * Add tty handlers * Update getScpUrl to add optional moderated params * Use FileTransferContainer in Connect * Update DocumentSSH * remove fmt * remove unused shellcmd key * lint fixes * remove pending file after get * remove pending file after get * Clean up console log * Update filetransfererquest to use download bool * Fix upload/download flow * Fix typecheck and FileTransfer tests * Fix UI display bugs and add license * Change filetransferresponse to filetransferdecision * Refactor useFileTransfer up to DocumentSsh * Add license * Fix lint error * update pending transfer text * use handleUpload/handleDownload, move FileTransferRequests to prop * Review feedback * Add usecallback to scpurls and useattempt * Apply patch * Update optional params

Add web terminal handlers for file transfer requests (#24581)

github-actions Bot added the size/md label Apr 13, 2023

github-actions Bot requested review from ravicious and tobiaszheller April 13, 2023 22:38

This was referenced Apr 13, 2023

[web] Moderated Session file transfers #24583

Merged

Add sshserver file transfer handlers #24580

Merged

tobiaszheller reviewed Apr 14, 2023

View reviewed changes

ravicious reviewed Apr 14, 2023

View reviewed changes

avatus added 5 commits April 16, 2023 16:29

Add file transfer handlers to web terminal

eabeeff

remove unused keys

5bec25a

Make fileTransferC buffered

3198c3f

Review feedback

55a9188

Fix typo

8521b0d

avatus force-pushed the michaelmyers/web_terminal_file_request_handlers branch from 959073c to 8521b0d Compare April 16, 2023 21:31

avatus added 7 commits April 16, 2023 16:51

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

78c4761

…chaelmyers/web_terminal_file_request_handlers

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

1b730da

…chaelmyers/web_terminal_file_request_handlers

Remove comment

5925b3b

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

076b78d

…chaelmyers/web_terminal_file_request_handlers

Simplify session param

82ed78d

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

ef178bd

…chaelmyers/web_terminal_file_request_handlers

Simplify file transfer param callsite

6efdc11

tobiaszheller approved these changes Apr 17, 2023

View reviewed changes

ravicious self-requested a review April 17, 2023 11:38

ravicious approved these changes Apr 17, 2023

View reviewed changes

ravicious reviewed Apr 17, 2023

View reviewed changes

rosstimothy requested changes Apr 17, 2023

View reviewed changes

avatus added 3 commits April 17, 2023 17:27

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

bd97802

…chaelmyers/web_terminal_file_request_handlers

Use api constants

e63dbe6

Split FileTransferRequestResponse methods

f69c8e7

rosstimothy reviewed Apr 18, 2023

View reviewed changes

avatus added 11 commits April 18, 2023 19:06

Merge base

4299567

Change name to FileTransferDecision

6e80001

Merge base

61dcc27

Update ssh request name

39a0985

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

bf50fb1

…chaelmyers/web_terminal_file_request_handlers

use FileTransferReq

a927a6a

Rename struct

d429e87

Add file transfer test

2bb6bf1

Add FileTransferDecision to test

df87268

Add dstPath to isApprovedFileTransfer

2ae127e

Remove debug comment

585cfb0

avatus requested a review from rosstimothy May 1, 2023 15:49

rosstimothy approved these changes May 3, 2023

View reviewed changes

Comment thread lib/sshutils/sftp/http.go Outdated

avatus added 4 commits May 3, 2023 14:17

Merge branch 'michaelmyers/ssh_server_file_transfer_handlers' into mi…

a994465

…chaelmyers/web_terminal_file_request_handlers

Rename query params

a20ac31

Prefix file transfer env vars with TELEPORT_

c8d29b6

avatus merged this pull request into michaelmyers/ssh_server_file_transfer_handlers May 3, 2023

avatus deleted the michaelmyers/web_terminal_file_request_handlers branch May 3, 2023 20:14

avatus added a commit that referenced this pull request May 3, 2023

Add sshserver file transfer handlers

6021829

Add web terminal handlers for file transfer requests (#24581)

avatus added a commit that referenced this pull request May 3, 2023

Add sshserver file transfer handlers (#24580)

f814b2d

Add web terminal handlers for file transfer requests (#24581)

avatus mentioned this pull request May 3, 2023

[v13] Add SFTP approval process to web during moderated sessions #25596

Merged

	case params := <-resizeC:
	// nil params indicates the channel was closed
	if params == nil {
	return
	}

Conversation

avatus commented Apr 13, 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!

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!

avatus commented Apr 16, 2023

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avatus commented Apr 13, 2023 •

edited

Loading