[PATCH proxmox-ve-rs v3 09/45] ve-config: sdn: microseg: narrow rules by protocol and ports

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

A rule can be narrowed to a protocol, optionally with ports or an
ICMP type, or to a firewall macro standing in for them. It then
decides only the traffic it matches.

For a pair, the highest priority with a rule for the whole pair gives
the verdict. Narrowed rules down to that priority are checked first,
in priority order. So a narrowed rule makes an exception to a
whole-pair rule of the same priority. The model also reports each
rule's place in the evaluation order, so a caller that lists the
rules does not have to work the order out again.

What a rule narrows to is part of its id, so two rules on the same
predicates can coexist. Like the predicates, it cannot be changed
after creation. A both-ways rule matches the reverse direction with
the ports swapped. It cannot be narrowed to an ICMP type, since the
reverse type cannot be derived in general.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 proxmox-ve-config/src/firewall/fw_macros.rs |   7 +
 proxmox-ve-config/src/firewall/ports.rs     |  65 +++++
 proxmox-ve-config/src/sdn/microseg/api.rs   |  64 ++++-
 proxmox-ve-config/src/sdn/microseg/mod.rs   | 266 ++++++++++++++++++--
 proxmox-ve-config/src/sdn/microseg/types.rs | 116 ++++++++-
 5 files changed, 496 insertions(+), 22 deletions(-)

diff --git a/proxmox-ve-config/src/firewall/fw_macros.rs b/proxmox-ve-config/src/firewall/fw_macros.rs
index 5fa8dab1ca26..e0bdf365635b 100644
--- a/proxmox-ve-config/src/firewall/fw_macros.rs
+++ b/proxmox-ve-config/src/firewall/fw_macros.rs
@@ -67,3 +67,10 @@ fn macros() -> &'static HashMap<String, FwMacro> {
 pub fn get_macro(name: &str) -> Option<&'static FwMacro> {
     macros().get(name)
 }
+
+pub fn macro_name(name: &str) -> Option<&'static str> {
+    macros()
+        .keys()
+        .find(|known| known.eq_ignore_ascii_case(name))
+        .map(String::as_str)
+}
diff --git a/proxmox-ve-config/src/firewall/ports.rs b/proxmox-ve-config/src/firewall/ports.rs
index f88a442adab4..62d3bacda228 100644
--- a/proxmox-ve-config/src/firewall/ports.rs
+++ b/proxmox-ve-config/src/firewall/ports.rs
@@ -78,3 +78,68 @@ pub fn parse_named_port(name: &str) -> Result<u16, Error> {
         .find(name)
         .ok_or_else(|| format_err!("unknown port name {name:?}"))
 }
+
+#[derive(Default)]
+struct NamedProtocols {
+    protocols: std::collections::HashMap<String, u8>,
+}
+
+impl NamedProtocols {
+    fn new() -> Self {
+        use std::io::BufRead;
+
+        log::trace!("loading /etc/protocols");
+
+        let mut this = Self::default();
+
+        let file = match std::fs::File::open("/etc/protocols") {
+            Ok(file) => file,
+            Err(_) => return this,
+        };
+
+        for line in std::io::BufReader::new(file).lines() {
+            let line = match line {
+                Ok(line) => line,
+                Err(_) => break,
+            };
+
+            let line = line.split_once('#').map_or(line.as_str(), |(text, _)| text);
+
+            let mut parts = line.split_ascii_whitespace();
+
+            let (name, number) = match (parts.next(), parts.next()) {
+                (Some(name), Some(number)) => (name, number),
+                _ => continue,
+            };
+
+            let number: u8 = match number.parse() {
+                Ok(number) => number,
+                Err(_) => continue,
+            };
+
+            // the first line naming a protocol wins, as for getprotobyname
+            for name in std::iter::once(name).chain(parts) {
+                this.protocols.entry(name.to_string()).or_insert(number);
+            }
+        }
+
+        this
+    }
+
+    fn find(&self, name: &str) -> Option<u8> {
+        self.protocols.get(name).copied()
+    }
+}
+
+fn named_protocols() -> &'static NamedProtocols {
+    static NAMED_PROTOCOLS: OnceLock<NamedProtocols> = OnceLock::new();
+
+    NAMED_PROTOCOLS.get_or_init(NamedProtocols::new)
+}
+
+/// Parse a named protocol with the help of `/etc/protocols`.
+pub fn parse_named_protocol(name: &str) -> Result<u8, Error> {
+    named_protocols()
+        .find(name)
+        .ok_or_else(|| format_err!("unknown protocol name {name:?}"))
+}
diff --git a/proxmox-ve-config/src/sdn/microseg/api.rs b/proxmox-ve-config/src/sdn/microseg/api.rs
index 08b5ee1f4653..fe125c4d4c1b 100644
--- a/proxmox-ve-config/src/sdn/microseg/api.rs
+++ b/proxmox-ve-config/src/sdn/microseg/api.rs
@@ -8,6 +8,8 @@ use anyhow::{Error, bail};
 use proxmox_section_config::typed::ApiSectionDataEntry;
 use serde::Deserialize;
 
+use crate::firewall::fw_macros;
+
 use super::types::{
     Binding, GroupSection, GuestAssignmentSection, GuestInfo, MicrosegEntry, MicrosegId,
     MicrosegRunningConfig, RegexAssignmentSection, RuleSection, Tag, TagAssignmentSection,
@@ -52,6 +54,16 @@ pub struct RuleCreate {
     #[serde(deserialize_with = "proxmox_serde::perl::deserialize_bool")]
     pub allow: bool,
     #[serde(default)]
+    pub proto: Option<String>,
+    #[serde(default)]
+    pub sport: Option<String>,
+    #[serde(default)]
+    pub dport: Option<String>,
+    #[serde(default, rename = "icmp-type")]
+    pub icmp_type: Option<String>,
+    #[serde(default, rename = "macro")]
+    pub fw_macro: Option<String>,
+    #[serde(default)]
     pub comment: Option<String>,
 }
 
@@ -195,6 +207,7 @@ fn rule_id(
     src_groups: &[&str],
     dst_match: PredicateMatch,
     dst_groups: &[&str],
+    narrowing: &[Option<&str>],
     bidirectional: bool,
 ) -> Result<MicrosegId, Error> {
     let mut hash = Fnv1a::new();
@@ -202,6 +215,9 @@ fn rule_id(
     hash.groups(src_groups);
     hash.match_kind(dst_match);
     hash.groups(dst_groups);
+    for field in narrowing {
+        hash.text(field.unwrap_or_default());
+    }
     hash.flag(bidirectional);
     format!("r{:016x}", hash.0).parse()
 }
@@ -289,11 +305,30 @@ fn build_entry(
         MicrosegCreate::Rule(rule) => {
             let src_groups = sorted_groups(&rule.src, entries)?;
             let dst_groups = sorted_groups(&rule.dst, entries)?;
+            let trimmed = |field: Option<String>| field.map(|value| value.trim().to_string());
+            let proto = trimmed(rule.proto);
+            let sport = trimmed(rule.sport);
+            let dport = trimmed(rule.dport);
+            let icmp_type = trimmed(rule.icmp_type);
+            // the expansion looks a macro up by its exact name, so the table's spelling is
+            // stored and an unknown name stays as given for validation to refuse
+            let fw_macro = trimmed(rule.fw_macro).map(|name| match fw_macros::macro_name(&name) {
+                Some(known) => known.to_string(),
+                None => name,
+            });
+            let narrowing = [
+                proto.as_deref(),
+                sport.as_deref(),
+                dport.as_deref(),
+                icmp_type.as_deref(),
+                fw_macro.as_deref(),
+            ];
             let id = rule_id(
                 rule.src_match,
                 &src_groups,
                 rule.dst_match,
                 &dst_groups,
+                &narrowing,
                 rule.bidirectional,
             )?;
             MicrosegEntry::Rule(RuleSection {
@@ -305,6 +340,11 @@ fn build_entry(
                 bidirectional: rule.bidirectional,
                 prio: rule.prio,
                 allow: rule.allow,
+                proto,
+                sport,
+                dport,
+                icmp_type,
+                fw_macro,
                 comment: rule.comment,
             })
         }
@@ -540,13 +580,33 @@ fn predicate_str(match_kind: PredicateMatch, groups: &[impl AsRef<str>]) -> Stri
     format!("{kind}{{{list}}}")
 }
 
+/// Rendered from the stored fields, since expanding a macro can fail and its name is what a user
+/// recognizes.
+fn narrowing_str(rule: &RuleSection) -> Option<String> {
+    if let Some(name) = &rule.fw_macro {
+        return Some(format!("macro {name}"));
+    }
+    let parts = [
+        rule.proto.clone(),
+        rule.sport.as_ref().map(|ports| format!("sport {ports}")),
+        rule.dport.as_ref().map(|ports| format!("dport {ports}")),
+        rule.icmp_type.as_ref().map(|kind| format!("type {kind}")),
+    ];
+    let narrowing = parts.into_iter().flatten().collect::<Vec<_>>().join(" ");
+    (!narrowing.is_empty()).then_some(narrowing)
+}
+
 fn describe_rule(rule: &RuleSection) -> String {
     let arrow = if rule.bidirectional { "<->" } else { "->" };
-    format!(
+    let base = format!(
         "rule {} {arrow} {}",
         predicate_str(rule.src_match, &rule.src),
         predicate_str(rule.dst_match, &rule.dst),
-    )
+    );
+    match narrowing_str(rule) {
+        Some(narrowing) => format!("{base} ({narrowing})"),
+        None => base,
+    }
 }
 
 /// A human-readable description of an entry for diagnostics, by what a user would recognize rather
diff --git a/proxmox-ve-config/src/sdn/microseg/mod.rs b/proxmox-ve-config/src/sdn/microseg/mod.rs
index 620f342ad2b0..3aaf299f8169 100644
--- a/proxmox-ve-config/src/sdn/microseg/mod.rs
+++ b/proxmox-ve-config/src/sdn/microseg/mod.rs
@@ -1,12 +1,12 @@
 //! Microseg subsystem of the SDN config.
 //!
 //! Microseg is an identity-based stateless firewall. Each managed guest NIC gets the *set* of
-//! *groups* of the first *assignment* in admin order that covers it. The render allocates a wire
+//! *groups* of the first *assignment* in match order that covers it. The render allocates a wire
 //! identity for the set of every assignment (see [`identity`]), and the running config carries it
 //! to every host, so a NIC can only land in a class that already exists. Each host looks the
-//! identities up and compiles the policy *rules* into a `(src_id, dst_id) -> allow|deny` map. A
-//! rule matches traffic by a `src` and a `dst` *predicate*, each a match kind over a list of
-//! groups.
+//! identities up and folds the policy *rules* per `(src_id, dst_id)` pair. The fold yields the
+//! narrowed rules to check and the verdict for the traffic none of them matches. A rule matches
+//! traffic by a `src` and a `dst` *predicate*, each a match kind over a list of groups.
 
 pub mod api;
 pub mod identity;
@@ -18,6 +18,7 @@ pub use types::{
     MicrosegRunningConfig, RegexAssignmentSection, RuleSection, Tag, TagAssignmentSection,
 };
 
+use std::cmp::Ordering;
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 
 use anyhow::{Context, Error, bail};
@@ -25,6 +26,8 @@ use identity::UNTAGGED_GROUP;
 use regex::Regex;
 use serde::Serialize;
 
+use crate::firewall::types::rule_match::{IcmpType, Icmpv6Type, Ports, Protocol, Sctp, Tcp, Udp};
+
 /// Validate a microseg config before anything renders from it, so resolving group names and
 /// expanding rules cannot fail further down.
 pub fn validate(entries: &BTreeMap<String, MicrosegEntry>) -> Result<(), Error> {
@@ -86,6 +89,14 @@ fn validate_entry(
     Ok(())
 }
 
+fn has_icmp_type(protocol: &Protocol) -> bool {
+    match protocol {
+        Protocol::Icmp(icmp) => !matches!(icmp.ty(), None | Some(IcmpType::Any)),
+        Protocol::Icmpv6(icmp) => !matches!(icmp.ty(), None | Some(Icmpv6Type::Any)),
+        _ => false,
+    }
+}
+
 fn validate_rule(
     entries: &BTreeMap<String, MicrosegEntry>,
     subject: &str,
@@ -133,6 +144,18 @@ fn validate_rule(
     for group in rule.src.iter().chain(&rule.dst) {
         known_group(entries, group.as_ref()).with_context(|| subject.to_string())?;
     }
+    let protocols = match rule.protocols() {
+        Ok(protocols) => protocols,
+        Err(err) => bail!("{subject}: {err}"),
+    };
+    // a macro can expand to an entry with an ICMP type, so the check reads the expanded
+    // matches and not the rule's own field
+    if rule.bidirectional && protocols.iter().any(has_icmp_type) {
+        bail!(
+            "{subject} is bidirectional and names an ICMP type, the mirror does not \
+             derive a reverse type"
+        );
+    }
     Ok(())
 }
 
@@ -405,6 +428,31 @@ pub fn render(
     })
 }
 
+fn evaluation_order(a: &ResolvedRule, b: &ResolvedRule) -> Ordering {
+    b.prio
+        .cmp(&a.prio)
+        .then(b.is_narrowed().cmp(&a.is_narrowed()))
+        .then(a.allow.cmp(&b.allow))
+        // the id keeps the order stable, and the orientation after it, since both orientations of
+        // a bidirectional rule carry the same id
+        .then(a.id.cmp(&b.id))
+        .then(a.reverse.cmp(&b.reverse))
+}
+
+/// Each rule's place in the evaluation order, by rule id. The first has the lowest number.
+pub fn rule_order(
+    entries: &BTreeMap<String, MicrosegEntry>,
+) -> Result<HashMap<MicrosegId, usize>, Error> {
+    let mut rules = resolved_rules(entries)?;
+    rules.sort_by(evaluation_order);
+    let mut order = HashMap::new();
+    for rule in rules {
+        let place = order.len();
+        order.entry(rule.id).or_insert(place);
+    }
+    Ok(order)
+}
+
 /// Whether a guest renamed from `old` to `new` enters or leaves the names a regex assignment
 /// matches, decided by the same expressions that resolve the NICs.
 pub fn name_match_changed(
@@ -439,10 +487,35 @@ pub fn assignment_order(entries: &BTreeMap<String, MicrosegEntry>) -> HashMap<St
 
 #[derive(Debug)]
 pub struct ResolvedRule {
+    id: MicrosegId,
     src: identity::Predicate,
     dst: identity::Predicate,
     prio: u16,
     allow: bool,
+    /// The protocol matches narrowing the rule. Empty for a rule deciding the whole pair.
+    protocols: Vec<Protocol>,
+    /// Set on the mirrored orientation of a bidirectional rule.
+    reverse: bool,
+}
+
+impl ResolvedRule {
+    fn is_narrowed(&self) -> bool {
+        !self.protocols.is_empty()
+    }
+}
+
+fn reversed_protocol(protocol: &Protocol) -> Protocol {
+    let swapped = |ports: &Ports| Ports::new(ports.dport().cloned(), ports.sport().cloned());
+    match protocol {
+        Protocol::Dccp(ports) => Protocol::Dccp(swapped(ports)),
+        Protocol::Sctp(sctp) => Protocol::Sctp(Sctp::new(swapped(sctp.ports()))),
+        Protocol::Tcp(tcp) => Protocol::Tcp(Tcp::new(swapped(tcp.ports()))),
+        Protocol::Udp(udp) => Protocol::Udp(Udp::new(swapped(udp.ports()))),
+        Protocol::UdpLite(ports) => Protocol::UdpLite(swapped(ports)),
+        Protocol::Icmp(_) | Protocol::Icmpv6(_) | Protocol::Named(_) | Protocol::Numeric(_) => {
+            protocol.clone()
+        }
+    }
 }
 
 pub fn resolved_rules(
@@ -452,16 +525,22 @@ pub fn resolved_rules(
     for rule in rule_entries(entries) {
         let context = || format!("rule '{}'", rule.id);
         let forward = ResolvedRule {
+            id: rule.id.clone(),
             src: predicate(entries, rule.src_match, &rule.src).with_context(context)?,
             dst: predicate(entries, rule.dst_match, &rule.dst).with_context(context)?,
             prio: rule.prio,
             allow: rule.allow,
+            protocols: rule.protocols().with_context(context)?,
+            reverse: false,
         };
         let mirrored = rule.bidirectional.then(|| ResolvedRule {
+            id: forward.id.clone(),
             src: forward.dst.clone(),
             dst: forward.src.clone(),
             prio: forward.prio,
             allow: forward.allow,
+            protocols: forward.protocols.iter().map(reversed_protocol).collect(),
+            reverse: true,
         });
         resolved.push(forward);
         resolved.extend(mirrored);
@@ -469,26 +548,39 @@ pub fn resolved_rules(
     Ok(resolved)
 }
 
-/// Fold the rules that fire on one `(src_class, dst_class)` pair into its verdict. The highest
-/// priority wins, and a single deny among the rules tied there overrides the allows. Without a
-/// firing rule the pair denies.
-pub fn fold<'a>(firing: impl IntoIterator<Item = &'a ResolvedRule>) -> bool {
-    let mut best: Option<(u16, bool)> = None;
-    for rule in firing {
-        best = Some(match best {
-            Some((prio, _)) if rule.prio > prio => (rule.prio, rule.allow),
-            Some((prio, allow)) if rule.prio == prio => (prio, allow && rule.allow),
-            Some(current) => current,
-            None => (rule.prio, rule.allow),
-        });
+#[derive(Debug)]
+pub struct Folded<'a> {
+    /// Narrowed rules in evaluation order. The first one whose protocol match fires decides.
+    pub checks: Vec<&'a ResolvedRule>,
+    pub allow: bool,
+}
+
+/// Fold the rules that fire on one `(src_class, dst_class)` pair into its verdict and checks. The
+/// verdict comes from the highest priority with a rule that decides the whole pair, and a single
+/// deny among those rules wins. Without such a rule the pair denies. The checks are the narrowed
+/// rules from the top priority down to that one, denies ahead of allows within a priority.
+pub fn fold<'a>(firing: impl IntoIterator<Item = &'a ResolvedRule>) -> Folded<'a> {
+    let mut firing: Vec<&ResolvedRule> = firing.into_iter().collect();
+    firing.sort_by(|a, b| evaluation_order(a, b));
+
+    let mut checks = Vec::new();
+    let mut allow = false;
+    for level in firing.chunk_by(|a, b| a.prio == b.prio) {
+        checks.extend(level.iter().filter(|rule| rule.is_narrowed()));
+        let mut deciding = level.iter().filter(|rule| !rule.is_narrowed()).peekable();
+        if deciding.peek().is_some() {
+            allow = deciding.all(|rule| rule.allow);
+            break;
+        }
     }
-    best.is_some_and(|(_, allow)| allow)
+    Folded { checks, allow }
 }
 
 #[cfg(test)]
 mod tests {
 
     use super::*;
+    use crate::firewall::types::rule_match::RuleOptions;
 
     pub(super) fn firing_rules<'a>(
         rules: &'a [ResolvedRule],
@@ -679,7 +771,9 @@ mod tests {
     }
 
     fn verdict(rules: &[ResolvedRule], src: &identity::GroupSet, dst: &identity::GroupSet) -> bool {
-        fold(firing_rules(rules, src, dst))
+        let folded = fold(firing_rules(rules, src, dst));
+        assert!(folded.checks.is_empty(), "no narrowed rules expected");
+        folded.allow
     }
 
     fn rule_full(
@@ -702,6 +796,11 @@ mod tests {
                 bidirectional: false,
                 prio,
                 allow,
+                proto: None,
+                sport: None,
+                dport: None,
+                icmp_type: None,
+                fw_macro: None,
                 comment: None,
             }),
         )
@@ -742,4 +841,135 @@ mod tests {
         // from an interface in web alone only the allow fires
         assert!(verdict(&tied_rules, &set(&["web"]), &set(&["db"])));
     }
+
+    fn narrowed(
+        name: &str,
+        src: &[&str],
+        dst: &[&str],
+        prio: u16,
+        allow: bool,
+        proto: Option<&str>,
+        dport: Option<&str>,
+    ) -> (String, MicrosegEntry) {
+        let entry = rule_full(
+            name,
+            src,
+            PredicateMatch::Any,
+            dst,
+            PredicateMatch::Any,
+            prio,
+            allow,
+        );
+        with_rule(entry, |rule| {
+            rule.proto = proto.map(str::to_string);
+            rule.dport = dport.map(str::to_string);
+        })
+    }
+
+    #[test]
+    fn cell_orders_checks_and_decides_the_rest() {
+        let entries: BTreeMap<String, MicrosegEntry> = BTreeMap::from([
+            group("web"),
+            group("db"),
+            rule_full(
+                "whole",
+                &["web"],
+                PredicateMatch::Any,
+                &["db"],
+                PredicateMatch::Any,
+                0,
+                true,
+            ),
+            narrowed("ssh", &["web"], &["db"], 10, false, Some("tcp"), Some("22")),
+            narrowed("dns", &["web"], &["db"], 10, true, Some("udp"), Some("53")),
+            narrowed("http", &["web"], &["db"], 20, true, Some("tcp"), Some("80")),
+        ]);
+        let rules = resolved(&entries);
+        let folded = fold(firing_rules(&rules, &set(&["web"]), &set(&["db"])));
+        let order: Vec<&str> = folded.checks.iter().map(|r| r.id.as_ref()).collect();
+        assert_eq!(
+            order,
+            ["http", "ssh", "dns"],
+            "prio first, then deny before allow"
+        );
+        assert!(folded.allow, "the whole-pair allow decides the rest");
+
+        // A whole-pair deny at prio 15 ends the walk before the narrowed rules below it.
+        let mut cut = entries.clone();
+        cut.extend([rule_full(
+            "cut",
+            &["web"],
+            PredicateMatch::Any,
+            &["db"],
+            PredicateMatch::Any,
+            15,
+            false,
+        )]);
+        let rules = resolved(&cut);
+        let folded = fold(firing_rules(&rules, &set(&["web"]), &set(&["db"])));
+        let order: Vec<&str> = folded.checks.iter().map(|r| r.id.as_ref()).collect();
+        assert_eq!(order, ["http"]);
+        assert!(!folded.allow);
+
+        // Only narrowed rules, so the rest is denied.
+        let mut only = entries.clone();
+        only.remove("whole");
+        let rules = resolved(&only);
+        let folded = fold(firing_rules(&rules, &set(&["web"]), &set(&["db"])));
+        assert_eq!(folded.checks.len(), 3);
+        assert!(!folded.allow);
+    }
+
+    #[test]
+    fn bidirectional_narrowing_swaps_the_ports() {
+        let tcp = |sport: Option<&str>, dport: Option<&str>| {
+            Protocol::from_options(&RuleOptions {
+                proto: Some("tcp".to_string()),
+                sport: sport.map(str::to_string),
+                dport: dport.map(str::to_string),
+                ..Default::default()
+            })
+            .expect("valid options")
+            .expect("a protocol")
+        };
+        let check = |rules: &[ResolvedRule], src: &str, dst: &str| {
+            let folded = fold(firing_rules(rules, &set(&[src]), &set(&[dst])));
+            assert_eq!(folded.checks.len(), 1, "one check per direction");
+            folded.checks[0].protocols.clone()
+        };
+
+        let explicit = BTreeMap::from([
+            group("web"),
+            group("db"),
+            bidirectional(narrowed(
+                "bsql",
+                &["web"],
+                &["db"],
+                0,
+                true,
+                Some("tcp"),
+                Some("5432"),
+            )),
+        ]);
+        validate(&explicit).expect("valid");
+        let rules = resolved(&explicit);
+        assert_eq!(check(&rules, "web", "db"), [tcp(None, Some("5432"))]);
+        assert_eq!(
+            check(&rules, "db", "web"),
+            [tcp(Some("5432"), None)],
+            "the reverse direction matches the port as a source port"
+        );
+
+        let from_macro = BTreeMap::from([
+            group("web"),
+            group("db"),
+            bidirectional(with_rule(rule("bhttps", &["web"], &["db"], true), |rule| {
+                rule.fw_macro = Some("HTTPS".into())
+            })),
+        ]);
+        validate(&from_macro).expect("valid");
+        let rules = resolved(&from_macro);
+        assert_eq!(check(&rules, "web", "db"), [tcp(None, Some("443"))]);
+        assert_eq!(check(&rules, "db", "web"), [tcp(Some("443"), None)]);
+    }
 }
diff --git a/proxmox-ve-config/src/sdn/microseg/types.rs b/proxmox-ve-config/src/sdn/microseg/types.rs
index 18367490e076..ee0a03beec51 100644
--- a/proxmox-ve-config/src/sdn/microseg/types.rs
+++ b/proxmox-ve-config/src/sdn/microseg/types.rs
@@ -2,12 +2,16 @@
 
 use std::collections::BTreeMap;
 
-use anyhow::{Error, bail};
+use anyhow::{Error, anyhow, bail};
 use const_format::concatcp;
 use serde::{Deserialize, Serialize};
 
 use proxmox_schema::{ApiStringFormat, ApiType, api, api_string_type, const_regex};
 
+use crate::firewall::fw_macros;
+use crate::firewall::ports::parse_named_protocol;
+use crate::firewall::types::rule_match::{Protocol, RuleOptions};
+
 use super::PredicateMatch;
 use super::identity;
 use super::identity::UNTAGGED_GROUP;
@@ -119,6 +123,36 @@ pub struct GroupSection {
             minimum: 0,
             maximum: 65535,
         },
+        proto: {
+            type: String,
+            optional: true,
+            max_length: 32,
+            description: "IP protocol, a name from /etc/protocols or a number.",
+        },
+        sport: {
+            type: String,
+            optional: true,
+            max_length: 128,
+            description: "Source port list, only with a port-carrying protocol.",
+        },
+        dport: {
+            type: String,
+            optional: true,
+            max_length: 128,
+            description: "Destination port list, only with a port-carrying protocol.",
+        },
+        "icmp-type": {
+            type: String,
+            optional: true,
+            max_length: 64,
+            description: "ICMP type, only with an ICMP protocol.",
+        },
+        "macro": {
+            type: String,
+            optional: true,
+            max_length: 128,
+            description: "Firewall macro standing in for the protocol and ports.",
+        },
         comment: {
             type: String,
             optional: true,
@@ -131,6 +165,9 @@ pub struct GroupSection {
 /// A policy rule. It fires on traffic from an interface whose group set matches `(src_match, src)`
 /// to one whose set matches `(dst_match, dst)`, and decides it with `allow`. A rule ahead of it in
 /// the evaluation order decides first. Traffic no rule decides is denied.
+///
+/// A rule may be narrowed to a protocol with ports or an ICMP type, or to a firewall macro that
+/// stands in for them.
 pub struct RuleSection {
     pub(crate) id: MicrosegId,
     /// Groups the source predicate lists.
@@ -145,7 +182,8 @@ pub struct RuleSection {
     /// How the destination predicate matches its groups.
     #[serde(default, rename = "dst-match")]
     pub(crate) dst_match: PredicateMatch,
-    /// Whether the rule also decides the reverse direction, with the predicates swapped.
+    /// Whether the rule also decides the reverse direction, with the predicates and the ports
+    /// swapped.
     #[serde(
         default,
         deserialize_with = "proxmox_serde::perl::deserialize_bool",
@@ -159,9 +197,83 @@ pub struct RuleSection {
     #[serde(deserialize_with = "proxmox_serde::perl::deserialize_bool")]
     pub(crate) allow: bool,
     #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub(crate) proto: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub(crate) sport: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub(crate) dport: Option<String>,
+    #[serde(default, rename = "icmp-type", skip_serializing_if = "Option::is_none")]
+    pub(crate) icmp_type: Option<String>,
+    #[serde(default, rename = "macro", skip_serializing_if = "Option::is_none")]
+    pub(crate) fw_macro: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub(crate) comment: Option<String>,
 }
 
+impl RuleSection {
+    /// The protocol matches narrowing this rule, empty for a rule that decides the whole pair.
+    /// Ports are only accepted with a port-carrying protocol and an ICMP type only with an ICMP
+    /// protocol, since either would otherwise be silently ignored.
+    pub(super) fn protocols(&self) -> Result<Vec<Protocol>, Error> {
+        let explicit = [&self.proto, &self.sport, &self.dport, &self.icmp_type]
+            .iter()
+            .any(|field| field.is_some());
+        if let Some(name) = &self.fw_macro {
+            if explicit {
+                bail!("a macro cannot be combined with a protocol, ports or an ICMP type");
+            }
+            return match fw_macros::get_macro(name) {
+                Some(fw_macro) => Ok(fw_macro.code.clone()),
+                None => bail!("unknown macro '{name}'"),
+            };
+        }
+        let Some(proto) = &self.proto else {
+            if explicit {
+                bail!("ports and an ICMP type need a protocol");
+            }
+            return Ok(Vec::new());
+        };
+        let proto = proto.trim();
+        let options = RuleOptions {
+            proto: (!proto.is_empty()).then(|| proto.to_string()),
+            sport: self.sport.clone(),
+            dport: self.dport.clone(),
+            icmp_type: self.icmp_type.clone(),
+            ..Default::default()
+        };
+        let mut protocol = Protocol::from_options(&options)?
+            .ok_or_else(|| anyhow!("empty protocol specification"))?;
+        // the data plane matches the number, so an unknown name fails validation rather than the
+        // apply
+        if let Protocol::Named(name) = &protocol {
+            let number = parse_named_protocol(name)?.to_string();
+            let options = RuleOptions {
+                proto: Some(number),
+                ..options
+            };
+            protocol = Protocol::from_options(&options)?
+                .ok_or_else(|| anyhow!("empty protocol specification"))?;
+        }
+        let ports = self.sport.is_some() || self.dport.is_some();
+        let carries_ports = matches!(
+            protocol,
+            Protocol::Tcp(_)
+                | Protocol::Udp(_)
+                | Protocol::Sctp(_)
+                | Protocol::Dccp(_)
+                | Protocol::UdpLite(_)
+        );
+        if ports && !carries_ports {
+            bail!("protocol '{proto}' carries no ports");
+        }
+        let icmp = matches!(protocol, Protocol::Icmp(_) | Protocol::Icmpv6(_));
+        if self.icmp_type.is_some() && !icmp {
+            bail!("an ICMP type needs an ICMP protocol, not '{proto}'");
+        }
+        Ok(vec![protocol])
+    }
+}
+
 fn is_false(value: &bool) -> bool {
     !*value
 }
-- 
2.47.3