From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id B2C261FF150 for ; Mon, 06 Jul 2026 13:56:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 26A8C2144B; Mon, 06 Jul 2026 13:56:50 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 06 Jul 2026 13:56:15 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH pve-access-control v2 05/10] fix #7294: acl: pool: add SDN VNets as pool members X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626131035.112374-1-d.riley@proxmox.com> <20260626131035.112374-6-d.riley@proxmox.com> In-Reply-To: <20260626131035.112374-6-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783338969737 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.023 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: WHC2HPWM7AWGEXHEEAKJEGTPWSVDO3RS X-Message-ID-Hash: WHC2HPWM7AWGEXHEEAKJEGTPWSVDO3RS X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Generally, it would be good if this patch also introduces the network_list entry in the repository's README, where the pool fields are documented for a quick overview. On Fri Jun 26, 2026 at 3:10 PM CEST, David Riley wrote: > diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm > index 7591aa9..d9372a0 100644 > --- a/src/PVE/RPCEnvironment.pm > +++ b/src/PVE/RPCEnvironment.pm > @@ -53,6 +53,21 @@ my $compile_acl_path =3D sub { > $data->{poolroles}->{"/storage/$storeid"}->{$role} = =3D 1; > } > } > + > + for my $network_key (keys $d->{network}->%*) { > + my ($type, @path) =3D split('/', $network_key); > + > + if ($type eq 'vnet') { > + my ($zoneid, $vnetid, $vlan) =3D @path; > + > + my $acl_path =3D "/sdn/zones/$zoneid/$vnetid"; > + $acl_path =3D "$acl_path/$vlan" if defined($vlan); nit: use the .=3D operator here > + > + for my $role (keys $pool_roles->%*) { > + $data->{poolroles}->{$acl_path}->{$role} =3D 1; > + } > + } > + } > } > } > =20 > @@ -63,15 +78,30 @@ my $compile_acl_path =3D sub { > # means the role is set > my $roles =3D PVE::AccessControl::roles($cfg, $user, $path); > =20 > + my $poolroles_path =3D $data->{poolroles}->{$path}; the factoring of $data->{poolroles}->{$path} should be done in a separate commit beforehand so the changes done in this commit are easier to understand and review. > + > + # Pool ACL paths are setup without propagation, therefore checking > + # /sdn/zones/// fails even if the user has the > + # permission for the base path /sdn/zones//. > + # To allow this, the roles of the base VNet path are looked up if > + # the exact tagged path is not found in the pool. > + if (!defined($poolroles_path) && $path =3D~ m|^/sdn/zones/[^/]+/[^/]= +/\d+$|) { > + my @parts =3D split('/', $path); > + my $base_vnet_path =3D join('/', @parts[0 .. 4]); # remove tag > + > + # Inherit the permissions of the base VNet path for this request > + $poolroles_path =3D $data->{poolroles}->{$base_vnet_path}; > + } > + nit: it's probably better if this is moved before the declaration of $roles so it's clear that it isn't depending on it/interacting with it, but only the code below is Besides that, I'm curious whether there might be an use case where this might not be wanted? AFAIK, assigning a VM to a VNet does not include the network as assigning a VM to a VNet VLAN tag would have, so the propagation might be a bit off here... It would be possible to introduce the syntax vnet///* to make the propagation to the individual vlan tag objects more explicit, so this can still be an opt-in feature, and otherwise vnet// only grants assigning the VNet to pool guests. Though this is only an idea. > # apply roles inherited from pools > - if ($data->{poolroles}->{$path}) { > + if ($poolroles_path) { > # NoAccess must not be trumped by pool ACLs > if (!defined($roles->{NoAccess})) { > - if ($data->{poolroles}->{$path}->{NoAccess}) { > + if ($poolroles_path->{NoAccess}) { > # but pool ACL NoAccess trumps regular ACL > $roles =3D { 'NoAccess' =3D> 0 }; > } else { > - foreach my $role (keys %{ $data->{poolroles}->{$path} })= { > + for my $role (keys $poolroles_path->%*) { > # only use role from pool ACL if regular ACL didn't = already > # set it, and never set propagation for pool-derived= ACLs > $roles->{$role} =3D 0 if !defined($roles->{$role}); > @@ -219,6 +249,8 @@ sub compute_api_permission { > $res->{vms}->{$priv} =3D 1; > } elsif ($priv =3D~ m/^Datastore\./) { > $res->{storage}->{$priv} =3D 1; > + } elsif ($priv =3D~ m/^SDN\./) { > + $res->{sdn}->{$priv} =3D 1; What is the intent here to add this? I'm not too familiar with compute_api_permission(), but it seems like it's only relevant for OpenID and TFA authentication? > } elsif ($priv eq 'Permissions.Modify') { > $res->{storage}->{$priv} =3D 1; > $res->{vms}->{$priv} =3D 1; > @@ -274,6 +306,17 @@ sub get_effective_permissions { > foreach my $storeid (keys %{ $d->{storage} }) { > $paths->{"/storage/$storeid"} =3D 1; > } > + > + for my $network_key (keys $d->{network}->%*) { > + my ($type, @path) =3D split('/', $network_key); > + > + if ($type eq 'vnet') { > + my ($zoneid, $vnetid, $vlan) =3D @path; > + my $vnet_path =3D "/sdn/zones/$zoneid/$vnetid"; > + $vnet_path .=3D "/$vlan" if defined($vlan); > + $paths->{$vnet_path} =3D 1; > + } This might benefit from a warn/log_warn if there's a malformed network pool entry or one with an unknown type... Though this might not get shown properly to users as this is only called from an API endpoint? > + } > } > =20 > my $perms =3D {}; > @@ -353,6 +396,25 @@ sub check_sdn_bridge { > } > } > =20 > + # check access to VLANs via pools > + for my $pool (keys $cfg->{pools}->%*) { > + my $poolcfg =3D $cfg->{pools}->{$pool}; > + next if !$poolcfg->{network}; > + > + for my $network_key (keys $poolcfg->{network}->%*) { > + my ($type, @path) =3D split('/', $network_key); > + > + if (defined($type) && $type eq 'vnet') { > + my ($zoneid, $vnetid, $vlan) =3D @path; > + > + if ($zoneid eq $zone && $vnetid eq $bridge && defined($v= lan)) { > + my $vlanpath =3D "/sdn/zones/$zoneid/$vnetid/$vlan"; > + return 1 if $self->check_any($username, $vlanpath, $= privs, 1); > + } > + } I think this should also have an else path, where we warn/log_warn that there's a malformed network pool entry or one with an unknown type... > + } > + } > + > # repeat check, but fatal > $self->check_any($username, $path, $privs, 0) if !$noerr; > =20 > diff --git a/src/test/parser_writer.pl b/src/test/parser_writer.pl > index ea2778e..a809e21 100755 > --- a/src/test/parser_writer.pl > +++ b/src/test/parser_writer.pl > @@ -238,24 +238,35 @@ my $default_cfg =3D { > vms =3D> {}, > storage =3D> {}, > pools =3D> {}, > + network =3D> {}, > }, > test_pool_members =3D> { > 'id' =3D> 'testpool', > vms =3D> { 123 =3D> 1, 1234 =3D> 1 }, > storage =3D> { 'local' =3D> 1, 'local-zfs' =3D> 1 }, > pools =3D> {}, > + network =3D> { "vnet/zone1/vnet1" =3D> 1, 'vnet/zone2/vnet2' =3D= > 1 }, > }, > test_pool_duplicate_vms =3D> { > 'id' =3D> 'test_duplicate_vms', > vms =3D> {}, > storage =3D> {}, > pools =3D> {}, > + network =3D> {}, > }, > test_pool_duplicate_storages =3D> { > 'id' =3D> 'test_duplicate_storages', > vms =3D> {}, > storage =3D> { 'local' =3D> 1, 'local-zfs' =3D> 1 }, > pools =3D> {}, > + network =3D> {}, > + }, > + test_pool_duplicate_networks =3D> { > + 'id' =3D> 'test_duplicate_networks', > + vms =3D> {}, > + storage =3D> {}, > + pools =3D> {}, > + network =3D> { "vnet/zone1/vnet1" =3D> 1, "vnet/zone2/vnet2" =3D= > 1 }, > }, > acl_simple_user =3D> { > 'path' =3D> '/', > @@ -431,12 +442,15 @@ my $default_raw =3D { > 'test_role_privs_invalid' =3D> 'role:testrole:VM.Invalid,Datasto= re.Audit,VM.Allocate:', > }, > pools =3D> { > - 'test_pool_empty' =3D> 'pool:testpool::::', > - 'test_pool_invalid' =3D> 'pool:testpool::non-numeric:inval!d:', > - 'test_pool_members' =3D> 'pool:testpool::123,1234:local,local-zf= s:', > - 'test_pool_duplicate_vms' =3D> 'pool:test_duplicate_vms::123,123= 4::', > - 'test_pool_duplicate_vms_expected' =3D> 'pool:test_duplicate_vms= ::::', > - 'test_pool_duplicate_storages' =3D> 'pool:test_duplicate_storage= s:::local,local-zfs:', > + 'test_pool_empty' =3D> 'pool:testpool:::::', > + 'test_pool_invalid' =3D> 'pool:testpool::non-numeric:inval!d::', > + 'test_pool_members' =3D> > + 'pool:testpool::123,1234:local,local-zfs:vnet/zone1/vnet1,vn= et/zone2/vnet2:', > + 'test_pool_duplicate_vms' =3D> 'pool:test_duplicate_vms::123,123= 4:::', > + 'test_pool_duplicate_vms_expected' =3D> 'pool:test_duplicate_vms= :::::', > + 'test_pool_duplicate_storages' =3D> 'pool:test_duplicate_storage= s:::local,local-zfs::', > + 'test_pool_duplicate_networks' =3D> > + 'pool:test_duplicate_networks::::vnet/zone1/vnet1,vnet/zone2= /vnet2:', > }, > acl =3D> { > 'acl_simple_user' =3D> 'acl:1:/:test@pam:PVEVMAdmin:', > @@ -696,6 +710,7 @@ my $tests =3D [ > $default_cfg->{test_pool_members}, > $default_cfg->{test_pool_duplicate_vms}, > $default_cfg->{test_pool_duplicate_storages}, > + $default_cfg->{test_pool_duplicate_networks}, > ], > ), > vms =3D> default_pool_vms_with([$default_cfg->{test_pool_mem= bers}]), > @@ -705,15 +720,37 @@ my $tests =3D [ > . "\n\n\n" > . $default_raw->{pools}->{'test_pool_members'} . "\n" > . $default_raw->{pools}->{'test_pool_duplicate_vms'} . "\n" > - . $default_raw->{pools}->{'test_pool_duplicate_storages'} . = "\n", > + . $default_raw->{pools}->{'test_pool_duplicate_storages'} . = "\n" > + . $default_raw->{pools}->{'test_pool_duplicate_networks'} . = "\n", > expected_raw =3D> "" > . $default_raw->{users}->{'root@pam'} > . "\n\n\n" > + . $default_raw->{pools}->{'test_pool_duplicate_networks'} . = "\n" > . $default_raw->{pools}->{'test_pool_duplicate_storages'} . = "\n" > . $default_raw->{pools}->{'test_pool_duplicate_vms_expected'= } . "\n" > . $default_raw->{pools}->{'test_pool_members'} > . "\n\n\n", > }, > + { > + name =3D> "pool_format_backward_compatibility", > + config =3D> { > + acl_root =3D> default_acls(), > + users =3D> default_users(), > + roles =3D> default_roles(), > + pools =3D> default_pools_with([$default_cfg->{test_pool_empt= y}]), > + }, > + raw =3D> "" > + . $default_raw->{users}->{'root@pam'} > + . "\n\n\n" > + # old format: 4 colons > + . "pool:testpool::::\n\n\n", > + expected_raw =3D> "" > + . $default_raw->{users}->{'root@pam'} > + . "\n\n\n" > + # new format 5: colons > + . $default_raw->{pools}->{'test_pool_empty'} > + . "\n\n\n", > + }, nice! > { > name =3D> "acl_simple_user", > config =3D> { > @@ -1102,7 +1139,7 @@ my $tests =3D [ > . 'user:test@pam:0:0::::::' . "\n" > . 'token:test@pam!test:0:0::' . "\n\n" > . 'group:testgroup:::' . "\n\n" > - . 'pool:testpool::::' . "\n\n" > + . 'pool:testpool:::::' . "\n\n" > . 'role:testrole::' . "\n\n", > }, > ];