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 821E31FF0B3 for ; Fri, 25 Sep 2026 11:42:36 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B77372169E; Fri, 25 Sep 2026 11:42:34 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v3 01/16] helpers: add helpers to update firewall object references Date: Fri, 25 Sep 2026 11:42:15 +0200 Message-ID: <20260925094230.844917-2-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260925094230.844917-1-a.bied-charreton@proxmox.com> References: <20260925094230.844917-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 AWL -1.116 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 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 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: L55GC6GZWWLLDWDPOZKV5VPM73HNBMOR X-Message-ID-Hash: L55GC6GZWWLLDWDPOZKV5VPM73HNBMOR 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, security groups 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 update_refs(), which finds such references and applies one of three actions to them: 'rename' points them at the new name, 'disable' turns off the referencing rules, and 'drop' removes them. An ipset member has no disabled state, so 'disable' removes members as well. Matching is case-insensitive and rewritten references are normalized to lowercase. When operating on the cluster config it also covers every downstream config (guest, host and vnet) across the cluster. On top of that, add three wrappers for the SDN-generated IPSets, which no caller can delete but which disappear once the configuration stops generating them: update_sdn_ipset_refs() for a list of such IPSets, update_vnet_ipset_refs() for the four IPSets of a VNet, and update_guest_ipam_ipset_refs() for a guest's IPAM IPSet. 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/, sdn/) if the rules have been added by manually editing the configs. 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. SDN IPSets can additionally be shadowed by IPSets defined in the cluster config. This is handled by also checking for same-named IPSets in the cluster config for SDN IPSets. The ipset and alias endpoints in the following commits build on this, as do PUT /cluster/sdn in pve-network and the guest destroy endpoints in qemu-server and pve-container. Signed-off-by: Arthur Bied-Charreton --- src/PVE/Firewall/Helpers.pm | 308 ++++++++++++++++++++++++++++++++++++ 1 file changed, 308 insertions(+) diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm index fa3646c..2331e27 100644 --- a/src/PVE/Firewall/Helpers.pm +++ b/src/PVE/Firewall/Helpers.pm @@ -9,6 +9,7 @@ use File::Basename qw(fileparse); use IO::Zlib; use PVE::Cluster; use PVE::Network; +use PVE::Network::SDN::Vnets; use PVE::Tools qw(file_get_contents file_set_contents); use base 'Exporter'; @@ -18,8 +19,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 +239,307 @@ 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' && $rule_env ne 'sdn'; + + 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, $rule_env, 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, $rule_env, 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, $rule_env, 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, $cluster_conf, $action) + +Apply C<$action> to all references to C<$old> across C<$conf>, renaming them to C<$new>, disabling +the referencing rules, or dropping them. + +Only exposed for testing, see POD for C for details. + +=cut + +sub rewrite_refs_in_conf { + my ($conf, $spec, $old, $new, $env, $is_guest, $cluster_conf, $action) = @_; + + my $ref_fields = ['source', 'dest', 'cidr']; + my $prefix = $spec->{prefix}; + + my $repl = {}; + for my $name ($old->@*) { + my $shadowed = $is_guest && $conf->{ $spec->{self} }->{$name}; + + my $scopes = []; + if ($env eq 'cluster') { + push $scopes->@*, 'dc/'; + push $scopes->@*, '' if !$shadowed; + } elsif ($env eq 'sdn') { + push $scopes->@*, 'sdn/'; + push $scopes->@*, '' if !$shadowed && !$cluster_conf->{ $spec->{self} }->{$name}; + } else { + push $scopes->@*, ''; + push $scopes->@*, 'guest/'; + } + + $repl->{"$prefix$_$name"} = defined($new) ? "$prefix$_$new" : undef for $scopes->@*; + } + + my $matches = sub { + my ($obj) = @_; + grep { exists($repl->{ lc($obj->{$_} // '') }) } $ref_fields->@*; + }; + + my $rename = sub { + my ($obj) = @_; + + for my $f ($ref_fields->@*) { + my $r = lc($obj->{$f} // ''); + $obj->{$f} = $repl->{$r} if exists($repl->{$r}); + } + + return $obj; + }; + + my $rewrite_rule = sub { + my ($obj) = @_; + + return $rename->($obj) if $action eq 'rename'; + return undef if $action eq 'drop'; + + $obj->{enable} = 0; + return $obj; + }; + + my $rewrite_member = sub { + my ($obj) = @_; + # an ipset member cannot be disabled + return $action eq 'rename' ? $rename->($obj) : undef; + }; + + my $modified = 0; + my ($rules, $ch) = map_items($conf->{rules}, $rewrite_rule, $matches); + $conf->{rules} = $rules; + $modified ||= $ch; + + for my $section ([groups => $rewrite_rule], [ipset => $rewrite_member]) { + my ($name, $rewrite) = $section->@*; + my $map = $conf->{$name} // {}; + 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, $action) + +Rename, disable or drop all references to a firewall object across the environment. + +References are matched in rules, security groups and IPSet members. Matching is case-insensitive +and renames are written back lowercased. + +C<$conf> is the firewall configuration the object is defined in. + +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. + +C<$old> is the name of the object whose references are to be edited. The disable and drop paths +also accept an array reference, so that references to several objects can be handled in a single +pass over the configs. + +C<$new> is the new name of the object, and is only used when renaming. + +C<$rule_env> describes the environment the object comes from, and may be C for cluster +objects, C for SDN objects, or C/C for guest-defined objects. + +C<$action> is what to do with the references, and may be C to point them at C<$new>, +C to disable the referencing rules, or C to remove them. IPSet members have no +disabled state, so C removes them as well. It defaults to C if C<$new> is +defined, and to C otherwise. + +The caller is responsible for locking and saving C<$conf>. + +If C<$conf> is the cluster config, i.e. if C<$rule_env> is C or C, guest, host and +vnet configs will be sequentially locked, updated and saved. Therefore, if this function is called +for one of those environments, 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, $action) = @_; + + $action //= defined($new) ? 'rename' : 'disable'; + + die "invalid action '$action'\n" if $action !~ m/^(rename|disable|drop)$/; + + my $lc_old = [map { lc($_) } (ref($old) eq 'ARRAY' ? $old->@* : $old)]; + my $lc_new = $action eq 'rename' ? lc($new) : undef; + + die "cannot rename more than one object at once\n" + if $action eq 'rename' && scalar($lc_old->@*) != 1; + + my $code = sub { + my ($fw_conf, $env, $is_guest) = @_; + return rewrite_refs_in_conf( + $fw_conf, $spec, $lc_old, $lc_new, $env, $is_guest, $conf, $action, + ); + }; + + return foreach_conf_in_env($conf, $rule_env, $code); +} + +=head3 update_sdn_ipset_refs($ipsets, $action) + +Apply C<$action> to all references to the SDN-generated IPSets named in C<$ipsets>, which are not +objects a caller could delete, but exist for as long as the SDN configuration generates them. + +=cut + +sub update_sdn_ipset_refs { + my ($ipsets, $action) = @_; + + PVE::Firewall::lock_clusterfw_conf( + 10, + sub { + my $conf = PVE::Firewall::load_clusterfw_conf(); + + update_refs($conf, get_object_spec('ipset'), $ipsets, undef, 'sdn', $action); + + PVE::Firewall::save_clusterfw_conf($conf); + }, + ); +} + +=head3 update_vnet_ipset_refs($vnets, $action) + +Apply C<$action> to all references to the IPSets generated for the VNets named in C<$vnets>, for +callers about to remove those VNets from the running SDN configuration. + +=cut + +sub update_vnet_ipset_refs { + my ($vnets, $action) = @_; + + my $ipsets = [ + map { + my $vnet = $_; + map { "$vnet-$_" } qw(all gateway no-gateway dhcp) + } $vnets->@* + ]; + + return update_sdn_ipset_refs($ipsets, $action); +} + +=head3 update_guest_ipam_ipset_refs($vmid, $action) + +Apply C<$action> to all references to the IPAM IPSet of the guest C<$vmid>, for callers about to +destroy that guest. Note that this IPSet also disappears whenever a guest releases its last IPAM +entry, which happens on ordinary network changes too, so this is only appropriate when the guest +is going away for good. + +=cut + +sub update_guest_ipam_ipset_refs { + my ($vmid, $action) = @_; + + return update_sdn_ipset_refs("guest-ipam-$vmid", $action); +} 1; -- 2.47.3