From e79e2a8738dce99aeab2d14663e10f95787acec2 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Mon, 26 Feb 2024 15:10:29 -0700 Subject: [PATCH 01/13] ensure moderated file transfers only preform allowed operations --- lib/srv/ctx.go | 28 ++++++ lib/srv/regular/sftp.go | 30 +++++-- lib/srv/regular/sshserver.go | 2 +- lib/srv/sess.go | 163 ++++++++++++++++++----------------- lib/srv/termhandlers.go | 9 +- lib/sshutils/sftp/http.go | 8 -- lib/sshutils/sftp/sftp.go | 10 +-- lib/web/files.go | 2 - tool/teleport/common/sftp.go | 139 +++++++++++++++++++++++++++-- 9 files changed, 273 insertions(+), 118 deletions(-) diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index ced2514d0fa13..c4d1c7241d8d8 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -452,6 +452,10 @@ type ServerContext struct { // UserCreatedByTeleport is true when the system user was created by Teleport user auto-provision. UserCreatedByTeleport bool + + // approvedFileReq is an approved file transfer request that will only be + // set when the session's pending file transfer request is approved. + approvedFileReq *FileTransferRequest } // NewServerContext creates a new *ServerContext which is used to pass and @@ -1399,3 +1403,27 @@ func (c *ServerContext) GetPortForwardEvent() apievents.PortForward { }, } } + +func (c *ServerContext) setApprovedFileTransferRequest(req *FileTransferRequest) { + c.mu.Lock() + c.approvedFileReq = req + c.mu.Unlock() +} + +// GetApprovedFileTransferRequest will return the approved file transfer +// request for this session if there is one present. Note that if an +// approved request is returned future calls to this method will return +// nil to prevent an approved request getting reused incorrectly. +func (c *ServerContext) GetApprovedFileTransferRequest() *FileTransferRequest { + c.mu.Lock() + defer c.mu.Unlock() + + if c.approvedFileReq == nil { + return nil + } + + req := c.approvedFileReq + c.approvedFileReq = nil + + return req +} diff --git a/lib/srv/regular/sftp.go b/lib/srv/regular/sftp.go index 99ec9f6143e6a..5952c24e366d4 100644 --- a/lib/srv/regular/sftp.go +++ b/lib/srv/regular/sftp.go @@ -21,6 +21,7 @@ package regular import ( "bufio" "context" + "encoding/json" "errors" "io" "os" @@ -44,18 +45,20 @@ import ( const copyingGoroutines = 2 type sftpSubsys struct { - sftpCmd *exec.Cmd - serverCtx *srv.ServerContext - errCh chan error - log *logrus.Entry + log *logrus.Entry + + fileTransferReq *srv.FileTransferRequest + sftpCmd *exec.Cmd + serverCtx *srv.ServerContext + errCh chan error } -func newSFTPSubsys() (*sftpSubsys, error) { - // TODO: add prometheus collectors? +func newSFTPSubsys(fileTransferReq *srv.FileTransferRequest) (*sftpSubsys, error) { return &sftpSubsys{ log: logrus.WithFields(logrus.Fields{ trace.Component: teleport.ComponentSubsystemSFTP, }), + fileTransferReq: fileTransferReq, }, nil } @@ -127,9 +130,22 @@ func (s *sftpSubsys) Start(ctx context.Context, if err != nil { return trace.Wrap(err) } - // TODO: put in cgroup? execRequest.Continue() + // Send the file transfer request if applicable + encodedReq := []byte{0x0} + if s.fileTransferReq != nil { + encodedReq, err = json.Marshal(s.fileTransferReq) + if err != nil { + return trace.Wrap(err) + } + encodedReq = append(encodedReq, 0x0) + } + _, err = chReadPipeIn.Write(encodedReq) + if err != nil { + return trace.Wrap(err) + } + // Copy the SSH channel to and from the anonymous pipes s.errCh = make(chan error, copyingGoroutines) go func() { diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index c9fd93ff40257..1a810be892867 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -2169,7 +2169,7 @@ func (s *Server) parseSubsystemRequest(req *ssh.Request, ctx *srv.ServerContext) return nil, trace.Wrap(err) } - return newSFTPSubsys() + return newSFTPSubsys(ctx.GetApprovedFileTransferRequest()) default: return nil, trace.BadParameter("unrecognized subsystem: %v", r.Name) } diff --git a/lib/srv/sess.go b/lib/srv/sess.go index b6900978de916..7c8b900afc3d7 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -437,48 +437,44 @@ func (s *SessionRegistry) GetTerminalSize(sessionID string) (*term.Winsize, erro } func (s *SessionRegistry) isApprovedFileTransfer(scx *ServerContext) (bool, error) { - s.sessionsMux.Lock() - defer s.sessionsMux.Unlock() - - // get the requested location from env vars - location, _ := scx.GetEnv(sftp.FileTransferDstPath) - if location == "" { - return false, nil - } - // if a sessID and requestID environment variables were not set, return not approved and no error. - // This means the file transfer came from a non-moderated session. sessionID will be passed after a + // If the sessID environment variable was not set, return not + // approved and no error. This means the file transfer came from + // a non-moderated session. sessionID will be passed after a // moderated session approval process has completed. sessID, _ := scx.GetEnv(string(sftp.ModeratedSessionID)) if sessID == "" { return false, nil } + // fetch session from registry with sessionID + s.sessionsMux.Lock() sess := s.sessions[rsession.ID(sessID)] + s.sessionsMux.Unlock() if sess == nil { // If they sent a sessionID and it wasn't found, send an actual error return false, trace.NotFound("Session not found") } - requestID, _ := scx.GetEnv(string(sftp.FileTransferRequestID)) - if requestID == "" { - return false, nil + sess.mu.Lock() + defer sess.mu.Unlock() + + if sess.fileTransferReq == nil { + return false, trace.NotFound("Session does not have a pending file transfer request") } - // find file transfer request in the session by requestID - req := sess.fileTransferRequests[requestID] - if req == nil { - // If they sent a fileTransferRequestID and it wasn't found, send an actual error - return false, trace.NotFound("File transfer request not found") + if sess.fileTransferReq.Requester != scx.Identity.TeleportUser { + return false, trace.AccessDenied("Teleport user does not match original requester") } - if req.location != location { - return false, trace.AccessDenied("requested destination path does not match the current request") + approved, err := sess.checkIfFileTransferApproved(sess.fileTransferReq) + if err != nil { + return false, trace.Wrap(err) } - - if req.requester != scx.Identity.TeleportUser { - return false, trace.AccessDenied("Teleport user does not match original requester") + if approved { + scx.setApprovedFileTransferRequest(sess.fileTransferReq) + sess.fileTransferReq = nil } - return sess.checkIfFileTransferApproved(req) + return approved, nil } // FileTransferRequestEvent is an event used to Notify party members during File Transfer Request approval process @@ -499,7 +495,7 @@ const ( // 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 its UI state accordingly. -func (s *SessionRegistry) NotifyFileTransferRequest(req *fileTransferRequest, res FileTransferRequestEvent, scx *ServerContext) error { +func (s *SessionRegistry) NotifyFileTransferRequest(req *FileTransferRequest, res FileTransferRequestEvent, scx *ServerContext) error { session := scx.getSession() if session == nil { s.log.Debugf("Unable to notify %s, no session found in context.", res) @@ -512,11 +508,11 @@ func (s *SessionRegistry) NotifyFileTransferRequest(req *fileTransferRequest, re ClusterName: scx.ClusterName, }, SessionMetadata: session.scx.GetSessionMetadata(), - RequestID: req.id, - Requester: req.requester, - Location: req.location, - Filename: req.filename, - Download: req.download, + RequestID: req.ID, + Requester: req.Requester, + Location: req.Location, + Filename: req.Filename, + Download: req.Download, Approvers: make([]string, 0), } @@ -659,10 +655,10 @@ type session struct { // participants at the end of a session. participants map[rsession.ID]*party - // fileTransferRequests is a set of fileTransferRequests that are currently in the approval - // process, or already approved and not yet executed during a moderated session. If a request is - // denied or, once it's been executed, it should be removed from this map. - fileTransferRequests map[string]*fileTransferRequest + // fileTransferReq a pending file transfer request for this session. + // If the request is denied or approved it should be set to nil to + // prevent its reuse. + fileTransferReq *FileTransferRequest io *TermManager inWriter io.WriteCloser @@ -761,7 +757,6 @@ func newSession(ctx context.Context, id rsession.ID, r *SessionRegistry, scx *Se id: id, registry: r, parties: make(map[rsession.ID]*party), - fileTransferRequests: make(map[string]*fileTransferRequest), participants: make(map[rsession.ID]*party), login: scx.Identity.Login, stopC: make(chan struct{}), @@ -1638,22 +1633,22 @@ func (s *session) checkPresence(ctx context.Context) error { return nil } -// fileTransferRequest is a request to upload or download a file from the node. -type fileTransferRequest struct { - id string - // requester is the Teleport User that requested the file transfer - requester string - // download is true if the request is a download, false if its an upload - download bool - // filename the name of the file to upload. - filename string - // location of the requested download or where a file will be uploaded - location string +// FileTransferRequest is a request to upload or download a file from a node. +type FileTransferRequest struct { + ID string + // Requester is the Teleport User that requested the file transfer + Requester string + // Download is true if the request is a download, false if its an upload + Download bool + // Filename is the name of the file to upload. + Filename string + // Location of the requested download or where a file will be uploaded + Location string // approvers is a list of participants of moderator or peer type that have approved the request approvers map[string]*party } -func (s *session) checkIfFileTransferApproved(req *fileTransferRequest) (bool, error) { +func (s *session) checkIfFileTransferApproved(req *FileTransferRequest) (bool, error) { var participants []auth.SessionAccessContext for _, party := range req.approvers { @@ -1677,44 +1672,49 @@ func (s *session) checkIfFileTransferApproved(req *fileTransferRequest) (bool, e } // newFileTransferRequest takes FileTransferParams and creates a new fileTransferRequest struct -func (s *session) newFileTransferRequest(params *rsession.FileTransferRequestParams) *fileTransferRequest { - return &fileTransferRequest{ - id: uuid.New().String(), - requester: params.Requester, - location: params.Location, - filename: params.Filename, - download: params.Download, +func (s *session) newFileTransferRequest(params *rsession.FileTransferRequestParams) *FileTransferRequest { + return &FileTransferRequest{ + ID: uuid.New().String(), + Requester: params.Requester, + Location: params.Location, + Filename: params.Filename, + Download: params.Download, approvers: make(map[string]*party), } } // addFileTransferRequest will create a new file transfer request and add it to the current session's fileTransferRequests map // and broadcast the appropriate string to the session. -func (s *session) addFileTransferRequest(params *rsession.FileTransferRequestParams, scx *ServerContext) *fileTransferRequest { +func (s *session) addFileTransferRequest(params *rsession.FileTransferRequestParams, scx *ServerContext) error { s.mu.Lock() defer s.mu.Unlock() - req := s.newFileTransferRequest(params) - s.fileTransferRequests[req.id] = req + if s.fileTransferReq != nil { + return trace.AlreadyExists("a file transfer request already exists for this session") + } + + s.fileTransferReq = s.newFileTransferRequest(params) if params.Download { s.BroadcastMessage("User %s would like to download: %s", params.Requester, params.Location) } else { s.BroadcastMessage("User %s would like to upload %s to: %s", params.Requester, params.Filename, params.Location) } + s.registry.NotifyFileTransferRequest(s.fileTransferReq, FileTransferUpdate, scx) - s.registry.NotifyFileTransferRequest(req, FileTransferUpdate, scx) - return req + return nil } // approveFileTransferRequest will add the approver to the approvers map of a file transfer request and notify the members // of the session if the updated approvers map would fulfill the moderated policy. -func (s *session) approveFileTransferRequest(params *rsession.FileTransferDecisionParams, scx *ServerContext) (*fileTransferRequest, error) { +func (s *session) approveFileTransferRequest(params *rsession.FileTransferDecisionParams, scx *ServerContext) error { s.mu.Lock() defer s.mu.Unlock() - fileTransferReq := s.fileTransferRequests[params.RequestID] - if fileTransferReq == nil { - return nil, trace.NotFound("File Transfer Request %s not found", params.RequestID) + if s.fileTransferReq == nil { + return trace.NotFound("File Transfer Request %s not found", params.RequestID) + } + if s.fileTransferReq.ID != params.RequestID { + return trace.BadParameter("current file transfer request is not %s", params.RequestID) } var approver *party @@ -1724,16 +1724,16 @@ func (s *session) approveFileTransferRequest(params *rsession.FileTransferDecisi } } if approver == nil { - return nil, trace.AccessDenied("cannot approve file transfer requests if not in the current moderated session") + return trace.AccessDenied("cannot approve file transfer requests if not in the current moderated session") } - fileTransferReq.approvers[approver.user] = approver - s.BroadcastMessage("%s approved file transfer request %s", scx.Identity.TeleportUser, fileTransferReq.id) + s.fileTransferReq.approvers[approver.user] = approver + s.BroadcastMessage("%s approved file transfer request %s", scx.Identity.TeleportUser, s.fileTransferReq.ID) // check if policy is fulfilled - approved, err := s.checkIfFileTransferApproved(fileTransferReq) + approved, err := s.checkIfFileTransferApproved(s.fileTransferReq) if err != nil { - return nil, trace.Wrap(err) + return trace.Wrap(err) } var eventType FileTransferRequestEvent @@ -1743,21 +1743,25 @@ func (s *session) approveFileTransferRequest(params *rsession.FileTransferDecisi eventType = FileTransferUpdate } - s.registry.NotifyFileTransferRequest(fileTransferReq, eventType, scx) + s.registry.NotifyFileTransferRequest(s.fileTransferReq, eventType, scx) - return fileTransferReq, nil + return nil } // 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. -func (s *session) denyFileTransferRequest(params *rsession.FileTransferDecisionParams, scx *ServerContext) (*fileTransferRequest, error) { +func (s *session) denyFileTransferRequest(params *rsession.FileTransferDecisionParams, scx *ServerContext) error { s.mu.Lock() defer s.mu.Unlock() - fileTransferReq := s.fileTransferRequests[params.RequestID] - if fileTransferReq == nil { - return nil, trace.NotFound("file transfer request %s not found", params.RequestID) + + if s.fileTransferReq == nil { + return trace.NotFound("file transfer request %s not found", params.RequestID) } + if s.fileTransferReq.ID != params.RequestID { + return trace.BadParameter("current file transfer request is not %s", params.RequestID) + } + var denier *party for _, p := range s.parties { if p.ctx.ID() == scx.ID() { @@ -1765,15 +1769,16 @@ func (s *session) denyFileTransferRequest(params *rsession.FileTransferDecisionP } } if denier == nil { - return nil, trace.AccessDenied("cannot deny file transfer requests if not in the current moderated session") + return trace.AccessDenied("cannot deny file transfer requests if not in the current moderated session") } - delete(s.fileTransferRequests, fileTransferReq.id) + req := s.fileTransferReq + s.fileTransferReq = nil - s.BroadcastMessage("%s denied file transfer request %s", scx.Identity.TeleportUser, fileTransferReq.id) - s.registry.NotifyFileTransferRequest(fileTransferReq, FileTransferDenied, scx) + s.BroadcastMessage("%s denied file transfer request %s", scx.Identity.TeleportUser, req.ID) + s.registry.NotifyFileTransferRequest(req, FileTransferDenied, scx) - return fileTransferReq, nil + return nil } func (s *session) checkIfStart() (bool, auth.PolicyOptions, error) { diff --git a/lib/srv/termhandlers.go b/lib/srv/termhandlers.go index c8d8b36af0644..1ab5bed136348 100644 --- a/lib/srv/termhandlers.go +++ b/lib/srv/termhandlers.go @@ -148,12 +148,10 @@ func (t *TermHandlers) HandleFileTransferDecision(ctx context.Context, ch ssh.Ch } if params.Approved { - _, err := session.approveFileTransferRequest(params, scx) - return trace.Wrap(err) + return trace.Wrap(session.approveFileTransferRequest(params, scx)) } - _, err = session.denyFileTransferRequest(params, scx) - return trace.Wrap(err) + return trace.Wrap(session.denyFileTransferRequest(params, scx)) } // HandleFileTransferRequest handles requests of type "file-transfer-request" which will @@ -172,8 +170,7 @@ func (t *TermHandlers) HandleFileTransferRequest(ctx context.Context, ch ssh.Cha return nil } - session.addFileTransferRequest(params, scx) - return nil + return trace.Wrap(session.addFileTransferRequest(params, scx)) } // HandleWinChange handles requests of type "window-change" which update the diff --git a/lib/sshutils/sftp/http.go b/lib/sshutils/sftp/http.go index 41c8df4a66f5e..1c075e5b9dbfe 100644 --- a/lib/sshutils/sftp/http.go +++ b/lib/sshutils/sftp/http.go @@ -38,14 +38,6 @@ import ( type contextKey string const ( - // FileTransferDstPath is the dstPath (location) for the requested file transfer. This would be equal - // to the file to be downloaded, or location for a file to be uploaded. - FileTransferDstPath string = "TELEPORT_FILE_TRANSFER_DST_PATH" - // FileTransferRequestID is an optional parameter id of an file transfer request that has gone through - // an approval process during a moderated session to allow a file transfer scp command to be executed - // used as a value in the file transfer context and env var for exec session - FileTransferRequestID contextKey = "TELEPORT_FILE_TRANSFER_REQUEST_ID" - // ModeratedSessionID is an optional parameter sent during SCP requests to specify which moderated session // to check for valid FileTransferRequests // used as a value in the file transfer context and env var for exec session diff --git a/lib/sshutils/sftp/sftp.go b/lib/sshutils/sftp/sftp.go index 4e123b779f33c..e7a9961c664a9 100644 --- a/lib/sshutils/sftp/sftp.go +++ b/lib/sshutils/sftp/sftp.go @@ -239,17 +239,11 @@ func (c *Config) TransferFiles(ctx context.Context, sshClient *ssh.Client) error } defer s.Close() - // File transfers in a moderated session require these two variables - // to check for approval on the ssh server. If they exist in the - // context, set them in our env vars + // File transfers in a moderated session require this variable + // to check for approval on the ssh server if moderatedSessionID, ok := ctx.Value(ModeratedSessionID).(string); ok { s.Setenv(string(ModeratedSessionID), moderatedSessionID) } - if fileTransferRequestID, ok := ctx.Value(FileTransferRequestID).(string); ok { - s.Setenv(string(FileTransferRequestID), fileTransferRequestID) - } - // set dstPath in env var to check against file transfer request location - s.Setenv(FileTransferDstPath, c.dstPath) pe, err := s.StderrPipe() if err != nil { diff --git a/lib/web/files.go b/lib/web/files.go index 8e9b89fdc0ace..71327282f20f7 100644 --- a/lib/web/files.go +++ b/lib/web/files.go @@ -142,8 +142,6 @@ func (h *Handler) transferFile(w http.ResponseWriter, r *http.Request, p httprou ctx := r.Context() if req.fileTransferRequestID != "" { - // These values should never exist independently of each other so we can set them at the same time - ctx = context.WithValue(ctx, sftp.FileTransferRequestID, req.fileTransferRequestID) ctx = context.WithValue(ctx, sftp.ModeratedSessionID, req.moderatedSessionID) } diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 8dbcde7efd33a..a2c5df0cc90cf 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -20,12 +20,15 @@ package common import ( "bytes" + "encoding/json" "errors" "fmt" "io" "io/fs" "os" "os/user" + "path" + "strings" "time" "github.com/gogo/protobuf/jsonpb" @@ -38,6 +41,7 @@ import ( apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" + "github.com/gravitational/teleport/lib/srv" "github.com/gravitational/teleport/lib/utils" ) @@ -75,17 +79,99 @@ func (c compositeCh) Close() error { return trace.NewAggregate(c.r.Close(), c.w.Close()) } +type byteReader struct { + buf []byte + io.Reader +} + +func newByteReader(r io.Reader) *byteReader { + return &byteReader{ + Reader: r, + buf: make([]byte, 1), + } +} + +func (r *byteReader) ReadByte() (byte, error) { + _, err := r.Reader.Read(r.buf) + if err != nil { + return 0, err + } + + return r.buf[0], nil +} + +type allowedOps struct { + write bool + path string +} + // sftpHandler provides handlers for a SFTP server. type sftpHandler struct { - logger *log.Entry - events chan<- *apievents.SFTP + logger *log.Entry + allowed *allowedOps + events chan<- *apievents.SFTP } -func newSFTPHandler(logger *log.Entry, events chan<- *apievents.SFTP) *sftpHandler { +func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, homeDir string, events chan<- *apievents.SFTP) (*sftpHandler, error) { + var allowed *allowedOps + if req != nil { + allowed := &allowedOps{ + write: !req.Download, + } + // make filepaths consistent by ensuring all separators use backslashes + if req.Download { + allowed.path = path.Clean(req.Location) + } else { + allowed.path = path.Clean(req.Filename) + } + + // expand home dir if necessary + if strings.HasPrefix(allowed.path, "~/") { + allowed.path = path.Join(homeDir, allowed.path[2:]) + } + } + return &sftpHandler{ - logger: logger, - events: events, + logger: logger, + allowed: allowed, + events: events, + }, nil +} + +// checkReq returns an error if the SFTP request isn't allowed based on +// the approved file transfer request for this session. +func (s *sftpHandler) checkReq(req *sftp.Request) error { + if s.allowed == nil { + return nil } + + switch req.Method { + case methodLstat, methodStat: + // these methods are allowed + case methodGet: + if s.allowed.write { + return fmt.Errorf("%q is not allowed to be read from", req.Filepath) + } + case methodPut: + if !s.allowed.write { + return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + } + case methodOpen: + pflags := req.Pflags() + if !s.allowed.write && pflags.Write { + return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + } else if s.allowed.write && pflags.Read { + return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + } + default: + return fmt.Errorf("method %s is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + } + + if s.allowed.path == path.Clean(req.Filepath) { + return nil + } + + return fmt.Errorf("method %s is not allowed on %q", strings.ToLower(req.Method), req.Filepath) } // OpenFile handles 'open' requests when opening a file for reading @@ -131,6 +217,10 @@ func (s *sftpHandler) Filewrite(req *sftp.Request) (_ io.WriterAt, retErr error) } func (s *sftpHandler) openFile(req *sftp.Request) (*os.File, error) { + if err := s.checkReq(req); err != nil { + return nil, err + } + var flags int pflags := req.Pflags() if pflags.Append { @@ -174,6 +264,9 @@ func (s *sftpHandler) Filecmd(req *sftp.Request) (retErr error) { if req.Filepath == "" { return os.ErrInvalid } + if err := s.checkReq(req); err != nil { + return err + } switch req.Method { case methodSetStat: @@ -308,6 +401,9 @@ func (s *sftpHandler) Filelist(req *sftp.Request) (_ sftp.ListerAt, retErr error if req.Filepath == "" { return nil, os.ErrInvalid } + if err := s.checkReq(req); err != nil { + return nil, err + } switch req.Method { case methodList: @@ -346,6 +442,9 @@ func (s *sftpHandler) Lstat(req *sftp.Request) (sftp.ListerAt, error) { if req.Filepath == "" { return nil, os.ErrInvalid } + if err := s.checkReq(req); err != nil { + return nil, err + } fi, err := os.Lstat(req.Filepath) if err != nil { @@ -503,7 +602,6 @@ func onSFTP() error { // Ensure the parent process will receive log messages from us l := utils.NewLogger() - l.SetOutput(os.Stderr) logger := l.WithField(trace.Component, teleport.ComponentSubsystemSFTP) currentUser, err := user.Current() @@ -515,8 +613,35 @@ func onSFTP() error { return trace.Wrap(err) } + // Read the file transfer request for this session without buffering + // so that this file can be used to read from an SFTP connection + // below + reqReader := newByteReader(chr) + var encodedReq []byte + var fileTransferReq *srv.FileTransferRequest + for { + b, err := reqReader.ReadByte() + if err != nil { + return trace.Wrap(err) + } + // the encoded request will end with a null byte + if b == 0x0 { + break + } + encodedReq = append(encodedReq, b) + } + if len(encodedReq) != 0 { + fileTransferReq = new(srv.FileTransferRequest) + if err := json.Unmarshal(encodedReq, fileTransferReq); err != nil { + return trace.Wrap(err) + } + } + sftpEvents := make(chan *apievents.SFTP, 1) - h := newSFTPHandler(logger, sftpEvents) + h, err := newSFTPHandler(logger, fileTransferReq, currentUser.HomeDir, sftpEvents) + if err != nil { + return trace.Wrap(err) + } handler := sftp.Handlers{ FileGet: h, FilePut: h, From 9459ce46675a3789f1c111bb39d73c0fe3d3364f Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Mon, 26 Feb 2024 15:21:09 -0700 Subject: [PATCH 02/13] fix sess test --- lib/srv/sess_test.go | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/lib/srv/sess_test.go b/lib/srv/sess_test.go index dad23b6cdbb01..b5b60698df2e1 100644 --- a/lib/srv/sess_test.go +++ b/lib/srv/sess_test.go @@ -141,31 +141,25 @@ func TestIsApprovedFileTransfer(t *testing.T) { name string expectedResult bool expectedError string - req *fileTransferRequest + req *FileTransferRequest reqID string location string }{ { - name: "no file request found with supplied ID", + name: "no pending file request", expectedResult: false, - expectedError: "", + expectedError: "Session does not have a pending file transfer request", reqID: "", req: nil, }, - { - name: "no file request found with supplied ID", - expectedResult: false, - expectedError: "File transfer request not found", - reqID: "111", - req: nil, - }, { name: "current requester does not match original requester", expectedResult: false, expectedError: "Teleport user does not match original requester", reqID: "123", - req: &fileTransferRequest{ - requester: "michael", + req: &FileTransferRequest{ + ID: "123", + Requester: "michael", approvers: make(map[string]*party), }, }, @@ -175,10 +169,11 @@ func TestIsApprovedFileTransfer(t *testing.T) { expectedError: "requested destination path does not match the current request", reqID: "123", location: "~/Downloads", - req: &fileTransferRequest{ - requester: "michael", + req: &FileTransferRequest{ + ID: "123", + Requester: "teleportUser", approvers: make(map[string]*party), - location: "~/badlocation", + Location: "~/badlocation", }, }, { @@ -187,10 +182,11 @@ func TestIsApprovedFileTransfer(t *testing.T) { expectedError: "", reqID: "123", location: "~/Downloads", - req: &fileTransferRequest{ - requester: "teleportUser", + req: &FileTransferRequest{ + ID: "123", + Requester: "teleportUser", approvers: approvers, - location: "~/Downloads", + Location: "~/Downloads", }, }, } @@ -200,16 +196,12 @@ func TestIsApprovedFileTransfer(t *testing.T) { // create and add a session to the registry sess, _ := testOpenSession(t, reg, accessRoleSet) - // create a fileTransferRequest. can be nil - sess.fileTransferRequests = map[string]*fileTransferRequest{ - "123": tt.req, - } + // create a FileTransferRequest. can be nil + sess.fileTransferReq = tt.req // new exec request context scx := newTestServerContext(t, reg.Srv, accessRoleSet) scx.SetEnv(string(sftp.ModeratedSessionID), sess.ID()) - scx.SetEnv(string(sftp.FileTransferRequestID), tt.reqID) - scx.SetEnv(sftp.FileTransferDstPath, tt.location) result, err := reg.isApprovedFileTransfer(scx) if err != nil { require.Equal(t, tt.expectedError, err.Error()) From 7ac45f89765a1acee626a7c3fa28c3521a5c9ff7 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Tue, 27 Feb 2024 15:56:48 -0700 Subject: [PATCH 03/13] wip integration test --- integration/integration_test.go | 109 ++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/integration/integration_test.go b/integration/integration_test.go index c5f60f3db44fc..2cd9bfebc2da4 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -193,6 +193,7 @@ func TestIntegrations(t *testing.T) { t.Run("DifferentPinnedIP", suite.bind(testDifferentPinnedIP)) t.Run("JoinOverReverseTunnelOnly", suite.bind(testJoinOverReverseTunnelOnly)) t.Run("SFTP", suite.bind(testSFTP)) + t.Run("ModeratedSFTP", suite.bind(testModeratedSFTP)) t.Run("EscapeSequenceTriggers", suite.bind(testEscapeSequenceTriggers)) t.Run("AuthLocalNodeControlStream", suite.bind(testAuthLocalNodeControlStream)) t.Run("AgentlessConnection", suite.bind(testAgentlessConnection)) @@ -8045,6 +8046,111 @@ func getRemoteAddrString(sshClientString string) string { return fmt.Sprintf("%s:%s", parts[0], parts[1]) } +func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { + modules.SetTestModules(t, &modules.TestModules{ + TestBuildType: modules.BuildEnterprise, + }) + + // Create Teleport instance + teleport := suite.newTeleport(t, nil, true) + t.Cleanup(func() { + teleport.StopAll() + }) + + ctx := context.Background() + aSrv := teleport.Process.GetAuthServer() + + // Create peer and moderator users and roles + username := suite.Me.Username + peerUsername := username + "-peer" + sshAccessRole, err := types.NewRole("ssh-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Logins: []string{username}, + NodeLabels: types.Labels{ + types.Wildcard: []string{types.Wildcard}, + }, + }, + }) + require.NoError(t, err) + _, err = aSrv.CreateRole(ctx, sshAccessRole) + require.NoError(t, err) + + peerRole, err := types.NewRole("peer", types.RoleSpecV6{ + Allow: types.RoleConditions{ + RequireSessionJoin: []*types.SessionRequirePolicy{ + { + Name: "Requires oversight", + Filter: `equals("true", "true")`, + Kinds: []string{ + string(types.SSHSessionKind), + }, + Count: 1, + Modes: []string{ + string(types.SessionModeratorMode), + }, + OnLeave: string(types.OnSessionLeaveTerminate), + }, + }, + }, + }) + require.NoError(t, err) + _, err = aSrv.CreateRole(ctx, peerRole) + require.NoError(t, err) + + peerUser, err := types.NewUser(peerUsername) + require.NoError(t, err) + peerUser.SetLogins([]string{username}) + peerUser.SetRoles([]string{sshAccessRole.GetName(), peerRole.GetName()}) + _, err = aSrv.CreateUser(ctx, peerUser) + require.NoError(t, err) + + modUsername := username + "-moderator" + modRole, err := types.NewRole("moderator", types.RoleSpecV6{ + Allow: types.RoleConditions{ + JoinSessions: []*types.SessionJoinPolicy{{ + Name: "Session moderator", + Roles: []string{sshAccessRole.GetName()}, + Kinds: []string{string(types.SSHSessionKind)}, + Modes: []string{string(types.SessionModeratorMode), string(types.SessionObserverMode)}, + }}, + }, + }) + require.NoError(t, err) + _, err = aSrv.CreateRole(ctx, modRole) + require.NoError(t, err) + + modUser, err := types.NewUser(modUsername) + require.NoError(t, err) + peerUser.SetLogins([]string{username}) + peerUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) + _, err = aSrv.CreateUser(ctx, modUser) + require.NoError(t, err) + + waitForNodesToRegister(t, teleport, helpers.Site) + + modTC, err := teleport.NewClient(helpers.ClientConfig{ + TeleportUser: modUsername, + Login: username, + Cluster: helpers.Site, + Host: Host, + }) + require.NoError(t, err) + + modClusterCli, err := modTC.ConnectToCluster(ctx) + require.NoError(t, err) + t.Cleanup(func() { + _ = modClusterCli.Close() + }) + + nodeDetails := client.NodeDetails{ + Addr: teleport.Config.SSH.Addr.Addr, + Namespace: modTC.Namespace, + Cluster: helpers.Site, + } + _, _, err = modClusterCli.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent) + require.NoError(t, err) +} + func testSFTP(t *testing.T, suite *integrationTestSuite) { // Create Teleport instance. teleport := suite.newTeleport(t, nil, true) @@ -8080,6 +8186,9 @@ func testSFTP(t *testing.T, suite *integrationTestSuite) { suite.Me.Username, ) require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, nodeClient.Close()) + }) sftpClient, err := sftp.NewClient(nodeClient.Client.Client) require.NoError(t, err) From b7e949bc5be1bdb3b22607d9a993713358e76180 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Tue, 27 Feb 2024 18:28:07 -0700 Subject: [PATCH 04/13] integration test working --- integration/integration_test.go | 220 ++++++++++++++++++++++++++++++-- tool/teleport/common/sftp.go | 6 +- 2 files changed, 210 insertions(+), 16 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 2cd9bfebc2da4..f8c3203331350 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -100,6 +100,7 @@ import ( "github.com/gravitational/teleport/lib/session" "github.com/gravitational/teleport/lib/srv/alpnproxy/common" "github.com/gravitational/teleport/lib/sshutils" + telesftp "github.com/gravitational/teleport/lib/sshutils/sftp" "github.com/gravitational/teleport/lib/sshutils/x11" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" @@ -8052,13 +8053,13 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }) // Create Teleport instance - teleport := suite.newTeleport(t, nil, true) + instance := suite.newTeleport(t, nil, true) t.Cleanup(func() { - teleport.StopAll() + instance.StopAll() }) ctx := context.Background() - aSrv := teleport.Process.GetAuthServer() + aSrv := instance.Process.GetAuthServer() // Create peer and moderator users and roles username := suite.Me.Username @@ -8109,7 +8110,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { Allow: types.RoleConditions{ JoinSessions: []*types.SessionJoinPolicy{{ Name: "Session moderator", - Roles: []string{sshAccessRole.GetName()}, + Roles: []string{peerRole.GetName()}, Kinds: []string{string(types.SSHSessionKind)}, Modes: []string{string(types.SessionModeratorMode), string(types.SessionObserverMode)}, }}, @@ -8121,14 +8122,71 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { modUser, err := types.NewUser(modUsername) require.NoError(t, err) - peerUser.SetLogins([]string{username}) - peerUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) + modUser.SetLogins([]string{username}) + modUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) _, err = aSrv.CreateUser(ctx, modUser) require.NoError(t, err) - waitForNodesToRegister(t, teleport, helpers.Site) + waitForNodesToRegister(t, instance, helpers.Site) + + // Start a shell so a moderated session is created + peerTC, err := instance.NewClient(helpers.ClientConfig{ + TeleportUser: peerUsername, + Login: username, + Cluster: helpers.Site, + Host: Host, + }) + require.NoError(t, err) + + peerClusterCli, err := peerTC.ConnectToCluster(ctx) + require.NoError(t, err) + t.Cleanup(func() { + _ = peerClusterCli.Close() + }) + + nodeDetails := client.NodeDetails{ + Addr: instance.Config.SSH.Addr.Addr, + Namespace: peerTC.Namespace, + Cluster: helpers.Site, + } + peerNodeCli, err := peerTC.ConnectToNode( + ctx, + peerClusterCli, + nodeDetails, + username, + ) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, peerNodeCli.Close()) + }) + + peerSSH := peerNodeCli.Client + cmdSess, err := peerSSH.NewSession(ctx) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, cmdSess.Close()) + }) - modTC, err := teleport.NewClient(helpers.ClientConfig{ + peerTerm := NewTerminal(250) + cmdSess.Stdin = peerTerm + cmdSess.Stdout = peerTerm + cmdSess.Stderr = peerTerm + err = cmdSess.Shell(ctx) + require.NoError(t, err) + + var sessTracker types.SessionTracker + require.Eventually(t, func() bool { + trackers, err := peerClusterCli.AuthClient.GetActiveSessionTrackers(ctx) + require.NoError(t, err) + if len(trackers) == 1 { + sessTracker = trackers[0] + return true + } + return false + }, 5*time.Second, 100*time.Millisecond) + + // Join the waiting session so it is approved + modTC, err := instance.NewClient(helpers.ClientConfig{ TeleportUser: modUsername, Login: username, Cluster: helpers.Site, @@ -8142,13 +8200,149 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { _ = modClusterCli.Close() }) - nodeDetails := client.NodeDetails{ - Addr: teleport.Config.SSH.Addr.Addr, - Namespace: modTC.Namespace, - Cluster: helpers.Site, + conn, details, err := modClusterCli.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent) + require.NoError(t, err) + sshConfig := modClusterCli.ProxyClient.SSHConfig(username) + modSSHConn, modSSHChans, modSSHReqs, err := tracessh.NewClientConn(ctx, conn, nodeDetails.ProxyFormat(), sshConfig) + require.NoError(t, err) + + // We pass an empty channel which we close right away to ssh.NewClient + // because the client need to handle requests itself. + emptyCh := make(chan *ssh.Request) + close(emptyCh) + modNodeCli := client.NodeClient{ + Client: tracessh.NewClient(modSSHConn, modSSHChans, emptyCh), + Namespace: nodeDetails.Namespace, + TC: modTC, + Tracer: modTC.Tracer, + FIPSEnabled: details.FIPS, + ProxyPublicAddr: modTC.WebProxyAddr, } - _, _, err = modClusterCli.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent) + + modSess, err := modNodeCli.Client.NewSession(ctx) + require.NoError(t, err) + err = modSess.Setenv(ctx, sshutils.SessionEnvVar, sessTracker.GetSessionID()) + require.NoError(t, err) + err = modSess.Setenv(ctx, teleport.EnvSSHJoinMode, string(types.SessionModeratorMode)) + require.NoError(t, err) + + modTerm := NewTerminal(250) + modSess.Stdin = modTerm + modSess.Stdout = modTerm + modSess.Stderr = modTerm + err = modSess.Shell(ctx) + require.NoError(t, err) + + // Create and approve a file download request + tempDir := t.TempDir() + dlFile := filepath.Join(tempDir, "dl-file") + err = os.WriteFile(dlFile, []byte("contents"), 0o666) + require.NoError(t, err) + + err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ + Download: true, + Location: dlFile, + }) + require.NoError(t, err) + + // Ignore session join event + <-modSSHReqs + + sshReq := <-modSSHReqs + var fileReq apievents.FileTransferRequestEvent + err = json.Unmarshal(sshReq.Payload, &fileReq) + require.NoError(t, err) + + err = modSess.ApproveFileTransferRequest(ctx, fileReq.RequestID) + require.NoError(t, err) + + // Ignore file transfer request approve event + <-modSSHReqs + + // Test that only operations needed to complete the download + // are allowed + transferSess, err := peerSSH.NewSession(ctx) + require.NoError(t, err) + t.Cleanup(func() { + _ = transferSess.Close() + }) + + err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID()) + require.NoError(t, err) + + err = transferSess.RequestSubsystem(ctx, "sftp") + require.NoError(t, err) + w, err := transferSess.StdinPipe() + require.NoError(t, err) + r, err := transferSess.StdoutPipe() + require.NoError(t, err) + sftpClient, err := sftp.NewClientPipe(r, w) + require.NoError(t, err) + + _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) + require.ErrorContains(t, err, `method \"get\" is not allowed`) + _, err = sftpClient.OpenFile(filepath.Join(tempDir, "bad-file"), os.O_WRONLY) + require.ErrorContains(t, err, `is not allowed to be written to`) + err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) + require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) + + // Opening the requested file for reading should work + rf, err := sftpClient.Open(dlFile) + require.NoError(t, err) + require.NoError(t, rf.Close()) + + require.NoError(t, sftpClient.Close()) + + // Create and approve a file upload request + err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ + Download: false, + Filename: dlFile, + }) + require.NoError(t, err) + + sshReq = <-modSSHReqs + err = json.Unmarshal(sshReq.Payload, &fileReq) + require.NoError(t, err) + + err = modSess.ApproveFileTransferRequest(ctx, fileReq.RequestID) + require.NoError(t, err) + + // Ignore file transfer request approve event + <-modSSHReqs + + // Ignore EOF error + _ = transferSess.Close() + transferSess, err = peerSSH.NewSession(ctx) + require.NoError(t, err) + t.Cleanup(func() { + _ = transferSess.Close() + }) + + err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID()) + require.NoError(t, err) + + // Test that only operations needed to complete the download + // are allowed + err = transferSess.RequestSubsystem(ctx, "sftp") + require.NoError(t, err) + w, err = transferSess.StdinPipe() + require.NoError(t, err) + r, err = transferSess.StdoutPipe() + require.NoError(t, err) + sftpClient, err = sftp.NewClientPipe(r, w) + require.NoError(t, err) + + _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) + require.ErrorContains(t, err, `is not allowed to be read from`) + _, err = sftpClient.OpenFile(filepath.Join(tempDir, "bad-file"), os.O_RDONLY) + require.ErrorContains(t, err, `is not allowed to be read from`) + err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) + require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) + + // Opening the requested file for reading should work + wf, err := sftpClient.OpenFile(dlFile, os.O_WRONLY) require.NoError(t, err) + require.NoError(t, wf.Close()) } func testSFTP(t *testing.T, suite *integrationTestSuite) { diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index a2c5df0cc90cf..cd7511b403f61 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -115,7 +115,7 @@ type sftpHandler struct { func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, homeDir string, events chan<- *apievents.SFTP) (*sftpHandler, error) { var allowed *allowedOps if req != nil { - allowed := &allowedOps{ + allowed = &allowedOps{ write: !req.Download, } // make filepaths consistent by ensuring all separators use backslashes @@ -164,14 +164,14 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { return fmt.Errorf("%q is not allowed to be written to", req.Filepath) } default: - return fmt.Errorf("method %s is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) } if s.allowed.path == path.Clean(req.Filepath) { return nil } - return fmt.Errorf("method %s is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) } // OpenFile handles 'open' requests when opening a file for reading From c2d82268b9a71811635d1148adf3248a5ead8e04 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Tue, 27 Feb 2024 20:46:46 -0700 Subject: [PATCH 05/13] tweak some error messages and test cases --- integration/integration_test.go | 32 +++++++++++++++++++++----------- tool/teleport/common/sftp.go | 12 +++++++----- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index f8c3203331350..96ef8043247e7 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8235,13 +8235,13 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Create and approve a file download request tempDir := t.TempDir() - dlFile := filepath.Join(tempDir, "dl-file") - err = os.WriteFile(dlFile, []byte("contents"), 0o666) + reqFile := filepath.Join(tempDir, "req-file") + err = os.WriteFile(reqFile, []byte("contents"), 0o666) require.NoError(t, err) err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: true, - Location: dlFile, + Location: reqFile, }) require.NoError(t, err) @@ -8279,15 +8279,20 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { sftpClient, err := sftp.NewClientPipe(r, w) require.NoError(t, err) + // A file not in the request shouldn't be allowed _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) require.ErrorContains(t, err, `method \"get\" is not allowed`) - _, err = sftpClient.OpenFile(filepath.Join(tempDir, "bad-file"), os.O_WRONLY) - require.ErrorContains(t, err, `is not allowed to be written to`) + // Since this is a download no files should be allowed to be written to + _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_WRONLY) + require.ErrorContains(t, err, `method \"put\" is not allowed`) + // Only stats, reads and writes should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) // Opening the requested file for reading should work - rf, err := sftpClient.Open(dlFile) + _, err = sftpClient.Stat(reqFile) + require.NoError(t, err) + rf, err := sftpClient.Open(reqFile) require.NoError(t, err) require.NoError(t, rf.Close()) @@ -8296,7 +8301,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Create and approve a file upload request err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: false, - Filename: dlFile, + Filename: reqFile, }) require.NoError(t, err) @@ -8332,15 +8337,20 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { sftpClient, err = sftp.NewClientPipe(r, w) require.NoError(t, err) + // A file not in the request shouldn't be allowed _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) - require.ErrorContains(t, err, `is not allowed to be read from`) - _, err = sftpClient.OpenFile(filepath.Join(tempDir, "bad-file"), os.O_RDONLY) - require.ErrorContains(t, err, `is not allowed to be read from`) + require.ErrorContains(t, err, `method \"get\" is not allowed`) + // Since this is an upload no files should be allowed to be read from + _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_RDONLY) + require.ErrorContains(t, err, `method \"get\" is not allowed`) + // Only stats, reads and writes should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) // Opening the requested file for reading should work - wf, err := sftpClient.OpenFile(dlFile, os.O_WRONLY) + _, err = sftpClient.Stat(reqFile) + require.NoError(t, err) + wf, err := sftpClient.OpenFile(reqFile, os.O_WRONLY) require.NoError(t, err) require.NoError(t, wf.Close()) } diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index cd7511b403f61..9d61b27434e6c 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -145,14 +145,20 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { return nil } + if s.allowed.path != path.Clean(req.Filepath) { + return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + } + switch req.Method { case methodLstat, methodStat: // these methods are allowed case methodGet: + // only allow reads for downloads if s.allowed.write { return fmt.Errorf("%q is not allowed to be read from", req.Filepath) } case methodPut: + // only allow writes for uploads if !s.allowed.write { return fmt.Errorf("%q is not allowed to be written to", req.Filepath) } @@ -167,11 +173,7 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) } - if s.allowed.path == path.Clean(req.Filepath) { - return nil - } - - return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return nil } // OpenFile handles 'open' requests when opening a file for reading From d6f70453e69fdc894180531dcb2d5269166caf6b Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Wed, 28 Feb 2024 13:11:16 -0700 Subject: [PATCH 06/13] allow setstat for uploads --- integration/integration_test.go | 20 +++++++++++++++----- tool/teleport/common/sftp.go | 29 +++++++++++++++++++---------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 96ef8043247e7..85005776d1774 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8285,13 +8285,19 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Since this is a download no files should be allowed to be written to _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_WRONLY) require.ErrorContains(t, err, `method \"put\" is not allowed`) - // Only stats, reads and writes should be allowed + // Only stats and reads should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) + // Since this is a download no files should be allowed to have + // their permissions changed + err = sftpClient.Chmod(reqFile, 0o777) + require.ErrorContains(t, err, `method \"setstat\" is not allowed`) - // Opening the requested file for reading should work + // Only necessary operations should be allowed _, err = sftpClient.Stat(reqFile) require.NoError(t, err) + _, err = sftpClient.Lstat(reqFile) + require.NoError(t, err) rf, err := sftpClient.Open(reqFile) require.NoError(t, err) require.NoError(t, rf.Close()) @@ -8301,7 +8307,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Create and approve a file upload request err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: false, - Filename: reqFile, + Location: reqFile, }) require.NoError(t, err) @@ -8343,13 +8349,17 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Since this is an upload no files should be allowed to be read from _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_RDONLY) require.ErrorContains(t, err, `method \"get\" is not allowed`) - // Only stats, reads and writes should be allowed + // Only stats, writes, and chmods should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) - // Opening the requested file for reading should work + // Only necessary operations should be allowed _, err = sftpClient.Stat(reqFile) require.NoError(t, err) + _, err = sftpClient.Lstat(reqFile) + require.NoError(t, err) + err = sftpClient.Chmod(reqFile, 0o777) + require.NoError(t, err) wf, err := sftpClient.OpenFile(reqFile, os.O_WRONLY) require.NoError(t, err) require.NoError(t, wf.Close()) diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 9d61b27434e6c..b348c2052d716 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -119,15 +119,15 @@ func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, homeDir str write: !req.Download, } // make filepaths consistent by ensuring all separators use backslashes - if req.Download { - allowed.path = path.Clean(req.Location) - } else { - allowed.path = path.Clean(req.Filename) - } + allowed.path = path.Clean(req.Location) - // expand home dir if necessary if strings.HasPrefix(allowed.path, "~/") { + // expand home dir to make an absolute path allowed.path = path.Join(homeDir, allowed.path[2:]) + } else if !strings.Contains(allowed.path, "/") { + // if no directories are specified the file is assumed to + // be in the user's home dir + allowed.path = path.Join(homeDir, allowed.path) } } @@ -138,6 +138,10 @@ func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, homeDir str }, nil } +func newDisallowedErr(req *sftp.Request) error { + return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) +} + // checkReq returns an error if the SFTP request isn't allowed based on // the approved file transfer request for this session. func (s *sftpHandler) checkReq(req *sftp.Request) error { @@ -146,7 +150,7 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { } if s.allowed.path != path.Clean(req.Filepath) { - return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return newDisallowedErr(req) } switch req.Method { @@ -155,12 +159,12 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { case methodGet: // only allow reads for downloads if s.allowed.write { - return fmt.Errorf("%q is not allowed to be read from", req.Filepath) + return newDisallowedErr(req) } case methodPut: // only allow writes for uploads if !s.allowed.write { - return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + return newDisallowedErr(req) } case methodOpen: pflags := req.Pflags() @@ -169,8 +173,13 @@ func (s *sftpHandler) checkReq(req *sftp.Request) error { } else if s.allowed.write && pflags.Read { return fmt.Errorf("%q is not allowed to be written to", req.Filepath) } + case methodSetStat: + // only allow chmods for uploads + if !s.allowed.write { + return newDisallowedErr(req) + } default: - return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return newDisallowedErr(req) } return nil From 53d7a10c4122444a356b47b26f4ce80c019b3c05 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 29 Feb 2024 10:16:46 -0700 Subject: [PATCH 07/13] address nits from code review --- lib/srv/ctx.go | 8 ++------ lib/srv/regular/sshserver.go | 2 +- lib/srv/sess.go | 2 ++ tool/teleport/common/sftp.go | 14 +++++++------- 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index c4d1c7241d8d8..e25700a5ee7ef 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -1410,18 +1410,14 @@ func (c *ServerContext) setApprovedFileTransferRequest(req *FileTransferRequest) c.mu.Unlock() } -// GetApprovedFileTransferRequest will return the approved file transfer +// ConsumeApprovedFileTransferRequest will return the approved file transfer // request for this session if there is one present. Note that if an // approved request is returned future calls to this method will return // nil to prevent an approved request getting reused incorrectly. -func (c *ServerContext) GetApprovedFileTransferRequest() *FileTransferRequest { +func (c *ServerContext) ConsumeApprovedFileTransferRequest() *FileTransferRequest { c.mu.Lock() defer c.mu.Unlock() - if c.approvedFileReq == nil { - return nil - } - req := c.approvedFileReq c.approvedFileReq = nil diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index 1a810be892867..1c3d5d4e3564c 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -2169,7 +2169,7 @@ func (s *Server) parseSubsystemRequest(req *ssh.Request, ctx *srv.ServerContext) return nil, trace.Wrap(err) } - return newSFTPSubsys(ctx.GetApprovedFileTransferRequest()) + return newSFTPSubsys(ctx.ConsumeApprovedFileTransferRequest()) default: return nil, trace.BadParameter("unrecognized subsystem: %v", r.Name) } diff --git a/lib/srv/sess.go b/lib/srv/sess.go index 7c8b900afc3d7..0604e9558b058 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -1635,6 +1635,8 @@ func (s *session) checkPresence(ctx context.Context) error { // FileTransferRequest is a request to upload or download a file from a node. type FileTransferRequest struct { + // ID is a UUID that uniquely identifies a file transfer request + // and is unlikely to collide with another file transfer request ID string // Requester is the Teleport User that requested the file transfer Requester string diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index b348c2052d716..4d3deb69bd81c 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -142,9 +142,9 @@ func newDisallowedErr(req *sftp.Request) error { return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) } -// checkReq returns an error if the SFTP request isn't allowed based on -// the approved file transfer request for this session. -func (s *sftpHandler) checkReq(req *sftp.Request) error { +// ensureReqIsAllowed returns an error if the SFTP request isn't +// allowed based on the approved file transfer request for this session. +func (s *sftpHandler) ensureReqIsAllowed(req *sftp.Request) error { if s.allowed == nil { return nil } @@ -228,7 +228,7 @@ func (s *sftpHandler) Filewrite(req *sftp.Request) (_ io.WriterAt, retErr error) } func (s *sftpHandler) openFile(req *sftp.Request) (*os.File, error) { - if err := s.checkReq(req); err != nil { + if err := s.ensureReqIsAllowed(req); err != nil { return nil, err } @@ -275,7 +275,7 @@ func (s *sftpHandler) Filecmd(req *sftp.Request) (retErr error) { if req.Filepath == "" { return os.ErrInvalid } - if err := s.checkReq(req); err != nil { + if err := s.ensureReqIsAllowed(req); err != nil { return err } @@ -412,7 +412,7 @@ func (s *sftpHandler) Filelist(req *sftp.Request) (_ sftp.ListerAt, retErr error if req.Filepath == "" { return nil, os.ErrInvalid } - if err := s.checkReq(req); err != nil { + if err := s.ensureReqIsAllowed(req); err != nil { return nil, err } @@ -453,7 +453,7 @@ func (s *sftpHandler) Lstat(req *sftp.Request) (sftp.ListerAt, error) { if req.Filepath == "" { return nil, os.ErrInvalid } - if err := s.checkReq(req); err != nil { + if err := s.ensureReqIsAllowed(req); err != nil { return nil, err } From 3e72b0e6fbabd080a548ccb7253619fd30cb6498 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 29 Feb 2024 10:53:25 -0700 Subject: [PATCH 08/13] expand paths when necessary when creating file transfer request --- integration/integration_test.go | 1 + lib/srv/sess.go | 79 ++++++++++++++++++++++++++++----- tool/teleport/common/sftp.go | 14 ++---- 3 files changed, 72 insertions(+), 22 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 85005776d1774..2cd34459a1fec 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8307,6 +8307,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Create and approve a file upload request err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: false, + Filename: "upload-file", Location: reqFile, }) require.NoError(t, err) diff --git a/lib/srv/sess.go b/lib/srv/sess.go index 0604e9558b058..f2c907ff6e69c 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -24,7 +24,11 @@ import ( "errors" "fmt" "io" + "os" + "os/user" + "path" "slices" + "strings" "sync" "sync/atomic" "time" @@ -1674,15 +1678,61 @@ func (s *session) checkIfFileTransferApproved(req *FileTransferRequest) (bool, e } // newFileTransferRequest takes FileTransferParams and creates a new fileTransferRequest struct -func (s *session) newFileTransferRequest(params *rsession.FileTransferRequestParams) *FileTransferRequest { - return &FileTransferRequest{ +func (s *session) newFileTransferRequest(params *rsession.FileTransferRequestParams) (*FileTransferRequest, error) { + location, err := s.expandFileTransferRequestPath(params.Location) + if err != nil { + return nil, trace.Wrap(err) + } + + req := FileTransferRequest{ ID: uuid.New().String(), Requester: params.Requester, - Location: params.Location, + Location: location, Filename: params.Filename, Download: params.Download, approvers: make(map[string]*party), } + + return &req, nil +} + +func (s *session) expandFileTransferRequestPath(p string) (string, error) { + expanded := path.Clean(p) + + var tildePrefixed bool + var noBaseDir bool + if strings.HasPrefix(expanded, "~/") { + tildePrefixed = true + } else if !strings.Contains(expanded, "/") { + noBaseDir = true + } + + if tildePrefixed || noBaseDir { + localUser, err := user.Lookup(s.login) + if err != nil { + return "", trace.Wrap(err) + } + + exists, err := CheckHomeDir(localUser) + if err != nil { + return "", trace.Wrap(err) + } + homeDir := localUser.HomeDir + if !exists { + homeDir = string(os.PathSeparator) + } + + if tildePrefixed { + // expand home dir to make an absolute path + expanded = path.Join(homeDir, expanded[2:]) + } else { + // if no directories are specified SFTP will assume the file + // to be in the user's home dir + expanded = path.Join(homeDir, expanded) + } + } + + return expanded, nil } // addFileTransferRequest will create a new file transfer request and add it to the current session's fileTransferRequests map @@ -1694,16 +1744,24 @@ func (s *session) addFileTransferRequest(params *rsession.FileTransferRequestPar if s.fileTransferReq != nil { return trace.AlreadyExists("a file transfer request already exists for this session") } + if !params.Download && params.Filename == "" { + return trace.BadParameter("no source file is set for the upload") + } + + req, err := s.newFileTransferRequest(params) + if err != nil { + return trace.Wrap(err) + } + s.fileTransferReq = req - s.fileTransferReq = s.newFileTransferRequest(params) if params.Download { s.BroadcastMessage("User %s would like to download: %s", params.Requester, params.Location) } else { s.BroadcastMessage("User %s would like to upload %s to: %s", params.Requester, params.Filename, params.Location) } - s.registry.NotifyFileTransferRequest(s.fileTransferReq, FileTransferUpdate, scx) + err = s.registry.NotifyFileTransferRequest(s.fileTransferReq, FileTransferUpdate, scx) - return nil + return trace.Wrap(err) } // approveFileTransferRequest will add the approver to the approvers map of a file transfer request and notify the members @@ -1744,10 +1802,9 @@ func (s *session) approveFileTransferRequest(params *rsession.FileTransferDecisi } else { eventType = FileTransferUpdate } + err = s.registry.NotifyFileTransferRequest(s.fileTransferReq, eventType, scx) - s.registry.NotifyFileTransferRequest(s.fileTransferReq, eventType, scx) - - return nil + return trace.Wrap(err) } // denyFileTransferRequest will deny a file transfer request and remove it from the current session's file transfer requests map. @@ -1778,9 +1835,9 @@ func (s *session) denyFileTransferRequest(params *rsession.FileTransferDecisionP s.fileTransferReq = nil s.BroadcastMessage("%s denied file transfer request %s", scx.Identity.TeleportUser, req.ID) - s.registry.NotifyFileTransferRequest(req, FileTransferDenied, scx) + err := s.registry.NotifyFileTransferRequest(req, FileTransferDenied, scx) - return nil + return trace.Wrap(err) } func (s *session) checkIfStart() (bool, auth.PolicyOptions, error) { diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 4d3deb69bd81c..8b8129489a56c 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -112,23 +112,15 @@ type sftpHandler struct { events chan<- *apievents.SFTP } -func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, homeDir string, events chan<- *apievents.SFTP) (*sftpHandler, error) { +func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, events chan<- *apievents.SFTP) (*sftpHandler, error) { var allowed *allowedOps if req != nil { allowed = &allowedOps{ write: !req.Download, } + // TODO(capnspacehook): reject relative paths and symlinks // make filepaths consistent by ensuring all separators use backslashes allowed.path = path.Clean(req.Location) - - if strings.HasPrefix(allowed.path, "~/") { - // expand home dir to make an absolute path - allowed.path = path.Join(homeDir, allowed.path[2:]) - } else if !strings.Contains(allowed.path, "/") { - // if no directories are specified the file is assumed to - // be in the user's home dir - allowed.path = path.Join(homeDir, allowed.path) - } } return &sftpHandler{ @@ -649,7 +641,7 @@ func onSFTP() error { } sftpEvents := make(chan *apievents.SFTP, 1) - h, err := newSFTPHandler(logger, fileTransferReq, currentUser.HomeDir, sftpEvents) + h, err := newSFTPHandler(logger, fileTransferReq, sftpEvents) if err != nil { return trace.Wrap(err) } From 3c8554b8a6d036584af62531dbcbe05018a8326b Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 29 Feb 2024 10:56:12 -0700 Subject: [PATCH 09/13] improve UX of errors in web UI --- integration/integration_test.go | 14 +++++++------- tool/teleport/common/sftp.go | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 2cd34459a1fec..350728a0b2533 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8281,17 +8281,17 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // A file not in the request shouldn't be allowed _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) - require.ErrorContains(t, err, `method \"get\" is not allowed`) + require.ErrorContains(t, err, `method get is not allowed`) // Since this is a download no files should be allowed to be written to _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_WRONLY) - require.ErrorContains(t, err, `method \"put\" is not allowed`) + require.ErrorContains(t, err, `method put is not allowed`) // Only stats and reads should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) - require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) + require.ErrorContains(t, err, `method mkdir is not allowed`) // Since this is a download no files should be allowed to have // their permissions changed err = sftpClient.Chmod(reqFile, 0o777) - require.ErrorContains(t, err, `method \"setstat\" is not allowed`) + require.ErrorContains(t, err, `method setstat is not allowed`) // Only necessary operations should be allowed _, err = sftpClient.Stat(reqFile) @@ -8346,13 +8346,13 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // A file not in the request shouldn't be allowed _, err = sftpClient.Open(filepath.Join(tempDir, "bad-file")) - require.ErrorContains(t, err, `method \"get\" is not allowed`) + require.ErrorContains(t, err, `method get is not allowed`) // Since this is an upload no files should be allowed to be read from _, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_RDONLY) - require.ErrorContains(t, err, `method \"get\" is not allowed`) + require.ErrorContains(t, err, `method get is not allowed`) // Only stats, writes, and chmods should be allowed err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir")) - require.ErrorContains(t, err, `method \"mkdir\" is not allowed`) + require.ErrorContains(t, err, `method mkdir is not allowed`) // Only necessary operations should be allowed _, err = sftpClient.Stat(reqFile) diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 8b8129489a56c..80591b5dc3609 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -131,7 +131,7 @@ func newSFTPHandler(logger *log.Entry, req *srv.FileTransferRequest, events chan } func newDisallowedErr(req *sftp.Request) error { - return fmt.Errorf("method %q is not allowed on %q", strings.ToLower(req.Method), req.Filepath) + return fmt.Errorf("method %s is not allowed on %s", strings.ToLower(req.Method), req.Filepath) } // ensureReqIsAllowed returns an error if the SFTP request isn't @@ -161,9 +161,9 @@ func (s *sftpHandler) ensureReqIsAllowed(req *sftp.Request) error { case methodOpen: pflags := req.Pflags() if !s.allowed.write && pflags.Write { - return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + return fmt.Errorf("%s is not allowed to be written to", req.Filepath) } else if s.allowed.write && pflags.Read { - return fmt.Errorf("%q is not allowed to be written to", req.Filepath) + return fmt.Errorf("%s is not allowed to be written to", req.Filepath) } case methodSetStat: // only allow chmods for uploads From 0725621442a2236f526ad24fb3748df7241e8f6d Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 29 Feb 2024 11:11:53 -0700 Subject: [PATCH 10/13] deny request when an invalid user tries to transfer files --- lib/srv/sess.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/srv/sess.go b/lib/srv/sess.go index f2c907ff6e69c..72659ee5edbcf 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -466,6 +466,14 @@ func (s *SessionRegistry) isApprovedFileTransfer(scx *ServerContext) (bool, erro return false, trace.NotFound("Session does not have a pending file transfer request") } if sess.fileTransferReq.Requester != scx.Identity.TeleportUser { + // to be safe deny and remove the pending request if the user + // doesn't match what we expect + req := sess.fileTransferReq + sess.fileTransferReq = nil + + sess.BroadcastMessage("file transfer request %s denied due to %s attempting to transfer files", req.ID, scx.Identity.TeleportUser) + _ = s.NotifyFileTransferRequest(req, FileTransferDenied, scx) + return false, trace.AccessDenied("Teleport user does not match original requester") } From dcc16a1ae15f9538143c8fb03d12a226b6172778 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 7 Mar 2024 10:58:11 -0700 Subject: [PATCH 11/13] don't allow open requests, they aren't done by the webui --- tool/teleport/common/sftp.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 80591b5dc3609..9647b3c9453db 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -137,6 +137,7 @@ func newDisallowedErr(req *sftp.Request) error { // ensureReqIsAllowed returns an error if the SFTP request isn't // allowed based on the approved file transfer request for this session. func (s *sftpHandler) ensureReqIsAllowed(req *sftp.Request) error { + // no specifically allowed operations, all requests are allowed if s.allowed == nil { return nil } @@ -158,13 +159,6 @@ func (s *sftpHandler) ensureReqIsAllowed(req *sftp.Request) error { if !s.allowed.write { return newDisallowedErr(req) } - case methodOpen: - pflags := req.Pflags() - if !s.allowed.write && pflags.Write { - return fmt.Errorf("%s is not allowed to be written to", req.Filepath) - } else if s.allowed.write && pflags.Read { - return fmt.Errorf("%s is not allowed to be written to", req.Filepath) - } case methodSetStat: // only allow chmods for uploads if !s.allowed.write { From 0dfe4ea3351cb6dac5ec03a2d238fcc22b736d40 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 7 Mar 2024 17:02:06 -0700 Subject: [PATCH 12/13] address feedback --- integration/integration_test.go | 83 ++++++++++++++++++--------------- lib/srv/regular/sftp.go | 5 +- lib/srv/sess.go | 16 ++++--- tool/teleport/common/sftp.go | 15 +++--- 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 350728a0b2533..a5f5d00763a14 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8047,6 +8047,14 @@ func getRemoteAddrString(sshClientString string) string { return fmt.Sprintf("%s:%s", parts[0], parts[1]) } +func isNilOrEOFErr(t *testing.T, err error) { + t.Helper() + + if err != nil { + require.ErrorIs(t, err, io.EOF) + } +} + func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { modules.SetTestModules(t, &modules.TestModules{ TestBuildType: modules.BuildEnterprise, @@ -8059,7 +8067,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }) ctx := context.Background() - aSrv := instance.Process.GetAuthServer() + authServer := instance.Process.GetAuthServer() // Create peer and moderator users and roles username := suite.Me.Username @@ -8073,7 +8081,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }, }) require.NoError(t, err) - _, err = aSrv.CreateRole(ctx, sshAccessRole) + _, err = authServer.CreateRole(ctx, sshAccessRole) require.NoError(t, err) peerRole, err := types.NewRole("peer", types.RoleSpecV6{ @@ -8095,14 +8103,14 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }, }) require.NoError(t, err) - _, err = aSrv.CreateRole(ctx, peerRole) + _, err = authServer.CreateRole(ctx, peerRole) require.NoError(t, err) peerUser, err := types.NewUser(peerUsername) require.NoError(t, err) peerUser.SetLogins([]string{username}) peerUser.SetRoles([]string{sshAccessRole.GetName(), peerRole.GetName()}) - _, err = aSrv.CreateUser(ctx, peerUser) + _, err = authServer.CreateUser(ctx, peerUser) require.NoError(t, err) modUsername := username + "-moderator" @@ -8117,14 +8125,14 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }, }) require.NoError(t, err) - _, err = aSrv.CreateRole(ctx, modRole) + _, err = authServer.CreateRole(ctx, modRole) require.NoError(t, err) modUser, err := types.NewUser(modUsername) require.NoError(t, err) modUser.SetLogins([]string{username}) modUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) - _, err = aSrv.CreateUser(ctx, modUser) + _, err = authServer.CreateUser(ctx, modUser) require.NoError(t, err) waitForNodesToRegister(t, instance, helpers.Site) @@ -8138,10 +8146,10 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }) require.NoError(t, err) - peerClusterCli, err := peerTC.ConnectToCluster(ctx) + peerClusterClient, err := peerTC.ConnectToCluster(ctx) require.NoError(t, err) t.Cleanup(func() { - _ = peerClusterCli.Close() + require.NoError(t, peerClusterClient.Close()) }) nodeDetails := client.NodeDetails{ @@ -8149,40 +8157,38 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { Namespace: peerTC.Namespace, Cluster: helpers.Site, } - peerNodeCli, err := peerTC.ConnectToNode( + peerNodeClient, err := peerTC.ConnectToNode( ctx, - peerClusterCli, + peerClusterClient, nodeDetails, username, ) require.NoError(t, err) t.Cleanup(func() { - require.NoError(t, peerNodeCli.Close()) + require.NoError(t, peerNodeClient.Close()) }) - peerSSH := peerNodeCli.Client - cmdSess, err := peerSSH.NewSession(ctx) + peerSSH := peerNodeClient.Client + peerSess, err := peerSSH.NewSession(ctx) require.NoError(t, err) t.Cleanup(func() { - require.NoError(t, cmdSess.Close()) + require.NoError(t, peerSess.Close()) }) peerTerm := NewTerminal(250) - cmdSess.Stdin = peerTerm - cmdSess.Stdout = peerTerm - cmdSess.Stderr = peerTerm - err = cmdSess.Shell(ctx) + peerSess.Stdin = peerTerm + peerSess.Stdout = peerTerm + peerSess.Stderr = peerTerm + err = peerSess.Shell(ctx) require.NoError(t, err) var sessTracker types.SessionTracker - require.Eventually(t, func() bool { - trackers, err := peerClusterCli.AuthClient.GetActiveSessionTrackers(ctx) + require.EventuallyWithT(t, func(t *assert.CollectT) { + trackers, err := peerClusterClient.AuthClient.GetActiveSessionTrackers(ctx) require.NoError(t, err) - if len(trackers) == 1 { + if assert.Len(t, trackers, 1) { sessTracker = trackers[0] - return true } - return false }, 5*time.Second, 100*time.Millisecond) // Join the waiting session so it is approved @@ -8194,15 +8200,15 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }) require.NoError(t, err) - modClusterCli, err := modTC.ConnectToCluster(ctx) + modClusterClient, err := modTC.ConnectToCluster(ctx) require.NoError(t, err) t.Cleanup(func() { - _ = modClusterCli.Close() + require.NoError(t, modClusterClient.Close()) }) - conn, details, err := modClusterCli.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent) + conn, details, err := modClusterClient.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent) require.NoError(t, err) - sshConfig := modClusterCli.ProxyClient.SSHConfig(username) + sshConfig := modClusterClient.ProxyClient.SSHConfig(username) modSSHConn, modSSHChans, modSSHReqs, err := tracessh.NewClientConn(ctx, conn, nodeDetails.ProxyFormat(), sshConfig) require.NoError(t, err) @@ -8239,16 +8245,18 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { err = os.WriteFile(reqFile, []byte("contents"), 0o666) require.NoError(t, err) - err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ + err = peerSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: true, Location: reqFile, }) require.NoError(t, err) - // Ignore session join event - <-modSSHReqs - sshReq := <-modSSHReqs + var joinEvent apievents.SessionJoin + err = json.Unmarshal(sshReq.Payload, &joinEvent) + require.NoError(t, err) + + sshReq = <-modSSHReqs var fileReq apievents.FileTransferRequestEvent err = json.Unmarshal(sshReq.Payload, &fileReq) require.NoError(t, err) @@ -8264,13 +8272,13 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { transferSess, err := peerSSH.NewSession(ctx) require.NoError(t, err) t.Cleanup(func() { - _ = transferSess.Close() + isNilOrEOFErr(t, transferSess.Close()) }) err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID()) require.NoError(t, err) - err = transferSess.RequestSubsystem(ctx, "sftp") + err = transferSess.RequestSubsystem(ctx, teleport.SFTPSubsystem) require.NoError(t, err) w, err := transferSess.StdinPipe() require.NoError(t, err) @@ -8305,7 +8313,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { require.NoError(t, sftpClient.Close()) // Create and approve a file upload request - err = cmdSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ + err = peerSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{ Download: false, Filename: "upload-file", Location: reqFile, @@ -8322,12 +8330,11 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Ignore file transfer request approve event <-modSSHReqs - // Ignore EOF error - _ = transferSess.Close() + isNilOrEOFErr(t, transferSess.Close()) transferSess, err = peerSSH.NewSession(ctx) require.NoError(t, err) t.Cleanup(func() { - _ = transferSess.Close() + require.NoError(t, transferSess.Close()) }) err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID()) @@ -8335,7 +8342,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { // Test that only operations needed to complete the download // are allowed - err = transferSess.RequestSubsystem(ctx, "sftp") + err = transferSess.RequestSubsystem(ctx, teleport.SFTPSubsystem) require.NoError(t, err) w, err = transferSess.StdinPipe() require.NoError(t, err) diff --git a/lib/srv/regular/sftp.go b/lib/srv/regular/sftp.go index 5952c24e366d4..c65c18b1b1a69 100644 --- a/lib/srv/regular/sftp.go +++ b/lib/srv/regular/sftp.go @@ -132,7 +132,10 @@ func (s *sftpSubsys) Start(ctx context.Context, } execRequest.Continue() - // Send the file transfer request if applicable + // Send the file transfer request if applicable. The SFTP process + // expects the file transfer request data will end with a null byte, + // so if there is no request to send just send a null byte so the + // SFTP process can detect that no request was sent. encodedReq := []byte{0x0} if s.fileTransferReq != nil { encodedReq, err = json.Marshal(s.fileTransferReq) diff --git a/lib/srv/sess.go b/lib/srv/sess.go index 72659ee5edbcf..f8de4dbcf3704 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -28,7 +28,6 @@ import ( "os/user" "path" "slices" - "strings" "sync" "sync/atomic" "time" @@ -441,10 +440,10 @@ func (s *SessionRegistry) GetTerminalSize(sessionID string) (*term.Winsize, erro } func (s *SessionRegistry) isApprovedFileTransfer(scx *ServerContext) (bool, error) { - // If the sessID environment variable was not set, return not - // approved and no error. This means the file transfer came from - // a non-moderated session. sessionID will be passed after a - // moderated session approval process has completed. + // If the TELEPORT_MODERATED_SESSION_ID environment variable was not + // set, return not approved and no error. This means the file + // transfer came from a non-moderated session. sessionID will be + // passed after a moderated session approval process has completed. sessID, _ := scx.GetEnv(string(sftp.ModeratedSessionID)) if sessID == "" { return false, nil @@ -459,6 +458,8 @@ func (s *SessionRegistry) isApprovedFileTransfer(scx *ServerContext) (bool, erro return false, trace.NotFound("Session not found") } + // aquire the session mutex lock so sess.fileTransferReq doesn't get + // written while we're reading it sess.mu.Lock() defer sess.mu.Unlock() @@ -1706,12 +1707,13 @@ func (s *session) newFileTransferRequest(params *rsession.FileTransferRequestPar func (s *session) expandFileTransferRequestPath(p string) (string, error) { expanded := path.Clean(p) + dir := path.Dir(expanded) var tildePrefixed bool var noBaseDir bool - if strings.HasPrefix(expanded, "~/") { + if dir == "~" { tildePrefixed = true - } else if !strings.Contains(expanded, "/") { + } else if dir == "." { noBaseDir = true } diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index 9647b3c9453db..e05e3028e922a 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -79,6 +79,7 @@ func (c compositeCh) Close() error { return trace.NewAggregate(c.r.Close(), c.w.Close()) } +// byteReader is used to read one byte at a time without buffering. type byteReader struct { buf []byte io.Reader @@ -92,10 +93,13 @@ func newByteReader(r io.Reader) *byteReader { } func (r *byteReader) ReadByte() (byte, error) { - _, err := r.Reader.Read(r.buf) + n, err := r.Reader.Read(r.buf) if err != nil { return 0, err } + if n > 1 { + return 0, fmt.Errorf("expected to read 1 byte, read %d bytes", n) + } return r.buf[0], nil } @@ -154,13 +158,8 @@ func (s *sftpHandler) ensureReqIsAllowed(req *sftp.Request) error { if s.allowed.write { return newDisallowedErr(req) } - case methodPut: - // only allow writes for uploads - if !s.allowed.write { - return newDisallowedErr(req) - } - case methodSetStat: - // only allow chmods for uploads + case methodPut, methodSetStat: + // only allow writes and chmods for uploads if !s.allowed.write { return newDisallowedErr(req) } From f60120eeba553aba3394267505ce8d5b1c89efb8 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Fri, 8 Mar 2024 19:46:21 -0700 Subject: [PATCH 13/13] use a buffered reader when reading the file request, address a few nits --- integration/integration_test.go | 18 +++++++-------- lib/srv/sess.go | 2 +- tool/teleport/common/sftp.go | 40 +++++++++++++-------------------- 3 files changed, 26 insertions(+), 34 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index a5f5d00763a14..91abde54830b1 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -8128,17 +8128,17 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { _, err = authServer.CreateRole(ctx, modRole) require.NoError(t, err) - modUser, err := types.NewUser(modUsername) + moderatorUser, err := types.NewUser(modUsername) require.NoError(t, err) - modUser.SetLogins([]string{username}) - modUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) - _, err = authServer.CreateUser(ctx, modUser) + moderatorUser.SetLogins([]string{username}) + moderatorUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()}) + _, err = authServer.CreateUser(ctx, moderatorUser) require.NoError(t, err) waitForNodesToRegister(t, instance, helpers.Site) // Start a shell so a moderated session is created - peerTC, err := instance.NewClient(helpers.ClientConfig{ + peerClient, err := instance.NewClient(helpers.ClientConfig{ TeleportUser: peerUsername, Login: username, Cluster: helpers.Site, @@ -8146,7 +8146,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { }) require.NoError(t, err) - peerClusterClient, err := peerTC.ConnectToCluster(ctx) + peerClusterClient, err := peerClient.ConnectToCluster(ctx) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, peerClusterClient.Close()) @@ -8154,10 +8154,10 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { nodeDetails := client.NodeDetails{ Addr: instance.Config.SSH.Addr.Addr, - Namespace: peerTC.Namespace, + Namespace: peerClient.Namespace, Cluster: helpers.Site, } - peerNodeClient, err := peerTC.ConnectToNode( + peerNodeClient, err := peerClient.ConnectToNode( ctx, peerClusterClient, nodeDetails, @@ -8185,7 +8185,7 @@ func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) { var sessTracker types.SessionTracker require.EventuallyWithT(t, func(t *assert.CollectT) { trackers, err := peerClusterClient.AuthClient.GetActiveSessionTrackers(ctx) - require.NoError(t, err) + assert.NoError(t, err) if assert.Len(t, trackers, 1) { sessTracker = trackers[0] } diff --git a/lib/srv/sess.go b/lib/srv/sess.go index f8de4dbcf3704..183a3f2c627f5 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -458,7 +458,7 @@ func (s *SessionRegistry) isApprovedFileTransfer(scx *ServerContext) (bool, erro return false, trace.NotFound("Session not found") } - // aquire the session mutex lock so sess.fileTransferReq doesn't get + // acquire the session mutex lock so sess.fileTransferReq doesn't get // written while we're reading it sess.mu.Lock() defer sess.mu.Unlock() diff --git a/tool/teleport/common/sftp.go b/tool/teleport/common/sftp.go index e05e3028e922a..c897f086cc2af 100644 --- a/tool/teleport/common/sftp.go +++ b/tool/teleport/common/sftp.go @@ -19,6 +19,7 @@ package common import ( + "bufio" "bytes" "encoding/json" "errors" @@ -79,29 +80,22 @@ func (c compositeCh) Close() error { return trace.NewAggregate(c.r.Close(), c.w.Close()) } -// byteReader is used to read one byte at a time without buffering. -type byteReader struct { - buf []byte - io.Reader -} +// bufferedReaderCloser wraps a [bufio.Reader] to make it an [io.ReadCloser]. +type bufferedReaderCloser struct { + bufio.Reader -func newByteReader(r io.Reader) *byteReader { - return &byteReader{ - Reader: r, - buf: make([]byte, 1), - } + inner io.ReadCloser } -func (r *byteReader) ReadByte() (byte, error) { - n, err := r.Reader.Read(r.buf) - if err != nil { - return 0, err - } - if n > 1 { - return 0, fmt.Errorf("expected to read 1 byte, read %d bytes", n) +func newBufferedReaderCloser(r io.ReadCloser) *bufferedReaderCloser { + return &bufferedReaderCloser{ + Reader: *bufio.NewReader(r), + inner: r, } +} - return r.buf[0], nil +func (b *bufferedReaderCloser) Close() error { + return b.inner.Close() } type allowedOps struct { @@ -589,7 +583,6 @@ func onSFTP() error { return trace.Wrap(err) } defer chw.Close() - ch := compositeCh{chr, chw} auditFile, err := openFD(5, "audit") if err != nil { return trace.Wrap(err) @@ -609,14 +602,12 @@ func onSFTP() error { return trace.Wrap(err) } - // Read the file transfer request for this session without buffering - // so that this file can be used to read from an SFTP connection - // below - reqReader := newByteReader(chr) + // Read the file transfer request for this session if one exists + bufferedReader := newBufferedReaderCloser(chr) var encodedReq []byte var fileTransferReq *srv.FileTransferRequest for { - b, err := reqReader.ReadByte() + b, err := bufferedReader.ReadByte() if err != nil { return trace.Wrap(err) } @@ -632,6 +623,7 @@ func onSFTP() error { return trace.Wrap(err) } } + ch := compositeCh{bufferedReader, chw} sftpEvents := make(chan *apievents.SFTP, 1) h, err := newSFTPHandler(logger, fileTransferReq, sftpEvents)