> On Feb 28, 2017, at 12:54 PM, David Schmitt <david.schm...@puppet.com> wrote: > > > > On 28 February 2017 at 16:34, Shawn Ferry <shawn.fe...@oracle.com > <mailto:shawn.fe...@oracle.com>> wrote: > > > > > -- > Shawn Ferry > On Feb 28, 2017, at 10:59, David Schmitt <david.schm...@puppet.com > <mailto:david.schm...@puppet.com>> wrote: > >> >> >> On 27 February 2017 at 18:57, Shawn Ferry <shawn.fe...@oracle.com >> <mailto:shawn.fe...@oracle.com>> wrote: >> >>> On Feb 27, 2017, at 9:59 AM, David Schmitt <david.schm...@puppet.com >>> <mailto:david.schm...@puppet.com>> wrote: >>> >>> Commands >>> >>> To use CLI commands in a safe and comfortable manner, the implementation >>> can use the commands method to access shell commands. You can either use a >>> full path, or a bare command name. In the latter case puppet will use the >>> system PATH setting to search for the command. If the commands are not >>> available, an error will be raised and the resources will fail in this run. >>> The commands are aware of whether noop is in effect or not, and will skip >>> the actual execution if necessary. >>> >>> Puppet::ResourceImplementation.register('apt_key') do >>> commands apt_key: '/usr/bin/apt-key' >>> commands gpg: 'gpg' >>> This will create methods called apt_get, and gpg, which will take CLI >>> arguments without any escaping, and run them in a safe environment (clean >>> working directory, clean environment). For example to call apt-key to >>> delete a specific key by id: >>> >> >> If the goal here is safe execution shouldn’t it escape at least shell >> special characters? My concern is based on some of the providers I work on >> which take free form strings as arguments and any type that doesn’t fully >> validate or munge inputs. >> >> There is a risk that any provider* today can suffer from shell command >> injection. It seems that using shelljoin on the argument array would be >> useful or shellesacpe on each element. However, for a simplified API it >> seems that it could be done more centrally here instead of pushing it to >> each implementation to make sure it is done if needed. >> >> * That can’t exclude shell characters and doesn’t munge input to escape >> them. >> >> The trick is to not use system() which calls through the shell, but the >> array variant of spawn() or popen() that doesn't go through a shell. Not >> only will that be much better performing due to fewer binaries being loaded, >> but also eliminates all shell parsing from the call chain. Having this >> wrapped up in the API as the primary mechanism to call CLI commands is >> intended to help people with that, instead of having them have to find the >> right ruby incantation themselves. > > At what point does Puppet::Util::Execution.execute change behavior? > > RTFM 4.x is an acceptable answer I only just got 4.7 integrated into our > environment but glancing at the code doesn't seem to do anything other than > Kernel.exec > > https://github.com/puppetlabs/puppet/blob/e39b265ef0b34644bb645b4f6674c257ddaa75fa/lib/puppet/util/execution.rb#L268 > > <https://github.com/puppetlabs/puppet/blob/e39b265ef0b34644bb645b4f6674c257ddaa75fa/lib/puppet/util/execution.rb#L268> > > As an existing API, we won't be able to change that quickly. Such a change is > also not really in scope for the resource API proposal. I would recommend > against using any Puppet::* APIs in new providers, to reduce dependencies to > the main puppet code. That's why the `commands` DSL has made it into the new > proposal, to provide a more "local" way to call commands.
I’m not calling Puppet::Util::Execution.execute directly. Using defined commands results in a Kernel.exec call. commands echo: “echo” echo(“This does bad things;echo this does bad things | wall") It’s something I’ve been thinking about making a pull for but I think the scope of the change is broad enough that the testing more complicated than I can commit to. I also think it’s a desirable or likely used behavior for Exec calls where it should probably still be allowed. In any case I’m stuck on the single point when it’s really a broader issue. My main point is that it command input could change before being sent onto the existing API for execution here so all new development under this implementation can take advantage of it by default. Thanks Shawn > > Cheers, David > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to puppet-dev+unsubscr...@googlegroups.com > <mailto:puppet-dev+unsubscr...@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/puppet-dev/CALF7fHaPBLuZDzHOV2OSrK_tJQWsFjn%3DK2apdXmRBnN18uUQJA%40mail.gmail.com > > <https://groups.google.com/d/msgid/puppet-dev/CALF7fHaPBLuZDzHOV2OSrK_tJQWsFjn%3DK2apdXmRBnN18uUQJA%40mail.gmail.com?utm_medium=email&utm_source=footer>. > For more options, visit https://groups.google.com/d/optout > <https://groups.google.com/d/optout>. -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/4089D8E5-E2C5-4251-B445-F548C0CB3DD1%40oracle.com. For more options, visit https://groups.google.com/d/optout.
