Skip to content

Comments

feat(p3): implement wasi:tls#12174

Open
rvolosatovs wants to merge 5 commits intobytecodealliance:mainfrom
rvolosatovs:feat/wasip3-tls
Open

feat(p3): implement wasi:tls#12174
rvolosatovs wants to merge 5 commits intobytecodealliance:mainfrom
rvolosatovs:feat/wasip3-tls

Conversation

@rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Dec 16, 2025

This is the implementation of current p3 draft WebAssembly/wasi-tls#17

refs #12102

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs force-pushed the feat/wasip3-tls branch 2 times, most recently from 4c9f3b8 to 187b386 Compare February 17, 2026 12:11
@rvolosatovs rvolosatovs marked this pull request as ready for review February 17, 2026 12:15
@rvolosatovs rvolosatovs requested review from a team as code owners February 17, 2026 12:15
@rvolosatovs rvolosatovs requested review from cfallin and removed request for a team February 17, 2026 12:15
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs requested a review from badeend February 19, 2026 16:15
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding host/mod.rs pretty gnarly here especially with Waker logic and such to the point that I'd have to dig more into what rustls is offering here to double-check all the logic. That being said I'd also be fine to defer to @badeend in terms of review on that.

I'll note though that this is a pretty beefy implementation with relatively light testing. Would it be possible to enhance the tests here or does the current test basically not pass unless all the bits and bobs are present?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that this is duplicating the p2 tests already present, but given that this is basically modeled as "run the thing" that could also be modeled as a p3_cli_* test run as part of tests/all/cli_tests.rs where it bottoms out in wasmtime run -S... foo.wasm.

I might recommend moving more in that direction than having librarified test here to make it a bit more uniform to run tests

mod client;
mod types;

macro_rules! mk_push {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this was copied from elsewhere, but personally I'd say that these macros are probably overkill given the that they're mostly one-liners around table.$method(thing) and the type annotations on Resource<T> is typically enough to guide everything type-inference wide. The benefit of these macros would be the extra error context information, but given how rarely these will all be triggered I'm not sure it's worth the complexity.

))
}

async fn connect<T>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connect should perform the handshake and not return before that has succeeded or failed. But in its current form this method doesn't do any I/O.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WIT file mentions:

Closing the cleartext stream will cause a close_notify packet to be emitted on the returned output stream.

I don't see where graceful shutdown is handled in the current implementation. I would expect a call to send_close_notify somewhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wasi-tls create already has a bunch of these types in crates/wasi-tls/src/lib.rs. E.g. WasiTls<'a>, WasiTlsCtx, WasiTlsCtxBuilder. Can the P3 implementation make use of those existing ones?

return Poll::Pending;
}

let state = match conn.process_new_packets() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice process_new_packets appear multiple times in this file.

The rustls documentation mentions that this method should only be called after a successful call to Connection::read_tls. This lines up with how e.g. tokio-rustls does it, where the term process_new_packets appears only once in the entire crate, right after the call to read_tls.

I think only CiphertextConsumer has to use process_new_packets, and the other ones can get by without:

  • The returned state variable is used as a heuristic for the capacity of .as_direct(..). A fixed size could work too, optionally bounded by dst.remaining().
  • peer_has_closed is also surfaced from the regular read call, when it returns Ok(0)

Comment on lines +231 to +233
ciphertext_producer.take().map(Waker::wake);
plaintext_consumer.take().map(Waker::wake);
plaintext_producer.take().map(Waker::wake);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error returned by process_new_packets only affects the read_tls side. The other directions may still continue to work. From rustls docs:

After an error is received from process_new_packets, you should not call read_tls any more (it will fill up buffers to no purpose). However, you may call the other methods on the connection, including write, send_close_notify, and write_tls. Most likely you will want to call write_tls to send any alerts queued by the error and then close the underlying connection.

So I don't know if waking everybody up is the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants