diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index 23f385d045c4..cbc0dce1bbd3 100644 --- a/integration/hostuser_test.go +++ b/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 { diff --git a/lib/utils/fs_unix_test.go b/lib/srv/hostusers_linux_test.go similarity index 90% rename from lib/utils/fs_unix_test.go rename to lib/srv/hostusers_linux_test.go index 403e2efbd9b0..378ac50bb989 100644 --- a/lib/utils/fs_unix_test.go +++ b/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 . */ -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 diff --git a/lib/srv/usermgmt.go b/lib/srv/usermgmt.go index 23397f8ef11d..c1949e098bc5 100644 --- a/lib/srv/usermgmt.go +++ b/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, diff --git a/lib/srv/usermgmt_linux.go b/lib/srv/usermgmt_linux.go index 3626a599beb7..84ffc698cdec 100644 --- a/lib/srv/usermgmt_linux.go +++ b/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,11 +242,107 @@ 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) @@ -249,6 +350,12 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS 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)) } diff --git a/lib/srv/usermgmt_other.go b/lib/srv/usermgmt_other.go index 6685b1c61b95..b8d4530a3082 100644 --- a/lib/srv/usermgmt_other.go +++ b/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") } diff --git a/lib/utils/fs.go b/lib/utils/fs.go index dbfd1b390d7b..b12c08a111f8 100644 --- a/lib/utils/fs.go +++ b/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 {