From d1de563428e295d78f55cc35f13cb1eddd221bda Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 15 Jul 2026 17:23:03 -0700 Subject: [PATCH 01/25] handle the DNS case unfortunately, the "URL-is-an-IP" case is gonna be 1000x more annoying because of...reqwest. yay. --- common/src/address.rs | 24 +++++++++++++++ nexus/src/app/external_dns.rs | 57 +++++++++++++++++++++++++++-------- nexus/src/app/mod.rs | 10 ++++++ nexus/src/app/rack.rs | 18 +++++++++-- 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index c3590534a9f..dcc0e320952 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -529,6 +529,30 @@ impl<'de, const N: u8> Deserialize<'de> for Ipv6Subnet { } } +impl Ipv6Subnet { + pub fn check_external_ip( + &self, + ip: IpAddr, + ) -> Result { + match ip { + IpAddr::V6(v6) if self.net().contains(v6) => { + Err(UnexpectedUnderlayIpError { ip, az_subnet: *self }) + } + _ => Ok(ip), + } + } +} + +#[derive(Debug, Clone, thiserror::Error)] +#[error( + "expected an external IP, but address ({ip}) is within the AZ underlay \ + subnet ({az_subnet})" +)] +pub struct UnexpectedUnderlayIpError { + ip: IpAddr, + az_subnet: Ipv6Subnet, +} + /// Represents a subnet which may be used for contacting DNS services. #[derive( Clone, Copy, Debug, Deserialize, Serialize, PartialEq, Eq, PartialOrd, Ord, diff --git a/nexus/src/app/external_dns.rs b/nexus/src/app/external_dns.rs index 2617e99a137..953d05b811e 100644 --- a/nexus/src/app/external_dns.rs +++ b/nexus/src/app/external_dns.rs @@ -2,8 +2,11 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use std::error::Error; use std::net::IpAddr; use std::net::SocketAddr; +use std::sync::Arc; +use std::sync::OnceLock; use hickory_resolver::TokioResolver; use hickory_resolver::config::NameServerConfig; @@ -12,15 +15,24 @@ use hickory_resolver::config::ResolverConfig; use hickory_resolver::config::ResolverOpts; use hickory_resolver::name_server::TokioConnectionProvider; use hickory_resolver::proto::xfer::Protocol; +use omicron_common::address::AZ_PREFIX; use omicron_common::address::DNS_PORT; +use omicron_common::address::Ipv6Subnet; +use omicron_common::address::RACK_PREFIX; use reqwest::dns::Name; /// Wrapper around hickory-resolver to provide name resolution /// using a given set of DNS servers for use with reqwest. -pub struct Resolver(TokioResolver); +pub struct Resolver { + resolver: TokioResolver, + rack_subnet: Arc>>, +} impl Resolver { - pub fn new(dns_servers: &[IpAddr]) -> Resolver { + pub fn new( + dns_servers: &[IpAddr], + rack_subnet: Arc>>, + ) -> Resolver { assert!(!dns_servers.is_empty()); let mut rc = ResolverConfig::new(); for addr in dns_servers { @@ -58,31 +70,50 @@ impl Resolver { opts.use_hosts_file = ResolveHosts::Never; // Do as many requests in parallel as we have configured servers opts.num_concurrent_reqs = dns_servers.len(); - Resolver( - TokioResolver::builder_with_config( + Resolver { + resolver: TokioResolver::builder_with_config( rc, TokioConnectionProvider::default(), ) .with_options(opts) .build(), - ) + rack_subnet, + } } } impl reqwest::dns::Resolve for Resolver { fn resolve(&self, name: Name) -> reqwest::dns::Resolving { - let resolver = self.0.clone(); + let resolver = self.resolver.clone(); + let rack_subnet = self.rack_subnet.clone(); Box::pin(async move { let ips = resolver.lookup_ip(name.as_str()).await?; + + // NOTE(eliza): this certainly *could* be a `tokio::sync::watch`, + // and we _could_ wait for it to be set here, instead of failing + // fast if we don't know the rack subnet yet. Maybe this is actually + // the wrong thing and we should wait for it instead, I dunno... + let rack_subnet = rack_subnet.get().ok_or_else(|| { + anyhow::anyhow!("cannot resolve external DNS names before RSS!") + })?; + let az_subnet = + Ipv6Subnet::::new(rack_subnet.net().addr()); let addrs = ips .into_iter() - // hickory-resolver returns `IpAddr`s but reqwest wants - // `SocketAddr`s (useful if you have a custom resolver that - // returns a scoped IPv6 address). The port provided here - // is ignored in favour of the scheme default (http/80, - // https/443) or any custom port provided in the URL. - .map(|ip| SocketAddr::new(ip, 0)); - Ok(Box::new(addrs) as Box<_>) + .map(|ip| { + // We expect this domain to resolve to an external IP + // address, *not* one within the underlay network. Reject + // any unexpected underlay addresses here. + let ip = az_subnet.check_external_ip(ip)?; + // hickory-resolver returns `IpAddr`s but reqwest wants + // `SocketAddr`s (useful if you have a custom resolver that + // returns a scoped IPv6 address). The port provided here + // is ignored in favour of the scheme default (http/80, + // https/443) or any custom port provided in the URL. + Ok(SocketAddr::new(ip, 0)) + }) + .collect::, Box>>()?; + Ok(Box::new(addrs.into_iter()) as Box<_>) }) } } diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 0f40c372f08..e907e3477da 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -34,7 +34,9 @@ use nexus_mgs_updates::MgsUpdateDriver; use nexus_types::deployment::PendingMgsUpdates; use nexus_types::deployment::ReconfiguratorConfigParam; +use omicron_common::address::Ipv6Subnet; use omicron_common::address::MGS_PORT; +use omicron_common::address::RACK_PREFIX; use omicron_common::api::external::ByteCount; use omicron_common::api::external::Error; use omicron_uuid_kinds::OmicronZoneUuid; @@ -316,6 +318,10 @@ pub struct Nexus { /// state of overall Nexus quiesce activity quiesce: NexusQuiesceHandle, + + /// the rack subnet, set once it has been loaded/once the rack has been + /// initialized. + rack_subnet: Arc>>, } impl Nexus { @@ -472,12 +478,15 @@ impl Nexus { background_tasks_internal, ) = background::BackgroundTasksInitializer::new(); + let rack_subnet = Arc::new(OnceLock::new()); let external_resolver = { if config.deployment.external_dns_servers.is_empty() { return Err("expected at least 1 external DNS server".into()); } + Arc::new(external_dns::Resolver::new( &config.deployment.external_dns_servers, + rack_subnet.clone(), )) }; @@ -582,6 +591,7 @@ impl Nexus { update_status: UpdateStatusHandle::new(blueprint_load_rx), quiesce, sitrep_load_rx, + rack_subnet, }; // TODO-cleanup all the extra Arcs here seems wrong diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index d5504abf1d0..3f30114a1a9 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -760,8 +760,22 @@ impl super::Nexus { match result { Ok(rack) => { if rack.initialized { - info!(self.log, "Rack initialized"); - return; + match rack_subnet(rack.rack_subnet) { + Ok(subnet) => { + info!(self.log, "Rack initialized"); + self.rack_subnet.set( + Ipv6Subnet::::from(subnet), + ); + return; + } + Err(e) => { + error!( + self.log, + "Rack claims to be initialized, but rack subnet is invalid!: {}", + e + ); + } + } } info!( self.log, From 27266ee49228c0f6c33f956d25f983a4cd33891d Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:17:53 -0700 Subject: [PATCH 02/25] refactor underlay subnets into a new thing --- common/src/address.rs | 19 +++++++++++++++++++ nexus/src/app/external_dns.rs | 25 +++++++++++++------------ nexus/src/app/mod.rs | 16 ++++++++-------- nexus/src/app/rack.rs | 14 ++++++++++---- 4 files changed, 50 insertions(+), 24 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index dcc0e320952..7411794498d 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -553,6 +553,25 @@ pub struct UnexpectedUnderlayIpError { az_subnet: Ipv6Subnet, } +/// A rack's underlay subnet, along with the AZ-wide underlay subnet +/// containing it. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct UnderlaySubnets { + pub rack_subnet: Ipv6Subnet, + pub az_subnet: Ipv6Subnet, +} + +impl UnderlaySubnets { + /// Constructs a new `UnderlaySubnets` from the rack subnet, widening it to + /// determine the AZ subnet. + pub fn new(rack_subnet: Ipv6Subnet) -> Self { + Self { + rack_subnet, + az_subnet: Ipv6Subnet::new(rack_subnet.net().addr()), + } + } +} + /// Represents a subnet which may be used for contacting DNS services. #[derive( Clone, Copy, Debug, Deserialize, Serialize, PartialEq, Eq, PartialOrd, Ord, diff --git a/nexus/src/app/external_dns.rs b/nexus/src/app/external_dns.rs index 953d05b811e..e56371999e8 100644 --- a/nexus/src/app/external_dns.rs +++ b/nexus/src/app/external_dns.rs @@ -15,23 +15,21 @@ use hickory_resolver::config::ResolverConfig; use hickory_resolver::config::ResolverOpts; use hickory_resolver::name_server::TokioConnectionProvider; use hickory_resolver::proto::xfer::Protocol; -use omicron_common::address::AZ_PREFIX; use omicron_common::address::DNS_PORT; -use omicron_common::address::Ipv6Subnet; -use omicron_common::address::RACK_PREFIX; +use omicron_common::address::UnderlaySubnets; use reqwest::dns::Name; /// Wrapper around hickory-resolver to provide name resolution /// using a given set of DNS servers for use with reqwest. pub struct Resolver { resolver: TokioResolver, - rack_subnet: Arc>>, + underlay_subnets: Arc>, } impl Resolver { pub fn new( dns_servers: &[IpAddr], - rack_subnet: Arc>>, + underlay_subnets: Arc>, ) -> Resolver { assert!(!dns_servers.is_empty()); let mut rc = ResolverConfig::new(); @@ -77,7 +75,7 @@ impl Resolver { ) .with_options(opts) .build(), - rack_subnet, + underlay_subnets, } } } @@ -85,7 +83,7 @@ impl Resolver { impl reqwest::dns::Resolve for Resolver { fn resolve(&self, name: Name) -> reqwest::dns::Resolving { let resolver = self.resolver.clone(); - let rack_subnet = self.rack_subnet.clone(); + let underlay_subnets = self.underlay_subnets.clone(); Box::pin(async move { let ips = resolver.lookup_ip(name.as_str()).await?; @@ -93,11 +91,14 @@ impl reqwest::dns::Resolve for Resolver { // and we _could_ wait for it to be set here, instead of failing // fast if we don't know the rack subnet yet. Maybe this is actually // the wrong thing and we should wait for it instead, I dunno... - let rack_subnet = rack_subnet.get().ok_or_else(|| { - anyhow::anyhow!("cannot resolve external DNS names before RSS!") - })?; - let az_subnet = - Ipv6Subnet::::new(rack_subnet.net().addr()); + let az_subnet = underlay_subnets + .get() + .ok_or_else(|| { + anyhow::anyhow!( + "cannot resolve external DNS names before RSS!" + ) + })? + .az_subnet; let addrs = ips .into_iter() .map(|ip| { diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index e907e3477da..59f2e0d5869 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -34,9 +34,8 @@ use nexus_mgs_updates::MgsUpdateDriver; use nexus_types::deployment::PendingMgsUpdates; use nexus_types::deployment::ReconfiguratorConfigParam; -use omicron_common::address::Ipv6Subnet; use omicron_common::address::MGS_PORT; -use omicron_common::address::RACK_PREFIX; +use omicron_common::address::UnderlaySubnets; use omicron_common::api::external::ByteCount; use omicron_common::api::external::Error; use omicron_uuid_kinds::OmicronZoneUuid; @@ -319,9 +318,10 @@ pub struct Nexus { /// state of overall Nexus quiesce activity quiesce: NexusQuiesceHandle, - /// the rack subnet, set once it has been loaded/once the rack has been - /// initialized. - rack_subnet: Arc>>, + /// the underlay subnets (rack and AZ), set once they have been loaded, or + /// once the rack has been initialized (if RSS has not already finished when + /// this Nexus process starts). + underlay_subnets: Arc>, } impl Nexus { @@ -478,7 +478,7 @@ impl Nexus { background_tasks_internal, ) = background::BackgroundTasksInitializer::new(); - let rack_subnet = Arc::new(OnceLock::new()); + let underlay_subnets = Arc::new(OnceLock::new()); let external_resolver = { if config.deployment.external_dns_servers.is_empty() { return Err("expected at least 1 external DNS server".into()); @@ -486,7 +486,7 @@ impl Nexus { Arc::new(external_dns::Resolver::new( &config.deployment.external_dns_servers, - rack_subnet.clone(), + underlay_subnets.clone(), )) }; @@ -591,7 +591,7 @@ impl Nexus { update_status: UpdateStatusHandle::new(blueprint_load_rx), quiesce, sitrep_load_rx, - rack_subnet, + underlay_subnets, }; // TODO-cleanup all the extra Arcs here seems wrong diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 3f30114a1a9..89682082122 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -31,7 +31,9 @@ use nexus_types::external_api::silo; use nexus_types::external_api::sled as sled_types; use nexus_types::inventory::SpType; use nexus_types::silo::silo_dns_name; -use omicron_common::address::{Ipv6Subnet, RACK_PREFIX_LENGTH, get_64_subnet}; +use omicron_common::address::{ + Ipv6Subnet, RACK_PREFIX_LENGTH, UnderlaySubnets, get_64_subnet, +}; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; @@ -763,9 +765,13 @@ impl super::Nexus { match rack_subnet(rack.rack_subnet) { Ok(subnet) => { info!(self.log, "Rack initialized"); - self.rack_subnet.set( - Ipv6Subnet::::from(subnet), - ); + let subnet = + Ipv6Subnet::::from(subnet); + // If the subnets were already set, the new + // value is identical, so the `Err` is benign. + let _ = self + .underlay_subnets + .set(UnderlaySubnets::new(subnet)); return; } Err(e) => { From 4c9a0499a443b32b83b9fce9bd4529e57685fb49 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:18:20 -0700 Subject: [PATCH 03/25] Update address.rs --- common/src/address.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index 7411794498d..1c21dca4fac 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -529,7 +529,7 @@ impl<'de, const N: u8> Deserialize<'de> for Ipv6Subnet { } } -impl Ipv6Subnet { +impl Ipv6Subnet { pub fn check_external_ip( &self, ip: IpAddr, @@ -550,7 +550,7 @@ impl Ipv6Subnet { )] pub struct UnexpectedUnderlayIpError { ip: IpAddr, - az_subnet: Ipv6Subnet, + az_subnet: Ipv6Subnet, } /// A rack's underlay subnet, along with the AZ-wide underlay subnet From 6dae9a3d3f456d60621c6d0722793c87bd2794fd Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:20:13 -0700 Subject: [PATCH 04/25] this feels a bit nicer --- common/src/address.rs | 44 +++++++++++++++++++++-------------- nexus/src/app/external_dns.rs | 13 ++++------- nexus/src/app/rack.rs | 4 +++- 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index 1c21dca4fac..c1850d921e8 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -529,28 +529,14 @@ impl<'de, const N: u8> Deserialize<'de> for Ipv6Subnet { } } -impl Ipv6Subnet { - pub fn check_external_ip( - &self, - ip: IpAddr, - ) -> Result { - match ip { - IpAddr::V6(v6) if self.net().contains(v6) => { - Err(UnexpectedUnderlayIpError { ip, az_subnet: *self }) - } - _ => Ok(ip), - } - } -} - #[derive(Debug, Clone, thiserror::Error)] #[error( - "expected an external IP, but address ({ip}) is within the AZ underlay \ - subnet ({az_subnet})" + "expected an external IP, but address ({ip}) is within the underlay \ + subnet ({subnet})" )] pub struct UnexpectedUnderlayIpError { ip: IpAddr, - az_subnet: Ipv6Subnet, + subnet: Ipv6Net, } /// A rack's underlay subnet, along with the AZ-wide underlay subnet @@ -570,6 +556,30 @@ impl UnderlaySubnets { az_subnet: Ipv6Subnet::new(rack_subnet.net().addr()), } } + + pub fn check_external_ip( + &self, + ip: IpAddr, + ) -> Result { + let az_subnet = self.az_subnet.net(); + match ip { + // Is it in the AZ underlay subnet? + IpAddr::V6(v6) if az_subnet.contains(v6) => { + Err(UnexpectedUnderlayIpError { ip, subnet: az_subnet }) + } + // The underlay multicast subnet is also part of the underlay + // network, so check that, too. + IpAddr::V6(v6) if UNDERLAY_MULTICAST_SUBNET.contains(v6) => { + Err(UnexpectedUnderlayIpError { + ip, + subnet: UNDERLAY_MULTICAST_SUBNET, + }) + } + // Note that we need not also check if the IP is contained by the + // rack subnet, because the AZ subnet contains the rack subnet. + _ => Ok(ip), + } + } } /// Represents a subnet which may be used for contacting DNS services. diff --git a/nexus/src/app/external_dns.rs b/nexus/src/app/external_dns.rs index e56371999e8..307b7b8fb48 100644 --- a/nexus/src/app/external_dns.rs +++ b/nexus/src/app/external_dns.rs @@ -91,21 +91,16 @@ impl reqwest::dns::Resolve for Resolver { // and we _could_ wait for it to be set here, instead of failing // fast if we don't know the rack subnet yet. Maybe this is actually // the wrong thing and we should wait for it instead, I dunno... - let az_subnet = underlay_subnets - .get() - .ok_or_else(|| { - anyhow::anyhow!( - "cannot resolve external DNS names before RSS!" - ) - })? - .az_subnet; + let underlay = underlay_subnets.get().ok_or_else(|| { + anyhow::anyhow!("cannot resolve external DNS names before RSS!") + })?; let addrs = ips .into_iter() .map(|ip| { // We expect this domain to resolve to an external IP // address, *not* one within the underlay network. Reject // any unexpected underlay addresses here. - let ip = az_subnet.check_external_ip(ip)?; + let ip = underlay.check_external_ip(ip)?; // hickory-resolver returns `IpAddr`s but reqwest wants // `SocketAddr`s (useful if you have a custom resolver that // returns a scoped IPv6 address). The port provided here diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 89682082122..ab2d34a223f 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -766,7 +766,9 @@ impl super::Nexus { Ok(subnet) => { info!(self.log, "Rack initialized"); let subnet = - Ipv6Subnet::::from(subnet); + Ipv6Subnet::::from( + subnet, + ); // If the subnets were already set, the new // value is identical, so the `Err` is benign. let _ = self From 4cc48a943461d28862df702770938f889c8e5a82 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 15 Jul 2026 18:35:14 -0700 Subject: [PATCH 05/25] nuke the old and bad thing --- nexus/src/app/mod.rs | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 59f2e0d5869..cecf182d074 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -1467,24 +1467,10 @@ async fn map_switch_zone_addrs( /// Begin configuring an external HTTP client, returning a /// `reqwest::ClientBuilder`. pub(crate) fn external_http_client_builder( - config: &nexus_config::ExternalHttpClientConfig, + _config: &nexus_config::ExternalHttpClientConfig, resolver: &Arc, ) -> reqwest::ClientBuilder { let mut builder = reqwest::ClientBuilder::new(); - builder = builder.dns_resolver(resolver.clone()); - - // If we are configured to only bind external TCP connections on a specific interface, do so. - #[cfg(any( - target_os = "linux", - target_os = "macos", - target_os = "illumos", - ))] - { - if let Some(ref interface) = config.interface { - builder = builder.interface(interface); - } - } - builder } From e6b0a95d36cf754285d31bec209e570153db557a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 28 Jul 2026 14:44:21 -0700 Subject: [PATCH 06/25] so it actually turns out this is complicated (WIP) --- Cargo.lock | 1 + common/src/address.rs | 34 -- nexus-config/src/nexus_config.rs | 38 ++ nexus/Cargo.toml | 1 + nexus/examples/config-second.toml | 7 + nexus/examples/config.toml | 7 + nexus/src/app/external_client.rs | 622 ++++++++++++++++++++++++++++++ nexus/src/app/external_dns.rs | 37 +- nexus/src/app/mod.rs | 23 +- nexus/src/app/webhook.rs | 19 +- nexus/tests/config.test.toml | 6 + schema/deployment-config.json | 32 +- sled-agent/src/services.rs | 2 + 13 files changed, 758 insertions(+), 71 deletions(-) create mode 100644 nexus/src/app/external_client.rs diff --git a/Cargo.lock b/Cargo.lock index d84200137d8..7d6a1e0fad1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9286,6 +9286,7 @@ dependencies = [ "tufaceous-artifact 0.1.0", "tufaceous-lib", "update-common", + "url", "usdt 0.5.0", "uuid", "zip 4.6.1", diff --git a/common/src/address.rs b/common/src/address.rs index c1850d921e8..c1358720957 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -529,16 +529,6 @@ impl<'de, const N: u8> Deserialize<'de> for Ipv6Subnet { } } -#[derive(Debug, Clone, thiserror::Error)] -#[error( - "expected an external IP, but address ({ip}) is within the underlay \ - subnet ({subnet})" -)] -pub struct UnexpectedUnderlayIpError { - ip: IpAddr, - subnet: Ipv6Net, -} - /// A rack's underlay subnet, along with the AZ-wide underlay subnet /// containing it. #[derive(Copy, Clone, Debug, PartialEq, Eq)] @@ -556,30 +546,6 @@ impl UnderlaySubnets { az_subnet: Ipv6Subnet::new(rack_subnet.net().addr()), } } - - pub fn check_external_ip( - &self, - ip: IpAddr, - ) -> Result { - let az_subnet = self.az_subnet.net(); - match ip { - // Is it in the AZ underlay subnet? - IpAddr::V6(v6) if az_subnet.contains(v6) => { - Err(UnexpectedUnderlayIpError { ip, subnet: az_subnet }) - } - // The underlay multicast subnet is also part of the underlay - // network, so check that, too. - IpAddr::V6(v6) if UNDERLAY_MULTICAST_SUBNET.contains(v6) => { - Err(UnexpectedUnderlayIpError { - ip, - subnet: UNDERLAY_MULTICAST_SUBNET, - }) - } - // Note that we need not also check if the IP is contained by the - // rack subnet, because the AZ subnet contains the rack subnet. - _ => Ok(ip), - } - } } /// Represents a subnet which may be used for contacting DNS services. diff --git a/nexus-config/src/nexus_config.rs b/nexus-config/src/nexus_config.rs index 32804671d1b..4711aeb78ba 100644 --- a/nexus-config/src/nexus_config.rs +++ b/nexus-config/src/nexus_config.rs @@ -276,6 +276,33 @@ struct UnvalidatedTunables { load_timeout: Option, } +/// Whether HTTP clients for external services permit requests to loopback +/// addresses. +/// +/// This is an enum rather than a `bool`, so that if you want to turn on the +/// test-only config, you have to type the string "yes_for_test_purposes_only" +/// in the config file so you know what you're doing. +#[derive( + Clone, + Copy, + Debug, + Default, + Deserialize, + Eq, + JsonSchema, + PartialEq, + Serialize, +)] +#[serde(rename_all = "snake_case")] +pub enum TreatLoopbackAsExternal { + /// Loopback addresses are considered "external". This must only be used + /// in test environments. + YesForTestPurposesOnly, + /// Loopback addresses are rejected (the default). + #[default] + No, +} + /// Configuration for HTTP clients to external services. #[derive( Clone, Debug, Default, Deserialize, PartialEq, Serialize, JsonSchema, @@ -285,6 +312,15 @@ pub struct ExternalHttpClientConfig { /// specified interface name. #[serde(default, skip_serializing_if = "Option::is_none")] pub interface: Option, + /// Whether external HTTP clients are permitted to make requests to + /// loopback addresses (and names in the special-use "localhost." zone). + /// + /// External HTTP clients normally refuse to make requests to any address + /// that isn't external to the rack, including loopback addresses. Test + /// environments, however, run their "external" servers on localhost, so + /// the test suite needs a way to turn that off. + #[serde(default)] + pub treat_loopback_as_external: TreatLoopbackAsExternal, } /// Tunable configuration parameters, intended for use in test environments or @@ -1266,6 +1302,7 @@ mod test { external_dns_servers = [ "1.1.1.1", "9.9.9.9" ] [deployment.external_http_clients] interface = "opte0" + treat_loopback_as_external = "yes_for_test_purposes_only" [deployment.dropshot_external] bind_address = "10.1.2.3:4567" default_request_body_max_bytes = 1024 @@ -1414,6 +1451,7 @@ mod test { ], external_http_clients: ExternalHttpClientConfig { interface: Some("opte0".to_string()), + treat_loopback_as_external: TreatLoopbackAsExternal::YesForTestPurposesOnly, }, }, pkg: PackageConfig { diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index 727a794c681..d5826626706 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -131,6 +131,7 @@ tokio-util = { workspace = true, features = ["codec", "rt"] } tough.workspace = true trust-quorum-types.workspace = true tufaceous-artifact.workspace = true +url.workspace = true usdt.workspace = true uuid.workspace = true diff --git a/nexus/examples/config-second.toml b/nexus/examples/config-second.toml index 33791421ccd..166afa72ba0 100644 --- a/nexus/examples/config-second.toml +++ b/nexus/examples/config-second.toml @@ -49,6 +49,13 @@ techport_external_server_port = 0 # These are the DNS servers it should use. external_dns_servers = ["1.1.1.1", "9.9.9.9"] +# Simulated/development deployments run "external" servers (e.g. webhook +# receivers) on localhost, so external HTTP clients must be permitted to +# connect to loopback addresses. This must never be enabled in production +# configs. +[deployment.external_http_clients] +treat_loopback_as_external = "yes_for_test_purposes_only" + [deployment.dropshot_external] # IP Address and TCP port on which to listen for the external API # This config file uses 12222 to avoid colliding with the usual 12220 that's diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 7ee8e86a69c..eeb4fa52666 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -37,6 +37,13 @@ rack_id = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc" # These are the DNS servers it should use. external_dns_servers = ["1.1.1.1", "9.9.9.9"] +# Simulated/development deployments run "external" servers (e.g. webhook +# receivers) on localhost, so external HTTP clients must be permitted to +# connect to loopback addresses. This must never be enabled in production +# configs. +[deployment.external_http_clients] +treat_loopback_as_external = "yes_for_test_purposes_only" + [deployment.dropshot_external] # IP Address and TCP port on which to listen for the external API bind_address = "127.0.0.1:12220" diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs new file mode 100644 index 00000000000..1311ce36a22 --- /dev/null +++ b/nexus/src/app/external_client.rs @@ -0,0 +1,622 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::external_dns; + +use nexus_config::ExternalHttpClientConfig; +use nexus_config::TreatLoopbackAsExternal; +use omicron_common::address::UNDERLAY_MULTICAST_SUBNET; +use omicron_common::address::UnderlaySubnets; +use oxnet::Ipv6Net; + +use qorb::resolver; +use reqwest::IntoUrl; +use reqwest::Method; +use std::net::IpAddr; +use std::sync::Arc; +use std::sync::OnceLock; +use url::Url; + +/// Policy for deciding whether an IP address is external to the rack. +#[derive(Clone, Debug)] +pub struct ExternalIpPolicy { + underlay_subnets: Arc>, + loopback_policy: TreatLoopbackAsExternal, +} + +/// Errors returned by [`ExternalIpPolicy::ensure_external_ip`]. +#[derive(Debug, Clone, thiserror::Error)] +pub enum ExternalIpError { + #[error( + "expected an external IP, but address {ip} is within the underlay \ + subnet {subnet}" + )] + Underlay { ip: IpAddr, subnet: Ipv6Net }, + #[error("expected an external IP, but {ip} is a loopback address")] + Loopback { ip: IpAddr }, + #[error( + "cannot check whether {ip} is an external IP address before the \ + rack's underlay subnets are known (RSS has not run yet)" + )] + RackNotInitialized { ip: IpAddr }, +} + +impl ExternalIpPolicy { + pub fn new( + underlay_subnets: Arc>, + loopback_policy: TreatLoopbackAsExternal, + ) -> Self { + Self { underlay_subnets, loopback_policy } + } + + /// Returns `Ok(ip)` if `ip` is external to the rack, and an + /// error describing why it is not otherwise. + /// + /// # Errors + /// + /// * [`ExternalIpError::Underlay`] if `ip` is within the AZ underlay + /// subnet or the underlay multicast subnet. + /// * [`ExternalIpError::Loopback`] if `ip` is a loopback address + /// * [`ExternalIpError::RackNotInitialized`] if the underlay subnets are + /// not yet known, in which case no determination can be made at all. + pub fn ensure_external_ip( + &self, + ip: IpAddr, + ) -> Result { + let subnets = self + .underlay_subnets + .get() + .ok_or(ExternalIpError::RackNotInitialized { ip })?; + let az_subnet = subnets.az_subnet.net(); + match ip { + // IPv6 addresses that are within the AZ's underlay subnet are not + // external to the rack. + IpAddr::V6(v6) if az_subnet.contains(v6) => { + Err(ExternalIpError::Underlay { ip, subnet: az_subnet }) + } + // The underlay multicast subnet is also part of the underlay + // network, so check that, too. + IpAddr::V6(v6) if UNDERLAY_MULTICAST_SUBNET.contains(v6) => { + Err(ExternalIpError::Underlay { + ip, + subnet: UNDERLAY_MULTICAST_SUBNET, + }) + } + // Loopback addresses are not external, regardless of whether they + // are v6 or v4... + ip if ip.is_loopback() + // ...unless we are inside of an integration test, in which + // case, lol. lmao. + && self.loopback_policy == TreatLoopbackAsExternal::No => + { + Err(ExternalIpError::Loopback { ip }) + } + // Note that we need not also check if the IP is contained by the + // rack subnet, because the AZ subnet contains the rack subnet. + _ => Ok(ip), + } + } +} + +#[derive(Clone)] +pub struct ExternalHttpClient { + client: reqwest::Client, + ip_policy: ExternalIpPolicy, +} + +/// Errors returned by [`ExternalHttpClient::request`] and friends. +#[derive(thiserror::Error, Debug)] +pub enum ExternalUrlError { + /// The supplied URL could not be parsed into a [`reqwest::Url`]. + #[error(transparent)] + IntoUrl(#[from] reqwest::Error), + /// The URL's host is an IP address which is not external, such as an + /// underlay network address or a loopback address --- or the rack is not + /// yet initialized, so no determination can be made. + #[error(transparent)] + NotExternalIp(#[from] ExternalIpError), + /// The URL's host is a name in the special-use `localhost.` zone, which + /// resolves to loopback addresses. + #[error( + "external HTTP requests may not be made to localhost (URL host \ + {host:?} is in the \"localhost.\" zone)" + )] + Localhost { host: String }, +} + +impl ExternalHttpClient { + /// Constructs a new `ExternalHttpClient` from the provided + /// [`ExternalHttpClientConfig`], using the provided + /// [`external_dns::Resolver`] for name resolution. + /// + /// # Errors + /// + /// This method fails if the [`reqwest::Client`] could not be built, such + /// as if a TLS backend cannot be initialized. + pub fn new( + config: &ExternalHttpClientConfig, + resolver: &Arc, + ) -> Result { + Self::from_builder(config, resolver, reqwest::ClientBuilder::new()) + } + + /// Constructs a new `ExternalHttpClient` from the provided + /// [`reqwest::ClientBuilder`] and [`ExternalHttpClientConfig`], using the + /// provided [`external_dns::Resolver`] for name resolution. + /// + /// # Errors + /// + /// This method fails if the [`reqwest::Client`] could not be built, such + /// as if a TLS backend cannot be initialized. + pub fn from_builder( + config: &ExternalHttpClientConfig, + resolver: &Arc, + mut builder: reqwest::ClientBuilder, + ) -> Result { + // If we are configured to only bind external TCP connections on a + // specific interface, do so. + #[cfg(any( + target_os = "linux", + target_os = "macos", + target_os = "illumos", + ))] + { + if let Some(ref interface) = config.interface { + builder = builder.interface(interface); + } + } + + let client = builder.dns_resolver(resolver.clone()).build()?; + let ip_policy = resolver.ip_policy().clone(); + + Ok(Self { client, ip_policy }) + } + + /// Convenience method to make a `GET` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn get( + &self, + url: U, + ) -> Result { + self.request(Method::GET, url) + } + + /// Convenience method to make a `POST` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn post( + &self, + url: U, + ) -> Result { + self.request(Method::POST, url) + } + + /// Convenience method to make a `PUT` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn put( + &self, + url: U, + ) -> Result { + self.request(Method::PUT, url) + } + + /// Convenience method to make a `PATCH` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn patch( + &self, + url: U, + ) -> Result { + self.request(Method::PATCH, url) + } + + /// Convenience method to make a `DELETE` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn delete( + &self, + url: U, + ) -> Result { + self.request(Method::DELETE, url) + } + + /// Convenience method to make a `HEAD` request to a URL. + /// + /// # Errors + /// + /// This method returns an error under the same conditions as + /// [`ExternalHttpClient::request`]. + pub fn head( + &self, + url: U, + ) -> Result { + self.request(Method::HEAD, url) + } + + /// Start building a `Request` with the `Method` and `Url`. + /// + /// Returns a `RequestBuilder`, which will allow setting headers and + /// the request body before sending. + /// + /// # Errors + /// + /// This method returns an error under the following conditions: + /// + /// * [`UrlError::IntoUrl`] if the provided `url` could not be parsed into a + /// URL. + /// * [`ExternalUrlError::NotExternalIp`] if the URL's host is an IP + /// address that is not external --- an underlay network address, or + /// (unless the policy permits it) a loopback address --- or if the + /// rack's underlay subnets are not yet known. + /// * [`ExternalUrlError::Localhost`] if the URL's host is a name in the + /// `localhost.` zone and the policy does not permit loopback addresses. + pub fn request( + &self, + method: Method, + url: U, + ) -> Result { + let url = url.into_url()?; + let url = ensure_external_url(url, &self.ip_policy)?; + Ok(self.client.request(method, url)) + } +} + +fn ensure_external_url( + url: Url, + policy: &ExternalIpPolicy, +) -> Result { + match url.host() { + // Domain names are mostly allowed here: if the name *resolves* to a + // non-external IP, that will be rejected by `external_dns::Resolver`, + // later. The exception is, of course, "localhost". As per RFC 6761 § + // 6.3, resolvers may answer names in it with loopback addresses without + // consulting DNS at all, so reject it eagerly here rather than relying + // on the resolver --- unless the loopback policy permits it, in which + // case the name is passed through to the resolver like any other. + Some(url::Host::Domain(domain)) + if policy.loopback_policy == TreatLoopbackAsExternal::No => + { + let name = domain.trim_end_matches('.'); + // The special-use "localhost." zone can, according to RFC 6761 § + // 6.3, contain subdomains. Although this usage is pretty uncommon, + // it would also resolve to a loopback address and therefore must be + // rejected as well. + let last_label = name.rsplit('.').next().unwrap_or(name); + if last_label.eq_ignore_ascii_case("localhost") { + return Err(ExternalUrlError::Localhost { + host: domain.to_string(), + }); + } + } + Some(url::Host::Domain(_)) => {} + // If the host is an IP address, check that it is considered an external + // IP. + Some(url::Host::Ipv6(v6)) => { + policy.ensure_external_ip(IpAddr::V6(v6))?; + } + Some(url::Host::Ipv4(v4)) => { + policy.ensure_external_ip(IpAddr::V4(v4))?; + } + None => {} + } + + Ok(url) +} + +#[cfg(test)] +mod test { + use super::*; + use omicron_common::address::{Ipv6Subnet, RACK_PREFIX}; + + fn test_policy( + loopback_policy: TreatLoopbackAsExternal, + ) -> ExternalIpPolicy { + let rack_subnet: ipnetwork::Ipv6Network = + nexus_test_utils::RACK_SUBNET.parse().unwrap(); + let subnets = + UnderlaySubnets::new(Ipv6Subnet::::from(rack_subnet)); + ExternalIpPolicy::new( + Arc::new(OnceLock::from(subnets)), + loopback_policy, + ) + } + + #[track_caller] + fn test_url(url: &str) -> Result { + let url = url + .parse::() + .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); + ensure_external_url(url, &test_policy(TreatLoopbackAsExternal::No)) + } + + #[track_caller] + fn test_url_loopback_allowed(url: &str) -> Result { + let url = url + .parse::() + .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); + ensure_external_url( + url, + &test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), + ) + } + + // Until the rack is initialized, no determination can be made about IP + // hosts, so they are rejected. Domain hosts still pass, since their + // resolved addresses are checked by `external_dns::Resolver` (which + // fails fast before rack initialization itself). + #[test] + fn test_ensure_external_url_rejects_ip_hosts_before_rack_init() { + let policy = ExternalIpPolicy::new( + Arc::new(OnceLock::new()), + TreatLoopbackAsExternal::No, + ); + let url = "http://[2001:db8::1]/".parse::().unwrap(); + let result = ensure_external_url(url, &policy); + assert!( + matches!( + &result, + Err(ExternalUrlError::NotExternalIp( + ExternalIpError::RackNotInitialized { .. } + )) + ), + "an IP-host URL must be rejected before the underlay subnets \ + are known, but got: {result:?}" + ); + } + + #[test] + fn test_ensure_external_url_rejects_underlay_ipv6_hosts() { + const CASES: &[&str] = &[ + // An address within this rack's subnet. + "http://[fd00:1122:3344:100::1]/", + // Explicit port, path, query, and fragment don't change the answer. + "http://[fd00:1122:3344:100::1]:12345/some/path?q=1#frag", + // Neither does the scheme... + "https://[fd00:1122:3344:100::1]/", + // ...nor userinfo. + "http://user:pass@[fd00:1122:3344:100::1]/", + // A sled subnet within this rack. + "http://[fd00:1122:3344:101::5]/", + // A different rack in the same AZ. + "http://[fd00:1122:3344:200::5]/", + // An internal DNS server address (in the reserved rack subnet). + "http://[fd00:1122:3344:1::1]/", + // The very first address in the AZ /48... + "http://[fd00:1122:3344::]/", + // ...and the very last. + "http://[fd00:1122:3344:ffff:ffff:ffff:ffff:ffff]/", + // The underlay multicast subnet (ff04::/64) is also part of the + // underlay network: first address... + "http://[ff04::]/", + // ...one in the middle... + "http://[ff04::1:2]/", + // ...and the last (`UNDERLAY_MULTICAST_SUBNET_LAST`). + "http://[ff04::ffff:ffff:ffff:ffff]/", + // Non-canonical textual forms are normalized by the URL parser + // before we see them: uppercase/verbose... + "http://[FD00:1122:3344:0100:0000:0000:0000:0001]/", + // ...and an embedded dotted-quad tail. + "http://[fd00:1122:3344:100::1.2.3.4]/", + ]; + for url in CASES { + let result = test_url(url); + assert!( + result.is_err(), + "URL {url:?} has an underlay IP host and should have been \ + rejected, but got: {result:?}" + ); + } + } + + #[test] + fn test_ensure_external_url_allows_external_ipv6_hosts() { + const CASES: &[&str] = &[ + // Global unicast. + "http://[2001:db8::1]/", + // A ULA that is *not* the underlay (e.g. a guest VPC prefix). + // ULA-ness alone must not be the discriminator. + "http://[fd12:3456:789a::1]/", + // Adjacent /48s: one below the AZ subnet... + "http://[fd00:1122:3343:ffff::1]/", + // ...and one above. + "http://[fd00:1122:3345::1]/", + // Link-local. + "http://[fe80::1]/", + // Admin-scoped multicast just *outside* the underlay multicast + // /64 subnet... + "http://[ff04:0:0:1::1]/", + // ...and multicast in a different scope entirely (site-local). + "http://[ff05::1:2]/", + // IPv4-mapped IPv6. + "http://[::ffff:1.2.3.4]/", + ]; + for url in CASES { + let result = test_url(url); + assert!( + result.is_ok(), + "URL {url:?} is not an underlay IP and should have been \ + allowed, but got: {result:?}" + ); + } + } + + // There is no IPv4 underlay, so non-loopback IPv4 hosts pass the check + // today, including RFC 1918 addresses. If an IPv4 range ever becomes + // internal-only, `ensure_external_ip` is the place to teach, and these + // cases should be revisited. + #[test] + fn test_ensure_external_url_allows_ipv4_hosts() { + const CASES: &[&str] = &[ + "http://1.2.3.4/", + "http://1.2.3.4:8080/webhooks", + "http://10.0.0.1/", + "http://192.168.1.1:8080/", + ]; + for url in CASES { + // Verify the parser actually produced an IPv4 host: without this, + // a case that parsed as a *domain* would also pass the check, and + // this test would silently stop covering the IPv4 arm. + let parsed = url.parse::().unwrap(); + assert!( + matches!(parsed.host(), Some(url::Host::Ipv4(_))), + "expected URL {url:?} to parse as an IPv4 host, but got: {:?}", + parsed.host(), + ); + let result = test_url(url); + assert!( + result.is_ok(), + "URL {url:?} is an IPv4 host and should have been allowed \ + (there is no IPv4 underlay), but got: {result:?}" + ); + } + } + + // Loopback is not external, in any of its spellings: IPv6, IPv4 (the + // whole 127.0.0.0/8), or names in the special-use "localhost." zone. + // + // NOTE: this means test environments cannot point an ExternalHttpClient + // at a server on localhost; tests exercising webhook delivery et al. + // must bind receivers to a non-loopback address. + #[test] + fn test_ensure_external_url_rejects_loopback() { + const CASES: &[&str] = &[ + // IPv6 loopback. + "http://[::1]/", + "https://[::1]:8443/receiver", + // IPv4 loopback, including the rest of 127.0.0.0/8. + "http://127.0.0.1/", + "http://127.0.0.1:8080/webhooks", + "http://127.255.255.254/", + // The WHATWG URL parser canonicalizes non-dotted-quad IPv4 forms + // into `Host::Ipv4` before we see them, so these hit the IPv4 arm + // rather than sneaking through as "domains": hexadecimal... + "http://0x7f.0.0.1/", + // ...and a single decimal integer (127.0.0.1). + "http://2130706433/", + // The "localhost." zone (RFC 6761 § 6.3): the name itself... + "http://localhost/", + "http://localhost:8080/webhooks", + // ...its subdomains, which are also in the zone... + "http://webhooks.localhost/", + // ...case-insensitively... + "http://LOCALHOST/", + // ...and fully-qualified (with a trailing dot). + "http://localhost./", + ]; + for url in CASES { + let result = test_url(url); + assert!( + result.is_err(), + "URL {url:?} is loopback and should have been rejected, \ + but got: {result:?}" + ); + } + } + + // With `LoopbackPolicy::AllowedForTests`, all the loopback spellings are + // permitted... + #[test] + fn test_ensure_external_url_allows_loopback_for_tests() { + const CASES: &[&str] = &[ + "http://[::1]/", + "https://[::1]:8443/receiver", + "http://127.0.0.1/", + "http://127.0.0.1:8080/webhooks", + "http://localhost/", + "http://localhost:8080/webhooks", + "http://webhooks.localhost/", + ]; + for url in CASES { + let result = test_url_loopback_allowed(url); + assert!( + result.is_ok(), + "URL {url:?} is loopback, which the policy permits in test \ + environments, but got: {result:?}" + ); + } + } + + // ...but underlay addresses are rejected regardless of the loopback + // policy: it must never weaken the underlay check. + #[test] + fn test_loopback_policy_does_not_weaken_underlay_check() { + const CASES: &[&str] = &[ + "http://[fd00:1122:3344:100::1]/", + "http://[fd00:1122:3344:1::1]/", + "http://[ff04::1:2]/", + ]; + for url in CASES { + let result = test_url_loopback_allowed(url); + assert!( + result.is_err(), + "URL {url:?} has an underlay IP host and should have been \ + rejected even with loopback allowed, but got: {result:?}" + ); + } + } + + // Domain hosts (other than the "localhost." zone) always pass this + // check. If they *resolve* to a non-external address, that's rejected by + // `external_dns::Resolver` at connect time instead. + #[test] + fn test_ensure_external_url_allows_domain_hosts() { + const CASES: &[&str] = &[ + "http://example.com/", + "https://webhooks.example.com:8443/receiver", + // Looks underlay-flavored, but it's a domain name. + "http://fd00.example.com/", + // IDN (punycoded by the parser). + "http://b\u{fc}cher.example/", + // "localhost" as a non-final label is *not* in the localhost + // zone: this is a legitimate external name. + "http://localhost.example.com/", + ]; + for url in CASES { + let result = test_url(url); + assert!( + result.is_ok(), + "URL {url:?} has a domain host and should have been allowed \ + (domains are checked at resolution time), but got: {result:?}" + ); + } + } + + // URLs with no host at all pass this check; reqwest itself refuses to send + // requests to non-http(s) schemes, so nothing is lost by letting them + // through here. + #[test] + fn test_ensure_external_url_allows_hostless_urls() { + const CASES: &[&str] = &[ + "file:///etc/passwd", + "data:text/plain,hello", + "mailto:nobody@example.com", + "unix:/var/run/thing.sock", + ]; + for url in CASES { + let result = test_url(url); + assert!( + result.is_ok(), + "URL {url:?} has no host and should have been allowed \ + (reqwest will reject it at send time), but got: {result:?}" + ); + } + } +} diff --git a/nexus/src/app/external_dns.rs b/nexus/src/app/external_dns.rs index 307b7b8fb48..f6d4b038ef8 100644 --- a/nexus/src/app/external_dns.rs +++ b/nexus/src/app/external_dns.rs @@ -5,8 +5,6 @@ use std::error::Error; use std::net::IpAddr; use std::net::SocketAddr; -use std::sync::Arc; -use std::sync::OnceLock; use hickory_resolver::TokioResolver; use hickory_resolver::config::NameServerConfig; @@ -16,20 +14,22 @@ use hickory_resolver::config::ResolverOpts; use hickory_resolver::name_server::TokioConnectionProvider; use hickory_resolver::proto::xfer::Protocol; use omicron_common::address::DNS_PORT; -use omicron_common::address::UnderlaySubnets; use reqwest::dns::Name; +use super::external_client::ExternalIpPolicy; + /// Wrapper around hickory-resolver to provide name resolution /// using a given set of DNS servers for use with reqwest. +#[derive(Clone, Debug)] pub struct Resolver { resolver: TokioResolver, - underlay_subnets: Arc>, + ip_policy: ExternalIpPolicy, } impl Resolver { pub fn new( dns_servers: &[IpAddr], - underlay_subnets: Arc>, + ip_policy: ExternalIpPolicy, ) -> Resolver { assert!(!dns_servers.is_empty()); let mut rc = ResolverConfig::new(); @@ -75,32 +75,39 @@ impl Resolver { ) .with_options(opts) .build(), - underlay_subnets, + ip_policy, } } } +impl Resolver { + /// Returns the [`ExternalIpPolicy`] this resolver enforces on resolved + /// addresses. + pub fn ip_policy(&self) -> &ExternalIpPolicy { + &self.ip_policy + } +} + impl reqwest::dns::Resolve for Resolver { fn resolve(&self, name: Name) -> reqwest::dns::Resolving { let resolver = self.resolver.clone(); - let underlay_subnets = self.underlay_subnets.clone(); + let ip_policy = self.ip_policy.clone(); Box::pin(async move { let ips = resolver.lookup_ip(name.as_str()).await?; - // NOTE(eliza): this certainly *could* be a `tokio::sync::watch`, - // and we _could_ wait for it to be set here, instead of failing - // fast if we don't know the rack subnet yet. Maybe this is actually - // the wrong thing and we should wait for it instead, I dunno... - let underlay = underlay_subnets.get().ok_or_else(|| { - anyhow::anyhow!("cannot resolve external DNS names before RSS!") - })?; + // NOTE(eliza): the policy's underlay subnets live in a `OnceLock` + // that isn't populated until RSS handoff, so this fails fast if + // we don't know the rack subnet yet. It certainly *could* be a + // `tokio::sync::watch`, and we _could_ wait for it to be set here + // instead. Maybe failing fast is actually the wrong thing and we + // should wait for it instead, I dunno... let addrs = ips .into_iter() .map(|ip| { // We expect this domain to resolve to an external IP // address, *not* one within the underlay network. Reject // any unexpected underlay addresses here. - let ip = underlay.check_external_ip(ip)?; + let ip = ip_policy.ensure_external_ip(ip)?; // hickory-resolver returns `IpAddr`s but reqwest wants // `SocketAddr`s (useful if you have a custom resolver that // returns a scoped IPv6 address). The port provided here diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index cecf182d074..c0b886af91a 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -72,6 +72,7 @@ pub mod crucible; mod deployment; mod device_auth; mod disk; +pub(crate) mod external_client; mod external_dns; pub(crate) mod external_endpoints; mod external_ip; @@ -479,6 +480,13 @@ impl Nexus { ) = background::BackgroundTasksInitializer::new(); let underlay_subnets = Arc::new(OnceLock::new()); + // The IP policy is shared by the external DNS resolver (which applies + // it to resolved addresses) and any `ExternalHttpClient` (which + // applies it to IP literals in URLs). + let external_ip_policy = external_client::ExternalIpPolicy::new( + underlay_subnets.clone(), + config.deployment.external_http_clients.treat_loopback_as_external, + ); let external_resolver = { if config.deployment.external_dns_servers.is_empty() { return Err("expected at least 1 external DNS server".into()); @@ -486,7 +494,7 @@ impl Nexus { Arc::new(external_dns::Resolver::new( &config.deployment.external_dns_servers, - underlay_subnets.clone(), + external_ip_policy, )) }; @@ -494,7 +502,7 @@ impl Nexus { // The webhook delivery HTTP client will send requests to endpoints // external to the rack, so apply the configuration for external // HTTP clients. - let builder = external_http_client_builder( + let builder = external_client::external_http_client_builder( &config.deployment.external_http_clients, &external_resolver, ); @@ -1463,14 +1471,3 @@ async fn map_switch_zone_addrs( switch_zone_addrs } - -/// Begin configuring an external HTTP client, returning a -/// `reqwest::ClientBuilder`. -pub(crate) fn external_http_client_builder( - _config: &nexus_config::ExternalHttpClientConfig, - resolver: &Arc, -) -> reqwest::ClientBuilder { - let mut builder = reqwest::ClientBuilder::new(); - builder = builder.dns_resolver(resolver.clone()); - builder -} diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index a032a5aecd5..ae78a6f538c 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -28,12 +28,14 @@ //! [RFD 538]: https://rfd.shared.oxide.computer/538 use crate::Nexus; +use crate::app::external_client::ExternalHttpClient; use anyhow::Context; use chrono::TimeDelta; use chrono::Utc; use hmac::{Hmac, Mac}; use http::HeaderName; use http::HeaderValue; +use nexus_config::ExternalHttpClientConfig; use nexus_db_lookup::LookupPath; use nexus_db_lookup::lookup; use nexus_db_queries::authz; @@ -317,9 +319,10 @@ impl Nexus { /// Construct a [`reqwest::Client`] configured for webhook delivery requests. pub(super) fn delivery_client( - builder: reqwest::ClientBuilder, -) -> Result { - builder + external_client_config: &ExternalHttpClientConfig, + resolver: &Arc, +) -> Result { + let builder = reqwest::ClientBuilder::new() // Per [RFD 538 § 4.3.1][1], webhook delivery does *not* follow // redirects. // @@ -335,8 +338,8 @@ pub(super) fn delivery_client( // each webhook delivery request. // // [1]: https://rfd.shared.oxide.computer/rfd/538#delivery-failure - .timeout(Duration::from_secs(30)) - .build() + .timeout(Duration::from_secs(30)); + ExternalHttpClient::from_builder(external_client_config, resolver, builder) } /// Everything necessary to send a delivery request to a webhook receiver. @@ -345,7 +348,7 @@ pub(super) fn delivery_client( /// background task, as it is used both by the deliverator RPW and by the Nexus /// API in the liveness probe endpoint. pub(crate) struct ReceiverClient<'a> { - client: &'a reqwest::Client, + client: &'a ExternalHttpClient, rx: &'a AlertReceiver, secrets: Vec<(WebhookSecretUuid, Hmac)>, hdr_rx_id: http::HeaderValue, @@ -354,7 +357,7 @@ pub(crate) struct ReceiverClient<'a> { impl<'a> ReceiverClient<'a> { pub(crate) fn new( - client: &'a reqwest::Client, + client: &'a ExternalHttpClient, secrets: impl IntoIterator, rx: &'a AlertReceiver, nexus_id: OmicronZoneUuid, @@ -468,7 +471,7 @@ impl<'a> ReceiverClient<'a> { }; let mut request = self .client - .post(&self.rx.endpoint) + .post(&self.rx.endpoint).expect("TODO ELIZA YOU HAVE TO ACTUALLY HANDLE THE EXTERNAL CLIENT ERROR HERE LOL") .header(HDR_RX_ID, self.hdr_rx_id.clone()) .header(HDR_DELIVERY_ID, delivery.id.to_string()) .header(HDR_ALERT_ID, delivery.alert_id.to_string()) diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index 1768cbf53f9..567291b21a3 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -45,6 +45,12 @@ techport_external_server_port = 0 # These are the DNS servers it should use. external_dns_servers = ["1.1.1.1", "9.9.9.9"] +# The test suite runs "external" servers (e.g. webhook receivers) on +# localhost, so external HTTP clients must be permitted to connect to +# loopback addresses. This must never be enabled in production configs. +[deployment.external_http_clients] +treat_loopback_as_external = "yes_for_test_purposes_only" + [deployment.dropshot_external] # NOTE: for the test suite, the port MUST be 0 (in order to bind to any # available port) because the test suite will be running many servers diff --git a/schema/deployment-config.json b/schema/deployment-config.json index e823d8a93a6..cfe72cdcfe9 100644 --- a/schema/deployment-config.json +++ b/schema/deployment-config.json @@ -28,7 +28,9 @@ }, "external_http_clients": { "description": "Configuration for HTTP clients to external services.", - "default": {}, + "default": { + "treat_loopback_as_external": "no" + }, "allOf": [ { "$ref": "#/definitions/ExternalHttpClientConfig" @@ -107,6 +109,15 @@ "string", "null" ] + }, + "treat_loopback_as_external": { + "description": "Whether external HTTP clients are permitted to make requests to loopback addresses (and names in the special-use \"localhost.\" zone).\n\nExternal HTTP clients normally refuse to make requests to any address that isn't external to the rack, including loopback addresses. Test environments, however, run their \"external\" servers on localhost, so the test suite needs a way to turn that off.", + "default": "no", + "allOf": [ + { + "$ref": "#/definitions/TreatLoopbackAsExternal" + } + ] } } }, @@ -196,6 +207,25 @@ "path": "omicron_uuid_kinds::RackUuid", "version": "*" } + }, + "TreatLoopbackAsExternal": { + "description": "Whether HTTP clients for external services permit requests to loopback addresses.\n\nThis is an enum rather than a `bool`, so that if you want to turn on the test-only config, you have to type the string \"yes_for_test_purposes_only\" in the config file so you know what you're doing.", + "oneOf": [ + { + "description": "Loopback addresses are considered \"external\". This must only be used in test environments.", + "type": "string", + "enum": [ + "yes_for_test_purposes_only" + ] + }, + { + "description": "Loopback addresses are rejected (the default).", + "type": "string", + "enum": [ + "no" + ] + } + ] } } } \ No newline at end of file diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index f3d38455420..e8150b17d7e 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -2460,6 +2460,8 @@ impl ServiceManager { external_http_clients: nexus_config::ExternalHttpClientConfig { interface: Some(opte_iface_name.to_string()), + treat_loopback_as_external: + nexus_config::TreatLoopbackAsExternal::No, }, }; From e5807032cfa9ccdc4ff8e21f9a7c07277ac30dfc Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 17 Jul 2026 16:57:12 -0700 Subject: [PATCH 07/25] my days of not loving reqwest are sure coming to a middle --- nexus/src/app/external_client.rs | 76 ++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 1311ce36a22..c29f45d452e 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -125,6 +125,15 @@ pub enum ExternalUrlError { Localhost { host: String }, } +/// Errors returned by [`ExternalHttpClient::execute`]. +#[derive(thiserror::Error, Debug)] +pub enum ExecuteExternalRequestError { + #[error(transparent)] + Reqwest(#[from] reqwest::Error), + #[error(transparent)] + ExternalUrl(#[from] ExternalUrlError), +} + impl ExternalHttpClient { /// Constructs a new `ExternalHttpClient` from the provided /// [`ExternalHttpClientConfig`], using the provided @@ -274,15 +283,74 @@ impl ExternalHttpClient { url: U, ) -> Result { let url = url.into_url()?; - let url = ensure_external_url(url, &self.ip_policy)?; + ensure_external_url(&url, &self.ip_policy)?; Ok(self.client.request(method, url)) } + + /// Executes a `Request`. + /// + /// A `Request` can be built manually with `Request::new()` or obtained + /// from a RequestBuilder with `RequestBuilder::build()`. + /// + /// The `reqwest` documentation tells us that we "should prefer to use the + /// `RequestBuilder` and `RequestBuilder::send()`". I disagree with them, + /// because doing so returns both errors that occurred while *building* the + /// request, which are likely to be programmer errors, and errors that + /// occurred while *sending* the request, which are generally runtime + /// errors, in the same function call, and forces you to manually go through + /// all the different error variants that `reqwest` is liable to return and + /// figure out what to do with them yourself. But I will repeat their + /// guidance here nonetheless, with a heavy sigh of frustration. + /// + /// # Errors + /// + /// This method fails if there was an error while sending request, + /// redirect loop was detected or redirect limit was exhausted. + pub fn execute( + &self, + request: reqwest::Request, + ) -> impl Future> + { + // ...and here we arrive at one of my least favorite things about + // `reqwest`'s API. It has this `execute` method, which takes an + // arbitrary `Request`, and...executes it. Like it says on the tin. But + // it _also_ has a `RequestBuilder::send` method, which is on the + // `RequestBuilder` type, and which builds the `Request` *and goes and + // sends it*. This works because the `RequestBuilder` owns a clone of + // the `Client` that it uses to send the request if you call + // `RequestBuilder::send`. + // + // I assume all of this was done to make it so you could feel slightly + // more like you were writing JavaScript, or something, but it makes + // wrapping the `reqwest::Client` in a thing that performs validation of + // the request (which is what this whole module is) infinitely more + // painful. Now, because one can send a request either by having the + // `RequestBuilder` build the request and pass it to `Client::execute`, + // *or* by calling `RequestBuilder::send`, which internally calls + // `Client::execute`, you have two different ways to send the request. + // If we don't validate the URL in *both* locations, you can + // (accidentally or on purpose) smuggle a request past the validation we + // want to do here. So we have to check the URL in both the + // `ExternalClient::request` method, which gives you a `RequestBuilder`, + // and *again* in `ExternalClient::execute`, even though the `Request` + // you are trying to `execute` *may* have come from + // `ExternalClient::request` in the first place, and therefore been + // validated *twice*. Have I mentioned that `reqwest`'s API is not + // really my favorite thing? + // + // Sigh. Whatever. It's fine. + let this = self.clone(); + async move { + ensure_external_url(request.url(), &this.ip_policy)?; + Ok(this.client.execute(request).await?) + } + } } fn ensure_external_url( - url: Url, + url: &Url, policy: &ExternalIpPolicy, -) -> Result { +) -> Result<(), ExternalUrlError> { match url.host() { // Domain names are mostly allowed here: if the name *resolves* to a // non-external IP, that will be rejected by `external_dns::Resolver`, @@ -318,7 +386,7 @@ fn ensure_external_url( None => {} } - Ok(url) + Ok(()) } #[cfg(test)] From 850cbcb1b3a1f50ef1e57254d1d0facb1073f87b Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 18 Jul 2026 09:33:28 -0700 Subject: [PATCH 08/25] whew okay this might actually sorta work --- nexus/src/app/background/init.rs | 5 +- .../background/tasks/webhook_deliverator.rs | 5 +- nexus/src/app/external_client.rs | 64 ++++++++++++------- nexus/src/app/mod.rs | 30 ++++----- nexus/src/app/webhook.rs | 36 ++++++++++- 5 files changed, 95 insertions(+), 45 deletions(-) diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 82bf16e693a..4ea84331d78 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -147,6 +147,7 @@ use super::tasks::vpc_routes; use super::tasks::webhook_deliverator; use crate::Nexus; use crate::app::background::tasks::populate_switch_ports; +use crate::app::external_client::ExternalHttpClient; use crate::app::oximeter::PRODUCER_LEASE_DURATION; use crate::app::quiesce::NexusQuiesceHandle; use crate::app::saga::StartSaga; @@ -1357,11 +1358,11 @@ pub struct BackgroundTasksData { pub tuf_artifact_replication_rx: mpsc::Receiver, /// Channel for exposing the latest loaded blueprint pub blueprint_load_tx: watch::Sender>, - /// `reqwest::Client` for webhook delivery requests. + /// [`ExternalHttpClient`] for webhook delivery requests. /// /// This is shared with the external API as it's also used when sending /// webhook liveness probe requests from the API. - pub webhook_delivery_client: reqwest::Client, + pub webhook_delivery_client: ExternalHttpClient, /// Channel for configuring pending MGS updates pub mgs_updates_tx: watch::Sender, /// handle for controlling Nexus quiesce diff --git a/nexus/src/app/background/tasks/webhook_deliverator.rs b/nexus/src/app/background/tasks/webhook_deliverator.rs index d7a0bc2a3b0..1eaa78ac2ca 100644 --- a/nexus/src/app/background/tasks/webhook_deliverator.rs +++ b/nexus/src/app/background/tasks/webhook_deliverator.rs @@ -31,6 +31,7 @@ //! [`app::webhook`]: crate::app::webhook use crate::app::background::BackgroundTask; +use crate::app::external_client::ExternalHttpClient; use crate::app::webhook::ReceiverClient; use futures::future::BoxFuture; use nexus_db_queries::context::OpContext; @@ -102,7 +103,7 @@ use std::sync::Arc; pub struct WebhookDeliverator { datastore: Arc, nexus_id: OmicronZoneUuid, - client: reqwest::Client, + client: ExternalHttpClient, cfg: DeliveryConfig, } @@ -135,7 +136,7 @@ impl WebhookDeliverator { datastore: Arc, cfg: DeliveryConfig, nexus_id: OmicronZoneUuid, - client: reqwest::Client, + client: ExternalHttpClient, ) -> Self { Self { datastore, nexus_id, cfg, client } } diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index c29f45d452e..be6b22bae37 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -10,7 +10,6 @@ use omicron_common::address::UNDERLAY_MULTICAST_SUBNET; use omicron_common::address::UnderlaySubnets; use oxnet::Ipv6Net; -use qorb::resolver; use reqwest::IntoUrl; use reqwest::Method; use std::net::IpAddr; @@ -125,15 +124,6 @@ pub enum ExternalUrlError { Localhost { host: String }, } -/// Errors returned by [`ExternalHttpClient::execute`]. -#[derive(thiserror::Error, Debug)] -pub enum ExecuteExternalRequestError { - #[error(transparent)] - Reqwest(#[from] reqwest::Error), - #[error(transparent)] - ExternalUrl(#[from] ExternalUrlError), -} - impl ExternalHttpClient { /// Constructs a new `ExternalHttpClient` from the provided /// [`ExternalHttpClientConfig`], using the provided @@ -143,6 +133,7 @@ impl ExternalHttpClient { /// /// This method fails if the [`reqwest::Client`] could not be built, such /// as if a TLS backend cannot be initialized. + #[allow(dead_code)] // may be used later pub fn new( config: &ExternalHttpClientConfig, resolver: &Arc, @@ -188,6 +179,7 @@ impl ExternalHttpClient { /// /// This method returns an error under the same conditions as /// [`ExternalHttpClient::request`]. + #[allow(dead_code)] // Added for completeness, may be used later pub fn get( &self, url: U, @@ -214,6 +206,7 @@ impl ExternalHttpClient { /// /// This method returns an error under the same conditions as /// [`ExternalHttpClient::request`]. + #[allow(dead_code)] // Added for completeness, may be used later pub fn put( &self, url: U, @@ -227,6 +220,7 @@ impl ExternalHttpClient { /// /// This method returns an error under the same conditions as /// [`ExternalHttpClient::request`]. + #[allow(dead_code)] // Added for completeness, may be used later pub fn patch( &self, url: U, @@ -240,6 +234,7 @@ impl ExternalHttpClient { /// /// This method returns an error under the same conditions as /// [`ExternalHttpClient::request`]. + #[allow(dead_code)] // Added for completeness, may be used later pub fn delete( &self, url: U, @@ -253,6 +248,7 @@ impl ExternalHttpClient { /// /// This method returns an error under the same conditions as /// [`ExternalHttpClient::request`]. + #[allow(dead_code)] // Added for completeness, may be used later pub fn head( &self, url: U, @@ -269,8 +265,8 @@ impl ExternalHttpClient { /// /// This method returns an error under the following conditions: /// - /// * [`UrlError::IntoUrl`] if the provided `url` could not be parsed into a - /// URL. + /// * [`ExternalUrlError::IntoUrl`] if the provided `url` could not be + /// parsed into a URL. /// * [`ExternalUrlError::NotExternalIp`] if the URL's host is an IP /// address that is not external --- an underlay network address, or /// (unless the policy permits it) a loopback address --- or if the @@ -292,6 +288,19 @@ impl ExternalHttpClient { /// A `Request` can be built manually with `Request::new()` or obtained /// from a RequestBuilder with `RequestBuilder::build()`. /// + /// This method's API is somewhat different from that of + /// [`reqwest::Client::execute`], which it is intended to replicate. + /// Reqwest's `execute` function returns a [`Future`]`>`, while this method + /// *synchronously* returns a [`Result`]* containing a similar [`Future`]. + /// The outer `Result` will be an [`ExternalUrlError`] if the request's URL + /// is not allowed by the external IP policy, while the inner [`Future`] + /// outputs the underlying [`reqwest::Response`] or [`reqwest::Error`] + /// returned by `reqwest` upon actually executing the HTTP request. Callers + /// can therefore handle "this URL is not allowed" and "the request failed + /// on the wire" at two different points, rather than having to dig both out + /// of a combined error type after the fact. + /// /// The `reqwest` documentation tells us that we "should prefer to use the /// `RequestBuilder` and `RequestBuilder::send()`". I disagree with them, /// because doing so returns both errors that occurred while *building* the @@ -304,13 +313,20 @@ impl ExternalHttpClient { /// /// # Errors /// - /// This method fails if there was an error while sending request, - /// redirect loop was detected or redirect limit was exhausted. + /// This method returns an [`ExternalUrlError`] if the request's URL is + /// rejected by the external IP policy, under the same conditions as + /// [`ExternalHttpClient::request`]. + /// + /// The returned future fails if there was an error while sending the + /// request, a redirect loop was detected, or the redirect limit was + /// exhausted. pub fn execute( &self, request: reqwest::Request, - ) -> impl Future> - { + ) -> Result< + impl Future>, + ExternalUrlError, + > { // ...and here we arrive at one of my least favorite things about // `reqwest`'s API. It has this `execute` method, which takes an // arbitrary `Request`, and...executes it. Like it says on the tin. But @@ -339,11 +355,9 @@ impl ExternalHttpClient { // really my favorite thing? // // Sigh. Whatever. It's fine. - let this = self.clone(); - async move { - ensure_external_url(request.url(), &this.ip_policy)?; - Ok(this.client.execute(request).await?) - } + ensure_external_url(request.url(), &self.ip_policy)?; + let client = self.client.clone(); + Ok(async move { client.execute(request).await }) } } @@ -412,7 +426,8 @@ mod test { let url = url .parse::() .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); - ensure_external_url(url, &test_policy(TreatLoopbackAsExternal::No)) + ensure_external_url(&url, &test_policy(TreatLoopbackAsExternal::No)) + .map(|()| url) } #[track_caller] @@ -421,9 +436,10 @@ mod test { .parse::() .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); ensure_external_url( - url, + &url, &test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), ) + .map(|()| url) } // Until the rack is initialized, no determination can be made about IP @@ -437,7 +453,7 @@ mod test { TreatLoopbackAsExternal::No, ); let url = "http://[2001:db8::1]/".parse::().unwrap(); - let result = ensure_external_url(url, &policy); + let result = ensure_external_url(&url, &policy); assert!( matches!( &result, diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index c0b886af91a..81bb8f3b898 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -238,7 +238,7 @@ pub struct Nexus { /// This lives on the Nexus struct as we would like to use the same client /// pool for the webhook deliverator background task and the webhook probe /// API. - webhook_delivery_client: reqwest::Client, + webhook_delivery_client: external_client::ExternalHttpClient, /// The tunable parameters from a configuration file tunables: Tunables, @@ -498,21 +498,19 @@ impl Nexus { )) }; - let webhook_delivery_client = { - // The webhook delivery HTTP client will send requests to endpoints - // external to the rack, so apply the configuration for external - // HTTP clients. - let builder = external_client::external_http_client_builder( - &config.deployment.external_http_clients, - &external_resolver, - ); - webhook::delivery_client(builder).map_err(|e| { - format!( - "failed to build webhook delivery client: {}", - InlineErrorChain::new(&e) - ) - })? - }; + // The webhook delivery HTTP client will send requests to endpoints + // external to the rack, so apply the configuration for external + // HTTP clients. + let webhook_delivery_client = webhook::delivery_client( + &config.deployment.external_http_clients, + &external_resolver, + ) + .map_err(|e| { + format!( + "failed to build webhook delivery client: {}", + InlineErrorChain::new(&e) + ) + })?; let mut mgs_resolver = qorb_resolver.for_service(ServiceName::ManagementGatewayService); diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index ae78a6f538c..d9a65db6aaf 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -29,6 +29,7 @@ use crate::Nexus; use crate::app::external_client::ExternalHttpClient; +use crate::app::external_dns; use anyhow::Context; use chrono::TimeDelta; use chrono::Utc; @@ -70,6 +71,7 @@ use omicron_uuid_kinds::WebhookDeliveryUuid; use omicron_uuid_kinds::WebhookSecretUuid; use sha2::Sha256; use slog_error_chain::InlineErrorChain; +use std::sync::Arc; use std::sync::LazyLock; use std::time::Duration; use std::time::Instant; @@ -509,8 +511,40 @@ impl<'a> ReceiverClient<'a> { } Ok(r) => r, }; + // The external IP policy check happens synchronously, when + // `ExternalHttpClient::execute` is called, rather than inside the + // returned future. If the receiver's URL is rejected by the policy, + // the request was never sent at all, so record the delivery attempt + // as having failed with no response status or duration, rather than + // trying to classify it as a wire-level error below. + let request_fut = match self.client.execute(request) { + Ok(request_fut) => request_fut, + Err(error) => { + slog::warn!( + &opctx.log, + "webhook receiver URL was rejected by the external IP \ + policy"; + "alert_id" => %delivery.alert_id, + "alert_class" => %alert_class, + "delivery_id" => %delivery.id, + "delivery_trigger" => %delivery.triggered_by, + "error" => InlineErrorChain::new(&error), + ); + return Ok(WebhookDeliveryAttempt { + id: WebhookDeliveryAttemptUuid::new_v4().into(), + delivery_id: delivery.id, + rx_id: delivery.rx_id, + attempt: SqlU8::new(delivery.attempts.0 + 1), + result: WebhookDeliveryAttemptResult::FailedUnreachable, + response_status: None, + response_duration: None, + time_created: chrono::Utc::now(), + deliverator_id: self.nexus_id.into(), + }); + } + }; let t0 = Instant::now(); - let result = self.client.execute(request).await; + let result = request_fut.await; let duration = t0.elapsed(); let (delivery_result, status) = match result { // Builder errors are our fault, that's weird! From e8ac3fe38a54def35e54489db739c842f3770c35 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 20 Jul 2026 12:02:33 -0700 Subject: [PATCH 09/25] redirects, dns tests --- Cargo.lock | 1 + nexus/Cargo.toml | 1 + nexus/src/app/external_client.rs | 449 +++++++++++++++++++++++++++++-- nexus/src/app/external_dns.rs | 41 ++- nexus/src/app/webhook.rs | 26 +- 5 files changed, 482 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7d6a1e0fad1..bc7100b41c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9281,6 +9281,7 @@ dependencies = [ "tokio-postgres", "tokio-util", "tough", + "transient-dns-server", "trust-quorum-types", "tufaceous 0.1.0", "tufaceous-artifact 0.1.0", diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index d5826626706..5ae00601c27 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -212,6 +212,7 @@ sp-sim.workspace = true strum.workspace = true subprocess.workspace = true term.workspace = true +transient-dns-server.workspace = true tufaceous.workspace = true tufaceous-artifact.workspace = true tufaceous-lib.workspace = true diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index be6b22bae37..9b9641c1bf0 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -12,6 +12,7 @@ use oxnet::Ipv6Net; use reqwest::IntoUrl; use reqwest::Method; +use reqwest::redirect; use std::net::IpAddr; use std::sync::Arc; use std::sync::OnceLock; @@ -124,36 +125,88 @@ pub enum ExternalUrlError { Localhost { host: String }, } -impl ExternalHttpClient { - /// Constructs a new `ExternalHttpClient` from the provided - /// [`ExternalHttpClientConfig`], using the provided - /// [`external_dns::Resolver`] for name resolution. +/// A builder for [`ExternalHttpClient`]s, wrapping a +/// [`reqwest::ClientBuilder`]. +/// +/// All client options *except* the redirect policy and DNS resolver are set +/// using the wrapped [`reqwest::ClientBuilder`], which is then converted into +/// an `ExternalClientBuilder` using its [`From`]`` +/// impl. +/// +/// ## Overwritten `reqwest::ClientBuilder` options +/// +/// The redirect policy must be set using [`ExternalClientBuilder::redirect`], +/// and *not* using [`reqwest::ClientBuilder::redirect`]. This is because the +/// built client checks the target of every redirect against the external IP +/// policy before consulting the redirect policy, which requires wrapping the +/// redirect policy in [our own](redirect::Policy::custom), and `reqwest` +/// provides no way to get a previously-set redirect policy back out of its +/// builder in order to wrap it. Unfortunately, there isn't any way to detect +/// whether we would be clobbering a policy provided by the caller, so...just +/// make sure not to do that, I guess. Sigh. +/// +/// Any DNS resolver set on the wrapped builder using +/// [`reqwest::ClientBuilder::dns_resolver`] is ignored, since this builder +/// always uses the [`external_dns::Resolver`]. +#[must_use = "builders do nothing unless, well...built"] +pub struct ExternalClientBuilder { + builder: reqwest::ClientBuilder, + redirect: Option, +} + +impl From for ExternalClientBuilder { + fn from(builder: reqwest::ClientBuilder) -> Self { + Self { builder, redirect: None } + } +} + +impl Default for ExternalClientBuilder { + fn default() -> Self { + Self::new() + } +} + +impl ExternalClientBuilder { + /// Returns a new `ExternalClientBuilder` with default options. + pub fn new() -> Self { + Self::from(reqwest::ClientBuilder::new()) + } + + /// Sets the [`redirect::Policy`] for the built client. /// - /// # Errors + /// The provided policy is consulted only for redirect targets that are + /// permitted by the external IP policy. A redirect target that is not + /// external (i.e., an underlay or loopback address, or a domain name that + /// resolves to one) fails the request with an [`ExternalUrlError`], without + /// consulting the provided policy at all. Note that this means a redirect + /// to a non-external target fails with an error even if the provided policy + /// is [`redirect::Policy::none`], which would otherwise stop and return the + /// redirect response itself. /// - /// This method fails if the [`reqwest::Client`] could not be built, such - /// as if a TLS backend cannot be initialized. - #[allow(dead_code)] // may be used later - pub fn new( - config: &ExternalHttpClientConfig, - resolver: &Arc, - ) -> Result { - Self::from_builder(config, resolver, reqwest::ClientBuilder::new()) + /// If this method is not called, the built client uses `reqwest`'s + /// default redirect policy (following up to 10 redirects), wrapped in the + /// same external IP policy check. + pub fn redirect(mut self, policy: redirect::Policy) -> Self { + self.redirect = Some(policy); + self } - /// Constructs a new `ExternalHttpClient` from the provided - /// [`reqwest::ClientBuilder`] and [`ExternalHttpClientConfig`], using the - /// provided [`external_dns::Resolver`] for name resolution. + /// Constructs a new [`ExternalHttpClient`] from the provided + /// [`ExternalHttpClientConfig`], using the provided + /// [`external_dns::Resolver`] for name resolution. /// /// # Errors /// /// This method fails if the [`reqwest::Client`] could not be built, such /// as if a TLS backend cannot be initialized. - pub fn from_builder( + pub fn build( + self, config: &ExternalHttpClientConfig, resolver: &Arc, - mut builder: reqwest::ClientBuilder, - ) -> Result { + ) -> Result { + #[allow(unused_mut)] // `mut` is unused on non-unix targets + let ExternalClientBuilder { mut builder, redirect } = self; + // If we are configured to only bind external TCP connections on a // specific interface, do so. #[cfg(any( @@ -167,10 +220,48 @@ impl ExternalHttpClient { } } - let client = builder.dns_resolver(resolver.clone()).build()?; let ip_policy = resolver.ip_policy().clone(); - Ok(Self { client, ip_policy }) + // Wrap the provided redirect policy (or reqwest's default, if none was + // provided) in one that checks redirect targets against the external IP + // policy before delegating to it. The check must come first, since + // `redirect::Action` is opaque, so we cannot ask the wrapped policy + // what it would do and check only the targets it would actually follow. + let redirect_policy = { + let inner = redirect.unwrap_or_default(); + let ip_policy = ip_policy.clone(); + redirect::Policy::custom(move |attempt| { + match ensure_external_url(attempt.url(), &ip_policy) { + Ok(()) => inner.redirect(attempt), + Err(error) => attempt.error(error), + } + }) + }; + + let client = builder + .redirect(redirect_policy) + .dns_resolver(resolver.clone()) + .build()?; + + Ok(ExternalHttpClient { client, ip_policy }) + } +} + +impl ExternalHttpClient { + /// Constructs a new `ExternalHttpClient` from the provided + /// [`ExternalHttpClientConfig`], using the provided + /// [`external_dns::Resolver`] for name resolution. + /// + /// # Errors + /// + /// This method fails if the [`reqwest::Client`] could not be built, such + /// as if a TLS backend cannot be initialized. + #[allow(dead_code)] // may be used later + pub fn new( + config: &ExternalHttpClientConfig, + resolver: &Arc, + ) -> Result { + ExternalClientBuilder::new().build(config, resolver) } /// Convenience method to make a `GET` request to a URL. @@ -406,7 +497,10 @@ fn ensure_external_url( #[cfg(test)] mod test { use super::*; + use internal_dns_types::config::DnsRecord; use omicron_common::address::{Ipv6Subnet, RACK_PREFIX}; + use std::collections::HashMap; + use transient_dns_server::TransientDnsServer; fn test_policy( loopback_policy: TreatLoopbackAsExternal, @@ -703,4 +797,317 @@ mod test { ); } } + + // A redirect to an underlay address fails the request, using the default + // (wrapped) redirect policy. + #[tokio::test] + async fn test_redirect_to_underlay_is_rejected() { + let server = httpmock::MockServer::start_async().await; + let mock = + mock_redirect(&server, "http://[fd00:1122:3344:100::1]/").await; + let client = client_without_dns(ExternalClientBuilder::new()); + let error = client + .get(server.url("/redirect")) + .expect("the initial URL is permitted") + .send() + .await + .expect_err("a redirect to an underlay address must fail"); + assert!( + error.is_redirect(), + "the failure should be a redirect error, but got: {error:?}" + ); + let chain = slog_error_chain::InlineErrorChain::new(&error).to_string(); + assert!( + chain.contains("underlay"), + "the error chain should mention the underlay, but got: {chain}" + ); + // The initial request must have been sent exactly once (i.e., the + // rejected redirect must not be retried). + mock.assert_async().await; + } + + // A redirect to a permitted target is delegated to the configured redirect + // policy: reqwest's default policy follows it to the final 200. + #[tokio::test] + async fn test_redirect_to_permitted_target_is_followed() { + let server = httpmock::MockServer::start_async().await; + let ok_mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET).path("/ok"); + then.status(200); + }) + .await; + let redirect_mock = mock_redirect(&server, &server.url("/ok")).await; + let client = client_without_dns(ExternalClientBuilder::new()); + let rsp = client + .get(server.url("/redirect")) + .expect("the initial URL is permitted") + .send() + .await + .expect("a redirect to a permitted target must be followed"); + assert_eq!(rsp.status(), reqwest::StatusCode::OK); + redirect_mock.assert_async().await; + ok_mock.assert_async().await; + } + + // A `redirect::Policy::none` is honored for permitted targets: the 3xx + // response itself is returned, and the redirect target is never + // requested. + #[tokio::test] + async fn test_wrapped_policy_none_stops_at_permitted_target() { + let server = httpmock::MockServer::start_async().await; + let ok_mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET).path("/ok"); + then.status(200); + }) + .await; + let redirect_mock = mock_redirect(&server, &server.url("/ok")).await; + let client = client_without_dns( + ExternalClientBuilder::new().redirect(redirect::Policy::none()), + ); + let rsp = client + .get(server.url("/redirect")) + .expect("the initial URL is permitted") + .send() + .await + .expect("`Policy::none` returns the 3xx response itself"); + assert_eq!(rsp.status(), reqwest::StatusCode::MOVED_PERMANENTLY); + redirect_mock.assert_async().await; + // The wrapped policy stopped the redirect, so the target must never + // be requested. + ok_mock.assert_calls_async(0).await; + } + + // A URL whose domain name *resolves* to an underlay address is rejected + // at resolution time by `external_dns::Resolver`: the URL-level check + // passes domain names through (it can't know what they resolve to), and + // the resolver applies the IP policy to the resolved addresses. + #[tokio::test] + async fn test_domain_resolving_to_underlay_is_rejected() { + use internal_dns_types::config::DnsRecord; + + let logctx = omicron_test_utils::dev::test_setup_log( + "test_domain_resolving_to_underlay_is_rejected", + ); + let records = std::collections::HashMap::from([( + "underlay".to_string(), + vec![DnsRecord::Aaaa("fd00:1122:3344:100::1".parse().unwrap())], + )]); + let (_dns, client) = client_with_dns_server( + &logctx.log, + records, + ExternalClientBuilder::new(), + ) + .await; + + let error = client + .get("http://underlay.example.com/") + .expect("a domain-host URL passes the URL-level check") + .send() + .await + .expect_err("a domain resolving to an underlay address must fail"); + let chain = + dbg!(slog_error_chain::InlineErrorChain::new(&error).to_string()); + assert!( + chain.contains("underlay"), + "the error chain should mention the underlay, but got: {chain}" + ); + logctx.cleanup_successful(); + } + + // A *redirect* to a URL whose domain name resolves to an underlay + // address is also rejected: the redirect-policy check passes domain + // names through (like the URL-level check), the redirect is followed, + // and the resolver then rejects the resolved address. + #[tokio::test] + async fn test_redirect_to_domain_resolving_to_underlay_is_rejected() { + use internal_dns_types::config::DnsRecord; + + let logctx = omicron_test_utils::dev::test_setup_log( + "test_redirect_to_domain_resolving_to_underlay_is_rejected", + ); + let records = std::collections::HashMap::from([( + "underlay".to_string(), + vec![DnsRecord::Aaaa("fd00:1122:3344:100::1".parse().unwrap())], + )]); + let (_dns, client) = client_with_dns_server( + &logctx.log, + records, + ExternalClientBuilder::new(), + ) + .await; + + let server = httpmock::MockServer::start_async().await; + let mock = mock_redirect(&server, "http://underlay.example.com/").await; + + let error = client + .get(server.url("/redirect")) + .expect("the initial URL is permitted") + .send() + .await + .expect_err( + "a redirect to a domain resolving to an underlay address \ + must fail", + ); + let chain = + dbg!(slog_error_chain::InlineErrorChain::new(&error).to_string()); + assert!( + chain.contains("underlay"), + "the error chain should mention the underlay, but got: {chain}" + ); + // The redirecting endpoint must have been requested exactly once. + mock.assert_async().await; + logctx.cleanup_successful(); + } + + // The `external_dns::Resolver` policy does *not* reject domains that + // resolve to addresses that aren't on the underlay (in this case, including + // loopback, since we are configured to permit it for tests). + #[tokio::test] + async fn test_domain_resolving_to_permitted_address_is_connected() { + let logctx = omicron_test_utils::dev::test_setup_log( + "test_domain_resolving_to_permitted_address_is_connected", + ); + let records = std::collections::HashMap::from([( + "ok".to_string(), + vec![DnsRecord::A(std::net::Ipv4Addr::LOCALHOST)], + )]); + let (_dns, client) = client_with_dns_server( + &logctx.log, + records, + ExternalClientBuilder::new(), + ) + .await; + + let server = httpmock::MockServer::start_async().await; + let ok_mock = server + .mock_async(|when, then| { + when.method(httpmock::Method::GET).path("/ok"); + then.status(200); + }) + .await; + + let rsp = client + .get(format!("http://ok.example.com:{}/ok", server.port())) + .expect("a domain-host URL passes the URL-level check") + .send() + .await + .expect("a domain resolving to a permitted address must work"); + assert_eq!(rsp.status(), reqwest::StatusCode::OK); + ok_mock.assert_async().await; + logctx.cleanup_successful(); + } + + // The external IP policy check runs *before* the wrapped policy: a + // redirect to an underlay address fails with an error even when the + // wrapped policy is `Policy::none`, which would otherwise stop and + // return the 3xx response, as in the test above. + #[tokio::test] + async fn test_underlay_redirect_rejected_even_with_policy_none() { + let server = httpmock::MockServer::start_async().await; + let mock = + mock_redirect(&server, "http://[fd00:1122:3344:100::1]/").await; + let client = client_without_dns( + ExternalClientBuilder::new().redirect(redirect::Policy::none()), + ); + let error = client + .get(server.url("/redirect")) + .expect("the initial URL is permitted") + .send() + .await + .expect_err( + "a redirect to an underlay address must fail even with \ + `Policy::none`", + ); + assert!( + error.is_redirect(), + "the failure should be a redirect error, but got: {error:?}" + ); + mock.assert_async().await; + } + + /// Mocks `GET /redirect` on `server` to respond with a 301 to + /// `redirect_to`. + async fn mock_redirect<'a>( + server: &'a httpmock::MockServer, + redirect_to: &str, + ) -> httpmock::Mock<'a> { + server + .mock_async(|when, then| { + when.method(httpmock::Method::GET).path("/redirect"); + then.status(301).header("location", redirect_to); + }) + .await + } + + /// Builds an `ExternalHttpClient` from `builder` using the provided + /// resolver, with loopback treated as external so that the test servers + /// on localhost are reachable. + fn client_with_resolver( + builder: ExternalClientBuilder, + resolver: Arc, + ) -> ExternalHttpClient { + let config = ExternalHttpClientConfig { + interface: None, + treat_loopback_as_external: + TreatLoopbackAsExternal::YesForTestPurposesOnly, + }; + builder.build(&config, &resolver).expect("client must build") + } + + /// Builds an `ExternalHttpClient` from `builder` for tests that use only + /// IP-literal URLs. + fn client_without_dns( + builder: ExternalClientBuilder, + ) -> ExternalHttpClient { + // The DNS server address is never actually queried, since the tests + // using this helper use IP-literal URLs; the resolver just needs to + // exist in order to build the client. + let resolver = Arc::new(external_dns::Resolver::new( + &[IpAddr::V4(std::net::Ipv4Addr::LOCALHOST)], + test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), + )); + client_with_resolver(builder, resolver) + } + + /// Spawns a `TransientDnsServer` serving the given `records` in the + /// `example.com` zone, and returns it along with an + /// `ExternalHttpClient` whose resolver points at it. + /// + /// The server is returned so that it stays alive for the duration of the + /// test. + async fn client_with_dns_server( + log: &slog::Logger, + records: HashMap>, + builder: ExternalClientBuilder, + ) -> (TransientDnsServer, ExternalHttpClient) { + use internal_dns_types::config::DnsConfigParams; + use internal_dns_types::config::DnsConfigZone; + use omicron_common::api::external::Generation; + + let dns = + TransientDnsServer::new(log).await.expect("DNS server must start"); + dns.initialize_with_config( + log, + &DnsConfigParams { + generation: Generation::new(), + serial: 0, + time_created: chrono::Utc::now(), + zones: vec![DnsConfigZone { + zone_name: "example.com".to_string(), + records, + }], + }, + ) + .await + .expect("DNS server must accept its config"); + + let resolver = Arc::new(external_dns::Resolver::new_from_addr( + dns.dns_server.local_address(), + test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), + )); + let client = client_with_resolver(builder, resolver); + (dns, client) + } } diff --git a/nexus/src/app/external_dns.rs b/nexus/src/app/external_dns.rs index f6d4b038ef8..df02c4b596f 100644 --- a/nexus/src/app/external_dns.rs +++ b/nexus/src/app/external_dns.rs @@ -31,13 +31,35 @@ impl Resolver { dns_servers: &[IpAddr], ip_policy: ExternalIpPolicy, ) -> Resolver { - assert!(!dns_servers.is_empty()); + let addrs = + dns_servers.iter().map(|&addr| SocketAddr::new(addr, DNS_PORT)); + Self::new_from_addrs(addrs, ip_policy) + } + + /// Constructs a `Resolver` using a single DNS server on an explicit socket + /// address, rather than from a set of IP addresses assuming the standard + /// DNS port. + /// + /// This is primarily useful for tests, where the DNS server is a + /// `transient_dns_server::TransientDnsServer` on an ephemeral port, since + /// an unprivileged test cannot bind port 53. Outside of tests, prefer + /// `Resolver::new`. + #[cfg(test)] + pub fn new_from_addr( + dns_server: SocketAddr, + ip_policy: ExternalIpPolicy, + ) -> Resolver { + Self::new_from_addrs(std::iter::once(dns_server), ip_policy) + } + + fn new_from_addrs( + dns_servers: impl IntoIterator, + ip_policy: ExternalIpPolicy, + ) -> Resolver { let mut rc = ResolverConfig::new(); + let mut n_dns_servers = 0; for addr in dns_servers { - let mut ns_config = NameServerConfig::new( - SocketAddr::new(*addr, DNS_PORT), - Protocol::Udp, - ); + let mut ns_config = NameServerConfig::new(addr, Protocol::Udp); // Explicltly set `trust_negative_responses` to false here to avoid // some churn. It reasonably could be `true` here. // @@ -61,13 +83,20 @@ impl Resolver { // builders to continue defaulting to trusting negative responses) ns_config.trust_negative_responses = false; rc.add_name_server(ns_config); + n_dns_servers += 1; } + + assert!( + n_dns_servers > 0, + "there must be at least one external DNS server" + ); + let mut opts = ResolverOpts::default(); // Enable edns for potentially larger records opts.edns0 = true; opts.use_hosts_file = ResolveHosts::Never; // Do as many requests in parallel as we have configured servers - opts.num_concurrent_reqs = dns_servers.len(); + opts.num_concurrent_reqs = n_dns_servers; Resolver { resolver: TokioResolver::builder_with_config( rc, diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index d9a65db6aaf..9a8d83fc5d5 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -28,6 +28,7 @@ //! [RFD 538]: https://rfd.shared.oxide.computer/538 use crate::Nexus; +use crate::app::external_client::ExternalClientBuilder; use crate::app::external_client::ExternalHttpClient; use crate::app::external_dns; use anyhow::Context; @@ -319,17 +320,13 @@ impl Nexus { } } -/// Construct a [`reqwest::Client`] configured for webhook delivery requests. +/// Construct an [`ExternalHttpClient`] configured for webhook delivery +/// requests. pub(super) fn delivery_client( external_client_config: &ExternalHttpClientConfig, resolver: &Arc, ) -> Result { - let builder = reqwest::ClientBuilder::new() - // Per [RFD 538 § 4.3.1][1], webhook delivery does *not* follow - // redirects. - // - // [1]: https://rfd.shared.oxide.computer/rfd/538#_success - .redirect(reqwest::redirect::Policy::none()) + let builder: ExternalClientBuilder = reqwest::ClientBuilder::new() // Per [RFD 538 § 4.3.2][1], the client must be able to connect to a // webhook receiver endpoint within 10 seconds, or the delivery is // considered failed. @@ -340,8 +337,19 @@ pub(super) fn delivery_client( // each webhook delivery request. // // [1]: https://rfd.shared.oxide.computer/rfd/538#delivery-failure - .timeout(Duration::from_secs(30)); - ExternalHttpClient::from_builder(external_client_config, resolver, builder) + .timeout(Duration::from_secs(30)) + .into(); + builder + // Per [RFD 538 § 4.3.1][1], webhook delivery does *not* follow + // redirects. + // + // N.B. that the redirect policy must be set on the + // `ExternalClientBuilder`, *not* the `reqwest::ClientBuilder`, or it + // will be silently overwritten. + // + // [1]: https://rfd.shared.oxide.computer/rfd/538#_success + .redirect(reqwest::redirect::Policy::none()) + .build(external_client_config, resolver) } /// Everything necessary to send a delivery request to a webhook receiver. From 37339db67ac94faabe557ea3b665f98cfa077140 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 20 Jul 2026 15:00:52 -0700 Subject: [PATCH 10/25] tidy up tests and test utils, add docs, etc --- nexus/src/app/external_client.rs | 387 ++++++++++++++++++++++--------- 1 file changed, 279 insertions(+), 108 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 9b9641c1bf0..76830056dad 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -2,6 +2,71 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +//! A [client](ExternalHttpClient) for requests to endpoints external to the +//! rack, and related machinery. +//! +//! ## Motivation +//! +//! In some cases, such as delivering webhook alerts, Nexus must communicate to +//! external endpoints whose URLs are provided by user configuration. When doing +//! so, it is important to ensure that the user-provided endpoint does not +//! resolve to an IP address on the underlay network. This is necessary to +//! prevent the risk of server-side request forgery (SSRF) attacks, in which +//! Nexus is used to smuggle an external request onto an underlay network +//! service which should otherwise not be exposed externally. In addition, a +//! non-malicious operator could inadvertantly misconfigure things so that the +//! IPv6 ULA address assigned to an external service collides with an underlay +//! network address, with confusing results. In such cases, it is better to fail +//! loudly than to accidentally send the request to an underlay service which is +//! not its intended destination. +//! +//! ## Theory of Operation +//! +//! We take a defense-in-depth approach to avoid accidentally sending external +//! requests to the underlay network. First, in the production control plane, we +//! configure the clients which are used to send external requests to bind +//! sockets only on the OPTE interface. The name of the interface in a given +//! Nexus zone is provided by the sled-agent through the +//! [`ExternalHttpClientConfig`]. +//! +//! Binding to the OPTE interface is in and of itself sufficient to provide +//! network-level isolation from the underlay for external clients. However, the +//! client type in this module also enforces external requests at the software +//! layer by rejecting any URL which either contains an underlay network address +//! as its host, or whose host is a domain name that resolves to underlay +//! network address(es). One reason we enforce this policy both at the client +//! and at the network level is for defense-in-depth; if either layer fails to +//! enforce the policy, we expect that the other will still ensure that external +//! requests are not sent to the underlay. In addition, checking at the client +//! level allows us to record better errors than the generic connection-level +//! errors that would otherwise be reported if the OPTE-level enforcement +//! prevents us from connecting to an address. This way, we can provide a more +//! useful error message for operators who are troubleshooting why a request was +//! not sent to the expected endpoint, and record when we prevented a potential +//! SSRF attack. +//! +//! The determination of which IP addresses are considered "external" is done by +//! the [`ExternalIpPolicy`] type, which is configured with the rack's +//! [`UnderlaySubnets`] once they are loaded from the database, or when rack is +//! initialized (if RSS has not yet run). This policy is used by the +//! [`ExternalHttpClient`] type in this module, which is a wrapper around a +//! [`reqwest::Client`] that enforces that all requested URLs are checked +//! against this policy before allowing a request to be sent. Unfortunately, +//! `reqwest` does not provide an integration point for a middleware that +//! decides which IP addresses are okay to connect to, so we have to wrap the +//! client and implement this on top of it. The policy is also passed to the +//! [`external_dns`] resolver, which is used when the client resolves a domain +//! name to IP addresses, and rejects any names that resolve to disallowed IPs. +//! Finally, we also wrap the redirect policy for the `reqwest` client to ensure +//! that following redirects also checks that the redirect does not point us at +//! an underlay address. +//! +//! Since we must construct this wrapper around the `reqwest::Client` API, we +//! also provide an [`ExternalClientBuilder`] to configure our client wrapper. +//! This is necessary because we must wrap the redirect policy used by the +//! client, as described above, and some external clients may need to follow +//! redirects, while others may not. + use super::external_dns; use nexus_config::ExternalHttpClientConfig; @@ -28,12 +93,9 @@ pub struct ExternalIpPolicy { /// Errors returned by [`ExternalIpPolicy::ensure_external_ip`]. #[derive(Debug, Clone, thiserror::Error)] pub enum ExternalIpError { - #[error( - "expected an external IP, but address {ip} is within the underlay \ - subnet {subnet}" - )] + #[error("address {ip} is within the underlay subnet {subnet}")] Underlay { ip: IpAddr, subnet: Ipv6Net }, - #[error("expected an external IP, but {ip} is a loopback address")] + #[error("address {ip} is a loopback address")] Loopback { ip: IpAddr }, #[error( "cannot check whether {ip} is an external IP address before the \ @@ -114,7 +176,10 @@ pub enum ExternalUrlError { /// The URL's host is an IP address which is not external, such as an /// underlay network address or a loopback address --- or the rack is not /// yet initialized, so no determination can be made. - #[error(transparent)] + #[error( + "IP addresses in URLs for external requests must be external to the \ + underlay network" + )] NotExternalIp(#[from] ExternalIpError), /// The URL's host is a name in the special-use `localhost.` zone, which /// resolves to loopback addresses. @@ -498,44 +563,14 @@ fn ensure_external_url( mod test { use super::*; use internal_dns_types::config::DnsRecord; - use omicron_common::address::{Ipv6Subnet, RACK_PREFIX}; + use omicron_common::address::{ + Ipv6Subnet, RACK_PREFIX, ReservedRackSubnet, UNDERLAY_MULTICAST_SUBNET, + UNDERLAY_MULTICAST_SUBNET_LAST, + }; use std::collections::HashMap; + use std::net::Ipv6Addr; use transient_dns_server::TransientDnsServer; - fn test_policy( - loopback_policy: TreatLoopbackAsExternal, - ) -> ExternalIpPolicy { - let rack_subnet: ipnetwork::Ipv6Network = - nexus_test_utils::RACK_SUBNET.parse().unwrap(); - let subnets = - UnderlaySubnets::new(Ipv6Subnet::::from(rack_subnet)); - ExternalIpPolicy::new( - Arc::new(OnceLock::from(subnets)), - loopback_policy, - ) - } - - #[track_caller] - fn test_url(url: &str) -> Result { - let url = url - .parse::() - .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); - ensure_external_url(&url, &test_policy(TreatLoopbackAsExternal::No)) - .map(|()| url) - } - - #[track_caller] - fn test_url_loopback_allowed(url: &str) -> Result { - let url = url - .parse::() - .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); - ensure_external_url( - &url, - &test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), - ) - .map(|()| url) - } - // Until the rack is initialized, no determination can be made about IP // hosts, so they are rejected. Domain hosts still pass, since their // resolved addresses are checked by `external_dns::Resolver` (which @@ -562,39 +597,57 @@ mod test { #[test] fn test_ensure_external_url_rejects_underlay_ipv6_hosts() { - const CASES: &[&str] = &[ - // An address within this rack's subnet. - "http://[fd00:1122:3344:100::1]/", + let subnets = underlay_subnets(); + let az = subnets.az_subnet.net(); + let rack = subnets.rack_subnet.net(); + // An arbitrary host address on this rack's subnet. + let rack_ip = underlay_ip(); + // An address in a sled subnet (the rack's second /64). + let sled_ip = nth_addr(rack, (1 << 64) + 5); + // An address in a different rack's /56, in the same AZ. + let other_rack_ip = { + let rack_size = 1u128 << (128 - RACK_PREFIX); + let rack_offset = + u128::from(rack.first_addr()) - u128::from(az.first_addr()); + nth_addr(az, rack_offset + rack_size + 5) + }; + // An internal DNS server address, derived from the reserved rack + // subnet the same way production does. + let dns_ip = ReservedRackSubnet::from_subnet(subnets.rack_subnet) + .get_dns_subnets()[0] + .dns_address(); + // An address in the middle of the underlay multicast subnet. + let multicast_ip = nth_addr(UNDERLAY_MULTICAST_SUBNET, (1 << 16) + 2); + + let cases = vec![ + format!("http://[{rack_ip}]/"), // Explicit port, path, query, and fragment don't change the answer. - "http://[fd00:1122:3344:100::1]:12345/some/path?q=1#frag", + format!("http://[{rack_ip}]:12345/some/path?q=1#frag"), // Neither does the scheme... - "https://[fd00:1122:3344:100::1]/", + format!("https://[{rack_ip}]/"), // ...nor userinfo. - "http://user:pass@[fd00:1122:3344:100::1]/", - // A sled subnet within this rack. - "http://[fd00:1122:3344:101::5]/", - // A different rack in the same AZ. - "http://[fd00:1122:3344:200::5]/", - // An internal DNS server address (in the reserved rack subnet). - "http://[fd00:1122:3344:1::1]/", - // The very first address in the AZ /48... - "http://[fd00:1122:3344::]/", + format!("http://user:pass@[{rack_ip}]/"), + format!("http://[{sled_ip}]/"), + format!("http://[{other_rack_ip}]/"), + format!("http://[{dns_ip}]/"), + // The very first address in the AZ subnet... + format!("http://[{}]/", az.first_addr()), // ...and the very last. - "http://[fd00:1122:3344:ffff:ffff:ffff:ffff:ffff]/", - // The underlay multicast subnet (ff04::/64) is also part of the - // underlay network: first address... - "http://[ff04::]/", + format!("http://[{}]/", az.last_addr()), + // The underlay multicast subnet is also part of the underlay + // network: first address... + format!("http://[{}]/", UNDERLAY_MULTICAST_SUBNET.first_addr()), // ...one in the middle... - "http://[ff04::1:2]/", - // ...and the last (`UNDERLAY_MULTICAST_SUBNET_LAST`). - "http://[ff04::ffff:ffff:ffff:ffff]/", + format!("http://[{multicast_ip}]/"), + // ...and the last. + format!("http://[{UNDERLAY_MULTICAST_SUBNET_LAST}]/"), // Non-canonical textual forms are normalized by the URL parser // before we see them: uppercase/verbose... - "http://[FD00:1122:3344:0100:0000:0000:0000:0001]/", + format!("http://[{}]/", verbose_upper(rack_ip)), // ...and an embedded dotted-quad tail. - "http://[fd00:1122:3344:100::1.2.3.4]/", + format!("http://[{}]/", dotted_quad_tail(rack)), ]; - for url in CASES { + for url in &cases { let result = test_url(url); assert!( result.is_err(), @@ -606,27 +659,40 @@ mod test { #[test] fn test_ensure_external_url_allows_external_ipv6_hosts() { - const CASES: &[&str] = &[ + let az = underlay_subnets().az_subnet.net(); + // The addresses immediately below and above the AZ subnet: the + // tightest possible off-by-one boundary cases. + let below_az = Ipv6Addr::from(u128::from(az.first_addr()) - 1); + let above_az = Ipv6Addr::from(u128::from(az.last_addr()) + 1); + // The address immediately after the underlay multicast subnet. + let after_multicast = + Ipv6Addr::from(u128::from(UNDERLAY_MULTICAST_SUBNET_LAST) + 1); + // A ULA that is *not* the underlay (e.g. a guest VPC prefix). + // ULA-ness alone must not be the discriminator. + let other_ula: Ipv6Addr = "fd12:3456:789a::1".parse().unwrap(); + assert!( + !az.contains(other_ula), + "the test's \"non-underlay ULA\" ({other_ula}) is within the \ + test AZ subnet ({az}); pick a different one" + ); + + let cases = vec![ // Global unicast. - "http://[2001:db8::1]/", - // A ULA that is *not* the underlay (e.g. a guest VPC prefix). - // ULA-ness alone must not be the discriminator. - "http://[fd12:3456:789a::1]/", - // Adjacent /48s: one below the AZ subnet... - "http://[fd00:1122:3343:ffff::1]/", - // ...and one above. - "http://[fd00:1122:3345::1]/", + "http://[2001:db8::1]/".to_string(), + format!("http://[{other_ula}]/"), + format!("http://[{below_az}]/"), + format!("http://[{above_az}]/"), // Link-local. - "http://[fe80::1]/", + "http://[fe80::1]/".to_string(), // Admin-scoped multicast just *outside* the underlay multicast - // /64 subnet... - "http://[ff04:0:0:1::1]/", + // subnet... + format!("http://[{after_multicast}]/"), // ...and multicast in a different scope entirely (site-local). - "http://[ff05::1:2]/", + "http://[ff05::1:2]/".to_string(), // IPv4-mapped IPv6. - "http://[::ffff:1.2.3.4]/", + "http://[::ffff:1.2.3.4]/".to_string(), ]; - for url in CASES { + for url in &cases { let result = test_url(url); assert!( result.is_ok(), @@ -736,12 +802,14 @@ mod test { // policy: it must never weaken the underlay check. #[test] fn test_loopback_policy_does_not_weaken_underlay_check() { - const CASES: &[&str] = &[ - "http://[fd00:1122:3344:100::1]/", - "http://[fd00:1122:3344:1::1]/", - "http://[ff04::1:2]/", + let cases = vec![ + format!("http://[{}]/", underlay_ip()), + format!( + "http://[{}]/", + nth_addr(UNDERLAY_MULTICAST_SUBNET, (1 << 16) + 2) + ), ]; - for url in CASES { + for url in &cases { let result = test_url_loopback_allowed(url); assert!( result.is_err(), @@ -802,9 +870,9 @@ mod test { // (wrapped) redirect policy. #[tokio::test] async fn test_redirect_to_underlay_is_rejected() { + let target = underlay_ip(); let server = httpmock::MockServer::start_async().await; - let mock = - mock_redirect(&server, "http://[fd00:1122:3344:100::1]/").await; + let mock = mock_redirect(&server, &format!("http://[{target}]/")).await; let client = client_without_dns(ExternalClientBuilder::new()); let error = client .get(server.url("/redirect")) @@ -816,11 +884,7 @@ mod test { error.is_redirect(), "the failure should be a redirect error, but got: {error:?}" ); - let chain = slog_error_chain::InlineErrorChain::new(&error).to_string(); - assert!( - chain.contains("underlay"), - "the error chain should mention the underlay, but got: {chain}" - ); + assert_underlay_rejection(&error, target); // The initial request must have been sent exactly once (i.e., the // rejected redirect must not be retried). mock.assert_async().await; @@ -890,9 +954,10 @@ mod test { let logctx = omicron_test_utils::dev::test_setup_log( "test_domain_resolving_to_underlay_is_rejected", ); + let target = underlay_ip(); let records = std::collections::HashMap::from([( "underlay".to_string(), - vec![DnsRecord::Aaaa("fd00:1122:3344:100::1".parse().unwrap())], + vec![DnsRecord::Aaaa(target)], )]); let (_dns, client) = client_with_dns_server( &logctx.log, @@ -907,12 +972,7 @@ mod test { .send() .await .expect_err("a domain resolving to an underlay address must fail"); - let chain = - dbg!(slog_error_chain::InlineErrorChain::new(&error).to_string()); - assert!( - chain.contains("underlay"), - "the error chain should mention the underlay, but got: {chain}" - ); + assert_underlay_rejection(&error, target); logctx.cleanup_successful(); } @@ -927,9 +987,10 @@ mod test { let logctx = omicron_test_utils::dev::test_setup_log( "test_redirect_to_domain_resolving_to_underlay_is_rejected", ); + let target = underlay_ip(); let records = std::collections::HashMap::from([( "underlay".to_string(), - vec![DnsRecord::Aaaa("fd00:1122:3344:100::1".parse().unwrap())], + vec![DnsRecord::Aaaa(target)], )]); let (_dns, client) = client_with_dns_server( &logctx.log, @@ -950,12 +1011,7 @@ mod test { "a redirect to a domain resolving to an underlay address \ must fail", ); - let chain = - dbg!(slog_error_chain::InlineErrorChain::new(&error).to_string()); - assert!( - chain.contains("underlay"), - "the error chain should mention the underlay, but got: {chain}" - ); + assert_underlay_rejection(&error, target); // The redirecting endpoint must have been requested exactly once. mock.assert_async().await; logctx.cleanup_successful(); @@ -1005,9 +1061,9 @@ mod test { // return the 3xx response, as in the test above. #[tokio::test] async fn test_underlay_redirect_rejected_even_with_policy_none() { + let target = underlay_ip(); let server = httpmock::MockServer::start_async().await; - let mock = - mock_redirect(&server, "http://[fd00:1122:3344:100::1]/").await; + let mock = mock_redirect(&server, &format!("http://[{target}]/")).await; let client = client_without_dns( ExternalClientBuilder::new().redirect(redirect::Policy::none()), ); @@ -1024,9 +1080,124 @@ mod test { error.is_redirect(), "the failure should be a redirect error, but got: {error:?}" ); + assert_underlay_rejection(&error, target); mock.assert_async().await; } + // === various test utilities === + + fn test_policy( + loopback_policy: TreatLoopbackAsExternal, + ) -> ExternalIpPolicy { + ExternalIpPolicy::new( + Arc::new(OnceLock::from(underlay_subnets())), + loopback_policy, + ) + } + + /// Returns the `UnderlaySubnets` for the test rack subnet + /// (`nexus_test_utils::RACK_SUBNET`). + /// + /// Test addresses are derived from this rather than hardcoded, so that + /// they remain on the underlay if the test rack subnet ever changes. + fn underlay_subnets() -> UnderlaySubnets { + let rack_subnet: ipnetwork::Ipv6Network = + nexus_test_utils::RACK_SUBNET.parse().unwrap(); + UnderlaySubnets::new(Ipv6Subnet::::from(rack_subnet)) + } + + /// Returns the `n`th address in `net`, asserting that it is actually + /// within `net` (i.e., that `n` doesn't overflow the subnet's host + /// bits). + fn nth_addr(net: Ipv6Net, n: u128) -> Ipv6Addr { + let addr = Ipv6Addr::from(u128::from(net.first_addr()) + n); + assert!(net.contains(addr), "address {addr} must be within {net}"); + addr + } + + /// An arbitrary host address on the test rack's subnet, used wherever a + /// test just needs "some underlay address". + fn underlay_ip() -> Ipv6Addr { + nth_addr(underlay_subnets().rack_subnet.net(), 1) + } + + /// Renders `addr` in fully-expanded, uppercase textual form (e.g. + /// `FD00:1122:3344:0100:0000:0000:0000:0001`), for exercising the URL + /// parser's normalization of non-canonical spellings. + fn verbose_upper(addr: Ipv6Addr) -> String { + addr.segments().map(|segment| format!("{segment:04X}")).join(":") + } + + /// Formats an address within `net` (whose bits 64..96 must be zero) in + /// the embedded dotted-quad textual form (e.g. + /// `fd00:1122:3344:100::1.2.3.4`), for exercising the URL parser's + /// normalization of non-canonical spellings. + fn dotted_quad_tail(net: Ipv6Net) -> String { + let s = net.first_addr().segments(); + assert_eq!( + s[4..6], + [0, 0], + "the `::` in the dotted-quad form requires bits 64..96 of {net} \ + to be zero" + ); + format!("{:x}:{:x}:{:x}:{:x}::1.2.3.4", s[0], s[1], s[2], s[3]) + } + + /// Asserts that `error`'s cause chain contains an + /// [`ExternalIpError::Underlay`] rejection of `ip`. + #[track_caller] + fn assert_underlay_rejection( + error: &(dyn std::error::Error + 'static), + expected_ip: Ipv6Addr, + ) { + let mut found_ip_error = None; + let mut next = Some(error); + while let Some(error) = dbg!(next) { + if let Some(e) = dbg!(error.downcast_ref::()) { + found_ip_error = Some(e); + break; + } + next = error.source(); + } + match found_ip_error { + Some(ExternalIpError::Underlay { ip, .. }) => assert_eq!( + *ip, + IpAddr::V6(expected_ip), + "the underlay rejection should be for {expected_ip}", + ), + other => panic!( + "the error chain should contain an underlay rejection, but \ + found {other:?} (chain: {})", + slog_error_chain::InlineErrorChain::new(error), + ), + } + } + + #[track_caller] + fn test_url(url: &str) -> Result { + let url = dbg!(url) + .parse::() + .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); + let result = ensure_external_url( + &url, + &test_policy(TreatLoopbackAsExternal::No), + ) + .map(|()| url); + dbg!(result) + } + + #[track_caller] + fn test_url_loopback_allowed(url: &str) -> Result { + let url = url + .parse::() + .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); + ensure_external_url( + &url, + &test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), + ) + .map(|()| url) + } + /// Mocks `GET /redirect` on `server` to respond with a 301 to /// `redirect_to`. async fn mock_redirect<'a>( From 9841080e608b8eb1893c4dbb8eb006aee360168a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 20 Jul 2026 15:36:54 -0700 Subject: [PATCH 11/25] oops get rid of todo --- nexus/src/app/webhook.rs | 45 +++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index 9a8d83fc5d5..80368919e9c 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -479,16 +479,40 @@ impl<'a> ReceiverClient<'a> { return Err(e).context(MSG); } }; - let mut request = self - .client - .post(&self.rx.endpoint).expect("TODO ELIZA YOU HAVE TO ACTUALLY HANDLE THE EXTERNAL CLIENT ERROR HERE LOL") - .header(HDR_RX_ID, self.hdr_rx_id.clone()) - .header(HDR_DELIVERY_ID, delivery.id.to_string()) - .header(HDR_ALERT_ID, delivery.alert_id.to_string()) - .header(HDR_ALERT_CLASS, alert_class.to_string()) - .header(HDR_ALERT_VERSION, alert_version.to_string()) - .header(HDR_TIMESTAMP, &sent_at) - .header(http::header::CONTENT_TYPE, "application/json"); + let mut request = match self.client.post(&self.rx.endpoint) { + Ok(req) => req, + Err(err) => { + slog::warn!( + &opctx.log, + "webhook receiver URL was rejected by the external IP \ + policy"; + "endpoint" => %self.rx.endpoint, + "alert_id" => %delivery.alert_id, + "alert_class" => %alert_class, + "delivery_id" => %delivery.id, + "delivery_trigger" => %delivery.triggered_by, + "error" => InlineErrorChain::new(&err), + ); + return Ok(WebhookDeliveryAttempt { + id: WebhookDeliveryAttemptUuid::new_v4().into(), + delivery_id: delivery.id, + rx_id: delivery.rx_id, + attempt: SqlU8::new(delivery.attempts.0 + 1), + result: WebhookDeliveryAttemptResult::FailedUnreachable, + response_status: None, + response_duration: None, + time_created: chrono::Utc::now(), + deliverator_id: self.nexus_id.into(), + }); + } + } + .header(HDR_RX_ID, self.hdr_rx_id.clone()) + .header(HDR_DELIVERY_ID, delivery.id.to_string()) + .header(HDR_ALERT_ID, delivery.alert_id.to_string()) + .header(HDR_ALERT_CLASS, alert_class.to_string()) + .header(HDR_ALERT_VERSION, alert_version.to_string()) + .header(HDR_TIMESTAMP, &sent_at) + .header(http::header::CONTENT_TYPE, "application/json"); // For each secret assigned to this webhook, calculate the HMAC and add a signature header. for (secret_id, mac) in &mut self.secrets { @@ -532,6 +556,7 @@ impl<'a> ReceiverClient<'a> { &opctx.log, "webhook receiver URL was rejected by the external IP \ policy"; + "endpoint" => %self.rx.endpoint, "alert_id" => %delivery.alert_id, "alert_class" => %alert_class, "delivery_id" => %delivery.id, From 3648896ef9996e8b088569691f674c8d0fefeccd Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 20 Jul 2026 15:56:51 -0700 Subject: [PATCH 12/25] no_proxy --- nexus/src/app/external_client.rs | 39 ++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 76830056dad..7ef17c06ce6 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -200,19 +200,25 @@ pub enum ExternalUrlError { /// /// ## Overwritten `reqwest::ClientBuilder` options /// -/// The redirect policy must be set using [`ExternalClientBuilder::redirect`], -/// and *not* using [`reqwest::ClientBuilder::redirect`]. This is because the -/// built client checks the target of every redirect against the external IP -/// policy before consulting the redirect policy, which requires wrapping the -/// redirect policy in [our own](redirect::Policy::custom), and `reqwest` -/// provides no way to get a previously-set redirect policy back out of its -/// builder in order to wrap it. Unfortunately, there isn't any way to detect -/// whether we would be clobbering a policy provided by the caller, so...just -/// make sure not to do that, I guess. Sigh. +/// * The redirect policy must be set using [`ExternalClientBuilder::redirect`], +/// and *not* using [`reqwest::ClientBuilder::redirect`]. This is because the +/// built client checks the target of every redirect against the external IP +/// policy before consulting the redirect policy, which requires wrapping the +/// redirect policy in [our own](redirect::Policy::custom), and `reqwest` +/// does not provide a way to get a previously-set redirect policy back out of its +/// builder in order to wrap it. Unfortunately, there isn't any way to detect +/// whether we would be clobbering a policy provided by the caller, so...just +/// make sure not to do that, I guess. Sigh. /// -/// Any DNS resolver set on the wrapped builder using -/// [`reqwest::ClientBuilder::dns_resolver`] is ignored, since this builder -/// always uses the [`external_dns::Resolver`]. +/// * Any DNS resolver set on the wrapped builder using +/// [`reqwest::ClientBuilder::dns_resolver`] is ignored, since this builder +/// always uses the [`external_dns::Resolver`]. +/// +/// * This is not *technically* an "overwritten `reqwest::ClientBuilder` +/// option", but I'm putting it here anyway: any +/// `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY` environment variables are ignored, +/// since nobody should be setting those in an Omicron zone in the first +/// place. #[must_use = "builders do nothing unless, well...built"] pub struct ExternalClientBuilder { builder: reqwest::ClientBuilder, @@ -306,6 +312,15 @@ impl ExternalClientBuilder { let client = builder .redirect(redirect_policy) .dns_resolver(resolver.clone()) + // Disable `reqwest`'s default behavior of honoring the + // `HTTP_PROXY`, `HTTPS_PROXY`, and `ALL_PROXY` environment + // variables. We don't ever expect these to be set in a real Omicron + // zone, and if they were to be set, they could cause us to send + // requests to underlay destinations despite all the work we go to + // not to do that here. If they're set on someone's dev box while + // running tests, honoring them would similarly screw up the test. + // So, turn this off. + .no_proxy() .build()?; Ok(ExternalHttpClient { client, ip_policy }) From 7b5db0fb9618a5588245cc8dd194a5066208f156 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 20 Jul 2026 16:01:54 -0700 Subject: [PATCH 13/25] stricter checks for loopback IPs --- nexus/src/app/external_client.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 7ef17c06ce6..e9af764f7c6 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -131,6 +131,9 @@ impl ExternalIpPolicy { .get() .ok_or(ExternalIpError::RackNotInitialized { ip })?; let az_subnet = subnets.az_subnet.net(); + // If this is an IPv4-mapped IPv6 address, convert it to the canonical + // form before checking it. + let ip = ip.to_canonical(); match ip { // IPv6 addresses that are within the AZ's underlay subnet are not // external to the rack. @@ -145,9 +148,9 @@ impl ExternalIpPolicy { subnet: UNDERLAY_MULTICAST_SUBNET, }) } - // Loopback addresses are not external, regardless of whether they - // are v6 or v4... - ip if ip.is_loopback() + // Loopback and unspecified addresses are not external, regardless of + // whether they are v6 or v4... + ip if (ip.is_loopback() || ip.is_unspecified()) // ...unless we are inside of an integration test, in which // case, lol. lmao. && self.loopback_policy == TreatLoopbackAsExternal::No => From 26cfa01a1656e217341a09fef73b670cea9eb00e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:23:40 -0700 Subject: [PATCH 14/25] post-rebase fixup --- nexus/src/app/external_client.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index e9af764f7c6..d52368d0714 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -582,8 +582,8 @@ mod test { use super::*; use internal_dns_types::config::DnsRecord; use omicron_common::address::{ - Ipv6Subnet, RACK_PREFIX, ReservedRackSubnet, UNDERLAY_MULTICAST_SUBNET, - UNDERLAY_MULTICAST_SUBNET_LAST, + Ipv6Subnet, RACK_PREFIX_LENGTH, ReservedRackSubnet, + UNDERLAY_MULTICAST_SUBNET, UNDERLAY_MULTICAST_SUBNET_LAST, }; use std::collections::HashMap; use std::net::Ipv6Addr; @@ -624,7 +624,7 @@ mod test { let sled_ip = nth_addr(rack, (1 << 64) + 5); // An address in a different rack's /56, in the same AZ. let other_rack_ip = { - let rack_size = 1u128 << (128 - RACK_PREFIX); + let rack_size = 1u128 << (128 - RACK_PREFIX_LENGTH); let rack_offset = u128::from(rack.first_addr()) - u128::from(az.first_addr()); nth_addr(az, rack_offset + rack_size + 5) @@ -1121,7 +1121,9 @@ mod test { fn underlay_subnets() -> UnderlaySubnets { let rack_subnet: ipnetwork::Ipv6Network = nexus_test_utils::RACK_SUBNET.parse().unwrap(); - UnderlaySubnets::new(Ipv6Subnet::::from(rack_subnet)) + UnderlaySubnets::new(Ipv6Subnet::::from( + rack_subnet, + )) } /// Returns the `n`th address in `net`, asserting that it is actually From 412972c421a623253407d755b97b941764af78f3 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:32:47 -0700 Subject: [PATCH 15/25] move bootstrap network prefix/length into `omicron_common::address` --- clients/ddm-admin-client/src/lib.rs | 7 ++++--- common/src/address.rs | 24 ++++++++++++++++++++++++ sled-hardware/src/underlay.rs | 4 ++-- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/clients/ddm-admin-client/src/lib.rs b/clients/ddm-admin-client/src/lib.rs index 46d0866f313..c9450ecefaa 100644 --- a/clients/ddm-admin-client/src/lib.rs +++ b/clients/ddm-admin-client/src/lib.rs @@ -11,10 +11,10 @@ pub use ddm_admin_client::types; use ddm_admin_client::Client as InnerClient; use either::Either; +use omicron_common::address::BOOTSTRAP_PREFIX; +use omicron_common::address::BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH; use omicron_common::address::DDMD_PORT; use oxnet::Ipv6Net; -use sled_hardware_types::underlay::BOOTSTRAP_MASK; -use sled_hardware_types::underlay::BOOTSTRAP_PREFIX; use sled_hardware_types::underlay::BootstrapInterface; use slog::Logger; use slog_error_chain::SlogInlineError; @@ -114,7 +114,8 @@ impl Client { Ok(prefixes.into_values().flat_map(|prefixes| { prefixes.into_iter().flat_map(|prefix| { let mut segments = prefix.destination.addr().segments(); - if prefix.destination.width() == BOOTSTRAP_MASK + if prefix.destination.width() + == BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH && segments[0] == BOOTSTRAP_PREFIX { Either::Left(interfaces.iter().map(move |interface| { diff --git a/common/src/address.rs b/common/src/address.rs index c1358720957..5715f6bedc8 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -449,6 +449,30 @@ pub const CP_SERVICES_RESERVED_ADDRESSES: u16 = 0xFFFF; /// with each sled in the Reconfigurator blueprint. pub const SLED_RESERVED_ADDRESSES: u16 = 2; +/// Initial octet of IPv6 for bootstrap addresses. +pub const BOOTSTRAP_PREFIX: u16 = 0xfdb0; + +/// IPv6 subnet for the *entire* bootstrap network. +/// +/// This is a /16 subnet with the prefix [`BOOTSTRAP_PREFIX`]. Individual sled +/// bootstrap networks are /64 subnets within this range, with the rest of the +/// previx constructed from the sled's MAC address. This constant is intended to +/// be used for checking whether an address is a bootstrap address on *any* +/// sled's bootstrap network. +pub const BOOTSTRAP_NETWORK_SUBNET: Ipv6Net = Ipv6Net::new_unchecked( + Ipv6Addr::new(BOOTSTRAP_PREFIX, 0, 0, 0, 0, 0, 0, 0), + 16, +); + +/// IPv6 prefix length for bootstrap network sled subnets. +/// +/// This is the length of the prefix for the bootstrap network *of an individual +/// sled*. The prefix begins with [`BOOTSTRAP_PREFIX`], and the rest of the /64 +/// prefix is constructed from the sled's MAC address. This is disticnt from the +/// prefix length of [`BOOTSTRAP_NETWORK_SUBNET`], which is the /16 subnet that +/// contains *all* sled bootstrap networks. +pub const BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH: u8 = 64; + /// Wraps an [`Ipv6Net`] with a compile-time prefix length. #[derive( Debug, diff --git a/sled-hardware/src/underlay.rs b/sled-hardware/src/underlay.rs index e8b3cf197b8..bd04c384c72 100644 --- a/sled-hardware/src/underlay.rs +++ b/sled-hardware/src/underlay.rs @@ -22,9 +22,9 @@ use omicron_common::api::external::MacAddr; use crate::DataLinks; #[doc(inline)] -pub use sled_hardware_types::underlay::BOOTSTRAP_MASK; +pub use omicron_common::address::BOOTSTRAP_PREFIX; #[doc(inline)] -pub use sled_hardware_types::underlay::BOOTSTRAP_PREFIX; +pub use omicron_common::address::BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH; #[doc(inline)] pub use sled_hardware_types::underlay::BootstrapInterface; From ca77a80ca97f3aad5a6bb736676788ce5cc48793 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:37:17 -0700 Subject: [PATCH 16/25] also reject bootstrap network IPs --- nexus/src/app/external_client.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index d52368d0714..696815703be 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -71,6 +71,7 @@ use super::external_dns; use nexus_config::ExternalHttpClientConfig; use nexus_config::TreatLoopbackAsExternal; +use omicron_common::address::BOOTSTRAP_NETWORK_SUBNET; use omicron_common::address::UNDERLAY_MULTICAST_SUBNET; use omicron_common::address::UnderlaySubnets; use oxnet::Ipv6Net; @@ -95,6 +96,11 @@ pub struct ExternalIpPolicy { pub enum ExternalIpError { #[error("address {ip} is within the underlay subnet {subnet}")] Underlay { ip: IpAddr, subnet: Ipv6Net }, + #[error( + "address {ip} is within the bootstrap network \ + {BOOTSTRAP_NETWORK_SUBNET}" + )] + BootstrapNetwork { ip: IpAddr }, #[error("address {ip} is a loopback address")] Loopback { ip: IpAddr }, #[error( @@ -119,6 +125,8 @@ impl ExternalIpPolicy { /// /// * [`ExternalIpError::Underlay`] if `ip` is within the AZ underlay /// subnet or the underlay multicast subnet. + /// * [`ExternalIpError::BootstrapNetwork`] if `ip` is within the bootstrap + /// network (and might be part of an individual sled's bootstrap subnet). /// * [`ExternalIpError::Loopback`] if `ip` is a loopback address /// * [`ExternalIpError::RackNotInitialized`] if the underlay subnets are /// not yet known, in which case no determination can be made at all. @@ -148,6 +156,11 @@ impl ExternalIpPolicy { subnet: UNDERLAY_MULTICAST_SUBNET, }) } + // IPv6 addresses that could be on a sled's bootstrap network are + // always disallowed. + IpAddr::V6(v6) if BOOTSTRAP_NETWORK_SUBNET.contains(v6) => { + Err(ExternalIpError::BootstrapNetwork { ip }) + } // Loopback and unspecified addresses are not external, regardless of // whether they are v6 or v4... ip if (ip.is_loopback() || ip.is_unspecified()) From 421b04a46025796b08c20a14602861b80395f17c Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:49:30 -0700 Subject: [PATCH 17/25] whoops forgot to finish doing the bootstrap network bits --- common/src/address.rs | 18 +++++++++++++----- sled-hardware/types/src/underlay.rs | 6 ------ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index 5715f6bedc8..c6cf677eb5f 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -19,6 +19,11 @@ use std::{ sync::LazyLock, }; +/// The routing prefix length of the bootstrap network /40. +/// +/// This is distinct from [`BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH`], which is the +/// prefix length of the /64 subnet for an individual sled's bootstrap +/// addresses. pub const BOOTSTRAP_SUBNET_PREFIX_LENGTH: u8 = 40; pub const AZ_PREFIX_LENGTH: u8 = 48; pub const RACK_PREFIX_LENGTH: u8 = 56; @@ -466,11 +471,14 @@ pub const BOOTSTRAP_NETWORK_SUBNET: Ipv6Net = Ipv6Net::new_unchecked( /// IPv6 prefix length for bootstrap network sled subnets. /// -/// This is the length of the prefix for the bootstrap network *of an individual -/// sled*. The prefix begins with [`BOOTSTRAP_PREFIX`], and the rest of the /64 -/// prefix is constructed from the sled's MAC address. This is disticnt from the -/// prefix length of [`BOOTSTRAP_NETWORK_SUBNET`], which is the /16 subnet that -/// contains *all* sled bootstrap networks. +/// This is the length of the prefix for the bootstrap network subnet *of an +/// individual sled*. The prefix begins with [`BOOTSTRAP_PREFIX`], and the rest +/// of the /64 prefix is constructed from the sled's MAC address. +/// +/// This is distinct from the prefix length of [`BOOTSTRAP_NETWORK_SUBNET`], +/// which is the /16 subnet that contains *all* sled bootstrap networks, and +/// from [`BOOTSTRAP_SUBNET_PREFIX_LENGTH`], which is the prefix length of the +/// announced bootstrap route. pub const BOOTSTRAP_SLED_SUBNET_PREFIX_LENGTH: u8 = 64; /// Wraps an [`Ipv6Net`] with a compile-time prefix length. diff --git a/sled-hardware/types/src/underlay.rs b/sled-hardware/types/src/underlay.rs index be81555d171..4fe7c60b409 100644 --- a/sled-hardware/types/src/underlay.rs +++ b/sled-hardware/types/src/underlay.rs @@ -2,12 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -/// Initial octet of IPv6 for bootstrap addresses. -pub const BOOTSTRAP_PREFIX: u16 = 0xfdb0; - -/// IPv6 prefix mask for bootstrap addresses. -pub const BOOTSTRAP_MASK: u8 = 64; - #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BootstrapInterface { GlobalZone, From bd1f7bc90c250b852d503b64934e3ab249c963e8 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:52:27 -0700 Subject: [PATCH 18/25] typo --- common/src/address.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/src/address.rs b/common/src/address.rs index c6cf677eb5f..6bd63c77a32 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -461,7 +461,7 @@ pub const BOOTSTRAP_PREFIX: u16 = 0xfdb0; /// /// This is a /16 subnet with the prefix [`BOOTSTRAP_PREFIX`]. Individual sled /// bootstrap networks are /64 subnets within this range, with the rest of the -/// previx constructed from the sled's MAC address. This constant is intended to +/// prefix constructed from the sled's MAC address. This constant is intended to /// be used for checking whether an address is a bootstrap address on *any* /// sled's bootstrap network. pub const BOOTSTRAP_NETWORK_SUBNET: Ipv6Net = Ipv6Net::new_unchecked( From 6de8ed44a6927db6f0e0146f6404fe8433adc821 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 10:59:27 -0700 Subject: [PATCH 19/25] that's two octets (for the price of one!) --- common/src/address.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/src/address.rs b/common/src/address.rs index 6bd63c77a32..953ccd40df2 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -454,7 +454,7 @@ pub const CP_SERVICES_RESERVED_ADDRESSES: u16 = 0xFFFF; /// with each sled in the Reconfigurator blueprint. pub const SLED_RESERVED_ADDRESSES: u16 = 2; -/// Initial octet of IPv6 for bootstrap addresses. +/// Initial octets of IPv6 for bootstrap addresses. pub const BOOTSTRAP_PREFIX: u16 = 0xfdb0; /// IPv6 subnet for the *entire* bootstrap network. From 9dcaef3d6d0b7ac09452fd23d1834f7a0fc13d5a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 11:35:09 -0700 Subject: [PATCH 20/25] add tests for bootstrap networks --- nexus/src/app/external_client.rs | 79 +++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 11 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 696815703be..90f6ce6e777 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -595,7 +595,7 @@ mod test { use super::*; use internal_dns_types::config::DnsRecord; use omicron_common::address::{ - Ipv6Subnet, RACK_PREFIX_LENGTH, ReservedRackSubnet, + BOOTSTRAP_PREFIX, Ipv6Subnet, RACK_PREFIX_LENGTH, ReservedRackSubnet, UNDERLAY_MULTICAST_SUBNET, UNDERLAY_MULTICAST_SUBNET_LAST, }; use std::collections::HashMap; @@ -722,6 +722,11 @@ mod test { "http://[ff05::1:2]/".to_string(), // IPv4-mapped IPv6. "http://[::ffff:1.2.3.4]/".to_string(), + // The addresses immediately below and above the bootstrap network + // (`fdb0::/16`): both are non-underlay ULAs, so the off-by-one + // boundaries of that /16 must pass. + "http://[fdaf:ffff:ffff:ffff:ffff:ffff:ffff:ffff]/".to_string(), + "http://[fdb1::]/".to_string(), ]; for url in &cases { let result = test_url(url); @@ -765,7 +770,11 @@ mod test { } // Loopback is not external, in any of its spellings: IPv6, IPv4 (the - // whole 127.0.0.0/8), or names in the special-use "localhost." zone. + // whole 127.0.0.0/8), IPv4-mapped IPv6 (which `ensure_external_ip` + // canonicalizes before checking), or names in the special-use + // "localhost." zone. The unspecified addresses (`0.0.0.0` and `::`) are + // rejected here too: they behave like loopback when establishing a + // connection, and therefore must also be rejected. // // NOTE: this means test environments cannot point an ExternalHttpClient // at a server on localhost; tests exercising webhook delivery et al. @@ -776,10 +785,17 @@ mod test { // IPv6 loopback. "http://[::1]/", "https://[::1]:8443/receiver", + // IPv4-mapped IPv6 loopback, folded to the IPv4 loopback by + // `to_canonical()` before the check. + "http://[::ffff:127.0.0.1]/", // IPv4 loopback, including the rest of 127.0.0.0/8. "http://127.0.0.1/", "http://127.0.0.1:8080/webhooks", "http://127.255.255.254/", + // The unspecified address, v4... + "http://0.0.0.0/", + // ...and v6. + "http://[::]/", // The WHATWG URL parser canonicalizes non-dotted-quad IPv4 forms // into `Host::Ipv4` before we see them, so these hit the IPv4 arm // rather than sneaking through as "domains": hexadecimal... @@ -813,6 +829,11 @@ mod test { const CASES: &[&str] = &[ "http://[::1]/", "https://[::1]:8443/receiver", + // IPv4-mapped IPv6 loopback and the unspecified addresses are + // permitted too, when the policy treats loopback as external. + "http://[::ffff:127.0.0.1]/", + "http://0.0.0.0/", + "http://[::]/", "http://127.0.0.1/", "http://127.0.0.1:8080/webhooks", "http://localhost/", @@ -850,6 +871,50 @@ mod test { } } + // Addresses in the bootstrap network (`fdb0::/16`) are internal to the + // rack and must be rejected. Like underlay addresses, this is regardless + // of whether we are using the test-only allow-loopback policy. + #[test] + fn test_ensure_external_url_rejects_bootstrap_network() { + let cases = vec![ + // The very first address in the bootstrap network... + format!("http://[{BOOTSTRAP_PREFIX:04x}::]/"), + // ...a realistic sled bootstrap address (taken from + // https://rfd.shared.oxide.computer/rfd/0063#_bootstrap_network) + format!("http://[{BOOTSTRAP_PREFIX:04x}:a840:2510:0001::1]/"), + // ...and the very last possible bootstrap address. + format!( + "http://[{BOOTSTRAP_PREFIX:04x}:ffff:ffff:ffff:ffff:ffff:ffff:ffff]/" + ), + ]; + for url in &cases { + // Rejected as `BootstrapNetwork` under the default policy... + let result = test_url(url); + assert!( + matches!( + &result, + Err(ExternalUrlError::NotExternalIp( + ExternalIpError::BootstrapNetwork { .. } + )) + ), + "URL {url:?} is in the bootstrap network and should have been \ + rejected as such, but got: {result:?}" + ); + // ...and still rejected when the loopback policy is permissive. + let result = test_url_loopback_allowed(url); + assert!( + matches!( + &result, + Err(ExternalUrlError::NotExternalIp( + ExternalIpError::BootstrapNetwork { .. } + )) + ), + "URL {url:?} is in the bootstrap network and must be rejected \ + even with loopback allowed, but got: {result:?}" + ); + } + } + // Domain hosts (other than the "localhost." zone) always pass this // check. If they *resolve* to a non-external address, that's rejected by // `external_dns::Resolver` at connect time instead. @@ -1208,15 +1273,7 @@ mod test { #[track_caller] fn test_url(url: &str) -> Result { - let url = dbg!(url) - .parse::() - .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); - let result = ensure_external_url( - &url, - &test_policy(TreatLoopbackAsExternal::No), - ) - .map(|()| url); - dbg!(result) + test_url_with_policy(url, TreatLoopbackAsExternal::No) } #[track_caller] From add9a12366d66ca45bb4b05b1eec73940d9c8241 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 21 Jul 2026 11:36:43 -0700 Subject: [PATCH 21/25] misc test tweaks --- nexus/src/app/external_client.rs | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 90f6ce6e777..6aa573d9dfc 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -927,8 +927,9 @@ mod test { "http://fd00.example.com/", // IDN (punycoded by the parser). "http://b\u{fc}cher.example/", - // "localhost" as a non-final label is *not* in the localhost - // zone: this is a legitimate external name. + // "localhost" as a non-final label is *not* in the localhost zone: + // this is a legitimate external name, even though you probably + // shouldn't do that. "http://localhost.example.com/", ]; for url in CASES { @@ -1250,8 +1251,8 @@ mod test { ) { let mut found_ip_error = None; let mut next = Some(error); - while let Some(error) = dbg!(next) { - if let Some(e) = dbg!(error.downcast_ref::()) { + while let Some(error) = next { + if let Some(e) = error.downcast_ref::() { found_ip_error = Some(e); break; } @@ -1278,14 +1279,27 @@ mod test { #[track_caller] fn test_url_loopback_allowed(url: &str) -> Result { + test_url_with_policy( + url, + TreatLoopbackAsExternal::YesForTestPurposesOnly, + ) + } + + #[track_caller] + fn test_url_with_policy( + url: &str, + loopback: TreatLoopbackAsExternal, + ) -> Result { + eprintln!( + "ensure_external_url({url:?}, TreatLoopbackAsExternal::{loopback:?})" + ); let url = url .parse::() .unwrap_or_else(|e| panic!("test URL {url:?} must parse: {e}")); - ensure_external_url( - &url, - &test_policy(TreatLoopbackAsExternal::YesForTestPurposesOnly), - ) - .map(|()| url) + let result = + ensure_external_url(&url, &test_policy(loopback)).map(|()| url); + eprintln!(" --> {result:?};\n"); + result } /// Mocks `GET /redirect` on `server` to respond with a 301 to From a940d9d4fea5006123ec87663c7c2d45044b5992 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 24 Jul 2026 10:45:01 -0700 Subject: [PATCH 22/25] nicer error logging for @inickles --- nexus/src/app/webhook.rs | 95 ++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 48 deletions(-) diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index 80368919e9c..dabff7325e1 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -30,6 +30,8 @@ use crate::Nexus; use crate::app::external_client::ExternalClientBuilder; use crate::app::external_client::ExternalHttpClient; +use crate::app::external_client::ExternalIpError; +use crate::app::external_client::ExternalUrlError; use crate::app::external_dns; use anyhow::Context; use chrono::TimeDelta; @@ -479,32 +481,52 @@ impl<'a> ReceiverClient<'a> { return Err(e).context(MSG); } }; + let handle_ext_url_error = |err: ExternalUrlError| { + // Log a different message depending on whether the webhook + // receiver URL is an invalid URL, or if it was rejected by the + // external IP policy. + let msg = match &err { + // If the URL just straight up cannot be parsed, that's not a + // security policy violation. + ExternalUrlError::IntoUrl(_) => "not a valid URL", + // We cannot check against the external IP policy if RSS hasn't + // run yet (this code really shouldn't be running at all, since + // no one can have created a receiver config in the first + // place!) + ExternalUrlError::NotExternalIp( + ExternalIpError::RackNotInitialized { .. }, + ) => { + return Err(anyhow::anyhow!(err)); + } + // Any other error indicates that the external IP policy did not + // allow the request to be sent. + _ => "rejected by external IP policy", + }; + slog::warn!( + &opctx.log, + "cannot deliver webhook requests to this endpoint: {msg}"; + "endpoint" => %self.rx.endpoint, + "alert_id" => %delivery.alert_id, + "alert_class" => %alert_class, + "delivery_id" => %delivery.id, + "delivery_trigger" => %delivery.triggered_by, + "error" => InlineErrorChain::new(&err), + ); + Ok(WebhookDeliveryAttempt { + id: WebhookDeliveryAttemptUuid::new_v4().into(), + delivery_id: delivery.id, + rx_id: delivery.rx_id, + attempt: SqlU8::new(delivery.attempts.0 + 1), + result: WebhookDeliveryAttemptResult::FailedUnreachable, + response_status: None, + response_duration: None, + time_created: chrono::Utc::now(), + deliverator_id: self.nexus_id.into(), + }) + }; let mut request = match self.client.post(&self.rx.endpoint) { Ok(req) => req, - Err(err) => { - slog::warn!( - &opctx.log, - "webhook receiver URL was rejected by the external IP \ - policy"; - "endpoint" => %self.rx.endpoint, - "alert_id" => %delivery.alert_id, - "alert_class" => %alert_class, - "delivery_id" => %delivery.id, - "delivery_trigger" => %delivery.triggered_by, - "error" => InlineErrorChain::new(&err), - ); - return Ok(WebhookDeliveryAttempt { - id: WebhookDeliveryAttemptUuid::new_v4().into(), - delivery_id: delivery.id, - rx_id: delivery.rx_id, - attempt: SqlU8::new(delivery.attempts.0 + 1), - result: WebhookDeliveryAttemptResult::FailedUnreachable, - response_status: None, - response_duration: None, - time_created: chrono::Utc::now(), - deliverator_id: self.nexus_id.into(), - }); - } + Err(err) => return handle_ext_url_error(err), } .header(HDR_RX_ID, self.hdr_rx_id.clone()) .header(HDR_DELIVERY_ID, delivery.id.to_string()) @@ -551,30 +573,7 @@ impl<'a> ReceiverClient<'a> { // trying to classify it as a wire-level error below. let request_fut = match self.client.execute(request) { Ok(request_fut) => request_fut, - Err(error) => { - slog::warn!( - &opctx.log, - "webhook receiver URL was rejected by the external IP \ - policy"; - "endpoint" => %self.rx.endpoint, - "alert_id" => %delivery.alert_id, - "alert_class" => %alert_class, - "delivery_id" => %delivery.id, - "delivery_trigger" => %delivery.triggered_by, - "error" => InlineErrorChain::new(&error), - ); - return Ok(WebhookDeliveryAttempt { - id: WebhookDeliveryAttemptUuid::new_v4().into(), - delivery_id: delivery.id, - rx_id: delivery.rx_id, - attempt: SqlU8::new(delivery.attempts.0 + 1), - result: WebhookDeliveryAttemptResult::FailedUnreachable, - response_status: None, - response_duration: None, - time_created: chrono::Utc::now(), - deliverator_id: self.nexus_id.into(), - }); - } + Err(err) => return handle_ext_url_error(err), }; let t0 = Instant::now(); let result = request_fut.await; From 2a605b4ca6d5034f06b296d345f8195b54f226c2 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 24 Jul 2026 11:01:25 -0700 Subject: [PATCH 23/25] also explicitly log policy violations post-DNS --- nexus/src/app/external_client.rs | 28 ++++++++++++------- nexus/src/app/webhook.rs | 48 ++++++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 6aa573d9dfc..dcc9e1739d2 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -206,6 +206,23 @@ pub enum ExternalUrlError { Localhost { host: String }, } +impl ExternalIpError { + /// Traverses `error`'s cause chain to extract an [`ExternalIplError`], if + /// the provided error was caused by one. + pub fn downcast_from<'err>( + error: &'err (dyn std::error::Error + 'static), + ) -> Option<&'err Self> { + let mut next = Some(error); + while let Some(error) = next { + if let Some(e) = error.downcast_ref::() { + return Some(e); + } + next = error.source(); + } + None + } +} + /// A builder for [`ExternalHttpClient`]s, wrapping a /// [`reqwest::ClientBuilder`]. /// @@ -1249,16 +1266,7 @@ mod test { error: &(dyn std::error::Error + 'static), expected_ip: Ipv6Addr, ) { - let mut found_ip_error = None; - let mut next = Some(error); - while let Some(error) = next { - if let Some(e) = error.downcast_ref::() { - found_ip_error = Some(e); - break; - } - next = error.source(); - } - match found_ip_error { + match ExternalIpError::downcast_from(error) { Some(ExternalIpError::Underlay { ip, .. }) => assert_eq!( *ip, IpAddr::V6(expected_ip), diff --git a/nexus/src/app/webhook.rs b/nexus/src/app/webhook.rs index dabff7325e1..7a6b5aa20bc 100644 --- a/nexus/src/app/webhook.rs +++ b/nexus/src/app/webhook.rs @@ -481,6 +481,10 @@ impl<'a> ReceiverClient<'a> { return Err(e).context(MSG); } }; + const ERR_BEFORE_RSS: &str = + "cannot deliver webhook requests before RSS"; + const ERR_IP_POLICY: &str = + "webhook receiver endpoint URL rejected by external IP policy"; let handle_ext_url_error = |err: ExternalUrlError| { // Log a different message depending on whether the webhook // receiver URL is an invalid URL, or if it was rejected by the @@ -488,7 +492,9 @@ impl<'a> ReceiverClient<'a> { let msg = match &err { // If the URL just straight up cannot be parsed, that's not a // security policy violation. - ExternalUrlError::IntoUrl(_) => "not a valid URL", + ExternalUrlError::IntoUrl(_) => { + "webhook receiver endpoint URL is not a valid URL" + } // We cannot check against the external IP policy if RSS hasn't // run yet (this code really shouldn't be running at all, since // no one can have created a receiver config in the first @@ -496,15 +502,15 @@ impl<'a> ReceiverClient<'a> { ExternalUrlError::NotExternalIp( ExternalIpError::RackNotInitialized { .. }, ) => { - return Err(anyhow::anyhow!(err)); + return Err(err).context(ERR_BEFORE_RSS); } // Any other error indicates that the external IP policy did not // allow the request to be sent. - _ => "rejected by external IP policy", + _ => ERR_IP_POLICY, }; slog::warn!( &opctx.log, - "cannot deliver webhook requests to this endpoint: {msg}"; + "{msg}"; "endpoint" => %self.rx.endpoint, "alert_id" => %delivery.alert_id, "alert_class" => %alert_class, @@ -595,7 +601,39 @@ impl<'a> ReceiverClient<'a> { return Err(e).context(MSG); } Err(e) => { - if let Some(status) = e.status() { + if let Some(ip_error) = ExternalIpError::downcast_from(&e) { + match ip_error { + // This one's on us --- again, it shouldn't be possible + // to deliver a webhook request if the rack hasn't been + // RSSed yet, so that's weird! + ExternalIpError::RackNotInitialized { .. } => { + return Err(e).context(ERR_BEFORE_RSS); + } + // Anything else indicates that the IP was rejected by + // the external IP policy. + _ => { + slog::warn!( + &opctx.log, + // This message should contain the same string + // as the one we emit if it's rejected pre-DNS, + // so that one can grep the logs for that + // string. + "{ERR_IP_POLICY} (after DNS resolution)"; + "endpoint" => %self.rx.endpoint, + "alert_id" => %delivery.alert_id, + "alert_class" => %alert_class, + "delivery_id" => %delivery.id, + "delivery_trigger" => %delivery.triggered_by, + "error" => InlineErrorChain::new(&e), + ); + + ( + WebhookDeliveryAttemptResult::FailedUnreachable, + None, + ) + } + } + } else if let Some(status) = e.status() { slog::warn!( &opctx.log, "webhook receiver endpoint returned an HTTP error"; From 94125e9308fa1d05948e7e8663629ac609a92ba1 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 24 Jul 2026 11:33:39 -0700 Subject: [PATCH 24/25] remove typo lol --- nexus/src/app/external_client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index dcc9e1739d2..0810c2bb65b 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -207,7 +207,7 @@ pub enum ExternalUrlError { } impl ExternalIpError { - /// Traverses `error`'s cause chain to extract an [`ExternalIplError`], if + /// Traverses `error`'s cause chain to extract an [`ExternalIpError`], if /// the provided error was caused by one. pub fn downcast_from<'err>( error: &'err (dyn std::error::Error + 'static), From 84e182fda794489ad2b97fdfb9aa7a0e00319b9c Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 28 Jul 2026 14:43:06 -0700 Subject: [PATCH 25/25] Update nexus/src/app/external_client.rs Co-authored-by: Levon Tarver <11586085+internet-diglett@users.noreply.github.com> --- nexus/src/app/external_client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/external_client.rs b/nexus/src/app/external_client.rs index 0810c2bb65b..28324bb3e00 100644 --- a/nexus/src/app/external_client.rs +++ b/nexus/src/app/external_client.rs @@ -14,7 +14,7 @@ //! prevent the risk of server-side request forgery (SSRF) attacks, in which //! Nexus is used to smuggle an external request onto an underlay network //! service which should otherwise not be exposed externally. In addition, a -//! non-malicious operator could inadvertantly misconfigure things so that the +//! non-malicious operator could inadvertently misconfigure things so that the //! IPv6 ULA address assigned to an external service collides with an underlay //! network address, with confusing results. In such cases, it is better to fail //! loudly than to accidentally send the request to an underlay service which is