[v15] Preserve non drop users

integration/hostuser_test.go

@@ -218,7 +218,7 @@ func TestRootHostUsers(t *testing.T) {
 		users := srv.NewHostUsers(context.Background(), presence, "host_uuid")
 
 		testGroups := []string{"group1", "group2"}
-		closer, err := users.UpsertUser(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)
@@ -243,7 +243,7 @@ func TestRootHostUsers(t *testing.T) {
 		_, err := user.LookupGroupId(testGID)
 		require.ErrorIs(t, err, user.UnknownGroupIdError(testGID))
 
-		closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{
+		closer, err := users.UpsertUser(testuser, services.HostUsersInfo{
 			Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
 			UID:  testUID,
 			GID:  testGID,
@@ -272,7 +272,7 @@ func TestRootHostUsers(t *testing.T) {
 		expectedHome := filepath.Join("/home", testuser)
 		require.NoDirExists(t, expectedHome)
 
-		closer, err := users.UpsertUser(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 +303,7 @@ func TestRootHostUsers(t *testing.T) {
 			host.UserDel(testuser)
 		})
 		closer, err := users.UpsertUser(testuser,
-			&services.HostUsersInfo{
+			services.HostUsersInfo{
 				Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
 			})
 		require.NoError(t, err)
@@ -333,12 +333,12 @@ func TestRootHostUsers(t *testing.T) {
 
 		deleteableUsers := []string{"teleport-user1", "teleport-user2", "teleport-user3"}
 		for _, user := range deleteableUsers {
-			_, err := users.UpsertUser(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.UpsertUser("teleport-user4", &services.HostUsersInfo{
+		closer, err := users.UpsertUser("teleport-user4", services.HostUsersInfo{
 			Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
 		})
 		require.NoError(t, err)
@@ -395,7 +395,7 @@ func TestRootHostUsers(t *testing.T) {
 
 				// 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{
+				_, err := users.UpsertUser(testuser, services.HostUsersInfo{
 					Groups: tc.firstGroups,
 					Mode:   types.CreateHostUserMode_HOST_USER_MODE_KEEP,
 				})
@@ -405,7 +405,7 @@ func TestRootHostUsers(t *testing.T) {
 				requireUserInGroups(t, u, tc.firstGroups)
 
 				// Verify that the user is updated with the second set of groups.
-				_, err = users.UpsertUser(testuser, &services.HostUsersInfo{
+				_, err = users.UpsertUser(testuser, services.HostUsersInfo{
 					Groups: tc.secondGroups,
 					Mode:   types.CreateHostUserMode_HOST_USER_MODE_KEEP,
 				})

lib/srv/sess.go

@@ -283,7 +283,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.UpsertUser(ctx.Identity.Login, ui)
+	userCloser, err := s.users.UpsertUser(ctx.Identity.Login, *ui)
 	if userCloser != nil {
 		ctx.AddCloser(userCloser)
 	}

lib/srv/usermgmt.go

@@ -26,6 +26,7 @@ import (
 	"maps"
 	"os/user"
 	"regexp"
+	"slices"
 	"strings"
 	"syscall"
 	"time"
@@ -106,6 +107,8 @@ type HostUsersBackend interface {
 	DeleteUser(name string) error
 	// CreateHomeDirectory creates the users home directory and copies in /etc/skel
 	CreateHomeDirectory(userHome string, uid, gid string) error
+	// GetDefaultHomeDirectory returns the default home directory path for the given user
+	GetDefaultHomeDirectory(user string) (string, error)
 }
 
 type userCloser struct {
@@ -148,7 +151,7 @@ func (*HostSudoersNotImplemented) RemoveSudoers(name string) error {
 
 type HostUsers interface {
 	// UpsertUser creates a temporary Teleport user in the TeleportServiceGroup
-	UpsertUser(name string, hostRoleInfo *services.HostUsersInfo) (io.Closer, error)
+	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
@@ -223,147 +226,71 @@ func (u *HostSudoersManagement) RemoveSudoers(name string) error {
 	return nil
 }
 
-// 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")
-	}
-
-	groupsToAdd := 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
-		}
-		groupsToAdd = append(groupsToAdd, group)
-	}
-	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
-		groupsToAdd = append(groupsToAdd, types.TeleportServiceGroup)
-	}
-	var errs []error
-	for _, group := range groupsToAdd {
-		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")
+func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo) error {
+	existingUser, err := u.backend.Lookup(name)
+	if err != nil {
+		return trace.Wrap(err)
 	}
 
-	tempUser, err := u.backend.Lookup(name)
-	if err != nil && !errors.Is(err, user.UnknownUserError(name)) {
-		return nil, trace.Wrap(err)
+	currentGroups := make(map[string]struct{}, len(ui.Groups))
+	groupIDs, err := u.backend.UserGIDs(existingUser)
+	if err != nil {
+		return trace.Wrap(err)
 	}
 
-	if tempUser != nil {
-		// Collect actions that need to be done together under a lock on the user.
-		actionsUnderLock := make([]func() error, 0, 2)
-		doWithUserLock := func() error {
-			if len(actionsUnderLock) == 0 {
-				return nil
-			}
-
-			return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
-				for _, action := range actionsUnderLock {
-					if err := action(); err != nil {
-						return trace.Wrap(err)
-					}
-				}
-				return nil
-			}))
-		}
-
-		// Get the user's current groups.
-		currentGroups := make(map[string]struct{}, len(groupsToAdd))
-		groupIds, err := u.backend.UserGIDs(tempUser)
+	for _, groupID := range groupIDs {
+		group, err := u.backend.LookupGroupByID(groupID)
 		if err != nil {
-			return nil, trace.Wrap(err)
-		}
-		for _, groupId := range groupIds {
-			group, err := u.backend.LookupGroupByID(groupId)
-			if err != nil {
-				return nil, trace.Wrap(err)
-			}
-			currentGroups[group.Name] = struct{}{}
+			return trace.Wrap(err)
 		}
 
-		// Get the groups that the user should end up with, including the primary group.
-		finalGroups := make(map[string]struct{}, len(groupsToAdd)+1)
-		for _, group := range groupsToAdd {
-			finalGroups[group] = struct{}{}
-		}
-		primaryGroup, err := u.backend.LookupGroupByID(tempUser.Gid)
-		if err != nil {
-			return nil, trace.Wrap(err)
-		}
-		finalGroups[primaryGroup.Name] = struct{}{}
+		currentGroups[group.Name] = struct{}{}
+	}
 
-		// Check if the user's groups need to be updated.
-		if !maps.Equal(currentGroups, finalGroups) {
-			actionsUnderLock = append(actionsUnderLock, func() error {
-				return trace.Wrap(u.backend.SetUserGroups(name, groupsToAdd))
-			})
-		}
+	_, hasSystemGroup := currentGroups[types.TeleportServiceGroup]
+	if hasSystemGroup && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
+		ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
+	}
 
-		systemGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup)
-		if err != nil {
-			if isUnknownGroupError(err, types.TeleportServiceGroup) {
-				// 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 {
-				found = true
-				break
-			}
-		}
-		if !found {
-			// User isn't managed by Teleport, so we don't need to update interaction time.
-			return nil, trace.Wrap(doWithUserLock())
-		}
+	finalGroups := make(map[string]struct{}, len(ui.Groups))
+	for _, group := range ui.Groups {
+		finalGroups[group] = struct{}{}
+	}
 
-		actionsUnderLock = append(actionsUnderLock, func() error {
-			return trace.Wrap(u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()))
-		})
-		if err := doWithUserLock(); err != nil {
-			return nil, trace.Wrap(err)
-		}
-		// try to delete even if the user already exists as only users
-		// in the teleport-system group will be deleted and this way
-		// if a user creates multiple sessions the account will
-		// succeed in deletion
-		return &userCloser{
-			username: name,
-			users:    u,
-			backend:  u.backend,
-		}, nil
+	primaryGroup, err := u.backend.LookupGroupByID(existingUser.Gid)
+	if err != nil {
+		return trace.Wrap(err)
 	}
+	finalGroups[primaryGroup.Name] = struct{}{}
 
+	if !maps.Equal(currentGroups, finalGroups) {
+		return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
+			return trace.Wrap(u.backend.SetUserGroups(name, ui.Groups))
+		}))
+	}
+
+	return nil
+}
+
+func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo) error {
 	var home string
-	if ui.Mode != types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
-		//nolint:staticcheck // SA4023. False positive on macOS.
-		home, err = readDefaultHome(name)
-		//nolint:staticcheck // SA4023. False positive on macOS.
+	var err error
+	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
+		ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
+	} else {
+		home, err = u.backend.GetDefaultHomeDirectory(name)
 		if err != nil {
-			return nil, trace.Wrap(err)
+			return trace.Wrap(err)
 		}
 	}
 
-	err = u.doWithUserLock(func(_ types.SemaphoreLease) error {
+	return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
 		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
 			err := u.backend.CreateGroup(name, ui.GID)
@@ -372,7 +299,7 @@ func (u *HostUserManagement) UpsertUser(name string, ui *services.HostUsersInfo)
 			}
 		}
 
-		err = u.backend.CreateUser(name, groupsToAdd, home, ui.UID, ui.GID)
+		err = u.backend.CreateUser(name, ui.Groups, home, ui.UID, ui.GID)
 		if err != nil && !trace.IsAlreadyExists(err) {
 			return trace.WrapWithMessage(err, "error while creating user")
 		}
@@ -389,22 +316,44 @@ func (u *HostUserManagement) UpsertUser(name string, ui *services.HostUsersInfo)
 		}
 
 		return nil
-	})
-	if err != nil {
-		return nil, trace.Wrap(err)
+	}))
+}
+
+// UpsertUser creates a temporary Teleport user in the TeleportServiceGroup
+func (u *HostUserManagement) UpsertUser(name string, ui services.HostUsersInfo) (io.Closer, error) {
+	var groupErrs []error
+	// cloning to prevent unintended mutation of passed in Groups slice
+	ui.Groups = slices.Clone(ui.Groups)
+	for _, group := range append(ui.Groups, types.TeleportServiceGroup) {
+		if err := u.createGroupIfNotExist(group); err != nil {
+			groupErrs = append(groupErrs, err)
+		}
 	}
 
-	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_KEEP {
-		return nil, nil
+	if err := trace.NewAggregate(groupErrs...); err != nil {
+		return nil, trace.WrapWithMessage(err, "error while creating groups")
 	}
 
-	closer := &userCloser{
-		username: name,
-		users:    u,
-		backend:  u.backend,
+	var closer io.Closer
+	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
+		closer = &userCloser{
+			username: name,
+			users:    u,
+			backend:  u.backend,
+		}
+	}
+
+	if err := u.updateUser(name, ui); err != nil {
+		if !errors.Is(err, user.UnknownUserError(name)) {
+			return nil, trace.Wrap(err)
+		}
+
+		if err := u.createUser(name, ui); err != nil {
+			return nil, trace.Wrap(err)
+		}
 	}
 
-	return closer, trace.Wrap(err)
+	return closer, nil
 }
 
 func (u *HostUserManagement) doWithUserLock(f func(types.SemaphoreLease) error) error {

lib/srv/usermgmt_linux.go

@@ -200,16 +200,16 @@ func readDefaultKey(key string, defaultValue string) (string, error) {
 	return defaultValue, nil
 }
 
-// readDefaultHome reads /etc/default/useradd for the HOME key,
+// GetDefaultHomeDirectory reads /etc/default/useradd for the HOME key,
 // defaulting to "/home" and join it with the user for the user
 // home directory
-func readDefaultHome(user string) (string, error) {
+func (u *HostUsersProvisioningBackend) GetDefaultHomeDirectory(user string) (string, error) {
 	const defaultHome = "/home"
 	home, err := readDefaultKey("HOME", defaultHome)
 	return filepath.Join(home, user), trace.Wrap(err)
 }
 
-// readDefaultHome reads /etc/default/useradd for the SKEL key, defaulting to "/etc/skel"
+// readDefaultSkel reads /etc/default/useradd for the SKEL key, defaulting to "/etc/skel"
 func readDefaultSkel() (string, error) {
 	const defaultSkel = "/etc/skel"
 	skel, err := readDefaultKey("SKEL", defaultSkel)

lib/srv/usermgmt_other.go

@@ -34,8 +34,3 @@ func newHostUsersBackend() (HostUsersBackend, error) {
 func newHostSudoersBackend(_ string) (HostSudoersBackend, error) {
 	return nil, trace.NotImplemented("Host user creation management is only supported on linux")
 }
-
-//nolint:staticcheck // intended to always return an error for non-linux builds
-func readDefaultHome(user string) (string, error) {
-	return "", trace.NotImplemented("readDefaultHome is only supported on linux")
-}