diff --git a/other/java/client/src/main/proto/filer.proto b/other/java/client/src/main/proto/filer.proto index 1515c169a..a327bdb17 100644 --- a/other/java/client/src/main/proto/filer.proto +++ b/other/java/client/src/main/proto/filer.proto @@ -527,6 +527,10 @@ message AtomicRenameEntryRequest { string new_directory = 3; string new_name = 4; repeated int32 signatures = 5; + // When true, the rename fails with ErrEntryAlreadyExists if the target + // already exists. Lets a caller that authorized the rename without delete + // access on an absent target stay safe if one is created concurrently. + bool fail_if_target_exists = 6; } message AtomicRenameEntryResponse { diff --git a/weed/pb/filer.proto b/weed/pb/filer.proto index 0abe60c55..dca77d2ec 100644 --- a/weed/pb/filer.proto +++ b/weed/pb/filer.proto @@ -535,6 +535,10 @@ message AtomicRenameEntryRequest { string new_directory = 3; string new_name = 4; repeated int32 signatures = 5; + // When true, the rename fails with ErrEntryAlreadyExists if the target + // already exists. Lets a caller that authorized the rename without delete + // access on an absent target stay safe if one is created concurrently. + bool fail_if_target_exists = 6; } message AtomicRenameEntryResponse { diff --git a/weed/pb/filer_pb/filer.pb.go b/weed/pb/filer_pb/filer.pb.go index 0c9190f81..94a21acc9 100644 --- a/weed/pb/filer_pb/filer.pb.go +++ b/weed/pb/filer_pb/filer.pb.go @@ -2921,14 +2921,18 @@ func (x *DeleteEntryResponse) GetMetadataEvent() *SubscribeMetadataResponse { } type AtomicRenameEntryRequest struct { - state protoimpl.MessageState `protogen:"open.v1"` - OldDirectory string `protobuf:"bytes,1,opt,name=old_directory,json=oldDirectory,proto3" json:"old_directory,omitempty"` - OldName string `protobuf:"bytes,2,opt,name=old_name,json=oldName,proto3" json:"old_name,omitempty"` - NewDirectory string `protobuf:"bytes,3,opt,name=new_directory,json=newDirectory,proto3" json:"new_directory,omitempty"` - NewName string `protobuf:"bytes,4,opt,name=new_name,json=newName,proto3" json:"new_name,omitempty"` - Signatures []int32 `protobuf:"varint,5,rep,packed,name=signatures,proto3" json:"signatures,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + state protoimpl.MessageState `protogen:"open.v1"` + OldDirectory string `protobuf:"bytes,1,opt,name=old_directory,json=oldDirectory,proto3" json:"old_directory,omitempty"` + OldName string `protobuf:"bytes,2,opt,name=old_name,json=oldName,proto3" json:"old_name,omitempty"` + NewDirectory string `protobuf:"bytes,3,opt,name=new_directory,json=newDirectory,proto3" json:"new_directory,omitempty"` + NewName string `protobuf:"bytes,4,opt,name=new_name,json=newName,proto3" json:"new_name,omitempty"` + Signatures []int32 `protobuf:"varint,5,rep,packed,name=signatures,proto3" json:"signatures,omitempty"` + // When true, the rename fails with ErrEntryAlreadyExists if the target + // already exists. Lets a caller that authorized the rename without delete + // access on an absent target stay safe if one is created concurrently. + FailIfTargetExists bool `protobuf:"varint,6,opt,name=fail_if_target_exists,json=failIfTargetExists,proto3" json:"fail_if_target_exists,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *AtomicRenameEntryRequest) Reset() { @@ -2996,6 +3000,13 @@ func (x *AtomicRenameEntryRequest) GetSignatures() []int32 { return nil } +func (x *AtomicRenameEntryRequest) GetFailIfTargetExists() bool { + if x != nil { + return x.FailIfTargetExists + } + return false +} + type AtomicRenameEntryResponse struct { state protoimpl.MessageState `protogen:"open.v1"` unknownFields protoimpl.UnknownFields @@ -7544,7 +7555,7 @@ const file_filer_proto_rawDesc = "" + " \x01(\bR\x10keepRemoteObject\"w\n" + "\x13DeleteEntryResponse\x12\x14\n" + "\x05error\x18\x01 \x01(\tR\x05error\x12J\n" + - "\x0emetadata_event\x18\x02 \x01(\v2#.filer_pb.SubscribeMetadataResponseR\rmetadataEvent\"\xba\x01\n" + + "\x0emetadata_event\x18\x02 \x01(\v2#.filer_pb.SubscribeMetadataResponseR\rmetadataEvent\"\xed\x01\n" + "\x18AtomicRenameEntryRequest\x12#\n" + "\rold_directory\x18\x01 \x01(\tR\foldDirectory\x12\x19\n" + "\bold_name\x18\x02 \x01(\tR\aoldName\x12#\n" + @@ -7552,7 +7563,8 @@ const file_filer_proto_rawDesc = "" + "\bnew_name\x18\x04 \x01(\tR\anewName\x12\x1e\n" + "\n" + "signatures\x18\x05 \x03(\x05R\n" + - "signatures\"\x1b\n" + + "signatures\x121\n" + + "\x15fail_if_target_exists\x18\x06 \x01(\bR\x12failIfTargetExists\"\x1b\n" + "\x19AtomicRenameEntryResponse\"\xba\x01\n" + "\x18StreamRenameEntryRequest\x12#\n" + "\rold_directory\x18\x01 \x01(\tR\foldDirectory\x12\x19\n" + diff --git a/weed/server/filer_grpc_server_rename.go b/weed/server/filer_grpc_server_rename.go index c55ddd46b..a1a6c682c 100644 --- a/weed/server/filer_grpc_server_rename.go +++ b/weed/server/filer_grpc_server_rename.go @@ -61,6 +61,18 @@ func (fs *FilerServer) AtomicRenameEntry(ctx context.Context, req *filer_pb.Atom return nil, fmt.Errorf("%s/%s not found: %v", req.OldDirectory, req.OldName, err) } + // The check runs under the rename path locks, which CreateEntry on the same + // filer also takes, so a target created concurrently cannot slip through. + if req.FailIfTargetExists && newParent.Child(req.NewName) != oldParent.Child(req.OldName) { + if _, findErr := fs.filer.FindEntry(ctx, newParent.Child(req.NewName)); findErr == nil { + fs.filer.RollbackTransaction(ctx) + return nil, fmt.Errorf("%s/%s: %w", req.NewDirectory, req.NewName, filer_pb.ErrEntryAlreadyExists) + } else if findErr != filer_pb.ErrNotFound { + fs.filer.RollbackTransaction(ctx) + return nil, findErr + } + } + var metadataEvents []metadataEvent var pendingChunkDeletes []*filer_pb.FileChunk var pendingRemoteCopies []*pendingRemoteCopy diff --git a/weed/sftpd/sftp_filer.go b/weed/sftpd/sftp_filer.go index 1262b75d8..70ff8669b 100644 --- a/weed/sftpd/sftp_filer.go +++ b/weed/sftpd/sftp_filer.go @@ -5,6 +5,7 @@ import ( "context" "crypto/md5" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -263,17 +264,148 @@ func (fs *SftpServer) renameEntry(absPath, absTarget string) error { if err := fs.checkFilePermission(targetDir, "write"); err != nil { return err } + requireAbsentTarget := false + if path.Clean(absPath) != path.Clean(absTarget) { + var err error + requireAbsentTarget, err = fs.checkRenameTargetDelete(absTarget, targetDir) + if err != nil { + return err + } + } + if err := fs.checkRenameKeepsAcl(absPath, absTarget); 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, + // A target the user may not delete must stay absent: the filer + // checks it again under the rename locks, where a create cannot + // slip in. + FailIfTargetExists: requireAbsentTarget, }) return err }) } +// checkRenameTargetDelete enforces delete access where a rename can destroy an +// entry. Overwriting an existing target deletes it, so it needs delete access +// on the target itself. A name that does not exist yet needs no delete check +// unless a configured ACL restricts that name itself: enforcing such an ACL +// even while the entry is absent keeps a concurrent create from slipping a +// protected target under the rename. When the target name simply inherits its +// directory's ACL and the user could not delete an entry there, the rename is +// still allowed for a new name but the caller is told to require the target to +// stay absent, so the filer rejects the overwrite of one created in the gap. +// Lookup failures other than not-found fail closed. +func (fs *SftpServer) checkRenameTargetDelete(absTarget, targetDir string) (requireAbsentTarget bool, err error) { + if _, err := fs.getEntry(absTarget); err == nil { + return false, fs.checkFilePermission(absTarget, "delete") + } else if !errors.Is(err, os.ErrNotExist) { + return false, err + } + if fs.user == nil || fs.user.Username == "root" || fs.user.Uid == 0 { + return false, nil + } + if matchingAclPrefix(fs.user, absTarget) != matchingAclPrefix(fs.user, targetDir) { + if !HasExplicitPermission(fs.user, absTarget, PermDelete, false) { + glog.V(0).Infof("permission denied for user %s: rename target %s carries a delete-protected ACL", fs.user.Username, absTarget) + return false, os.ErrPermission + } + return false, nil + } + if err := fs.checkFilePermission(absTarget, "delete"); err != nil { + if errors.Is(err, os.ErrPermission) { + return true, nil + } + return false, err + } + return false, nil +} + +// matchingAclPrefix returns the longest configured ACL path that applies to p, +// or "" when no configured path covers it. +func matchingAclPrefix(user *user.User, p string) string { + best := "" + for aclPath := range user.Permissions { + cleaned := path.Clean(aclPath) + if pathWithin(cleaned, p) && len(cleaned) > len(best) { + best = cleaned + } + } + return best +} + +// checkRenameKeepsAcl denies a rename that would move entries to locations +// where ACLs grant the user read, list, write, or delete access the source +// side lacked. It compares the moved root and every configured ACL path under +// either the source or the target (mapped back to the source) so both +// inherited restrictions and deeper destination grants are caught. +func (fs *SftpServer) checkRenameKeepsAcl(absPath, absTarget string) error { + if fs.user == nil || fs.user.Username == "root" || fs.user.Uid == 0 { + return nil + } + type aclPair struct { + src, dst string + isDir bool + } + srcEntry, err := fs.getEntry(absPath) + if err != nil { + return err + } + pairs := []aclPair{{absPath, absTarget, srcEntry.IsDirectory}} + seen := map[string]bool{absPath: true} + for p := range fs.user.Permissions { + cleaned := path.Clean(p) + var src string + switch { + case pathWithin(absPath, cleaned): + src = cleaned + case pathWithin(absTarget, cleaned): + src = path.Join(absPath, strings.TrimPrefix(cleaned, absTarget)) + default: + continue + } + if seen[src] { + continue + } + seen[src] = true + // A rule only protects an entry that actually moves; a path reserved + // for a missing child must not block the rename. A lookup error is not + // proof that the child is missing, so only os.ErrNotExist may skip. + entry, err := fs.getEntry(src) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + return err + } + relocated := path.Join(absTarget, strings.TrimPrefix(src, absPath)) + pairs = append(pairs, aclPair{src, relocated, entry.IsDirectory}) + } + for _, pair := range pairs { + // A moved directory carries file children whose ACLs resolve with + // isDirectory=false, where list no longer counts as read. Comparing + // only the directory shape would miss children gaining read, so a + // directory is checked in the file shape too. + shapes := []bool{pair.isDir} + if pair.isDir { + shapes = append(shapes, false) + } + for _, isDir := range shapes { + for _, perm := range []string{PermRead, PermList, PermWrite, PermDelete} { + if !HasExplicitPermission(fs.user, pair.src, perm, isDir) && HasExplicitPermission(fs.user, pair.dst, perm, isDir) { + glog.V(0).Infof("permission denied for user %s: rename of %s to %s would drop the ACL on %s", fs.user.Username, absPath, absTarget, pair.src) + return os.ErrPermission + } + } + } + } + return nil +} + func (fs *SftpServer) setFileStatWithRequest(absPath string, r *sftp.Request) error { if err := fs.checkFilePermission(absPath, "write"); err != nil { return err diff --git a/weed/sftpd/sftp_permissions_test.go b/weed/sftpd/sftp_permissions_test.go index ae0c69355..e9dbc7013 100644 --- a/weed/sftpd/sftp_permissions_test.go +++ b/weed/sftpd/sftp_permissions_test.go @@ -2,6 +2,7 @@ package sftpd import ( "context" + "errors" "net" "os" "testing" @@ -136,6 +137,96 @@ func TestNewFileWriterChecksTargetFile(t *testing.T) { } } +// 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. +func TestRenameEntryChecksTargetFile(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/own.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + "/home/alice/ok.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + "/home/alice/locked.txt": {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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/locked.txt": {"read", "list"}, + }, + }, + } + + renameDenied := func(src, dst string) bool { + return errors.Is(fs.renameEntry(src, dst), os.ErrPermission) + } + + if !renameDenied("/home/alice/own.txt", "/home/alice/locked.txt") { + t.Error("expected rename over a read-only file to be denied") + } + if err := fs.renameEntry("/home/alice/own.txt", "/home/alice/ok.txt"); err != nil { + t.Errorf("expected rename over a writable file to succeed, got %v", err) + } + if err := fs.renameEntry("/home/alice/ok.txt", "/home/alice/new.txt"); err != nil { + t.Errorf("expected rename onto a new path in a writable dir to succeed, got %v", err) + } + if err := fs.renameEntry("/home/alice/new.txt", "/home/alice/new.txt"); err != nil { + t.Errorf("expected a no-op rename to skip the target delete check, got %v", err) + } +} + +// Renaming a directory must not strand a more specific ACL configured under +// it: after /sub moves to /sub2, /sub/ro.txt no longer matches its read-only +// ACL, so the move is denied unless the destination grants no new access. +func TestRenameEntryKeepsNestedAcl(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub/ro.txt": {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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/sub/ro.txt": {"read", "list"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/sub", "/home/alice/sub2"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename that strands the /sub/ro.txt ACL to be denied, got %v", err) + } + delete(fs.user.Permissions, "/home/alice/sub/ro.txt") + if err := fs.renameEntry("/home/alice/sub", "/home/alice/sub2"); err != nil { + t.Errorf("expected rename without a nested carve-out to succeed, got %v", err) + } +} + func TestCheckFilePermissionMissingAncestorsTerminates(t *testing.T) { lis, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { @@ -160,12 +251,304 @@ func TestCheckFilePermissionMissingAncestorsTerminates(t *testing.T) { type permissionTestFiler struct { filer_pb.UnimplementedSeaweedFilerServer - entries map[string]*filer_pb.Entry + entries map[string]*filer_pb.Entry + lookupErr map[string]error + // createBeforeRename inserts an entry at the rename target inside the RPC, + // simulating a file a concurrent connection creates after the checks ran. + createBeforeRename map[string]*filer_pb.Entry } func (f *permissionTestFiler) LookupDirectoryEntry(_ context.Context, req *filer_pb.LookupDirectoryEntryRequest) (*filer_pb.LookupDirectoryEntryResponse, error) { - if entry, ok := f.entries[req.Directory+"/"+req.Name]; ok { + key := req.Directory + "/" + req.Name + if err, ok := f.lookupErr[key]; ok { + return nil, err + } + if entry, ok := f.entries[key]; ok { return &filer_pb.LookupDirectoryEntryResponse{Entry: entry}, nil } return nil, status.Error(codes.NotFound, "no entry is found") } + +func (f *permissionTestFiler) AtomicRenameEntry(_ context.Context, req *filer_pb.AtomicRenameEntryRequest) (*filer_pb.AtomicRenameEntryResponse, error) { + oldKey := req.OldDirectory + "/" + req.OldName + entry, ok := f.entries[oldKey] + if !ok { + return nil, status.Error(codes.NotFound, "no entry is found") + } + newKey := req.NewDirectory + "/" + req.NewName + if created, ok := f.createBeforeRename[newKey]; ok { + f.entries[newKey] = created + } + if req.FailIfTargetExists { + if _, exists := f.entries[newKey]; exists && newKey != oldKey { + return nil, status.Error(codes.AlreadyExists, "entry already exists") + } + } + delete(f.entries, oldKey) + f.entries[newKey] = entry + return &filer_pb.AtomicRenameEntryResponse{}, nil +} + +// checkRenameKeepsAcl also compares the moved root (inherited restrictions) and +// ACL paths configured under the destination (deeper grants), while configured +// paths without a real entry do not block the rename. +func TestRenameEntryKeepsInheritedAndDestinationAcl(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/restricted": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/restricted/sub": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub/locked": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub/locked/secret": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + "/home/alice/sub3": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + }} + 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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/restricted": {"read", "list", "rename"}, + "/home/alice/sub/locked": {"read", "list"}, + "/home/alice/sub2/locked": {"read", "list"}, + "/home/alice/sub2/locked/secret": {"read", "write", "list", "delete"}, + "/home/alice/sub3/ghost.txt": {"read", "list"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/restricted/sub", "/home/alice/sub5"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename escaping the inherited restricted ACL to be denied, got %v", err) + } + if err := fs.renameEntry("/home/alice/sub", "/home/alice/sub2"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename into a deeper destination grant to be denied, got %v", err) + } + if err := fs.renameEntry("/home/alice/sub3", "/home/alice/sub4"); err != nil { + t.Errorf("expected rename with a missing reserved ACL child to succeed, got %v", err) + } +} + +// A rename onto a name that does not exist needs no delete access unless a +// configured ACL restricts that name itself. Without the scoped check a user +// holding write+rename but not delete could no longer move files to new names. +func TestRenameEntryNewNameNeedsNoDelete(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/own.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + "/home/alice/existing.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + "/home/alice/removable.txt": {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": {"read", "write", "list", "rename"}, + "/home/alice/reserved.txt": {"read", "list"}, + "/home/alice/managed.txt": {"read", "write", "list", "delete"}, + "/home/alice/removable.txt": {"read", "write", "list", "delete", "rename"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/own.txt", "/home/alice/new.txt"); err != nil { + t.Errorf("expected rename to a new name without delete access to succeed, got %v", err) + } + if err := fs.renameEntry("/home/alice/new.txt", "/home/alice/existing.txt"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename over an existing file without delete access to be denied, got %v", err) + } + if err := fs.renameEntry("/home/alice/new.txt", "/home/alice/reserved.txt"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename onto a delete-protected configured name to be denied, got %v", err) + } + if err := fs.renameEntry("/home/alice/removable.txt", "/home/alice/managed.txt"); err != nil { + t.Errorf("expected rename onto a configured name that grants delete to succeed, got %v", err) + } +} + +// checkRenameKeepsAcl fails closed: a lookup error on a configured ACL path is +// not proof that the entry is missing, so it must stop the rename instead of +// skipping the comparison. +func TestRenameEntryFailsClosedOnLookupError(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/sub/ro.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + }, + lookupErr: map[string]error{ + "/home/alice/sub/ro.txt": status.Error(codes.Unavailable, "filer unavailable"), + }, + } + 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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/sub/ro.txt": {"read", "list"}, + }, + }, + } + + err = fs.renameEntry("/home/alice/sub", "/home/alice/sub2") + if err == nil || errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename to fail closed on a lookup error, got %v", err) + } + delete(filer.lookupErr, "/home/alice/sub/ro.txt") + if err := fs.renameEntry("/home/alice/sub", "/home/alice/sub2"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected the stranded ACL check to deny the rename once the lookup succeeds, got %v", err) + } +} + +// A rename must not grant access the source side lacked, including read and +// directory listing: moving a subtree whose ACL withholds read/list into a +// readable destination would expose its contents. +func TestRenameEntryKeepsReadAndListAcl(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/noread": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/noread/f.txt": {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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/noread": {"write", "delete", "rename"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/noread", "/home/alice/visible"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename that grants read/list on the moved directory to be denied, got %v", err) + } +} + +// A moved directory carries file children whose ACLs resolve with +// isDirectory=false, where list does not count as read. Moving a directory +// that grants list but not read into a readable destination must be denied: +// its file children would become readable even though the directory check +// passes. +func TestRenameEntryKeepsFileChildAcl(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/private": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/private/secret.txt": {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": {"read", "write", "list", "delete", "rename"}, + "/home/alice/private": {"list", "write", "delete", "rename"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/private", "/home/alice/public"); !errors.Is(err, os.ErrPermission) { + t.Errorf("expected rename that grants read on file children to be denied, got %v", err) + } + fs.user.Permissions["/home/alice/public"] = []string{"list", "write", "delete", "rename"} + if err := fs.renameEntry("/home/alice/private", "/home/alice/public"); err != nil { + t.Errorf("expected rename onto an equally restricted ACL to succeed, got %v", err) + } +} + +// A rename onto a new name needs no delete access, but a target created in the +// gap between the check and the move must not be silently overwritten: the +// filer is asked to fail the rename when the target appears. +func TestRenameEntryRejectsConcurrentTarget(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{ + "/home/alice": {IsDirectory: true, Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0755}}, + "/home/alice/own.txt": {Attributes: &filer_pb.FuseAttributes{Uid: 1000, FileMode: 0644}}, + }, + createBeforeRename: map[string]*filer_pb.Entry{ + "/home/alice/taken.txt": {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": {"read", "write", "list", "rename"}, + }, + }, + } + + if err := fs.renameEntry("/home/alice/own.txt", "/home/alice/free.txt"); err != nil { + t.Errorf("expected rename to a name that stays absent to succeed, got %v", err) + } + if err := fs.renameEntry("/home/alice/free.txt", "/home/alice/taken.txt"); err == nil { + t.Error("expected rename to fail when the target appears concurrently and the user lacks delete access") + } +}