[pve-devel] applied: [PATCH v2 firewall] Fix #1606 Add nf_conntrack_allow_invalid option

[Thomas Lamprecht]

Am 2/1/19 um 10:46 AM schrieb Christian Ebner:> This adds the nf_conntrack_allow_invalid host firewall option to allow to disable
> the dropping of invalid packets from the connection tracker point of view.
> This is needed for some rare setups with asymmetrical multi-path routing.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> 
> Version 2:
>     * ruleset_chain_add_conn_filters now takes just the allow_invalid option
>       as parameter instead of the whole hash, as suggested.
>     * The conntrack invalid packets are now only allowed for the PVEFW_FORWARD
>       chain, in case the option is set in the config

applied, thanks! Two small code style nits inline, for future patches :-)

> 
>  src/PVE/Firewall.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 39f79d4..734aeb4 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -1242,6 +1242,12 @@ our $host_option_properties = {
>  	type => 'boolean',
>  	optional => 1,
>      },
> +    nf_conntrack_allow_invalid => {
> +	description => "Allow invalid packets on connection tracking.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
>  };
>  
>  our $vm_option_properties = {
> @@ -2128,9 +2134,11 @@ sub ruleset_chain_add_ndp {
>  }
>  
>  sub ruleset_chain_add_conn_filters {
> -    my ($ruleset, $chain, $accept) = @_;
> +    my ($ruleset, $chain, $allow_invalid, $accept) = @_;

If there's no other similar parameter in a method's signature it may make sense
to add new ones at the end. This would allow to omit two changes below, as a
not passed parameter equals 'undefined', which is the default for
$allow_invalid (i.e., do not allow invalid here).  I mean, independent of the
new parameters position, sometimes you want to explicitly set the default
nonetheless, maybe for clarity to show that you did not just forget the other
call places, so no strong feelings about that one.

>  
> -    ruleset_addrule($ruleset, $chain, "-m conntrack --ctstate INVALID", "-j DROP");
> +    if (!$allow_invalid) {
> +	ruleset_addrule($ruleset, $chain, "-m conntrack --ctstate INVALID", "-j DROP");
> +    }
>      ruleset_addrule($ruleset, $chain, "-m conntrack --ctstate RELATED,ESTABLISHED", "-j $accept");
>  }
>  
> @@ -2356,7 +2364,7 @@ sub enable_host_firewall {
>  
>      ruleset_addrule($ruleset, $chain, "-i lo", "-j ACCEPT");
>  
> -    ruleset_chain_add_conn_filters($ruleset, $chain, 'ACCEPT');
> +    ruleset_chain_add_conn_filters($ruleset, $chain, 0, 'ACCEPT');

this could be saved if $allow_invalid was put at the method signature's end,
see above.

>      ruleset_chain_add_ndp($ruleset, $chain, $ipversion, $options, 'IN', '-j RETURN');
>      ruleset_chain_add_input_filters($ruleset, $chain, $ipversion, $options, $cluster_conf, $loglevel);
>  
> @@ -2414,7 +2422,7 @@ sub enable_host_firewall {
>  
>      ruleset_addrule($ruleset, $chain, "-o lo", "-j ACCEPT");
>  
> -    ruleset_chain_add_conn_filters($ruleset, $chain, 'ACCEPT');
> +    ruleset_chain_add_conn_filters($ruleset, $chain, 0, 'ACCEPT');
this too.

>  
>      # we use RETURN because we may want to check other thigs later
>      $accept_action = 'RETURN';
> @@ -2638,7 +2646,7 @@ sub parse_hostfw_option {
>  
>      my $loglevels = "emerg|alert|crit|err|warning|notice|info|debug|nolog";
>  
> -    if ($line =~ m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack):\s*(0|1)\s*$/i) {
> +    if ($line =~ m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid):\s*(0|1)\s*$/i) {
>  	$opt = lc($1);
>  	$value = int($2);
>      } elsif ($line =~ m/^(log_level_in|log_level_out|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i) {
> @@ -3461,7 +3469,7 @@ sub compile_iptables_filter {
>      # fixme: what log level should we use here?
>      my $loglevel = get_option_log_level($hostfw_options, "log_level_out");
>  
> -    ruleset_chain_add_conn_filters($ruleset, "PVEFW-FORWARD", "ACCEPT");
> +    ruleset_chain_add_conn_filters($ruleset, "PVEFW-FORWARD", $hostfw_options->{nf_conntrack_allow_invalid}, "ACCEPT");

IMO, It would be a bit more readable if we'd avoid the hash access on calling
but do something like:

my $conn_allow_invalid = $hostfw_options->{nf_conntrack_allow_invalid};
ruleset_chain_add_conn_filters($ruleset, "PVEFW-FORWARD", "ACCEPT", $conn_allow_invalid);

I fixed this up in a follow up

>  
>      ruleset_create_chain($ruleset, "PVEFW-FWBR-IN");
>      ruleset_chain_add_input_filters($ruleset, "PVEFW-FWBR-IN", $ipversion, $hostfw_options, $cluster_conf, $loglevel);
>