FTP vs SFTP Root Folder Differences

[ottodashadow]

We are in the process of migrating users from an existing setup using local storage with vsftpd, and OpenSSH for our client's FTP/SFTP accounts to sftpgo with S3. The user's home folders follow a pattern similar to /home/[username]/files, but then OpenSSH is configured to chroot users to /home/[username]. The root user owns the /home/[username] due to security rules, hence the need for the user-writable files folder. A client who connects via FTP will see all their files under the / root folder with these settings. On the other hand, when a client connects via SFTP, they must include the /files folder to see the same list.

We have been working on an update that would add a virtual /files prefix to all paths under ssh connections, which would apply to all users by adding a FolderPrefix to the sftpd.Configuration struct. The prefix is then removed in the process of resolving the incoming requested file path on the vfs.

For example: sftpd/handler.go

func (c *Connection) Filelist(request *sftp.Request) (sftp.ListerAt, error) {
	c.UpdateLastActivity()

	// inside of the folder prefix bounds
	if c.containsFolderPrefix(request.Filepath) {
		virtualPath := c.removeFolderPrefix(request.Filepath)
		p, err := c.Fs.ResolvePath(virtualPath)
		
		switch request.Method {
		case "List":
			files, err := c.ListDir(p, virtualPath)
			...

Is there another way we can solve the root folder difference between protocols? Would a PR for this feature need to be user-level configurable before it was acceptable?

[drakkan]

Hi,

not sure I fully understand your issue - do you want to switch to SFTPGo and still use OpenSSH and vsftpd at the same time? If you want to use SFTPGo only you can just set a home directory and it will be the same for both SFTP and FTP and then the directory listing will be the same. SFTPGo uses virtual permissions, unrelated to filesystem permissions, so you can have a read-only directory even if it's writable at the filesystem level. Virtual permissions can be set per-directory and not only globally at the account level

You can also change the home dir dinamically using the pre-login hook. This could be racy for your use case with the current code.

As for a PR for this function, if I understand the purpose correctly, I think it is not enough to simply hide a directory from the list, a user could write a full download / upload path so you should update all other methods as well and probably also the method of scanning the quotas. But I should look to the PR to understand better. Having this feature for all users doesn't even seem ideal.

Please let me better understand, thank you

[ottodashadow]

We want to use just SFTPGo in place of vsftpd and OpenSSH. OpenSSH will remain on the server for SSH access to maintain the server, but SFTP connections will go to SFTPGo. I have created a gist Dockerfile to demonstrate how our current setup functions.

If you want to use SFTPGo only you can just set a home directory and it will be the same for both SFTP and FTP and then the directory listing will be the same.

Typically this would be the ideal setup. However, unlike my gist example with two users, we have thousands of users with anywhere from a couple to a dozen integrations per user. If we migrate a user's files from /home/client1/files to the S3 key prefix users/client1, existing SFTP integrations will return not found errors because existing integrations would include a /files prefix. If we migrate /home/client1 to users/client, then existing FTP integrations result in not found errors, because their filenames would not include the /files prefix.

Integration Examples

Protocol: FTP
Port: 21
Username: client1
Filename: data1.txt

Protocol: SFTP
Port: 22
Username: client1
Filename: files/data1.txt

[drakkan]

Ok, so you basically want to migrate from a local fs solution to S3 and at the same time keep compatibility with the current clients/integrations expecting a different path for FTP and SFTP.

I think changing the list directory result is not the best approach, it is probably better to change the S3 KeyPrefix according to the protocol. This way the path change applies to all SFTP/FTP methods and not only to the directory listing.

Without changing the SFTPGo code this could be done using the pre-login hook, for example something like this should work (error checking is omitted):

#!/usr/bin/env python

import json
import os
import sys

protocol = os.environ.get("SFTPGO_LOGIND_PROTOCOL", "")
user = json.loads(os.environ.get("SFTPGO_LOGIND_USER", ""))

if protocol == "SSH":
	user["filesystem"]["s3config"]["key_prefix"] = "users/" + user["username"] + "/"
elif protocol == "FTP":
	user["filesystem"]["s3config"]["key_prefix"] = "users/" + user["username"] + "/files/"
else:
	# webdav
	sys.exit(0)

print(json.dumps(user))
sys.exit(0)

If you care about performance an HTTP hook will avoid the overhead to start an external program each time a user tries to login.

Your use case is very specific and an external hook is the best option in my opinion (so we avoid to add to SFTPGo very specific code), however if you want to prepare a PR I'll try to evaluate it.

The PR should allow to override the key prefix per-protocol adding a new configuration key to S3FsConfig something like this (maybe with better names :)):

KeyPrefixOverrides []keyPrefixOverride `json:"key_prefix_overrides,omitempty"`

where keyPrefixOverride is something like this:

type keyPrefixOverride struct {
	KeyPrefix string `json:"key_prefix,omitempty"`
	Protocol  string `json:"protocol,omitempty"`
}

the PR should add this support to the REST API, web admin and to the REST API cli, thank you.

P.S. please note that S3 backend has some limitations compared to the local fs

[ottodashadow]

We are hesitant to use the pre-login hook solution. We know from logs that we have users who access their accounts from both FTP and SFTP, potentially in quick succession. If a user's S3 key-prefix changed after an SFTP login, and then while the SFTP connection was still open, a new FTP login changed the prefix again, what would happen? Would the SFTP connection continue to use settings established at login, or would it fall back to the current database value? Is this the potential scenario you were thinking about when you said the pre-login hook could be racy?

My idea for the root folder prefix followed the thought process of the virtual folders, instead of hiding an existing folder. If the root folder prefix under an SFTP connection was /files and the user requested the listing of the / directory, a virtual files directory os.FileInfo interface would be created. Then for all other SSH commands the prefix would be removed. For reference, this is what I have.

If was going to submit a PR for the S3FsConfig with your keyPrefixOverride is there any reason why we wouldn't just go with

type S3FsConfig struct {
	KeyPrefixOverrides keyPrefixOverrides `json:"key_prefix_overrides,omitempty"`
}

type keyPrefixOverrides struct {
	SSH    string `json:"ssh,omitempty"`
	WebDAV string `json:"webdav,omitempty"`
	FTP    string `json:"ftp,omitempty"`
}

I cannot think of any reason why we'd have multiple overrides for the same protocol.

[drakkan]

We are hesitant to use the pre-login hook solution. We know from logs that we have users who access their accounts from both FTP and SFTP, potentially in quick succession. If a user's S3 key-prefix changed after an SFTP login, and then while the SFTP connection was still open, a new FTP login changed the prefix again, what would happen? Would the SFTP connection continue to use settings established at login, or would it fall back to the current database value? Is this the potential scenario you were thinking about when you said the pre-login hook could be racy?

each connection will continue to use the settings established at login, the previous code, for the pre-login hook, was this one:

        if userID == 0 {
		err = provider.addUser(u)
	} else {
		err = provider.updateUser(u)
	}
	if err != nil {
		return u, err
	}
	providerLog(logger.LevelDebug, "user %#v added/updated from pre-login hook response, id: %v", username, userID)
	return provider.userExists(username)

so if another provider.updateUser(u) happened before return provider.userExists(username) we could return the wrong prefix, basically the same user would have to log in via FTP and SFTP at the same millisecond to have this race condition, however it should be fixed now

My idea for the root folder prefix followed the thought process of the virtual folders, instead of hiding an existing folder. If the root folder prefix under an SFTP connection was /files and the user requested the listing of the / directory, a virtual files directory os.FileInfo interface would be created. Then for all other SSH commands the prefix would be removed. For reference, this is what I have.

I did a quick test using your branch and setting "folder_prefix": "/files". I haven't looked at the code but the results are quite unexpected, please take a look here:

sftp -P 2022 [email protected]
[email protected]'s password: 
Connected to 127.0.0.1.
sftp> ls
files  
sftp> put /tmp/file.txt file_root.txt
Uploading /tmp/file.txt to /file_root.txt
remote open("/file_root.txt"): No such file or directory
sftp> put /tmp/file.txt files/file.txt
Uploading /tmp/file.txt to /files/file.txt
/tmp/file.txt                                                                    100% 4034     3.5MB/s   00:00    
sftp> mkdir files1
sftp> put /tmp/file.txt files1/file1.txt
Uploading /tmp/file.txt to /files1/file1.txt
/tmp/file.txt                                                                    100% 4034     4.9MB/s   00:00    
sftp> mkdir aaaa
Couldn't create directory: No such file or directory
sftp> cd files1
sftp> ls
file1.txt  
sftp> rename file1.txt file.txt
sftp> ls
file.txt  
sftp> pwd
Remote working directory: /files1
sftp> cd ..
Couldn't stat remote file: Operation unsupported
sftp> cd /files
sftp> ls
1         file.txt  
sftp> cd ..
Couldn't stat remote file: Operation unsupported
sftp> ls -la
drwxr-xr-x    2 1000     1000           60 Jan  5 10:22 1
-rw-r--r--    1 1000     1000         4034 Jan  5 10:21 file.txt
sftp> cd 1
sftp> ls
file.txt  
sftp> cd ..
sftp> ls
1         file.txt  
sftp> quit

there is an evident bug with prefix handling, but even after fixing it it is quite questionable that a user cannot write to the / directory or that cd .. should not work

If was going to submit a PR for the S3FsConfig with your keyPrefixOverride is there any reason why we wouldn't just go with

type S3FsConfig struct {
	KeyPrefixOverrides keyPrefixOverrides `json:"key_prefix_overrides,omitempty"`
}

type keyPrefixOverrides struct {
	SSH    string `json:"ssh,omitempty"`
	WebDAV string `json:"webdav,omitempty"`
	FTP    string `json:"ftp,omitempty"`
}

I cannot think of any reason why we'd have multiple overrides for the same protocol.

yes, this would be better.

I need some time to think about this contribution, sub-dir permissions, extensions filters, patterns filters and in general every thing that is based on a virtual path will be involved and will need to be changed here: /sub will be a different path for SFTP and FTP after this change. Basically we should dynamically change all these user settings in the same way as you should within the pre-login hook and carefully check that it is enough and it is ok for all uses cases also the ones not needed for your usage.

I'm not aware of any other similar software with this feature, do you know a commercial sw that implements it? If so, and if it supports a feature set similar to SFTPGo, we could take inspiration. Thank you