From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 225F31FF133 for ; Mon, 11 May 2026 15:37:06 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A3DCE177A2; Mon, 11 May 2026 15:37:05 +0200 (CEST) Date: Mon, 11 May 2026 15:36:52 +0200 From: Arthur Bied-Charreton To: Stefan Hanreich Subject: Re: [PATCH proxmox-ve-rs v4 12/31] ve-config: fabrics: wireguard add validation for wireguard config Message-ID: References: <20260507124008.417223-1-s.hanreich@proxmox.com> <20260507124008.417223-13-s.hanreich@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260507124008.417223-13-s.hanreich@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778506502468 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.779 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 5HIPL27YBDCDATLZNLR7ONYPAOPSTQQJ X-Message-ID-Hash: 5HIPL27YBDCDATLZNLR7ONYPAOPSTQQJ X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, May 07, 2026 at 02:39:47PM +0200, Stefan Hanreich wrote: > Implement validation for the invariants of the wireguard > configuration: > > * All interfaces referenced in peer definitions must exist > * Listen ports cannot be duplicated > * Interface names must be unique on a node > > Wireguard Interface names are validated separately for uniqueness, > since they can be referenced by other fabrics and this would trigger > the duplicate check. > > Signed-off-by: Stefan Hanreich > --- > proxmox-ve-config/Cargo.toml | 1 + > proxmox-ve-config/src/sdn/fabric/mod.rs | 167 ++++++++++++++++-- > .../src/sdn/fabric/section_config/node.rs | 2 +- > .../section_config/protocol/wireguard.rs | 65 ++++++- > 4 files changed, 218 insertions(+), 17 deletions(-) > > diff --git a/proxmox-ve-config/Cargo.toml b/proxmox-ve-config/Cargo.toml > index 2f8cded..8485a0f 100644 > --- a/proxmox-ve-config/Cargo.toml > +++ b/proxmox-ve-config/Cargo.toml > @@ -33,3 +33,4 @@ frr = ["dep:proxmox-frr"] > > [dev-dependencies] > insta = "1.21" > +pretty_assertions = "1.4.0" > diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs b/proxmox-ve-config/src/sdn/fabric/mod.rs > index e062e50..2c2ad52 100644 > --- a/proxmox-ve-config/src/sdn/fabric/mod.rs > +++ b/proxmox-ve-config/src/sdn/fabric/mod.rs > @@ -31,7 +31,7 @@ use crate::sdn::fabric::section_config::protocol::ospf::{ > }; > use crate::sdn::fabric::section_config::protocol::wireguard::{ > WireGuardDeletableProperties, WireGuardNode, WireGuardNodeDeletableProperties, > - WireGuardNodeUpdater, WireGuardPropertiesUpdater, > + WireGuardNodePeer, WireGuardNodeUpdater, WireGuardPropertiesUpdater, > }; > use crate::sdn::fabric::section_config::{FabricOrNode, Section}; > > @@ -73,6 +73,12 @@ pub enum FabricConfigError { > OverlappingIp4Prefix(String, String, String, String), > #[error("IPv6 prefix {0} in fabric '{1}' overlaps with IPv6 prefix {2} in fabric '{3}'")] > OverlappingIp6Prefix(String, String, String, String), > + #[error("peer configuration references non-existing local interface '{0}'")] > + InvalidLocalInterfaceReference(String), > + #[error("peer configuration references non-existing interface '{0}' on node '{1}'")] > + InvalidRemoteInterfaceReference(String, String), > + #[error("WireGuard interface listen port duplicated in node configuration: {0}")] > + DuplicatePort(String), > } > > /// An entry in a [`FabricConfig`]. > @@ -500,7 +506,52 @@ impl Validatable for FabricEntry { > let mut ips = HashSet::new(); > let mut ip6s = HashSet::new(); > > + if let FabricEntry::WireGuard(entry) = self { > + // check if all interfaces referenced by the peer definitions exist inside the > + // fabric > + let mut all_interfaces = HashSet::new(); > + let mut internal_peers = HashSet::new(); > + > + for node_id in entry.nodes.keys() { > + let node_section = entry.node_section(node_id)?; > + > + if let WireGuardNode::Internal(node) = node_section.properties() { > + all_interfaces.extend( > + node.interfaces() > + .map(|interface| (&node_section.id.node_id, &interface.name)), > + ); > + > + internal_peers.extend(node.peers().filter_map(|peer| { > + if let WireGuardNodePeer::Internal(peer) = peer { > + return Some((&peer.node, &peer.node_iface)); > + } > + > + None > + })); > + } > + } > + > + for (node_id, interface) in internal_peers.difference(&all_interfaces) { > + return Err(FabricConfigError::InvalidRemoteInterfaceReference( > + interface.to_string(), > + node_id.to_string(), > + )); > + } Might make sense to also check for interface IPs that are already set on other nodes to prevent having 2 nodes with the same IP in a WG fabric. > + } > + > for (_id, node) in self.nodes() { > + node.validate()?; > + > + // Node IPs need to be unique inside a fabric > + if !node.ip().map(|ip| ips.insert(ip)).unwrap_or(true) { > + return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > + } > + > + // Node IPs need to be unique inside a fabric > + if !node.ip6().map(|ip| ip6s.insert(ip)).unwrap_or(true) { > + return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > + } > + > // Check IPv4 prefix and ip > match (fabric.ip_prefix(), node.ip()) { > (None, Some(ip)) => { > @@ -554,18 +605,6 @@ impl Validatable for FabricEntry { > } > _ => {} > } > - > - // Node IPs need to be unique inside a fabric > - if !node.ip().map(|ip| ips.insert(ip)).unwrap_or(true) { > - return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > - } > - > - // Node IPs need to be unique inside a fabric > - if !node.ip6().map(|ip| ip6s.insert(ip)).unwrap_or(true) { > - return Err(FabricConfigError::DuplicateNodeIp(fabric.id().to_string())); > - } > - > - node.validate()?; > } > > fabric.validate() > @@ -600,6 +639,7 @@ impl Validatable for FabricConfig { > /// - all the ospf fabrics have different areas > /// - IP prefixes of fabrics do not overlap > fn validate(&self) -> Result<(), FabricConfigError> { > + let mut wireguard_interfaces = HashSet::new(); > let mut node_interfaces = HashSet::new(); > let mut ospf_area = HashSet::new(); > > @@ -634,6 +674,7 @@ impl Validatable for FabricConfig { > } > > // validate that each (node, interface) combination exists only once across all fabrics > + // additionally, for wireguard check the listen ports of the interfaces as well > for entry in self.fabrics.values() { > if let FabricEntry::Ospf(entry) = entry { > if !ospf_area.insert( > @@ -662,8 +703,14 @@ impl Validatable for FabricConfig { > return Err(FabricConfigError::DuplicateInterface); > } > } > - Node::WireGuard(_node_section) => { > - return Ok(()); > + Node::WireGuard(node_section) => { > + if let WireGuardNode::Internal(internal_node) = node_section.properties() { > + if !internal_node.interfaces().all(|interface| { > + wireguard_interfaces.insert((node_id, interface.name.as_str())) > + }) { > + return Err(FabricConfigError::DuplicateInterface); > + } > + } > } > } > } > @@ -969,3 +1016,93 @@ impl Valid { > Section::write_section_config("fabrics.cfg", &self.into_section_config()) > } > } > + > +#[cfg(test)] > +mod tests { > + use crate::sdn::fabric::FabricConfig; > + use proxmox_section_config::typed::ApiSectionDataEntry; > + > + use super::*; > + > + #[test] > + fn test_wireguard_validation_duplicate_interface() -> Result<(), anyhow::Error> { > + let section_config = r#" > +wireguard_fabric: wireg > + > +wireguard_node: wireg_internal > + role internal > + endpoint 192.0.2.1:123 > + public_key Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51112,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > +"#; > + let parsed_config = Section::parse_section_config("fabrics.cfg", section_config)?; > + FabricConfig::from_section_config(parsed_config) > + .expect_err("duplicate interface name on node"); > + > + Ok(()) > + } > + > + #[test] > + fn test_wireguard_validation_duplicate_listen_port() -> Result<(), anyhow::Error> { > + let section_config = r#" > +wireguard_fabric: wireg > + > +wireguard_node: wireg_internal > + role internal > + endpoint 192.0.2.1:123 > + public_key Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg1,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > +"#; > + let parsed_config = Section::parse_section_config("fabrics.cfg", section_config)?; > + FabricConfig::from_section_config(parsed_config) > + .expect_err("duplicate listen_port on node"); > + > + Ok(()) > + } > + > + #[test] > + fn test_wireguard_validation_node_interface_does_not_exist() -> Result<(), anyhow::Error> { > + let section_config = r#" > +wireguard_fabric: wireg > + > +wireguard_node: wireg_internal > + role internal > + endpoint 192.0.2.1:123 > + public_key Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + peers type=internal,node=invalid,node_iface=invalid,iface=wg0 > +"#; > + let parsed_config = Section::parse_section_config("fabrics.cfg", section_config)?; > + FabricConfig::from_section_config(parsed_config) > + .expect_err("interface referenced in peer definition does not exist"); > + > + Ok(()) > + } > + > + #[test] > + fn test_wireguard_validation_local_interface_does_not_exist() -> Result<(), anyhow::Error> { > + let section_config = r#" > +wireguard_fabric: wireg > + > +wireguard_node: wireg_internal > + role internal > + endpoint 192.0.2.1:123 > + public_key Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + > +wireguard_node: wireg_internal2 > + role internal > + endpoint 192.0.2.2:123 > + public_key Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + interfaces name=wg0,listen_port=51111,public_key=Kay64UG8yvCyLhqU000LxzYeUm0L/hLIl5S8kyKWbdc= > + peers type=internal,node=internal,node_iface=wg0,iface=wg1 > +"#; > + let parsed_config = Section::parse_section_config("fabrics.cfg", section_config)?; > + FabricConfig::from_section_config(parsed_config) > + .expect_err("local interface in peer definition does not exist"); > + > + Ok(()) > + } > +} > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/node.rs b/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > index 437428b..69c222d 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/node.rs > @@ -227,7 +227,7 @@ impl Validatable for Node { > match self { > Node::Openfabric(node_section) => node_section.validate(), > Node::Ospf(node_section) => node_section.validate(), > - Node::WireGuard(_node_section) => Ok(()), > + Node::WireGuard(node_section) => node_section.validate(), > } > } > } > diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/wireguard.rs b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/wireguard.rs > index e885ef8..78bbcb4 100644 > --- a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/wireguard.rs > +++ b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/wireguard.rs > @@ -28,6 +28,7 @@ > //! definition can be overridden in the peer definition, if e.g. a different endpoint is required > //! for connecting to a node. > > +use std::collections::HashSet; > use std::ops::{Deref, DerefMut}; > > use anyhow::Result; > @@ -44,7 +45,10 @@ use proxmox_sdn_types::wireguard::PersistentKeepalive; > use proxmox_wireguard::PublicKey; > use serde::{Deserialize, Serialize}; > > -use crate::sdn::fabric::section_config::node::NodeId; > +use crate::common::valid::Validatable; > +use crate::sdn::fabric::section_config::fabric::FabricSection; > +use crate::sdn::fabric::section_config::node::{NodeId, NodeSection}; > +use crate::sdn::fabric::FabricConfigError; > > pub const WIREGUARD_INTERFACE_NAME_REGEX_STR: &str = "[a-zA-Z0-9][a-zA-Z0-9-]{0,6}[a-zA-Z0-9]?"; > > @@ -79,6 +83,14 @@ pub struct WireGuardProperties { > pub(crate) persistent_keepalive: Option, > } > > +impl Validatable for FabricSection { > + type Error = FabricConfigError; > + > + fn validate(&self) -> Result<(), Self::Error> { > + Ok(()) > + } > +} > + > #[derive(Clone, Debug, Serialize, Deserialize)] > #[serde(rename_all = "snake_case")] > pub enum WireGuardDeletableProperties { > @@ -159,6 +171,18 @@ impl ApiType for WireGuardNode { > .schema(); > } > > +impl Validatable for NodeSection { > + type Error = FabricConfigError; > + > + fn validate(&self) -> Result<(), Self::Error> { > + if let WireGuardNode::Internal(node) = self.properties() { > + return node.validate(); > + } > + > + Ok(()) > + } > +} > + > #[derive(Debug, Clone, Serialize, Deserialize, Hash)] > #[serde(rename_all = "snake_case", tag = "role")] > pub enum WireGuardNodeUpdater { > @@ -291,6 +315,45 @@ impl InternalWireGuardNode { > } > } > > +impl Validatable for InternalWireGuardNode { > + type Error = FabricConfigError; > + > + /// Validates the [FabricSection]. > + /// > + /// Checks if we have either an IPv4 or an IPv6 address. If neither is set, return an error. Looks like this doc comment is the result of a copy-paste error > + fn validate(&self) -> Result<(), Self::Error> { > + let mut local_interfaces = HashSet::new(); > + let mut listen_ports = HashSet::new(); > + > + for interface in self.interfaces() { > + // check if interface names are unique > + if !local_interfaces.insert(&interface.name) { > + return Err(FabricConfigError::DuplicateInterface); > + } [...]