diff --git a/common/src/network.rs b/common/src/network.rs index e6935fbb..b5f55ae3 100644 --- a/common/src/network.rs +++ b/common/src/network.rs @@ -41,14 +41,31 @@ pub fn generate_ipv6_link_local(mac: MacAddr) -> Ipv6Addr { #[derive(Error, Debug, Clone)] pub enum VlanError { /// Not a valid VLAN ID - #[error("Invalid VLAN tag: {}", .0)] + #[error("Invalid VLAN ID {}, must be in the range 1..=4094", .0)] InvalidVlan(u16), } +/// Validate a VLAN ID against the configurable range `1..=4094`. +/// +/// Per [IEEE 802.1Q] ยง9.6, VID 0 is the null VID used for priority tagging +/// and carries no VLAN membership, and VID 4095 is reserved for +/// implementation use and must not be configured. VID 1 is the default +/// port VID. +/// +/// # Errors +/// +/// Returns [`VlanError::InvalidVlan`] if the ID falls outside `1..=4094`. +/// +/// [IEEE 802.1Q]: https://ieeexplore.ieee.org/document/10004498 +// TODO: replace with a validated `VlanId` newtype in oxnet, shared with +// omicron's `common/src/vlan.rs` bounds check. pub fn validate_vlan(id: impl Into) -> Result<(), VlanError> { let id: u16 = id.into(); - #[allow(clippy::manual_range_contains)] - if id < 2 || id > 4095 { Err(VlanError::InvalidVlan(id)) } else { Ok(()) } + if !(1..=4094).contains(&id) { + Err(VlanError::InvalidVlan(id)) + } else { + Ok(()) + } } #[cfg(test)] diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 1a97c6e2..b98b8e76 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -552,7 +552,7 @@ async fn test_group_creation_with_validation() -> TestResult { nat_target: Some(nat_target.clone()), }, external_forwarding: types::ExternalForwarding { - vlan_id: Some(4096), // Invalid: VLAN ID must be 1-4095 + vlan_id: Some(4096), // Invalid: VLAN ID must be 1-4094 }, sources: None, }; @@ -574,7 +574,7 @@ async fn test_group_creation_with_validation() -> TestResult { _ => panic!("Expected ErrorResponse for invalid group ID"), } - // Test with reserved VLAN ID 0 (also invalid) + // Test with the null VLAN ID 0 (also invalid) let external_vlan0 = types::MulticastGroupCreateExternalEntry { group_ip: IpAddr::V4(MULTICAST_TEST_IPV4), tag: Some(TEST_TAG.to_string()), @@ -582,7 +582,7 @@ async fn test_group_creation_with_validation() -> TestResult { nat_target: Some(nat_target.clone()), }, external_forwarding: types::ExternalForwarding { - vlan_id: Some(0), // Invalid: VLAN 0 is reserved + vlan_id: Some(0), // Invalid: VLAN 0 is the null VID }, sources: None, }; @@ -591,7 +591,7 @@ async fn test_group_creation_with_validation() -> TestResult { .client .multicast_group_create_external(&external_vlan0) .await - .expect_err("Should fail with reserved VLAN ID 0"); + .expect_err("Should fail with null VLAN ID 0"); match res { Error::ErrorResponse(inner) => { @@ -601,37 +601,37 @@ async fn test_group_creation_with_validation() -> TestResult { "Expected 400 Bad Request status code for VLAN 0" ); } - _ => panic!("Expected ErrorResponse for reserved VLAN ID 0"), + _ => panic!("Expected ErrorResponse for null VLAN ID 0"), } - // Test with reserved VLAN ID 1 (also invalid) - let external_vlan1 = types::MulticastGroupCreateExternalEntry { + // Test with reserved VLAN ID 4095 (also invalid) + let external_vlan4095 = types::MulticastGroupCreateExternalEntry { group_ip: IpAddr::V4(MULTICAST_TEST_IPV4), tag: Some(TEST_TAG.to_string()), internal_forwarding: types::InternalForwarding { nat_target: Some(nat_target.clone()), }, external_forwarding: types::ExternalForwarding { - vlan_id: Some(1), // Invalid: VLAN 1 is reserved + vlan_id: Some(4095), // Invalid: VLAN 4095 is reserved }, sources: None, }; let res = switch .client - .multicast_group_create_external(&external_vlan1) + .multicast_group_create_external(&external_vlan4095) .await - .expect_err("Should fail with reserved VLAN ID 1"); + .expect_err("Should fail with reserved VLAN ID 4095"); match res { Error::ErrorResponse(inner) => { assert_eq!( inner.status(), 400, - "Expected 400 Bad Request status code for VLAN 1" + "Expected 400 Bad Request status code for VLAN 4095" ); } - _ => panic!("Expected ErrorResponse for reserved VLAN ID 1"), + _ => panic!("Expected ErrorResponse for reserved VLAN ID 4095"), } // Test with valid parameters diff --git a/uplinkd/src/main.rs b/uplinkd/src/main.rs index 08d3c83a..9e6e91b4 100644 --- a/uplinkd/src/main.rs +++ b/uplinkd/src/main.rs @@ -110,11 +110,10 @@ struct Opt { } fn parse_vlan_id(vlan_id: &str) -> Result, anyhow::Error> { - match vlan_id.parse() { - Err(_) => Err(anyhow!("invalid vlan id: {vlan_id}")), - Ok(vlan_id) if vlan_id > 1 && vlan_id < 4096 => Ok(Some(vlan_id)), - Ok(vlan_id) => Err(anyhow!("vlan id out of range: {vlan_id}")), - } + let id: u16 = + vlan_id.parse().map_err(|_| anyhow!("invalid vlan id: {vlan_id}"))?; + common::network::validate_vlan(id)?; + Ok(Some(id)) } // Given an interface, return a tuple with the underlying link name and any diff --git a/uplinkd/src/sys.rs b/uplinkd/src/sys.rs index 33633e7e..b3a5f660 100644 --- a/uplinkd/src/sys.rs +++ b/uplinkd/src/sys.rs @@ -64,17 +64,13 @@ impl FromStr for UplinkAddress { let address = address .parse() .map_err(|_| anyhow!("not a valid ip address: {address}"))?; - let vlan_id = match vlan_id { - None => Ok(None), - Some(v) => match v.parse() { - Err(_) => Err(anyhow!("invalid vlan id: {v}")), - Ok(vlan_id) if vlan_id > 1 && vlan_id < 4096 => { - Ok(Some(vlan_id)) - } - Ok(vlan_id) => Err(anyhow!("vlan id out of range: {vlan_id}")), - }, - }?; - Ok(UplinkAddress { address, vlan_id }) + let vlan_id = vlan_id + .map(|v| { + v.parse::() + .map_err(|_| anyhow!("invalid vlan id: {v}")) + }) + .transpose()?; + UplinkAddress::new(address, vlan_id) } }