From b788cc51e9cf94f4a727d678a85f2be56dd2815b Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Mon, 20 May 2024 14:57:18 -0700 Subject: [PATCH] Update host user groups for existing users This change fixes a bug in host user creation where Teleport would not update the groups of a returning user if groups were changed in the user's role. --- integration/hostuser_test.go | 83 ++++++++++++++++++++++++--- lib/srv/sess.go | 2 +- lib/srv/usermgmt.go | 105 +++++++++++++++++++++-------------- lib/srv/usermgmt_linux.go | 6 ++ lib/srv/usermgmt_test.go | 31 +++++++---- lib/utils/host/hostusers.go | 12 ++-- 6 files changed, 169 insertions(+), 70 deletions(-) diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go index 10041cb4a7631..076328c9e4441 100644 --- a/integration/hostuser_test.go +++ b/integration/hostuser_test.go @@ -28,6 +28,7 @@ import ( "os/exec" "os/user" "path/filepath" + "slices" "testing" "github.com/gravitational/trace" @@ -180,7 +181,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) @@ -189,7 +190,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() { @@ -219,7 +224,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) @@ -244,7 +249,7 @@ func TestRootHostUsers(t *testing.T) { _, err := user.LookupGroupId(testGID) require.ErrorIs(t, err, user.UnknownGroupIdError(testGID)) - closer, err := users.CreateUser(testuser, &services.HostUsersInfo{ + closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{ Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, UID: testUID, GID: testGID, @@ -273,7 +278,7 @@ func TestRootHostUsers(t *testing.T) { expectedHome := filepath.Join("/home", testuser) require.NoDirExists(t, expectedHome) - closer, err := users.CreateUser(testuser, &services.HostUsersInfo{Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP}) + 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)) @@ -303,7 +308,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_INSECURE_DROP, }) @@ -334,12 +339,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_INSECURE_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) @@ -360,4 +365,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 495c688945a7b..1fa5d7b30036e 100644 --- a/lib/srv/sess.go +++ b/lib/srv/sess.go @@ -281,7 +281,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 74bef06319e01..8a1a7bc697f9d 100644 --- a/lib/srv/usermgmt.go +++ b/lib/srv/usermgmt.go @@ -96,6 +96,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. @@ -145,8 +147,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 @@ -221,29 +223,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_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 && !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 { @@ -252,16 +296,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 @@ -272,30 +314,7 @@ 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, 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_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 } var home string @@ -307,8 +326,10 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) } 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 @@ -341,7 +362,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 da34ad9637014..1f4c5d71a985a 100644 --- a/lib/srv/usermgmt_linux.go +++ b/lib/srv/usermgmt_linux.go @@ -79,6 +79,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 cd54df30f17f9..7003237105e2a 100644 --- a/lib/srv/usermgmt_test.go +++ b/lib/srv/usermgmt_test.go @@ -89,6 +89,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] { @@ -175,7 +183,7 @@ func TestUserMgmt_CreateTemporaryUser(t *testing.T) { Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_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") @@ -185,8 +193,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 @@ -197,8 +205,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) } @@ -217,7 +225,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_INSECURE_DROP, }) @@ -241,16 +249,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{ + _, 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{ + _, err = users.UpsertUser("testuser", &services.HostUsersInfo{ Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP, }) - require.True(t, trace.IsAlreadyExists(err)) + require.NoError(t, err) }) } @@ -286,7 +293,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 ec8a1b9a02b08..bac96a1c8d1fd 100644 --- a/lib/utils/host/hostusers.go +++ b/lib/utils/host/hostusers.go @@ -92,17 +92,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)