random-uuid should use a cryptographically strong PRNG if available

Question

{{random-uuid}} currently uses {{Math/random}} via {{rand-int}} to generate random numbers for v4 UUIDs. This patch aims to use a cryptographically strong PRNG (pseudo random number generator) instead, if available.

The functions used are:
{{window.crypto.getRandomValues}} in most browsers
{{window.msCrypto.getRandomValues}} in IE11
* {{Math/random}} in browsers not supporting the former or if the crypto module is not made available on Node.js

Currently not used:
* {{crypto.randomBytes}} on Node.js

Google Closure doesn't seem to provide feature detection or wrappers for the crypto-APIs, so the attached patch proposal implements a shim based on feature detection.

One open question is how the Node.js {{crypto}} module can be made available, since ClojureScripts {{core.cljs}} doesn't seem to have conditional {{require}} of Node.js-modules and maybe it should be kept this way.

Answer 1

Comment made by: dnolen

Some initial feedback don't switch on target, just do feature detection please. Don't use js/window, use goog.global.

Answer 2

Comment made by: aralo

Should we use js-mod since the UInt8 array guarantees them to be positive numbers?

Possibly outside of scope but instead of using str we could get some speedup by manually using (.join ... "") and (.join ... "-")

Answer 3

Comment made by: abp

Added 0001-CLJS-2386-no-target-js-mod.patch which removes the * } switch and replaces {{js/window}} with {{goog.global}} as per Davids comment.

This patch also uses {{js-mod}} instead of {{mod}} like A.R. suggested and adds a docstring for {{random-uuid}} explaining the behavior.
I could implement the {{.join}} suggestion, too, just not right now.

The ticket description has been updated to reflect the current state.

Answer 4

Comment made by: aralo

@Adrian : Don't use {{goog.global.window}} since {{goog.global}} is already the {{window}} object when in the browser. It happens to work since the window object also keeps a refernce to itself, so you could write {{js/window.window.window.window.crypto}}. Currently your code wouldn't work on nodejs.

So just use {{goog.global.crypto}} and use {{js/Uint8ClampedArray}} as you had it previously.

@David: We should probably add a {{bool-expr}} around the {{js-in}} macro, then we could use that for feature detection code.

Answer 5

Comment made by: abp

Oh that search/replace went wrong and wasn't even intended, testing succeeded for the reasons you explained, will fix it.

Answer 6

_Comment made by: abp_

0002-CLJS-2386-remove-window.patch fixes the problems pointed out by A. R. {{.join}} refactoring is still postponed.

@A. R. for {{(exists? crypto)}} on node i can't use {{goog.global}} because it is a module, right? Thanks for the help/reviews!

Answer 7

_Comment made by: aralo_

1. There is no reason to check for {{goog.global}}, it'll exist
2. You're checking for {{goog.global.msCrypto.getRandomValues}} but then calling {{goog.global.crypto.getRandomValues}}
3. You can't check like this {{(exists? crypto) ;; nodejs}} for node modules. crypto has to resolve to something. Just omit this {{nodejs}} case for now. Unless David can give better advice on how to conditionally require a crypto libraray in node.

Answer 8

Comment made by: abp

Sorry for the bad patches the last two days, always have been in a hurry.
Attached 0003-CLJS-2386-remove-node.patch fixes A. Rs latest finds.

Answer 9

Comment made by: abp

Attached 0004-CLJS-2386-join-array.patch uses {{.join}} on an {{array}} instead of {{str}}

An alternative implementation could be to convert the {{Uint8ClampedArray}} to a regular array after generating the random numbers and then just {{aset}} the string conversions in place and {{.join}} the result. But I don't know if it would be more efficient and how to convert the typed array to a regular array.

Answer 10

Comment made by: abp

0005-CLJS-2386-join-aset.patch should be more efficient because it doesn't use a closure to access the random values from the typed array. It just generates un/typed arrays with random values and then converts and transfers those into an untyped array to {{.join}}.

Answer 11

Comment made by: abp

0006-CLJS-2386- fix-join-aset.patch fixes a bug in 0005-CLJS-2386-join-aset.patch and supersedes it.

Answer 12

Comment made by: mfikes

0006-CLJS-2386-fix-join-aset.patch no longer applies

Answer 13

Comment made by: abp

Added 0007-CLJS-2386-fix-rand-array-len.patch containing a fix for a bug in {{random-uuid-vals}} where not enough random numbers were generated when {{crypto.getRandomValues}} is not available.

0007-CLJS-2386-fix-rand-array-len.patch also applies to the latest master

Answer 14

Comment made by: abp

Added 0008-CLJS-2386-fix-ie.patch removing use of {{Uint8ClampedArray}} which causes IE11 to throw a {{TypeMismatchError}} in {{msCrypto.getRandomValues}}. Also removed {{Uint8ClampedArray}} from other browsers implementation since it shouldn't be necessary.

Answer 15

Comment made by: mfikes

0008-CLJS-2386-fix-ie.patch passes CI (/)