Skip to content

Commit

Permalink
Update host user groups for existing users (#41919) (#42884)
Browse files Browse the repository at this point in the history 
This change fixes a bug in host user creation where Teleport would not
update the groups of a returning user if groups were changed in
the user's role.
  • Loading branch information
@atburke
atburke committed Jun 14, 2024
1 parent 041a1ac commit f81d601
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 71 deletions.
86 changes: 78 additions & 8 deletions integration/hostuser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"os/exec"
"os/user"
"path/filepath"
"slices"
"testing"

"github.com/gravitational/trace"
Expand Down Expand Up @@ -180,7 +181,7 @@ func TestRootHostUsersBackend(t *testing.T) {
})
}

func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) {
func getUserGroups(t *testing.T, u *user.User) []string {
var userGroups []string
userGids, err := u.GroupIds()
require.NoError(t, err)
Expand All @@ -189,7 +190,11 @@ func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) {
require.NoError(t, err)
userGroups = append(userGroups, group.Name)
}
require.Subset(t, userGroups, requiredGroups)
return userGroups
}

func requireUserInGroups(t *testing.T, u *user.User, requiredGroups []string) {
require.Subset(t, getUserGroups(t, u), requiredGroups)
}

func cleanupUsersAndGroups(users []string, groups []string) func() {
Expand Down Expand Up @@ -219,7 +224,7 @@ func TestRootHostUsers(t *testing.T) {
users := srv.NewHostUsers(context.Background(), presence, "host_uuid")

testGroups := []string{"group1", "group2"}
closer, err := users.CreateUser(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)
Expand All @@ -244,7 +249,7 @@ func TestRootHostUsers(t *testing.T) {
_, err := user.LookupGroupId(testGID)
require.ErrorIs(t, err, user.UnknownGroupIdError(testGID))

closer, err := users.CreateUser(testuser, &services.HostUsersInfo{
closer, err := users.UpsertUser(testuser, &services.HostUsersInfo{
Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
UID: testUID,
GID: testGID,
Expand Down Expand Up @@ -273,7 +278,7 @@ func TestRootHostUsers(t *testing.T) {
expectedHome := filepath.Join("/home", testuser)
require.NoDirExists(t, expectedHome)

closer, err := users.CreateUser(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))
Expand Down Expand Up @@ -303,7 +308,7 @@ func TestRootHostUsers(t *testing.T) {
os.Remove(sudoersPath(testuser, uuid))
host.UserDel(testuser)
})
closer, err := users.CreateUser(testuser,
closer, err := users.UpsertUser(testuser,
&services.HostUsersInfo{
Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
})
Expand Down Expand Up @@ -334,12 +339,12 @@ func TestRootHostUsers(t *testing.T) {

deleteableUsers := []string{"teleport-user1", "teleport-user2", "teleport-user3"}
for _, user := range deleteableUsers {
_, err := users.CreateUser(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.CreateUser("teleport-user4", &services.HostUsersInfo{
closer, err := users.UpsertUser("teleport-user4", &services.HostUsersInfo{
Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
})
require.NoError(t, err)
Expand All @@ -360,4 +365,69 @@ func TestRootHostUsers(t *testing.T) {
require.Equal(t, err, user.UnknownUserError(us))
}
})

t.Run("test update changed groups", func(t *testing.T) {
tests := []struct {
name string
firstGroups []string
secondGroups []string
}{
{
name: "add groups",
secondGroups: []string{"group1", "group2"},
},
{
name: "delete groups",
firstGroups: []string{"group1", "group2"},
},
{
name: "change groups",
firstGroups: []string{"group1", "group2"},
secondGroups: []string{"group2", "group3"},
},
{
name: "no change",
firstGroups: []string{"group1", "group2"},
secondGroups: []string{"group2", "group1"},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
allGroups := make([]string, 0, len(tc.firstGroups)+len(tc.secondGroups))
allGroups = append(allGroups, tc.firstGroups...)
allGroups = append(allGroups, tc.secondGroups...)
t.Cleanup(cleanupUsersAndGroups([]string{testuser}, allGroups))

// 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{
Groups: tc.firstGroups,
Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
})
require.NoError(t, err)
u, err := user.Lookup(testuser)
require.NoError(t, err)
requireUserInGroups(t, u, tc.firstGroups)

// Verify that the user is updated with the second set of groups.
_, err = users.UpsertUser(testuser, &services.HostUsersInfo{
Groups: tc.secondGroups,
Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
})
require.NoError(t, err)
u, err = user.Lookup(testuser)
require.NoError(t, err)
requireUserInGroups(t, u, tc.secondGroups)

// Verify that the appropriate groups form the first set were deleted.
userGroups := getUserGroups(t, u)
for _, group := range tc.firstGroups {
if !slices.Contains(tc.secondGroups, group) {
require.NotContains(t, userGroups, group)
}
}
})
}
})
}
2 changes: 1 addition & 1 deletion lib/srv/sess.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.CreateUser(ctx.Identity.Login, ui)
userCloser, err := s.users.UpsertUser(ctx.Identity.Login, ui)
if userCloser != nil {
ctx.AddCloser(userCloser)
}
Expand Down
107 changes: 64 additions & 43 deletions lib/srv/usermgmt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ type HostUsersBackend interface {
LookupGroup(group string) (*user.Group, error)
// LookupGroupByID retrieves a group by its ID.
LookupGroupByID(gid string) (*user.Group, error)
// SetUserGroups sets a user's groups, replacing their existing groups.
SetUserGroups(name string, groups []string) error
// CreateGroup creates a group on a host.
CreateGroup(group string, gid string) error
// CreateUser creates a user on a host.
Expand Down Expand Up @@ -145,8 +147,8 @@ func (*HostSudoersNotImplemented) RemoveSudoers(name string) error {
}

type HostUsers interface {
// CreateUser creates a temporary Teleport user in the TeleportServiceGroup
CreateUser(name string, hostRoleInfo *services.HostUsersInfo) (io.Closer, error)
// UpsertUser creates a temporary Teleport user in the TeleportServiceGroup
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
Expand Down Expand Up @@ -221,29 +223,71 @@ func (u *HostSudoersManagement) RemoveSudoers(name string) error {
return nil
}

// CreateUser creates a temporary Teleport user in the TeleportServiceGroup
func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo) (io.Closer, error) {
// 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")
}

groups := 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
}
groups = append(groups, group)
}
if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
groups = append(groups, types.TeleportServiceGroup)
}
var errs []error
for _, group := range groups {
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")
}

tempUser, err := u.backend.Lookup(name)
if err != nil && err != user.UnknownUserError(name) {
if err != nil && !errors.Is(err, user.UnknownUserError(name)) {
return nil, trace.Wrap(err)
}

if tempUser != nil {
gids, err := u.backend.UserGIDs(tempUser)
if err != nil {
return nil, trace.Wrap(err)
// Collect actions that need to be done together under a lock on the user.
actionsUnderLock := []func() error{
func() error {
// If the user exists, set user groups again as they might have changed.
return trace.Wrap(u.backend.SetUserGroups(name, groups))
},
}
doWithUserLock := func() error {
return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
for _, action := range actionsUnderLock {
if err := action(); err != nil {
return trace.Wrap(err)
}
}
return nil
}))
}

systemGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup)
if err != nil {
if isUnknownGroupError(err, types.TeleportServiceGroup) {
return nil, trace.AlreadyExists("User %q already exists, however no users are currently managed by teleport", name)
// 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 {
Expand All @@ -252,16 +296,14 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo)
}
}
if !found {
return nil, trace.AlreadyExists("User %q already exists and is not managed by teleport", name)
// User isn't managed by Teleport, so we don't need to update interaction time.
return nil, trace.Wrap(doWithUserLock())
}

err = u.doWithUserLock(func(_ types.SemaphoreLease) error {
if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil {
return trace.Wrap(err)
}
return nil
actionsUnderLock = append(actionsUnderLock, func() error {
return trace.Wrap(u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()))
})
if err != nil {
if err := doWithUserLock(); err != nil {
return nil, trace.Wrap(err)
}
// try to delete even if the user already exists as only users
Expand All @@ -272,30 +314,7 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo)
username: name,
users: u,
backend: u.backend,
}, trace.AlreadyExists("User %q already exists", name)
}

groups := 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
}
groups = append(groups, group)
}
if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
groups = append(groups, types.TeleportServiceGroup)
}
var errs []error
for _, group := range groups {
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")
}, nil
}

var home string
Expand All @@ -307,8 +326,10 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo)
}

err = u.doWithUserLock(func(_ types.SemaphoreLease) error {
if err := u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()); err != nil {
return trace.Wrap(err)
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
Expand Down Expand Up @@ -341,7 +362,7 @@ func (u *HostUserManagement) CreateUser(name string, ui *services.HostUsersInfo)
}

if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_KEEP {
return nil, trace.Wrap(err)
return nil, nil
}

closer := &userCloser{
Expand Down
6 changes: 6 additions & 0 deletions lib/srv/usermgmt_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ func (*HostUsersProvisioningBackend) LookupGroupByID(gid string) (*user.Group, e
return user.LookupGroupId(gid)
}

// SetUserGroups sets a user's groups, replacing their existing groups.
func (*HostUsersProvisioningBackend) SetUserGroups(name string, groups []string) error {
_, err := host.SetUserGroups(name, groups)
return trace.Wrap(err)
}

// GetAllUsers returns a full list of users present on a system
func (*HostUsersProvisioningBackend) GetAllUsers() ([]string, error) {
users, _, err := host.GetAllUsers()
Expand Down

0 comments on commit f81d601

Please sign in to comment.