Skip to content
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

fix: don't pass all environment variables through sshd #1186

Merged

Conversation

rb-determined-ai
Copy link
Member

Description

The HOME variable was always set to / for non-root shells, when sshd should be setting it.

While we are at it, we might as well filter out several other environment variables which I think ought not to be passed in this way at all.

# After openssh 8+ is the only version of openssh supported (that is, after we
# only support ubuntu >= 20.04), we can use the more obvious SetEnv option and
# skip this awkwardness.
vars="$(env | sed -e 's/=.*//')"
blacklist="(_|HOME|TERM|LANG|LC_[^=]*)"
vars="$(env | sed -E -e '/^('"$blacklist"')=/d; s/=.*//')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking: Looks like there's an unnecessary set of parentheses around the blacklist here, and things can be simplified a bit if you switch the two statements. I think this does the same thing:

blacklist="^(_|HOME|TERM|LANG|LC_.*)\$"
vars="$(env | sed -E -e "s/=.*//; /$blacklist/d")"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes good call on both accounts.

@rb-determined-ai rb-determined-ai merged commit e255979 into determined-ai:master Aug 28, 2020
@rb-determined-ai rb-determined-ai deleted the fix-shell-home branch August 28, 2020 16:45
@dannysauer dannysauer added this to the 0.13.2 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants