crypto: expose crypto.isKeyObject() helper

[panva]

Exposes crypto.isKeyObject to be able to check that a provided input is a prepared crypto key object from the crypto module add in v11.6.0 (#24234)

This is to get around a dirty practice we have to use now (example below) that allows to use instanceof.

const { generateKeyPairSync, createSecretKey } = require('crypto')

const { publicKey, privateKey } = generateKeyPairSync('ec', { namedCurve: 'P-256' })

const PrivateKeyObject = privateKey.constructor
const PublicKeyObject = publicKey.constructor
const SecretKeyObject = createSecretKey(Buffer.allocUnsafe(1)).constructor

module.exports = { PrivateKeyObject, PublicKeyObject, SecretKeyObject }

With this check in place this can be replaced with

const { isKeyObject } = require('crypto')

module.exports.isPrivateKey = (value) => {
  return isKeyObject(value) && value.type === 'private'
}
module.exports.isPublicKey = (value) => {
  return isKeyObject(value) && value.type === 'public'
}
module.exports.isSecretKey = (value) => {
  return isKeyObject(value) && value.type === 'secret'
}

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

[panva]

🤔 shall i expose this under crypto instead?

[BridgeAR]

Instead of exposing the internal isKeyObject function, it would be good to expose the KeyObject class itself (as done with all other crypto classes). The function is only a helper for obj instanceof KeyObject and that can be done as soon as the class is exposed.

@nodejs/crypto @tniessen was there a specific reason KeyObject was not exposed in the first place?

[panva]

@BridgeAR this was my very original question to @tniessen as well, quoting my communication with him

I intentionally did not expose the constructors and I still think it is best not to do so, primarily to prevent third-party applications from depending on Node.js internals. What about a function such as util.types.isCryptoKeyObject? That would match our existing conventions for such checks. The kind of key can then be checked via the .type property etc.

Now the constructor can only be called with internal handles so I tend to agree with not exposing it.

Putting it under utils tho fails CI for ?reasons? (see https://travis-ci.com/nodejs/node/builds/101419655). I have it passing under crypto.isKeyObject locally and can push that.

[panva]

@BridgeAR would you mind changing the PR label?

[tniessen]

@BridgeAR I'd prefer not to expose CryptoKey. The only legitimate use is instanceof CryptoKey, no other operations should be possible or permitted, and exposing a class just to be able to do instanceof is not the best approach IMO.

I suggested to add util.types.isCryptoKeyObject to @panva in a private discussion. cc @addaleax who designed the util.types API IIRC.

[panva]

I couldn't get around make -j4 test failing when the helper was in util.types, hence its inclusion in crypto

See the two different approaches

• (15f8b49) https://travis-ci.com/nodejs/node/builds/101418700
• (854625f) https://travis-ci.com/nodejs/node/builds/101419655

[addaleax]

@tniessen I didn’t design much of that API, I just exposed it :)

crypto.isKeyObject() seems okay, but I’m fine with either approach.

[tniessen]

Thanks for the input, @addaleax! :)

I prefer util.types.isCryptoKeyObject (or maybe util.types.isKeyObject) to try to avoid bloating the crypto module further. Maybe @sam-github has a preference or opinion? (Sorry for pulling more people into this, I just really want to know your opinions!)

[sam-github]

I'm not totally clear on the purpose. Why is it necessary to make this check specifically for key objects? What about SecureContext, which has an Object form, or can be an actual SecureContext (like key objects)? Or ReadStream vs WriteStream? Etc. My concern there is about whether there is something specific to crypto KeyObjects that run-time checking like this is necessary (as opposed to just using the object, and letting the method error if its incorrect), and if its not specific to KeyObjects, rather than growing a crypto-wart API, maybe a more general pattern is required?

For example, if I wanted a js function to check if its arg is a key object I would prefer to do something like this rather than is-checking:

function wantsPubKey(key) {
  // Will throw if key cannot be used to construct a key, and return key if its already a public key
  key = crypto.createPublicKey(key)
  ...
}

For is-checking, util.types.* could be a place for a more general pattern, but ATM it seems to be entirely built-in js types, I'm not sure this would fit into it. Also, I didn't know it even existed until now.... :-) So it might be an obscure place.

I'm not opposed to this, but it feels a bit like an ad-hoc solution to an unspecified problem.

[panva]

@sam-github

I'm not totally clear on the purpose.

Fail-fast and harmonizing the object a library will be working with. The exact use for me, is importing keys to a key store, specifically a JWKS and the input will not be used for crypto operations straight away. You can see the snippet using the constructors pulled from my hack in the description in this gist.

Using an already validated key material is appealing to that use case since the key object may already be present. It may be in an already existing keystore that i'm copying keys from or any other means, knowing its an immutable internal KeyObject is why it's okay to just use the input as-is and avoid calling .export() on it just so that it can be parsed again via createSecretKey/createPublicKey/createPrivateKey.

I can see from that code the helper could be omitted IF an existing KeyObject instance was valid input to createSecretKey/createPublicKey/createPrivateKey (it is not today). But this is just one use, there may be more.

[BridgeAR]

Should we add a note that this does not work with other contexts / realms?

[panva]

I'm afraid I do not follow :(

[BridgeAR]

The function uses obj instanceof KeyObject internally. This does not work in case some code is run in e.g. the vm.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_context_(e.g._frames_or_windows) for further details.

[panva]

Can you suggest the language? I'm happy to add it.

[BridgeAR]

Maybe:

Returns `true` if the value is a [`KeyObject`][] instance from the [current execution context][].

// ...

[current execution context]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_context_(e.g._frames_or_windows)

But this might be confusing to others. So it's maybe something we do not have to add?

[tniessen]

Change itself LGTM. Remaining questions:

1. Do we want to have this function?
2. Do we want to have this function at this location with this name?

Regarding (1) I guess it does no harm to have this function and I believe there are instances where it might be useful.

[panva]

Regarding (2) i wasn’t able to get tests to pass under util.types and would need someone’s help to make it happen.

[sam-github]

The more I think about it, the more uncertain I am this API is a good idea. I'm not sure it "does no harm" to add it if it adds an idiosyncratic appendage to an already large and complex crypto API.

Why is there not a family of APIs, isHmac(), isCipher(), etc.?

I looks to me because they all expose the class, so instanceof checks can be made.

So, why don't we expose the class here?

Perhaps because we want API users to not use classes directly?

But this leads me back to the first question, why is there not a family of APIs so we recommend people away from direct class usage?

I also quite prefer my suggestion that input be sanitized by passing it through its expected "create" function. It ensures that its converted to its preferred pre-parsed object (for performance) and also asserts that its of the correct type/object form as a side effect, and can be used as a pure assertion of type, if that's all that's wanted.

[panva]

if that's all that's wanted

indeed it is in my case

[addaleax]

@tniessen @sam-github Any further thoughts on this?

[sam-github]

@addaleax There has been no response to my last comments, so no further thoughts, my thoughts remain the same:

I think the need for a one-off is-function for key types indicates an underlying problem that should be fixed.

I think we should expose the class. At that point the is-function becomes unnecessary.

If we want people to stop using the classes (all the classes), we should either provide a family of is-functions simultaneously for ALL the classes, or (and?) change the create APIs so they can take an object of their create type, and return the arg.

[panva]

I'm fine with all the options

• expose the class, knowing it can't be used for anything else than instanceof
• this helper
• make the create*Key functions accept an existing instance

There's already a PR to support privateKeyObject instance as argument for createPublicKey. My preference would be both exposing the class and accepting existing instances. Whichever comes first will help me to get rid of this, unstable looking export to aid with normalizing key import

[sam-github]

@tniessen what do you think? I assume you didn't expose the class partly because all the other classes have "do not use this" in their docs, but is there any other reason?

The next week is busy, but I think I can expose the class pretty easily, and look at the input args later.

Unless @panva you would be willing to do that? Assuming @tniessen is OK with it.

[tniessen]

Sorry Sam, I was away for a couple of days.

what do you think? I assume you didn't expose the class partly because all the other classes have "do not use this" in their docs, but is there any other reason?

Exactly, I did not expose the class to prevent people from attempting to use it directly. In my opinion, exposing the classes just to allow instanceof is a workaround. In any case, only the KeyObject base class should be exposed since all other classes are implementation details IMO.

There's already a PR to support privateKeyObject instance as argument for createPublicKey. My preference would be both exposing the class and accepting existing instances

I can easily add support for calling createPublicKey etc. on existing KeyObjects if that's what we want.

Why is there not a family of APIs, isHmac(), isCipher(), etc.?

I guess there are very few use cases where one needs such checks. While there are different ways to represent key objects, there is only one way to represent an HMAC object (within node core), and no other object can be converted to an HMAC object. (Sadly, we also still have two classes for isCipher, Cipher and Cipheriv...)

To be honest, I'm not sure what's the best way here. If the only use case is to sanitize user inputs, accepting instances in createPrivateKey etc makes sense without having to expose the class or adding isXYZ.

[sam-github]

I like things when the API is the "right" way, the way we think it should be, but not so much when only part of the API is the right way, and other parts aren't, that just taxes my memory systems.

Ideally, none of the classes should have been exposed, but for now, I think we should expose the KeyObject base-class (just the base, as you say).

Sometime in the future, when we have a solid way for people do instanceof checks, either through passing through a sanitization function that returns identify if its already a key object or is*() functions, then we can deprecate all the classes at the same time and slowly get rid of them.

How's that for a plan?

[tniessen]

Ideally, none of the classes should have been exposed, but for now, I think we should expose the KeyObject base-class (just the base, as you say).

I understand that this might be appealing for reasons of consistency and simplicity. Just for my own understanding, does this actually solve any problems that wouldn't be solved by allowing to pass existing instances to create*Key?

Sometime in the future, when we have a solid way for people do instanceof checks, either through passing through a sanitization function that returns identify if its already a key object or is*() functions, then we can deprecate all the classes at the same time and slowly get rid of them.

I am still wondering whether people are actually doing checks like x instanceof crypto.Hash. I cannot think of any use cases, except checking argument types supplied by users.

[panva]

Ideally, none of the classes should have been exposed, but for now, I think we should expose the KeyObject base-class (just the base, as you say).

I understand that this might be appealing for reasons of consistency and simplicity. Just for my own understanding, does this actually solve any problems that wouldn't be solved by allowing to pass existing instances to create*Key?

While that would solve ONE of my uses, i can solve more by having access to the constructor or the is* helper. Passing every user input to create*Key can get costly.

There are now 2 PRs solving a real application problem based on two member recommendations. Please pick one ;)

[tniessen]

There are now 2 PRs solving a real application problem based on two member recommendations. Please pick one ;)

I am not convinced either of the PRs is a good idea, that's why I didn't "pick one" yet. I agree with @sam-github though, so if it's this or the other, I prefer exposing the class as a whole.

[tniessen]

I am marking this as blocked based on my and @sam-github's opinion and our preference towards exposing the class itself. @BridgeAR and @benjamingr, feel free to chime in.

Abstain

[BridgeAR]

I do not have a strong opinion about it. For consistency reasons, I'd go and expose the class but I very well understand why that's not ideal either. So I'll just abstain.

[sam-github]

Can we close this, then? Its functionally replaced by @panva 's other PR: #26438

[panva]

Indeed.