Get random values

[LinusU]

Test Plan:

Apart from the added integration tests, an easy way to test this out is to generate a new empty project with react-native --init and add this single line to the render function:

  <Text>{crypto.getRandomValues(new Uint8Array(1))[0]}</Text>

When running the app you should see a number between 0 and 255 (inclusively).

Release Notes:

Help reviewers and the release process by writing your own release notes. See below for an example.

[GENERAL] [FEATURE] [JSC] - Add crypto.getRandomValues to the global JavaScript Context

Motivation:

The motivation for getting this into core, instead of in a module, is that it's very hard to build this as a module. Since there is no way to expose synchronous functions from a module, the only way to do this that I can think of would be to expose an initial secure seed from the module, and the implement a secure RNG in JavaScript.

Including it here makes it use the built-in secure RNG which I think is a huge plus, since it moves the job of devising a secure RNG to the platform.

As for including this at all, there are two strong arguments:

1. Having access to good random numbers is useful for a number of functions, and getRandomValues provides much stronger guarantees, and a more robust API, over Math.random(). If you want to generate a random password, generate the answer to a guessing game, or let the "AI" of your game do things at random, you want good random numbers.

2. There are a lot of libraries on npm which uses getRandomValues in the background, by adding this in a standards-compliant way, they will just work out of the box on React Native. There are also many libraries who are falling back on Math.random and in many cases breaks expectation by not using a secure RNG.

   A good example is the uuid package, a very popular package for generating UUIDs. UUID v4 is supposed to be 122 bits of randomness, but Math.random doesn't make guarantees that the result will contain that many bits of entropy. getRandomValues is also a much nicer API then repeatedly calling Math.random().

[LinusU]

analyze test is failing on this known upstream issue:

$ flow check
Error ----------------------------------------------------- node_modules/metro-config/src/configTypes.flow.js.flow:18:27

Cannot resolve module `metro/src/DeltaBundler/types.flow.js`.

   18| import type {Module} from 'metro/src/DeltaBundler/types.flow.js';
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^



Found 1 error

Going to fix that in the Metro repository when I get a chance 👍

[LinusU]

Oh no 😞

The Android build is failing because the JSC version is too old to have the explicit C++ functions for dealing with Typed Arrays. There are ways to work around this, I'll investigate that now.

[LinusU]

Hmm, so I tried using __has_include(<JavaScriptCore/JSTypedArray.h>) to feature detect the JSTypedArray API, but apparently that goes through even on the Android build 🤔

I'll check on the platform instead. When we upgrade JSC we can remove the old path 👍

[LinusU]

Okay, should be good to go now 🙌

All the tests pass locally, and on Circle CI 🚀

Am happy to answer any questions about the implementation ☺️

[elicwhite]

You should be able to expose sync methods in 3rd party modules just as easily as you can in core. If not, that is a bigger problem. Perhaps you should open an issue about the problems you've run into with that.

Adding more functionality like this seems like it is the opposite direction we want to be going in so I'd rather focus on making sure people have the functionality they need to maintain 3rd party modules.

[LinusU]

You should be able to expose sync methods in 3rd party modules just as easily as you can in core. If not, that is a bigger problem.

It seems to be specifically stated that this is not supported, from the documentation:

The return type of bridge methods is always void. React Native bridge is asynchronous, so the only way to pass a result to JavaScript is by using callbacks or emitting events (see below).

ref: https://facebook.github.io/react-native/docs/native-modules-android

I think that this is good since this allows the bridge to work via message passing (which I think is how it works? but I might be wrong here). Providing a way to call synchronous feels excessive to me, since there are very few use-cases that actually requires that. I'll explain why I think this is one of them further down.

Adding more functionality like this seems like it is the opposite direction we want to be going in so I'd rather focus on making sure people have the functionality they need to maintain 3rd party modules.

I am generally really on board with getting as many things away from core as possible. I'd actually argue for ripping out every component (possible not View though), but there are a few things that I would like to stay. We have this discussion constantly in Node.js as well, where we want to have a as small core as possible.

The things I do think belong in the core though, are things that I consider to be a part of a modern JavaScript platform. Things that makes packages on npm work, and that helps writing packages that works across Web Browsers, Node.js and React Native.

I believe that is why e.g. fetch have been added to React Native. That is a great example of an API that is part of the platform, and that helps with web compatibility. It's a part of the platform, and many JavaScript libraries expect it to be available.

I think that getRandomValues falls into the exact same category as fetch. It's a well-specified API, defined by a web standards, that many libraries expect to be available. Adding getRandomValues would really help with web compatibility.

As with fetch, all the browsers have implemented this, and I think that there are huge benefits of being web compatible. It helps the React Native ecosystem by opening up for more npm packages to work out-of-the-box ❤️

[elicwhite]

You make some good points that we should consider but I still want to focus on you being able to technically create this outside of core. That would let you move fast on supporting it without having to wait for PRs. Once it exists then a separate conversation can be had about bringing it into core.
For synchronous methods, note that I haven’t actually tried to create a 3rd party module with synchronous methods so I am only speaking from what I believe is possible. Help us understand where our understanding becomes invalid with 3rd party modules.

Check out this recent comment from @fkgozali on how to do it: react-native-community/discussions-and-proposals#11 (comment).

[LinusU]

Cool, using those "tricks" I managed to create a native module that does this! Thanks for pointing me in the right direction.

https://github.com/LinusU/react-native-get-random-values

One drawback is that it sends Base64-encoded strings over the native bridge, since I didn't find a way to pass a reference to a TypedArray.

I still think it would be very nice to include it in the core though, because of the same reason that I mentioned before. Web compatibility ❤️

[elicwhite]

You would have the same problem with needing to use a base64 string if it was in the core right?

For whether it is in the core, we don’t have a committee that decides such things and I’m just one person with an opinion...

My perspective is that we are currently trying to pull so much out of the core into separate packages to reduce maintenance burden that it doesn’t really seem to make sense to be adding new features to core that could easily be separate. I’m not really sure web compatibility is a goal in and of itself. If it was there would be a ton of things we need to add to the core and we would never be able to maintain them properly.

I would recommend you maintain this as the separate package that you have created. I’m going to close out this PR but if there are other core contributors who feel strongly that this should indeed be part of core we can revisit this.

[LinusU]

You would have the same problem with needing to use a base64 string if it was in the core right?

Nope, if you look at the code in this PR you will see that it fills the TypedArray right away, since it's using the plain JSC bindings to add this.

I’m not really sure web compatibility is a goal in and of itself.

I really think it should be, there are so many things that will just work easier out-of-the-box for our users.

If it was there would be a ton of things we need to add to the core and we would never be able to maintain them properly.

I don't think that there is too much stuff to add, and maintaining them is something that the community will help out with, the same as with the current features.

As I said earlier, I think that moving things out from core is great! But I think that we should get things in that makes us more of a platform, like web compat.

I'm not saying that this absolutely must be reopened, but I think that it would be nice to start a discussion on wether or not we want to be web compat, how much web compat we would like to be, and how much it would cost (in terms of maintaining it)...

[LinusU]

Also, in terms of the cost of maintaining this, I think it's quite low because of a few things:

• The code is very well contained, all the logic is in the Crypto.cpp file, which only exports a single public function
• It uses the JSC API, and don't depend on anything from React Native in itself. The entire bridge could be refactored and this part wouldn't be affected.
• On iOS it uses SecRandomCopyBytes, which hasn't changed since iOS 2.0 and has no indications of it going away. This is the only external function used.
• On Android it uses /dev/urandom and the POSIX standard APIs to read a file, this has been working since the first early release of Android and have no indications on going away.

[elicwhite]

Perhaps to start the discussion on whether we want web compat you should post in the discussions and proposals repo. The comment I linked to previously about how to do sync functions was in that repo.

[mikehardy]

@TheSavior I just bumped against this as I was migrating to the new version of the UUID library from @LinusU - this seems like the sort of thing that core could/should provide out of the box. It works as a native module providing a polyfill but for security related items exposure to many eyes (in core) seems like a big benefit

[lorenc-tomasz]

Totally agree with @mikehardy 👍

[LinusU]

Just chiming in to say that I still think that it would be great to have this directly in core to be more "Web Compatible", and that I'd be happy to rebase/update the pull request ☺️

[rigdern]

There are some things that distinguish an API for generating cryptographically random values from many other kinds of APIs:

1. It takes expertise and careful inspection to evaluate whether an implementation is correct
2. Flaws in incorrect implementations will likely manifest as subtle issues in production

I think this makes a strong case for including a crypto random value API in core. If this is instead solved as a third-party library, it puts developers at risk of both (1) and (2).

Additionally, we should consider designing the API so that it's compatible with popular battle-tested JavaScript libraries that require a crypto random value API (e.g. uuid).

@TheSavior @cpojer should we revisit this topic?

[elicwhite]

I totally agree @rigdern!

1. It takes expertise and careful inspection to evaluate whether an implementation is correct
   1. As Facebook wouldn't use this, Facebook expertise wouldn't be spent building this correctly or evaluating its implementation. It would be worse off as it would be dead code written by some community member in a PR that is slowed down by needing a Facebook employee to merge fixes
2. Flaws in incorrect implementations will likely manifest as subtle issues in production
   1. As Facebook wouldn't use this, people would have a false sense of security that it works in production like the rest of core.

Both of the reasons you listed are exactly why I think this belongs outside of core. Ideally written and maintained by reputable people and companies with a reliance on its accuracy.

[paulmillr]

This needs to be thoroughly reconsidered: React Native right now encourages writing non-secure apps! facebook/hermes#1003