Fallback to root when user's home directory is not accessible #47524
Conversation
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.
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.
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.
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
eriktate
force-pushed
the
tross/home_dir_perms
branch
from
3ec066b to
a3e2b84
Compare
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.
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.
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)
eriktate
force-pushed
the
tross/home_dir_perms
branch
3 times, most recently
from
1d6706d to
7b4037b
Compare
lib/srv/reexec.go
Outdated
| if trace.IsNotFound(err) || trace.IsAccessDenied(err) || trace.IsBadParameter(err) { | ||
| return false, err | ||
| } | ||
|
|
||
| return false, trace.Wrap(err) |
There was a problem hiding this comment.
Is there any reason to return an unwrapped error in some cases? Can the inner if be removed?
| if trace.IsNotFound(err) || trace.IsAccessDenied(err) || trace.IsBadParameter(err) { | |
| return false, err | |
| } | |
| return false, trace.Wrap(err) | |
| return false, trace.Wrap(err) |
There was a problem hiding this comment.
No I think I accidentally pushed some changes I was testing with. Line 1270 should actually be return false, nil.
eriktate
force-pushed
the
tross/home_dir_perms
branch
from
a74918d to
aecd849
Compare
eriktate
force-pushed
the
tross/home_dir_perms
branch
from
bad6ae8 to
cec15fa
Compare
public-teleport-github-review-bot
bot
removed the request for review
from tigrato
access and child processes fallback to the root directory ("/") in the
case that they do not
eriktate
force-pushed
the
tross/home_dir_perms
branch
from
cec15fa to
aae1b74
Compare
This PR adds an additional check to
CheckHomeDirto 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.