factors: Update outbound networking #2737
Conversation
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
lann
force-pushed
the
factors-update-outbound-net
branch
from
8f96df9 to
4237a81
Compare
| let hosts_future = allowed_hosts_future.clone(); | ||
| let allowed_hosts = OutboundAllowedHosts { | ||
| allowed_hosts_future: allowed_hosts_future.clone(), | ||
| disallowed_host_callback: self.disallowed_host_callback, | ||
| }; | ||
| wasi_builder.outbound_socket_addr_check(move |addr, addr_use| { | ||
| let hosts_future = hosts_future.clone(); | ||
| let allowed_hosts = allowed_hosts.clone(); | ||
| async move { | ||
| match hosts_future.await { | ||
| Ok(allowed_hosts) => { | ||
| // TODO: validate against existing spin-core behavior | ||
| let scheme = match addr_use { | ||
| SocketAddrUse::TcpBind => return false, | ||
| SocketAddrUse::TcpConnect => "tcp", | ||
| SocketAddrUse::UdpBind | SocketAddrUse::UdpConnect | SocketAddrUse::UdpOutgoingDatagram => "udp", | ||
| }; | ||
| spin_outbound_networking::check_url(&addr.to_string(),scheme, &allowed_hosts) | ||
| } | ||
| Err(err) => { | ||
| // TODO: should this trap (somehow)? | ||
| tracing::error!(%err, "allowed_outbound_hosts variable resolution failed"); | ||
| false | ||
| } | ||
| } | ||
| // TODO: validate against existing spin-core behavior | ||
| let scheme = match addr_use { | ||
| SocketAddrUse::TcpBind => return false, | ||
| SocketAddrUse::TcpConnect => "tcp", | ||
| SocketAddrUse::UdpBind | SocketAddrUse::UdpConnect | SocketAddrUse::UdpOutgoingDatagram => "udp", | ||
| }; | ||
| allowed_hosts.check_url(&addr.to_string(), scheme).await.unwrap_or_else(|err| { | ||
| // TODO: should this trap (somehow)? | ||
| tracing::error!(%err, "allowed_outbound_hosts variable resolution failed"); | ||
| false | ||
| }) |
There was a problem hiding this comment.
This whole diff chunk is basically just switching from spin_outbound_networking::check_url to OutboundAllowedHosts::check_url.
| terminal::warn!("A component tried to make a request to non-allowed url '{url}'."); | ||
| let (scheme, host, port) = (url.scheme, url.host, url.port); | ||
| let msg = if let Some(port) = port { | ||
| format!("`allowed_outbound_hosts = [\"{scheme}://{host}:{port}\"]`") | ||
| } else { | ||
| format!("`allowed_outbound_hosts = [\"{scheme}://{host}:$PORT\"]` (where $PORT is the correct port number)") | ||
| }; | ||
| eprintln!("To allow requests, add {msg} to the manifest component section."); |
There was a problem hiding this comment.
This language is merged with the outbound HTTP warning language in disallowed_host_callback below. This is probably a bit of a regression around the "where $PORT is the correct port number" stuff, but I feel like we could probably do better there anyway.
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
| @@ -58,7 +57,6 @@ impl spin_factor_outbound_http::OutboundHttpInterceptor for OutboundHttpIntercep | |||
| let resp = HostFutureIncomingResponse::pending(wasmtime_wasi::runtime::spawn(resp_fut)); | |||
| InterceptOutcome::Complete(Ok(resp)) | |||
| } else { | |||
| request.extensions_mut().insert(self.origin.clone()); | |||
There was a problem hiding this comment.
@rylev It turns out this was a bad idea. WHO COULD HAVE GUESSED
| .await | ||
| .map_err(|_| ErrorCode::HttpRequestUriInvalid)?; | ||
| if !is_allowed { | ||
| return Ok(Err(ErrorCode::HttpRequestDenied)); |
There was a problem hiding this comment.
I think we're missing a call to outbound_allowed_hosts.report_disallowed_host here, no?
I see later on that report_disallowed_host gets called automatically in check_url. Doing it in the check_url call but not in allows_relative_url is confusing. I think we should encapsulate the call to report_disallowed_host in the same equivalent places for both relative and non-relative requests.
There was a problem hiding this comment.
I reworked this significantly; see the latest commit.
| let origin = request | ||
| .extensions() | ||
| .get::<SelfRequestOrigin>() | ||
| .cloned() |
| .map_err(|_| ErrorCode::HttpRequestUriInvalid)?; | ||
| if !allowed_hosts.allows(&outbound_url) { | ||
| return Ok(handle_not_allowed(request.uri(), false)); | ||
| } | ||
| } | ||
|
|
||
| if let Some(authority) = request.uri().authority() { |
There was a problem hiding this comment.
Nit: I know this was here before, but it seems strange we have two different blocks for when request.uri().authority() is Some instead of just combining them.
There was a problem hiding this comment.
Hmm...authority should always be set here. I'll change it to an error.
| disallowed_host_callback: Option<DisallowedHostCallback>, | ||
| } | ||
|
|
||
| pub type DisallowedHostCallback = fn(scheme: &str, authority: &str); |
There was a problem hiding this comment.
I know it's more typing, but should we only limit this to function pointers instead of any impl Fn(&str, &str)?
There was a problem hiding this comment.
Ugh but its so much more typing 😐
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Fell down this rabbit hole while implementing OutboundHttpFactor's fermyon:spin/http interface:
SelfRequestOriginto be a more straightforward InstanceState setter; the extension method didn't work well for the spin interface anyway