Allow passing KeyObjects via postMessage

[tniessen]

This patch allows to pass KeyObject instances between threads/contexts, and is based on some great work by @addaleax.

While the public API has not changed, the internal structure has become a little complicated. In order to be cloneable, the KeyObject class must be implemented in C++. However, KeyObject relies on its JavaScript class hierarchy. To get around this problem, a new base class NativeKeyObject is added, from which all KeyObject classes inherit:

             +---------------------+
             |     BaseObject      |
             +---------------------+
                        |
                        |
                        |
             +---------------------+
             |   NativeKeyObject   |
             +---------------------+
                        |
                        |
                        |
             +---------------------+
             |      KeyObject      |
             +---------------------+
               /                 \
              /                   \
             /                     \
            /                       \
+---------------------+    +---------------------+
|   SecretKeyObject   |    | AsymmetricKeyObject |
+---------------------+    +---------------------+
                             /                 \
                            /                   \
                           /                     \
                          /                       \
              +---------------------+   +---------------------+
              |   PublicKeyObject   |   |   PrivateKeyObject  |
              +---------------------+   +---------------------+

NativeKeyObject has no properties and doesn't change the behavior of KeyObject. It only exists to be cloneable.

Each KeyObject k has an associated KeyObjectHandle, which is stored in k[kHandle]. The public KeyObject API uses the internal KeyObjectHandle API to perform most operations, however, a KeyObjectHandle is a BaseObject and cannot be shared between threads directly. Instead, each KeyObjectHandle references a KeyObjectData object, which contains the actual key. This object can be shared between threads:

+-----------------+
| NativeKeyObject |
+-----------------+
        ^
     extends
        |
+-----------------+    +-----------------+    +---------------+
| KeyObject  (JS) | -> | KeyObjectHandle | -> | KeyObjectData |
+-----------------+    +-----------------+    +---------------+

When a key is passed via a MessagePort, the receiver creates a new KeyObjectHandle and the correct KeyObject (one of SecretKeyObject, PublicKeyObject, PrivateKeyObject). However, it does not create a new KeyObjectData object: All threads always refer to the same KeyObjectData for the same key:

+-------------------+
| NativeKeyObject 1 | ------------------------------------------+
+-------------------+                                           |
        ^                                                       |
     extends                                                    |
        |                                                       |
+-------------------+    +-------------------+                  |
| KeyObject 1  (JS) | -> | KeyObjectHandle 1 | --------------+  |
+-------------------+    +-------------------+               |  |
                                                             |  |
                                                             |  |
                                                             |  |
                                                             |  |
                                                             |  |
+-------------------+                                        |  |
| NativeKeyObject 2 | ------------------------------------+  |  |
+-------------------+                                     |  |  |
        ^                                                 |  |  |
     extends                                              |  |  |
        |                                                 |  |  |
+-------------------+    +-------------------+            |  |  |
| KeyObject 2  (JS) | -> | KeyObjectHandle 2 | --------+  |  |  |
+-------------------+    +-------------------+         |  |  |  |
                                                       |  |  |  |
                                                       |  |  |  |
                                                       |  |  |  |
                                                       |  |  |  |
                                                       |  |  |  |
+-------------------+                                  |  |  |  |
| NativeKeyObject 3 | ------------------------------+  |  |  |  |
+-------------------+                               |  |  |  |  |
        ^                                           |  |  |  |  |
     extends                                        |  |  |  |  |
        |                                           v  v  v  v  v
+-------------------+    +-------------------+    +---------------+
| KeyObject 3  (JS) | -> | KeyObjectHandle 3 | -> | KeyObjectData |
+-------------------+    +-------------------+    +---------------+

Since almost all operations go through the KeyObjectHandle, it makes sense for the KeyObjectHandle to contain a pointer to the KeyObjectData. However, the NativeKeyObject class implements CloneForMessaging and therefore also needs access to the same KeyObjectData. In theory, we could go through this[kHandle]->Data(), but it is easier to also maintain a pointer from the NativeKeyObject to the same KeyObjectData.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

cc @addaleax @panva @nodejs/crypto @nodejs/security-wg

[addaleax]

I assume that key instanceof Object is true here, but port2Moved should only create objects in context and not in the outer/main context (see also the comment for TransferData::Deserialize()).

NativeKeyObject::KeyObjectTransferData::Deserialize() should either fail if context != env->context(), or env->crypto_key_object_secret_constructor() etc. need to be stored as templates instead.

[tniessen]

Can you give me a hint how to store the constructors as templates? I didn't think it was possible with classes defined in JavaScript :)

[addaleax]

Ah right, it’s not … I guess that means that you’ll have to throw an exception here if the contexts mismatch

(The alternative would be giving node_crypto multi-context support, but we’re not really quite there yet I’d say … and in any case, that would be a lot more work than this PR)

[tniessen]

I can't quite figure out where to throw. Throwing in Deserialize compiles, but then how would the user catch it? It doesn't seem to cause an error event. Throwing in CloneForMessaging seems like the better choice to me, but that seems to be even more difficult (since it only happens in SerializerDelegate::Finish without access to the environment/context etc.). Do you have another hint for me? :)

[addaleax]

Hm – Deserialize() would be the right place, yes. Maybe adding

  TryCatchScope try_catch(env());
  try_catch.SetVerbose(true);

after the Context::Scope context_scope(context); line in MessagePort::OnMessage() helps? It would still result in an uncaught exception, I think, but I’m not really sure what the alternative would be… there is onmessageerror on the Web MessagePort, but we’re not currently implementing that.

(I think even just discarding the message would also be fine as a step for now – really anything that does not result in an object being created in the wrong Context.)

[panva]

@tniessen @addaleax thanks for taking my proposal into consideration. I've checked out the branch and did some initial testing.

The goal I had in mind for this is to have somewhat of an answer to #678. In my JOSE library I encourage developers to work with JWK key abstraction object instances that use crypto.KeyObject instances behind the scenes for clear performance gains when it comes to executing sign/verify/publicEncrypt/privateDecrypt ops with a repeated set of keys.

Still, depending on the [operation, algorithm, key type] in question the executed crypto can be really slow and most importantly blocking the thread.

Using the feature from this PR I can add an async interface that would unblock the main thread for operations that I know are going to be more expensive than the cost of a worker_thread message roundtrip without the need for crypto to completely re-parse the key before the op. It's a great optimization to what would otherwise be a waste of cpu cycles.

As you can see in my testing KeyObject instance transfer is always faster then key export and subsequent import in a worker thread.

The test doesn't run any crypto operation other than importing the key (in case of export-import) and then replying back to the main thread. ops/sec therefore observe the cost of key export(optional), sending a message, key import(optional), response.

What you can observe from the results is that keyobject transfer as more or less stable ops/sec where as export-import depends on the type of a key.

secret  transfer x 31,701 ops/sec ±4.30% (78 runs sampled)
secret   export-import x 27,923 ops/sec ±5.50% (76 runs sampled)

private rsa transfer x 30,459 ops/sec ±8.98% (79 runs sampled)
private rsa der export-import x 16,252 ops/sec ±1.72% (79 runs sampled)

public rsa transfer x 31,965 ops/sec ±3.18% (84 runs sampled)
public rsa der export-import x 19,767 ops/sec ±1.74% (79 runs sampled)

private ec transfer x 31,345 ops/sec ±4.23% (81 runs sampled)
private ec der export-import x 9,483 ops/sec ±1.32% (81 runs sampled)

public ec transfer x 31,025 ops/sec ±6.02% (83 runs sampled)
public ec der export-import x 10,384 ops/sec ±1.08% (83 runs sampled)

private ed25519 transfer x 30,734 ops/sec ±5.83% (82 runs sampled)
private ed25519 der export-import x 10,699 ops/sec ±0.95% (81 runs sampled)

public ed25519 transfer x 31,406 ops/sec ±5.35% (80 runs sampled)
public ed25519 der export-import x 22,013 ops/sec ±1.36% (81 runs sampled)

[himself65]

use private filed? I wouldn't like to add an extra symbol which is only used once, in this simple class KeyObject

like:

  class KeyObject extends NativeKeyObject {
    #type

    constructor(type, handle) {
      super(checkKeyTypeAndHandle(type, handle) || handle);

      this.#type = type;

      ObjectDefineProperty(this, kHandle, {
        value: handle,
        enumerable: false,
        configurable: false,
        writable: false
      });
    }

    get type() {
      return this.#type;
    }
  }

[tniessen]

That's pre-existing code, but I'll consider it. I guess the same is true for a lot of existing class fields in core.

[addaleax]

@tniessen Let me know if you’d like me to rebase this, the conflicts are kind of my fault after all :)

[tniessen]

@addaleax I rebased it, but either (a) there was a bug in this PR before, or (b) I broke something while rebasing this PR, or (c) something changed on the upstream branch that I didn't see.

Passing KeyObject instances to postMessage now throws:

DOMException [DataCloneError]: Cannot transfer object of unsupported type.

[tniessen]

Also, I see that you added a JSTransferable class. Would it make more sense to use that instead of NativeKeyObject?

[addaleax]

Also, I see that you added a JSTransferable class. Would it make more sense to use that instead of NativeKeyObject?

@tniessen Before #33772, it didn’t really occur to me that we could use that trick to make JS wrapper objects transferable. It may or may not be easier to keep going that route instead of changing the class structure here, but I think you can judge that better than me :)

As for the error … in CreateNativeKeyObject(), the FunctionTemplate doesn’t inherit from the BaseObject template. I think that was the place where I mentioned I was getting merge conflicts :)

(As a side note, you generally don’t want to create a new FunctionTemplate per object but rather use a single one, because V8 caches FunctionTemplates, so this would appear like a memory leak, similar to the one fixed by 978d89f.)

[tniessen]

As for the error … in CreateNativeKeyObject(), the FunctionTemplate doesn’t inherit from the BaseObject template. I think that was the place where I mentioned I was getting merge conflicts :)

Right, you mentioned that! Thank you :)

As a side note, you generally don’t want to create a new FunctionTemplate per object but rather use a single one, because V8 caches FunctionTemplates, so this would appear like a memory leak, similar to the one fixed by 978d89f.

@addaleax Could you clarify this? From my perspective, CreateNativeKeyObject creates the JS class NativeKeyObject (once), and creating instances of NativeKeyObject should not cause new FunctionTemplates to be created. (I have to admit, the name is misleading.)

[addaleax]

As a side note, you generally don’t want to create a new FunctionTemplate per object but rather use a single one, because V8 caches FunctionTemplates, so this would appear like a memory leak, similar to the one fixed by 978d89f.

@addaleax Could you clarify this? From my perspective, CreateNativeKeyObject creates the JS class NativeKeyObject (once), and creating instances of NativeKeyObject should not cause new FunctionTemplates to be created. (I have to admit, the name is misleading.)

Right, I was misled by the name to think that it would create one KeyObject object per call, not multiple classes :) Sorry!

[addaleax]

Nice work, LGTM!

[jstewmon]

👋 Hi, I don't have a simple reproduction yet, but I think this change has resulted in a memory leak where every instance inherited from NativeKeyObject is retained forever.

I noticed this in a production application which exhibits a clear memory leak on node 12.19.0 but not on node 12.18.4.

I've taken heap dumps of my application surrounding a few minutes of generated requests, and I can see that *KeyObject constructors are never being freed (these objects are constructed by my application at request time):

I'm not exactly an expert at using the snapshot analyzer, but I think Global handles is significant:

I can also locate these object in the Containment view at (GC roots) -> (Global handles).

For context, my application is experiencing this when using the jose package to format JWKs - the key objects are created using the create* functions from crypto: https://unpkg.com/[email protected]/lib/help/key_object.js

[panva]

@addaleax, @tniessen is this what #35481 was about? In that case, should it be backported and part of 12.20.0?

[jstewmon]

@panva, thanks for the issue reference. The fix you mentioned was released in v14.13.1, and I confirmed the leak does not reproduce on v14.15.0.

[addaleax]

@panva @jstewmon Yes, exactly 👍