[pve-devel] [RFC firewall/proxmox{-ve-rs, -firewall, -perl-rs} 00/21] autogenerate ipsets for sdn objects
Max Carrara
m.carrara at proxmox.com
Tue Aug 13 18:06:01 CEST 2024
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 at proxmox.com>
Tested-by: Max Carrara <m.carrara at 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(-)
More information about the pve-devel
mailing list