util: add util.types.isKeyObject and util.types.isCryptoKey

[panva]

closes#38611

cc @tniessen@mscdex @nodejs/crypto @nodejs/util

[tniessen]

This would be the first visible deviation from the WebCrypto standard, wouldn't it? It might not be a violation of the spec since we are still implementing the required interface members, but it would still add a non-standard function to a standard class.

One of the very few benefits of WebCrypto is portability, and this change appears to reduce that further. (But we are already doing that with Node.js specific algorithms, so...)

[panva]

This would be the first visible deviation from the WebCrypto standard, wouldn't it? It might not be a violation of the spec since we are still implementing the required interface members, but it would still add a non-standard function to a standard class.

One of the very few benefits of WebCrypto is portability, and this change appears to reduce that further. (But we are already doing that with Node.js specific algorithms, so...)

Plus i made sure to mention so in the method's documentation.

[tniessen]

Plus i made sure to mention so in the method's documentation.

Sure, but that doesn't prevent code that uses crypto.subtle in Node.js from breaking once it is used in a browser or in Deno.

Why is instanceof not enough? And if it isn't, why doesn't the standard provide an API? And what do other runtimes do to solve this?

[jasnell]

@tniessen

Why is instanceof not enough? And if it isn't, why doesn't the standard provide an API? And what do other runtimes do to solve this?

instanceof is both slow and does not work across contexts. Both of which may be acceptable limitations.

None of the JavaScript or Web Platform standards that I'm aware of include APIs for brand checks like this. Node.js has always been unique in doing so (e.g. util/types).

Other platforms don't solve this. Most rely on instanceof ... but then again, Node.js is unique in that it allows creating multiple contexts via the vm module.

[jasnell]

@panva ... I don't think Crypto.isCryptoKey() is a good idea. We've been criticized in the past for attempting to add non-standard things to standard APIs and I'd rather not go down that road. Adding is as a utility method off crypto or even a static on KeyObject would work tho... e.g. KeyObject.isCryptoKey() and KeyObject.isKeyObject() ... given that CryptoKey is just a thin wrapper around KeyObject I don't think that's too much of a stretch.

[panva]

@panva ... I don't think Crypto.isCryptoKey() is a good idea. We've been criticized in the past for attempting to add non-standard things to standard APIs and I'd rather not go down that road. Adding is as a utility method off crypto or even a static on KeyObject would work tho... e.g. KeyObject.isCryptoKey() and KeyObject.isKeyObject() ... given that CryptoKey is just a thin wrapper around KeyObject I don't think that's too much of a stretch.

I don't see a problem per se, but i'm fine with going either way. Depends on what the (hopefully) coming consensus here is going to be.

[tniessen]

As I said before, here and in various other places, from my perspective, the main benefit of having WebCrypto in Node.js is compatibility with other systems that implement the same standard. instanceof might be slow, but WebCrypto really isn't great for scalable crypto performance anyway. Contexts are a good point, but even then, extending a standard class for a Node.js-specific problem doesn't seem like a good idea to me.

[panva]

Would this be less controversial then?

KeyObject.isKeyObject(any)KeyObject.isCryptoKey(any)

[tniessen]

Would this be less controversial then?

KeyObject.isKeyObject(any)KeyObject.isCryptoKey(any)

KeyObject.isKeyObject appears to be in line with Buffer.isBuffer, Array.isArray etc. ✔️

KeyObject.isCryptoKey, introduces a strong reference to the CryptoKey class in the KeyObject API, which, so far, could as well exist in an environment that doesn't support WebCrypto.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/38080/

[tniessen]

@panva Your code LGTM :) But, to be honest, I'm confused. I'd like to ask a few more things for my own understanding.

---

From #38611 (comment):

as with Buffer.isBuffer() (where the detection method did change over time).

I traced the current behavior back to at least 22c68fd, which was eight years ago:

node/lib/util.js

Lines 508 to 510 in 22c68fd

	functionisBuffer(arg){
	returnarginstanceofBuffer;
	}

It appears to be the same logic that's still there today, so while the detection method probably changed in the past, it's been stable for at least eight years:

node/lib/buffer.js

Lines 507 to 509 in 70157b9

	Buffer.isBuffer=functionisBuffer(b){
	returnbinstanceofBuffer;
	};

---

instanceof is both slow and does not work across contexts. Both of which may be acceptable limitations.

@jasnell Is there a fundamental difference between Buffer and KeyObject/CryptoKey that would justify using instanceof for Buffer but not for KeyObject/CryptoKey? Or should Buffer really not be using instanceof but it's there for backward compatibility / lack of alternatives?

does not work across contexts

@jasnell I am trying to come up with a scenario in which obj instanceof KeyObject would return false. I assume this means that KeyObjectClassInContextA !== KeyObjectClassInContextB. But wouldn't that usually imply kHandleInContextA !== kHandleInContextB?

I don't usually use the vm module, so I'm not sure how this would work. I was under the impression that Node.js builtins would be shared either entirely or not at all.

---

Recapping #26200 and #26438:

• I originally intentionally did not expose KeyObject to make it more difficult to misuse the API, but this made it difficult to check whether any given value was a KeyObject.
• Hence crypto: expose crypto.isKeyObject() helper #26200, which suggested exposing isKeyObject and was based on a discussion between @panva and myself. However, others pointed out that isKeyObject was merely doing instanceof, and that it would be more consistent with the rest of the crypto module to expose KeyObject than to add isKeyObject, since we also don't have isCipher, isHash, isHmac, etc.
• Hence crypto: expose KeyObject class #26438, which exposed the KeyObject class to allow using instanceof, and which landed with no controversial discussion.

Code fixed.

[panva]

Recapping #26200 and #26438:

• I originally intentionally did not expose KeyObject to make it more difficult to misuse the API, but this made it difficult to check whether any given value was a KeyObject.
• Hence crypto: expose crypto.isKeyObject() helper #26200, which suggested exposing isKeyObject and was based on a discussion between @panva and myself. However, others pointed out that isKeyObject was merely doing instanceof, and that it would be more consistent with the rest of the crypto module to expose KeyObject than to add isKeyObject, since we also don't have isCipher, isHash, isHmac, etc.
• Hence crypto: expose KeyObject class #26438, which exposed the KeyObject class to allow using instanceof, and which landed with no controversial discussion.

since we also don't have isCipher, isHash, isHmac, etc.

I have yet to come across the need to verify user input is Cipher, Hash, or Hmac. KeyObject/CryptoKey OTOH is much more likely to be a function input, specifically one that needs to be distinguished from Buffers, strings, objects that are otherwise accepted in the same argument.

Hence #26200, which suggested exposing isKeyObject and was based on a discussion between @panva and myself. However, others pointed out that isKeyObject was merely doing instanceof

I didn't know better at the time 🤷 as shown above doing instanceof is leaving performance on the table.

[tniessen]

@panva Neither of us should have to justify the design choices back then, I was just recapping for context. I think all your contributions to the crypto APIs were well-designed :)

[tniessen]

LGTM but I'd love to see an example where this helps across contexts, maybe even an added test.

[panva]

Landed in 3ee1f9a

[tniessen]

I realize that this was landed, but there are still some unanswered questions...

Is there a fundamental difference between Buffer and KeyObject/CryptoKey that would justify using instanceof for Buffer but not for KeyObject/CryptoKey? Or should Buffer really not be using instanceof but it's there for backward compatibility / lack of alternatives?

I am trying to come up with a scenario in which obj instanceof KeyObject would return false. I assume this means that KeyObjectClassInContextA !== KeyObjectClassInContextB. But wouldn't that usually imply kHandleInContextA !== kHandleInContextB?

I'd love to see an example where this helps across contexts, maybe even an added test.

[jasnell]

@tniessen

In the most typical case the context stuff won't matter, you're right, because even with the vm module, instanceof will still work. I'm thinking about more exotic cases like electron, where someone may have both the chromium implementation of a CryptoKeyand Node.js CryptoKey (similar to the fact that you can have both chromium's URL and Node.js URL ... which is the reason we have isURLInstance()). In that case, key instanceof CryptoKey could be ambiguous and could fail if you really do not know which CryptoKey class you're working with. util.types.isCryptoKey() is fast and unambiguous and tells you exactly if this thing you have is Node.js' idea of a CryptoKey.

As for KeyObject, yes, instanceof works but having a faster and consistently similar API for checking the type makes sense.

[tniessen]

Thank you for the explanation, @jasnell!

[panva]

@nodejs/backporters leaving a note here that if backport were to happen to do so for util.types.isKeyObject, not util.types.isCryptoKey.

(not sure i'm following the process right here, please correct me if i'm wrong to request a backport, or if i can open a backport PR myself?)

[targos]

The WebCrypto API doesn't exist in versions <15.x