crypto: don't crash on unknown keyObject.asymmetricKeyType

[panva]

Fixes#26775

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]

I'd welcome hints and key material other than (ed/x25519 or ed/x448) for adding tests to this.

[sam-github]

Maybe take an unencrypted X488 key and damage the curve OID so it will always be invalid? Or perhaps that will refuse to import?

[mscdex]

I'm not sure if we should use 'unknown' or 'unsupported', I'd probably lean towards the latter since the type must be known (otherwise OpenSSL would probably throw an error), but node does not support/know about it yet.

[sam-github]

I guess flat-out removing the asymetricKeyType isn't an option? We are returning this string, but it doesn't appear to have any purpose in our API, am I missing something?

[mscdex]

Having the key type is useful, I don't think we should get rid of it.

[panva]

I guess flat-out removing the asyme,tricKeyType isn't an option?

No. The information we have about a key is scarse as-is.

So, 'unsupported' or 'unknown'? I'd also be fine with undefined.

[sam-github]

definitely not 'undefined', its not. 'unsupported' was a good choice if we keep it.

@mscdex What use is the asymetricKeyType? Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too. Not too push too hard, but the numeric equivalent in openssl can actually be used (must be used) to do things, not so in our API. I know we hate to take things out, but this is the time to pull it if its problematic, while its still only in 11.x.

@panva I'm super sympathetic to the lack of key data, and support that data being made available, but keeping this around doesn't get you that. You still don't know what kind of DH or DSA key it is, what the EC curve is, etc, etc.

[panva]

@sam-github the API is already in use by libraries today. To begin with, it's a decent way to ignore keys that are not useful in any context, e.g. jose library having no use for DSA keys, keys that do not have asn1.js implementation yet, etc.

Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too.

Indeed it is. ed25519 will get passed to oneshot for EdDSA while rsa will not. ec will be used with createECDH while rsa will be used for RSA-OAEP to wrap/derive a CEK.

If this got removed we must consider a stable replacement. E.g. #26709, we didn't land that one because the information it provided was already delivered by asymmetricKeyType so i'm quite surprised for that to be on the cutting room floor now.

[mscdex]

What use is the asymetricKeyType? Its not accepted as input by any API, it can't be switched on to decide what API to pass a key too.

The key type may not be utilized within node core, but it's certainly helpful in userland when you're accepting arbitrary keys in your application/module.

[panva]

Maybe take an unencrypted X488 key and damage the curve OID so it will always be invalid? Or perhaps that will refuse to import?

It'll refuse to import.

Error: error:0609E09C:digital envelope routines:pkey_set_type:unsupported algorithm at createPrivateKey (internal/crypto/keys.js:323:10) 

Edit: tests added.

[sam-github]

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

[tniessen]

I am working on support for RSA-PSS so testing this might become a bit trickier soon.

[panva]

I am working on support for RSA-PSS so testing this might become a bit trickier soon.

I was hoping you'd miss that

[tniessen]

I am afraid a rebase is necessary.

[panva]

@tniessen done

[tniessen]

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

[BridgeAR]

Is there a reason why we do not just throw an error while running createPrivateKey()? That would be my expectation as a user.

[tniessen]

That's debatable, but certainly also a valid approach. The only upside I see of throwing is that people will notify us when we forget to add a key type. However, even if asymmetricKeyType returns 'unsupported', people might be able to use the key since OpenSSL supports it, node just doesn't recognize it. Ideally, this value is never returned, but we aren't quite there yet.

[BridgeAR]

@tniessen thanks for clarifying this. I guess in that case both approaches seem legit. What about adding a warning in case 'unsupported' is returned somewhere? That way we could tell users to notify us but the code itself will still work.

I personally have a slight preference for 'unknown' over 'unsupported'. If I would stumble upon this and would read unsupported, I'd have to look up in what way it's unsupported while unknown would be a indication that Node.js can not tell what type it is without having to look up anything.

[panva]

@BridgeAR, we went back and forth on the appropriate value to use. As this crash makes the use of KeyObjects impossible with third-party inputs, could we move forward with it as-is? The ✅ are there.

[tniessen]

Personally, I prefer 'unsupported'. I don't think we should warn or throw. If we don't recognize the key type, the user might be linking against a different crypto lib or OpenSSL version, we should try to be flexible there IMO.

[sam-github]

The thing about 'unsupported' is that I think it is supported, in the the key can be re-encoded as PEM, and used (possibly) in sign/verify, etc. Isn't that the case?

How about we just encode the numeric key id as a string? default: .asymetricKeyType = sprintf("key-type-%d', nid) (obviously pseudo-code). So, then the nid is known to the user, and if their code depends on the specific key type, and something isn't working, we'll get a useful bug report, and they will have an idea what is wrong.

Which argues for a .nid property on keys.... and maybe a .oid, if its guaranteed by the openssl API that every EVP_KEY_X has a nid and OID.

[panva]

Here’s the thing. If we return a constructed string it might look intentional and someone might start using it thinking it’s final.

But then we add a string for the type later and it’ll be breaking for the developer.

[tniessen]

Here’s the thing. If we return a constructed string it might look intentional and someone might start using it thinking it’s final.

I share this concern. We could choose something inbetween such as ${oid}-unsupported.

Which argues for a .nid property on keys.... and maybe a .oid, if its guaranteed by the openssl API that every EVP_KEY_X has a nid and OID.

Ref #26709. I think we should avoid using NIDs since they are specific to OpenSSL IIRC. Each key type must have an associated OID, though, since OIDs are used to identify key types when encoded as PKCS#8 / DER etc.

[sam-github]

I share this concern. We could choose something inbetween such as ${oid}-unsupported.

Its in-between, but has the exact same problem as key-id-$(nid).

Anything, other than an actual string specific to the key id will eventually change, and be semver-major when we start to support the numeric type. This argues strongly against returning any string for the .asymetricKeyType if we don't recognize the numeric key type. Perhaps the property should be defined, but with an actual value of undefined? Changing that to a string when we add support for a new EVP key ID should not be semver-major.

(EDIT: I rewrote the above slightly, sorry, I think the old text was confusing).

I can think of a couple solutions:

1. add a property that has an OID valud (or nid, it doesn't matter to me that its an openssl API artifact, we only support openssl), because they will always exist, and in order to not have people be typing OIDs into their code, add crypto.constants.KEY_ID_RSA, crypto.constants.KEY_ID_EC, etc., that have an OID as a value, and tell people to start using those constants to check key types, not .asymetricKeyType. Maybe .asymetricKeyType can be deprecated.

2. Start throwing errors. If someone gets the error thrown, they can make a feature request for support.

3. Add every one of the existing key ids, along with a node.js string equivalent, to the switch statement of key types, and stop calling them "supported key types" in our docs, just "key types". Do this along with 2. (which should no longer happen, unless there is an openssl update that adds a key type and we didn't notice).

[sam-github]

May I change this PR to undefined instead then?, as a followup you’ll be able to add all switch statements.

Sounds good to me, and also to @tniessen , so yes, please.

[tniessen]

Ref #26996

[tniessen]

Ping @nodejs/crypto

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[tniessen]

Landed in 7c1fc93.