From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A54251FF142 for ; Mon, 02 Mar 2026 13:17:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0EE131D7EA; Mon, 2 Mar 2026 13:18:30 +0100 (CET) Message-ID: <3d87020b-1b14-4302-a63a-6dba50a53dba@proxmox.com> Date: Mon, 2 Mar 2026 13:17:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Stefan Hanreich Subject: Re: [RFC proxmox 10/22] firewall-api-types: add FirewallPortList types To: Dietmar Maurer , pve-devel@lists.proxmox.com References: <20260216104401.3959270-1-dietmar@proxmox.com> <20260216104401.3959270-11-dietmar@proxmox.com> Content-Language: en-US In-Reply-To: <20260216104401.3959270-11-dietmar@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.341 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.012 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 1.188 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.93 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 Message-ID-Hash: JFFUGXUTZBRIPRWV76UE3JHEQZPFISIS X-Message-ID-Hash: JFFUGXUTZBRIPRWV76UE3JHEQZPFISIS 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: comments inline On 2/16/26 11:46 AM, Dietmar Maurer wrote: > Add new port specification types for firewall rules: > - FirewallPortListEntry: single numeric port, numeric port range or named service > - FirewallPortList: comma-separated list of port entries > > Includes comprehensive parsing with validation and unit tests. > > Signed-off-by: Dietmar Maurer > --- > proxmox-firewall-api-types/Cargo.toml | 1 + > proxmox-firewall-api-types/src/lib.rs | 5 + > proxmox-firewall-api-types/src/port.rs | 173 +++++++++++++++++++++++++ > 3 files changed, 179 insertions(+) > create mode 100644 proxmox-firewall-api-types/src/port.rs > > diff --git a/proxmox-firewall-api-types/Cargo.toml b/proxmox-firewall-api-types/Cargo.toml > index 3122d813..97b477b8 100644 > --- a/proxmox-firewall-api-types/Cargo.toml > +++ b/proxmox-firewall-api-types/Cargo.toml > @@ -15,6 +15,7 @@ enum-fallback = ["dep:proxmox-fixed-string"] > [dependencies] > anyhow.workspace = true > regex.workspace = true > +const_format.workspace = true > > serde = { workspace = true, features = [ "derive" ] } > serde_plain = { workspace = true } > diff --git a/proxmox-firewall-api-types/src/lib.rs b/proxmox-firewall-api-types/src/lib.rs > index 993115d8..b099be0c 100644 > --- a/proxmox-firewall-api-types/src/lib.rs > +++ b/proxmox-firewall-api-types/src/lib.rs > @@ -20,3 +20,8 @@ pub use node_options::FirewallNodeOptions; > > mod firewall_ref; > pub use firewall_ref::{FirewallRef, FirewallRefType}; > + > +mod port; > +pub use port::{ > + FirewallPortList, FirewallPortListEntry, FIREWALL_DPORT_API_SCHEMA, FIREWALL_SPORT_API_SCHEMA, > +}; > diff --git a/proxmox-firewall-api-types/src/port.rs b/proxmox-firewall-api-types/src/port.rs > new file mode 100644 > index 00000000..46989ba4 > --- /dev/null > +++ b/proxmox-firewall-api-types/src/port.rs > @@ -0,0 +1,173 @@ > +use std::fmt::Display; > +use std::str::FromStr; > + > +use anyhow::{bail, Error}; > +use const_format::concatcp; > +use proxmox_schema::{ApiStringFormat, Schema, StringSchema, UpdaterType}; > + > +#[derive(Clone, Debug, PartialEq)] > +/// Single entry in a TCP/UDP port list. > +/// > +/// Can be a named service, a numeric port or a port range. > +pub enum FirewallPortListEntry { > + Named(String), > + Numeric(u16), > + Range(u16, u16), > +} > + > +impl FromStr for FirewallPortListEntry { > + type Err = Error; > + fn from_str(s: &str) -> Result { > + Ok(match s.trim().split_once(':') { > + None => { > + if s.is_empty() { > + bail!("empty port specification"); > + } > + if s.find(|c: char| !(c.is_digit(10))).is_some() { nit: .chars().any() is potentially clearer here? There's also `is_ascii_digit` which might be better suited than is_digit. > + // Note: arbitrary length limit, longer than anything in /etc/services > + if s.len() < 256 { > + if s.contains(|c: char| !(c.is_ascii_alphanumeric() || c == '-')) { > + bail!("invalid characters in port name"); > + } > + FirewallPortListEntry::Named(s.to_string()) > + } else { > + bail!("port name too long"); > + } > + } else { > + let port = s.parse::()?; > + FirewallPortListEntry::Numeric(port) > + } > + } > + Some((first, second)) => { > + let first = first.parse::()?; > + let second = second.parse::()?; > + if first > second { > + bail!("invalid port range: start port greater than end port") > + } > + FirewallPortListEntry::Range(first, second) > + } > + }) > + } > +} > + > +impl Display for FirewallPortListEntry { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + match self { > + FirewallPortListEntry::Named(name) => write!(f, "{}", name), > + FirewallPortListEntry::Numeric(number) => write!(f, "{}", number), > + FirewallPortListEntry::Range(first, second) => write!(f, "{}:{}", first, second), > + } > + } > +} > + > +#[derive(Clone, Debug, PartialEq)] > +/// TCP/UDP port list. nit: could consider using #[repr(transparent)] - but doesn't really matter for API types.. > +pub struct FirewallPortList(pub Vec); > + > +const PORT_FORMAT_DESCRIPTION: &'static str = r#"You can use service names or simple numbers (0-65535), > +as defined in '/etc/services'. Port ranges can be specified with '\d+:\d+', > +for example '80:85', and you can use comma separated list to match several ports or ranges."#; > + > +/// API schema for firewall source port list. > +pub const FIREWALL_SPORT_API_SCHEMA: Schema = StringSchema::new(concatcp!( > + "Restrict TCP/UDP source port. ", > + PORT_FORMAT_DESCRIPTION > +)) > +.format(&ApiStringFormat::VerifyFn(verify_firewall_port_list)) > +.schema(); > + > +/// API schema for firewall destination port list. > +pub const FIREWALL_DPORT_API_SCHEMA: Schema = StringSchema::new(concatcp!( > + "Restrict TCP/UDP destination port. ", > + PORT_FORMAT_DESCRIPTION > +)) > +.format(&ApiStringFormat::VerifyFn(verify_firewall_port_list)) > +.schema(); > + > +serde_plain::derive_deserialize_from_fromstr!(FirewallPortList, "valid port list"); > +serde_plain::derive_serialize_from_display!(FirewallPortList); > + > +impl FromStr for FirewallPortList { > + type Err = Error; > + fn from_str(s: &str) -> Result { > + let mut res = Vec::new(); > + for part in s.split(',') { > + let entry = FirewallPortListEntry::from_str(part.trim())?; > + res.push(entry); > + } > + Ok(FirewallPortList(res)) > + } > +} Generally collection types (like FirewallPortList) should implement FromIterator and implementing it would simplify the FromStr implementation: fn from_str(s: &str) -> Result { s.split(',').map(FirewallPortListEntry::from_str).collect() } > + > +impl Display for FirewallPortList { > + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { > + for (i, entry) in self.0.iter().enumerate() { > + if i > 0 { > + write!(f, ",")?; > + } > + write!(f, "{}", entry)?; > + } > + Ok(()) > + } > +} > + > +fn verify_firewall_port_list(s: &str) -> Result<(), Error> { > + FirewallPortList::from_str(s)?; > + Ok(()) > +} constructing/deserializing the type parses (and therefore validates) the port list anyway and afaik the verify function doesn't really add anything to the API documentation either (correct me if I am wrong) - so we could skip adding a verification function to the schema entirely? Generally I've encountered this conundrum a few times as well, and I'm unsure about what is the best option generally, since there are mainly two scenarios I can see: Either we use a StringSchema and then deserialize the value into a String - but then we have to take another pass at parsing the list if we want to e.g. generate a firewall rule out of it in the firewall daemon (where we always require the structured format, which then in turn gets morphed into the nftables JSON). Validating the list requires parsing it anyway, so we're doing the work and then throw away the result, requiring us to parse it a second time. So in that case it'd be preferable to be able to deserialize it directly into the structured form. Although the expensive part in that scenario most likely isn't parsing / validating but rather the allocations when constructing the new 'fancy' type (of course depends on the validation logic as well). With this function the validation function would perform all the allocations required for constructing the type - just to throw them away after - so that's quite awkward as well. (unless the compiler is smart enough to optimize that away? don't think so though) In other instances it is perfectly fine to just validate the String and then potentially re-submit it as is (e.g. we could synchronize firewall rulesets across multiple remotes via PDM). Storing it in the structured Rust form would require allocating the newtype and then also allocating a new String when we just want to re-submit the value as-is. In the firewall daemon I *always* deserialize into the structured, 'fancy' rust types when parsing the firewall configuration since we always require them in the firewall daemon anyway. I also prefer to use the structured types internally, since it makes function signatures a lot clearer, doesn't clutter the logic with the validation + error-handling dance and I can rely on the fact that the data I get passed is validated by definition. This allows for earlier failure as well (which can be a downside as well though if not handled correctly - see IPSet/Alias scopes issues in proxmox-firewall for instance). We could consider having primitive types with just validation + more structured types and convert between them via TryFrom? Introduces a lot of additional types and boilerplate into the codebase as downside. Or we could consider storing the original value somewhere and use that in cases where we just want the string (but that doesn't avoid the additional allocations...). > +#[cfg(test)] > +mod tests { > + use super::*; > + > + #[test] > + fn test_parse_port_entry() { > + let mut port_entry: FirewallPortListEntry = "12345".parse().expect("valid port entry"); > + assert_eq!(port_entry, FirewallPortListEntry::Numeric(12345)); > + > + port_entry = "0:65535".parse().expect("valid port entry"); > + assert_eq!(port_entry, FirewallPortListEntry::Range(0, 65535)); > + > + "ssh:80".parse::().unwrap_err(); > + "65536".parse::().unwrap_err(); > + "100:80".parse::().unwrap_err(); > + "100:100000".parse::().unwrap_err(); > + "any-name".parse::().unwrap(); > + "TOS-network-unreachable" > + .parse::() > + .unwrap(); > + "no_underscores" > + .parse::() > + .unwrap_err(); > + "imap2".parse::().unwrap(); > + "".parse::().unwrap_err(); > + } > + > + #[test] > + fn test_parse_port_list() { > + let mut port_list = FirewallPortList::from_str("12345").expect("valid port list"); > + assert_eq!( > + port_list, > + FirewallPortList(vec![FirewallPortListEntry::Numeric(12345)]) > + ); > + > + port_list = > + FirewallPortList::from_str("12345,0:65535,1337,https").expect("valid port list"); > + > + assert_eq!( > + port_list, > + FirewallPortList(vec![ > + FirewallPortListEntry::from_str("12345").unwrap(), > + FirewallPortListEntry::from_str("0:65535").unwrap(), > + FirewallPortListEntry::from_str("1337").unwrap(), > + FirewallPortListEntry::from_str("https").unwrap(), > + ]) > + ); > + > + FirewallPortList::from_str("0::1337").unwrap_err(); > + FirewallPortList::from_str("0:1337,").unwrap_err(); > + FirewallPortList::from_str("70000").unwrap_err(); > + FirewallPortList::from_str("ssh:80").unwrap_err(); > + FirewallPortList::from_str("").unwrap_err(); > + } > +}