FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: add generatePrime/checkPrime by jasnell · Pull Request #36997 · nodejs/node · GitHub

/ node Public

crypto: add generatePrime/checkPrime - #36997

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:random-prime
ConversationCommits (1)ChecksFiles changed
Closed

crypto: add generatePrime/checkPrime#36997
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:random-prime

Conversation

jasnell commented Jan 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

APIs for generating and checking pseudo-random primes.

Scratching an itch here. For a set of crypto benchmarks that I've been running I've needed the ability to generate prime test vectors but always had to rely on an external module. Which is silly since openssl has this built in and quite easy to expose.

One of the use cases is to generate primes for diffie hellman with a bit more control over the generation parameters.

crypto.generatePrime(32, {
  safe: true,
  add: 12n,
  rem: 11n,
}, (err, prime) => {
  const dh = crypto.createDiffieHellman(prime);
});

The crypto.checkPrime() does exactly what its name suggest... verifies that an input is a prime within a reasonable margin of error.

Signed-off-by: James M Snell jasnell@gmail.com

jasnell 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 Jan 19, 2021
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 Jan 19, 2021

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Jan 19, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

panva commented Jan 19, 2021

Copy link
Copy Markdown
Member

cc @nodejs/crypto

targos reviewed Jan 19, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated

jasnell commented Jan 22, 2021

Copy link
Copy Markdown
Member Author

The PR has been updated to split checkPrime() into sync and async variants. The check prime operation can take a while based on the size of the prime candidate and the number of checks so moving that off loop makes the most sense.

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Jan 22, 2021

Copy link
Copy Markdown
Collaborator

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

jasnell added the review wanted PRs that need reviews. label Jan 22, 2021

nodejs-github-bot commented Jan 22, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

tniessen reviewed Jan 23, 2021
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Using ArrayBuffer as the primary representation of prime numbers might be WebCrypto-ish, but it doesn't feel JavaScript-ish or Node.js-ish to me. Both OpenSSL and JavaScript have a BigInt/BIGNUM type, and I don't think our new APIs should steer away from that fact. While static representations of keys typically encode numbers as byte sequences, programmers should have access to types with better semantics. (Sure, higher-level types don't provide memory safety etc., but... it's JavaScript.)

Comment thread doc/api/crypto.md Outdated
tniessen requested changes Jan 23, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread src/crypto/crypto_random.cc Outdated
Comment thread src/crypto/crypto_random.cc Outdated
jasnell force-pushed the random-prime branch 2 times, most recently from 688eeaa to f16510f Compare January 23, 2021 19:29
jasnell requested a review from tniessen January 23, 2021 19:54

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Jan 24, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

jasnell requested a review from targos January 24, 2021 17:22
jasnell commented Jan 25, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
jasnell commented Jan 25, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
jasnell force-pushed the random-prime branch 2 times, most recently from 17e8dd6 to 6ce845a Compare January 25, 2021 15:28
jasnell changed the title crypto: add randomPrime/randomPrimeSync/checkPrime crypto: add generatePrime/checkPrime Jan 25, 2021

jasnell commented Jan 25, 2021

Copy link
Copy Markdown
Member Author

Update: I've renamed the function to generatePrime()/generatePrimeSync() to avoid some name bikeshedding debates later on

tniessen reviewed Jan 25, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
Comment thread src/crypto/crypto_random.cc Outdated
crypto: add generatePrime/checkPrime
00f5546 
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell requested a review from tniessen January 25, 2021 23:06

nodejs-github-bot commented Jan 25, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

tniessen commented Jan 26, 2021

Copy link
Copy Markdown
Member

LGTM. I think it's great to support BigInt, and I understand the hesitation to use BigInt as the default output format even though I personally believe it would be a better fit. rem and add likely don't need to be in secure memory, but they are short-lived, so it doesn't matter much.

jasnell removed the review wanted PRs that need reviews. label Jan 26, 2021

jasnell commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

Landed in bb13469

jasnell closed this Jan 26, 2021
jasnell added a commit that referenced this pull request Jan 26, 2021
crypto: add generatePrime/checkPrime
bb13469 
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36997
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
tniessen mentioned this pull request Feb 1, 2021
Closed
targos pushed a commit that referenced this pull request Feb 2, 2021
crypto: add generatePrime/checkPrime
110063d 
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36997
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos mentioned this pull request Feb 2, 2021
Merged
targos added a commit that referenced this pull request Feb 2, 2021
2021-02-02, Version 15.8.0 (Current)
5a59dde 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
targos added a commit that referenced this pull request Feb 2, 2021
2021-02-02, Version 15.8.0 (Current)
bc1c428 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
targos added a commit that referenced this pull request Feb 2, 2021
2021-02-02, Version 15.8.0 (Current)
211574b 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
tniessen added a commit to tniessen/node that referenced this pull request Mar 17, 2023
test: fix 'checks' validation test for checkPrime
2416f78 
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: nodejs#36997
tniessen mentioned this pull request Mar 17, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Mar 19, 2023
test: fix 'checks' validation test for checkPrime
fbd526b 
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
test: fix 'checks' validation test for checkPrime
fb08d74 
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
test: fix 'checks' validation test for checkPrime
45e7b10 
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
test: fix 'checks' validation test for checkPrime
6fee605 
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen approved these changes
targos Awaiting requested review from targos

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL