Re: [pve-devel] [PATCH manager v2 13/21] api: backup/vzdump: add permission check for fleecing storage

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Mar 15, 2024 at 11:24:54AM +0100, Fiona Ebner wrote:
> Similar to how Datastore.AllocateSpace is required for the backup
> storage, it should also be required for the fleecing storage.
> 
> Removing a fleecing storage from a job does not require more
> permissions than for modifying the job.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
>  PVE/API2/Backup.pm | 10 ++++++++--
>  PVE/API2/VZDump.pm |  9 +++++----
>  PVE/VZDump.pm      |  2 +-
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 70753c2e..86f7dbdd 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -42,7 +42,7 @@ my $vzdump_job_id_prop = {
>  
>  # NOTE: also used by the vzdump API call.
>  sub assert_param_permission_common {
> -    my ($rpcenv, $user, $param) = @_;
> +    my ($rpcenv, $user, $param, $is_delete) = @_;
>      return if $user eq 'root@pam'; # always OK
>  
>      for my $key (qw(tmpdir dumpdir script)) {
> @@ -52,6 +52,12 @@ sub assert_param_permission_common {
>      if (grep { defined($param->{$_}) } qw(bwlimit ionice performance)) {
>  	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
>      }
> +
> +    if ($param->{fleecing} && !$is_delete) {
> +	my $fleecing = PVE::VZDump::parse_fleecing($param);

^ The parse_fleecing sub does not actually return the hash, at least not
explicitly, and when it is not set it returns undef, so the `if` guard
in the statement below tries to access `undef->{storage}`.

If the parameter does exist then the first run through the function
which performs the actual string->hash conversion will *accidentally*
also return the hash implicitly, because there's no explicit return
statement for it.
Subsequent calls on the other hand will run into the
    return if ref($fleecing) eq 'HASH';
and thus return an empty list making `$fleecing` undef again.

> +	$rpcenv->check($user, "/storage/$fleecing->{storage}", [ 'Datastore.AllocateSpace' ])
> +	    if $fleecing->{storage};
> +    }
>  }
>  
>  my sub assert_param_permission_create {
> @@ -70,7 +76,7 @@ my sub assert_param_permission_update {
>      return if $user eq 'root@pam'; # always OK
>  
>      assert_param_permission_common($rpcenv, $user, $update);
> -    assert_param_permission_common($rpcenv, $user, $delete);
> +    assert_param_permission_common($rpcenv, $user, $delete, 1);
>  
>      if ($update->{storage}) {
>  	$rpcenv->check($user, "/storage/$update->{storage}", [ 'Datastore.Allocate' ])
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index f66fc740..7f92e7ec 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -41,10 +41,11 @@ __PACKAGE__->register_method ({
>      description => "Create backup.",
>      permissions => {
>  	description => "The user needs 'VM.Backup' permissions on any VM, and "
> -	    ."'Datastore.AllocateSpace' on the backup storage. The 'tmpdir', 'dumpdir' and "
> -	    ."'script' parameters are restricted to the 'root\@pam' user. The 'maxfiles' and "
> -	    ."'prune-backups' settings require 'Datastore.Allocate' on the backup storage. The "
> -	    ."'bwlimit', 'performance' and 'ionice' parameters require 'Sys.Modify' on '/'. ",
> +	    ."'Datastore.AllocateSpace' on the backup storage (and fleecing storage when fleecing "
> +	    ."is used). The 'tmpdir', 'dumpdir' and 'script' parameters are restricted to the "
> +	    ."'root\@pam' user. The 'maxfiles' and 'prune-backups' settings require "
> +	    ."'Datastore.Allocate' on the backup storage. The 'bwlimit', 'performance' and "
> +	    ."'ionice' parameters require 'Sys.Modify' on '/'.",
>  	user => 'all',
>      },
>      protected => 1,
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 74eb0c83..88149d68 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -130,7 +130,7 @@ my $generate_notes = sub {
>      return $notes_template;
>  };
>  
> -my sub parse_fleecing {
> +sub parse_fleecing {
>      my ($param) = @_;
>  
>      if (defined(my $fleecing = $param->{fleecing})) {

^ So this should be updated to actually return the hash.