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.

Wasmtime GitHub notifications bot (Feb 28 2025 at 22:18):

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 28 2025 at 22:18):

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

[x] 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 28 2025 at 22:30):

jsturtevant commented on PR #10249:

(aka landing the wasm-tools fixes and integrating that here).

I tried pulling the fix (https://github.com/bytecodealliance/wasm-tools/pull/2076) in from the main branch but looks like due to https://github.com/bytecodealliance/wasm-tools/pull/2073, there were a few missing cases that need to be added to wit-bindgen and here. Would it be helpful to open PR's to stub those out or is there work being done on them? No rush/pressure, just want to be helpful if I can.

Wasmtime GitHub notifications bot (Feb 28 2025 at 22:37):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Feb 28 2025 at 22:44):

alexcrichton commented on PR #10249:

Good point! I started the update a few days ago but didn't publish it -- https://github.com/bytecodealliance/wasmtime/pull/10314 -- I'll work on landing that once all the wasm-tools changes are in, but if you want to rebase on that it can give a preview of what things are likely to look like

Wasmtime GitHub notifications bot (Mar 01 2025 at 00:07):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 01 2025 at 00:27):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 01 2025 at 00:34):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 01 2025 at 00:48):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 01 2025 at 01:08):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Mar 01 2025 at 01:08):

jsturtevant created PR review comment:

made some updates to address this. but now getting register mapping is currently only implemented for x86_64 on some of the other platforms which I were working previously. I am not sure what this means yet, as I wouldn't have expected the changes to make a difference.

Wasmtime GitHub notifications bot (Mar 03 2025 at 20:56):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 03 2025 at 21:49):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 03 2025 at 22:17):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 03 2025 at 22:32):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Mar 03 2025 at 22:32):

jsturtevant created PR review comment:

Looked a bit more at this. It wasn't passing prior to my changes. I am not sure what causes this, if it is expected, or how to debug it.

@alexcrichton could you give me a pointer to what might be going wrong here?

Wasmtime GitHub notifications bot (Mar 04 2025 at 15:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 15:34):

alexcrichton created PR review comment:

Ah that's a bad error, but it bottoms out in config.debug_info(true); which isn't supported on Pulley. I'll go try and improve that error.

Wasmtime GitHub notifications bot (Mar 04 2025 at 19:59):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 04 2025 at 20:23):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Mar 04 2025 at 20:23):

jsturtevant created PR review comment:

Thanks! Removing that line fixed the issue.

Wasmtime GitHub notifications bot (Mar 04 2025 at 20:25):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 06 2025 at 22:28):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 06 2025 at 22:29):

jsturtevant submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2025 at 22:29):

jsturtevant created PR review comment:

fixed in https://github.com/bytecodealliance/wasm-tools/releases/tag/v1.227.0

Wasmtime GitHub notifications bot (Mar 06 2025 at 22:32):

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.

[x] 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

[x] 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 (Mar 07 2025 at 16:55):

jsturtevant updated PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 17:54):

jsturtevant commented on PR #10249:

@badeend @alexcrichton We've merged all the outstand issues and I've rebased to bring in all the changes. I think this is ready for a final review. Thanks!

Wasmtime GitHub notifications bot (Mar 07 2025 at 18:15):

alexcrichton updated PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 18:16):

alexcrichton submitted PR review:

I pushed a fix for the CI failure, and I double-checked and it looks like everything might actually work on s390x/riscv64 nowadays so I've updated to drop all the #[cfg] related to that. Otherwise looks good :+1:

Wasmtime GitHub notifications bot (Mar 07 2025 at 18:16):

alexcrichton has enabled auto merge for PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 18:38):

alexcrichton updated PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 19:10):

alexcrichton updated PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 19:10):

alexcrichton has enabled auto merge for PR #10249.

Wasmtime GitHub notifications bot (Mar 07 2025 at 19:50):

alexcrichton merged PR #10249.

Last updated: Apr 18 2025 at 04:04 UTC