Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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 with force_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 Tim Graham, 9 years ago

Would it be correct to check that force_text(SECRET_KEY) and force_bytes(SECRET_KEY) don't throw an exception? There's logic in django/conf/__init__.py to verify that SECRET_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?

comment:2 by Baptiste Mispelon, 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 Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:4 by bipsandbytes, 9 years ago

Owner: changed from nobody to bipsandbytes
Status: newassigned

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 bipsandbytes, 9 years ago

Initial pass at fixing this ticket here: https://github.com/django/django/pull/4930

comment:6 by Sarthak Mehrish, 9 years ago

Owner: changed from bipsandbytes to Sarthak Mehrish

comment:7 by MaartenPI, 8 years ago

Owner: changed from Sarthak Mehrish to MaartenPI
Triage Stage: AcceptedReady for checkin

comment:8 by Tim Graham, 8 years ago

Has patch: set
Triage Stage: Ready for checkinAccepted

Updated PR

comment:9 by MaartenPI, 8 years ago

Updated the code according to the review comments. Can this PR be merged?

comment:10 by MaartenPI, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Left some comments on the PR.

comment:12 by Tim Graham, 8 years ago

Resolution: needsinfo
Status: assignedclosed

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 Aymeric Augustin, 8 years ago

Resolution: needsinfo
Status: closednew

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 Andres Mejia, 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 Tim Graham, 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 Tim Graham, 8 years ago

Component: Core (System checks)Documentation
Patch needs improvement: unset
Summary: Document/check that settings.SECRET_KEY should be a valid unicode stringDocument 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.

comment:17 by GitHub <noreply@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 9e734875:

Fixed #24994 -- Documented the expected type of settings.SECRET_KEY.

comment:18 by Tim Graham <timograham@…>, 8 years ago

In 3e1be301:

[1.10.x] Fixed #24994 -- Documented the expected type of settings.SECRET_KEY.

Backport of 9e734875fe7fb078aa4ef0e84578aa7e641f5563 from master

Note: See TracTickets for help on using tickets.
Back to Top