Re: [pve-devel] [PATCH firewall] implement fail2ban backend

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Again, I'm not convinced that the approach may be the best we can do, but
as I already looked at it I'll quick review that one nonetheless.

missing "partially fix #1065" prefix.

On 25/08/2021 11:47, Oguz Bektas wrote:
> adds a section "[FAIL2BAN]" in the hostfw configuration, which allows
> the properties 'maxretry' and 'bantime' (in minutes) for the GUI ports.
> 

misses at least some description about what is done with it, as currently it
is "magic" that does the actually banning. Also a link to the wiki this was
derived from, i.e., https://pve.proxmox.com/wiki/Fail2ban would be good.


> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> RFC->PATCH:
> 
> * better parser regex to allow comments in hostfw
> * use heredoc for multiline file contents
> * check if filter file exists, and if the jail configuration has changed
> before writing it out
> * removed the unrelated empty lines that i forgot, and the debug print :D
> * error out if we can't parse an option
> 
> 
>  debian/control      |  1 +
>  src/PVE/Firewall.pm | 79 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 79 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,
>           ipset,
>           iptables,
>           libpve-access-control,
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index edc5336..ed13ca5 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,

no, please do not enable that by default, follow the common defaults of the
firewall and disable it at start.

Also, FWICT this setting is useless, as it is nowhere checked for??

> +	},
> +	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,39 @@ sub ruleset_generate_vm_rules {
>      }
>  }
>  
> +sub generate_fail2ban_config {
> +    my ($maxretry, $bantime) = @_;
> +
> +    my $bantime_seconds = $bantime * 60;
> +
> +    my $fail2ban_filter = <<CONFIG;
> +[Definition]
> +failregex = pvedaemon\\[.*authentication failure; rhost=<HOST> user=.* msg=.*
> +ignoreregex =
> +CONFIG
> +    my $filter_path = '/etc/fail2ban/filter.d/proxmox.conf';
> +    PVE::Tools::file_set_contents($filter_path, $fail2ban_filter) unless -f $filter_path;

we do not use `unless` see our style guide[0]

[0]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

> +
> +
> +    my $fail2ban_jail = <<CONFIG;
> +[proxmox]
> +enabled = true

always enabled and we nowhere check for the "enable" property to either set this
false or delete the config if it is false, meh...

> +port = https,http,8006
> +filter = proxmox
> +logpath = /var/log/daemon.log
> +maxretry = $maxretry
> +bantime = $bantime_seconds
> +CONFIG
> +
> +    my $jail_path = "/etc/fail2ban/jail.d/proxmox.conf";
> +    my $current_fail2ban_jail = PVE::Tools::file_get_contents($jail_path);
> +
> +    if ($current_fail2ban_jail ne $fail2ban_jail) {
> +	PVE::Tools::file_set_contents($jail_path, $fail2ban_jail);
> +	run_command([qw(systemctl try-reload-or-restart fail2ban.service)]);
> +    }
> +}
> +
>  sub generate_nfqueue {
>      my ($options) = @_;
>  
> @@ -2937,6 +2990,16 @@ sub parse_alias {
>      return undef;
>  }
>  
> +sub parse_fail2ban_option {
> +    my ($line) = @_;
> +
> +    if ($line =~ m/^(maxretry|bantime):\s+(\d+)\s*(?:#\s*(.*?)\s*)?$/) {

Note that I just mentioned the weird regex and that your example config wouldn't
get parsed by it, not that it has to accept comments.

So, do other FW config properties actually allow comments too? As else it would be
really weird if just those two accept them but others don't..

> +	return ($1, $2 // $fail2ban_option_properties->{$1}->{default});
> +    } else {
> +	die "error parsing fail2ban options: $line";
> +    }
> +}
> +
>  sub generic_fw_config_parser {
>      my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
>  
> @@ -2965,6 +3028,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 +3114,13 @@ sub generic_fw_config_parser {
>  		$res->{aliases}->{lc($data->{name})} = $data;
>  	    };
>  	    warn "$prefix: $@" if $@;
> +	} elsif ($section eq 'fail2ban') {
> +	    my ($opt, $value) = eval { parse_fail2ban_option($line) };
> +	    if (my $err = $@) {
> +		warn "fail2ban parsing error: $err";

adds duplicate context as the parse_fail2ban_option already has some, so an warning
would look like:

"fail2ban parsing error: error parsing fail2ban options: <cfg line>"

Not really useful/nice IMO so I'd drop one of them.

> +		next;
> +	    }
> +	    $res->{fail2ban}->{$opt} = $value;
>  	} elsif ($section eq 'rules') {
>  	    my $rule;
>  	    eval { $rule = parse_fw_rule($prefix, $line, $cluster_conf, $res, $rule_env); };
> @@ -3620,7 +3695,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');
>  }
>  
> @@ -4590,6 +4665,8 @@ sub update {
>  	}
>  
>  	my $hostfw_conf = load_hostfw_conf($cluster_conf);
> +	my $fail2ban_opts = $hostfw_conf->{fail2ban};
> +	generate_fail2ban_config($fail2ban_opts->{maxretry}, $fail2ban_opts->{bantime});

could also just pass the whole $fail2ban_opts as reference, but no hard feelings there,
can be nice to have named parameters if its only a few.

>  
>  	my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf);
>  
>