[v16] Prevent group reconciliation for existing users

api/types/constants.go

@@ -1275,10 +1275,14 @@ var KubernetesClusterWideResourceKinds = []string{
 }
 
 const (
-	// TeleportServiceGroup is a default group that users of the
-	// teleport automated user provisioning system get added to so
-	// already existing users are not deleted
-	TeleportServiceGroup = "teleport-system"
+	// TeleportDropGroup is a default group that users of the teleport automated user
+	// provisioning system get added to when provisioned in INSECURE_DROP mode. This
+	// prevents already existing users from being tampered with or deleted.
+	TeleportDropGroup = "teleport-system"
+	// TeleportKeepGroup is a default group that users of the teleport automated user
+	// provisioning system get added to when provisioned in KEEP mode. This prevents
+	// already existing users from being tampered with or deleted.
+	TeleportKeepGroup = "teleport-keep"
 )
 
 const (

integration/hostuser_test.go

@@ -226,7 +226,7 @@ func TestRootHostUsers(t *testing.T) {
 		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)
+		testGroups = append(testGroups, types.TeleportDropGroup)
 		t.Cleanup(cleanupUsersAndGroups([]string{testuser}, testGroups))
 
 		u, err := user.Lookup(testuser)
@@ -255,7 +255,7 @@ func TestRootHostUsers(t *testing.T) {
 		})
 		require.NoError(t, err)
 
-		t.Cleanup(cleanupUsersAndGroups([]string{testuser}, []string{types.TeleportServiceGroup}))
+		t.Cleanup(cleanupUsersAndGroups([]string{testuser}, []string{types.TeleportDropGroup}))
 
 		group, err := user.LookupGroupId(testGID)
 		require.NoError(t, err)
@@ -280,7 +280,7 @@ func TestRootHostUsers(t *testing.T) {
 		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))
+		t.Cleanup(cleanupUsersAndGroups([]string{testuser}, []string{types.TeleportKeepGroup}))
 
 		u, err := user.Lookup(testuser)
 		require.NoError(t, err)
@@ -351,7 +351,7 @@ func TestRootHostUsers(t *testing.T) {
 
 		t.Cleanup(cleanupUsersAndGroups(
 			[]string{"teleport-user1", "teleport-user2", "teleport-user3", "teleport-user4"},
-			[]string{types.TeleportServiceGroup}))
+			[]string{types.TeleportDropGroup, types.TeleportKeepGroup}))
 
 		err = users.DeleteAllUsers()
 		require.NoError(t, err)

lib/srv/reexec.go

@@ -570,9 +570,9 @@ func (o *osWrapper) startNewParker(ctx context.Context, credential *syscall.Cred
 		return nil
 	}
 
-	group, err := o.LookupGroup(types.TeleportServiceGroup)
+	group, err := o.LookupGroup(types.TeleportDropGroup)
 	if err != nil {
-		if isUnknownGroupError(err, types.TeleportServiceGroup) {
+		if isUnknownGroupError(err, types.TeleportDropGroup) {
 			// The service group doesn't exist. Auto-provision is disabled, do nothing.
 			return nil
 		}
@@ -593,7 +593,7 @@ func (o *osWrapper) startNewParker(ctx context.Context, credential *syscall.Cred
 	}
 
 	if !found {
-		// Check if the new user guid matches the TeleportServiceGroup. If not
+		// Check if the new user guid matches the TeleportDropGroup. If not
 		// this user hasn't been created by Teleport, and we don't need the parker.
 		return nil
 	}

lib/srv/reexec_test.go

@@ -94,8 +94,8 @@ func TestStartNewParker(t *testing.T) {
 			newOsPack: func(t *testing.T) (*osWrapper, func()) {
 				return &osWrapper{
 					LookupGroup: func(name string) (*user.Group, error) {
-						require.Equal(t, types.TeleportServiceGroup, name)
-						return nil, user.UnknownGroupError(types.TeleportServiceGroup)
+						require.Equal(t, types.TeleportDropGroup, name)
+						return nil, user.UnknownGroupError(types.TeleportDropGroup)
 					},
 				}, func() {}
 			},
@@ -106,7 +106,7 @@ func TestStartNewParker(t *testing.T) {
 			newOsPack: func(t *testing.T) (*osWrapper, func()) {
 				return &osWrapper{
 					LookupGroup: func(name string) (*user.Group, error) {
-						require.Equal(t, types.TeleportServiceGroup, name)
+						require.Equal(t, types.TeleportDropGroup, name)
 						return &user.Group{Gid: "1234"}, nil
 					},
 					CommandContext: func(ctx context.Context, name string, arg ...string) *exec.Cmd {
@@ -132,7 +132,7 @@ func TestStartNewParker(t *testing.T) {
 
 				return &osWrapper{
 						LookupGroup: func(name string) (*user.Group, error) {
-							require.Equal(t, types.TeleportServiceGroup, name)
+							require.Equal(t, types.TeleportDropGroup, name)
 							return &user.Group{Gid: currentUser.Gid}, nil
 						},
 						CommandContext: func(ctx context.Context, name string, arg ...string) *exec.Cmd {

lib/srv/sess.go

@@ -287,12 +287,17 @@ func (s *SessionRegistry) TryCreateHostUser(identityContext IdentityContext) (cr
 	if trace.IsAccessDenied(err) && existsErr != nil {
 		return false, nil, trace.WrapWithMessage(err, "Insufficient permission for host user creation")
 	}
+
 	userCloser, err := s.users.UpsertUser(identityContext.Login, *ui)
-	if err != nil && !trace.IsAlreadyExists(err) {
+	if err != nil && !trace.IsAlreadyExists(err) && !errors.Is(err, unmanagedUserErr) {
 		log.Debugf("Error creating user %s: %s", identityContext.Login, err)
 		return false, nil, trace.Wrap(err)
 	}
 
+	if errors.Is(err, unmanagedUserErr) {
+		log.Warnf("User %q is not managed by teleport. Either manually delete the user from this machine or update the host_groups defined in their role to include %q. https://goteleport.com/docs/enroll-resources/server-access/guides/host-user-creation/#migrating-unmanaged-users", identityContext.Login, types.TeleportKeepGroup)
+	}
+
 	return true, userCloser, nil
 }
 

lib/srv/usermgmt.go

@@ -118,7 +118,7 @@ type userCloser struct {
 }
 
 func (u *userCloser) Close() error {
-	teleportGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup)
+	teleportGroup, err := u.backend.LookupGroup(types.TeleportDropGroup)
 	if err != nil {
 		return trace.Wrap(err)
 	}
@@ -131,15 +131,15 @@ func (u *userCloser) Close() error {
 var ErrUserLoggedIn = errors.New("User logged in error")
 
 type HostSudoers interface {
-	// WriteSudoers creates a temporary Teleport user in the TeleportServiceGroup
+	// WriteSudoers creates a temporary Teleport user in the TeleportDropGroup
 	WriteSudoers(name string, sudoers []string) error
 	// RemoveSudoers removes the users sudoer file
 	RemoveSudoers(name string) error
 }
 
 type HostSudoersNotImplemented struct{}
 
-// WriteSudoers creates a temporary Teleport user in the TeleportServiceGroup
+// WriteSudoers creates a temporary Teleport user in the TeleportDropGroup
 func (*HostSudoersNotImplemented) WriteSudoers(string, []string) error {
 	return trace.NotImplemented("host sudoers functionality not implemented on this platform")
 }
@@ -150,12 +150,12 @@ func (*HostSudoersNotImplemented) RemoveSudoers(name string) error {
 }
 
 type HostUsers interface {
-	// UpsertUser creates a temporary Teleport user in the TeleportServiceGroup
+	// UpsertUser creates a temporary Teleport user in the TeleportDropGroup
 	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
-	// DeleteAllUsers deletes all suer in the TeleportServiceGroup
+	// DeleteAllUsers deletes all suer in the TeleportDropGroup
 	DeleteAllUsers() error
 	// UserCleanup starts a periodic user deletion cleanup loop for
 	// users that failed to delete
@@ -226,7 +226,11 @@ func (u *HostSudoersManagement) RemoveSudoers(name string) error {
 	return nil
 }
 
+// unmanagedUserErr is returned when attempting to modify or interact with a user that is not managed by Teleport.
+var unmanagedUserErr = errors.New("user not managed by teleport")
+
 func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo) error {
+
 	existingUser, err := u.backend.Lookup(name)
 	if err != nil {
 		return trace.Wrap(err)
@@ -247,9 +251,35 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)
 		currentGroups[group.Name] = struct{}{}
 	}
 
-	_, hasSystemGroup := currentGroups[types.TeleportServiceGroup]
-	if hasSystemGroup && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
-		ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
+	// allow for explicit assignment of teleport-keep group in order to facilitate migrating KEEP users that existed before we added
+	// the teleport-keep group
+	migrateKeepUser := slices.Contains(ui.Groups, types.TeleportKeepGroup)
+
+	_, hasDropGroup := currentGroups[types.TeleportDropGroup]
+	_, hasKeepGroup := currentGroups[types.TeleportKeepGroup]
+	if !hasDropGroup && !hasKeepGroup && !migrateKeepUser {
+		return trace.Errorf("%q %w", name, unmanagedUserErr)
+	}
+
+	switch ui.Mode {
+	case types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP:
+		ui.Groups = append(ui.Groups, types.TeleportDropGroup)
+	case types.CreateHostUserMode_HOST_USER_MODE_KEEP:
+		if !hasKeepGroup {
+			home, err := u.backend.GetDefaultHomeDirectory(name)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+
+			if err := u.backend.CreateHomeDirectory(home, existingUser.Uid, existingUser.Gid); err != nil {
+				return trace.Wrap(err)
+			}
+		}
+
+		// no need to duplicate the group if it's already there
+		if !migrateKeepUser {
+			ui.Groups = append(ui.Groups, types.TeleportKeepGroup)
+		}
 	}
 
 	finalGroups := make(map[string]struct{}, len(ui.Groups))
@@ -275,9 +305,12 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)
 func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo) error {
 	var home string
 	var err error
-	if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
-		ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
-	} else {
+
+	switch ui.Mode {
+	case types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP:
+		ui.Groups = append(ui.Groups, types.TeleportDropGroup)
+	case types.CreateHostUserMode_HOST_USER_MODE_KEEP:
+		ui.Groups = append(ui.Groups, types.TeleportKeepGroup)
 		home, err = u.backend.GetDefaultHomeDirectory(name)
 		if err != nil {
 			return trace.Wrap(err)
@@ -319,19 +352,54 @@ func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo)
 	}))
 }
 
-// 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) {
+func (u *HostUserManagement) ensureGroupsExist(groups ...string) error {
+	var errs []error
+
+	for _, group := range groups {
 		if err := u.createGroupIfNotExist(group); err != nil {
-			groupErrs = append(groupErrs, err)
+			errs = append(errs, err)
 		}
 	}
 
-	if err := trace.NewAggregate(groupErrs...); err != nil {
-		return nil, trace.WrapWithMessage(err, "error while creating groups")
+	return trace.NewAggregate(errs...)
+}
+
+// UpsertUser creates a temporary Teleport user in the TeleportDropGroup
+func (u *HostUserManagement) UpsertUser(name string, ui services.HostUsersInfo) (io.Closer, error) {
+	// allow for explicit assignment of teleport-keep group in order to facilitate migrating KEEP users that existed before we added
+	// the teleport-keep group
+	migrateKeepUser := slices.Contains(ui.Groups, types.TeleportKeepGroup)
+	skipKeepGroup := migrateKeepUser && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP
+
+	if skipKeepGroup {
+		log.Warnf("explicit assignment of %q group is not possible in 'insecure-drop' mode", types.TeleportKeepGroup)
+	}
+
+	groupSet := make(map[string]struct{}, len(ui.Groups))
+	groups := make([]string, 0, len(ui.Groups))
+	for _, group := range ui.Groups {
+		// the TeleportDropGroup is managed automatically and should not be allowed direct assignment
+		if group == types.TeleportDropGroup {
+			continue
+		}
+
+		if skipKeepGroup && group == types.TeleportKeepGroup {
+			continue
+		}
+
+		if _, ok := groupSet[group]; !ok {
+			groupSet[group] = struct{}{}
+			groups = append(groups, group)
+		}
+	}
+	ui.Groups = groups
+
+	if err := u.ensureGroupsExist(types.TeleportDropGroup, types.TeleportKeepGroup); err != nil {
+		return nil, trace.WrapWithMessage(err, "creating teleport system groups")
+	}
+
+	if err := u.ensureGroupsExist(groups...); err != nil {
+		return nil, trace.WrapWithMessage(err, "creating configured groups")
 	}
 
 	var closer io.Closer
@@ -409,10 +477,10 @@ func (u *HostUserManagement) DeleteAllUsers() error {
 	if err != nil {
 		return trace.Wrap(err)
 	}
-	teleportGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup)
+	teleportGroup, err := u.backend.LookupGroup(types.TeleportDropGroup)
 	if err != nil {
-		if isUnknownGroupError(err, types.TeleportServiceGroup) {
-			log.Debugf("%q group not found, not deleting users", types.TeleportServiceGroup)
+		if isUnknownGroupError(err, types.TeleportDropGroup) {
+			log.Debugf("%q group not found, not deleting users", types.TeleportDropGroup)
 			return nil
 		}
 		return trace.Wrap(err)