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

[Hannes Laimer <h.laimer@proxmox.com>]

thanks for taking a look!
two comments inline, will prepare a v2

On 2026-09-23 12:00, Stefan Hanreich wrote:
> 
> 
> 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>
>> ---

..

>> -
>> -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.
> 


yes, something like

```
         for rule in rules.iter_mut() {
-            if family
-                .zip(rule.family())
-                .is_some_and(|(wanted, pinned)| wanted != pinned)
-            {
-                continue;
-            }
-            for statement in protocol::matches(self) {
-                rule.push(statement);
-            }
-            if let Some(family) = family {
+            if let Some(family) = self.family() {
+                if rule.family().is_some_and(|pinned| pinned != family) {
+                    continue;
+                }
                 rule.set_family(family);
             }
+            rule.extend(protocol::matches(self));
         }
```
seems better


>> -
>> -            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);
>>              }
>>          }
>>  

..

>> +
>> +/// 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?
> 

with `meta l4proto` present nft doesnt add l3 checks that it would
otherwise for an `icmpv6` payload, basically
```
nft add rule inet t c meta l4proto icmpv6 icmpv6 code 4
nft add rule inet t c icmpv6 code 4
```
and `nft --debug=netlink list ruleset` shows the diff

I guess emitting `meta nfproto ipv6` ourself could work as well(?),
but for that we'd have to know the table family here..

> 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
>> +}
> 
> 
> 
> 
>