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

Christian Ebner c.ebner at proxmox.com
Thu Mar 14 14:07:44 CET 2019


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 <t.lamprecht at proxmox.com> 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 <c.ebner at proxmox.com>
> > ---
> > 
> > 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 {
> >  	} else {
> >  	    next if $rule->{type} ne $lc_direction;
> >  	    eval {
> > +		$rule->{logmsg} = "$rule->{action}: ";
> >  		if ($direction eq 'OUT') {
> >  		    rule_substitude_action($rule, { ACCEPT => "PVEFW-SET-ACCEPT-MARK", REJECT => "PVEFW-reject" });
> > -		    ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $vmfw_conf);
> > +		    ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $vmfw_conf, $vmid);
> >  		} else {
> >  		    rule_substitude_action($rule, { ACCEPT => $in_accept , REJECT => "PVEFW-reject" });
> > -		    ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $vmfw_conf);
> > +		    ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $vmfw_conf, $vmid);
> >  		}
> >  	    };
> >  	    warn $@ if $@;
> > @@ -2317,7 +2324,7 @@ sub generate_tap_rules_direction {
> >      ruleset_create_vm_chain($ruleset, $tapchain, $ipversion, $options, $macaddr, $ipfilter_ipset, $direction);
> >  
> >      if ($options->{enable}) {
> > -	ruleset_generate_vm_rules($ruleset, $rules, $cluster_conf, $vmfw_conf, $tapchain, $netid, $direction, $options, $ipversion);
> > +	ruleset_generate_vm_rules($ruleset, $rules, $cluster_conf, $vmfw_conf, $tapchain, $netid, $direction, $options, $ipversion, $vmid);
> >  
> >  	ruleset_generate_vm_ipsrules($ruleset, $options, $direction, $iface);
> >  
> > @@ -2385,7 +2392,7 @@ sub enable_host_firewall {
> >  		ruleset_add_group_rule($ruleset, $cluster_conf, $chain, $rule, 'IN', $accept_action, $ipversion);
> >  	    } elsif ($rule->{type} eq 'in') {
> >  		rule_substitude_action($rule, { ACCEPT => $accept_action, REJECT => "PVEFW-reject" });
> > -		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $hostfw_conf);
> > +		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $hostfw_conf, 0);
> >  	    }
> >  	};
> >  	warn $@ if $@;
> > @@ -2437,11 +2444,12 @@ sub enable_host_firewall {
> >  
> >  	$rule->{iface_out} = $rule->{iface} if $rule->{iface};
> >  	eval {
> > +	    $rule->{logmsg} = "$rule->{action}: ";
> >  	    if ($rule->{type} eq 'group') {
> >  		ruleset_add_group_rule($ruleset, $cluster_conf, $chain, $rule, 'OUT', $accept_action, $ipversion);
> >  	    } elsif ($rule->{type} eq 'out') {
> >  		rule_substitude_action($rule, { ACCEPT => $accept_action, REJECT => "PVEFW-reject" });
> > -		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $hostfw_conf);
> > +		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, $cluster_conf, $hostfw_conf, 0);
> >  	    }
> >  	};
> >  	warn $@ if $@;
> > @@ -2580,6 +2588,10 @@ sub parse_fw_rule {
> >  	    $rule->{dest} = $1;
> >  	    next;
> >  	}
> > +	if ($line =~ s/^-log (emerg|alert|crit|err|warning|notice|info|debug|nolog)\s*//) {
> > +	    $rule->{log} = $1;
> > +	    next;
> > +	}
> >  
> >  	last;
> >      }
> > @@ -3032,6 +3044,7 @@ my $format_rules = sub {
> >  		$raw .= " -p $rule->{proto}" if $rule->{proto};
> >  		$raw .= " -dport $rule->{dport}" if $rule->{dport};
> >  		$raw .= " -sport $rule->{sport}" if $rule->{sport};
> > +		$raw .= " -log $rule->{log}" if $rule->{log};
> >  	    }
> >  
> >  	    $raw .= " # " . encode('utf8', $rule->{comment})
> > @@ -3190,9 +3203,7 @@ sub get_option_log_level {
> >  
> >      return undef if $v eq '' || $v eq 'nolog';
> >  
> > -    $v = $log_level_hash->{$v} if defined($log_level_hash->{$v});
> > -
> > -    return $v if ($v >= 0) && ($v <= 7);
> > +    return $v if defined($log_level_hash->{$v});
> >  
> >      warn "unknown log level ($k = '$v')\n";
> >  
> > @@ -3225,7 +3236,7 @@ sub generate_std_chains {
> >  	ruleset_create_chain($ruleset, $chain);
> >  	foreach my $rule (@{$std_chains->{$chain}}) {
> >  	    if (ref($rule)) {
> > -		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule);
> > +		ruleset_generate_rule($ruleset, $chain, $ipversion, $rule, 0);
> >  	    } else {
> >  		die "rule $rule as string - should not happen";
> >  	    }
> > 
>



More information about the pve-devel mailing list