Skip to content

Conversation

@jasnell
Copy link
Member

@jasnelljasnell 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)=>{constdh=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]

@jasnelljasnell 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-botnodejs-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

cc @nodejs/crypto

@jasnell
Copy link
MemberAuthor

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

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

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

@jasnelljasnellforce-pushed the random-prime branch 2 times, most recently from 688eeaa to f16510fCompareJanuary 23, 2021 19:29
@jasnelljasnell requested a review from tniessenJanuary 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

@jasnelljasnell requested a review from targosJanuary 24, 2021 17:22
@jasnelljasnellforce-pushed the random-prime branch 2 times, most recently from 17e8dd6 to 6ce845aCompareJanuary 25, 2021 15:28
@jasnelljasnell changed the title crypto: add randomPrime/randomPrimeSync/checkPrimecrypto: add generatePrime/checkPrimeJan 25, 2021
@jasnell
Copy link
MemberAuthor

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

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.

@jasnelljasnell removed the review wanted PRs that need reviews. label Jan 26, 2021
@jasnell
Copy link
MemberAuthor

Landed in bb13469

@jasnelljasnell 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
@targostargos 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 freeto 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++.cryptoIssues and PRs related to the crypto subsystem.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-minorPRs 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

@jasnell@nodejs-github-bot@panva@tniessen@targos