Skip to content

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

Merged
merged 9 commits into from
May 15, 2021

Conversation

PiotrSikora
Copy link
Member

Signed-off-by: Piotr Sikora [email protected]

@PiotrSikora PiotrSikora requested a review from mathetake as a code owner April 6, 2021 00:57
@PiotrSikora
Copy link
Member Author

cc @asraa

@mathetake
Copy link
Contributor

mathetake commented Apr 6, 2021

can this be runtime-agnostic, or is there any V8-specific stuff here?

Copy link
Contributor

@mathetake mathetake left a 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?

@mathetake
Copy link
Contributor

at least I would like to have tests for this feature here.

@PiotrSikora
Copy link
Member Author

This would add openssl dependency on proxy-wasm-cpp-host

We already have that dependency (in exports, for WASI random).

I see that the logc can be runtime-agnostic by only using getCustomSection

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 (getCustomSection, getStrippedSource, buildFunctionNameIndex), logic around precompiled modules and now this signature verification should be extracted to common utils.

Filled: #148

so I would suggest that we move this logic to Envoy side. WDYT?

There is absolutely no reason for this code to live in Envoy, which doesn't operate on the Wasm bytecode.

at least I would like to have tests for this feature here.

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.

@mathetake
Copy link
Contributor

Right. Thanks for clarifying!

@PiotrSikora
Copy link
Member Author

can this be runtime-agnostic, or is there any V8-specific stuff here?

Done, PTAL.

@PiotrSikora PiotrSikora requested a review from mathetake May 12, 2021 10:02
@mathetake
Copy link
Contributor

Do you mind adding tests? 🙏

@PiotrSikora
Copy link
Member Author

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.

@mathetake
Copy link
Contributor

Makes sense, thanks! let me have a look at 163.

@mathetake
Copy link
Contributor

just merged #163 :)

@PiotrSikora
Copy link
Member Author

@mathetake tests added and passed, PTAL.

Copy link
Contributor

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@PiotrSikora PiotrSikora merged commit e4042ae into proxy-wasm:master May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants