#24994 closed Cleanup/optimization (fixed)
Document expectations about settings.SECRET_KEY type
Reported by: | Baptiste Mispelon | Owned by: | MaartenPI |
---|---|---|---|
Component: | Documentation | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The documentation for SECRET_KEY [1] is not very explicit as to whether it should contain text or bytes.
Using bytes sort of works but can break with cryptic error messages.
From a quick git grep
, the secret key is used only in two places:
- In
core/signing.py
where it's cast into bytes withforce_bytes()
- In
utils/crypto.py
where it's%s
formatted into a unicode string (so Python2 will transparently try to decode it if needed)
We should at least document the exact type we're expecting and I think it would also be worthwhile to add a check to make sure the user's key passes our expectations.
[1] https://docs.djangoproject.com/en/dev/ref/settings/#std:setting-SECRET_KEY
Change History (18)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
I think trying force_text()
, force_bytes
and catching any EncodingError
would be a good approach and should be "backwards-compatible" in the sense that projects with a SECRET_KEY that currently works should not trigger the check.
As for how that person ended up with this weird bytestring SECRET_KEY, I have no idea.
comment:3 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Would it be appropriate to check for it django/core/checks/security/base.py
? There are already checks for SECRET_KEY
's length and unique characters there.
comment:5 by , 9 years ago
Initial pass at fixing this ticket here: https://github.com/django/django/pull/4930
comment:6 by , 9 years ago
Owner: | changed from | to
---|
PR updated https://github.com/django/django/pull/5216.
comment:7 by , 8 years ago
Owner: | changed from | to
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:9 by , 8 years ago
Updated the code according to the review comments. Can this PR be merged?
comment:10 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:11 by , 8 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Left some comments on the PR.
comment:12 by , 8 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
from mejiaa on the pull request:
What are those cryptic errors that occur when using bytes? I saw no links to issues pertaining to these cryptic messages.
Also, I'm not sure of this but there seems to be a belief that keys should be unicode characters and truly random bytes as keys are strange and/or not used in practice. Using truly random byte strings (bytes with values 0 - 255) as keys is not strange at all. In fact, it is recommended practice. See also https://tools.ietf.org/html/rfc4086.
I'm not seeing a compelling argument for ensuring the secret key is a valid unicode string. At worst, you'll be adding an extra burden on your users to base64 encode their secret keys or do some other transformation in order to continue using Django. As it stands, this change does not make it clear what issues are being resolved, offers no enhanced security, implies to users that they go against recommended practice for generating cryptographic keys, and adds an unnecessary burden on your users to generate a proper cryptographic key. I would prefer if this restriction is not accepted and users like myself can continue to use random byte strings as secret keys, with the consideration that it seems this change offers no benefit.
Given the lack of details here, I think it's best to close the ticket until a more compelling case for this check can be offered.
comment:13 by , 8 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Once Django drops support for Python 2 you'll have to go out of your way to put bytes in the SECRET_KEY.
Currently, since the type isn't enforced, every app that wants to do something with SECRET_KEY should conditionally convert it to bytes, typically with force_bytes
, since it _could_ be bytes. I don't think this is common knowledge; it's even a pitfall.
Enforcing the type would simplify that code and avoid errors for the minority who uses bytes in pluggable apps that assume text e.g. by doing settings.SECRET_KEY.encode()
.
mejiaa has spent a lot of time lobbying for making this change in the debug toolbar but his opinion has received little support until now and I disagree with them.
comment:14 by , 8 years ago
Cryptographic keys are suppose to be random bytes that don't necessarily represent a Unicode string. See also the RFC I linked in my comment.
I think it's fair to assume devs using the SECRET_KEY know it must be used as bytes. Various crypto libraries will refuse to accept them otherwise. This is true of the hmac, cryptography, and pyOpenSSL libraries.
As for my use case, a common practice is to use an external script or program to pipe secrets into processes that need them. I use something like this to not only setup my Django sites but to also rotate the secrets in them whenever necessary. The output from a subprocess.check_output() call is in bytes. As of now, since Django accepts the SECRET_KEY as bytes, I use random bytes for my SECRET_KEY and have it loaded in my Django sites via an external program.
comment:15 by , 8 years ago
I noticed that #19980 came to the conclusion that SECRET_KEY
could or should be bytes. That ticket fixed Signer
if SECRET_KEY
is non-ASCII bytes which is what the proposed check disallows (or warns against) here. I started a django-developers discussion to try to come to some conclusion.
comment:16 by , 8 years ago
Component: | Core (System checks) → Documentation |
---|---|
Patch needs improvement: | unset |
Summary: | Document/check that settings.SECRET_KEY should be a valid unicode string → Document expectations about settings.SECRET_KEY type |
Following the django-developers discussion, it seems there isn't consensus for this check after all. I've created a PR to document the expectation about the type of the secret key.
Would it be correct to check that
force_text(SECRET_KEY)
andforce_bytes(SECRET_KEY)
don't throw an exception? There's logic indjango/conf/__init__.py
to verify thatSECRET_KEY
isn't an empty, so maybe the check could be done there. On the other hand, it wouldn't be nice to break sites that are functioning with keys that don't meet these requirements. Did you find out how someone ended up with a broken key in the first place?