Allow sudoer files to be created separately from host user creation#31793
Allow sudoer files to be created separately from host user creation#31793
Conversation
lxea
force-pushed
the
lxea/separated-sudoers
branch
from
25f0a11 to
d95505d
Compare
r0mant
requested review from
espadolini and
r0mant
and removed request for
avatus
| // clean up sudoers before starting, in case any were left behind | ||
| // due to a crash for example | ||
| if err := s.sudoers.CleanupSudoers(); err != nil { | ||
| return trace.Wrap(err) | ||
| } |
There was a problem hiding this comment.
@espadolini can correct me if I'm wrong but I suspect that this might cause us to nuke sudoers for existing connections during graceful restart.
I don't know if we should be doing this sort of cleanup when starting the server. Can we just clean up sudoers for a particular user before their respective connection (unless they're already connected)?
There was a problem hiding this comment.
Yes, we will reach this point long before old sessions are guaranteed to be finished.
Do we have to delete old sudoers files? I would very much prefer to be conservative about deleting system-level things.
There was a problem hiding this comment.
I think it should be fine to not do the sudoers cleanup at startup time. The current behavior is the clean up the sudoers at session end so i think it makes sense to keep that.
We shouldnt need to remove the sudoers file before connection starts, it gets overwritten anyway with whatever the contents of the current host sudoers permission is
There was a problem hiding this comment.
Unrelated to this change: why do we need a stable sort here? Is it ever legal to have two roles with the same name in a roleset?
There was a problem hiding this comment.
No i dont think so, this should just be regular sort actually.
There was a problem hiding this comment.
We never check explicitly check the return value of GetHostSudoers() for a nil return value, from what I can tell; should the forwarding server return a no-op implementation of HostSudoers that always returns errors instead?
There was a problem hiding this comment.
I think i'd prefer GetHostSudoers to return nil, notimplemented from the forwarder
lxea
force-pushed
the
lxea/separated-sudoers
branch
from
d32cb79 to
ea6ef85
Compare
There was a problem hiding this comment.
Do we clean up sudoers entry only upon session exit now? What if Teleport process crashes?
We should clean up before session start as well - was this removed or did I miss it?
There was a problem hiding this comment.
From golang.org/x/exp/slices:
| roleSet := make([]types.Role, len(a.RoleSet)) | |
| copy(roleSet, a.RoleSet) | |
| roleSet := slices.Clone(a.RoleSet) |
lxea
force-pushed
the
lxea/separated-sudoers
branch
from
877964c to
17847f0
Compare
lxea
force-pushed
the
lxea/separated-sudoers
branch
from
17847f0 to
c353601
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
Are there any docs updates necessary to describe this new behavior? (Also note, the docs still refer to us using |
5 participants
This separates host user creation and sudoer file creation so sudoers files can be configured without the requirement that the user was a dynamically created user
#29230