[Django] #24994: Document/check that settings.SECRET_KEY should be a valid unicode string
Django
------------------------------------------------+------------------------
Reporter: bmispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24994>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bmispelon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:1>
Django
Reporter: bmispelon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:2>
Django
Reporter: bmispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: 1.8
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 timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:3>
Django
Reporter: bmispelon | Owner:
Type: | bipsandbytes
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 1.8
checks) |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bipsandbytes):
* owner: nobody => bipsandbytes
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:4>
Django
Reporter: bmispelon | Owner:
Type: | bipsandbytes
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bipsandbytes):
Initial pass at fixing this ticket here:
https://github.com/django/django/pull/4930
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:5>
Django
Reporter: bmispelon | Owner:
Type: | sarthakmeh03
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sarthakmeh03):
* owner: bipsandbytes => sarthakmeh03
Comment:
PR updated https://github.com/django/django/pull/5216.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:6>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MaartenPI):
* owner: Sarthak Mehrish => MaartenPI
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:7>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Updated [https://github.com/django/django/pull/7510 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:8>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MaartenPI):
Updated the code according to the review comments. Can this PR be merged?
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:9>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MaartenPI):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:10>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Left some comments on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:11>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:12>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Aymeric Augustin):
* status: closed => new
* resolution: needsinfo =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:13>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: new
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Andres Mejia):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:14>
Django
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: new
Cleanup/optimization |
Component: Core (System | Version: 1.8
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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 [https://groups.google.com/d/topic/django-
developers/SOKz1e3TTcg/discussion django-developers discussion] to try to
come to some conclusion.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
* component: Core (System checks) => Documentation
Comment:
Following the django-developers discussion, it seems there isn't consensus
for this check after all. I've created a
[https://github.com/django/django/pull/7750 PR] to document the
expectation about the type of the secret key.
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: MaartenPI
Cleanup/optimization |
Component: Documentation | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"9e734875fe7fb078aa4ef0e84578aa7e641f5563" 9e734875]:
{{{
#!CommitTicketReference repository=""
revision="9e734875fe7fb078aa4ef0e84578aa7e641f5563"
Fixed #24994 -- Documented the expected type of settings.SECRET_KEY.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: MaartenPI
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3e1be301e286b38a4f4f03c3030cae92b1153361" 3e1be301]:
{{{
#!CommitTicketReference repository=""
revision="3e1be301e286b38a4f4f03c3030cae92b1153361"
[1.10.x] Fixed #24994 -- Documented the expected type of
settings.SECRET_KEY.
Backport of 9e734875fe7fb078aa4ef0e84578aa7e641f5563 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24994#comment:18>