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 81E591FF0A7 for ; Tue, 18 Aug 2026 15:35:37 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 359D3216B0; Tue, 18 Aug 2026 15:34:26 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v2 1/9] api: helpers: add helper to update firewall object references Date: Tue, 18 Aug 2026 15:34:05 +0200 Message-ID: <20260818133413.450776-2-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260818133413.450776-1-a.bied-charreton@proxmox.com> References: <20260818133413.450776-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 AWL -0.666 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) KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Message-ID-Hash: AOO3KV7KFCH7IB3ITI2CWFTGL67QVR3W X-Message-ID-Hash: AOO3KV7KFCH7IB3ITI2CWFTGL67QVR3W X-MailFrom: abied-charreton@jett.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: Renaming or deleting a firewall object (ipset or alias) that is still referenced by rules or ipset members leaves dangling references. The firewall fails to parse the affected rules and drops them, so an edit can effectively disable a whole set of rules. Add a shared helper that finds and rewrites (on rename) or removes (on delete) all such references. When operating on the cluster config it also does so for every downstream config (guest, host and vnet) across the cluster. Matching is case-insensitive and rewritten references are normalized to lowercase. Downstream configs are locked and saved individually as they are visited, so on a cluster rename the caller must persist the config with the new object already present before calling update_refs (keep both the old and new object until all references are migrated). Otherwise a concurrent firewall compilation could encounter a reference to an object that does not exist yet and drop the rule. Object references are not guaranteed to be scoped (dc/, guest/). This is not an issue for cluster, host and vnet configs, as in those cases the reference can only point to an object defined in the cluster config. Guest configs can however define their own objects. Unscoped references in guest rules are therefore resolved by first checking for a definition in the relevant guest config, and only then in the cluster config, to prevent overwriting the wrong reference. The ipset and alias endpoints build on this in the following commits. Signed-off-by: Arthur Bied-Charreton --- src/PVE/Firewall/Helpers.pm | 195 ++++++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm index fa3646c..3bdb7dc 100644 --- a/src/PVE/Firewall/Helpers.pm +++ b/src/PVE/Firewall/Helpers.pm @@ -18,8 +18,12 @@ our @EXPORT_OK = qw( clone_vmfw_conf collect_refs flush_fw_ct_entries_by_mark + update_refs + get_object_spec ); +require PVE::Firewall; + my $pvefw_conf_dir = "/etc/pve/firewall"; sub lock_vmfw_conf { @@ -234,4 +238,195 @@ sub flush_fw_ct_entries_by_mark { ); } +=head3 map_items($items, $action, $matches) + +Apply C<$action> to each item C<$item> in C<$items> for which C<$matches->($item)> is true. Remove +C<$item> from C<$items> if C<$action->($item)> returns C. + +Return the updated items arrayref and a boolean indicating whether any item was matched. + +=cut + +sub map_items { + my ($items, $action, $matches) = @_; + my @result; + my $modified = 0; + for my $item (($items // [])->@*) { + if ($matches->($item)) { + $modified = 1; + my $new = $action->($item); + push @result, $new if defined $new; + } else { + push @result, $item; + } + } + return (\@result, $modified); +} + +=head3 foreach_conf_in_env($conf, $rule_env, $rewrite) + +Apply C<$rewrite> to the main firewall configs and, if C<$rule_env> is 'cluster', to all guest, host +and vnet firewall configs across the cluster. Configs where C<$rewrite> returns true are saved. The +caller is responsible for locking and saving the cluster config (C<$conf>). + +=cut + +sub foreach_conf_in_env { + my ($conf, $rule_env, $rewrite) = @_; + + $rewrite->($conf, $rule_env, 0); + + return if $rule_env ne 'cluster'; + + my $vmlist = PVE::Cluster::get_vmlist(); + my $vmids = ($vmlist // {})->{ids} // {}; + for my $vmid (keys $vmids->%*) { + PVE::Firewall::lock_vmfw_conf( + $vmid, + 10, + sub { + my $env = $vmlist->{ids}->{$vmid}->{type} eq 'lxc' ? 'ct' : 'vm'; + my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $env, $vmid); + if ($rewrite->($guest_conf, 'cluster', 1)) { + PVE::Firewall::save_vmfw_conf($vmid, $guest_conf); + } + }, + ); + } + + for my $node (PVE::Cluster::get_nodelist()->@*) { + my $host_conf_path = "/etc/pve/nodes/$node/host.fw"; + PVE::Firewall::lock_hostfw_conf( + $node, + 10, + sub { + my $host_conf = PVE::Firewall::load_hostfw_conf($conf, $host_conf_path); + if ($rewrite->($host_conf, 'cluster', 0)) { + PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); + } + }, + ); + } + + my $vnets = (PVE::Network::SDN::Vnets::config(1) // {})->{ids} // {}; + for my $vnet (keys $vnets->%*) { + PVE::Firewall::lock_vnetfw_conf( + $vnet, + 10, + sub { + my $vnet_conf = PVE::Firewall::load_vnetfw_conf($conf, 'vnet', $vnet); + if ($rewrite->($vnet_conf, 'cluster', 0)) { + PVE::Firewall::save_vnetfw_conf($vnet, $vnet_conf); + } + }, + ); + } +} + +my $object_ref_specs = { + ipset => { prefix => '+', self => 'ipset' }, + aliases => { prefix => '', self => 'aliases' }, +}; + +=head3 get_object_spec($kind) + +Get the spec hash for C<$kind>. Refer to the C POD for details. + +=cut + +sub get_object_spec { + my ($kind) = @_; + return $object_ref_specs->{$kind}; +} + +=head3 rewrite_refs_in_conf($conf, $spec, $old, $new, $env, $is_guest) + +Rename all references (or with C<$new> undef, delete referencing rules) to C<$old> across C<$conf>. + +Only exposed for testing, see POD for C for details. + +=cut + +sub rewrite_refs_in_conf { + my ($conf, $spec, $old, $new, $env, $is_guest) = @_; + + my $ref_fields = ['source', 'dest', 'cidr']; + my $shadowed = $is_guest && $conf->{ $spec->{self} }->{$old}; + + my $scopes = []; + if ($env eq 'cluster') { + push $scopes->@*, 'dc/'; + push $scopes->@*, '' if !$shadowed; + } else { + push $scopes->@*, ''; + push $scopes->@*, 'guest/'; + } + + my $prefix = $spec->{prefix}; + my $repl = { map { ("$prefix$_$old" => defined($new) ? "$prefix$_$new" : undef) } $scopes->@* }; + + my $matches = sub { + my ($obj) = @_; + grep { exists($repl->{ lc($obj->{$_} // '') }) } $ref_fields->@*; + }; + + my $rewrite = sub { + my ($obj) = @_; + return undef if !defined($new); + for my $f ($ref_fields->@*) { + my $r = lc($obj->{$f} // ''); + $obj->{$f} = $repl->{$r} if exists($repl->{$r}); + } + return $obj; + }; + + my $modified = 0; + my ($rules, $ch) = map_items($conf->{rules}, $rewrite, $matches); + $conf->{rules} = $rules; + $modified ||= $ch; + + for my $section (qw(groups ipset)) { + my $map = $conf->{$section} // {}; + for my $key (keys $map->%*) { + ($map->{$key}, my $c) = map_items($map->{$key}, $rewrite, $matches); + $modified ||= $c; + } + } + + return $modified; +} + +=head3 update_refs($conf, $spec, $old, $new, $rule_env) + +Rename (or, with C<$new> undef, delete) all references to a firewall object across the environment. +C<$spec> describes the object kind: + + { prefix => '+' | '', self => 'ipset' | 'aliases' } + +C is the prefix a reference carries, C is the section a downstream config may use to +shadow a same-named cluster object. References are matched in rules, security groups and IPSet +members. Matching is case-insensitive and renames are written back lowercased. + +The caller is responsible for locking and saving C<$conf>. + +If C<$conf> is the cluster config, i.e. C<$rule_env eq 'cluster'>, guest, host and vnet configs will +be sequentially locked, updated and saved. Therefore, if this function is called for the cluster +environment, a I caller must first persist C<$conf> with the new (renamed) object present, +so references rewritten in those downstream configs do not point at a not-yet-saved object during +concurrent compilations. + +=cut + +sub update_refs { + my ($conf, $spec, $old, $new, $rule_env) = @_; + + my ($lc_old, $lc_new) = (lc($old), defined($new) ? lc($new) : undef); + + my $code = sub { + my ($fw_conf, $env, $is_guest) = @_; + return rewrite_refs_in_conf($fw_conf, $spec, $lc_old, $lc_new, $env, $is_guest); + }; + + return foreach_conf_in_env($conf, $rule_env, $code); +} 1; -- 2.47.3