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 cmd.run on MacOS -- wrong environment variables #54079

Open
wants to merge 4 commits into
base: 2019.2
from

Conversation

@ScoreUnder
Copy link
Contributor

commented Jul 31, 2019

What does this PR do?

I am trying to merge this as a fix to 2019.2. Not sure if this is how things are done so please correct me if that is wrong

Arranges the environment and working directory correctly when invoking cmd.run on MacOS.

What issues does this PR fix or reference?

PRs
Relates: #47212 -- Keeps su -l which works around launchctl bootstrapping weirdness
Improves upon: #52632 -- Takes test and adds a few more.

Issues
Fixes: #51008
Fixes an apparently unreported issue where environment variables like PATH are not set correctly, resulting in brew and pkg.installed failing to work.

Previous Behavior

With runas, the command was invoked with su -l, which blanked the environment and reset the cwd.

New Behavior

With runas, the command is invoked with su -l, then the environment is corrected by running bash -l, then cd is used to correct the cwd.

Tests written?

Yes

Commits signed with GPG?

No

ScoreUnder added 2 commits Jul 31, 2019
Also fix a unit test that was testing for broken behaviour.
@ScoreUnder ScoreUnder requested a review from saltstack/team-core as a code owner Jul 31, 2019
@pull-assigner pull-assigner bot requested a review from xeacott Jul 31, 2019
@xeacott

This comment has been minimized.

Copy link

commented Jul 31, 2019

Hi @ScoreUnder thanks for the PR, and thanks for pointing out exactly what this relates to. I skimmed the related PR and issue so I'm a bit more up to speed on this. However, I will ask if @waynew or @garethgreenaway can help with this as they have more knowledge on Mac platforms 👍

ScoreUnder added 2 commits Aug 1, 2019
- ensure runas user is created when used
- mark runas tests destructive due to user creation/deletion
- fix tmpdir permission issue when cd'ing as runas user
@waynew

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Thanks for the PR! AFAICT it's looking good. Were you trying to get this into 2019.2.1 or 2019.2 (which would come out with 2019.2.3)?

@xeacott
xeacott approved these changes Aug 1, 2019
@ScoreUnder

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Hopefully I would like to see it in the next minor version after the merge, as it fixes some issues I'm having with the current version, but if it's more appropriate in another version that's fine by me (I will be running the patched module anyway)

# Ensure the login is simulated correctly (note: su runs sh, not bash,
# which causes the environment to be initialised incorrectly, which is
# fixed by the previous line of code)
cmd = 'su -l {0} -c {1}'.format(_cmd_quote(runas), _cmd_quote(cmd))

This comment has been minimized.

Copy link
@damon-atkins

damon-atkins Aug 6, 2019

Member

cmd = [ 'su', '-l', runas, '-c', 'cd '{0}' && {1}'.format(_cmd_quote(cwd), cmd) ]
When past as a list a shell is not invoke to before running the su (no security issue). When past as a string a shell is invoke to call the su and its arguments which means $abc is expanded before calling su and at the high privileged.(security issue).

This comment has been minimized.

Copy link
@ScoreUnder

ScoreUnder Aug 6, 2019

Author Contributor

The command is coerced to a string before executing, so either case is the same. I have covered this specific exploit in the integration test test_avoid_injecting_shell_code_as_root (part of this PR) which passes on the machines I have tested on. Do you know of an input which could bypass the current quoting mechanisms?

@waynew
waynew approved these changes Aug 6, 2019
@Akm0d
Akm0d approved these changes Aug 6, 2019
@garethgreenaway garethgreenaway changed the base branch from 2019.2 to 2019.2.1 Aug 6, 2019
@garethgreenaway garethgreenaway changed the base branch from 2019.2.1 to 2019.2 Aug 6, 2019
@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@ScoreUnder Thanks for the PR. Would you be able to rework this against 2019.2.1? Once it's merged there we'll make sure it gets to 2019.2. Unfortunately switching the branch via Github results in some conflicts so it might need to be a fresh PR.

@ScoreUnder

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I have created a separate PR with the commits rebased onto the other branch (#54136)

@cdalvaro

This comment has been minimized.

Copy link

commented Aug 8, 2019

Fix #51008 was already fixed in #51012

Copy link

left a comment

I have tested this module in my current salt installation and some states like macdefaults.wirte are been applied every time.

Other states, like git.latest are failing because since the selected shell for the command is /bin/bash it is not compatible with bash_completions and failed to check some refs. You can see the comment from the highstate returner.

Failed to check remote refs: /usr/local/share/bash-completion/bash_completion:
line 1893: declare: -A: invalid option declare: usage: declare [-afFirtx] [-p]
[name[=value] ...] /usr/local/share/bash-completion/bash_completion: line 2057:
complete: -D: invalid option complete: usage: complete [-abcdefgjksuv] [-pr]
[-o option] [-A action] [-G globpat] [-W wordlist] [-P prefix] [-S suffix]
[-X filterpat] [-F function] [-C command] [name ...] ls: /Users/Carlos/.bashrc.d/*:
No such file or directory fatal: unable to access
'https://github.com/vim-airline/vim-airline-themes.git/':
LibreSSL SSL_read: SSL_ERROR_SYSCALL, errno 60

# Ensure environment is correct for a newly logged-in user by running
# the command under bash as a login shell
cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd))

This comment has been minimized.

Copy link
@cdalvaro

cdalvaro Aug 8, 2019

First of all, great work!

I have a question related with this line. What does happen if my default shell is zsh for example and all my settings are provided in the ~/.zshrc file instead of the ~/.bash_profile one?

Will this change load my PATH or any other exports?

This is an important point to take into account since starting with macOS Catalina, the default shell for new users will be zsh. More info at: https://support.apple.com/en-us/HT208050

This comment has been minimized.

Copy link
@cdalvaro

cdalvaro Aug 8, 2019

As an example, when the following command is executed:

sudo salt-call --local cmd.run cmd='env' runas='Carlos'

this is the result with line cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd)) uncommented

local:
    /usr/local/share/bash-completion/bash_completion: line 1893: declare: -A: invalid option
    declare: usage: declare [-afFirtx] [-p] [name[=value] ...]
    /usr/local/share/bash-completion/bash_completion: line 2057: complete: -D: invalid option
    complete: usage: complete [-abcdefgjksuv] [-pr] [-o option] [-A action] [-G globpat] [-W wordlist] [-P prefix] [-S suffix] [-X filterpat] [-F function] [-C command] [name ...]
    ls: /Users/Carlos/.bashrc.d/*: No such file or directory
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    LESS_TERMCAP_md=
    TERM=xterm-256color                                                                                                                                                                                     SHELL=/usr/local/bin/zsh
    HISTSIZE=32768
    DISABLE_FZF_KEY_BINDINGS=false
    PROMPT_DIRTRIM=3
    LC_ALL=en_US.UTF-8
    HISTFILESIZE=32768
    USER=Carlos
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    PWD=/tmp
    EDITOR=/usr/local/bin/mvim --remote-wait-silent
    LANG=en_US.UTF-8
    HISTCONTROL=ignoreboth
    GPG_TTY=/dev/ttys002
    SHLVL=1
    HOME=/Users/Carlos
    LOGNAME=Carlos
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    OLDPWD=/Users/Carlos
    _=/usr/bin/env

and this is the result with the same line commented:

local:
    USER=Carlos
    HOME=/Users/Carlos
    SHELL=/usr/local/bin/zsh
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    TERM=xterm-256color
    LOGNAME=Carlos
    SHLVL=0
    PWD=/tmp
    OLDPWD=/Users/Carlos
    LANG=en_US.UTF-8
    LC_ALL=en_US.UTF-8
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    GPG_TTY=/dev/ttys002
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    DISABLE_FZF_KEY_BINDINGS=false
    _=/usr/bin/env

As you can see, with that line commented zsh stills getting the right environment, so a more specific solution is required for bash shells instead of making it the default solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.