From 5bf5cea18774f4bb4a1cd58caf9ebd60ac324e0f Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 19 Aug 2024 11:09:27 +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 the listening address. Signed-off-by: Ryan Levick --- crates/trigger-http2/src/lib.rs | 13 ++++-- crates/trigger-http2/src/outbound_http.rs | 3 +- crates/trigger-http2/src/server.rs | 53 +++++++++++++++++------ 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/crates/trigger-http2/src/lib.rs b/crates/trigger-http2/src/lib.rs index 856468916..d36b70d6b 100644 --- a/crates/trigger-http2/src/lib.rs +++ b/crates/trigger-http2/src/lib.rs @@ -67,7 +67,8 @@ pub(crate) type InstanceState = (); /// The Spin HTTP trigger. pub struct HttpTrigger { - listen_addr: SocketAddr, + /// The address the server will listen on. + addr_to_bind: SocketAddr, tls_config: Option, router: Router, // Component ID -> component trigger config @@ -112,7 +113,7 @@ impl Trigger for HttpTrigger { ); Ok(Self { - listen_addr: cli_args.address, + addr_to_bind: cli_args.address, tls_config: cli_args.into_tls_config(), router, component_trigger_configs, @@ -121,7 +122,7 @@ impl Trigger for HttpTrigger { async fn run(self, trigger_app: TriggerApp) -> anyhow::Result<()> { let Self { - listen_addr, + addr_to_bind: listen_addr, tls_config, router, component_trigger_configs, @@ -131,6 +132,12 @@ impl Trigger for HttpTrigger { .await .with_context(|| format!("Unable to listen on {listen_addr}"))?; + // Get the address the server is actually listening on + // We can't use `self.listen_addr` because it might not + // be fully resolved (e.g, port 0). + let listen_addr = listener + .local_addr() + .context("failed to retrieve address server is listening on")?; let server = Arc::new(HttpServer::new( listen_addr, trigger_app, diff --git a/crates/trigger-http2/src/outbound_http.rs b/crates/trigger-http2/src/outbound_http.rs index ac99a1e7f..623eb6b69 100644 --- a/crates/trigger-http2/src/outbound_http.rs +++ b/crates/trigger-http2/src/outbound_http.rs @@ -3,6 +3,7 @@ use std::{ sync::Arc, }; +use http::uri::Scheme; use spin_factor_outbound_http::{ HostFutureIncomingResponse, InterceptOutcome, OutgoingRequestConfig, Request, SelfRequestOrigin, }; @@ -42,7 +43,7 @@ impl spin_factor_outbound_http::OutboundHttpInterceptor for OutboundHttpIntercep let server = self.server.clone(); let resp_fut = async move { match server - .handle_trigger_route(req, route_match, CHAINED_CLIENT_ADDR) + .handle_trigger_route(req, route_match, Scheme::HTTP, CHAINED_CLIENT_ADDR) .await { Ok(resp) => Ok(Ok(IncomingResponse { diff --git a/crates/trigger-http2/src/server.rs b/crates/trigger-http2/src/server.rs index 8e346369b..1474c9ddd 100644 --- a/crates/trigger-http2/src/server.rs +++ b/crates/trigger-http2/src/server.rs @@ -37,6 +37,7 @@ use crate::{ }; pub struct HttpServer { + /// The address the server is listening on. listen_addr: SocketAddr, trigger_app: TriggerApp, router: Router, @@ -74,7 +75,8 @@ impl HttpServer { self.print_startup_msgs("http", &listener)?; loop { let (stream, client_addr) = listener.accept().await?; - self.clone().serve_connection(stream, client_addr); + self.clone() + .serve_connection(stream, Scheme::HTTP, client_addr); } } @@ -88,7 +90,9 @@ impl HttpServer { loop { let (stream, client_addr) = listener.accept().await?; match acceptor.accept(stream).await { - Ok(stream) => self.clone().serve_connection(stream, client_addr), + Ok(stream) => self + .clone() + .serve_connection(stream, Scheme::HTTPS, client_addr), Err(err) => tracing::error!(?err, "Failed to start TLS session"), } } @@ -98,10 +102,10 @@ impl HttpServer { async fn handle( self: &Arc, mut req: Request, - scheme: Scheme, + server_scheme: Scheme, client_addr: SocketAddr, ) -> anyhow::Result> { - set_req_uri(&mut req, scheme, self.listen_addr)?; + set_req_uri(&mut req, server_scheme.clone())?; strip_forbidden_headers(&mut req); spin_telemetry::extract_trace_context(&req); @@ -124,7 +128,7 @@ impl HttpServer { match self.router.route(&path) { Ok(route_match) => { - self.handle_trigger_route(req, route_match, client_addr) + self.handle_trigger_route(req, route_match, server_scheme, client_addr) .await } Err(_) => Self::not_found(NotFoundRouteKind::Normal(path.to_string())), @@ -135,6 +139,7 @@ impl HttpServer { self: &Arc, req: Request, route_match: RouteMatch, + server_scheme: Scheme, client_addr: SocketAddr, ) -> anyhow::Result> { let app_id = self @@ -155,9 +160,15 @@ impl HttpServer { let mut instance_builder = self.trigger_app.prepare(component_id)?; // Set up outbound HTTP request origin and service chaining - let uri = req.uri(); - let origin = SelfRequestOrigin::from_uri(uri) - .with_context(|| format!("invalid request URI {uri:?}"))?; + let origin = SelfRequestOrigin { + scheme: server_scheme, + authority: self.listen_addr.to_string().parse().with_context(|| { + format!( + "server address '{}' is not a valid authority", + self.listen_addr + ) + })?, + }; instance_builder .factor_builders() .outbound_http() @@ -259,6 +270,7 @@ impl HttpServer { fn serve_connection( self: Arc, stream: S, + server_scheme: Scheme, client_addr: SocketAddr, ) { task::spawn(async move { @@ -267,7 +279,11 @@ impl HttpServer { .serve_connection( TokioIo::new(stream), service_fn(move |request| { - self.clone().instrumented_service_fn(client_addr, request) + self.clone().instrumented_service_fn( + server_scheme.clone(), + client_addr, + request, + ) }), ) .await @@ -279,6 +295,7 @@ impl HttpServer { async fn instrumented_service_fn( self: Arc, + server_scheme: Scheme, client_addr: SocketAddr, request: Request, ) -> anyhow::Result> { @@ -291,7 +308,7 @@ impl HttpServer { body.map_err(wasmtime_wasi_http::hyper_response_error) .boxed() }), - Scheme::HTTP, + server_scheme, client_addr, ) .await; @@ -322,11 +339,21 @@ impl HttpServer { /// 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) -> anyhow::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 no `Host` header is +/// present or if it is not parsable as an `Authority`. +fn set_req_uri(req: &mut Request, scheme: Scheme) -> anyhow::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 host_header = headers + .get(http::header::HOST) + .context("missing 'Host' header")? + .to_str() + .context("'Host' header is not valid UTF-8")?; + let authority = host_header + .parse() + .context("'Host' header contains an invalid authority")?; parts.scheme = Some(scheme); parts.authority = Some(authority); *req.uri_mut() = Uri::from_parts(parts).unwrap(); From af04bc2a2a0685f5d34f7afbd22bc190e0e2c084 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 19 Aug 2024 11:20:04 +0200 Subject: [PATCH 2/3] Move uri munging inside handle_trigger_route Signed-off-by: Ryan Levick --- crates/trigger-http2/src/server.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/trigger-http2/src/server.rs b/crates/trigger-http2/src/server.rs index 1474c9ddd..72ffe9e0f 100644 --- a/crates/trigger-http2/src/server.rs +++ b/crates/trigger-http2/src/server.rs @@ -99,21 +99,23 @@ impl HttpServer { } /// Handles incoming requests using an HTTP executor. + /// + /// This method handles well known paths and routes requests to the handler when the router + /// matches the requests path. async fn handle( self: &Arc, mut req: Request, server_scheme: Scheme, client_addr: SocketAddr, ) -> anyhow::Result> { - set_req_uri(&mut req, server_scheme.clone())?; strip_forbidden_headers(&mut req); spin_telemetry::extract_trace_context(&req); - tracing::info!("Processing request on URI {}", req.uri()); - let path = req.uri().path().to_string(); + tracing::info!("Processing request on path '{path}'"); + // Handle well-known spin paths if let Some(well_known) = path.strip_prefix(spin_http::WELL_KNOWN_PREFIX) { return match well_known { @@ -135,13 +137,15 @@ impl HttpServer { } } + /// Handles a successful route match. pub async fn handle_trigger_route( self: &Arc, - req: Request, + mut req: Request, route_match: RouteMatch, server_scheme: Scheme, client_addr: SocketAddr, ) -> anyhow::Result> { + set_req_uri(&mut req, server_scheme.clone())?; let app_id = self .trigger_app .app() From fdec60b9fdd2cc98b6e6cc1fe4ff3aec643b277c Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 19 Aug 2024 16:41:25 +0200 Subject: [PATCH 3/3] PR Feedback Signed-off-by: Ryan Levick --- crates/factor-outbound-http/src/lib.rs | 12 ++++++++++++ crates/trigger-http2/src/lib.rs | 9 ++++++--- crates/trigger-http2/src/server.rs | 19 ++++++++++--------- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/crates/factor-outbound-http/src/lib.rs b/crates/factor-outbound-http/src/lib.rs index 78d05848d..7db483bb0 100644 --- a/crates/factor-outbound-http/src/lib.rs +++ b/crates/factor-outbound-http/src/lib.rs @@ -3,6 +3,8 @@ mod wasi; pub mod wasi_2023_10_18; pub mod wasi_2023_11_10; +use std::net::SocketAddr; + use anyhow::Context; use http::{ uri::{Authority, Parts, PathAndQuery, Scheme}, @@ -101,6 +103,16 @@ pub struct SelfRequestOrigin { } impl SelfRequestOrigin { + pub fn create(scheme: Scheme, addr: &SocketAddr) -> anyhow::Result { + Ok(SelfRequestOrigin { + scheme, + authority: addr + .to_string() + .parse() + .with_context(|| format!("address '{addr}' is not a valid authority"))?, + }) + } + pub fn from_uri(uri: &Uri) -> anyhow::Result { Ok(Self { scheme: uri.scheme().context("URI missing scheme")?.clone(), diff --git a/crates/trigger-http2/src/lib.rs b/crates/trigger-http2/src/lib.rs index d36b70d6b..abb731982 100644 --- a/crates/trigger-http2/src/lib.rs +++ b/crates/trigger-http2/src/lib.rs @@ -68,7 +68,10 @@ pub(crate) type InstanceState = (); /// The Spin HTTP trigger. pub struct HttpTrigger { /// The address the server will listen on. - addr_to_bind: SocketAddr, + /// + /// Note that this might not be the actual socket address that ends up being bound to. + /// If the port is set to 0, the actual address will be determined by the OS. + listen_addr: SocketAddr, tls_config: Option, router: Router, // Component ID -> component trigger config @@ -113,7 +116,7 @@ impl Trigger for HttpTrigger { ); Ok(Self { - addr_to_bind: cli_args.address, + listen_addr: cli_args.address, tls_config: cli_args.into_tls_config(), router, component_trigger_configs, @@ -122,7 +125,7 @@ impl Trigger for HttpTrigger { async fn run(self, trigger_app: TriggerApp) -> anyhow::Result<()> { let Self { - addr_to_bind: listen_addr, + listen_addr, tls_config, router, component_trigger_configs, diff --git a/crates/trigger-http2/src/server.rs b/crates/trigger-http2/src/server.rs index 72ffe9e0f..396a1870b 100644 --- a/crates/trigger-http2/src/server.rs +++ b/crates/trigger-http2/src/server.rs @@ -164,15 +164,7 @@ impl HttpServer { let mut instance_builder = self.trigger_app.prepare(component_id)?; // Set up outbound HTTP request origin and service chaining - let origin = SelfRequestOrigin { - scheme: server_scheme, - authority: self.listen_addr.to_string().parse().with_context(|| { - format!( - "server address '{}' is not a valid authority", - self.listen_addr - ) - })?, - }; + let origin = SelfRequestOrigin::create(server_scheme, &self.listen_addr)?; instance_builder .factor_builders() .outbound_http() @@ -358,6 +350,15 @@ fn set_req_uri(req: &mut Request, scheme: Scheme) -> anyhow::Result<()> { let authority = host_header .parse() .context("'Host' header contains an invalid authority")?; + // Ensure that if `req.authority` is set, it matches what was in the `Host` header + // https://github.com/hyperium/hyper/issues/1612 + if let Some(a) = parts.authority.as_ref() { + if a != &authority { + return Err(anyhow::anyhow!( + "authority in 'Host' header does not match authority in URI" + )); + } + } parts.scheme = Some(scheme); parts.authority = Some(authority); *req.uri_mut() = Uri::from_parts(parts).unwrap();