[pve-devel] [PATCH pve-firewall] ebtables: add arp filtering

[Alexandre DERUMIER]

> + my $arpchain = $tapchain."-ARP"; 
> + ruleset_addrule($ruleset, $tapchain, "-p ARP", "-j $arpchain"); 
> + ruleset_create_chain($ruleset, $arpchain); 
> + 
> + foreach my $ip (@{$arpfilter}) { 
> + ruleset_addrule($ruleset, $arpchain, "-p ARP --arp-ip-src $ip", '-j RETURN'); 

>>I wish this could just use the actual ipset :-\ 

we can't use ipset in ebtables :(


> if (defined($options->{layer2_protocols})){ 

>>This hunk below seems unrelated? Should probably be a separate patch, or 
>>is this related somehow? 

We need it, because we need an accept at the end of main tap chain.

(for arp, we need a jump to arpchain, return if ip exist or drop),

I'll make it in a separate patch for more visibility. (and I think it's possible to avoid the extra proto chain)



I'll rework the patch with all your comments, and send a V2 monday


----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Vendredi 8 Mars 2019 10:25:22
Objet: Re: [pve-devel] [PATCH pve-firewall] ebtables: add arp filtering

On Fri, Mar 08, 2019 at 09:14:08AM +0100, Alexandre Derumier wrote: 
> This implemented arp filtering if ipfilter is enable 
> https://bugzilla.proxmox.com/show_bug.cgi?id=2125 
> 
> They are another filters possible (ipv4,rarp), 
> i don't known if we need them. 
> 
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com> 
> --- 
> src/PVE/Firewall.pm | 42 +++++++++++++++++++++++++++++++++--------- 
> 1 file changed, 33 insertions(+), 9 deletions(-) 
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm 
> index 2125d3b..f8ed345 100644 
> --- a/src/PVE/Firewall.pm 
> +++ b/src/PVE/Firewall.pm 
> @@ -3694,9 +3694,16 @@ sub compile_ebtables_filter { 
> next if !$net->{firewall}; 
> my $iface = "tap${vmid}i$1"; 
> my $macaddr = $net->{macaddr}; 
> - 
> - generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid); 
> - 
> + my $arpfilter = []; 
> + my $ipsets = $vmfw_conf->{ipset}; 
> + if (defined($ipsets->{"ipfilter-$netid"})){ 
> + foreach my $ipaddr (@{$ipsets->{"ipfilter-$netid"} }) { 

Please try to avoid duplicating hash accesses. Eg. use: 

if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})){ 
foreach my $ipaddr (@$ipset) { 

> + my($ip,$version) = parse_ip_or_cidr($ipaddr->{cidr}); 

Missing spaces (`my ($ip, $version) = `) 

> + next if !$ip || ($version && $version != 4); 
> + push(@$arpfilter, $ip); 
> + } 
> + } 
> + generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter); 
> } 
> }; 
> warn $@ if $@; # just to be sure - should not happen 
> @@ -3716,7 +3723,9 @@ sub compile_ebtables_filter { 
> next if !$net->{firewall}; 
> my $iface = "veth${vmid}i$1"; 
> my $macaddr = $net->{hwaddr}; 
> - generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid); 
> + my $arpfilter = []; 
> + push(@$arpfilter, $net->{ip}) if $net->{ip}; 

We do have ipfilter-net* ipsets for containers as well, so this would 
need to be filled, too. 

> + generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter); 
> } 
> }; 
> warn $@ if $@; # just to be sure - should not happen 
> @@ -3726,7 +3735,7 @@ sub compile_ebtables_filter { 
> } 
> 
> sub generate_tap_layer2filter { 
> - my ($ruleset, $iface, $macaddr, $vmfw_conf, $vmid) = @_; 
> + my ($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter) = @_; 
> my $options = $vmfw_conf->{options}; 
> 
> my $tapchain = $iface."-OUT"; 
> @@ -3741,15 +3750,30 @@ sub generate_tap_layer2filter { 
> ruleset_addrule($ruleset, $tapchain, "-s ! $macaddr", '-j DROP'); 
> } 
> 
> + if (defined($arpfilter)){ 

This is always defined because it starts out as an array ref (empty but 
defined). You need to use `if (@$arpfilter) {` here. 
(As with this currently it'll always jump to an empty -ARP chain 
containing only the final `-j DROP` if no ipfilter-netX exists.) 

> + my $arpchain = $tapchain."-ARP"; 
> + ruleset_addrule($ruleset, $tapchain, "-p ARP", "-j $arpchain"); 
> + ruleset_create_chain($ruleset, $arpchain); 
> + 
> + foreach my $ip (@{$arpfilter}) { 
> + ruleset_addrule($ruleset, $arpchain, "-p ARP --arp-ip-src $ip", '-j RETURN'); 

I wish this could just use the actual ipset :-\ 

> + } 
> + ruleset_addrule($ruleset, $arpchain, '', '-j DROP'); 
> + } 
> + 
> if (defined($options->{layer2_protocols})){ 

This hunk below seems unrelated? Should probably be a separate patch, or 
is this related somehow? 

> + my $protochain = $tapchain."-PROTO"; 
> + ruleset_addrule($ruleset, $tapchain, '', "-j $protochain"); 
> + ruleset_create_chain($ruleset, $protochain); 
> + 
> foreach my $proto (split(/,/, $options->{layer2_protocols})) { 
> - ruleset_addrule($ruleset, $tapchain, "-p $proto", '-j ACCEPT'); 
> + ruleset_addrule($ruleset, $protochain, "-p $proto", '-j RETURN'); 
> } 
> - ruleset_addrule($ruleset, $tapchain, '', "-j DROP"); 
> - } else { 
> - ruleset_addrule($ruleset, $tapchain, '', '-j ACCEPT'); 
> + ruleset_addrule($ruleset, $protochain, '', '-j DROP'); 
> } 
> 
> + ruleset_addrule($ruleset, $tapchain, '', '-j ACCEPT'); 
> + 
> ruleset_addrule($ruleset, 'PVEFW-FWBR-OUT', "-i $iface", "-j $tapchain"); 
> } 
> 
> -- 
> 2.11.0