Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding automatic migration path for existing static host users #46804

Merged
merged 1 commit into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions api/gen/proto/go/teleport/userprovisioning/v2/statichostuser.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions api/proto/teleport/userprovisioning/v2/statichostuser.proto
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ message Matcher {
int64 gid = 6;
// default_shell is the new user's default shell
string default_shell = 7;
// take_ownership_if_user_exists will take ownership of existing, unmanaged users
bool take_ownership_if_user_exists = 8;
}

// StaticHostUserSpec is the static host user spec.
Expand Down
1 change: 0 additions & 1 deletion lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -5918,7 +5918,6 @@ func (a *Server) CleanupNotifications(ctx context.Context) {
// If this notification state is for a notification which doesn't exist in either the non-expired global notifications map or
// the non-expired user notifications map, then delete it.
if nonExpiredGlobalNotificationsByID[id] == nil && nonExpiredUserNotificationsByID[id] == nil {
fmt.Printf("\n\nTHIS IS RUN\n\n")
select {
case <-notificationsDeleteLimiter.C:
case <-ctx.Done():
Expand Down
5 changes: 5 additions & 0 deletions lib/services/access_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,11 @@ type HostUsersInfo struct {
GID string
// Shell is the default login shell for a host user
Shell string
// TakeOwnership determines whether or not an existing user should be
// taken over by teleport. This currently only applies to 'static' mode
// users, 'keep' mode users still need to assign 'teleport-keep' in the
// Groups slice in order to take ownership.
TakeOwnership bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should eventually support this on the role spec as well and consider removing the implicit migration when teleport-keep is present in a role's host_groups.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the moment does this only apply to static host users? If so, let's note that in the comment for this field to set expectations.

}

// HostUsers returns host user information matching a server or nil if
Expand Down
7 changes: 4 additions & 3 deletions lib/srv/statichostusers.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,10 @@ func (s *StaticHostUserHandler) handleNewHostUser(ctx context.Context, hostUser

slog.DebugContext(ctx, "Attempt to update matched static host user.", "login", login)
ui := services.HostUsersInfo{
Groups: createUser.Groups,
Mode: services.HostUserModeStatic,
Shell: createUser.DefaultShell,
Groups: createUser.Groups,
Mode: services.HostUserModeStatic,
Shell: createUser.DefaultShell,
TakeOwnership: createUser.TakeOwnershipIfUserExists,
}
if createUser.Uid != 0 {
ui.UID = strconv.Itoa(int(createUser.Uid))
Expand Down
19 changes: 11 additions & 8 deletions lib/srv/usermgmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,18 +254,21 @@ func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo)

// 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)
migrateKeepUser := slices.Contains(ui.Groups, types.TeleportKeepGroup) && ui.Mode == services.HostUserModeKeep
migrateStaticUser := ui.TakeOwnership && ui.Mode == services.HostUserModeStatic

_, hasDropGroup := currentGroups[types.TeleportDropGroup]
_, hasKeepGroup := currentGroups[types.TeleportKeepGroup]
_, hasStaticGroup := currentGroups[types.TeleportStaticGroup]
if !(hasDropGroup || hasKeepGroup || hasStaticGroup || migrateKeepUser) {
isManagedUser := hasDropGroup || hasKeepGroup || hasStaticGroup
if !(isManagedUser || migrateKeepUser || migrateStaticUser) {
return nil, trace.Errorf("%q %w", name, unmanagedUserErr)
}

// Do not convert/update groups from static to non-static user, and vice versa.
isStaticUser := ui.Mode == services.HostUserModeStatic
if hasStaticGroup != isStaticUser {
isStaticMode := ui.Mode == services.HostUserModeStatic
managedStaticConversion := isManagedUser && (hasStaticGroup != isStaticMode)
if managedStaticConversion {
slog.DebugContext(context.Background(),
"Aborting host user creation, can't convert between auto-provisioned and static host users.",
"login", name)
Expand Down Expand Up @@ -430,10 +433,10 @@ func (u *HostUserManagement) ensureGroupsExist(groups ...string) error {
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 != services.HostUserModeKeep
hasKeepGroup := slices.Contains(ui.Groups, types.TeleportKeepGroup)
migrateKeepUser := hasKeepGroup && ui.Mode == services.HostUserModeKeep

if skipKeepGroup {
if hasKeepGroup && !migrateKeepUser {
log.Warnf("explicit assignment of %q group is not possible in 'insecure-drop' mode", types.TeleportKeepGroup)
}

Expand All @@ -445,7 +448,7 @@ func (u *HostUserManagement) UpsertUser(name string, ui services.HostUsersInfo)
case types.TeleportDropGroup, types.TeleportStaticGroup:
continue
case types.TeleportKeepGroup:
if skipKeepGroup {
if !migrateKeepUser {
continue
}
}
Expand Down
22 changes: 18 additions & 4 deletions lib/srv/usermgmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -638,15 +638,16 @@ func Test_DontUpdateUnmanagedUsers(t *testing.T) {
}
}

// teleport-keep can be included explicitly in the Groups slice in order to flag an
// existing user as being managed by teleport
// teleport-keep can be included explicitly in the Groups slice, or TakeOwnership can be set on HostUsersInfo,
// in order to flag an existing user as being managed by teleport.
func Test_AllowExplicitlyManageExistingUsers(t *testing.T) {
t.Parallel()

allGroups := []string{"foo", types.TeleportKeepGroup, types.TeleportDropGroup}
users, backend := initBackend(t, allGroups)

assert.NoError(t, backend.CreateUser("alice-keep", []string{}, host.UserOpts{}))
assert.NoError(t, backend.CreateUser("alice-static", []string{}, host.UserOpts{}))
assert.NoError(t, backend.CreateUser("alice-drop", []string{}, host.UserOpts{}))
userinfo := services.HostUsersInfo{
Groups: slices.Clone(allGroups),
Expand All @@ -662,20 +663,33 @@ func Test_AllowExplicitlyManageExistingUsers(t *testing.T) {
assert.ElementsMatch(t, allGroups[:2], backend.users["alice-keep"])
assert.NotContains(t, backend.users["alice-keep"], types.TeleportDropGroup)

// Take ownership of existing user when in STATIC mode
userinfo.Mode = services.HostUserModeStatic
userinfo.TakeOwnership = true
closer, err = users.UpsertUser("alice-static", userinfo)
assert.NoError(t, err)
assert.Equal(t, nil, closer)
assert.Equal(t, 2, backend.setUserGroupsCalls)
assert.Contains(t, backend.users["alice-static"], "foo")
assert.Contains(t, backend.users["alice-static"], types.TeleportStaticGroup)
assert.NotContains(t, backend.users["alice-static"], types.TeleportKeepGroup)
assert.NotContains(t, backend.users["alice-static"], types.TeleportDropGroup)

// Don't take ownership of existing user when in DROP mode
userinfo.Mode = services.HostUserModeDrop
userinfo.TakeOwnership = false
closer, err = users.UpsertUser("alice-drop", userinfo)
assert.ErrorIs(t, err, unmanagedUserErr)
assert.Equal(t, nil, closer)
assert.Equal(t, 1, backend.setUserGroupsCalls)
assert.Equal(t, 2, backend.setUserGroupsCalls)
assert.Empty(t, backend.users["alice-drop"])

// Don't assign teleport-keep to users created in DROP mode
userinfo.Mode = services.HostUserModeDrop
closer, err = users.UpsertUser("bob", userinfo)
assert.NoError(t, err)
assert.NotEqual(t, nil, closer)
assert.Equal(t, 1, backend.setUserGroupsCalls)
assert.Equal(t, 2, backend.setUserGroupsCalls)
assert.ElementsMatch(t, []string{"foo", types.TeleportDropGroup}, backend.users["bob"])
assert.NotContains(t, backend.users["bob"], types.TeleportKeepGroup)
}
Expand Down
Loading