[pve-devel] [PATCH] prepare code for more generic firewall logging

[Tom Weber]

Am Montag, den 18.09.2017, 12:21 +0200 schrieb Wolfgang Bumiller:
> Improving logging makes sense, the current state might be confuse for
> some (given that drop-rules simply generate a `-j DROP` iptables
> rules
> and therefore don't get logged).
> This seems to be a good first step, although I'd be much happier if
> iptables would allow setting the log-prefix and performing the log
> action separately, then we could simply introduce a log-drop chain
> instead.

well, once we have one single place that actually creates the rules, it
should be way easier to implement there.
My intention is to get away from hardwired "rulegeneration" which we
have in the code on several places to separate ruledefinitions and code
that only generates the rules from that definitions.

> I'm assuming your intention is to be able to duplicate the matching
> part
> of a rule so that you can first add it with `-j NFLOG` and afterwards
> its `-j DROP` action (or whatever action was requested). In this
> case,
> also note that with groups the actions may not be executed
> immediately
> and instead set a mark and return out of the current chain.

yes thats the idea.
And I'm aware of the marks.

> With that in mind, I have no objections to this patch (or a version
> of
> it, see the inline comments below).
> 
> But first things first: please read https://pve.proxmox.com/wiki/Deve
> loper_Documentation
> for details about patches and CLA (which is required for us to apply
> external patches).

Martin already has the signed CLA.

> Also, the spaces in your patch have been replaced by non-breaking-
> space
> characters, causing git-am to fail on it. You should check your
> mailer
> settings to avoid this.

if you don't mind i'll send the next version to you directly just to
let you verify the cosmetic things before cluttering the list again :) 

> On Thu, Sep 14, 2017 at 07:08:54PM +0200, Tom Weber wrote:
> > 
> > making ruleset generation aware of a match and action
> > part in iptable rules.
> > code will generate the same iptables as before! (except for
> > a few additional spaces between match and action).
> Note that these spaces are currently not accepted by the testcases
> and
> requires:
> -    $rule =~ s/^-A $chain // || die "got strange rule: $rule";
> +    $rule =~ s/^-A $chain +// || die "got strange rule: $rule";
> in FirewallSimulator.pm's rule_match()
> 
> Please use `make check` in the future to check your changes ;-)

To be honest, I didn't catch what the use of FirewallSimumlator
actually was/is :-)
It's not that there's much comments or dokumentation at all.

> > ---
> >  src/PVE/Firewall.pm | 168 +++++++++++++++++++++++++++++++---------
> > ------------
> >  1 file changed, 99 insertions(+), 69 deletions(-)
> > 
> > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> > index cc81325..61f07e0 100644
> > --- a/src/PVE/Firewall.pm
> > +++ b/src/PVE/Firewall.pm
> > @@ -1648,8 +1648,6 @@ sub enable_bridge_firewall {
> >      $bridge_firewall_enabled = 1;
> >  }
> >  
> > -my $rule_format = "%-15s %-30s %-30s %-15s %-15s %-15s\n";
> > -
> Removes an unrelated unused variable. Such cleanups are preferred as
> separate patches.

ok. 
 
> > -    my @cmds = ();
> > +    my @mstrs = ();
> > +    my @astrs = ();
> >      foreach my $tmp (@$rules) {
> > -	if (my $cmdstr = ruleset_generate_cmdstr($ruleset, $chain,
> > $ipversion, $tmp, $actions, $goto, $cluster_conf, $fw_conf)) {
> > -	    push @cmds, $cmdstr;
> > +	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 : "";
> While this is all part of a small chunk of code, I'd prefer a single
> array containing pairs of [$match, $action] as elements, rather than
> worrying about future changes possibly bringing @mstrs and @astrs out
> of
> sync.

agreed. I consider this ugly too, but this is code that will probably
be replaced in further steps.
The intention of this patch was to only separate match and action and
show where i'm heading to with as few changes as possible to the actual
logic.

> > -sub ruleset_addrule {
> > +sub ruleset_addrule_old {
> The name suggests that you plan on removing this later on. If this is

yes. Only one place in the code where I couldn't easily split action
from match, hence I had to keep it. Will also be eliminated in a later
step.


thanks for your feedback!
  Tom