Host User Creation - update groups for existing users

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)
+					}
+				}
+			})
+		}
+	})
 }

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)
 	}

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{

lib/srv/usermgmt_linux.go

@@ -90,6 +90,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()