Add sshserver file transfer handlers by avatus · Pull Request #24580 · gravitational/teleport

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

This PR will add the handlers to the sshserver to listen for File Transfer Request events. There is a bit of refactor as well from #24260, but the main thing being that file transfers over SFTP does not include an ssh.Request so I removed the comparison check.

partner PR (that also includes web) #24581

capnspacehook · 2023-04-14T15:05:10Z

 		case teleport.ForceTerminateRequest:
 			return s.termHandlers.HandleForceTerminate(ch, req, serverContext)
 		case sshutils.EnvRequest, tracessh.EnvsRequest:
+		case sshutils.FileTransferRequestResponse:


I'm not sure whether the lib/srv/forward SSH server needs to support this as well, probably not but for connections to agentless nodes it might.

zmb3 · 2023-04-14T20:25:40Z

 	Moderated bool `json:"moderated"`
 }

+// FileTransferParams contain parameters for requesting a file transfer


This is a bit misleading, because it looks like this can be a request or a response based on the fields below.

I agree. A comment on the other PR lead me to separate these from requests and responses so it will change (not sure outcome yet, but will update comment regardless)

zmb3 · 2023-04-14T20:26:16Z

+	// Direction is either upload or download
+	Direction string `json:"direction"`
+	// Location is location of file to download, or where to put an upload
+	Location string `json:"location"`


Would Path be a better name?

i was keeping it in line with the presentation on the frontend and current file transfer (not request) params. Path does seem like a better name but a change for all the other aspects that aren't in this PR might muddy it up. I'll double check, and if its too many small changed, I'll separate into another PR

zmb3 · 2023-04-14T20:26:44Z

+	// Filename is the name of the file to be uploaded
+	Filename string `json:"filename"`
+	// Size is the size of the file to be uploaded
+	Size string `json:"size"`


What would this look like? Why a string?

This was vestigial from testing/tinkering. will remove as it's not used

zmb3 · 2023-04-14T20:27:26Z

+	// Requster is the authenticated Teleport user who requested the file transfer
+	Requester string `json:"requester"`
+	// Approvers is a list of teleport users who have approved the file transfer request
+	Approvers []Party `json:"approvers"`


Why do you need an Approved field if you have approvers? Is it possible for Approved to be false but Approvers to be non-empty?

This is a bad consequence of having the request/response in this single struct. What Approved actually meant was "is this response an approval or deny", which now that I've seen through the weeds is NOT very clear. Removing it from this and making a discrete Response struct will clear it up

zmb3 · 2023-04-14T20:29:36Z

+)
+
+// NotifyFileTransferRequest is called to notify all members of a party that a file transfer request has been created/approved/denied.
+// The notification is a global ssh request and requires the client to update it's UI state accordingly.


Suggested change

// The notification is a global ssh request and requires the client to update it's UI state accordingly.

// The notification is a global ssh request and requires the client to update its UI state accordingly.

zmb3 · 2023-04-14T20:30:03Z

+	session := scx.getSession()
+	if session == nil {
+		s.log.Debugf("Unable to notify %s, no session found in context.", res)
+		return nil


Should this be an error or is it expected that there will sometimes not be a session?

Should be an error, thanks!

zmb3 · 2023-04-14T20:32:23Z

 	}

-	return sess.checkIfFileTransferApproved(req)
+	fileTransferEvent := &apievents.FileTransferRequestEvent{


Aren't the events in apievents for the audit log?

I haven't added the audit event yet, but a similar pattern with Resize is to create the audit event and send it, and then reuse that to send to the client which I liked. It looks more weird since the audit event isn't happening yet.

zmb3 · 2023-04-14T20:34:51Z

+
+// denyFileTransferRequest will deny a file transfer request and remove it from the current session's file transfer requests map.
+// A file transfer request does not persist after deny, so there is no "denied" state. Deny in this case is synonymous with delete
+// with the addition of checking for a valid denier.


So what happens in this scenario?

2 moderator approvals are required for transfer

3 moderators are watching the session

moderator A approves the transfer

moderator B denies the transfer

Does the request immediately get dropped, or does moderator C still have a chance to approve?

Request will immediately be dropped. Similar to access requests, if moderated A approved and B denied, C won't be able to answer as the result is already in a denied state. (except there isn't a denied state here, its just gone, but you get the idea)

zmb3 · 2023-04-17T02:06:45Z

+	Approved bool `json:"approved"`
+}
+
+// UnmarshalFileTransferResponseParams takes a serialized string that contains the


Don't forget to update the godoc to account for the rename (here and below)

I don't even think these methods are needed anymore. If i'm not checking/setting any defaults and just making a new struct, I'll just do it at the callsite and remove these all together. 6efdc11

zmb3 · 2023-04-17T02:08:03Z

+
+// UnmarshalFileTransferResponseParams takes a serialized string that contains the
+// file transfer request response parameters and returns a *FileTransferParams.
+func NewFileTransferResponseParams(requestId string, approved bool) (*FileTransferResponseParams, error) {


Fix the godoc. It should start with the name of the function you're documenting.

This never returns an error, so I would simplify the signature and remove the error return.

rosstimothy · 2023-04-17T13:04:37Z

+	case sshutils.FileTransferRequest:
+		return s.termHandlers.HandleFileTransferRequest(ctx, ch, req, serverContext)
+	case sshutils.FileTransferRequestResponse:
+		return s.termHandlers.HandleFileTransferRequestResponse(ctx, ch, req, serverContext)


We need to be careful when backporting these requests. Up until very recently unknown session requests would terminate the entire session instead of rejecting the request and keeping the session alive.

capnspacehook

Looks good assuming all other feedback is addressed

rosstimothy · 2023-04-25T20:37:50Z

+const (
+	// FileTransferRequest is used when creating a new file transfer request
+	FileTransferRequest string = "file-transfer-request@goteleport.com"
+	// FileTransferDecision is a request that will approve or deny an active file transfer


Maybe add a note indicating that this is one of possibly many decisions for a given FileTransferRequest if there are multiple approvers required?

rosstimothy · 2023-04-25T20:39:40Z

+
+const (
+	// FileTransferRequest is used when creating a new file transfer request
+	FileTransferRequest string = "file-transfer-request@goteleport.com"


Renaming this would allow you to use FileTransferReq for the name of the payload instead of FileTransferRequestReq

Suggested change

FileTransferRequest string = "file-transfer-request@goteleport.com"

InitiateFileTransfer string = "file-transfer@goteleport.com"

rosstimothy · 2023-04-25T20:40:48Z

+  // Download is true if the requested file transfer is a download, false if an upload
+  bool Download = 7 [(gogoproto.jsontag) = "download"];


Is it kosher to change this? Are there any compatibility concerns here?

Yes, this event was created by me to support this PR. so it's not consumed by anything yet. No compatibility concerns yet

rosstimothy · 2023-04-25T20:41:55Z

+	return sess.checkIfFileTransferApproved(req)
+}
+
+type FileTransferRequestEvent string


nit: missing go docs

rosstimothy · 2023-04-25T20:43:57Z

+	session := scx.getSession()
+	if session == nil {
+		s.log.Debugf("Unable to notify %s, no session found in context.", res)
+		return trace.NotFound("No session found in context.")


Error strings shouldn't be capitalized or end with punctuation: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Suggested change

return trace.NotFound("No session found in context.")

return trace.NotFound("no session found in context")

rosstimothy · 2023-04-25T20:45:01Z

+		}
+	}
+	if approver == nil {
+		return nil, trace.AccessDenied("Cannot approve file transfer requests if not in the current moderated session")


Suggested change

return nil, trace.AccessDenied("Cannot approve file transfer requests if not in the current moderated session")

return nil, trace.AccessDenied("cannot approve file transfer requests if not in the current moderated session")

rosstimothy · 2023-04-25T20:45:22Z

+		}
+	}
+	if denier == nil {
+		return nil, trace.AccessDenied("Cannot deny file transfer requests if not in the current moderated session")


Suggested change

return nil, trace.AccessDenied("Cannot deny file transfer requests if not in the current moderated session")

return nil, trace.AccessDenied("cannot deny file transfer requests if not in the current moderated session")

rosstimothy · 2023-04-25T20:46:34Z

+	} else {
+		_, err := session.denyFileTransferRequest(params, scx)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+	}
+	return nil


Suggestion: drop the else to unindent: https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Suggested change

} else {

_, err := session.denyFileTransferRequest(params, scx)

if err != nil {

return trace.Wrap(err)

}

}

return nil

}

_, err := session.denyFileTransferRequest(params, scx)

return trace.Wrap(err)

rosstimothy · 2023-05-03T12:31:34Z

+	for _, p := range session.parties {
+
+		// Send the message as a global request.
+		_, _, err = p.sconn.SendRequest(teleport.SessionEvent, false, eventPayload)


Suggested change

for _, p := range session.parties {

// Send the message as a global request.

_, _, err = p.sconn.SendRequest(teleport.SessionEvent, false, eventPayload)

for _, p := range session.parties {

// Send the message as a global request.

_, _, err = p.sconn.SendRequest(teleport.SessionEvent, false, eventPayload)

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

github-actions Bot requested review from capnspacehook and codingllama April 13, 2023 22:35

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

avatus mentioned this pull request Apr 13, 2023

Add web terminal handlers for file transfer requests #24581

Merged