crypto: add OID constants for EVP_PKEY_ types

[sam-github]

The existence of the constant can be used to check for support for the
key types, and the value is useful when encoding/decoding keys using
ASN.1.

---

Follow-on to #26786 (comment), these values might be useful if we move to key type constants, rather than strings like "rsa", etc.

Also, when PRs are backported from master to 11.x that use EVP key types that aren't supported by OpenSSL 1.1.0, it is trivial to ifdef out the C++ code that uses them. It's less trivial to deal with in the tests. Checking openssl version numbers explicitly is fragile (and hard if range checks are required). It will be easier to just test for existence of the define (effectively) from js. I didn't want to expose the NIDs, but the OIDs are useful values (I think).

@tniessen @panva Thoughts?

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

[nodejs-github-bot]

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3229

[panva]

@sam-github would the OID be then also available on a KeyObject instance? I can easily replace asymmetricKeyType with this in my code if the key instances also expose the value.

[sam-github]

@panva In this PR, no, but this is an incremental step towards it. This PR exposes the values that would be necessary for comparisons: if (keyObject.oid === crypto.constants.EVP_PKEY_X448) ...

[panva]

Sounds good.

[bnoordhuis]

This more or less duplicates OBJ_nid2ln(), doesn't it?

(Or maybe OBJ_nid2ln() followed by OBJ_nid2sn() if the former doesn't find anything.)

[sam-github]

No, check out nid_objs in deps/openssl/openssl/crypto/objects/obj_dat.h. I'd hoped the ln would be what I want, but it's the name used in the ASN.1 (as opposed to the sn which is the openssl abbreviated name). Its the "txt" I want, and that can only be gotten this way (AFAICT).

[bnoordhuis]

Oh, I get it now. Okay, objection withdrawn but can you use CHECK_NOT_NULL(obj)?

[bnoordhuis]

Wrap these in #ifdef EVP_PKEY_ED25519 etc.?

[sam-github]

master doesn't support anything earlier than openssl 1.1.1, and I didn't want to introduce the possibility that someone might try and get a superficially working build. This is constructed this way (without the #define EVPS(V) V( EVP_PKEY_DH) idiom so that when this backports to 11.x or 10.x, it will be trivial to put in the macros.

[tniessen]

I don't know. IIRC #26709 was closed because we did not see much use for OIDs as long as there was a 1:1 mapping of key type strings ('rsa') to OIDs, which there still is.

The existence of the constant can be used to check for support for the
key types, and the value is useful when encoding/decoding keys using
ASN.1.

True, but on the other hand, these constants won't make it much easier to implement ASN.1. So the only reason I can currently see is to determine which key types are supported, and I wonder whether this is the best way to do that.

Also, when PRs are backported from master to 11.x that use EVP key types that aren't supported by OpenSSL 1.1.0, it is trivial to ifdef out the C++ code that uses them. It's less trivial to deal with in the tests. Checking openssl version numbers explicitly is fragile (and hard if range checks are required). It will be easier to just test for existence of the define (effectively) from js. I didn't want to expose the NIDs, but the OIDs are useful values (I think).

I agree not to expose NIDs. For our own testing, we don't need to expose them to users, though, we could use internal constants.

[panva]

If this makes backporting easier why not (so long as it is accompanied by key.asymmetricKeyOid). But i think it's too late for v12 not to have asymmetricKeyType am i right?

[bnoordhuis]

LGTM with suggestions.

[bnoordhuis]

Oh, I get it now. Okay, objection withdrawn but can you use CHECK_NOT_NULL(obj)?

[bnoordhuis]

sizeof(buf)

Consider having the caller pass in the buffer: declare the param as char (*buf)[128], the caller then passes in &buf and can't get the size wrong. (See GetHumanReadableProcessName() for another example.)

[sam-github]

I rewrote as you suggested, but I don't use sizeof(buf) because it is size_t, and obj2txt() returns int, and comparison causes warnings on sign mismatch. I can static_cast<size_t>(OBJ_obj2txt()), but that doesn't seem clearly better.

[sam-github]

@panva we can't make breaking (semver-major) changes to 12.x after its released, but we can make additive (semver-minor) changes. I don't think any of these features under discussion are anything other than purely additive.

@tniessen Good reminder. Perhaps I'll make them properties of the internal crypto binding. They'll be able to be re-exported onto the public crypto module when we decide. That said, its not JUST our unit tests that might want to do feature testing of the OpenSSL its built against, and as APIs go, a couple properties on the constants object is pretty low commitment.

Maybe it's time to add keyObject.asymmetricKeyTypeOid now if we like it? Or would you rather a more sweeping introducing .fields, because it could be a keyObject.fields.keyTypeOid (and a .fields.curveOid, etc.)?

[tniessen]

Perhaps I'll make them properties of the internal crypto binding.

Definitely no objections to having this for internal unit tests as long as it isn't exposed to users.

as APIs go, a couple properties on the constants object is pretty low commitment.

That's true, except that the names are very OpenSSL-specific (EVP_PKEY_*). I know, we have had these discussions before, it's difficult to decide how close our API should be to OpenSSL. From a user's perspective, EVP_PKEY_ has no context in node. Personally, I like that OpenSSL shapes our API, but not that it also influences the names of our APIs where it does not make sense. (This is fine for internal constants, of course.)

That said, its not JUST our unit tests that might want to do feature testing of the OpenSSL its built against

I remember there was an issue about feature detection in Node.js. We are currently doing that half-heartedly at best. For each recently added feature, there might be a way to tell if it is there, but there certainly is no uniform way across builtin modules. If adding these constants is the best way to do it, then we probably should do it.

Feature detection aside, I guess it boils down to this question: Do users need to deal with OIDs, and are there any benefits of using OIDs over strings? For example, as far as I know, all EC curves have well-known names, so returning the name instead of the OID might be easier for users. I feel like adding numeric constants (or string representations of these) and using them as parameters or to check for equality is usually just a more difficult way of expressing a more abstract principle: key.asymmetricKeyTypeOid === crypto.constants.EVP_PKEY_RSA is just a fancy way of writing key.asymmetricKeyType === 'rsa'. I never really understood why we chose to add options such as padding: crypto.constants.RSA_PKCS1_OAEP_PADDING instead of padding: 'pkcs1-oaep'. But there might be enough valid use cases to add OIDs, I honestly don't know.

Maybe it's time to add keyObject.asymmetricKeyTypeOid now if we like it? Or would you rather a more sweeping introducing .fields, because it could be a keyObject.fields.keyTypeOid (and a .fields.curveOid, etc.)?

I think if we decide to add asymmetricKeyTypeOid, it should be on the KeyObject itself, just like asymmetricKeyType is.

[panva]

EC curves may not be the best example with curves being given different names by different standards bodies. OIDs are definitive in this case.

I think we shouldn’t be adding stuff that is ultimately part of .fields and could end up being duplicated.

[sam-github]

I'll do some more thinking about this, its clearly not ready to land as-is. I will consier just transforming this into the minimal change I need to the binding layer or constants so that feature detection is easy.