search

quattor / CAF

Perl Common Application Framework
www.quattor.org
Other
4 stars 14 forks source link

CAF::Process: Deprecate support for executing in a subshell #266

Closed jrha closed 6 years ago

jrha commented 6 years ago

As discussed in #255.

jrha commented 6 years ago

Hmpf. JUnit seems to have blown up.

jrha commented 6 years ago

@stdweird or @jouvin do you have time to review (and merge)?

ned21 commented 6 years ago

In light of filecopy using this, we probably shouldn't merge it.

ned21 commented 6 years ago

Given that we've already identified a crucial one, I think this is premature. We don't know how to fix filecopy so it's just going to generate support requests. We need to remediate the components we know about before we start looking for ones we don't!

jouvin commented 6 years ago

You are probably right... I think the only possible fix for filecopy is to tokenize the command string. The main advantage over the current situation is probably to avoid running in a shell, avoiding that using a vulnerability somebody can escape to the shell which is running as root. We have to understand if we can do this tokenization in a reasonable way so that it works for almost every command used in our configurations without changing them and that it is easy to fix the other ones....

stdweird commented 6 years ago

maybe instead of warn, use verbose (or info), add some DEVELOPER ONLY FEEDBACK string to make clear it's not an issue. then after a few months/releases, we can gather usage by grepping logfiles or something similar. for filecopy, i think we should start deprecating any commands that are not exe arg1 arg2 ... (eg with pipe or something nasty) in the schema.

it's clear that for filecopy (or other components that require to run arbitrary code passed via profile) we need a better solution. i'm personnally in favour of replacing such behaviour with a script and run the script (so instead of command option in the schema, a script option, and write that contents to file and run that file)

jrha commented 6 years ago

Ok, bumped to next release, needs discussion. Can we do that in the issue (#255) please?

stdweird commented 6 years ago

retest this please

jrha commented 6 years ago

No consensus yet, hopefully we can discuss at workshop.

jrha commented 6 years ago

Seems to be too hard to remove the need for this, we will very strongly discourage use of subshells during PR reviews.