crypto: add keyObject.asymmetricKeyDetails for asymmetric keys by panva · Pull Request #36188 · nodejs/node

panva · 2020-11-20T12:07:33Z

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.

This replaces and closes #30045

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

nodejs-github-bot · 2020-11-20T12:07:38Z

Review requested:

@nodejs/crypto
@nodejs/quic

src/crypto/crypto_keys.cc

lib/internal/crypto/keys.js

tniessen · 2020-11-20T14:15:23Z

This seems to be a replacement for #30045 (which I'm fine with, I barely had time for any OSS recently), but it doesn't solve the problems mentioned there. For example, publicExponent is not guaranteed to fit into a 53-bit integer.

panva · 2020-11-20T14:22:02Z

But it doesn't solve the problems mentioned there. For example, publicExponent is not guaranteed to fit into a 53-bit integer.

It uses the same internals as key.algorithm.publicExponent of crypto.webcrypto.CryptoKey. So it will be undefined if it doesn't fit.

The alternative would be to return the same way as webcrypto input for rsa keygen (a Uint8Array)

lib/internal/crypto/keys.js

doc/api/crypto.md

tniessen · 2020-11-28T18:30:50Z

It uses the same internals as key.algorithm.publicExponent of crypto.webcrypto.CryptoKey. So it will be undefined if it doesn't fit.

Admittedly, this is an unlikely scenario, but still one that could occur, and I don't think we should design an API that we won't be able to patch later to support larger exponents. I know we had countless discussions about how to represent big integers in JavaScript, and I think I am leaning towards using JS BigInt values. They are terrible for cryptography, but Buffers and strings and numbers aren't much better.

Let's face it, JavaScript is not great for cryptography. Secure memory management is virtually impossible, and BigInt arithmetic will absolutely introduce side-channel vulnerabilities when used on any sensitive data. However, BigInts are probably still the most JavaScriptish way of representing these numbers, even if WebCrypto deviated from this idea and defined BigInteger to be an Uint8Array.

panva · 2020-11-30T10:41:59Z

@tniessen it would be great if there was an effort to come to a consensus, i don't care one way or the other. The need for this API is clear and the topic was stalled for months. I took your PR as a reference and used the already existing internals after @jasnell's refactoring so that you don't have to worry about rebasing your PR and adjusting to refactored crypto.

panva · 2020-11-30T11:25:08Z

@tniessen 5cc9cf0 makes publicExponent a bigint, the other properties do not require this treatment. With that I believe we can move forward, ✅, and 🚢

mcollina

lgtm

tniessen

Great work @panva, and thank you for picking this up!

Are all properties compatible with generateKeyPair? Can users call generateKeyPairSync(key.asymmetricKeyType, key.asymmetricKeyDetails)?

test/parallel/test-crypto-keygen.js

doc/api/crypto.md

panva · 2020-11-30T12:21:47Z

@tniessen

Are all properties compatible with generateKeyPair?

Did not consider this a goal of this PR. But let's see.

RSA - it could if we stuck with number instead bigint, a follow up could be planned for making generateKeyPair accept a bigint.
EC - yes
DH - no, the underlying function returns empty, so missing primeLength or group or prime, could be planned for a followup. I checked with your original PR, it also did not handle DH key details.
ed25519, ed448, x25519, x448 - yes, as those do not have any options.

tniessen · 2020-11-30T13:29:50Z

Are all properties compatible with generateKeyPair?

Did not consider this a goal of this PR. But let's see.

It's not strictly necessary, but I think it would be great :)

RSA - it could if we stuck with number instead bigint, a follow up could be planned for making generateKeyPair accept a bigint.

I can do the latter.

DH - no, the underlying function returns empty, so missing primeLength or group or prime, could be planned for a followup. I checked with your original PR, it also did not handle DH key details.

I believe my PR is older than DH support :)

tniessen

Thank you @panva and I'm sorry I didn't have much time to work on this recently.

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

panva · 2021-01-14T10:30:09Z

@tniessen @jasnell @mcollina a re-✅ so we can land this finally 🙏

tniessen

LGTM apart from one concern about the return value of GetAsymmetricKeyDetail.

tniessen · 2021-01-14T10:44:07Z

lib/internal/crypto/util.js

+  }
+
+  return result;
+}


Suggestion, feel free to ignore: Out of curiosity, did you try benchmarking this versus

BigInt(`0x${Buffer.from(input).toString('hex')}`)

Or, if performance really is a concern in this code path,

BigInt(`0x${Buffer.from(input.buffer, input.byteOffset, input.byteLength).toString('hex')}`)

(I know that this is essentially the same as the existing function bigIntArrayToUnsignedInt above, I am just curious what the performance impact is.)

lib/internal/crypto/util.js

doc/api/crypto.md

src/crypto/crypto_keys.cc

mcollina

lgtm

nodejs-github-bot · 2021-01-14T11:44:05Z

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

mcollina

lgtm

panva · 2021-01-14T13:26:17Z

Landed in 1772ae7

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 <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

This patch changes GetRsaKeyDetail to work in older supported versions of OpenSSL. Refs: openssl/openssl#10217 PR-URL: #36877 Refs: #36188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 Nov 20, 2020

addaleax reviewed Nov 20, 2020

View reviewed changes

src/crypto/crypto_keys.cc Outdated Show resolved Hide resolved

panva commented Nov 20, 2020

View reviewed changes

lib/internal/crypto/keys.js Outdated Show resolved Hide resolved

panva force-pushed the keyobject-details branch from 04b4cd1 to 3438cca Compare November 20, 2020 12:21

jasnell reviewed Nov 20, 2020

View reviewed changes

lib/internal/crypto/keys.js Outdated Show resolved Hide resolved

jasnell reviewed Nov 20, 2020

View reviewed changes

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

panva mentioned this pull request Nov 20, 2020

crypto: add KeyObject.from and keyObject.export JWK format support #36203

Closed

4 tasks

panva requested review from addaleax and jasnell November 30, 2020 11:26

mcollina approved these changes Nov 30, 2020

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020

This comment has been minimized.

Sign in to view

tniessen reviewed Nov 30, 2020

View reviewed changes

test/parallel/test-crypto-keygen.js Outdated Show resolved Hide resolved

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

tniessen approved these changes Nov 30, 2020

View reviewed changes

Trott approved these changes Dec 2, 2020

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 2, 2020

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020

This comment has been minimized.

Sign in to view

panva added 2 commits January 13, 2021 15:34

fixup! crypto: add keyObject.asymmetricKeyDetails for asymmetric keys

95fc883

panva marked this pull request as ready for review January 13, 2021 14:35

panva force-pushed the keyobject-details branch from b5ba559 to 95fc883 Compare January 13, 2021 14:35

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2021

This comment has been minimized.

Sign in to view

panva removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2021

fixup! crypto: add keyObject.asymmetricKeyDetails for asymmetric keys

3559e94

This comment has been minimized.

Sign in to view

panva removed request for addaleax and jasnell January 14, 2021 09:26

panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021

tniessen reviewed Jan 14, 2021

View reviewed changes

mcollina approved these changes Jan 14, 2021

View reviewed changes

fixup! crypto: add keyObject.asymmetricKeyDetails for asymmetric keys

ec80c32

panva removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 14, 2021

tniessen approved these changes Jan 14, 2021

View reviewed changes

mcollina approved these changes Jan 14, 2021

View reviewed changes

panva closed this Jan 14, 2021

ruyadorno mentioned this pull request Jan 22, 2021

v15.7.0 proposal #37020

Merged

targos added dont-land-on-v12.x labels Aug 8, 2021

panva deleted the keyobject-details branch October 13, 2022 09:13

Uh oh!

Conversation

panva commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 20, 2020

Uh oh!

Uh oh!

Uh oh!

tniessen commented Nov 20, 2020

Uh oh!

panva commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tniessen commented Nov 28, 2020

Uh oh!

panva commented Nov 30, 2020

Uh oh!

panva commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

panva commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Nov 30, 2020

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

panva commented Jan 14, 2021

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 14, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

panva commented Jan 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

panva commented Nov 20, 2020 •

edited

Loading

panva commented Nov 20, 2020 •

edited

Loading

panva commented Nov 30, 2020 •

edited

Loading

panva commented Nov 30, 2020 •

edited

Loading