Skip to content

Commit

Permalink
prevent teleport-system group assignment for users created (#45594)
Browse files Browse the repository at this point in the history 
outside of INSECURE_DROP mode
  • Loading branch information
@eriktate
eriktate authored Aug 19, 2024
1 parent 8987252 commit 5106fa5
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 162 deletions.
16 changes: 8 additions & 8 deletions integration/hostuser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func TestRootHostUsers(t *testing.T) {
users := srv.NewHostUsers(context.Background(), presence, "host_uuid")

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

closer, err := users.UpsertUser(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 @@ -272,7 +272,7 @@ func TestRootHostUsers(t *testing.T) {
expectedHome := filepath.Join("/home", testuser)
require.NoDirExists(t, expectedHome)

closer, err := users.UpsertUser(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 +303,7 @@ func TestRootHostUsers(t *testing.T) {
host.UserDel(testuser)
})
closer, err := users.UpsertUser(testuser,
&services.HostUsersInfo{
services.HostUsersInfo{
Mode: types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP,
})
require.NoError(t, err)
Expand Down Expand Up @@ -333,12 +333,12 @@ func TestRootHostUsers(t *testing.T) {

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

// 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{
_, err := users.UpsertUser(testuser, services.HostUsersInfo{
Groups: tc.firstGroups,
Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
})
Expand All @@ -402,7 +402,7 @@ func TestRootHostUsers(t *testing.T) {
requireUserInGroups(t, u, tc.firstGroups)

// Verify that the user is updated with the second set of groups.
_, err = users.UpsertUser(testuser, &services.HostUsersInfo{
_, err = users.UpsertUser(testuser, services.HostUsersInfo{
Groups: tc.secondGroups,
Mode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
})
Expand Down
2 changes: 1 addition & 1 deletion lib/srv/sess.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,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.UpsertUser(ctx.Identity.Login, ui)
userCloser, err := s.users.UpsertUser(ctx.Identity.Login, *ui)
if userCloser != nil {
ctx.AddCloser(userCloser)
}
Expand Down
211 changes: 80 additions & 131 deletions lib/srv/usermgmt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"maps"
"os/user"
"regexp"
"slices"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -106,6 +107,8 @@ type HostUsersBackend interface {
DeleteUser(name string) error
// CreateHomeDirectory creates the users home directory and copies in /etc/skel
CreateHomeDirectory(userHome string, uid, gid string) error
// GetDefaultHomeDirectory returns the default home directory path for the given user
GetDefaultHomeDirectory(user string) (string, error)
}

type userCloser struct {
Expand Down Expand Up @@ -148,7 +151,7 @@ func (*HostSudoersNotImplemented) RemoveSudoers(name string) error {

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

// 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")
}

groupsToAdd := 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
}
groupsToAdd = append(groupsToAdd, group)
}
if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
groupsToAdd = append(groupsToAdd, types.TeleportServiceGroup)
}
var errs []error
for _, group := range groupsToAdd {
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")
func (u *HostUserManagement) updateUser(name string, ui services.HostUsersInfo) error {
existingUser, err := u.backend.Lookup(name)
if err != nil {
return trace.Wrap(err)
}

tempUser, err := u.backend.Lookup(name)
if err != nil && !errors.Is(err, user.UnknownUserError(name)) {
return nil, trace.Wrap(err)
currentGroups := make(map[string]struct{}, len(ui.Groups))
groupIDs, err := u.backend.UserGIDs(existingUser)
if err != nil {
return trace.Wrap(err)
}

if tempUser != nil {
// Collect actions that need to be done together under a lock on the user.
actionsUnderLock := make([]func() error, 0, 2)
doWithUserLock := func() error {
if len(actionsUnderLock) == 0 {
return nil
}

return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
for _, action := range actionsUnderLock {
if err := action(); err != nil {
return trace.Wrap(err)
}
}
return nil
}))
}

// Get the user's current groups.
currentGroups := make(map[string]struct{}, len(groupsToAdd))
groupIds, err := u.backend.UserGIDs(tempUser)
for _, groupID := range groupIDs {
group, err := u.backend.LookupGroupByID(groupID)
if err != nil {
return nil, trace.Wrap(err)
}
for _, groupId := range groupIds {
group, err := u.backend.LookupGroupByID(groupId)
if err != nil {
return nil, trace.Wrap(err)
}
currentGroups[group.Name] = struct{}{}
return trace.Wrap(err)
}

// Get the groups that the user should end up with, including the primary group.
finalGroups := make(map[string]struct{}, len(groupsToAdd)+1)
for _, group := range groupsToAdd {
finalGroups[group] = struct{}{}
}
primaryGroup, err := u.backend.LookupGroupByID(tempUser.Gid)
if err != nil {
return nil, trace.Wrap(err)
}
finalGroups[primaryGroup.Name] = struct{}{}
currentGroups[group.Name] = struct{}{}
}

// Check if the user's groups need to be updated.
if !maps.Equal(currentGroups, finalGroups) {
actionsUnderLock = append(actionsUnderLock, func() error {
return trace.Wrap(u.backend.SetUserGroups(name, groupsToAdd))
})
}
_, hasSystemGroup := currentGroups[types.TeleportServiceGroup]
if hasSystemGroup && ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
}

systemGroup, err := u.backend.LookupGroup(types.TeleportServiceGroup)
if err != nil {
if isUnknownGroupError(err, types.TeleportServiceGroup) {
// 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 {
found = true
break
}
}
if !found {
// User isn't managed by Teleport, so we don't need to update interaction time.
return nil, trace.Wrap(doWithUserLock())
}
finalGroups := make(map[string]struct{}, len(ui.Groups))
for _, group := range ui.Groups {
finalGroups[group] = struct{}{}
}

actionsUnderLock = append(actionsUnderLock, func() error {
return trace.Wrap(u.storage.UpsertHostUserInteractionTime(u.ctx, name, time.Now()))
})
if err := doWithUserLock(); err != nil {
return nil, trace.Wrap(err)
}
// try to delete even if the user already exists as only users
// in the teleport-system group will be deleted and this way
// if a user creates multiple sessions the account will
// succeed in deletion
return &userCloser{
username: name,
users: u,
backend: u.backend,
}, nil
primaryGroup, err := u.backend.LookupGroupByID(existingUser.Gid)
if err != nil {
return trace.Wrap(err)
}
finalGroups[primaryGroup.Name] = struct{}{}

if !maps.Equal(currentGroups, finalGroups) {
return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
return trace.Wrap(u.backend.SetUserGroups(name, ui.Groups))
}))
}

return nil
}

func (u *HostUserManagement) createUser(name string, ui services.HostUsersInfo) error {
var home string
if ui.Mode != types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
//nolint:staticcheck // SA4023. False positive on macOS.
home, err = readDefaultHome(name)
//nolint:staticcheck // SA4023. False positive on macOS.
var err error
if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
ui.Groups = append(ui.Groups, types.TeleportServiceGroup)
} else {
home, err = u.backend.GetDefaultHomeDirectory(name)
if err != nil {
return nil, trace.Wrap(err)
return trace.Wrap(err)
}
}

err = u.doWithUserLock(func(_ types.SemaphoreLease) error {
return trace.Wrap(u.doWithUserLock(func(_ types.SemaphoreLease) error {
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
err := u.backend.CreateGroup(name, ui.GID)
Expand All @@ -372,7 +299,7 @@ func (u *HostUserManagement) UpsertUser(name string, ui *services.HostUsersInfo)
}
}

err = u.backend.CreateUser(name, groupsToAdd, home, ui.UID, ui.GID)
err = u.backend.CreateUser(name, ui.Groups, home, ui.UID, ui.GID)
if err != nil && !trace.IsAlreadyExists(err) {
return trace.WrapWithMessage(err, "error while creating user")
}
Expand All @@ -389,22 +316,44 @@ func (u *HostUserManagement) UpsertUser(name string, ui *services.HostUsersInfo)
}

return nil
})
if err != nil {
return nil, trace.Wrap(err)
}))
}

// 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) {
if err := u.createGroupIfNotExist(group); err != nil {
groupErrs = append(groupErrs, err)
}
}

if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_KEEP {
return nil, nil
if err := trace.NewAggregate(groupErrs...); err != nil {
return nil, trace.WrapWithMessage(err, "error while creating groups")
}

closer := &userCloser{
username: name,
users: u,
backend: u.backend,
var closer io.Closer
if ui.Mode == types.CreateHostUserMode_HOST_USER_MODE_INSECURE_DROP {
closer = &userCloser{
username: name,
users: u,
backend: u.backend,
}
}

if err := u.updateUser(name, ui); err != nil {
if !errors.Is(err, user.UnknownUserError(name)) {
return nil, trace.Wrap(err)
}

if err := u.createUser(name, ui); err != nil {
return nil, trace.Wrap(err)
}
}

return closer, trace.Wrap(err)
return closer, nil
}

func (u *HostUserManagement) doWithUserLock(f func(types.SemaphoreLease) error) error {
Expand Down
6 changes: 3 additions & 3 deletions lib/srv/usermgmt_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,16 +200,16 @@ func readDefaultKey(key string, defaultValue string) (string, error) {
return defaultValue, nil
}

// readDefaultHome reads /etc/default/useradd for the HOME key,
// GetDefaultHomeDirectory reads /etc/default/useradd for the HOME key,
// defaulting to "/home" and join it with the user for the user
// home directory
func readDefaultHome(user string) (string, error) {
func (u *HostUsersProvisioningBackend) GetDefaultHomeDirectory(user string) (string, error) {
const defaultHome = "/home"
home, err := readDefaultKey("HOME", defaultHome)
return filepath.Join(home, user), trace.Wrap(err)
}

// readDefaultHome reads /etc/default/useradd for the SKEL key, defaulting to "/etc/skel"
// readDefaultSkel reads /etc/default/useradd for the SKEL key, defaulting to "/etc/skel"
func readDefaultSkel() (string, error) {
const defaultSkel = "/etc/skel"
skel, err := readDefaultKey("SKEL", defaultSkel)
Expand Down
5 changes: 0 additions & 5 deletions lib/srv/usermgmt_other.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,3 @@ func newHostUsersBackend() (HostUsersBackend, error) {
func newHostSudoersBackend(_ string) (HostSudoersBackend, error) {
return nil, trace.NotImplemented("Host user creation management is only supported on linux")
}

//nolint:staticcheck // intended to always return an error for non-linux builds
func readDefaultHome(user string) (string, error) {
return "", trace.NotImplemented("readDefaultHome is only supported on linux")
}
Loading

0 comments on commit 5106fa5

Please sign in to comment.