[Django] #31842: django.core.signing.dumps() and loads() not backwards compatible
Django
---------------------------------------------+------------------------
Reporter: Markus Holtermann | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.1
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
In Django 3.1, the default algorithm for `django.core.signing.Signer` was
changed from sha1 to sha256. That's good!
However, the `django.core.signing.dumps()` and
`django.core.signing.loads()` functions don't expose the algorithm. I
think, that that makes an upgrade from 3.0 to 3.1 impossible when one uses
the `dumps()` function in a multi-node Django setup.
Let's say there are two servers. Both run an application with Django 3.0.
and make use of `dumps()` and `loads()`. Then server 1 is upgraded to use
Django 3.1. At this point, because of to the backwards compatibility parts
in the decoding/signature validation, a token signed by server 2 can be
loaded on server 1 and 2. However, a token signed by server 1 cannot be
loaded on server 2.
This problem could be mitigated by exposing the algorithm in the `dumps()`
method and setting it to `sha1`. Once all servers run Django 3.1, the
algorithm could be changed to `sha256`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Markus Holtermann | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.1
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 Tim Graham):
I'm skeptical that Django should facilitate running multiple Django
versions concurrently. I can't imagine there won't be other issues. Schema
migrations are the first that come to mind.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:1>
Django
Reporter: Markus Holtermann | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.1
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 Simon Charette):
> I'm skeptical that Django should facilitate running multiple Django
versions concurrently.
I think we should allow find a way to multi-node upgrades to happen just
like we do with `pickle_version` for anything that uses `pickle.dumps`.
Large project Django upgrades are often
[https://landing.google.com/sre/workbook/chapters/canarying-releases/
tentatively deployed through canarying] and eventually rolled out to
multiple nodes gradually (pods, dynos, etc.).
Not having a way to a graceful roll out of a version should be considered
a release blocker IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:2>
Django
Reporter: Markus Holtermann | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.1
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 felixxm):
* stage: Unreviewed => Accepted
Comment:
Adding the `algorithm='sha256'` parameter to `loads()` and `dumps()`
sounds reasonable, it's not much of a burden. However, I'm not sure if
that's all you will need.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:3>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: assigned
Component: Core (Other) | Version: 3.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* owner: nobody => felixxm
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:4>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/13260 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:5>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:6>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:7>
Django
Reporter: Markus Holtermann | Owner: felixxm
Component: Core (Other) | Version: 3.1
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 Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d907371ef99a1e4ca6bc1660f57d81f265750984" d907371e]:
{{{
#!CommitTicketReference repository=""
revision="d907371ef99a1e4ca6bc1660f57d81f265750984"
Fixed #31842 -- Added DEFAULT_HASHING_ALGORITHM transitional setting.
It's a transitional setting helpful in migrating multiple instance of
the same project to Django 3.1+.
Thanks Markus Holtermann for the report and review, Florian
Apolloner for the implementation idea and review, and Carlton Gibson
for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:8>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"985735265563d4bb0ff44cf6b823446e1813fa34" 98573526]:
{{{
#!CommitTicketReference repository=""
revision="985735265563d4bb0ff44cf6b823446e1813fa34"
[3.1.x] Fixed #31842 -- Added DEFAULT_HASHING_ALGORITHM transitional
setting.
It's a transitional setting helpful in migrating multiple instance of
the same project to Django 3.1+.
Thanks Markus Holtermann for the report and review, Florian
Apolloner for the implementation idea and review, and Carlton Gibson
for the review.
Backport of d907371ef99a1e4ca6bc1660f57d81f265750984 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:9>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by אורי):
Does `DEFAULT_HASHING_ALGORITHM = 'sha1'` work for you with several
servers with Django 3.1 and 3.0? I checked on my staging server and it
seems not to work - exceptions are raised if I revert from Django 3.1 to
3.0 after users log in.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:10>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by אורי):
* cc: אורי (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:11>
Django
Reporter: Markus Holtermann | Owner: felixxm
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
אורי this is a separate issue, sessions respect
`DEFAULT_HASHING_ALGORITHM`, but format has changed, see #31864.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:12>
Django
Reporter: Markus Holtermann | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: Core (Other) | Version: 3.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0aa6a602b2b1cac6fe8e55051eed493b9cea7b81" 0aa6a602]:
{{{
#!CommitTicketReference repository=""
revision="0aa6a602b2b1cac6fe8e55051eed493b9cea7b81"
Refs #31842 -- Removed DEFAULT_HASHING_ALGORITHM transitional setting.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:13>
Django
Reporter: Markus Holtermann | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Riley Chase):
Was there a reason the `DEFAULT_HASHING_ALGORITHM` solution was used in
preference to adding the `algorithm` parameter to the two methods?
The project I'm working on needs to be able to select the hashing
algorithm so we can progressively upgrade several systems that all use
these methods. Updating them all at once is not feasible and we have a
requirement to maintain compatibility for a short period while the upgrade
is happening. Exposing the `algorithm` parameter would allow us to do this
easily and provide the same functionality if the default algorithm changes
again in the future.
Exposing the `algorithm` parameter would also allow users to opt into more
secure hashing algorithms ahead of Django making it the default.
If possible, I'd like to see this reopened (or another ticket if that's
the preference) so we can add the `algorithm` parameter to these methods.
I'd also be willing to put a PR together, I've not contributed to Django
before but this seems straight forward enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/31842#comment:14>