Port/ICMP parameter ranges should enforce first <= last (#8578)

The console correctly prevents ranges like `3000-1000` for
`L4PortRange`s, but we don't catch this in the API itself. This PR
enforces that on the frontend, with some allowances for the fact that we
might still have such rows in CRDB.

I was torn between enforcing this using transparent types to apply the
first/last constraint, which worked in terms of being invisible in the
JsonSchema, versus what I did here which is just checking the invariant
on conversion to a db::VpcFirewallRule. Unfortunately the former was far
messier, and ended up splitting the logic such that it was a little
harder to intuit what was checked and where. So, we validate this next
to the filter array length checks, which felt fairly natural.

Author: FelixMcFelix

common/src/api/external/mod.rs

@@ -1891,10 +1891,16 @@ impl From<u8> for IcmpParamRange {
     }
 }
 
-impl From<RangeInclusive<u8>> for IcmpParamRange {
-    fn from(value: RangeInclusive<u8>) -> Self {
-        let (first, last) = value.into_inner();
-        Self { first, last }
+impl TryFrom<RangeInclusive<u8>> for IcmpParamRange {
+    type Error = IcmpParamRangeError;
+
+    fn try_from(value: RangeInclusive<u8>) -> Result<Self, Self::Error> {
+        if value.is_empty() {
+            Err(IcmpParamRangeError::EmptyRange)
+        } else {
+            let (first, last) = value.into_inner();
+            Ok(Self { first, last })
+        }
     }
 }
 
@@ -2052,6 +2058,7 @@ impl FromStr for L4PortRange {
                     .parse::<NonZeroU16>()
                     .map_err(|e| L4PortRangeError::Value(right.into(), e))?
                     .into();
+
                 Ok(L4PortRange { first, last })
             }
         }
@@ -2064,6 +2071,8 @@ pub enum L4PortRangeError {
     MissingStart,
     #[error("range has no end value")]
     MissingEnd,
+    #[error("range has larger start value than end value")]
+    EmptyRange,
     #[error("{0:?} unparsable for type: {1}")]
     Value(String, ParseIntError),
 }
@@ -2132,10 +2141,16 @@ impl From<L4Port> for L4PortRange {
     }
 }
 
-impl From<RangeInclusive<L4Port>> for L4PortRange {
-    fn from(value: RangeInclusive<L4Port>) -> Self {
-        let (first, last) = value.into_inner();
-        Self { first, last }
+impl TryFrom<RangeInclusive<L4Port>> for L4PortRange {
+    type Error = L4PortRangeError;
+
+    fn try_from(value: RangeInclusive<L4Port>) -> Result<Self, Self::Error> {
+        if value.is_empty() {
+            Err(L4PortRangeError::EmptyRange)
+        } else {
+            let (first, last) = value.into_inner();
+            Ok(Self { first, last })
+        }
     }
 }
 
@@ -2169,6 +2184,7 @@ impl FromStr for IcmpParamRange {
                 let last = right
                     .parse::<u8>()
                     .map_err(|e| IcmpParamRangeError::Value(right.into(), e))?;
+
                 Ok(IcmpParamRange { first, last })
             }
         }
@@ -2181,6 +2197,8 @@ pub enum IcmpParamRangeError {
     MissingStart,
     #[error("range has no end value")]
     MissingEnd,
+    #[error("range has larger start value than end value")]
+    EmptyRange,
     #[error("{0:?} unparsable for type: {1}")]
     Value(String, ParseIntError),
 }
@@ -4125,7 +4143,7 @@ mod test {
         assert_eq!(
             VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter {
                 icmp_type: 60,
-                code: Some((0..=10).into())
+                code: Some((0..=10).try_into().unwrap())
             })),
             "icmp:60,0-10".parse().unwrap()
         );

nexus/db-fixed-data/src/vpc_firewall_rule.rs

@@ -106,7 +106,7 @@ pub static NEXUS_ICMP_FW_RULE: LazyLock<VpcFirewallRuleUpdate> =
                     // Type 3 -- Destination Unreachable
                     icmp_type: 3,
                     // Codes 3,4 -- Port Unreachable, Fragmentation needed
-                    code: Some((3..=4).into()),
+                    code: Some((3..=4).try_into().expect("3 <= 4")),
                 })),
                 VpcFirewallRuleProtocol::Icmp(Some(VpcFirewallIcmpFilter {
                     // Type 5 -- Redirect

nexus/db-model/src/vpc_firewall_rule.rs

@@ -211,6 +211,39 @@ fn ensure_max_len<T>(
     Ok(())
 }
 
+fn validate_port_ranges(
+    items: &[external::L4PortRange],
+) -> Result<(), external::Error> {
+    for range in items {
+        if range.last < range.first {
+            return Err(external::L4PortRangeError::EmptyRange.into());
+        }
+    }
+    Ok(())
+}
+
+fn validate_protocols(
+    items: &[external::VpcFirewallRuleProtocol],
+) -> Result<(), external::Error> {
+    for proto in items {
+        if let external::VpcFirewallRuleProtocol::Icmp(Some(
+            external::VpcFirewallIcmpFilter {
+                code: Some(external::IcmpParamRange { first, last }),
+                ..
+            },
+        )) = proto
+        {
+            if last < first {
+                return Err(external::Error::invalid_value(
+                    "code",
+                    external::IcmpParamRangeError::EmptyRange.to_string(),
+                ));
+            }
+        }
+    }
+    Ok(())
+}
+
 impl VpcFirewallRule {
     pub fn new(
         rule_id: Uuid,
@@ -232,9 +265,11 @@ impl VpcFirewallRule {
         }
         if let Some(ports) = rule.filters.ports.as_ref() {
             ensure_max_len(&ports, "filters.ports", MAX_FW_RULE_PARTS)?;
+            validate_port_ranges(ports.as_slice())?;
         }
         if let Some(protocols) = rule.filters.protocols.as_ref() {
             ensure_max_len(&protocols, "filters.protocols", MAX_FW_RULE_PARTS)?;
+            validate_protocols(protocols.as_slice())?;
         }
 
         Ok(Self {

nexus/tests/integration_tests/vpc_firewall.rs

@@ -17,11 +17,12 @@ use nexus_test_utils::resource_helpers::{
 use nexus_test_utils_macros::nexus_test;
 use nexus_types::external_api::views::Vpc;
 use omicron_common::api::external::{
-    IdentityMetadata, L4Port, L4PortRange, ServiceIcmpConfig, VpcFirewallRule,
-    VpcFirewallRuleAction, VpcFirewallRuleDirection, VpcFirewallRuleFilter,
-    VpcFirewallRuleHostFilter, VpcFirewallRulePriority,
-    VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget,
-    VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, VpcFirewallRules,
+    IcmpParamRange, IdentityMetadata, L4Port, L4PortRange, ServiceIcmpConfig,
+    VpcFirewallIcmpFilter, VpcFirewallRule, VpcFirewallRuleAction,
+    VpcFirewallRuleDirection, VpcFirewallRuleFilter, VpcFirewallRuleHostFilter,
+    VpcFirewallRulePriority, VpcFirewallRuleProtocol, VpcFirewallRuleStatus,
+    VpcFirewallRuleTarget, VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams,
+    VpcFirewallRules,
 };
 use omicron_nexus::Nexus;
 use std::convert::TryFrom;
@@ -591,6 +592,68 @@ async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) {
     );
 }
 
+#[nexus_test]
+async fn test_firewall_rules_illegal_range(
+    cptestctx: &ControlPlaneTestContext,
+) {
+    let client = &cptestctx.external_client;
+
+    let project_name = "my-project";
+    create_project(&client, &project_name).await;
+
+    let mut rule = VpcFirewallRuleUpdate {
+        name: "illegal-range".parse().unwrap(),
+        description: "".to_string(),
+        status: VpcFirewallRuleStatus::Enabled,
+        direction: VpcFirewallRuleDirection::Inbound,
+        targets: vec![],
+        filters: VpcFirewallRuleFilter {
+            hosts: None,
+            protocols: Some(vec![VpcFirewallRuleProtocol::Icmp(Some(
+                VpcFirewallIcmpFilter {
+                    icmp_type: 0,
+                    code: Some(IcmpParamRange { first: 21, last: 20 }),
+                },
+            ))]),
+            ports: None,
+        },
+        action: VpcFirewallRuleAction::Allow,
+        priority: VpcFirewallRulePriority(65534),
+    };
+
+    let error = object_put_error(
+        client,
+        &format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name),
+        &VpcFirewallRuleUpdateParams { rules: vec![rule.clone()] },
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(error.error_code, Some("InvalidValue".to_string()));
+    assert_eq!(
+        error.message,
+        "unsupported value for \"code\": range has larger start value than end value"
+    );
+
+    rule.filters.protocols = None;
+    rule.filters.ports = Some(vec![L4PortRange {
+        first: L4Port::try_from(1000).unwrap(),
+        last: L4Port::try_from(900).unwrap(),
+    }]);
+
+    let error = object_put_error(
+        client,
+        &format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name),
+        &VpcFirewallRuleUpdateParams { rules: vec![rule] },
+        StatusCode::BAD_REQUEST,
+    )
+    .await;
+    assert_eq!(error.error_code, Some("InvalidValue".to_string()));
+    assert_eq!(
+        error.message,
+        "unsupported value for \"l4_port_range\": range has larger start value than end value"
+    );
+}
+
 async fn icmp_rule_is_enabled(
     enabled: bool,
     datastore: &DataStore,