diff --git a/weed/sftpd/sftp_copy_existing_test.go b/weed/sftpd/sftp_copy_existing_test.go index 1597d5d8b4..85c45b8466 100644 --- a/weed/sftpd/sftp_copy_existing_test.go +++ b/weed/sftpd/sftp_copy_existing_test.go @@ -227,3 +227,27 @@ func TestRequireParentDir(t *testing.T) { t.Fatalf("file as parent: got %v, want os.ErrNotExist", err) } } + +// rm takes files and rmdir directories; a missing path is an error for both. +func TestRequireEntry(t *testing.T) { + fs := newCopyTestServer(t, "", map[string]*filer_pb.Entry{ + "/home/d": {Name: "d", IsDirectory: true}, + "/home/d/a.txt": {Name: "a.txt"}, + }) + + if err := fs.requireEntry("/home/d/a.txt", false); err != nil { + t.Fatalf("rm of a file: %v", err) + } + if err := fs.requireEntry("/home/d", true); err != nil { + t.Fatalf("rmdir of a directory: %v", err) + } + if err := fs.requireEntry("/home/d/missing", false); err != os.ErrNotExist { + t.Fatalf("rm of a missing path: got %v, want os.ErrNotExist", err) + } + if err := fs.requireEntry("/home/d/a.txt", true); err != os.ErrNotExist { + t.Fatalf("rmdir of a file: got %v, want os.ErrNotExist", err) + } + if err := fs.requireEntry("/home/d", false); err == nil || err == os.ErrNotExist { + t.Fatalf("rm of a directory: got %v, want a failure", err) + } +} diff --git a/weed/sftpd/sftp_filer.go b/weed/sftpd/sftp_filer.go index f173eddb0a..2f2a2b59ba 100644 --- a/weed/sftpd/sftp_filer.go +++ b/weed/sftpd/sftp_filer.go @@ -256,6 +256,9 @@ func writeZeros(w io.Writer, size int64) error { } func (fs *SftpServer) removeEntry(absPath string) error { + if err := fs.requireEntry(absPath, false); err != nil { + return err + } return fs.deleteEntry(absPath, false) } @@ -523,7 +526,7 @@ func (fs *SftpServer) makeDir(absPath string) error { return err } // default mode and ownership - err := filer_pb.Mkdir(context.Background(), fs, string(dir), name, func(entry *filer_pb.Entry) { + err := filer_pb.MkdirExclusive(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+"/") { @@ -540,6 +543,9 @@ func (fs *SftpServer) makeDir(absPath string) error { } entry.Extended["creator"] = []byte(fs.user.Username) }) + if errors.Is(err, filer_pb.ErrEntryAlreadyExists) { + return os.ErrExist + } return err } @@ -562,9 +568,29 @@ func (fs *SftpServer) requireParentDir(absPath string) error { // removeDir deletes a directory. func (fs *SftpServer) removeDir(absPath string) error { + if err := fs.requireEntry(absPath, true); err != nil { + return err + } return fs.deleteEntry(absPath, false) } +// requireEntry fails unless absPath exists and is a directory exactly when +// isDir is set (rm takes files, rmdir directories), with the errors OpenSSH's +// sftp-server gives: no such file for a missing path or rmdir of a file. +func (fs *SftpServer) requireEntry(absPath string, isDir bool) error { + entry, err := fs.getEntry(absPath) + if err != nil { + return err + } + if isDir && !entry.IsDirectory { + return os.ErrNotExist + } + if !isDir && entry.IsDirectory { + return fmt.Errorf("%s is a directory", absPath) + } + return nil +} + func (fs *SftpServer) putFile(filepath string, reader io.Reader, user *user.User) error { dir, filename := util.FullPath(filepath).DirAndName() // Escape the path so a "?" in the filename cannot inject filer query commands like cp.from/mv.from. diff --git a/weed/sftpd/sftp_permissions_test.go b/weed/sftpd/sftp_permissions_test.go index 8dff396299..48ebcf3772 100644 --- a/weed/sftpd/sftp_permissions_test.go +++ b/weed/sftpd/sftp_permissions_test.go @@ -210,6 +210,51 @@ func TestRequireParentDirEnforced(t *testing.T) { } } +// mkdir of a taken name fails instead of replacing the entry, file or +// directory alike: the create is exclusive, so a concurrent creator wins. +func TestMakeDirExistingPath(t *testing.T) { + lis, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + filer := &permissionTestFiler{entries: map[string]*filer_pb.Entry{ + "/": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}}, + "/home": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{FileMode: 0755}}, + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/d": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/a.t": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + }} + grpcServer := grpc.NewServer() + filer_pb.RegisterSeaweedFilerServer(grpcServer, filer) + go func() { _ = grpcServer.Serve(lis) }() + t.Cleanup(grpcServer.Stop) + + _, port, _ := net.SplitHostPort(lis.Addr().String()) + fs := &SftpServer{ + filerAddr: pb.ServerAddress("127.0.0.1:0." + port), + grpcDialOption: grpc.WithTransportCredentials(insecure.NewCredentials()), + user: &user.User{ + Username: "alice", Uid: 1000, Gid: 1000, HomeDir: "/home/alice", + Permissions: map[string][]string{ + "/home/alice": {"*"}, + }, + }, + } + + if err := fs.makeDir("/home/alice/d"); err != os.ErrExist { + t.Fatalf("mkdir of an existing directory: got %v, want os.ErrExist", err) + } + if err := fs.makeDir("/home/alice/a.t"); err != os.ErrExist { + t.Fatalf("mkdir of an existing file: got %v, want os.ErrExist", err) + } + if got := filer.entries["/home/alice/d"].Attributes.FileMode; got != 0755 { + t.Errorf("existing directory was overwritten: mode %o", got) + } + if filer.entries["/home/alice/a.t"].IsDirectory { + t.Error("existing file was overwritten by a directory") + } +} + // renameEntry must honor a more specific ACL on an existing target: renaming // over a read-only file deletes it, so it requires delete permission on the // target file itself, not only write permission on the target directory. @@ -363,7 +408,16 @@ func (f *permissionTestFiler) AtomicRenameEntry(_ context.Context, req *filer_pb } func (f *permissionTestFiler) CreateEntry(_ context.Context, req *filer_pb.CreateEntryRequest) (*filer_pb.CreateEntryResponse, error) { - f.entries[req.Directory+"/"+req.Entry.Name] = req.Entry + key := req.Directory + "/" + req.Entry.Name + if req.OExcl { + if _, ok := f.entries[key]; ok { + return &filer_pb.CreateEntryResponse{ + Error: "entry already exists", + ErrorCode: filer_pb.FilerError_ENTRY_ALREADY_EXISTS, + }, nil + } + } + f.entries[key] = req.Entry return &filer_pb.CreateEntryResponse{}, nil }