Stream: git-wasmtime

Topic: wasmtime / issue #3726 Add support for digital signatures

Wasmtime GitHub notifications bot (Jan 25 2022 at 21:04):

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>

Wasmtime GitHub notifications bot (Feb 02 2022 at 18:43):

jedisct1 commented on issue #3726:

Any feedback on this?

Wasmtime GitHub notifications bot (Feb 10 2022 at 19:48):

jedisct1 commented on issue #3726:

The --public-keys has been renamed to --experimental-public-keys.

Wasmtime GitHub notifications bot (Feb 16 2022 at 17:39):

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 nested structs).

  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.

Learn more.

</details>

Wasmtime GitHub notifications bot (Feb 23 2022 at 08:50):

jedisct1 commented on issue #3726:

Ping?

(no code changes in the rebases, these were just to fix merge conflicts introduced by the memfd work)

Wasmtime GitHub notifications bot (Feb 23 2022 at 23:22):

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.

Wasmtime GitHub notifications bot (Mar 01 2022 at 18:22):

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 and wast 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 the wasmsign2 crate and the integration in this PR:

• This is adding, in the Rust sense, a "public" dependency on wasmsign2 because wasmsign2'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 the wasmtime::Config::public_keys method which takes wasmsign2::PublicKeySet where we're not reviewing or seeing changes to the PublicKeySet 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 when wasi-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, but wasi-crypto is also much more opt-in in that it's not part of the wasmtime 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 the wasmsign2 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 the wasmparser crate. Ideally the wasmsign2 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: Oct 23 2024 at 20:03 UTC