On 28 February 2017 at 21:31, Shawn Ferry <[email protected]> wrote:
> > On Feb 28, 2017, at 12:54 PM, David Schmitt <[email protected]> > wrote: > > > > 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/e39b265ef0b34644bb >> 645b4f6674c257ddaa75fa/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. > Yup. The new API will only accept array input. Cheers, David > > > 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 [email protected]. > 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. > > > -- > 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/4089D8E5-E2C5-4251-B445-F548C0CB3DD1%40oracle.com > <https://groups.google.com/d/msgid/puppet-dev/4089D8E5-E2C5-4251-B445-F548C0CB3DD1%40oracle.com?utm_medium=email&utm_source=footer> > . > > For more options, visit 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALF7fHajAPv1ZZ5KKR%3DdBtb25X%3D0vtfiHmycsdAzEUXs%3DN3Chw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
