search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.13k stars 5.47k forks source link

[2015.2] cmd.run/cmd.shell changes should be more thoroughly documented #22052

Closed iggy closed 9 years ago

iggy commented 9 years ago

The changes that were made to cmd.run (and the addition of cmd.shell) should be (much) more thoroughly documented. Both in the documentation and in the 2015.2 release notes.

In my opinion this is a rather large, backward incompatible change for anyone using even semi-complex cmd.run states.

It should be documented in the release notes so people know what they will have to change in their states.

The restrictions and their implications should also be documented in the main docs (i.e. you can no longer use pipes in cmd.run calls, etc). This also means all the salt.modules.cmd.run docs need to be fixed (there are multiple examples with |'s in there).

rallytime commented 9 years ago

Thanks for the report @iggy. We'll make sure we get this into the docs as well as the release notes.

iggy commented 9 years ago

Alternatively, revert that change ( #19914 ) so you don't inconvenience a shit ton of people.

jfindlay commented 9 years ago

That backwards-incompatible change was made to eliminate shell injection vulnerabilities. It's inconvenient, but necessary.

iggy commented 9 years ago

Example?

I have a sneaking suspicion everybody is just going to s/cmd.run/cmd.shell/ and you'll be in the same place as before.

belvedere-trading-user commented 9 years ago

This is kind of a jarring change. I know that I have a LOT of code that will have to be changed to be compatible with this. I'm guessing that I'm not alone there. Can this either be made optional (setting to control it via the config file), or perhaps moved out of the 2015.2.0 release?

Also, iggy's find/replace example isn't going to work for someone who is trying to do a phased rollout, since cmd.shell isn't available in 2014.7.

puneetk commented 9 years ago

This affects an upgrade path, please be documenting in big letters. any reason we didnt use a config to gate this option ?

basepi commented 9 years ago

I want to note that we have mitigated the expected impact of this change to only affect cmd.run calls within custom execution and state modules, as that's the primary place that shell injection vulnerabilities happen.

This will still potentially break a good deal of custom execution modules which rely on shellisms (pipes, etc). There will be a config option which can be set to revert the behavior to provide time for code to be changed to either set python_shell=True for those calls, or to remove shellisms from the calls.

This will all definitely be documented in the release notes and announcement.

iggy commented 9 years ago

Thanks for taking this seriously guys. We all appreciate the hard work you guys do.