-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add native-tls
backend for wasi-tls
#11064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… from the rest of the implementation. - `client.rs`: The rustls parts. - `io.rs`: WASI I/O conversion utility types. - `host.rs`: The host types + impls. - `bindings.rs`: The generated bindings.
It used to perform a "half-close" after sending the HTTP request. This is a TLS1.3+ feature, though Rustls & OpenSSL already supported it for TLS1.2 and lower. Technically, that makes them non-spec compliant, but they chose to align with the semantics of the underlying TCP connection. I suspect the TLS1.3 spec was updated to match what was already happening in reality. Anyhow, SChannel does not support half-closed connections and so the `read` call after the `close_output+shutdown` failed. I've reordered the test to first perform the HTTP conversation and _then_ do the TLS+TCP teardown.
…implementation was copied from the TCP equivalent, which doesn't need flushing. The TLS implementations _do_ maintain an internal buffer, so the `flush` call need to be hooked up.
…nto native_tls # Conflicts: # Cargo.lock
This all looks great to me, thanks again for working on this! My only comment would be could this update CI checks to perform a build of the I'll file a separate PR to perform the vets as well. |
Is this the right place to do that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great finds in the bugs. LGTM me overall
crates/wasi-tls/src/providers/mod.rs
Outdated
} else if #[cfg(feature = "nativetls")] { | ||
pub use NativeTlsProvider as DefaultProvider; | ||
} else { | ||
compile_error!("At least one TLS provider must be enabled."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to have to set two feature flags to enable the feature or should we provide a default to enable the feature or should we provide a default?
Also a bit confused in the way to set this up. Do I need to configure both feature flags (wasi-tls
and nativetls
or rusttls
) and then also set the provider with wasi_tls_ctx: WasiTlsCtxBuilder::new().provider(provider).build(),
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustls
is part of the default
feature. So by default, DefaultProvider
will point to the rustls provider and there's no need to configure anything on the WasiTlsCtxBuilder
.
Similarly, if you build with --no-default-features --features nativetls
, then DefaultProvider
will point to the native_tls provider and there's again no need to configure anything on the WasiTlsCtxBuilder
.
The only reason to call WasiTlsCtxBuilder::provider
is when the crate has been built with multiple providers and you want to switch between them at run time. This is what happens in e.g. the test suite and in the CLI code.
I've added some docs to clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that works 👍
Hm ok so this is actually going to be kind of tricky. We test crates in CI with One option might be to have a feature such as That's not an ideal option though IMO as ideally we wouldn't test native-tls by default and we'd instead have a dedicated job for just testin native-tls, but with |
How about moving Edit: And set up a separate CI job to test Edit2: A downside of this approach is that the wasmtime CLI may not reference the |
I think what you're proposing is probably the best option for now. You're right that it means that the CLI won't be easily buildable with native-tls, but that seems like a reasonable state of affairs for the time being and we can address that later if need be |
…nto native_tls
Could you take a look at my latest changes? I've ran it with |
During the initial wasi-tls implementation discussion, the suggestion was floated to support both
rustls
andnative-tls
. The initial/current implementation only supportsrustls
. This PR addsnative-tls
as an alternative TLS backend.The default TLS provider remains
rustls
. Consumers can switch to thenative-tls
backend by enabling the corresponding feature flags and then use the newly createdWasiTlsCtxBuilder::provider
method. On the CLI, this is exposed as-S tls-provider=nativetls
.Adding this new backend immediately surfaced two bugs:
1. Improved compatibility of
test_tls_sample_application
The test used to perform a "half-close" after sending the HTTP request. This is a TLS1.3+ feature, though Rustls & OpenSSL already supported it for TLS1.2 and lower. Technically, that makes them non-spec compliant, but they chose to align with the semantics of the underlying TCP connection. I suspect the TLS1.3 spec was updated to match what was already happening in reality.
Anyhow, SChannel does not support half-closed connections and so the
read
call after theclose_output+shutdown
failed. I've reordered the test to first perform the HTTP conversation and then do the TLS+TCP teardown.2. Implement flushing on the AsyncWriteStream.
The AsyncWriteStream implementation was copied from the TCP equivalent, which doesn't need flushing. The TLS implementations do maintain an internal buffer, so the
flush
call needs to be hooked up. Without it, the test would hang after making the change mentioned above.Misc: Reorganized wasi-tls folder
To make life easier for myself, I've shuffled some code around. I've isolated the rustls-specific bits from the rest of the implementation. The folder structure now looks like:
providers/*
: The rustls & native-tls specific parts.bindings.rs
: The generated bindings.host.rs
: The host types + impls.io.rs
: WASI I/O conversion utility types.CC: @jsturtevant