On 28 February 2017 at 16:34, Shawn Ferry <[email protected]> wrote:
> > > > > -- > Shawn Ferry > On Feb 28, 2017, at 10:59, David Schmitt <[email protected]> wrote: > > > > On 27 February 2017 at 18:57, Shawn Ferry <[email protected]> wrote: > >> >> On Feb 27, 2017, at 9:59 AM, David Schmitt <[email protected]> >> 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/e39b265ef0b34644bb645b4f6674c2 > 57ddaa75fa/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. 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALF7fHaPBLuZDzHOV2OSrK_tJQWsFjn%3DK2apdXmRBnN18uUQJA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
