Re: [pve-devel] [RFC firewall/proxmox{-ve-rs, -firewall, -perl-rs} 00/21] autogenerate ipsets for sdn objects

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

On Wed Jun 26, 2024 at 2:15 PM CEST, Stefan Hanreich wrote:
> This patch series adds support for autogenerating ipsets for SDN objects. It
> autogenerates ipsets for every VNet as follows:
>
> * ipset containing all IP ranges of the VNet
> * ipset containing all gateways of the VNet
> * ipset containing all IP ranges of the subnet - except gateways
> * ipset containing all dhcp ranges of the vnet

Gave the entire RFC a spin - apart from a few minor version bumps and
one small rejected hunk, everything applied and built just fine.
Encountered no other issues in that regard.

My full review can be found below.

Review: RFC: autogenerate ipsets for sdn objects
================================================

Building
--------

As I also mention in some patches inline, a couple version bumps were
necessary to get everything to build correctly.

Those are as follows:

- proxmox-ve-rs
  - proxmox-sys = "0.6.2"
  - serde_with = "3.8.1"

- proxmox-firewall
  - proxmox-sys = "0.6.2"

Additionally, patch 21 contained one rejected hunk:

diff a/pve-rs/Cargo.toml b/pve-rs/Cargo.toml    (rejected hunks)
@@ -43,3 +43,4 @@ proxmox-subscription = "0.4"
 proxmox-sys = "0.5"
 proxmox-tfa = { version = "4.0.4", features = ["api"] }
 proxmox-time = "2"
+proxmox-ve-config = { version = "0.1.0" }


This got rejected because `proxmox-sys` was already bumped to "0.6" in
`pve-rs`.

Simply adding the dependency manually allowed me to build `pve-rs`.

Testing
-------

You saw a lot of this off-list already (thanks again for the help btw),
but want to mention it here as well, so it doesn't get lost.

Setup
*****

- Installed all packages on my development VM on which I then
  performed my tests.
- No issues were encountered during installation.
- Installed `dnsmasq` on the VM.
- Disabled `dnsmasq` permanently via `systemctl disable --now dnsmasq`.

Simple Zone & VNet
******************

- Added a new simple zone in Datacenter > SDN > Zones.
  - Enabled automatic DHCP on the zone.
- Added a new VNet named `vnet0` and assigned it to the new simple zone.
  - Subnet: 172.16.100.0/24
    - Gateway: 172.16.100.1
    - DHCP Range: 172.16.100.100 - 172.16.100.200
    - SNAT: enabled

Cluster Firewall
****************

- Edited cluster firewall rules.
- Immediately noticed that the new ipsets appeared in the UI and were
  able to be selected.
- Configured a basic firewall rule as example.
  - Type: out
  - Action: REJECT
  - Macro: Web
  - Source: +dc/vnet0-all
  - Log: info
- While this does not block web traffic for VMs, this was nevertheless
  useful to check whether the ipsets and `iptables` FW config were
  generated correctly.
- Relevant output of `iptables-save`:

# iptables-save | grep -E '\-\-dport (80|443) '
-A PVEFW-HOST-OUT -p tcp -m set --match-set PVEFW-0-vnet0-all-v4 src -m tcp --dport 80 -m limit --limit 1/sec -j NFLOG --nflog-prefix ":0:6:PVEFW-HOST-OUT: REJECT: "
-A PVEFW-HOST-OUT -p tcp -m set --match-set PVEFW-0-vnet0-all-v4 src -m tcp --dport 80 -j PVEFW-reject
-A PVEFW-HOST-OUT -p tcp -m set --match-set PVEFW-0-vnet0-all-v4 src -m tcp --dport 443 -m limit --limit 1/sec -j NFLOG --nflog-prefix ":0:6:PVEFW-HOST-OUT: REJECT: "
-A PVEFW-HOST-OUT -p tcp -m set --match-set PVEFW-0-vnet0-all-v4 src -m tcp --dport 443 -j PVEFW-reject

- The set passed via `--match-set` also appears in `ipset -L`:

# ipset -L | grep -A 8 'PVEFW-0-vnet0-all-v4'
Name: PVEFW-0-vnet0-all-v4
Type: hash:net
Revision: 7
Header: family inet hashsize 64 maxelem 64 bucketsize 12 initval 0xa4c09bc0
Size in memory: 504
References: 12
Number of entries: 1
Members:
172.16.100.0/24

- Very nice.

- All of the other ipsets for the VNet also appear in `ipset -L` as expected
  (-all, -dhcp, -gateway for v4 and v6 each)

- When removing removing `+dc/vnet0-all` from the firewall rule and
  leaving the source empty, outgoing web traffic was blocked, as
  expected.
  - Keeping it there does *not* block outgoing web traffic, as expected.

Host Firewall
*************

- The cluster firewall rule above was deactivated.
- Otherwise, the exact same steps as above were performed, just on the
  host firewall.
- The results are exactly the same, as expected.

VM / CT ipsets
**************

- All containers and VMs correctly got their own ipset
  (checked with `ipset -L`).
- Assigning a VM to the VNet makes it show up in IPAM and also updates
  its corresponding ipset correctly.
- Adding the same firewall rule as above to a VM blocks the VM's
  outgoing web traffic, as expected.
  - Changing the source to the VM's ipset, in this case `+guest-ipam-102`,
    also blocks the VM's outgoing web traffic, as expected.

- Output of `iptables-save | grep -E '\-\-dport (80|443) '` on the node:

# iptables-save | grep -E '\-\-dport (80|443) '                                                                                                                                                                                                                                                                                               [17:59:48]
-A tap102i0-OUT -p tcp -m set --match-set PVEFW-0-guest-ipam-102-v4 src -m tcp --dport 80 -m limit --limit 1/sec -j NFLOG --nflog-prefix ":102:6:tap102i0-OUT: REJECT: "
-A tap102i0-OUT -p tcp -m set --match-set PVEFW-0-guest-ipam-102-v4 src -m tcp --dport 80 -j PVEFW-reject
-A tap102i0-OUT -p tcp -m set --match-set PVEFW-0-guest-ipam-102-v4 src -m tcp --dport 443 -m limit --limit 1/sec -j NFLOG --nflog-prefix ":102:6:tap102i0-OUT: REJECT: "
-A tap102i0-OUT -p tcp -m set --match-set PVEFW-0-guest-ipam-102-v4 src -m tcp --dport 443 -j PVEFW-reject

Code Review
-----------

There's not much to say here except that the code looks fantastic as
always - I'm especially a fan of the custom error types. As Gabriel
mentioned, maybe the `thiserror` crate would come in handy eventually,
but that's honestly your decision.

There are a couple more comments inline, but they're rather minor, IMO.

Slightly off-topic, but because you already mentioned off-list that
you're working on updating / adding docstrings and implementing custom
error types for the other things as well, I've got no more things to
mention.

Great work! I really like this feature a lot.

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>

>
> Additionally it generates an IPSet for every guest that has one or more IPAM
> entries in the pve IPAM.
>
> Those can then be used in the cluster / host / guest firewalls. Firewall rules
> automatically update on changes of the SDN / IPAM configuration. This patch
> series works for the old firewall as well as the new firewall.
>
> The ipsets in nftables currently get generated as named ipsets in every table,
> this means that the `nft list ruleset` output can get quite crowded for large
> SDN configurations or large IPAM databases. Another option would be to only
> include them as anonymous IPsets in the rules, which would make the nft output
> far less crowded but this way would use more memory when making extensive use of
> the sdn ipsets, since everytime it is used in a rule we create an entirely new
> ipset.
>
> The current series generates all those ipsets in the datacenter scope. My
> initial approach was to introduce an separate scope (sdn/), but I changed my
> mind during the development because that would require non-trivial changes in
> pve-firewall, which is something I wanted to avoid. With this approach we just
> pass a flag to the cluster config loading wherever we need the SDN config - we
> get everything else (rule validation, API output, rule generation) for 'free'
> basically.
>
> Otherwise, the other way I see would need to introduce a completely new
> parameter into all function calls, or at least a new key in the dc config. All
> call sites would need privileges, due to the IPAM being in /etc/pve/priv. We
> would need to parse the SDN configuration everywhere we need the cluster
> configuration, since otherwise we wouldn't be able to parse / validate the
> cluster configuration and then generate rules.
>
> I'm still unsure whether the upside of having a separate scope is worth the
> effort, so any input w.r.t this topic is much appreciated. Introducing a new
> scope and then adapting the firewall is something I wanted to get some feedback
> on before diving into it, which is why I've refrained from doing it for now.
>
> Of course one downside is that we're kinda locking us in here with this
> decision. With the new firewall adding new scopes should be a lot easier, but if
> we decide to go forward with the SDN ipsets in the datacenter scope we would
> need to support that as well or find some migration path.
>
>
> This patch series is based on my private repositories that split the existing
> proxmox-firewall package into proxmox-firewall and proxmox-ve-rs. Those can be
> found in my staff repo:
>
> staff/s.hanreich/proxmox-ve-rs.git master
> staff/s.hanreich/proxmox-firewall.git no-config
>
> Please note that I included the debian packaging commit in this patch series,
> since it is new and should get reviewed as well, I suppose. It is already
> included when pulling from the proxmox-ve-rs repository.
>
> Dependencies:
> * proxmox-perl-rs and proxmox-firewall depend on proxmox-ve-rs
> * pve-firewall depends on proxmox-perl-rs
>
> proxmox-ve-rs:
>
> Stefan Hanreich (15):
>   debian: add files for packaging
>   firewall: add ip range types
>   firewall: address: use new iprange type for ip entries
>   ipset: add range variant to addresses
>   iprange: add methods for converting an ip range to cidrs
>   ipset: address: add helper methods
>   firewall: guest: derive traits according to rust api guidelines
>   common: add allowlist
>   sdn: add name types
>   sdn: add ipam module
>   sdn: ipam: add method for generating ipsets
>   sdn: add config module
>   sdn: config: add method for generating ipsets
>   tests: add sdn config tests
>   tests: add ipam tests
>
>  .cargo/config.toml                            |    5 +
>  .gitignore                                    |    8 +
>  Cargo.toml                                    |   17 +
>  Makefile                                      |   69 +
>  build.sh                                      |   35 +
>  bump.sh                                       |   44 +
>  proxmox-ve-config/Cargo.toml                  |   16 +-
>  proxmox-ve-config/debian/changelog            |    5 +
>  proxmox-ve-config/debian/control              |   43 +
>  proxmox-ve-config/debian/copyright            |   19 +
>  proxmox-ve-config/debian/debcargo.toml        |    4 +
>  proxmox-ve-config/src/common/mod.rs           |   30 +
>  .../src/firewall/types/address.rs             | 1171 ++++++++++++++++-
>  proxmox-ve-config/src/firewall/types/alias.rs |    4 +-
>  proxmox-ve-config/src/firewall/types/ipset.rs |   29 +-
>  proxmox-ve-config/src/firewall/types/rule.rs  |    6 +-
>  proxmox-ve-config/src/guest/types.rs          |    8 +-
>  proxmox-ve-config/src/guest/vm.rs             |    8 +-
>  proxmox-ve-config/src/lib.rs                  |    2 +
>  proxmox-ve-config/src/sdn/config.rs           |  643 +++++++++
>  proxmox-ve-config/src/sdn/ipam.rs             |  382 ++++++
>  proxmox-ve-config/src/sdn/mod.rs              |  243 ++++
>  proxmox-ve-config/tests/sdn/main.rs           |  189 +++
>  proxmox-ve-config/tests/sdn/resources/ipam.db |   26 +
>  .../tests/sdn/resources/running-config.json   |   54 +
>  25 files changed, 2975 insertions(+), 85 deletions(-)
>  create mode 100644 .cargo/config.toml
>  create mode 100644 .gitignore
>  create mode 100644 Cargo.toml
>  create mode 100644 Makefile
>  create mode 100755 build.sh
>  create mode 100755 bump.sh
>  create mode 100644 proxmox-ve-config/debian/changelog
>  create mode 100644 proxmox-ve-config/debian/control
>  create mode 100644 proxmox-ve-config/debian/copyright
>  create mode 100644 proxmox-ve-config/debian/debcargo.toml
>  create mode 100644 proxmox-ve-config/src/common/mod.rs
>  create mode 100644 proxmox-ve-config/src/sdn/config.rs
>  create mode 100644 proxmox-ve-config/src/sdn/ipam.rs
>  create mode 100644 proxmox-ve-config/src/sdn/mod.rs
>  create mode 100644 proxmox-ve-config/tests/sdn/main.rs
>  create mode 100644 proxmox-ve-config/tests/sdn/resources/ipam.db
>  create mode 100644 proxmox-ve-config/tests/sdn/resources/running-config.json
>
>
> proxmox-firewall:
>
> Stefan Hanreich (3):
>   cargo: update dependencies
>   config: tests: add support for loading sdn and ipam config
>   ipsets: autogenerate ipsets for vnets and ipam
>
>  proxmox-firewall/Cargo.toml                   |    2 +-
>  proxmox-firewall/src/config.rs                |   69 +
>  proxmox-firewall/src/firewall.rs              |   22 +-
>  proxmox-firewall/src/object.rs                |   41 +-
>  .../tests/input/.running-config.json          |   45 +
>  proxmox-firewall/tests/input/ipam.db          |   32 +
>  proxmox-firewall/tests/integration_tests.rs   |   10 +
>  .../integration_tests__firewall.snap          | 1288 +++++++++++++++++
>  proxmox-nftables/src/expression.rs            |   17 +-
>  9 files changed, 1511 insertions(+), 15 deletions(-)
>  create mode 100644 proxmox-firewall/tests/input/.running-config.json
>  create mode 100644 proxmox-firewall/tests/input/ipam.db
>
>
> pve-firewall:
>
> Stefan Hanreich (2):
>   add support for loading sdn firewall configuration
>   api: load sdn ipsets
>
>  src/PVE/API2/Firewall/Cluster.pm |  3 ++-
>  src/PVE/API2/Firewall/Rules.pm   | 18 +++++++------
>  src/PVE/API2/Firewall/VM.pm      |  3 ++-
>  src/PVE/Firewall.pm              | 43 ++++++++++++++++++++++++++++++--
>  4 files changed, 56 insertions(+), 11 deletions(-)
>
>
> proxmox-perl-rs:
>
> Stefan Hanreich (1):
>   add PVE::RS::Firewall::SDN module
>
>  pve-rs/Cargo.toml          |   1 +
>  pve-rs/Makefile            |   1 +
>  pve-rs/src/firewall/mod.rs |   1 +
>  pve-rs/src/firewall/sdn.rs | 130 +++++++++++++++++++++++++++++++++++++
>  pve-rs/src/lib.rs          |   1 +
>  5 files changed, 134 insertions(+)
>  create mode 100644 pve-rs/src/firewall/mod.rs
>  create mode 100644 pve-rs/src/firewall/sdn.rs
>
>
> Summary over all repositories:
>   43 files changed, 4676 insertions(+), 111 deletions(-)



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel