Re: [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib

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

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 <h.laimer@proxmox.com>
> ---
> 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<NftRule>, 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<NftRule>, 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<NftRule>, 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<NftRule>, 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<NftRule>, _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<NftRule>, _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<NftRule>, _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<Expression>) -> 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<Expression>, dport: Option<Expression>) -> Vec<Statement> {
> +    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<Expression>, code: Option<Expression>) -> Vec<Statement> {
> +    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<Statement> {
> +    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<Statement> {
> +    let mut statements = vec![l4proto(protocol)];
> +    statements.extend(ports(
> +        config.sport().map(Expression::from),
> +        config.dport().map(Expression::from),
> +    ));
> +    statements
> +}