From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <w.bumiller@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Mon, 8 Apr 2024 10:47:38 +0200 (CEST)
Date: Mon, 8 Apr 2024 10:47:37 +0200
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Message-ID: <q5t7ju4byueriaicrfqpusazj5ehhugunxod4ihmi2fpakw5ns@mhc3ougfz2bn>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <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.