Re: [pve-devel] [RFC firewall] fix: #2123 Logging of user defined firewall rules

2019-03-18 Thread Thomas Lamprecht
On 3/14/19 2:07 PM, Christian Ebner wrote:
> As I understand it this matches the 5 packets from the burst and then only if 
> the burst refilled.
> See 
> https://thelowedown.wordpress.com/2008/07/03/iptables-how-to-use-the-limits-module/
> 
> We could let the user decide the rate and burst limit, but that would 
> probably be a bit overkill and add unwanted complexity.
> Without rate limit there is the danger of spaming the logs...

is that a danger? You mean IO wise?
I mean, it can be just fine like you did it - we should at least document it
clearly so that no wrong expectations or new confusion is created.
If people start requesting it we can always still add it I'd go on a per-rule
limit then, though, rather a host level or DC level option.

> 
> I doubt that you will get any hints about unlogged packages as they simply 
> won't match the rule for logging, although I still need to test this.
> 
>> On March 14, 2019 at 1:31 PM Thomas Lamprecht  
>> wrote:
>>
>>
>> On 3/14/19 1:06 PM, Christian Ebner wrote:
>>> This allows a user to log traffic filtered by a self defined firewall rule.
>>> Therefore the API is extended to include a 'log' option allow to specify the
>>> log level for each rule individually.
>>>
>>> The 'log' option can also be specified in the fw config. In order to reduce 
>>> the
>>> log amount, logging is limited to 1 entry per second.
>>
>> quick glance over the code looks not bad, but I'm not sure of the hard-coded 
>> limit,
>> maybe not even about the limit itself...
>>
>> Also the docs state:
>>> This module matches at a limited rate using a token bucket filter. A rule 
>>> using
>>> this extension will match until this limit is reached. It can be used in
>>> combination with the LOG target to give limited logging, for example.
>>> --limit rate[/second|/minute|/hour|/day]
>>>   Maximum average matching rate: specified as a number, with an optional 
>>> `/second',
>>>   `/minute', `/hour', or `/day' suffix; the default is 3/hour. 
>>> --limit-burst
>>> ...
>> -- http://ipset.netfilter.org/iptables-extensions.man.html
>>
>>
>> So how does this logs in reality, does it logs a full second, or does it 
>> always
>> logs for the burst (default 5) and then only if the burst refilled?
>> Does one sees hints about "not logged packages", e.g., like systemd does, 
>> e.g.,
>> IIRC something like "suppressed XYZ count of log messages from service foo" 
>> is
>> done there.
>>
>> Just not that we come again in a situation where the user thinks _all_ is 
>> logged,
>> but infact all is then _not_ logged.
>>
>>>
>>> For now the rule has to be created or edited via the pvesh API call or via 
>>> the
>>> firewall config in order to set the log level.
>>>
>>> Signed-off-by: Christian Ebner 
>>> ---
>>>
>>> This is a tentative patch in order to allow fine grained logging of packets
>>> dropped by user defined rules.
>>> Feedback is very much appreciated.
>>>
>>>  src/PVE/API2/Firewall/Rules.pm |  3 +++
>>>  src/PVE/Firewall.pm| 59 
>>> +-
>>>  2 files changed, 38 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
>>> index 1670986..f0bc562 100644
>>> --- a/src/PVE/API2/Firewall/Rules.pm
>>> +++ b/src/PVE/API2/Firewall/Rules.pm
>>> @@ -141,6 +141,9 @@ sub register_get_rule {
>>> type => 'integer',
>>> optional => 1,
>>> },
>>> +   log => PVE::Firewall::get_standard_option('pve-fw-loglevel', {
>>> +   description => 'Log level for firewall rule',
>>> +   }),
>>> iface => {
>>> type => 'string',
>>> optional => 1,
>>> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
>>> index 6ac3038..8bb7bb9 100644
>>> --- a/src/PVE/Firewall.pm
>>> +++ b/src/PVE/Firewall.pm
>>> @@ -1363,6 +1363,9 @@ my $rule_properties = {
>>> minimum => 0,
>>> optional => 1,
>>>  },
>>> +log => get_standard_option('pve-fw-loglevel', {
>>> +   description => "Log level for firewall rule.",
>>> +}),
>>>  sport => {
>>> description => "Restrict TCP/UDP source port. $port_descr",
>>> type => 'string', format => 'pve-fw-sport-spec',
>>> @@ -2008,8 +2011,10 @@ sub ipt_rule_to_cmds {
>>>  }
>>>  
>>>  my @iptcmds;
>>> -if (defined $rule->{log} && $rule->{log}) {
>>> -   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
>>> $rule->{log});
>>> +if ($rule->{log} && $rule->{log} ne 'nolog') {
>>> +   my $log = $rule->{log};
>>> +   my $loglevel = $log_level_hash->{$log};
>>> +   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
>>> $loglevel);
>>> push @iptcmds, "-A $chain $matchstr $logaction";
>>>  }
>>>  push @iptcmds, "-A $chain $matchstr $targetstr";
>>> @@ -2017,7 +2022,7 @@ sub ipt_rule_to_cmds {
>>>  }
>>>  
>>>  sub ruleset_generate_rule {
>>> -my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, 

Re: [pve-devel] [RFC firewall] fix: #2123 Logging of user defined firewall rules

2019-03-14 Thread Christian Ebner
As I understand it this matches the 5 packets from the burst and then only if 
the burst refilled.
See 
https://thelowedown.wordpress.com/2008/07/03/iptables-how-to-use-the-limits-module/

We could let the user decide the rate and burst limit, but that would probably 
be a bit overkill and add unwanted complexity.
Without rate limit there is the danger of spaming the logs...

I doubt that you will get any hints about unlogged packages as they simply 
won't match the rule for logging, although I still need to test this.

> On March 14, 2019 at 1:31 PM Thomas Lamprecht  wrote:
> 
> 
> On 3/14/19 1:06 PM, Christian Ebner wrote:
> > This allows a user to log traffic filtered by a self defined firewall rule.
> > Therefore the API is extended to include a 'log' option allow to specify the
> > log level for each rule individually.
> > 
> > The 'log' option can also be specified in the fw config. In order to reduce 
> > the
> > log amount, logging is limited to 1 entry per second.
> 
> quick glance over the code looks not bad, but I'm not sure of the hard-coded 
> limit,
> maybe not even about the limit itself...
> 
> Also the docs state:
> > This module matches at a limited rate using a token bucket filter. A rule 
> > using
> > this extension will match until this limit is reached. It can be used in
> > combination with the LOG target to give limited logging, for example.
> > --limit rate[/second|/minute|/hour|/day]
> >   Maximum average matching rate: specified as a number, with an optional 
> > `/second',
> >   `/minute', `/hour', or `/day' suffix; the default is 3/hour. 
> > --limit-burst
> > ...
> -- http://ipset.netfilter.org/iptables-extensions.man.html
> 
> 
> So how does this logs in reality, does it logs a full second, or does it 
> always
> logs for the burst (default 5) and then only if the burst refilled?
> Does one sees hints about "not logged packages", e.g., like systemd does, 
> e.g.,
> IIRC something like "suppressed XYZ count of log messages from service foo" is
> done there.
> 
> Just not that we come again in a situation where the user thinks _all_ is 
> logged,
> but infact all is then _not_ logged.
> 
> > 
> > For now the rule has to be created or edited via the pvesh API call or via 
> > the
> > firewall config in order to set the log level.
> > 
> > Signed-off-by: Christian Ebner 
> > ---
> > 
> > This is a tentative patch in order to allow fine grained logging of packets
> > dropped by user defined rules.
> > Feedback is very much appreciated.
> > 
> >  src/PVE/API2/Firewall/Rules.pm |  3 +++
> >  src/PVE/Firewall.pm| 59 
> > +-
> >  2 files changed, 38 insertions(+), 24 deletions(-)
> > 
> > diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
> > index 1670986..f0bc562 100644
> > --- a/src/PVE/API2/Firewall/Rules.pm
> > +++ b/src/PVE/API2/Firewall/Rules.pm
> > @@ -141,6 +141,9 @@ sub register_get_rule {
> > type => 'integer',
> > optional => 1,
> > },
> > +   log => PVE::Firewall::get_standard_option('pve-fw-loglevel', {
> > +   description => 'Log level for firewall rule',
> > +   }),
> > iface => {
> > type => 'string',
> > optional => 1,
> > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> > index 6ac3038..8bb7bb9 100644
> > --- a/src/PVE/Firewall.pm
> > +++ b/src/PVE/Firewall.pm
> > @@ -1363,6 +1363,9 @@ my $rule_properties = {
> > minimum => 0,
> > optional => 1,
> >  },
> > +log => get_standard_option('pve-fw-loglevel', {
> > +   description => "Log level for firewall rule.",
> > +}),
> >  sport => {
> > description => "Restrict TCP/UDP source port. $port_descr",
> > type => 'string', format => 'pve-fw-sport-spec',
> > @@ -2008,8 +2011,10 @@ sub ipt_rule_to_cmds {
> >  }
> >  
> >  my @iptcmds;
> > -if (defined $rule->{log} && $rule->{log}) {
> > -   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
> > $rule->{log});
> > +if ($rule->{log} && $rule->{log} ne 'nolog') {
> > +   my $log = $rule->{log};
> > +   my $loglevel = $log_level_hash->{$log};
> > +   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
> > $loglevel);
> > push @iptcmds, "-A $chain $matchstr $logaction";
> >  }
> >  push @iptcmds, "-A $chain $matchstr $targetstr";
> > @@ -2017,7 +2022,7 @@ sub ipt_rule_to_cmds {
> >  }
> >  
> >  sub ruleset_generate_rule {
> > -my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf) = @_;
> > +my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf, 
> > $vmid) = @_;
> >  
> >  my $rules;
> >  
> > @@ -2030,7 +2035,7 @@ sub ruleset_generate_rule {
> >  # update all or nothing
> >  my @ipt_rule_cmds;
> >  foreach my $r (@$rules) {
> > -   push @ipt_rule_cmds, ipt_rule_to_cmds($r, $chain, $ipversion, 
> > $cluster_conf, $fw_conf);
> > +   push 

Re: [pve-devel] [RFC firewall] fix: #2123 Logging of user defined firewall rules

2019-03-14 Thread Thomas Lamprecht
On 3/14/19 1:06 PM, Christian Ebner wrote:
> This allows a user to log traffic filtered by a self defined firewall rule.
> Therefore the API is extended to include a 'log' option allow to specify the
> log level for each rule individually.
> 
> The 'log' option can also be specified in the fw config. In order to reduce 
> the
> log amount, logging is limited to 1 entry per second.

quick glance over the code looks not bad, but I'm not sure of the hard-coded 
limit,
maybe not even about the limit itself...

Also the docs state:
> This module matches at a limited rate using a token bucket filter. A rule 
> using
> this extension will match until this limit is reached. It can be used in
> combination with the LOG target to give limited logging, for example.
> --limit rate[/second|/minute|/hour|/day]
>   Maximum average matching rate: specified as a number, with an optional 
> `/second',
>   `/minute', `/hour', or `/day' suffix; the default is 3/hour. 
> --limit-burst
> ...
-- http://ipset.netfilter.org/iptables-extensions.man.html


So how does this logs in reality, does it logs a full second, or does it always
logs for the burst (default 5) and then only if the burst refilled?
Does one sees hints about "not logged packages", e.g., like systemd does, e.g.,
IIRC something like "suppressed XYZ count of log messages from service foo" is
done there.

Just not that we come again in a situation where the user thinks _all_ is 
logged,
but infact all is then _not_ logged.

> 
> For now the rule has to be created or edited via the pvesh API call or via the
> firewall config in order to set the log level.
> 
> Signed-off-by: Christian Ebner 
> ---
> 
> This is a tentative patch in order to allow fine grained logging of packets
> dropped by user defined rules.
> Feedback is very much appreciated.
> 
>  src/PVE/API2/Firewall/Rules.pm |  3 +++
>  src/PVE/Firewall.pm| 59 
> +-
>  2 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
> index 1670986..f0bc562 100644
> --- a/src/PVE/API2/Firewall/Rules.pm
> +++ b/src/PVE/API2/Firewall/Rules.pm
> @@ -141,6 +141,9 @@ sub register_get_rule {
>   type => 'integer',
>   optional => 1,
>   },
> + log => PVE::Firewall::get_standard_option('pve-fw-loglevel', {
> + description => 'Log level for firewall rule',
> + }),
>   iface => {
>   type => 'string',
>   optional => 1,
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 6ac3038..8bb7bb9 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -1363,6 +1363,9 @@ my $rule_properties = {
>   minimum => 0,
>   optional => 1,
>  },
> +log => get_standard_option('pve-fw-loglevel', {
> + description => "Log level for firewall rule.",
> +}),
>  sport => {
>   description => "Restrict TCP/UDP source port. $port_descr",
>   type => 'string', format => 'pve-fw-sport-spec',
> @@ -2008,8 +2011,10 @@ sub ipt_rule_to_cmds {
>  }
>  
>  my @iptcmds;
> -if (defined $rule->{log} && $rule->{log}) {
> - my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
> $rule->{log});
> +if ($rule->{log} && $rule->{log} ne 'nolog') {
> + my $log = $rule->{log};
> + my $loglevel = $log_level_hash->{$log};
> + my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
> $loglevel);
>   push @iptcmds, "-A $chain $matchstr $logaction";
>  }
>  push @iptcmds, "-A $chain $matchstr $targetstr";
> @@ -2017,7 +2022,7 @@ sub ipt_rule_to_cmds {
>  }
>  
>  sub ruleset_generate_rule {
> -my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf) = @_;
> +my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf, $vmid) 
> = @_;
>  
>  my $rules;
>  
> @@ -2030,7 +2035,7 @@ sub ruleset_generate_rule {
>  # update all or nothing
>  my @ipt_rule_cmds;
>  foreach my $r (@$rules) {
> - push @ipt_rule_cmds, ipt_rule_to_cmds($r, $chain, $ipversion, 
> $cluster_conf, $fw_conf);
> + push @ipt_rule_cmds, ipt_rule_to_cmds($r, $chain, $ipversion, 
> $cluster_conf, $fw_conf, $vmid);
>  }
>  foreach my $c (@ipt_rule_cmds) {
>   ruleset_add_ipt_cmd($ruleset, $chain, $c);
> @@ -2064,17 +2069,18 @@ sub ruleset_add_ipt_cmd {
>  }
>  
>  sub ruleset_addrule {
> -   my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = @_;
> -
> -   die "no such chain '$chain'\n" if !$ruleset->{$chain};
> +my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = @_;
>  
> -   if (defined($log) && $log) {
> - my $logaction = get_log_rule_base($chain, $vmid, $logmsg, $log);
> +die "no such chain '$chain'\n" if !$ruleset->{$chain};
> + 
> +if ($log) {
> + my $loglevel = $log_level_hash->{$log};
> + my $logaction = get_log_rule_base($chain, 

[pve-devel] [RFC firewall] fix: #2123 Logging of user defined firewall rules

2019-03-14 Thread Christian Ebner
This allows a user to log traffic filtered by a self defined firewall rule.
Therefore the API is extended to include a 'log' option allow to specify the
log level for each rule individually.

The 'log' option can also be specified in the fw config. In order to reduce the
log amount, logging is limited to 1 entry per second.

For now the rule has to be created or edited via the pvesh API call or via the
firewall config in order to set the log level.

Signed-off-by: Christian Ebner 
---

This is a tentative patch in order to allow fine grained logging of packets
dropped by user defined rules.
Feedback is very much appreciated.

 src/PVE/API2/Firewall/Rules.pm |  3 +++
 src/PVE/Firewall.pm| 59 +-
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm
index 1670986..f0bc562 100644
--- a/src/PVE/API2/Firewall/Rules.pm
+++ b/src/PVE/API2/Firewall/Rules.pm
@@ -141,6 +141,9 @@ sub register_get_rule {
type => 'integer',
optional => 1,
},
+   log => PVE::Firewall::get_standard_option('pve-fw-loglevel', {
+   description => 'Log level for firewall rule',
+   }),
iface => {
type => 'string',
optional => 1,
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 6ac3038..8bb7bb9 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1363,6 +1363,9 @@ my $rule_properties = {
minimum => 0,
optional => 1,
 },
+log => get_standard_option('pve-fw-loglevel', {
+   description => "Log level for firewall rule.",
+}),
 sport => {
description => "Restrict TCP/UDP source port. $port_descr",
type => 'string', format => 'pve-fw-sport-spec',
@@ -2008,8 +2011,10 @@ sub ipt_rule_to_cmds {
 }
 
 my @iptcmds;
-if (defined $rule->{log} && $rule->{log}) {
-   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
$rule->{log});
+if ($rule->{log} && $rule->{log} ne 'nolog') {
+   my $log = $rule->{log};
+   my $loglevel = $log_level_hash->{$log};
+   my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, 
$loglevel);
push @iptcmds, "-A $chain $matchstr $logaction";
 }
 push @iptcmds, "-A $chain $matchstr $targetstr";
@@ -2017,7 +2022,7 @@ sub ipt_rule_to_cmds {
 }
 
 sub ruleset_generate_rule {
-my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf) = @_;
+my ($ruleset, $chain, $ipversion, $rule, $cluster_conf, $fw_conf, $vmid) = 
@_;
 
 my $rules;
 
@@ -2030,7 +2035,7 @@ sub ruleset_generate_rule {
 # update all or nothing
 my @ipt_rule_cmds;
 foreach my $r (@$rules) {
-   push @ipt_rule_cmds, ipt_rule_to_cmds($r, $chain, $ipversion, 
$cluster_conf, $fw_conf);
+   push @ipt_rule_cmds, ipt_rule_to_cmds($r, $chain, $ipversion, 
$cluster_conf, $fw_conf, $vmid);
 }
 foreach my $c (@ipt_rule_cmds) {
ruleset_add_ipt_cmd($ruleset, $chain, $c);
@@ -2064,17 +2069,18 @@ sub ruleset_add_ipt_cmd {
 }
 
 sub ruleset_addrule {
-   my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = @_;
-
-   die "no such chain '$chain'\n" if !$ruleset->{$chain};
+my ($ruleset, $chain, $match, $action, $log, $logmsg, $vmid) = @_;
 
-   if (defined($log) && $log) {
-   my $logaction = get_log_rule_base($chain, $vmid, $logmsg, $log);
+die "no such chain '$chain'\n" if !$ruleset->{$chain};
+ 
+if ($log) {
+   my $loglevel = $log_level_hash->{$log};
+   my $logaction = get_log_rule_base($chain, $vmid, $logmsg, $loglevel);
push @{$ruleset->{$chain}}, "-A $chain $match $logaction";
-   }
-   # for stable ebtables digests avoid double-spaces to match ebtables-save 
output
-   $match .= ' ' if length($match);
-   push @{$ruleset->{$chain}}, "-A $chain ${match}$action";
+}
+# for stable ebtables digests avoid double-spaces to match ebtables-save 
output
+$match .= ' ' if length($match);
+push @{$ruleset->{$chain}}, "-A $chain ${match}$action";
 }
 
 sub ruleset_insertrule {
@@ -2094,7 +2100,7 @@ sub get_log_rule_base {
 # Note: we use special format for prefix to pass further
 # info to log daemon (VMID, LOGLEVEL and CHAIN)
 
-return "-j NFLOG --nflog-prefix \":$vmid:$loglevel:$chain: $msg\"";
+return "-m limit --limit 1/sec -j NFLOG --nflog-prefix 
\":$vmid:$loglevel:$chain: $msg\"";
 }
 
 sub ruleset_add_chain_policy {
@@ -2234,7 +2240,7 @@ sub ruleset_add_group_rule {
 }
 
 sub ruleset_generate_vm_rules {
-my ($ruleset, $rules, $cluster_conf, $vmfw_conf, $chain, $netid, 
$direction, $options, $ipversion) = @_;
+my ($ruleset, $rules, $cluster_conf, $vmfw_conf, $chain, $netid, 
$direction, $options, $ipversion, $vmid) = @_;
 
 my $lc_direction = lc($direction);
 
@@ -2251,12 +2257,13 @@ sub ruleset_generate_vm_rules {