crypto: always accept private keys as public keys by tniessen · Pull Request #25217 · nodejs/node · GitHub

Some APIs already accept private keys instead of public keys. This changes all relevant crypto APIs to do so.

As a nice side effect, this also allows to derive public keys from private keys using the new KeyObject API.

I am not 100% sure whether this is the best way to go, so I would love opinions from @nodejs/crypto or other people! Allowing private keys to be passed to verify makes sense in my opinion, but what if the user passes a public and a private key (both in the same PEM string) to createPublicKey? With this change, the public key would be used, but we should make sure to be on the same page about this.

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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2061/pipeline

Technical contents of the PR LGTM % a rebase.

what if the user passes a public and a private key (both in the same PEM string) to createPublicKey? With this change, the public key would be used

Personally, I'd expect it to use the private key as that's the ultimate source of truth.

Some APIs already accept private keys instead of public keys. This changes all relevant crypto APIs to do so.

one tiny suggestion on the doc text, but otherwise LGTM

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

Landed in ae2d1f0

Some APIs already accept private keys instead of public keys. This changes all relevant crypto APIs to do so. PR-URL: #25217 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Some APIs already accept private keys instead of public keys. This changes all relevant crypto APIs to do so. PR-URL: nodejs#25217 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Hi @tniessen

Not sure if this is related to the nightly node build i'm using or a bug with the implementation, i tried crypto.createPublicKey with various inputs.

In essence if i pass a private key loaded from the filesystem it works fine, but when i pass an existing instance of PrivateKeyObject it 💥

> ko=crypto.createPrivateKey(fs.readFileSync('ec.key'))
PrivateKeyObject { [Symbol(kKeyType)]: 'private' }
> crypto.createPublicKey(fs.readFileSync('ec.key'))
PublicKeyObject { [Symbol(kKeyType)]: 'public' }
> crypto.createPublicKey(ko)
/Users/panva/node-nightly/bin/node[66757]: ../src/node_crypto.cc:3168:node::crypto::ManagedEVPPKey node::crypto::GetPublicOrPrivateKeyFromJs(const FunctionCallbackInfo<v8::Value> &, unsigned int *, bool): Assertion `args[*offset]->IsObject() && allow_key_object' failed.
 1: 0x10006310d node::Abort() [/Users/panva/node-nightly/bin/node]
 2: 0x10006303f node::PrintErrorString(char const*, ...) [/Users/panva/node-nightly/bin/node]
 3: 0x100114f0b node::crypto::GetPublicOrPrivateKeyFromJs(v8::FunctionCallbackInfo<v8::Value> const&, unsigned int*, bool) [/Users/panva/node-nightly/bin/node]
 4: 0x1001142e8 node::crypto::KeyObject::Init(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/panva/node-nightly/bin/node]
 5: 0x1002240dd v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/Users/panva/node-nightly/bin/node]
 6: 0x100223696 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/panva/node-nightly/bin/node]
 7: 0x100222d90 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/panva/node-nightly/bin/node]
 8: 0x100e1ee6e v8_Default_embedded_blob_ [/Users/panva/node-nightly/bin/node]
 9: 0x2c2c9f40816e 
10: 0x2c2c9f40816e 
11: 0x100d8e2a3 v8_Default_embedded_blob_ [/Users/panva/node-nightly/bin/node]

@panva Mhhh weird, that should throw in the JS layer. I'll have a look at it tomorrow, I wanted to add support for that operation anyway.

Notable Changes * compression / zlib: * Added brotli support (Anna Henningsen and Zach Vacura) nodejs#24938 * console: * Added `inspectOptions` option (Ruben Bridgewater) nodejs#24978 * crypto: * Always accept private keys as public keys (Tobias Nießen) nodejs#25217 * deps: * Upgrade npm to v6.5.0 (Jordan Harband) nodejs#25234 * fs: * Use internalBinding('fs') internally instead of process.binding('fs') (Masashi Hirano) nodejs#22478 * http(s): * Support overriding http\\s.globalAgent (Roy Sommer) nodejs#25170 * util: * Inspect ArrayBuffers contents closely (Ruben Bridgewater) nodejs#25006 * worker: * Expose workers by default and remove `--experimental-worker` flag (Anna Henningsen) nodejs#25361 PR-URL: nodejs#25537

Thank you for reporting this, @panva! Fixed in #25562.

no problem, @tniessen the fix suggests that this operation is not intended, is that not intended "yet" and it'll be supported in the future?

Notable Changes * compression / zlib: * Added brotli support (Anna Henningsen and Zach Vacura) #24938 * console: * Added `inspectOptions` option (Ruben Bridgewater) #24978 * crypto: * Always accept private keys as public keys (Tobias Nießen) #25217 * deps: * Upgrade npm to v6.5.0 (Jordan Harband) #25234 * fs: * Use internalBinding('fs') internally instead of process.binding('fs') (Masashi Hirano) #22478 * http(s): * Support overriding http\\s.globalAgent (Roy Sommer) #25170 * util: * Inspect ArrayBuffers contents closely (Ruben Bridgewater) #25006 * worker: * Expose workers by default and remove `--experimental-worker` flag (Anna Henningsen) #25361 PR-URL: #25537

@panva It is not intended yet, I'm trying to improve the API in small incremental changes, allowing to derive public key objects from private key objects is one of those.

Great, thanks for the heads up! I appreciate the changes you're making, what you call small incremental changes are huge leaps in nodejs crypto usability.

tniessen requested a review from sam-github December 25, 2018 13:30

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Dec 25, 2018

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 26, 2018

addaleax approved these changes Dec 26, 2018

View reviewed changes

Comment thread doc/api/crypto.md Outdated

Comment thread doc/api/crypto.md Outdated

jasnell approved these changes Dec 27, 2018

View reviewed changes

bnoordhuis approved these changes Dec 28, 2018

View reviewed changes

crypto: always accept private keys as public keys

bd5e127

Some APIs already accept private keys instead of public keys. This changes all relevant crypto APIs to do so.

tniessen force-pushed the crypto-always-accept-private-key-as-public-key branch from c2d1597 to bd5e127 Compare January 3, 2019 19:39

tniessen added 2 commits January 3, 2019 20:42

fixup! crypto: always accept private keys as public keys

8f6040b

fixup! crypto: always accept private keys as public keys

6602a44

sam-github approved these changes Jan 3, 2019

View reviewed changes

Comment thread doc/api/crypto.md Outdated

fixup! fixup! crypto: always accept private keys as public keys

63dabd5

addaleax closed this Jan 7, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

tniessen mentioned this pull request Jan 18, 2019

crypto: fix key handle extraction #25562

Closed

3 tasks

akihikodaki mentioned this pull request Jan 18, 2019

Use a feature of Node v11.7.0 to extract a public key misskey-dev/misskey#3921

Closed

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

crypto: always accept private keys as public keys by tniessen · Pull Request #25217 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: always accept private keys as public keys - #25217

crypto: always accept private keys as public keys#25217
tniessen wants to merge 4 commits into
nodejs:masterfrom
tniessen:crypto-always-accept-private-key-as-public-key

tniessen commented Dec 25, 2018

Uh oh!

nodejs-github-bot commented Dec 25, 2018

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Uh oh!

sam-github left a comment

Uh oh!

Uh oh!

tniessen commented Jan 7, 2019

Uh oh!

addaleax commented Jan 7, 2019

Uh oh!

panva commented Jan 15, 2019

Uh oh!

tniessen commented Jan 15, 2019

Uh oh!

tniessen commented Jan 18, 2019

Uh oh!

panva commented Jan 18, 2019

Uh oh!

tniessen commented Jan 18, 2019

Uh oh!

panva commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: always accept private keys as public keys by tniessen · Pull Request #25217 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: always accept private keys as public keys - #25217

crypto: always accept private keys as public keys#25217tniessen wants to merge 4 commits intonodejs:masternodejs/node:masterfrom tniessen:crypto-always-accept-private-key-as-public-keytniessen/node:crypto-always-accept-private-key-as-public-keyCopy head branch name to clipboard

Conversation

tniessen commented Dec 25, 2018

Checklist

Uh oh!

nodejs-github-bot commented Dec 25, 2018

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tniessen commented Jan 7, 2019

Uh oh!

addaleax commented Jan 7, 2019

Uh oh!

panva commented Jan 15, 2019

Uh oh!

tniessen commented Jan 15, 2019

Uh oh!

tniessen commented Jan 18, 2019

Uh oh!

panva commented Jan 18, 2019

Uh oh!

tniessen commented Jan 18, 2019

Uh oh!

panva commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: always accept private keys as public keys#25217
tniessen wants to merge 4 commits into
nodejs:masterfrom
tniessen:crypto-always-accept-private-key-as-public-key