[Django] #34384: SECRET_KEY_FALLBACKS is not used for sessions
Django
----------------------------------------+------------------------
Reporter: Eric Zarowny | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
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 |
----------------------------------------+------------------------
I recently rotated my secret key, made the old one available in
`SECRET_KEY_FALLBACKS` and I'm pretty sure everyone on our site is logged
out now.
I think the docs for [https://docs.djangoproject.com/en/4.1/ref/settings
/#secret-key-fallbacks SECRET_KEY_FALLBACKS] may be incorrect when stating
the following:
In order to rotate your secret keys, set a new SECRET_KEY and move the
previous value to the beginning of SECRET_KEY_FALLBACKS. Then remove the
old values from the end of the SECRET_KEY_FALLBACKS when you are ready to
expire the sessions, password reset tokens, and so on, that make use of
them.
When looking at the Django source code, I see that the
[https://github.com/django/django/blob/4.1.7/django/utils/crypto.py#L19-L45
salted_hmac] function uses the `SECRET_KEY` by default and the
[https://github.com/django/django/blob/4.1.7/django/contrib/auth/base_user.py#L127-L136
AbstractBaseUser.get_session_auth_hash] method does not call `salted_hmac`
with a value for the `secret` keyword argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/34384>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Eric Zarowny | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Timothy Schilling):
* cc: Timothy Schilling (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:1>
Django
------------------------------+--------------------------------------
Reporter: Eric Zarowny | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
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
------------------------------+--------------------------------------
* cc: Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:2>
Django
------------------------------+--------------------------------------
Reporter: Eric Zarowny | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.1
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
------------------------------+--------------------------------------
* cc: Joey Lange (added)
Comment:
Hi! I'm a colleague of Eric's, and we were discussing some of the
ramifications of fixing this issue and I thought I'd write them here for
posterity.
In particular for user sessions, using fallback keys in the
`AuthenticationMiddleware`/`auth.get_user(request)` will keep existing
`_auth_user_hash` values from before the rotation being seen as valid,
which is nice during the rotation period, but without any ''upgrading'' of
the `_auth_user_hash` values, when the rotation is finished and the
fallback keys are removed, all of those sessions will essentially be
invalidated again.
So, I think possibly an additional need here is a way to upgrade the
cookies when a fallback key is used? Or at least documentation calling out
this drawback.
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:3>
Django
Reporter: Eric Zarowny | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 4.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 Mariusz Felisiak):
* cc: Carlton Gibson, Andreas Pelme, terrameijar (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. Agreed, we should check fallback session hashes.
Bug in 0dcd549bbe36c060f536ec270d34d9e7d4b8e6c7.
> In particular for user sessions, using fallback keys in the
`AuthenticationMiddleware`/`auth.get_user(request)` will keep existing
`_auth_user_hash` values from before the rotation being seen as valid,
which is nice during the rotation period, but without any upgrading of the
`_auth_user_hash` values, when the rotation is finished and the fallback
keys are removed, all of those sessions will essentially be invalidated
again.
>
> So, I think possibly an additional need here is a way to upgrade the
cookies when a fallback key is used? Or at least documentation calling out
this drawback.
> Edit: It's possible I'm conflating a cookie value and a session value,
but either way I think the principle of what I wrote stands?
As far as I'm aware, this is a new feature request not a bug in #30360, so
we should discuss it separately. Maybe we could call
`update_session_auth_hash()` when a fallback hash is valid 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:4>
Django
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* has_patch: 0 => 1
* status: new => assigned
Comment:
[https://github.com/django/django/pull/16631 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:5>
Django
---------------------------------+-----------------------------------------
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------------------
Comment (by Florian Apolloner):
Replying to [comment:4 Mariusz Felisiak]:
> Maybe we could call `update_session_auth_hash()` when a fallback hash is
valid 🤔
Most likely yes, we don't want to pay the calculation overhead every
request :)
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:6>
Django
---------------------------------+-----------------------------------------
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:6 Florian Apolloner]:
> Replying to [comment:4 Mariusz Felisiak]:
> > Maybe we could call `update_session_auth_hash()` when a fallback hash
is valid 🤔
> Most likely yes, we don't want to pay the calculation overhead every
request :)
OK, so we can accept this as a part of the bugfix.
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:7>
Django
---------------------------------+-----------------------------------------
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:8>
Django
---------------------------------+-----------------------------------------
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:9>
Django
---------------------------------+-----------------------------------------
Reporter: Eric Zarowny | Owner: David Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:10>
Django
Reporter: Eric Zarowny | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Eric Zarowny | Owner: David
| Wobrock
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2396933ca99c6bfb53bda9e53968760316646e01" 2396933c]:
{{{
#!CommitTicketReference repository=""
revision="2396933ca99c6bfb53bda9e53968760316646e01"
Fixed #34384 -- Fixed session validation when rotation secret keys.
Bug in 0dcd549bbe36c060f536ec270d34d9e7d4b8e6c7.
Thanks Eric Zarowny for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Eric Zarowny | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6937c921691746a85e2993406b46da0527b6f2ee" 6937c921]:
{{{
#!CommitTicketReference repository=""
revision="6937c921691746a85e2993406b46da0527b6f2ee"
[4.2.x] Fixed #34384 -- Fixed session validation when rotation secret
keys.
Bug in 0dcd549bbe36c060f536ec270d34d9e7d4b8e6c7.
Thanks Eric Zarowny for the report.
Backport of 2396933ca99c6bfb53bda9e53968760316646e01 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Eric Zarowny | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: contrib.auth | Version: 4.1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"ba1654cb54eccef3ba29e455cd5065aed84e1f90" ba1654cb]:
{{{
#!CommitTicketReference repository=""
revision="ba1654cb54eccef3ba29e455cd5065aed84e1f90"
[4.1.x] Fixed #34384 -- Fixed session validation when rotation secret
keys.
Bug in 0dcd549bbe36c060f536ec270d34d9e7d4b8e6c7.
Thanks Eric Zarowny for the report.
Backport of 2396933ca99c6bfb53bda9e53968760316646e01 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34384#comment:14>