Opened 5 years ago

Closed 5 years ago

#16199 closed New feature (fixed)

Create a secure cookie-based session backend

Reported by: floguy Owned by: nobody
Component: contrib.sessions Version: master
Severity: Normal Keywords:
Cc: Jannis Leidel Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have built a third-party implementation of this, but after talking to some core devs at DjangoCon.eu, it seems there's some interest in having this be in core.

Here's my implementation: https://github.com/ericflo/django-cookie-sessions

There's really not a whole lot of code there, it passes all of Django's session tests, and it seems to work great (except the one that checks for existence, which doesn't make sense in a cookie backend, so that test is overwritten with a no-op.) Most of the code is here: https://github.com/ericflo/django-cookie-sessions/blob/master/cookiesessions/engine.py

Attachments (7)

16199.1.diff (5.1 KB) - added by Jannis Leidel 5 years ago.
Ported over Eric's backend
16199.2.diff (5.1 KB) - added by Jannis Leidel 5 years ago.
d'oh, fixing typo in tests
16199.3.diff (6.2 KB) - added by Jannis Leidel 5 years ago.
updated backend to use pickle
16199.4.diff (10.0 KB) - added by Jannis Leidel 5 years ago.
Updated docs with warning and cleaned up tests slightly
16199.5.diff (10.5 KB) - added by Jannis Leidel 5 years ago.
moar warnings
16199.6.diff (12.0 KB) - added by Jannis Leidel 5 years ago.
release notes
16199.7.diff (17.4 KB) - added by Jannis Leidel 5 years ago.
Removed code duplication and disabled test.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 5 years ago by Aymeric Augustin

(not marking as Accepted because this is a large change and I haven't seen a decision by a core developer yet)

comment:3 Changed 5 years ago by Jannis Leidel

Triage Stage: Design decision neededAccepted

Oh definitely accepted, I was one of the core devs Eric talked to in Amsterdam.

Changed 5 years ago by Jannis Leidel

Attachment: 16199.1.diff added

Ported over Eric's backend

comment:4 Changed 5 years ago by Jannis Leidel

Has patch: set

comment:5 Changed 5 years ago by Florian Apolloner

Needs documentation: set
Patch needs improvement: set

This backend uses signing.dumps which writes the data using JSON, which difers from the other backends which do use pickle. The docs should point that out (eg you can put arbitrary data into the session) and probably also add a warning that the data is just signed and not encrypted…

comment:6 Changed 5 years ago by Florian Apolloner

The patch has a CacheDBSessionTests class which probably should get renamed to something more matching like SignedCookieSessionTests

Changed 5 years ago by Jannis Leidel

Attachment: 16199.2.diff added

d'oh, fixing typo in tests

Changed 5 years ago by Jannis Leidel

Attachment: 16199.3.diff added

updated backend to use pickle

comment:7 Changed 5 years ago by Jannis Leidel

Patch needs improvement: unset

Changed 5 years ago by Jannis Leidel

Attachment: 16199.4.diff added

Updated docs with warning and cleaned up tests slightly

comment:8 Changed 5 years ago by Luke Plant

#14579 was a duplicate.

Changed 5 years ago by Jannis Leidel

Attachment: 16199.5.diff added

moar warnings

Changed 5 years ago by Jannis Leidel

Attachment: 16199.6.diff added

release notes

comment:9 Changed 5 years ago by Florian Apolloner

Since we are 2.5+ we can replace session_key = property(_get_session_key, _set_session_key) with @property. Aside from that contrib/session/tests.py need future imports for the with statement.

EDIT:// just noticed that property.setter is 2.6+, not 2.5+, so the property comment isn't useful -- sry

Last edited 5 years ago by Florian Apolloner (previous) (diff)

comment:10 Changed 5 years ago by Paul McMillan

I see we have docs about max cookie length, but I'm concerned that's not going to be enough to help users who run into that issue debug it. Can we raise a warning (that could be caught and logged) when users are sending a cookie larger than the standard accepted size? It should trigger if the length of name + value + one for the equals sign is > 4095 (IE has the lowest limit).

The warning might be best raised later on in the response. Since we're encouraging users to store lots of data in the cookie here, we should help them when they store too much.

If others think it appropriate, I could split this warning feature off into a different ticket.

Last edited 5 years ago by Paul McMillan (previous) (diff)

comment:11 Changed 5 years ago by Paul McMillan

Patch needs improvement: set

I commented on #16182, but will go into more detail here. r16356 added 5 digits of precision to the signed timestamp so that this cookie backend can pass the cookie rotation test. Adding these extra bits to our already limited cookie length isn't necessary, and the test should be modified to work with single-second precision for this backend.

Unlike server-side session stores, when we rotate a cookie with this backend, we don't invalidate the previous one. In server side session stores, rotating a cookie should destroy access to the data server side, so we need to test that this rotation works immediately. In this backend, we can't invalidate previously signed cookies (other than letting them expire naturally). So requiring that setting a cookie then immediately rotating it produce a different cookie isn't meaningful.

We can either skip the test entirely, wait for one second to pass before trying the rotate function, or do something excessively clever to modify the time in-place. I'm in favor of skipping the test and reverting r16356.

Last edited 5 years ago by Paul McMillan (previous) (diff)

comment:12 Changed 5 years ago by Paul McMillan

Also Also (sorry for so many comments here), we're duplicating a lot of functionality. The string signer already does a bunch of the stuff that's in this module, albeit with JSON instead of pickle. Could we modify the signer to work with either JSON or pickle, and save ourselves a bunch of duplicated code? (e.g. the signer does a check to make sure the gzip is actually shorter, I don't see that here).

comment:13 in reply to:  11 Changed 5 years ago by Jannis Leidel

Replying to PaulM:

I commented on #16182, but will go into more detail here. r16356 added 5 digits of precision to the signed timestamp so that this cookie backend can pass the cookie rotation test. Adding these extra bits to our already limited cookie length isn't necessary, and the test should be modified to work with single-second precision for this backend.

Unlike server-side session stores, when we rotate a cookie with this backend, we don't invalidate the previous one. In server side session stores, rotating a cookie should destroy access to the data server side, so we need to test that this rotation works immediately. In this backend, we can't invalidate previously signed cookies (other than letting them expire naturally). So requiring that setting a cookie then immediately rotating it produce a different cookie isn't meaningful.

This does indeed make sense to me, at least more than r16356 made when I saw it. I'm +1 on reverting it back, although Andrew needs to be informed first.

We can either skip the test entirely, wait for one second to pass before trying the rotate function, or do something excessively clever to modify the time in-place. I'm in favor of skipping the test and reverting r16356.

+1

comment:14 in reply to:  12 Changed 5 years ago by Jannis Leidel

Replying to PaulM:

Also Also (sorry for so many comments here), we're duplicating a lot of functionality. The string signer already does a bunch of the stuff that's in this module, albeit with JSON instead of pickle. Could we modify the signer to work with either JSON or pickle, and save ourselves a bunch of duplicated code? (e.g. the signer does a check to make sure the gzip is actually shorter, I don't see that here).

Yeah, full agreement here, it felt wrong when I moved it over to using pickle yesterday and wouldn't mind having a small abstraction (or additional argument) for the loads/dumps functions.

Changed 5 years ago by Jannis Leidel

Attachment: 16199.7.diff added

Removed code duplication and disabled test.

comment:15 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16466]:

Fixed #16199 -- Added a Cookie based session backend. Many thanks to Eric Florenzano for his initial work and Florian Apollaner for reviewing.

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