From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id A3A631FF135 for ; Thu, 02 Jul 2026 15:26:54 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3A7042148D; Thu, 02 Jul 2026 15:26:51 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 02 Jul 2026 15:26:15 +0200 Message-Id: Subject: Re: [PATCH pve-access-control v4 3/5] fix: #7520: sdn: add VNet ACL migration From: "Daniel Kral" To: "David Riley" , X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626105258.56914-1-d.riley@proxmox.com> <20260626105258.56914-4-d.riley@proxmox.com> In-Reply-To: <20260626105258.56914-4-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782998769506 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.075 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: VZGH7YLSERSVHBZ6H2DEN6FVC4WBJDPZ X-Message-ID-Hash: VZGH7YLSERSVHBZ6H2DEN6FVC4WBJDPZ X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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=3D7520 > Signed-off-by: David Riley > --- > 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); > } > =20 > +sub migrate_sdn_resource_access { > + my ($migrations) =3D @_; # [ { src_path =3D> [...], dest_path =3D> [= ...] } ] > + if (!defined($migrations) || scalar(@$migrations) =3D=3D 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 =3D sub { > + my $usercfg =3D cfs_read_file("user.cfg"); > + my $modified =3D 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->{$ke= y}) 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 =3D ['sdn', @{ $migration->{src_path} }]; > + my $dest_path =3D ['sdn', @{ $migration->{dest_path} }]; nit: use postfix $migration->{src_path}->@* > + > + my ($src_parent, $src_last_key, $acl_node) =3D > + lookup_acl_node($usercfg->{acl_root}, $src_path); > + > + if (defined($src_parent) && defined($src_last_key) && define= d($acl_node)) { > + # probe dest for conflicts > + my (undef, undef, $existing_dest_node) =3D > + lookup_acl_node($usercfg->{acl_root}, $dest_path); > + > + if (acl_tree_has_permissions($existing_dest_node)) { > + my $conflict_path =3D '/' . join('/', @$dest_path); > + my $source_path =3D '/' . join('/', @$src_path); > + die > + "Destination '$conflict_path' already has permis= sions configured (Source:" > + . " '$source_path'). Please remove the target AC= Ls 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 =3D> $src_parent, > + src_last_key =3D> $src_last_key, > + dest_path =3D> $dest_path, > + node =3D> $acl_node, > + }, > + ); > + } > + } > + > + for my $move (@prepared_moves) { > + delete $move->{src_parent}->{children}->{ $move->{src_last_k= ey} }; > + > + my $current =3D $usercfg->{acl_root}; > + my @dest_segments =3D @{ $move->{dest_path} }; > + my $dest_last_key =3D pop @dest_segments; > + > + for my $segment (@dest_segments) { > + if (!defined($current->{children}->{$segment})) { > + $current->{children}->{$segment} =3D {}; > + } nit: use post-if here > + $current =3D $current->{children}->{$segment}; > + } > + $current->{children}->{$dest_last_key} =3D $move->{node}; > + > + $modified =3D 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) =3D @_; > + > + 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 =3D { > u2f =3D> 1, > oath =3D> 1,