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]>
lima-vm · Apr 15, 2022 · 2c16264 · 2c16264
1 parent 77504d5
commit 2c16264
Show file tree

Hide file tree

Showing 4 changed files with 213 additions and 57 deletions.
diff --git a/README.md b/README.md
@@ -57,6 +57,15 @@ SSH flags:
 * `-F`, `--ssh-config=FILE`: specify SSH config file used for `ssh -F`
 * `--ssh-persist=(true|false)` (default: `true`): enable ControlPersist
 
+SSHFS flags:
+* `--sshfs-noempty` (default: `false`): enable sshfs nonempty
+
+SFTP server flags:
+* `--driver=DRIVER` (default: `auto`): SFTP server driver. `builtin` (legacy) or `openssh-sftp-server` (robust and secure, recommended).
+   `openssh-sftp-server` is chosen by default when the OpenSSH SFTP Server binary is detected.
+* `--openssh-sftp-server=BINARY`: OpenSSH SFTP Server binary.
+   Automatically detected when installed in well-known locations such as `/usr/libexec/sftp-server`.
+
 ### Subcommand: `help`
 Shows help
 
diff --git a/cmd/sshocker/run.go b/cmd/sshocker/run.go
@@ -42,6 +42,16 @@ var (
 			Usage: "enable sshfs nonempty",
 			Value: false,
 		},
+		&cli.StringFlag{
+			Name:  "driver",
+			Usage: "SFTP server driver. \"builtin\" (legacy) or \"openssh-sftp-server\" (robust and secure, recommended), automatically chosen by default",
+			Value: "auto",
+		},
+		&cli.StringFlag{
+			Name:  "openssh-sftp-server",
+			Usage: "OpenSSH SFTP Server binary, automatically chosen by default",
+			Value: "",
+		},
 	}
 	runCommand = &cli.Command{
 		Name:   "run",
@@ -85,11 +95,13 @@ func runAction(clicontext *cli.Context) error {
 		sshfsAdditionalArgs = append(sshfsAdditionalArgs, "-o", "nonempty")
 	}
 	x := &sshocker.Sshocker{
-		SSHConfig:           sshConfig,
-		Host:                host,
-		Port:                port,
-		Command:             clicontext.Args().Tail(),
-		SSHFSAdditionalArgs: sshfsAdditionalArgs,
+		SSHConfig:               sshConfig,
+		Host:                    host,
+		Port:                    port,
+		Command:                 clicontext.Args().Tail(),
+		SSHFSAdditionalArgs:     sshfsAdditionalArgs,
+		Driver:                  clicontext.String("driver"),
+		OpensshSftpServerBinary: clicontext.String("openssh-sftp-server"),
 	}
 	if len(x.Command) > 0 && x.Command[0] == "--" {
 		x.Command = x.Command[1:]

diff --git a/pkg/reversesshfs/reversesshfs.go b/pkg/reversesshfs/reversesshfs.go
@@ -10,22 +10,34 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/lima-vm/sshocker/pkg/ssh"
 	"github.com/lima-vm/sshocker/pkg/util"
 	"github.com/pkg/sftp"
 	"github.com/sirupsen/logrus"
 )
 
+type Driver = string
+
+const (
+	DriverAuto              = Driver("auto")                // Default
+	DriverBuiltin           = Driver("builtin")             // Legacy. Unrecommended.
+	DriverOpensshSftpServer = Driver("openssh-sftp-server") // More robust and secure. Recommended.
+)
+
 type ReverseSSHFS struct {
 	*ssh.SSHConfig
-	LocalPath           string
-	Host                string
-	Port                int
-	RemotePath          string
-	Readonly            bool
-	sshCmd              *exec.Cmd
-	SSHFSAdditionalArgs []string
+	Driver                  Driver
+	OpensshSftpServerBinary string // used only when Driver == DriverOpensshSftpServer
+	LocalPath               string
+	Host                    string
+	Port                    int
+	RemotePath              string
+	Readonly                bool
+	sshCmd                  *exec.Cmd
+	opensshSftpServerCmd    *exec.Cmd
+	SSHFSAdditionalArgs     []string
 }
 
 func (rsf *ReverseSSHFS) Prepare() error {
@@ -48,6 +60,55 @@ func (rsf *ReverseSSHFS) Prepare() error {
 	return nil
 }
 
+func DetectOpensshSftpServerBinary() string {
+	homebrewSSHD := []string{
+		"/usr/local/sbin/sshd",
+		"/opt/homebrew/sbin/sshd",
+	}
+	for _, f := range homebrewSSHD {
+		// sshd is like "/usr/local/Cellar/openssh/8.9p1/sbin/sshd"
+		sshd, err := filepath.EvalSymlinks(f)
+		if err != nil {
+			continue
+		}
+		// local is like "/usr/local/Cellar/openssh"
+		local := filepath.Dir(filepath.Dir(sshd))
+		// sftpServer is like "/usr/local/Cellar/openssh/8.9p1/libexec/sftp-server"
+		sftpServer := filepath.Join(local, "libexec", "sftp-server")
+		if exe, err := exec.LookPath(sftpServer); err == nil {
+			return exe
+		}
+	}
+	candidates := []string{
+		"/usr/libexec/sftp-server",         // macOS, OpenWrt
+		"/usr/libexec/openssh/sftp-server", // Fedora
+		"/usr/lib/sftp-server",             // Debian (symlink to openssh/sftp-server)
+		"/usr/lib/openssh/sftp-server",     // Debian
+		"/usr/lib/ssh/sftp-server",         // Alpine
+	}
+	for _, cand := range candidates {
+		if exe, err := exec.LookPath(cand); err == nil {
+			return exe
+		}
+	}
+	return ""
+}
+
+func DetectDriver(explicitOpensshSftpServerBinary string) (Driver, string, error) {
+	if explicitOpensshSftpServerBinary != "" {
+		exe, err := exec.LookPath(explicitOpensshSftpServerBinary)
+		if err != nil {
+			return "", "", err
+		}
+		return DriverOpensshSftpServer, exe, nil
+	}
+	exe := DetectOpensshSftpServerBinary()
+	if exe != "" {
+		return DriverOpensshSftpServer, exe, nil
+	}
+	return DriverBuiltin, "", nil
+}
+
 func (rsf *ReverseSSHFS) Start() error {
 	sshBinary := rsf.SSHConfig.Binary()
 	sshArgs := rsf.SSHConfig.Args()
@@ -68,44 +129,103 @@ func (rsf *ReverseSSHFS) Start() error {
 	sshArgs = append(sshArgs, rsf.SSHFSAdditionalArgs...)
 	rsf.sshCmd = exec.Command(sshBinary, sshArgs...)
 	rsf.sshCmd.Stderr = os.Stderr
-	stdinPipe, err := rsf.sshCmd.StdinPipe()
-	if err != nil {
-		return err
-	}
-	stdoutPipe, err := rsf.sshCmd.StdoutPipe()
-	if err != nil {
-		return err
-	}
-	stdio := &util.RWC{
-		ReadCloser:  stdoutPipe,
-		WriteCloser: stdinPipe,
-	}
-	var sftpOpts []sftp.ServerOption
-	if rsf.Readonly {
-		sftpOpts = append(sftpOpts, sftp.ReadOnly())
+	driver := rsf.Driver
+	opensshSftpServerBinary := rsf.OpensshSftpServerBinary
+	switch driver {
+	case DriverBuiltin, DriverOpensshSftpServer:
+		// NOP
+	case "", DriverAuto:
+		var err error
+		driver, opensshSftpServerBinary, err = DetectDriver(opensshSftpServerBinary)
+		if err != nil {
+			return fmt.Errorf("failed to choose driver automatically: %w", err)
+		}
+		logrus.Debugf("Chosen driver %q", driver)
+	default:
+		return fmt.Errorf("unknown driver %q", driver)
 	}
-	// NOTE: sftp.NewServer doesn't support specifying the root.
-	// https://github.com/pkg/sftp/pull/238
-	//
-	// TODO: use sftp.NewRequestServer with custom handlers to mitigate potential vulnerabilities.
-	server, err := sftp.NewServer(stdio, sftpOpts...)
-	if err != nil {
-		return err
+	var builtinSftpServer *sftp.Server
+	switch driver {
+	case DriverBuiltin:
+		stdinPipe, err := rsf.sshCmd.StdinPipe()
+		if err != nil {
+			return err
+		}
+		stdoutPipe, err := rsf.sshCmd.StdoutPipe()
+		if err != nil {
+			return err
+		}
+		stdio := &util.RWC{
+			ReadCloser:  stdoutPipe,
+			WriteCloser: stdinPipe,
+		}
+		var sftpOpts []sftp.ServerOption
+		if rsf.Readonly {
+			sftpOpts = append(sftpOpts, sftp.ReadOnly())
+		}
+		// NOTE: sftp.NewServer doesn't support specifying the root.
+		// https://github.com/pkg/sftp/pull/238
+		//
+		// TODO: use sftp.NewRequestServer with custom handlers to mitigate potential vulnerabilities.
+		builtinSftpServer, err = sftp.NewServer(stdio, sftpOpts...)
+		if err != nil {
+			return err
+		}
+	case DriverOpensshSftpServer:
+		if opensshSftpServerBinary == "" {
+			opensshSftpServerBinary = DetectOpensshSftpServerBinary()
+			if opensshSftpServerBinary == "" {
+				return errors.New("no openssh sftp-server found")
+			}
+		}
+		logrus.Debugf("Using OpenSSH SFTP Server %q", opensshSftpServerBinary)
+		sftpServerArgs := []string{
+			// `-e` available since OpenSSH 5.4p1 (2010) https://github.com/openssh/openssh-portable/commit/7bee06ab
+			"-e",
+			// `-d` available since OpenSSH 6.2p1 (2013) https://github.com/openssh/openssh-portable/commit/502ab0ef
+			// NOTE: `-d` just chdirs the sftp server process to the specified directory.
+			// This is expected to be used in conjunction with chroot (in future), however, macOS does not support unprivileged chroot.
+			"-d", strings.ReplaceAll(rsf.LocalPath, "%", "%%"),
+		}
+		if rsf.Readonly {
+			// `-R` available since OpenSSH 5.4p1 (2010) https://github.com/openssh/openssh-portable/commit/db7bf825
+			sftpServerArgs = append(sftpServerArgs, "-R")
+		}
+		rsf.opensshSftpServerCmd = exec.Command(opensshSftpServerBinary, sftpServerArgs...)
+		rsf.opensshSftpServerCmd.Stderr = os.Stderr
+		var err error
+		rsf.opensshSftpServerCmd.Stdin, err = rsf.sshCmd.StdoutPipe()
+		if err != nil {
+			return err
+		}
+		rsf.sshCmd.Stdin, err = rsf.opensshSftpServerCmd.StdoutPipe()
+		if err != nil {
+			return err
+		}
 	}
 	logrus.Debugf("executing ssh for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
 	if err := rsf.sshCmd.Start(); err != nil {
 		return err
 	}
 	logrus.Debugf("starting sftp server for %v", rsf.LocalPath)
-	go func() {
-		if srvErr := server.Serve(); srvErr != nil {
-			if errors.Is(srvErr, io.EOF) {
-				logrus.WithError(srvErr).Debugf("sftp server for %v exited with EOF (negligible)", rsf.LocalPath)
-			} else {
-				logrus.WithError(srvErr).Errorf("sftp server for %v exited", rsf.LocalPath)
+	switch driver {
+	case DriverBuiltin:
+		go func() {
+			if srvErr := builtinSftpServer.Serve(); srvErr != nil {
+				if errors.Is(srvErr, io.EOF) {
+					logrus.WithError(srvErr).Debugf("sftp server for %v exited with EOF (negligible)", rsf.LocalPath)
+				} else {
+					logrus.WithError(srvErr).Errorf("sftp server for %v exited", rsf.LocalPath)
+				}
 			}
+		}()
+	case DriverOpensshSftpServer:
+		logrus.Debugf("executing OpenSSH SFTP Server: %s %v", rsf.opensshSftpServerCmd.Path, rsf.opensshSftpServerCmd.Args)
+		if err := rsf.opensshSftpServerCmd.Start(); err != nil {
+			return err
 		}
-	}()
+	}
+	logrus.Debugf("waiting for remote ready")
 	if err := rsf.waitForRemoteReady(); err != nil {
 		// not a fatal error
 		logrus.WithError(err).Warnf("failed to confirm whether %v [remote] is successfully mounted", rsf.RemotePath)
@@ -158,9 +278,20 @@ done
 }
 
 func (rsf *ReverseSSHFS) Close() error {
-	logrus.Debugf("killing ssh server for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
-	if err := rsf.sshCmd.Process.Kill(); err != nil {
-		return err
+	logrus.Debugf("killing processes for remote sshfs: %s %v", rsf.sshCmd.Path, rsf.sshCmd.Args)
+	var errors []error
+	if rsf.sshCmd != nil && rsf.sshCmd.Process != nil {
+		if err := rsf.sshCmd.Process.Kill(); err != nil {
+			errors = append(errors, err)
+		}
+	}
+	if rsf.opensshSftpServerCmd != nil && rsf.opensshSftpServerCmd.Process != nil {
+		if err := rsf.opensshSftpServerCmd.Process.Kill(); err != nil {
+			errors = append(errors, err)
+		}
+	}
+	if len(errors) > 0 {
+		return fmt.Errorf("%v", errors)
 	}
 	return nil
 }
diff --git a/pkg/sshocker/sshocker.go b/pkg/sshocker/sshocker.go
@@ -15,12 +15,14 @@ import (
 
 type Sshocker struct {
 	*ssh.SSHConfig
-	Host                string   // Required
-	Port                int      // Required
-	Command             []string // Optional
-	Mounts              []mount.Mount
-	LForwards           []string
-	SSHFSAdditionalArgs []string
+	Host                    string   // Required
+	Port                    int      // Required
+	Command                 []string // Optional
+	Mounts                  []mount.Mount
+	LForwards               []string
+	SSHFSAdditionalArgs     []string
+	Driver                  reversesshfs.Driver
+	OpensshSftpServerBinary string
 }
 
 func (x *Sshocker) Run() error {
@@ -48,13 +50,15 @@ func (x *Sshocker) Run() error {
 		switch m.Type {
 		case mount.MountTypeReverseSSHFS:
 			rsf := &reversesshfs.ReverseSSHFS{
-				SSHConfig:           x.SSHConfig,
-				LocalPath:           m.Source,
-				Host:                x.Host,
-				Port:                x.Port,
-				RemotePath:          m.Destination,
-				Readonly:            m.Readonly,
-				SSHFSAdditionalArgs: x.SSHFSAdditionalArgs,
+				Driver:                  x.Driver,
+				OpensshSftpServerBinary: x.OpensshSftpServerBinary,
+				SSHConfig:               x.SSHConfig,
+				LocalPath:               m.Source,
+				Host:                    x.Host,
+				Port:                    x.Port,
+				RemotePath:              m.Destination,
+				Readonly:                m.Readonly,
+				SSHFSAdditionalArgs:     x.SSHFSAdditionalArgs,
 			}
 			if err := rsf.Prepare(); err != nil {
 				return fmt.Errorf("failed to prepare mounting %q (local) onto %q (remote): %w", rsf.LocalPath, rsf.RemotePath, err)