Code

Opened 4 years ago

Closed 3 years ago

#12417 closed New feature (fixed)

Add signing and signed cookies to Django

Reported by: simon Owned by: simon
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Attachments (7)

ticket12417.diff (28.1 KB) - added by simon 4 years ago.
ticket12417-v2.diff (28.0 KB) - added by simon 4 years ago.
Updated based on feedback on the mailing list
ticket12417-v3.diff (28.0 KB) - added by simon 4 years ago.
ticket12417-v4.diff (29.3 KB) - added by jezdez 3 years ago.
Updated ticket with changes discussed in mailing list and ticket comments
ticket12417-v5.diff (29.0 KB) - added by steph 3 years ago.
Updated patch to use salted_hmac from django.utils.crypto
ticket12417-v6.diff (28.9 KB) - added by jezdez 3 years ago.
Updated method argument signature as discussed on the mailing list
ticket12417-v7.diff (35.2 KB) - added by jezdez 3 years ago.
Fixed baseconv.

Download all attachments as: .zip

Change History (22)

Changed 4 years ago by simon

comment:1 Changed 4 years ago by simon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 4 years ago by simon

Updated based on feedback on the mailing list

Changed 4 years ago by simon

comment:2 Changed 4 years ago by simon

  • Has patch set
  • milestone set to 1.2
  • Version changed from 1.1 to SVN

Patch is basically ready, but we're going to hold off committing it until we've had it reviewed by a real cryptographer. The plan is to get it in before the 1.2 beta feature freeze.

comment:3 Changed 4 years ago by anonymous

  • milestone changed from 1.2 to 1.3
  • Owner changed from nobody to anonymous
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by anonymous

  • milestone changed from 1.3 to 1.2
  • Owner changed from anonymous to simom
  • Status changed from assigned to new
  • Triage Stage changed from Accepted to Unreviewed

comment:5 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

We're past the feature freeze, this will have to wait until 1.3.

comment:6 Changed 4 years ago by russellm

  • Owner changed from simom to simon

Also, no idea who simom is :-)

comment:7 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 4 years ago by mr.kamu@…

Did this get reviewed by a cryptographer?

comment:9 Changed 4 years ago by lukeplant

A few comments on the patch, while I remember:

I think that get_cookie_signer should return a Signer that is unique for cookies e.g. uses "django.http.cookies" + settings.SECRET_KEY as the key to pass to Signer. This reduces the likelihood of someone being able to forge cookies by using another Signer in the project that happens to be exposed and allows attackers to generate signatures for arbitrary content.

Method Signer.unsign should replace if sig != expected with code that is not vulnerable to timing attacks.

Also there are some PEP8 fixes needed - double spaces between top level definitions in a module, no spaces around '=' for keyword arguments.

comment:10 Changed 3 years ago by EmilStenstrom

@mr.kamu: The mailing list thread above concluded that a many-eyes approach was better than getting a single cryptographer to review the code. So the review was done in this mail thread: http://groups.google.com/group/django-developers/browse_thread/thread/297e8b22006f7f3a - I'm not sure how to deem a crowd sourced mail thread "complete" so I leave that up to you.

comment:11 Changed 3 years ago by tsuraan

There are no signs of this work in django's svn repository. Are signed cookies planned for 1.3?

comment:12 Changed 3 years ago by jezdez

  • milestone 1.3 deleted

The deadline for new features has passed, removing 1.3 milestone.

comment:13 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:14 Changed 3 years ago by jezdez

  • Easy pickings unset
  • Patch needs improvement set

Changed 3 years ago by jezdez

Updated ticket with changes discussed in mailing list and ticket comments

Changed 3 years ago by steph

Updated patch to use salted_hmac from django.utils.crypto

Changed 3 years ago by jezdez

Updated method argument signature as discussed on the mailing list

Changed 3 years ago by jezdez

Fixed baseconv.

comment:15 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16253]:

Fixed #12417 -- Added signing functionality, including signing cookies. Many thanks to Simon, Stephan, Paul and everyone else involved.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.