github-actions[bot] commented on issue #3726:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
jedisct1 commented on issue #3726:
Any feedback on this?
jedisct1 commented on issue #3726:
The
--public-keys
has been renamed to--experimental-public-keys
.
github-actions[bot] commented on issue #3726:
Label Messager: wasmtime:config
It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:
[ ] If you added a new
Config
method, you wrote extensive documentation for
it.<details>
Our documentation should be of the following form:
```text
Short, simple summary sentence.More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.Is this method fallible? If so, when can it return an error?
Can this method panic? If so, when does it panic?
Example
Optional example here.
```</details>
[ ] If you added a new
Config
method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.<details>
For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config
][fuzzing-config] (or one
of its nestedstruct
s).Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.</details>
[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html
<details>
To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.
To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.</details>
jedisct1 commented on issue #3726:
Ping?
(no code changes in the rebases, these were just to fix merge conflicts introduced by the memfd work)
alexcrichton commented on issue #3726:
I believe that @tschneidereit was previously taking a look at this and I don't know where he left off. I also believe that he's away this week, but I can ping him about this next week when he's back.
alexcrichton commented on issue #3726:
Ok I talked with Till and it sounds like y'all mainly talked about the CLI interface and high-level concerns about this being experimental, so I'll focus more on the technical implementation.
Overall I'm personally concerned about the implementation of this where very little is in this repository and 99% of this is in an external crate. We do that for crates like
wasmparser
andwast
and such but it's pretty rare to outsource large implementation details from Wasmtime. This can run the risk of integration issues and also runs the risk of diluting the quality of code coming into Wasmtime because the external code is not reviewed in the same manner as code in this repository. Some specific things I noticed reading over thewasmsign2
crate and the integration in this PR:
This is adding, in the Rust sense, a "public" dependency on
wasmsign2
becausewasmsign2
's public types are appearing in Wasmtime's API. This is something we try to avoid to ensure that all APIs are well-audited, well documented, and well-tested. This is specifically thewasmtime::Config::public_keys
method which takeswasmsign2::PublicKeySet
where we're not reviewing or seeing changes to thePublicKeySet
API over time. It would be better if Wasmtime had standalone configuration of this feature which only required standard library or otherwise very common types.The
wasmsign2
crate is making decisions about cryptography implementations and what to use, but I don't think we have project consensus about the right approach here. I'm no security expert myself but I do believe that selecting a crypto implementation is a pretty big decision, and additionally it's something we'd want to be consistent with. I don't think we historically considered this whenwasi-crypto
was added experimentally as well, but as this continues to grow this is something we need to consider before stabilization of any form.The
wasmsign2
crate currently duplicates parsing the WebAssembly module with Wasmtime. The signature-check step is entirely independent of Wasmtime's own parsing of the module, meaning that it's not only double-parsed but it's also parsed with an entirely different set of Rust functions. I don't personally think that this is the best way to integrate this feature. I would expect that a wasm binary is parsed precisely once and integration of a signature-checking feature would be interleaved with the existing parsing (or somehow integrated in the general vicinity)These are some of the more major points at least but I figured it's at least a starting point. In some sense this PR matches how
wasi-crypto
was integrated where a large external dependency is added that's largely outside of this repository, butwasi-crypto
is also much more opt-in in that it's not part of thewasmtime
crate API and it's only about host functions rather than integrating into the flow of processing wasm modules.I think one possible way to improve the integration here would be to split the
wasmsign2
crate into a reader/writer half where Wasmtime would only depend on the reading portions and thewasmsign2
crate would have APIs for creating the context used to process signatures and then have the ability to be fed individual sections as they're parsed in the main loop of parsing a wasm module with thewasmparser
crate. Ideally thewasmsign2
crate would not do any parsing itself, or would have a mode where when integrated into Wasmtime it doesn't do any of its own parsing.
Last updated: Dec 23 2024 at 13:07 UTC