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

["Daniel Kral" <d.kral@proxmox.com>]

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.

>      # 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?

>                  } 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?

> +        }
>      }
>  
>      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...

> +        }
> +    }
> +
>      # 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",
>      },
>  ];