From df96ea7e9f19877f1644e991781efeec7aba8a99 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Tue, 17 Sep 2019 08:53:45 +0200 Subject: [PATCH] add a new permission for overwriting existing files The upload permission is required to allow file overwrite --- README.md | 2 + api/schema/openapi.yaml | 2 + dataprovider/dataprovider.go | 2 +- dataprovider/user.go | 3 ++ scripts/README.md | 5 +- scripts/sftpgo_api_cli.py | 4 +- sftpd/handler.go | 4 ++ sftpd/scp.go | 9 +++- sftpd/sftpd_test.go | 92 +++++++++++++++++++++++++++++++++--- 9 files changed, 110 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index d6aed3b3..fd208645 100644 --- a/README.md +++ b/README.md @@ -282,6 +282,7 @@ For each account the following properties can be configured: - `list` list items is allowed - `download` download files is allowed - `upload` upload files is allowed + - `overwrite` overwrite an existing file, while uploading, is allowed. `upload` permission is required to allow file overwrite - `delete` delete files or directories is allowed - `rename` rename files or directories is allowed - `create_dirs` create directories is allowed @@ -400,6 +401,7 @@ The logs can be divided into the following categories: - [viper](https://github.com/spf13/viper) - [cobra](https://github.com/spf13/cobra) - [xid](https://github.com/rs/xid) +- [nathanaelle/password](https://github.com/nathanaelle/password) Some code was initially taken from [Pterodactyl sftp server](https://github.com/pterodactyl/sftp-server) diff --git a/api/schema/openapi.yaml b/api/schema/openapi.yaml index b79b9c31..c72b640f 100644 --- a/api/schema/openapi.yaml +++ b/api/schema/openapi.yaml @@ -511,6 +511,7 @@ components: - list - download - upload + - overwrite - delete - rename - create_dirs @@ -521,6 +522,7 @@ components: * `list` - list items is allowed * `download` - download files is allowed * `upload` - upload files is allowed + * `overwrite` - overwrite an existing file, while uploading, is allowed. upload permission is required to allow file overwrite * `delete` - delete files or directories is allowed * `rename` - rename files or directories is allowed * `create_dirs` - create directories is allowed diff --git a/dataprovider/dataprovider.go b/dataprovider/dataprovider.go index 3bfb077d..6814d54d 100644 --- a/dataprovider/dataprovider.go +++ b/dataprovider/dataprovider.go @@ -55,7 +55,7 @@ var ( provider Provider sqlPlaceholders []string validPerms = []string{PermAny, PermListItems, PermDownload, PermUpload, PermDelete, PermRename, - PermCreateDirs, PermCreateSymlinks} + PermCreateDirs, PermCreateSymlinks, PermOverwrite} hashPwdPrefixes = []string{argonPwdPrefix, bcryptPwdPrefix, pbkdf2SHA1Prefix, pbkdf2SHA256Prefix, pbkdf2SHA512Prefix, sha512cryptPwdPrefix} pbkdfPwdPrefixes = []string{pbkdf2SHA1Prefix, pbkdf2SHA256Prefix, pbkdf2SHA512Prefix} diff --git a/dataprovider/user.go b/dataprovider/user.go index 4722abe2..8251d3ad 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -17,6 +17,9 @@ const ( PermDownload = "download" // upload files is allowed PermUpload = "upload" + // overwrite an existing file, while uploading, is allowed + // upload permission is required to allow file overwrite + PermOverwrite = "overwrite" // delete files or directories is allowed PermDelete = "delete" // rename files or directories is allowed diff --git a/scripts/README.md b/scripts/README.md index 3d70f867..7a4b82c5 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -41,7 +41,7 @@ Let's see a sample usage for each REST API. Command: ``` -python sftpgo_api_cli.py add-user test_username --password "test_pwd" --home-dir="/tmp/test_home_dir" --uid 33 --gid 1000 --max-sessions 2 --quota-size 0 --quota-files 3 --permissions "list" "download" "upload" "delete" "rename" "create_dirs" --upload-bandwidth 100 --download-bandwidth 60 +python sftpgo_api_cli.py add-user test_username --password "test_pwd" --home-dir="/tmp/test_home_dir" --uid 33 --gid 1000 --max-sessions 2 --quota-size 0 --quota-files 3 --permissions "list" "download" "upload" "delete" "rename" "create_dirs" "overwrite" --upload-bandwidth 100 --download-bandwidth 60 ``` Output: @@ -62,7 +62,8 @@ Output: "upload", "delete", "rename", - "create_dirs" + "create_dirs", + "overwrite" ], "used_quota_size": 0, "used_quota_files": 0, diff --git a/scripts/sftpgo_api_cli.py b/scripts/sftpgo_api_cli.py index 54404752..86c2ec20 100755 --- a/scripts/sftpgo_api_cli.py +++ b/scripts/sftpgo_api_cli.py @@ -62,7 +62,7 @@ class SFTPGoApiRequests: download_bandwidth=0): user = {"id":user_id, "username":username, "uid":uid, "gid":gid, "max_sessions":max_sessions, "quota_size":quota_size, "quota_files":quota_files, - "upload_bandwidth":upload_bandwidth,"download_bandwidth":download_bandwidth} + "upload_bandwidth":upload_bandwidth, "download_bandwidth":download_bandwidth} if password: user.update({"password":password}) if public_keys: @@ -136,7 +136,7 @@ def addCommonUserArguments(parser): help='Maximum size allowed as bytes. 0 means unlimited. Default: %(default)s') parser.add_argument('-F', '--quota-files', type=int, default=0, help="default: %(default)s") parser.add_argument('-G', '--permissions', type=str, nargs='+', default=[], - choices=['*', 'list', 'download', 'upload', 'delete', 'rename', 'create_dirs', + choices=['*', 'list', 'download', 'upload', 'overwrite', 'delete', 'rename', 'create_dirs', 'create_symlinks'], help='Default: %(default)s') parser.add_argument('-U', '--upload-bandwidth', type=int, default=0, help='Maximum upload bandwidth as KB/s, 0 means unlimited. Default: %(default)s') diff --git a/sftpd/handler.go b/sftpd/handler.go index 04cdf5e3..096ae2d3 100644 --- a/sftpd/handler.go +++ b/sftpd/handler.go @@ -131,6 +131,10 @@ func (c Connection) Filewrite(request *sftp.Request) (io.WriterAt, error) { return nil, sftp.ErrSshFxOpUnsupported } + if !c.User.HasPerm(dataprovider.PermOverwrite) { + return nil, sftp.ErrSshFxPermissionDenied + } + return c.handleSFTPUploadToExistingFile(request.Pflags(), p, filePath, stat.Size()) } diff --git a/sftpd/scp.go b/sftpd/scp.go index da4e0d3f..a7b77b62 100644 --- a/sftpd/scp.go +++ b/sftpd/scp.go @@ -242,7 +242,7 @@ func (c *scpCommand) handleUpload(uploadFilePath string, sizeToRead int64) error updateConnectionActivity(c.connection.ID) if !c.connection.User.HasPerm(dataprovider.PermUpload) { err := fmt.Errorf("Permission denied") - c.connection.Log(logger.LevelWarn, logSenderSCP, "error uploading file: %#v, permission denied", uploadFilePath) + c.connection.Log(logger.LevelWarn, logSenderSCP, "cannot upload file: %#v, permission denied", uploadFilePath) c.sendErrorMessage(err.Error()) return err } @@ -275,6 +275,13 @@ func (c *scpCommand) handleUpload(uploadFilePath string, sizeToRead int64) error return err } + if !c.connection.User.HasPerm(dataprovider.PermOverwrite) { + err := fmt.Errorf("Permission denied") + c.connection.Log(logger.LevelWarn, logSenderSCP, "cannot overwrite file: %#v, permission denied", uploadFilePath) + c.sendErrorMessage(err.Error()) + return err + } + if uploadMode == uploadModeAtomic { err = os.Rename(p, filePath) if err != nil { diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index 7fb15190..2524277d 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -1233,7 +1233,7 @@ func TestPermList(t *testing.T) { usePubKey := true u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete, dataprovider.PermRename, - dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1263,7 +1263,7 @@ func TestPermDownload(t *testing.T) { usePubKey := true u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermUpload, dataprovider.PermDelete, dataprovider.PermRename, - dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1305,7 +1305,7 @@ func TestPermUpload(t *testing.T) { usePubKey := false u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermDelete, dataprovider.PermRename, - dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1334,11 +1334,48 @@ func TestPermUpload(t *testing.T) { os.RemoveAll(user.GetHomeDir()) } +func TestPermOverwrite(t *testing.T) { + usePubKey := false + u := getTestUser(usePubKey) + u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete, + dataprovider.PermRename, dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + user, _, err := api.AddUser(u, http.StatusOK) + if err != nil { + t.Errorf("unable to add user: %v", err) + } + client, err := getSftpClient(user, usePubKey) + if err != nil { + t.Errorf("unable to create sftp client: %v", err) + } else { + defer client.Close() + testFileName := "test_file.dat" + testFilePath := filepath.Join(homeBasePath, testFileName) + testFileSize := int64(65535) + err = createTestFile(testFilePath, testFileSize) + if err != nil { + t.Errorf("unable to create test file: %v", err) + } + err = sftpUploadFile(testFilePath, testFileName, testFileSize, client) + if err != nil { + t.Errorf("error uploading file: %v", err) + } + err = sftpUploadFile(testFilePath, testFileName, testFileSize, client) + if err == nil { + t.Errorf("file overwrite without permission should not succeed") + } + } + _, err = api.RemoveUser(user, http.StatusOK) + if err != nil { + t.Errorf("unable to remove user: %v", err) + } + os.RemoveAll(user.GetHomeDir()) +} + func TestPermDelete(t *testing.T) { usePubKey := false u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermRename, - dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1375,7 +1412,7 @@ func TestPermRename(t *testing.T) { usePubKey := false u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete, - dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks} + dataprovider.PermCreateDirs, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1416,7 +1453,7 @@ func TestPermCreateDirs(t *testing.T) { usePubKey := false u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete, - dataprovider.PermRename, dataprovider.PermCreateSymlinks} + dataprovider.PermRename, dataprovider.PermCreateSymlinks, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1453,7 +1490,7 @@ func TestPermSymlink(t *testing.T) { usePubKey := false u := getTestUser(usePubKey) u.Permissions = []string{dataprovider.PermListItems, dataprovider.PermDownload, dataprovider.PermUpload, dataprovider.PermDelete, - dataprovider.PermRename, dataprovider.PermCreateDirs} + dataprovider.PermRename, dataprovider.PermCreateDirs, dataprovider.PermOverwrite} user, _, err := api.AddUser(u, http.StatusOK) if err != nil { t.Errorf("unable to add user: %v", err) @@ -1794,6 +1831,47 @@ func TestSCPPermUpload(t *testing.T) { } } +func TestSCPPermOverwrite(t *testing.T) { + if len(scpPath) == 0 { + t.Skip("scp command not found, unable to execute this test") + } + usePubKey := true + u := getTestUser(usePubKey) + u.Permissions = []string{dataprovider.PermUpload, dataprovider.PermCreateDirs} + user, _, err := api.AddUser(u, http.StatusOK) + if err != nil { + t.Errorf("unable to add user: %v", err) + } + testFileName := "test_file.dat" + testFilePath := filepath.Join(homeBasePath, testFileName) + testFileSize := int64(65536) + err = createTestFile(testFilePath, testFileSize) + if err != nil { + t.Errorf("unable to create test file: %v", err) + } + remoteUpPath := fmt.Sprintf("%v@127.0.0.1:%v", user.Username, "/tmp") + err = scpUpload(testFilePath, remoteUpPath, true, false) + if err != nil { + t.Errorf("scp upload error: %v", err) + } + err = scpUpload(testFilePath, remoteUpPath, true, false) + if err == nil { + t.Errorf("scp upload must fail, the user cannot ovewrite existing files") + } + err = os.Remove(testFilePath) + if err != nil { + t.Errorf("error removing test file") + } + err = os.RemoveAll(user.GetHomeDir()) + if err != nil { + t.Errorf("error removing uploaded files") + } + _, err = api.RemoveUser(user, http.StatusOK) + if err != nil { + t.Errorf("unable to remove user: %v", err) + } +} + func TestSCPPermDownload(t *testing.T) { if len(scpPath) == 0 { t.Skip("scp command not found, unable to execute this test")