Adds verification key for Wasm module #177

asraa · 2021-07-02T14:42:35Z

DRAFT: I think the argument should be a VerificationOptions struct containing possibly more keys, a verification type, all/none specification.

Signed-off-by: Asra Ali <[email protected]>

PiotrSikora

I think the hardcoded key should stay (and take a priority if used), since for binary authorization we want to make sure that if Envoy is compiled with a given key, then only Wasm modules signed with that key could be loaded and nothing else (even if provided in configuration). Yes, this is "I'm paranoid" feature :)

PiotrSikora · 2021-07-02T17:44:08Z

src/signature_util.cc

-  static const auto ed25519_pubkey = hex2pubkey<32>(PROXY_WASM_VERIFY_WITH_ED25519_PUBKEY);
+  char pubkey_char[65];
+  strcpy(pubkey_char, pubkey.c_str());
+  static const auto ed25519_pubkey = hex2pubkey<32>(pubkey_char);


This is static variable, so the first key used would persist forever and it would be used for all verifications, leading to false negatives.

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-08-12T14:55:54Z

Added some tests now for build time key precedence and no key present verification

asraa · 2021-08-12T15:11:02Z

Oh one quick comment I just remembered: should the pubkey inputs be validated at all? Or should we trust these relatively because of it's a configuration value that's likely valid

PiotrSikora

Could you rebase it on top of master, fix the failing tests and format errors?

PiotrSikora · 2021-09-09T10:42:33Z

.github/workflows/cpp.yml

@@ -88,3 +88,4 @@ jobs:
    - name: Test (signed Wasm module)
      run: |
        bazel test --define runtime=${{ matrix.runtime }} --per_file_copt=//...@-DPROXY_WASM_VERIFY_WITH_ED25519_PUBKEY=\"$(xxd -p -c 256 test/test_data/signature_key1.pub | cut -b9-)\" //test:signature_util_test
+


Style: unnecesary change / newline.

PiotrSikora · 2021-09-09T10:53:01Z

include/proxy-wasm/wasm.h

@@ -345,8 +345,10 @@ using WasmHandleCloneFactory =
    std::function<std::shared_ptr<WasmHandleBase>(std::shared_ptr<WasmHandleBase> wasm)>;

 // Returns nullptr on failure (i.e. initialization of the VM fails).
+// TODO: Consider a VerificationOptions struct rather than a single pubkey.


s/TODO/TODO(asraa)/

PiotrSikora · 2021-09-09T11:12:17Z

include/proxy-wasm/signature_util.h

@@ -27,7 +27,7 @@ class SignatureUtil {
   * @param message is the reference to store the message (success or error).
   * @return indicates whether the bytecode has a valid Wasm signature.
   */
-  static bool verifySignature(std::string_view bytecode, std::string &message);
+  static bool verifySignature(std::string_view bytecode, const std::string pubkey, std::string &message);


Style: clang-format warning (line too long).

PiotrSikora · 2021-09-09T11:13:03Z

include/proxy-wasm/wasm.h

 std::shared_ptr<WasmHandleBase>
-createWasm(std::string vm_key, std::string code, std::shared_ptr<PluginBase> plugin,
+    createWasm(std::string vm_key, std::string code, std::string pubkey,
+               std::shared_ptr<PluginBase> plugin,


Style: clang-format warning (weird indent).

PiotrSikora · 2021-09-09T11:30:33Z

src/wasm.cc

@@ -463,6 +463,7 @@ WasmForeignFunction WasmBase::getForeignFunction(std::string_view function_name)
 }

 std::shared_ptr<WasmHandleBase> createWasm(std::string vm_key, std::string code,
+                                           std::string pubkey,


Style: clang-format warning (should be in the previous line).

PiotrSikora · 2021-09-09T11:44:23Z

src/wasm.cc

@@ -249,9 +249,9 @@ bool WasmBase::load(const std::string &code, bool allow_precompiled) {
    return true;
  }

-  // Verify signature.
+  // Verify signature if a pubkey is present.


Suggested change

// Verify signature if a pubkey is present.

// Verify signature if a public key is present (embedded at build-time or provided via pubkey).

PiotrSikora · 2021-09-09T11:46:58Z

test/signature_util_test.cc

@@ -24,36 +24,72 @@

 namespace proxy_wasm {

-TEST(TestSignatureUtil, GoodSignature) {
+std::string BytesToHex(std::vector<uint8_t> bytes) {


Suggested change

std::string BytesToHex(std::vector<uint8_t> bytes) {

static std::string bytesToHex(std::vector<uint8_t> bytes) {

PiotrSikora · 2021-09-09T11:47:29Z

test/signature_util_test.cc

+  return result;
+}
+
+static std::string publickey(std::string filename) {


Suggested change

static std::string publickey(std::string filename) {

static std::string publicKey(std::string filename) {

PiotrSikora · 2021-09-09T12:11:19Z

src/signature_util.cc

+  if (!pubkey.empty()) {
+    char pubkey_char[65];
+    strcpy(pubkey_char, pubkey.c_str());
+    return hex2pubkey<32>(pubkey_char);


hex2pubkey is a constexpr template written to decode hex literal at the build-time with zero-cost at runtime.

But if you have to copy all the bytes to use it, then it kind of misses the point, and it would be better to add hexstr2pubkey function that can decode it in a single pass (basically, a runtime equivalent of hex2pubkey).

asraa added 2 commits July 2, 2021 09:57

add pubkey

38468fc

Signed-off-by: Asra Ali <[email protected]>

Merge remote-tracking branch 'upstream/master' into add-pubkey

36161d6

asraa requested review from mathetake and PiotrSikora as code owners July 2, 2021 14:42

This was referenced Jul 2, 2021

Wasm module signature verification envoyproxy/envoy#17220

Open

Adds verification key for Wasm module envoyproxy/envoy#17221

Closed

PiotrSikora requested changes Jul 2, 2021

View reviewed changes

asraa added 2 commits August 12, 2021 09:57

Merge remote-tracking branch 'upstream/master' into add-pubkey

50c207b

fix comments

884a366

Signed-off-by: Asra Ali <[email protected]>

PiotrSikora requested changes Sep 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds verification key for Wasm module #177

Adds verification key for Wasm module #177

asraa commented Jul 2, 2021

PiotrSikora left a comment

PiotrSikora Jul 2, 2021

asraa commented Aug 12, 2021

asraa commented Aug 12, 2021

PiotrSikora left a comment

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

PiotrSikora Sep 9, 2021

	// Verify signature if a pubkey is present.
	// Verify signature if a public key is present (embedded at build-time or provided via pubkey).

	std::string BytesToHex(std::vector<uint8_t> bytes) {
	static std::string bytesToHex(std::vector<uint8_t> bytes) {

	static std::string publickey(std::string filename) {
	static std::string publicKey(std::string filename) {

Adds verification key for Wasm module #177

Are you sure you want to change the base?

Adds verification key for Wasm module #177

Conversation

asraa commented Jul 2, 2021

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Aug 12, 2021

asraa commented Aug 12, 2021

PiotrSikora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment