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

Use os.urandom instead of OpenSSL.rand.bytes #69

Merged
merged 1 commit into from
Sep 21, 2017

Conversation

philngo
Copy link
Contributor

@philngo philngo commented Sep 14, 2017

@AndrewPix
Copy link

AndrewPix commented Sep 16, 2017

@philngo Maybe we should use Python3.6 secrets module to generate cryptographically strong token?

Starting with Python 3.6 the standard library includes the secrets module, which can be used for generating cryptographically secure random numbers, with specific helpers for text-based formats.

@fahdk
Copy link

fahdk commented Sep 18, 2017

@AndrewPix https://github.com/python/cpython/blob/master/Lib/secrets.py
secrets.token_bytes is just a wrapper around os.urandom

@007
Copy link

007 commented Sep 20, 2017

Ref pyca/pyopenssl#675

@belugame
Copy link
Collaborator

@James1345 has deliberately avoided using urandom, so I would like to hear his opinion on how to step forward here:

From the Readme:

DRF Tokens are generated with os.urandom, which is not cryptographically secure.

@belugame belugame requested a review from James1345 September 21, 2017 11:28
Copy link
Member

@James1345 James1345 left a comment

Choose a reason for hiding this comment

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

Agreed - with the deprecation of OpenSSL.rand

@James1345
Copy link
Member

I did originally avoid urandom in favour of OpenSSL's random, as that was the "best" cross platform random at the time. Now it is agreed that urandom is the correct way (which is why the OpenSSL method is being dropped) so we'll use that

@James1345 James1345 merged commit 7d90c8c into jazzband:master Sep 21, 2017
@James1345
Copy link
Member

Aaaaand I didn't realize that was done on master so have just merged that in completely the wrong way.

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.

6 participants