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 2 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
136 changes: 113 additions & 23 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,11 +462,85 @@ func (p *sshFxpOpenPacket) respond(svr *Server) responsePacket {
osFlags |= os.O_EXCL
}

f, err := os.OpenFile(svr.toLocalPath(p.Path), osFlags, 0o644)
name := svr.toLocalPath(p.Path)
mode := os.FileMode(0o644)

var applyAttrs []func(*os.File) error

// Both `sshFileXferAttrPermissions` and `sshFileXferAttrACmodTime` are set
// by e.g. `sftp`. Just in case, we handle all other cases as well.
// Only apply for newly created files.
var err error
useAttrs := true
if _, err := os.Stat(name); err == nil {
mafredri marked this conversation as resolved.
Show resolved Hide resolved
useAttrs = false
}
if b, ok := p.Attrs.([]byte); useAttrs && ok {
if (p.Flags & sshFileXferAttrSize) != 0 {
mafredri marked this conversation as resolved.
Show resolved Hide resolved
var size uint64
if size, b, err = unmarshalUint64Safe(b); err == nil {
applyAttrs = append(applyAttrs, func(f *os.File) error {
return f.Truncate(int64(size))
mafredri marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrUIDGID) != 0 {
var uid uint32
var gid uint32
if uid, b, err = unmarshalUint32Safe(b); err != nil {
} else if gid, b, err = unmarshalUint32Safe(b); err != nil {
} else {
applyAttrs = append(applyAttrs, func(f *os.File) error {
return f.Chown(int(uid), int(gid))
})
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrPermissions) != 0 {
var attrMode uint32
if attrMode, b, err = unmarshalUint32Safe(b); err == nil {
// Optionally, we could apply umask here.
mode = os.FileMode(attrMode)
mafredri marked this conversation as resolved.
Show resolved Hide resolved
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrACmodTime) != 0 {
var atime uint32
var mtime uint32
if atime, b, err = unmarshalUint32Safe(b); err != nil {
} else if mtime, _, err = unmarshalUint32Safe(b); err != nil {
} else {
atimeT := time.Unix(int64(atime), 0)
mtimeT := time.Unix(int64(mtime), 0)
applyAttrs = append(applyAttrs, func(f *os.File) error {
return os.Chtimes(f.Name(), atimeT, mtimeT)
})
}
}
if err != nil {
return statusFromError(p.ID, err)
}
}

f, err := os.OpenFile(name, osFlags, mode)
if err != nil {
return statusFromError(p.ID, err)
}

for _, applyAttr := range applyAttrs {
if err := applyAttr(f); err != nil {
_ = f.Close()
return statusFromError(p.ID, err)
}
}

handle := svr.nextHandle(f)
return &sshFxpHandlePacket{ID: p.ID, Handle: handle}
}
Expand Down Expand Up @@ -509,33 +583,41 @@ func (p *sshFxpSetstatPacket) respond(svr *Server) responsePacket {
err = os.Truncate(p.Path, int64(size))
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrUIDGID) != 0 {
var uid uint32
var gid uint32
if uid, b, err = unmarshalUint32Safe(b); err != nil {
} else if gid, b, err = unmarshalUint32Safe(b); err != nil {
} else {
err = os.Chown(p.Path, int(uid), int(gid))
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrPermissions) != 0 {
var mode uint32
if mode, b, err = unmarshalUint32Safe(b); err == nil {
err = os.Chmod(p.Path, os.FileMode(mode))
mafredri marked this conversation as resolved.
Show resolved Hide resolved
}
}
if err != nil {
return statusFromError(p.ID, 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 if mtime, _, 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)
}

Expand All @@ -556,33 +638,41 @@ func (p *sshFxpFsetstatPacket) respond(svr *Server) responsePacket {
err = f.Truncate(int64(size))
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrUIDGID) != 0 {
var uid uint32
var gid uint32
if uid, b, err = unmarshalUint32Safe(b); err != nil {
} else if gid, b, err = unmarshalUint32Safe(b); err != nil {
} else {
err = f.Chown(int(uid), int(gid))
}
}
if err != nil {
return statusFromError(p.ID, err)
}
if (p.Flags & sshFileXferAttrPermissions) != 0 {
var mode uint32
if mode, b, err = unmarshalUint32Safe(b); err == nil {
err = f.Chmod(os.FileMode(mode))
mafredri marked this conversation as resolved.
Show resolved Hide resolved
}
}
if err != nil {
return statusFromError(p.ID, 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 if mtime, _, 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 (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))
}
}

return statusFromError(p.ID, err)
}

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