[Factors] Change how incoming-request.authority is set.
#2723
Conversation
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 <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
crates/trigger-http2/src/lib.rs
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
I wouldn't change this; the CLI flag is --listen...
There was a problem hiding this comment.
I hear ya. I wanted to draw attention to the fact that this might not be the actual SocketAddr the socket is listening on (in the case of a 0 port), but perhaps that's not worth the potential confusion in the other direction.
There was a problem hiding this comment.
By the way, the CLI arg is actually --address... But I'll still change it back.
There was a problem hiding this comment.
Unfortunately it is a trick:
#[clap(long = "listen", ...)]
pub address: SocketAddr,| fn set_req_uri(req: &mut Request<Body>, 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`. |
There was a problem hiding this comment.
Based on hyperium/hyper#1612 I believe this should also consider the req.uri().authority(), verifying that they are identical if both are present.
| client_addr: SocketAddr, | ||
| ) -> anyhow::Result<Response<Body>> { | ||
| set_req_uri(&mut req, scheme, self.listen_addr)?; |
There was a problem hiding this comment.
Could this be done at the top of instrumented_service_fn? The http_span! there emits the scheme for o11y. As a bonus it would avoid needing to thread the server_scheme param through here.
There was a problem hiding this comment.
Putting it in instrumented_service_fn moves the logic up the call stack where we need to move it down the call stack.
The call graph is: instrumented_service_fn -> handle -> handle_trigger_route. The handler for service chained calls calls handle_trigger_route which is why we moved set_req_uri to that function - the service chained request also needs its uri manipulated. If we were to move that logic up to instrumented_service_fn, the issue this change is trying to address would persist.
There was a problem hiding this comment.
Ah gotcha. Perhaps it would make sense to split the scheme and host updates? 🤔
Signed-off-by: Ryan Levick <[email protected]>
| 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 { |
There was a problem hiding this comment.
I wonder if we should be setting the SelfRequestOrigin unconditionally in this interceptor. It seems like self requests ought to work in service-chained handlers? 🤔
| 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" | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry, I wasn't very clear. I believe it is valid to have just the host header, just the uri authority, or both set, where in the "both" case they must match.
Egads this seems complicated: https://www.rfc-editor.org/rfc/rfc9113.html#section-8.3.1-2.3.3
The moral equivalent of #2684 for factors.
The second commit fixes a remaining issue:
incoming-request.authoritywon't otherwise be set in service chained requests. This is because authority was set a layer above where service chained requests hook into the http trigger stack. The setting ofauthorityhas been moved intoHttpServer::handle_trigger_routefrom theHttpServer::handlemethod. I believe this will work fine as the only thing thehandlefunction does in addition to callinghandle_trigger_routeis handling the well known routes, 404s, andextract_trace_contextall of which shouldn't need the scheme and authority.