Code

Opened 13 months ago

Last modified 8 weeks ago

#20081 new New feature

Minimize the risk of SECRET_KEY leaks

Reported by: jacob Owned by: nobody
Component: Core (Other) Version: 1.5
Severity: Normal Keywords: nlsprint14
Cc: unai@…, eromijn@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Paul:

We should consider generating it on the fly something like the way Horizon does:
https://github.com/openstack/horizon/blob/master/horizon/utils/secret_key.py

The things we lose when the secret key changes (sessions and password reset links IIRC) are generally per-deployment specific anyway. There's still a lot of bikeshedding to be found behind that issue (should production servers have the ability to write this file), but it's not a bad approach for those same users who commit their secret keys to their public github repos.

Jacob:

Oh this is neat! So basically when the server starts up it writes out
a secret key file if it one doesn't already exist? I like it, seems
like a really good thing to do in core. Security-by-default and all
that.

How does this handle multiple web servers, though? Seems like if you
weren't careful you'd end up with differing secrets across your web
nodes, bad news. Couldn't we do something similar but store the key in
the database? That seems like it wouldn't be any more or less secure
than the filesystem, and would solve N=1 problems.

Paul:

Yeah, the multiple servers deployment its downside and the reason I originally objected to including it in Horizon. What it does is bump the user-facing issue (you have to deal with and understand a secret key) a bit further along the line. I'd be willing to guess that a majority of Django sites that leak the secret key fall into the N=1 category, or would do the correct thing WRT secret key if it weren't so easy to commit to the repo and was a specific action they had to take when N > 1. The downside of course is that there will always be a learning curve.

I'm -1 on storing the key in the database - databases tend to be easier to compromise than file systems, and people tend to be lazy about the security of their DB backups.

Attachments (0)

Change History (9)

comment:2 Changed 13 months ago by jdunck

Previous was me.

comment:3 Changed 13 months ago by Ryan Leckey <leckey.ryan@…>

My proposal would be the following:

  • startproject does not set SECRET_KEY
  • generate_secret_key (or some smaller name) be added to core (the command would just output the key so someone could pipe it to settings.py or some file referenced by settings.py
  • runserver uses some default non-secret key (its development anyway) if SECRET_KEY is not set

comment:4 Changed 13 months ago by lukeplant

My own practice is to put secret key in a secrets.json file, which is parsed by settings.py. The more I think about it, the more benefits I can see:

  • The code to use it from settings.py is really easy:
    import json
    secrets = json.load(file("secrets.json"))
    SECRET_KEY = secrets['SECRET_KEY']
    
  • It is less likely to be checked into source control due to the different file extension
  • If it is checked into VCS, then it should be relatively simple to rebuild the VCS history without that single file, allowing you to republish your fixed repo.
  • You can put more things in there - any other passwords etc. that should not be checked into source control.
  • Django management commands could correctly parse and re-write this file automatically, unlike something that was Python syntax.
  • Other systems can parse this file to extract passwords (I often need this in deployment)

comment:5 Changed 7 months ago by unaizalakain

I do that too but, like mentioned by Jacob:

How does this handle multiple web servers, though? Seems like if you
weren't careful you'd end up with differing secrets across your web
nodes, bad news.

comment:6 Changed 7 months ago by unaizalakain

  • Cc unai@… added

comment:7 Changed 3 months ago by wim@…

For me, a very easy solution would work:

from secrets import SECRET_KEY

It is easier to remember to keep secrets.py out of your repo than to keep settings.py out of your repo, and I would like to have the default project layout to use this approach.

On multiple servers, just copy secrets.py to the other servers.

comment:8 Changed 2 months ago by erikr

  • Cc eromijn@… added

comment:9 Changed 8 weeks ago by erikr

  • Keywords nlsprint14 added

I think the horizon method is almost perfect. For N=1 deployments, it simply fixes the problem without any special effort being required. Yes, importing from a secrets module or JSON file also adds security, but simply require more effort than the horizon method.

For N>1 deployments, we could fall back to a form of the current management, which seems to mean that most people roll something themselves. That means we add no security for N>1, but improving it for N=1 is already a great step forward. So, the default setting for secret key would be to autogenerate or read, but it can be overridden like any other setting. The patch for all of this is fairly simple, and I would be happy to contribute it.

However, I see an important issue for which I do not have an answer yet: if someone moves from N=1 to N>1 deployment, they will silently start using different secret keys. No warning is given. Depending on chance, users will encounter random failures with some features, and the cause will not be obvious. If we want to make this change, we should try to find some way so that developers are reminded to change their secret key management when they move from N=1 to N>1.

I can't think of any particular way to do this. Django is not aware of how many servers are running. And when trying to validate some data which involves the secret key, we can only check whether or not it is valid, not whether it might be valid with the secret key that was configured on another server. Secret key misalignment is not distinguishable from any other issue. Any suggestions?

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.