From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 08AA81FF161 for ; Tue, 13 Aug 2024 18:06:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8025468B3; Tue, 13 Aug 2024 18:06:39 +0200 (CEST) Mime-Version: 1.0 Date: Tue, 13 Aug 2024 18:06:01 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240626121550.292290-1-s.hanreich@proxmox.com> In-Reply-To: <20240626121550.292290-1-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 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 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, firewall.pm, config.rs, ipam.rs, rule.rs, bump.sh, address.rs, cluster.pm, build.sh, expression.rs, alias.rs, firewall.rs, object.rs, types.rs, rules.pm, vm.rs, ipset.rs, vm.pm, sdn.rs, main.rs] Subject: Re: [pve-devel] [RFC firewall/proxmox{-ve-rs, -firewall, -perl-rs} 00/21] autogenerate ipsets for sdn objects 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 Tested-by: Max Carrara > > 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