Host users take ownership of existing home directories

integration/hostuser_test.go

@@ -32,6 +32,7 @@ import (
 	"path/filepath"
 	"slices"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -81,7 +82,8 @@ func getUserShells(path string) (map[string]string, error) {
 func TestRootHostUsersBackend(t *testing.T) {
 	utils.RequireRoot(t)
 	sudoersTestDir := t.TempDir()
-	usersbk := srv.HostUsersProvisioningBackend{}
+	usersbk, err := srv.NewHostUsersBackend(utils.NewSlogLoggerForTests())
+	require.NoError(t, err)
 	sudoersbk := srv.HostSudoersProvisioningBackend{
 		SudoersPath: sudoersTestDir,
 		HostUUID:    "hostuuid",
@@ -203,6 +205,75 @@ func TestRootHostUsersBackend(t *testing.T) {
 		require.ErrorIs(t, err, os.ErrExist)
 		require.NoFileExists(t, "/tmp/ignoreme")
 	})
+
+	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)
+
+		err = usersbk.CreateUser(otheruser, nil, host.UserOpts{})
+		require.NoError(t, err)
+
+		tuser, err := usersbk.Lookup(testuser)
+		require.NoError(t, err)
+
+		ouser, err := usersbk.Lookup(otheruser)
+		require.NoError(t, err)
+
+		require.NoError(t, os.WriteFile("/etc/skel/testfile", []byte("test\n"), 0o700))
+
+		testHome := filepath.Join("/home", testuser)
+		err = usersbk.CreateHomeDirectory(testHome, ouser.Uid, ouser.Gid)
+		t.Cleanup(func() {
+			os.RemoveAll(testHome)
+		})
+		require.NoError(t, err)
+
+		// 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)
+
+		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.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
+	})
 }
 
 func getUserGroups(t *testing.T, u *user.User) []string {

lib/utils/fs_unix_test.go → lib/srv/hostusers_linux_test.go

@@ -1,5 +1,5 @@
-//go:build !windows
-// +build !windows
+//go:build linux
+// +build linux
 
 /*
  * Teleport
@@ -19,7 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-package utils
+package srv
 
 import (
 	"os"
@@ -29,6 +29,7 @@ import (
 	"syscall"
 	"testing"
 
+	"github.com/gravitational/teleport/lib/utils"
 	"github.com/stretchr/testify/require"
 )
 
@@ -90,17 +91,18 @@ func verifyOwnership(t *testing.T, path string, uid, gid int) {
 }
 
 func TestRecursiveChown(t *testing.T) {
+	log := utils.NewSlogLoggerForTests()
 	t.Run("notFoundError", func(t *testing.T) {
 		t.Parallel()
 
-		require.Error(t, RecursiveChown("/invalid/path/to/nowhere", 1000, 1000))
+		require.Error(t, recursiveChown(log, "/invalid/path/to/nowhere", 1000, 1000, false))
 	})
 	t.Run("simpleChown", func(t *testing.T) {
 		t.Parallel()
 		_, _, newUid, newGid, _ := setupRecursiveChownUser(t)
 		dir1Path, dir1FilePath, _, _ := setupRecursiveChownFiles(t)
 
-		require.NoError(t, RecursiveChown(dir1Path, newUid, newGid))
+		require.NoError(t, recursiveChown(log, dir1Path, newUid, newGid, false))
 		// validate ownership matches expected ids
 		verifyOwnership(t, dir1Path, newUid, newGid)
 		verifyOwnership(t, dir1FilePath, newUid, newGid)
@@ -114,7 +116,7 @@ func TestRecursiveChown(t *testing.T) {
 		}
 		_, dir1FilePath, dir2Path, dir2SymlinkToFile := setupRecursiveChownFiles(t)
 
-		require.NoError(t, RecursiveChown(dir2Path, newUid, newGid))
+		require.NoError(t, recursiveChown(log, dir2Path, newUid, newGid, false))
 		// Validate symlink has changed
 		verifyOwnership(t, dir2SymlinkToFile, newUid, newGid)
 		// Validate pointed file has not changed

lib/srv/usermgmt.go

@@ -44,8 +44,10 @@ import (
 
 // NewHostUsers initialize a new HostUsers object
 func NewHostUsers(ctx context.Context, storage services.PresenceInternal, uuid string) HostUsers {
+	log := slog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentHostUsers))
+
 	//nolint:staticcheck // SA4023. False positive on macOS.
-	backend, err := newHostUsersBackend()
+	backend, err := NewHostUsersBackend(log)
 	switch {
 	case trace.IsNotImplemented(err), trace.IsNotFound(err):
 		slog.DebugContext(ctx, "Skipping host user management", "error", err)
@@ -56,7 +58,7 @@ func NewHostUsers(ctx context.Context, storage services.PresenceInternal, uuid s
 	}
 	cancelCtx, cancelFunc := context.WithCancel(ctx)
 	return &HostUserManagement{
-		log:       slog.With(teleport.ComponentKey, teleport.ComponentHostUsers),
+		log:       log,
 		backend:   backend,
 		ctx:       cancelCtx,
 		cancel:    cancelFunc,

lib/srv/usermgmt_linux.go

@@ -20,14 +20,18 @@ package srv
 
 import (
 	"bufio"
+	"context"
 	"errors"
 	"fmt"
+	"io/fs"
+	"log/slog"
 	"os"
 	"os/exec"
 	"os/user"
 	"path/filepath"
 	"strconv"
 	"strings"
+	"syscall"
 
 	"github.com/gravitational/trace"
 	log "github.com/sirupsen/logrus"
@@ -38,6 +42,7 @@ import (
 
 // HostUsersProvisioningBackend is used to implement HostUsersBackend
 type HostUsersProvisioningBackend struct {
+	log *slog.Logger
 }
 
 // HostSudoersProvisioningBackend is used to implement HostSudoersBackend
@@ -48,8 +53,8 @@ type HostSudoersProvisioningBackend struct {
 	SudoersPath string
 }
 
-// newHostUsersBackend initializes a new OS specific HostUsersBackend
-func newHostUsersBackend() (HostUsersBackend, error) {
+// NewHostUsersBackend initializes a new OS specific HostUsersBackend
+func NewHostUsersBackend(log *slog.Logger) (HostUsersBackend, error) {
 	var missing []string
 	for _, requiredBin := range []string{"usermod", "useradd", "getent", "groupadd", "visudo"} {
 		if _, err := exec.LookPath(requiredBin); err != nil {
@@ -60,7 +65,7 @@ func newHostUsersBackend() (HostUsersBackend, error) {
 		return nil, trace.NotFound("missing required binaries: %s", strings.Join(missing, ","))
 	}
 
-	return &HostUsersProvisioningBackend{}, nil
+	return &HostUsersProvisioningBackend{log: log}, nil
 }
 
 // newHostUsersBackend initializes a new OS specific HostUsersBackend
@@ -237,18 +242,120 @@ func readDefaultSkel() (string, error) {
 	return skel, trace.Wrap(err)
 }
 
+func canTakeOwnership(observedUIDs map[uint32]bool, uid int, fi fs.FileInfo) (bool, error) {
+	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))
+	// only set exists to true if the current owner differs from the given uid
+	exists = stat.Uid != uint32(uid)
+	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(log *slog.Logger, dir string, uid, gid int, safe bool) error {
+	ctx := context.Background()
+	l := log.With("uid", uid, "gid", gid, "safe", safe)
+	observedUIDs := map[uint32]bool{
+		0: true, // root should always exist, so we can preload that UID
+	}
+
+	dirFI, err := os.Lstat(dir)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+
+	// we can short circuit in safe mode if the directory itself can't be taken over
+	if safe {
+		canTakeDir, err := canTakeOwnership(observedUIDs, uid, dirFI)
+		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)
+		}
+	}
+
+	err = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return trace.Wrap(err)
+		}
+
+		l = log.With("path", path)
+		if safe {
+			fi, err := d.Info()
+			if err != nil {
+				l.WarnContext(ctx, "Could not retrieve file info for safe file ownership change", "error", err)
+				return nil
+			}
+
+			takeOwnership, err := canTakeOwnership(observedUIDs, uid, fi)
+			if err != nil {
+				l.WarnContext(ctx, "Could not determine if file ownership change is safe", "error", err)
+				return nil
+			}
+
+			if !takeOwnership {
+				return nil
+			}
+		}
+
+		if err := os.Lchown(path, uid, gid); err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				// Unexpected condition where file was removed after discovery.
+				l.WarnContext(ctx, "File was removed before ownership change", "error", err)
+				return nil
+			}
+			return trace.Wrap(err)
+		}
+
+		return nil
+	})
+
+	if err != nil {
+		return trace.Wrap(err)
+	}
+
+	return nil
+}
+
 func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS string) error {
 	uid, err := strconv.Atoi(uidS)
 	if err != nil {
 		return trace.Wrap(err)
 	}
+
 	gid, err := strconv.Atoi(gidS)
 	if err != nil {
 		return trace.Wrap(err)
 	}
 
 	err = os.Mkdir(userHome, 0o700)
 	if err != nil {
+		if os.IsExist(err) {
+			if chownErr := recursiveChown(u.log, userHome, uid, gid, true); chownErr != nil {
+				return trace.Wrap(chownErr)
+			}
+		}
+
 		return trace.Wrap(err)
 	}
 
@@ -277,9 +384,5 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS
 		}
 	}
 
-	if err := utils.RecursiveChown(userHome, uid, gid); err != nil {
-		return trace.Wrap(err)
-	}
-
-	return nil
+	return trace.Wrap(recursiveChown(u.log, userHome, uid, gid, false))
 }

lib/srv/usermgmt_other.go

@@ -22,11 +22,13 @@
 package srv
 
 import (
+	"log/slog"
+
 	"github.com/gravitational/trace"
 )
 
 //nolint:staticcheck // intended to always return an error for non-linux builds
-func newHostUsersBackend() (HostUsersBackend, error) {
+func NewHostUsersBackend(log *slog.Logger) (HostUsersBackend, error) {
 	return nil, trace.NotImplemented("Host user creation management is only supported on linux")
 }
 

lib/utils/fs.go

@@ -393,35 +393,6 @@ func RemoveFileIfExist(filePath string) error {
 	return nil
 }
 
-func RecursiveChown(dir string, uid, gid int) 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 {
-		if err != nil {
-			return trace.Wrap(err)
-		}
-		pathsToUpdate = append(pathsToUpdate, path)
-		return nil
-	})
-	if err != nil {
-		return trace.Wrap(err)
-	}
-
-	// 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]
-		if err := os.Lchown(path, uid, gid); err != nil {
-			if errors.Is(err, os.ErrNotExist) {
-				// Unexpected condition where file was removed after discovery.
-				continue
-			}
-			return trace.Wrap(err)
-		}
-	}
-	return nil
-}
-
 func CopyFile(src, dest string, perm os.FileMode) error {
 	srcFile, err := os.Open(src)
 	if err != nil {