From b9d7f36f92e2a40a273b8423555a90cb1215b190 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Mon, 23 Oct 2023 21:00:43 -0700 Subject: [PATCH 01/30] Use errors.Is This correctly handles wrapped errors as well. --- request-example.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/request-example.go b/request-example.go index 519b3b76..854fbe7c 100644 --- a/request-example.go +++ b/request-example.go @@ -72,7 +72,7 @@ func (fs *root) putfile(pathname string, file *memFile) error { return os.ErrInvalid } - if _, err := fs.lfetch(pathname); err != os.ErrNotExist { + if _, err := fs.lfetch(pathname); !errors.Is(err, os.ErrNotExist) { return os.ErrExist } @@ -209,7 +209,7 @@ func (fs *root) rename(oldpath, newpath string) error { } target, err := fs.lfetch(newpath) - if err != os.ErrNotExist { + if !errors.Is(err, os.ErrNotExist) { if target == file { // IEEE 1003.1: if oldpath and newpath are the same directory entry, // then return no error, and perform no further action. @@ -507,7 +507,7 @@ func (fs *root) exists(path string) bool { _, err = fs.lfetch(path) - return err != os.ErrNotExist + return !errors.Is(err, os.ErrNotExist) } func (fs *root) fetch(pathname string) (*memFile, error) { From 8513fed9dead1e709adae5e94c4ac8edcaebdd41 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Mon, 30 Oct 2023 17:01:14 -0700 Subject: [PATCH 02/30] Start tracking the file mode. --- request-example.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/request-example.go b/request-example.go index 854fbe7c..1ca132c3 100644 --- a/request-example.go +++ b/request-example.go @@ -110,6 +110,7 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { file := &memFile{ modtime: time.Now(), + mode: 0644, } if err := fs.putfile(pathname, file); err != nil { @@ -544,6 +545,7 @@ type memFile struct { modtime time.Time symlink string isdir bool + mode uint32 mu sync.RWMutex content []byte @@ -569,7 +571,7 @@ func (f *memFile) Mode() os.FileMode { if f.symlink != "" { return os.FileMode(0777) | os.ModeSymlink } - return os.FileMode(0644) + return os.FileMode(f.mode) } func (f *memFile) ModTime() time.Time { return f.modtime } func (f *memFile) IsDir() bool { return f.isdir } From 07a6b6a544afc48226467b32fd91400f3d0956fc Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Mon, 30 Oct 2023 17:02:10 -0700 Subject: [PATCH 03/30] Move memfile's Setstat function, add features. This makes it possible for memfile to handle mode changes, ctime changes, in addition to the already existing file size changes. --- request-example.go | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/request-example.go b/request-example.go index 1ca132c3..924de0c9 100644 --- a/request-example.go +++ b/request-example.go @@ -141,6 +141,25 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { return file, nil } +func (fs *root) SetStat(r *Request, flags *FileAttrFlags, attrs *FileStat) error { + file, err := fs.openfile(r.Filepath, sshFxfWrite) + if err != nil { + return err + } + + if flags.Permissions { + file.mode = attrs.Mode + } + // We only have mtime, not atime. + if flags.Acmodtime { + file.modtime = time.Unix(int64(attrs.Mtime), 0) + } + if flags.Size { + return file.Truncate(int64(attrs.Size)) + } + return nil +} + func (fs *root) Filecmd(r *Request) error { if fs.mockErr != nil { return fs.mockErr @@ -152,16 +171,8 @@ func (fs *root) Filecmd(r *Request) error { switch r.Method { case "Setstat": - file, err := fs.openfile(r.Filepath, sshFxfWrite) - if err != nil { - return err - } - - if r.AttrFlags().Size { - return file.Truncate(int64(r.Attributes().Size)) - } - - return nil + flags := r.AttrFlags() + return fs.SetStat(r, &flags, r.Attributes()) case "Rename": // SFTP-v2: "It is an error if there already exists a file with the name specified by newpath." From 7bf7a25d28a84d6862c0d21c5013d84f315bd459 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Mon, 30 Oct 2023 17:03:25 -0700 Subject: [PATCH 04/30] Make a new interface for direct SetStat access. I'm really not especially happy with this. It works, it's minimally invasive, and it feels like the wrong solution. I'm pretty sure that the right solution is a breaking change of the Request structure, to just flat out contain FileAttrFlags and FileStat, having requestFromPacket do the parsing into those structures. But that's an API change, and I doubt that we want a major version bump for this. As an alternative, we could have a new Request structure type, with that layout, and have public functions for converting between them. That would at least make it possible for someone to construct the Request structure for a Setstat command. The use case of this is being able to populate a memfile backed sftp server with test data that has specific timestamps, as part of the test framework of an internal program. I'm definitely open to other ideas on how to solve this problem. --- request-interfaces.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/request-interfaces.go b/request-interfaces.go index da56ed40..63411f23 100644 --- a/request-interfaces.go +++ b/request-interfaces.go @@ -155,3 +155,9 @@ type ListerAt interface { type TransferError interface { TransferError(err error) } + +// FileStatSetter is an optional interface that a writerAt could implement in order to allow direct setting of stats. +// This is primarily used for the inmemhandler backend. +type FileStatSetter interface { + SetStat(r *Request, flags *FileAttrFlags, attrs *FileStat) error +} From 6b2b978a186a545706f8148e7c5fcfe627aa732f Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Mon, 30 Oct 2023 17:55:17 -0700 Subject: [PATCH 05/30] Add SetAttributes as an alternate option. This would replace the SetStat public function from the last commit. Instead, this makes it practical to construct a Setstat request. From some quick testing, this appears to do the job correctly, and it feels like a lot less of hack. It still doesn't feel entirely right, but it's closer. --- request-attrs.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/request-attrs.go b/request-attrs.go index b5c95b4a..15c82559 100644 --- a/request-attrs.go +++ b/request-attrs.go @@ -44,6 +44,37 @@ func newFileAttrFlags(flags uint32) FileAttrFlags { } } +func (r *Request) SetAttributes(flags FileAttrFlags, attrs FileStat) error { + r.Method = "Setstat" + + buf := []byte{} + + r.Flags = 0 + + if flags.Size { + r.Flags |= sshFileXferAttrSize + buf = marshalUint64(buf, attrs.Size) + } + if flags.UidGid { + r.Flags |= sshFileXferAttrUIDGID + buf = marshalUint32(buf, attrs.UID) + buf = marshalUint32(buf, attrs.GID) + } + if flags.Permissions { + r.Flags |= sshFileXferAttrPermissions + buf = marshalUint32(buf, attrs.Mode) + } + + if flags.Acmodtime { + r.Flags |= sshFileXferAttrACmodTime + buf = marshalUint32(buf, attrs.Atime) + buf = marshalUint32(buf, attrs.Mtime) + } + + r.Attrs = buf + return nil +} + // AttrFlags returns a FileAttrFlags boolean struct based on the // bitmap/uint32 file attribute flags from the SFTP packaet. func (r *Request) AttrFlags() FileAttrFlags { From 688c05566aa2c21f9c2125aaebd869449553c720 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:08:27 -0700 Subject: [PATCH 06/30] Rip out SetAttributes. This is in favor of an alternate approach, we can always pull it back out of git history if we want to. --- request-attrs.go | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/request-attrs.go b/request-attrs.go index 15c82559..b5c95b4a 100644 --- a/request-attrs.go +++ b/request-attrs.go @@ -44,37 +44,6 @@ func newFileAttrFlags(flags uint32) FileAttrFlags { } } -func (r *Request) SetAttributes(flags FileAttrFlags, attrs FileStat) error { - r.Method = "Setstat" - - buf := []byte{} - - r.Flags = 0 - - if flags.Size { - r.Flags |= sshFileXferAttrSize - buf = marshalUint64(buf, attrs.Size) - } - if flags.UidGid { - r.Flags |= sshFileXferAttrUIDGID - buf = marshalUint32(buf, attrs.UID) - buf = marshalUint32(buf, attrs.GID) - } - if flags.Permissions { - r.Flags |= sshFileXferAttrPermissions - buf = marshalUint32(buf, attrs.Mode) - } - - if flags.Acmodtime { - r.Flags |= sshFileXferAttrACmodTime - buf = marshalUint32(buf, attrs.Atime) - buf = marshalUint32(buf, attrs.Mtime) - } - - r.Attrs = buf - return nil -} - // AttrFlags returns a FileAttrFlags boolean struct based on the // bitmap/uint32 file attribute flags from the SFTP packaet. func (r *Request) AttrFlags() FileAttrFlags { From a0a4a521e19eaecc324b1ab5f5c4ff506f4ac3bb Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:09:43 -0700 Subject: [PATCH 07/30] Get rid of FileStatSetter. It will be replaced with an alternate method. --- request-interfaces.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/request-interfaces.go b/request-interfaces.go index 63411f23..da56ed40 100644 --- a/request-interfaces.go +++ b/request-interfaces.go @@ -155,9 +155,3 @@ type ListerAt interface { type TransferError interface { TransferError(err error) } - -// FileStatSetter is an optional interface that a writerAt could implement in order to allow direct setting of stats. -// This is primarily used for the inmemhandler backend. -type FileStatSetter interface { - SetStat(r *Request, flags *FileAttrFlags, attrs *FileStat) error -} From 8ee6563336bb9eab4ae085670cf2f5ec15cc4309 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:11:25 -0700 Subject: [PATCH 08/30] Use errors.Is everywhere. This should cover every single case where we were using == or != on an err. There may be other cases to address, but this covers a big one. --- client.go | 12 ++++++------ client_integration_test.go | 6 +++--- examples/go-sftp-server/main.go | 3 ++- examples/request-server/main.go | 3 ++- packet.go | 2 +- request-example.go | 2 +- request-server.go | 2 +- request-server_test.go | 3 ++- request.go | 10 +++++----- server.go | 4 ++-- 10 files changed, 25 insertions(+), 22 deletions(-) diff --git a/client.go b/client.go index 0df125e1..e10518b2 100644 --- a/client.go +++ b/client.go @@ -275,7 +275,7 @@ func (c *Client) nextID() uint32 { func (c *Client) recvVersion() error { typ, data, err := c.recvPacket(0) if err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { return fmt.Errorf("server unexpectedly closed connection: %w", io.ErrUnexpectedEOF) } @@ -368,7 +368,7 @@ func (c *Client) ReadDir(p string) ([]os.FileInfo, error) { return nil, unimplementedPacketErr(typ) } } - if err == io.EOF { + if errors.Is(err, io.EOF) { err = nil } return attrs, err @@ -1238,7 +1238,7 @@ func (f *File) writeToSequential(w io.Writer) (written int64, err error) { } if err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { return written, nil // return nil explicitly. } @@ -1435,7 +1435,7 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) { } if packet.err != nil { - if packet.err == io.EOF { + if errors.Is(packet.err, io.EOF) { return written, nil } @@ -1726,7 +1726,7 @@ func (f *File) ReadFromWithConcurrency(r io.Reader, concurrency int) (read int64 } if err != nil { - if err != io.EOF { + if !errors.Is(err, io.EOF) { errCh <- rwErr{off, err} } return @@ -1878,7 +1878,7 @@ func (f *File) ReadFrom(r io.Reader) (int64, error) { } if err != nil { - if err == io.EOF { + if errors.Is(err, io.EOF) { return read, nil // return nil explicitly. } diff --git a/client_integration_test.go b/client_integration_test.go index 35ccbea5..8c34a2ff 100644 --- a/client_integration_test.go +++ b/client_integration_test.go @@ -1240,7 +1240,7 @@ func TestClientReadSimple(t *testing.T) { defer f2.Close() stuff := make([]byte, 32) n, err := f2.Read(stuff) - if err != nil && err != io.EOF { + if err != nil && !errors.Is(err, io.EOF) { t.Fatalf("err: %v", err) } if n != 5 { @@ -2152,7 +2152,7 @@ func TestMatch(t *testing.T) { pattern := tt.pattern s := tt.s ok, err := Match(pattern, s) - if ok != tt.match || err != tt.err { + if ok != tt.match || !errors.Is(err, tt.err) { t.Errorf("Match(%#q, %#q) = %v, %q want %v, %q", pattern, s, ok, errp(err), tt.match, errp(tt.err)) } } @@ -2411,7 +2411,7 @@ func benchmarkRead(b *testing.B, bufsize int, delay time.Duration) { for offset < size { n, err := io.ReadFull(f2, buf) offset += n - if err == io.ErrUnexpectedEOF && offset != size { + if errors.Is(err, io.ErrUnexpectedEOF) && offset != size { b.Fatalf("read too few bytes! want: %d, got: %d", size, n) } diff --git a/examples/go-sftp-server/main.go b/examples/go-sftp-server/main.go index e6a737a3..24feb356 100644 --- a/examples/go-sftp-server/main.go +++ b/examples/go-sftp-server/main.go @@ -4,6 +4,7 @@ package main import ( + "errors" "flag" "fmt" "io" @@ -136,7 +137,7 @@ func main() { if err != nil { log.Fatal(err) } - if err := server.Serve(); err == io.EOF { + if err := server.Serve(); errors.Is(err, io.EOF) { server.Close() log.Print("sftp client exited session.") } else if err != nil { diff --git a/examples/request-server/main.go b/examples/request-server/main.go index 9f0db4d0..f71fca65 100644 --- a/examples/request-server/main.go +++ b/examples/request-server/main.go @@ -4,6 +4,7 @@ package main import ( + "errors" "flag" "fmt" "io" @@ -120,7 +121,7 @@ func main() { root := sftp.InMemHandler() server := sftp.NewRequestServer(channel, root) - if err := server.Serve(); err == io.EOF { + if err := server.Serve(); errors.Is(err, io.EOF) { server.Close() log.Print("sftp client exited session.") } else if err != nil { diff --git a/packet.go b/packet.go index 1232ff1e..ecea33e2 100644 --- a/packet.go +++ b/packet.go @@ -292,7 +292,7 @@ func recvPacket(r io.Reader, alloc *allocator, orderID uint32) (uint8, []byte, e if _, err := io.ReadFull(r, b[:length]); err != nil { // ReadFull only returns EOF if it has read no bytes. // In this case, that means a partial packet, and thus unexpected. - if err == io.EOF { + if errors.Is(err, io.EOF) { err = io.ErrUnexpectedEOF } debug("recv packet %d bytes: err %v", length, err) diff --git a/request-example.go b/request-example.go index 924de0c9..d90948f8 100644 --- a/request-example.go +++ b/request-example.go @@ -86,7 +86,7 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { pflags := newFileOpenFlags(flags) file, err := fs.fetch(pathname) - if err == os.ErrNotExist { + if errors.Is(err, os.ErrNotExist) { if !pflags.Creat { return nil, os.ErrNotExist } diff --git a/request-server.go b/request-server.go index 7a99db64..8fbd5952 100644 --- a/request-server.go +++ b/request-server.go @@ -189,7 +189,7 @@ func (rs *RequestServer) Serve() error { // make sure all open requests are properly closed // (eg. possible on dropped connections, client crashes, etc.) for handle, req := range rs.openRequests { - if err == io.EOF { + if errors.Is(err, io.EOF) { err = io.ErrUnexpectedEOF } req.transferError(err) diff --git a/request-server_test.go b/request-server_test.go index 6825bf43..6430be62 100644 --- a/request-server_test.go +++ b/request-server_test.go @@ -2,6 +2,7 @@ package sftp import ( "context" + "errors" "fmt" "io" "io/ioutil" @@ -240,7 +241,7 @@ func TestRequestJustRead(t *testing.T) { defer rf.Close() contents := make([]byte, 5) n, err := rf.Read(contents) - if err != nil && err != io.EOF { + if err != nil && !errors.Is(err, io.EOF) { t.Fatalf("err: %v", err) } assert.Equal(t, 5, n) diff --git a/request.go b/request.go index 57d788df..5510b46a 100644 --- a/request.go +++ b/request.go @@ -385,7 +385,7 @@ func fileget(h FileReader, r *Request, pkt requestPacket, alloc *allocator, orde n, err := rd.ReadAt(data, offset) // only return EOF error if no data left to read - if err != nil && (err != io.EOF || n == 0) { + if err != nil && (!errors.Is(err, io.EOF) || n == 0) { return statusFromError(pkt.id(), err) } @@ -422,7 +422,7 @@ func fileputget(h FileWriter, r *Request, pkt requestPacket, alloc *allocator, o n, err := rw.ReadAt(data, offset) // only return EOF error if no data left to read - if err != nil && (err != io.EOF || n == 0) { + if err != nil && (!errors.Is(err, io.EOF) || n == 0) { return statusFromError(pkt.id(), err) } @@ -507,7 +507,7 @@ func filelist(h FileLister, r *Request, pkt requestPacket) responsePacket { switch r.Method { case "List": - if err != nil && (err != io.EOF || n == 0) { + if err != nil && (!errors.Is(err, io.EOF) || n == 0) { return statusFromError(pkt.id(), err) } @@ -560,7 +560,7 @@ func filestat(h FileLister, r *Request, pkt requestPacket) responsePacket { switch r.Method { case "Stat", "Lstat": - if err != nil && err != io.EOF { + if err != nil && !errors.Is(err, io.EOF) { return statusFromError(pkt.id(), err) } if n == 0 { @@ -576,7 +576,7 @@ func filestat(h FileLister, r *Request, pkt requestPacket) responsePacket { info: finfo[0], } case "Readlink": - if err != nil && err != io.EOF { + if err != nil && !errors.Is(err, io.EOF) { return statusFromError(pkt.id(), err) } if n == 0 { diff --git a/server.go b/server.go index 2e419f59..b742738a 100644 --- a/server.go +++ b/server.go @@ -289,7 +289,7 @@ func handlePacket(s *Server, p orderedRequest) error { err = nil data := p.getDataSlice(s.pktMgr.alloc, orderID) n, _err := f.ReadAt(data, int64(p.Offset)) - if _err != nil && (_err != io.EOF || n == 0) { + if _err != nil && (!errors.Is(_err, io.EOF) || n == 0) { err = _err } rpkt = &sshFxpDataPacket{ @@ -354,7 +354,7 @@ func (svr *Server) Serve() error { pktType, pktBytes, err = svr.serverConn.recvPacket(svr.pktMgr.getNextOrderID()) if err != nil { // Check whether the connection terminated cleanly in-between packets. - if err == io.EOF { + if errors.Is(err, io.EOF) { err = nil } // we don't care about releasing allocated pages here, the server will quit and the allocator freed From f2157404864a4bae9e8a13a58d2bf05c0a88b3f3 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:23:07 -0700 Subject: [PATCH 09/30] Merge SetStat back into Filecmd. With us no longer offering an interface with SetStat, there is no reason for this to be it's own function anymore. This also cleaned up how we handle truncation errors. --- request-example.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/request-example.go b/request-example.go index d90948f8..382ef3f8 100644 --- a/request-example.go +++ b/request-example.go @@ -141,25 +141,6 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { return file, nil } -func (fs *root) SetStat(r *Request, flags *FileAttrFlags, attrs *FileStat) error { - file, err := fs.openfile(r.Filepath, sshFxfWrite) - if err != nil { - return err - } - - if flags.Permissions { - file.mode = attrs.Mode - } - // We only have mtime, not atime. - if flags.Acmodtime { - file.modtime = time.Unix(int64(attrs.Mtime), 0) - } - if flags.Size { - return file.Truncate(int64(attrs.Size)) - } - return nil -} - func (fs *root) Filecmd(r *Request) error { if fs.mockErr != nil { return fs.mockErr @@ -172,7 +153,26 @@ func (fs *root) Filecmd(r *Request) error { switch r.Method { case "Setstat": flags := r.AttrFlags() - return fs.SetStat(r, &flags, r.Attributes()) + attrs := r.Attributes() + file, err := fs.openfile(r.Filepath, sshFxfWrite) + if err != nil { + return err + } + + if flags.Size { + if err = file.Truncate(int64(attrs.Size)); err != nil { + return err + } + } + if flags.Permissions { + const mask = uint32(os.ModePerm | s_ISUID | s_ISGID | s_ISVTX) + file.mode = (file.mode &^ mask) | (attrs.Mode & mask) + } + // We only have mtime, not atime. + if flags.Acmodtime { + file.modtime = time.Unix(int64(attrs.Mtime), 0) + } + return nil case "Rename": // SFTP-v2: "It is an error if there already exists a file with the name specified by newpath." From d41c7d48fe1254d5dea884365444a16ee42bdb1c Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:24:39 -0700 Subject: [PATCH 10/30] Export ToFileMode and FromFileMode. For the sake of sanity, we really should be offering _some_ way for users of memfile to get the file mode right. These two are what we use internally, so they make perfect sense to expose. --- attrs.go | 4 ++-- client.go | 2 +- ls_formatting.go | 2 +- stat_posix.go | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/attrs.go b/attrs.go index 758cd4ff..97c42a9c 100644 --- a/attrs.go +++ b/attrs.go @@ -32,7 +32,7 @@ func (fi *fileInfo) Name() string { return fi.name } func (fi *fileInfo) Size() int64 { return int64(fi.stat.Size) } // Mode returns file mode bits. -func (fi *fileInfo) Mode() os.FileMode { return toFileMode(fi.stat.Mode) } +func (fi *fileInfo) Mode() os.FileMode { return ToFileMode(fi.stat.Mode) } // ModTime returns the last modification time of the file. func (fi *fileInfo) ModTime() time.Time { return time.Unix(int64(fi.stat.Mtime), 0) } @@ -92,7 +92,7 @@ func fileStatFromInfo(fi os.FileInfo) (uint32, *FileStat) { fileStat := &FileStat{ Size: uint64(fi.Size()), - Mode: fromFileMode(fi.Mode()), + Mode: FromFileMode(fi.Mode()), Mtime: uint32(mtime), Atime: uint32(atime), } diff --git a/client.go b/client.go index e10518b2..af4aa6b0 100644 --- a/client.go +++ b/client.go @@ -2009,7 +2009,7 @@ func flags(f int) uint32 { // toChmodPerm converts Go permission bits to POSIX permission bits. // -// This differs from fromFileMode in that we preserve the POSIX versions of +// This differs from FromFileMode in that we preserve the POSIX versions of // setuid, setgid and sticky in m, because we've historically supported those // bits, and we mask off any non-permission bits. func toChmodPerm(m os.FileMode) (perm uint32) { diff --git a/ls_formatting.go b/ls_formatting.go index 19271ad7..e0c069fa 100644 --- a/ls_formatting.go +++ b/ls_formatting.go @@ -47,7 +47,7 @@ func runLs(idLookup NameLookupFileLister, dirent os.FileInfo) string { // format: // {directory / char device / etc}{rwxrwxrwx} {number of links} owner group size month day [time (this year) | year (otherwise)] name - symPerms := sshfx.FileMode(fromFileMode(dirent.Mode())).String() + symPerms := sshfx.FileMode(FromFileMode(dirent.Mode())).String() var numLinks uint64 = 1 uid, gid := "0", "0" diff --git a/stat_posix.go b/stat_posix.go index 5b870e23..bc5160c3 100644 --- a/stat_posix.go +++ b/stat_posix.go @@ -49,8 +49,8 @@ func isRegular(mode uint32) bool { return mode&S_IFMT == syscall.S_IFREG } -// toFileMode converts sftp filemode bits to the os.FileMode specification -func toFileMode(mode uint32) os.FileMode { +// ToFileMode converts sftp filemode bits to the os.FileMode specification +func ToFileMode(mode uint32) os.FileMode { var fm = os.FileMode(mode & 0777) switch mode & S_IFMT { @@ -83,8 +83,8 @@ func toFileMode(mode uint32) os.FileMode { return fm } -// fromFileMode converts from the os.FileMode specification to sftp filemode bits -func fromFileMode(mode os.FileMode) uint32 { +// FromFileMode converts from the os.FileMode specification to sftp filemode bits +func FromFileMode(mode os.FileMode) uint32 { ret := uint32(mode & os.ModePerm) switch mode & os.ModeType { From 87fa2180bf0a2e0113298fc7822792da145be48b Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 20:30:30 -0700 Subject: [PATCH 11/30] Silence a linter warning. --- request.go | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/request.go b/request.go index 5510b46a..9f2667da 100644 --- a/request.go +++ b/request.go @@ -140,6 +140,91 @@ type Request struct { cancelCtx context.CancelFunc } +// This is essentially a more unmarshaled Request. +// I say more unmarshaled, because StatAttrs is a FileStat, which while better +// than a binary array, is still supposed to be sftp protocol values instead of +// something like os or fs package types and values. +type ParsedRequest struct { + Method string + Filepath string + OpenFlags FileOpenFlags + StatFlags FileAttrFlags + StatAttrs FileStat + Target string // for renames and sym-links +} + +// Marshal is used to turn a ParsedRequest into a Request. +func (pr *ParsedRequest) Marshal() (r *Request) { + r = NewRequest(pr.Method, pr.Filepath) + + switch pr.Method { + case "Get", "Put": + r.Flags = 0 + if pr.OpenFlags.Read { + r.Flags |= sshFxfRead + } + if pr.OpenFlags.Write { + r.Flags |= sshFxfWrite + } + if pr.OpenFlags.Append { + r.Flags |= sshFxfAppend + } + if pr.OpenFlags.Creat { + r.Flags |= sshFxfCreat + } + if pr.OpenFlags.Trunc { + r.Flags |= sshFxfTrunc + } + if pr.OpenFlags.Excl { + r.Flags |= sshFxfExcl + } + + case "Stat", "Setstat": + var buf []byte + r.Flags = 0 + + if pr.StatFlags.Size { + r.Flags |= sshFileXferAttrSize + buf = marshalUint64(buf, pr.StatAttrs.Size) + } + if pr.StatFlags.UidGid { + r.Flags |= sshFileXferAttrUIDGID + buf = marshalUint32(buf, pr.StatAttrs.UID) + buf = marshalUint32(buf, pr.StatAttrs.GID) + } + if pr.StatFlags.Permissions { + r.Flags |= sshFileXferAttrPermissions + buf = marshalUint32(buf, FromFileMode(os.FileMode(pr.StatAttrs.Mode))) + } + + if pr.StatFlags.Acmodtime { + r.Flags |= sshFileXferAttrACmodTime + buf = marshalUint32(buf, pr.StatAttrs.Atime) + buf = marshalUint32(buf, pr.StatAttrs.Mtime) + } + + r.Attrs = buf + } + + return r +} + +// Unmarshal is used to turn a Request into a ParsedRequest. +func (r *Request) Unmarshal() (pr *ParsedRequest) { + pr = &ParsedRequest{} + + pr.Method = r.Method + switch pr.Method { + case "Get", "Put": + pr.OpenFlags = r.Pflags() + case "Stat", "Setstat": + pr.StatFlags = r.AttrFlags() + pr.StatAttrs = *r.Attributes() + } + + return pr +} + // NewRequest creates a new Request object. func NewRequest(method, path string) *Request { return &Request{ @@ -456,7 +541,7 @@ func packetData(p requestPacket, alloc *allocator, orderID uint32) (data []byte, // wrap FileCmder handler func filecmd(h FileCmder, r *Request, pkt requestPacket) responsePacket { - switch p := pkt.(type) { + switch p := pkt.(type) { // nolint: gocritic case *sshFxpFsetstatPacket: r.Flags = p.Flags r.Attrs = p.Attrs.([]byte) From a5d43165b277548b97b03992cc45b7af7fe827ed Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 21:02:43 -0700 Subject: [PATCH 12/30] memfile: Check file permissions. We now check to ensure that the permissions on a given file allow the user to perform the requested operation. For Filereader, the file must be readable. For Filewriter, the file must be writable. For OpenFile, the file must be readable and writable. --- request-example.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/request-example.go b/request-example.go index 382ef3f8..7a56a08e 100644 --- a/request-example.go +++ b/request-example.go @@ -37,7 +37,8 @@ func (fs *root) Fileread(r *Request) (io.ReaderAt, error) { return nil, os.ErrInvalid } - return fs.OpenFile(r) + // Needs to be readable by the user. + return fs.openFileModeCheck(r, 0o0400) } func (fs *root) Filewrite(r *Request) (io.WriterAt, error) { @@ -47,10 +48,16 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) { return nil, os.ErrInvalid } - return fs.OpenFile(r) + // Needs to be writable by the user. + return fs.openFileModeCheck(r, 0o0200) } func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) { + // Needs to be readable and writable by the user. + return fs.openFileModeCheck(r, 0o0200|0o0400) +} + +func (fs *root) openFileModeCheck(r *Request, mode uint32) (WriterAtReaderAt, error) { if fs.mockErr != nil { return nil, fs.mockErr } @@ -59,7 +66,16 @@ func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) { fs.mu.Lock() defer fs.mu.Unlock() - return fs.openfile(r.Filepath, r.Flags) + f, err := fs.openfile(r.Filepath, r.Flags) + if err != nil { + return nil, err + } + + if f.mode&mode != mode { + return nil, os.ErrPermission + } + + return f, nil } func (fs *root) putfile(pathname string, file *memFile) error { From 4404cc8feb55c22690a137a97e5dfa617b111c80 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 21:06:14 -0700 Subject: [PATCH 13/30] Add some comments. --- request-example.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/request-example.go b/request-example.go index 7a56a08e..8dbdd7c3 100644 --- a/request-example.go +++ b/request-example.go @@ -124,6 +124,8 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { link, err = fs.lfetch(pathname) } + // The mode is hard coded because the sftp protocol does not specify a + // mode at file open time. file := &memFile{ modtime: time.Now(), mode: 0644, @@ -168,6 +170,10 @@ func (fs *root) Filecmd(r *Request) error { switch r.Method { case "Setstat": + // Note: fs.openfile does not support opening a directory. + // So there is currently no way to set the mode of a directory. + // That's a good thing, because it means that we don't have to do + // permissions checks on parent directories. flags := r.AttrFlags() attrs := r.Attributes() file, err := fs.openfile(r.Filepath, sshFxfWrite) @@ -592,6 +598,8 @@ func (f *memFile) Size() int64 { return f.size() } func (f *memFile) Mode() os.FileMode { + // Hardcoded values, because we do not even try to support changing the + // mode of directories or symlinks. if f.isdir { return os.FileMode(0755) | os.ModeDir } From 656a9db63ace53e4d5b4321098bec779dd36169f Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 21:06:44 -0700 Subject: [PATCH 14/30] Explictly forbid changing the mode of a symlink. --- request-example.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/request-example.go b/request-example.go index 8dbdd7c3..41c6419f 100644 --- a/request-example.go +++ b/request-example.go @@ -170,6 +170,14 @@ func (fs *root) Filecmd(r *Request) error { switch r.Method { case "Setstat": + // We explicitly do not support changing the mode of symlinks. + // Nor do we want to change the mode of whatever the symlink is pointed + // at. + link, err := fs.Readlink(r.Filepath) + if link != "" || err == nil || !errors.Is(err, os.ErrInvalid) { + return err + } + // Note: fs.openfile does not support opening a directory. // So there is currently no way to set the mode of a directory. // That's a good thing, because it means that we don't have to do From b1d53f7ed74c3fa0076b7c1e3b5161369f431e0b Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Tue, 31 Oct 2023 21:17:28 -0700 Subject: [PATCH 15/30] Expose To/FromFileMode for plan9. Missed this in the first pass. --- stat_plan9.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stat_plan9.go b/stat_plan9.go index 761abdf5..dcb26ea7 100644 --- a/stat_plan9.go +++ b/stat_plan9.go @@ -46,8 +46,8 @@ func isRegular(mode uint32) bool { return mode&S_IFMT == syscall.S_IFREG } -// toFileMode converts sftp filemode bits to the os.FileMode specification -func toFileMode(mode uint32) os.FileMode { +// ToFileMode converts sftp filemode bits to the os.FileMode specification +func ToFileMode(mode uint32) os.FileMode { var fm = os.FileMode(mode & 0777) switch mode & S_IFMT { @@ -70,8 +70,8 @@ func toFileMode(mode uint32) os.FileMode { return fm } -// fromFileMode converts from the os.FileMode specification to sftp filemode bits -func fromFileMode(mode os.FileMode) uint32 { +// FromFileMode converts from the os.FileMode specification to sftp filemode bits +func FromFileMode(mode os.FileMode) uint32 { ret := uint32(mode & os.ModePerm) switch mode & os.ModeType { From cbf7fcdbe1ce569ae8b9d4fa9f4d3b27eb148912 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:21:58 -0700 Subject: [PATCH 16/30] Add FileStat.MarshalTo and FAF.ForRequest. FileStat.MarshalTo takes in flags, and generates the []byte that goes into Request.Attr FileAttrFlags.ForRequest() generates the uint32 bitmap that goes into Request.Flags --- attrs.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ request-attrs.go | 17 +++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/attrs.go b/attrs.go index 97c42a9c..29f4164c 100644 --- a/attrs.go +++ b/attrs.go @@ -56,6 +56,53 @@ type FileStat struct { Extended []StatExtended } +func (fs FileStat) MarshalTo(flags FileAttrFlags) []byte { + // attributes variable struct, and also variable per protocol version + // spec version 3 attributes: + // uint32 flags + // uint64 size present only if flag SSH_FILEXFER_ATTR_SIZE + // uint32 uid present only if flag SSH_FILEXFER_ATTR_UIDGID + // uint32 gid present only if flag SSH_FILEXFER_ATTR_UIDGID + // uint32 permissions present only if flag SSH_FILEXFER_ATTR_PERMISSIONS + // uint32 atime present only if flag SSH_FILEXFER_ACMODTIME + // uint32 mtime present only if flag SSH_FILEXFER_ACMODTIME + // uint32 extended_count present only if flag SSH_FILEXFER_ATTR_EXTENDED + // string extended_type + // string extended_data + // ... more extended data (extended_type - extended_data pairs), + // so that number of pairs equals extended_count + var b []byte + + f := flags.ForRequest() + b = marshalUint32(b, f) + + if flags.Size { + b = marshalUint64(b, fs.Size) + } + if flags.UidGid { + b = marshalUint32(b, fs.UID) + b = marshalUint32(b, fs.GID) + } + if flags.Permissions { + b = marshalUint32(b, fs.Mode) + } + if flags.Acmodtime { + b = marshalUint32(b, fs.Atime) + b = marshalUint32(b, fs.Mtime) + } + + if len(fs.Extended) > 0 { + b = marshalUint32(b, uint32(len(fs.Extended))) + + for _, attr := range fs.Extended { + b = marshalString(b, attr.ExtType) + b = marshalString(b, attr.ExtData) + } + } + + return b +} + // StatExtended contains additional, extended information for a FileStat. type StatExtended struct { ExtType string diff --git a/request-attrs.go b/request-attrs.go index b5c95b4a..4d70df5c 100644 --- a/request-attrs.go +++ b/request-attrs.go @@ -35,6 +35,23 @@ type FileAttrFlags struct { Size, UidGid, Permissions, Acmodtime bool } +func (faf FileAttrFlags) ForRequest() (flags uint32) { + if faf.Size { + flags |= sshFileXferAttrSize + } + if faf.UidGid { + flags |= sshFileXferAttrUIDGID + } + if faf.Permissions { + flags |= sshFileXferAttrPermissions + } + if faf.Acmodtime { + flags |= sshFileXferAttrACmodTime + } + + return flags +} + func newFileAttrFlags(flags uint32) FileAttrFlags { return FileAttrFlags{ Size: (flags & sshFileXferAttrSize) != 0, From 4b73c93ecb737bb2c4a50d4fc158446f9aa5a8db Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:47:35 -0700 Subject: [PATCH 17/30] Don't put the flags in the attrs. --- attrs.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/attrs.go b/attrs.go index 29f4164c..153d2d16 100644 --- a/attrs.go +++ b/attrs.go @@ -73,9 +73,6 @@ func (fs FileStat) MarshalTo(flags FileAttrFlags) []byte { // so that number of pairs equals extended_count var b []byte - f := flags.ForRequest() - b = marshalUint32(b, f) - if flags.Size { b = marshalUint64(b, fs.Size) } From 82cd5ca4be5e614b41913147660a3b95f295126d Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:47:50 -0700 Subject: [PATCH 18/30] FileOpenFlags.ForRequest --- request-attrs.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/request-attrs.go b/request-attrs.go index 4d70df5c..4d4fd63b 100644 --- a/request-attrs.go +++ b/request-attrs.go @@ -22,6 +22,29 @@ func newFileOpenFlags(flags uint32) FileOpenFlags { } } +func (fof FileOpenFlags) ForRequest() (flags uint32) { + if fof.Read { + flags |= sshFxfRead + } + if fof.Write { + flags |= sshFxfWrite + } + if fof.Append { + flags |= sshFxfAppend + } + if fof.Creat { + flags |= sshFxfCreat + } + if fof.Trunc { + flags |= sshFxfTrunc + } + if fof.Excl { + flags |= sshFxfExcl + } + + return flags +} + // Pflags converts the bitmap/uint32 from SFTP Open packet pflag values, // into a FileOpenFlags struct with booleans set for flags set in bitmap. func (r *Request) Pflags() FileOpenFlags { From 5fd4ff4210cfda317d9945f4febe6e54f1a2982a Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:49:37 -0700 Subject: [PATCH 19/30] Revert "Expose To/FromFileMode for plan9." This reverts commit b1d53f7ed74c3fa0076b7c1e3b5161369f431e0b. --- stat_plan9.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stat_plan9.go b/stat_plan9.go index dcb26ea7..761abdf5 100644 --- a/stat_plan9.go +++ b/stat_plan9.go @@ -46,8 +46,8 @@ func isRegular(mode uint32) bool { return mode&S_IFMT == syscall.S_IFREG } -// ToFileMode converts sftp filemode bits to the os.FileMode specification -func ToFileMode(mode uint32) os.FileMode { +// toFileMode converts sftp filemode bits to the os.FileMode specification +func toFileMode(mode uint32) os.FileMode { var fm = os.FileMode(mode & 0777) switch mode & S_IFMT { @@ -70,8 +70,8 @@ func ToFileMode(mode uint32) os.FileMode { return fm } -// FromFileMode converts from the os.FileMode specification to sftp filemode bits -func FromFileMode(mode os.FileMode) uint32 { +// fromFileMode converts from the os.FileMode specification to sftp filemode bits +func fromFileMode(mode os.FileMode) uint32 { ret := uint32(mode & os.ModePerm) switch mode & os.ModeType { From 660c973ec0ae916c32145c0d003b54828a391afb Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:49:39 -0700 Subject: [PATCH 20/30] Revert "Export ToFileMode and FromFileMode." This reverts commit d41c7d48fe1254d5dea884365444a16ee42bdb1c. --- attrs.go | 4 ++-- client.go | 2 +- ls_formatting.go | 2 +- stat_posix.go | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/attrs.go b/attrs.go index 153d2d16..2d623d8d 100644 --- a/attrs.go +++ b/attrs.go @@ -32,7 +32,7 @@ func (fi *fileInfo) Name() string { return fi.name } func (fi *fileInfo) Size() int64 { return int64(fi.stat.Size) } // Mode returns file mode bits. -func (fi *fileInfo) Mode() os.FileMode { return ToFileMode(fi.stat.Mode) } +func (fi *fileInfo) Mode() os.FileMode { return toFileMode(fi.stat.Mode) } // ModTime returns the last modification time of the file. func (fi *fileInfo) ModTime() time.Time { return time.Unix(int64(fi.stat.Mtime), 0) } @@ -136,7 +136,7 @@ func fileStatFromInfo(fi os.FileInfo) (uint32, *FileStat) { fileStat := &FileStat{ Size: uint64(fi.Size()), - Mode: FromFileMode(fi.Mode()), + Mode: fromFileMode(fi.Mode()), Mtime: uint32(mtime), Atime: uint32(atime), } diff --git a/client.go b/client.go index af4aa6b0..e10518b2 100644 --- a/client.go +++ b/client.go @@ -2009,7 +2009,7 @@ func flags(f int) uint32 { // toChmodPerm converts Go permission bits to POSIX permission bits. // -// This differs from FromFileMode in that we preserve the POSIX versions of +// This differs from fromFileMode in that we preserve the POSIX versions of // setuid, setgid and sticky in m, because we've historically supported those // bits, and we mask off any non-permission bits. func toChmodPerm(m os.FileMode) (perm uint32) { diff --git a/ls_formatting.go b/ls_formatting.go index e0c069fa..19271ad7 100644 --- a/ls_formatting.go +++ b/ls_formatting.go @@ -47,7 +47,7 @@ func runLs(idLookup NameLookupFileLister, dirent os.FileInfo) string { // format: // {directory / char device / etc}{rwxrwxrwx} {number of links} owner group size month day [time (this year) | year (otherwise)] name - symPerms := sshfx.FileMode(FromFileMode(dirent.Mode())).String() + symPerms := sshfx.FileMode(fromFileMode(dirent.Mode())).String() var numLinks uint64 = 1 uid, gid := "0", "0" diff --git a/stat_posix.go b/stat_posix.go index bc5160c3..5b870e23 100644 --- a/stat_posix.go +++ b/stat_posix.go @@ -49,8 +49,8 @@ func isRegular(mode uint32) bool { return mode&S_IFMT == syscall.S_IFREG } -// ToFileMode converts sftp filemode bits to the os.FileMode specification -func ToFileMode(mode uint32) os.FileMode { +// toFileMode converts sftp filemode bits to the os.FileMode specification +func toFileMode(mode uint32) os.FileMode { var fm = os.FileMode(mode & 0777) switch mode & S_IFMT { @@ -83,8 +83,8 @@ func ToFileMode(mode uint32) os.FileMode { return fm } -// FromFileMode converts from the os.FileMode specification to sftp filemode bits -func FromFileMode(mode os.FileMode) uint32 { +// fromFileMode converts from the os.FileMode specification to sftp filemode bits +func fromFileMode(mode os.FileMode) uint32 { ret := uint32(mode & os.ModePerm) switch mode & os.ModeType { From 6c452cc400e50585ea178f52641a12427d291917 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:50:08 -0700 Subject: [PATCH 21/30] Revert "Use errors.Is everywhere." This reverts commit 8ee6563336bb9eab4ae085670cf2f5ec15cc4309. --- client.go | 12 ++++++------ client_integration_test.go | 6 +++--- examples/go-sftp-server/main.go | 3 +-- examples/request-server/main.go | 3 +-- packet.go | 2 +- request-example.go | 2 +- request-server.go | 2 +- request-server_test.go | 3 +-- request.go | 10 +++++----- server.go | 4 ++-- 10 files changed, 22 insertions(+), 25 deletions(-) diff --git a/client.go b/client.go index e10518b2..0df125e1 100644 --- a/client.go +++ b/client.go @@ -275,7 +275,7 @@ func (c *Client) nextID() uint32 { func (c *Client) recvVersion() error { typ, data, err := c.recvPacket(0) if err != nil { - if errors.Is(err, io.EOF) { + if err == io.EOF { return fmt.Errorf("server unexpectedly closed connection: %w", io.ErrUnexpectedEOF) } @@ -368,7 +368,7 @@ func (c *Client) ReadDir(p string) ([]os.FileInfo, error) { return nil, unimplementedPacketErr(typ) } } - if errors.Is(err, io.EOF) { + if err == io.EOF { err = nil } return attrs, err @@ -1238,7 +1238,7 @@ func (f *File) writeToSequential(w io.Writer) (written int64, err error) { } if err != nil { - if errors.Is(err, io.EOF) { + if err == io.EOF { return written, nil // return nil explicitly. } @@ -1435,7 +1435,7 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) { } if packet.err != nil { - if errors.Is(packet.err, io.EOF) { + if packet.err == io.EOF { return written, nil } @@ -1726,7 +1726,7 @@ func (f *File) ReadFromWithConcurrency(r io.Reader, concurrency int) (read int64 } if err != nil { - if !errors.Is(err, io.EOF) { + if err != io.EOF { errCh <- rwErr{off, err} } return @@ -1878,7 +1878,7 @@ func (f *File) ReadFrom(r io.Reader) (int64, error) { } if err != nil { - if errors.Is(err, io.EOF) { + if err == io.EOF { return read, nil // return nil explicitly. } diff --git a/client_integration_test.go b/client_integration_test.go index 8c34a2ff..35ccbea5 100644 --- a/client_integration_test.go +++ b/client_integration_test.go @@ -1240,7 +1240,7 @@ func TestClientReadSimple(t *testing.T) { defer f2.Close() stuff := make([]byte, 32) n, err := f2.Read(stuff) - if err != nil && !errors.Is(err, io.EOF) { + if err != nil && err != io.EOF { t.Fatalf("err: %v", err) } if n != 5 { @@ -2152,7 +2152,7 @@ func TestMatch(t *testing.T) { pattern := tt.pattern s := tt.s ok, err := Match(pattern, s) - if ok != tt.match || !errors.Is(err, tt.err) { + if ok != tt.match || err != tt.err { t.Errorf("Match(%#q, %#q) = %v, %q want %v, %q", pattern, s, ok, errp(err), tt.match, errp(tt.err)) } } @@ -2411,7 +2411,7 @@ func benchmarkRead(b *testing.B, bufsize int, delay time.Duration) { for offset < size { n, err := io.ReadFull(f2, buf) offset += n - if errors.Is(err, io.ErrUnexpectedEOF) && offset != size { + if err == io.ErrUnexpectedEOF && offset != size { b.Fatalf("read too few bytes! want: %d, got: %d", size, n) } diff --git a/examples/go-sftp-server/main.go b/examples/go-sftp-server/main.go index 24feb356..e6a737a3 100644 --- a/examples/go-sftp-server/main.go +++ b/examples/go-sftp-server/main.go @@ -4,7 +4,6 @@ package main import ( - "errors" "flag" "fmt" "io" @@ -137,7 +136,7 @@ func main() { if err != nil { log.Fatal(err) } - if err := server.Serve(); errors.Is(err, io.EOF) { + if err := server.Serve(); err == io.EOF { server.Close() log.Print("sftp client exited session.") } else if err != nil { diff --git a/examples/request-server/main.go b/examples/request-server/main.go index f71fca65..9f0db4d0 100644 --- a/examples/request-server/main.go +++ b/examples/request-server/main.go @@ -4,7 +4,6 @@ package main import ( - "errors" "flag" "fmt" "io" @@ -121,7 +120,7 @@ func main() { root := sftp.InMemHandler() server := sftp.NewRequestServer(channel, root) - if err := server.Serve(); errors.Is(err, io.EOF) { + if err := server.Serve(); err == io.EOF { server.Close() log.Print("sftp client exited session.") } else if err != nil { diff --git a/packet.go b/packet.go index ecea33e2..1232ff1e 100644 --- a/packet.go +++ b/packet.go @@ -292,7 +292,7 @@ func recvPacket(r io.Reader, alloc *allocator, orderID uint32) (uint8, []byte, e if _, err := io.ReadFull(r, b[:length]); err != nil { // ReadFull only returns EOF if it has read no bytes. // In this case, that means a partial packet, and thus unexpected. - if errors.Is(err, io.EOF) { + if err == io.EOF { err = io.ErrUnexpectedEOF } debug("recv packet %d bytes: err %v", length, err) diff --git a/request-example.go b/request-example.go index 41c6419f..304dfa7e 100644 --- a/request-example.go +++ b/request-example.go @@ -102,7 +102,7 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { pflags := newFileOpenFlags(flags) file, err := fs.fetch(pathname) - if errors.Is(err, os.ErrNotExist) { + if err == os.ErrNotExist { if !pflags.Creat { return nil, os.ErrNotExist } diff --git a/request-server.go b/request-server.go index 8fbd5952..7a99db64 100644 --- a/request-server.go +++ b/request-server.go @@ -189,7 +189,7 @@ func (rs *RequestServer) Serve() error { // make sure all open requests are properly closed // (eg. possible on dropped connections, client crashes, etc.) for handle, req := range rs.openRequests { - if errors.Is(err, io.EOF) { + if err == io.EOF { err = io.ErrUnexpectedEOF } req.transferError(err) diff --git a/request-server_test.go b/request-server_test.go index 6430be62..6825bf43 100644 --- a/request-server_test.go +++ b/request-server_test.go @@ -2,7 +2,6 @@ package sftp import ( "context" - "errors" "fmt" "io" "io/ioutil" @@ -241,7 +240,7 @@ func TestRequestJustRead(t *testing.T) { defer rf.Close() contents := make([]byte, 5) n, err := rf.Read(contents) - if err != nil && !errors.Is(err, io.EOF) { + if err != nil && err != io.EOF { t.Fatalf("err: %v", err) } assert.Equal(t, 5, n) diff --git a/request.go b/request.go index 9f2667da..b7e34e4b 100644 --- a/request.go +++ b/request.go @@ -470,7 +470,7 @@ func fileget(h FileReader, r *Request, pkt requestPacket, alloc *allocator, orde n, err := rd.ReadAt(data, offset) // only return EOF error if no data left to read - if err != nil && (!errors.Is(err, io.EOF) || n == 0) { + if err != nil && (err != io.EOF || n == 0) { return statusFromError(pkt.id(), err) } @@ -507,7 +507,7 @@ func fileputget(h FileWriter, r *Request, pkt requestPacket, alloc *allocator, o n, err := rw.ReadAt(data, offset) // only return EOF error if no data left to read - if err != nil && (!errors.Is(err, io.EOF) || n == 0) { + if err != nil && (err != io.EOF || n == 0) { return statusFromError(pkt.id(), err) } @@ -592,7 +592,7 @@ func filelist(h FileLister, r *Request, pkt requestPacket) responsePacket { switch r.Method { case "List": - if err != nil && (!errors.Is(err, io.EOF) || n == 0) { + if err != nil && (err != io.EOF || n == 0) { return statusFromError(pkt.id(), err) } @@ -645,7 +645,7 @@ func filestat(h FileLister, r *Request, pkt requestPacket) responsePacket { switch r.Method { case "Stat", "Lstat": - if err != nil && !errors.Is(err, io.EOF) { + if err != nil && err != io.EOF { return statusFromError(pkt.id(), err) } if n == 0 { @@ -661,7 +661,7 @@ func filestat(h FileLister, r *Request, pkt requestPacket) responsePacket { info: finfo[0], } case "Readlink": - if err != nil && !errors.Is(err, io.EOF) { + if err != nil && err != io.EOF { return statusFromError(pkt.id(), err) } if n == 0 { diff --git a/server.go b/server.go index b742738a..2e419f59 100644 --- a/server.go +++ b/server.go @@ -289,7 +289,7 @@ func handlePacket(s *Server, p orderedRequest) error { err = nil data := p.getDataSlice(s.pktMgr.alloc, orderID) n, _err := f.ReadAt(data, int64(p.Offset)) - if _err != nil && (!errors.Is(_err, io.EOF) || n == 0) { + if _err != nil && (_err != io.EOF || n == 0) { err = _err } rpkt = &sshFxpDataPacket{ @@ -354,7 +354,7 @@ func (svr *Server) Serve() error { pktType, pktBytes, err = svr.serverConn.recvPacket(svr.pktMgr.getNextOrderID()) if err != nil { // Check whether the connection terminated cleanly in-between packets. - if errors.Is(err, io.EOF) { + if err == io.EOF { err = nil } // we don't care about releasing allocated pages here, the server will quit and the allocator freed From 65632495a593457bd29b35623713ff8c61722b44 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 18:52:15 -0700 Subject: [PATCH 22/30] Revert "Silence a linter warning." This reverts commit 87fa2180bf0a2e0113298fc7822792da145be48b. --- request.go | 87 +----------------------------------------------------- 1 file changed, 1 insertion(+), 86 deletions(-) diff --git a/request.go b/request.go index b7e34e4b..57d788df 100644 --- a/request.go +++ b/request.go @@ -140,91 +140,6 @@ type Request struct { cancelCtx context.CancelFunc } -// This is essentially a more unmarshaled Request. -// I say more unmarshaled, because StatAttrs is a FileStat, which while better -// than a binary array, is still supposed to be sftp protocol values instead of -// something like os or fs package types and values. -type ParsedRequest struct { - Method string - Filepath string - OpenFlags FileOpenFlags - StatFlags FileAttrFlags - StatAttrs FileStat - Target string // for renames and sym-links -} - -// Marshal is used to turn a ParsedRequest into a Request. -func (pr *ParsedRequest) Marshal() (r *Request) { - r = NewRequest(pr.Method, pr.Filepath) - - switch pr.Method { - case "Get", "Put": - r.Flags = 0 - if pr.OpenFlags.Read { - r.Flags |= sshFxfRead - } - if pr.OpenFlags.Write { - r.Flags |= sshFxfWrite - } - if pr.OpenFlags.Append { - r.Flags |= sshFxfAppend - } - if pr.OpenFlags.Creat { - r.Flags |= sshFxfCreat - } - if pr.OpenFlags.Trunc { - r.Flags |= sshFxfTrunc - } - if pr.OpenFlags.Excl { - r.Flags |= sshFxfExcl - } - - case "Stat", "Setstat": - var buf []byte - r.Flags = 0 - - if pr.StatFlags.Size { - r.Flags |= sshFileXferAttrSize - buf = marshalUint64(buf, pr.StatAttrs.Size) - } - if pr.StatFlags.UidGid { - r.Flags |= sshFileXferAttrUIDGID - buf = marshalUint32(buf, pr.StatAttrs.UID) - buf = marshalUint32(buf, pr.StatAttrs.GID) - } - if pr.StatFlags.Permissions { - r.Flags |= sshFileXferAttrPermissions - buf = marshalUint32(buf, FromFileMode(os.FileMode(pr.StatAttrs.Mode))) - } - - if pr.StatFlags.Acmodtime { - r.Flags |= sshFileXferAttrACmodTime - buf = marshalUint32(buf, pr.StatAttrs.Atime) - buf = marshalUint32(buf, pr.StatAttrs.Mtime) - } - - r.Attrs = buf - } - - return r -} - -// Unmarshal is used to turn a Request into a ParsedRequest. -func (r *Request) Unmarshal() (pr *ParsedRequest) { - pr = &ParsedRequest{} - - pr.Method = r.Method - switch pr.Method { - case "Get", "Put": - pr.OpenFlags = r.Pflags() - case "Stat", "Setstat": - pr.StatFlags = r.AttrFlags() - pr.StatAttrs = *r.Attributes() - } - - return pr -} - // NewRequest creates a new Request object. func NewRequest(method, path string) *Request { return &Request{ @@ -541,7 +456,7 @@ func packetData(p requestPacket, alloc *allocator, orderID uint32) (data []byte, // wrap FileCmder handler func filecmd(h FileCmder, r *Request, pkt requestPacket) responsePacket { - switch p := pkt.(type) { // nolint: gocritic + switch p := pkt.(type) { case *sshFxpFsetstatPacket: r.Flags = p.Flags r.Attrs = p.Attrs.([]byte) From ad0d7df0720301163e9ef20e82b5b6a8fd571409 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:12:11 -0700 Subject: [PATCH 23/30] Comment fixes. --- request-example.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/request-example.go b/request-example.go index 304dfa7e..5cca9445 100644 --- a/request-example.go +++ b/request-example.go @@ -37,7 +37,7 @@ func (fs *root) Fileread(r *Request) (io.ReaderAt, error) { return nil, os.ErrInvalid } - // Needs to be readable by the user. + // Needs to be readable by the owner. return fs.openFileModeCheck(r, 0o0400) } @@ -48,12 +48,12 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) { return nil, os.ErrInvalid } - // Needs to be writable by the user. + // Needs to be writable by the owner. return fs.openFileModeCheck(r, 0o0200) } func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) { - // Needs to be readable and writable by the user. + // Needs to be readable and writable by the owner. return fs.openFileModeCheck(r, 0o0200|0o0400) } @@ -124,8 +124,7 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) { link, err = fs.lfetch(pathname) } - // The mode is hard coded because the sftp protocol does not specify a - // mode at file open time. + // The mode is currently hard coded because this library doesn't parse out the mode at file open time. file := &memFile{ modtime: time.Now(), mode: 0644, @@ -606,11 +605,11 @@ func (f *memFile) Size() int64 { return f.size() } func (f *memFile) Mode() os.FileMode { - // Hardcoded values, because we do not even try to support changing the - // mode of directories or symlinks. + // At this time, we do not implement directory modes. if f.isdir { return os.FileMode(0755) | os.ModeDir } + // Under POSIX, symlinks have a fixed mode which can not be changed. if f.symlink != "" { return os.FileMode(0777) | os.ModeSymlink } From 7e1a518e99896435a85131478ac2b93e27c55cec Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:12:32 -0700 Subject: [PATCH 24/30] Let Setstat follow symlinks, and rework comments. As best as I can tell, under POSIX chmod will follow symlinks. However I am definitely not sure if that is the case for common sftp servers on POSIX filesystems, we should consider checking... Some day. --- request-example.go | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/request-example.go b/request-example.go index 5cca9445..f82720e7 100644 --- a/request-example.go +++ b/request-example.go @@ -169,18 +169,11 @@ func (fs *root) Filecmd(r *Request) error { switch r.Method { case "Setstat": - // We explicitly do not support changing the mode of symlinks. - // Nor do we want to change the mode of whatever the symlink is pointed - // at. - link, err := fs.Readlink(r.Filepath) - if link != "" || err == nil || !errors.Is(err, os.ErrInvalid) { - return err - } - - // Note: fs.openfile does not support opening a directory. - // So there is currently no way to set the mode of a directory. - // That's a good thing, because it means that we don't have to do - // permissions checks on parent directories. + // Some notes: + // + // openfile will follow symlinks, however as best as I can tell this is the correct POSIX behavior for chmod. + // + // openfile does not currently support opening a directory, and at this time we do not implement directory permissions. flags := r.AttrFlags() attrs := r.Attributes() file, err := fs.openfile(r.Filepath, sshFxfWrite) From d3a55c7ff9812ca288f3f62d223ed14a3ff8c6fb Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:22:04 -0700 Subject: [PATCH 25/30] Shadow err to avoid the value leaking. --- request-example.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/request-example.go b/request-example.go index f82720e7..43b7ffaf 100644 --- a/request-example.go +++ b/request-example.go @@ -182,7 +182,7 @@ func (fs *root) Filecmd(r *Request) error { } if flags.Size { - if err = file.Truncate(int64(attrs.Size)); err != nil { + if err := file.Truncate(int64(attrs.Size)); err != nil { return err } } From bff300bd4742c633a2b8d3c6cb1e6d2c35d88f08 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:22:19 -0700 Subject: [PATCH 26/30] Move the chmod logic to it's own method. --- request-example.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/request-example.go b/request-example.go index 43b7ffaf..77735d62 100644 --- a/request-example.go +++ b/request-example.go @@ -187,8 +187,7 @@ func (fs *root) Filecmd(r *Request) error { } } if flags.Permissions { - const mask = uint32(os.ModePerm | s_ISUID | s_ISGID | s_ISVTX) - file.mode = (file.mode &^ mask) | (attrs.Mode & mask) + file.chmod(attrs.Mode) } // We only have mtime, not atime. if flags.Acmodtime { @@ -682,3 +681,8 @@ func (f *memFile) TransferError(err error) { f.err = err } + +func (f *memFile) chmod(mode uint32) { + const mask = uint32(os.ModePerm | s_ISUID | s_ISGID | s_ISVTX) + f.mode = (f.mode &^ mask) | (mode & mask) +} From 1d040d69f98750a029ec501da6b5cbf81e8850f4 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:27:00 -0700 Subject: [PATCH 27/30] Make marshalFileInfo use fileStat.MarshalTo This avoids logic duplication. --- packet.go | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/packet.go b/packet.go index 1232ff1e..f785a0cd 100644 --- a/packet.go +++ b/packet.go @@ -54,31 +54,10 @@ func marshalFileInfo(b []byte, fi os.FileInfo) []byte { // so that number of pairs equals extended_count flags, fileStat := fileStatFromInfo(fi) + f := newFileAttrFlags(flags) b = marshalUint32(b, flags) - if flags&sshFileXferAttrSize != 0 { - b = marshalUint64(b, fileStat.Size) - } - if flags&sshFileXferAttrUIDGID != 0 { - b = marshalUint32(b, fileStat.UID) - b = marshalUint32(b, fileStat.GID) - } - if flags&sshFileXferAttrPermissions != 0 { - b = marshalUint32(b, fileStat.Mode) - } - if flags&sshFileXferAttrACmodTime != 0 { - b = marshalUint32(b, fileStat.Atime) - b = marshalUint32(b, fileStat.Mtime) - } - - if flags&sshFileXferAttrExtended != 0 { - b = marshalUint32(b, uint32(len(fileStat.Extended))) - - for _, attr := range fileStat.Extended { - b = marshalString(b, attr.ExtType) - b = marshalString(b, attr.ExtData) - } - } + b = append(b, fileStat.MarshalTo(f)...) return b } From 706f4f121bd0445609fd83489a60d6959128b062 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Wed, 1 Nov 2023 19:32:45 -0700 Subject: [PATCH 28/30] Use 3 digit modes, not 4. That is, 0oNNN instead of 0oNNNN, this is because we don't have any current cases where we're using the 4th digit, and so things turned into 0o0200. --- request-example.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/request-example.go b/request-example.go index 77735d62..89df58b3 100644 --- a/request-example.go +++ b/request-example.go @@ -38,7 +38,7 @@ func (fs *root) Fileread(r *Request) (io.ReaderAt, error) { } // Needs to be readable by the owner. - return fs.openFileModeCheck(r, 0o0400) + return fs.openFileModeCheck(r, 0o400) } func (fs *root) Filewrite(r *Request) (io.WriterAt, error) { @@ -49,12 +49,12 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) { } // Needs to be writable by the owner. - return fs.openFileModeCheck(r, 0o0200) + return fs.openFileModeCheck(r, 0o200) } func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) { // Needs to be readable and writable by the owner. - return fs.openFileModeCheck(r, 0o0200|0o0400) + return fs.openFileModeCheck(r, 0o200|0o400) } func (fs *root) openFileModeCheck(r *Request, mode uint32) (WriterAtReaderAt, error) { From 1ae273828253723972a4cbafd329179bc82aaaf8 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Thu, 2 Nov 2023 17:45:00 -0700 Subject: [PATCH 29/30] MarshalTo now takes a []byte argument. --- attrs.go | 4 +--- packet.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/attrs.go b/attrs.go index 2d623d8d..b633d207 100644 --- a/attrs.go +++ b/attrs.go @@ -56,7 +56,7 @@ type FileStat struct { Extended []StatExtended } -func (fs FileStat) MarshalTo(flags FileAttrFlags) []byte { +func (fs FileStat) MarshalTo(b []byte, flags FileAttrFlags) []byte { // attributes variable struct, and also variable per protocol version // spec version 3 attributes: // uint32 flags @@ -71,8 +71,6 @@ func (fs FileStat) MarshalTo(flags FileAttrFlags) []byte { // string extended_data // ... more extended data (extended_type - extended_data pairs), // so that number of pairs equals extended_count - var b []byte - if flags.Size { b = marshalUint64(b, fs.Size) } diff --git a/packet.go b/packet.go index f785a0cd..076f73d4 100644 --- a/packet.go +++ b/packet.go @@ -57,7 +57,7 @@ func marshalFileInfo(b []byte, fi os.FileInfo) []byte { f := newFileAttrFlags(flags) b = marshalUint32(b, flags) - b = append(b, fileStat.MarshalTo(f)...) + b = fileStat.MarshalTo(b, f) return b } From 3d7324a0a384211cd1fdc40da8cc88041beacd55 Mon Sep 17 00:00:00 2001 From: Zeph / Liz Loss-Cutler-Hull Date: Thu, 2 Nov 2023 17:48:02 -0700 Subject: [PATCH 30/30] Add warning comments around the Extended logic. --- attrs.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/attrs.go b/attrs.go index b633d207..6027c6af 100644 --- a/attrs.go +++ b/attrs.go @@ -86,6 +86,8 @@ func (fs FileStat) MarshalTo(b []byte, flags FileAttrFlags) []byte { b = marshalUint32(b, fs.Mtime) } + // NOTE: This is subtle, this logic must not be changed without also changing the login in fileStatFromInfo. + // The rules on how sshFileXferAttrExtended gets set must match the rules on how we generate the packet. if len(fs.Extended) > 0 { b = marshalUint32(b, uint32(len(fs.Extended))) @@ -151,6 +153,9 @@ func fileStatFromInfo(fi os.FileInfo) (uint32, *FileStat) { fileStat.GID = fiExt.Gid() } + // NOTE: This is subtle, this logic must not be changed without also changing the login in marshalTo. + // The rules on how sshFileXferAttrExtended gets set must match the rules on how we generate the packet. + // // if fi implements FileInfoExtendedData, retrieve extended data from it if fiExt, ok := fi.(FileInfoExtendedData); ok { fileStat.Extended = fiExt.Extended()