rvolosatovs opened PR #12174 from rvolosatovs:feat/wasip3-tls to bytecodealliance:main:
This is a draft implementation, which will be developed alongside the spec PR (will open shortly and add ref)
refs #12102
rvolosatovs edited PR #12174:
This is a draft implementation, which will be developed alongside the spec PR (https://github.com/WebAssembly/wasi-tls/pull/17)
refs #12102
rvolosatovs updated PR #12174.
rvolosatovs updated PR #12174.
rvolosatovs updated PR #12174.
rvolosatovs has marked PR #12174 as ready for review.
rvolosatovs requested wasmtime-wasi-reviewers for a review on PR #12174.
rvolosatovs requested wasmtime-core-reviewers for a review on PR #12174.
rvolosatovs requested cfallin for a review on PR #12174.
rvolosatovs requested wasmtime-default-reviewers for a review on PR #12174.
rvolosatovs updated PR #12174.
rvolosatovs edited PR #12174:
This is the implementation of current p3 draft https://github.com/WebAssembly/wasi-tls/pull/17
refs #12102
rvolosatovs updated PR #12174.
rvolosatovs updated PR #12174.
badeend submitted PR review.
badeend created PR review comment:
Do you want me to cut a new release?
badeend created PR review comment:
The P2 bindings are built on top of the
TlsProvider/TlsTransport/TlsStreamabstractions defined incrates/wasi-tls/src/lib.rs. Those abstractions make the bindings agnostic to the actual underlying TLS implementation. Currently, wasmtime has three backends for the user to choose from:
rustls(the default of thiswasmtime-wasi-tlscrate)wasmtime-wasi-tls-nativetlswasmtime-wasi-tls-opensslThe P3 bindings in this PR bypass the existing abstractions and target
rustlsdirectly.Do you think it is possible to make the P3 bindings backend-agnostic as well? Doesn't necessarily have to be part of this PR right now
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
That would be awesome!
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
I would rather start with the simplest approach using the library directly. It should definitely be possible to abstract the implementation here, however there might be some challenges here, as we can't quite directly use Tokio I/O abstractions here due to the lack of (CM) cancellation support in Tokio.
Let's figure this out in a follow-up?
rvolosatovs edited PR review comment.
rvolosatovs edited PR review comment.
rvolosatovs edited PR review comment.
badeend submitted PR review.
badeend created PR review comment:
Sure :+1:
badeend submitted PR review.
badeend created PR review comment:
v0.2.0-draft+6781ae2
rvolosatovs updated PR #12174.
rvolosatovs requested badeend for a review on PR #12174.
alexcrichton created PR review comment:
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 oftests/all/cli_tests.rswhere it bottoms out inwasmtime 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
alexcrichton created PR review comment:
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 onResource<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.
alexcrichton submitted PR review:
I'm finding
host/mod.rspretty gnarly here especially withWakerlogic 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?
badeend submitted PR review.
badeend created PR review comment:
The WIT file mentions:
Closing the
cleartextstream will cause aclose_notifypacket 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_notifysomewhere
badeend created PR review comment:
I notice
process_new_packetsappear 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 termprocess_new_packetsappears only once in the entire crate, right after the call toread_tls.I think only
CiphertextConsumerhas to useprocess_new_packets, and the other ones can get by without:
- The returned
statevariable is used as a heuristic for the capacity of.as_direct(..). A fixed size could work too, optionally bounded bydst.remaining().peer_has_closedis also surfaced from the regularreadcall, when it returns Ok(0)
badeend created PR review comment:
The
wasi-tlscreate already has a bunch of these types incrates/wasi-tls/src/lib.rs. E.g.WasiTls<'a>,WasiTlsCtx,WasiTlsCtxBuilder. Can the P3 implementation make use of those existing ones?
badeend created PR review comment:
connectshould 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.
badeend created PR review comment:
The error returned by
process_new_packetsonly affects theread_tlsside. 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.
rvolosatovs updated PR #12174.
rvolosatovs updated PR #12174.
rvolosatovs submitted PR review.
rvolosatovs created PR review comment:
Eventually, these should be unified. The current approach is consistent with all other p3 interface implementations and we do not know yet what will be necessary to implement https://github.com/bytecodealliance/wasmtime/pull/12174#discussion_r2823885681
badeend submitted PR review:
I've given
mod.rs&client.rstwo review passes by now.Once the remaining comments are resolved, I think it mostly checks out albeit still being quite verbose with all the state-machine'y & waker stuff. Don't know how much time we should spend polishing this right now, because if we want to support other backends (see comment above) much of this will need to be revisited anyway.
I'll leave it up for you to decide.
badeend created PR review comment:
In its new form
connectawaits the handshake, but doesn't report the status.
Failures during the handshake phase are reported as "success" and only surfaced later on on its streams.
Maybe the easiest way to validate the intended behavior is to updatetest_tls_invalid_certificateto _only_ rely on the result returned byconnectand don't attempt any I/O on the streams/futures afterwards
badeend created PR review comment:
Is it intentional for
_send_err_txto be dropped here?
badeend submitted PR review:
@alexcrichton I've given
mod.rs&client.rstwo review passes by now.Once the remaining comments are resolved, I think it mostly checks out albeit still being quite verbose with all the state-machine'y & waker stuff. Don't know how much time we should spend polishing this right now, because if we want to support other backends (see comment above) much of this will need to be revisited anyway.
I'll leave it up for you to decide.
rvolosatovs submitted PR review:
I was intending to follow-up on this later in the week, but as I received another review from @badeend I'll just go ahead and write a reply now:
First, for context, the implementation was done for the original version of the interface I proposed in https://github.com/WebAssembly/wasi-tls/pull/17/changes/91f93ceffacbe977a3c2cb3ce7ef1fdf6c6716ee - it was later hastily adapted to match the OOP-centric design that was suggested in https://github.com/WebAssembly/wasi-tls/pull/17#issuecomment-3686095920
The intention of this PR was only to build a PoC - that has now been achieved.
In it's current state the implementation is very brittle and PoC quality at best. That said, I struggle to see a way to implement this "nicely" given the APIs we have today.
Short-term I will not have either the time or incentive to fully address feedback on this PR and/or maintain this code. I don't think I will work on this PR any time soon (if ever).
I would not like to stall progress on this, therefore, I see a few options:
- Merge and take care of the rest in follow-ups
- I close this PR and someone (@badeend ?) opens a different one either based on this implementation or a completely fresh one
- Someone (@badeend ?) takes over this PR and gets it over the line
rvolosatovs created PR review comment:
Indeed. I would also prefer
connectto return an error on connection failures - the only "simple" way of doing that with the existing API I can come up with would be changinghandshakeoneshot element value to be aResult, would that address your concern?
connectand don't attempt any I/O on the streams/futures afterwardsIn current design the guest drives the I/O, so
receivestream must be polled forconnectto be able to do work. With the existing Wasmtime API, the only way for the host to consume data from aStreamReaderis topipeit into a consumer implementation, that's whatreceiveandsenddo now - they "register" a consumer. There is no way to deregister a consumer at a later point and there is an additional hazard in that dropping an unpipedStreamReaderwould leak it. So even if the relevantStreamReaders would be plumbed through toconnectimplementation and proper bookkeeping would be implemented indrop, we would not be able to directly read bytes from the stream inconnect.The only other option then is for consumer itself to spawn a (Tokio) task - given wasip3 implementation direction and design so far, I believe we want to avoid having to do that unless absolutely necessary. In my personal opinion if we cannot use Rust
asyncmachinery directly in host implementation without relying on Tokio tasks and message-passing, it suggests that we should probably revisit the Wasmtime's async support APIs to fix that. I don't think we should be reimplementing https://github.com/bytecodealliance/wasmtime/blob/d42d0b6df22d8d7be21212899a9cae21767c6992/crates/wasi/src/p2/write_stream.rs in wasip3
rvolosatovs created PR review comment:
There is no scenario, when an error could be sent on the future returned by
send, so yes.
rvolosatovs created PR review comment:
That is what I originally had, but have eventually removed and brought back. The challenge is that
send_close_notifycan only be called after handshake has finished. I made sure that's the case by carefully arranging theoneshotsends fromconnect
rvolosatovs submitted PR review:
I was intending to follow-up on this later in the week, but as I received another review from @badeend I'll just go ahead and write a reply now:
First, for context, the implementation was done for the original version of the interface I proposed in https://github.com/WebAssembly/wasi-tls/pull/17/changes/91f93ceffacbe977a3c2cb3ce7ef1fdf6c6716ee - it was later hastily adapted to match the OOP-centric design that was suggested in https://github.com/WebAssembly/wasi-tls/pull/17#issuecomment-3686095920
The intention of this PR was only to build a PoC - that has now been achieved.
In it's current state the implementation is very brittle and PoC quality at best. That said, I struggle to see a way to implement this "nicely" given the APIs we have today. I do believe that the original interface version I proposed in https://github.com/WebAssembly/wasi-tls/commit/91f93ceffacbe977a3c2cb3ce7ef1fdf6c6716ee would allow for a much nicer and simpler host implementation.
Short-term I will not have either the time or incentive to fully address feedback on this PR and/or maintain this code. I don't think I will work on this PR any time soon (if ever).
I would not like to stall progress on this, therefore, I see a few options:
- Merge and take care of the rest in follow-ups
- I close this PR and someone (@badeend ?) opens a different one either based on this implementation or a completely fresh one
- Someone (@badeend ?) takes over this PR and gets it over the line
alexcrichton commented on PR #12174:
I'm not familiar with the current design of wasi-tls much less the async-aware wasip3 version so I can only comment at a sort of high-level as opposed to offering thoughts on specifics. From that perspective I'd personally lean towards leaving this as a PR to avoid having too much mostly-unmaintained code in-tree. It sounds like @badeend you'd ideally see this built on the preexisting abstractions and @rvolosatovs you don't have the time/motivation right now to pursue such a refactoring. I'd personally prefer to avoid an interim-state landed in Wasmtime where there are no plans for improvement and no one's quite satisfied with the current implementation.
Does that sound reasonable enough to leave this as a PR for now, and revisit once time/motivation/energy have been unblocked?
Last updated: Mar 23 2026 at 16:19 UTC