Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add support for attrs in sshFxpOpenPacket for Server #567

Closed
wants to merge 6 commits into from
Closed

fix: add support for attrs in sshFxpOpenPacket for Server #567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c771381
fix: add support for attrs in sshFxpOpenPacket for Server
mafredri Dec 12, 2023
8218e92
avoid changing permission on existing files
mafredri Dec 12, 2023
211f87b
simplify attrs for sshFxpOpenPacket
mafredri Dec 16, 2023
a407de1
switch to toFileMode
mafredri Dec 16, 2023
268f8b0
fix rogue os.FileMode conversion in FileStat
mafredri Dec 16, 2023
cb46041
clean up attr usage in stat
mafredri Dec 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,12 +681,13 @@ type sshFxpOpenPacket struct {
ID uint32
Path string
Pflags uint32
Flags uint32 // ignored
Flags uint32
Attrs interface{}
}

func (p *sshFxpOpenPacket) id() uint32 { return p.ID }

func (p *sshFxpOpenPacket) MarshalBinary() ([]byte, error) {
func (p *sshFxpOpenPacket) marshalPacket() ([]byte, []byte, error) {
l := 4 + 1 + 4 + // uint32(length) + byte(type) + uint32(id)
4 + len(p.Path) +
4 + 4
Expand All @@ -698,7 +699,14 @@ func (p *sshFxpOpenPacket) MarshalBinary() ([]byte, error) {
b = marshalUint32(b, p.Pflags)
b = marshalUint32(b, p.Flags)

return b, nil
payload := marshal(nil, p.Attrs)
puellanivis marked this conversation as resolved.
Show resolved Hide resolved

return b, payload, nil
}

func (p *sshFxpOpenPacket) MarshalBinary() ([]byte, error) {
header, payload, err := p.marshalPacket()
return append(header, payload...), err
}

func (p *sshFxpOpenPacket) UnmarshalBinary(b []byte) error {
Expand All @@ -709,9 +717,10 @@ func (p *sshFxpOpenPacket) UnmarshalBinary(b []byte) error {
return err
} else if p.Pflags, b, err = unmarshalUint32Safe(b); err != nil {
return err
} else if p.Flags, _, err = unmarshalUint32Safe(b); err != nil {
} else if p.Flags, b, err = unmarshalUint32Safe(b); err != nil {
return err
}
p.Attrs = b
return nil
}

Expand Down
18 changes: 18 additions & 0 deletions packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,24 @@ func TestSendPacket(t *testing.T) {
0x0, 0x0, 0x0, 0x0,
},
},
{
packet: &sshFxpOpenPacket{
ID: 3,
Path: "/foo",
Pflags: flags(os.O_WRONLY | os.O_CREATE | os.O_TRUNC),
Flags: sshFileXferAttrPermissions,
Attrs: []uint8{0x0, 0x0, 0x1, 0xed}, // 0o755
},
want: []byte{
0x0, 0x0, 0x0, 0x19,
0x3,
0x0, 0x0, 0x0, 0x3,
0x0, 0x0, 0x0, 0x4, '/', 'f', 'o', 'o',
0x0, 0x0, 0x0, 0x1a,
0x0, 0x0, 0x0, 0x4,
0x0, 0x0, 0x1, 0xed,
},
},
{
packet: &sshFxpWritePacket{
ID: 124,
Expand Down
2 changes: 1 addition & 1 deletion request-attrs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (r *Request) AttrFlags() FileAttrFlags {

// FileMode returns the Mode SFTP file attributes wrapped as os.FileMode
func (a FileStat) FileMode() os.FileMode {
return os.FileMode(a.Mode)
return toFileMode(a.Mode)
}

// Attributes parses file attributes byte blob and return them in a
Expand Down
100 changes: 32 additions & 68 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,16 @@ func (p *sshFxpOpenPacket) respond(svr *Server) responsePacket {
osFlags |= os.O_EXCL
}

f, err := os.OpenFile(svr.toLocalPath(p.Path), osFlags, 0o644)
mode := os.FileMode(0o644)
// Like OpenSSH, we only handle permissions here if the file is
// being created. Otherwise, the permissions are ignored.
if b, ok := p.Attrs.([]byte); ok && (p.Flags&sshFileXferAttrPermissions) != 0 {
fs, _ := unmarshalFileStat(p.Flags, b)
mode = toFileMode(fs.Mode)
}

name := svr.toLocalPath(p.Path)
f, err := os.OpenFile(name, osFlags, mode)
if err != nil {
return statusFromError(p.ID, err)
}
Expand Down Expand Up @@ -496,47 +505,11 @@ func (p *sshFxpReaddirPacket) respond(svr *Server) responsePacket {
}

func (p *sshFxpSetstatPacket) respond(svr *Server) responsePacket {
// additional unmarshalling is required for each possibility here
b := p.Attrs.([]byte)
var err error

p.Path = svr.toLocalPath(p.Path)

debug("setstat name \"%s\"", p.Path)
if (p.Flags & sshFileXferAttrSize) != 0 {
var size uint64
if size, b, err = unmarshalUint64Safe(b); err == nil {
err = os.Truncate(p.Path, int64(size))
}
}
if (p.Flags & sshFileXferAttrPermissions) != 0 {
var mode uint32
if mode, b, err = unmarshalUint32Safe(b); err == nil {
err = os.Chmod(p.Path, os.FileMode(mode))
}
}
if (p.Flags & sshFileXferAttrACmodTime) != 0 {
var atime uint32
var mtime uint32
if atime, b, err = unmarshalUint32Safe(b); err != nil {
} else if mtime, b, err = unmarshalUint32Safe(b); err != nil {
} else {
atimeT := time.Unix(int64(atime), 0)
mtimeT := time.Unix(int64(mtime), 0)
err = os.Chtimes(p.Path, atimeT, mtimeT)
}
}
if (p.Flags & sshFileXferAttrUIDGID) != 0 {
var uid uint32
var gid uint32
if uid, b, err = unmarshalUint32Safe(b); err != nil {
} else if gid, _, err = unmarshalUint32Safe(b); err != nil {
} else {
err = os.Chown(p.Path, int(uid), int(gid))
}
}

return statusFromError(p.ID, err)
b := p.Attrs.([]byte)
return statusFromError(p.ID, applyAttrsToFile(p.Path, p.Flags, b))
}

func (p *sshFxpFsetstatPacket) respond(svr *Server) responsePacket {
Expand All @@ -545,45 +518,36 @@ func (p *sshFxpFsetstatPacket) respond(svr *Server) responsePacket {
return statusFromError(p.ID, EBADF)
}

// additional unmarshalling is required for each possibility here
debug("fsetstat name \"%s\"", f.Name())
b := p.Attrs.([]byte)
var err error
return statusFromError(p.ID, applyAttrsToFile(f.Name(), p.Flags, b))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear. :( I’m unsure of how this subtle change from operating on a handle to operating on the filename would work. We’ve run into issues before where performing operations on what should be an open handle but done to the filename, or on a filename but through an open/close block.

I know Chtimes only exists for the filename, and not on a handle… but we could at least attempt to isolate this operation.

Also know that various systems will open a file, then delete it. This is a really weird corner case, but it seems like everyone runs into this weird pattern eventually and get tripped up assuming that a file will always be accessible from its filename while it is currently open.

}

debug("fsetstat name \"%s\"", f.Name())
if (p.Flags & sshFileXferAttrSize) != 0 {
var size uint64
if size, b, err = unmarshalUint64Safe(b); err == nil {
err = f.Truncate(int64(size))
func applyAttrsToFile(name string, flags uint32, attrs []byte) error {
fs, _ := unmarshalFileStat(flags, attrs)
if (flags & sshFileXferAttrSize) != 0 {
if err := os.Truncate(name, int64(fs.Size)); err != nil {
return err
}
}
if (p.Flags & sshFileXferAttrPermissions) != 0 {
var mode uint32
if mode, b, err = unmarshalUint32Safe(b); err == nil {
err = f.Chmod(os.FileMode(mode))
if (flags & sshFileXferAttrUIDGID) != 0 {
if err := os.Chown(name, int(fs.UID), int(fs.GID)); err != nil {
return err
}
}
if (p.Flags & sshFileXferAttrACmodTime) != 0 {
var atime uint32
var mtime uint32
if atime, b, err = unmarshalUint32Safe(b); err != nil {
} else if mtime, b, err = unmarshalUint32Safe(b); err != nil {
} else {
atimeT := time.Unix(int64(atime), 0)
mtimeT := time.Unix(int64(mtime), 0)
err = os.Chtimes(f.Name(), atimeT, mtimeT)
if (flags & sshFileXferAttrPermissions) != 0 {
if err := os.Chmod(name, toFileMode(fs.Mode)); err != nil {
return err
}
}
if (p.Flags & sshFileXferAttrUIDGID) != 0 {
var uid uint32
var gid uint32
if uid, b, err = unmarshalUint32Safe(b); err != nil {
} else if gid, _, err = unmarshalUint32Safe(b); err != nil {
} else {
err = f.Chown(int(uid), int(gid))
if (flags & sshFileXferAttrACmodTime) != 0 {
atimeT := time.Unix(int64(fs.Atime), 0)
mtimeT := time.Unix(int64(fs.Mtime), 0)
if err := os.Chtimes(name, atimeT, mtimeT); err != nil {
return err
}
}

return statusFromError(p.ID, err)
return nil
}

func statusFromError(id uint32, err error) *sshFxpStatusPacket {
Expand Down
45 changes: 45 additions & 0 deletions server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,51 @@ func TestOpenStatRace(t *testing.T) {
checkServerAllocator(t, server)
}

func TestOpenWithPermissions(t *testing.T) {
client, server := clientServerPair(t)
defer client.Close()
defer server.Close()

tmppath := path.Join(os.TempDir(), "open_permissions")
pflags := flags(os.O_RDWR | os.O_CREATE | os.O_TRUNC)
puellanivis marked this conversation as resolved.
Show resolved Hide resolved
ch := make(chan result, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This channel should never have more than one in flight. (The design of clientConn guarantees an “at most once” delivery on a channel given to dispatchRequest, and after the first dispatchRequest this goroutine blocks waiting for that response.)

id1 := client.nextID()
id2 := client.nextID()

// New files should have their permissions set.
client.dispatchRequest(ch, &sshFxpOpenPacket{
ID: id1,
Path: tmppath,
Pflags: pflags,
Flags: sshFileXferAttrPermissions,
Attrs: []byte{0x0, 0x0, 0x1, 0xe5}, // 0o745 -- a slightly strange permission to test.
})
<-ch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably test the error returned from the dispatchRequest channel return?

Why not actually just avoid channel tedium here entirely and use the slightly higher level sendPacket which will do the receive and extract the error from the result into a return value for you?

stat, err := os.Stat(tmppath)
assert.NoError(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a require.NoError as if err != nil then stat == nil and the following line will panic.

if !assert.Equal(t, os.FileMode(0o745), stat.Mode()&os.ModePerm) {
t.Logf("stat.Mode() = %v", stat.Mode())
}

// Existing files should not have their permissions changed.
client.dispatchRequest(ch, &sshFxpOpenPacket{
ID: id2,
Path: tmppath,
Pflags: pflags,
Flags: sshFileXferAttrPermissions,
Attrs: []byte{0x0, 0x0, 0x1, 0xed}, // 0o755
})
<-ch
stat, err = os.Stat(tmppath)
assert.NoError(t, err, "mode should not have changed")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test failure message doesn’t make sense. We don’t know if the mode has changed or not yet. We just know that the Stat failed, and this could point to a deeper issue than just “mode should not have changed“.

Fix: just don’t put a log message here. Simply there being an error message popping up here is clue enough that an unexpected error occurred, and deeper review is necessary of what is happening.

if !assert.Equal(t, os.FileMode(0o745), stat.Mode()&os.ModePerm) {
t.Logf("stat.Mode() = %v", stat.Mode())
}

os.Remove(tmppath)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this in a defer so that it gets performed even if a t.Fail is called.

checkServerAllocator(t, server)
}

// Ensure that proper error codes are returned for non existent files, such
// that they are mapped back to a 'not exists' error on the client side.
func TestStatNonExistent(t *testing.T) {
Expand Down