[pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes

[Wolfgang Bumiller]

On Wed, Sep 27, 2017 at 12:02:33AM +0200, Tom Weber wrote:
> ---
>  src/PVE/Firewall.pm | 220 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 117 insertions(+), 103 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index f8a9300..179617a 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -142,6 +142,20 @@ my $log_level_hash = {
>      emerg => 0,
>  };
>  
> +# %rule
> +#

+1 for the documentation, but what happened to the right sides of most
of these arrows? ;-) (yeah some of these options are being abused a
bit...)

> +# name => optional
> +# action =>
> +# proto =>
> +# sport =>
> +# dport =>
> +# log => optional, loglevel

Why not call it loglevel then? Also, with this being new it should be
mentioned in the commit message.

> +# logmsg => optional, logmsg - overwrites default

And this.

> +# iface_in
> +# iface_out
> +# match => optional, overwrites generation of match
> +# target => optional, overwrites action
> +
>  # we need to overwrite some macros for ipv6
>  my $pve_ipv6fw_macros = {
>      'Ping' => [
> @@ -536,7 +550,7 @@ my $FWACCEPTMARK_OFF = "0x00000000/0x80000000";



> @@ -1948,19 +1970,24 @@ sub ruleset_generate_rule {
>  
>      # update all or nothing
>  
> +    # fixme: lots of temporary ugliness

+1 for the 'temporary' in there ;-)

>      my @mstrs = ();
>      my @astrs = ();
> +    my @logging = ();
> +    my @logmsg = ();
>      foreach my $tmp (@$rules) {
>  	my $m = ruleset_generate_match($ruleset, $chain, $ipversion, $tmp, $actions, $goto, $cluster_conf, $fw_conf);
>  	my $a = ruleset_generate_action($ruleset, $chain, $ipversion, $tmp, $actions, $goto, $cluster_conf, $fw_conf);
>  	if (defined $m or defined $a) {
>  	    push @mstrs, defined($m) ? $m : "";
>  	    push @astrs, defined($a) ? $a : "";
> +	    push @logging, $tmp->{log};
> +	    push @logmsg, $tmp->{logmsg};
>  	}
>      }
>  
>      for my $i (0 .. $#mstrs) {
> -	ruleset_addrule($ruleset, $chain, $mstrs[$i], $astrs[$i]);
> +	ruleset_addrule($ruleset, $chain, $mstrs[$i], $astrs[$i], $logging[$i], $logmsg[$i]);
>      }
>  }
>  
> @@ -1993,14 +2020,6 @@ sub ruleset_chain_exist {
>      return $ruleset->{$chain} ? 1 : undef;
>  }
>  
> -sub ruleset_addrule_old {
> -   my ($ruleset, $chain, $rule) = @_;
> -
> -   die "no such chain '$chain'\n" if !$ruleset->{$chain};
> -
> -   push @{$ruleset->{$chain}}, "-A $chain $rule";
> -}
> -
>  sub ruleset_addrule {
>     my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = @_;
>  
> @@ -3127,26 +3146,21 @@ sub generate_std_chains {
>      my $std_chains = $pve_std_chains->{$ipversion} || die "internal error";
>  
>      my $loglevel = get_option_log_level($options, 'smurf_log_level');
> -
> -    my $chain;
> -
> -    if ($ipversion == 4) {
> -	# same as shorewall smurflog.
> -	$chain = 'PVEFW-smurflog';
> -	$std_chains->{$chain} = [];
> -	
> -	push @{$std_chains->{$chain}}, get_log_rule_base($chain, 0, "DROP: ", $loglevel) if $loglevel;
> -	push @{$std_chains->{$chain}}, "-j DROP";
> +    my $chain = 'PVEFW-smurflog';
> +    if ( $loglevel && $std_chains->{$chain} ) {

This shouldn't check for $loglevel as it can also have changed from
non-zero to zero in which case the change would happen. Iow. you cannot
turn off the smurf logs anymore.

> +	foreach my $r (@{$std_chains->{$chain}}) {
> +	  $r->{log} = $loglevel;

Wouldn't it be better to just give ruleset_generate_rule an optional
default loglevel parameter for when rules don't define their own {log}?

> +	}
>      }
>  
>      # same as shorewall logflags action.
>      $loglevel = get_option_log_level($options, 'tcp_flags_log_level');
>      $chain = 'PVEFW-logflags';
> -    $std_chains->{$chain} = [];
> -
> -    # fixme: is this correctly logged by pvewf-logger? (ther is no --log-ip-options for NFLOG)
> -    push @{$std_chains->{$chain}}, get_log_rule_base($chain, 0, "DROP: ", $loglevel) if $loglevel;
> -    push @{$std_chains->{$chain}}, "-j DROP";
> +    if ( $loglevel && $std_chains->{$chain} ) {
> +	foreach my $r (@{$std_chains->{$chain}}) {
> +	  $r->{log} = $loglevel;
> +	}
> +    }
>  
>      foreach my $chain (keys %$std_chains) {
>  	ruleset_create_chain($ruleset, $chain);
> @@ -3154,7 +3168,7 @@ sub generate_std_chains {
>  	    if (ref($rule)) {
>  		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule);
>  	    } else {
> -		ruleset_addrule_old($ruleset, $chain, $rule);
> +		die "rule $rule as string - should not happen";
>  	    }
>  	}
>      }
> -- 
> 2.7.4