Skip to content
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

Feature Request: Safe/reliable crypto.KeyObject and webcrypto.CryptoKey detection in userland #38611

Closed
mscdex opened this issue May 9, 2021 · 6 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@mscdex
Copy link
Contributor

mscdex commented May 9, 2021

Currently there are no public methods for checking for KeyObject and CryptoKey objects. Having reliable checks for these would be very nice to have for userland code that wants to support these types of objects in their code.

It seems there are already internal methods for achieving this, but I'm not sure why they are not currently exposed to userland. I was thinking maybe something like how Buffer handles this: KeyObject.isKeyObject() and CryptoKey.isCryptoKey(). The former is currently pretty easy to do in userland since it's just doing an instanceof check, but the latter checks "private" symbols, which is not as easy/ideal for userland (although I guess at least currently instanceof CryptoKey would work?). Either way, having these encapsulated in helper methods would help insulate changes to the checking logic, should it ever change.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. labels May 9, 2021
@tniessen
Copy link
Member

tniessen commented May 9, 2021

Re KeyObject: When I designed the new APIs, I intentionally did not expose that class directly. The only reason we did so eventually is such that users can rely on instanceof KeyObject.

@tniessen
Copy link
Member

tniessen commented May 9, 2021

See #26200 and #26438.

@mscdex
Copy link
Contributor Author

mscdex commented May 9, 2021

I still think it would be a good idea to expose static .is*() methods in case the method of detection should change in the future (e.g. instanceof to symbol check) as with Buffer.isBuffer() (where the detection method did change over time).

@panva
Copy link
Member

panva commented May 10, 2021

Both instanceof crypto.KeyObject and instanceof crypto.webcrypto.CryptoKey work so long as the code doesn't run e.g. in a vm context.

I think exposing a symbol check function would be better.

@jasnell
Copy link
Member

jasnell commented May 10, 2021

CryptoKey.isCryptoKey() is problematic because it extends the standard API in a non-standard way. As I suggest in @panva's PR, we could add both checks to KeyObject:

KeyObject.isKeyObject()
KeyObject.isCryptoKey()

Alternatively, we could add both to util/types

const { isKeyObject, isCryptoKey } = require('util/types');

isKeyObject(obj)
isCryptoKey(obj)

If the Node.js binary is built without crypto support, these would still exist but would always return false.

@mscdex
Copy link
Contributor Author

mscdex commented May 10, 2021

I don't have a strong preference where the functions live, just that they're available in some sensible place.

panva added a commit to panva/node that referenced this issue May 13, 2021
panva added a commit to panva/node that referenced this issue May 14, 2021
panva added a commit to panva/node that referenced this issue May 14, 2021
@panva panva closed this as completed in 3ee1f9a May 17, 2021
targos pushed a commit that referenced this issue May 18, 2021
closes #38611

PR-URL: #38619
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants