[Django] #20081: Minimize the risk of SECRET_KEY leaks
Django
----------------------------------------+------------------------
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
'''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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
For posterity, a non-master-HEAD reference to the same file:
https://github.com/openstack/horizon/blob/a3d136e36597c513b2942cc2dabe522d70533842/horizon/utils/secret_key.py
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:1>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jdunck):
Previous was me.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:2>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:3>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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)
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:4>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:5>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* cc: unai@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:6>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:7>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* cc: eromijn@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:8>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* keywords: => nlsprint14
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:9>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bendavis78):
* cc: bendavis78 (added)
Comment:
I'm also +1 for auto-generating the key à la horizon. A good point is made
in comment:9 that one potential problem is when moving from a "single" to
"multi" deployment (eg, if a new dev is unfamiliar with SECRET_KEY they
might be caught by surprise).
I think `django.conf.Settings()` should generate `SECRET_KEY` based on a
new setting called `SECRET_KEY_FILE`. If that setting is empty,
`Settings.__init__` will fail in the same way it currently does if
`SECRET_KEY` is empty. The new setting for `SECRET_KEY_FILE` can be
accompanied by documentation (using comments in settings.py) saying that
if the file does not exist it will be auto-generated, along with the
caveat that the key must remain the same in multi-server environments.
When starting a new project via "startproject", `SECRET_KEY_FILE` should
default to an empty string. So whether a dev is running startproject or
upgrading, they will be forced to look at the setting and understand what
it does.
We could further protect devs from slipping up by storing known value in
the database that is hashed using the secret key. This could be set during
the auto-generate function. Then, during startup we could check the stored
value against the hashed one, and if they don't match raise an exception
explaining that if you *know* the secret key must be changed and you
understand the implications, then you should manually re-hash that known
value in the database.
Also the function that generates the key should raise an exception if the
file exists. In that case file should have to be removed or changed
manually.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:10>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bendavis78):
Actually, after thinking it through a bit more, I might be -1 on auto-
generating the key, but still +1 on having `SECRET_KEY_FILE` and possibly
a hashed known value stored in the db to protect against change. A
management command is more explicit, and forces the dev to be involved in
the security process. I also think this process should be consistent
whether or not DEBUG is True.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:11>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aj7may):
Maybe setting the default value in settings.py to something like the
following would encourage some good practices:
SECRET_KEY = os.getenv('SECRET_KEY')
developers can then run the test server with the default secret key, while
the real secret key remains a secret on the production app servers.
Inspired by http://12factor.net/config
What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:12>
Django
Reporter: jacob | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aj7may):
In addition, a generatesecret command, as mentioned earlier, could be
implemented and used to generate those keys.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:13>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Component: Core (Other) | Version: 1.5
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aj7may):
* owner: nobody => aj7may
* status: new => assigned
Comment:
Here is a patch that accomplishes what I stated above. Looking for
feedback.
https://github.com/django/django/pull/2714
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:14>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aj7may):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:15>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by PaulM):
I left a comment on the PR, but to summarize here:
-1 on storing this in an environment variable.
It is already possible to follow that advice with Django, but I disagree
strongly with the 12factor website about the security and practicality of
that approach in the context of a typical Django deployment.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:16>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
So there is https://github.com/django/django/pull/2714 which implements
this. I like having a command to generate a secret key, and the idea of a
secret key file. However I think that automatically generating the secret
key is going to lead to footgun-ey behavior. This mentions N=1
deployments, however it doesn't account for migrations or the like. If
we're creating this file for users, there is a good chance they won't know
it even exists. This adds a file that they should be backing up/copying
over during a server migration that they aren't even really aware exists.
This is worse on a platform like Heroku, where if they leave this to
generate the file by default, it's going to get a new value every time the
app restarts due to the ephemeral FS. Last I looked Heroku restarted the
dyno's at least once a day, and for a n=1 deployment there is a decent
chance it's going to get idled out as well. Essentially I think the
automatic generation is being too clever and implicit.
I also am not a giant fan of importing from django inside of the settings
file. It seems like it's a good way to introduce circular imports that
will be tricky to debug.
I think it would b ebetter for the PR to add a setting,
{{{SECRET_KEY_FILE}}} which will be used to securely load the secret key
from a file (doing all of the proper things to ensure that the file has
safe permissions). There should also be the command that the PR has to
generate such a file, or pass {{{-}}} to pipe it to stdout.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:17>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aj7may):
Got lots of feedback. I'm working on incorporating some of these ideas.
I'll leave a note here when the PR has been updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:18>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aj7may):
Pull request updated: https://github.com/django/django/pull/2714
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:19>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by slurms):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:20>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* needs_better_patch: 0 => 1
* version: 1.5 => master
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:21>
Django
Reporter: jacob | Owner: aj7may
Type: New feature | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:22>
Django
Reporter: Jacob | Owner: aj7may
Type: New feature | Status: closed
Component: Core (Other) | Version: master
Keywords: nlsprint14 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => needsinfo
Comment:
So [https://groups.google.com/d/topic/django-
developers/CIPgeTetYpk/discussion the idea of making the settings file
more "12 factor friendly" came up again on the mailing list] — a quite
long thread raising many different points, and approaches.
One key point was, Paul's one from above, that environment variables only
suit a subset of projects. Then there was multiple settings files, and
secrets files, and better deployment docs and...
I have the impression that this ticket was based on a quite specific idea
from a conversion but it's grown quite amorphous, such that I'm not sure
any longer it's actionable. I'm going to close it as needsinfo. Can I ask
that if there's a concrete proposal based on it, that we start a fresh
ticket, perhaps referencing this one if necessary.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/20081#comment:23>