From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 6AAF21FF135 for ; Thu, 02 Jul 2026 15:15:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EA05A213E8; Thu, 02 Jul 2026 15:15:10 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 02 Jul 2026 15:14:37 +0200 Message-Id: Subject: Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion 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-2-d.riley@proxmox.com> In-Reply-To: <20260626105258.56914-2-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782998070794 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.150 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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: HXJQCGVWYQTOTPNWNJN7HAQHJANBFQZE X-Message-ID-Hash: HXJQCGVWYQTOTPNWNJN7HAQHJANBFQZE 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: > 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 in preparation of 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=3D7520 > Signed-off-by: David Riley > --- > 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= "); > } > =20 > +sub remove_sdn_resource_access { > + my ($paths) =3D @_; # [ ['zones', ''], ['zones', '', ''] ] $paths might be confusing here as in pve-access-control variables named 'path' are usually the string representation, e.g. /zones/. > + > + my $delete_resource_fn =3D sub { > + my $usercfg =3D cfs_read_file("user.cfg"); > + my $modified =3D 0; > + > + for my $segments (@$paths) { > + my @full_path =3D ('sdn', @$segments); > + my $current =3D $usercfg->{acl_root}; > + my ($parent, $last_key) =3D lookup_acl_node($current, \@full= _path); > + > + if ($parent && $last_key) { > + delete $parent->{children}->{$last_key}; > + $modified =3D 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) =3D @_; > + > + my $current =3D $root; > + my $parent =3D undef; > + my $last_key =3D 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 =3D $current; > + $last_key =3D $step; > + $current =3D $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) =3D find_acl_tree_node_full($root, $path); return $node; } No hard feelings here though. > + > my $USER_CONTROLLED_TFA_TYPES =3D { > u2f =3D> 1, > oath =3D> 1,