diff --git a/nexus/db-model/src/vpc_firewall_rule.rs b/nexus/db-model/src/vpc_firewall_rule.rs index 120d3e14738..9c6a02e098c 100644 --- a/nexus/db-model/src/vpc_firewall_rule.rs +++ b/nexus/db-model/src/vpc_firewall_rule.rs @@ -211,12 +211,34 @@ pub struct VpcFirewallRule { pub priority: VpcFirewallRulePriority, } +/// Cap on the number of rules in a VPC. Also used as a max length on other vecs +/// in the rule body. + +/// We could have 5 constants, but don't bother unless they need to be +/// different. The value is meant to be big enough that customers will rarely +/// run into it in practice. We just want to prevent a single request from being +/// able to write tons of rows to the DB or blow up OPTE. +const MAX_FW_RULES_PER_VPC: usize = 512; + +fn ensure_max_length( + items: &Vec, + label: &str, +) -> Result<(), external::Error> { + if items.len() > MAX_FW_RULES_PER_VPC { + return Err(external::Error::invalid_value( + label, + format!("max length {}", MAX_FW_RULES_PER_VPC), + )); + } + Ok(()) +} + impl VpcFirewallRule { pub fn new( rule_id: Uuid, vpc_id: Uuid, rule: &external::VpcFirewallRuleUpdate, - ) -> Self { + ) -> Result { let identity = VpcFirewallRuleIdentity::new( rule_id, external::IdentityMetadataCreateParams { @@ -224,7 +246,20 @@ impl VpcFirewallRule { description: rule.description.clone(), }, ); - Self { + + ensure_max_length(&rule.targets, "targets")?; + + if let Some(hosts) = rule.filters.hosts.as_ref() { + ensure_max_length(&hosts, "filters.hosts")?; + } + if let Some(ports) = rule.filters.ports.as_ref() { + ensure_max_length(&ports, "filters.ports")?; + } + if let Some(protocols) = rule.filters.protocols.as_ref() { + ensure_max_length(&protocols, "filters.protocols")?; + } + + Ok(Self { identity, vpc_id, status: rule.status.into(), @@ -248,7 +283,7 @@ impl VpcFirewallRule { }), action: rule.action.into(), priority: rule.priority.into(), - } + }) } pub fn vec_from_params( @@ -256,11 +291,12 @@ impl VpcFirewallRule { params: external::VpcFirewallRuleUpdateParams, ) -> Result, external::Error> { ensure_no_duplicates(¶ms)?; - Ok(params + ensure_max_length(¶ms.rules, "rules")?; + params .rules - .iter() - .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, rule)) - .collect()) + .into_iter() + .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, &rule)) + .collect() } } diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 5322e20dbfb..3b33628f248 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -195,20 +195,24 @@ impl DataStore { .map(|rule| (rule.name().clone(), rule)) .collect::>(); - fw_rules.entry(DNS_VPC_FW_RULE.name.clone()).or_insert_with(|| { - VpcFirewallRule::new( + // these have to be done this way because the contructor returns a result + if !fw_rules.contains_key(&DNS_VPC_FW_RULE.name) { + let rule = VpcFirewallRule::new( Uuid::new_v4(), *SERVICES_VPC_ID, &DNS_VPC_FW_RULE, - ) - }); - fw_rules.entry(NEXUS_VPC_FW_RULE.name.clone()).or_insert_with(|| { - VpcFirewallRule::new( + )?; + fw_rules.insert(DNS_VPC_FW_RULE.name.clone(), rule); + } + + if !fw_rules.contains_key(&NEXUS_VPC_FW_RULE.name) { + let rule = VpcFirewallRule::new( Uuid::new_v4(), *SERVICES_VPC_ID, &NEXUS_VPC_FW_RULE, - ) - }); + )?; + fw_rules.insert(NEXUS_VPC_FW_RULE.name.clone(), rule); + } let rules = fw_rules .into_values() diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 83379bef888..ecb661d34ba 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -6,7 +6,7 @@ use http::method::Method; use http::StatusCode; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; use nexus_test_utils::resource_helpers::{ - create_project, create_vpc, object_get, object_put_error, + create_project, create_vpc, object_get, object_put, object_put_error, }; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::views::Vpc; @@ -321,3 +321,248 @@ async fn test_firewall_rules_same_name(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.error_code, Some("InvalidValue".to_string())); assert_eq!(error.message, "unsupported value for \"rules\": Rule names must be unique. Duplicates: [\"dupe\"]"); } + +#[nexus_test] +async fn test_firewall_rules_max_lengths(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + let project_name = "my-project"; + create_project(&client, &project_name).await; + + let base_rule = VpcFirewallRuleUpdate { + name: "my-rule".parse().unwrap(), + description: "".to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets: vec![], + filters: VpcFirewallRuleFilter { + hosts: None, + protocols: None, + ports: None, + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }; + + let rule_n = |i: usize| VpcFirewallRuleUpdate { + name: format!("rule{}", i).parse().unwrap(), + ..base_rule.clone() + }; + + let url = + format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name); + + // fine with max count + let max: usize = 512; + let rules: VpcFirewallRules = object_put( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: (0..max).map(rule_n).collect::>(), + }, + ) + .await; + assert_eq!(rules.rules.len(), max); + + // fails with max + 1 + let error = object_put_error( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: (0..max + 1).map(rule_n).collect::>(), + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!( + error.message, + format!("unsupported value for \"rules\": max length {max}") + ); + + /////////////////////// + // TARGETS + /////////////////////// + + let target = VpcFirewallRuleTarget::Vpc("default".parse().unwrap()); + + // fine with max + let rules: VpcFirewallRules = object_put( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + targets: (0..max).map(|_i| target.clone()).collect(), + ..base_rule.clone() + }], + }, + ) + .await; + assert_eq!(rules.rules[0].targets.len(), max); + + // fails with max + 1 + let error = object_put_error( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + targets: (0..max + 1).map(|_i| target.clone()).collect(), + ..base_rule.clone() + }], + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!( + error.message, + format!("unsupported value for \"targets\": max length {max}") + ); + + /////////////////////// + // HOST FILTERS + /////////////////////// + + let host = VpcFirewallRuleHostFilter::Vpc("default".parse().unwrap()); + + // fine with max + let rules: VpcFirewallRules = object_put( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + hosts: Some((0..max).map(|_i| host.clone()).collect()), + protocols: None, + ports: None, + }, + ..base_rule.clone() + }], + }, + ) + .await; + assert_eq!(rules.rules[0].filters.hosts.as_ref().unwrap().len(), max); + + // fails with max + 1 + let error = object_put_error( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + hosts: Some((0..max + 1).map(|_i| host.clone()).collect()), + protocols: None, + ports: None, + }, + ..base_rule.clone() + }], + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!( + error.message, + format!("unsupported value for \"filters.hosts\": max length {max}") + ); + + /////////////////////// + // PORT FILTERS + /////////////////////// + + let port: L4PortRange = "1234".parse().unwrap(); + + // fine with max + let rules: VpcFirewallRules = object_put( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + ports: Some((0..max).map(|_i| port.clone()).collect()), + protocols: None, + hosts: None, + }, + ..base_rule.clone() + }], + }, + ) + .await; + assert_eq!(rules.rules[0].filters.ports.as_ref().unwrap().len(), max); + + // fails with max + 1 + let error = object_put_error( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + ports: Some((0..max + 1).map(|_i| port.clone()).collect()), + protocols: None, + hosts: None, + }, + ..base_rule.clone() + }], + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!( + error.message, + format!("unsupported value for \"filters.ports\": max length {max}") + ); + + /////////////////////// + // PROTOCOL FILTERS + /////////////////////// + + let protocol = VpcFirewallRuleProtocol::Tcp; + + // fine with max + let rules: VpcFirewallRules = object_put( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + protocols: Some( + (0..max).map(|_i| protocol.clone()).collect(), + ), + ports: None, + hosts: None, + }, + ..base_rule.clone() + }], + }, + ) + .await; + assert_eq!(rules.rules[0].filters.protocols.as_ref().unwrap().len(), max); + + // fails with max + 1 + let error = object_put_error( + client, + &url, + &VpcFirewallRuleUpdateParams { + rules: vec![VpcFirewallRuleUpdate { + filters: VpcFirewallRuleFilter { + protocols: Some( + (0..max + 1).map(|_i| protocol.clone()).collect(), + ), + ports: None, + hosts: None, + }, + ..base_rule.clone() + }], + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!( + error.message, + format!( + "unsupported value for \"filters.protocols\": max length {max}" + ) + ); +}