* 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