Host User Management deletes users that are not managed by Teleport

[strideynet]

Expected behavior:

When using Drop or DropInsecure mode, only users that were created by Teleport will be deleted after they disconnect.

Current behavior:

When using Drop or DropInsecure mode, any user that logs in is deleted after they disconnect (for a second time).

Introduced by #41919

The PR included changes to ensure that the groups for existing users were reconciled on log in. The key flaw here is that the groups for any user are always updated to include the teleport-system group when the mode is set to Drop or DropInsecure. This means that after a pre-existing user logins in, they are now marked as a user that was created by Teleport. The logic should instead only add this group if the user did not already exist.

Bug details:

• Teleport version v14 (since 14.3.21), v15 (since 15.4.5), v16 (since 16.0.0)
• Recreation steps
• Debug logs

[strideynet]

Additionally, I think there's another problematic behaviour here where we are updating groups for users regardless of whether or not they were created by Teleport. Surely we only want to manage the groups of users that were created by Teleport?

[eriktate]

Additionally, I think there's another problematic behaviour here where we are updating groups for users regardless of whether or not they were created by Teleport. Surely we only want to manage the groups of users that were created by Teleport?

Do we know if this test case is a true constraint? Would we ever create a user that later needs the teleport service group attached? @rosstimothy

If so would we be comfortable with something like:

• If teleport-system is present in HostUserInfo.Groups, process the upsert regardless of existing group membership.
• If teleport-system is not present in HostUserInfo.Groups, short circuit the update portion of the upsert when user is not already part of the teleport-system group.

[strideynet]

Do we know if this test case is a true constraint? Would we ever create a user that later needs the teleport service group attached?

That test case only seems to asserts that there are no errors - it's not awfully clear to me what behaviour it is trying to assert there.

[rosstimothy]

A user should only be added to the teleport-system group if it is an ephemeral user by virtue of HOST_USER_MODE_DROP or HOST_USER_MODE_INSECURE_DROP. There is also the possibility that the local user may have already existed prior to Teleport trying to automatically create said user, and it looks like the test you linked was specifically added to address just that case, see #13662.

[eriktate]

I opted to focus on the immediate problem with deletions, but it seems worthwhile to look into preventing group modification altogether for non-teleport users. Should that be its own issue?