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 72C301FF161 for ; Wed, 6 Nov 2024 14:13:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 87608E55C; Wed, 6 Nov 2024 14:13:09 +0100 (CET) Date: Wed, 6 Nov 2024 14:13:03 +0100 From: Wolfgang Bumiller To: Stefan Hanreich Message-ID: References: <20241010155637.255451-1-s.hanreich@proxmox.com> <20241010155637.255451-6-s.hanreich@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20241010155637.255451-6-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.080 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_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [address.rs] Subject: Re: [pve-devel] [PATCH proxmox-ve-rs v2 05/25] firewall: add ip range types X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: pve-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Thu, Oct 10, 2024 at 05:56:17PM GMT, Stefan Hanreich wrote: > Currently we are using tuples to represent IP ranges which is > suboptimal. Validation logic and invariant checking needs to happen at > every site using the IP range rather than having a unified struct for > enforcing those invariants. > > Signed-off-by: Stefan Hanreich > --- > .../src/firewall/types/address.rs | 230 +++++++++++++++++- > 1 file changed, 228 insertions(+), 2 deletions(-) > > diff --git a/proxmox-ve-config/src/firewall/types/address.rs b/proxmox-ve-config/src/firewall/types/address.rs > index e48ac1b..42ec1a1 100644 > --- a/proxmox-ve-config/src/firewall/types/address.rs > +++ b/proxmox-ve-config/src/firewall/types/address.rs > @@ -1,9 +1,9 @@ > -use std::fmt; > +use std::fmt::{self, Display}; > use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; > use std::ops::Deref; > > use anyhow::{bail, format_err, Error}; > -use serde_with::DeserializeFromStr; > +use serde_with::{DeserializeFromStr, SerializeDisplay}; > > #[derive(Clone, Copy, Debug, Eq, PartialEq)] > pub enum Family { > @@ -239,6 +239,202 @@ impl> From for Ipv6Cidr { > } > } > > +#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] > +pub enum IpRangeError { > + MismatchedFamilies, > + StartGreaterThanEnd, > + InvalidFormat, > +} > + > +impl std::error::Error for IpRangeError {} > + > +impl Display for IpRangeError { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + f.write_str(match self { > + IpRangeError::MismatchedFamilies => "mismatched ip address families", > + IpRangeError::StartGreaterThanEnd => "start is greater than end", > + IpRangeError::InvalidFormat => "invalid ip range format", > + }) > + } > +} > + > +/// Represents a range of IPv4 or IPv6 addresses > +/// > +/// For more information see [`AddressRange`] > +#[derive(Clone, Copy, Debug, PartialEq, Eq, SerializeDisplay, DeserializeFromStr)] > +pub enum IpRange { > + V4(AddressRange), > + V6(AddressRange), > +} > + > +impl IpRange { > + /// returns the family of the IpRange > + pub fn family(&self) -> Family { > + match self { > + IpRange::V4(_) => Family::V4, > + IpRange::V6(_) => Family::V6, > + } > + } > + > + /// creates a new [`IpRange`] from two [`IpAddr`] > + /// > + /// # Errors > + /// > + /// This function will return an error if start and end IP address are not from the same family. > + pub fn new(start: impl Into, end: impl Into) -> Result { > + match (start.into(), end.into()) { > + (IpAddr::V4(start), IpAddr::V4(end)) => Self::new_v4(start, end), > + (IpAddr::V6(start), IpAddr::V6(end)) => Self::new_v6(start, end), > + _ => Err(IpRangeError::MismatchedFamilies), > + } > + } > + > + /// construct a new Ipv4 Range > + pub fn new_v4( > + start: impl Into, > + end: impl Into, > + ) -> Result { > + Ok(IpRange::V4(AddressRange::new_v4(start, end)?)) > + } > + > + pub fn new_v6( > + start: impl Into, > + end: impl Into, > + ) -> Result { > + Ok(IpRange::V6(AddressRange::new_v6(start, end)?)) > + } > +} > + > +impl std::str::FromStr for IpRange { > + type Err = IpRangeError; > + > + fn from_str(s: &str) -> Result { > + if let Ok(range) = s.parse() { > + return Ok(IpRange::V4(range)); > + } > + > + if let Ok(range) = s.parse() { > + return Ok(IpRange::V6(range)); > + } > + > + Err(IpRangeError::InvalidFormat) > + } > +} > + > +impl fmt::Display for IpRange { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + match self { > + IpRange::V4(range) => range.fmt(f), > + IpRange::V6(range) => range.fmt(f), > + } > + } > +} > + > +/// Represents a range of IP addresses from start to end > +/// > +/// This type is for encapsulation purposes for the [`IpRange`] enum and should be instantiated via > +/// that enum. > +/// > +/// # Invariants > +/// > +/// * start and end have the same IP address family > +/// * start is lesser than or equal to end lesser -> less Also: This range *includes* the `end`. In rust `std` we have `std::ops::Range` while *this* works like `std::ops::RangeInclusive`. This might be fine, given it's a vastly different context, however... > +/// > +/// # Textual representation > +/// > +/// Two IP addresses separated by a hyphen, e.g.: `127.0.0.1-127.0.0.255` > +#[derive(Clone, Copy, Debug, PartialEq, Eq)] > +pub struct AddressRange { > + start: T, > + end: T, ...I think we should name this `last`, so it's less confusing and... > +} > + > +impl AddressRange { > + pub(crate) fn new_v4( > + start: impl Into, > + end: impl Into, > + ) -> Result, IpRangeError> { > + let (start, end) = (start.into(), end.into()); > + > + if start > end { > + return Err(IpRangeError::StartGreaterThanEnd); > + } > + > + Ok(Self { start, end }) > + } > +} > + > +impl AddressRange { > + pub(crate) fn new_v6( > + start: impl Into, > + end: impl Into, > + ) -> Result, IpRangeError> { > + let (start, end) = (start.into(), end.into()); > + > + if start > end { > + return Err(IpRangeError::StartGreaterThanEnd); > + } > + > + Ok(Self { start, end }) > + } > +} > + > +impl AddressRange { > + pub fn start(&self) -> &T { > + &self.start > + } > + > + pub fn end(&self) -> &T { ... similarly these getters should be named `last`. Mainly because with the ranges being inclusive, this represents the "*last* usable address", while "end" is also used in `std::ops::Range` to mean "fist *unusable* number". > + &self.end > + } > +} > + > +impl std::str::FromStr for AddressRange { > + type Err = IpRangeError; > + > + fn from_str(s: &str) -> Result { > + if let Some((start, end)) = s.split_once('-') { > + let start_address = start > + .parse::() > + .map_err(|_| IpRangeError::InvalidFormat)?; > + > + let end_address = end > + .parse::() > + .map_err(|_| IpRangeError::InvalidFormat)?; > + > + return Self::new_v4(start_address, end_address); > + } > + > + Err(IpRangeError::InvalidFormat) > + } > +} > + > +impl std::str::FromStr for AddressRange { > + type Err = IpRangeError; > + > + fn from_str(s: &str) -> Result { > + if let Some((start, end)) = s.split_once('-') { > + let start_address = start > + .parse::() > + .map_err(|_| IpRangeError::InvalidFormat)?; > + > + let end_address = end > + .parse::() > + .map_err(|_| IpRangeError::InvalidFormat)?; > + > + return Self::new_v6(start_address, end_address); > + } > + > + Err(IpRangeError::InvalidFormat) > + } > +} > + > +impl fmt::Display for AddressRange { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + write!(f, "{}-{}", self.start, self.end) > + } > +} > + > #[derive(Clone, Debug)] > #[cfg_attr(test, derive(Eq, PartialEq))] > pub enum IpEntry { > @@ -612,4 +808,34 @@ mod tests { > ]) > .expect_err("cannot mix ip families in ip list"); > } > + > + #[test] > + fn test_ip_range() { > + IpRange::new([10, 0, 0, 2], [10, 0, 0, 1]).unwrap_err(); > + > + IpRange::new( > + [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0x1000], > + [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0], > + ) > + .unwrap_err(); > + > + let v4_range = IpRange::new([10, 0, 0, 0], [10, 0, 0, 100]).unwrap(); > + assert_eq!(v4_range.family(), Family::V4); > + > + let v6_range = IpRange::new( > + [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0], > + [0x2001, 0x0db8, 0, 0, 0, 0, 0, 0x1000], > + ) > + .unwrap(); > + assert_eq!(v6_range.family(), Family::V6); > + > + "10.0.0.1-10.0.0.100".parse::().unwrap(); > + "2001:db8::1-2001:db8::f".parse::().unwrap(); > + > + "10.0.0.1-2001:db8::1000".parse::().unwrap_err(); > + "2001:db8::1-192.168.0.2".parse::().unwrap_err(); > + > + "10.0.0.1-10.0.0.0".parse::().unwrap_err(); > + "2001:db8::1-2001:db8::0".parse::().unwrap_err(); > + } > } > -- > 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel