[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