Stream: git-wasmtime

Topic: wasmtime / issue #6732 Should wasi-crypto be removed from...

Wasmtime GitHub notifications bot (Jul 15 2023 at 01:12):

alexcrichton opened issue #6732:

I have personally run into a number of issues over the past few years dealing with wasi-crypto in-tree and I've gotten to the point that I would like to pose the question of whether we should remove it from this repository.

Support for wasi-crypto was added in early 2021 and it was updated later that year in October, but since then it has received no updates in-tree. There was an abandoned update from a year later which was followed up with a currently open PR which is blocked on vetting the dependency updates. In terms of vetting the dependency tree of wasi-crypto represents 1.5M lines of code to audit out of Wasmtime's total 5.5M lines to audit (minus v8). This is one of Wasmtime's heaviest dependencies at nearly 30% of the entire audit backlog (minus v8).

Currently wasi-crypto is behind both a compile-time Cargo feature gate (features = ["wasi-crypto"]) as well as a runtime feature gate (--wasi-modules=experimental-wasi-crypto). The wasi-crypto crate compiles for some CI platforms but does not compile for AArch64 platforms or MinGW for example. This is handled by our ci/run-tests.sh script explicitly skipping wasi-crypto tests.

I believe the original purpose for adding this to Wasmtime was to provide a testing ground for the APIs and enable users to more easily enable it and test with it. I'm not sure to what extent this has happened or to what degree the implementation here in Wasmtime has informed development of the upstream proposal. If others could help fill in info on this it'd be much appreciated.

One thing I have noticed poking around historically in wasi-crypto is that it noticeably contains an transmute of guest-owned memory into a &'static mut [u8] type. This does not appear to cause any issues today since it appears that function either always bottoms out in an error or it's stored into an Arc<Mutex<...>> but never read. Despite this though it at least personally worries me not only because it's very easy to get &'static mut T wrong but there are no comments indicating why this is done or what the purpose is (there are actually only 16 lines of comments in the 6kloc making up wasi-crypto and its hostcall implementation). Personally one concern I would have about this is that it seems much of the wasi-crypto code is largely un-reviewed and not necessarily up to the quality standards of the rest of Wasmtime.

Personally I'm also not actually sure if there are any maintainers of the wasi-crypto code in Wasmtime. I don't believe any of the typical active Wasmtime contributors consider themselves maintainers, but I would otherwise assume that @jedisct1 is the maintainer as he's been the source of the major contributions so far. I'm not sure if it's ever been made official though (not that we have a great place to officially document this), but @jedisct1 do you want to be the official maintainer of wasi-crypto in Wasmtime? I haven't been able to tell historical as I haven't heard back from you on other changes and the current issues with vetting haven't had much further discussion as well.

All that said, my current personal leaning is that these issues lead me to concluding we should remove wasi-crypto from the tree for now:

• So far there has not been an active maintainer for wasi-crypto. The wasi-crypto proposal had many updates in the past years but the support in Wasmtime has stagnated. It feels like the code was added to Wasmtime but then effectively left in largely the original state so I'm not sure how much testing/experience has been gained.
• Personally I have reservations about the code quality of the implementation (e.g. the transmute above and lack of comments in general). I have not personally tested the code myself though or tried to run programs.
• The wasi-crypto crate brings in a very large tree of dependencies (1.5M+ lines of code) to audit, review, and ideally maintain. This makes it I think one of the larger of the WASI proposals, especially relative to the amount of review we're giving it.

I also don't want to jump to any conclusions though. If an active maintainer comes on, we get active review of existing and new code, and the dependency tree is perhaps slimmed down then I think it's reasonable to continue to keep wasi-crypto in-tree. I still think there's a huge amount of value in having something built-in for testing, and if users have been actively evaluating Wasmtime's support for wasi-crypto to help development of the upstream proposal that would be great to know.

Wasmtime GitHub notifications bot (Jul 15 2023 at 14:59):

jedisct1 commented on issue #6732:

The wasi-crypto crate itself keeps being updated. The wasmtime integration, OTOH, is currently difficult to maintain and test.

Having the entire implementation outside of wasmtime could indeed be better, especially to iterate quickly.

wasmedge supports plugins, that can be updated independently from the runtime. This includes a plugin for wasi-crypto. Having the ability for wasmtime to do the same may be the solution.

Wasmtime GitHub notifications bot (Jul 15 2023 at 18:06):

jedisct1 commented on issue #6732:

wasi-crypto itself doesn't have a lot of dependencies, and there's nothing uncommon here. Unfortunately, there's no crypto in the Rust standard library.

Something we can do, though, is remove most of the Rust implementations, and replace them with calls to BoringSSL.

That wasi-crypto implementation would just be an interface to a well-known library that is easy to vet. And the number of indirect dependencies would be drastically reduced. Would that help to keep it as an option in wasmtime?

Wasmtime GitHub notifications bot (Jul 15 2023 at 20:38):

tschneidereit commented on issue #6732:

The wasi-crypto crate itself keeps being updated. The wasmtime integration, OTOH, is currently difficult to maintain and test.

Can you say more about what makes the integration difficult to maintain and test? Given that wasi-crypto is pretty self-contained, ISTM that those concerns should mostly be applicable to all APIs, so it'd be great to get more details on this.

Having the entire implementation outside of wasmtime could indeed be better, especially to iterate quickly.

wasmedge supports plugins, that can be updated independently from the runtime. This includes a plugin for wasi-crypto. Having the ability for wasmtime to do the same may be the solution.

Related to the above: how do you see a plugin API helping with maintaining the integration? Regardless of where the implementation lives, if it's hard to maintain, I'm not sure a plugin API would really change much about that.

Wasmtime GitHub notifications bot (Jul 16 2023 at 10:07):

jedisct1 commented on issue #6732:

Hi Till,

wasi-crypto is a larger API than other proposals, but it is simple to follow and implement, and is very stable. There have been no breaking changes since its original design, the design is still believed to be quite future proof, and it comes with extensive documentation to write compatible implementations and libraries.

These APIs provide significant security and efficiency improvements for service providers.

The Rust example implementation of the hostcalls, currently maintained along with the specification, has two parts:

1. A library, that provides a safe and consistent interface for the algorithms recommended in the specification. The public interface looks like a conventional Rust API, but has exactly the same structure as the wasi-crypto specification.
2. A glue layer, that wraps functions from the library into the API expected by wiggle so that the library is accessible from wasmtime.

The library is not meant to be only used in wasmtime. Nor even by applications compiled to webassembly. It can be used like any other Rust crate, in native code.
This is useful for multiple reasons:

• It makes développement easier and faster. All the native profiling, testing and debugging tools can be used.
• Portable applications written in Rust and willing to take advantage of wasi-crypto when compiled to WebAssembly can also used it when compiled to native code.
• This is also a good way to understand how the wasi-crypto API works, and experiment with it. No wasm file to compile and run, nothing wrapped inside functions and macros that can be hard to debug, just include it like any Rust dependency, and test it natively.

All the code examples in the specification use the idiomatic Rust interface from the library. Because it’s so much easier to understand, follow and test than references to WITX/WIT/WAI definitions. Yet, it’s a 1-1 map with the specification. The automatically generated rustdoc of that library is also a useful way to navigate the API.

When wasi-crypto support for wasmtime was initially proposed , both parts (the library and the wiggle glue) were in the same project.

That was extremely convenient. And pretty much necessary to work on a fairly large API.
If a change was made to the library, but not in the glue, there was instantaneously a compile error. If a function was not wrapped in the glue, there was instantaneously a “warning: unused function” warning. If the glue didn’t match the witx definitions, there was immediately a compile error.

As the API was designed and implemented, working on three things simultaneously was required (witx, library, glue). Having them all in the same project, checked on save by VSCode, gave quick feedback and ensured that everything was consistent.

wasmtime integration was also simple: there was a single wasi-crypto dependency to add, no other code to include. Very minimal changes to wasmtime and no submodules to add (the wasi-crypto glue had a function to export its own witx interface).

Then, it was requested to split the library and wasmtime glue into different crates, and add a copy of the specification only to access the witx definitions. This is what was done, and how optional support was merged to wasmtime.

That instantaneously made maintenance far more complicated. Every change to the API now has to be done in different places. We need to make sure that the submodules with the witx definition is up to date in wasmtime. Changes have to be made in different projects. There’s no instant feedback any more when developing and testing the changes.

It also requires working on a wasmtime fork, that has these changes. And until these changes are picked up upstream, it’s not possible to update the library. But because the library is meant to always be in sync with the specification, this is also blocking updates to the specification. And because wasmtime needs the specification as a submodules, there’s also kind of a cycle.

Contributing, and, more importantly, updating and maintaining support of WASI proposals in wasmtime currently feels way more complicated than other runtimes.

A plugin system would solve pretty much all these issues. Implementations and maintenance of WASI proposals would be way easier.

With a plugin system:

• Proposals can be implemented without having to maintain wasmtime forks with uncommitted changes. A standard wasmtime release is enough.
• Implementations can keep all the moving parts that being are working on simultaneously at the same place, in the same project.
• No additional dependencies are added to wasmtime, so there won’t be version conflicts.
• Plug-ins can have different policies than the runtimes they are used with.
• Plug-ins can have their own release cycle.
• Plug-ins can be written in languages different than the runtime.
• Less compile-time flags in wasmtime, no need for multiple pre-compiled binaries.
• Applications can choose to use an older version of the plugin with a newer version of wasmtime or the other way round, if only for compliance reasons.
• Ideally, plug-ins could be used by other runtimes, which would help a lot with adoption
• Applications can chooose between multiple implementations or compilation options of a plug-in. For a large API such as wasi-crypto, some users may want to compile only a subset of it, or enable site-specific extensions. Such flexibility wouldn’t be easy to add as compilation flags in wasmtime.

Wasmtime GitHub notifications bot (Jul 16 2023 at 10:22):

jedisct1 commented on issue #6732:

Wasmedge plugins: https://wasmedge.org/docs/contribute/plugin/intro/

They are shared libraries, distributed and developed independently from the runtime.

For example, that allows them to ship support for wasi-nn with multiple backends. Users still download a single runtime and the plugin they need.

In wasmtime, wasi-nn is limited to a single backend, and supporting other options would require more compilation flags, more dependencies, and more binaries to distribute.

Wasmtime GitHub notifications bot (Jul 16 2023 at 15:32):

abrown commented on issue #6732:

(I think that last statement is slightly incorrect: since 2021, wasi-nn has supported multiple backends--https://github.com/bytecodealliance/wasmtime/pull/3174 --but the other backends have just not been upstreamed for one reason or another. But I don't think me saying this should change the general thrust of what you're saying, which I'm sympathetic to: making it easier to "plugin" new host functionality would be quite useful).

Wasmtime GitHub notifications bot (Jul 17 2023 at 02:31):

tschneidereit commented on issue #6732:

Thank you for the very detailed explanation and context.

I have some questions below that I hope will help get closer to a shared understanding of the situation.

The library is not meant to be only used in wasmtime. Nor even by applications compiled to webassembly. It can be used like any other Rust crate, in native code.

Based on this, it sounds like you're saying that it's actively useful for wasi-crypto to be split into two projects, instead of a single code base living inside the wasmtime project?

All the code examples in the specification use the idiomatic Rust interface from the library. Because it’s so much easier to understand, follow and test than references to WITX/WIT/WAI definitions. Yet, it’s a 1-1 map with the specification. The automatically generated rustdoc of that library is also a useful way to navigate the API.

That sounds really great!

That instantaneously made maintenance far more complicated. Every change to the API now has to be done in different places. We need to make sure that the submodules with the witx definition is up to date in wasmtime. Changes have to be made in different projects. There’s no instant feedback any more when developing and testing the changes.

Given the above, I'm not sure I understand what you're saying here. IIUC, development of changes to the wasi-crypto implementation should be possible without ever touching wasmtime at all, right? If so, shouldn't the integration into wasmtime boil down to

1. update the submodule to the latest version of the wasi-crypto repository
2. regenerate the bindings with wiggle
3. make necessary changes to crates/wasi-crypto/src/lib.rs, if any

Am I missing any steps? If yes, please let me know. If not, can you clarify what about this process would be fundamentally simpler with a plugin system?

It also requires working on a wasmtime fork, that has these changes. And until these changes are picked up upstream, it’s not possible to update the library. But because the library is meant to always be in sync with the specification, this is also blocking updates to the specification. And because wasmtime needs the specification as a submodules, there’s also kind of a cycle.

One thing that seems to make this a bit weirder than it otherwise would be is that the library implementation lives in the spec repo. I'm not sure that's the best setup, because it means that the spec proposal kind of assumes that this Rust implementation is _the_ implementation, and all other implementations are kind of second-class citizens. Wouldn't it be better for the spec repo to only contain the specification itself, such that all implementations are equal, and people are free to implement the spec in languages such as C/C++, Go, Zig, etc?

Wasmtime would of course continue to use the Rust implementation, but it could just use it as a cargo dependency, and only use a submodule for the interface definitions. (And once the proposal is updated to wit, we can use wit-deps for that instead.

Wasmtime GitHub notifications bot (Jul 17 2023 at 14:27):

alexcrichton commented on issue #6732:

One thing I think worth pointing out is that while having a plugin system would probably be nice for Wasmtime this doesn't exist today and I'm not aware of anyone working on implementing it. We have an RFC process for anyone interested in proposing such a change, but in lieu of that I don't want to ignore the problems we're facing today in the issue description above. My guess is that a plugin system likely won't materialize for some time at the least and in the meantime I'd ideally like to resolve the maintenance issues before then.

Wasmtime GitHub notifications bot (Jul 17 2023 at 16:02):

alexcrichton commented on issue #6732:

I've added this as an agenda item for Wasmtime's next meeting in case there's more to talk about in the future too.

Wasmtime GitHub notifications bot (Jul 17 2023 at 18:41):

fitzgen commented on issue #6732:

FWIW, there is also https://sourcegraph.com/crates/rawbytes@v0.1.2/-/blob/src/lib.rs?L14-16 which is a dep of xoodyak which is a dep of wasi-crytpo. That function is fundamentally unsafe: it allows you to change a NonZeroU32 into zero or make an enum's discriminant value invalid, for example. That should probably be a RUSTSEC advisory. The crate's repo also no longer seems to exist, so it is unclear where to report this issue.

Wasmtime GitHub notifications bot (Jul 17 2023 at 18:45):

fitzgen commented on issue #6732:

While we don't support .so plugins at the moment, there is nothing stopping wasi-crypto from living in its own repo while still exposing its add_to_linker functionality. This allows anyone to depend on it and use it with Wasmtime without Wasmtime itself pulling it in and its dependency tree.

Wasmtime GitHub notifications bot (Jul 17 2023 at 18:47):

fitzgen edited a comment on issue #6732:

FWIW, there is also https://sourcegraph.com/crates/rawbytes@v0.1.2/-/blob/src/lib.rs?L14-16 which is a dep of xoodyak which is a dep of wasi-crytpo. That function is fundamentally unsafe: it allows you to change a NonZeroU32 into zero or make an enum's discriminant value invalid, for example. That should probably be a RUSTSEC advisory. The crate's repo also no longer seems to exist, so it is unclear where to report this issue.

Edit: luckily it doesn't seem like that unsoundness actually affects wasi-crypto or Wasmtime, and doesn't merit a Wasmtime CVE, but it is another example of the dep subtree not being up to Wasmtime's coding standards. That would never have passed review in Wasmtime.

Wasmtime GitHub notifications bot (Jul 20 2023 at 19:00):

alexcrichton commented on issue #6732:

This was discussed at today's Wasmtime meeting and I'm going to attempt to summarize the discussion and consensus here. If others have corrections please let me know!

Overall the consenus was that wasi-crypto should be removed from the Wasmtime repository at this time due to issues with the size of the dependency tree and additionally the quality of the implementation (see the issues mentioned in the above comments to dep/in-tree issues). According to our tiers of support documentation wasi-crypto is currently tier 3 which means that it can be removed at any time. Notably though wasi-crypto does not meet two key requirements of tier 3 any more which is the code quality and "does not hinder Wasmtime's development" requirements.

We additionally discussed the implications of plugin systems and such at great length which I won't attempt to summarize in their entirety here. The main conclusion though was that irrespective of any future plans or possibilities for a plugin system there's a real problem to address today. While a plugin system might change the calculus involved here it depends on specifics, of which at this time there are none.

It seemed like, however, @jedisct1 you unfortunately were not able to make the meeting. You clearly have a vested interest in wasi-crypto and the hindrance to Wasmtime's development is not of the utmost urgency, so the conclusion was to continue the discussion here for a bit as well. Pending other plans, however, the current plan would be to remove wasi-crypto from in-tree after August 5 which would mean that Wasmtime 12 would be the last release with wasi-crypto.

---

We didn't talk about this specifically in the meeting today but in my opinion Wasmtime still wants to support wasi-crypto. I believe that the requirements of tier 3 need to be reestablished before reintroducing an implementation, but if those are met I'm at least personally interested in seeing the proposal re-integrated into Wasmtime.

Wasmtime GitHub notifications bot (Jul 20 2023 at 20:51):

jedisct1 commented on issue #6732:

Agreed, it would be most effective to keep the maintenance of runtime and general extensions separate.

This would allow for runtimes not to be obligated to support code and APIs they're hesitant to commit to, and extensions could control their release cycles independently.

Recently, we encountered a situation where the rawbyte indirect dependency from the xoodyak crate, which no longer exists in current versions of the crate nor in the current wasi-crypto release, posed issues due to the asynchronous nature of updates in multiple projects. The fact that updates in multiple projects can’t be atomic can create confusion and potential incompatibilities.

In terms of Wasmtime support, we can merge the necessary 'glue' back into the Rust example implementation. This approach allows Rust projects to use the standard Wasmtime crate with add_to_linker() calls, as per existing documentation.

However, this approach may not be a perfect fit for everyone using libwasmtime or the CLI.

For libwasmtime, it could be viable for the wasi-crypto glue itself to export a function that receives a wasmtime_linker_t pointer and registers the crypto modules.

As for the CLI, the optimal situation would be a minimal CLI app that relies solely on the public releases of the wasmtime-cli crate. However, since wasmtime version 0.10.0, the use of that crate outside of the Wasmtime workspace doesn’t appear to be supported any longer, if it ever was meant to be the case.

This is not a major issue, as maintaining a Wasmtime fork and distributing binaries with enabled extensions are both feasible strategies.

Wasmtime GitHub notifications bot (Aug 07 2023 at 17:47):

alexcrichton commented on issue #6732:

Wasmtime 13 has branched, so I've posted the removal of wasi-crypto to https://github.com/bytecodealliance/wasmtime/pull/6816

Wasmtime GitHub notifications bot (Aug 07 2023 at 18:22):

jedisct1 commented on issue #6732:

Implementations from the spec repository have been moved to the https://github.com/wasm-crypto organization. Adapters for different runtimes and ABIs will now live there as well.

Wasmtime GitHub notifications bot (Aug 07 2023 at 18:22):

jedisct1 edited a comment on issue #6732:

Implementations from the spec repository have been moved to the wasm-crypto organization. Adapters for different runtimes and ABIs will now live there as well.

Wasmtime GitHub notifications bot (Aug 08 2023 at 16:41):

alexcrichton closed issue #6732:

I have personally run into a number of issues over the past few years dealing with wasi-crypto in-tree and I've gotten to the point that I would like to pose the question of whether we should remove it from this repository.

Support for wasi-crypto was added in early 2021 and it was updated later that year in October, but since then it has received no updates in-tree. There was an abandoned update from a year later which was followed up with a currently open PR which is blocked on vetting the dependency updates. In terms of vetting the dependency tree of wasi-crypto represents 1.5M lines of code to audit out of Wasmtime's total 5.5M lines to audit (minus v8). This is one of Wasmtime's heaviest dependencies at nearly 30% of the entire audit backlog (minus v8).

Currently wasi-crypto is behind both a compile-time Cargo feature gate (features = ["wasi-crypto"]) as well as a runtime feature gate (--wasi-modules=experimental-wasi-crypto). The wasi-crypto crate compiles for some CI platforms but does not compile for AArch64 platforms or MinGW for example. This is handled by our ci/run-tests.sh script explicitly skipping wasi-crypto tests.

I believe the original purpose for adding this to Wasmtime was to provide a testing ground for the APIs and enable users to more easily enable it and test with it. I'm not sure to what extent this has happened or to what degree the implementation here in Wasmtime has informed development of the upstream proposal. If others could help fill in info on this it'd be much appreciated.

One thing I have noticed poking around historically in wasi-crypto is that it noticeably contains an transmute of guest-owned memory into a &'static mut [u8] type. This does not appear to cause any issues today since it appears that function either always bottoms out in an error or it's stored into an Arc<Mutex<...>> but never read. Despite this though it at least personally worries me not only because it's very easy to get &'static mut T wrong but there are no comments indicating why this is done or what the purpose is (there are actually only 16 lines of comments in the 6kloc making up wasi-crypto and its hostcall implementation). Personally one concern I would have about this is that it seems much of the wasi-crypto code is largely un-reviewed and not necessarily up to the quality standards of the rest of Wasmtime.

Personally I'm also not actually sure if there are any maintainers of the wasi-crypto code in Wasmtime. I don't believe any of the typical active Wasmtime contributors consider themselves maintainers, but I would otherwise assume that @jedisct1 is the maintainer as he's been the source of the major contributions so far. I'm not sure if it's ever been made official though (not that we have a great place to officially document this), but @jedisct1 do you want to be the official maintainer of wasi-crypto in Wasmtime? I haven't been able to tell historical as I haven't heard back from you on other changes and the current issues with vetting haven't had much further discussion as well.

All that said, my current personal leaning is that these issues lead me to concluding we should remove wasi-crypto from the tree for now:

• So far there has not been an active maintainer for wasi-crypto. The wasi-crypto proposal had many updates in the past years but the support in Wasmtime has stagnated. It feels like the code was added to Wasmtime but then effectively left in largely the original state so I'm not sure how much testing/experience has been gained.
• Personally I have reservations about the code quality of the implementation (e.g. the transmute above and lack of comments in general). I have not personally tested the code myself though or tried to run programs.
• The wasi-crypto crate brings in a very large tree of dependencies (1.5M+ lines of code) to audit, review, and ideally maintain. This makes it I think one of the larger of the WASI proposals, especially relative to the amount of review we're giving it.

I also don't want to jump to any conclusions though. If an active maintainer comes on, we get active review of existing and new code, and the dependency tree is perhaps slimmed down then I think it's reasonable to continue to keep wasi-crypto in-tree. I still think there's a huge amount of value in having something built-in for testing, and if users have been actively evaluating Wasmtime's support for wasi-crypto to help development of the upstream proposal that would be great to know.

Last updated: Nov 22 2024 at 16:03 UTC