Re: [pve-devel] [RFC firewall] implement fail2ban in firewall

[Oguz Bektas <o.bektas@proxmox.com>]

hi,

thank you for the review :)

On Tue, Aug 24, 2021 at 08:58:10PM +0200, Thomas Lamprecht wrote:
> On 23/08/2021 16:07, Oguz Bektas wrote:
> > only as POC/RFC
> > 
> 
> some thoughts/discussions about the methods/decisions you made would get here.
> 
> E.g., did you think about checking the log just directly here, after all we run
> every 10s anyway, so one could just directly parse the daemon log and add the
> rules directly here, no extra daemon and external instance, which adapts filter
> rules, required (the latter is racy anyway). Not necessarily a must, but the
> simple regex on a single file would be easy, hardest thing would be to handle
> rotations and make reading not completely inefficient; but neither to complicated
> either.

in the v2 it works better after implementing your suggestions (i'll send
it today), now we check if the jail file has changed and only write it then.

> 
> any how, does fail2ban always flushes all their rules, as else our rewrite of
> the filter and raw tables on each update would make it somewhat moot?

i'm not exactly sure, but in my tests the banned IP addresses stayed
even after changing configuration and reloading the services. you can
check with `fail2ban-client banned`.

> 
> Still a few comments for review of that approach, which does not means that
> it is the right approach (see above), but nonetheless, check them out.
> 
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> > 
> > known issues:
> > - see FIXME in generate_fail2ban_config. when update/compile is called
> >   the fail2ban service will be restarted, that leads to reload every 10s
> >   (also the jail conf file is re-written, which i'd like to avoid)
> > - no API integration yet. add to host.fw:
> > 
> > ==============
> > [FAIL2BAN]
> > maxretry: N
> > bantime: N # minutes
> 
> your parser would not support above line with the comment though, as
> /^(bantime):\s+(\d+)\S*$/

fixed in v2 along with the other suggestions

> 
> would not allow any whitespace (but arbitrary non-whitespace???) characters
> after the integer..
> 
> > ==============
> > 
> > 
> >  debian/control      |  1 +
> >  src/PVE/Firewall.pm | 81 ++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 81 insertions(+), 1 deletion(-)
> > 
> > diff --git a/debian/control b/debian/control
> > index 4684c5b..377c9ae 100644
> > --- a/debian/control
> > +++ b/debian/control
> > @@ -17,6 +17,7 @@ Package: pve-firewall
> >  Architecture: any
> >  Conflicts: ulogd,
> >  Depends: ebtables,
> > +         fail2ban,
> 
> meh, would prefer to avoid a hard dependency on that package, rather use "Suggests" and
> catch + error out if it isn't available.


ah well, i've enabled it for default, that's why i added it as a hard
dependency.

> 
> >           ipset,
> >           iptables,
> >           libpve-access-control,
> > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> > index edc5336..73ae396 100644
> > --- a/src/PVE/Firewall.pm
> > +++ b/src/PVE/Firewall.pm
> > @@ -1347,6 +1347,26 @@ our $host_option_properties = {
> >      },
> >  };
> >  
> > +our $fail2ban_option_properties = {
> > +	enable => {
> > +	    description => "Enable or disable fail2ban on a node.",
> > +	    type => 'boolean',
> > +	    default => 1,
> > +	},
> > +	maxretry => {
> > +	    description => "Amount of failed tries to ban after.",
> > +	    type => 'integer',
> > +	    minimum => 1,
> > +	    default => 3,
> > +	},
> > +	bantime => {
> > +	    description => "Minutes to ban suspicious IPs.",
> > +	    type => 'integer',
> > +	    minimum => 1,
> > +	    default => 5,
> > +	},
> > +};
> > +
> >  our $vm_option_properties = {
> >      enable => {
> >  	description => "Enable/disable firewall rules.",
> > @@ -2407,6 +2427,30 @@ sub ruleset_generate_vm_rules {
> >      }
> >  }
> >  
> > +sub generate_fail2ban_config {
> > +    my ($maxretry, $bantime) = @_;
> > +
> > +    my $fail2ban_filter = "
> > +[Definition]
> > +failregex = pvedaemon\\[.*authentication failure; rhost=<HOST> user=.* msg=.*
> > +ignoreregex =\n";
> > +    my $filter_path = '/etc/fail2ban/filter.d/proxmox.conf';
> 
> unnecessary intermediate variable that is only used once locally, just use the path
> directly..
> 
> > +    PVE::Tools::file_set_contents($filter_path, $fail2ban_filter);
> > +
> > +    my $fail2ban_config = "
> > +[proxmox]
> > +enabled = true
> > +port = https,http,8006
> > +filter = proxmox
> > +logpath = /var/log/daemon.log
> > +maxretry = $maxretry
> > +bantime = $bantime\n";
> 
> we normally use heredoc strings for such multiline stuff
> 
> > +
> > +    my $jail_path = '/etc/fail2ban/jail.d/proxmox.conf';
> > +    PVE::Tools::file_set_contents($jail_path, $fail2ban_config);
> > +    #run_command([qw(systemctl try-reload-or-restart fail2ban.service)]); # FIXME: excessive... gets called by update/compile every 10 seconds so disable for now
> 
> you can simply check if the existing configuration, if any, and the one you write
> above are the same and use that to decide if a reload would be required.
> 
> > +}
> > +
> >  sub generate_nfqueue {
> >      my ($options) = @_;
> >  
> > @@ -2937,6 +2981,22 @@ sub parse_alias {
> >      return undef;
> >  }
> >  
> > +sub parse_fail2ban_option {
> > +    my ($line) = @_;
> > +
> > +    my ($opt, $value);
> > +
> > +    if ($line =~ m/^(maxretry):\s+(\d+)\S*$/) {
> 
> regex seems wrong, why the non-whitespace match at the end?
> 
> > +	$opt = $1;
> > +	$value = $2 // $fail2ban_option_properties->{maxretry}->{default};
> > +    } elsif ($line =~ m/^(bantime):\s+(\d+)\S*$/) {
> 
> regex seems wrong, why the non-whitespace match at the end?
> 
> > +	$opt = $1;
> > +	$value = $2 * 60 // $fail2ban_option_properties->{bantime}->{default} * 60;
> 
> why duplicate the multiplication?
> 
> $value = ($2 // $fail2ban_option_properties->{bantime}->{default}) * 60;
> 
> 
> or better, do the `* 60` only when writing the fail2ban config and then you could
> use a single if here, i.e. something along the lines of:
> 
> if ($line =~ m/^(maxretry|bantime):\s+(\d+)\S*$/) {
>    return ($1, $2 // $fail2ban_option_properties->{$1}->{default})
> }
> 
> > +    }
> > +
> > +    return ($opt, $value);
> > +}
> > +
> >  sub generic_fw_config_parser {
> >      my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
> >  
> > @@ -2965,6 +3025,11 @@ sub generic_fw_config_parser {
> >  
> >  	my $prefix = "$filename (line $linenr)";
> >  
> > +	if ($empty_conf->{fail2ban} && ($line =~ m/^\[fail2ban\]$/i)) {
> > +	    $section = 'fail2ban';
> > +	    next;
> > +	}
> > +
> >  	if ($empty_conf->{options} && ($line =~ m/^\[options\]$/i)) {
> >  	    $section = 'options';
> >  	    next;
> > @@ -3046,6 +3111,12 @@ sub generic_fw_config_parser {
> >  		$res->{aliases}->{lc($data->{name})} = $data;
> >  	    };
> >  	    warn "$prefix: $@" if $@;
> > +	} elsif ($section eq 'fail2ban') {
> > +	    my ($opt, $value);
> > +	    eval {
> > +		($opt, $value) = parse_fail2ban_option($line);
> > +		$res->{fail2ban}->{$opt} = $value;
> > +	    };
> 
> got one line evals we try to assign the result directly nowadays, i.e., something like:
> 
> my ($opt, $value) = eval { parse_fail2ban_option($line) };
> if (my $err = $@) {
>     warn "fail2ban parsing error: $err"
>     next;
> }
> $res->{fail2ban}->{$opt} = $value;
> 
> would be nicer to read and provide better UX.. but why eval it in the first place if
> you never can die in parse_fail2ban_option anyway?
> 
> >  	} elsif ($section eq 'rules') {
> >  	    my $rule;
> >  	    eval { $rule = parse_fw_rule($prefix, $line, $cluster_conf, $res, $rule_env); };
> > @@ -3620,7 +3691,7 @@ sub load_hostfw_conf {
> >  
> >      $filename = $hostfw_conf_filename if !defined($filename);
> >  
> > -    my $empty_conf = { rules => [], options => {}};
> > +    my $empty_conf = { rules => [], options => {}, fail2ban => {}};
> >      return generic_fw_config_parser($filename, $cluster_conf, $empty_conf, 'host');
> >  }
> >  
> > @@ -4575,10 +4646,15 @@ sub init {
> >  
> >      return if !$enable;
> >  
> > +
> 
> adding unrelated new line
> 
> >      # load required modules here
> >  }
> >  
> >  sub update {
> > +
> > +    use Sys::Syslog;
> > +    syslog('info', "\n\nupdating stuff again...\n");
> > +
> 
> left over debug print
> 
> >      my $code = sub {
> >  
> >  	my $cluster_conf = load_clusterfw_conf();
> > @@ -4589,7 +4665,10 @@ sub update {
> >  	    return;
> >  	}
> >  
> > +
> 
> adding unecessarry new line
> 
> >  	my $hostfw_conf = load_hostfw_conf($cluster_conf);
> > +	my $fail2ban_opts = $hostfw_conf->{fail2ban};
> > +	generate_fail2ban_config($fail2ban_opts->{maxretry}, $fail2ban_opts->{bantime});
> >  
> >  	my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf);
> >  
> > 
>