From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B70C11FF17A for ; Tue, 9 Dec 2025 10:12:43 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EA12E20119; Tue, 9 Dec 2025 10:13:21 +0100 (CET) Mime-Version: 1.0 Date: Tue, 09 Dec 2025 10:13:10 +0100 Message-Id: From: "Lukas Wagner" To: "Proxmox Datacenter Manager development discussion" , "Christoph Heiss" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20251205112528.373387-1-c.heiss@proxmox.com> <20251205112528.373387-3-c.heiss@proxmox.com> In-Reply-To: <20251205112528.373387-3-c.heiss@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1765271584868 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 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 T_SPF_HELO_TEMPERROR 0.01 SPF: test of HELO record failed (temperror) T_SPF_TEMPERROR 0.01 SPF: test of record failed (temperror) Subject: Re: [pdm-devel] [PATCH proxmox v2 02/14] network-types: move `Fqdn` type from proxmox-installer-common X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" Looks good in general, I assume this was reviewed before this it was moved from somewhere else. One note inline. Reviewed-by: Lukas Wagner On Fri Dec 5, 2025 at 12:25 PM CET, Christoph Heiss wrote: > This introduces an `Fqdn` type for safely representing (valid) FQDNs on > Debian, following all relevant RFCs as well as restrictions given by > Debian. > > Signed-off-by: Christoph Heiss > --- > Changes v1 -> v2: > * no changes > > proxmox-network-types/Cargo.toml | 3 +- > proxmox-network-types/debian/control | 2 + > proxmox-network-types/src/fqdn.rs | 248 +++++++++++++++++++++++++++ > proxmox-network-types/src/lib.rs | 1 + > 4 files changed, 253 insertions(+), 1 deletion(-) > create mode 100644 proxmox-network-types/src/fqdn.rs > > diff --git a/proxmox-network-types/Cargo.toml b/proxmox-network-types/Cargo.toml > index 6333a37f..25c4bcf2 100644 > --- a/proxmox-network-types/Cargo.toml > +++ b/proxmox-network-types/Cargo.toml > @@ -10,9 +10,10 @@ exclude.workspace = true > rust-version.workspace = true > > [dependencies] > -regex = { workspace = true, optional = true} > +regex = { workspace = true, optional = true } > serde = { workspace = true, features = [ "derive", "std" ] } > serde_with = "3.8.1" > +serde_plain.workspace = true > thiserror.workspace = true > > proxmox-schema = { workspace = true, features = [ "api-macro", "api-types" ], optional = true} > diff --git a/proxmox-network-types/debian/control b/proxmox-network-types/debian/control > index 8b68deb1..08df0f9f 100644 > --- a/proxmox-network-types/debian/control > +++ b/proxmox-network-types/debian/control > @@ -9,6 +9,7 @@ Build-Depends-Arch: cargo:native , > librust-serde-1+default-dev , > librust-serde-1+derive-dev , > librust-serde-1+std-dev , > + librust-serde-plain-1+default-dev , > librust-serde-with-3+default-dev (>= 3.8.1-~~) , > librust-thiserror-2+default-dev > Maintainer: Proxmox Support Team > @@ -26,6 +27,7 @@ Depends: > librust-serde-1+default-dev, > librust-serde-1+derive-dev, > librust-serde-1+std-dev, > + librust-serde-plain-1+default-dev, > librust-serde-with-3+default-dev (>= 3.8.1-~~), > librust-thiserror-2+default-dev > Suggests: > diff --git a/proxmox-network-types/src/fqdn.rs b/proxmox-network-types/src/fqdn.rs > new file mode 100644 > index 00000000..9582639d > --- /dev/null > +++ b/proxmox-network-types/src/fqdn.rs > @@ -0,0 +1,248 @@ > +//! A type for safely representing fully-qualified domain names (FQDNs). > + > +use std::{fmt, str::FromStr}; > + > +use serde::Deserialize; > + > +/// Possible errors that might occur when parsing FQDNs. > +#[derive(Debug, Eq, PartialEq)] > +pub enum FqdnParseError { > + MissingHostname, > + NumericHostname, > + InvalidPart(String), > + TooLong(usize), > +} > + > +impl fmt::Display for FqdnParseError { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + use FqdnParseError::*; > + match self { > + MissingHostname => write!(f, "missing hostname part"), > + NumericHostname => write!(f, "hostname cannot be purely numeric"), > + InvalidPart(part) => write!( > + f, > + "FQDN must only consist of alphanumeric characters and dashes. Invalid part: '{part}'", > + ), > + TooLong(len) => write!(f, "FQDN too long: {len} > {}", Fqdn::MAX_LENGTH), > + } > + } > +} > + > +/// A type for safely representing fully-qualified domain names (FQDNs). > +/// > +/// It considers following RFCs: > +/// - [RFC952] (sec. "ASSUMPTIONS", 1.) > +/// - [RFC1035] (sec. 2.3. "Conventions") > +/// - [RFC1123] (sec. 2.1. "Host Names and Numbers") > +/// - [RFC3492] > +/// - [RFC4343] > +/// > +/// .. and applies some restriction given by Debian, e.g. 253 instead of 255 > +/// maximum total length and maximum 63 characters per label, per the > +/// [hostname(7)]. > +/// > +/// Additionally: > +/// - It enforces the restriction as per Bugzilla #1054, in that > +/// purely numeric hostnames are not allowed - against RFC1123 sec. 2.1. > +/// > +/// Some terminology: > +/// - "label" - a single part of a FQDN, e.g. {label}.{label}.{tld} > +/// > +/// [RFC952]: > +/// [RFC1035]: > +/// [RFC1123]: > +/// [RFC3492]: > +/// [RFC4343]: > +/// [hostname(7)]: > +#[derive(Clone, Debug, Eq)] > +pub struct Fqdn { > + parts: Vec, > +} > + > +impl Fqdn { > + /// Maximum length of a single label of the FQDN > + const MAX_LABEL_LENGTH: usize = 63; > + /// Maximum total length of the FQDN > + const MAX_LENGTH: usize = 253; > + > + pub fn from(fqdn: &str) -> Result { > + if fqdn.len() > Self::MAX_LENGTH { > + return Err(FqdnParseError::TooLong(fqdn.len())); > + } > + > + let parts = fqdn > + .split('.') > + .map(ToOwned::to_owned) > + .collect::>(); > + > + for part in &parts { > + if !Self::validate_single(part) { > + return Err(FqdnParseError::InvalidPart(part.clone())); > + } > + } > + > + if parts.len() < 2 { > + Err(FqdnParseError::MissingHostname) (a) Since `Fqdn::from` seems to be the only way to instantiate the Fqdn type, and since the parts.len() < 2 establishes the invariant that the number of parts can only ever be >= 2, > + } else if parts[0].chars().all(|c| c.is_ascii_digit()) { > + // Do not allow a purely numeric hostname, see: > + // https://bugzilla.proxmox.com/show_bug.cgi?id=1054 > + Err(FqdnParseError::NumericHostname) > + } else { > + Ok(Self { parts }) > + } > + } > + > + pub fn host(&self) -> Option<&str> { > + self.has_host().then_some(&self.parts[0]) > + } (c) ... therefore you could assume that it is safe to just access self.parts[0] and avoid the `Option<..>` in the return type? I'm always on board with being careful, but since this type is relatively small, I think it would be reasonable to rely on the invariant, which makes the call sites a bit nicer since it does not have to handle the Option. What do you think? > + > + pub fn domain(&self) -> String { > + let parts = if self.has_host() { > + &self.parts[1..] > + } else { > + &self.parts > + }; > + > + parts.join(".") > + } > + > + /// Checks whether the FQDN has a hostname associated with it, i.e. is has more than 1 part. > + fn has_host(&self) -> bool { > + self.parts.len() > 1 > + } (b) ... which means that this should always return true, ... > + > + fn validate_single(s: &str) -> bool { > + !s.is_empty() > + && s.len() <= Self::MAX_LABEL_LENGTH > + // First character must be alphanumeric > + && s.chars() > + .next() > + .map(|c| c.is_ascii_alphanumeric()) > + .unwrap_or_default() > + // .. last character as well, > + && s.chars() > + .last() > + .map(|c| c.is_ascii_alphanumeric()) > + .unwrap_or_default() > + // and anything between must be alphanumeric or - > + && s.chars() > + .skip(1) > + .take(s.len().saturating_sub(2)) > + .all(|c| c.is_ascii_alphanumeric() || c == '-') > + } > +} > + > +impl FromStr for Fqdn { > + type Err = FqdnParseError; > + > + fn from_str(value: &str) -> Result { > + Self::from(value) > + } > +} > + > +impl fmt::Display for Fqdn { > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { > + write!(f, "{}", self.parts.join(".")) > + } > +} > + > +serde_plain::derive_serialize_from_display!(Fqdn); > + > +impl<'de> Deserialize<'de> for Fqdn { > + fn deserialize(deserializer: D) -> Result > + where > + D: serde::Deserializer<'de>, > + { > + let s: String = Deserialize::deserialize(deserializer)?; > + s.parse() > + .map_err(|_| serde::de::Error::custom("invalid FQDN")) > + } > +} > + > +impl PartialEq for Fqdn { > + // Case-insensitive comparison, as per RFC 952 "ASSUMPTIONS", RFC 1035 sec. 2.3.3. "Character > + // Case" and RFC 4343 as a whole > + fn eq(&self, other: &Self) -> bool { > + if self.parts.len() != other.parts.len() { > + return false; > + } > + > + self.parts > + .iter() > + .zip(other.parts.iter()) > + .all(|(a, b)| a.to_lowercase() == b.to_lowercase()) > + } > +} > + > +#[cfg(test)] > +mod tests { > + use super::*; > + > + #[test] > + fn fqdn_construct() { > + use FqdnParseError::*; > + assert!(Fqdn::from("foo.example.com").is_ok()); > + assert!(Fqdn::from("foo-bar.com").is_ok()); > + assert!(Fqdn::from("a-b.com").is_ok()); > + > + assert_eq!(Fqdn::from("foo"), Err(MissingHostname)); > + > + assert_eq!(Fqdn::from("-foo.com"), Err(InvalidPart("-foo".to_owned()))); > + assert_eq!(Fqdn::from("foo-.com"), Err(InvalidPart("foo-".to_owned()))); > + assert_eq!(Fqdn::from("foo.com-"), Err(InvalidPart("com-".to_owned()))); > + assert_eq!(Fqdn::from("-o-.com"), Err(InvalidPart("-o-".to_owned()))); > + > + // https://bugzilla.proxmox.com/show_bug.cgi?id=1054 > + assert_eq!(Fqdn::from("123.com"), Err(NumericHostname)); > + assert!(Fqdn::from("foo123.com").is_ok()); > + assert!(Fqdn::from("123foo.com").is_ok()); > + > + assert!(Fqdn::from(&format!("{}.com", "a".repeat(63))).is_ok()); > + assert_eq!( > + Fqdn::from(&format!("{}.com", "a".repeat(250))), > + Err(TooLong(254)), > + ); > + assert_eq!( > + Fqdn::from(&format!("{}.com", "a".repeat(64))), > + Err(InvalidPart("a".repeat(64))), > + ); > + > + // https://bugzilla.proxmox.com/show_bug.cgi?id=5230 > + assert_eq!( > + Fqdn::from("123@foo.com"), > + Err(InvalidPart("123@foo".to_owned())) > + ); > + } > + > + #[test] > + fn fqdn_parts() { > + let fqdn = Fqdn::from("pve.example.com").unwrap(); > + assert_eq!(fqdn.host().unwrap(), "pve"); > + assert_eq!(fqdn.domain(), "example.com"); > + assert_eq!( > + fqdn.parts, > + &["pve".to_owned(), "example".to_owned(), "com".to_owned()] > + ); > + } > + > + #[test] > + fn fqdn_display() { > + assert_eq!( > + Fqdn::from("foo.example.com").unwrap().to_string(), > + "foo.example.com" > + ); > + } > + > + #[test] > + fn fqdn_compare() { > + assert_eq!(Fqdn::from("example.com"), Fqdn::from("example.com")); > + assert_eq!(Fqdn::from("example.com"), Fqdn::from("ExAmPle.Com")); > + assert_eq!(Fqdn::from("ExAmPle.Com"), Fqdn::from("example.com")); > + assert_ne!( > + Fqdn::from("subdomain.ExAmPle.Com"), > + Fqdn::from("example.com") > + ); > + assert_ne!(Fqdn::from("foo.com"), Fqdn::from("bar.com")); > + assert_ne!(Fqdn::from("example.com"), Fqdn::from("example.net")); > + } > +} > diff --git a/proxmox-network-types/src/lib.rs b/proxmox-network-types/src/lib.rs > index ee26b1c1..e5d31285 100644 > --- a/proxmox-network-types/src/lib.rs > +++ b/proxmox-network-types/src/lib.rs > @@ -1,5 +1,6 @@ > #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] > #![deny(unsafe_op_in_unsafe_fn)] > > +pub mod fqdn; > pub mod ip_address; > pub mod mac_address; _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel