1042191 - Abstract crypto.randomBytes into loop/utils or somewhere else

Status: VERIFIED WONTFIX

(Hello (Loop) :: Server, defect)

Description

[Peter deHaan [:pdehaan]]

Steps to reproduce:
1. $ grep -ir "crypto.randomBytes" . | grep -v "/\(node_modules\|coverage\)/"


Actual results:
There are about 28 instances of `crypto.randomBytes(16).toString('hex')` in our source code (although 23 of those are in /test/). We should probably just create some simple helper that returns 16 random hex bytes so we aren't using as much duplicated code in the project.

```
./loop/index.js:    var callId = crypto.randomBytes(16).toString('hex');
./loop/index.js:    var wsCalleeToken = crypto.randomBytes(16).toString('hex');
./loop/index.js:    var wsCallerToken = crypto.randomBytes(16).toString('hex');
./loop/tokbox.js:    return crypto.randomBytes(number_of_bytes).toString('base64')
./loop/tokenlib.js:  return crypto.randomBytes(bytes)
./test/encrypt_test.js:      var passphrase = crypto.randomBytes(32).toString("hex");
./test/encrypt_test.js:      var passphrase = crypto.randomBytes(32).toString("hex");
./test/encrypt_test.js:      var passphrase = crypto.randomBytes(32).toString("hex");
./test/functional_test.js:          callId:          crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCallerToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCalleeToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          callId:          crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCallerToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCalleeToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          callId:          crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCallerToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:          wsCalleeToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:        callId:          crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:        wsCallerToken:   crypto.randomBytes(16).toString("hex"),
./test/functional_test.js:        wsCalleeToken:   crypto.randomBytes(16).toString("hex"),
./test/index_test.js:          hawkCredentials.id = crypto.randomBytes(16).toString("hex");
./test/storage_test.js:          callId:       crypto.randomBytes(16).toString("hex"),
./test/storage_test.js:          callId:       crypto.randomBytes(16).toString("hex"),
./test/storage_test.js:          callId:       crypto.randomBytes(16).toString("hex"),
./test/websockets_test.js:    var callId = crypto.randomBytes(16).toString('hex');
./test/websockets_test.js:      var callId = crypto.randomBytes(16).toString('hex');
./test/websockets_test.js:    var callId = crypto.randomBytes(16).toString('hex');
./test/websockets_test.js:      callId = crypto.randomBytes(16).toString('hex');
```


Expected results:

maybe add something like this to loop/utils.js (plus add it to `module.exports` object):

```
function randomBytes(len, type) {
  len = len || 16;
  type = type || 'hex';
  return crypto.randomBytes(len).toString(type);
}
```

And then throughout the code we can use something like this:
```
var callId = util.randomBytes(16);
--or--
var callId = util.randomBytes();
--or--
var passphrase = util.randomBytes(32);
--or--
return util.randomBytes(number_of_bytes, 'base64');
```

Comment 1

[Rémy Hubscher (:natim)]

I am not sure of the benefit of this, `crypto.randomBytes(len).toString(type)` looks rather fast forward why would you like to create a function for this and have `util.randomBytes(number_of_bytes, 'base64');` why is the same length and add an indirection level for new developer?

Comment 2

[Tarek Ziadé (:tarek)]

I think that make sense to simplify the code, as you don't need to think twice about how to create random strings. It's also a good thing for security reviews/updates in case we want to change how we generate random secure strings in js

Comment 3

[Alexis Metaireau (:alexis)]

I honestly think that's better to have things like that defined explicitly rather than adding a level of indirection, most of the time.

Changing this doesn't simplify the code, but I see some benefit into having this factorized, in case we would like to change that in the future, but that sounds like premature refactoring here.

I'm not that reluctant to add a function in the middle, but I would like to have some more info about why this is considered a bad practice to use crypto.randomBytes in multiple locations, because I consider it pretty straightforward (I don't have the impression of repeating myself more than doing utils.randomBytes for instance).

Comment 4

[James Bonacci [:jbonacci]]

Feedback from :pdehaan needed...

Comment 5

[Peter deHaan [:pdehaan]]

I'm fine with smacking this down.
We did a bit of simplification with a recent ESLint refactor. And I doubt that we'll completely refactor the way we generate `randomBytes()` in the near future, so this is probably a gross over-optimization when we have bigger things that need attention.

Lets leave it resolved as "shut-up-and-get-lost" and we can reopen/refile in the future, if it makes sense.

#killitwithfire

Comment 6

[James Bonacci [:jbonacci]]

OK.