Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion

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

Thanks for the feedback.
Some comments inline.

On 7/2/26 3:15 PM, Daniel Kral wrote:
> On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
>> Add a helper routine to prune ACL entries under the '/sdn/' path when
>> the corresponding resources are deleted. Compare the running
>> configuration against the newly compiled state during config commit.
>>
>> This prevents dangling permission states and ensures configuration
>> consistency across manual API/UI applies as well as automatic reloads
>> during system boot.
> The patch message does describe functionality which is only implemented
> in one of the following patches (#5).
>
> If a patch implements something that is going to be used in the
> following patches, it is usually fine to state this, e.g.
> "Add <some> in preparation of <thing> in the following patches".
>
> It might make sense to introduce the lookup_acl_node() (or another
> variant, see below for more) in a separate commit, so those concerns are
> separated from both adding lookup_acl_node() for both
> remove_sdn_resource_access() and the VNet Migration.
>
> Usually, the patches which only introduce the helpers do not need to be
> prefixed with 'fix #XYZ:', but only the patches, which actually fix the
> behavior, i.e., the last patch.

Alright that make sense. Will do that.

>> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
>> Signed-off-by: David Riley <d.riley@proxmox.com>
>> ---
>>   src/PVE/AccessControl.pm | 46 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
>> index 0d632b3..10526db 100644
>> --- a/src/PVE/AccessControl.pm
>> +++ b/src/PVE/AccessControl.pm
>> @@ -1974,6 +1974,52 @@ sub remove_vm_from_pool {
>>       lock_user_config($delVMfromPoolFn, "pool cleanup for VM $vmid failed");
>>   }
>>   
>> +sub remove_sdn_resource_access {
>> +    my ($paths) = @_; # [ ['zones', '<zone>'], ['zones', '<zone>', '<vnet>'] ]
> $paths might be confusing here as in pve-access-control variables named
> 'path' are usually the string representation, e.g. /zones/<zone>.

You are right. After thinking about it a little not sure if there is actually any benefit in
using arrays over a simple string /zones/<zone>/...
I will adapt this to use strings to keep it aligned with the actual ACL paths.

>
>> +
>> +    my $delete_resource_fn = sub {
>> +        my $usercfg = cfs_read_file("user.cfg");
>> +        my $modified = 0;
>> +
>> +        for my $segments (@$paths) {
>> +            my @full_path = ('sdn', @$segments);
>> +            my $current = $usercfg->{acl_root};
>> +            my ($parent, $last_key) = lookup_acl_node($current, \@full_path);
>> +
>> +            if ($parent && $last_key) {
>> +                delete $parent->{children}->{$last_key};
>> +                $modified = 1;
>> +            }
>> +        }
>> +
>> +        if ($modified) {
>> +            cfs_write_file("user.cfg", $usercfg);
>> +        }
>> +    };
>> +
>> +    lock_user_config($delete_resource_fn, "SDN ACL cleanup failed");
>> +}
>> +
>> +sub lookup_acl_node {
>> +    my ($root, $path) = @_;
>> +
>> +    my $current = $root;
>> +    my $parent = undef;
>> +    my $last_key = undef;
>> +
>> +    for my $step (@$path) {
> nit: another patch uses $segment (instead of $step) for the same
>       concept, so it might also make more sense to use the same term here
>       (if this is still needed with the comment below)


Good catch. Will adapt this to keep it consistent.

>> +        if (defined($current->{children}) && defined($current->{children}->{$step})) {
>> +            $parent = $current;
>> +            $last_key = $step;
>> +            $current = $current->{children}->{$step};
>> +        } else {
>> +            return (undef, undef, undef);
>> +        }
>> +    }
>> +
>> +    return ($parent, $last_key, $current);
>> +}
> If $path is a string, then this could reuse the code from the existing
> find_acl_tree_node() subroutine. Either adapt it with a
>
>      return wantarray ? ($node, $parent, $last_key) : $node;
>
> or a wrapper
>
>      sub find_acl_tree_node_full($root, $path) { ... }
>
>      sub find_acl_tree_node($root, $path) {
>          my ($node) = find_acl_tree_node_full($root, $path);
>
>          return $node;
>      }
>
> No hard feelings here though.


I did have a look at the find_acl_tree_node subroutine.

However, it automatically creates intermediate nodes if they do not
exist. This felt somewhat counterintuitive, as the goal is to remove
these paths and not create them.

Additionally, I need the parent node so I can call |delete| on that
specific child key. Adapting find_acl_tree_node to fit my use case
without breaking existing callers would be quite tricky and not
worth the regression risk, in my opinion.


>> +
>>   my $USER_CONTROLLED_TFA_TYPES = {
>>       u2f => 1,
>>       oath => 1,
>
>
>
>
>