[PATCH pve-firewall 1/2] firewall: ebtables: resolve aliases in ipfilter ipsets

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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 <a.bied-charreton@proxmox.com>
---
 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