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

[Tom Weber]

First of all, this all is work in progress. I thought I commit for
easier understanding of the way i'm heading - instead of one huge
commit which turns everything inside out. and for feedback of course.

My goal are defined structures for rules, chains, macros (which i think
are just arrays of "rule templates") etc and code which deals with
these structures instead of printing out iptables commands in various
places.
While doing this, keeping "input" and "output" identical to whats there
doesn't make this an easy task and therefore I sometimes use these
'temporary ugliness' approaches at places that I intend to replace
anyway.


Am Mittwoch, den 27.09.2017, 09:53 +0200 schrieb 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...)

still in the progress of defining this :-).. Right now it's more like a
scratchpad where I collect what I encounter.

> > +# 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.

this is for now and for backward compatibility. I'd prefer this as a
boolean, but I'm still carrying the loglevel around since it's used in
the frontend and the logformat for the pvefw-logger.

Do we really need the loglevel as it is or would a boolean be
sufficient? And how would we get rid of it? 

> > +# 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.

huh? if $loglevel == 0 it will not turn on logging.

> > +	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}?

how would this help with turning smurf / tcpflags logging separately on
and off then? (not that I like the current way of setting these - but
that's what's in the frontend right now)

  Tom