Node: keyObject.asymmetricKeySize returns incorrect values for EC and Ed keys

[panva]

The API added in #26387 (landed in 4895927) returns incorrect key sizes for Ed25519, Ed448 and all EC keys.

It's using EVP_PKEY_size which according to this doc returns the maximum size of a signature in bytes, not the key size, which is fine for RSA and DSA keys but not for the aforementioned keys.

• Version: v12.0.0-nightly20190313e6fa50e953

Code repro

const crypto = require('crypto')

let EC_P521 = crypto.generateKeyPairSync('ec', { namedCurve: 'P-521' })

EC_P521 = {
  private: EC_P521.privateKey,
  public: EC_P521.publicKey
}

let EC_P384 = crypto.generateKeyPairSync('ec', { namedCurve: 'P-384' })

EC_P384 = {
  private: EC_P384.privateKey,
  public: EC_P384.publicKey
}

let EC_P256 = crypto.generateKeyPairSync('ec', { namedCurve: 'P-256' })

EC_P256 = {
  private: EC_P256.privateKey,
  public: EC_P256.publicKey
}

const Ed25519 = {
  private: crypto.createPrivateKey(`-----BEGIN PRIVATE KEY-----\nMC4CAQAwBQYDK2VwBCIEIHXLsXm1lsq5HtyqJwQyFmpfEluuf0KOqP6DqMgGxxDL\n-----END PRIVATE KEY-----`),
  public: crypto.createPublicKey(`-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEAEXRYV3v5ucrHVR3mKqyPXxXqU34lASwc7Y7MoOvaqcs=\n-----END PUBLIC KEY-----`)
}

const Ed448 = {
  private: crypto.createPrivateKey(`-----BEGIN PRIVATE KEY-----\nMEcCAQAwBQYDK2VxBDsEObxytD95dGN3Hxk7kVk+Lig1rGYTRr3YdaHjRog++Sgk\nQD7KwKmxroBURtkE2N0JbQ3ctdrpGRB5DQ==\n-----END PRIVATE KEY-----`),
  public: crypto.createPublicKey(`-----BEGIN PUBLIC KEY-----\nMEMwBQYDK2VxAzoAIESY3jnpGdB5UVJDCznrv0vmBFIzgSMu+gafsbCX1rFtsJwR\nM6XUDQiEY7dk6rmm/Fktyawna5EA\n-----END PUBLIC KEY-----`)
}

console.log('Ed25519.private.asymmetricKeyType', Ed25519.private.asymmetricKeyType)
console.log('Ed25519.private.asymmetricKeySize, expected 32, got %i', Ed25519.private.asymmetricKeySize)
console.log('Ed25519.public.asymmetricKeyType', Ed25519.public.asymmetricKeyType)
console.log('Ed25519.public.asymmetricKeySize, expected 32, got %i', Ed25519.public.asymmetricKeySize)

console.log('\n')

console.log('Ed448.private.asymmetricKeyType', Ed448.private.asymmetricKeyType)
console.log('Ed448.private.asymmetricKeySize, expected 57, got %i', Ed448.private.asymmetricKeySize)
console.log('Ed448.public.asymmetricKeyType', Ed448.public.asymmetricKeyType)
console.log('Ed448.public.asymmetricKeySize, expected 57, got %i', Ed448.public.asymmetricKeySize)

console.log('\n')

console.log('EC_P521.private.asymmetricKeyType', EC_P521.private.asymmetricKeyType)
console.log('EC_P521.private.asymmetricKeySize, expected 💥 521/8, got %i', EC_P521.private.asymmetricKeySize)
console.log('EC_P521.public.asymmetricKeyType', EC_P521.public.asymmetricKeyType)
console.log('EC_P521.public.asymmetricKeySize, expected 💥 521/8, got %i', EC_P521.public.asymmetricKeySize)

console.log('\n')

console.log('EC_P384.private.asymmetricKeyType', EC_P384.private.asymmetricKeyType)
console.log('EC_P384.private.asymmetricKeySize, expected 48, got %i', EC_P384.private.asymmetricKeySize)
console.log('EC_P384.public.asymmetricKeyType', EC_P384.public.asymmetricKeyType)
console.log('EC_P384.public.asymmetricKeySize, expected 48, got %i', EC_P384.public.asymmetricKeySize)

console.log('\n')

console.log('EC_P256.private.asymmetricKeyType', EC_P256.private.asymmetricKeyType)
console.log('EC_P256.private.asymmetricKeySize, expected 32, got %i', EC_P256.private.asymmetricKeySize)
console.log('EC_P256.public.asymmetricKeyType', EC_P256.public.asymmetricKeyType)
console.log('EC_P256.public.asymmetricKeySize, expected 32, got %i', EC_P256.public.asymmetricKeySize)

Ed25519.private.asymmetricKeySize, expected 32, got 64
Ed25519.public.asymmetricKeySize, expected 32, got 64

Ed448.private.asymmetricKeySize, expected 57, got 114
Ed448.public.asymmetricKeySize, expected 57, got 114

EC_P521.private.asymmetricKeySize, expected 💥 521/8, got 141
EC_P521.public.asymmetricKeySize, expected 💥 521/8, got 141

EC_P384.private.asymmetricKeySize, expected 48, got 104
EC_P384.public.asymmetricKeySize, expected 48, got 104

EC_P256.private.asymmetricKeySize, expected 32, got 72
EC_P256.public.asymmetricKeySize, expected 32, got 72

The EC Key lengths i'm not 100% sure about, see DSS Table D-1: Bit Lengths of the Underlying Fields of the Recommended Curves on page 88

[paroga]

Maybe we should just rename the property and/or change the documentation, to match the returned value

[panva]

@paroga I find the property useful and my heart skipped a beat when i found it (given it would return correct) in the nightly build, it's primary use for me would be to figure out the used EC curve and therefore know which algorithms are available for this given key. I could omit having to parse the key myself to figure the curve out.

For RSA i actually don't need it because all RS sign/verify algorithms work with key of any size and for Ed keys i don't need it either because the asymmetricKeyType gives me all i need.

As for renaming, is there any use for it as-is? I don't think so.

[paroga]

My primary use is similar: I could skip the parsing of the key myself too. The only information I need is the size of the resulting signature, which is what EVP_PKEY_size is made for IMHO.
What about renaming it to asymetricSignatureSize?

[panva]

/cc @tniessen the mastermind behind KeyObject

[tniessen]

This is the first time I see the PR that introduced the property, I was not aware of it. Maybe we should make pings to @nodejs/crypto mandatory...

Said PR exposed one property of certain keys. We (especially @sam-github and I) have had multiple discussions about exposing information about keys, possibly with some kind of compatibility to existing key information provided using the TLS APIs, and I think exposing one field after the other is not a good way to approach the problem. The "key size" only makes sense for certain types of keys (including RSA), and even amongst those, the key size has completely different meanings. The PR explains the property like this:

For asymmetric keys, this property represents the size of the embedded key in
bytes.

"the size of the embedded key in bytes" sounds unusual to me. For DSA, for example, there are different ways to measure the "size" of the key IIRC. For other key types, measuring the size in bytes might not make sense at all, either because there is no useful measure of the "size" of the key or because it is not a multiple of eight bits.

Other properties such as the "maximum signature size" make sense for all kinds of keys that support signing, however, that value may still deviate from actual signature sizes.

[paroga]

What about just renaming it to asymetricMaximumSignatureSize instead of reverting it?

[tniessen]

Personally, I just want to make sure that we don't add faulty APIs to active release lines, and reverting a change is often the fastest way to ensure that.

[omsmith]

Just to chime in, I think what I would appreciate (and I expect perhaps @panva for his use-case, since I believe it's the same as mine) is the OID.

That use-case being for JWS/JWA. When processing an RS256/384/512 signature, being able to check it is an RSA key. When processing ES256, checking it is an secp256v1 key (which has a unique OID) etc.

[panva]

Indeed. +1. Also the x, y, d, and all key components in essentially any format so that i can expose a JWK. But @tniessen knows that already;)

[tniessen]

Yes, all of these suggestions are great and I would love to work on them, but we should design the API first. I briefly discussed a fields or components property on KeyObjects with @sam-github. Said property would then contain the components of the key. This would not include properties such as the OID, though. It might make sense to expose that on the key object itself, I like that idea :)

[sam-github]

I agree with reverting it for now. From https://www.openssl.org/docs/man1.1.1/man3/EVP_PKEY_size.html, it looks like this size, if useful, would make more sense exposed as Sign.maximumSize(privateKey).

That is an unfortunately named OpenSSL API. I suspect it dates from the RSA-only days, because key size and signature size are the same for RSA.