diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index 284854bebf491..cbc0dce1bbd34 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -82,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", diff --git a/lib/srv/hostusers_linux_test.go b/lib/srv/hostusers_linux_test.go index a822cd770c261..378ac50bb9899 100644 --- a/lib/srv/hostusers_linux_test.go +++ b/lib/srv/hostusers_linux_test.go @@ -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 8a782870bbc52..c1949e098bc55 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, @@ -588,7 +590,7 @@ func (u *HostUserManagement) UserCleanup() { err := u.DeleteAllUsers() switch { case trace.IsNotFound(err): - u.log.Debug("Error during temporary user cleanup, stopping cleanup job", "error", err) + u.log.DebugContext(u.ctx, "Error during temporary user cleanup, stopping cleanup job", "error", err) return case err != nil: u.log.ErrorContext(u.ctx, "Error during temporary user cleanup", "error", err) diff --git a/lib/srv/usermgmt_linux.go b/lib/srv/usermgmt_linux.go index 25e0584d710bf..84ffc698cdec5 100644 --- a/lib/srv/usermgmt_linux.go +++ b/lib/srv/usermgmt_linux.go @@ -20,9 +20,11 @@ package srv import ( "bufio" + "context" "errors" "fmt" "io/fs" + "log/slog" "os" "os/exec" "os/user" @@ -40,6 +42,7 @@ import ( // HostUsersProvisioningBackend is used to implement HostUsersBackend type HostUsersProvisioningBackend struct { + log *slog.Logger } // HostSudoersProvisioningBackend is used to implement HostSudoersBackend @@ -50,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 { @@ -62,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 @@ -251,7 +254,8 @@ func canTakeOwnership(observedUIDs map[uint32]bool, uid int, fi fs.FileInfo) (bo } _, err := user.LookupId(strconv.FormatUint(uint64(stat.Uid), 10)) - exists = true + // 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) @@ -266,7 +270,9 @@ func canTakeOwnership(observedUIDs map[uint32]bool, uid int, fi fs.FileInfo) (bo // 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 { +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 } @@ -293,14 +299,17 @@ func recursiveChown(dir string, uid, gid int, safe bool) error { 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 } @@ -312,6 +321,7 @@ func recursiveChown(dir string, uid, gid int, safe bool) error { 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) @@ -341,7 +351,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, true); chownErr != nil { + if chownErr := recursiveChown(u.log, userHome, uid, gid, true); chownErr != nil { return trace.Wrap(chownErr) } } @@ -374,5 +384,5 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS } } - return trace.Wrap(recursiveChown(userHome, uid, gid, false)) + 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 6685b1c61b950..b8d4530a3082a 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") }