From 381b1d2c0f6ab19cbe389792ecf21d7f35b2e324 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 2 May 2024 22:57:22 +0000 Subject: [PATCH] More WIP - Add newtype for IP allowlist that checks for empty lists on construction. This statically guarantees we have a non-empty list, so we can remove some panics and centralize conversion / checking. - Modify existing firewall rules for the internal services VPC, rather than adding new ones --- common/src/api/internal/shared.rs | 75 ++++++- nexus/db-model/src/allowed_source_ip.rs | 36 ++- .../src/db/datastore/allowed_source_ips.rs | 34 +-- nexus/networking/src/firewall_rules.rs | 207 +++++++----------- nexus/src/app/allowed_source_ips.rs | 7 +- sled-agent/src/bootstrap/params.rs | 2 +- sled-agent/src/rack_setup/config.rs | 3 +- sled-agent/src/rack_setup/plan/service.rs | 3 +- 8 files changed, 186 insertions(+), 181 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index eb9eb41075e..a757401c9b9 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -432,7 +432,77 @@ pub enum AllowedSourceIps { /// Restrict access to a specific set of source IP addresses or subnets. /// /// All others are prevented from reaching rack services. - List(Vec), + List(IpAllowList), +} + +impl TryFrom> for AllowedSourceIps { + type Error = &'static str; + fn try_from(list: Vec) -> Result { + IpAllowList::try_from(list).map(Self::List) + } +} + +impl TryFrom<&[IpNetwork]> for AllowedSourceIps { + type Error = &'static str; + fn try_from(list: &[IpNetwork]) -> Result { + IpAllowList::try_from(list).map(Self::List) + } +} + +/// A non-empty allowlist of IP addresses or subnets +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(try_from = "Vec", into = "Vec")] +#[schemars(transparent)] +pub struct IpAllowList(Vec); + +impl IpAllowList { + /// Return the entries of the list as a slice. + pub fn as_slice(&self) -> &[IpNet] { + &self.0 + } + + /// Return an iterator over the entries of the list. + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } + + /// Consume the list into an iterator. + pub fn into_iter(self) -> impl Iterator { + self.0.into_iter() + } + + /// Return the number of entries in the allowlist. + /// + /// Note that this is always >= 1, though we return a usize for simplicity. + pub fn len(&self) -> usize { + self.0.len() + } +} + +impl From for Vec { + fn from(list: IpAllowList) -> Self { + list.0 + } +} + +impl TryFrom> for IpAllowList { + type Error = &'static str; + fn try_from(list: Vec) -> Result { + if list.is_empty() { + return Err("IP allowlist must not be empty"); + } + Ok(Self(list)) + } +} + +impl TryFrom<&[IpNetwork]> for IpAllowList { + type Error = &'static str; + fn try_from(list: &[IpNetwork]) -> Result { + if list.is_empty() { + return Err("IP allowlist must not be empty"); + } + Ok(Self(list.iter().copied().map(Into::into).collect())) + } } #[cfg(test)] @@ -452,7 +522,7 @@ mod tests { .unwrap(); assert_eq!( parsed, - AllowedSourceIps::List(vec![ + AllowedSourceIps::try_from(vec![ IpNet::from(Ipv4Addr::LOCALHOST), IpNet::V4(Ipv4Net( Ipv4Network::new(Ipv4Addr::new(10, 0, 0, 0), 24).unwrap() @@ -465,6 +535,7 @@ mod tests { .unwrap() )), ]) + .unwrap() ); } diff --git a/nexus/db-model/src/allowed_source_ip.rs b/nexus/db-model/src/allowed_source_ip.rs index a56bf539d37..389eed2e0c6 100644 --- a/nexus/db-model/src/allowed_source_ip.rs +++ b/nexus/db-model/src/allowed_source_ip.rs @@ -15,7 +15,6 @@ use nexus_types::external_api::params; use nexus_types::external_api::views; use omicron_common::api::external; use omicron_common::api::external::Error; -use omicron_common::api::external::IpNet; use serde::Deserialize; use serde::Serialize; use uuid::Uuid; @@ -46,20 +45,17 @@ impl AllowedSourceIp { } /// Create an `AllowedSourceIps` type from the contained address. - pub fn allowed_source_ips(&self) -> Result { + pub fn allowed_source_ips( + &self, + ) -> Result { match &self.allowed_ips { - Some(list) => { - if list.is_empty() { - Err(Error::internal_error( + Some(list) => external::AllowedSourceIps::try_from(list.as_slice()) + .map_err(|_| { + Error::internal_error( "Allowlist from database is empty, but NULL \ - should be used to allow any source IP" - )) - } else { - Ok(external::AllowedSourceIps::List( - list.iter().copied().map(IpNet::from).collect(), - )) - } - } + should be used to allow any source IP", + ) + }), None => Ok(external::AllowedSourceIps::Any), } } @@ -88,14 +84,10 @@ impl TryFrom for views::AllowedSourceIps { type Error = Error; fn try_from(db: AllowedSourceIp) -> Result { - db - .allowed_source_ips() - .map(|allowed_ips| { - Self { - time_created: db.time_created, - time_modified: db.time_modified, - allowed_ips, - } - }) + db.allowed_source_ips().map(|allowed_ips| Self { + time_created: db.time_created, + time_modified: db.time_modified, + allowed_ips, + }) } } diff --git a/nexus/db-queries/src/db/datastore/allowed_source_ips.rs b/nexus/db-queries/src/db/datastore/allowed_source_ips.rs index d2cc112d3f0..9bd5cef52f6 100644 --- a/nexus/db-queries/src/db/datastore/allowed_source_ips.rs +++ b/nexus/db-queries/src/db/datastore/allowed_source_ips.rs @@ -87,10 +87,9 @@ mod tests { datastore::test_utils::datastore_test, fixed_data::allowed_source_ips::ALLOWED_SOURCE_IPS_ID, }; - use nexus_db_model::AllowedSourceIp; use nexus_test_utils::db::test_setup_database; use omicron_common::api::external::{ - self, Error, IpNet, LookupType, ResourceType, + self, Error, LookupType, ResourceType, }; use omicron_test_utils::dev; @@ -114,22 +113,11 @@ mod tests { "Expected an ObjectNotFound error when there is no IP allowlist" ); - // Helper to get IP addresses from a record. - fn get_ips(record: &AllowedSourceIp) -> Vec { - record - .allowed_ips - .as_ref() - .expect("Record should have non-NULL allowlist") - .iter() - .copied() - .map(IpNet::from) - .collect() - } - // Upsert an allowlist, with some specific IPs. let ips = vec!["10.0.0.0/8".parse().unwrap(), "::1/64".parse().unwrap()]; - let allowed_ips = external::AllowedSourceIps::List(ips.clone()); + let allowed_ips = + external::AllowedSourceIps::try_from(ips.as_slice()).unwrap(); let record = datastore .allowed_source_ips_upsert(&opctx, allowed_ips) .await @@ -140,14 +128,15 @@ mod tests { ); assert_eq!( ips, - get_ips(&record), + record.allowed_ips.unwrap(), "Inserted and re-read incorrect allowed source ips" ); // Upsert a new list, verify it's changed. let new_ips = vec!["10.0.0.0/4".parse().unwrap(), "fd00::10/32".parse().unwrap()]; - let allowed_ips = external::AllowedSourceIps::List(new_ips.clone()); + let allowed_ips = + external::AllowedSourceIps::try_from(new_ips.as_slice()).unwrap(); let new_record = datastore .allowed_source_ips_upsert(&opctx, allowed_ips) .await @@ -165,8 +154,8 @@ mod tests { "Time modified should have changed" ); assert_eq!( - new_ips, - get_ips(&new_record), + &new_ips, + new_record.allowed_ips.as_ref().unwrap(), "Updated allowed IPs are incorrect" ); @@ -196,7 +185,8 @@ mod tests { // Lastly change it back to a real list again. let record = new_record; - let allowed_ips = external::AllowedSourceIps::List(new_ips.clone()); + let allowed_ips = + external::AllowedSourceIps::try_from(new_ips.as_slice()).unwrap(); let new_record = datastore .allowed_source_ips_upsert(&opctx, allowed_ips) .await @@ -214,8 +204,8 @@ mod tests { "Time modified should have changed" ); assert_eq!( - new_ips, - get_ips(&new_record), + &new_ips, + new_record.allowed_ips.as_ref().unwrap(), "Updated allowed IPs are incorrect" ); diff --git a/nexus/networking/src/firewall_rules.rs b/nexus/networking/src/firewall_rules.rs index 45bb9630a17..ba1e0abac32 100644 --- a/nexus/networking/src/firewall_rules.rs +++ b/nexus/networking/src/firewall_rules.rs @@ -9,7 +9,6 @@ use ipnetwork::IpNetwork; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; -use nexus_db_queries::db::datastore::SERVICES_DB_NAME; use nexus_db_queries::db::fixed_data::vpc::SERVICES_VPC_ID; use nexus_db_queries::db::identity::Asset; use nexus_db_queries::db::identity::Resource; @@ -21,11 +20,7 @@ use omicron_common::api::external; use omicron_common::api::external::AllowedSourceIps; use omicron_common::api::external::Error; use omicron_common::api::external::IpNet; -use omicron_common::api::external::L4PortRange; use omicron_common::api::external::ListResultVec; -use omicron_common::api::external::VpcFirewallRuleAction; -use omicron_common::api::external::VpcFirewallRuleDirection; -use omicron_common::api::external::VpcFirewallRuleStatus; use omicron_common::api::internal::nexus::HostIdentifier; use omicron_common::api::internal::shared::NetworkInterface; use slog::debug; @@ -194,6 +189,45 @@ pub async fn resolve_firewall_rules_for_sled_agent( "subnet_networks" => ?subnet_networks, ); + // Lookup an rules implied by the user-facing services IP allowlist. + // + // These rules are implicit, and not stored in the firewall rule table, + // since they're logically a different thing. However, we implement the + // allowlist by _modifying_ any existing rules targeting the internal Oxide + // services VPC. The point here is to restrict the hosts allowed to make + // connections, but otherwise leave the rules unmodified. For example, we + // want to make sure that our external DNS server only receives UDP traffic + // on port 53. Adding a _new_ firewall rule for the allowlist with higher + // priority would remove this port / protocol requirement. Instead, we + // modify the rules in-place. + let should_apply_allowlist = allowlist_applies_to_vpc(vpc); + let allowed_ips = if should_apply_allowlist { + let allowed_ips = + lookup_allowed_source_ips(datastore, opctx, log).await?; + match &allowed_ips { + AllowedSourceIps::Any => { + debug!( + log, + "Allowlist for user-facing services is set to \ + allow any inbound traffic. Existing VPC firewall \ + rules will not be modified." + ); + } + AllowedSourceIps::List(list) => { + debug!( + log, + "Found allowlist for user-facing services \ + with explicit IP list. Existing VPC firewall \ + rules will be modified to match."; + "allow_list" => ?list, + ); + } + } + Some(allowed_ips) + } else { + None + }; + // Compile resolved rules for the sled agents. let mut sled_agent_rules = Vec::with_capacity(rules.len()); for rule in rules { @@ -274,9 +308,43 @@ pub async fn resolve_firewall_rules_for_sled_agent( continue; } - let filter_hosts = match &rule.filter_hosts { - None => None, - Some(hosts) => { + // Construct the set of filter hosts from the DB rule. + let filter_hosts = match (&rule.filter_hosts, should_apply_allowlist) { + // No host filters, but we need to insert the allowlist entries. + // + // This is the expected case when applying rules for the built-in + // Oxide-services VPCs, which do not contain host filters. (See + // `nexus_db_queries::fixed_data::vpc_firewall_rule` for those + // rules.) If those rules change to include any filter hosts, this + // logic needs to change as well. + (None, true) => match allowed_ips.as_ref().unwrap() { + AllowedSourceIps::Any => None, + AllowedSourceIps::List(list) => Some( + list.iter() + .copied() + .map(|ip| HostIdentifier::Ip(ip).into()) + .collect(), + ), + }, + + // No rules exist, and we don't need to add anything for the + // allowlist. + (None, false) => None, + + (Some(_), true) => { + return Err(Error::internal_error( + "While trying to apply the user-facing services allowlist, \ + we found unexpected host filters already in the rules. These \ + are expected to have no built-in rules which filter on \ + the hosts, so that we can modify the rules to apply the \ + allowlist without worrying about destroying those built-in \ + host filters." + )); + } + + // There are host filters, but we don't need to apply the allowlist + // to this VPC either, so insert the rules as-is. + (Some(hosts), false) => { let mut host_addrs = Vec::with_capacity(hosts.len()); for host in hosts { match &host.0 { @@ -353,45 +421,6 @@ pub async fn resolve_firewall_rules_for_sled_agent( priority: rule.priority.0 .0, }); } - - // Add rules that implement our IP allowlist for user-facing services. - // - // These rules are implicit, and not stored in the firewall rule table, - // since they're logically a different thing. We'll convert them into - // sled-agent firewall rules here and insert them. - // - // Note that this only applies to the internal services VPC, so we ignore - // these rules if we have no such interfaces for that. - if vpc.id() == *SERVICES_VPC_ID { - if let AllowedSourceIps::List(allowed_ips) = - lookup_allowed_source_ips(datastore, opctx, log).await? - { - let name = SERVICES_DB_NAME.parse().unwrap(); - if let Some(interfaces) = vpc_interfaces.get(&name) { - debug!( - log, - "constructing firewall rules implementing \ - user-facing services IP allowlist"; - "allowed_ips" => ?allowed_ips, - ); - if let Some(mut new_rules) = - allowlist_to_firewall_rules(interfaces, allowed_ips).await - { - sled_agent_rules.append(&mut new_rules); - } - } else { - warn!( - log, - "Found services VPC ID but no NICs for this service!" - ); - } - } else { - debug!( - log, - "User-facing services allowlist is set to allow any inbound traffic" - ); - } - } debug!( log, "resolved firewall rules for sled agents"; @@ -487,70 +516,9 @@ pub async fn plumb_service_firewall_rules( Ok(()) } -/// Compute firewall rules implied by an IP allowlist. -/// -/// This looks up the source IP allowlist in the database, and generates -/// firewall rules attached to each provided NIC that implements the allowlist. -/// If the allowlist is "any", there are no restrictions and no rules are -/// returned. -/// -/// Otherwise, two sets of rules are returned: -/// -/// - A low-priority rule that denies all traffic -/// - A higher-priority rule that allows it from the specific IPs. -async fn allowlist_to_firewall_rules( - nics: &[NetworkInterface], - allowed_ips: Vec, -) -> Option> { - if nics.is_empty() { - return None; - } - - // Convert to host identifiers, how the sled-agent types expose these. - assert!(!allowed_ips.is_empty(), "Allowlist must not be empty here"); - let filter_hosts = Some( - allowed_ips - .into_iter() - .map(|net| sled_agent_client::types::HostIdentifier::Ip(net.into())) - .collect(), - ); - - let low_priority_deny = sled_agent_client::types::VpcFirewallRule { - status: VpcFirewallRuleStatus::Enabled.into(), - direction: VpcFirewallRuleDirection::Inbound.into(), - targets: nics.to_vec(), - filter_hosts: filter_hosts.clone(), - filter_ports: Some(vec![L4PortRange { - first: 1.try_into().unwrap(), - last: u16::MAX.try_into().unwrap(), - } - .into()]), - filter_protocols: None, - action: VpcFirewallRuleAction::Deny.into(), - priority: ALLOW_LIST_LOW_PRIORTY, - }; - - let allow = sled_agent_client::types::VpcFirewallRule { - status: VpcFirewallRuleStatus::Enabled.into(), - direction: VpcFirewallRuleDirection::Inbound.into(), - targets: nics.to_vec(), - filter_hosts, - - // TODO-correctness: Having no filter on ports or protocols is not - // techniically correct. Nexus should only receive TCP traffic, on HTTP - // or HTTPs traffic, for example. However, at this point in the system, - // we don't really have access to _which service_ this NIC is for, only - // that it is for _one_ of our services. - // - // We may want to reorganize this code to apply these rules on a - // per-service basis. This is discussed in RFD 474. - filter_ports: None, - filter_protocols: None, - action: VpcFirewallRuleAction::Allow.into(), - priority: ALLOWLIST_HIGH_PRIORITY, - }; - - Some(vec![low_priority_deny, allow]) +/// Return true if the user-facing services allowlist applies to a VPC. +fn allowlist_applies_to_vpc(vpc: &db::model::Vpc) -> bool { + vpc.id() == *SERVICES_VPC_ID } /// Return the list of allowed IPs from the database. @@ -561,15 +529,8 @@ async fn lookup_allowed_source_ips( ) -> Result { match datastore.allowed_source_ips_view(opctx).await { Ok(allowed) => { - debug!(log, "fetched allowlist from DB"; "allowed" => ?allowed); - let ips = if let Some(list) = allowed.allowed_ips { - AllowedSourceIps::List( - list.into_iter().map(Into::into).collect(), - ) - } else { - AllowedSourceIps::Any - }; - Ok(ips) + slog::trace!(log, "fetched allowlist from DB"; "allowed" => ?allowed); + allowed.allowed_source_ips() } Err(e) => { error!(log, "failed to fetch allowlist from DB"; "err" => ?e); @@ -577,9 +538,3 @@ async fn lookup_allowed_source_ips( } } } - -/// FW rule priority for allow-list default-deny rule. -const ALLOW_LIST_LOW_PRIORTY: u16 = u16::MAX - 1; - -/// FW rule priority for allow-list explicit allow rule. -const ALLOWLIST_HIGH_PRIORITY: u16 = ALLOW_LIST_LOW_PRIORTY - 1; diff --git a/nexus/src/app/allowed_source_ips.rs b/nexus/src/app/allowed_source_ips.rs index 70bc9ed92a4..3cdd738e661 100644 --- a/nexus/src/app/allowed_source_ips.rs +++ b/nexus/src/app/allowed_source_ips.rs @@ -6,12 +6,12 @@ //! Nexus methods for operating on source IP allowlists. -use std::net::IpAddr; use nexus_db_queries::context::OpContext; use nexus_types::external_api::params; use nexus_types::external_api::views::AllowedSourceIps; use omicron_common::api::external; use omicron_common::api::external::Error; +use std::net::IpAddr; impl super::Nexus { /// Fetch the allowlist of source IPs that can reach user-facing services. @@ -43,11 +43,6 @@ impl super::Nexus { ); return Err(Error::invalid_request(message)); } - if list.is_empty() { - return Err(Error::invalid_request( - "Source IP allow list must not be empty", - )); - } // Some basic sanity-checks on the addresses in the allowlist. // diff --git a/sled-agent/src/bootstrap/params.rs b/sled-agent/src/bootstrap/params.rs index 84571f74f33..255704ad6fc 100644 --- a/sled-agent/src/bootstrap/params.rs +++ b/sled-agent/src/bootstrap/params.rs @@ -503,7 +503,7 @@ mod tests { bgp: Vec::new(), bfd: Vec::new(), }, - allowed_source_ips: Default::default(), + allowed_source_ips: AllowedSourceIps::Any, }; // Valid configs: all external DNS IPs are contained in the IP pool diff --git a/sled-agent/src/rack_setup/config.rs b/sled-agent/src/rack_setup/config.rs index b540fdf1dde..91cdc5c9b5f 100644 --- a/sled-agent/src/rack_setup/config.rs +++ b/sled-agent/src/rack_setup/config.rs @@ -94,6 +94,7 @@ mod test { use anyhow::Context; use camino::Utf8PathBuf; use omicron_common::address::IpRange; + use omicron_common::api::internal::shared::AllowedSourceIps; use omicron_common::api::internal::shared::RackNetworkConfig; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; @@ -129,7 +130,7 @@ mod test { bgp: Vec::new(), bfd: Vec::new(), }, - allowed_source_ips: Default::default(), + allowed_source_ips: AllowedSourceIps::Any, }; assert_eq!( diff --git a/sled-agent/src/rack_setup/plan/service.rs b/sled-agent/src/rack_setup/plan/service.rs index c22f0c5a2d2..b4a6fe76f6e 100644 --- a/sled-agent/src/rack_setup/plan/service.rs +++ b/sled-agent/src/rack_setup/plan/service.rs @@ -1164,6 +1164,7 @@ mod tests { use crate::bootstrap::params::BootstrapAddressDiscovery; use crate::bootstrap::params::RecoverySiloConfig; use omicron_common::address::IpRange; + use omicron_common::api::internal::shared::AllowedSourceIps; use omicron_common::api::internal::shared::RackNetworkConfig; const EXPECTED_RESERVED_ADDRESSES: u16 = 2; @@ -1267,7 +1268,7 @@ mod tests { bgp: Vec::new(), bfd: Vec::new(), }, - allowed_source_ips: Default::default(), + allowed_source_ips: AllowedSourceIps::Any, }; let mut svp = ServicePortBuilder::new(&config);