[pve-devel] [PATCH] Check only first part of script parameter for executability

[pve-devel] [PATCH] Check only first part of script parameter for executability

[Phillipp Röll]

When giving a script parameter including arguments to the dump process,
the full script parameter including the arguments is checked for
executablility. The following will always abort with a "not executable"
error:

vzdump 100 --script "/usr/local/bin/myscript.pl myarg"

If we split the script argument and check only the first part, the
check works as expected.
---
 PVE/VZDump.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 88ac11b3..fe1a7f23 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -630,7 +630,9 @@ sub run_hook_script {
     my $script = $opts->{script};
     return if !$script;
 
-    if (!-x $script) {
+    my @script_args = split /\s+/, $script;
+
+    if (!-x $script_args[0]) {
 	die "The hook script '$script' is not executable.\n";
     }
 
-- 
2.33.0

Re: [pve-devel] [PATCH] Check only first part of script parameter for executability

[Thomas Lamprecht]

Hi,

thanks for your effort in contributing to Proxmox VE!

A few things inline

On 21.10.21 10:46, Phillipp Röll wrote:
> When giving a script parameter including arguments to the dump process,
> the full script parameter including the arguments is checked for
> executablility. The following will always abort with a "not executable"
> error:
> 
> vzdump 100 --script "/usr/local/bin/myscript.pl myarg"
> 
> If we split the script argument and check only the first part, the
> check works as expected.

your sign-off is missing here and it doesn't seems like we have a signed
CLA from you or your company here, and to be able to take in a patch you'd
need to send one to <office@proxmox.com>. For details please check:
https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

> ---
>  PVE/VZDump.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 88ac11b3..fe1a7f23 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -630,7 +630,9 @@ sub run_hook_script {
>      my $script = $opts->{script};
>      return if !$script;
>  
> -    if (!-x $script) {
> +    my @script_args = split /\s+/, $script;

That would break existing configuration that use a script with whitespace in its
path name.

Can you please provide your usecase where the arguments Proxmox VE sets for the
script aren't enough? So that we've a better picture about possible options.

cheers,
Thomas

Re: [pve-devel] [PATCH] Check only first part of script parameter for executability

[Phillipp Röll]

Hi Thomas,
I'll skip the legal stuff for now, until we figure out if the patch has 
a chance
to be merged, ok?

That would break existing configuration that use a script with 
whitespace in its
path name.

There could be two checks, check if either the full script name is 
executable OR
just the first part (split by whitespaces).

Can you please provide your usecase where the arguments Proxmox VE sets 
for the
script aren't enough? So that we've a better picture about possible 
options.

We have a backup & restore management system, where the state, retention 
period etc.
of a backup is stored. All proxmox backup states are updated by a hook 
script calling
a REST API. We need to pass the ID of the backup from the management 
system to the
hook script, which is no longer possible since the check was introduced.

We comment/patch these checks out automatically on all systems, but 
that's a bit
annoying.

So actually the (much) earlier introduction of the check already broke 
our use case,
but before today the pain to report it/change something simply wasn't 
great enough.

---
Mit freundlichen Grüßen
Phillipp Röll

Am 2021-10-21 11:05, schrieb Thomas Lamprecht:
> Hi,
> 
> thanks for your effort in contributing to Proxmox VE!
> 
> A few things inline
> 
> On 21.10.21 10:46, Phillipp Röll wrote:
>> When giving a script parameter including arguments to the dump 
>> process,
>> the full script parameter including the arguments is checked for
>> executablility. The following will always abort with a "not 
>> executable"
>> error:
>> 
>> vzdump 100 --script "/usr/local/bin/myscript.pl myarg"
>> 
>> If we split the script argument and check only the first part, the
>> check works as expected.
> 
> your sign-off is missing here and it doesn't seems like we have a 
> signed
> CLA from you or your company here, and to be able to take in a patch 
> you'd
> need to send one to <office@proxmox.com>. For details please check:
> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
> 
>> ---
>>  PVE/VZDump.pm | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index 88ac11b3..fe1a7f23 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -630,7 +630,9 @@ sub run_hook_script {
>>      my $script = $opts->{script};
>>      return if !$script;
>> 
>> -    if (!-x $script) {
>> +    my @script_args = split /\s+/, $script;
> 
> That would break existing configuration that use a script with 
> whitespace in its
> path name.
> 
> Can you please provide your usecase where the arguments Proxmox VE sets 
> for the
> script aren't enough? So that we've a better picture about possible 
> options.
> 
> cheers,
> Thomas

Re: [pve-devel] [PATCH] Check only first part of script parameter for executability

[Thomas Lamprecht]

Hi Phillipp,

On 21/10/2021 20:15, Phillipp Röll wrote:
> I'll skip the legal stuff for now, until we figure out if the patch has a chance
> to be merged, ok?

sounds fine to me.

> 
>> That would break existing configuration that use a script with whitespace in its
>> path name.
> 
> There could be two checks, check if either the full script name is executable OR
> just the first part (split by whitespaces).

Hmm, that could be an option, for splitting I'd recommend using the `split_args` helper
from the PVE::Tools module, it's made for this case and should handle some subtleties
better than a plain split on any whitespace.

> 
>> Can you please provide your usecase where the arguments Proxmox VE sets for the
>> script aren't enough? So that we've a better picture about possible options.
> 
> We have a backup & restore management system, where the state, retention period etc.
> of a backup is stored. All proxmox backup states are updated by a hook script calling
> a REST API. We need to pass the ID of the backup from the management system to the
> hook script, which is no longer possible since the check was introduced.

Is the ID related to the VMID? As in, could it be possible to save a mapping from
VMID to current backup ID somewhere (node local or cluster shared on /etc/pve through
pmxcfs), e.g., as JSON file, read that file in the hook script and get the BACKUP ID
then from that mapping using the VMID as key?

Could be slightly more robust and could potentially reduce the amount of REST calls
and pmxcfs updates, I mean those resources are not _that_ scarce or performance
critical, but with many guests and big clusters the pmxcfs load may get noticeable.

But, that said, I'm open into introducing your implementation, that formerly rather
worked by accident, as actual and documented possible behaviour.

> 
> We comment/patch these checks out automatically on all systems, but that's a bit
> annoying.
> 
> So actually the (much) earlier introduction of the check already broke our use case,
> but before today the pain to report it/change something simply wasn't great enough.
> 

It could help leading with that in the commit message or the like, as having an
existing use case broken should get you more attention in your favour compared to
people thinking you want add a new behaviour as we do not want to break existing
use cases if possible, but that side effect was not recognized as being used :)