From ebb16cca141cae75150072bfb9d556b020508b4e Mon Sep 17 00:00:00 2001
From: Erik Tate <erik.tate@goteleport.com>
Date: Thu, 29 Aug 2024 14:22:11 -0400
Subject: [PATCH] resolves the correct primary gid for logins that share a name
 with a group in host_groups

---
 integration/hostuser_test.go |  2 +-
 lib/srv/usermgmt.go          | 37 +++++++++++++++++++++-----
 lib/srv/usermgmt_test.go     | 50 ++++++++++++++++++++++++++++++++++++
 lib/utils/host/hostusers.go  |  8 ++++++
 4 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/integration/hostuser_test.go b/integration/hostuser_test.go
index 25ea1c0c161e7..0dc3533892380 100644
--- a/integration/hostuser_test.go
+++ b/integration/hostuser_test.go
@@ -305,7 +305,7 @@ func TestRootHostUsers(t *testing.T) {
 
 		t.Cleanup(func() {
 			os.Remove(sudoersPath(testuser, uuid))
-			host.UserDel(testuser)
+			cleanupUsersAndGroups([]string{testuser}, nil)
 		})
 		closer, err := users.UpsertUser(testuser,
 			services.HostUsersInfo{
diff --git a/lib/srv/usermgmt.go b/lib/srv/usermgmt.go
index 7059b5050c2c6..1939a1b85fed1 100644
--- a/lib/srv/usermgmt.go
+++ b/lib/srv/usermgmt.go
@@ -302,6 +302,32 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)
 	return nil
 }
 
+func (u *HostUserManagement) resolveGID(username string, groups []string, gid string) (string, error) {
+	if gid != "" {
+		// ensure user's primary group exists if a gid is explicitly provided
+		err := u.backend.CreateGroup(username, gid)
+		if err != nil && !trace.IsAlreadyExists(err) {
+			return "", trace.Wrap(err)
+		}
+
+		return gid, nil
+	}
+
+	// user's without an explicit gid should use the group that shares their login
+	// name if defined, otherwise user creation will fail due to their primary group
+	// already existing
+	if slices.Contains(groups, username) {
+		return username, nil
+	}
+
+	// avoid automatic assignment of groups not defined in the role
+	if _, err := u.backend.LookupGroup(username); err == nil {
+		return "", trace.AlreadyExists("host login %q conflicts with an existing group that is not defined in user's role, either add %q to host_groups or explicitly assign a GID", username, username)
+	}
+
+	return "", nil
+}
+
 func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo) error {
 	var home string
 	var err error
@@ -324,15 +350,12 @@ func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo)
 			}
 		}
 
-		if ui.GID != "" {
-			// if gid is specified a group must already exist
-			err := u.backend.CreateGroup(name, ui.GID)
-			if err != nil && !trace.IsAlreadyExists(err) {
-				return trace.Wrap(err)
-			}
+		gid, err := u.resolveGID(name, ui.Groups, ui.GID)
+		if err != nil {
+			return trace.Wrap(err)
 		}
 
-		err = u.backend.CreateUser(name, ui.Groups, home, ui.UID, ui.GID)
+		err = u.backend.CreateUser(name, ui.Groups, home, ui.UID, gid)
 		if err != nil && !trace.IsAlreadyExists(err) {
 			return trace.WrapWithMessage(err, "error while creating user")
 		}
diff --git a/lib/srv/usermgmt_test.go b/lib/srv/usermgmt_test.go
index 753e409454d28..03df578b5e35e 100644
--- a/lib/srv/usermgmt_test.go
+++ b/lib/srv/usermgmt_test.go
@@ -654,3 +654,53 @@ func Test_AllowExplicitlyManageExistingUsers(t *testing.T) {
 	assert.NotContains(t, backend.users["bob"], types.TeleportKeepGroup)
 
 }
+
+func TestCreateUserWithExistingPrimaryGroup(t *testing.T) {
+	t.Parallel()
+	backend := newTestUserMgmt()
+	bk, err := memory.New(memory.Config{})
+	require.NoError(t, err)
+	pres := local.NewPresenceService(bk)
+	users := HostUserManagement{
+		backend: backend,
+		storage: pres,
+	}
+
+	existingGroups := []string{"alice", "simon"}
+	for _, group := range existingGroups {
+		require.NoError(t, backend.CreateGroup(group, ""))
+	}
+
+	userinfo := services.HostUsersInfo{
+		Groups: []string{},
+		Mode:   types.CreateHostUserMode_HOST_USER_MODE_DROP,
+	}
+
+	// create a user without an existing primary group
+	closer, err := users.UpsertUser("bob", userinfo)
+	assert.NoError(t, err)
+	assert.NotEqual(t, nil, closer)
+	assert.Zero(t, backend.setUserGroupsCalls)
+
+	// create a user with primary group defined in userinfo.Groups, but not yet on the host
+	userinfo.Groups = []string{"fred"}
+	closer, err = users.UpsertUser("fred", userinfo)
+	assert.NoError(t, err)
+	assert.NotEqual(t, nil, closer)
+	assert.Zero(t, backend.setUserGroupsCalls)
+
+	// create a user with primary group defined in userinfo.Groups that already exists on the host
+	userinfo.Groups = []string{"alice"}
+	closer, err = users.UpsertUser("alice", userinfo)
+	assert.NoError(t, err)
+	assert.NotEqual(t, nil, closer)
+	assert.Zero(t, backend.setUserGroupsCalls)
+
+	// create a user with primary group that already exists on the host but is not defined in userinfo.Groups
+	userinfo.Groups = []string{""}
+	closer, err = users.UpsertUser("simon", userinfo)
+	assert.True(t, trace.IsAlreadyExists(err))
+	assert.Contains(t, err.Error(), "conflicts with an existing group")
+	assert.Equal(t, nil, closer)
+	assert.Zero(t, backend.setUserGroupsCalls)
+}
diff --git a/lib/utils/host/hostusers.go b/lib/utils/host/hostusers.go
index 5bd0278f26f4d..89312750e9b06 100644
--- a/lib/utils/host/hostusers.go
+++ b/lib/utils/host/hostusers.go
@@ -24,6 +24,7 @@ import (
 	"os"
 	"os/exec"
 	"os/user"
+	"slices"
 	"strings"
 
 	"github.com/gravitational/trace"
@@ -81,6 +82,13 @@ func UserAdd(username string, groups []string, home, uid, gid string) (exitCode
 		home = string(os.PathSeparator)
 	}
 
+	// user's without an explicit gid should be added to the group that shares their
+	// login name if it's defined, otherwise user creation will fail because their primary
+	// group already exists
+	if slices.Contains(groups, username) && gid == "" {
+		gid = username
+	}
+
 	// useradd ---no-create-home (username) (groups)...
 	args := []string{"--no-create-home", "--home-dir", home, username}
 	if len(groups) != 0 {