-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Fallback to root when user's home directory is not accessible #47524
base: master
Are you sure you want to change the base?
Conversation
lib/srv/reexec.go
Outdated
if err != nil { | ||
return io.Discard, teleport.HomeDirNotFound, nil | ||
return io.Discard, teleport.HomeDirNotAccessible, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect per this comment:
// HomeDirNotAccessible is returned when a the "teleport checkhomedir" command has
// found the user's home directory, but the user does NOT have permissions to
// access it.
If there is no user then there is no home dir, yet we are returning the code as if that dir existed (but we couldn't access it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree this is a confusing response. I think a generic RemoteCommandFailure
might make more sense here since user.Current()
returning an error would be fairly unexpected and I'm not sure adding a new error code would be worth it
lib/srv/reexec.go
Outdated
stat, ok := fi.Sys().(*syscall.Stat_t) | ||
if !ok { | ||
return trace.AccessDenied("could not retrieve owner of home directory") | ||
} | ||
|
||
uid := strconv.Itoa(int(stat.Uid)) | ||
gid := strconv.Itoa(int(stat.Gid)) | ||
|
||
if uid == localUser.Uid && gid == localUser.Gid { | ||
return nil | ||
} | ||
|
||
return trace.AccessDenied("user does not own configured home directory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect, here are some scenarios:
- if the home is owned by user
1000
and group999
, but the user isUID=1000
/GID=1000
(&&
vs||
) - user is a member of whatever group owning the home dir
- if home is
o+rwx
- ACLs are used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great points. We're probably better off not inspecting the the ownership information manually and instead just trying to stat the directory as the target user. This was mostly to avoid having to reexec if we didn't have to, but I think we do have to 😄 I'll get a new commit up for this 👍
lib/srv/reexec.go
Outdated
func CheckHomeDir(localUser *user.User) (bool, error) { | ||
if fi, err := os.Stat(localUser.HomeDir); err == nil { | ||
return fi.IsDir(), nil | ||
err := HasAccessibleHomeDir(localUser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnily enough, simple os.Stat()
is likely more complete check due to leveraging the actual system stack to do the check.
What is the problem with the root user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem with the root user?
Can you add to this? I'm not sure I understand what you're asking 🤔
if trace.IsAccessDenied(err) { | ||
return false, nil | ||
// don't spawn a subcommand if already running as the user in question | ||
if currentUser.Uid == localUser.Uid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there was an assumption baked in that CheckHomeDir
would always be called by the root/primary user, but I didn't observe that during testing. In fact I never actually saw the subcommand getting spawned while testing ssh connections through the web UI 🤔 This check prevents dropping into another sub process if we're already running as the target user
…and using a call to os.Chdir since os.Stat doesn't necessarily tell us what we need to know
3ec066b
to
a3e2b84
Compare
because of mismatched HomeDir values when calling user.Current()
lib/srv/reexec.go
Outdated
homeDir = "/" | ||
hasAccess, err := CheckHomeDirAccess(localUser) | ||
if err != nil { | ||
return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err, "failed to confirm access to home directory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to abort here? Should this be surfaced as an error and the command proceed using /
instead of the home directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that probably makes the most sense. I'm now writing the error directly to errorWriter
and then carrying on. If there's a better way to do that, I'm definitely open to it since I'm not entirely sure this output would ever land anywhere 🤔 I'm also not sure it matters since interactive sessions ultimately end up with a message sent back to the client that says something to the effect of Could not set shell's cwd to home directory "/home/ubuntu", defaulting to "/"
. We could maybe add some additional context to that message explaining why (e.g. directory doesn't exist, no access, or something unexpected)
5e7b744
to
1d6706d
Compare
1d6706d
to
7b4037b
Compare
This PR adds an additional check to
CheckHomeDir
to see if the login user has access to their configured home directory. If not, we fall back to logging them in under the root directory which is the current behavior when their home directory doesn't exist.changelog: Fixed an issue preventing connections with users whose configured home directories were inaccessible.