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 4AB7E911AE for ; Wed, 3 Apr 2024 12:46:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2C92C15FE2 for ; Wed, 3 Apr 2024 12:46: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 12:46:19 +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 E196244D24 for ; Wed, 3 Apr 2024 12:46:18 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 03 Apr 2024 12:46:17 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240402171629.536804-1-s.hanreich@proxmox.com> In-Reply-To: <20240402171629.536804-1-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -1.832 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy FSL_BULK_SIG 0.001 Bulk signature with no Unsubscribe KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RAZOR2_CF_RANGE_51_100 1.886 Razor2 gives confidence level above 50% RAZOR2_CHECK 0.922 Listed in Razor2 (http://razor.sf.net/) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_SBL_A 0.1 Contains URL's A record listed in the Spamhaus SBL blocklist [185.199.109.153, 185.199.110.153, 185.199.111.153, 185.199.108.153] Subject: Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation 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, 03 Apr 2024 10:46:50 -0000 On Tue Apr 2, 2024 at 7:15 PM CEST, Stefan Hanreich wrote: > ## Introduction > This RFC provides a drop-in replacement for the current pve-firewall pack= age > that is based on Rust and nftables. I've now read through all of the code and I think it's safe to say that this looks absolutely pristine to me - I've only got a couple of very minor things to add, some of which are mentioned here, some others can be found as replies to individual patches. First and foremost, all of the structs and enums as well as their respective implementations are very straightforward and easy to follow; there are no surprises to be found anywhere in the code. This is what a proper typesafe abstraction should look like. The vast majority of the code is easy to follow, even for someone like me who isn't as versed in networking like you. Additional context is provided by comments where necessary, which is also great (I *despise* useless comments). Furthermore, it's always a pleasure to see tests - and you've addded quite a lot of them! As I've also smoke-tested this series (we discussed all that off list before already) I can also vouch that this series worked as a perfect drop-in replacement for the existing firewall I had running in a VM. All in all I think this is great - there may be a couple little things to polish and some issues to take care of, as you mentioned below, but I'm very confident that those will eventually be resolved. Otherwise, there are some more comments below and also inline. > Four overall things I want to mention: 1. IMO a lot of the `pub` items should eventually be documented, preferably once the actual series is out. I don't think we need to be as thorough as e.g. the Rust STL's documentation, but I don't think it would hurt if the overall functionality of things was documented. (Of course, e.g. saying that `pub fn hostname()` "gets the hostname" isn't necessary; but you get what I mean :P ) 2. Constants and defaults should also be documented, simply because it makes it easier to refer to those defaults if necessary. On top of that, it's also more obvious if those constants / defaults ever have to be changed for some reason. That way we would avoid accidental semver-breakage. There's a more specific example inline. 3. Would it perhaps actually make sense to use `thiserror` instead of `anyhow`? I know we've speculated a little off list about this already - I still am not 100% convinced that `thiserror` is necessary, but then again, it would be quite nice in the library crates, as you don't really need to propagate any `anyhow::Context` anyways ... There's already `NftError` in proxmox-nftables that *could perhaps* just be implemented via `thiserror`, I think. 4. Some of the types (in particular in `proxmox-ve-config` and `proxmox-nftables`) could use some more trait-deriving - a lot of the structs and enums could benefit from deriving `PartialOrd`, `Ord` and `Hash` for interoperability's sake [0]. While it's probably unlikely that some types will ever be used as keys in a hashmap, deriving the trait IMO doesn't hurt. A lot of types also implement `PartialEq` and `Eq` only for tests, but IMO those traits could theoretically just always be implemented for most of them. As this affects a lot of types I've decided to just sum this up here by the way; if you need more concrete examples, please let me know and I'll add respective comments inline. [0]: https://rust-lang.github.io/api-guidelines/interoperability.html > It consists of three crates: > * proxmox-ve-config > for parsing firewall and guest configuration files, as well as some hel= pers > to access host configuration (particularly networking) > * proxmox-nftables > contains bindings for libnftables as well as types that implement the J= SON > schema defined by libnftables-json > * proxmox-firewall > uses the other two crates to read the firewall configuration and create= the > respective nftables configuration > > > ## Installation > * Build & install all deb packages on your PVE instance > * Enable the nftables firewall by going to > Web UI > > Firewall > Options > proxmox-nftables > * Enable the firewall datacenter-wide if you haven't already > * Restarting running VMs/CTs is required so the changes to the fwbr creat= ion > go into effect > > For your convenience I have provided pre-built packages on our share unde= r > `shanreich-proxmox-firewall`. > > The source code is also available on my staff repo as `proxmox-firewall`. > > > ## Configuration > The firewall should work as a drop-in replacement for the pve-firewall, s= o you > should be able to configure the firewall as usual via the Web UI or > configuration files. > > > ## Known Issues > There is currently one major issue that we still need to solve: REJECTing > packets from the guest firewalls is currently not possible for incoming t= raffic > (it will instead be dropped). > > This is due to the fact that we are using the postrouting hook of nftable= s in a > table with type bridge for incoming traffic. In the bridge table in the > postrouting hook we cannot tell whether the packet has also been sent to = other > ports in the bridge (e.g. when a MAC has not yet been learned and the pac= ket > then gets flooded to all bridge ports). If we would then REJECT a packet = in the > postrouting hook this can lead to a bug where the firewall rules for one = guest > REJECT a packet and send a response (RST for TCP, ICMP port/host-unreacha= ble > otherwise). > > This has also been explained in the respective commit introducing the > restriction [1]. > > We were able to circumvent this restriction in the old firewall due to us= ing > firewall bridges and rejecting in the firewall bridge itself. Doing this = leads > to the behavior described above, which has tripped up some of our users b= efore > [2] [3] and which is, frankly, wrong. > > I currently see two possible solutions for this, both of which carry down= sides. > Your input on this matter would be much appreciated, particularly if you = can > think of another solution which I cannot currently see: > > 1. Only REJECT packets in the prerouting chain of the firewall bridge wit= h the > destination MAC address set to the MAC address of the network device, oth= erwise > DROP > > The downside of this is that we, once again, will have to resort to using > firewall bridges, which we wanted to eliminate. This would also be the so= le > reason for still having to resort to using firewall bridges. > > 2. Only allow DROP in the guest firewall for incoming traffic > > This would be quite awkward since, well, rejecting traffic would be quite= nice > for a firewall I'd say ;) > > I'm happy for all input regarding this matter. > > > ## Useful Commands > > You can check if firewall rules got created by running > > ``` > nft list ruleset > ``` > > You can also check that `iptables` rules are not created via > ``` > iptables-save > ``` > > Further info about the services: > ``` > systemctl status proxmox-firewall.{service,timer} > ``` > > You can grab the debug output from the new firewall like so: > > ``` > RUST_LOG=3Dtrace proxmox-firewall > ``` > > ## Upcoming > > There are some (very minor) features missing: > * automatically generating an ipfilter based on the link-local IPv6 addre= ss > * complete list of ICMP codes > > I also have some improvements for the code base in mind, but I wanted to = get the > RFC out now, since I feel like the new firewall is already in a decent st= ate and > the architecture is relatively solid. Nevertheless there are still a few > improvements that I will be working on: > * move error handling in the library crates to custom error types / thise= rror > * integration tests for the firewall itself > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/comm= it/net/bridge/netfilter/nft_reject_bridge.c?h=3Dv6.8.2&id=3D127917c29a432c3= b798e014a1714e9c1af0f87fe > [2] https://bugzilla.proxmox.com/show_bug.cgi?id=3D4964 > [3] https://forum.proxmox.com/threads/proxmox-claiming-mac-address.52601/= page-2#post-415493 > > > > PS: Since the changestats are broken for patch series including the initi= al > commit of a repo here is the cloc output for proxmox-firewall instead: > > -------------------------------------------------------------------------= ------ > Language files blank comment = code > -------------------------------------------------------------------------= ------ > Rust 37 1642 78 = 7749 > JSON 2 0 0 = 948 > TOML 3 10 0 = 59 > -------------------------------------------------------------------------= ------ > SUM: 42 1652 78 = 8756 > -------------------------------------------------------------------------= ------ > > proxmox-firewall: > > Stefan Hanreich (33): > config: add proxmox-ve-config crate > config: firewall: add types for ip addresses > config: firewall: add types for ports > config: firewall: add types for log level and rate limit > config: firewall: add types for aliases > config: host: add helpers for host network configuration > config: guest: add helpers for parsing guest network config > config: firewall: add types for ipsets > config: firewall: add types for rules > config: firewall: add types for security groups > config: firewall: add generic parser for firewall configs > config: firewall: add cluster-specific config + option types > config: firewall: add host specific config + option types > config: firewall: add guest-specific config + option types > config: firewall: add firewall macros > config: firewall: add conntrack helper types > nftables: add crate for libnftables bindings > nftables: add helpers > nftables: expression: add types > nftables: expression: implement conversion traits for firewall config > nftables: statement: add types > nftables: statement: add conversion traits for config types > nftables: commands: add types > nftables: types: add conversion traits > nftables: add libnftables bindings > firewall: add firewall crate > firewall: add base ruleset > firewall: add config loader > firewall: add rule generation logic > firewall: add object generation logic > firewall: add ruleset generation logic > firewall: add proxmox-firewall binary > firewall: add files for debian packaging > > > qemu-server: > > Stefan Hanreich (1): > firewall: add handling for new nft firewall > > vm-network-scripts/pve-bridge | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > > pve-container: > > Stefan Hanreich (1): > firewall: add handling for new nft firewall > > src/PVE/LXC.pm | 5 +++++ > 1 file changed, 5 insertions(+) > > > pve-firewall: > > Stefan Hanreich (1): > add configuration option for new nftables firewall > > src/PVE/Firewall.pm | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > > pve-manager: > > Stefan Hanreich (1): > firewall: expose configuration option for new nftables firewall > > www/manager6/grid/FirewallOptions.js | 1 + > 1 file changed, 1 insertion(+) > > > Summary over all repositories: > 4 files changed, 29 insertions(+), 6 deletions(-)