crypto: add keyObject.params for asymmetric keys #30045

tniessen · 2019-10-20T17:13:32Z

This API exposes key parameters. It is conceptually different from the previously discussed keyObject.fields property since it does not give access to information that could compromise the security of the key, and the obtained information cannot be used to uniquely identify a key.

The intended purpose is to determine "security properties" of keys, e.g. to generate a new key pair with the same parameters, or to decide whether a key is secure enough.

I have not implemented the publicExponent property yet, mostly because I am not sure whether to use a number or a BigInt. In practice, most (all?) public exponents will fit into 32 bits and thus easily into Number.MAX_SAFE_INTEGER, and generateKeyPair currently only accepts safe integers, too. However, in theory, public exponents can be almost arbitrarily long, even though it can introduce efficiency and security concerns. I would like to hear opinions from @nodejs/crypto (or others).

Refs: nodejs/webcrypto#16

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

This API exposes key parameters. It is conceptually different from the previously discussed keyObject.fields property since it does not give access to information that could compromise the security of the key, and the obtained information cannot be used to uniquely identify a key. Refs: nodejs/webcrypto#16

panva · 2019-10-21T09:01:52Z

@tniessen

I'm wondering about why this approach would be preferred over working out .fields export and subsequent import roundtrip, which solves more use cases, such as easier and dependency-free JWK import.

It is conceptually different from the previously discussed keyObject.fields property since it does not give access to information that could compromise the security of the key, and the obtained information cannot be used to uniquely identify a key.

Given that all keys have an .export() which may be parsed to get that info, how would .fields expose anything that's not exposed already?

bnoordhuis

I have not implemented the publicExponent property yet, mostly because I am not sure whether to use a number or a BigInt.

The precedence here is tlssocket.getPeerCertificate() which returns it as a hex string.

bnoordhuis · 2019-10-21T10:21:32Z

src/node_crypto.cc

+    // TODO(tniessen): This should really be EVP_PKEY_get0_RSA, but OpenSSL does
+    // not support that for RSA-PSS.
+    rsa = reinterpret_cast<RSA*>(EVP_PKEY_get0(pkey));
+    SetParam(env(), params, "modulusLength",


Suggestion: add the string to PER_ISOLATE_STRING_PROPERTIES in env.h. Ditto the other strings.

src/node_crypto.cc

tniessen · 2019-10-21T13:06:00Z

I'm wondering about why this approach would be preferred over working out .fields export and subsequent import roundtrip, which solves more use cases, such as easier and dependency-free JWK import.

I think it is possible to see .fields and .params as related, but disjoint sets of properties. For example, where .params has a modulusLength property, .fields could have a modulus property, but modulusLength is not really a field of the key.

.fields seems to be far more difficult to design and will most likely also be difficult to use. And so far, I didn't find many use cases for .fields that would not be solved by .params, apart from your specific JWK use case.

Given that all keys have an .export() which may be parsed to get that info, how would .fields expose anything that's not exposed already?

.fields does not expose anything new, but it does expose critical information, which is exactly why you could never write the value of .fields to a log file, show it to a user, pass it to an untrusted function, or display it as part of your application. Whereas .params can be logged, used, passed and displayed without any security issues.

panva · 2019-10-21T13:42:38Z

I think it is possible to see .fields and .params as related, but disjoint sets of properties. For example, where .params has a modulusLength property, .fields could have a modulus property, but modulusLength is not really a field of the key.

Fair enough, i can see how this method can be labeled as "log safe" / not containing private information.

.fields seems to be far more difficult to design and will most likely also be difficult to use. And so far, I didn't find many use cases for .fields that would not be solved by .params, apart from your specific JWK use case.

Other than the philosophical discussion about types and property names - what's the difficult part and how could i help to move it forward? PEM, DER and JWK are all treated as first-class under webcrypto's CryptoKey and I only wish to see the same with KeyObject one day.

doc/api/crypto.md

cjihrig · 2019-10-23T01:53:10Z

lib/internal/crypto/keys.js


 class AsymmetricKeyObject extends KeyObject {
  get asymmetricKeyType() {
    return this[kAsymmetricKeyType] ||
           (this[kAsymmetricKeyType] = this[kHandle].getAsymmetricKeyType());
  }
+
+  get params() {
+    return this[kParams] ||


Nit, but could you rewrite this as:

if (this[kParams] === undefined) this[kParams] = this[kHandle].getAsymmetricParams(); return this[kParams];

I was going to suggest something similar at first but it's locally consistent, get asymmetricKeyType() uses the same style.

BridgeAR · 2020-01-02T16:11:57Z

@tniessen is there still something to do here, since this already got two LGs?

tniessen · 2020-01-02T16:51:01Z

Yes, I still need to implement some properties. I am working on a different PR to fix a few crypto issues and need to make sure these two are compatible. Sorry about the delay.

willclarktech · 2020-04-20T16:56:45Z

.fields seems to be far more difficult to design and will most likely also be difficult to use. And so far, I didn't find many use cases for .fields that would not be solved by .params, apart from your specific JWK use case.

I have a potential use case for exposing .fields. I'm trying to implement a private set intersection algorithm based on RSA keys, but I need to do two things which don't seem to be supported currently by Node's crypto:

Sign a number directly (i.e. without hashing it first): I can easily write this function to avoid using a crypto.Sign object, but not without accessing the private key fields.
Perform some modular arithmetic using the public key fields.

Currently I think my best option is to export the keys and parse the DER/PEM, which is a bit of a headache.

panva · 2020-10-14T15:39:29Z

@tniessen is it possible to pick this up? Especially getting the namedCurve out of an ec type KeyObject is a great optimization - i don't have to export and parse the DER to figure out what curve a key is...

tniessen · 2020-10-16T12:26:52Z

@panva In theory, sure, I just don't know if this makes a whole lot of sense now that WebCrypto is in core. What do you think?

panva · 2020-10-16T12:47:32Z

@panva In theory, sure, I just don't know if this makes a whole lot of sense now that WebCrypto is in core. What do you think?

I think @jasnell will concur, that WebCrypto API is not meant as a replacement for the much more flexible and performant crypto module. I think we should not stop innovating it. I'm still looking forward to landing both .params and eventually even .fields KeyObject properties.

WebCrypto API helps in writing isomorphic modules for different runtimes but it comes at a huge performance cost and inflexibility especially because of its CryptoKey abstraction that closely ties the key with the algorithm it's intended to be used for. Generally, when talking to developers they see Web Crypto API as painful to use, and i concur there too - having just ported jose to be isomorphic (unreleased and unpublished private branch for now) it puts a lot of restrains on the API one can offer, restrains I didn't have when considering only crypto to be the runtime of choice.

I'm not sure how likely WebCrypto API is to be backported to lts/12 and lts/14 but .params and .fields should be an easy one with large impact (for my modules at least).

I would still choose to use crypto in libuv threads over WebCrypto when given the choice.

jasnell · 2020-10-16T12:50:50Z

I think @jasnell will concur, that WebCrypto API is not meant as a replacement for the much more flexible and performant crypto module. I think we should not stop innovating it. I'm still looking forward to landing both .params and eventually even .fields KeyObject properties.

I 100% agree. The WebCrypto API is not meant to replace the existing crypto system.

This API exposes key details. It is conceptually different from the previously discussed keyObject.fields property since it does not give access to information that could compromise the security of the key, and the obtained information cannot be used to uniquely identify a key. The intended purpose is to determine "security properties" of keys, e.g. to generate a new key pair with the same parameters, or to decide whether a key is secure enough. closes nodejs#30045

This API exposes key details. It is conceptually different from the previously discussed keyObject.fields property since it does not give access to information that could compromise the security of the key, and the obtained information cannot be used to uniquely identify a key. The intended purpose is to determine "security properties" of keys, e.g. to generate a new key pair with the same parameters, or to decide whether a key is secure enough. closes #30045 PR-URL: #36188 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 Oct 20, 2019

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 Oct 20, 2019

tniessen requested a review from bnoordhuis October 20, 2019 17:14

tniessen mentioned this pull request Oct 20, 2019

Imported RSA keys should have modulusLength and publicExponent nodejs/webcrypto#16

Closed

bnoordhuis reviewed Oct 21, 2019

View reviewed changes

fixup! crypto: add keyObject.params for asymmetric keys

ca028d9

bnoordhuis approved these changes Oct 22, 2019

View reviewed changes

cjihrig approved these changes Oct 23, 2019

View reviewed changes

ad-m mentioned this pull request Oct 28, 2019

Drop node-rsa & node-forge hyperonecom/h1-cli#460

Open

fixup! crypto: add keyObject.params for asymmetric keys

bfdddd0

tniessen mentioned this pull request Jan 4, 2020

Feature request: Support create public key from components method? #24471

Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

panva mentioned this pull request Nov 20, 2020

crypto: add keyObject.asymmetricKeyDetails for asymmetric keys #36188

Closed

4 tasks

panva closed this in 1772ae7 Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: add keyObject.params for asymmetric keys #30045

crypto: add keyObject.params for asymmetric keys #30045

tniessen commented Oct 20, 2019

panva commented Oct 21, 2019 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Oct 21, 2019

tniessen commented Oct 21, 2019

panva commented Oct 21, 2019

cjihrig Oct 23, 2019

bnoordhuis Oct 23, 2019

BridgeAR commented Jan 2, 2020

tniessen commented Jan 2, 2020

willclarktech commented Apr 20, 2020

panva commented Oct 14, 2020

tniessen commented Oct 16, 2020

panva commented Oct 16, 2020

jasnell commented Oct 16, 2020

crypto: add keyObject.params for asymmetric keys #30045

crypto: add keyObject.params for asymmetric keys #30045

Conversation

tniessen commented Oct 20, 2019

Checklist

panva commented Oct 21, 2019 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 21, 2019

Choose a reason for hiding this comment

tniessen commented Oct 21, 2019

panva commented Oct 21, 2019

cjihrig Oct 23, 2019

Choose a reason for hiding this comment

bnoordhuis Oct 23, 2019

Choose a reason for hiding this comment

BridgeAR commented Jan 2, 2020

tniessen commented Jan 2, 2020

willclarktech commented Apr 20, 2020

panva commented Oct 14, 2020

tniessen commented Oct 16, 2020

panva commented Oct 16, 2020

jasnell commented Oct 16, 2020

panva commented Oct 21, 2019 •

edited

Loading