From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 84D111FF13F for ; Thu, 12 Mar 2026 11:51:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AD27A10FBB; Thu, 12 Mar 2026 11:51:11 +0100 (CET) Date: Thu, 12 Mar 2026 11:51:03 +0100 From: Wolfgang Bumiller To: Dietmar Maurer Subject: Re: [RFC proxmox 11/22] firewall-api-types: add FirewallIcmpType Message-ID: <6ioutptc6rfgjv3zhnocnwyskjrclgbuxpsuf5sdopddrbifyl@u3btu4nzz7re> References: <20260216104401.3959270-1-dietmar@proxmox.com> <20260216104401.3959270-12-dietmar@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260216104401.3959270-12-dietmar@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773312628215 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.083 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_MSPIKE_H2 0.001 Average reputation (+2) 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: ADCOQCCWJUQIGSAYXSGJRVZ66OMEDXFD X-Message-ID-Hash: ADCOQCCWJUQIGSAYXSGJRVZ66OMEDXFD X-MailFrom: w.bumiller@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 CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, Feb 16, 2026 at 11:43:49AM +0100, Dietmar Maurer wrote: > This adds the `FirewallIcmpType` enum, which can represent ICMP types > either as a named variant (using `FirewallIcmpTypeName`) or as a raw > numeric value (u8). > > The `FirewallIcmpTypeName` enum covers standard ICMPv4 and ICMPv6 types, > including deprecated ones and those with codes (e.g., `destination-unreachable`). > > It provides `ipv4()` and `ipv6()` helper methods to check if a specific > named type is valid for the respective protocol. > > Serialization and deserialization are handled via `serde` and `serde_plain`, > allowing for easy conversion to/from strings. > > Includes tests for serialization, deserialization, and protocol validity checks. > --- > proxmox-firewall-api-types/src/icmp_type.rs | 555 ++++++++++++++++++++ > proxmox-firewall-api-types/src/lib.rs | 3 + > 2 files changed, 558 insertions(+) > create mode 100644 proxmox-firewall-api-types/src/icmp_type.rs > > diff --git a/proxmox-firewall-api-types/src/icmp_type.rs b/proxmox-firewall-api-types/src/icmp_type.rs > new file mode 100644 > index 00000000..b45c1505 > --- /dev/null > +++ b/proxmox-firewall-api-types/src/icmp_type.rs > @@ -0,0 +1,555 @@ > +use std::fmt; > + > +use anyhow::{bail, Error}; > +use serde::{Deserialize, Serialize}; > + > +#[cfg(feature = "enum-fallback")] > +use proxmox_fixed_string::FixedString; > + > +use proxmox_schema::{ApiStringFormat, ApiType, Schema, StringSchema}; > + > +#[derive(Debug, Copy, Clone, PartialEq)] > +/// ICMP type, either named or numeric. > +pub enum FirewallIcmpType { > + /// Named ICMP type, e.g. "echo-request" > + Named(FirewallIcmpTypeName), > + /// Numeric ICMP type, e.g. "8" > + Numeric(u8), > +} > + > +impl ApiType for FirewallIcmpType { > + const API_SCHEMA: Schema = StringSchema::new( > + r#"ICMP type, either named or numeric. > + Only valid if proto equals 'icmp' or 'icmpv6'/'ipv6-icmp'."#, ^ as a "raw" string this includes the whitespace here. Just use this a regular string with a backslash after the first line. > + ) > + .format(&ApiStringFormat::VerifyFn(verify_firewall_icmp_type)) > + .schema(); > +} > + > +fn verify_firewall_icmp_type(value: &str) -> Result<(), Error> { > + value.parse::().map(|_| ()) ( .map(drop) is IMO faster to read) > +} > + > +impl std::str::FromStr for FirewallIcmpType { > + type Err = Error; > + > + fn from_str(s: &str) -> Result { > + let s = s.trim(); > + > + if let Ok(ty) = s.parse::() { > + return Ok(Self::Numeric(ty)); > + } > + > + if let Ok(named) = serde_plain::from_str::(s) { > + return Ok(Self::Named(named)); > + } > + > + bail!("{s:?} is not a valid icmp type"); > + } > +} > + > +impl fmt::Display for FirewallIcmpType { > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { > + match self { > + FirewallIcmpType::Numeric(ty) => write!(f, "{ty}"), > + FirewallIcmpType::Named(ty) => write!(f, "{ty}"), > + } > + } > +} > + > +serde_plain::derive_deserialize_from_fromstr!(FirewallIcmpType, "valid icmp type name or number"); > +serde_plain::derive_serialize_from_display!(FirewallIcmpType); > + > +#[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq)] > +#[serde(rename_all = "kebab-case")] > +/// Named ICMP type, e.g. "echo-request". > +pub enum FirewallIcmpTypeName { > + Any, > + > + // IPv4 specific > + HostUnreachable, // 3:1 (ICMP-TYPE, ICMP-CODE) > + ProtocolUnreachable, // 3:2 > + PortUnreachable, // 3:3 > + FragmentationNeeded, // 3:4 > + SourceRouteFailed, // 3:5 > + NetworkUnknown, // 3:6 > + HostUnknown, // 3:7 > + NetworkProhibited, // 3:9 > + HostProhibited, // 3:10 > + #[serde(rename = "TOS-network-unreachable")] > + TOSNetworkUnreachable, // 3:11 > + #[serde(rename = "TOS-host-unreachable")] > + TOSHostUnreachable, // 3:12 > + CommunicationProhibited, // 3:13 > + HostPrecedenceViolation, // 3:14 > + PrecedenceCutoff, // 3:15 > + SourceQuench, // 4 > + NetworkRedirect, // 5:0 > + HostRedirect, // 5:1 > + #[serde(rename = "TOS-network-redirect")] > + TOSNetworkRedirect, // 5:2 > + #[serde(rename = "TOS-host-redirect")] > + TOSHostRedirect, // 5:3 > + TimestampRequest, // 13 > + TimestampReply, // 14 > + AddressMaskRequest, // 17 > + AddressMaskReply, // 18 > + RouterAdvertisement, // 9 > + RouterSolicitation, // 10 > + IpHeaderBad, // 12:0 > + RequiredOptionMissing, // 12:1 > + > + // IPv6 specific > + NoRoute, // 1:0 > + BeyondScope, // 1:2 > + AddressUnreachable, // 1:3 > + FailedPolicy, // 1:5 > + RejectRoute, // 1:6 > + PacketTooBig, // 2 > + BadHeader, // 4:0 > + UnknownHeaderType, // 4:1 > + UnknownOption, // 4:2 > + NeighborSolicitation, // 135 > + NeighbourSolicitation, // 135 > + NeighborAdvertisement, // 136 > + NeighbourAdvertisement, // 136 > + > + // Common > + DestinationUnreachable, // 3:0 (v4), 1:0 (v6) ? v6 has no-route > + NetworkUnreachable, // 3:0 > + EchoReply, // 0 (v4), 129 (v6) > + EchoRequest, // 8 (v4), 128 (v6) > + TimeExceeded, // 11 (v4), 3 (v6) > + TtlZeroDuringTransit, // 11:0 (v4), 3:0 (v6) > + TtlZeroDuringReassembly, // 11:1 (v4), 3:1 (v6) > + ParameterProblem, // 12 (v4), 4 (v6) > + Redirect, // 5 (v4), 137 (v6) > + > + #[cfg(feature = "enum-fallback")] > + #[serde(untagged)] > + /// Unknwon > + UnknownEnumValue(FixedString), > +} > + > +serde_plain::derive_display_from_serialize!(FirewallIcmpTypeName); > +serde_plain::derive_fromstr_from_deserialize!(FirewallIcmpTypeName); > + > +impl FirewallIcmpTypeName { > + pub const fn ipv4(&self) -> bool { > + match self { > + FirewallIcmpTypeName::Any => true, > + FirewallIcmpTypeName::EchoReply => true, > + FirewallIcmpTypeName::DestinationUnreachable => true, > + FirewallIcmpTypeName::NetworkUnreachable => true, > + FirewallIcmpTypeName::HostUnreachable => true, > + FirewallIcmpTypeName::ProtocolUnreachable => true, > + FirewallIcmpTypeName::PortUnreachable => true, > + FirewallIcmpTypeName::FragmentationNeeded => true, > + FirewallIcmpTypeName::SourceRouteFailed => true, > + FirewallIcmpTypeName::NetworkUnknown => true, > + FirewallIcmpTypeName::HostUnknown => true, > + FirewallIcmpTypeName::NetworkProhibited => true, > + FirewallIcmpTypeName::HostProhibited => true, > + FirewallIcmpTypeName::TOSNetworkUnreachable => true, > + FirewallIcmpTypeName::TOSHostUnreachable => true, > + FirewallIcmpTypeName::CommunicationProhibited => true, > + FirewallIcmpTypeName::HostPrecedenceViolation => true, > + FirewallIcmpTypeName::PrecedenceCutoff => true, > + FirewallIcmpTypeName::SourceQuench => true, > + FirewallIcmpTypeName::Redirect => true, > + FirewallIcmpTypeName::NetworkRedirect => true, > + FirewallIcmpTypeName::HostRedirect => true, > + FirewallIcmpTypeName::TOSNetworkRedirect => true, > + FirewallIcmpTypeName::TOSHostRedirect => true, > + FirewallIcmpTypeName::EchoRequest => true, > + FirewallIcmpTypeName::RouterAdvertisement => true, > + FirewallIcmpTypeName::RouterSolicitation => true, > + FirewallIcmpTypeName::TimeExceeded => true, > + FirewallIcmpTypeName::TtlZeroDuringTransit => true, > + FirewallIcmpTypeName::TtlZeroDuringReassembly => true, > + FirewallIcmpTypeName::ParameterProblem => true, > + FirewallIcmpTypeName::IpHeaderBad => true, > + FirewallIcmpTypeName::RequiredOptionMissing => true, > + FirewallIcmpTypeName::TimestampRequest => true, > + FirewallIcmpTypeName::TimestampReply => true, > + FirewallIcmpTypeName::AddressMaskRequest => true, > + FirewallIcmpTypeName::AddressMaskReply => true, > + _ => false, > + } Clippy complains that this is a custom `matches!(self, Self::Any | Self::EchoReply | ...)` implementation. And I agree, since it's very hard to really see *visually* that these are all `=> true,` and there's no `=> false,` hiding somewhere. This *could* be better with a table-formatted match statement kept so via `#[rustfmt::skip]`, but `matches!()` with an OR pattern makes more sense. Also, using `Self::` makes it much shorter again ;-) > + } > + > + pub const fn ipv6(&self) -> bool { > + match self { same here > + FirewallIcmpTypeName::Any => true, > + FirewallIcmpTypeName::EchoReply => true, > + FirewallIcmpTypeName::DestinationUnreachable => true, > + FirewallIcmpTypeName::PacketTooBig => true, > + FirewallIcmpTypeName::TimeExceeded => true, > + FirewallIcmpTypeName::ParameterProblem => true, > + FirewallIcmpTypeName::EchoRequest => true, > + FirewallIcmpTypeName::RouterSolicitation => true, > + FirewallIcmpTypeName::RouterAdvertisement => true, > + FirewallIcmpTypeName::NeighborSolicitation => true, > + FirewallIcmpTypeName::NeighbourSolicitation => true, > + FirewallIcmpTypeName::NeighborAdvertisement => true, > + FirewallIcmpTypeName::NeighbourAdvertisement => true, > + FirewallIcmpTypeName::Redirect => true, > + FirewallIcmpTypeName::NoRoute => true, > + FirewallIcmpTypeName::CommunicationProhibited => true, > + FirewallIcmpTypeName::BeyondScope => true, > + FirewallIcmpTypeName::AddressUnreachable => true, > + FirewallIcmpTypeName::PortUnreachable => true, > + FirewallIcmpTypeName::FailedPolicy => true, > + FirewallIcmpTypeName::RejectRoute => true, > + FirewallIcmpTypeName::TtlZeroDuringTransit => true, > + FirewallIcmpTypeName::TtlZeroDuringReassembly => true, > + FirewallIcmpTypeName::BadHeader => true, > + FirewallIcmpTypeName::UnknownHeaderType => true, > + FirewallIcmpTypeName::UnknownOption => true, > + _ => false, > + } > + } > +} > + > +#[cfg(test)] > +mod tests { > + use super::*; > + > + #[test] > + > + fn test_icmp_types() { > + // (name, variant, ipv4, ipv6) > + let tests = [ > + ("any", FirewallIcmpTypeName::Any, true, true), > + ("echo-reply", FirewallIcmpTypeName::EchoReply, true, true), > + ( > + "destination-unreachable", > + FirewallIcmpTypeName::DestinationUnreachable, > + true, > + true, > + ), > + ( > + "network-unreachable", > + FirewallIcmpTypeName::NetworkUnreachable, > + true, > + false, > + ), > + ( > + "host-unreachable", > + FirewallIcmpTypeName::HostUnreachable, > + true, > + false, > + ), > + ( > + "protocol-unreachable", > + FirewallIcmpTypeName::ProtocolUnreachable, > + true, > + false, > + ), > + ( > + "port-unreachable", > + FirewallIcmpTypeName::PortUnreachable, > + true, > + true, > + ), > + ( > + "fragmentation-needed", > + FirewallIcmpTypeName::FragmentationNeeded, > + true, > + false, > + ), > + ( > + "source-route-failed", > + FirewallIcmpTypeName::SourceRouteFailed, > + true, > + false, > + ), > + ( > + "network-unknown", > + FirewallIcmpTypeName::NetworkUnknown, > + true, > + false, > + ), > + ( > + "host-unknown", > + FirewallIcmpTypeName::HostUnknown, > + true, > + false, > + ), > + ( > + "network-prohibited", > + FirewallIcmpTypeName::NetworkProhibited, > + true, > + false, > + ), > + ( > + "host-prohibited", > + FirewallIcmpTypeName::HostProhibited, > + true, > + false, > + ), > + ( > + "TOS-network-unreachable", > + FirewallIcmpTypeName::TOSNetworkUnreachable, > + true, > + false, > + ), > + ( > + "TOS-host-unreachable", > + FirewallIcmpTypeName::TOSHostUnreachable, > + true, > + false, > + ), > + ( > + "communication-prohibited", > + FirewallIcmpTypeName::CommunicationProhibited, > + true, > + true, > + ), > + ( > + "host-precedence-violation", > + FirewallIcmpTypeName::HostPrecedenceViolation, > + true, > + false, > + ), > + ( > + "precedence-cutoff", > + FirewallIcmpTypeName::PrecedenceCutoff, > + true, > + false, > + ), > + ( > + "source-quench", > + FirewallIcmpTypeName::SourceQuench, > + true, > + false, > + ), > + ("redirect", FirewallIcmpTypeName::Redirect, true, true), > + ( > + "network-redirect", > + FirewallIcmpTypeName::NetworkRedirect, > + true, > + false, > + ), > + ( > + "host-redirect", > + FirewallIcmpTypeName::HostRedirect, > + true, > + false, > + ), > + ( > + "TOS-network-redirect", > + FirewallIcmpTypeName::TOSNetworkRedirect, > + true, > + false, > + ), > + ( > + "TOS-host-redirect", > + FirewallIcmpTypeName::TOSHostRedirect, > + true, > + false, > + ), > + ( > + "echo-request", > + FirewallIcmpTypeName::EchoRequest, > + true, > + true, > + ), > + ( > + "router-advertisement", > + FirewallIcmpTypeName::RouterAdvertisement, > + true, > + true, > + ), > + ( > + "router-solicitation", > + FirewallIcmpTypeName::RouterSolicitation, > + true, > + true, > + ), > + ( > + "time-exceeded", > + FirewallIcmpTypeName::TimeExceeded, > + true, > + true, > + ), > + ( > + "ttl-zero-during-transit", > + FirewallIcmpTypeName::TtlZeroDuringTransit, > + true, > + true, > + ), > + ( > + "ttl-zero-during-reassembly", > + FirewallIcmpTypeName::TtlZeroDuringReassembly, > + true, > + true, > + ), > + ( > + "parameter-problem", > + FirewallIcmpTypeName::ParameterProblem, > + true, > + true, > + ), > + ( > + "ip-header-bad", > + FirewallIcmpTypeName::IpHeaderBad, > + true, > + false, > + ), > + ( > + "required-option-missing", > + FirewallIcmpTypeName::RequiredOptionMissing, > + true, > + false, > + ), > + ( > + "timestamp-request", > + FirewallIcmpTypeName::TimestampRequest, > + true, > + false, > + ), > + ( > + "timestamp-reply", > + FirewallIcmpTypeName::TimestampReply, > + true, > + false, > + ), > + ( > + "address-mask-request", > + FirewallIcmpTypeName::AddressMaskRequest, > + true, > + false, > + ), > + ( > + "address-mask-reply", > + FirewallIcmpTypeName::AddressMaskReply, > + true, > + false, > + ), > + ("no-route", FirewallIcmpTypeName::NoRoute, false, true), > + ( > + "beyond-scope", > + FirewallIcmpTypeName::BeyondScope, > + false, > + true, > + ), > + ( > + "address-unreachable", > + FirewallIcmpTypeName::AddressUnreachable, > + false, > + true, > + ), > + ( > + "failed-policy", > + FirewallIcmpTypeName::FailedPolicy, > + false, > + true, > + ), > + ( > + "reject-route", > + FirewallIcmpTypeName::RejectRoute, > + false, > + true, > + ), > + ( > + "packet-too-big", > + FirewallIcmpTypeName::PacketTooBig, > + false, > + true, > + ), > + ("bad-header", FirewallIcmpTypeName::BadHeader, false, true), > + ( > + "unknown-header-type", > + FirewallIcmpTypeName::UnknownHeaderType, > + false, > + true, > + ), > + ( > + "unknown-option", > + FirewallIcmpTypeName::UnknownOption, > + false, > + true, > + ), > + ( > + "neighbor-solicitation", > + FirewallIcmpTypeName::NeighborSolicitation, > + false, > + true, > + ), > + ( > + "neighbour-solicitation", > + FirewallIcmpTypeName::NeighbourSolicitation, > + false, > + true, > + ), > + ( > + "neighbor-advertisement", > + FirewallIcmpTypeName::NeighborAdvertisement, > + false, > + true, > + ), > + ( > + "neighbour-advertisement", > + FirewallIcmpTypeName::NeighbourAdvertisement, > + false, > + true, > + ), > + ]; > + > + for (input, expected, v4, v6) in tests { > + let deserialized: FirewallIcmpTypeName = > + serde_plain::from_str(input).expect("deserialize"); > + assert_eq!(deserialized, expected); > + let serialized = serde_plain::to_string(&deserialized).expect("serialize"); > + assert_eq!(serialized, input); > + > + assert_eq!(deserialized.ipv4(), v4, "ipv4 check failed for {}", input); > + assert_eq!(deserialized.ipv6(), v6, "ipv6 check failed for {}", input); > + } > + } > + > + #[test] > + fn test_firewall_icmp_type_enum() { > + // Numeric > + for (input, output) in [ > + ("0", FirewallIcmpType::Numeric(0)), > + ("10", FirewallIcmpType::Numeric(10)), > + ("255", FirewallIcmpType::Numeric(255)), > + ] { > + let ty: FirewallIcmpType = input.parse().expect("valid numeric icmp type"); > + assert_eq!(ty, output); > + assert_eq!(ty.to_string(), input); > + } > + > + // Named > + for (input, output) in [ > + ("echo-request", FirewallIcmpTypeName::EchoRequest), > + ("any", FirewallIcmpTypeName::Any), > + ] { > + let ty: FirewallIcmpType = input.parse().expect("valid named icmp type"); > + assert_eq!(ty, FirewallIcmpType::Named(output)); > + assert_eq!(ty.to_string(), input); > + } > + > + // Invalid > + #[cfg(not(feature = "enum-fallback"))] > + for input in ["echo-reques", "an", "256", "-1", "foo"] { > + input > + .parse::() > + .expect_err("invalid icmp type"); > + } > + > + // Invalid, but with enum fallback enabled, should be parsed as unknown > + #[cfg(feature = "enum-fallback")] > + for input in ["echo-reques", "an", "256", "-1", "foo"] { > + let ty = input.parse::().expect("valid icmp type"); > + assert_eq!( > + ty, > + FirewallIcmpType::Named(FirewallIcmpTypeName::UnknownEnumValue( > + FixedString::new(input).unwrap(), > + )), > + ); > + } > + } > +} > diff --git a/proxmox-firewall-api-types/src/lib.rs b/proxmox-firewall-api-types/src/lib.rs > index b099be0c..610282bb 100644 > --- a/proxmox-firewall-api-types/src/lib.rs > +++ b/proxmox-firewall-api-types/src/lib.rs > @@ -1,6 +1,9 @@ > mod conntrack; > pub use conntrack::FirewallConntrackHelper; > > +mod icmp_type; > +pub use icmp_type::{FirewallIcmpType, FirewallIcmpTypeName}; > + > mod log; > pub use log::{ > FirewallLogLevel, FirewallLogRateLimit, FirewallPacketRate, FirewallPacketRateTimescale, > -- > 2.47.3 > > > >