From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4816D6EBB8 for ; Wed, 25 Aug 2021 15:36:17 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3A30D118E6 for ; Wed, 25 Aug 2021 15:36:17 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D621B118D8 for ; Wed, 25 Aug 2021 15:36:12 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id AEFE643920 for ; Wed, 25 Aug 2021 15:36:11 +0200 (CEST) Message-ID: Date: Wed, 25 Aug 2021 15:36:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.0 Content-Language: en-GB To: Proxmox VE development discussion , Oguz Bektas References: <20210825094703.374025-1-o.bektas@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210825094703.374025-1-o.bektas@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.004 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.305 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, firewall.pm] Subject: Re: [pve-devel] [PATCH firewall] implement fail2ban backend X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Aug 2021 13:36:17 -0000 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 > --- > 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 = < +[Definition] > +failregex = pvedaemon\\[.*authentication failure; rhost= 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 = < +[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: " 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); > >