diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index cbd0786905d07..5d49f14ccff26 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -26,6 +26,7 @@ import ( "os/exec" "os/user" "path/filepath" + "slices" "testing" "github.com/gravitational/trace" @@ -166,7 +167,7 @@ func TestRootHostUsersBackend(t *testing.T) { }) } -func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) { +func getUserGroups(t *testing.T, u *user.User) []string { var userGroups []string userGids, err := u.GroupIds() require.NoError(t, err) @@ -175,7 +176,11 @@ func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) { require.NoError(t, err) userGroups = append(userGroups, group.Name) } - require.Subset(t, userGroups, requiredGroups) + return userGroups +} + +func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) { + require.Subset(t, getUserGroups(t, u), requiredGroups) } func cleanupUsersAndGroups(users []string, groups []string) func() { @@ -205,7 +210,7 @@ func TestRootHostUsers(t *testing.T) { users := srv.NewHostUsers(context.Background(), presence, "host_uuid") testGroups := []string{"group1", "group2"} - closer, err := users.CreateUser(testuser, &services.HostUsersInfo{Groups: testGroups, Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP}) + closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{Groups: testGroups, Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP}) require.NoError(t, err) testGroups = append(testGroups, types.TeleportServiceGroup) @@ -227,7 +232,7 @@ func TestRootHostUsers(t *testing.T) { users := srv.NewHostUsers(context.Background(), presence, "host_uuid") testGroups := []string{"group1", "group2"} - closer, err := users.CreateUser(testuser, &services.HostUsersInfo{Groups: testGroups, Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP}) + closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{Groups: testGroups, Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP}) require.NoError(t, err) testGroups = append(testGroups, types.TeleportServiceGroup) @@ -252,8 +257,8 @@ func TestRootHostUsers(t *testing.T) { _, err := user.LookupGroupId(testGID) require.ErrorIs(t, err, user.UnknownGroupIdError(testGID)) - closer, err := users.CreateUser(testuser, &services.HostUsersInfo{ - Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, + closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{ + Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, UID: testUID, GID: testGID, }) @@ -278,6 +283,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.UpsertUser(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") @@ -294,7 +318,7 @@ func TestRootHostUsers(t *testing.T) { os.Remove(sudoersPath(testuser, uuid)) host.UserDel(testuser) }) - closer, err := users.CreateUser(testuser, + closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{ Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, }) @@ -325,12 +349,12 @@ func TestRootHostUsers(t *testing.T) { deleteableUsers := []string{"teleport-user1", "teleport-user2", "teleport-user3"} for _, user := range deleteableUsers { - _, err := users.CreateUser(user, &services.HostUsersInfo{Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP}) + _, err := users.UpsertUser(user, &services.HostUsersInfo{Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP}) require.NoError(t, err) } // this user should not be in the service group as it was created with mode keep. - closer, err := users.CreateUser("teleport-user4", &services.HostUsersInfo{ + closer, err := users.UpsertUser("teleport-user4", &services.HostUsersInfo{ Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP, }) require.NoError(t, err) @@ -351,4 +375,66 @@ func TestRootHostUsers(t *testing.T) { require.Equal(t, err, user.UnknownUserError(us)) } }) + + t.Run("test update changed groups", func(t *testing.T) { + tests := []struct { + name string + firstGroups []string + secondGroups []string + }{ + { + name: "add groups", + secondGroups: []string{"group1", "group2"}, + }, + { + name: "delete groups", + firstGroups: []string{"group1", "group2"}, + }, + { + name: "change groups", + firstGroups: []string{"group1", "group2"}, + secondGroups: []string{"group2", "group3"}, + }, + { + name: "no change", + firstGroups: []string{"group1", "group2"}, + secondGroups: []string{"group2", "group1"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Cleanup(cleanupUsersAndGroups([]string{testuser}, slices.Concat(tc.firstGroups, tc.secondGroups))) + + // Verify that the user is created with the first set of groups. + users := srv.NewHostUsers(context.Background(), presence, "host_uuid") + _, err := users.UpsertUser(testuser, &services.HostUsersInfo{ + Groups: tc.firstGroups, + Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP, + }) + require.NoError(t, err) + u, err := user.Lookup(testuser) + require.NoError(t, err) + requireUserInGroups(t, u, tc.firstGroups) + + // Verify that the user is updated with the second set of groups. + _, err = users.UpsertUser(testuser, &services.HostUsersInfo{ + Groups: tc.secondGroups, + Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP, + }) + require.NoError(t, err) + u, err = user.Lookup(testuser) + require.NoError(t, err) + requireUserInGroups(t, u, tc.secondGroups) + + // Verify that the appropriate groups form the first set were deleted. + userGroups := getUserGroups(t, u) + for _, group := range tc.firstGroups { + if !slices.Contains(tc.secondGroups, group) { + require.NotContains(t, userGroups, group) + } + } + }) + } + }) } diff --git a/lib/srv/sess.go b/lib/srv/sess.go index dbc22562b3473..90f2f2b6144f2 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -280,7 +280,7 @@ func (s *SessionRegistry) TryCreateHostUser(ctx *ServerContext) error { if trace.IsAccessDenied(err) && existsErr != nil { return trace.WrapWithMessage(err, "Insufficient permission for host user creation") } - userCloser, err := s.users.CreateUser(ctx.Identity.Login, ui) + userCloser, err := s.users.UpsertUser(ctx.Identity.Login, ui) if userCloser != nil { ctx.AddCloser(userCloser) } diff --git a/lib/srv/usermgmt.go b/lib/srv/usermgmt.go index 7b3969077fe91..f4abeffc05e48 100644 --- a/lib/srv/usermgmt.go +++ b/lib/srv/usermgmt.go @@ -94,6 +94,8 @@ type HostUsersBackend interface { LookupGroup(group string) (*user.Group, error) // LookupGroupByID retrieves a group by its ID. LookupGroupByID(gid string) (*user.Group, error) + // SetUserGroups sets a user's groups, replacing their existing groups. + SetUserGroups(name string, groups []string) error // CreateGroup creates a group on a host. CreateGroup(group string, gid string) error // CreateUser creates a user on a host. @@ -143,8 +145,8 @@ func (*HostSudoersNotImplemented) RemoveSudoers(name string) error { } type HostUsers interface { - // CreateUser creates a temporary Teleport user in the TeleportServiceGroup - CreateUser(name string, hostRoleInfo *services.HostUsersInfo) (io.Closer, error) + // UpsertUser creates a temporary Teleport user in the TeleportServiceGroup + UpsertUser(name string, hostRoleInfo *services.HostUsersInfo) (io.Closer, error) // DeleteUser deletes a temporary Teleport user only if they are // in a specified group DeleteUser(name string, gid string) error @@ -219,29 +221,71 @@ func (u *HostSudoersManagement) RemoveSudoers(name string) error { return nil } -// CreateUser creates a temporary Teleport user in the TeleportServiceGroup -func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) (io.Closer, error) { +// UpsertUser creates a temporary Teleport user in the TeleportServiceGroup +func (u *HostUserManagement) UpsertUser(name string, ui *services.HostUsersInfo) (io.Closer, error) { if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_UNSPECIFIED { return nil, trace.BadParameter("Mode is a required argument to CreateUser") } + groups := make([]string, 0, len(ui.Groups)) + for _, group := range ui.Groups { + if group == name { + // this causes an error as useradd expects the group with the same name as the user to be available + log.Debugf("Skipping group creation with name the same as login user (%q, %q).", name, group) + continue + } + groups = append(groups, group) + } + if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_DROP || ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP { + groups = append(groups, types.TeleportServiceGroup) + } + var errs []error + for _, group := range groups { + if err := u.createGroupIfNotExist(group); err != nil { + errs = append(errs, err) + continue + } + } + if err := trace.NewAggregate(errs...); err != nil { + return nil, trace.WrapWithMessage(err, "error while creating groups") + } + tempUser, err := u.backend.Lookup(name) - if err != nil && err != user.UnknownUserError(name) { + if err != nil && !errors.Is(err, user.UnknownUserError(name)) { return nil, trace.Wrap(err) } if tempUser != nil { - gids, err := u.backend.UserGIDs(tempUser) - if err != nil { - return nil, trace.Wrap(err) + // Collect actions that need to be done together under a lock on the user. + actionsUnderLock := []func() error{ + func() error { + // If the user exists, set user groups again as they might have changed. + return trace.Wrap(u.backend.SetUserGroups(name, groups)) + }, } + doWithUserLock := func() error { + return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error { + for _, action := range actionsUnderLock { + if err := action(); err != nil { + return trace.Wrap(err) + } + } + return nil + })) + } + systemGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup) if err != nil { if isUnknownGroupError(err, types.TeleportServiceGroup) { - return nil, trace.AlreadyExists("User %q already exists, however no users are currently managed by teleport", name) + // Teleport service group doesn't exist, so we don't need to update interaction time. + return nil, trace.Wrap(doWithUserLock()) } return nil, trace.Wrap(err) } + gids, err := u.backend.UserGIDs(tempUser) + if err != nil { + return nil, trace.Wrap(err) + } var found bool for _, gid := range gids { if gid == systemGroup.Gid { @@ -250,16 +294,14 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) } } if !found { - return nil, trace.AlreadyExists("User %q already exists and is not managed by teleport", name) + // User isn't managed by Teleport, so we don't need to update interaction time. + return nil, trace.Wrap(doWithUserLock()) } - err = u.doWithUserLock(func(_ types.SemaphoreLease) error { - if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil { - return trace.Wrap(err) - } - return nil + actionsUnderLock = append(actionsUnderLock, func() error { + return trace.Wrap(u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now())) }) - if err != nil { + if err := doWithUserLock(); err != nil { return nil, trace.Wrap(err) } // try to delete even if the user already exists as only users @@ -270,28 +312,14 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) username: name, users: u, backend: u.backend, - }, trace.AlreadyExists("User %q already exists", name) - } - - groups := make([]string, len(ui.Groups)) - copy(groups, ui.Groups) - if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_DROP || ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP { - groups = append(groups, types.TeleportServiceGroup) - } - var errs []error - for _, group := range groups { - if err := u.createGroupIfNotExist(group); err != nil { - errs = append(errs, err) - continue - } - } - if err := trace.NewAggregate(errs...); err != nil { - return nil, trace.WrapWithMessage(err, "error while creating groups") + }, nil } err = u.doWithUserLock(func(_ types.SemaphoreLease) error { - if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil { - return trace.Wrap(err) + if ui.Mode != types.CreateHostUserMode_HOST_USER_MODE_KEEP { + if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil { + return trace.Wrap(err) + } } if ui.GID != "" { // if gid is specified a group must already exist @@ -324,7 +352,7 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) } if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_KEEP { - return nil, trace.Wrap(err) + return nil, nil } closer := &userCloser{ diff --git a/lib/srv/usermgmt_linux.go b/lib/srv/usermgmt_linux.go index 7b926c705b655..87c440e43b238 100644 --- a/lib/srv/usermgmt_linux.go +++ b/lib/srv/usermgmt_linux.go @@ -77,6 +77,12 @@ func (*HostUsersProvisioningBackend) LookupGroupByID(gid string) (*user.Group, e return user.LookupGroupId(gid) } +// SetUserGroups sets a user's groups, replacing their existing groups. +func (*HostUsersProvisioningBackend) SetUserGroups(name string, groups []string) error { + _, err := host.SetUserGroups(name, groups) + return trace.Wrap(err) +} + // GetAllUsers returns a full list of users present on a system func (*HostUsersProvisioningBackend) GetAllUsers() ([]string, error) { users, _, err := host.GetAllUsers() diff --git a/lib/srv/usermgmt_test.go b/lib/srv/usermgmt_test.go index 119ce5286b202..52de42b3cdad8 100644 --- a/lib/srv/usermgmt_test.go +++ b/lib/srv/usermgmt_test.go @@ -87,6 +87,14 @@ func (tm *testHostUserBackend) LookupGroupByID(gid string) (*user.Group, error) }, nil } +func (tm *testHostUserBackend) SetUserGroups(name string, groups []string) error { + if _, ok := tm.users[name]; !ok { + return trace.NotFound("User %q doesn't exist", name) + } + tm.users[name] = groups + return nil +} + func (tm *testHostUserBackend) UserGIDs(u *user.User) ([]string, error) { ids := make([]string, 0, len(tm.users[u.Username])) for _, id := range tm.users[u.Username] { @@ -173,7 +181,7 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) { Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, } // create a user with some groups - closer, err := users.CreateUser("bob", userinfo) + closer, err := users.UpsertUser("bob", userinfo) require.NoError(t, err) require.NotNil(t, closer, "user closer was nil") @@ -183,8 +191,8 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) { }, backend.users["bob"]) // try creat the same user again - secondCloser, err := users.CreateUser("bob", userinfo) - require.True(t, trace.IsAlreadyExists(err)) + secondCloser, err := users.UpsertUser("bob", userinfo) + require.NoError(t, err) require.NotNil(t, secondCloser) // Close will remove the user if the user is in the teleport-system group @@ -195,8 +203,8 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) { backend.CreateUser("simon", []string{}, "", "") // try to create a temporary user for simon - closer, err = users.CreateUser("simon", userinfo) - require.True(t, trace.IsAlreadyExists(err)) + closer, err = users.UpsertUser("simon", userinfo) + require.NoError(t, err) require.Nil(t, closer) } @@ -215,7 +223,7 @@ func TestUserMgmtSudoers_CreateTemporaryUser(t *testing.T) { backend: backend, } - closer, err := users.CreateUser("bob", &services.HostUsersInfo{ + closer, err := users.UpsertUser("bob", &services.HostUsersInfo{ Groups: []string{"hello", "sudo"}, Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, }) @@ -239,16 +247,15 @@ func TestUserMgmtSudoers_CreateTemporaryUser(t *testing.T) { // test user already exists but teleport-service group has not yet // been created backend.CreateUser("testuser", nil, "", "") - _, err := users.CreateUser("testuser", &services.HostUsersInfo{ - Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, + _, err := users.UpsertUser("testuser", &services.HostUsersInfo{ + Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, }) - require.True(t, trace.IsAlreadyExists(err)) + require.NoError(t, err) backend.CreateGroup(types.TeleportServiceGroup, "") - // IsAlreadyExists error when teleport-service group now exists - _, err = users.CreateUser("testuser", &services.HostUsersInfo{ - Mode: types.CreateHostUserMode_HOST_USER_MODE_DROP, + _, err = users.UpsertUser("testuser", &services.HostUsersInfo{ + Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, }) - require.True(t, trace.IsAlreadyExists(err)) + require.NoError(t, err) }) } @@ -284,7 +291,7 @@ func TestUserMgmt_DeleteAllTeleportSystemUsers(t *testing.T) { mgmt.CreateGroup(group, "") } if slices.Contains(user.groups, types.TeleportServiceGroup) { - users.CreateUser(user.user, &services.HostUsersInfo{Groups: user.groups}) + users.UpsertUser(user.user, &services.HostUsersInfo{Groups: user.groups}) } else { mgmt.CreateUser(user.user, user.groups, "", "") } diff --git a/lib/utils/host/hostusers.go b/lib/utils/host/hostusers.go index 7b2759dedd142..4b9d1bc8ba090 100644 --- a/lib/utils/host/hostusers.go +++ b/lib/utils/host/hostusers.go @@ -87,17 +87,15 @@ func UserAdd(username string, groups []string, home, uid, gid string) (exitCode return cmd.ProcessState.ExitCode(), trace.Wrap(err) } -// AddUserToGroups adds a user to a list of specified groups on a host using `usermod` -func AddUserToGroups(username string, groups []string) (exitCode int, err error) { +// SetUserGroups adds a user to a list of specified groups on a host using `usermod`, +// overriding any existing supplementary groups. +func SetUserGroups(username string, groups []string) (exitCode int, err error) { usermodBin, err := exec.LookPath("usermod") if err != nil { return -1, trace.Wrap(err, "cant find usermod binary") } - args := []string{"-aG"} - args = append(args, groups...) - args = append(args, username) - // usermod -aG (append groups) (username) - cmd := exec.Command(usermodBin, args...) + // usermod -G (replace groups) (username) + cmd := exec.Command(usermodBin, "-G", strings.Join(groups, ","), username) output, err := cmd.CombinedOutput() log.Debugf("%s output: %s", cmd.Path, string(output)) return cmd.ProcessState.ExitCode(), trace.Wrap(err)