Repository: pkg/sftp
Pull request #414 (2021-03-11 16:45:30)

Support os.Mode{Setuid,Setgid,Sticky} in Chmod
Previously, these bits were ignored by Chmod, which sent the numerical value of the mode argument as-is to the server. As a result, callers had to supply POSIX values for setuid, setgid and sticky to Chmod.

The new version supports both the POSIX values and the Go values, at the expense of some code duplication.

Also added a note to the docs to clarify that the umask is not subtracted from the mode, and why that is. The only portable way to get the umask is to set it, then reset it, but that's racy. On Linux, we could parse /proc/self/status, but that doesn't work portably and will fail where /proc is not available (some Docker containers, notably). Ref #335.

greatroar: Tests fail on Travis, while they work locally. May be a capability issue (?)
greatroar: Got it. The test works with sftp, but not with -testserver.

Patch:
diff --git a/client.go b/client.go
index a0000f8..1753f7c 100644
--- a/client.go
+++ b/client.go
@@ -273,10 +273,7 @@ func (c *Client) recvVersion() error {
 		return &unexpectedPacketErr{sshFxpVersion, typ}
 	}
 
-	version, data, err := unmarshalUint32Safe(data)
-	if err != nil {
-		return err
-	}
+	version, data := unmarshalUint32(data)
 	if version != sftpProtocolVersion {
 		return &unexpectedVersionErr{sftpProtocolVersion, version}
 	}
@@ -563,8 +560,12 @@ func (c *Client) Chown(path string, uid, gid int) error {
 }
 
 // Chmod changes the permissions of the named file.
+//
+// Chmod does not apply a umask, because even retrieving the umask is not
+// possible in a portable way without causing a race condition. Callers
+// should mask off umask bits, if desired.
 func (c *Client) Chmod(path string, mode os.FileMode) error {
-	return c.setstat(path, sshFileXferAttrPermissions, uint32(mode))
+	return c.setstat(path, sshFileXferAttrPermissions, toChmodPerm(mode))
 }
 
 // Truncate sets the size of the named file. Although it may be safely assumed
@@ -867,12 +868,12 @@ func (c *Client) MkdirAll(path string) error {
 
 	// Slow path: make sure parent exists and then call Mkdir for path.
 	i := len(path)
-	for i > 0 && path[i-1] == '/' { // Skip trailing path separator.
+	for i > 0 && os.IsPathSeparator(path[i-1]) { // Skip trailing path separator.
 		i--
 	}
 
 	j := i
-	for j > 0 && path[j-1] != '/' { // Scan backward over element.
+	for j > 0 && !os.IsPathSeparator(path[j-1]) { // Scan backward over element.
 		j--
 	}
 
@@ -1722,6 +1723,8 @@ func (f *File) Chown(uid, gid int) error {
 }
 
 // Chmod changes the permissions of the current file.
+//
+// See Client.Chmod for details.
 func (f *File) Chmod(mode os.FileMode) error {
 	return f.c.Chmod(f.path, mode)
 }
@@ -1833,3 +1836,25 @@ func flags(f int) uint32 {
 	}
 	return out
 }
+
+// toChmodPerm converts Go permission bits to POSIX permission bits.
+//
+// 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) {
+	const mask = os.ModePerm | s_ISUID | s_ISGID | s_ISVTX
+	perm = uint32(m & mask)
+
+	if m&os.ModeSetuid != 0 {
+		perm |= s_ISUID
+	}
+	if m&os.ModeSetgid != 0 {
+		perm |= s_ISGID
+	}
+	if m&os.ModeSticky != 0 {
+		perm |= s_ISVTX
+	}
+
+	return perm
+}
diff --git a/stat_plan9.go b/stat_plan9.go
index 32f17e0..25074fe 100644
--- a/stat_plan9.go
+++ b/stat_plan9.go
@@ -94,3 +94,11 @@ func fromFileMode(mode os.FileMode) uint32 {
 
 	return ret
 }
+
+// Plan 9 doesn't have setuid, setgid or sticky, but a Plan 9 client should
+// be able to send these bits to a POSIX server.
+const (
+	s_ISUID = 04000
+	s_ISGID = 02000
+	S_ISVTX = 01000
+)
diff --git a/stat_posix.go b/stat_posix.go
index 38ba26a..71080ac 100644
--- a/stat_posix.go
+++ b/stat_posix.go
@@ -114,3 +114,9 @@ func fromFileMode(mode os.FileMode) uint32 {
 
 	return ret
 }
+
+const (
+	s_ISUID = syscall.S_ISUID
+	s_ISGID = syscall.S_ISGID
+	s_ISVTX = syscall.S_ISVTX
+)


Test patch:
diff --git a/client_integration_test.go b/client_integration_test.go
index e040115..cca1078 100644
--- a/client_integration_test.go
+++ b/client_integration_test.go
@@ -873,6 +873,59 @@ func TestClientChmodReadonly(t *testing.T) {
 	}
 }
 
+func TestClientSetuid(t *testing.T) {
+	skipIfWindows(t) // No UNIX permissions.
+	if *testServerImpl {
+		t.Skipf("skipping with -testserver")
+	}
+
+	sftp, cmd := testClient(t, READWRITE, NODELAY)
+	defer cmd.Wait()
+	defer sftp.Close()
+
+	f, err := ioutil.TempFile("", "sftptest-setuid")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(f.Name())
+	f.Close()
+
+	const allPerm = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky |
+		s_ISUID | s_ISGID | s_ISVTX
+
+	for _, c := range []struct {
+		goPerm    os.FileMode
+		posixPerm uint32
+	}{
+		{os.ModeSetuid, s_ISUID},
+		{os.ModeSetgid, s_ISGID},
+		{os.ModeSticky, s_ISVTX},
+		{os.ModeSetuid | os.ModeSticky, s_ISUID | s_ISVTX},
+	} {
+		goPerm := 0700 | c.goPerm
+		posixPerm := 0700 | c.posixPerm
+
+		err = sftp.Chmod(f.Name(), goPerm)
+		require.NoError(t, err)
+
+		info, err := sftp.Stat(f.Name())
+		require.NoError(t, err)
+		require.Equal(t, goPerm, info.Mode()&allPerm)
+
+		err = sftp.Chmod(f.Name(), 0700) // Reset funny bits.
+		require.NoError(t, err)
+
+		// For historical reasons, we also support literal POSIX mode bits in
+		// Chmod. Stat should still translate these to Go os.FileMode bits.
+		err = sftp.Chmod(f.Name(), os.FileMode(posixPerm))
+		require.NoError(t, err)
+
+		info, err = sftp.Stat(f.Name())
+		require.NoError(t, err)
+		require.Equal(t, goPerm, info.Mode()&allPerm)
+	}
+}
+
 func TestClientChown(t *testing.T) {
 	skipIfWindows(t) // No UNIX permissions.
 	sftp, cmd := testClient(t, READWRITE, NODELAY)
diff --git a/client_test.go b/client_test.go
index 05b885c..b2eacbc 100644
--- a/client_test.go
+++ b/client_test.go
@@ -228,14 +228,3 @@ func TestClientZeroLengthPacket(t *testing.T) {
 		c.Close()
 	}
 }
-
-func TestClientShortPacket(t *testing.T) {
-	// init packet too short.
-	packet := []byte{0, 0, 0, 1, 2}
-
-	r := bytes.NewReader(packet)
-	_, err := NewClientPipe(r, &sink{})
-	if !errors.Is(err, errShortPacket) {
-		t.Fatalf("expected error: %v, got: %v", errShortPacket, err)
-	}
-}