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 3C6141FF142 for ; Fri, 03 Jul 2026 14:10:38 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7982A21373; Fri, 03 Jul 2026 14:10:37 +0200 (CEST) Message-ID: <8d7a699c-f820-49c0-939b-e4a91668b50d@proxmox.com> Date: Fri, 3 Jul 2026 14:10:32 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260626105258.56914-1-d.riley@proxmox.com> <20260626105258.56914-2-d.riley@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783080625225 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.077 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: 5LRQDTTKSNJ5BIII6FSMHBHGMKNCND3R X-Message-ID-Hash: 5LRQDTTKSNJ5BIII6FSMHBHGMKNCND3R X-MailFrom: d.riley@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: 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 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. Alright that make sense. Will do that. >> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520 >> 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"); >> } >> >> +sub remove_sdn_resource_access { >> + my ($paths) = @_; # [ ['zones', ''], ['zones', '', ''] ] > $paths might be confusing here as in pve-access-control variables named > 'path' are usually the string representation, e.g. /zones/. 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//... 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, > > > > >