Add support for sudo password to the local and remote runners #3867
Conversation
using echo -e won't work. Update affected code to do that.
case invalid sudo password is provided or sudo is not configured for that user.
to sudo command via stdin.
tail that execution.
…runners_sudo_password_support
invalid sudo password is provided.
Kami
changed the title
…ich allows users to immediately tail new execution after is has been scheduled.
CHANGELOG.rst
Outdated
| * Add the ability of ``st2 key load`` to load non-string values (objects, arrays, integers, | ||
| booleans) and convert them to JSON before going into the datastore, this conversion requires the | ||
| user passing in the ``-c/--convert`` flag. (improvement) #3815 | ||
| * Updat ``st2 key load`` to load all properties of a key/value pair, now secret values can be |
There was a problem hiding this comment.
Typo ("Updat" should be "Update")
| @@ -35,6 +35,11 @@ Added | |||
| common code inside a ``lib`` directory inside a pack (with an ``__init__.py`` inside ``lib`` | |||
| directory to declare it a python package). You can then import the common code in sensors and | |||
| actions. Please refer to documentation for samples and guidelines. #3490 | |||
| * Add support for password protected sudo to the local and remote runner. Password can be provided | |||
| via the new ``sudo_password`` runner parameter. (new feature) #3867 | |||
| * Add new ``--tail`` flag to the ``st2 run`` / ``st2 action execute`` and ``st2 execution re-run`` | |||
There was a problem hiding this comment.
Could you update the PR title and description accordingly? Based on what's there now, I wouldn't assume this is included.
There was a problem hiding this comment.
Done, updated the description.
| @@ -1,4 +1,4 @@ | |||
| [flake8] | |||
| max-line-length = 100 | |||
| ignore = E128,E402 | |||
| ignore = E128,E402,E722 | |||
There was a problem hiding this comment.
What's this for? I don't see any bare excepts
There was a problem hiding this comment.
We have a bunch of them splashes over the code base and it breaks with new version of flake8.
|
|
||
| if self.sudo_password: | ||
| # Mask sudo password | ||
| command_string = 'echo -e \'%s\n\' | %s' % (MASKED_ATTRIBUTE_VALUE, command_string) |
There was a problem hiding this comment.
I'm a bit confused by this. How does this mask the sudo password? It doesn't seem to me that command_string ever includes the password, as the password is provided via stdin directly to shell.run_command
There was a problem hiding this comment.
That's correct - it doesn't include it for the local runner actions, but it does include it for remote runner actions (e.g. see https://github.com/StackStorm/st2/pull/3867/files#diff-c98f2920e8ffd90eae51578a09c4bcb5R71 for an example).
In local runner we use subprocess.Popen so doing echo -e "foo\n" | sudo -S .... won't work and we need to explicitly pass it to the command subprocess via stdin of an existing subprocess.
| @@ -30,6 +30,7 @@ | |||
| from st2common.constants.action import LIBS_DIR as ACTION_LIBS_DIR | |||
| from st2common.util.secrets import get_secret_parameters | |||
| from st2common.util.secrets import mask_secret_parameters | |||
| from st2common.logging.formatters import MASKED_ATTRIBUTE_VALUE | |||
There was a problem hiding this comment.
Shouldn't this be from https://github.com/StackStorm/st2/blob/master/st2common/st2common/constants/secrets.py? Seems to me that we should only have MASKED_ATTRIBUTE_VALUE in one place.
There was a problem hiding this comment.
Yeah, it looks like we actually have the same constant in two places - we should try to consolidate that.
| @@ -21,12 +21,17 @@ | |||
|
|
|||
| from st2common.models.system.action import ShellCommandAction | |||
| from st2common.models.system.action import ShellScriptAction | |||
|
|
|||
| from st2common.logging.formatters import MASKED_ATTRIBUTE_VALUE | |||
There was a problem hiding this comment.
Import from secrets here as well?
There was a problem hiding this comment.
It looks like it's actually an in-direct import, which is kinda nasty.
I need to check why pylint didn't detect this - we have "all" in st2common.logging.formatters which doesn't include MASKED_ATTRIBUTE_VALUE so it should detect that.
| command = 'sudo %s -- bash -c %s' % (sudo_arguments, command) | ||
|
|
||
| if self.sudo_password: | ||
| command = 'echo -e %s | %s' % (quote_unix('%s\n' % (self.sudo_password)), command) |
There was a problem hiding this comment.
What about command history for bash? Would this not show up in cleartext in ~/.bash_history by default?
There was a problem hiding this comment.
That's correctly - sadly not much we can do it besides documenting this (it affects remote runner, but not local one).
Well, we could actually try to set some environment variables and try to disable bash history, but this it not a bullet-proof solution anyway.
It's similar with other actions which pass sensitive / secret stuff as command line arguments. We already have best practice docs which says sensitive data should be retrieved inside an action to reduce surface area and prevent exposure.
I will also document that limitation when using sudo password.
…runners_sudo_password_support
There was a problem hiding this comment.
LGTM - definitely follow up with a docs PR re: #3867 (comment) and provide a nice big warning on the remote runner that bash history must be disabled
|
@Mierdin I verified and bash history is not actually saved for remote runner commands even though login shell is used because HISTFILE is not set. Having said that, I will still push a small change to explicitly disable bash history for that shell in case |
This pull request adds support for sudo password.
As an additional change, I added new
--tailflag tost2 runandst2 execution re-runcommands. When this flag is provided, new execution is automatically tailed once it's scheduled (it saves typing one command - previously you first need to do run then tail).TODO