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

refactor(node/crypto): port polyfill to Rust for randomInt, randomFill, randomFillSync #18658

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

levex
Copy link
Contributor

@levex levex commented Apr 11, 2023

Pretty much as per the title, I'd welcome some feedback especially around the
array/buffer handling in the two randomFill functions.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Looks good. Let's fix formatting and linting problems and see if the tests are passing fine

ext/node/polyfills/internal/crypto/_randomInt.ts Outdated Show resolved Hide resolved
@levex levex requested a review from bartlomieju April 11, 2023 19:04
@levex levex changed the title node/crypto: port polyfill to Rust for randomInt, randomFill, randomFillSync refactor(node/crypto): port polyfill to Rust for randomInt, randomFill, randomFillSync Apr 11, 2023
@levex levex force-pushed the node-crypto-random branch 3 times, most recently from 9ef1ba8 to d5a670c Compare April 11, 2023 22:29
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@sepehrst
Copy link

Out of curiosity, shouldn't these be generated by a CSPRNG? or at least by getrandom crate maybe?

@bartlomieju
Copy link
Member

Out of curiosity, shouldn't these be generated by a CSPRNG? or at least by getrandom crate maybe?

We're using rand::ThreadRng here which appears to have the same properties as getrandom crate.

@levex levex merged commit 65b0d23 into denoland:main Apr 12, 2023
@levex levex deleted the node-crypto-random branch April 12, 2023 00:58
@littledivy
Copy link
Member

@sepehrst ThreadRng is a CSPRNG

@sepehrst
Copy link

I was reading https://rust-random.github.io/book/guide-rngs.html#state-and-seeding and just missed the mention of ThreadRng as a CSPRNG in that document. Thanks for the clarification guys. Please forgive the noise.

levex added a commit that referenced this pull request Apr 12, 2023
…l, randomFillSync (#18658)

Pretty much as per the title, I'd welcome some feedback especially
around the
array/buffer handling in the two randomFill functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants