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
Merged

WIP: op_get_random_values #2327

merged 4 commits into from
May 17, 2019

Conversation

chiefbiiko
Copy link
Contributor

@chiefbiiko chiefbiiko 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
Copy link

CLAassistant commented May 9, 2019

CLA assistant check
All committers have signed the CLA.

@zekth
Copy link
Contributor

zekth 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My rustc demands it.

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

ry 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"

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - nice addition

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.

None yet

4 participants