Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#34384 closed Bug (fixed)

SECRET_KEY_FALLBACKS is not used for sessions

Reported by: Eric Zarowny Owned by: David Wobrock
Component: contrib.auth Version: 4.1
Severity: Release blocker Keywords:
Cc: Timothy Schilling, Florian Apolloner, Joey Lange, Carlton Gibson, Andreas Pelme, terrameijar, David Wobrock Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 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 salted_hmac function uses the SECRET_KEY by default and the AbstractBaseUser.get_session_auth_hash method does not call salted_hmac with a value for the secret keyword argument.

Change History (14)

comment:1 by Timothy Schilling, 15 months ago

Cc: Timothy Schilling added

comment:2 by Mariusz Felisiak, 15 months ago

Cc: Florian Apolloner added

comment:3 by Joey Lange, 15 months ago

Cc: Joey Lange added

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.

Version 0, edited 15 months ago by Joey Lange (next)

comment:4 by Mariusz Felisiak, 15 months ago

Cc: Carlton Gibson Andreas Pelme terrameijar added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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 🤔

comment:5 by David Wobrock, 15 months ago

Cc: David Wobrock added
Has patch: set
Owner: changed from nobody to David Wobrock
Status: newassigned

in reply to:  4 ; comment:6 by Florian Apolloner, 15 months ago

Replying to 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 :)

in reply to:  6 comment:7 by Mariusz Felisiak, 15 months ago

Replying to Florian Apolloner:

Replying to 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.

comment:8 by Mariusz Felisiak, 15 months ago

Needs documentation: set
Patch needs improvement: set

comment:9 by David Wobrock, 15 months ago

Needs documentation: unset
Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 15 months ago

Patch needs improvement: set

comment:11 by Mariusz Felisiak, 15 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

Resolution: fixed
Status: assignedclosed

In 2396933c:

Fixed #34384 -- Fixed session validation when rotation secret keys.

Bug in 0dcd549bbe36c060f536ec270d34d9e7d4b8e6c7.

Thanks Eric Zarowny for the report.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

In 6937c921:

[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

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

In ba1654cb:

[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

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