diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index 0a070e3eb5cd0..284854bebf491 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -206,7 +206,14 @@ func TestRootHostUsersBackend(t *testing.T) { }) t.Run("Test CreateHomeDirectory recursively takes ownership of existing directory", func(t *testing.T) { + getOwnerUID := func(path string) uint32 { + info, err := os.Stat(path) + require.NoError(t, err) + return info.Sys().(*syscall.Stat_t).Uid + } + otheruser := "other-user" + t.Cleanup(func() { cleanupUsersAndGroups([]string{testuser, otheruser}, nil) }) err := usersbk.CreateUser(testuser, nil, host.UserOpts{}) require.NoError(t, err) @@ -229,19 +236,42 @@ func TestRootHostUsersBackend(t *testing.T) { }) require.NoError(t, err) - info, err := os.Stat(testHome) - require.NoError(t, err) - initialOwnerUID := info.Sys().(*syscall.Stat_t).Uid + // add a file owned by root that _shouldn't_ be owned at the end + require.NoError(t, os.WriteFile(filepath.Join(testHome, "rootfile"), []byte("test\n"), 0o700)) + + // grab initial owners + initialRootFileOwner := getOwnerUID(filepath.Join(testHome, "rootfile")) + initialHomeOwner := getOwnerUID(testHome) + initialTestFileOwner := getOwnerUID(filepath.Join(testHome, "testfile")) + // don't take ownership when the user still exists err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid) require.ErrorIs(t, err, os.ErrExist) - info, err = os.Stat(testHome) - require.NoError(t, err) - finalOwnerUID := info.Sys().(*syscall.Stat_t).Uid + finalHomeOwner := getOwnerUID(testHome) + finalTestFileOwner := getOwnerUID(filepath.Join(testHome, "testfile")) + finalRootFileOwner := getOwnerUID(filepath.Join(testHome, "rootfile")) + + require.Equal(t, initialRootFileOwner, finalRootFileOwner) // "rootfile" ownership unchanged + require.Equal(t, initialHomeOwner, finalHomeOwner) // initial owner still owns directory + require.Equal(t, initialTestFileOwner, finalTestFileOwner) // initial owner still owns directory contents + require.NotEqual(t, tuser.Uid, fmt.Sprintf("%d", finalHomeOwner)) + require.NotEqual(t, tuser.Uid, fmt.Sprintf("%d", finalTestFileOwner)) + + // take ownership of directory and contained files if the original user no longer exists + cleanupUsersAndGroups([]string{otheruser}, nil) + err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid) + require.ErrorIs(t, err, os.ErrExist) + + finalHomeOwner = getOwnerUID(testHome) + finalTestFileOwner = getOwnerUID(filepath.Join(testHome, "testfile")) + finalRootFileOwner = getOwnerUID(filepath.Join(testHome, "rootfile")) - require.NotEqual(t, initialOwnerUID, finalOwnerUID) - require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalOwnerUID)) + require.Equal(t, initialRootFileOwner, finalRootFileOwner) // root is still the owner + require.NotEqual(t, initialHomeOwner, finalHomeOwner) // owner is no longer initial user + require.NotEqual(t, initialTestFileOwner, finalTestFileOwner) // owner is no longer initial user + require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalHomeOwner)) // ensure new owner is test-user + require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalTestFileOwner)) // ensure new owner is test-user }) } diff --git a/lib/srv/usermgmt_linux.go b/lib/srv/usermgmt_linux.go index 194404678b562..362c3a3e89545 100644 --- a/lib/srv/usermgmt_linux.go +++ b/lib/srv/usermgmt_linux.go @@ -29,6 +29,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "github.com/gravitational/trace" log "github.com/sirupsen/logrus" @@ -238,7 +239,39 @@ func readDefaultSkel() (string, error) { return skel, trace.Wrap(err) } -func recursiveChown(dir string, uid, gid int) error { +func canTakeOwnership(observedUIDs map[uint32]bool, uid int, path string) (bool, error) { + fi, err := os.Lstat(path) + if err != nil { + return false, trace.Wrap(err) + } + + stat := fi.Sys().(*syscall.Stat_t) + if stat == nil { + return false, nil + } + + exists, seen := observedUIDs[stat.Uid] + if seen { + return !exists, nil + } + + _, err = user.LookupId(strconv.FormatUint(uint64(stat.Uid), 10)) + exists = true + if err != nil { + if !errors.Is(err, user.UnknownUserIdError(stat.Uid)) { + return false, trace.Wrap(err) + } + + exists = false + } + observedUIDs[stat.Uid] = exists + + return !exists, nil +} + +// recursiveChown changes ownership of a directory and its contents. If called in safe mode, the contained files' ownership will +// be left unchanged if the original owner still exists. +func recursiveChown(dir string, uid, gid int, safe bool) error { // First, walk the directory to gather a list of files and directories to update before we open up to modifications var pathsToUpdate []string err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { @@ -252,11 +285,35 @@ func recursiveChown(dir string, uid, gid int) error { return trace.Wrap(err) } - pathsToUpdate = append(pathsToUpdate, dir) - // filepath.WalkDir is documented to walk the paths in lexical order, iterating - // in the reverse order ensures that files are always Lchowned before their parent directory - for i := len(pathsToUpdate) - 1; i >= 0; i-- { - path := pathsToUpdate[i] + observedUIDs := map[uint32]bool{ + 0: true, // root should always exist, so we can preload that UID + } + + if safe { + canTakeDir, err := canTakeOwnership(observedUIDs, uid, dir) + if err != nil { + return trace.WrapWithMessage(err, "taking ownership of %q", dir) + } + + if !canTakeDir { + return trace.WrapWithMessage(os.ErrExist, "can not safely take ownership of %q when owning user still exists", dir) + } + } + + var errs []error + for _, path := range pathsToUpdate { + if safe { + takeOwnership, err := canTakeOwnership(observedUIDs, uid, path) + if err != nil { + errs = append(errs, err) + continue + } + + if !takeOwnership { + continue + } + } + if err := os.Lchown(path, uid, gid); err != nil { if errors.Is(err, os.ErrNotExist) { // Unexpected condition where file was removed after discovery. @@ -265,7 +322,8 @@ func recursiveChown(dir string, uid, gid int) error { return trace.Wrap(err) } } - return nil + + return trace.NewAggregate(errs...) } func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS string) error { @@ -282,7 +340,7 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS err = os.Mkdir(userHome, 0o700) if err != nil { if os.IsExist(err) { - if chownErr := recursiveChown(userHome, uid, gid); chownErr != nil { + if chownErr := recursiveChown(userHome, uid, gid, true); chownErr != nil { return trace.Wrap(chownErr) } } @@ -315,5 +373,5 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS } } - return trace.Wrap(recursiveChown(userHome, uid, gid)) + return trace.Wrap(recursiveChown(userHome, uid, gid, false)) }