Better error messaging when writing invalid sudoer entries #46731

eriktate · 2024-09-18T19:38:04Z

Related to #34515

Adds an expanded error message and WARN level log when writing sudoers fails due to invalid entries in a role's host_sudoers or a static host user's sudoers.

espadolini · 2024-09-19T08:26:15Z

lib/srv/usermgmt.go

@@ -218,6 +218,10 @@ func (u *HostSudoersManagement) WriteSudoers(name string, sudoers []string) erro
 		sudoersOut.WriteString(fmt.Sprintf("%s %s\n", name, entry))
 	}
 	err := u.backend.WriteSudoersFile(name, []byte(sudoersOut.String()))
+	if errors.Is(err, host.ErrInvalidSudoers) {
+		log.Warnf("encountered invalid sudoers entry in: %s", sudoersOut.String())


Use structured logging whenever possible:

Suggested change

log.Warnf("encountered invalid sudoers entry in: %s", sudoersOut.String())

log.WithFields(logrus.Fields{

"host_username": name,

"sudoers_entry": sudoersOut.String(),

}).Warn("Encountered invalid sudoers entry.")

Should we truncate the string? Do we have concerns about it being potentially sensitive?

Should we truncate the string? Do we have concerns about it being potentially sensitive?

I wondered about this, but I couldn't think of a good reason not to off the top of my head 🤔 An alternative, that actually might be more useful to end-users, would be to validate the entries while resolving matching rules. That way we could inform them which role has the broken entry instead of falling back to logging the entire sudoers file and leaving it up to them to figure out which role needs attention.

Hrm yeah I agree that dumping the entire sudoers file into a log message might not be ideal. What if we reworded the message to indicate that writing the sudoers file failed due an invalid host_sudoers entry in one of the users roles?

I rebased this on the slog changes and adjusted the message we're logging a bit. The log and the error are a bit redundant in cases where the returned error also ends up getting logged (e.g. in regular/sshserver.go), but I think that's probably okay since this shouldn't happen unless there's an incorrectly configured resource. And I think it's better in this case to ensure we're always surfacing something actionable when this happens

espadolini · 2024-09-19T08:27:51Z

lib/srv/usermgmt.go

@@ -218,6 +218,10 @@ func (u *HostSudoersManagement) WriteSudoers(name string, sudoers []string) erro
 		sudoersOut.WriteString(fmt.Sprintf("%s %s\n", name, entry))
 	}
 	err := u.backend.WriteSudoersFile(name, []byte(sudoersOut.String()))
+	if errors.Is(err, host.ErrInvalidSudoers) {
+		log.Warnf("encountered invalid sudoers entry in: %s", sudoersOut.String())
+		return trace.Errorf("invalid sudoers entry for login %q, inspect role's host_sudoers field or static host user's sudoers field for invalid syntax", name)


Maybe a BadParameterError?

Suggested change

return trace.Errorf("invalid sudoers entry for login %q, inspect role's host_sudoers field or static host user's sudoers field for invalid syntax", name)

return trace.BadParameter("invalid sudoers entry for login %q, inspect role's host_sudoers field or static host user's sudoers field for invalid syntax", name)

rosstimothy · 2024-10-01T21:16:53Z

lib/srv/usermgmt.go

@@ -222,6 +225,10 @@ func (u *HostSudoersManagement) WriteSudoers(name string, sudoers []string) erro
 		sudoersOut.WriteString(fmt.Sprintf("%s %s\n", name, entry))
 	}
 	err := u.backend.WriteSudoersFile(name, []byte(sudoersOut.String()))
+	if errors.Is(err, host.ErrInvalidSudoers) {
+		u.log.With("error", err, "host_username", name).Warn("Invalid sudoers entry. If using a login managed by a static host user resource, inspect its configured sudoers field for invalid entries. Otherwise, inspect the host_sudoers field for roles targeting this host.")


With is a small optimization that formats arguments a single time, which is beneficial if they are to be included in multiple log messages. For one off arguments to a single message prefer passing them in directly to the logging function.

Also don't forget to use the context variant for logging.

Suggested change

u.log.With("error", err, "host_username", name).Warn("Invalid sudoers entry. If using a login managed by a static host user resource, inspect its configured sudoers field for invalid entries. Otherwise, inspect the host_sudoers field for roles targeting this host.")

u.log.WarnContext(<some_context_here>, "Invalid sudoers entry. If using a login managed by a static host user resource, inspect its configured sudoers field for invalid entries. Otherwise, inspect the host_sudoers field for roles targeting this host.", "error", err, "host_username", name)

This might be the final nudge I need to get the linter integrated properly into my editor 😅

rosstimothy · 2024-10-01T21:17:45Z

lib/srv/usermgmt.go

@@ -78,6 +78,7 @@ func NewHostSudoers(uuid string) HostSudoers {
 	}
 	return &HostSudoersManagement{
 		backend: backend,
+		log:     slog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentHostUsers)),


You really only need to use teleport.Component if you have multiple components for the key. For single values you can pass the string in directly.

Suggested change

log: slog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentHostUsers)),

log: slog.With(teleport.ComponentKey, teleport.ComponentHostUsers),

public-teleport-github-review-bot · 2024-10-03T18:50:11Z

@eriktate See the table below for backport results.

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

github-actions bot requested a review from espadolini September 18, 2024 19:38

github-actions bot added the size/sm label Sep 18, 2024

github-actions bot requested a review from tcsc September 18, 2024 19:38

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

gravitational deleted a comment from github-actions bot Sep 19, 2024

espadolini reviewed Sep 19, 2024

View reviewed changes

eriktate added backport/branch/v14 backport/branch/v15 backport/branch/v16 labels Sep 20, 2024

eriktate force-pushed the eriktate/34515/better-errors-on-invalid-sudoers branch 2 times, most recently from 26281ca to 28bef41 Compare October 1, 2024 21:11

rosstimothy reviewed Oct 1, 2024

View reviewed changes

eriktate force-pushed the eriktate/34515/better-errors-on-invalid-sudoers branch 2 times, most recently from 2973331 to 1c7b562 Compare October 1, 2024 21:23

adding better error messaging when writing invalid sudoer entries

733ce30

eriktate force-pushed the eriktate/34515/better-errors-on-invalid-sudoers branch from 1c7b562 to 733ce30 Compare October 1, 2024 21:24

rosstimothy approved these changes Oct 1, 2024

View reviewed changes

espadolini approved these changes Oct 1, 2024

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from tcsc October 1, 2024 21:39

eriktate removed the backport/branch/v14 label Oct 2, 2024

eriktate added this pull request to the merge queue Oct 3, 2024

Merged via the queue into master with commit 7df2747 Oct 3, 2024
40 checks passed

eriktate deleted the eriktate/34515/better-errors-on-invalid-sudoers branch October 3, 2024 18:48

This was referenced Oct 3, 2024

[v15] Better error messaging when writing invalid sudoer entries #47167

Merged

[v16] Better error messaging when writing invalid sudoer entries #47168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error messaging when writing invalid sudoer entries #46731

Better error messaging when writing invalid sudoer entries #46731

eriktate commented Sep 18, 2024

espadolini Sep 19, 2024 •

edited

Loading

eriktate Sep 19, 2024

rosstimothy Sep 19, 2024

eriktate Oct 1, 2024

espadolini Sep 19, 2024

eriktate Oct 1, 2024

rosstimothy Oct 1, 2024 •

edited

Loading

eriktate Oct 1, 2024

rosstimothy Oct 1, 2024

public-teleport-github-review-bot bot commented Oct 3, 2024

-		log.Warnf("encountered invalid sudoers entry in: %s", sudoersOut.String())
+		log.WithFields(logrus.Fields{
+			"host_username": name,
+			"sudoers_entry": sudoersOut.String(),
+		}).Warn("Encountered invalid sudoers entry.")

	return trace.Errorf("invalid sudoers entry for login %q, inspect role's host_sudoers field or static host user's sudoers field for invalid syntax", name)
	return trace.BadParameter("invalid sudoers entry for login %q, inspect role's host_sudoers field or static host user's sudoers field for invalid syntax", name)

	u.log.With("error", err, "host_username", name).Warn("Invalid sudoers entry. If using a login managed by a static host user resource, inspect its configured sudoers field for invalid entries. Otherwise, inspect the host_sudoers field for roles targeting this host.")
	u.log.WarnContext(<some_context_here>, "Invalid sudoers entry. If using a login managed by a static host user resource, inspect its configured sudoers field for invalid entries. Otherwise, inspect the host_sudoers field for roles targeting this host.", "error", err, "host_username", name)

	log: slog.With(teleport.ComponentKey, teleport.Component(teleport.ComponentHostUsers)),
	log: slog.With(teleport.ComponentKey, teleport.ComponentHostUsers),

Better error messaging when writing invalid sudoer entries #46731

Better error messaging when writing invalid sudoer entries #46731

Conversation

eriktate commented Sep 18, 2024

espadolini Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

eriktate Sep 19, 2024

Choose a reason for hiding this comment

rosstimothy Sep 19, 2024

Choose a reason for hiding this comment

eriktate Oct 1, 2024

Choose a reason for hiding this comment

espadolini Sep 19, 2024

Choose a reason for hiding this comment

eriktate Oct 1, 2024

Choose a reason for hiding this comment

rosstimothy Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

eriktate Oct 1, 2024

Choose a reason for hiding this comment

rosstimothy Oct 1, 2024

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Oct 3, 2024

espadolini Sep 19, 2024 •

edited

Loading

rosstimothy Oct 1, 2024 •

edited

Loading