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 75DD71FF0C2 for ; Thu, 22 Oct 2026 14:48:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 53A32217D0; Thu, 22 Oct 2026 14:47:58 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v4 14/27] fix #8176: firewall: groups: handle dangling references on delete Date: Thu, 22 Oct 2026 14:46:55 +0200 Message-ID: <20261022124708.429069-15-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261022124708.429069-1-a.bied-charreton@proxmox.com> References: <20261022124708.429069-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792673237443 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.891 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) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: 5TIYUKD3UGDCB5NZS7WULSEELBBUB7NQ X-Message-ID-Hash: 5TIYUKD3UGDCB5NZS7WULSEELBBUB7NQ X-MailFrom: a.bied-charreton@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: Deleting a security group referenced by rules leaves dangling references. Nothing checks that a group rule names a group that exists. The dangling reference is carried into the generated ruleset and what happens then depends on the firewall implementation. Add a 'dangling-references' option to the delete endpoint, to either 'disable' the referencing rules or 'drop' them along with the security group. The default, 'keep', leaves them as they are. The delete runs as a task for observability, all changes made to configs are logged to the task log. Link: https://bugzilla.proxmox.com/show_bug.cgi?id=8176 Signed-off-by: Arthur Bied-Charreton --- src/PVE/API2/Firewall/Rules.pm | 53 ++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm index 785ad05..17f76df 100644 --- a/src/PVE/API2/Firewall/Rules.pm +++ b/src/PVE/API2/Firewall/Rules.pm @@ -487,6 +487,7 @@ package PVE::API2::Firewall::GroupRules; use strict; use warnings; use PVE::JSONSchema qw(get_standard_option); +use PVE::Firewall::References qw(update_refs run_ref_update); use base qw(PVE::API2::Firewall::RulesBase); @@ -539,27 +540,55 @@ __PACKAGE__->register_method({ additionalProperties => 0, properties => { group => get_standard_option('pve-security-group-name'), + 'dangling-references' => { + type => 'string', + enum => ['keep', 'disable', 'drop'], + optional => 1, + description => + "Handle references that the deletion would leave dangling. Use 'disable' " + . "to disable the referencing rules, or 'drop' to remove them entirely.", + default => 'keep', + }, }, }, - returns => { type => 'null' }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; - __PACKAGE__->lock_config( - $param, - sub { - my ($param) = @_; + my $action = $param->{'dangling-references'} // 'keep'; + my $subject = "security group '$param->{group}' deleted"; - my (undef, $cluster_conf, $rules) = __PACKAGE__->load_config($param); + my $code = sub { + __PACKAGE__->lock_config( + $param, + sub { + my ($param) = @_; - die "Security group '$param->{group}' is not empty\n" - if scalar(@$rules); + my (undef, $cluster_conf, $rules) = __PACKAGE__->load_config($param); - __PACKAGE__->save_rules($param, $cluster_conf, undef); - }, - ); + die "Security group '$param->{group}' is not empty\n" + if scalar(@$rules); + + update_refs({ + conf => $cluster_conf, + kind => 'groups', + old => $param->{group}, + env => 'cluster', + action => $action eq 'keep' ? undef : $action, + subject => $subject, + }); + + __PACKAGE__->save_rules($param, $cluster_conf, undef); + }, + ); + }; + + # fail on the request instead of inside the task. + my (undef, undef, $rules) = __PACKAGE__->load_config($param); + + die "Security group '$param->{group}' is not empty\n" if scalar($rules->@*); - return undef; + return run_ref_update('fw-group-delete', $param->{group}, $code); }, }); -- 2.47.3