Re: [PATCH pve-access-control v2 05/10] fix #7294: acl: pool: add SDN VNets as pool members

[David Riley <d.riley@proxmox.com>]

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/<zone>/<vnet>/<tag> fails even if the user has the
>> +    # permission for the base path /sdn/zones/<zone>/<vnet>.
>> +    # 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/<zoneid>/<vnetid>/*
>
> to make the propagation to the individual vlan tag objects more
> explicit, so this can still be an opt-in feature, and otherwise
>
>      vnet/<zoneid>/<vnetid>
>
> 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",
>>       },
>>   ];
>