From 0b9a96ec6b060e7fa8c1f0f438686030e66d5d09 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 12 Jun 2022 12:04:48 +0200 Subject: [PATCH] restore fast path for recursive permissions check and update some docs Signed-off-by: Nicola Murino --- common/connection.go | 26 +++++++++++++++++++++++--- common/connection_test.go | 24 +++++++++++++++++++++--- dataprovider/user.go | 5 ++++- docs/httpfs.md | 2 ++ docs/repo.md | 4 ++-- httpd/httpd_test.go | 6 ++++++ openapi/httpfs.yaml | 4 +++- sftpd/internal_test.go | 13 ++++++++++++- sftpd/sftpd_test.go | 6 +++--- sftpd/ssh_cmd.go | 26 +++++++++++++++++++++++--- vfs/httpfs.go | 21 ++++++++------------- 11 files changed, 107 insertions(+), 30 deletions(-) diff --git a/common/connection.go b/common/connection.go index 3e146f8b..567e3f87 100644 --- a/common/connection.go +++ b/common/connection.go @@ -502,7 +502,8 @@ func (c *BaseConnection) Rename(virtualSourcePath, virtualTargetPath string) err virtualSourcePath) return c.GetOpUnsupportedError() } - if err = c.checkRecursiveRenameDirPermissions(fsSrc, fsDst, fsSourcePath, fsTargetPath); err != nil { + if err = c.checkRecursiveRenameDirPermissions(fsSrc, fsDst, fsSourcePath, fsTargetPath, + virtualSourcePath, virtualTargetPath, srcInfo); err != nil { c.Log(logger.LevelDebug, "error checking recursive permissions before renaming %#v: %+v", fsSourcePath, err) return err } @@ -767,7 +768,24 @@ func (c *BaseConnection) truncateFile(fs vfs.Fs, fsPath, virtualPath string, siz return err } -func (c *BaseConnection) checkRecursiveRenameDirPermissions(fsSrc, fsDst vfs.Fs, sourcePath, targetPath string) error { +func (c *BaseConnection) checkRecursiveRenameDirPermissions(fsSrc, fsDst vfs.Fs, sourcePath, targetPath, + virtualSourcePath, virtualTargetPath string, fi os.FileInfo, +) error { + if !c.User.HasPermissionsInside(virtualSourcePath) && + !c.User.HasPermissionsInside(virtualTargetPath) { + if !c.isRenamePermitted(fsSrc, fsDst, sourcePath, targetPath, virtualSourcePath, virtualTargetPath, fi) { + c.Log(logger.LevelInfo, "rename %#v -> %#v is not allowed, virtual destination path: %#v", + sourcePath, targetPath, virtualTargetPath) + return c.GetPermissionDeniedError() + } + // if all rename permissions are granted we have finished, otherwise we have to walk + // because we could have the rename dir permission but not the rename file and the dir to + // rename could contain files + if c.User.HasPermsRenameAll(path.Dir(virtualSourcePath)) && c.User.HasPermsRenameAll(path.Dir(virtualTargetPath)) { + return nil + } + } + return fsSrc.Walk(sourcePath, func(walkedPath string, info os.FileInfo, err error) error { if err != nil { return c.GetFsError(fsSrc, err) @@ -810,7 +828,9 @@ func (c *BaseConnection) hasRenamePerms(virtualSourcePath, virtualTargetPath str c.User.HasAnyPerm(perms, path.Dir(virtualTargetPath)) } -func (c *BaseConnection) isRenamePermitted(fsSrc, fsDst vfs.Fs, fsSourcePath, fsTargetPath, virtualSourcePath, virtualTargetPath string, fi os.FileInfo) bool { +func (c *BaseConnection) isRenamePermitted(fsSrc, fsDst vfs.Fs, fsSourcePath, fsTargetPath, virtualSourcePath, + virtualTargetPath string, fi os.FileInfo, +) bool { if !c.isLocalOrSameFolderRename(virtualSourcePath, virtualTargetPath) { c.Log(logger.LevelInfo, "rename %#v->%#v is not allowed: the paths must be local or on the same virtual folder", virtualSourcePath, virtualTargetPath) diff --git a/common/connection_test.go b/common/connection_test.go index 74fa3659..7a2c3138 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -116,10 +116,28 @@ func TestSetStatMode(t *testing.T) { } func TestRecursiveRenameWalkError(t *testing.T) { - fs := vfs.NewOsFs("", os.TempDir(), "") - conn := NewBaseConnection("", ProtocolWebDAV, "", "", dataprovider.User{}) - err := conn.checkRecursiveRenameDirPermissions(fs, fs, "/source", "/target") + fs := vfs.NewOsFs("", filepath.Clean(os.TempDir()), "") + conn := NewBaseConnection("", ProtocolWebDAV, "", "", dataprovider.User{ + BaseUser: sdk.BaseUser{ + Permissions: map[string][]string{ + "/": {dataprovider.PermListItems, dataprovider.PermUpload, + dataprovider.PermDownload, dataprovider.PermRenameDirs}, + }, + }, + }) + err := conn.checkRecursiveRenameDirPermissions(fs, fs, filepath.Join(os.TempDir(), "/source"), + filepath.Join(os.TempDir(), "/target"), "/source", "/target", + vfs.NewFileInfo("source", true, 0, time.Now(), false)) assert.ErrorIs(t, err, os.ErrNotExist) + conn.User.Permissions["/"] = []string{dataprovider.PermListItems, dataprovider.PermUpload, + dataprovider.PermDownload, dataprovider.PermRenameFiles} + // no dir rename permission, the quick check path returns permission error without walking + err = conn.checkRecursiveRenameDirPermissions(fs, fs, filepath.Join(os.TempDir(), "/source"), + filepath.Join(os.TempDir(), "/target"), "/source", "/target", + vfs.NewFileInfo("source", true, 0, time.Now(), false)) + if assert.Error(t, err) { + assert.EqualError(t, err, conn.GetPermissionDeniedError().Error()) + } } func TestCrossRenameFsErrors(t *testing.T) { diff --git a/dataprovider/user.go b/dataprovider/user.go index b7dbd7e5..c8ab0611 100644 --- a/dataprovider/user.go +++ b/dataprovider/user.go @@ -743,7 +743,10 @@ func (u *User) HasVirtualFoldersInside(virtualPath string) bool { // HasPermissionsInside returns true if the specified virtualPath has no permissions itself and // no subdirs with defined permissions func (u *User) HasPermissionsInside(virtualPath string) bool { - for dir := range u.Permissions { + for dir, perms := range u.Permissions { + if len(perms) == 1 && perms[0] == PermAny { + continue + } if dir == virtualPath { return true } else if len(dir) > len(virtualPath) { diff --git a/docs/httpfs.md b/docs/httpfs.md index 368f2ae3..78e06b41 100644 --- a/docs/httpfs.md +++ b/docs/httpfs.md @@ -2,6 +2,8 @@ SFTPGo can use custom storage backend implementations compliant with the REST API documented [here](./../openapi/httpfs.yaml). +:warning: HTTPFs is a work in progress and makes no API stability promises. + The only required parameter is the HTTP/S endpoint that SFTPGo must use to make API calls. If you define `http://127.0.0.1:9999/api/v1` as endpoint, SFTPGo will add the API path, for example for the `stat` API it will invoke `http://127.0.0.1:9999/api/v1/stat/{name}`. diff --git a/docs/repo.md b/docs/repo.md index d74c6884..36f30428 100644 --- a/docs/repo.md +++ b/docs/repo.md @@ -9,7 +9,7 @@ Supported distributions: - Debian 10 "buster" - Debian 11 "bullseye" -Import the public key used by the package management system using the following command: +Import the public key used by the package management system: ```shell curl -sS https://ftp.osuosl.org/pub/sftpgo/apt/gpg.key | sudo gpg --dearmor -o /usr/share/keyrings/sftpgo-archive-keyring.gpg @@ -39,7 +39,7 @@ sudo apt install sftpgo The YUM repository supports generic Red Hat based distributions. -Create the SFTPGo repository using the following command: +Create the SFTPGo repository: ```shell ARCH=`uname -m` diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go index 7f30efdc..1b67d8a9 100644 --- a/httpd/httpd_test.go +++ b/httpd/httpd_test.go @@ -2429,6 +2429,12 @@ func TestAddUserInvalidFsConfig(t *testing.T) { if assert.NoError(t, err) { assert.Contains(t, string(resp), "cannot save a user with a redacted secret") } + u.FsConfig.HTTPConfig.APIKey = nil + u.FsConfig.HTTPConfig.Endpoint = "/api/v1" + _, resp, err = httpdtest.AddUser(u, http.StatusBadRequest) + if assert.NoError(t, err) { + assert.Contains(t, string(resp), "invalid endpoint schema") + } } func TestUserRedactedPassword(t *testing.T) { diff --git a/openapi/httpfs.yaml b/openapi/httpfs.yaml index 7bc48afd..8626a712 100644 --- a/openapi/httpfs.yaml +++ b/openapi/httpfs.yaml @@ -3,7 +3,9 @@ tags: - name: fs info: title: SFTPGo HTTPFs - description: 'SFTPGo HTTP Filesystem API' + description: | + SFTPGo can use custom storage backend implementations compliant with the API defined here. + HTTPFs is a work in progress and makes no API stability promises. version: 0.1.0 servers: - url: /v1 diff --git a/sftpd/internal_test.go b/sftpd/internal_test.go index bb203a5c..d55a02b7 100644 --- a/sftpd/internal_test.go +++ b/sftpd/internal_test.go @@ -1991,7 +1991,18 @@ func TestRecursiveCopyErrors(t *testing.T) { args: []string{"adir", "another"}, } // try to copy a missing directory - err = sshCmd.checkRecursiveCopyPermissions(fs, fs, "adir", "another", "/another") + sshCmd.connection.User.Permissions["/another"] = []string{ + dataprovider.PermCreateDirs, + dataprovider.PermCreateSymlinks, + dataprovider.PermListItems, + } + err = sshCmd.checkRecursiveCopyPermissions(fs, fs, "adir", "another/sub", "/adir", "/another/sub") + assert.Error(t, err) + sshCmd.connection.User.Permissions["/another"] = []string{ + dataprovider.PermListItems, + dataprovider.PermCreateDirs, + } + err = sshCmd.checkRecursiveCopyPermissions(fs, fs, "adir", "another", "/adir/sub", "/another/sub/dir") assert.Error(t, err) } diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index cc92b4f9..f79e90f7 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -4799,10 +4799,9 @@ func TestVirtualFolders(t *testing.T) { assert.NoError(t, err) err = sftpUploadFile(testFilePath, path.Join("vdir2", testFileName), testFileSize, client) assert.NoError(t, err) - // we don't have upload permission on testDir, we can only create dirs + // we don't have rename permission in testDir and vdir2 contains a file err = client.Rename("vdir2", testDir) assert.Error(t, err) - // on testDir1 only symlink aren't allowed err = client.Rename("vdir2", testDir1) assert.NoError(t, err) err = client.Rename(testDir1, "vdir2") @@ -8633,7 +8632,8 @@ func TestSSHCopy(t *testing.T) { assert.NoError(t, err) err = os.Chmod(subPath, 0001) assert.NoError(t, err) - // checkRecursiveCopyPermissions will fail scanning subdirs + // c.connection.fs.GetDirSize(fsSourcePath) will fail scanning subdirs + // checkRecursiveCopyPermissions will work since it will skip subdirs with no permissions _, err = runSSHCommand(fmt.Sprintf("sftpgo-copy %v %v", vdirPath1, "newdir"), user, usePubKey) assert.Error(t, err) err = os.Chmod(subPath, os.ModePerm) diff --git a/sftpd/ssh_cmd.go b/sftpd/ssh_cmd.go index 774f6a59..5d2caf62 100644 --- a/sftpd/ssh_cmd.go +++ b/sftpd/ssh_cmd.go @@ -589,10 +589,28 @@ func (c *sshCommand) hasCopyPermissions(sshSourcePath, sshDestPath string, srcIn } // fsSourcePath must be a directory -func (c *sshCommand) checkRecursiveCopyPermissions(fsSrc vfs.Fs, fsDst vfs.Fs, fsSourcePath, fsDestPath, sshDestPath string) error { +func (c *sshCommand) checkRecursiveCopyPermissions(fsSrc vfs.Fs, fsDst vfs.Fs, fsSourcePath, fsDestPath, + sshSourcePath, sshDestPath string, +) error { if !c.connection.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(sshDestPath)) { return common.ErrPermissionDenied } + if !c.connection.User.HasPermissionsInside(sshSourcePath) && + !c.connection.User.HasPermissionsInside(sshDestPath) { + // if there are no subdirs with defined permissions we can just check source and destination paths + dstPerms := []string{ + dataprovider.PermCreateDirs, + dataprovider.PermCreateSymlinks, + dataprovider.PermUpload, + } + if c.connection.User.HasPerm(dataprovider.PermListItems, sshSourcePath) && + c.connection.User.HasPerms(dstPerms, sshDestPath) { + return nil + } + // we don't return an error here because we checked all the required permissions above + // for example the directory could not have symlinks inside, so we have to walk to check + // permissions for each item + } return fsSrc.Walk(fsSourcePath, func(walkedPath string, info os.FileInfo, err error) error { if err != nil { @@ -608,9 +626,11 @@ func (c *sshCommand) checkRecursiveCopyPermissions(fsSrc vfs.Fs, fsDst vfs.Fs, f }) } -func (c *sshCommand) checkCopyPermissions(fsSrc vfs.Fs, fsDst vfs.Fs, fsSourcePath, fsDestPath, sshSourcePath, sshDestPath string, info os.FileInfo) error { +func (c *sshCommand) checkCopyPermissions(fsSrc vfs.Fs, fsDst vfs.Fs, fsSourcePath, fsDestPath, sshSourcePath, + sshDestPath string, info os.FileInfo, +) error { if info.IsDir() { - return c.checkRecursiveCopyPermissions(fsSrc, fsDst, fsSourcePath, fsDestPath, sshDestPath) + return c.checkRecursiveCopyPermissions(fsSrc, fsDst, fsSourcePath, fsDestPath, sshSourcePath, sshDestPath) } if !c.hasCopyPermissions(sshSourcePath, sshDestPath, info) { return c.connection.GetPermissionDeniedError() diff --git a/vfs/httpfs.go b/vfs/httpfs.go index d1636e6f..f721c7d8 100644 --- a/vfs/httpfs.go +++ b/vfs/httpfs.go @@ -31,6 +31,10 @@ const ( httpFsName = "httpfs" ) +var ( + supportedEndpointSchema = []string{"http://", "https://"} +) + // HTTPFsConfig defines the configuration for HTTP based filesystem type HTTPFsConfig struct { sdk.BaseHTTPFsConfig @@ -95,6 +99,9 @@ func (c *HTTPFsConfig) validate() error { if err != nil { return fmt.Errorf("httpfs: invalid endpoint: %w", err) } + if !util.IsStringPrefixInSlice(c.Endpoint, supportedEndpointSchema) { + return errors.New("httpfs: invalid endpoint schema: http and https are supported") + } if c.Password.IsEncrypted() && !c.Password.IsValid() { return errors.New("httpfs: invalid encrypted password") } @@ -150,9 +157,7 @@ func NewHTTPFs(connectionID, localTempDir, mountPath string, config HTTPFsConfig localTempDir = filepath.Clean(os.TempDir()) } } - if err := config.validate(); err != nil { - return nil, err - } + config.setEmptyCredentialsIfNil() if !config.Password.IsEmpty() { if err := config.Password.TryDecrypt(); err != nil { return nil, err @@ -343,16 +348,6 @@ func (*HTTPFs) Readlink(name string) (string, error) { // Chown changes the numeric uid and gid of the named file. func (fs *HTTPFs) Chown(name string, uid int, gid int) error { - /*ctx, cancelFn := context.WithDeadline(context.Background(), time.Now().Add(fs.ctxTimeout)) - defer cancelFn() - - queryString := fmt.Sprintf("?uid=%d&gid=%d", uid, gid) - resp, err := fs.sendHTTPRequest(ctx, http.MethodPatch, "chown", name, queryString, "", nil) - if err != nil { - return err - } - defer resp.Body.Close() - return nil*/ return ErrVfsUnsupported }