From dc0009d47c071d851e9ca3d4f87e7d4d153a12ba Mon Sep 17 00:00:00 2001 From: Andrew Burke <31974658+atburke@users.noreply.github.com> Date: Mon, 10 Jun 2024 09:00:53 -0700 Subject: [PATCH] Fix home dir for insecure_drop mode (#42660) This change fixes a bug where file uploads/downloads in the web UI would fail if the local user (provisioned by Teleport in insecure-drop mode) didn't have a home directory. --- integration/hostuser_test.go | 77 +++++++++++++++++++++++++----------- lib/srv/usermgmt.go | 18 ++++++--- lib/srv/usermgmt_linux.go | 15 ++----- lib/srv/usermgmt_other.go | 4 ++ lib/srv/usermgmt_test.go | 8 ++-- lib/utils/host/hostusers.go | 17 +++++++- 6 files changed, 93 insertions(+), 46 deletions(-) diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index d268945dd3b08..10041cb4a7631 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -61,22 +61,26 @@ func TestRootHostUsersBackend(t *testing.T) { SudoersPath: sudoersTestDir, HostUUID: "hostuuid", } - t.Cleanup(func() { - // cleanup users if they got left behind due to a failing test - host.UserDel(testuser) - cmd := exec.Command("groupdel", testgroup) - cmd.Run() - }) t.Run("Test CreateGroup", func(t *testing.T) { + t.Cleanup(cleanupUsersAndGroups(nil, []string{testgroup})) + err := usersbk.CreateGroup(testgroup, "") require.NoError(t, err) err = usersbk.CreateGroup(testgroup, "") require.True(t, trace.IsAlreadyExists(err)) + + _, err = usersbk.LookupGroup(testgroup) + require.NoError(t, err) }) t.Run("Test CreateUser and group", func(t *testing.T) { - err := usersbk.CreateUser(testuser, []string{testgroup}, "", "") + t.Cleanup(cleanupUsersAndGroups([]string{testuser}, []string{testgroup})) + err := usersbk.CreateGroup(testgroup, "") + require.NoError(t, err) + + testHome := filepath.Join("/home", testuser) + err = usersbk.CreateUser(testuser, []string{testgroup}, testHome, "", "") require.NoError(t, err) tuser, err := usersbk.Lookup(testuser) @@ -89,17 +93,26 @@ func TestRootHostUsersBackend(t *testing.T) { require.NoError(t, err) require.Contains(t, tuserGids, group.Gid) - err = usersbk.CreateUser(testuser, []string{}, "", "") + err = usersbk.CreateUser(testuser, []string{}, testHome, "", "") require.True(t, trace.IsAlreadyExists(err)) - require.NoFileExists(t, filepath.Join("/home", testuser)) - err = usersbk.CreateHomeDirectory(testuser, tuser.Uid, tuser.Gid) + require.NoFileExists(t, testHome) + err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid) require.NoError(t, err) - require.FileExists(t, filepath.Join("/home", testuser, ".bashrc")) + t.Cleanup(func() { + os.RemoveAll(testHome) + }) + require.FileExists(t, filepath.Join(testHome, ".bashrc")) }) t.Run("Test DeleteUser", func(t *testing.T) { - err := usersbk.DeleteUser(testuser) + t.Cleanup(cleanupUsersAndGroups([]string{testuser}, nil)) + err := usersbk.CreateUser(testuser, nil, "", "", "") + require.NoError(t, err) + _, err = usersbk.Lookup(testuser) + require.NoError(t, err) + + err = usersbk.DeleteUser(testuser) require.NoError(t, err) _, err = user.Lookup(testuser) @@ -108,13 +121,10 @@ func TestRootHostUsersBackend(t *testing.T) { t.Run("Test GetAllUsers", func(t *testing.T) { checkUsers := []string{"teleport-usera", "teleport-userb", "teleport-userc"} - t.Cleanup(func() { - for _, u := range checkUsers { - usersbk.DeleteUser(u) - } - }) + t.Cleanup(cleanupUsersAndGroups(checkUsers, nil)) + for _, u := range checkUsers { - err := usersbk.CreateUser(u, []string{}, "", "") + err := usersbk.CreateUser(u, []string{}, "", "", "") require.NoError(t, err) } @@ -143,7 +153,8 @@ func TestRootHostUsersBackend(t *testing.T) { }) t.Run("Test CreateHomeDirectory does not follow symlinks", func(t *testing.T) { - err := usersbk.CreateUser(testuser, []string{testgroup}, "", "") + t.Cleanup(cleanupUsersAndGroups([]string{testuser}, nil)) + err := usersbk.CreateUser(testuser, nil, "", "", "") require.NoError(t, err) tuser, err := usersbk.Lookup(testuser) @@ -154,14 +165,15 @@ func TestRootHostUsersBackend(t *testing.T) { bashrcPath := filepath.Join("/home", testuser, ".bashrc") require.NoFileExists(t, bashrcPath) - require.NoError(t, os.MkdirAll(filepath.Join("/home", testuser), 0o700)) + testHome := filepath.Join("/home", testuser) + require.NoError(t, os.MkdirAll(testHome, 0o700)) require.NoError(t, os.Symlink("/tmp/ignoreme", bashrcPath)) require.NoFileExists(t, "/tmp/ignoreme") - err = usersbk.CreateHomeDirectory(testuser, tuser.Uid, tuser.Gid) + err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid) t.Cleanup(func() { - os.RemoveAll(filepath.Join("/home", testuser)) + os.RemoveAll(testHome) }) require.NoError(t, err) require.NoFileExists(t, "/tmp/ignoreme") @@ -216,7 +228,7 @@ func TestRootHostUsers(t *testing.T) { u, err := user.Lookup(testuser) require.NoError(t, err) requireUserInGroups(t, u, testGroups) - require.NoDirExists(t, u.HomeDir) + require.Equal(t, string(os.PathSeparator), u.HomeDir) require.NoError(t, closer.Close()) _, err = user.Lookup(testuser) @@ -256,6 +268,25 @@ func TestRootHostUsers(t *testing.T) { require.Equal(t, err, user.UnknownUserError(testuser)) }) + t.Run("test create permanent user", func(t *testing.T) { + users := srv.NewHostUsers(context.Background(), presence, "host_uuid") + expectedHome := filepath.Join("/home", testuser) + require.NoDirExists(t, expectedHome) + + closer, err := users.CreateUser(testuser, &services.HostUsersInfo{Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP}) + require.NoError(t, err) + require.Nil(t, closer) + t.Cleanup(cleanupUsersAndGroups([]string{testuser}, nil)) + + u, err := user.Lookup(testuser) + require.NoError(t, err) + require.Equal(t, expectedHome, u.HomeDir) + require.DirExists(t, expectedHome) + t.Cleanup(func() { + os.RemoveAll(expectedHome) + }) + }) + t.Run("test create sudoers enabled users", func(t *testing.T) { if _, err := exec.LookPath("visudo"); err != nil { t.Skip("Visudo not found on path") diff --git a/lib/srv/usermgmt.go b/lib/srv/usermgmt.go index 3ed3727191cf4..054e4efef991b 100644 --- a/lib/srv/usermgmt.go +++ b/lib/srv/usermgmt.go @@ -99,11 +99,11 @@ type HostUsersBackend interface { // CreateGroup creates a group on a host. CreateGroup(group string, gid string) error // CreateUser creates a user on a host. - CreateUser(name string, groups []string, uid, gid string) error + CreateUser(name string, groups []string, home, uid, gid string) error // DeleteUser deletes a user from a host. DeleteUser(name string) error // CreateHomeDirectory creates the users home directory and copies in /etc/skel - CreateHomeDirectory(user string, uid, gid string) error + CreateHomeDirectory(userHome string, uid, gid string) error } type userCloser struct { @@ -298,6 +298,14 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) return nil, trace.WrapWithMessage(err, "error while creating groups") } + var home string + if ui.Mode != types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP { + home, err = readDefaultHome(name) + if err != nil { + return nil, trace.Wrap(err) + } + } + err = u.doWithUserLock(func(_ types.SemaphoreLease) error { if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil { return trace.Wrap(err) @@ -310,7 +318,7 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) } } - err = u.backend.CreateUser(name, groups, ui.UID, ui.GID) + err = u.backend.CreateUser(name, groups, home, ui.UID, ui.GID) if err != nil && !trace.IsAlreadyExists(err) { return trace.WrapWithMessage(err, "error while creating user") } @@ -320,8 +328,8 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) return trace.Wrap(err) } - if ui.Mode != types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP { - if err := u.backend.CreateHomeDirectory(name, user.Uid, user.Gid); err != nil { + if home != "" { + if err := u.backend.CreateHomeDirectory(home, user.Uid, user.Gid); err != nil { return trace.Wrap(err) } } diff --git a/lib/srv/usermgmt_linux.go b/lib/srv/usermgmt_linux.go index a55e141e135c1..da34ad9637014 100644 --- a/lib/srv/usermgmt_linux.go +++ b/lib/srv/usermgmt_linux.go @@ -92,12 +92,8 @@ func (*HostUsersProvisioningBackend) CreateGroup(name string, gid string) error } // CreateUser creates a user on a host -func (*HostUsersProvisioningBackend) CreateUser(name string, groups []string, uid, gid string) error { - home, err := readDefaultHome(name) - if err != nil { - return trace.Wrap(err) - } - _, err = host.UserAdd(name, groups, home, uid, gid) +func (*HostUsersProvisioningBackend) CreateUser(name string, groups []string, home, uid, gid string) error { + _, err := host.UserAdd(name, groups, home, uid, gid) return trace.Wrap(err) } @@ -214,7 +210,7 @@ func readDefaultSkel() (string, error) { return skel, trace.Wrap(err) } -func (u *HostUsersProvisioningBackend) CreateHomeDirectory(user string, uidS, gidS string) error { +func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS string) error { uid, err := strconv.Atoi(uidS) if err != nil { return trace.Wrap(err) @@ -224,11 +220,6 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(user string, uidS, gi return trace.Wrap(err) } - userHome, err := readDefaultHome(user) - if err != nil { - return trace.Wrap(err) - } - err = os.Mkdir(userHome, 0o700) if err != nil { if os.IsExist(err) { diff --git a/lib/srv/usermgmt_other.go b/lib/srv/usermgmt_other.go index 6685b1c61b950..b2615b821388b 100644 --- a/lib/srv/usermgmt_other.go +++ b/lib/srv/usermgmt_other.go @@ -34,3 +34,7 @@ func newHostUsersBackend() (HostUsersBackend, error) { func newHostSudoersBackend(_ string) (HostSudoersBackend, error) { return nil, trace.NotImplemented("Host user creation management is only supported on linux") } + +func readDefaultHome(user string) (string, error) { + return "", trace.NotImplemented("readDefaultHome is only supported on linux") +} diff --git a/lib/srv/usermgmt_test.go b/lib/srv/usermgmt_test.go index 73b8702188836..cd54df30f17f9 100644 --- a/lib/srv/usermgmt_test.go +++ b/lib/srv/usermgmt_test.go @@ -106,7 +106,7 @@ func (tm *testHostUserBackend) CreateGroup(group, gid string) error { return nil } -func (tm *testHostUserBackend) CreateUser(user string, groups []string, uid, gid string) error { +func (tm *testHostUserBackend) CreateUser(user string, groups []string, home, uid, gid string) error { _, ok := tm.users[user] if ok { return trace.AlreadyExists("Group %q, already exists", user) @@ -194,7 +194,7 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) { require.NotContains(t, backend.users, "bob") backend.CreateGroup("testgroup", "") - backend.CreateUser("simon", []string{}, "", "") + backend.CreateUser("simon", []string{}, "", "", "") // try to create a temporary user for simon closer, err = users.CreateUser("simon", userinfo) @@ -240,7 +240,7 @@ func TestUserMgmtSudoers_CreateTemporaryUser(t *testing.T) { } // test user already exists but teleport-service group has not yet // been created - backend.CreateUser("testuser", nil, "", "") + backend.CreateUser("testuser", nil, "", "", "") _, err := users.CreateUser("testuser", &services.HostUsersInfo{ Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, }) @@ -288,7 +288,7 @@ func TestUserMgmt_DeleteAllTeleportSystemUsers(t *testing.T) { if slices.Contains(user.groups, types.TeleportServiceGroup) { users.CreateUser(user.user, &services.HostUsersInfo{Groups: user.groups}) } else { - mgmt.CreateUser(user.user, user.groups, "", "") + mgmt.CreateUser(user.user, user.groups, "", "", "") } } require.NoError(t, users.DeleteAllUsers()) diff --git a/lib/utils/host/hostusers.go b/lib/utils/host/hostusers.go index 0831425abc199..ec8a1b9a02b08 100644 --- a/lib/utils/host/hostusers.go +++ b/lib/utils/host/hostusers.go @@ -21,7 +21,9 @@ package host import ( "bytes" "errors" + "os" "os/exec" + "os/user" "strings" "github.com/gravitational/trace" @@ -65,7 +67,8 @@ func UserAdd(username string, groups []string, home, uid, gid string) (exitCode } if home == "" { - return -1, trace.BadParameter("home is a required parameter") + // Users without a home directory should land at the root, to match OpenSSH behavior. + home = string(os.PathSeparator) } // useradd ---no-create-home (username) (groups)... @@ -111,8 +114,18 @@ func UserDel(username string) (exitCode int, err error) { if err != nil { return -1, trace.Wrap(err, "cant find userdel binary") } + u, err := user.Lookup(username) + if err != nil { + return -1, trace.Wrap(err) + } + args := make([]string, 0, 2) + // Only remove the home dir if it exists and isn't the root. + if u.HomeDir != "" && u.HomeDir != string(os.PathSeparator) { + args = append(args, "--remove") + } + args = append(args, username) // userdel --remove (remove home) username - cmd := exec.Command(userdelBin, "--remove", username) + cmd := exec.Command(userdelBin, args...) output, err := cmd.CombinedOutput() log.Debugf("%s output: %s", cmd.Path, string(output)) return cmd.ProcessState.ExitCode(), trace.Wrap(err)