Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
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.
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
Am Mittwoch, den 27.09.2017, 11:53 +0200 schrieb Wolfgang Bumiller:
> On Wed, Sep 27, 2017 at 12:02:33AM +0200, Tom Weber wrote:
> >
> > ---
> > +'PVEFW-smurflog' => [
> > + { action => 'DROP', logmsg => 'DROP: ' },
> > +],
> > +'PVEFW-logflags' => [
> > + { action => 'DROP', logmsg => 'DROP: ' },
> > ],
> > };
>
> Just noticed this is missing in the ipv6 part below it (my ip6tables
> aren't updating anymore).
yes, missed it. but only the PVEFW-logflags? smurflog has never been
part of ipv6.
Tom
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
On Wed, Sep 27, 2017 at 12:02:33AM +0200, Tom Weber wrote:
> ---
> +'PVEFW-smurflog' => [
> + { action => 'DROP', logmsg => 'DROP: ' },
> +],
> +'PVEFW-logflags' => [
> + { action => 'DROP', logmsg => 'DROP: ' },
> ],
> };
Just noticed this is missing in the ipv6 part below it (my ip6tables
aren't updating anymore).
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
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.
>
> > > + 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.
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
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 = "0x/0x8000";
>
>
> >
> > @@ -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}?
Re: [pve-devel] [PATCH v2 firewall 4/4] convert string based rule definitions to hashes
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 = "0x/0x8000";
> @@ -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
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
