quota: improve size check

get the remaining allowed size when an upload starts and check it against the uploaded bytes Fixes drakkan#128
devopsmakers · Jun 18, 2020 · e86089a · e86089a
1 parent 3ceba7a
commit e86089a
Show file tree

Hide file tree

Showing 11 changed files with 298 additions and 48 deletions.
diff --git a/docs/account.md b/docs/account.md
@@ -20,7 +20,7 @@ For each account, the following properties can be configured:
   - `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 if this permission is granted on target path. You can enable rename in a more controlled way granting `delete` permission on source directory and `upload` permission on target directory
+  - `rename` rename a file or a directory is allowed if this permission is granted on target path. You can enable rename in a more controlled way granting `delete` permission on source directory and `upload`/`create_dirs` permissions on target directory. Please be aware that no subdir permission is checked for the directories rename case
   - `create_dirs` create directories is allowed
   - `create_symlinks` create symbolic links is allowed
   - `chmod` changing file or directory permissions is allowed. On Windows, only the 0200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. The other bits are currently unused. Use mode 0400 for a read-only file and 0600 for a readable+writable file.

diff --git a/docs/custom-actions.md b/docs/custom-actions.md
@@ -3,7 +3,7 @@
 The `actions` struct inside the "sftpd" configuration section allows to configure the actions for file operations and SSH commands.
 The `hook` can be defined as the absolute path of your program or an HTTP URL.
 
-The `upload` condition includes both uploads to new files and overwrite of existing files. The `ssh_cmd` condition will be triggered after a command is successfully executed via SSH. `scp` will trigger the `download` and `upload` conditions and not `ssh_cmd`.
+The `upload` condition includes both uploads to new files and overwrite of existing files. If an upload is aborted for quota limits SFTPGo tries to remove the partial file, so if the notification reports a zero size file and a quota exceeded error the file has been deleted. The `ssh_cmd` condition will be triggered after a command is successfully executed via SSH. `scp` will trigger the `download` and `upload` conditions and not `ssh_cmd`.
 The notification will indicate if an error is detected and so, for example, a partial file is uploaded.
 The `pre-delete` action, if defined, will be called just before files deletion. If the external command completes with a zero exit status or the HTTP notification response code is `200` then SFTPGo will assume that the file was already deleted/moved and so it will not try to remove the file and it will not execute the hook defined for the `delete` action.
 
@@ -26,7 +26,7 @@ The external program can also read the following environment variables:
 - `SFTPGO_ACTION_FS_PROVIDER`, `0` for local filesystem, `1` for S3 backend, `2` for Google Cloud Storage (GCS) backend
 - `SFTPGO_ACTION_BUCKET`, non-empty for S3 and GCS backends
 - `SFTPGO_ACTION_ENDPOINT`, non-empty for S3 backend if configured
-- `SFTPGO_ACTION_STATUS`, integer. 0 means an error occurred. 1 means no error
+- `SFTPGO_ACTION_STATUS`, integer. 0 means a generic error occurred. 1 means no error, 2 means quota exceeded error
 
 Previous global environment variables aren't cleared when the script is called.
 The program must finish within 30 seconds.
@@ -42,7 +42,7 @@ If the `hook` defines an HTTP URL then this URL will be invoked as HTTP POST. Th
 - `fs_provider`, `0` for local filesystem, `1` for S3 backend, `2` for Google Cloud Storage (GCS) backend
 - `bucket`, not null for S3 and GCS backends
 - `endpoint`, not null for S3 backend if configured
-- `status`, integer. 0 means an error occurred. 1 means no error
+- `status`, integer. 0 means a generic error occurred. 1 means no error, 2 means quota exceeded error
 
 The HTTP request will use the global configuration for HTTP clients.
 

diff --git a/docs/ssh-commands.md b/docs/ssh-commands.md
@@ -21,7 +21,7 @@ SFTPGo support the following built-in SSH commands:
 - `scp`, SFTPGo implements the SCP protocol so we can support it for cloud filesystems too and we can avoid the other system commands limitations. SCP between two remote hosts is supported using the `-3` scp option.
 - `md5sum`, `sha1sum`, `sha256sum`, `sha384sum`, `sha512sum`. Useful to check message digests for uploaded files.
 - `cd`, `pwd`. Some SFTP clients do not support the SFTP SSH_FXP_REALPATH packet type, so they use `cd` and `pwd` SSH commands to get the initial directory. Currently `cd` does nothing and `pwd` always returns the `/` path.
-- `sftpgo-copy`. This is a built-in copy implementation. It allows server side copy for files and directories. The first argument is the source file/directory and the second one is the destination file/directory, for example `sftpgo-copy <src> <dst>`. The command will fail if the destination directory exists. Copy for directories spanning virtual folders is not supported. Only local filesystem is supported: recursive copy for Cloud Storage filesystems requires a new request for every file in any case, so a server side copy is not possibile.
+- `sftpgo-copy`. This is a built-in copy implementation. It allows server side copy for files and directories. The first argument is the source file/directory and the second one is the destination file/directory, for example `sftpgo-copy <src> <dst>`. The command will fail if the destination directory exists. Copy for directories spanning virtual folders is not supported. Only local filesystem is supported: recursive copy for Cloud Storage filesystems requires a new request for every file in any case, so a server side copy is not possibile. Please be aware that only the `list` permission for the source path and the `upload` and `create_dirs` (for directories) permissions for the destination path are checked.
 - `sftpgo-remove`. This is a built-in remove implementation. It allows to remove single files and to recursively remove directories. The first argument is the file/directory to remove, for example `sftpgo-remove <dst>`. Only local filesystem is supported: recursive remove for Cloud Storage filesystems requires a new request for every file in any case, so a server side remove is not possibile.
 
 The following SSH commands are enabled by default:

diff --git a/sftpd/handler.go b/sftpd/handler.go
@@ -531,6 +531,7 @@ func (c Connection) handleSFTPUploadToNewFile(resolvedPath, filePath, requestPat
 		isFinished:     false,
 		minWriteOffset: 0,
 		requestPath:    requestPath,
+		maxWriteSize:   quotaResult.GetRemainingSize(),
 		lock:           new(sync.Mutex),
 	}
 	addTransfer(&transfer)
@@ -570,6 +571,9 @@ func (c Connection) handleSFTPUploadToExistingFile(pflags sftp.FileOpenFlags, re
 	}
 
 	initialSize := int64(0)
+	// if there is a size limit remaining size cannot be 0 here, since quotaResult.HasSpace
+	// will return false in this case and we deny the upload before
+	maxWriteSize := quotaResult.GetRemainingSize()
 	if pflags.Append && osFlags&os.O_TRUNC == 0 {
 		c.Log(logger.LevelDebug, logSender, "upload resume requested, file path: %#v initial size: %v", filePath, fileSize)
 		minWriteOffset = fileSize
@@ -587,6 +591,9 @@ func (c Connection) handleSFTPUploadToExistingFile(pflags sftp.FileOpenFlags, re
 		} else {
 			initialSize = fileSize
 		}
+		if maxWriteSize > 0 {
+			maxWriteSize += fileSize
+		}
 	}
 
 	vfs.SetPathPermissions(c.fs, filePath, c.User.GetUID(), c.User.GetGID())
@@ -611,12 +618,69 @@ func (c Connection) handleSFTPUploadToExistingFile(pflags sftp.FileOpenFlags, re
 		minWriteOffset: minWriteOffset,
 		initialSize:    initialSize,
 		requestPath:    requestPath,
+		maxWriteSize:   maxWriteSize,
 		lock:           new(sync.Mutex),
 	}
 	addTransfer(&transfer)
 	return &transfer, nil
 }
 
+// hasSpaceForCrossRename checks the quota after a rename between different folders
+func (c Connection) hasSpaceForCrossRename(quotaResult vfs.QuotaCheckResult, initialSize int64, sourcePath string) bool {
+	if !quotaResult.HasSpace && initialSize == -1 {
+		// we are over quota and this is not a file replace
+		return false
+	}
+	fi, err := c.fs.Lstat(sourcePath)
+	if err != nil {
+		c.Log(logger.LevelWarn, logSender, "cross rename denied, stat error for path %#v: %v", sourcePath, err)
+		return false
+	}
+	var sizeDiff int64
+	var filesDiff int
+	if fi.Mode().IsRegular() {
+		sizeDiff = fi.Size()
+		filesDiff = 1
+		if initialSize != -1 {
+			sizeDiff -= initialSize
+			filesDiff = 0
+		}
+	} else if fi.IsDir() {
+		filesDiff, sizeDiff, err = c.fs.GetDirSize(sourcePath)
+		if err != nil {
+			c.Log(logger.LevelWarn, logSender, "cross rename denied, error getting size for directory %#v: %v", sourcePath, err)
+			return false
+		}
+	}
+	if !quotaResult.HasSpace && initialSize != -1 {
+		// we are over quota but we are overwriting an existing file so we check if the quota size after the rename is ok
+		if quotaResult.QuotaSize == 0 {
+			return true
+		}
+		c.Log(logger.LevelDebug, logSender, "cross rename overwrite, source %#v, used size %v, size to add %v",
+			sourcePath, quotaResult.UsedSize, sizeDiff)
+		quotaResult.UsedSize += sizeDiff
+		return quotaResult.GetRemainingSize() >= 0
+	}
+	if quotaResult.QuotaFiles > 0 {
+		remainingFiles := quotaResult.GetRemainingFiles()
+		c.Log(logger.LevelDebug, logSender, "cross rename, source %#v remaining file %v to add %v", sourcePath,
+			remainingFiles, filesDiff)
+		if remainingFiles < filesDiff {
+			return false
+		}
+	}
+	if quotaResult.QuotaSize > 0 {
+		remainingSize := quotaResult.GetRemainingSize()
+		c.Log(logger.LevelDebug, logSender, "cross rename, source %#v remaining size %v to add %v", sourcePath,
+			remainingSize, sizeDiff)
+		if remainingSize < sizeDiff {
+			return false
+		}
+	}
+	return true
+}
+
 func (c Connection) hasSpaceForRename(request *sftp.Request, initialSize int64, sourcePath string) bool {
 	if dataprovider.GetQuotaTracking() == 0 {
 		return true
@@ -639,24 +703,7 @@ func (c Connection) hasSpaceForRename(request *sftp.Request, initialSize int64,
 		return true
 	}
 	quotaResult := c.hasSpace(true, request.Target)
-	if !quotaResult.HasSpace {
-		if initialSize != -1 {
-			// we are overquota but we are overwriting a file so we check the quota size
-			quotaResult = c.hasSpace(false, request.Target)
-			if quotaResult.HasSpace {
-				// we have enough quota size
-				return true
-			}
-			if fi, err := c.fs.Lstat(sourcePath); err == nil {
-				if fi.Mode().IsRegular() {
-					// we have space if we are overwriting a bigger file with a smaller one
-					return initialSize >= fi.Size()
-				}
-			}
-		}
-		return false
-	}
-	return true
+	return c.hasSpaceForCrossRename(quotaResult, initialSize, sourcePath)
 }
 
 func (c Connection) hasSpace(checkFiles bool, requestPath string) vfs.QuotaCheckResult {
@@ -772,6 +819,12 @@ func (c Connection) isRenamePermitted(sourcePath string, request *sftp.Request)
 			!c.User.HasPerm(dataprovider.PermUpload, path.Dir(request.Target))) {
 		return false
 	}
+	if !c.User.HasPerm(dataprovider.PermRename, path.Dir(request.Target)) &&
+		!c.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(request.Target)) {
+		if fi, err := c.fs.Lstat(sourcePath); err == nil && fi.IsDir() {
+			return false
+		}
+	}
 	return true
 }
 

diff --git a/sftpd/internal_test.go b/sftpd/internal_test.go
@@ -1042,6 +1042,40 @@ func TestRsyncOptions(t *testing.T) {
 	assert.EqualError(t, err, errUnsupportedConfig.Error())
 }
 
+func TestSpaceForCrossRename(t *testing.T) {
+	if runtime.GOOS == osWindows {
+		t.Skip("this test is not available on Windows")
+	}
+	permissions := make(map[string][]string)
+	permissions["/"] = []string{dataprovider.PermAny}
+	user := dataprovider.User{
+		Permissions: permissions,
+		HomeDir:     os.TempDir(),
+	}
+	fs, err := user.GetFilesystem("123")
+	assert.NoError(t, err)
+	conn := Connection{
+		User: user,
+		fs:   fs,
+	}
+	quotaResult := vfs.QuotaCheckResult{
+		HasSpace: true,
+	}
+	assert.False(t, conn.hasSpaceForCrossRename(quotaResult, -1, filepath.Join(os.TempDir(), "a missing file")))
+	testDir := filepath.Join(os.TempDir(), "dir")
+	err = os.MkdirAll(testDir, os.ModePerm)
+	assert.NoError(t, err)
+	err = ioutil.WriteFile(filepath.Join(testDir, "afile"), []byte("content"), os.ModePerm)
+	assert.NoError(t, err)
+	err = os.Chmod(testDir, 0001)
+	assert.NoError(t, err)
+	assert.False(t, conn.hasSpaceForCrossRename(quotaResult, -1, testDir))
+	err = os.Chmod(testDir, os.ModePerm)
+	assert.NoError(t, err)
+	err = os.RemoveAll(testDir)
+	assert.NoError(t, err)
+}
+
 func TestSystemCommandSizeForPath(t *testing.T) {
 	permissions := make(map[string][]string)
 	permissions["/"] = []string{dataprovider.PermAny}
@@ -1164,7 +1198,7 @@ func TestSystemCommandErrors(t *testing.T) {
 		lock:         new(sync.Mutex)}
 	destBuff := make([]byte, 65535)
 	dst := bytes.NewBuffer(destBuff)
-	_, err = transfer.copyFromReaderToWriter(dst, sshCmd.connection.channel, 0)
+	_, err = transfer.copyFromReaderToWriter(dst, sshCmd.connection.channel)
 	assert.EqualError(t, err, readErr.Error())
 
 	mockSSHChannel = MockChannel{
@@ -1174,7 +1208,8 @@ func TestSystemCommandErrors(t *testing.T) {
 		WriteError:   nil,
 	}
 	sshCmd.connection.channel = &mockSSHChannel
-	_, err = transfer.copyFromReaderToWriter(dst, sshCmd.connection.channel, 1)
+	transfer.maxWriteSize = 1
+	_, err = transfer.copyFromReaderToWriter(dst, sshCmd.connection.channel)
 	assert.EqualError(t, err, errQuotaExceeded.Error())
 
 	mockSSHChannel = MockChannel{
@@ -1185,9 +1220,10 @@ func TestSystemCommandErrors(t *testing.T) {
 		ShortWriteErr: true,
 	}
 	sshCmd.connection.channel = &mockSSHChannel
-	_, err = transfer.copyFromReaderToWriter(sshCmd.connection.channel, dst, 0)
+	_, err = transfer.copyFromReaderToWriter(sshCmd.connection.channel, dst)
 	assert.EqualError(t, err, io.ErrShortWrite.Error())
-	_, err = transfer.copyFromReaderToWriter(sshCmd.connection.channel, dst, -1)
+	transfer.maxWriteSize = -1
+	_, err = transfer.copyFromReaderToWriter(sshCmd.connection.channel, dst)
 	assert.EqualError(t, err, errQuotaExceeded.Error())
 	err = os.RemoveAll(homeDir)
 	assert.NoError(t, err)
@@ -2032,9 +2068,21 @@ func TestRenamePermission(t *testing.T) {
 	assert.True(t, conn.isRenamePermitted("", request))
 	request = sftp.NewRequest("Rename", "/dir3/testfile")
 	request.Target = "/dir2/testfile"
-	// delete is granted on Source and Upload on Target, this is enough
+	// delete is granted on Source and Upload on Target, the target is a file this is enough
 	assert.True(t, conn.isRenamePermitted("", request))
 	request = sftp.NewRequest("Rename", "/dir2/testfile")
 	request.Target = "/dir3/testfile"
 	assert.False(t, conn.isRenamePermitted("", request))
+	tmpDir := filepath.Join(os.TempDir(), "dir")
+	err = os.Mkdir(tmpDir, os.ModePerm)
+	assert.NoError(t, err)
+	request.Filepath = "/dir"
+	request.Target = "/dir2/dir"
+	// the source is a dir and the target has no createDirs perm
+	assert.False(t, conn.isRenamePermitted(tmpDir, request))
+	conn.User.Permissions["/dir2"] = []string{dataprovider.PermUpload, dataprovider.PermCreateDirs}
+	// the source is a dir and the target has createDirs perm
+	assert.True(t, conn.isRenamePermitted(tmpDir, request))
+	err = os.RemoveAll(tmpDir)
+	assert.NoError(t, err)
 }
diff --git a/sftpd/scp.go b/sftpd/scp.go
@@ -188,7 +188,7 @@ func (c *scpCommand) getUploadFileData(sizeToRead int64, transfer *Transfer) err
 }
 
 func (c *scpCommand) handleUploadFile(resolvedPath, filePath string, sizeToRead int64, isNewFile bool, fileSize int64, requestPath string) error {
-	quotaResult := c.connection.hasSpace(true, requestPath)
+	quotaResult := c.connection.hasSpace(isNewFile, requestPath)
 	if !quotaResult.HasSpace {
 		err := fmt.Errorf("denying file write due to quota limits")
 		c.connection.Log(logger.LevelWarn, logSenderSCP, "error uploading file: %#v, err: %v", filePath, err)
@@ -204,6 +204,7 @@ func (c *scpCommand) handleUploadFile(resolvedPath, filePath string, sizeToRead
 	}
 
 	initialSize := int64(0)
+	maxWriteSize := quotaResult.GetRemainingSize()
 	if !isNewFile {
 		if vfs.IsLocalOsFs(c.connection.fs) {
 			vfolder, err := c.connection.User.GetVirtualFolderForPath(path.Dir(requestPath))
@@ -218,6 +219,9 @@ func (c *scpCommand) handleUploadFile(resolvedPath, filePath string, sizeToRead
 		} else {
 			initialSize = fileSize
 		}
+		if maxWriteSize > 0 {
+			maxWriteSize += fileSize
+		}
 	}
 
 	vfs.SetPathPermissions(c.connection.fs, filePath, c.connection.User.GetUID(), c.connection.User.GetGID())
@@ -242,6 +246,7 @@ func (c *scpCommand) handleUploadFile(resolvedPath, filePath string, sizeToRead
 		minWriteOffset: 0,
 		initialSize:    initialSize,
 		requestPath:    requestPath,
+		maxWriteSize:   maxWriteSize,
 		lock:           new(sync.Mutex),
 	}
 	addTransfer(&transfer)

diff --git a/sftpd/sftpd.go b/sftpd/sftpd.go
@@ -170,7 +170,9 @@ func newActionNotification(user dataprovider.User, operation, filePath, target,
 	} else if user.FsConfig.Provider == 2 {
 		bucket = user.FsConfig.GCSConfig.Bucket
 	}
-	if err != nil {
+	if err == errQuotaExceeded {
+		status = 2
+	} else if err != nil {
 		status = 0
 	}
 	return actionNotification{