From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <o.bektas@proxmox.com> 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 8B25976C3E for <pve-devel@lists.proxmox.com>; Wed, 20 Oct 2021 14:11:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6FC6611D42 for <pve-devel@lists.proxmox.com>; Wed, 20 Oct 2021 14:11:20 +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 0440C11D09 for <pve-devel@lists.proxmox.com>; Wed, 20 Oct 2021 14:11: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 B47B7468DC for <pve-devel@lists.proxmox.com>; Wed, 20 Oct 2021 14:11:18 +0200 (CEST) Date: Wed, 20 Oct 2021 14:11:16 +0200 From: Oguz Bektas <o.bektas@proxmox.com> To: Dominik Csapak <d.csapak@proxmox.com> Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Message-ID: <YXAHZKkSs38FtP/G@gaia> Mail-Followup-To: Oguz Bektas <o.bektas@proxmox.com>, Dominik Csapak <d.csapak@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com> References: <20211011105704.760773-1-o.bektas@proxmox.com> <20211011105704.760773-2-o.bektas@proxmox.com> <39f67e0b-143e-93f6-fd96-7b208b86a3ae@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <39f67e0b-143e-93f6-fd96-7b208b86a3ae@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.917 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v4 firewall 1/2] implement fail2ban backend and API X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Wed, 20 Oct 2021 12:11:50 -0000 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