From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 8C2981FF183 for ; Wed, 30 Jul 2025 15:45:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 32435FBC2; Wed, 30 Jul 2025 15:47:16 +0200 (CEST) Date: Wed, 30 Jul 2025 15:47:09 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20250730132325.106314-1-f.ebner@proxmox.com> In-Reply-To: <20250730132325.106314-1-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1753882652.arempshbjm.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753883221700 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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. [storage.pm, proxmox.com] Subject: Re: [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On July 30, 2025 3:23 pm, Fiona Ebner wrote: > In the tests, legacy volids resulting in 'images' vtype will be > allowed for both 'ct-vol' and 'vm-vol'. > > New test cases for new-form guest image volids are added too. > > Signed-off-by: Fiona Ebner > --- > > Is based on top of Wolfgang's staff repo as well as [0]. > > [0]: https://lore.proxmox.com/pve-devel/20250730130506.96278-1-f.ebner@proxmox.com/T/#u > > src/PVE/Storage.pm | 31 ++++++++++++++++++--- > src/test/run_volume_access_tests.pl | 43 +++++++++++++++++++++++++++++ > 2 files changed, 70 insertions(+), 4 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index 91a0278..e6dabc3 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -646,9 +646,32 @@ sub check_volume_access { > if ($sid) { > my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid); > > - # Need to allow 'images' when expecting 'rootdir' too - not cleanly separated in plugins. > - die "unable to use volume $volid - content type needs to be '$type'\n" > - if defined($type) && $vtype ne $type && ($type ne 'rootdir' || $vtype ne 'images'); > + my $is_guest_image_vtype = > + $vtype eq 'ct-vol' || $vtype eq 'vm-vol' || $vtype eq 'images' || $vtype eq 'rootdir'; this one here should be a helper invocation - see my replies to Wolfgang's patch series > + > + if (defined($type)) { > + my $msg = "unable to use volume $volid - volume type needs to be '$type'"; > + > + # TODO PVE 11.x die or remove completely, once all callers are adapted > + $type = 'vm-vol' if $type eq 'images'; > + $type = 'ct-vol' if $type eq 'rootdir'; couldn't we just adjust all callers right now? there aren't many that set $type in the first place, and even less that set it to a vdisk type.. > + > + if ($is_guest_image_vtype) { > + if ($type eq 'vm-vol') { > + die "$msg or 'images'\n" if $vtype ne $type && $vtype ne 'images'; > + } elsif ($type eq 'ct-vol') { > + # need to allow 'images' when expecting 'ct-vol' too for backwards-compat > + die "$msg or 'rootdir' or 'images'\n" > + if $vtype ne $type && $vtype ne 'rootdir' && $vtype ne 'images'; > + } else { > + # $type is not for guest-images, but $vtype is, disallow > + die "$msg\n"; > + } > + } else { > + # can check directly > + die "$msg\n" if $vtype ne $type; > + } > + } > > return if $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate'], 1); > > @@ -664,7 +687,7 @@ sub check_volume_access { > } elsif ($vtype eq 'backup' && $ownervm) { > $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']); > $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']); > - } elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) { > + } elsif ($is_guest_image_vtype && $ownervm) { > $rpcenv->check($user, "/storage/$sid", ['Datastore.Audit']); > $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']); > } else { > diff --git a/src/test/run_volume_access_tests.pl b/src/test/run_volume_access_tests.pl > index ce9fc2e..9744b65 100755 > --- a/src/test/run_volume_access_tests.pl > +++ b/src/test/run_volume_access_tests.pl > @@ -78,6 +78,28 @@ my @tests = ( > 'backup' => 1, > }, > }, > + { > + volid => 'dir:100/vol-vm-100-disk-0.qcow2', > + denied_users => { > + 'backup@pve' => 1, > + 'dsaudit@pve' => 1, > + }, > + allowed_types => { > + 'images' => 1, > + 'vm-vol' => 1, > + }, > + }, > + { > + volid => 'dir:100/vol-ct-100-disk-0.raw', > + denied_users => { > + 'backup@pve' => 1, > + 'dsaudit@pve' => 1, > + }, > + allowed_types => { > + 'rootdir' => 1, > + 'ct-vol' => 1, > + }, > + }, > { > volid => 'dir:100/vm-100-disk-0.qcow2', > denied_users => { > @@ -87,6 +109,8 @@ my @tests = ( > allowed_types => { > 'images' => 1, > 'rootdir' => 1, > + 'ct-vol' => 1, > + 'vm-vol' => 1, > }, > }, > { > @@ -105,6 +129,17 @@ my @tests = ( > 'iso' => 1, > }, > }, > + { > + volid => 'dir:111/subvol-ct-111-disk-0.subvol', > + denied_users => { > + 'backup@pve' => 1, > + 'dsaudit@pve' => 1, > + }, > + allowed_types => { > + 'rootdir' => 1, > + 'ct-vol' => 1, > + }, > + }, > { > volid => 'dir:111/subvol-111-disk-0.subvol', > denied_users => { > @@ -114,6 +149,8 @@ my @tests = ( > allowed_types => { > 'images' => 1, > 'rootdir' => 1, > + 'ct-vol' => 1, > + 'vm-vol' => 1, > }, > }, > # test different VM IDs > @@ -138,6 +175,8 @@ my @tests = ( > allowed_types => { > 'images' => 1, > 'rootdir' => 1, > + 'ct-vol' => 1, > + 'vm-vol' => 1, > }, > }, > { > @@ -155,6 +194,8 @@ my @tests = ( > allowed_types => { > 'images' => 1, > 'rootdir' => 1, > + 'ct-vol' => 1, > + 'vm-vol' => 1, > }, > }, > { > @@ -184,6 +225,8 @@ my @tests = ( > allowed_types => { > 'images' => 1, > 'rootdir' => 1, > + 'ct-vol' => 1, > + 'vm-vol' => 1, > }, > }, > # test paths > -- > 2.47.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel