[PATCH] Use secrets and fall back to random.SystemRandom for keys

[Jeremy Cline]

On Thu, Oct 10, 2019 at 09:30:50AM +1100, Daniel Axtens wrote:
> Hi Jeremy,
> 
> > The random module uses the Mersenne Twister pseudorandom number
> > generator and is not a cryptographically secure random number
> > generator[0]. The secrets[1] module is intended for generating
> > cryptographically strong random numbers, so recommend using that to
> > generate the secret key. It's new in Python 3, so if it's unavailable
> > fall back to using the ``os.urandom()`` backed implementation of random.
> >
> > [0] https://docs.python.org/3/library/random.html
> > [1] https://docs.python.org/3/library/secrets.html
> >
> 
> Thanks for your patch.
> 
> I agree that correctly generated randomness is the right way to go.
> 
> Do you think we need to advise existing implementations to roll their
> secret? My feeling is that given the way the twister has been seeded
> since Python 2.4 (os.urandom if available), existing installations are
> probably OK, but I'd be interested in your take.
> 

If I were running the service I would, but I do agree that given it's
likely it was seeded with os.urandom existing installations are probably
fine.

> > Signed-off-by: Jeremy Cline <jcline at redhat.com>
> > ---
> >  docs/deployment/installation.rst                     | 10 ++++++++--
> >  patchwork/settings/production.example.py             | 12 +++++++++---
> >  ...andom.SystemRandom-for-keys-9ceb496919a1bb6f.yaml |  5 +++++
> >  3 files changed, 22 insertions(+), 5 deletions(-)
> >  create mode 100644 releasenotes/notes/use-secrets-and-fall-back-to-random.SystemRandom-for-keys-9ceb496919a1bb6f.yaml
> >
> > diff --git a/docs/deployment/installation.rst b/docs/deployment/installation.rst
> > index d422573..f477a11 100644
> > --- a/docs/deployment/installation.rst
> > +++ b/docs/deployment/installation.rst
> > @@ -254,9 +254,15 @@ This should be a random value and kept secret. You can generate and a value for
> >  
> >  .. code-block:: python
> >  
> > -   import string, random
> > +   import string
> > +   try:
> > +       import secrets
> > +   except ImportError:  # Python < 3.6
> > +       import random
> > +       secrets = random.SystemRandom()
> 
> We're dropping Python 2 soon, not in the next version coming out Real
> Soon Now, but in the version after that. Would it be worth holding this
> patch until then so as we can avoid this messy try:import? I have a
> topic branch for this and I'd be happy to include this patch in it.
> 

An alternate approach here would be to just not use the secrets API.
CPython is just proxying the calls to secrets.choice to
random.SystemRandom().choice() so it comes to the same thing. I just
prefer the secrets API since its promise is to be fit for cryptographic
purposes and if they find a bug they'll fix it.

In my own projects I prefer to use the newer APIs when possible and fall
back as I don't find the clean-up to be terribly difficult (I just grep
for ImportErrors), but I do get that it's ugly. I don't have a strong
opinion about doing just random.SystemRandom().choice(), just
secrets.choice() in a deferred branch, or this, so I'll leave it up to
you.


Thanks,
Jeremy