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

[Tom Weber]

Am Mittwoch, den 27.09.2017, 11:51 +0200 schrieb Wolfgang Bumiller:
> On Wed, Sep 27, 2017 at 11:09:29AM +0200, Tom Weber wrote:
> > 
> > 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.
> That's long overdue anyway and might be useful later on when we want
> to
> start using nftables.
> 
> > 
> > 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.
> That's fine.
> 
> > 
> > > 
> > > > 
> > > > @@ -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.
> This modifies the fields in the global $pve_std_chains, so it doesn't
> need to not turn it on, it needs to turn if off. Such modifications
> are
> something I'd prefer to reduce if possible.

yes you're right - didn't look at it as running service.
so, the ultimate solution would be to have the std_chains template
defined in external config files (which i have in mind from the
beginning) from which we'll generate our working internal tree which
we'd then manipulate according to various settings?

> > > > +	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)
> Could be passed depending on $chain in the loop below, then we could
> avoid touching the global hashes entirely.
> The function would still prefer the rule's ->{log} member over the
> default if it exists.