sftpd: minor improvements and docs for the prefix middleware

docs/full-configuration.md

@@ -106,6 +106,7 @@ The configuration file contains the following sections:
   - `enabled_ssh_commands`, list of enabled SSH commands. `*` enables all supported commands. More information can be found [here](./ssh-commands.md).
   - `keyboard_interactive_auth_hook`, string. Absolute path to an external program or an HTTP URL to invoke for keyboard interactive authentication. See [Keyboard Interactive Authentication](./keyboard-interactive.md) for more details.
   - `password_authentication`, boolean. Set to false to disable password authentication. This setting will disable multi-step authentication method using public key + password too. It is useful for public key only configurations if you need to manage old clients that will not attempt to authenticate with public keys if the password login method is advertised. Default: true.
+  - `folder_prefix`, string. Virtual root folder prefix to include in all file operations (ex: `/files`). The virtual paths used for per-directory permissions, file patterns etc. must not include the folder prefix. The prefix is only applied to SFTP requests, SCP and other SSH commands will be automatically disabled if you configure a prefix. This setting can help some specific migrations from SFTP servers based on OpenSSH and it is not recommended for general usage. Default: empty.
 - **"ftpd"**, the configuration for the FTP server
   - `bindings`, list of structs. Each struct has the following fields:
     - `port`, integer. The port used for serving FTP requests. 0 means disabled. Default: 0.

sftpd/internal_test.go

@@ -2061,3 +2061,26 @@ func newFakeListener(err error) net.Listener {
 		err:    err,
 	}
 }
+
+func TestFolderPrefix(t *testing.T) {
+	c := Configuration{
+		FolderPrefix: "files",
+	}
+	c.checkFolderPrefix()
+	assert.Equal(t, "/files", c.FolderPrefix)
+	c.FolderPrefix = ""
+	c.checkFolderPrefix()
+	assert.Empty(t, c.FolderPrefix)
+	c.FolderPrefix = "/"
+	c.checkFolderPrefix()
+	assert.Empty(t, c.FolderPrefix)
+	c.FolderPrefix = "/."
+	c.checkFolderPrefix()
+	assert.Empty(t, c.FolderPrefix)
+	c.FolderPrefix = "."
+	c.checkFolderPrefix()
+	assert.Empty(t, c.FolderPrefix)
+	c.FolderPrefix = ".."
+	c.checkFolderPrefix()
+	assert.Empty(t, c.FolderPrefix)
+}

sftpd/middleware.go

@@ -4,7 +4,6 @@ import (
 	"io"
 	"os"
 	"path"
-	"path/filepath"
 	"strings"
 	"time"
 
@@ -13,7 +12,7 @@ import (
 	"github.com/drakkan/sftpgo/v2/vfs"
 )
 
-// Middleware defines the interface for sftp middlewares
+// Middleware defines the interface for SFTP middlewares
 type Middleware interface {
 	sftp.FileReader
 	sftp.FileWriter
@@ -77,17 +76,15 @@ func (p *prefixMiddleware) Filelist(request *sftp.Request) (sftp.ListerAt, error
 		request.Filepath, _ = p.removeFolderPrefix(request.Filepath)
 		return p.next.Filelist(request)
 	case pathIsPrefixParent:
-		Now := time.Now()
 		switch request.Method {
 		case methodList:
-			FileName := p.nextListFolder(request.Filepath)
+			fileName := p.nextListFolder(request.Filepath)
 			return listerAt([]os.FileInfo{
-				// vfs.NewFileInfo(`.`, true, 0, Now, false),
-				vfs.NewFileInfo(FileName, true, 0, Now, false),
+				vfs.NewFileInfo(fileName, true, 0, time.Now(), false),
 			}), nil
 		case methodStat:
 			return listerAt([]os.FileInfo{
-				vfs.NewFileInfo(request.Filepath, true, 0, Now, false),
+				vfs.NewFileInfo(request.Filepath, true, 0, time.Now(), false),
 			}), nil
 		default:
 			return nil, sftp.ErrSSHFxOpUnsupported
@@ -153,16 +150,16 @@ func (p *prefixMiddleware) StatVFS(request *sftp.Request) (*sftp.StatVFS, error)
 }
 
 func (p *prefixMiddleware) nextListFolder(requestPath string) string {
-	cleanPath := filepath.Clean(`/` + requestPath)
-	cleanPrefix := filepath.Clean(`/` + p.prefix)
+	cleanPath := path.Clean(`/` + requestPath)
+	cleanPrefix := path.Clean(`/` + p.prefix)
 
-	FileName := cleanPrefix[len(cleanPath):]
-	FileName = strings.TrimLeft(FileName, `/`)
-	SlashIndex := strings.Index(FileName, `/`)
-	if SlashIndex > 0 {
-		return FileName[0:SlashIndex]
+	fileName := cleanPrefix[len(cleanPath):]
+	fileName = strings.TrimLeft(fileName, `/`)
+	slashIndex := strings.Index(fileName, `/`)
+	if slashIndex > 0 {
+		return fileName[0:slashIndex]
 	}
-	return FileName
+	return fileName
 }
 
 func (p *prefixMiddleware) containsPrefix(virtualPath string) bool {
@@ -184,7 +181,7 @@ func (p *prefixMiddleware) removeFolderPrefix(virtualPath string) (string, bool)
 		return virtualPath, true
 	}
 
-	virtualPath = filepath.Clean(`/` + virtualPath)
+	virtualPath = path.Clean(`/` + virtualPath)
 	if p.containsPrefix(virtualPath) {
 		effectivePath := virtualPath[len(p.prefix):]
 		if effectivePath == `` {
@@ -195,9 +192,9 @@ func (p *prefixMiddleware) removeFolderPrefix(virtualPath string) (string, bool)
 	return virtualPath, false
 }
 
-func getPrefixHierarchy(prefix, path string) prefixMatch {
-	prefixSplit := strings.Split(filepath.Clean(`/`+prefix), `/`)
-	pathSplit := strings.Split(filepath.Clean(`/`+path), `/`)
+func getPrefixHierarchy(prefix, virtualPath string) prefixMatch {
+	prefixSplit := strings.Split(path.Clean(`/`+prefix), `/`)
+	pathSplit := strings.Split(path.Clean(`/`+virtualPath), `/`)
 
 	for {
 		// stop if either slice is empty of the current head elements do not match

sftpd/middleware_test.go

@@ -109,6 +109,31 @@ func (Suite *PrefixMiddlewareSuite) TestOpenFile() {
 	}
 }
 
+func (Suite *PrefixMiddlewareSuite) TestStatVFS() {
+	prefix := prefixMiddleware{prefix: `/files`}
+
+	// parent of prefix
+	res, err := prefix.StatVFS(&sftp.Request{Filepath: `/`})
+	Suite.Nil(res)
+	Suite.Equal(sftp.ErrSSHFxPermissionDenied, err)
+
+	// file path and prefix are unrelated
+	res, err = prefix.StatVFS(&sftp.Request{Filepath: `/random`})
+	Suite.Nil(res)
+	Suite.Equal(sftp.ErrSSHFxPermissionDenied, err)
+
+	// file path is sub path of configured prefix
+	// mocked returns are not import, just the call to the next file writer
+	statVFSMock := mocks.NewMockMiddleware(Suite.MockCtl)
+	statVFSMock.EXPECT().
+		StatVFS(&sftp.Request{Filepath: `/data`}).
+		Return(nil, nil)
+	prefix.next = statVFSMock
+	res, err = prefix.StatVFS(&sftp.Request{Filepath: `/files/data`})
+	Suite.Nil(err)
+	Suite.Nil(res)
+}
+
 func (Suite *PrefixMiddlewareSuite) TestFileListForwarding() {
 	var tests = []struct {
 		Method   string

sftpd/server.go

@@ -119,7 +119,11 @@ type Configuration struct {
 	KeyboardInteractiveHook string `json:"keyboard_interactive_auth_hook" mapstructure:"keyboard_interactive_auth_hook"`
 	// PasswordAuthentication specifies whether password authentication is allowed.
 	PasswordAuthentication bool `json:"password_authentication" mapstructure:"password_authentication"`
-	// Virtual root folder prefix to include in all file operations (ex: /files)
+	// Virtual root folder prefix to include in all file operations (ex: /files).
+	// The virtual paths used for per-directory permissions, file patterns etc. must not include the folder prefix.
+	// The prefix is only applied to SFTP requests, SCP and other SSH commands will be automatically disabled if
+	// you configure a prefix.
+	// This setting can help some migrations from OpenSSH. It is not recommended for general usage.
 	FolderPrefix     string `json:"folder_prefix" mapstructure:"folder_prefix"`
 	certChecker      *ssh.CertChecker
 	parsedUserCAKeys []ssh.PublicKey
@@ -479,27 +483,8 @@ func (c *Configuration) handleSftpConnection(channel ssh.Channel, connection *Co
 	common.Connections.Add(connection)
 	defer common.Connections.Remove(connection.GetID())
 
-	var handlers sftp.Handlers
-
-	if c.FolderPrefix != "" {
-		prefixMiddleware := newPrefixMiddleware(c.FolderPrefix, connection)
-		handlers = sftp.Handlers{
-			FileGet:  prefixMiddleware,
-			FilePut:  prefixMiddleware,
-			FileCmd:  prefixMiddleware,
-			FileList: prefixMiddleware,
-		}
-	} else {
-		handlers = sftp.Handlers{
-			FileGet:  connection,
-			FilePut:  connection,
-			FileCmd:  connection,
-			FileList: connection,
-		}
-	}
-
 	// Create the server instance for the channel using the handler we created above.
-	server := sftp.NewRequestServer(channel, handlers, sftp.WithRSAllocator())
+	server := sftp.NewRequestServer(channel, c.createHandlers(connection), sftp.WithRSAllocator())
 
 	defer server.Close()
 	if err := server.Serve(); err == io.EOF {
@@ -512,6 +497,26 @@ func (c *Configuration) handleSftpConnection(channel ssh.Channel, connection *Co
 	}
 }
 
+func (c *Configuration) createHandlers(connection *Connection) sftp.Handlers {
+	if c.FolderPrefix != "" {
+		prefixMiddleware := newPrefixMiddleware(c.FolderPrefix, connection)
+
+		return sftp.Handlers{
+			FileGet:  prefixMiddleware,
+			FilePut:  prefixMiddleware,
+			FileCmd:  prefixMiddleware,
+			FileList: prefixMiddleware,
+		}
+	}
+
+	return sftp.Handlers{
+		FileGet:  connection,
+		FilePut:  connection,
+		FileCmd:  connection,
+		FileList: connection,
+	}
+}
+
 func checkAuthError(ip string, err error) {
 	if authErrors, ok := err.(*ssh.ServerAuthError); ok {
 		// check public key auth errors here
@@ -604,7 +609,13 @@ func (c *Configuration) checkSSHCommands() {
 func (c *Configuration) checkFolderPrefix() {
 	if c.FolderPrefix != "" {
 		c.FolderPrefix = path.Join("/", c.FolderPrefix)
-		logger.Debug(logSender, "", "folder prefix %#v configured", c.FolderPrefix)
+		if c.FolderPrefix == "/" {
+			c.FolderPrefix = ""
+		}
+	}
+	if c.FolderPrefix != "" {
+		c.EnabledSSHCommands = nil
+		logger.Debug(logSender, "", "folder prefix %#v configured, SSH commands are disabled", c.FolderPrefix)
 	}
 }
 

sftpd/sftpd_test.go

@@ -277,6 +277,26 @@ func TestMain(m *testing.M) {
 	waitTCPListening(sftpdConf.Bindings[0].GetAddress())
 	getHostKeysFingerprints(sftpdConf.HostKeys)
 
+	prefixedConf := sftpdConf
+	prefixedConf.Bindings = []sftpd.Binding{
+		{
+			Port:             2226,
+			ApplyProxyConfig: false,
+		},
+	}
+	prefixedConf.PasswordAuthentication = true
+	prefixedConf.FolderPrefix = "/prefix/files"
+	go func() {
+		logger.Debug(logSender, "", "initializing SFTP server with config %+v and proxy protocol %v",
+			prefixedConf, common.Config.ProxyProtocol)
+		if err := prefixedConf.Initialize(configDir); err != nil {
+			logger.ErrorToConsole("could not start SFTP server with proxy protocol 2: %v", err)
+			os.Exit(1)
+		}
+	}()
+
+	waitTCPListening(prefixedConf.Bindings[0].GetAddress())
+
 	exitCode := m.Run()
 	os.Remove(logFilePath)
 	os.Remove(loginBannerFile)
@@ -481,6 +501,56 @@ func TestBasicSFTPFsHandling(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestFolderPrefix(t *testing.T) {
+	usePubKey := true
+	u := getTestUser(usePubKey)
+	u.QuotaFiles = 1000
+	user, _, err := httpdtest.AddUser(u, http.StatusCreated)
+	assert.NoError(t, err)
+	err = os.RemoveAll(user.GetHomeDir())
+	assert.NoError(t, err)
+	conn, client, err := getSftpClientWithAddr(user, usePubKey, "127.0.0.1:2226")
+	if assert.NoError(t, err) {
+		defer conn.Close()
+		defer client.Close()
+		err = checkBasicSFTP(client)
+		assert.NoError(t, err)
+		_, err = client.Stat("path")
+		assert.ErrorIs(t, err, os.ErrPermission)
+		_, err = client.Stat("/prefix/path")
+		assert.ErrorIs(t, err, os.ErrPermission)
+		_, err = client.Stat("/prefix/files1")
+		assert.ErrorIs(t, err, os.ErrPermission)
+		contents, err := client.ReadDir("/")
+		if assert.NoError(t, err) {
+			if assert.Len(t, contents, 1) {
+				assert.Equal(t, "prefix", contents[0].Name())
+			}
+		}
+		contents, err = client.ReadDir("/prefix")
+		if assert.NoError(t, err) {
+			if assert.Len(t, contents, 1) {
+				assert.Equal(t, "files", contents[0].Name())
+			}
+		}
+		_, err = client.OpenFile(testFileName, os.O_WRONLY)
+		assert.ErrorIs(t, err, os.ErrPermission)
+		_, err = client.OpenFile(testFileName, os.O_RDONLY)
+		assert.ErrorIs(t, err, os.ErrPermission)
+
+		f, err := client.OpenFile(path.Join("prefix", "files", testFileName), os.O_WRONLY)
+		assert.NoError(t, err)
+		_, err = f.Write([]byte("test"))
+		assert.NoError(t, err)
+		err = f.Close()
+		assert.NoError(t, err)
+	}
+	_, err = httpdtest.RemoveUser(user, http.StatusOK)
+	assert.NoError(t, err)
+	err = os.RemoveAll(user.GetHomeDir())
+	assert.NoError(t, err)
+}
+
 func TestLoginNonExistentUser(t *testing.T) {
 	usePubKey := true
 	user := getTestUser(usePubKey)