fix(ext/node): implement safe, add, rem options for crypto.generatePrime#32618
Open
bartlomieju wants to merge 5 commits intomainfrom
Open
fix(ext/node): implement safe, add, rem options for crypto.generatePrime#32618bartlomieju wants to merge 5 commits intomainfrom
bartlomieju wants to merge 5 commits intomainfrom
Conversation
Implements the previously stubbed `safe`, `add`, and `rem` options for `crypto.generatePrime()` and `crypto.generatePrimeSync()`, and fixes `checkPrime`/`checkPrimeSync` for large bigint candidates. - Add `Prime::generate_with_options()` in Rust supporting safe primes and add/rem constraints matching OpenSSL/Node.js behavior - Update `op_node_gen_prime` and `op_node_gen_prime_async` ops to accept safe, add, rem parameters - Fix bigint truncation bug in `checkPrime`/`checkPrimeSync` where bigints > 2^63 were silently truncated to i64 - Add `ERR_OSSL_BN_BIGNUM_TOO_LONG` validation for oversized candidates - Fix `unsignedBigIntToBuffer` using wrong primordial method on BigInt - Fix async `generatePrime` promise error handling to prevent double callback invocation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
approved these changes
Mar 11, 2026
Contributor
kajukitli
left a comment
kajukitli
left a comment
There was a problem hiding this comment.
lgtm, solid implementation of safe/add/rem options
key fixes:
- bigint truncation bug fixed by converting to bytes in JS before passing to op
safeprime generation correctly checks (p-1)/2 is also primeadd/remconstraints properly adjust candidates to satisfy p % add == rem- default rem values match Node.js: 1 for normal, 3 for safe primes
the edge case handling for large add values (where only a few candidates exist) is good — directly iterating instead of random sampling.
minor observations:
- the
is_biguint_probably_primewith 20 rounds is solid (matches OpenSSL's default) - the
gen_biguint_belowrejection sampling loop could theoretically run many times for unlucky values, but in practice it converges quickly
the promise fix (.then().catch() → two-arg .then()) prevents double callback — good catch.
kajukitli
reviewed
Mar 11, 2026
Contributor
kajukitli
left a comment
kajukitli
left a comment
There was a problem hiding this comment.
found one correctness bug: options.add = 0 is accepted, but the rust side assumes a non-zero modulus and can hit divide-by-zero / invalid arithmetic in prime generation
kajukitli
reviewed
Mar 11, 2026
Contributor
kajukitli
left a comment
kajukitli
left a comment
There was a problem hiding this comment.
found one correctness bug: options.add = 0 is accepted, but the rust side assumes a non-zero modulus and can hit divide-by-zero / invalid arithmetic in prime generation
A zero-length or all-zero add buffer would pass validation and reach the Rust generator where modulus operations assume non-zero add, causing a division-by-zero panic. Throw ERR_OUT_OF_RANGE early. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
littledivy
reviewed
Mar 12, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
safe,add, andremoptions forcrypto.generatePrime()/crypto.generatePrimeSync(), matching Node.js/OpenSSL behaviorcheckPrime/checkPrimeSyncbigint truncation bug where candidates > 2^63 were silently truncated toi64, causing incorrect primality resultsunsignedBigIntToBufferusingStringPrototypeToString(a String.prototype method) on BigInt valuesERR_OSSL_BN_BIGNUM_TOO_LONGvalidation for oversized buffer candidatesgeneratePrimepromise error handling (.then().catch()→ two-arg.then(onResolve, onReject)) to prevent double callback invocation