From f09aaffa6457eb1d7b2ff80b729c72dcde8fcd41 Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Wed, 21 Aug 2024 12:46:14 -0400 Subject: [PATCH] Address review feedback Signed-off-by: Lann Martin --- crates/factor-outbound-http/src/spin.rs | 4 +- crates/factor-outbound-http/src/wasi.rs | 27 ++++--- crates/factor-outbound-networking/src/lib.rs | 81 +++++++++++--------- crates/trigger2/src/factors.rs | 4 +- 4 files changed, 64 insertions(+), 52 deletions(-) diff --git a/crates/factor-outbound-http/src/spin.rs b/crates/factor-outbound-http/src/spin.rs index 22ea723f2..c9dd6f186 100644 --- a/crates/factor-outbound-http/src/spin.rs +++ b/crates/factor-outbound-http/src/spin.rs @@ -1,4 +1,3 @@ -use spin_factor_outbound_networking::OutboundUrl; use spin_world::{ async_trait, v1::http, @@ -9,8 +8,7 @@ use spin_world::{ impl http::Host for crate::InstanceState { async fn send_request(&mut self, req: Request) -> Result { // FIXME(lann): This is all just a stub to test allowed_outbound_hosts - let outbound_url = OutboundUrl::parse(&req.uri, "https").or(Err(HttpError::InvalidUrl))?; - match self.allowed_hosts.allows(&outbound_url).await { + match self.allowed_hosts.check_url(&req.uri, "https").await { Ok(true) => (), _ => { return Err(HttpError::DestinationNotAllowed); diff --git a/crates/factor-outbound-http/src/wasi.rs b/crates/factor-outbound-http/src/wasi.rs index 925c2f9f9..e63f40b53 100644 --- a/crates/factor-outbound-http/src/wasi.rs +++ b/crates/factor-outbound-http/src/wasi.rs @@ -127,20 +127,24 @@ async fn send_request_impl( let is_allowed = outbound_allowed_hosts .check_url(&request.uri().to_string(), "https") .await - .map_err(|_| ErrorCode::HttpRequestUriInvalid)?; + .unwrap_or(false); if !is_allowed { return Ok(Err(ErrorCode::HttpRequestDenied)); } } else { // Relative URI ("self" request) - let allowed_hosts = outbound_allowed_hosts.resolve().await?; - if !allowed_hosts.allows_relative_url(&["http", "https"]) { - outbound_allowed_hosts.report_disallowed_host("http", "self"); + let is_allowed = outbound_allowed_hosts + .check_relative_url(&["http", "https"]) + .await + .unwrap_or(false); + if !is_allowed { return Ok(Err(ErrorCode::HttpRequestDenied)); } - let origin = self_request_origin - .context("cannot send relative outbound request; no 'origin' set by host")?; + let Some(origin) = self_request_origin else { + tracing::error!("Couldn't handle outbound HTTP request to relative URI; no origin set"); + return Ok(Err(ErrorCode::HttpRequestUriInvalid)); + }; config.use_tls = origin.use_tls(); @@ -150,12 +154,11 @@ async fn send_request_impl( *request.uri_mut() = origin.into_uri(path_and_query); } - if let Some(authority) = request.uri().authority() { - let current_span = tracing::Span::current(); - current_span.record("server.address", authority.host()); - if let Some(port) = authority.port() { - current_span.record("server.port", port.as_u16()); - } + let authority = request.uri().authority().context("authority not set")?; + let current_span = tracing::Span::current(); + current_span.record("server.address", authority.host()); + if let Some(port) = authority.port() { + current_span.record("server.port", port.as_u16()); } Ok(send_request_handler(request, config, tls_client_config).await) diff --git a/crates/factor-outbound-networking/src/lib.rs b/crates/factor-outbound-networking/src/lib.rs index f5e1c783e..7af5f34f1 100644 --- a/crates/factor-outbound-networking/src/lib.rs +++ b/crates/factor-outbound-networking/src/lib.rs @@ -24,20 +24,18 @@ pub type SharedFutureResult = Shared, Arc, + disallowed_host_handler: Option>, } -pub type DisallowedHostCallback = fn(scheme: &str, authority: &str); - impl OutboundNetworkingFactor { pub fn new() -> Self { Self::default() } - /// Sets a function to be called when a request is disallowed by an + /// Sets a handler to be called when a request is disallowed by an /// instance's configured `allowed_outbound_hosts`. - pub fn set_disallowed_host_callback(&mut self, callback: DisallowedHostCallback) { - self.disallowed_host_callback = Some(callback); + pub fn set_disallowed_host_handler(&mut self, handler: impl DisallowedHostHandler + 'static) { + self.disallowed_host_handler = Some(Arc::new(handler)); } } @@ -106,7 +104,7 @@ impl Factor for OutboundNetworkingFactor { // Update Wasi socket allowed ports let allowed_hosts = OutboundAllowedHosts { allowed_hosts_future: allowed_hosts_future.clone(), - disallowed_host_callback: self.disallowed_host_callback, + disallowed_host_handler: self.disallowed_host_handler.clone(), }; wasi_builder.outbound_socket_addr_check(move |addr, addr_use| { let allowed_hosts = allowed_hosts.clone(); @@ -137,7 +135,7 @@ impl Factor for OutboundNetworkingFactor { Ok(InstanceBuilder { allowed_hosts_future, component_tls_configs, - disallowed_host_callback: self.disallowed_host_callback, + disallowed_host_handler: self.disallowed_host_handler.clone(), }) } } @@ -150,14 +148,14 @@ pub struct AppState { pub struct InstanceBuilder { allowed_hosts_future: SharedFutureResult, component_tls_configs: ComponentTlsConfigs, - disallowed_host_callback: Option, + disallowed_host_handler: Option>, } impl InstanceBuilder { pub fn allowed_hosts(&self) -> OutboundAllowedHosts { OutboundAllowedHosts { allowed_hosts_future: self.allowed_hosts_future.clone(), - disallowed_host_callback: self.disallowed_host_callback, + disallowed_host_handler: self.disallowed_host_handler.clone(), } } @@ -178,33 +176,10 @@ impl FactorInstanceBuilder for InstanceBuilder { #[derive(Clone)] pub struct OutboundAllowedHosts { allowed_hosts_future: SharedFutureResult, - disallowed_host_callback: Option, + disallowed_host_handler: Option>, } impl OutboundAllowedHosts { - pub async fn resolve(&self) -> anyhow::Result> { - self.allowed_hosts_future.clone().await.map_err(|err| { - // TODO: better way to handle this? - anyhow::Error::msg(err) - }) - } - - /// Checks if the given URL is allowed by this component's - /// `allowed_outbound_hosts`. - pub async fn allows(&self, url: &OutboundUrl) -> anyhow::Result { - Ok(self.resolve().await?.allows(url)) - } - - /// Report that an outbound connection has been disallowed by e.g. - /// [`OutboundAllowedHosts::allows`] returning `false`. - /// - /// Calls the [`DisallowedHostCallback`] if set. - pub fn report_disallowed_host(&self, scheme: &str, authority: &str) { - if let Some(disallowed_host_callback) = self.disallowed_host_callback { - disallowed_host_callback(scheme, authority); - } - } - /// Checks address against allowed hosts /// /// Calls the [`DisallowedHostCallback`] if set and URL is disallowed. @@ -217,11 +192,47 @@ impl OutboundAllowedHosts { }; let allowed_hosts = self.resolve().await?; - let is_allowed = allowed_hosts.allows(&url); if !is_allowed { self.report_disallowed_host(url.scheme(), &url.authority()); } Ok(is_allowed) } + + /// Checks if allowed hosts permit relative requests + /// + /// Calls the [`DisallowedHostCallback`] if set and relative requests are + /// disallowed. + pub async fn check_relative_url(&self, schemes: &[&str]) -> anyhow::Result { + let allowed_hosts = self.resolve().await?; + let is_allowed = allowed_hosts.allows_relative_url(schemes); + if !is_allowed { + let scheme = schemes.first().unwrap_or(&""); + self.report_disallowed_host(scheme, "self"); + } + Ok(is_allowed) + } + + async fn resolve(&self) -> anyhow::Result> { + self.allowed_hosts_future.clone().await.map_err(|err| { + tracing::error!("Error resolving allowed_outbound_hosts variables: {err}"); + anyhow::Error::msg(err) + }) + } + + fn report_disallowed_host(&self, scheme: &str, authority: &str) { + if let Some(handler) = &self.disallowed_host_handler { + handler.handle_disallowed_host(scheme, authority); + } + } +} + +pub trait DisallowedHostHandler: Send + Sync { + fn handle_disallowed_host(&self, scheme: &str, authority: &str); +} + +impl DisallowedHostHandler for F { + fn handle_disallowed_host(&self, scheme: &str, authority: &str) { + self(scheme, authority); + } } diff --git a/crates/trigger2/src/factors.rs b/crates/trigger2/src/factors.rs index e4719e506..42b2dae13 100644 --- a/crates/trigger2/src/factors.rs +++ b/crates/trigger2/src/factors.rs @@ -45,7 +45,7 @@ fn wasi_factor(working_dir: impl Into, allow_transient_writes: bool) -> } fn outbound_networking_factor() -> OutboundNetworkingFactor { - fn disallowed_host_callback(scheme: &str, authority: &str) { + fn disallowed_host_handler(scheme: &str, authority: &str) { let host_pattern = format!("{scheme}://{authority}"); tracing::error!("Outbound network destination not allowed: {host_pattern}"); if scheme.starts_with("http") && authority == "self" { @@ -59,7 +59,7 @@ fn outbound_networking_factor() -> OutboundNetworkingFactor { } let mut factor = OutboundNetworkingFactor::new(); - factor.set_disallowed_host_callback(disallowed_host_callback); + factor.set_disallowed_host_handler(disallowed_host_handler); factor }