* fix: SFTP HomeDir path translation for user operations When users have a non-root HomeDir (e.g., '/sftp/user'), their SFTP operations should be relative to that directory. Previously, when a user uploaded to '/' via SFTP, the path was not translated to their home directory, causing 'permission denied for / for permission write'. This fix adds a toAbsolutePath() method that implements chroot-like behavior where the user's HomeDir becomes their root. All file and directory operations now translate paths through this method. Example: User with HomeDir='/sftp/user' uploading to '/' now correctly maps to '/sftp/user'. Fixes: https://github.com/seaweedfs/seaweedfs/issues/7470 * test: add SFTP integration tests Add comprehensive integration tests for the SFTP server including: - HomeDir path translation tests (verifies fix for issue #7470) - Basic file upload/download operations - Directory operations (mkdir, rmdir, list) - Large file handling (1MB test) - File rename operations - Stat/Lstat operations - Path edge cases (trailing slashes, .., unicode filenames) - Admin root access verification The test framework starts a complete SeaweedFS cluster with: - Master server - Volume server - Filer server - SFTP server with test user credentials Test users are configured in testdata/userstore.json: - admin: HomeDir=/ with full access - testuser: HomeDir=/sftp/testuser with access to home - readonly: HomeDir=/public with read-only access * fix: correct SFTP HomeDir path translation and add CI Fix path.Join issue where paths starting with '/' weren't joined correctly. path.Join('/sftp/user', '/file') returns '/file' instead of '/sftp/user/file'. Now we strip the leading '/' before joining. Test improvements: - Update go.mod to Go 1.24 - Fix weed binary discovery to prefer local build over PATH - Add stabilization delay after service startup - All 8 SFTP integration tests pass locally Add GitHub Actions workflow for SFTP tests: - Runs on push/PR affecting sftpd code or tests - Tests HomeDir path translation, file ops, directory ops - Covers issue #7470 fix verification * security: update golang.org/x/crypto to v0.45.0 Addresses security vulnerability in golang.org/x/crypto < 0.45.0 * security: use proper SSH host key verification in tests Replace ssh.InsecureIgnoreHostKey() with ssh.FixedHostKey() that verifies the server's host key matches the known test key we generated. This addresses CodeQL warning go/insecure-hostkeycallback. Also updates go.mod to specify go 1.24.0 explicitly. * security: fix path traversal vulnerability in SFTP toAbsolutePath The previous implementation had a critical security vulnerability: - Path traversal via '../..' could escape the HomeDir chroot jail - Absolute paths were not correctly prefixed with HomeDir The fix: 1. Concatenate HomeDir with userPath directly, then clean 2. Add security check to ensure final path stays within HomeDir 3. If traversal detected, safely return HomeDir instead Also adds path traversal prevention tests to verify the fix. * fix: address PR review comments 1. Fix SkipCleanup check to use actual test config instead of default - Added skipCleanup field to SftpTestFramework struct - Store config.SkipCleanup during Setup() - Use f.skipCleanup in Cleanup() instead of DefaultTestConfig() 2. Fix path prefix check false positive in mkdir - Changed from strings.HasPrefix(absPath, fs.user.HomeDir) - To: absPath == fs.user.HomeDir || strings.HasPrefix(absPath, fs.user.HomeDir+"/") - Prevents matching partial directory names (e.g., /sftp/username when HomeDir is /sftp/user) * fix: check write permission on parent dir for mkdir Aligns makeDir's permission check with newFileWriter for consistency. To create a directory, a user needs write permission on the parent directory, not mkdir permission on the new directory path. * fix: refine SFTP path traversal logic and tests 1. Refine toAbsolutePath: - Use path.Join with strings.TrimPrefix for idiomatic path construction - Return explicit error on path traversal attempt instead of clamping - Updated all call sites to handle the error 2. Add Unit Tests: - Added sftp_server_test.go to verify toAbsolutePath logic - Covers normal paths, root path, and various traversal attempts 3. Update Integration Tests: - Updated PathTraversalPrevention test to reflect that standard SFTP clients sanitize paths before sending. The test now verifies successful containment within the jail rather than blocking (since the server receives a clean path). - The server-side blocking is verified by the new unit tests. 4. Makefile: - Removed -v from default test target * fix: address PR comments on tests and makefile 1. Enhanced Unit Tests: - Added edge cases (empty path, multiple slashes, trailing slash) to sftp_server_test.go 2. Makefile Improvements: - Added 'all' target as default entry point 3. Code Clarity: - Added comment to mkdir permission check explaining defensive nature of HomeDir check * fix: address PR review comments on permissions and tests 1. Security: - Added write permission check on target directory in renameEntry 2. Logging: - Changed dispatch log verbosity from V(0) to V(1) 3. Testing: - Updated Makefile .PHONY targets - Added unit test cases for empty/root HomeDir behavior in toAbsolutePath * fix: set SFTP starting directory to virtual root 1. Critical Fix: - Changed sftp.WithStartDirectory from fs.user.HomeDir to '/' - Prevents double-prefixing when toAbsolutePath translates paths - Users now correctly start at their virtual root which maps to HomeDir 2. Test Improvements: - Use pointer for homeDir in tests for clearer nil vs empty distinction * fix: clean HomeDir at config load time Clean HomeDir path when loading users from JSON config. This handles trailing slashes and other path anomalies at the source, ensuring consistency throughout the codebase and avoiding repeated cleaning on every toAbsolutePath call. * test: strengthen assertions and add error checking in SFTP tests 1. Add error checking for cleanup operations in TestWalk 2. Strengthen cwd assertion to expect '/' explicitly in TestCurrentWorkingDirectory 3. Add error checking for cleanup in PathTraversalPrevention test
444 lines
13 KiB
Go
444 lines
13 KiB
Go
// sftp_filer_refactored.go
|
|
package sftpd
|
|
|
|
import (
|
|
"context"
|
|
"crypto/md5"
|
|
"encoding/json"
|
|
"fmt"
|
|
"io"
|
|
"net/http"
|
|
"os"
|
|
"path"
|
|
"strings"
|
|
"time"
|
|
|
|
"github.com/pkg/sftp"
|
|
"github.com/seaweedfs/seaweedfs/weed/glog"
|
|
"github.com/seaweedfs/seaweedfs/weed/pb"
|
|
filer_pb "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
|
|
weed_server "github.com/seaweedfs/seaweedfs/weed/server"
|
|
"github.com/seaweedfs/seaweedfs/weed/sftpd/user"
|
|
"github.com/seaweedfs/seaweedfs/weed/util"
|
|
"google.golang.org/grpc"
|
|
)
|
|
|
|
const (
|
|
defaultTimeout = 30 * time.Second
|
|
defaultListLimit = 1000
|
|
)
|
|
|
|
// ==================== Filer RPC Helpers ====================
|
|
|
|
// callWithClient wraps a gRPC client call with timeout and client creation.
|
|
func (fs *SftpServer) callWithClient(streaming bool, fn func(ctx context.Context, client filer_pb.SeaweedFilerClient) error) error {
|
|
return fs.withTimeoutContext(func(ctx context.Context) error {
|
|
return fs.WithFilerClient(streaming, func(client filer_pb.SeaweedFilerClient) error {
|
|
return fn(ctx, client)
|
|
})
|
|
})
|
|
}
|
|
|
|
// getEntry retrieves a single directory entry by path.
|
|
func (fs *SftpServer) getEntry(p string) (*filer_pb.Entry, error) {
|
|
dir, name := util.FullPath(p).DirAndName()
|
|
var entry *filer_pb.Entry
|
|
err := fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
r, err := client.LookupDirectoryEntry(ctx, &filer_pb.LookupDirectoryEntryRequest{Directory: dir, Name: name})
|
|
if err != nil {
|
|
if isNotExistError(err) {
|
|
return os.ErrNotExist
|
|
}
|
|
return err
|
|
}
|
|
if r.Entry == nil {
|
|
return fmt.Errorf("%s not found in %s", name, dir)
|
|
}
|
|
entry = r.Entry
|
|
return nil
|
|
})
|
|
if err != nil {
|
|
if isNotExistError(err) {
|
|
return nil, os.ErrNotExist
|
|
}
|
|
return nil, fmt.Errorf("lookup %s: %w", p, err)
|
|
}
|
|
return entry, nil
|
|
}
|
|
|
|
func isNotExistError(err error) bool {
|
|
return strings.Contains(err.Error(), "not found") ||
|
|
strings.Contains(err.Error(), "no entry is found") ||
|
|
strings.Contains(err.Error(), "file does not exist") ||
|
|
err == os.ErrNotExist
|
|
}
|
|
|
|
// updateEntry sends an UpdateEntryRequest for the given entry.
|
|
func (fs *SftpServer) updateEntry(dir string, entry *filer_pb.Entry) error {
|
|
return fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
_, err := client.UpdateEntry(ctx, &filer_pb.UpdateEntryRequest{Directory: dir, Entry: entry})
|
|
return err
|
|
})
|
|
}
|
|
|
|
// ==================== FilerClient Interface ====================
|
|
|
|
func (fs *SftpServer) AdjustedUrl(location *filer_pb.Location) string { return location.Url }
|
|
func (fs *SftpServer) GetDataCenter() string { return fs.dataCenter }
|
|
func (fs *SftpServer) WithFilerClient(streamingMode bool, fn func(filer_pb.SeaweedFilerClient) error) error {
|
|
addr := fs.filerAddr.ToGrpcAddress()
|
|
return pb.WithGrpcClient(streamingMode, util.RandomInt32(), func(conn *grpc.ClientConn) error {
|
|
return fn(filer_pb.NewSeaweedFilerClient(conn))
|
|
}, addr, false, fs.grpcDialOption)
|
|
}
|
|
func (fs *SftpServer) withTimeoutContext(fn func(ctx context.Context) error) error {
|
|
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
|
|
defer cancel()
|
|
return fn(ctx)
|
|
}
|
|
|
|
// ==================== Command Dispatcher ====================
|
|
|
|
func (fs *SftpServer) dispatchCmd(r *sftp.Request) error {
|
|
absPath, err := fs.toAbsolutePath(r.Filepath)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
glog.V(1).Infof("Dispatch: %s %s (absolute: %s)", r.Method, r.Filepath, absPath)
|
|
switch r.Method {
|
|
case "Remove":
|
|
return fs.removeEntry(absPath)
|
|
case "Rename":
|
|
absTarget, err := fs.toAbsolutePath(r.Target)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
return fs.renameEntry(absPath, absTarget)
|
|
case "Mkdir":
|
|
return fs.makeDir(absPath)
|
|
case "Rmdir":
|
|
return fs.removeDir(absPath)
|
|
case "Setstat":
|
|
return fs.setFileStatWithRequest(absPath, r)
|
|
default:
|
|
return fmt.Errorf("unsupported: %s", r.Method)
|
|
}
|
|
}
|
|
|
|
// ==================== File Operations ====================
|
|
|
|
func (fs *SftpServer) readFile(r *sftp.Request) (io.ReaderAt, error) {
|
|
absPath, err := fs.toAbsolutePath(r.Filepath)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if err := fs.checkFilePermission(absPath, "read"); err != nil {
|
|
return nil, err
|
|
}
|
|
entry, err := fs.getEntry(absPath)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return NewSeaweedFileReaderAt(fs, entry), nil
|
|
}
|
|
|
|
func (fs *SftpServer) newFileWriter(r *sftp.Request) (io.WriterAt, error) {
|
|
absPath, err := fs.toAbsolutePath(r.Filepath)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
dir, _ := util.FullPath(absPath).DirAndName()
|
|
if err := fs.checkFilePermission(dir, "write"); err != nil {
|
|
glog.Errorf("Permission denied for %s", dir)
|
|
return nil, err
|
|
}
|
|
// Create a temporary file to buffer writes
|
|
tmpFile, err := os.CreateTemp("", "sftp-upload-*")
|
|
if err != nil {
|
|
return nil, fmt.Errorf("failed to create temp file: %w", err)
|
|
}
|
|
|
|
return &SeaweedSftpFileWriter{
|
|
fs: *fs,
|
|
req: r,
|
|
absPath: absPath,
|
|
tmpFile: tmpFile,
|
|
permissions: 0644,
|
|
uid: fs.user.Uid,
|
|
gid: fs.user.Gid,
|
|
offset: 0,
|
|
}, nil
|
|
}
|
|
|
|
func (fs *SftpServer) removeEntry(absPath string) error {
|
|
return fs.deleteEntry(absPath, false)
|
|
}
|
|
|
|
func (fs *SftpServer) renameEntry(absPath, absTarget string) error {
|
|
if err := fs.checkFilePermission(absPath, "rename"); err != nil {
|
|
return err
|
|
}
|
|
targetDir, _ := util.FullPath(absTarget).DirAndName()
|
|
if err := fs.checkFilePermission(targetDir, "write"); err != nil {
|
|
return err
|
|
}
|
|
oldDir, oldName := util.FullPath(absPath).DirAndName()
|
|
newDir, newName := util.FullPath(absTarget).DirAndName()
|
|
return fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
_, err := client.AtomicRenameEntry(ctx, &filer_pb.AtomicRenameEntryRequest{
|
|
OldDirectory: oldDir, OldName: oldName,
|
|
NewDirectory: newDir, NewName: newName,
|
|
})
|
|
return err
|
|
})
|
|
}
|
|
|
|
func (fs *SftpServer) setFileStatWithRequest(absPath string, r *sftp.Request) error {
|
|
if err := fs.checkFilePermission(absPath, "write"); err != nil {
|
|
return err
|
|
}
|
|
entry, err := fs.getEntry(absPath)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
dir, _ := util.FullPath(absPath).DirAndName()
|
|
// apply attrs
|
|
if r.AttrFlags().Permissions {
|
|
entry.Attributes.FileMode = uint32(r.Attributes().FileMode())
|
|
}
|
|
if r.AttrFlags().UidGid {
|
|
entry.Attributes.Uid = uint32(r.Attributes().UID)
|
|
entry.Attributes.Gid = uint32(r.Attributes().GID)
|
|
}
|
|
if r.AttrFlags().Acmodtime {
|
|
entry.Attributes.Mtime = int64(r.Attributes().Mtime)
|
|
}
|
|
if r.AttrFlags().Size {
|
|
entry.Attributes.FileSize = uint64(r.Attributes().Size)
|
|
}
|
|
return fs.updateEntry(dir, entry)
|
|
}
|
|
|
|
// ==================== Directory Operations ====================
|
|
|
|
func (fs *SftpServer) listDir(r *sftp.Request) (sftp.ListerAt, error) {
|
|
absPath, err := fs.toAbsolutePath(r.Filepath)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if err := fs.checkFilePermission(absPath, "list"); err != nil {
|
|
return nil, err
|
|
}
|
|
if r.Method == "Stat" || r.Method == "Lstat" {
|
|
entry, err := fs.getEntry(absPath)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
fi := &EnhancedFileInfo{FileInfo: FileInfoFromEntry(entry), uid: entry.Attributes.Uid, gid: entry.Attributes.Gid}
|
|
return listerat([]os.FileInfo{fi}), nil
|
|
}
|
|
return fs.listAllPages(absPath)
|
|
}
|
|
|
|
func (fs *SftpServer) listAllPages(dirPath string) (sftp.ListerAt, error) {
|
|
var all []os.FileInfo
|
|
last := ""
|
|
for {
|
|
page, err := fs.fetchDirectoryPage(dirPath, last)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
all = append(all, page...)
|
|
if len(page) < defaultListLimit {
|
|
break
|
|
}
|
|
last = page[len(page)-1].Name()
|
|
}
|
|
return listerat(all), nil
|
|
}
|
|
|
|
func (fs *SftpServer) fetchDirectoryPage(dirPath, start string) ([]os.FileInfo, error) {
|
|
var list []os.FileInfo
|
|
err := fs.callWithClient(true, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
stream, err := client.ListEntries(ctx, &filer_pb.ListEntriesRequest{Directory: dirPath, StartFromFileName: start, Limit: defaultListLimit})
|
|
if err != nil {
|
|
return err
|
|
}
|
|
for {
|
|
r, err := stream.Recv()
|
|
if err == io.EOF {
|
|
break
|
|
}
|
|
if err != nil || r.Entry == nil {
|
|
continue
|
|
}
|
|
p := path.Join(dirPath, r.Entry.Name)
|
|
if err := fs.checkFilePermission(p, "list"); err != nil {
|
|
continue
|
|
}
|
|
list = append(list, &EnhancedFileInfo{FileInfo: FileInfoFromEntry(r.Entry), uid: r.Entry.Attributes.Uid, gid: r.Entry.Attributes.Gid})
|
|
}
|
|
return nil
|
|
})
|
|
return list, err
|
|
}
|
|
|
|
// makeDir creates a new directory with proper permissions.
|
|
func (fs *SftpServer) makeDir(absPath string) error {
|
|
if fs.user == nil {
|
|
return fmt.Errorf("cannot create directory: no user info")
|
|
}
|
|
dir, name := util.FullPath(absPath).DirAndName()
|
|
if err := fs.checkFilePermission(dir, "write"); err != nil {
|
|
return err
|
|
}
|
|
// default mode and ownership
|
|
err := filer_pb.Mkdir(context.Background(), fs, string(dir), name, func(entry *filer_pb.Entry) {
|
|
mode := uint32(0755 | os.ModeDir)
|
|
// Defensive check: all paths should be under HomeDir after toAbsolutePath translation
|
|
if absPath == fs.user.HomeDir || strings.HasPrefix(absPath, fs.user.HomeDir+"/") {
|
|
mode = uint32(0700 | os.ModeDir)
|
|
}
|
|
entry.Attributes.FileMode = mode
|
|
entry.Attributes.Uid = fs.user.Uid
|
|
entry.Attributes.Gid = fs.user.Gid
|
|
now := time.Now().Unix()
|
|
entry.Attributes.Crtime = now
|
|
entry.Attributes.Mtime = now
|
|
if entry.Extended == nil {
|
|
entry.Extended = make(map[string][]byte)
|
|
}
|
|
entry.Extended["creator"] = []byte(fs.user.Username)
|
|
})
|
|
return err
|
|
}
|
|
|
|
// removeDir deletes a directory.
|
|
func (fs *SftpServer) removeDir(absPath string) error {
|
|
return fs.deleteEntry(absPath, false)
|
|
}
|
|
|
|
func (fs *SftpServer) putFile(filepath string, reader io.Reader, user *user.User) error {
|
|
dir, filename := util.FullPath(filepath).DirAndName()
|
|
uploadUrl := fmt.Sprintf("http://%s%s", fs.filerAddr, filepath)
|
|
|
|
// Compute MD5 while uploading
|
|
hash := md5.New()
|
|
body := io.TeeReader(reader, hash)
|
|
|
|
// We can skip ContentLength if unknown (chunked transfer encoding)
|
|
req, err := http.NewRequest(http.MethodPut, uploadUrl, body)
|
|
if err != nil {
|
|
return fmt.Errorf("create request: %w", err)
|
|
}
|
|
req.Header.Set("Content-Type", "application/octet-stream")
|
|
|
|
resp, err := http.DefaultClient.Do(req)
|
|
if err != nil {
|
|
return fmt.Errorf("upload to filer: %w", err)
|
|
}
|
|
defer resp.Body.Close()
|
|
|
|
respBody, err := io.ReadAll(resp.Body)
|
|
if err != nil {
|
|
return fmt.Errorf("read response: %w", err)
|
|
}
|
|
|
|
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated {
|
|
return fmt.Errorf("upload failed with status %d: %s", resp.StatusCode, string(respBody))
|
|
}
|
|
|
|
var result weed_server.FilerPostResult
|
|
if err := json.Unmarshal(respBody, &result); err != nil {
|
|
return fmt.Errorf("parse response: %w", err)
|
|
}
|
|
if result.Error != "" {
|
|
return fmt.Errorf("filer error: %s", result.Error)
|
|
}
|
|
// Update file ownership using the same pattern as other functions
|
|
if user != nil {
|
|
err := fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
// Look up the file to get its current entry
|
|
lookupResp, err := client.LookupDirectoryEntry(ctx, &filer_pb.LookupDirectoryEntryRequest{
|
|
Directory: dir,
|
|
Name: filename,
|
|
})
|
|
if err != nil {
|
|
return fmt.Errorf("lookup file for attribute update: %w", err)
|
|
}
|
|
|
|
if lookupResp.Entry == nil {
|
|
return fmt.Errorf("file not found after upload: %s/%s", dir, filename)
|
|
}
|
|
|
|
// Update the entry with new uid/gid
|
|
entry := lookupResp.Entry
|
|
entry.Attributes.Uid = user.Uid
|
|
entry.Attributes.Gid = user.Gid
|
|
|
|
// Update the entry in the filer
|
|
_, err = client.UpdateEntry(ctx, &filer_pb.UpdateEntryRequest{
|
|
Directory: dir,
|
|
Entry: entry,
|
|
})
|
|
return err
|
|
})
|
|
|
|
if err != nil {
|
|
// Log the error but don't fail the whole operation
|
|
glog.Errorf("Failed to update file ownership for %s: %v", filepath, err)
|
|
}
|
|
}
|
|
|
|
return nil
|
|
}
|
|
|
|
// ==================== Common Arguments Helpers ====================
|
|
|
|
func FileInfoFromEntry(e *filer_pb.Entry) FileInfo {
|
|
return FileInfo{name: e.Name, size: int64(e.Attributes.FileSize), mode: os.FileMode(e.Attributes.FileMode), modTime: time.Unix(e.Attributes.Mtime, 0), isDir: e.IsDirectory}
|
|
}
|
|
|
|
func (fs *SftpServer) deleteEntry(p string, recursive bool) error {
|
|
if err := fs.checkFilePermission(p, "delete"); err != nil {
|
|
return err
|
|
}
|
|
dir, name := util.FullPath(p).DirAndName()
|
|
return fs.callWithClient(false, func(ctx context.Context, client filer_pb.SeaweedFilerClient) error {
|
|
r, err := client.DeleteEntry(ctx, &filer_pb.DeleteEntryRequest{Directory: dir, Name: name, IsDeleteData: true, IsRecursive: recursive})
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if r.Error != "" {
|
|
return fmt.Errorf("%s", r.Error)
|
|
}
|
|
return nil
|
|
})
|
|
}
|
|
|
|
// ==================== Custom Types ====================
|
|
|
|
type EnhancedFileInfo struct {
|
|
FileInfo
|
|
uid uint32
|
|
gid uint32
|
|
}
|
|
|
|
// FileStat represents file statistics in a platform-independent way
|
|
type FileStat struct {
|
|
Uid uint32
|
|
Gid uint32
|
|
}
|
|
|
|
func (fi *EnhancedFileInfo) Sys() interface{} {
|
|
return &FileStat{Uid: fi.uid, Gid: fi.gid}
|
|
}
|
|
|
|
func (fi *EnhancedFileInfo) Owner() (uid, gid int) {
|
|
return int(fi.uid), int(fi.gid)
|
|
}
|
|
|
|
func (fs *SftpServer) checkFilePermission(filepath string, permissions string) error {
|
|
return fs.CheckFilePermission(filepath, permissions)
|
|
}
|