Add checks to ensure that: * It is actually has a hostname, not just a domain name * Properly check if the hostname is purely numeric, which was broken/different to how the GUI installer does it
The custom error type also allows for easier future adaptions, as the changes can be entirely contained to the `Fqdn` type. Signed-off-by: Christoph Heiss <c.he...@proxmox.com> --- v1: https://lists.proxmox.com/pipermail/pve-devel/2023-June/057845.html Changes v1 -> v2: * Fix & add some more unit tests * Add custom error type `FqdnParseError` * Move hostname checks directly into `Fqdn::from()` proxmox-tui-installer/src/main.rs | 5 +-- proxmox-tui-installer/src/utils.rs | 68 ++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs index 42f278b..5dd8ef8 100644 --- a/proxmox-tui-installer/src/main.rs +++ b/proxmox-tui-installer/src/main.rs @@ -587,7 +587,7 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView { .get_value::<EditView, _>(1) .ok_or("failed to retrieve host FQDN")? .parse::<Fqdn>() - .map_err(|_| "failed to parse hostname".to_owned())?; + .map_err(|err| format!("hostname does not look valid:\n\n{err}"))?; let address = view .get_value::<CidrAddressEditView, _>(2) @@ -609,9 +609,6 @@ fn network_dialog(siv: &mut Cursive) -> InstallerView { Err("host and gateway IP address version must not differ".to_owned()) } else if address.addr().is_ipv4() != dns_server.is_ipv4() { Err("host and DNS IP address version must not differ".to_owned()) - } else if fqdn.to_string().chars().all(|c| c.is_ascii_digit()) { - // Not supported/allowed on Debian - Err("hostname cannot be purely numeric".to_owned()) } else if fqdn.to_string().ends_with(".invalid") { Err("hostname does not look valid".to_owned()) } else { diff --git a/proxmox-tui-installer/src/utils.rs b/proxmox-tui-installer/src/utils.rs index 3245fac..516f9c6 100644 --- a/proxmox-tui-installer/src/utils.rs +++ b/proxmox-tui-installer/src/utils.rs @@ -110,20 +110,51 @@ fn mask_limit(addr: &IpAddr) -> usize { } } -#[derive(Clone, Debug)] +/// Possible errors that might occur when parsing FQDNs. +#[derive(Debug, Eq, PartialEq)] +pub enum FqdnParseError { + MissingHostname, + NumericHostname, + InvalidPart(String), +} + +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}'", + ), + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] pub struct Fqdn { parts: Vec<String>, } impl Fqdn { - pub fn from(fqdn: &str) -> Result<Self, ()> { + pub fn from(fqdn: &str) -> Result<Self, FqdnParseError> { let parts = fqdn .split('.') .map(ToOwned::to_owned) .collect::<Vec<String>>(); - if !parts.iter().all(&Self::validate_single) { - Err(()) + for part in &parts { + if !Self::validate_single(part) { + return Err(FqdnParseError::InvalidPart(part.clone())); + } + } + + if parts.len() < 2 { + Err(FqdnParseError::MissingHostname) + } else if parts[0].chars().all(|c| c.is_ascii_digit()) { + // Not allowed/supported on Debian systems. + Err(FqdnParseError::NumericHostname) } else { Ok(Self { parts }) } @@ -143,20 +174,24 @@ impl Fqdn { 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 } fn validate_single(s: &String) -> bool { !s.is_empty() + // 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)) @@ -165,7 +200,7 @@ impl Fqdn { } impl FromStr for Fqdn { - type Err = (); + type Err = FqdnParseError; fn from_str(value: &str) -> Result<Self, Self::Err> { Self::from(value) @@ -194,17 +229,22 @@ mod tests { use super::*; #[test] - fn fqdn_validate_single() { + fn fqdn_construct() { + use FqdnParseError::*; assert!(Fqdn::from("foo.example.com").is_ok()); - assert!(Fqdn::from("1.example.com").is_ok()); assert!(Fqdn::from("foo-bar.com").is_ok()); assert!(Fqdn::from("a-b.com").is_ok()); - assert!(Fqdn::from("foo").is_err()); - assert!(Fqdn::from("-foo.com").is_err()); - assert!(Fqdn::from("foo-.com").is_err()); - assert!(Fqdn::from("foo.com-").is_err()); - assert!(Fqdn::from("-o-.com").is_err()); + 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()))); + + assert_eq!(Fqdn::from("123.com"), Err(NumericHostname)); + assert!(Fqdn::from("foo123.com").is_ok()); + assert!(Fqdn::from("123foo.com").is_ok()); } #[test] @@ -212,6 +252,10 @@ mod tests { 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] -- 2.41.0 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
