From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id EE89E1FF0AB for ; Wed, 23 Sep 2026 12:00:34 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EC67C2156B; Wed, 23 Sep 2026 12:00:29 +0200 (CEST) Message-ID: Date: Wed, 23 Sep 2026 12:00:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib To: pve-devel@lists.proxmox.com References: <20260827131524.1411472-1-h.laimer@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260827131524.1411472-1-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.726 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: BT6O4AJMLAB6CRHRG2OQHPTSHWWCKFKP X-Message-ID-Hash: BT6O4AJMLAB6CRHRG2OQHPTSHWWCKFKP X-MailFrom: s.hanreich@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 8/27/26 3:15 PM, Hannes Laimer wrote: > The statements selecting a transport protocol, its ports or an ICMP type > and code were generated in proxmox-firewall, although which statements > select a protocol is nftables knowledge and does not depend on the > firewall config. Move the rendering into proxmox-nftables, working on > plain expressions, and add an entry point taking the firewall's protocol > type to the config extension. The firewall keeps its IP family pinning > on top. > > No functional change intended. > > Signed-off-by: Hannes Laimer > --- > spiritually depends/based on [1], but apply order doesnt really matter, > and they are both good on their own > > [1] https://lore.proxmox.com/pve-devel/20260827115803.1331245-1-h.laimer@proxmox.com/ > > proxmox-firewall/src/rule.rs | 166 +++---------------------------- > proxmox-nftables/src/lib.rs | 1 + > proxmox-nftables/src/protocol.rs | 79 +++++++++++++++ > 3 files changed, 93 insertions(+), 153 deletions(-) > create mode 100644 proxmox-nftables/src/protocol.rs > > diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs > index 048c00e..ea63a08 100644 > --- a/proxmox-firewall/src/rule.rs > +++ b/proxmox-firewall/src/rule.rs > @@ -6,6 +6,7 @@ use proxmox_log as log; > use proxmox_nftables::{ > Expression, Statement, > expression::{Ct, IpFamily, Meta, Payload, Prefix}, > + protocol, > statement::{Log, LogLevel, Match, Operator}, > types::{AddRule, ChainPart, SetName, TableFamily, TablePart}, > }; > @@ -19,9 +20,7 @@ use proxmox_ve_config::{ > ipset::{Ipfilter, IpsetName, RuleIpsetName}, > log::LogRateLimit, > rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict}, > - rule_match::{ > - Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp, > - }, > + rule_match::{IpAddrMatch, IpMatch, Protocol, RuleMatch}, > }, > }, > guest::types::Vmid, > @@ -596,162 +595,23 @@ impl ToNftRules for IpMatch { > } > } > > -fn handle_protocol(rules: &mut [NftRule], _env: &NftRuleEnv, name: &str) -> Result<(), Error> { > - for rule in rules.iter_mut() { > - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(name)).into()); > - } > - > - Ok(()) > -} > - > impl ToNftRules for Protocol { > - fn to_nft_rules(&self, rules: &mut Vec, env: &NftRuleEnv) -> Result<(), Error> { > - log::trace!("adding protocol: {self:?}"); > - > - match self { > - Protocol::Tcp(tcp) => tcp.to_nft_rules(rules, env), > - Protocol::Udp(udp) => udp.to_nft_rules(rules, env), > - Protocol::Dccp(ports) => { > - handle_protocol(rules, env, "dccp")?; > - ports.to_nft_rules(rules, env) > - } > - Protocol::UdpLite(ports) => { > - handle_protocol(rules, env, "udplite")?; > - ports.to_nft_rules(rules, env) > - } > - Protocol::Sctp(sctp) => sctp.to_nft_rules(rules, env), > - Protocol::Icmp(icmp) => icmp.to_nft_rules(rules, env), > - Protocol::Icmpv6(icmpv6) => icmpv6.to_nft_rules(rules, env), > - Protocol::Named(name) => handle_protocol(rules, env, name), > - Protocol::Numeric(id) => { > - for rule in rules.iter_mut() { > - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(*id)).into()); > - } > - > - Ok(()) > - } > - } > - } > -} > - > -impl ToNftRules for Tcp { > - fn to_nft_rules(&self, rules: &mut Vec, env: &NftRuleEnv) -> Result<(), Error> { > - handle_protocol(rules, env, "tcp")?; > - self.ports().to_nft_rules(rules, env) > - } > -} > - > -impl ToNftRules for Udp { > - fn to_nft_rules(&self, rules: &mut Vec, env: &NftRuleEnv) -> Result<(), Error> { > - handle_protocol(rules, env, "udp")?; > - self.ports().to_nft_rules(rules, env) > - } > -} > - > -impl ToNftRules for Sctp { > - fn to_nft_rules(&self, rules: &mut Vec, env: &NftRuleEnv) -> Result<(), Error> { > - handle_protocol(rules, env, "sctp")?; > - self.ports().to_nft_rules(rules, env) > - } > -} > - > -impl ToNftRules for Icmp { > fn to_nft_rules(&self, rules: &mut Vec, _env: &NftRuleEnv) -> Result<(), Error> { > - for rule in rules.iter_mut() { > - if matches!(rule.family(), Some(Family::V4) | None) { > - if let Some(icmp_type) = self.ty() { > - rule.push( > - Match::new_eq(Payload::field("icmp", "type"), Expression::from(icmp_type)) > - .into(), > - ); > - } > - > - if let Some(icmp_code) = self.code() { > - rule.push( > - Match::new_eq(Payload::field("icmp", "code"), Expression::from(icmp_code)) > - .into(), > - ); > - } > - > - if self.code().is_none() && self.ty().is_none() { > - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from("icmp")).into()); > - } > - > - rule.set_family(Family::V4); > - } > - } > - > - Ok(()) > - } > -} > - > -impl ToNftRules for Icmpv6 { > - fn to_nft_rules(&self, rules: &mut Vec, _env: &NftRuleEnv) -> Result<(), Error> { > - log::trace!("applying icmpv6: {self:?}"); > + log::trace!("adding protocol: {self:?}"); > > + let family = self.family(); > for rule in rules.iter_mut() { > - if matches!(rule.family(), Some(Family::V6) | None) { > - if let Some(icmp_type) = self.ty() { > - rule.push( > - Match::new_eq( > - Payload::field("icmpv6", "type"), > - Expression::from(icmp_type), > - ) > - .into(), > - ); > - } > - > - if let Some(icmp_code) = self.code() { > - rule.push( > - Match::new_eq( > - Payload::field("icmpv6", "code"), > - Expression::from(icmp_code), > - ) > - .into(), > - ); > - } > - > - if self.code().is_none() && self.ty().is_none() { > - rule.push( > - Match::new_eq(Meta::new("l4proto"), Expression::from("icmpv6")).into(), > - ); > - } > - > - rule.set_family(Family::V6); > + if family > + .zip(rule.family()) > + .is_some_and(|(wanted, pinned)| wanted != pinned) > + { > + continue; > } > - } > - > - Ok(()) > - } > -} > - > -impl ToNftRules for Ports { > - fn to_nft_rules(&self, rules: &mut Vec, _env: &NftRuleEnv) -> Result<(), Error> { > - log::trace!("applying ports: {self:?}"); > - > - for rule in rules { > - if let Some(sport) = self.sport() { > - log::trace!("applying sport: {sport:?}"); > - > - rule.push( > - Match::new_eq( > - Expression::from(Payload::field("th", "sport")), > - Expression::from(sport), > - ) > - .into(), > - ) > + for statement in protocol::matches(self) { > + rule.push(statement); > } nit: could be extend? generally the whole function reads a bit awkward to me, but I cant think of anything better on the spot. > - > - if let Some(dport) = self.dport() { > - log::trace!("applying dport: {dport:?}"); > - > - rule.push( > - Match::new_eq( > - Expression::from(Payload::field("th", "dport")), > - Expression::from(dport), > - ) > - .into(), > - ) > + if let Some(family) = family { > + rule.set_family(family); > } > } > > diff --git a/proxmox-nftables/src/lib.rs b/proxmox-nftables/src/lib.rs > index 2003e1b..c54905b 100644 > --- a/proxmox-nftables/src/lib.rs > +++ b/proxmox-nftables/src/lib.rs > @@ -2,6 +2,7 @@ pub mod client; > pub mod command; > pub mod expression; > pub mod helper; > +pub mod protocol; > pub mod statement; > pub mod types; > > diff --git a/proxmox-nftables/src/protocol.rs b/proxmox-nftables/src/protocol.rs > new file mode 100644 > index 0000000..fcd657f > --- /dev/null > +++ b/proxmox-nftables/src/protocol.rs > @@ -0,0 +1,79 @@ > +//! Rendering of a transport protocol match into the statements that select it, which are > +//! `meta l4proto` for the protocol, the transport-header ports, and the ICMP type and code. > +//! The statements carry no IP family test of their own, so they work unchanged in the inet > +//! and bridge families. An ICMP match still only applies to its own IP family, which is left > +//! to the caller. > + > +#[cfg(feature = "config-ext")] > +use proxmox_ve_config::firewall::types::rule_match::{Ports, Protocol}; > + > +use crate::expression::{Meta, Payload}; > +use crate::statement::Match; > +use crate::{Expression, Statement}; > + > +/// Selects a protocol by name or number. > +pub fn l4proto(protocol: impl Into) -> Statement { > + Match::new_eq(Meta::new("l4proto"), protocol.into()).into() > +} > + > +/// Selects the transport-header source and destination ports that are given. > +pub fn ports(sport: Option, dport: Option) -> Vec { > + let mut statements = Vec::new(); > + if let Some(sport) = sport { > + statements.push(Match::new_eq(Payload::field("th", "sport"), sport).into()); > + } > + if let Some(dport) = dport { > + statements.push(Match::new_eq(Payload::field("th", "dport"), dport).into()); > + } > + statements > +} > + > +/// Selects an ICMP flavour by type and code. Matching either already implies the protocol, so > +/// only a bare match needs the explicit `l4proto` test. > +pub fn icmp(protocol: &str, ty: Option, code: Option) -> Vec { > + if ty.is_none() && code.is_none() { > + return vec![l4proto(protocol)]; > + } nit: is it necessary to short circuit rather than just do this and have the two if statements below? let mut statements = vec![l4proto(protocol)]; iirc, nftables simplifies this when creating the rule anyway, so it shouldn't be less efficient in the ruleset: $ nft 'add rule inet test test-chain meta l4proto icmpv6 icmpv6 code 4' $ nft 'list chain inet test test-chain' chain test-chain { icmpv6 code 4 } > + let mut statements = Vec::new(); > + if let Some(ty) = ty { > + statements.push(Match::new_eq(Payload::field(protocol, "type"), ty).into()); > + } > + if let Some(code) = code { > + statements.push(Match::new_eq(Payload::field(protocol, "code"), code).into()); > + } > + statements > +} > + > +/// The statements selecting `protocol`, in evaluation order. > +#[cfg(feature = "config-ext")] > +pub fn matches(protocol: &Protocol) -> Vec { > + match protocol { > + Protocol::Tcp(tcp) => with_ports("tcp", tcp.ports()), > + Protocol::Udp(udp) => with_ports("udp", udp.ports()), > + Protocol::Sctp(sctp) => with_ports("sctp", sctp.ports()), > + Protocol::Dccp(config) => with_ports("dccp", config), > + Protocol::UdpLite(config) => with_ports("udplite", config), > + Protocol::Icmp(config) => icmp( > + "icmp", > + config.ty().map(Expression::from), > + config.code().map(Expression::from), > + ), > + Protocol::Icmpv6(config) => icmp( > + "icmpv6", > + config.ty().map(Expression::from), > + config.code().map(Expression::from), > + ), > + Protocol::Named(name) => vec![l4proto(name.as_str())], > + Protocol::Numeric(id) => vec![l4proto(*id)], > + } > +} > + > +#[cfg(feature = "config-ext")] > +fn with_ports(protocol: &str, config: &Ports) -> Vec { > + let mut statements = vec![l4proto(protocol)]; > + statements.extend(ports( > + config.sport().map(Expression::from), > + config.dport().map(Expression::from), > + )); > + statements > +}