Skip to content

Add support for digital signatures#3726

Closed
jedisct1 wants to merge 1 commit into
bytecodealliance:mainfrom
jedisct1:signatures
Closed

Add support for digital signatures#3726
jedisct1 wants to merge 1 commit into
bytecodealliance:mainfrom
jedisct1:signatures

Conversation

@jedisct1
Copy link
Copy Markdown
Contributor

@jedisct1 jedisct1 commented Jan 25, 2022

This adds a new cargo feature flag digital-signatures that brings support for signature verification, using the the current proposal for WebAssembly modules signatures (https://github.com/WebAssembly/tool-conventions/blob/main/Signatures.md).

No behavior changes unless the --experimental-public-keys option is used with the run command. This options accepts one or more public keys, that the entire module must have been signed with in order to run.

@github-actions github-actions Bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 25, 2022
@github-actions
Copy link
Copy Markdown

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 .github/subscribe-to-label.json configuration file.

Learn more.

@jedisct1 jedisct1 force-pushed the signatures branch 2 times, most recently from 8b704be to 1006d51 Compare January 25, 2022 22:01
@jedisct1
Copy link
Copy Markdown
Contributor Author

jedisct1 commented Feb 2, 2022

Any feedback on this?

@jedisct1 jedisct1 force-pushed the signatures branch 3 times, most recently from d90d6d6 to 58fedec Compare February 10, 2022 19:46
@jedisct1
Copy link
Copy Markdown
Contributor Author

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

@jedisct1 jedisct1 force-pushed the signatures branch 2 times, most recently from 088dade to 1a2d40f Compare February 14, 2022 12:28
@github-actions github-actions Bot added the wasmtime:config Issues related to the configuration of Wasmtime label Feb 16, 2022
@github-actions
Copy link
Copy Markdown

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:

    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.
    
  • 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 (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 for more 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.


Details

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

This adds a new cargo feature flag `digital-signatures` that
brings support for signature verification, using the the current proposal
for WebAssembly modules signatures.
(https://github.com/WebAssembly/tool-conventions/blob/main/Signatures.md)

No behavior changes unless the `--experimental-public-keys` option is used
with the `run` command. This options accepts one or more public keys, that
the entire module must be signed with in order to run.
@jedisct1
Copy link
Copy Markdown
Contributor Author

Ping?

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

@alexcrichton
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

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.

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 wasmtime:config Issues related to the configuration of Wasmtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants