[pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet

["Michael Köppl" <m.koeppl@proxmox.com>]

Add checks for valid subnet mask (greater than /0 and at most /32 for
IPv4). In addition, check if the address entered by the user is valid
within the given subnet, i.e. not a network address or broadcast
address. /31 is considered an exception in accordance with RFC3021 [0],
considering any of the 2 available addresses to be valid host addresses.

[0] https://datatracker.ietf.org/doc/html/rfc3021

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Some input / discussion would be much appreciated here, since this might
again be considered too restrictive.

 proxmox-auto-installer/tests/parse-answer.rs  |  3 +
 .../parse_answer/ipv4_and_subnet_31.json      | 19 +++++
 .../parse_answer/ipv4_and_subnet_31.toml      | 18 +++++
 .../ipv4_and_subnet_addr_is_network.json      |  3 +
 .../ipv4_and_subnet_addr_is_network.toml      | 18 +++++
 .../ipv4_and_subnet_mask_33.json              |  3 +
 .../ipv4_and_subnet_mask_33.toml              | 18 +++++
 proxmox-installer-common/src/utils.rs         | 81 ++++++++++++++-----
 8 files changed, 143 insertions(+), 20 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml

diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index f560483..6f627a0 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -127,6 +127,7 @@ mod tests {
             fqdn_from_dhcp_no_dhcp_domain_with_default_domain,
             full_fqdn_from_dhcp_with_default_domain,
             hashed_root_password,
+            ipv4_and_subnet_31,
             minimal,
             nic_matching,
             specific_nic,
@@ -146,6 +147,8 @@ mod tests {
             duplicate_disk,
             fqdn_from_dhcp_no_default_domain,
             fqdn_hostname_only,
+            ipv4_and_subnet_addr_is_network,
+            ipv4_and_subnet_mask_33,
             lvm_maxroot_greater_than_maximum,
             lvm_swapsize_greater_than_hdsize,
             no_fqdn_from_dhcp,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
new file mode 100644
index 0000000..2a475fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
@@ -0,0 +1,19 @@
+{
+  "autoreboot": 1,
+  "cidr": "10.10.10.10/31",
+  "country": "at",
+  "dns": "10.10.10.1",
+  "domain": "testinstall",
+  "filesys": "ext4",
+  "gateway": "10.10.10.1",
+  "hdsize": 223.57088470458984,
+  "existing_storage_auto_rename": 1,
+  "hostname": "pveauto",
+  "keymap": "de",
+  "mailto": "mail@no.invalid",
+  "mngmt_nic": "enp129s0f1np1",
+  "root_password": { "plain": "12345678" },
+  "target_hd": "/dev/sda",
+  "timezone": "Europe/Vienna",
+  "first_boot": { "enabled": 0 }
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
new file mode 100644
index 0000000..0ca6ca5
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/31"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
new file mode 100644
index 0000000..b28699d
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
@@ -0,0 +1,3 @@
+{
+  "parse-error": "error parsing answer.toml: Invalid CIDR: address is a network address"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
new file mode 100644
index 0000000..7a9141b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.32/27"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
new file mode 100644
index 0000000..6b2888b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
@@ -0,0 +1,3 @@
+{
+    "parse-error": "error parsing answer.toml: Invalid CIDR: mask cannot be greater than 32"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
new file mode 100644
index 0000000..16a560f
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/33"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 1fe6a74..fea98db 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -1,7 +1,7 @@
 use std::{
+    error::Error,
     fmt,
     net::{AddrParseError, IpAddr},
-    num::ParseIntError,
     str::FromStr,
 };
 
@@ -15,20 +15,26 @@ pub enum CidrAddressParseError {
     /// The IP address part could not be parsed.
     InvalidAddr(AddrParseError),
     /// The mask could not be parsed.
-    InvalidMask(Option<ParseIntError>),
+    InvalidMask(Box<dyn Error>),
+    /// The IP address is a network address.
+    IsNetworkAddr,
+    /// The IP address is a broadcast address.
+    IsBroadcastAddr,
 }
 
 impl fmt::Display for CidrAddressParseError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let msg = match &self {
+        write!(f, "Invalid CIDR: ")?;
+
+        match self {
             CidrAddressParseError::NoDelimiter => {
-                String::from("No delimiter for separating address and mask was found")
+                write!(f, "no delimiter for separating address and mask was found")
             }
-            CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
-            CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
-        };
-
-        write!(f, "Invalid CIDR: {msg}")
+            CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
+            CidrAddressParseError::InvalidMask(err) => write!(f, "{err}"),
+            CidrAddressParseError::IsNetworkAddr => write!(f, "address is a network address"),
+            CidrAddressParseError::IsBroadcastAddr => write!(f, "address is a broadcast address"),
+        }
     }
 }
 
@@ -61,11 +67,10 @@ impl CidrAddress {
     pub fn new<T: Into<IpAddr>>(addr: T, mask: usize) -> Result<Self, CidrAddressParseError> {
         let addr = addr.into();
 
-        if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
-        }
+        check_mask_limit(&addr, mask)?;
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 
     /// Returns only the IP address part of the address.
@@ -101,13 +106,12 @@ impl FromStr for CidrAddress {
 
         let mask = mask
             .parse()
-            .map_err(|err| CidrAddressParseError::InvalidMask(Some(err)))?;
+            .map_err(|err| CidrAddressParseError::InvalidMask(Box::new(err)))?;
 
-        if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
-        }
+        check_mask_limit(&addr, mask)?;
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 }
 
@@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
     if addr.is_ipv4() { 32 } else { 128 }
 }
 
+fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+    return if mask > mask_limit(&addr) {
+        Err(CidrAddressParseError::InvalidMask(
+            "mask cannot be greater than 32".into(),
+        ))
+    } else {
+        Ok(())
+    };
+}
+
+fn check_addr_valid_in_subnet(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+    match &addr {
+        IpAddr::V4(addr_v4) => {
+            if mask >= 31 {
+                return Ok(());
+            }
+
+            let num_host_bits = 32 - mask;
+            let host_part_mask = (1u32 << num_host_bits) - 1;
+
+            let ip_bits = u32::from_be_bytes(addr_v4.octets());
+
+            let network_addr = ip_bits & !host_part_mask;
+            let broadcast_addr = network_addr | host_part_mask;
+
+            if ip_bits == network_addr {
+                Err(CidrAddressParseError::IsNetworkAddr)
+            } else if ip_bits == broadcast_addr {
+                Err(CidrAddressParseError::IsBroadcastAddr)
+            } else {
+                Ok(())
+            }
+        }
+        IpAddr::V6(_) => Ok(()),
+    }
+}
+
 /// Possible errors that might occur when parsing FQDNs.
 #[derive(Debug, Eq, PartialEq)]
 pub enum FqdnParseError {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel