Hashing Session Keys in backends

[mark]

After renewed interest because of potential database timing attacks (T31412) I'm looking into an existing PR (PR8736 for T21076) for adding the possibility of storing hashes of session keys.

I'm looking to get some feedback on two things;

After going through the existing commits of Chris Griffin, I agree with Aymeric Augustin (who did an initial review of the pull request) that there should be a clearer distinction between the incoming session key (Aymeric talks about a "clear text session key") and the key that gets stored in the sessions backend (Aymeric talks about a "hashed if needed session key").

I'm suggesting to use the names frontend_key and backend_key for these two concepts.

My second suggestion is to refactor the SessionBase class to make sure the session-key-hashing happens in one place and isn't spread across all different backend implementations as is the case now because the subclasses have to implemented public methods that receive the frontend_key. I'm suggesting to basically have subclasses implement private methods that receive a backend_key, which will be invoked by the public methods in the BaseClass. Obviously this will have consequences for any existing custom backends out there, though I think those will be affected either way. 

I welcome any thoughts on both the naming convention and the refactoring.

[Adam Johnson]

Hi Mark

Thanks for looking into this tricky security issue.

I'm suggesting to use the names frontend_key and backend_key for these two concepts.

They seem reasonable to me, as long as there's an explanatory comment. Perhaps it even needs documenting for third party backends.

My second suggestion is to refactor the SessionBase class to make sure the session-key-hashing happens in one place and isn't spread across all different backend implementations as is the case now because the subclasses have to implemented public methods that receive the frontend_key.

Yes that seems reasonable, although I haven't looked closely. Is there a way to avoid breaking third party backends, but raising deprecation warnings? Perhaps using func_supports_parameter or similar that we've used in past deprecations?

I see the PR is quite old and not owned by you. If you want to open a new PR, rebase the existing code, and refactor it as you see fit, you can use Co-Authored-By to acknowledge Chris' original work.

Thanks,

Adam

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/41c26919-f15f-4151-aa82-1281e24656da%40googlegroups.com.

--

Adam

[mark]

Hey Adam, thanks for the feedback, I'll make sure to credit Chris' original work in a new PR, I think I'm getting close to having one ready.

Is there a way to avoid breaking third party backends, but raising deprecation warnings?

 

I could create a new SessionBase child class (something like HashingSessionBase) and make all the default backends inherit from this class, instead of from SessionBase directly. Any sessions backend that does not inherit from the new HashingSessionBase could then be nominated for deprecation. This will also allow making no/minimal changes to the SessionBase class to optimise backward compatibility.

To unsubscribe from this group and stop receiving emails from it, send an email to django-d...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/41c26919-f15f-4151-aa82-1281e24656da%40googlegroups.com.

--

Adam

[mark]

Initial (draft) Pull Request: https://github.com/django/django/pull/12814 

The pull request at the very least still needs documentation, but would be good to have a review of the implementation first.