From 4702941ee08a72f7b89d30250d04ce09c29e10ae Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Thu, 25 Jul 2024 17:12:05 +0200 Subject: [PATCH 1/3] Change how incoming-request.authority is set. Always rely on the `Host` header for authority incoming-reques.authority. To determine where `self` requests should be routed, explicitly pass a `self_authority` parameter which is set to the listening address. Signed-off-by: Ryan Levick --- crates/trigger-http/src/handler.rs | 46 ++++++++--------- crates/trigger-http/src/lib.rs | 50 +++++++++++++++---- crates/trigger-http/src/wagi.rs | 1 + tests/runtime-tests/src/lib.rs | 2 +- .../src/runtimes/in_process_spin.rs | 14 ++++-- 5 files changed, 72 insertions(+), 41 deletions(-) diff --git a/crates/trigger-http/src/handler.rs b/crates/trigger-http/src/handler.rs index 4ff4aa3980..42bc57c208 100644 --- a/crates/trigger-http/src/handler.rs +++ b/crates/trigger-http/src/handler.rs @@ -33,6 +33,7 @@ impl HttpExecutor for HttpHandlerExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, + self_authority: &str, ) -> Result> { let component_id = route_match.component_id(); @@ -46,7 +47,7 @@ impl HttpExecutor for HttpHandlerExecutor { unreachable!() }; - set_http_origin_from_request(&mut store, engine.clone(), self, &req); + set_http_origin(&mut store, engine.clone(), self, &self_authority); // set the client tls options for the current component_id. // The OutboundWasiHttpHandler in this file is only used @@ -390,36 +391,31 @@ impl HandlerType { } } -fn set_http_origin_from_request( +fn set_http_origin( store: &mut Store, engine: Arc>, handler: &HttpHandlerExecutor, - req: &Request, + self_authority: &str, ) { - if let Some(authority) = req.uri().authority() { - if let Some(scheme) = req.uri().scheme_str() { - let origin = format!("{}://{}", scheme, authority); - if let Some(outbound_http_handle) = engine - .engine - .find_host_component_handle::>() - { - let outbound_http_data = store - .host_components_data() - .get_or_insert(outbound_http_handle); - - outbound_http_data.origin.clone_from(&origin); - store.as_mut().data_mut().as_mut().allowed_hosts = - outbound_http_data.allowed_hosts.clone(); - } + let origin = format!("http://{self_authority}"); + if let Some(outbound_http_handle) = engine + .engine + .find_host_component_handle::>() + { + let outbound_http_data = store + .host_components_data() + .get_or_insert(outbound_http_handle); - let chained_request_handler = ChainedRequestHandler { - engine: engine.clone(), - executor: handler.clone(), - }; - store.as_mut().data_mut().as_mut().origin = Some(origin); - store.as_mut().data_mut().as_mut().chained_handler = Some(chained_request_handler); - } + outbound_http_data.origin.clone_from(&origin); + store.as_mut().data_mut().as_mut().allowed_hosts = outbound_http_data.allowed_hosts.clone(); } + + let chained_request_handler = ChainedRequestHandler { + engine: engine.clone(), + executor: handler.clone(), + }; + store.as_mut().data_mut().as_mut().origin = Some(origin); + store.as_mut().data_mut().as_mut().chained_handler = Some(chained_request_handler); } fn contextualise_err(e: anyhow::Error) -> anyhow::Error { diff --git a/crates/trigger-http/src/lib.rs b/crates/trigger-http/src/lib.rs index c5c5f066c1..0d86e21fc0 100644 --- a/crates/trigger-http/src/lib.rs +++ b/crates/trigger-http/src/lib.rs @@ -238,7 +238,7 @@ impl HttpTrigger { server_addr: SocketAddr, client_addr: SocketAddr, ) -> Result> { - set_req_uri(&mut req, scheme, server_addr)?; + set_req_uri(&mut req, scheme)?; strip_forbidden_headers(&mut req); spin_telemetry::extract_trace_context(&req); @@ -288,6 +288,7 @@ impl HttpTrigger { &route_match, req, client_addr, + server_addr.to_string().as_str(), ) .await } @@ -302,6 +303,7 @@ impl HttpTrigger { &route_match, req, client_addr, + server_addr.to_string().as_str(), ) .await } @@ -370,6 +372,7 @@ impl HttpTrigger { stream: S, server_addr: SocketAddr, client_addr: SocketAddr, + scheme: Scheme, ) { task::spawn(async move { if let Err(e) = http1::Builder::new() @@ -377,8 +380,12 @@ impl HttpTrigger { .serve_connection( TokioIo::new(stream), service_fn(move |request| { - self.clone() - .instrumented_service_fn(server_addr, client_addr, request) + self.clone().instrumented_service_fn( + server_addr, + client_addr, + scheme.clone(), + request, + ) }), ) .await @@ -392,6 +399,7 @@ impl HttpTrigger { self: Arc, server_addr: SocketAddr, client_addr: SocketAddr, + scheme: Scheme, request: Request, ) -> Result> { let span = http_span!(request, client_addr); @@ -403,7 +411,7 @@ impl HttpTrigger { body.map_err(wasmtime_wasi_http::hyper_response_error) .boxed() }), - Scheme::HTTP, + scheme, server_addr, client_addr, ) @@ -419,7 +427,7 @@ impl HttpTrigger { loop { let (stream, client_addr) = listener.accept().await?; self.clone() - .serve_connection(stream, listen_addr, client_addr); + .serve_connection(stream, listen_addr, client_addr, Scheme::HTTP); } } @@ -435,7 +443,10 @@ impl HttpTrigger { loop { let (stream, addr) = listener.accept().await?; match acceptor.accept(stream).await { - Ok(stream) => self.clone().serve_connection(stream, listen_addr, addr), + Ok(stream) => { + self.clone() + .serve_connection(stream, listen_addr, addr, Scheme::HTTPS) + } Err(err) => tracing::error!(?err, "Failed to start TLS session"), } } @@ -475,11 +486,21 @@ fn parse_listen_addr(addr: &str) -> anyhow::Result { /// The incoming request's scheme and authority /// -/// The incoming request's URI is relative to the server, so we need to set the scheme and authority -fn set_req_uri(req: &mut Request, scheme: Scheme, addr: SocketAddr) -> Result<()> { +/// The incoming request's URI is relative to the server, so we need to set the scheme and authority. +/// The `Host` header is used to set the authority. This function will error if not `Host` header is +/// present or if it is not parseable as an `Authority`. +fn set_req_uri(req: &mut Request, scheme: Scheme) -> Result<()> { let uri = req.uri().clone(); let mut parts = uri.into_parts(); - let authority = format!("{}:{}", addr.ip(), addr.port()).parse().unwrap(); + let headers = req.headers(); + let authority = headers + .get(HOST) + .context("missing Host header")? + .to_str() + .context("Host header is not valid UTF-8")?; + let authority = authority + .parse() + .context("Host header contains an invalid authority")?; parts.scheme = Some(scheme); parts.authority = Some(authority); *req.uri_mut() = Uri::from_parts(parts).unwrap(); @@ -573,6 +594,7 @@ pub(crate) trait HttpExecutor: Clone + Send + Sync + 'static { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, + self_authority: &str, ) -> Result>; } @@ -620,9 +642,17 @@ impl HttpRuntimeData { let between_bytes_timeout = config.between_bytes_timeout; + let self_authority = this.origin.clone().expect("origin must be set"); let resp_fut = async move { match handler - .execute(engine.clone(), base, &route_match, request, client_addr) + .execute( + engine.clone(), + base, + &route_match, + request, + client_addr, + &self_authority, + ) .await { Ok(resp) => Ok(Ok(IncomingResponse { diff --git a/crates/trigger-http/src/wagi.rs b/crates/trigger-http/src/wagi.rs index 0cb0202006..d602060de6 100644 --- a/crates/trigger-http/src/wagi.rs +++ b/crates/trigger-http/src/wagi.rs @@ -28,6 +28,7 @@ impl HttpExecutor for WagiHttpExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, + _self_authority: &str, ) -> Result> { let component = route_match.component_id(); diff --git a/tests/runtime-tests/src/lib.rs b/tests/runtime-tests/src/lib.rs index 560fdd851b..6dac62cdb9 100644 --- a/tests/runtime-tests/src/lib.rs +++ b/tests/runtime-tests/src/lib.rs @@ -147,7 +147,7 @@ impl RuntimeTest { pub fn run(&mut self) { self.run_test(|env| { let runtime = env.runtime_mut(); - let response = runtime.make_http_request(Request::new(Method::Get, "/"))?; + let response = runtime.make_http_request(Request::full(Method::Get, "/", &[("Host", "example.com")], None))?; if response.status() == 200 { return Ok(()); } diff --git a/tests/testing-framework/src/runtimes/in_process_spin.rs b/tests/testing-framework/src/runtimes/in_process_spin.rs index 5574c72b55..91a72cc6a9 100644 --- a/tests/testing-framework/src/runtimes/in_process_spin.rs +++ b/tests/testing-framework/src/runtimes/in_process_spin.rs @@ -40,12 +40,16 @@ impl InProcessSpin { pub fn make_http_request(&self, req: Request<'_, &[u8]>) -> anyhow::Result { tokio::runtime::Runtime::new()?.block_on(async { let method: reqwest::Method = req.method.into(); - let req = http::request::Request::builder() + let mut builder = http::request::Request::builder() .method(method) - .uri(req.path) - // TODO(rylev): convert headers and body as well - .body(spin_http::body::empty()) - .unwrap(); + .uri(req.path); + + for (key, value) in req.headers { + builder = builder.header(*key, *value); + } + + // TODO(rylev): convert body as well + let req = builder.body(spin_http::body::empty()).unwrap(); let response = self .trigger .handle( From 79663eb5521189f83c292b454204c209b77e6a72 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 26 Jul 2024 10:42:19 +0200 Subject: [PATCH 2/3] Weave scheme through in the definition of self requests Signed-off-by: Ryan Levick --- crates/trigger-http/src/handler.rs | 14 +++++++++----- crates/trigger-http/src/lib.rs | 25 +++++++++++++++++++------ crates/trigger-http/src/wagi.rs | 4 ++-- tests/integration.rs | 2 +- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/crates/trigger-http/src/handler.rs b/crates/trigger-http/src/handler.rs index 42bc57c208..90a04d51d9 100644 --- a/crates/trigger-http/src/handler.rs +++ b/crates/trigger-http/src/handler.rs @@ -1,6 +1,9 @@ use std::{net::SocketAddr, str, str::FromStr}; -use crate::{Body, ChainedRequestHandler, HttpExecutor, HttpInstance, HttpTrigger, Store}; +use crate::{ + Body, ChainedRequestHandler, HttpExecutor, HttpInstance, HttpTrigger, SelfRequestDefinition, + Store, +}; use anyhow::{anyhow, Context, Result}; use futures::TryFutureExt; use http::{HeaderName, HeaderValue}; @@ -33,7 +36,7 @@ impl HttpExecutor for HttpHandlerExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - self_authority: &str, + self_definition: SelfRequestDefinition, ) -> Result> { let component_id = route_match.component_id(); @@ -47,7 +50,7 @@ impl HttpExecutor for HttpHandlerExecutor { unreachable!() }; - set_http_origin(&mut store, engine.clone(), self, &self_authority); + set_http_origin(&mut store, engine.clone(), self, self_definition); // set the client tls options for the current component_id. // The OutboundWasiHttpHandler in this file is only used @@ -395,9 +398,9 @@ fn set_http_origin( store: &mut Store, engine: Arc>, handler: &HttpHandlerExecutor, - self_authority: &str, + self_definition: SelfRequestDefinition, ) { - let origin = format!("http://{self_authority}"); + let origin = format!("{}://{}", self_definition.scheme, self_definition.authority); if let Some(outbound_http_handle) = engine .engine .find_host_component_handle::>() @@ -413,6 +416,7 @@ fn set_http_origin( let chained_request_handler = ChainedRequestHandler { engine: engine.clone(), executor: handler.clone(), + self_definition, }; store.as_mut().data_mut().as_mut().origin = Some(origin); store.as_mut().data_mut().as_mut().chained_handler = Some(chained_request_handler); diff --git a/crates/trigger-http/src/lib.rs b/crates/trigger-http/src/lib.rs index 0d86e21fc0..3220d233e9 100644 --- a/crates/trigger-http/src/lib.rs +++ b/crates/trigger-http/src/lib.rs @@ -238,7 +238,7 @@ impl HttpTrigger { server_addr: SocketAddr, client_addr: SocketAddr, ) -> Result> { - set_req_uri(&mut req, scheme)?; + set_req_uri(&mut req, scheme.clone())?; strip_forbidden_headers(&mut req); spin_telemetry::extract_trace_context(&req); @@ -278,6 +278,12 @@ impl HttpTrigger { let trigger = self.component_trigger_configs.get(component_id).unwrap(); let executor = trigger.executor.as_ref().unwrap_or(&HttpExecutorType::Http); + // Set the definition of outbound requests to `self` to be equal to + // the incoming request's scheme and the bound listening address. + let self_definition = SelfRequestDefinition { + scheme, + authority: server_addr.to_string(), + }; let res = match executor { HttpExecutorType::Http => { @@ -288,7 +294,7 @@ impl HttpTrigger { &route_match, req, client_addr, - server_addr.to_string().as_str(), + self_definition, ) .await } @@ -303,7 +309,7 @@ impl HttpTrigger { &route_match, req, client_addr, - server_addr.to_string().as_str(), + self_definition, ) .await } @@ -594,14 +600,22 @@ pub(crate) trait HttpExecutor: Clone + Send + Sync + 'static { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - self_authority: &str, + self_definition: SelfRequestDefinition, ) -> Result>; } +/// The definition of the `self` host for outbound requests. +#[derive(Clone)] +pub struct SelfRequestDefinition { + scheme: Scheme, + authority: String, +} + #[derive(Clone)] struct ChainedRequestHandler { engine: Arc>, executor: HttpHandlerExecutor, + self_definition: SelfRequestDefinition, } #[derive(Default)] @@ -642,7 +656,6 @@ impl HttpRuntimeData { let between_bytes_timeout = config.between_bytes_timeout; - let self_authority = this.origin.clone().expect("origin must be set"); let resp_fut = async move { match handler .execute( @@ -651,7 +664,7 @@ impl HttpRuntimeData { &route_match, request, client_addr, - &self_authority, + chained_handler.self_definition, ) .await { diff --git a/crates/trigger-http/src/wagi.rs b/crates/trigger-http/src/wagi.rs index d602060de6..2133d4a80c 100644 --- a/crates/trigger-http/src/wagi.rs +++ b/crates/trigger-http/src/wagi.rs @@ -1,6 +1,6 @@ use std::{io::Cursor, net::SocketAddr, sync::Arc}; -use crate::HttpInstance; +use crate::{HttpInstance, SelfRequestDefinition}; use anyhow::{anyhow, ensure, Context, Result}; use async_trait::async_trait; use http_body_util::BodyExt; @@ -28,7 +28,7 @@ impl HttpExecutor for WagiHttpExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - _self_authority: &str, + _self_authority: SelfRequestDefinition, ) -> Result> { let component = route_match.component_id(); diff --git a/tests/integration.rs b/tests/integration.rs index 99ac5aeb99..b62cbfde3f 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -355,7 +355,7 @@ Caused by: } #[test] - fn outbound_http_works() -> anyhow::Result<()> { + fn outbound_http_to_same_app_works() -> anyhow::Result<()> { run_test( "outbound-http-to-same-app", SpinConfig { From 2a0c116cf94f6b84e6e76977233017e5f250c388 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 29 Jul 2024 11:55:56 +0200 Subject: [PATCH 3/3] PR feedback Signed-off-by: Ryan Levick --- crates/trigger-http/src/handler.rs | 13 ++++++------- crates/trigger-http/src/lib.rs | 22 +++++++++++----------- crates/trigger-http/src/wagi.rs | 4 ++-- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/crates/trigger-http/src/handler.rs b/crates/trigger-http/src/handler.rs index 90a04d51d9..4fa3dd44ea 100644 --- a/crates/trigger-http/src/handler.rs +++ b/crates/trigger-http/src/handler.rs @@ -1,8 +1,7 @@ use std::{net::SocketAddr, str, str::FromStr}; use crate::{ - Body, ChainedRequestHandler, HttpExecutor, HttpInstance, HttpTrigger, SelfRequestDefinition, - Store, + Body, ChainedRequestHandler, HttpExecutor, HttpInstance, HttpTrigger, SelfRequestOrigin, Store, }; use anyhow::{anyhow, Context, Result}; use futures::TryFutureExt; @@ -36,7 +35,7 @@ impl HttpExecutor for HttpHandlerExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - self_definition: SelfRequestDefinition, + self_origin: SelfRequestOrigin, ) -> Result> { let component_id = route_match.component_id(); @@ -50,7 +49,7 @@ impl HttpExecutor for HttpHandlerExecutor { unreachable!() }; - set_http_origin(&mut store, engine.clone(), self, self_definition); + set_http_origin(&mut store, engine.clone(), self, self_origin); // set the client tls options for the current component_id. // The OutboundWasiHttpHandler in this file is only used @@ -398,9 +397,9 @@ fn set_http_origin( store: &mut Store, engine: Arc>, handler: &HttpHandlerExecutor, - self_definition: SelfRequestDefinition, + self_origin: SelfRequestOrigin, ) { - let origin = format!("{}://{}", self_definition.scheme, self_definition.authority); + let origin = format!("{}://{}", self_origin.scheme, self_origin.authority); if let Some(outbound_http_handle) = engine .engine .find_host_component_handle::>() @@ -416,7 +415,7 @@ fn set_http_origin( let chained_request_handler = ChainedRequestHandler { engine: engine.clone(), executor: handler.clone(), - self_definition, + self_origin, }; store.as_mut().data_mut().as_mut().origin = Some(origin); store.as_mut().data_mut().as_mut().chained_handler = Some(chained_request_handler); diff --git a/crates/trigger-http/src/lib.rs b/crates/trigger-http/src/lib.rs index 3220d233e9..d7f5749197 100644 --- a/crates/trigger-http/src/lib.rs +++ b/crates/trigger-http/src/lib.rs @@ -280,7 +280,7 @@ impl HttpTrigger { let executor = trigger.executor.as_ref().unwrap_or(&HttpExecutorType::Http); // Set the definition of outbound requests to `self` to be equal to // the incoming request's scheme and the bound listening address. - let self_definition = SelfRequestDefinition { + let self_origin = SelfRequestOrigin { scheme, authority: server_addr.to_string(), }; @@ -294,7 +294,7 @@ impl HttpTrigger { &route_match, req, client_addr, - self_definition, + self_origin, ) .await } @@ -309,7 +309,7 @@ impl HttpTrigger { &route_match, req, client_addr, - self_definition, + self_origin, ) .await } @@ -493,18 +493,18 @@ fn parse_listen_addr(addr: &str) -> anyhow::Result { /// The incoming request's scheme and authority /// /// The incoming request's URI is relative to the server, so we need to set the scheme and authority. -/// The `Host` header is used to set the authority. This function will error if not `Host` header is +/// The `Host` header is used to set the authority. This function will error if no `Host` header is /// present or if it is not parseable as an `Authority`. fn set_req_uri(req: &mut Request, scheme: Scheme) -> Result<()> { let uri = req.uri().clone(); let mut parts = uri.into_parts(); let headers = req.headers(); - let authority = headers + let host_header = headers .get(HOST) .context("missing Host header")? .to_str() .context("Host header is not valid UTF-8")?; - let authority = authority + let authority = host_header .parse() .context("Host header contains an invalid authority")?; parts.scheme = Some(scheme); @@ -600,13 +600,13 @@ pub(crate) trait HttpExecutor: Clone + Send + Sync + 'static { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - self_definition: SelfRequestDefinition, + self_origin: SelfRequestOrigin, ) -> Result>; } -/// The definition of the `self` host for outbound requests. +/// The origin of the `self` host for outbound requests. #[derive(Clone)] -pub struct SelfRequestDefinition { +pub struct SelfRequestOrigin { scheme: Scheme, authority: String, } @@ -615,7 +615,7 @@ pub struct SelfRequestDefinition { struct ChainedRequestHandler { engine: Arc>, executor: HttpHandlerExecutor, - self_definition: SelfRequestDefinition, + self_origin: SelfRequestOrigin, } #[derive(Default)] @@ -664,7 +664,7 @@ impl HttpRuntimeData { &route_match, request, client_addr, - chained_handler.self_definition, + chained_handler.self_origin, ) .await { diff --git a/crates/trigger-http/src/wagi.rs b/crates/trigger-http/src/wagi.rs index 2133d4a80c..d2f605d1fd 100644 --- a/crates/trigger-http/src/wagi.rs +++ b/crates/trigger-http/src/wagi.rs @@ -1,6 +1,6 @@ use std::{io::Cursor, net::SocketAddr, sync::Arc}; -use crate::{HttpInstance, SelfRequestDefinition}; +use crate::{HttpInstance, SelfRequestOrigin}; use anyhow::{anyhow, ensure, Context, Result}; use async_trait::async_trait; use http_body_util::BodyExt; @@ -28,7 +28,7 @@ impl HttpExecutor for WagiHttpExecutor { route_match: &RouteMatch, req: Request, client_addr: SocketAddr, - _self_authority: SelfRequestDefinition, + _self_origin: SelfRequestOrigin, ) -> Result> { let component = route_match.component_id();