Adding better error logging when roles block host user creation by eriktate · Pull Request #46091 · gravitational/teleport

eriktate · 2024-08-30T22:21:05Z

Related to #40370
Fixes #46252

Adds more descriptive error logging when a role blocks user creation on a host, including the name of the role that blocked them. The role included in the message isn't necessarily the only role blocking access since the AccessChecker fails fast, but this should provide enough info for debugging purposes.

changelog: Fixed an issue preventing session joining while host user creation was in use.

github-actions · 2024-08-30T22:21:42Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

eriktate · 2024-08-30T22:23:15Z

 		// 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())


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

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

rosstimothy

Can we add test coverage?

nklaassen · 2024-09-03T20:40:04Z

+	existsErr := s.users.UserExists(identityContext.Login)
+	if existsErr != nil {
+		if trace.IsAccessDenied(accessErr) {
+			return false, nil, trace.WrapWithMessage(accessErr, "Insufficent permission for host user creation")


We need to be really careful not to introduce a regression here. Previously, we returned no error if trace.IsAccessDenied(accessErr) && existsErr != nil, now we do. Which I know is the entire point of this PR but I think there are cases where the SSH connection should actually succeed:

the UserExists implementation might return an error even though the user does, in fact, exist (it would on non-linux, but GetCreateHostUser() above would protect us there. Maybe there are other cases)

the user doesn't exist now, but will be created just-in-time by a PAM module https://goteleport.com/docs/enroll-resources/server-access/guides/ssh-pam/#create-local-unix-users-on-login

I think we should probably only return the access denied error if the SSH connection actually fails because the user doesn't exist. Hopefully there's a way to catch that error and add the extra context

eriktate · 2024-09-05T21:55:13Z


 	// Create host user.
-	created, userCloser, err := s.termHandlers.SessionRegistry.TryCreateHostUser(identityContext)
+	created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)


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

nklaassen · 2024-09-06T19:14:36Z

+	created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)
 	if err != nil {
-		return ctx, trace.Wrap(err)
+		log.Warnf("error while creating host users: %s", err)


I'm not sure what this PR is really getting us now, weren't we already logging warnings? But the user rarely sees those logs. If we are going to close the linked issue I would love to see in the PR description what the user actually sees when encountering the error now and how we've improved it

I'm also against logging at the warning level for conditions that we totally expect, like not creating a host user for a teleport user that has no permissions to create host users, which is the expected path for most SSH connections probably

I wonder if we could instead update (*Server).replyError to try to detect errors related to the user not existing and add context to the error we send back to the user there

Oh I just saw we're not actually trying to close #40370 with this anymore

Yeah part of this solution ended up overlapping with a bugfix for the second issue now linked in the description. I plan on opening another PR with the work to propagate errors appropriately to the client. For now I can at least reduce the log level, because I agree we shouldn't be warning for something expected out of a healthy system 👍

…cking for create_host_user_mode and surfacing for helpful error logging server side

eriktate · 2024-09-10T21:15:40Z

@nklaassen I think I've addressed all of your comments. Let me know if there's anything else you'd like to see changed, otherwise I'll plan on merging this tomorrow afternoon 👍

nklaassen · 2024-09-10T21:49:04Z

+	created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)
 	if err != nil {
-		return ctx, trace.Wrap(err)
+		log.Infof("error while creating host users: %s", err)


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

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.

This comment would apply here too

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

nklaassen · 2024-09-10T21:58:00Z

+			},
+
+			expectCreated: false,
+			expectErrIs:   trace.AccessDenied("test"),


I suggest this is not an error and it should return nil

Suggested change

expectErrIs: trace.AccessDenied("test"),

Similar to this, I'm not sure we should assume these aren't errors in this context. If we could verify that they weren't at this level I would agree, but since we only know for sure at a later time I think this case (and the one below) should still return errors

I guess I was a bit hung up on this because it's, like, the default way that teleport works. for probably 99% of successful SSH connections the host user already exists and the teleport user is not allowed to create host users. I wish we were differentiating "this is definitely an error" from "this is 99% probably fine but if the connection fails for a specific reason this might be why". But for now I guess it's fine to keep returning it as an error that only gets logged

nklaassen

alright thanks for putting up with all my comments. I see the net improvement here, let's ship it, hopefully we can add more visibility later on if the connection actually fails

nklaassen · 2024-09-11T16:01:31Z

+			},
+
+			expectCreated: false,
+			expectErrIs:   trace.AccessDenied("test"),


I guess I was a bit hung up on this because it's, like, the default way that teleport works. for probably 99% of successful SSH connections the host user already exists and the teleport user is not allowed to create host users. I wish we were differentiating "this is definitely an error" from "this is 99% probably fine but if the connection fails for a specific reason this might be why". But for now I guess it's fine to keep returning it as an error that only gets logged

nklaassen · 2024-09-11T16:07:46Z

+	created, userCloser, err := s.termHandlers.SessionRegistry.UpsertHostUser(identityContext)
 	if err != nil {
-		return ctx, trace.Wrap(err)
+		log.Infof("error while creating host users: %s", err)


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

public-teleport-github-review-bot · 2024-09-11T17:06:30Z

@eriktate See the table below for backport results.

Branch	Result
branch/v14	Create PR
branch/v15	Create PR
branch/v16	Create PR

github-actions Bot added the size/sm label Aug 30, 2024

github-actions Bot requested review from camscale and nklaassen August 30, 2024 22:21

eriktate commented Aug 30, 2024

View reviewed changes

camscale approved these changes Sep 3, 2024

View reviewed changes

eriktate added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry labels Sep 3, 2024

nklaassen reviewed Sep 3, 2024

View reviewed changes

Comment thread lib/srv/sess.go Outdated

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch 5 times, most recently from 16952e4 to c5648f2 Compare September 3, 2024 19:38

rosstimothy reviewed Sep 3, 2024

View reviewed changes

nklaassen reviewed Sep 3, 2024

View reviewed changes

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch 3 times, most recently from 0daf04d to 3794f44 Compare September 5, 2024 21:51

eriktate commented Sep 5, 2024

View reviewed changes

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch from 3794f44 to 4fc352d Compare September 5, 2024 22:05

eriktate changed the title ~~Adding better error messaging when roles block host user creation~~ Adding better error logging when roles block host user creation Sep 5, 2024

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch 3 times, most recently from 4ae56ad to 4502dd5 Compare September 6, 2024 02:17

rosstimothy approved these changes Sep 6, 2024

View reviewed changes

Comment thread lib/srv/sess.go Outdated

nklaassen reviewed Sep 6, 2024

View reviewed changes

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch from 4502dd5 to c3018d3 Compare September 10, 2024 19:36

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch 2 times, most recently from 5785b7f to f1be7da Compare September 10, 2024 20:16

adding which role caused auth failure to the resulting error when che…

61e8c3c

…cking for create_host_user_mode and surfacing for helpful error logging server side

eriktate force-pushed the eriktate/40370/better-error-for-blocked-host-user-creation branch from f1be7da to 61e8c3c Compare September 10, 2024 20:35

nklaassen reviewed Sep 10, 2024

View reviewed changes

nklaassen approved these changes Sep 11, 2024

View reviewed changes

eriktate added this pull request to the merge queue Sep 11, 2024

Merged via the queue into master with commit 4c1b8bc Sep 11, 2024

eriktate deleted the eriktate/40370/better-error-for-blocked-host-user-creation branch September 11, 2024 17:04

This was referenced Sep 11, 2024

[v16] Adding better error logging when roles block host user creation #46501

Merged

[v15] Adding better error logging when roles block host user creation #46502

Merged

[v14] Adding better error logging when roles block host user creation #46503

Merged

eriktate removed the no-changelog Indicates that a PR does not require a changelog entry label Sep 11, 2024

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

Conversation

eriktate commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriktate commented Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nklaassen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Sep 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eriktate commented Aug 30, 2024 •

edited

Loading