wasi-http: Allow setting TLS root certs for default_send_request

[lann]

Currently, the TLS roots are hard-coded to the webpki-roots set. This is a good default, but in some scenarios private roots are required. We should be able to add options to OutgoingRequestConfig to extend and/or replace that default set of roots with custom root(s).

Zulip context

[bacongobbler]

Do you want the ability to add a small list of certificates to the existing webpki-roots set, or add a list of system certificates? RootCertStore has two functions:

• add(): Add a single DER-encoded certificate to the store. This is suitable for a small set of root certificates that are expected to parse successfully.
• add_parsable_certificates(): Parse the given DER-encoded certificates and add all that can be parsed in a best-effort fashion.

The former would be good when adding a small number of certificates to the store, whereas the latter would be better when adding a large collection of root certificates.

I am assuming we'd probably want to use add to add only a few select certificates on top of the webpki-roots set, but thought I'd check to make sure.

[bacongobbler]

to extend and/or replace that default set of roots with custom root(s).

Assuming we're going the route of "extend and/or replace", would it be better if the caller provided its own RootCertStore to use in place of the store provided by the default_send_request_handler? If it isn't present in the OutgoingRequestConfig, then we can initialize a root_cert_store with the webpki-roots set.

[lann]

It looks like add is more general purpose so if default to that approach.

[bacongobbler]

Okay great. That solves the "extend" part of the equation.

Do we want to solve for replacement of the webpki-roots set? Would that be valuable to most callers of this function? Or would we just assume the caller should pass their own custom send_request_handler at that point?

[lann]

Maybe it can take an Option<rustls::RootCertStore> where None represents the current behavior. It isn't too hard to reconstruct the defaults and extend if that's what you need.

[rajatjindal]

I have been exploring what it will take to implement this. one question I have is how do folks expect the additional root ca config to be passed.

• should this be a reference to a file name that contains the additional custom root ca certs or
• should this be a string representation of the additional certs

The problem (with my limited understanding of how wasmtime works) with reference to a filename is that they may be different from the guest and host perspective. so I was thinking maybe passing a string representation of additional certs may work out better. but that would mean we are assuming that guest has access to the certs in the first place.

do you have any suggestions/established-patterns about something like this?

[rajatjindal]

one additional functionality i am exploring while trying this out is to support client cert auth. in addition to above question (file vs string representation of cert), another question I have is around structuring the types.

so far I was thinking:

• add following to the OutgoingRequestConfig

/// The custom root ca to add to root ca store
pub custom_root_ca: Option<String>,
/// The client auth configuration
pub client_cert_auth: Option<ClientCertAuth>,

• ClientCertAuth looks like following:

#[derive(Clone)]
/// Configuration for client cert auth.
pub struct ClientCertAuth {
    /// The auth cert chain to use for client-auth
    pub cert_chain: String,
    /// The private key to use for client-auth
    pub private_key: String,
}

• if custom_root_ca is provided, add this to the default cert root store
• if ClientCertAuth is provided, change the tls connector to use ClientConfig with with_client_auth_cert(cert_chain_der, private_key_der). The default behavior remains same as how it works today and uses with_no_client_auth.

does this sound like a reasonable approach?

[alexcrichton]

Given the complexity of TLS configuration and the litany of options/formats I might throw another possibility into the ring which would be to keep the rustls bits we have right now as-is and require further customization to go through a different trait method such as:

trait WasiHttpView {
    fn sender(&mut self, tls: bool, authority: &str, timeout: Option<Duration>) -> Result<SendRequest<HyperOutgoingBody>>;
    // ...
}

While more difficult to integrate with that would expose the ability to make custom TCP connections using any TLS library. Additionally it would enable configurations such as pooling in theory. We'd need to refactor the default_send_request function a bit to plumb this through to there though.

[lann]

a different trait method

That seems like it would probably account for roughly 50% of the existing default send impl:

wasmtime/crates/wasi-http/src/types.rs

Lines 277 to 369 in 0f48f93

	let tcp_stream = timeout(connect_timeout, TcpStream::connect(&authority))
	.await
	.map_err(|_| types::ErrorCode::ConnectionTimeout)?
	.map_err(|e| match e.kind() {
	std::io::ErrorKind::AddrNotAvailable => {
	dns_error("address not available".to_string(), 0)
	}
	_ => {
	if e.to_string()
	.starts_with("failed to lookup address information")
	{
	dns_error("address not available".to_string(), 0)
	} else {
	types::ErrorCode::ConnectionRefused
	}
	}
	})?;
	let (mut sender, worker) = if use_tls {
	#[cfg(any(target_arch = "riscv64", target_arch = "s390x"))]
	{
	return Err(crate::bindings::http::types::ErrorCode::InternalError(
	Some("unsupported architecture for SSL".to_string()),
	));
	}
	#[cfg(not(any(target_arch = "riscv64", target_arch = "s390x")))]
	{
	use rustls::pki_types::ServerName;
	// derived from https://github.com/rustls/rustls/blob/main/examples/src/bin/simpleclient.rs
	let root_cert_store = rustls::RootCertStore {
	roots: webpki_roots::TLS_SERVER_ROOTS.into(),
	};
	let config = rustls::ClientConfig::builder()
	.with_root_certificates(root_cert_store)
	.with_no_client_auth();
	let connector = tokio_rustls::TlsConnector::from(std::sync::Arc::new(config));
	let mut parts = authority.split(":");
	let host = parts.next().unwrap_or(&authority);
	let domain = ServerName::try_from(host)
	.map_err(|e| {
	tracing::warn!("dns lookup error: {e:?}");
	dns_error("invalid dns name".to_string(), 0)
	})?
	.to_owned();
	let stream = connector.connect(domain, tcp_stream).await.map_err(|e| {
	tracing::warn!("tls protocol error: {e:?}");
	types::ErrorCode::TlsProtocolError
	})?;
	let stream = TokioIo::new(stream);
	let (sender, conn) = timeout(
	connect_timeout,
	hyper::client::conn::http1::handshake(stream),
	)
	.await
	.map_err(|_| types::ErrorCode::ConnectionTimeout)?
	.map_err(hyper_request_error)?;
	let worker = wasmtime_wasi::runtime::spawn(async move {
	match conn.await {
	Ok(()) => {}
	// TODO: shouldn't throw away this error and ideally should
	// surface somewhere.
	Err(e) => tracing::warn!("dropping error {e}"),
	}
	});
	(sender, worker)
	}
	} else {
	let tcp_stream = TokioIo::new(tcp_stream);
	let (sender, conn) = timeout(
	connect_timeout,
	// TODO: we should plumb the builder through the http context, and use it here
	hyper::client::conn::http1::handshake(tcp_stream),
	)
	.await
	.map_err(|_| types::ErrorCode::ConnectionTimeout)?
	.map_err(hyper_request_error)?;
	let worker = wasmtime_wasi::runtime::spawn(async move {
	match conn.await {
	Ok(()) => {}
	// TODO: same as above, shouldn't throw this error away.
	Err(e) => tracing::warn!("dropping error {e}"),
	}
	});
	(sender, worker)
	};

I think its totally fine to say that client certs represent too much customization for this kind of common implementation. Given Spin's needs that originally motivated this issue I think we would be better off just forking the impl rather than introduce another trait method here.

[alexcrichton]

That's a good point yeah, and I agree with the conclusion that the best option here might be to copy what's currently done instead of having more hooks for customization (assuming that's ok for Spin of course)

[lann]

After finishing some refactoring around Spin's implementation of this, I'd like to consider adding a field to OutgoingRequestConfig, e.g. tls_config: Option<Arc<rustls::ClientConfig>>.

This would capture pretty much anything that a client would want to configure about TLS and should be easy to implement, basically just hooking in here:

wasmtime/crates/wasi-http/src/types.rs

Lines 355 to 361 in c8a5acd

	let root_cert_store = rustls::RootCertStore {
	roots: webpki_roots::TLS_SERVER_ROOTS.into(),
	};
	let config = rustls::ClientConfig::builder()
	.with_root_certificates(root_cert_store)
	.with_no_client_auth();
	let connector = tokio_rustls::TlsConnector::from(std::sync::Arc::new(config));

The main downside is that it would strongly couple the interface to rustls.

[pchickey]

Given that the implementation already implies rustls for the (overridable) default behavior, I would be in favor of adding a cargo feature for any tls support via rustls, and then allowing the use of that crate in public interfaces.

(Context: I just built an integration by overriding the default handler to proxy the plaintext to another part of the system which handles TLS, but rustls is in my cargo vets even though it is unreachable.)