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

Thomas Lamprecht t.lamprecht at proxmox.com
Mon Mar 18 08:37:01 CET 2019


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 <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