Opened 13 years ago
Closed 12 years ago
#12417 closed New feature (fixed)
Add signing and signed cookies to Django
| Reported by: | simon | Owned by: | simon |
|---|---|---|---|
| Component: | Core (Other) | Version: | dev |
| 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: |
Description
Attachments (7)
Change History (22)
Changed 13 years ago by
| Attachment: | ticket12417.diff added |
|---|
comment:1 Changed 13 years ago by
Changed 13 years ago by
| Attachment: | ticket12417-v2.diff added |
|---|
Updated based on feedback on the mailing list
Changed 13 years ago by
| Attachment: | ticket12417-v3.diff added |
|---|
comment:2 Changed 13 years ago by
| Has patch: | set |
|---|---|
| milestone: | → 1.2 |
| Version: | 1.1 → 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 13 years ago by
| milestone: | 1.2 → 1.3 |
|---|---|
| Owner: | changed from nobody to anonymous |
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
comment:4 Changed 13 years ago by
| milestone: | 1.3 → 1.2 |
|---|---|
| Owner: | changed from anonymous to simom |
| Status: | assigned → new |
| Triage Stage: | Accepted → Unreviewed |
comment:5 Changed 13 years ago by
| milestone: | 1.2 → 1.3 |
|---|
We're past the feature freeze, this will have to wait until 1.3.
comment:7 Changed 13 years ago by
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:9 Changed 12 years ago by
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 12 years ago by
@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 12 years ago by
There are no signs of this work in django's svn repository. Are signed cookies planned for 1.3?
comment:12 Changed 12 years ago by
| milestone: | 1.3 |
|---|
The deadline for new features has passed, removing 1.3 milestone.
comment:13 Changed 12 years ago by
| Severity: | → Normal |
|---|---|
| Type: | → New feature |
comment:14 Changed 12 years ago by
| Easy pickings: | unset |
|---|---|
| Patch needs improvement: | set |
Changed 12 years ago by
| Attachment: | ticket12417-v4.diff added |
|---|
Updated ticket with changes discussed in mailing list and ticket comments
Changed 12 years ago by
| Attachment: | ticket12417-v5.diff added |
|---|
Updated patch to use salted_hmac from django.utils.crypto
Changed 12 years ago by
| Attachment: | ticket12417-v6.diff added |
|---|
Updated method argument signature as discussed on the mailing list
Under discussion here: http://groups.google.com/group/django-developers/browse_thread/thread/d9d635afb6d1820f