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 E0B151FF09B for ; Mon, 12 Oct 2026 11:40:41 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 88657215CA; Mon, 12 Oct 2026 11:40:23 +0200 (CEST) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall 1/2] firewall: ebtables: resolve aliases in ipfilter ipsets Date: Mon, 12 Oct 2026 11:38:07 +0200 Message-ID: <20261012093956.285433-2-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261012093956.285433-1-a.bied-charreton@proxmox.com> References: <20261012093956.285433-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: 1791798016745 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.017 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: O4YOFMW7G2NIBOHZXW2GKENMI6SWT2QM X-Message-ID-Hash: O4YOFMW7G2NIBOHZXW2GKENMI6SWT2QM 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: Entries of an ipfilter-netX ipset were passed straight to parse_ip_or_cidr() when building the ARP filter, so an alias entry or one the parser already flagged as invalid, made it die. The error dropped all ebtables rules for the guest, including the MAC and ARP source filters, and was logged on every update cycle without saying which guest was affected. Resolve entries the same way generate_ipset_chains() does, skip erroneous ones, and log resolution errors per entry with context. Signed-off-by: Arthur Bied-Charreton --- src/PVE/Firewall.pm | 70 +++++++++++++++--------- src/test/Makefile | 2 +- src/test/test_ebtables_ipfilter.pl | 85 ++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 25 deletions(-) create mode 100755 src/test/test_ebtables_ipfilter.pl diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index da6976e..bb3350b 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -4074,6 +4074,16 @@ sub generate_std_chains { } } +my sub resolve_ipset_entry { + my ($cluster_conf, $fw_conf, $cidr) = @_; + + if ($cidr =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) { + return resolve_alias($cluster_conf, $fw_conf, $2, $1 // ""); + } + + return parse_ip_or_cidr($cidr); +} + sub generate_ipset_chains { my ($ipset_ruleset, $clusterfw_conf, $fw_conf, $device_ips, $ipsets) = @_; @@ -4092,14 +4102,8 @@ sub generate_ipset_chains { foreach my $entry (@$options) { next if $entry->{errors}; # skip entries with errors eval { - my ($cidr, $ver); - if ($entry->{cidr} =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) { - my $scope = $1 // ""; - my $alias = $2; - ($cidr, $ver) = resolve_alias($clusterfw_conf, $fw_conf, $alias, $scope); - } else { - ($cidr, $ver) = parse_ip_or_cidr($entry->{cidr}); - } + my ($cidr, $ver) = + resolve_ipset_entry($clusterfw_conf, $fw_conf, $entry->{cidr}); #http://backreference.org/2013/03/01/ipv6-address-normalization/ if ($ver == 6) { # ip_compress_address takes an address only, no CIDR @@ -4810,6 +4814,32 @@ sub compile_ipsets { return $ipset_ruleset; } +my sub resolve_arpfilter { + my ($ipset, $cluster_conf, $vm_conf, $vmid, $netid) = @_; + return [] if !defined($ipset); + + my $arpfilter = []; + for my $entry ($ipset->@*) { + next if $entry->{errors}; + + my ($ip, $ver) = eval { resolve_ipset_entry($cluster_conf, $vm_conf, $entry->{cidr}) }; + if ($@) { + my $err = "$vmid: ipfilter-$netid: skipping '$entry->{cidr}': $@"; + if ($verbose) { + warn $err; + } else { + syslog('warn', $err); + } + next; + } + + next if $ver != 4; + push $arpfilter->@*, $ip; + } + + return $arpfilter; +} + sub compile_ebtables_filter { my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata) = @_; @@ -4842,14 +4872,10 @@ sub compile_ebtables_filter { my $iface = "tap${vmid}i$1"; next if !valid_interface_name($iface); my $macaddr = $net->{macaddr}; - my $arpfilter = []; - if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) { - foreach my $ipaddr (@$ipset) { - my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr}); - next if !$ip || ($version && $version != 4); - push(@$arpfilter, $ip); - } - } + my $arpfilter = resolve_arpfilter( + $ipsets->{"ipfilter-$netid"}, + $cluster_conf, $vmfw_conf, $vmid, $netid, + ); generate_tap_layer2filter( $ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter, ); @@ -4881,14 +4907,10 @@ sub compile_ebtables_filter { my $iface = "veth${vmid}i$1"; next if !valid_interface_name($iface); my $macaddr = $net->{hwaddr}; - my $arpfilter = []; - if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) { - foreach my $ipaddr (@$ipset) { - my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr}); - next if !$ip || ($version && $version != 4); - push(@$arpfilter, $ip); - } - } + my $arpfilter = resolve_arpfilter( + $ipsets->{"ipfilter-$netid"}, + $cluster_conf, $vmfw_conf, $vmid, $netid, + ); if (defined(my $ip = $net->{ip}) && $vmfw_conf->{options}->{ipfilter}) { # ebtables changes this to a .0/MASK network but we just # want the address here, no network - see #2193 diff --git a/src/test/Makefile b/src/test/Makefile index 272a19a..74f9e35 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -1,4 +1,4 @@ -TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_simulator_protocols test_interface_names test_restore_errors +TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_ebtables_ipfilter test_simulator_protocols test_interface_names test_restore_errors .PHONY: all test $(TESTS) clean all: test diff --git a/src/test/test_ebtables_ipfilter.pl b/src/test/test_ebtables_ipfilter.pl new file mode 100755 index 0000000..9716ac2 --- /dev/null +++ b/src/test/test_ebtables_ipfilter.pl @@ -0,0 +1,85 @@ +#!/usr/bin/env perl + +use strict; +use warnings; + +use lib qw(..); + +use Test::More; + +use PVE::Firewall; + +plan(skip_all => 'PVE::QemuServer::Network is not installed') + if !eval { require PVE::QemuServer::Network; 1 }; + +PVE::Firewall::set_verbose(1); + +sub compile_arp_sources { + my ($entries) = @_; + + my $cluster = { + options => {}, + aliases => { dcalias => { name => 'dcalias', cidr => '192.0.2.2', ipversion => 4 } }, + }; + my $vmfw_conf = { + vmid => 100, + options => { enable => 1 }, + aliases => + { guestalias => { name => 'guestalias', cidr => '192.0.2.1', ipversion => 4 } }, + ipset => { 'ipfilter-net0' => $entries }, + }; + my $vmdata = { + qemu => { 100 => { net0 => 'virtio=02:00:00:00:00:01,bridge=vmbr0,firewall=1' } }, + lxc => {}, + }; + + my @warnings; + local $SIG{__WARN__} = sub { push @warnings, @_ }; + + my $ruleset = + PVE::Firewall::compile_ebtables_filter($cluster, {}, { 100 => $vmfw_conf }, $vmdata); + + ok(exists($ruleset->{'tap100i0-OUT'}), 'guest keeps its layer-2 filter'); + my @sources = map { /--arp-ip-src (\S+)/ ? $1 : () } ($ruleset->{'tap100i0-OUT-ARP'} // [])->@*; + + return (\@sources, \@warnings); +} + +subtest 'aliases are resolved' => sub { + my ($sources, $warnings) = compile_arp_sources([ + { cidr => '192.0.2.10' }, + { cidr => 'guestalias' }, + { cidr => 'guest/guestalias' }, + { cidr => 'dc/dcalias' }, + { cidr => 'dcalias' }, + ]); + is_deeply( + $sources, + ['192.0.2.10', '192.0.2.1', '192.0.2.1', '192.0.2.2', '192.0.2.2'], + 'ARP filter contains resolved addresses', + ); + is_deeply($warnings, [], 'no warnings'); +}; + +subtest 'entries flagged by the parser are skipped' => sub { + my ($sources, $warnings) = compile_arp_sources([ + { cidr => 'c', errors => { cidr => 'invalid' } }, { cidr => '192.0.2.10' }, + ]); + is_deeply($sources, ['192.0.2.10'], 'valid entry is kept'); + is_deeply($warnings, [], 'no warnings'); +}; + +subtest 'unresolvable entries are skipped with context' => sub { + my ($sources, $warnings) = compile_arp_sources([ + { cidr => 'missing' }, { cidr => '192.0.2.10' }, + ]); + is_deeply($sources, ['192.0.2.10'], 'valid entry is kept'); + is(scalar($warnings->@*), 1, 'one warning'); + like( + $warnings->[0] // '', + qr/^100: ipfilter-net0: skipping 'missing': /, + 'warning names guest, ipset and entry', + ); +}; + +done_testing(); -- 2.47.3