public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH firewall] implement fail2ban backend
@ 2021-08-25  9:47 Oguz Bektas
  2021-08-25 10:01 ` Mark Schouten
  2021-08-25 13:36 ` Thomas Lamprecht
  0 siblings, 2 replies; 3+ messages in thread
From: Oguz Bektas @ 2021-08-25  9:47 UTC (permalink / raw)
  To: pve-devel

adds a section "[FAIL2BAN]" in the hostfw configuration, which allows
the properties 'maxretry' and 'bantime' (in minutes) for the GUI ports.

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,
+	},
+	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;
+
+
+    my $fail2ban_jail = <<CONFIG;
+[proxmox]
+enabled = true
+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*)?$/) {
+	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";
+		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});
 
 	my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf);
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH firewall] implement fail2ban backend
  2021-08-25  9:47 [pve-devel] [PATCH firewall] implement fail2ban backend Oguz Bektas
@ 2021-08-25 10:01 ` Mark Schouten
  2021-08-25 13:36 ` Thomas Lamprecht
  1 sibling, 0 replies; 3+ messages in thread
From: Mark Schouten @ 2021-08-25 10:01 UTC (permalink / raw)
  To: Proxmox VE development discussion


Hi,


Few comments:
* I'd say you would enable this on a cluster-level, for all nodes
* Please allow a whitelist to be added
* Feature request: When fail2ban on a node bans an IP, also add it to a blocklist used by VM's with a Proxmox firewall enabled.


-- 
Mark Schouten 
CTO, Tuxis B.V. | https://www.tuxis.nl/ 
<mark@tuxis.nl> | +31 318 200208



 From:   Oguz Bektas <o.bektas@proxmox.com> 
 To:   <pve-devel@lists.proxmox.com> 
 Sent:   2021-08-25 11:47 
 Subject:   [pve-devel] [PATCH firewall] implement fail2ban backend 

adds a section "[FAIL2BAN]" in the hostfw configuration, which allows 
the properties 'maxretry' and 'bantime' (in minutes) for the GUI ports. 
 
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, 
+     }, 
+     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; 
+ 
+ 
+    my $fail2ban_jail = <<CONFIG; 
+[proxmox] 
+enabled = true 
+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*)?$/) { 
+     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"; 
+          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}); 
  
      my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf); 
  
--  
2.30.2 
 
 
 
_______________________________________________ 
pve-devel mailing list 
pve-devel@lists.proxmox.com 
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH firewall] implement fail2ban backend
  2021-08-25  9:47 [pve-devel] [PATCH firewall] implement fail2ban backend Oguz Bektas
  2021-08-25 10:01 ` Mark Schouten
@ 2021-08-25 13:36 ` Thomas Lamprecht
  1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2021-08-25 13:36 UTC (permalink / raw)
  To: Proxmox VE development discussion, Oguz Bektas

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-25 13:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-25  9:47 [pve-devel] [PATCH firewall] implement fail2ban backend Oguz Bektas
2021-08-25 10:01 ` Mark Schouten
2021-08-25 13:36 ` Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal