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 C91181FF138 for ; Mon, 20 Jul 2026 10:24:20 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 5D93F21476; Mon, 20 Jul 2026 10:24:20 +0200 (CEST) Message-ID: <62179d8a-f522-4327-9dc3-600ccd01c71d@proxmox.com> Date: Mon, 20 Jul 2026 10:23:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: David Riley Subject: Re: [PATCH pve-access-control v2 05/10] fix #7294: acl: pool: add SDN VNets as pool members To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260626131035.112374-1-d.riley@proxmox.com> <20260626131035.112374-6-d.riley@proxmox.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784535800399 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.124 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) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust 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: Z3PACDEWKGTLQO6VCZW4YZCFRKX6CSS3 X-Message-ID-Hash: Z3PACDEWKGTLQO6VCZW4YZCFRKX6CSS3 X-MailFrom: d.riley@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: comments inline. I'll address the nits in v3. Thanks. On 7/6/26 1:56 PM, Daniel Kral wrote: > 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 = sub { >> $data->{poolroles}->{"/storage/$storeid"}->{$role} = 1; >> } >> } >> + >> + for my $network_key (keys $d->{network}->%*) { >> + my ($type, @path) = split('/', $network_key); >> + >> + if ($type eq 'vnet') { >> + my ($zoneid, $vnetid, $vlan) = @path; >> + >> + my $acl_path = "/sdn/zones/$zoneid/$vnetid"; >> + $acl_path = "$acl_path/$vlan" if defined($vlan); > nit: use the .= operator here > >> + >> + for my $role (keys $pool_roles->%*) { >> + $data->{poolroles}->{$acl_path}->{$role} = 1; >> + } >> + } >> + } >> } >> } >> >> @@ -63,15 +78,30 @@ my $compile_acl_path = sub { >> # means the role is set >> my $roles = PVE::AccessControl::roles($cfg, $user, $path); >> >> + my $poolroles_path = $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 =~ m|^/sdn/zones/[^/]+/[^/]+/\d+$|) { >> + my @parts = split('/', $path); >> + my $base_vnet_path = join('/', @parts[0 .. 4]); # remove tag >> + >> + # Inherit the permissions of the base VNet path for this request >> + $poolroles_path = $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. Thanks for the suggestion. I think the using a '*' to make it more explicit is a great idea. I'll try it out and check if there are any downsides to this. >> # 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 = { 'NoAccess' => 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} = 0 if !defined($roles->{$role}); >> @@ -219,6 +249,8 @@ sub compute_api_permission { >> $res->{vms}->{$priv} = 1; >> } elsif ($priv =~ m/^Datastore\./) { >> $res->{storage}->{$priv} = 1; >> + } elsif ($priv =~ m/^SDN\./) { >> + $res->{sdn}->{$priv} = 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? I think you are right this was a mistake on my side. I'll remove it completely. >> } elsif ($priv eq 'Permissions.Modify') { >> $res->{storage}->{$priv} = 1; >> $res->{vms}->{$priv} = 1; >> @@ -274,6 +306,17 @@ sub get_effective_permissions { >> foreach my $storeid (keys %{ $d->{storage} }) { >> $paths->{"/storage/$storeid"} = 1; >> } >> + >> + for my $network_key (keys $d->{network}->%*) { >> + my ($type, @path) = split('/', $network_key); >> + >> + if ($type eq 'vnet') { >> + my ($zoneid, $vnetid, $vlan) = @path; >> + my $vnet_path = "/sdn/zones/$zoneid/$vnetid"; >> + $vnet_path .= "/$vlan" if defined($vlan); >> + $paths->{$vnet_path} = 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? I agree, that would actually improve debugging faulty configurations. I'll add a log statement here. >> + } >> } >> >> my $perms = {}; >> @@ -353,6 +396,25 @@ sub check_sdn_bridge { >> } >> } >> >> + # check access to VLANs via pools >> + for my $pool (keys $cfg->{pools}->%*) { >> + my $poolcfg = $cfg->{pools}->{$pool}; >> + next if !$poolcfg->{network}; >> + >> + for my $network_key (keys $poolcfg->{network}->%*) { >> + my ($type, @path) = split('/', $network_key); >> + >> + if (defined($type) && $type eq 'vnet') { >> + my ($zoneid, $vnetid, $vlan) = @path; >> + >> + if ($zoneid eq $zone && $vnetid eq $bridge && defined($vlan)) { >> + my $vlanpath = "/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... Will add that for visibility. >> + } >> + } >> + >> # repeat check, but fatal >> $self->check_any($username, $path, $privs, 0) if !$noerr; >> >> 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 = { >> vms => {}, >> storage => {}, >> pools => {}, >> + network => {}, >> }, >> test_pool_members => { >> 'id' => 'testpool', >> vms => { 123 => 1, 1234 => 1 }, >> storage => { 'local' => 1, 'local-zfs' => 1 }, >> pools => {}, >> + network => { "vnet/zone1/vnet1" => 1, 'vnet/zone2/vnet2' => 1 }, >> }, >> test_pool_duplicate_vms => { >> 'id' => 'test_duplicate_vms', >> vms => {}, >> storage => {}, >> pools => {}, >> + network => {}, >> }, >> test_pool_duplicate_storages => { >> 'id' => 'test_duplicate_storages', >> vms => {}, >> storage => { 'local' => 1, 'local-zfs' => 1 }, >> pools => {}, >> + network => {}, >> + }, >> + test_pool_duplicate_networks => { >> + 'id' => 'test_duplicate_networks', >> + vms => {}, >> + storage => {}, >> + pools => {}, >> + network => { "vnet/zone1/vnet1" => 1, "vnet/zone2/vnet2" => 1 }, >> }, >> acl_simple_user => { >> 'path' => '/', >> @@ -431,12 +442,15 @@ my $default_raw = { >> 'test_role_privs_invalid' => 'role:testrole:VM.Invalid,Datastore.Audit,VM.Allocate:', >> }, >> pools => { >> - 'test_pool_empty' => 'pool:testpool::::', >> - 'test_pool_invalid' => 'pool:testpool::non-numeric:inval!d:', >> - 'test_pool_members' => 'pool:testpool::123,1234:local,local-zfs:', >> - 'test_pool_duplicate_vms' => 'pool:test_duplicate_vms::123,1234::', >> - 'test_pool_duplicate_vms_expected' => 'pool:test_duplicate_vms::::', >> - 'test_pool_duplicate_storages' => 'pool:test_duplicate_storages:::local,local-zfs:', >> + 'test_pool_empty' => 'pool:testpool:::::', >> + 'test_pool_invalid' => 'pool:testpool::non-numeric:inval!d::', >> + 'test_pool_members' => >> + 'pool:testpool::123,1234:local,local-zfs:vnet/zone1/vnet1,vnet/zone2/vnet2:', >> + 'test_pool_duplicate_vms' => 'pool:test_duplicate_vms::123,1234:::', >> + 'test_pool_duplicate_vms_expected' => 'pool:test_duplicate_vms:::::', >> + 'test_pool_duplicate_storages' => 'pool:test_duplicate_storages:::local,local-zfs::', >> + 'test_pool_duplicate_networks' => >> + 'pool:test_duplicate_networks::::vnet/zone1/vnet1,vnet/zone2/vnet2:', >> }, >> acl => { >> 'acl_simple_user' => 'acl:1:/:test@pam:PVEVMAdmin:', >> @@ -696,6 +710,7 @@ my $tests = [ >> $default_cfg->{test_pool_members}, >> $default_cfg->{test_pool_duplicate_vms}, >> $default_cfg->{test_pool_duplicate_storages}, >> + $default_cfg->{test_pool_duplicate_networks}, >> ], >> ), >> vms => default_pool_vms_with([$default_cfg->{test_pool_members}]), >> @@ -705,15 +720,37 @@ my $tests = [ >> . "\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 => "" >> . $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 => "pool_format_backward_compatibility", >> + config => { >> + acl_root => default_acls(), >> + users => default_users(), >> + roles => default_roles(), >> + pools => default_pools_with([$default_cfg->{test_pool_empty}]), >> + }, >> + raw => "" >> + . $default_raw->{users}->{'root@pam'} >> + . "\n\n\n" >> + # old format: 4 colons >> + . "pool:testpool::::\n\n\n", >> + expected_raw => "" >> + . $default_raw->{users}->{'root@pam'} >> + . "\n\n\n" >> + # new format 5: colons >> + . $default_raw->{pools}->{'test_pool_empty'} >> + . "\n\n\n", >> + }, > nice! > >> { >> name => "acl_simple_user", >> config => { >> @@ -1102,7 +1139,7 @@ my $tests = [ >> . '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", >> }, >> ]; >