-
Notifications
You must be signed in to change notification settings - Fork 73
Add support for loading and verifying signed Wasm modules. #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cc @asraa |
can this be runtime-agnostic, or is there any V8-specific stuff here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would add openssl dependency on proxy-wasm-cpp-host and I see that the logc can be runtime-agnostic by only using getCustomSection
, so I would suggest that we move this logic to Envoy side. WDYT?
at least I would like to have tests for this feature here. |
We already have that dependency (in exports, for WASI random).
This is indeed runtime-agnostic, and it shoudn't be here. Unfortunately, we have a lot of code duplication and runtime-agnostic code in runtime-specific implementations. At the very least, all bytecode parsing ( Filled: #148
There is absolutely no reason for this code to live in Envoy, which doesn't operate on the Wasm bytecode.
Agreed. Note that this is mostly me "throwing the code over the wall", so that @asraa can use it as a base to implement this feature end-to-end in Envoy (including trusted public key configuration over xDS vs embedded in the binary at build-time as in this PR), so it's not expected to stay this way. We can also wait until #148 is fixed before merging this. |
Right. Thanks for clarifying! |
Signed-off-by: Piotr Sikora <[email protected]>
Done, PTAL. |
Do you mind adding tests? 🙏 |
I've made tests, but they require a new Rust tool for generating signatures, so I had to split Rust updates into #163, otherwise the PR was dominated by unrelated changes. Once that PR is merged, I'm going to push tests. |
Makes sense, thanks! let me have a look at 163. |
just merged #163 :) |
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
@mathetake tests added and passed, PTAL. |
Signed-off-by: Piotr Sikora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Piotr Sikora [email protected]