Repository navigation
keyobject.asymmetricKeyType crashes on C++ assertion #26775
Description
A followup discussion to #26319
We're still crashing here on any unhandled key. e.g:
-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VuBCIEILD/13Y5R/tmcCjZVSooIcpfGvZxf+qt6dMu5FYaOC1a
-----END PRIVATE KEY-----
This makes the API rather unstable since end-users can force key material that will crash the process. Developers have no way to detect this other than parsing the key material before sending it to create(private|public)Key, kinda killing the KeyObject point.
The sane thing here would be return "unsupported"/"unknown" or to not instantiate the KeyObject at all and throw in JS instead.
Returning unsupported is enough for libraries using KeyObjects to check if they want to accept the key.
/cc @tniessen @sam-github @bnoordhuis what do you think?
Activity
tniessen commentedon Mar 19, 2019
You are right... Maybe we shouldn't allow such keys to be created in the first place? There are some OIDs we should support and I intend to work on that soon (currently working on
.fields), but I don't know whether ECDH keys are useful in this context.tniessen commentedon Mar 19, 2019
Or, as you said, we could allow to construct them as long as OpenSSL supports it and return
unsupported... This did not sound like a good idea to me at first, but now that I'm thinking about it, it might just work. Initially, I thought it would be easy to enumerate all key types supported by OpenSSL, but in the end, we might keep running into such problems.panva commentedon Mar 19, 2019
they are, see:
panva commentedon Mar 19, 2019
Not instantiating i don't think i can quite pull off myself, returning
unsupportedi can open a fix for.sam-github commentedon Mar 19, 2019
or throwing an error?
panva commentedon Mar 19, 2019
Throwing an error on a getter of an existing KeyObject instance sounds rather awkward to me.
sam-github commentedon Mar 19, 2019
I didn't read carefully enough, ignore that.
'unknown'as a default sounds reasonable.Is there any reason we shouldn't also add all the currently defined values in evp.h to the switch?
tniessen commentedon Mar 19, 2019
@sam-github That sounds reasonable. (A couple of days ago, I desperately tried to get node to accept an
EVP_PKEY_RSA2key without success...)crypto: don't crash on unknown asymmetricKeyType
crypto: don't crash on unknown asymmetricKeyType