Skip to content

Generate tokens with CSPRNG #372

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

Merged
merged 1 commit into from
Feb 12, 2016
Merged

Conversation

dchest
Copy link
Contributor

@dchest dchest commented Feb 12, 2016

Package hat, used to generate session and some other tokens uses
non-cryptographic random number generator. Replace it with the
cryptographically secure one. The result has the same format
(32-character hex string, 128 bits of entropy).

@dchest
Copy link
Contributor Author

dchest commented Feb 12, 2016

Ideally, I'd create a function:

function randomToken() {
   return crypto.randomBytes(16).toString('hex');
}

and use it everywhere (removing hat dependency), but don't know where to put it. Should we have utils.js? Please let me know how to proceed and I'll modify this PR.

@nlutsenko
Copy link
Contributor

This is a nice idea, security few!

Agreed with a custom function for these things. util.js sounds good, though maybe cryptoUtils.js, since the former has a tendency to grow with a bunch of random unrelated things.
One more thing: we probably can remove hat at all from package.json, since we won't need it anymore.

Feel free to update this PR and we'll happily merge this in, unless @drew-gross has any objections.

@dchest
Copy link
Contributor Author

dchest commented Feb 12, 2016

All right, I've introduced cryptoUtils.js and moved all related random string generation there, removing 2 dependencies (hat and randomstring). I've also created some tests. (I haven't yet tested this stuff locally, as I haven't set up environment on this computer yet.)

@facebook-github-bot
Copy link

@dchest updated the pull request.

@dchest
Copy link
Contributor Author

dchest commented Feb 12, 2016

I'll rebase it in a few hours.

@nlutsenko
Copy link
Contributor

Great, looks good so far. Can we use ES6 import/export style by any chance?

@nlutsenko
Copy link
Contributor

Passing back purely for rebase.

@facebook-github-bot
Copy link

@dchest updated the pull request.

@facebook-github-bot
Copy link

@dchest updated the pull request.

@facebook-github-bot
Copy link

@dchest updated the pull request.

@facebook-github-bot
Copy link

@dchest updated the pull request.

@dchest
Copy link
Contributor Author

dchest commented Feb 12, 2016

Whee! Sorry for the noise, it's ready for review now. With regards to modules, I chose to use whatever import style the file uses: when it's CommonJS-style, I used CommonJS, when it already had ES6 imports, I used ES6 import. The .spec.js files are not transpiled, so I used require there.

@gfosco
Copy link
Contributor

gfosco commented Feb 12, 2016

Can you rebase this? Thank you.

Move object ID, token, and random string generation into their own
module, cryptoUtils.

Remove hat dependency, which was used to generate session and some other
tokens, because it  used non-cryptographic random number generator.
Replace it with the cryptographically secure one. The result has the
same format (32-character hex string, 128 bits of entropy).

Remove randomstring dependency, as we already have this functionality.

Add tests.
@facebook-github-bot
Copy link

@dchest updated the pull request.

drew-gross added a commit that referenced this pull request Feb 12, 2016
Generate tokens with CSPRNG
@drew-gross drew-gross merged commit 2510cab into parse-community:master Feb 12, 2016
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.

5 participants