[pve-devel] [PATCH installer v2] tui: fix FQDN validation

[pve-devel] [PATCH installer v2] tui: fix FQDN validation

[Christoph Heiss]

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.heiss@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] applied: [PATCH installer v2] tui: fix FQDN validation

[Thomas Lamprecht]

On 19/07/2023 18:31, Christoph Heiss wrote:
> 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.heiss@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(-)
> 
>

applied, thanks!