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

[Thomas Lamprecht]

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.

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?

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 at 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*$/

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.

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