The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Feature #4884

An exec provider that executes unfiltered bash code.

Added by Jesse Wolfe over 3 years ago. Updated 11 months ago.

Status:ClosedStart date:09/30/2010
Priority:UrgentDue date:
Assignee:-% Done:

0%

Category:exec
Target version:-
Affected Puppet version: Branch:https://github.com/daniel-pittman/puppet/tree/feature/2.6.next/4884-an-exec-provider-that-executes-unfiltered-bash-code
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

Some users have been using a workaround to allow Exec resources to run inline bash scripts (rather than calling an executable), as seen in the comments on #4288

We should consider adding this as a feature; I am opening this ticket for discussion.


Related issues

Related to Puppet - Bug #4859: exec regression - shell negation doesn't work anymore Rejected 09/28/2010
Related to Puppet - Bug #4288: Error if exec command starts with shell built-in like "if" Closed 07/19/2010
Related to Puppet - Bug #4299: Exec fails if command is in given path but "which" is not Closed 07/20/2010
Related to Puppet - Feature #4254: "shell" parameter for exec resource Rejected 07/16/2010
Related to Puppet Documentation - Feature #1268: Improve the documentation of how exec works Closed
Related to Puppet - Bug #6711: exec type refresh cmd should do tries and log Accepted 03/14/2011
Related to Puppet - Bug #6723: Puppet::Util::Execution withenv method not resetting envi... Closed 03/15/2011
Related to Puppet - Bug #17603: Puppet execute has unexpected semantics Accepted
Related to Puppet - Bug #19711: behavior of exec type when array given as command is undo... Rejected
Related to Puppet - Bug #19713: error "private method 'scan' called for ...:Array" when a... Closed
Duplicated by Puppet - Bug #5317: 2.6.3rc3 exec cannot start with a subshell Duplicate 11/16/2010
Duplicates Puppet - Bug #2193: Exec's "command" parameter should accept lists in additio... Duplicate 04/24/2009

History

#1 Updated by Alan Barrett over 3 years ago

I have some very complex exec commands, extending over several lines, with lots of embedded backslashes and quotation marks. Having to change from command => “complex command” to command => “/bin/sh -c ‘complex command with even more backslashes than before’” will be both error prone (it’s easy to get the wrong number of backslashes) and annoying (it seems as though puppet is deliberately removing useful behaviour for no obvious benefit).

If we get the ability to pass arrays to exec, as in command => [“/bin/sh”, “-c”, “complex command”], then it solves my problem.

#2 Updated by Alan Harder over 3 years ago

Perhaps command => ‘…’ keeps current behavior and a new script => ‘…’ option is added, well documented that this is passed directly to the shell and is considered less secure. The array idea above is also good.. I’d be happy with both :–)

#3 Updated by Alan Barrett over 3 years ago

Alan Harder wrote:

Perhaps command => ‘…’ keeps current behavior […]

Current behaviour is that the command gets passed to the shell. This happens after some sanity checks which sometimes give the wrong answers (see issues #4859, #4288, #4299), but it does happen. Or did puppet-2.6 change this to not use the shell at all?

#4 Updated by Peter Meier over 3 years ago

I just wanted to add that cd is also builtin, which could be quite usefull to sometimes use it…

#5 Updated by Alan Harder over 3 years ago

Sorry AlanB.. when I said “current behavior” I meant the “intent of the current behavior”.. ie, that command => “…” would be a locked-down/secure way to exec, and script => “…” could go directly to shell.

#6 Updated by Nigel Kersten over 3 years ago

Alan Barrett wrote:

If we get the ability to pass arrays to exec, as in command => [“/bin/sh”, “-c”, “complex command”], then it solves my problem.

Is this an acceptable solution for others? I’m leaning towards this rather than adding another parameter.

#7 Updated by Ian Ward Comfort over 3 years ago

Supplying an array would work for me. It’s probably the way execs always should have worked; we’ve gotten used to treating commands like system(3) invocations and now have a lot of manifests to update before moving to 2.6.x. (And having Puppet parse and split my string is not a whole lot better than the shell doing it — at least I know shell parsing rules.)

FWIW (maybe not much), this feature is probably a 2.6.x blocker for us, since double- and triple-quoting things for the shell is likely to get painful.

#8 Updated by Nigel Kersten over 3 years ago

  • Status changed from Unreviewed to Accepted
  • Target version set to 69

I agree. This is a 2.6.x fix.

#9 Updated by Nigel Kersten over 3 years ago

  • Priority changed from Normal to High

#10 Updated by James Turnbull over 3 years ago

  • Category set to exec

#11 Updated by Nigel Kersten over 3 years ago

  • Priority changed from High to Urgent
  • Target version changed from 69 to 2.6.5

People really do care about this. There are a lot of Execs out there in the wild.

#12 Updated by Stefan Schulte over 3 years ago

Maybe we can simulate the behaviour of perl’s system(). If you pass a single value that has metacharacters in it (space, star, simicolon etc) use the shell. Otherwise don’t use it. So we would have the following behaviour:

1) exec => 'uname -a'        # use shell (cause the space)
2) exec => 'uname'           # don't use shell
3) exec => 'start_$HOSTNAME' # use shell (cause the dollar sign)
4) exec => [ 'uname -a' ]    # use shell (cause the space)
5) exec => [ 'uname', '-a' ] # don't use shell

Note: There is no way for the type (at least not that I am aware of) to differ between 1) and 4). Propertyvalues are always arrays.

The described behaviour may break in a few situations (programname with whitespaces in it) and maybe we can spent a parameter (surpress_shell or use_shell or anything similar) for this situations

#13 Updated by Nigel Kersten about 3 years ago

  • Target version changed from 2.6.5 to 2.6.x

#14 Updated by konrad rzentarzewski about 3 years ago

wouldn’t it be reasonable to revert to previous behaviour for backwards compatibility sake? at least unless it’s fixed. for now it only leads to changing all inline execs from “if true ; then script ; fi” to “/bin/true ; if true ; then script ; fi” which provides no additional security whatsoever and thus defeats a reason for this change.

#15 Updated by James Turnbull about 3 years ago

  • Target version changed from 2.6.x to 2.6.6

#16 Updated by Nigel Kersten about 3 years ago

  • Status changed from Accepted to Investigating
  • Assignee set to Nigel Kersten

#17 Updated by Daniel Pittman about 3 years ago

After investigation the sanest long term approach seems to be that we do two things:

First, we define that the behaviour of command as an array is that it will directly execute the arguments as passed. This extends the current exec parameter to add new semantics, and allows people to pass things to alternate interpreters reasonably easily.

Second, we add a “shell” provider to the exec type (not default), which passes the string unmodified to /bin/sh to execute; this does not provide any additional configuration to allow other shells or anything like that. If those are desired features then the array form with the default provider should be used.

Finally, this interacts with the path parameter as follows:

Default provider, string: as 2.6 behaves now, which treats the first whitespace-separated word as a command, and qualifies that with path if it is not already absolute.

Default provider, array: treats the first array entry as the command, qualifies it across path if not absolute.

Shell provider: sets PATH in the environment to the supplied value before executing the subshell, does nothing to try and make this live through the shell initialization files.

#18 Updated by Nigel Kersten about 3 years ago

  • Assignee changed from Nigel Kersten to Daniel Pittman

Are you working on this issue actively Daniel?

#19 Updated by Daniel Pittman about 3 years ago

  • Branch set to https://github.com/daniel-pittman/puppet/tree/feature/2.6.next/4884-an-exec-provider-that-executes-unfiltered-bash-code

Nigel Kersten wrote:

Are you working on this issue actively Daniel?

Yes, and I failed to communicate that to the community at large. I might just fix that failure of my process.

I have a branch in progress at https://github.com/daniel-pittman/puppet/tree/feature/2.6.next/4884-an-exec-provider-that-executes-unfiltered-bash-code that is working toward an implementation.

Be warned, if you plan to depend on this code, that I am force-pushing that branch occasionally if I rewrite history, and I will eventually squash the long series of work-in-progress commits into a coherent patch series for public consumption.

The code is not currently usable, but instead is focused on adding tests to the exec type, and eventually providers, to ensure they don’t regress during this set of code changes.

#20 Updated by Daniel Pittman about 3 years ago

I have pushed to my branch with the current state of my code. It has found a number of failures in the type/provider split as it stands, and contains intermingled fixes for those changes. This is a “working state”, not a final state product.

#21 Updated by James Turnbull about 3 years ago

  • Target version changed from 2.6.6 to 2.6.x

#22 Updated by Daniel Pittman about 3 years ago

  • Status changed from Investigating to Accepted

This should have been “accepted” for a while, since we are actively developing it for the 2.6 branch.

#23 Updated by Max Martin about 3 years ago

Matt and I have been working on this, and I’ve pushed our in-progress unfinished code to a branch on my GitHub: https://github.com/MaxMartin/puppet/tree/feature/2.6.next/4884-an-exec-provider-that-executes-unfiltered-bash-code

#24 Updated by Matt Robinson about 3 years ago

Moving forward the shell provider should use the array form of commands to Util.execute to prepend the “sh -c” instead of doing it in string interpolation. Also, to test this, rather than doing expects on execute we should actually let a few simple shell commands run (true, false, echo) and then assert what the output is on the type returns property.

#25 Updated by Matt Robinson about 3 years ago

This is pretty close to done and tested https://github.com/MaxMartin/puppet/tree/feature/2.6.next/4884-an-exec-provider-that-executes-unfiltered-bash-code. Daniel will review with Max.

#26 Updated by Nigel Kersten about 3 years ago

Can we update the status if it’s not just “Accepted” anymore?

#27 Updated by Max Martin about 3 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

Daniel reviewed the final changes today, and I just merged the branch in in commit:462a560

#28 Updated by James Turnbull about 3 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version changed from 2.6.x to 2.6.8

#29 Updated by James Turnbull almost 3 years ago

  • Status changed from Merged - Pending Release to Closed

#30 Updated by eric sorenson almost 3 years ago

  • Status changed from Closed to Re-opened
  • Target version deleted (2.6.8)

Sorry to update on a closed ticket but this is a pretty big behavioural change that isn’t documented in the release notes, except the following under 2.6.8:

Notable Features and Bug Fixes
Bug #4884:

Shell exec provider that executes code as a raw shell script

Nor under the current type reference except to note that ‘provider =>’ shell is available.

Could this be punched up a bit? Maybe to include Markus' discussion of the security issues at http://projects.puppetlabs.com/issues/4288#note-16

Minimally, some kind of caveat in the 2.6.0 “New features” section of release notes, ideally there plus a description on the exec type about how the command attribute’s parameters get passed to the shell.

Thanks.

#31 Updated by Nick Fagerlund almost 3 years ago

  • Status changed from Re-opened to Closed

Sure. That’s a docs issue, though, so I’m re-closing this one; see issue #8302.

#32 Updated by Daniel Pittman 11 months ago

  • Assignee deleted (Daniel Pittman)

Also available in: Atom PDF