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

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

[Oguz Bektas]

only as POC/RFC

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


 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,
          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';
+    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";
+
+    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
+}
+
 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*$/) {
+	$opt = $1;
+	$value = $2 // $fail2ban_option_properties->{maxretry}->{default};
+    } elsif ($line =~ m/^(bantime):\s+(\d+)\S*$/) {
+	$opt = $1;
+	$value = $2 * 60 // $fail2ban_option_properties->{bantime}->{default} * 60;
+    }
+
+    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;
+	    };
 	} 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;
 
+
     # load required modules here
 }
 
 sub update {
+
+    use Sys::Syslog;
+    syslog('info', "\n\nupdating stuff again...\n");
+
     my $code = sub {
 
 	my $cluster_conf = load_clusterfw_conf();
@@ -4589,7 +4665,10 @@ sub update {
 	    return;
 	}
 
+
 	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);
 
-- 
2.30.2

Re: [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@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);
>  
> 

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

[Oguz Bektas]

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

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

[Thomas Lamprecht]

On 25/08/2021 11:34, Oguz Bektas wrote:
> On Tue, Aug 24, 2021 at 08:58:10PM +0200, Thomas Lamprecht wrote:
>> 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.
> 

that has zero to do with thinking and evaluating about just doing it ourself here
though? As there's still an additional dependency with an extra daemon running that
may not even interact correctly with how we operate the iptables...

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

I did not asked about the banned IP address view from the fail2ban daemon but
rather if the actual *iptables* rules persist, would be good to get sure about
such stuff if wanting to integrate such a feature..

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

[Oguz Bektas]

hi,

> 
> that has zero to do with thinking and evaluating about just doing it ourself here
> though? As there's still an additional dependency with an extra daemon running that
> may not even interact correctly with how we operate the iptables...

fail2ban already does all of this, is supported and recommended in our wiki
with an example config that's also used in this patch, imho it makes sense to
just use it instead of reinventing the wheel here. surely it wouldn't be hard
to add a reject rule into the chain, but there are other things to think about
as well.

this approach also has the benefit for users who already have our fail2ban
config, who should be able to use this without any interference. whereas if we
separately try to implement the parsing of logs, rotating them, adding banned
addresses to firewall chain (and keeping timestamps of ban times), unbanning them
automatically after 'bantime' expires, there's a lot of things to handle that
i'm also forgetting here... so it's not as simple as one would imagine :)


> 
> >>
> >> 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`.
> 
> I did not asked about the banned IP address view from the fail2ban daemon but
> rather if the actual *iptables* rules persist, would be good to get sure about
> such stuff if wanting to integrate such a feature..

it adds a new INPUT chain falled "f2b-proxmox" when a new rule is added (a.k.a. someone is banned after hitting 'maxretry')

$ iptables -L -v | grep -i f2b -C 3

Chain INPUT (policy ACCEPT 2 packets, 120 bytes)
 pkts bytes target     prot opt in     out     source               destination
  188 72943 f2b-proxmox  tcp  --  any    any     anywhere             anywhere             multiport dports https,http,8006
  466 91452 PVEFW-INPUT  all  --  any    any     anywhere             anywhere
...
Chain f2b-proxmox (1 references)
 pkts bytes target     prot opt in     out     source               destination
   10   520 REJECT     all  --  any    any     192.168.31.213       anywhere             reject-with icmp-port-unreachable
  178 72423 RETURN     all  --  any    any     anywhere             anywhere



the rules persist when restarting the firewall with "pve-firewall restart" or
via systemctl, they also persist with "pve-firewall compile" followed by
service restart. changing firewall rules also has no effect.
so without "systemctl stop fail2ban" i was unable to get rid of the chain and rules.

after starting it again the REJECT rules in the chain still persist (if bantime didn't expire for that entry).
after bantime is over the rule is removed (chain stays as long as service is running).

also i noticed a small bug in my last patch waiting for review, i'll send a fix for that with a new version.

regards,
oguz