Re: [PATCH proxmox-ve-rs v4 12/31] ve-config: fabrics: wireguard add validation for wireguard config

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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 <s.hanreich@proxmox.com>
> ---
>  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<FabricConfig> {
>          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<PersistentKeepalive>,
>  }
>  
> +impl Validatable for FabricSection<WireGuardProperties> {
> +    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<WireGuardNode> {
> +    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<WireGuardNodeProperties>].
> +    ///
> +    /// 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);
> +            }
[...]