Skip to content

Commit 8bc4620

Browse files
AkihiroSudaAkihiroSuda
committed
Use OpenSSH SFTP Server instead of github.com/pkg/sftp (when available)
The OpenSSH SFTP Server Driver (`--driver=openssh-sftp-server`) is more robust and secure than the legacy builtin driver (`--driver=builtin`). No performance benefit though. The OpenSSH SFTP Server Driver is used by default when `sftp-server` binary is installed on the host. (Installed by default on macOS) Signed-off-by: Akihiro Suda <[email protected]>
1 parent 5539a27 commit 8bc4620

File tree

4 files changed

+213
-57
lines changed

4 files changed

+213
-57
lines changed

‎README.md

+9
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,15 @@ SSH flags:
5757
* `-F`, `--ssh-config=FILE`: specify SSH config file used for `ssh -F`
5858
* `--ssh-persist=(true|false)` (default: `true`): enable ControlPersist
5959

60+
SSHFS flags:
61+
* `--sshfs-noempty` (default: `false`): enable sshfs nonempty
62+
63+
SFTP server flags:
64+
* `--driver=DRIVER` (default: `auto`): SFTP server driver. `builtin` (legacy) or `openssh-sftp-server` (robust and secure, recommended).
65+
`openssh-sftp-server` is chosen by default when the OpenSSH SFTP Server binary is detected.
66+
* `--openssh-sftp-server=BINARY`: OpenSSH SFTP Server binary.
67+
Automatically detected when installed in well-known locations such as `/usr/libexec/sftp-server`.
68+
6069
### Subcommand: `help`
6170
Shows help
6271

‎cmd/sshocker/run.go

+17-5
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,16 @@ var (
4242
Usage: "enable sshfs nonempty",
4343
Value: false,
4444
},
45+
&cli.StringFlag{
46+
Name: "driver",
47+
Usage: "SFTP server driver. \"builtin\" (legacy) or \"openssh-sftp-server\" (robust and secure, recommended), automatically chosen by default",
48+
Value: "auto",
49+
},
50+
&cli.StringFlag{
51+
Name: "openssh-sftp-server",
52+
Usage: "OpenSSH SFTP Server binary, automatically chosen by default",
53+
Value: "",
54+
},
4555
}
4656
runCommand = &cli.Command{
4757
Name: "run",
@@ -85,11 +95,13 @@ func runAction(clicontext *cli.Context) error {
8595
sshfsAdditionalArgs = append(sshfsAdditionalArgs, "-o", "nonempty")
8696
}
8797
x := &sshocker.Sshocker{
88-
SSHConfig: sshConfig,
89-
Host: host,
90-
Port: port,
91-
Command: clicontext.Args().Tail(),
92-
SSHFSAdditionalArgs: sshfsAdditionalArgs,
98+
SSHConfig: sshConfig,
99+
Host: host,
100+
Port: port,
101+
Command: clicontext.Args().Tail(),
102+
SSHFSAdditionalArgs: sshfsAdditionalArgs,
103+
Driver: clicontext.String("driver"),
104+
OpensshSftpServerBinary: clicontext.String("openssh-sftp-server"),
93105
}
94106
if len(x.Command) > 0 && x.Command[0] == "--" {
95107
x.Command = x.Command[1:]

‎pkg/reversesshfs/reversesshfs.go

+170-39
Original file line numberDiff line numberDiff line change
@@ -10,22 +10,34 @@ import (
1010
"os/exec"
1111
"path/filepath"
1212
"strconv"
13+
"strings"
1314

1415
"github.com/lima-vm/sshocker/pkg/ssh"
1516
"github.com/lima-vm/sshocker/pkg/util"
1617
"github.com/pkg/sftp"
1718
"github.com/sirupsen/logrus"
1819
)
1920

21+
type Driver = string
22+
23+
const (
24+
DriverAuto = Driver("auto") // Default
25+
DriverBuiltin = Driver("builtin") // Legacy. Unrecommended.
26+
DriverOpensshSftpServer = Driver("openssh-sftp-server") // More robust and secure. Recommended.
27+
)
28+
2029
type ReverseSSHFS struct {
2130
*ssh.SSHConfig
22-
LocalPath string
23-
Host string
24-
Port int
25-
RemotePath string
26-
Readonly bool
27-
sshCmd *exec.Cmd
28-
SSHFSAdditionalArgs []string
31+
Driver Driver
32+
OpensshSftpServerBinary string // used only when Driver == DriverOpensshSftpServer
33+
LocalPath string
34+
Host string
35+
Port int
36+
RemotePath string
37+
Readonly bool
38+
sshCmd *exec.Cmd
39+
opensshSftpServerCmd *exec.Cmd
40+
SSHFSAdditionalArgs []string
2941
}
3042

3143
func (rsf *ReverseSSHFS) Prepare() error {
@@ -48,6 +60,55 @@ func (rsf *ReverseSSHFS) Prepare() error {
4860
return nil
4961
}
5062

63+
func DetectOpensshSftpServerBinary() string {
64+
homebrewSSHD := []string{
65+
"/usr/local/sbin/sshd",
66+
"/opt/homebrew/sbin/sshd",
67+
}
68+
for _, f := range homebrewSSHD {
69+
// sshd is like "/usr/local/Cellar/openssh/8.9p1/sbin/sshd"
70+
sshd, err := filepath.EvalSymlinks(f)
71+
if err != nil {
72+
continue
73+
}
74+
// local is like "/usr/local/Cellar/openssh"
75+
local := filepath.Dir(filepath.Dir(sshd))
76+
// sftpServer is like "/usr/local/Cellar/openssh/8.9p1/libexec/sftp-server"
77+
sftpServer := filepath.Join(local, "libexec", "sftp-server")
78+
if exe, err := exec.LookPath(sftpServer); err == nil {
79+
return exe
80+
}
81+
}
82+
candidates := []string{
83+
"/usr/libexec/sftp-server", // macOS, OpenWrt
84+
"/usr/libexec/openssh/sftp-server", // Fedora
85+
"/usr/lib/sftp-server", // Debian (symlink to openssh/sftp-server)
86+
"/usr/lib/openssh/sftp-server", // Debian
87+
"/usr/lib/ssh/sftp-server", // Alpine
88+
}
89+
for _, cand := range candidates {
90+
if exe, err := exec.LookPath(cand); err == nil {
91+
return exe
92+
}
93+
}
94+
return ""
95+
}
96+
97+
func DetectDriver(explicitOpensshSftpServerBinary string) (Driver, string, error) {
98+
if explicitOpensshSftpServerBinary != "" {
99+
exe, err := exec.LookPath(explicitOpensshSftpServerBinary)
100+
if err != nil {
101+
return "", "", err
102+
}
103+
return DriverOpensshSftpServer, exe, nil
104+
}
105+
exe := DetectOpensshSftpServerBinary()
106+
if exe != "" {
107+
return DriverOpensshSftpServer, exe, nil
108+
}
109+
return DriverBuiltin, "", nil
110+
}
111+
51112
func (rsf *ReverseSSHFS) Start() error {
52113
sshBinary := rsf.SSHConfig.Binary()
53114
sshArgs := rsf.SSHConfig.Args()
@@ -68,44 +129,103 @@ func (rsf *ReverseSSHFS) Start() error {
68129
sshArgs = append(sshArgs, rsf.SSHFSAdditionalArgs...)
69130
rsf.sshCmd = exec.Command(sshBinary, sshArgs...)
70131
rsf.sshCmd.Stderr = os.Stderr
71-
stdinPipe, err := rsf.sshCmd.StdinPipe()
72-
if err != nil {
73-
return err
74-
}
75-
stdoutPipe, err := rsf.sshCmd.StdoutPipe()
76-
if err != nil {
77-
return err
78-
}
79-
stdio := &util.RWC{
80-
ReadCloser: stdoutPipe,
81-
WriteCloser: stdinPipe,
82-
}
83-
var sftpOpts []sftp.ServerOption
84-
if rsf.Readonly {
85-
sftpOpts = append(sftpOpts, sftp.ReadOnly())
132+
driver := rsf.Driver
133+
opensshSftpServerBinary := rsf.OpensshSftpServerBinary
134+
switch driver {
135+
case DriverBuiltin, DriverOpensshSftpServer:
136+
// NOP
137+
case "", DriverAuto:
138+
var err error
139+
driver, opensshSftpServerBinary, err = DetectDriver(opensshSftpServerBinary)
140+
if err != nil {
141+
return fmt.Errorf("failed to choose driver automatically: %w", err)
142+
}
143+
logrus.Debugf("Chosen driver %q", driver)
144+
default:
145+
return fmt.Errorf("unknown driver %q", driver)
86146
}
87-
// NOTE: sftp.NewServer doesn't support specifying the root.
88-
// https://github.com/pkg/sftp/pull/238
89-
//
90-
// TODO: use sftp.NewRequestServer with custom handlers to mitigate potential vulnerabilities.
91-
server, err := sftp.NewServer(stdio, sftpOpts...)
92-
if err != nil {
93-
return err
147+
var builtinSftpServer *sftp.Server
148+
switch driver {
149+
case DriverBuiltin:
150+
stdinPipe, err := rsf.sshCmd.StdinPipe()
151+
if err != nil {
152+
return err
153+
}
154+
stdoutPipe, err := rsf.sshCmd.StdoutPipe()
155+
if err != nil {
156+
return err
157+
}
158+
stdio := &util.RWC{
159+
ReadCloser: stdoutPipe,
160+
WriteCloser: stdinPipe,
161+
}
162+
var sftpOpts []sftp.ServerOption
163+
if rsf.Readonly {
164+
sftpOpts = append(sftpOpts, sftp.ReadOnly())
165+
}
166+
// NOTE: sftp.NewServer doesn't support specifying the root.
167+
// https://github.com/pkg/sftp/pull/238
168+
//
169+
// TODO: use sftp.NewRequestServer with custom handlers to mitigate potential vulnerabilities.
170+
builtinSftpServer, err = sftp.NewServer(stdio, sftpOpts...)
171+
if err != nil {
172+
return err
173+
}
174+
case DriverOpensshSftpServer:
175+
if opensshSftpServerBinary == "" {
176+
opensshSftpServerBinary = DetectOpensshSftpServerBinary()
177+
if opensshSftpServerBinary == "" {
178+
return errors.New("no openssh sftp-server found")
179+
}
180+
}
181+
logrus.Debugf("Using OpenSSH SFTP Server %q", opensshSftpServerBinary)
182+
sftpServerArgs := []string{
183+
// `-e` available since OpenSSH 5.4p1 (2010) https://github.com/openssh/openssh-portable/commit/7bee06ab
184+
"-e",
185+
// `-d` available since OpenSSH 6.2p1 (2013) https://github.com/openssh/openssh-portable/commit/502ab0ef
186+
// NOTE: `-d` just chdirs the sftp server process to the specified directory.
187+
// This is expected to be used in conjunction with chroot (in future), however, macOS does not support unprivileged chroot.
188+
"-d", strings.ReplaceAll(rsf.LocalPath, "%", "%%"),
189+
}
190+
if rsf.Readonly {
191+
// `-R` available since OpenSSH 5.4p1 (2010) https://github.com/openssh/openssh-portable/commit/db7bf825
192+
sftpServerArgs = append(sftpServerArgs, "-R")
193+
}
194+
rsf.opensshSftpServerCmd = exec.Command(opensshSftpServerBinary, sftpServerArgs...)
195+
rsf.opensshSftpServerCmd.Stderr = os.Stderr
196+
var err error
197+
rsf.opensshSftpServerCmd.Stdin, err = rsf.sshCmd.StdoutPipe()
198+
if err != nil {
199+
return err
200+
}
201+
rsf.sshCmd.Stdin, err = rsf.opensshSftpServerCmd.StdoutPipe()
202+
if err != nil {
203+
return err
204+
}
94205
}
95206
logrus.Debugf("executing ssh for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
96207
if err := rsf.sshCmd.Start(); err != nil {
97208
return err
98209
}
99210
logrus.Debugf("starting sftp server for %v", rsf.LocalPath)
100-
go func() {
101-
if srvErr := server.Serve(); srvErr != nil {
102-
if errors.Is(srvErr, io.EOF) {
103-
logrus.WithError(srvErr).Debugf("sftp server for %v exited with EOF (negligible)", rsf.LocalPath)
104-
} else {
105-
logrus.WithError(srvErr).Errorf("sftp server for %v exited", rsf.LocalPath)
211+
switch driver {
212+
case DriverBuiltin:
213+
go func() {
214+
if srvErr := builtinSftpServer.Serve(); srvErr != nil {
215+
if errors.Is(srvErr, io.EOF) {
216+
logrus.WithError(srvErr).Debugf("sftp server for %v exited with EOF (negligible)", rsf.LocalPath)
217+
} else {
218+
logrus.WithError(srvErr).Errorf("sftp server for %v exited", rsf.LocalPath)
219+
}
106220
}
221+
}()
222+
case DriverOpensshSftpServer:
223+
logrus.Debugf("executing OpenSSH SFTP Server: %s %v", rsf.opensshSftpServerCmd.Path, rsf.opensshSftpServerCmd.Args)
224+
if err := rsf.opensshSftpServerCmd.Start(); err != nil {
225+
return err
107226
}
108-
}()
227+
}
228+
logrus.Debugf("waiting for remote ready")
109229
if err := rsf.waitForRemoteReady(); err != nil {
110230
// not a fatal error
111231
logrus.WithError(err).Warnf("failed to confirm whether %v [remote] is successfully mounted", rsf.RemotePath)
@@ -158,9 +278,20 @@ done
158278
}
159279

160280
func (rsf *ReverseSSHFS) Close() error {
161-
logrus.Debugf("killing ssh server for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
162-
if err := rsf.sshCmd.Process.Kill(); err != nil {
163-
return err
281+
logrus.Debugf("killing processes for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
282+
var errors []error
283+
if rsf.sshCmd != nil && rsf.sshCmd.Process != nil {
284+
if err := rsf.sshCmd.Process.Kill(); err != nil {
285+
errors = append(errors, err)
286+
}
287+
}
288+
if rsf.opensshSftpServerCmd != nil && rsf.opensshSftpServerCmd.Process != nil {
289+
if err := rsf.opensshSftpServerCmd.Process.Kill(); err != nil {
290+
errors = append(errors, err)
291+
}
292+
}
293+
if len(errors) > 0 {
294+
return fmt.Errorf("%v", errors)
164295
}
165296
return nil
166297
}

‎pkg/sshocker/sshocker.go

+17-13
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@ import (
1515

1616
type Sshocker struct {
1717
*ssh.SSHConfig
18-
Host string // Required
19-
Port int // Required
20-
Command []string // Optional
21-
Mounts []mount.Mount
22-
LForwards []string
23-
SSHFSAdditionalArgs []string
18+
Host string // Required
19+
Port int // Required
20+
Command []string // Optional
21+
Mounts []mount.Mount
22+
LForwards []string
23+
SSHFSAdditionalArgs []string
24+
Driver reversesshfs.Driver
25+
OpensshSftpServerBinary string
2426
}
2527

2628
func (x *Sshocker) Run() error {
@@ -48,13 +50,15 @@ func (x *Sshocker) Run() error {
4850
switch m.Type {
4951
case mount.MountTypeReverseSSHFS:
5052
rsf := &reversesshfs.ReverseSSHFS{
51-
SSHConfig: x.SSHConfig,
52-
LocalPath: m.Source,
53-
Host: x.Host,
54-
Port: x.Port,
55-
RemotePath: m.Destination,
56-
Readonly: m.Readonly,
57-
SSHFSAdditionalArgs: x.SSHFSAdditionalArgs,
53+
Driver: x.Driver,
54+
OpensshSftpServerBinary: x.OpensshSftpServerBinary,
55+
SSHConfig: x.SSHConfig,
56+
LocalPath: m.Source,
57+
Host: x.Host,
58+
Port: x.Port,
59+
RemotePath: m.Destination,
60+
Readonly: m.Readonly,
61+
SSHFSAdditionalArgs: x.SSHFSAdditionalArgs,
5862
}
5963
if err := rsf.Prepare(); err != nil {
6064
return fmt.Errorf("failed to prepare mounting %q (local) onto %q (remote): %w", rsf.LocalPath, rsf.RemotePath, err)

0 commit comments

Comments
 (0)