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: always accept private keys as public keys #25217

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@tniessen
Copy link
Member

tniessen commented Dec 25, 2018

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 tniessen requested a review from sam-github Dec 25, 2018

Show resolved Hide resolved doc/api/crypto.md Outdated
Show resolved Hide resolved doc/api/crypto.md Outdated
@bnoordhuis
Copy link
Member

bnoordhuis left a comment

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.

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

@tniessen tniessen force-pushed the tniessen:crypto-always-accept-private-key-as-public-key branch from c2d1597 to bd5e127 Jan 3, 2019

tniessen added some commits Jan 3, 2019

@sam-github
Copy link
Member

sam-github left a comment

one tiny suggestion on the doc text, but otherwise LGTM

Show resolved Hide resolved doc/api/crypto.md Outdated
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 7, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 7, 2019

Landed in ae2d1f0

@addaleax addaleax closed this Jan 7, 2019

addaleax added a commit that referenced this pull request Jan 7, 2019

crypto: always accept private keys as public keys
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>

addaleax added a commit that referenced this pull request Jan 9, 2019

crypto: always accept private keys as public keys
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>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

crypto: always accept private keys as public keys
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>
@panva

This comment has been minimized.

Copy link
Contributor

panva commented Jan 15, 2019

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]
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 15, 2019

@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.

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

PR-URL: nodejs#25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

@tniessen tniessen referenced this pull request Jan 18, 2019

Closed

crypto: fix key handle extraction #25562

3 of 3 tasks complete
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 18, 2019

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

@panva

This comment has been minimized.

Copy link
Contributor

panva commented Jan 18, 2019

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?

BridgeAR added a commit that referenced this pull request Jan 18, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Jan 18, 2019

@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.

@panva

This comment has been minimized.

Copy link
Contributor

panva commented Jan 18, 2019

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.

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment