Skip to content
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: add generatePrime/checkPrime #36997

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 19, 2021

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 [email protected]

@jasnell 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 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
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 19, 2021

@panva
Copy link
Member

panva commented Jan 19, 2021

cc @nodejs/crypto

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

jasnell commented Jan 22, 2021

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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the review wanted PRs that need reviews. label Jan 22, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 22, 2021

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
src/crypto/crypto_random.cc Outdated Show resolved Hide resolved
src/crypto/crypto_random.cc Show resolved Hide resolved
@jasnell jasnell force-pushed the random-prime branch 2 times, most recently from 688eeaa to f16510f Compare January 23, 2021 19:29
@jasnell jasnell requested a review from tniessen January 23, 2021 19:54
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 24, 2021

@jasnell jasnell requested a review from targos January 24, 2021 17:22
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the random-prime branch 2 times, most recently from 17e8dd6 to 6ce845a Compare January 25, 2021 15:28
@jasnell jasnell changed the title crypto: add randomPrime/randomPrimeSync/checkPrime crypto: add generatePrime/checkPrime Jan 25, 2021
@jasnell
Copy link
Member Author

jasnell commented Jan 25, 2021

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

doc/api/crypto.md Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
src/crypto/crypto_random.cc Outdated Show resolved Hide resolved
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 25, 2021

@tniessen
Copy link
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 jasnell removed the review wanted PRs that need reviews. label Jan 26, 2021
@jasnell
Copy link
Member Author

jasnell commented Jan 26, 2021

Landed in bb13469

@jasnell jasnell closed this Jan 26, 2021
jasnell added a commit that referenced this pull request Jan 26, 2021
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <[email protected]>

PR-URL: #36997
Reviewed-By: Tobias Nießen <[email protected]>
targos pushed a commit that referenced this pull request Feb 2, 2021
APIs for generating and checking pseudo-random primes

Signed-off-by: James M Snell <[email protected]>

PR-URL: #36997
Reviewed-By: Tobias Nießen <[email protected]>
targos added a commit that referenced this pull request Feb 2, 2021
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: TODO
@targos targos mentioned this pull request Feb 2, 2021
targos added a commit that referenced this pull request Feb 2, 2021
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
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
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
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
nodejs-github-bot pushed a commit that referenced this pull request Mar 19, 2023
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants