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

crypto: add API to retrieve key type OID #26709

Closed
wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

Some users suggested that it would be helpful to have access to the OID of KeyObjects. Luckily, OpenSSL does manage OIDs, so there is little to do on our side.

What do you think? Should we expose the OID? And if so, is this the right name for such a property?

OpenSSL also provides nice names for most OIDs, but as far as I know, these are not standardized and not guaranteed to exist for all OIDs, and there are sometimes two variants (a short and a long name), so I am not sure whether it would be a good idea to expose these. Maybe someone else knows more.


cc @nodejs/crypto, @panva, @omsmith.

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

@tniessen tniessen requested a review from sam-github March 17, 2019 00:49
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 17, 2019
@omsmith
Copy link
Contributor

omsmith commented Mar 17, 2019

but as far as I know, these are not standardized and not guaranteed to exist for all OIDs, and there are sometimes two variants

This was my thinking when making the suggestion. Using ec curves as an example, RFC 4492 calls out between 1 and 3 different names for a curve, depending on whether a standards body picked it up, plus whatever colloquial variations may exist (p256 vs P-256 vs NIST P-256). With that in mind, I don't know how Node chooses which name to expose in a props object.

Fortunately the OIDs largely (if not totally) satisfy the desirable information for ec/ed keys, but obviously rsa keys are still stuck with exposing key size separately.

Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add EC key tests too?

@tniessen
Copy link
Member Author

With that in mind, I don't know how Node chooses which name to expose in a props object.

We would have to rely on OpenSSL. Developers would still have to look up the specific strings, so I don't think the names would do us much good there.

Would it make sense to add EC key tests too?

Added, also for DSA.

@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 17, 2019
@panva
Copy link
Member

panva commented Mar 18, 2019

@tniessen I might've misunderstood this a bit, the OID returned for all EC keys is 1.2.840.10045.2.1, i expected one of 1.2.840.10045.3.1.7, 1.3.132.0.34 or 1.3.132.0.35. I understand my expectation was wrong now. The function does exactly what it says it will do, the key's OID 🤦‍♂️.

I'm guessing that the discussed .fields is what would bring that distinction for EC keys right?

@omsmith
Copy link
Contributor

omsmith commented Mar 18, 2019

Ah yes, that makes a lot of sense. The OID I was thinking of is buried a bit more.

PrivateKeyInfo -> privateKeyAlgorithm -> parameters(ECParameters) -> namedCurve

PublicKeyInfo -> algorithm -> parameters (ECParameters) -> namedCurve

Wasn't even thinking about the "ECC" algorithm OID that occurs earlier.

So, my fault.

This OID could be used in lieu of "rsa", "ec" and "DSA" provided by asymmetricKeyType, but it doesn't provide any of the new information I was thinking of / hoping for.

@tniessen
Copy link
Member Author

tniessen commented Mar 18, 2019

Right, all EC curves use the same OID for the key. The OID of the curve is part of the algorithm parameters which are not exposed as part of this PR.

After clearing this misunderstanding, is there any use for the OID? There certainly is if we decide to use a single asymmetricKeyType for ed25519 and ed448 (which I am not a huge fan of).

@panva
Copy link
Member

panva commented Mar 18, 2019

There certainly is if we decide to use a single asymmetricKeyType for ed25519 and ed448 (which I am not a huge fan of).

💯 Right, if ed25519 and ed448 become 'okp' or similar the OID helps. Maybe for RSA-PSS keys as well, but those i assume would be their own asymmetricKeyType.

NB. How would X25519 and X448 factor into this?

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is useful. The OID doesn't tell people anything they don't already know from the type. Also, OIDs are famous for being many to 1, the right OID to use when encoding a key might depend on the context. I can see this going in .fields, as part of the "bag of info" that OpenSSL API can return about a key (with no warranty from us on its usefulness), but I'm not sure its worth having at the top-level here. Its not the curve identifier some were hoping for. So, I guess I'm -0.5 unless someone speaks up with a use-case.

@sam-github
Copy link
Contributor

There certainly is if we decide to use a single asymmetricKeyType for ed25519 and ed448 (which I am not a huge fan of).

It doesn't sound like anyone is a fan of that.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panva you approved this, but I think that predates you finding out its not the OID you thought it was? I think I'll put a 'Request Changes' on this so it doesn't land by accident. It sounds like @tniessen @panva and I are all drifting negative on it.

@panva
Copy link
Member

panva commented Mar 18, 2019

Indeed. The only thing that would sway me is not having ed* as key type.

@tniessen
Copy link
Member Author

I am going to land #26554 with separate key types soon. Closing this based on the feedback and my own opinion that this does not help (right now). If we ever have key types with multiple OIDs, we can reopen this.

@panva
Copy link
Member

panva commented Mar 19, 2019

🤔 coming back to my question above about X25519 and X448, should those be also in place (#26774) then i think it doesn't make sense anymore for all to be separate types, maybe they should be type okp (octet key pair), then these would be the same type and have different OIDs

   id-X25519    OBJECT IDENTIFIER ::= { 1 3 101 110 }
   id-X448      OBJECT IDENTIFIER ::= { 1 3 101 111 }
   id-Ed25519   OBJECT IDENTIFIER ::= { 1 3 101 112 }
   id-Ed448     OBJECT IDENTIFIER ::= { 1 3 101 113 }

@tniessen
Copy link
Member Author

I guess the question is what the asymmetricKeyType is supposed to resemble. X25519 and Ed25519 are certainly not interchangeable.

they should be type okp (octet key pair)

I have only seen this name as part of JSON crypto specs, never in actual cryptography. If we use it, it means that the asymmetricKeyType no longer resembles which algorithm the key is intended for:

"OKP" (Octet Key Pair) ... for public key algorithms that use octet strings as private and public keys. ... Do not assume that there is an underlying elliptic curve, despite the existence of the "crv" and "x" parameters. (For instance, this key type could be extended to represent Diffie-Hellman (DH) algorithms based on hyperelliptic surfaces.)

This is fine for storing keys, but what information does "okp" give to a user who wishes to use the key?

@sam-github
Copy link
Contributor

I think discussing key types shouldn't be an abstract taxonomical discussion, it has to take into account what the key type will be used for, specifically, what crypto APIs will accept a key type, or return it.

In EVP, the key type is used when doing something that can't be done with an abstract API. Interacting with underlying key data is the main thing (generating key pairs, pulling key data out to encode it or display it to the user, creating key API objects from raw data, etc). We try to hide a lot of that in the node API, so until we expose the key .fields, it looks to me that generation is the main thing that requires knowlege of the key type. I think that the key type should map directly to the OpenSSL EVP_PKEY_XXX values, because when OpenSSL's non-abstract APIs need to be called, its based on the EVP_PKEY_XXX values.

It won't really help us to try to make our own key type that isn't 1-1 with OpenSSL key types, not as far as I can see. I'd have to be convinced be seeing a set of crypto APIs and a description of how they will work better with a key type that isn't 1-1 with OpenSSL's key types.

@panva
Copy link
Member

panva commented Mar 19, 2019

my $.02

prior knowledge of openssl APIs and types should not be a pre-requisite to using/understanding the node crypto API, same argument for #26626 - as a user i would expect to be able to use X25519 and X448 with createECDH, regardless of what the internals in openssl are. In the background I come from (JOSE), these are one key type (OKP) with different "crv" curve values.

I'm fine leaving it like this if it makes sense to others. Just wanted to offer an alternative POV.

@tniessen
Copy link
Member Author

JOSE and JWK are probably the only frameworks that use OKP, though, at least I have not been able to find any other frameworks. As I said before, OKP sounds reasonable to store and encode keys, but I don't see any value for a user who wants to use the key. What does asymmetricKeyType === 'okp' mean? It just means that it is any key that is encoded using an octet string, it does not even have to be related to curves. The user would have to check the type of the key using a different method anyway, so why not just use the asymmetricKeyType in the first place?

@panva
Copy link
Member

panva commented Mar 19, 2019

you have me convinced no worries :)

@sam-github
Copy link
Contributor

prior knowledge of openssl APIs and types should not be a pre-requisite to using/understanding the node crypto API

@panva To be clear, I agree with you on that.

But while I don't expect node.js users to know how the OpenSSL API works, we know (or should :-), and I think our API objects should reflect OpenSSL's, and the methods on them should reflect theirs (though probably simplified, and we can drop legacy cruft they themselves regret, and obviously our API would use js idioms, not C). Or, at least that should be the default API design.

Going our own way risks making an API that is gratuitously different, but not actually better, and that may require an ever deepening mountain of code to maintain as new capabilities are added to OpenSSL (EdDSA...) and we have to figure out how to add them to our own reimagining of a crypto API.

@panva
Copy link
Member

panva commented Apr 11, 2019

@sam-github, @tniessen in light of #27181, shall we reopen this?

@sam-github
Copy link
Contributor

I think we should reopen. @tniessen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants