wasmtime / issue #8748 wasi-http: Allow setting TLS root ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #8748 wasi-http: Allow setting TLS root ...

Wasmtime GitHub notifications bot (Jun 05 2024 at 19:58):

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).

Wasmtime GitHub notifications bot (Jun 05 2024 at 19:59):

lann edited issue #8748:

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

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:53):

bacongobbler commented on issue #8748:

Do you want the ability to add a few TLS certs 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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:54):

bacongobbler edited a comment on issue #8748:

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:55):

bacongobbler commented on issue #8748:

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

Assuming we're going the route of "replace", would it be better if the caller provided its own RootCertStore? If not present, then we initialize a root_cert_store with the webpki-roots set?

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:56):

bacongobbler edited a comment on issue #8748:

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? If not present, then we initialize a root_cert_store with the webpki-roots set?

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:56):

bacongobbler edited a comment on issue #8748:

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 wasmtime? If not present, then we initialize a root_cert_store with the webpki-roots set?

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:56):

bacongobbler edited a comment on issue #8748:

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 not present, then we initialize a root_cert_store with the webpki-roots set?

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:57):

lann commented on issue #8748:

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

Wasmtime GitHub notifications bot (Jun 11 2024 at 21:57):

bacongobbler edited a comment on issue #8748:

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 22:00):

bacongobbler commented on issue #8748:

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?

Wasmtime GitHub notifications bot (Jun 11 2024 at 22:13):

lann commented on issue #8748:

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.

Wasmtime GitHub notifications bot (Jun 16 2024 at 02:05):

rajatjindal commented on issue #8748:

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?

Wasmtime GitHub notifications bot (Jun 16 2024 at 02:14):

rajatjindal commented on issue #8748:

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?

Wasmtime GitHub notifications bot (Jun 17 2024 at 16:25):

alexcrichton commented on issue #8748:

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.

Wasmtime GitHub notifications bot (Jun 17 2024 at 17:00):

lann commented on issue #8748:

a different trait method

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

https://github.com/bytecodealliance/wasmtime/blob/0f48f939b9870036562ca02ff21253547a9f1a5c/crates/wasi-http/src/types.rs#L277-L369

I think its totally fine to say that client certs represents too much customization for this kind of common implementation and 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.

Wasmtime GitHub notifications bot (Jun 17 2024 at 17:01):

lann edited a comment on issue #8748:

a different trait method

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

https://github.com/bytecodealliance/wasmtime/blob/0f48f939b9870036562ca02ff21253547a9f1a5c/crates/wasi-http/src/types.rs#L277-L369

I think its totally fine to say that client certs represent too much customization for this kind of common implementation and 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.

Wasmtime GitHub notifications bot (Jun 17 2024 at 17:01):

lann edited a comment on issue #8748:

a different trait method

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

https://github.com/bytecodealliance/wasmtime/blob/0f48f939b9870036562ca02ff21253547a9f1a5c/crates/wasi-http/src/types.rs#L277-L369

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.

Wasmtime GitHub notifications bot (Jun 17 2024 at 17:14):

alexcrichton commented on issue #8748:

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)

Wasmtime GitHub notifications bot (Aug 02 2024 at 21:41):

lann commented on issue #8748:

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: https://github.com/bytecodealliance/wasmtime/blob/c8a5acd9831f4ec3780f34d01965b45ddf44a297/crates/wasi-http/src/types.rs#L355-L361

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

Wasmtime GitHub notifications bot (Aug 05 2024 at 21:43):

pchickey commented on issue #8748:

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.)

Last updated: Nov 22 2024 at 16:03 UTC