[pve-devel] [PATCH-SERIES manager firewall v4 0/2] fix #1065: implement fail2ban api and gui

[pve-devel] [PATCH-SERIES manager firewall v4 0/2] fix #1065: implement fail2ban api and gui

[Oguz Bektas]

v3->v4:
* fix adding default values when enabling via API for the first time (to
avoid generating empty jail configuration)

pve-manager:

Oguz Bektas (1):
  ui: fail2ban gui for nodes

 www/manager6/Makefile                |  1 +
 www/manager6/grid/Fail2banOptions.js | 51 ++++++++++++++++++++++++++++
 www/manager6/node/Config.js          |  7 ++++
 3 files changed, 59 insertions(+)
 create mode 100644 www/manager6/grid/Fail2banOptions.js


pve-firewall:

Oguz Bektas (1):
  implement fail2ban backend and API

 debian/control                |   1 +
 src/PVE/API2/Firewall/Host.pm |  98 +++++++++++++++++++++++++++++++++
 src/PVE/Firewall.pm           | 101 +++++++++++++++++++++++++++++++++-
3 files changed, 199 insertions(+), 1 deletion(-)


-- 
2.30.2

[pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Oguz Bektas]

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

enable: whether fail2ban jail is enabled or not
maxretry: amount of login tries allowed
bantime: amount of minutes to ban suspicious host

the configuration file is derived from our wiki [0]

example API usage
=====
$ pvesh set /nodes/localhost/firewall/fail2ban --enable 1 --bantime 10 --maxretry 3

$ pvesh get /nodes/localhost/firewall/fail2ban
┌──────────┬───────┐
│ key      │ value │
╞══════════╪═══════╡
│ bantime  │ 10    │
├──────────┼───────┤
│ enable   │ 1     │
├──────────┼───────┤
│ maxretry │ 3     │
└──────────┴───────┘

$ pvesh set /nodes/localhost/firewall/fail2ban --bantime 100
$ pvesh get /nodes/localhost/firewall/fail2ban
┌──────────┬───────┐
│ key      │ value │
╞══════════╪═══════╡
│ bantime  │ 100   │
├──────────┼───────┤
│ enable   │ 1     │
├──────────┼───────┤
│ maxretry │ 3     │
└──────────┴───────┘

$ pvesh set /nodes/localhost/firewall/fail2ban --enable 0
$ pvesh get /nodes/localhost/firewall/fail2ban
┌──────────┬───────┐
│ key      │ value │
╞══════════╪═══════╡
│ bantime  │ 100   │
├──────────┼───────┤
│ enable   │ 0     │
├──────────┼───────┤
│ maxretry │ 3     │
└──────────┴───────┘
=====

[0]: https://pve.proxmox.com/wiki/Fail2ban

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v3->v4:
* fix default values when enabling via API


 debian/control                |   1 +
 src/PVE/API2/Firewall/Host.pm |  98 +++++++++++++++++++++++++++++++++
 src/PVE/Firewall.pm           | 101 +++++++++++++++++++++++++++++++++-
 3 files changed, 199 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/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
index b66ca55..535f188 100644
--- a/src/PVE/API2/Firewall/Host.pm
+++ b/src/PVE/API2/Firewall/Host.pm
@@ -62,6 +62,17 @@ my $add_option_properties = sub {
     return $properties;
 };
 
+my $fail2ban_properties = $PVE::Firewall::fail2ban_option_properties;
+
+my $add_fail2ban_properties = sub {
+    my ($properties) = @_;
+
+    foreach my $k (keys %$fail2ban_properties) {
+	$properties->{$k} = $fail2ban_properties->{$k};
+    }
+
+    return $properties;
+};
 
 __PACKAGE__->register_method({
     name => 'get_options',
@@ -148,6 +159,93 @@ __PACKAGE__->register_method({
 	return undef;
     }});
 
+__PACKAGE__->register_method({
+    name => 'get_fail2ban',
+    path => 'fail2ban',
+    method => 'GET',
+    description => "Get host firewall fail2ban options.",
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/nodes/{node}', [ 'Sys.Audit' ]],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	},
+    },
+    returns => {
+	type => "object",
+	properties => $fail2ban_properties,
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
+	my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
+
+	return PVE::Firewall::copy_opject_with_digest($hostfw_conf->{fail2ban});
+    }});
+
+
+
+__PACKAGE__->register_method({
+    name => 'set_fail2ban',
+    path => 'fail2ban',
+    method => 'PUT',
+    description => "Set host firewall fail2ban options.",
+    protected => 1,
+    proxyto => 'node',
+    permissions => {
+	check => ['perm', '/nodes/{node}', [ 'Sys.Modify' ]],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => &$add_fail2ban_properties({
+	    node => get_standard_option('pve-node'),
+	    delete => {
+		type => 'string', format => 'pve-configid-list',
+		description => "A list of settings you want to delete.",
+		optional => 1,
+	    },
+	    digest => get_standard_option('pve-config-digest'),
+	}),
+    },
+    returns => { type => "null" },
+    code => sub {
+	my ($param) = @_;
+	PVE::Firewall::lock_hostfw_conf(10, sub {
+	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
+	    my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
+
+	    my (undef, $digest) = PVE::Firewall::copy_opject_with_digest($hostfw_conf->{fail2ban});
+	    PVE::Tools::assert_if_modified($digest, $param->{digest});
+
+	    if ($param->{delete}) {
+		foreach my $opt (PVE::Tools::split_list($param->{delete})) {
+		    raise_param_exc({ delete => "no such option '$opt'" })
+			if !$fail2ban_properties->{$opt};
+		    delete $hostfw_conf->{fail2ban}->{$opt};
+		}
+	    }
+
+	    if (defined($param->{enable})) {
+		$param->{enable} = $param->{enable} ? 1 : 0;
+		$hostfw_conf->{fail2ban}->{maxretry} = $param->{maxretry} ? $param->{maxretry} : $fail2ban_properties->{maxretry}->{default};
+		$hostfw_conf->{fail2ban}->{bantime} = $param->{bantime} ? $param->{bantime} : $fail2ban_properties->{bantime}->{default};
+	    }
+
+	    foreach my $k (keys %$fail2ban_properties) {
+		next if !defined($param->{$k});
+		$hostfw_conf->{fail2ban}->{$k} = $param->{$k};
+	    }
+
+	    PVE::Firewall::save_hostfw_conf($hostfw_conf);
+	});
+
+	return undef;
+    }});
+
 __PACKAGE__->register_method({
     name => 'log',
     path => 'log',
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index edc5336..92b77a4 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1347,6 +1347,29 @@ our $host_option_properties = {
     },
 };
 
+our $fail2ban_option_properties = {
+	enable => {
+	    description => "Enable or disable fail2ban on a node.",
+	    type => 'boolean',
+	    optional => 1,
+	    default => 1,
+	},
+	maxretry => {
+	    description => "Amount of failed tries to ban after.",
+	    type => 'integer',
+	    optional => 1,
+	    minimum => 1,
+	    default => 3,
+	},
+	bantime => {
+	    description => "Minutes to ban suspicious IPs.",
+	    type => 'integer',
+	    optional => 1,
+	    minimum => 1,
+	    default => 5,
+	},
+};
+
 our $vm_option_properties = {
     enable => {
 	description => "Enable/disable firewall rules.",
@@ -2407,6 +2430,41 @@ sub ruleset_generate_vm_rules {
     }
 }
 
+sub generate_fail2ban_config {
+    my ($fail2ban_opts) = @_;
+
+    my $enable = $fail2ban_opts->{enable} ? 'true' : 'false';
+    my $maxretry = $fail2ban_opts->{maxretry};
+    my $bantime = $fail2ban_opts->{bantime} * 60; # convert minutes to seconds
+
+    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) if !-f $filter_path;
+
+
+    my $fail2ban_jail = <<CONFIG;
+[proxmox]
+enabled = $enable
+port = https,http,8006
+filter = proxmox
+logpath = /var/log/daemon.log
+maxretry = $maxretry
+bantime = $bantime
+CONFIG
+
+    my $jail_path = "/etc/fail2ban/jail.d/proxmox.conf";
+    my $current_fail2ban_jail = PVE::Tools::file_get_contents($jail_path) if -f $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 +2995,16 @@ sub parse_alias {
     return undef;
 }
 
+sub parse_fail2ban_option {
+    my ($line) = @_;
+
+    if ($line =~ m/^(enable|maxretry|bantime):\s+(\d+)(?:\s*#.*)?$/) {
+	return ($1, int($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 +3033,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 +3119,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 "$err";
+		next;
+	    }
+	    $res->{fail2ban}->{$opt} = $value;
 	} elsif ($section eq 'rules') {
 	    my $rule;
 	    eval { $rule = parse_fw_rule($prefix, $line, $cluster_conf, $res, $rule_env); };
@@ -3251,6 +3331,21 @@ my $format_options = sub {
     return $raw;
 };
 
+my $format_fail2ban = sub {
+    my ($fail2ban_options) = @_;
+
+    my $raw = '';
+
+    $raw .= "[FAIL2BAN]\n\n";
+    foreach my $opt (keys %$fail2ban_options) {
+	$raw .= "$opt: $fail2ban_options->{$opt}\n";
+    }
+    $raw .= "\n";
+
+    return $raw;
+
+};
+
 my $format_aliases = sub {
     my ($aliases) = @_;
 
@@ -3620,7 +3715,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');
 }
 
@@ -3630,7 +3725,9 @@ sub save_hostfw_conf {
     my $raw = '';
 
     my $options = $hostfw_conf->{options};
+    my $fail2ban_options = $hostfw_conf->{fail2ban};
     $raw .= &$format_options($options) if $options && scalar(keys %$options);
+    $raw .= &$format_fail2ban($fail2ban_options) if $fail2ban_options && scalar(keys %$fail2ban_options);
 
     my $rules = $hostfw_conf->{rules};
     if ($rules && scalar(@$rules)) {
@@ -4590,6 +4687,8 @@ sub update {
 	}
 
 	my $hostfw_conf = load_hostfw_conf($cluster_conf);
+	my $fail2ban_opts = $hostfw_conf->{fail2ban};
+	generate_fail2ban_config($fail2ban_opts) if scalar(keys %$fail2ban_opts);
 
 	my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf);
 
-- 
2.30.2

Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Dominik Csapak]

while the code looks ok IMHO, i have some general questions:
* does it really make sense to hard depend on fail2ban?
   could it not also make sense to have it as 'recommends' or 'suggests'?
   setting enabled to 1 could then check if its installed and
   raise an error

* if we do not plan to add more fail2ban options in our config,
   i would rather see a combined fail2ban option (propertystring?)
   that would go into the general host firewall options

   that way we would not have to c&p the whole config parsing/setting api
   and could have a single new option line in the gui instead
   of a whole new panel with only 3 options (i think the majority of our
   users will not use fail2ban)

does that make sense to you?

On 10/11/21 12:57, Oguz Bektas wrote:
> adds a section "[FAIL2BAN]" in the hostfw configuration, which allows
> the properties 'maxretry' and 'bantime' (in minutes) for the GUI ports.
> 
> enable: whether fail2ban jail is enabled or not
> maxretry: amount of login tries allowed
> bantime: amount of minutes to ban suspicious host
> 
> the configuration file is derived from our wiki [0]
> 
> example API usage
> =====
> $ pvesh set /nodes/localhost/firewall/fail2ban --enable 1 --bantime 10 --maxretry 3
> 
> $ pvesh get /nodes/localhost/firewall/fail2ban
> ┌──────────┬───────┐
> │ key      │ value │
> ╞══════════╪═══════╡
> │ bantime  │ 10    │
> ├──────────┼───────┤
> │ enable   │ 1     │
> ├──────────┼───────┤
> │ maxretry │ 3     │
> └──────────┴───────┘
> 
> $ pvesh set /nodes/localhost/firewall/fail2ban --bantime 100
> $ pvesh get /nodes/localhost/firewall/fail2ban
> ┌──────────┬───────┐
> │ key      │ value │
> ╞══════════╪═══════╡
> │ bantime  │ 100   │
> ├──────────┼───────┤
> │ enable   │ 1     │
> ├──────────┼───────┤
> │ maxretry │ 3     │
> └──────────┴───────┘
> 
> $ pvesh set /nodes/localhost/firewall/fail2ban --enable 0
> $ pvesh get /nodes/localhost/firewall/fail2ban
> ┌──────────┬───────┐
> │ key      │ value │
> ╞══════════╪═══════╡
> │ bantime  │ 100   │
> ├──────────┼───────┤
> │ enable   │ 0     │
> ├──────────┼───────┤
> │ maxretry │ 3     │
> └──────────┴───────┘
> =====
> 
> [0]: https://pve.proxmox.com/wiki/Fail2ban
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v3->v4:
> * fix default values when enabling via API
> 
> 
>   debian/control                |   1 +
>   src/PVE/API2/Firewall/Host.pm |  98 +++++++++++++++++++++++++++++++++
>   src/PVE/Firewall.pm           | 101 +++++++++++++++++++++++++++++++++-
>   3 files changed, 199 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/API2/Firewall/Host.pm b/src/PVE/API2/Firewall/Host.pm
> index b66ca55..535f188 100644
> --- a/src/PVE/API2/Firewall/Host.pm
> +++ b/src/PVE/API2/Firewall/Host.pm
> @@ -62,6 +62,17 @@ my $add_option_properties = sub {
>       return $properties;
>   };
>   
> +my $fail2ban_properties = $PVE::Firewall::fail2ban_option_properties;
> +
> +my $add_fail2ban_properties = sub {
> +    my ($properties) = @_;
> +
> +    foreach my $k (keys %$fail2ban_properties) {
> +	$properties->{$k} = $fail2ban_properties->{$k};
> +    }
> +
> +    return $properties;
> +};
>   
>   __PACKAGE__->register_method({
>       name => 'get_options',
> @@ -148,6 +159,93 @@ __PACKAGE__->register_method({
>   	return undef;
>       }});
>   
> +__PACKAGE__->register_method({
> +    name => 'get_fail2ban',
> +    path => 'fail2ban',
> +    method => 'GET',
> +    description => "Get host firewall fail2ban options.",
> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/nodes/{node}', [ 'Sys.Audit' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	},
> +    },
> +    returns => {
> +	type => "object",
> +	properties => $fail2ban_properties,
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +	my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
> +
> +	return PVE::Firewall::copy_opject_with_digest($hostfw_conf->{fail2ban});
> +    }});
> +
> +
> +
> +__PACKAGE__->register_method({
> +    name => 'set_fail2ban',
> +    path => 'fail2ban',
> +    method => 'PUT',
> +    description => "Set host firewall fail2ban options.",
> +    protected => 1,
> +    proxyto => 'node',
> +    permissions => {
> +	check => ['perm', '/nodes/{node}', [ 'Sys.Modify' ]],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => &$add_fail2ban_properties({
> +	    node => get_standard_option('pve-node'),
> +	    delete => {
> +		type => 'string', format => 'pve-configid-list',
> +		description => "A list of settings you want to delete.",
> +		optional => 1,
> +	    },
> +	    digest => get_standard_option('pve-config-digest'),
> +	}),
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +	PVE::Firewall::lock_hostfw_conf(10, sub {
> +	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
> +	    my $hostfw_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
> +
> +	    my (undef, $digest) = PVE::Firewall::copy_opject_with_digest($hostfw_conf->{fail2ban});
> +	    PVE::Tools::assert_if_modified($digest, $param->{digest});
> +
> +	    if ($param->{delete}) {
> +		foreach my $opt (PVE::Tools::split_list($param->{delete})) {
> +		    raise_param_exc({ delete => "no such option '$opt'" })
> +			if !$fail2ban_properties->{$opt};
> +		    delete $hostfw_conf->{fail2ban}->{$opt};
> +		}
> +	    }
> +
> +	    if (defined($param->{enable})) {
> +		$param->{enable} = $param->{enable} ? 1 : 0;
> +		$hostfw_conf->{fail2ban}->{maxretry} = $param->{maxretry} ? $param->{maxretry} : $fail2ban_properties->{maxretry}->{default};
> +		$hostfw_conf->{fail2ban}->{bantime} = $param->{bantime} ? $param->{bantime} : $fail2ban_properties->{bantime}->{default};
> +	    }
> +
> +	    foreach my $k (keys %$fail2ban_properties) {
> +		next if !defined($param->{$k});
> +		$hostfw_conf->{fail2ban}->{$k} = $param->{$k};
> +	    }
> +
> +	    PVE::Firewall::save_hostfw_conf($hostfw_conf);
> +	});
> +
> +	return undef;
> +    }});
> +
>   __PACKAGE__->register_method({
>       name => 'log',
>       path => 'log',
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index edc5336..92b77a4 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -1347,6 +1347,29 @@ our $host_option_properties = {
>       },
>   };
>   
> +our $fail2ban_option_properties = {
> +	enable => {
> +	    description => "Enable or disable fail2ban on a node.",
> +	    type => 'boolean',
> +	    optional => 1,
> +	    default => 1,
> +	},
> +	maxretry => {
> +	    description => "Amount of failed tries to ban after.",
> +	    type => 'integer',
> +	    optional => 1,
> +	    minimum => 1,
> +	    default => 3,
> +	},
> +	bantime => {
> +	    description => "Minutes to ban suspicious IPs.",
> +	    type => 'integer',
> +	    optional => 1,
> +	    minimum => 1,
> +	    default => 5,
> +	},
> +};
> +
>   our $vm_option_properties = {
>       enable => {
>   	description => "Enable/disable firewall rules.",
> @@ -2407,6 +2430,41 @@ sub ruleset_generate_vm_rules {
>       }
>   }
>   
> +sub generate_fail2ban_config {
> +    my ($fail2ban_opts) = @_;
> +
> +    my $enable = $fail2ban_opts->{enable} ? 'true' : 'false';
> +    my $maxretry = $fail2ban_opts->{maxretry};
> +    my $bantime = $fail2ban_opts->{bantime} * 60; # convert minutes to seconds
> +
> +    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) if !-f $filter_path;
> +
> +
> +    my $fail2ban_jail = <<CONFIG;
> +[proxmox]
> +enabled = $enable
> +port = https,http,8006
> +filter = proxmox
> +logpath = /var/log/daemon.log
> +maxretry = $maxretry
> +bantime = $bantime
> +CONFIG
> +
> +    my $jail_path = "/etc/fail2ban/jail.d/proxmox.conf";
> +    my $current_fail2ban_jail = PVE::Tools::file_get_contents($jail_path) if -f $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 +2995,16 @@ sub parse_alias {
>       return undef;
>   }
>   
> +sub parse_fail2ban_option {
> +    my ($line) = @_;
> +
> +    if ($line =~ m/^(enable|maxretry|bantime):\s+(\d+)(?:\s*#.*)?$/) {
> +	return ($1, int($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 +3033,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 +3119,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 "$err";
> +		next;
> +	    }
> +	    $res->{fail2ban}->{$opt} = $value;
>   	} elsif ($section eq 'rules') {
>   	    my $rule;
>   	    eval { $rule = parse_fw_rule($prefix, $line, $cluster_conf, $res, $rule_env); };
> @@ -3251,6 +3331,21 @@ my $format_options = sub {
>       return $raw;
>   };
>   
> +my $format_fail2ban = sub {
> +    my ($fail2ban_options) = @_;
> +
> +    my $raw = '';
> +
> +    $raw .= "[FAIL2BAN]\n\n";
> +    foreach my $opt (keys %$fail2ban_options) {
> +	$raw .= "$opt: $fail2ban_options->{$opt}\n";
> +    }
> +    $raw .= "\n";
> +
> +    return $raw;
> +
> +};
> +
>   my $format_aliases = sub {
>       my ($aliases) = @_;
>   
> @@ -3620,7 +3715,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');
>   }
>   
> @@ -3630,7 +3725,9 @@ sub save_hostfw_conf {
>       my $raw = '';
>   
>       my $options = $hostfw_conf->{options};
> +    my $fail2ban_options = $hostfw_conf->{fail2ban};
>       $raw .= &$format_options($options) if $options && scalar(keys %$options);
> +    $raw .= &$format_fail2ban($fail2ban_options) if $fail2ban_options && scalar(keys %$fail2ban_options);
>   
>       my $rules = $hostfw_conf->{rules};
>       if ($rules && scalar(@$rules)) {
> @@ -4590,6 +4687,8 @@ sub update {
>   	}
>   
>   	my $hostfw_conf = load_hostfw_conf($cluster_conf);
> +	my $fail2ban_opts = $hostfw_conf->{fail2ban};
> +	generate_fail2ban_config($fail2ban_opts) if scalar(keys %$fail2ban_opts);
>   
>   	my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = compile($cluster_conf, $hostfw_conf);
>   
> 

Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Oguz Bektas]

On Tue, Oct 19, 2021 at 03:43:49PM +0200, Dominik Csapak wrote:
> while the code looks ok IMHO, i have some general questions:
> * does it really make sense to hard depend on fail2ban?
>   could it not also make sense to have it as 'recommends' or 'suggests'?
>   setting enabled to 1 could then check if its installed and
>   raise an error
> 
> * if we do not plan to add more fail2ban options in our config,
>   i would rather see a combined fail2ban option (propertystring?)
>   that would go into the general host firewall options
> 
>   that way we would not have to c&p the whole config parsing/setting api
>   and could have a single new option line in the gui instead
>   of a whole new panel with only 3 options (i think the majority of our
>   users will not use fail2ban)

> 
> does that make sense to you?
> 

well if we hide it like that inside the menu, then surely nobody will
use it ;)

separate panel makes it more visible IMO. it shouldn't be hidden 3
layers deep in the options menu (Host -> Firewall -> Options -> Fail2ban
-> Enable) for such a simple feature, i think a lot more people would
use it if it's placed in a visible location (Host -> Fail2ban ->
Enable). if you really insist on putting it in the firewall options menu
then i'll have to insist for it to be installed & enabled by default :)

i didn't see any good reason to not have it installed and enabled with
sane defaults since it doesn't hurt anything IMHO... that's why i went
with hard dependency instead of 'recommends' or 'suggests'.

also i wanted to have a separate API endpoint which is why i didnt go for
adding it into the options section.
though we can still make it take a property string as option i suppose.

oguz

Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Thomas Lamprecht]

On 20.10.21 14:11, Oguz Bektas wrote:
> On Tue, Oct 19, 2021 at 03:43:49PM +0200, Dominik Csapak wrote:
>> while the code looks ok IMHO, i have some general questions:
>> * does it really make sense to hard depend on fail2ban?
>>   could it not also make sense to have it as 'recommends' or 'suggests'?
>>   setting enabled to 1 could then check if its installed and
>>   raise an error
>>
>> * if we do not plan to add more fail2ban options in our config,
>>   i would rather see a combined fail2ban option (propertystring?)
>>   that would go into the general host firewall options
>>
>>   that way we would not have to c&p the whole config parsing/setting api
>>   and could have a single new option line in the gui instead
>>   of a whole new panel with only 3 options (i think the majority of our
>>   users will not use fail2ban)
> 
>>
>> does that make sense to you?
>>
> 
> well if we hide it like that inside the menu, then surely nobody will
> use it ;)
> 
> separate panel makes it more visible IMO. it shouldn't be hidden 3
> layers deep in the options menu (Host -> Firewall -> Options -> Fail2ban
> -> Enable) for such a simple feature, i think a lot more people would
> use it if it's placed in a visible location (Host -> Fail2ban ->
> Enable). if you really insist on putting it in the firewall options menu
> then i'll have to insist for it to be installed & enabled by default :)

With that arguing every option would need to be its own panel placed at the
top-level.. PVE's interface is complex as is, sensible grouping simple one time
option actually helps in UX and to find stuff, documentation can ensure features
are found, they provide a central searchable place for that, after all.

Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API

[Thomas Lamprecht]

On 19.10.21 15:43, Dominik Csapak wrote:
> while the code looks ok IMHO, i have some general questions:
> * does it really make sense to hard depend on fail2ban?
>   could it not also make sense to have it as 'recommends' or 'suggests'?
>   setting enabled to 1 could then check if its installed and
>   raise an error

fwiw, it does not make sense to me to have a hard dependency here, as I pointed
out in pretty much every revision of this series, that and most other things (e.g.,
trying if we can simply generate the rules here ourself) where rather ignored so
after the third iteration I went "tit for tat" and ignored the whole thing..

> 
> * if we do not plan to add more fail2ban options in our config,
>   i would rather see a combined fail2ban option (propertystring?)
>   that would go into the general host firewall options
> 
>   that way we would not have to c&p the whole config parsing/setting api
>   and could have a single new option line in the gui instead
>   of a whole new panel with only 3 options (i think the majority of our
>   users will not use fail2ban)

would make much more sense, it's an simple option and bringing down UX by
crowding the interface for a simple an option that one sets one-time only
anyway seems not ideal to me..

[pve-devel] [PATCH v4 manager 2/2] fix #1065: ui: fail2ban gui for nodes

[Oguz Bektas]

adds a simple grid for fail2ban options into the node config panel

---
v4:
* no changes


 www/manager6/Makefile                |  1 +
 www/manager6/grid/Fail2banOptions.js | 51 ++++++++++++++++++++++++++++
 www/manager6/node/Config.js          |  7 ++++
 3 files changed, 59 insertions(+)
 create mode 100644 www/manager6/grid/Fail2banOptions.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 7d491f57..ad9fe58a 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -74,6 +74,7 @@ JSSRC= 							\
 	grid/BackupView.js				\
 	grid/FirewallAliases.js				\
 	grid/FirewallOptions.js				\
+	grid/Fail2banOptions.js				\
 	grid/FirewallRules.js				\
 	grid/PoolMembers.js				\
 	grid/Replication.js				\
diff --git a/www/manager6/grid/Fail2banOptions.js b/www/manager6/grid/Fail2banOptions.js
new file mode 100644
index 00000000..5de0c18c
--- /dev/null
+++ b/www/manager6/grid/Fail2banOptions.js
@@ -0,0 +1,51 @@
+Ext.define('PVE.Fail2banOptions', {
+    extend: 'Proxmox.grid.ObjectGrid',
+    alias: ['widget.pveFail2banOptions'],
+
+    base_url: undefined,
+
+    initComponent: function() {
+	var me = this;
+
+	me.rows = {};
+
+	me.add_boolean_row('enable', gettext("Enable Fail2Ban"));
+	me.add_integer_row('maxretry', gettext("Max retries"));
+	me.add_integer_row('bantime', gettext("Minutes to ban"));
+
+	var edit_btn = new Ext.Button({
+	    text: gettext('Edit'),
+	    disabled: true,
+	    handler: function() { me.run_editor(); },
+	});
+
+	var set_button_status = function() {
+	    var sm = me.getSelectionModel();
+	    var rec = sm.getSelection()[0];
+
+	    if (!rec) {
+		edit_btn.disable();
+		return;
+	    }
+	    var rowdef = me.rows[rec.data.key];
+	    edit_btn.setDisabled(!rowdef.editor);
+	};
+
+	Ext.apply(me, {
+	    url: "/api2/json" + me.base_url,
+	    tbar: [edit_btn],
+	    editorConfig: {
+		url: "/api2/extjs" + me.base_url,
+	    },
+	    listeners: {
+		itemdblclick: me.run_editor,
+		selectionchange: set_button_status,
+	    },
+	});
+
+	me.callParent();
+	me.on('activate', me.rstore.startUpdate);
+	me.on('destroy', me.rstore.stopUpdate);
+	me.on('deactivate', me.rstore.stopUpdate);
+    },
+});
diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js index 68f80391..9dbe8d0c 100644
--- a/www/manager6/node/Config.js
+++ b/www/manager6/node/Config.js
@@ -276,6 +276,13 @@ Ext.define('PVE.node.Config', {
 		    base_url: '/nodes/' + nodename + '/firewall/options',
 		    fwtype: 'node',
 		    itemId: 'firewall-options',
+		},
+		{
+		    xtype: 'pveFail2banOptions',
+		    iconCls: 'fa fa-legal',
+		    title: gettext('Fail2ban'),
+		    base_url: '/nodes/' + nodename + '/firewall/fail2ban',
+		    itemId: 'fail2ban-options',
 		});
 	}
 
-- 
2.30.2

Re: [pve-devel] [PATCH v4 manager 2/2] fix #1065: ui: fail2ban gui for nodes

[Dominik Csapak]

looks mostly ok (besides my comment about the propertystring and options
thing of the previous patch)

comment inline:

On 10/11/21 12:57, Oguz Bektas wrote:
> adds a simple grid for fail2ban options into the node config panel
> 
> ---
> v4:
> * no changes
> 
> 
>   www/manager6/Makefile                |  1 +
>   www/manager6/grid/Fail2banOptions.js | 51 ++++++++++++++++++++++++++++
>   www/manager6/node/Config.js          |  7 ++++
>   3 files changed, 59 insertions(+)
>   create mode 100644 www/manager6/grid/Fail2banOptions.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 7d491f57..ad9fe58a 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -74,6 +74,7 @@ JSSRC= 							\
>   	grid/BackupView.js				\
>   	grid/FirewallAliases.js				\
>   	grid/FirewallOptions.js				\
> +	grid/Fail2banOptions.js				\
>   	grid/FirewallRules.js				\
>   	grid/PoolMembers.js				\
>   	grid/Replication.js				\
> diff --git a/www/manager6/grid/Fail2banOptions.js b/www/manager6/grid/Fail2banOptions.js
> new file mode 100644
> index 00000000..5de0c18c
> --- /dev/null
> +++ b/www/manager6/grid/Fail2banOptions.js
> @@ -0,0 +1,51 @@
> +Ext.define('PVE.Fail2banOptions', {
> +    extend: 'Proxmox.grid.ObjectGrid',
> +    alias: ['widget.pveFail2banOptions'],
> +
> +    base_url: undefined,
> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	me.rows = {};
> +
> +	me.add_boolean_row('enable', gettext("Enable Fail2Ban"));
> +	me.add_integer_row('maxretry', gettext("Max retries"));
> +	me.add_integer_row('bantime', gettext("Minutes to ban"));
> +
> +	var edit_btn = new Ext.Button({
> +	    text: gettext('Edit'),
> +	    disabled: true,
> +	    handler: function() { me.run_editor(); },
> +	});
> +
> +	var set_button_status = function() {
> +	    var sm = me.getSelectionModel();
> +	    var rec = sm.getSelection()[0];
> +
> +	    if (!rec) {
> +		edit_btn.disable();
> +		return;
> +	    }
> +	    var rowdef = me.rows[rec.data.key];
> +	    edit_btn.setDisabled(!rowdef.editor);
> +	};
> +
> +	Ext.apply(me, {
> +	    url: "/api2/json" + me.base_url,
> +	    tbar: [edit_btn],
> +	    editorConfig: {
> +		url: "/api2/extjs" + me.base_url,
> +	    },
> +	    listeners: {
> +		itemdblclick: me.run_editor,
> +		selectionchange: set_button_status,
> +	    },
> +	});

i know its mostly copy&pasted, but i'd still rather see a more
declarative approach to this, even if it just defining the functions 
inline here.

> +
> +	me.callParent();
> +	me.on('activate', me.rstore.startUpdate);
> +	me.on('destroy', me.rstore.stopUpdate);
> +	me.on('deactivate', me.rstore.stopUpdate);
> +    },
> +});
> diff --git a/www/manager6/node/Config.js b/www/manager6/node/Config.js index 68f80391..9dbe8d0c 100644
> --- a/www/manager6/node/Config.js
> +++ b/www/manager6/node/Config.js
> @@ -276,6 +276,13 @@ Ext.define('PVE.node.Config', {
>   		    base_url: '/nodes/' + nodename + '/firewall/options',
>   		    fwtype: 'node',
>   		    itemId: 'firewall-options',
> +		},
> +		{
> +		    xtype: 'pveFail2banOptions',
> +		    iconCls: 'fa fa-legal',
> +		    title: gettext('Fail2ban'),
> +		    base_url: '/nodes/' + nodename + '/firewall/fail2ban',
> +		    itemId: 'fail2ban-options',
>   		});
>   	}
>   
>