Skip to content

Adding better error logging when roles block host user creation #46091

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.

Sign up for GitHub

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

Merged
eriktate merged 1 commit into from
Sep 11, 2024
Merged

Adding better error logging when roles block host user creation #46091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/services/access_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ func (a *accessChecker) HostUsers(s types.Server) (*HostUsersInfo, error) {
// if any of the matching roles do not enable create host
// user, the user should not be allowed on
if createHostUserMode == types.CreateHostUserMode_HOST_USER_MODE_OFF {
return nil, trace.AccessDenied("user is not allowed to create host users")
return nil, trace.AccessDenied("role %q prevents creating host users", role.GetName())
Copy link
Copy Markdown
Contributor Author

@eriktate eriktate Aug 30, 2024

Choose a reason for hiding this comment

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

Is there any concern around exposing the role that blocked an action like this? It was requested in the original issue and it definitely makes troubleshooting a lot simpler, but I'm not 100% sure if it's wise to write the role back to the client? Nothing bad immediately comes to mind, but just calling it out in case someone else can think of a good reason not to

Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Aug 31, 2024

Choose a reason for hiding this comment

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

I believe that users have the ability retrieve and view their own roles, so I don't know that this poses any problems.

}

if mode == types.CreateHostUserMode_HOST_USER_MODE_UNSPECIFIED {
Expand Down
10 changes: 6 additions & 4 deletions lib/srv/regular/sshserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1264,20 +1264,22 @@ func (s *Server) HandleNewConn(ctx context.Context, ccx *sshutils.ConnectionCont
}

// Create host user.
created, userCloser, err := s.termHandlers.SessionRegistry.TryCreateHostUser(identityContext)
created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)
Copy link
Copy Markdown
Contributor Author

@eriktate eriktate Sep 5, 2024

Choose a reason for hiding this comment

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

Dropping the Try nomenclature since these functions return errors. Eventually I'd like to change the public interface of usermgmt to avoid this confusion, but for now renaming it is easy

if err != nil {
return ctx, trace.Wrap(err)
log.Infof("error while creating host users: %s", err)
Copy link
Copy Markdown
Contributor

@nklaassen nklaassen Sep 10, 2024

Choose a reason for hiding this comment

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

I suggest making the function only return unexpected errors and returning them here

Suggested change
log.Infof("error while creating host users: %s", err)
return ctx, trace.Wrap(err, "creating host user")

Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Sep 11, 2024

Choose a reason for hiding this comment

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

The intent with logging errors here is to try and always permit access. Even if the users session is degraded in some way, I think that would be preferred over some error in host user creation completly preventing access to hosts. We really want to avoid forcing users to use their break glass mechanism if they don't need to because something went wrong here.

Copy link
Copy Markdown
Contributor Author

@eriktate eriktate Sep 11, 2024

Choose a reason for hiding this comment

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

This comment would apply here too

Copy link
Copy Markdown
Contributor

@nklaassen nklaassen Sep 11, 2024

Choose a reason for hiding this comment

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

I'm operating under the assumption that nobody is ever going to see these logs, especially if they don't have access to the box. But I agree that we should try to allow access if we can. Hopefully we can add some visibility in a later PR if the SSH connection does fail

}

// Indicate that the user was created by Teleport.
ccx.UserCreatedByTeleport = created
if userCloser != nil {
ccx.AddCloser(userCloser)
}

sudoersCloser, err := s.termHandlers.SessionRegistry.TryWriteSudoersFile(identityContext)
sudoersCloser, err := s.termHandlers.SessionRegistry.WriteSudoersFile(identityContext)
if err != nil {
return ctx, trace.Wrap(err)
log.Warnf("error while writing sudoers: %s", err)
Comment thread
nklaassen marked this conversation as resolved.
}

if sudoersCloser != nil {
ccx.AddCloser(sudoersCloser)
}
Expand Down
4 changes: 2 additions & 2 deletions lib/srv/regular/sshserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3065,13 +3065,13 @@ func TestHostUserCreationProxy(t *testing.T) {
reg, err := srv.NewSessionRegistry(srv.SessionRegistryConfig{Srv: proxy, SessionTrackerService: proxyClient})
require.NoError(t, err)

_, err = reg.TryWriteSudoersFile(srv.IdentityContext{
_, err = reg.WriteSudoersFile(srv.IdentityContext{
AccessChecker: &fakeAccessChecker{},
})
assert.NoError(t, err)
assert.Equal(t, 0, sudoers.writeAttempts)

_, _, err = reg.TryCreateHostUser(srv.IdentityContext{
_, _, err = reg.UpsertHostUser(srv.IdentityContext{
AccessChecker: &fakeAccessChecker{},
})
assert.NoError(t, err)
Expand Down
59 changes: 36 additions & 23 deletions lib/srv/sess.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,11 +235,15 @@ func (sc *sudoersCloser) Close() error {
return nil
}

// TryWriteSudoersFile tries to write the needed sudoers entry to the sudoers
// WriteSudoersFile tries to write the needed sudoers entry to the sudoers
// file, if any. If the returned closer is not nil, it must be called at the
// end of the session to cleanup the sudoers file.
func (s *SessionRegistry) TryWriteSudoersFile(identityContext IdentityContext) (io.Closer, error) {
// Pulling sudoers directly from the Srv so TryWriteSudoersFile always
func (s *SessionRegistry) WriteSudoersFile(identityContext IdentityContext) (io.Closer, error) {
if identityContext.Login == teleport.SSHSessionJoinPrincipal {
return nil, nil
}

// Pulling sudoers directly from the Srv so WriteSudoersFile always
// respects the invariant that we shouldn't write sudoers on proxy servers.
// This might invalidate the cached sudoers field on SessionRegistry, so
// we may be able to remove that in a future PR
Expand All @@ -265,42 +269,51 @@ func (s *SessionRegistry) TryWriteSudoersFile(identityContext IdentityContext) (
return &sudoersCloser{
username: identityContext.Login,
userSessions: s.sessionsByUser,
cleanup: s.sudoers.RemoveSudoers,
cleanup: sudoWriter.RemoveSudoers,
}, nil
}

// TryCreateHostUser attempts to create a local user on the host if needed.
// If the returned closer is not nil, it must be called at the end of the
// session to clean up the local user.
func (s *SessionRegistry) TryCreateHostUser(identityContext IdentityContext) (created bool, closer io.Closer, err error) {
// UpsertHostUser attempts to create or update a local user on the host if needed.
// If the returned closer is not nil, it must be called at the end of the session to
// clean up the local user.
func (s *SessionRegistry) UpsertHostUser(identityContext IdentityContext) (bool, io.Closer, error) {
if identityContext.Login == teleport.SSHSessionJoinPrincipal {
return false, nil, nil
}

if !s.Srv.GetCreateHostUser() || s.users == nil {
s.log.Debug("Not creating host user: node has disabled host user creation.")
return false, nil, nil // not an error to not be able to create host users
}

ui, err := identityContext.AccessChecker.HostUsers(s.Srv.GetInfo())
if err != nil {
if trace.IsAccessDenied(err) {
log.Warnf("Unable to create host users: %v", err)
return false, nil, nil
ui, accessErr := identityContext.AccessChecker.HostUsers(s.Srv.GetInfo())
if trace.IsAccessDenied(accessErr) {
existsErr := s.users.UserExists(identityContext.Login)
if existsErr != nil {
if trace.IsNotFound(existsErr) {
return false, nil, trace.WrapWithMessage(accessErr, "insufficient permissions for host user creation")
Comment thread
nklaassen marked this conversation as resolved.
}

return false, nil, trace.Wrap(existsErr)
}
log.Debug("Error while checking host users creation permission: ", err)
return false, nil, trace.Wrap(err)
}

existsErr := s.users.UserExists(identityContext.Login)
if trace.IsAccessDenied(err) && existsErr != nil {
return false, nil, trace.WrapWithMessage(err, "Insufficient permission for host user creation")
if accessErr != nil {
return false, nil, trace.Wrap(accessErr)
Comment thread
nklaassen marked this conversation as resolved.
}

userCloser, err := s.users.UpsertUser(identityContext.Login, *ui)
Comment thread
nklaassen marked this conversation as resolved.
if err != nil && !trace.IsAlreadyExists(err) && !errors.Is(err, unmanagedUserErr) {
if err != nil {
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)
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 false, nil, nil
}

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

return true, userCloser, nil
Expand Down
Loading