Re: [RFC proxmox 10/22] firewall-api-types: add FirewallPortList types

[Stefan Hanreich <s.hanreich@proxmox.com>]

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 <dietmar@proxmox.com>
> ---
>  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<Self, Self::Err> {
> +        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::<u16>()?;
> +                    FirewallPortListEntry::Numeric(port)
> +                }
> +            }
> +            Some((first, second)) => {
> +                let first = first.parse::<u16>()?;
> +                let second = second.parse::<u16>()?;
> +                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<FirewallPortListEntry>);
> +
> +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<Self, Self::Err> {
> +        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<Self, Self::Err> {
  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::<FirewallPortListEntry>().unwrap_err();
> +        "65536".parse::<FirewallPortListEntry>().unwrap_err();
> +        "100:80".parse::<FirewallPortListEntry>().unwrap_err();
> +        "100:100000".parse::<FirewallPortListEntry>().unwrap_err();
> +        "any-name".parse::<FirewallPortListEntry>().unwrap();
> +        "TOS-network-unreachable"
> +            .parse::<FirewallPortListEntry>()
> +            .unwrap();
> +        "no_underscores"
> +            .parse::<FirewallPortListEntry>()
> +            .unwrap_err();
> +        "imap2".parse::<FirewallPortListEntry>().unwrap();
> +        "".parse::<FirewallPortListEntry>().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();
> +    }
> +}