wasmtime / PR #10249 Initial implementation of Wasi-tls (... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10249 Initial implementation of Wasi-tls (...

Wasmtime GitHub notifications bot (Feb 19 2025 at 23:40):

jsturtevant opened PR #10249 from jsturtevant:wasi-tls-initial to bytecodealliance:main:

fixes: https://github.com/bytecodealliance/wasmtime/issues/10089

This adds a crate that provides the Wasmtime host implementation for the wasi-tls API.
The wasi-tls world allows WebAssembly modules to perform SSL/TLS operations,
such as establishing secure connections to servers.

This initially implements it with rustls which was already included in the repository.

There are a few items that still need to be resolved but are not specially related to the implementation but wanted to start the review to make sure things moving in the correct direction.

add a close-notify function to the wasi-tls wit in the proposal to properly close down the

resolve importing stable types into unstable features (https://github.com/bytecodealliance/wasm-tools/issues/1995). Write now I have a workaround on a branch that is being used in rust cargo patch.

Thanks goes out to @badeend and @dicej as much of this was based on their prior work.

Wasmtime GitHub notifications bot (Feb 19 2025 at 23:40):

jsturtevant requested pchickey for a review on PR #10249.

Wasmtime GitHub notifications bot (Feb 19 2025 at 23:40):

jsturtevant requested wasmtime-core-reviewers for a review on PR #10249.

Wasmtime GitHub notifications bot (Feb 19 2025 at 23:40):

jsturtevant requested wasmtime-default-reviewers for a review on PR #10249.

Wasmtime GitHub notifications bot (Feb 20 2025 at 00:54):

jsturtevant edited PR #10249:

fixes: https://github.com/bytecodealliance/wasmtime/issues/10089

This adds a crate that provides the Wasmtime host implementation for the wasi-tls API.
The wasi-tls world allows WebAssembly modules to perform SSL/TLS operations,
such as establishing secure connections to servers.

This initially implements it with rustls which was already included in the repository.

There are a few items that still need to be resolved but are not specially related to the implementation but wanted to start the review to make sure things moving in the correct direction.

add a close-notify function to the wasi-tls wit in the proposal to properly close down the https://github.com/WebAssembly/wasi-tls/pull/5

resolve importing stable types into unstable features (https://github.com/bytecodealliance/wasm-tools/issues/1995). Write now I have a workaround on a branch that is being used in rust cargo patch.

Thanks goes out to @badeend and @dicej as much of this was based on their prior work.

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend commented on PR #10249:

Looking great!

CI fails because Clippy being Clippy, and I suspect the build fails because there's some missing conditional compilation.

Aside from that, I;ve left behind some remarks:

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend created PR review comment:

Looks like left-over test code

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend created PR review comment:

Regarding this impl AsyncWrite for TcpStreams block and the impl AsyncRead for TcpStreams block below: I'm fine with them existing here, but AFAIK these have nothing to do with TLS right? If so, we could move them into the wasi-io subcrate at some point.

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend created PR review comment:

Might be nice to rename references to the term "TCP" within this file to something more generic. Since this TLS implementation doesn't actually depend on TCP, and the inner streams may come from anywhere.

Wasmtime GitHub notifications bot (Feb 20 2025 at 09:10):

badeend created PR review comment:

I don't think this is cancel-safe. If the .await is canceled, the streamState will be left behind in the Closed state forever. Cancellation may happen when waiting for multiple Pollables at once.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:15):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:15):

jsturtevant created PR review comment:

The reason for this being the wit file directly is https://github.com/bytecodealliance/wasm-tools/issues/1996#issuecomment-2640748051

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:16):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:16):

jsturtevant created PR review comment:

this branch contains the temp fix for https://github.com/bytecodealliance/wasm-tools/issues/1995

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:31):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:31):

jsturtevant created PR review comment:

correct, not specific to TLS it just enables passing wasi-io streams to libraries that require those traits (i.e. in this case the connect() method for rustls)

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:41):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 17:41):

jsturtevant created PR review comment:

Will take a look, is there an example of how to test that?

Wasmtime GitHub notifications bot (Feb 20 2025 at 18:02):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 18:02):

badeend created PR review comment:

To reproduce, you could try adjusting the timeout of blocking_finish in crates/test-programs/src/tls.rs to something non-0, but very small, so that both pollables are polled at least once, but the monotonic_clock pollable will finish first. After that, I suspect that the ClientHandshake::finish pollable will never resolve.

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:42):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:43):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:43):

jsturtevant created PR review comment:

updated

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:59):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 20 2025 at 22:59):

jsturtevant created PR review comment:

you are indeed correct.

Wasmtime GitHub notifications bot (Feb 21 2025 at 01:16):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Feb 21 2025 at 01:17):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2025 at 01:17):

jsturtevant created PR review comment:

I've pushed an update for this, I tested it locally but working getting a test written for this scenario

Wasmtime GitHub notifications bot (Feb 21 2025 at 01:18):

jsturtevant edited PR review comment.

Wasmtime GitHub notifications bot (Feb 21 2025 at 21:06):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Feb 21 2025 at 21:17):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Feb 21 2025 at 21:44):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Feb 21 2025 at 21:44):

jsturtevant created PR review comment:

added test that covers this in https://github.com/bytecodealliance/wasmtime/pull/10249/commits/f13938ee922ef11f1893957d77465d46ae442c14

Wasmtime GitHub notifications bot (Feb 24 2025 at 22:09):

alexcrichton submitted PR review:

This looks great to me :+1:

In terms of testing, would it be possible to also add a test (or multiple) that deal with the server-side as well. Ideally there'd either be a pre-generated cert check-in to the repo that they use or something generated at test-time perhaps per-test. That might help enable removing the dependency on example.com as well which has been flaky in the past, and while it's not the end of the world to have some flakiness it'd be best to insulate ourselves and have all the networking be purely local. Although I realize it's also good to test "is the system cert store actually used?" which necessarily requires external communication basially.

It might also be interesting, if local certs work, to test a variety of configurations related to invalid certificates to ensure that they do indeed all return an error instead of succeeding the authentication and such.

Wasmtime GitHub notifications bot (Feb 24 2025 at 22:09):

alexcrichton created PR review comment:

For this I might recommend adding a commit with prtest:full somewhere in the commit message to ensure that this can get past CI. I vaguely recall that rustls and/or ring and/or something doesn't compile on riscv64/s390x, hence the conditional here. That'll make the integration of wasmtime-wasi-tls into the CLI a bit hairier if so, but probably still workable.

Wasmtime GitHub notifications bot (Feb 25 2025 at 18:51):

jsturtevant commented on PR #10249:

In terms of testing, would it be possible to also add a test (or multiple) that deal with the server-side as well.

Just to clarify, you mean to implement a server similar to what is done on wasi-http but with certs? We are going to need this for more advance scenarios later so works for me. I'll get started on it here

Wasmtime GitHub notifications bot (Feb 25 2025 at 18:57):

alexcrichton commented on PR #10249:

That's one route yeah, another being something similar to what wasi-sockets does where the client/server are both purely in-wasm and doens't require any external integration. The latter is a bit nicer from a "share this test suite in more places" perspective as well, but regardless I think either way is fine in the long run

Wasmtime GitHub notifications bot (Feb 25 2025 at 22:03):

jsturtevant commented on PR #10249:

sounds good. We didn't add the server side parts to the wasi-tls spec yet but it shouldn't be to hard to do a minimal version as well now that the client side is done. @badeend wdyt?

Wasmtime GitHub notifications bot (Feb 25 2025 at 22:05):

alexcrichton commented on PR #10249:

Oh I didn't look too closely at the tls bits here but if the server parts aren't implemented yet I don't mean to add more work here, so I think it's totally reasonable to land without that and plan to add more tests later too.

Wasmtime GitHub notifications bot (Feb 26 2025 at 16:24):

badeend commented on PR #10249:

I'd prefer not to block this PR on the creation of server TLS connections from within a WASI guest. Creating server TLS connections brings along additional design complexity surrounding the handling of certificates & private keys from within a WASI guest.

If the goal is to reduce CI flakiness, we could also spin up a local TCP/TLS listener socket outside the guest, just before calling the test, and then connect to that from within the guest test.

Wasmtime GitHub notifications bot (Feb 26 2025 at 19:58):

alexcrichton commented on PR #10249:

I think let's focus on getting this initial set landed in-tree for now (aka landing the wasm-tools fixes and integrating that here). This'll be a bit flaky without a local server but that can be a follow-up item to do after landing.

Last updated: Feb 28 2025 at 02:27 UTC