[PATCH access-control/network v4 0/5] fix #7520: sdn: prune orphaned ACLs and handle VNet migrations

[PATCH access-control/network v4 0/5] fix #7520: sdn: prune orphaned ACLs and handle VNet migrations

[David Riley]

Implement a pruning mechanism to clean up orphaned SDN ACL entries by
comparing the running configuration with the newly compiled state
during configuration commit.

This ensures state consistency for manual applies via the UI/API
as well as during the automatic configuration reload on system boot.
The pruning covers: 
* Zones
* VNets
* Fabrics
* Controllers
* Route maps
* Prefix lists 

IPAMs and DNS are excluded as they are not staged.

Difference from v3:
* Refactored tests
* Perl style guide refactor (for instead of foreach)
* Update: route_map_suffix regex used for detecting digits to match
  style guide recommendation

Difference from v2:
* Relocate VNet ACLs to the new zone path when a VNet is moved,
  including a validation check to abort on path conflicts.
* Refactor diff generation in preparation for an upcoming patch 
  series, resolving #7294 [0]. The upcoming series will hook into the 
  VNet diff to clean up pool members.
* Add unit testing for pruning and migration mechanism 

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7294


pve-access-control:

David Riley (4):
  fix: #7520: sdn: prune orphaned ACLs on resource deletion
  fix #7520: test: add unit tests for sdn acl pruning logic
  fix: #7520: sdn: add VNet ACL migration
  fix #7520: test: add unit tests for sdn acl migration logic

 src/PVE/AccessControl.pm           | 147 +++++++++++++++++++++++
 src/test/Makefile                  |   3 +
 src/test/sdn_acl_migration_test.pl | 183 +++++++++++++++++++++++++++++
 src/test/sdn_acl_pruning_test.pl   | 148 +++++++++++++++++++++++
 4 files changed, 481 insertions(+)
 create mode 100644 src/test/sdn_acl_migration_test.pl
 create mode 100644 src/test/sdn_acl_pruning_test.pl


pve-network:

David Riley (1):
  fix #7520: config: prune orphaned ACLs and relocate moved VNets

 src/PVE/Network/SDN.pm | 141 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)


Summary over all repositories:
  5 files changed, 622 insertions(+), 0 deletions(-)

-- 
Generated by murpp 0.11.0

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

[David Riley]

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.

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>'] ]
+
+    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) {
+        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);
+}
+
 my $USER_CONTROLLED_TFA_TYPES = {
     u2f => 1,
     oath => 1,
-- 
2.47.3

[PATCH pve-access-control v4 2/5] fix #7520: test: add unit tests for sdn acl pruning logic

[David Riley]

Add unit tests to validate the behavior of the SDN resource access
pruning logic.

The tests cover all designated SDN resource paths including zones,
vnets (with/without tags), controllers, prefix-lists, route-maps, and
fabrics, ensuring explicit permissions are dropped when resources are
deleted.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/test/Makefile                |   1 +
 src/test/sdn_acl_pruning_test.pl | 148 +++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)
 create mode 100644 src/test/sdn_acl_pruning_test.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 53f2da7..2f44186 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -14,3 +14,4 @@ check:
 	perl -I.. perm-test8.pl
 	perl -I.. realm_sync_test.pl
 	perl -I.. api-tests.pl
+	perl -I.. sdn_acl_pruning_test.pl
diff --git a/src/test/sdn_acl_pruning_test.pl b/src/test/sdn_acl_pruning_test.pl
new file mode 100644
index 0000000..f9d4582
--- /dev/null
+++ b/src/test/sdn_acl_pruning_test.pl
@@ -0,0 +1,148 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::AccessControl;
+use PVE::Tools;
+use PVE::RPCEnvironment;
+
+use Test::More;
+use Test::MockModule;
+
+# test cases
+my $tests = [
+    {
+        description => 'successful pruning of sdn resource acls',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        user:user3\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        role:SDNAudit:SDN.Audit
+        acl:1:/sdn/zones/zone1:user3\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2:user3\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet1:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet1/10:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/100:user1\@pve:SDNAudit:
+        acl:1:/sdn/controllers/control:user2\@pve:SDNAdmin:
+        acl:1:/sdn/prefix-lists/list:user2\@pve:SDNAdmin:
+        acl:1:/sdn/route-maps/map:user2\@pve:SDNAdmin:
+        acl:1:/sdn/fabrics/fabric:user2\@pve:SDNAdmin:
+        EOF
+        prune_paths => [
+            ['zones', 'zone2'],
+            ['zones', 'zone1', 'vnet1'],
+            ['zones', 'zone1', 'vnet2', '100'],
+            ['controllers', 'control'],
+            ['prefix-lists', 'list'],
+            ['route-maps', 'map'],
+            ['fabrics', 'fabric'],
+        ],
+        expected_roles => [
+            # zone 1 remains, zone 2 is pruned
+            ['user3@pve', '/sdn/zones/zone1', 'SDNAdmin'],
+            ['user3@pve', '/sdn/zones/zone2', ''],
+
+            # vnet1 is completely pruned
+            ['user1@pve', '/sdn/zones/zone1/vnet1', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet1/10', ''],
+
+            # vnet2 remains, but the specific vnet2/100 override (SDNAudit) is removed,
+            # so it falls back to inheriting SDNAdmin from vnet2
+            ['user1@pve', '/sdn/zones/zone1/vnet2', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/100', 'SDNAdmin'],
+
+            # other SDN resources pruned
+            ['user2@pve', '/sdn/controllers/control', ''],
+            ['user2@pve', '/sdn/prefix-lists/list', ''],
+            ['user2@pve', '/sdn/route-maps/map', ''],
+            ['user2@pve', '/sdn/fabrics/fabric', ''],
+        ],
+    },
+];
+
+# mocking
+my $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->noop('cfs_update');
+
+my $mocked_user_cfg_tree;
+my $access_control_module = Test::MockModule->new('PVE::AccessControl');
+
+$access_control_module->mock(
+    'cfs_read_file',
+    sub {
+        my ($filename) = @_;
+        if ($filename eq 'user.cfg') {
+            return $mocked_user_cfg_tree;
+        }
+        die "mock cfs_read_file: unexpected file $filename";
+    },
+);
+
+$access_control_module->mock(
+    'cfs_write_file',
+    sub {
+        my ($filename, $cfg) = @_;
+        if ($filename eq 'user.cfg') {
+            $mocked_user_cfg_tree = $cfg;
+        }
+    },
+);
+
+$access_control_module->mock(
+    'lock_user_config',
+    sub {
+        my ($code, $errmsg) = @_;
+        eval { $code->() };
+        if (my $err = $@) {
+            die "$errmsg: $err\n";
+        }
+    },
+);
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+# helper
+sub check_roles {
+    my ($user, $path, $expected_result) = @_;
+
+    my $roles = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
+    my $res = join(',', sort keys %$roles);
+
+    is($res, $expected_result, "Roles for $user on $path should be '$expected_result'");
+}
+
+sub main {
+    for my $test ($tests->@*) {
+        subtest $test->{description} => sub {
+            $mocked_user_cfg_tree =
+                PVE::AccessControl::parse_user_config("user.cfg", $test->{raw});
+            $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+            eval { PVE::AccessControl::remove_sdn_resource_access($test->{prune_paths}); };
+            my $err = $@;
+
+            if ($test->{expected_error}) {
+                ok($err, $test->{error_desc});
+                like($err, $test->{expected_error}, "Error message matches expected string");
+            } else {
+                is($err, '', "Pruning completed without errors");
+
+                $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+                for my $role_check ($test->{expected_roles}->@*) {
+                    check_roles(@$role_check);
+                }
+            }
+        };
+    }
+
+    done_testing();
+}
+
+main();
+exit(0);
-- 
2.47.3

[PATCH pve-access-control v4 3/5] fix: #7520: sdn: add VNet ACL migration

[David Riley]

Moving VNets between zones can lead to stale or orphaned ACL entries.
Introduce a migration routine to safely relocate ACLs during VNet
moves to maintain configuration integrity.

* Conflict Validation: Abort the migration and the SDN apply
  operation if the destination path has existing permissions. This
  prevents accidental privilege escalation or overwrites.
* Relocation: Move ACLs to the new zone path.
* Error Handling: Report both the source and destination paths on
  failure to guide manual resolution.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/AccessControl.pm | 101 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 10526db..34e56b6 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -2020,6 +2020,107 @@ sub lookup_acl_node {
     return ($parent, $last_key, $current);
 }
 
+sub migrate_sdn_resource_access {
+    my ($migrations) = @_; # [ { src_path => [...], dest_path => [...] } ]
+    if (!defined($migrations) || scalar(@$migrations) == 0) {
+        return;
+    }
+
+    my $migrate_fn = sub {
+        my $usercfg = cfs_read_file("user.cfg");
+        my $modified = 0;
+
+        my @prepared_moves;
+        for my $migration (@$migrations) {
+            for my $key (qw(src_path dest_path)) {
+                if (!defined($migration->{$key}) || ref($migration->{$key}) ne 'ARRAY') {
+                    die "Invalid '$key': must be an array reference.\n";
+                }
+            }
+
+            my $src_path = ['sdn', @{ $migration->{src_path} }];
+            my $dest_path = ['sdn', @{ $migration->{dest_path} }];
+
+            my ($src_parent, $src_last_key, $acl_node) =
+                lookup_acl_node($usercfg->{acl_root}, $src_path);
+
+            if (defined($src_parent) && defined($src_last_key) && defined($acl_node)) {
+                # probe dest for conflicts
+                my (undef, undef, $existing_dest_node) =
+                    lookup_acl_node($usercfg->{acl_root}, $dest_path);
+
+                if (acl_tree_has_permissions($existing_dest_node)) {
+                    my $conflict_path = '/' . join('/', @$dest_path);
+                    my $source_path = '/' . join('/', @$src_path);
+                    die
+                        "Destination '$conflict_path' already has permissions configured (Source:"
+                        . " '$source_path'). Please remove the target ACLs manually before"
+                        . " retrying.\n";
+                }
+                # stage move
+                push(
+                    @prepared_moves,
+                    {
+                        src_parent => $src_parent,
+                        src_last_key => $src_last_key,
+                        dest_path => $dest_path,
+                        node => $acl_node,
+                    },
+                );
+            }
+        }
+
+        for my $move (@prepared_moves) {
+            delete $move->{src_parent}->{children}->{ $move->{src_last_key} };
+
+            my $current = $usercfg->{acl_root};
+            my @dest_segments = @{ $move->{dest_path} };
+            my $dest_last_key = pop @dest_segments;
+
+            for my $segment (@dest_segments) {
+                if (!defined($current->{children}->{$segment})) {
+                    $current->{children}->{$segment} = {};
+                }
+                $current = $current->{children}->{$segment};
+            }
+            $current->{children}->{$dest_last_key} = $move->{node};
+
+            $modified = 1;
+        }
+
+        if ($modified) {
+            cfs_write_file("user.cfg", $usercfg);
+        }
+    };
+
+    lock_user_config($migrate_fn, "ACL migration failed");
+}
+
+# recursively checks a node and all its children for active permissions
+sub acl_tree_has_permissions {
+    my ($node) = @_;
+
+    if (ref($node) ne 'HASH') {
+        return 0;
+    }
+
+    for my $key (keys %$node) {
+        if ($key ne 'children') {
+            return 1;
+        }
+    }
+
+    if (ref($node->{children}) eq 'HASH') {
+        for my $child_key (keys $node->{children}->%*) {
+            if (acl_tree_has_permissions($node->{children}->{$child_key})) {
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
 my $USER_CONTROLLED_TFA_TYPES = {
     u2f => 1,
     oath => 1,
-- 
2.47.3

[PATCH pve-access-control v4 4/5] fix #7520: test: add unit tests for sdn acl migration logic

[David Riley]

Add unit tests to validate the behavior of the SDN VNet access
migration logic.

The tests cover multiple migration edge cases:
* VNet migration between zones
* Subtree retention, moving multiple VLAN tags under a VNet
* Unconfigured resources, ensuring empty source paths do not create
  empty target nodes
* Conflict prevention, aborting on destination collisions

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/test/Makefile                  |   2 +
 src/test/sdn_acl_migration_test.pl | 183 +++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+)
 create mode 100644 src/test/sdn_acl_migration_test.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 2f44186..7d4267f 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -15,3 +15,5 @@ check:
 	perl -I.. realm_sync_test.pl
 	perl -I.. api-tests.pl
 	perl -I.. sdn_acl_pruning_test.pl
+	perl -I.. sdn_acl_migration_test.pl
+
diff --git a/src/test/sdn_acl_migration_test.pl b/src/test/sdn_acl_migration_test.pl
new file mode 100644
index 0000000..175e070
--- /dev/null
+++ b/src/test/sdn_acl_migration_test.pl
@@ -0,0 +1,183 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::AccessControl;
+use PVE::Tools;
+use PVE::RPCEnvironment;
+
+use Test::More;
+use Test::MockModule;
+
+# test cases
+my $tests = [
+    {
+        description => 'successful migration of vnets and child vlans',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet1:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/100:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/200:user1\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => ['zones', 'zone1', 'vnet1'],
+                dest_path => ['zones', 'zone2', 'vnet1'],
+            },
+            {
+                src_path => ['zones', 'zone1', 'vnet2'],
+                dest_path => ['zones', 'zone2', 'vnet2'],
+            },
+            {
+                src_path => ['zones', 'zone1', 'vnet4'],
+                dest_path => ['zones', 'zone2', 'vnet4'],
+            }, # fictional move
+        ],
+        expected_roles => [
+            # src paths, should be empty after the migration
+            ['user1@pve', '/sdn/zones/zone1/vnet1', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/100', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/200', ''],
+
+            # dest paths, should have the migrated permissions
+            ['user1@pve', '/sdn/zones/zone2/vnet1', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2/100', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2/200', 'SDNAdmin'],
+
+            # fictional move, should be empty
+            ['user1@pve', '/sdn/zones/zone1/vnet4', ''],
+            ['user1@pve', '/sdn/zones/zone2/vnet4', ''],
+        ],
+    },
+    {
+        description => 'conflict without vlan tag',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet3:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2/vnet3:user2\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => ['zones', 'zone1', 'vnet3'],
+                dest_path => ['zones', 'zone2', 'vnet3'],
+            },
+        ],
+        expected_error => qr/already has permissions configured/,
+        error_desc => 'Migration must abort if direct target destination node already exists',
+    },
+    {
+        description => 'conflict with vlan tag',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet5:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2/vnet5/100:user2\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => ['zones', 'zone1', 'vnet5'],
+                dest_path => ['zones', 'zone2', 'vnet5'],
+            },
+        ],
+        expected_error => qr/already has permissions configured/,
+        error_desc =>
+            'Migration must abort if a child of the target destination node already exists',
+    },
+];
+
+# mocking
+my $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->noop('cfs_update');
+
+my $mocked_user_cfg_tree;
+my $access_control_module = Test::MockModule->new('PVE::AccessControl');
+
+$access_control_module->mock(
+    'cfs_read_file',
+    sub {
+        my ($filename) = @_;
+        if ($filename eq 'user.cfg') {
+            return $mocked_user_cfg_tree;
+        }
+        die "mock cfs_read_file: unexpected file $filename";
+    },
+);
+
+$access_control_module->mock(
+    'cfs_write_file',
+    sub {
+        my ($filename, $cfg) = @_;
+        if ($filename eq 'user.cfg') {
+            $mocked_user_cfg_tree = $cfg;
+        }
+    },
+);
+
+$access_control_module->mock(
+    'lock_user_config',
+    sub {
+        my ($code, $errmsg) = @_;
+        eval { $code->() };
+        if (my $err = $@) {
+            die "$errmsg: $err\n";
+        }
+    },
+);
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+# helper
+sub check_roles {
+    my ($user, $path, $expected_result) = @_;
+
+    my $roles = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
+    my $res = join(',', sort keys %$roles);
+
+    is($res, $expected_result, "Roles for $user on $path should be '$expected_result'");
+}
+
+sub test_migration {
+    my ($case) = @_;
+
+    $mocked_user_cfg_tree = PVE::AccessControl::parse_user_config("user.cfg", $case->{raw});
+    $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+    eval { PVE::AccessControl::migrate_sdn_resource_access($case->{migrations}); };
+    my $err = $@;
+
+    if ($case->{expected_error}) {
+        ok($err, $case->{error_desc});
+        like($err, $case->{expected_error}, "Error message matches expected conflict string");
+    } else {
+        is($err, '', "Migration completed without errors");
+
+        $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+        for my $role_check ($case->{expected_roles}->@*) {
+            check_roles(@$role_check);
+        }
+    }
+}
+
+sub main {
+    for my $case ($tests->@*) {
+        subtest $case->{description} => sub {
+            test_migration($case);
+        };
+    }
+
+    done_testing();
+}
+
+main();
+exit(0);
-- 
2.47.3

[PATCH pve-network v4 5/5] fix #7520: config: prune orphaned ACLs and relocate moved VNets

[David Riley]

Compare the running configuration with the newly compiled state during
config commit to identify and prune orphaned SDN ACL entries. This
ensures state consistency for manual applies via the UI/API as well as
during automatic configuration reloads on system boot.

Track configuration changes across:
* Zones
* VNets
* Fabrics
* Controllers
* Route maps
* Prefix lists

Trigger a validation check when a VNet is moved between zones to
mitigate potential ACL path conflicts. Abort the apply operation if a
conflict is detected to prevent configuration overwrites. If the path
is clear, relocate the ACLs to the updated zone path.

Suggested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/Network/SDN.pm | 141 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)

diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
index 33a3cf3..cd563d2 100644
--- a/src/PVE/Network/SDN.pm
+++ b/src/PVE/Network/SDN.pm
@@ -10,6 +10,7 @@ use LWP::UserAgent;
 use Net::SSLeay;
 use UUID;
 
+use PVE::AccessControl;
 use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
 use PVE::INotify;
 use PVE::RESTEnvironment qw(log_warn);
@@ -238,11 +239,151 @@ sub compile_running_cfg {
 }
 
 sub commit_config {
+    my $old_cfg = cfs_read_file($RUNNING_CFG_FILENAME);
     my $cfg = compile_running_cfg();
 
+    cleanup($old_cfg, $cfg);
+
     cfs_write_file($RUNNING_CFG_FILENAME, $cfg);
 }
 
+sub cleanup {
+    my ($old_cfg, $cfg) = @_;
+    if (!defined($old_cfg) || !defined($cfg)) {
+        return;
+    }
+
+    my $route_map_paths = diff_route_maps($old_cfg, $cfg);
+    my $generic_paths = diff_generic_resources($old_cfg, $cfg);
+    my ($vnet_delete_paths, $vnet_move_paths) = diff_vnets($old_cfg, $cfg);
+
+    if (@$vnet_move_paths) {
+        PVE::AccessControl::migrate_sdn_resource_access($vnet_move_paths);
+    }
+
+    my @paths_to_delete = (@$vnet_delete_paths, @$route_map_paths, @$generic_paths);
+    if (@paths_to_delete) {
+        PVE::AccessControl::remove_sdn_resource_access(\@paths_to_delete);
+    }
+
+}
+
+sub diff_generic_resources {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete;
+
+    my @types = qw(zones controllers fabrics prefix-lists);
+    for my $type (@types) {
+        if (!defined($old_cfg->{$type}) || !defined($old_cfg->{$type}->{ids})) {
+            next;
+        }
+
+        my $old_ids = $old_cfg->{$type}->{ids};
+        my $new_ids = {};
+
+        if (defined($cfg->{$type}) && defined($cfg->{$type}->{ids})) {
+            $new_ids = $cfg->{$type}->{ids};
+        }
+
+        for my $id (keys %$old_ids) {
+            if (defined($new_ids->{$id})) {
+                next;
+            }
+
+            push(@paths_to_delete, [$type, $id]);
+        }
+    }
+
+    return \@paths_to_delete;
+}
+
+sub diff_route_maps {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete;
+
+    if (defined($old_cfg->{'route-maps'}) && defined($old_cfg->{'route-maps'}->{ids})) {
+        my $old_route_maps = $old_cfg->{'route-maps'}->{ids};
+        my $route_map_suffix = qr/_[0-9]+$/;
+
+        my %active_route_maps;
+        if (defined($cfg->{'route-maps'}) && defined($cfg->{'route-maps'}->{ids})) {
+            for my $id (keys %{ $cfg->{'route-maps'}->{ids} }) {
+                (my $base_name = $id) =~ s/$route_map_suffix//;
+                $active_route_maps{$base_name} = 1;
+            }
+        }
+
+        my %queued_route_maps;
+        for my $id (keys %$old_route_maps) {
+            if (defined($cfg->{'route-maps'}->{ids}->{$id})) {
+                next;
+            }
+
+            (my $base_name = $id) =~ s/$route_map_suffix//;
+
+            if ($active_route_maps{$base_name}) {
+                next;
+            }
+
+            if ($queued_route_maps{$base_name}) {
+                next;
+            }
+
+            push(@paths_to_delete, ['route-maps', $base_name]);
+            $queued_route_maps{$base_name} = 1;
+        }
+    }
+
+    return \@paths_to_delete;
+}
+
+sub diff_vnets {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete;
+    my @paths_to_move;
+
+    if (defined($old_cfg->{vnets}) && defined($old_cfg->{vnets}->{ids})) {
+        my $old_vnets = $old_cfg->{vnets}->{ids};
+        my $new_vnets = {};
+
+        if (defined($cfg->{vnets}) && defined($cfg->{vnets}->{ids})) {
+            $new_vnets = $cfg->{vnets}->{ids};
+        }
+
+        for my $vnetid (keys %$old_vnets) {
+            my $old_zone = $old_vnets->{$vnetid}->{zone};
+
+            if (!defined($new_vnets->{$vnetid})) {
+                if (defined($old_zone)) {
+                    push(@paths_to_delete, ['zones', $old_zone, $vnetid]);
+                } else {
+                    log_warn(
+                        "SDN Cleanup: Could not find zone for deleted VNet $vnetid, skipping ACL"
+                            . " cleanup");
+                }
+                next;
+            }
+
+            my $new_zone = $new_vnets->{$vnetid}->{zone};
+
+            if (defined($old_zone) && defined($new_zone) && $old_zone ne $new_zone) {
+                push(
+                    @paths_to_move,
+                    {
+                        src_path => ['zones', $old_zone, $vnetid],
+                        dest_path => ['zones', $new_zone, $vnetid],
+                    },
+                );
+            }
+        }
+    }
+
+    return (\@paths_to_delete, \@paths_to_move);
+}
+
 sub has_pending_changes {
     my $running_cfg = PVE::Network::SDN::running_config();
 
-- 
2.47.3

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

[Daniel Kral]

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.

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

> +
> +    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)

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

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

Re: [PATCH pve-access-control v4 2/5] fix #7520: test: add unit tests for sdn acl pruning logic

[Daniel Kral]

Nice unit test cases!

Some small inline notes.

The fix #7520 in the patch subject might be unnecessary as test cases
aren't directly related to the bugzilla entry, but no hard feelings
about this.

On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
> Add unit tests to validate the behavior of the SDN resource access
> pruning logic.
>
> The tests cover all designated SDN resource paths including zones,
> vnets (with/without tags), controllers, prefix-lists, route-maps, and
> fabrics, ensuring explicit permissions are dropped when resources are
> deleted.
>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/test/Makefile                |   1 +
>  src/test/sdn_acl_pruning_test.pl | 148 +++++++++++++++++++++++++++++++
>  2 files changed, 149 insertions(+)
>  create mode 100644 src/test/sdn_acl_pruning_test.pl
>
> diff --git a/src/test/Makefile b/src/test/Makefile
> index 53f2da7..2f44186 100644
> --- a/src/test/Makefile
> +++ b/src/test/Makefile
> @@ -14,3 +14,4 @@ check:
>  	perl -I.. perm-test8.pl
>  	perl -I.. realm_sync_test.pl
>  	perl -I.. api-tests.pl
> +	perl -I.. sdn_acl_pruning_test.pl
> diff --git a/src/test/sdn_acl_pruning_test.pl b/src/test/sdn_acl_pruning_test.pl
> new file mode 100644
> index 0000000..f9d4582
> --- /dev/null
> +++ b/src/test/sdn_acl_pruning_test.pl
> @@ -0,0 +1,148 @@
> +#!/usr/bin/env perl
> +
> +use strict;
> +use warnings;

for new modules/scripts:

use v5.36;

> +
> +use lib qw(..);
> +
> +use PVE::AccessControl;
> +use PVE::Tools;
> +use PVE::RPCEnvironment;

should be ideally sorted alphabetically [0]

> +
> +use Test::More;
> +use Test::MockModule;

these should be sorted and before the PVE packages [0]

[0] https://pve.proxmox.com/wiki/Perl_Style_Guide#Module_Dependencies

> +
> +# test cases
> +my $tests = [

[ snip ]

> +];
> +
> +# mocking
> +my $cluster_module = Test::MockModule->new('PVE::Cluster');
> +$cluster_module->noop('cfs_update');
> +
> +my $mocked_user_cfg_tree;
> +my $access_control_module = Test::MockModule->new('PVE::AccessControl');
> +
> +$access_control_module->mock(
> +    'cfs_read_file',
> +    sub {
> +        my ($filename) = @_;
> +        if ($filename eq 'user.cfg') {
> +            return $mocked_user_cfg_tree;
> +        }
> +        die "mock cfs_read_file: unexpected file $filename";
> +    },
> +);
> +
> +$access_control_module->mock(
> +    'cfs_write_file',
> +    sub {
> +        my ($filename, $cfg) = @_;
> +        if ($filename eq 'user.cfg') {
> +            $mocked_user_cfg_tree = $cfg;
> +        }
> +    },
> +);
> +
> +$access_control_module->mock(
> +    'lock_user_config',
> +    sub {
> +        my ($code, $errmsg) = @_;
> +        eval { $code->() };
> +        if (my $err = $@) {
> +            die "$errmsg: $err\n";
> +        }
> +    },
> +);

nit: can be written as

    $access_control_module->mock(
        cfs_read_file => sub { ... },
        cfs_write_file => sub { ... },
        lock_user_config => sub { ... },
    );


> +
> +my $rpcenv = PVE::RPCEnvironment->init('cli');
> +
> +# helper
> +sub check_roles {
> +    my ($user, $path, $expected_result) = @_;
> +
> +    my $roles = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
> +    my $res = join(',', sort keys %$roles);
> +
> +    is($res, $expected_result, "Roles for $user on $path should be '$expected_result'");
> +}
> +
> +sub main {
> +    for my $test ($tests->@*) {
> +        subtest $test->{description} => sub {
> +            $mocked_user_cfg_tree =
> +                PVE::AccessControl::parse_user_config("user.cfg", $test->{raw});
> +            $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
> +
> +            eval { PVE::AccessControl::remove_sdn_resource_access($test->{prune_paths}); };
> +            my $err = $@;
> +
> +            if ($test->{expected_error}) {
> +                ok($err, $test->{error_desc});
> +                like($err, $test->{expected_error}, "Error message matches expected string");
> +            } else {
> +                is($err, '', "Pruning completed without errors");
> +
> +                $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
> +
> +                for my $role_check ($test->{expected_roles}->@*) {
> +                    check_roles(@$role_check);
> +                }
> +            }
> +        };
> +    }
> +
> +    done_testing();
> +}
> +
> +main();

for the perl script, the for loop + done_testing() could be the last
lines, so no need for the main() subroutine here ;)

> +exit(0);

Hm, is the exit(0) really needed here?

Re: [PATCH pve-network v4 5/5] fix #7520: config: prune orphaned ACLs and relocate moved VNets

[Daniel Kral]

On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
> Compare the running configuration with the newly compiled state during
> config commit to identify and prune orphaned SDN ACL entries. This
> ensures state consistency for manual applies via the UI/API as well as
> during automatic configuration reloads on system boot.
>
> Track configuration changes across:
> * Zones
> * VNets
> * Fabrics
> * Controllers
> * Route maps
> * Prefix lists
>
> Trigger a validation check when a VNet is moved between zones to
> mitigate potential ACL path conflicts. Abort the apply operation if a
> conflict is detected to prevent configuration overwrites. If the path
> is clear, relocate the ACLs to the updated zone path.
>
> Suggested-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/PVE/Network/SDN.pm | 141 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 141 insertions(+)
>
> diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
> index 33a3cf3..cd563d2 100644
> --- a/src/PVE/Network/SDN.pm
> +++ b/src/PVE/Network/SDN.pm
> @@ -10,6 +10,7 @@ use LWP::UserAgent;
>  use Net::SSLeay;
>  use UUID;
>  
> +use PVE::AccessControl;
>  use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::INotify;
>  use PVE::RESTEnvironment qw(log_warn);
> @@ -238,11 +239,151 @@ sub compile_running_cfg {
>  }
>  
>  sub commit_config {
> +    my $old_cfg = cfs_read_file($RUNNING_CFG_FILENAME);
>      my $cfg = compile_running_cfg();
>  
> +    cleanup($old_cfg, $cfg);
> +
>      cfs_write_file($RUNNING_CFG_FILENAME, $cfg);
>  }
>  
> +sub cleanup {

maybe a more telling name like cleanup_sdn_resource_acls(...) or
something similar? Otherwise, it might suggest that it cleans up
something in the SDN config.

> +    my ($old_cfg, $cfg) = @_;
> +    if (!defined($old_cfg) || !defined($cfg)) {
> +        return;
> +    }

AFAICT $old_cfg and $cfg are always defined as the cfs_read_file()
parser for $RUNNING_CFG_FILENAME will always at least return an empty
hash ref and compile_running_cfg() will also always return a hash ref,
so this should not be needed?

> +
> +    my $route_map_paths = diff_route_maps($old_cfg, $cfg);
> +    my $generic_paths = diff_generic_resources($old_cfg, $cfg);
> +    my ($vnet_delete_paths, $vnet_move_paths) = diff_vnets($old_cfg, $cfg);
> +
> +    if (@$vnet_move_paths) {
> +        PVE::AccessControl::migrate_sdn_resource_access($vnet_move_paths);
> +    }
> +
> +    my @paths_to_delete = (@$vnet_delete_paths, @$route_map_paths, @$generic_paths);
> +    if (@paths_to_delete) {
> +        PVE::AccessControl::remove_sdn_resource_access(\@paths_to_delete);
> +    }
> +
> +}
> +
> +sub diff_generic_resources {
> +    my ($old_cfg, $cfg) = @_;
> +
> +    my @paths_to_delete;

nit: initialize with empty array ()

> +
> +    my @types = qw(zones controllers fabrics prefix-lists);
> +    for my $type (@types) {
> +        if (!defined($old_cfg->{$type}) || !defined($old_cfg->{$type}->{ids})) {
> +            next;
> +        }
> +
> +        my $old_ids = $old_cfg->{$type}->{ids};
> +        my $new_ids = {};
> +
> +        if (defined($cfg->{$type}) && defined($cfg->{$type}->{ids})) {
> +            $new_ids = $cfg->{$type}->{ids};
> +        }

As compile_running_cfg() does always set the $cfg->{$type}->{ids} this
could probably be set unconditionally?

> +
> +        for my $id (keys %$old_ids) {
> +            if (defined($new_ids->{$id})) {
> +                next;
> +            }

nit: use post-if statement

> +
> +            push(@paths_to_delete, [$type, $id]);
> +        }
> +    }
> +
> +    return \@paths_to_delete;
> +}
> +
> +sub diff_route_maps {
> +    my ($old_cfg, $cfg) = @_;
> +
> +    my @paths_to_delete;

nit: initialize with empty array ()

> +
> +    if (defined($old_cfg->{'route-maps'}) && defined($old_cfg->{'route-maps'}->{ids})) {
> +        my $old_route_maps = $old_cfg->{'route-maps'}->{ids};
> +        my $route_map_suffix = qr/_[0-9]+$/;

nit: suffix with _re(gex)

> +
> +        my %active_route_maps;

nit: initialize with empty hash ()

> +        if (defined($cfg->{'route-maps'}) && defined($cfg->{'route-maps'}->{ids})) {
> +            for my $id (keys %{ $cfg->{'route-maps'}->{ids} }) {

nit: use postfix:    keys $cfg->{'route-maps'}->{ids}->%*

> +                (my $base_name = $id) =~ s/$route_map_suffix//;
> +                $active_route_maps{$base_name} = 1;
> +            }
> +        }
> +
> +        my %queued_route_maps;

nit: initialize with empty hash ()

> +        for my $id (keys %$old_route_maps) {
> +            if (defined($cfg->{'route-maps'}->{ids}->{$id})) {
> +                next;
> +            }

nit: use post-if statement

> +
> +            (my $base_name = $id) =~ s/$route_map_suffix//;
> +
> +            if ($active_route_maps{$base_name}) {
> +                next;
> +            }

nit: use post-if statement

> +
> +            if ($queued_route_maps{$base_name}) {
> +                next;
> +            }

nit: use post-if statement

> +
> +            push(@paths_to_delete, ['route-maps', $base_name]);
> +            $queued_route_maps{$base_name} = 1;
> +        }
> +    }
> +
> +    return \@paths_to_delete;
> +}
> +
> +sub diff_vnets {
> +    my ($old_cfg, $cfg) = @_;
> +
> +    my @paths_to_delete;
> +    my @paths_to_move;

nit: initialize with empty arrays ()

> +
> +    if (defined($old_cfg->{vnets}) && defined($old_cfg->{vnets}->{ids})) {
> +        my $old_vnets = $old_cfg->{vnets}->{ids};
> +        my $new_vnets = {};
> +
> +        if (defined($cfg->{vnets}) && defined($cfg->{vnets}->{ids})) {
> +            $new_vnets = $cfg->{vnets}->{ids};
> +        }

same as above, could be set unconditionally as compile_running_cfg()
always sets the $cfg->{vnets}->{ids} hash ref

> +
> +        for my $vnetid (keys %$old_vnets) {
> +            my $old_zone = $old_vnets->{$vnetid}->{zone};
> +
> +            if (!defined($new_vnets->{$vnetid})) {
> +                if (defined($old_zone)) {
> +                    push(@paths_to_delete, ['zones', $old_zone, $vnetid]);
> +                } else {
> +                    log_warn(
> +                        "SDN Cleanup: Could not find zone for deleted VNet $vnetid, skipping ACL"
> +                            . " cleanup");
> +                }
> +                next;
> +            }
> +
> +            my $new_zone = $new_vnets->{$vnetid}->{zone};
> +
> +            if (defined($old_zone) && defined($new_zone) && $old_zone ne $new_zone) {
> +                push(
> +                    @paths_to_move,
> +                    {
> +                        src_path => ['zones', $old_zone, $vnetid],
> +                        dest_path => ['zones', $new_zone, $vnetid],
> +                    },
> +                );
> +            }

Hm, there's a lot of testing whether the zone exists... Can this
actually happen besides users directly editing the sdn/vnets.cfg file
and removing the vnet property?

The API handler doesn't permit me to add a VNet without or with a
non-existing zone (only the zone 'abc' is existing; the error message is
a bit misleading, because the type is undefined here because the zone
does not exist at all):

# pvesh create /cluster/sdn/vnets --vnet aeaewk --zone '' --type 'vnet'
create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326.
# pvesh create /cluster/sdn/vnets --vnet aeaewk --zone 'abc' --type 'vnet'
# pvesh create /cluster/sdn/vnets --vnet aeaewk2 --zone 'def' --type 'vnet'
create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326.

> +        }
> +    }
> +
> +    return (\@paths_to_delete, \@paths_to_move);
> +}
> +
>  sub has_pending_changes {
>      my $running_cfg = PVE::Network::SDN::running_config();
>  

Re: [PATCH pve-access-control v4 3/5] fix: #7520: sdn: add VNet ACL migration

[Daniel Kral]

On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
> Moving VNets between zones can lead to stale or orphaned ACL entries.
> Introduce a migration routine to safely relocate ACLs during VNet
> moves to maintain configuration integrity.
>
> * Conflict Validation: Abort the migration and the SDN apply
>   operation if the destination path has existing permissions. This
>   prevents accidental privilege escalation or overwrites.
> * Relocation: Move ACLs to the new zone path.
> * Error Handling: Report both the source and destination paths on
>   failure to guide manual resolution.
>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/PVE/AccessControl.pm | 101 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 10526db..34e56b6 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -2020,6 +2020,107 @@ sub lookup_acl_node {
>      return ($parent, $last_key, $current);
>  }
>  
> +sub migrate_sdn_resource_access {
> +    my ($migrations) = @_; # [ { src_path => [...], dest_path => [...] } ]
> +    if (!defined($migrations) || scalar(@$migrations) == 0) {
> +        return;
> +    }

nit: it's enough to use

    return if !$migrations;

here, because Perl does evaluate $migrations in a scalar context here
already.

> +
> +    my $migrate_fn = sub {
> +        my $usercfg = cfs_read_file("user.cfg");
> +        my $modified = 0;
> +
> +        my @prepared_moves;

nit: initialize with empty array ()

> +        for my $migration (@$migrations) {
> +            for my $key (qw(src_path dest_path)) {
> +                if (!defined($migration->{$key}) || ref($migration->{$key}) ne 'ARRAY') {
> +                    die "Invalid '$key': must be an array reference.\n";

die'ing here might be a bit harsh as that is more of a programming error
and a user cannot resolve that problem... Maybe a warn makes more sense
or just skipping the current $migration?

> +                }
> +            }
> +
> +            my $src_path = ['sdn', @{ $migration->{src_path} }];
> +            my $dest_path = ['sdn', @{ $migration->{dest_path} }];

nit: use postfix $migration->{src_path}->@*

> +
> +            my ($src_parent, $src_last_key, $acl_node) =
> +                lookup_acl_node($usercfg->{acl_root}, $src_path);
> +
> +            if (defined($src_parent) && defined($src_last_key) && defined($acl_node)) {
> +                # probe dest for conflicts
> +                my (undef, undef, $existing_dest_node) =
> +                    lookup_acl_node($usercfg->{acl_root}, $dest_path);
> +
> +                if (acl_tree_has_permissions($existing_dest_node)) {
> +                    my $conflict_path = '/' . join('/', @$dest_path);
> +                    my $source_path = '/' . join('/', @$src_path);
> +                    die
> +                        "Destination '$conflict_path' already has permissions configured (Source:"
> +                        . " '$source_path'). Please remove the target ACLs manually before"
> +                        . " retrying.\n";

nice for doing this before actually migrating so users can resolve the
issues and then cleanly commit the newly compiled running config and
not having any inconsistent state inbetween!

> +                }
> +                # stage move
> +                push(
> +                    @prepared_moves,
> +                    {
> +                        src_parent => $src_parent,
> +                        src_last_key => $src_last_key,
> +                        dest_path => $dest_path,
> +                        node => $acl_node,
> +                    },
> +                );
> +            }
> +        }
> +
> +        for my $move (@prepared_moves) {
> +            delete $move->{src_parent}->{children}->{ $move->{src_last_key} };
> +
> +            my $current = $usercfg->{acl_root};
> +            my @dest_segments = @{ $move->{dest_path} };
> +            my $dest_last_key = pop @dest_segments;
> +
> +            for my $segment (@dest_segments) {
> +                if (!defined($current->{children}->{$segment})) {
> +                    $current->{children}->{$segment} = {};
> +                }

nit: use post-if here

> +                $current = $current->{children}->{$segment};
> +            }
> +            $current->{children}->{$dest_last_key} = $move->{node};
> +
> +            $modified = 1;
> +        }
> +
> +        if ($modified) {
> +            cfs_write_file("user.cfg", $usercfg);
> +        }
> +    };
> +
> +    lock_user_config($migrate_fn, "ACL migration failed");

nit: prefix "SDN" here too, so it's clear that it's about the SDN ACL
     migration

> +}
> +
> +# recursively checks a node and all its children for active permissions
> +sub acl_tree_has_permissions {
> +    my ($node) = @_;
> +
> +    if (ref($node) ne 'HASH') {
> +        return 0;
> +    }
> +
> +    for my $key (keys %$node) {
> +        if ($key ne 'children') {
> +            return 1;
> +        }
> +    }
> +
> +    if (ref($node->{children}) eq 'HASH') {
> +        for my $child_key (keys $node->{children}->%*) {
> +            if (acl_tree_has_permissions($node->{children}->{$child_key})) {
> +                return 1;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}

maybe acl_tree_has_permissions() could reuse iterate_acl_tree() here?

We also don't seem to do a hash ref check there, so it seems like for
the already populated ACL tree nodes, there already is a children hash
ref entry defined, so the check for the for loop might not be needed...
But I'm not sure about this.

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

Re: [PATCH pve-network v4 5/5] fix #7520: config: prune orphaned ACLs and relocate moved VNets

[David Riley]

Thanks for taking a look.
Will address all the nits in v5.
Some comments inline.

On 7/2/26 3:26 PM, Daniel Kral wrote:
> On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
>> Compare the running configuration with the newly compiled state during
>> config commit to identify and prune orphaned SDN ACL entries. This
>> ensures state consistency for manual applies via the UI/API as well as
>> during automatic configuration reloads on system boot.
>>
>> Track configuration changes across:
>> * Zones
>> * VNets
>> * Fabrics
>> * Controllers
>> * Route maps
>> * Prefix lists
>>
>> Trigger a validation check when a VNet is moved between zones to
>> mitigate potential ACL path conflicts. Abort the apply operation if a
>> conflict is detected to prevent configuration overwrites. If the path
>> is clear, relocate the ACLs to the updated zone path.
>>
>> Suggested-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> Signed-off-by: David Riley <d.riley@proxmox.com>
>> ---
>>   src/PVE/Network/SDN.pm | 141 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 141 insertions(+)
>>
>> diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
>> index 33a3cf3..cd563d2 100644
>> --- a/src/PVE/Network/SDN.pm
>> +++ b/src/PVE/Network/SDN.pm
>> @@ -10,6 +10,7 @@ use LWP::UserAgent;
>>   use Net::SSLeay;
>>   use UUID;
>>   
>> +use PVE::AccessControl;
>>   use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
>>   use PVE::INotify;
>>   use PVE::RESTEnvironment qw(log_warn);
>> @@ -238,11 +239,151 @@ sub compile_running_cfg {
>>   }
>>   
>>   sub commit_config {
>> +    my $old_cfg = cfs_read_file($RUNNING_CFG_FILENAME);
>>       my $cfg = compile_running_cfg();
>>   
>> +    cleanup($old_cfg, $cfg);
>> +
>>       cfs_write_file($RUNNING_CFG_FILENAME, $cfg);
>>   }
>>   
>> +sub cleanup {
> maybe a more telling name like cleanup_sdn_resource_acls(...) or
> something similar? Otherwise, it might suggest that it cleans up
> something in the SDN config.
>
Intentionally kept it generic, because I'm hooking into this cleanup routine
in my Pool Patch series [0], which adds VNets as Pool Members. I use the Vnet
diff to remove/move VNets in the pool configuration so that it stays in  sync with
the state of the SDN configuration [1].
So if I name it cleanup_sdn_resource_acls it might not be obvious that it also
touches the pool configuration, but one could argue that Pools are related to
ACLs.

I will most likely rename it to:
cleanup_access_control

[0] https://lore.proxmox.com/pve-devel/20260626131035.112374-1-d.riley@proxmox.com/
[1] https://lore.proxmox.com/pve-devel/20260626131035.112374-8-d.riley@proxmox.com/
>> +    my ($old_cfg, $cfg) = @_;
>> +    if (!defined($old_cfg) || !defined($cfg)) {
>> +        return;
>> +    }
> AFAICT $old_cfg and $cfg are always defined as the cfs_read_file()
> parser for $RUNNING_CFG_FILENAME will always at least return an empty
> hash ref and compile_running_cfg() will also always return a hash ref,
> so this should not be needed?
I think you are right. I was being very defensive in this patch series.
Will drop this check.
>> +
>> +    my $route_map_paths = diff_route_maps($old_cfg, $cfg);
>> +    my $generic_paths = diff_generic_resources($old_cfg, $cfg);
>> +    my ($vnet_delete_paths, $vnet_move_paths) = diff_vnets($old_cfg, $cfg);
>> +
>> +    if (@$vnet_move_paths) {
>> +        PVE::AccessControl::migrate_sdn_resource_access($vnet_move_paths);
>> +    }
>> +
>> +    my @paths_to_delete = (@$vnet_delete_paths, @$route_map_paths, @$generic_paths);
>> +    if (@paths_to_delete) {
>> +        PVE::AccessControl::remove_sdn_resource_access(\@paths_to_delete);
>> +    }
>> +
>> +}
>> +
>> +sub diff_generic_resources {
>> +    my ($old_cfg, $cfg) = @_;
>> +
>> +    my @paths_to_delete;
> nit: initialize with empty array ()
>
>> +
>> +    my @types = qw(zones controllers fabrics prefix-lists);
>> +    for my $type (@types) {
>> +        if (!defined($old_cfg->{$type}) || !defined($old_cfg->{$type}->{ids})) {
>> +            next;
>> +        }
>> +
>> +        my $old_ids = $old_cfg->{$type}->{ids};
>> +        my $new_ids = {};
>> +
>> +        if (defined($cfg->{$type}) && defined($cfg->{$type}->{ids})) {
>> +            $new_ids = $cfg->{$type}->{ids};
>> +        }
> As compile_running_cfg() does always set the $cfg->{$type}->{ids} this
> could probably be set unconditionally?
That's true, will simplify that and drop the redundant definition checks.
>> +
>> +        for my $id (keys %$old_ids) {
>> +            if (defined($new_ids->{$id})) {
>> +                next;
>> +            }
> nit: use post-if statement
>
>> +
>> +            push(@paths_to_delete, [$type, $id]);
>> +        }
>> +    }
>> +
>> +    return \@paths_to_delete;
>> +}
>> +
>> +sub diff_route_maps {
>> +    my ($old_cfg, $cfg) = @_;
>> +
>> +    my @paths_to_delete;
> nit: initialize with empty array ()
>
>> +
>> +    if (defined($old_cfg->{'route-maps'}) && defined($old_cfg->{'route-maps'}->{ids})) {
>> +        my $old_route_maps = $old_cfg->{'route-maps'}->{ids};
>> +        my $route_map_suffix = qr/_[0-9]+$/;
> nit: suffix with _re(gex)
>
>> +
>> +        my %active_route_maps;
> nit: initialize with empty hash ()
>
>> +        if (defined($cfg->{'route-maps'}) && defined($cfg->{'route-maps'}->{ids})) {
>> +            for my $id (keys %{ $cfg->{'route-maps'}->{ids} }) {
> nit: use postfix:    keys $cfg->{'route-maps'}->{ids}->%*
>
>> +                (my $base_name = $id) =~ s/$route_map_suffix//;
>> +                $active_route_maps{$base_name} = 1;
>> +            }
>> +        }
>> +
>> +        my %queued_route_maps;
> nit: initialize with empty hash ()
>
>> +        for my $id (keys %$old_route_maps) {
>> +            if (defined($cfg->{'route-maps'}->{ids}->{$id})) {
>> +                next;
>> +            }
> nit: use post-if statement
>
>> +
>> +            (my $base_name = $id) =~ s/$route_map_suffix//;
>> +
>> +            if ($active_route_maps{$base_name}) {
>> +                next;
>> +            }
> nit: use post-if statement
>
>> +
>> +            if ($queued_route_maps{$base_name}) {
>> +                next;
>> +            }
> nit: use post-if statement
>
>> +
>> +            push(@paths_to_delete, ['route-maps', $base_name]);
>> +            $queued_route_maps{$base_name} = 1;
>> +        }
>> +    }
>> +
>> +    return \@paths_to_delete;
>> +}
>> +
>> +sub diff_vnets {
>> +    my ($old_cfg, $cfg) = @_;
>> +
>> +    my @paths_to_delete;
>> +    my @paths_to_move;
> nit: initialize with empty arrays ()
>
>> +
>> +    if (defined($old_cfg->{vnets}) && defined($old_cfg->{vnets}->{ids})) {
>> +        my $old_vnets = $old_cfg->{vnets}->{ids};
>> +        my $new_vnets = {};
>> +
>> +        if (defined($cfg->{vnets}) && defined($cfg->{vnets}->{ids})) {
>> +            $new_vnets = $cfg->{vnets}->{ids};
>> +        }
> same as above, could be set unconditionally as compile_running_cfg()
> always sets the $cfg->{vnets}->{ids} hash ref
>
>> +
>> +        for my $vnetid (keys %$old_vnets) {
>> +            my $old_zone = $old_vnets->{$vnetid}->{zone};
>> +
>> +            if (!defined($new_vnets->{$vnetid})) {
>> +                if (defined($old_zone)) {
>> +                    push(@paths_to_delete, ['zones', $old_zone, $vnetid]);
>> +                } else {
>> +                    log_warn(
>> +                        "SDN Cleanup: Could not find zone for deleted VNet $vnetid, skipping ACL"
>> +                            . " cleanup");
>> +                }
>> +                next;
>> +            }
>> +
>> +            my $new_zone = $new_vnets->{$vnetid}->{zone};
>> +
>> +            if (defined($old_zone) && defined($new_zone) && $old_zone ne $new_zone) {
>> +                push(
>> +                    @paths_to_move,
>> +                    {
>> +                        src_path => ['zones', $old_zone, $vnetid],
>> +                        dest_path => ['zones', $new_zone, $vnetid],
>> +                    },
>> +                );
>> +            }
> Hm, there's a lot of testing whether the zone exists... Can this
> actually happen besides users directly editing the sdn/vnets.cfg file
> and removing the vnet property?
You are absolutely right, these checks don't add any real value since the API
and parser enforce this. I refactor this in v5.

> The API handler doesn't permit me to add a VNet without or with a
> non-existing zone (only the zone 'abc' is existing; the error message is
> a bit misleading, because the type is undefined here because the zone
> does not exist at all):
>
> # pvesh create /cluster/sdn/vnets --vnet aeaewk --zone '' --type 'vnet'
> create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326.
> # pvesh create /cluster/sdn/vnets --vnet aeaewk --zone 'abc' --type 'vnet'
> # pvesh create /cluster/sdn/vnets --vnet aeaewk2 --zone 'def' --type 'vnet'
> create sdn vnet object failed: cannot lookup undefined type! at /usr/share/perl5/PVE/API2/Network/SDN/Vnets.pm line 326.
>
>> +        }
>> +    }
>> +
>> +    return (\@paths_to_delete, \@paths_to_move);
>> +}
>> +
>>   sub has_pending_changes {
>>       my $running_cfg = PVE::Network::SDN::running_config();
>>   
>

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

[David Riley]

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,
>
>
>
>
>

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

[Daniel Kral]

On Fri Jul 3, 2026 at 2:10 PM CEST, David Riley wrote:
> 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:

[ snip ]

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

Yeah, good point about it re-creating the path nodes and not
accidentally breaking existing callers!

If you keep it, then I'd add it next to find_acl_tree_node() (maybe
align with that name, e.g. find_acl_tree_node_with_parent()?) and add a
subroutine comment what it does (differently), so others are aware that
this exists as well.

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