Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation

["Max Carrara" <m.carrara@proxmox.com>]

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 package
> 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 helpers
>   to access host configuration (particularly networking)
> * proxmox-nftables
>   contains bindings for libnftables as well as types that implement the JSON
>   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 > <Host> > 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 creation
>   go into effect
>
> For your convenience I have provided pre-built packages on our share under
> `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, so 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 traffic
> (it will instead be dropped).
>
> This is due to the fact that we are using the postrouting hook of nftables 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 packet
> 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-unreachable
> 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 using
> 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 before
> [2] [3] and which is, frankly, wrong.
>
> I currently see two possible solutions for this, both of which carry downsides.
> 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 with the
> destination MAC address set to the MAC address of the network device, otherwise
> 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 sole
> 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=trace proxmox-firewall
> ```
>
> ## Upcoming
>
> There are some (very minor) features missing:
> * automatically generating an ipfilter based on the link-local IPv6 address
> * 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 state 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 / thiserror
> * integration tests for the firewall itself
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/bridge/netfilter/nft_reject_bridge.c?h=v6.8.2&id=127917c29a432c3b798e014a1714e9c1af0f87fe
> [2] https://bugzilla.proxmox.com/show_bug.cgi?id=4964
> [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 initial
> 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(-)