Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#31842 closed Bug (fixed)

django.core.signing.dumps() and loads() not backwards compatible

Reported by: Markus Holtermann Owned by: Mariusz Felisiak
Component: Core (Other) Version: 3.1
Severity: Release blocker 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

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.

Change History (14)

comment:1 by Tim Graham, 4 years ago

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.

comment:2 by Simon Charette, 4 years ago

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 tentatively deployed through canarying and eventually rolled out to multiple nodes gradually (pods, dynos, etc.).

Not having a way to do a graceful roll out of a version should be considered a release blocker IMO.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:3 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted

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.

comment:4 by Mariusz Felisiak, 4 years ago

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:5 by Mariusz Felisiak, 4 years ago

Has patch: set

comment:6 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:7 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In d907371e:

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.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 98573526:

[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.

comment:10 by אורי, 4 years ago

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.

comment:11 by אורי, 4 years ago

Cc: אורי added

comment:12 by Mariusz Felisiak, 4 years ago

אורי this is a separate issue, sessions respect DEFAULT_HASHING_ALGORITHM, but format has changed, see #31864.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 0aa6a602:

Refs #31842 -- Removed DEFAULT_HASHING_ALGORITHM transitional setting.

Per deprecation timeline.

comment:14 by Riley Chase, 3 years ago

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.

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