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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 11F276E9AF for ; Tue, 24 Aug 2021 20:58:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E83048818 for ; Tue, 24 Aug 2021 20:58:22 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id C22D18807 for ; Tue, 24 Aug 2021 20:58:18 +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 85DDF43268 for ; Tue, 24 Aug 2021 20:58:12 +0200 (CEST) Message-ID: <9ca9e438-6983-9dc8-ad8d-f8a7b5ce9455@proxmox.com> Date: Tue, 24 Aug 2021 20:58:10 +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: <20210823140736.1371942-1-o.bektas@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210823140736.1371942-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.370 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 -2.023 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. [firewall.pm] Subject: Re: [pve-devel] [RFC firewall] implement fail2ban in firewall 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: Tue, 24 Aug 2021 18:58:53 -0000 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 > --- > > 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= 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); > >