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

WIP: op_get_random_values #2327

Merged
merged 4 commits into from May 17, 2019

Conversation

4 participants
@chiefbiiko
Copy link
Contributor

commented May 9, 2019

First shot at #2321

The CSPRNG in use is Rust's rand::rngs::ThreadRng.

Exposed as globalThis.crypto.getRandomValues().

Following Crypto.getRandomValues(), but not completely.

Overlaps:

  • function signature (passing an integer-based TypedArray)

Differences:

  • errors
    • not throwing a QuotaExceedError if the input length is gt 65536
      • now throwing an AssertionError for this case
    • passing null as input does not raise an exception until reaching Rustland
  • sync and async variations
  • additional support for BigInt64Array and BigUint64Array

Since these differences are incompatible with the browers' Crypto interface I just polluted Deno... 4give

@CLAassistant

This comment has been minimized.

Copy link

commented May 9, 2019

CLA assistant check
All committers have signed the CLA.

@zekth

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

i personnaly think Crypto.getRandomValues has to be similar as the browser implementation. So maybe add Crypto.getRandomValuesAsync() ?

Just a question : https://github.com/denoland/deno/pull/2327/files#diff-78d0badf3715ce7e0e4ddd06cb279f35

How you ensure the result of getRandomValues is the sametype as the input typed array? You don't check the input type on the rust side. I was stuck on this part on my side.
You check with assertNotEquals but you don't check the type of the entries of the array or even the type of the output array. By using the abstraction ArrayBufferView the typearray type may change no?

@@ -11,6 +11,7 @@ extern crate clap;
extern crate deno;
#[cfg(unix)]
extern crate nix;
extern crate rand;

This comment has been minimized.

Copy link
@zekth

zekth May 9, 2019

Contributor

this is not necessary IMO.

This comment has been minimized.

Copy link
@chiefbiiko

chiefbiiko May 17, 2019

Author Contributor

My rustc demands it.

Show resolved Hide resolved js/get_random_values.ts Outdated
Show resolved Hide resolved js/deno.ts Outdated
Show resolved Hide resolved js/globals.ts
@ry

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Currently CI is failing due to formatting. If you look at the end of the logs you'll see this:

Run tools/format.py 
 M cli/ops.rs

Also be sure to run "tools/lint.py"

@ry

ry approved these changes May 17, 2019

Copy link
Collaborator

left a comment

LGTM - nice addition

@ry ry merged commit 00f6fa4 into denoland:master May 17, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.