From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B0E9092934 for ; Mon, 8 Apr 2024 10:47:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9234F69F0 for ; Mon, 8 Apr 2024 10:47:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 8 Apr 2024 10:47:39 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B94944455D for ; Mon, 8 Apr 2024 10:47:38 +0200 (CEST) Date: Mon, 8 Apr 2024 10:47:37 +0200 From: Wolfgang Bumiller To: Fiona Ebner Cc: pve-devel@lists.proxmox.com Message-ID: References: <20240315102502.84163-1-f.ebner@proxmox.com> <20240315102502.84163-14-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240315102502.84163-14-f.ebner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.087 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [vzdump.pm, backup.pm] Subject: Re: [pve-devel] [PATCH manager v2 13/21] api: backup/vzdump: add permission check for fleecing storage X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Apr 2024 08:47:39 -0000 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 > Signed-off-by: Fiona Ebner > --- > > 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.