On Mon, Dec 21, 2009 at 7:43 PM, Simon Willison <si...@simonwillison.net> wrote: > I've uploaded the patch for adding signing and signed cookies to > Django: > > http://code.djangoproject.com/attachment/ticket/12417/ticket12417.diff > > You can also read the documentation directly on my GitHub branch: > > http://github.com/simonw/django/blob/signed/docs/topics/signing.txt > http://github.com/simonw/django/blob/signed/docs/ref/request-response.txt#L224 > http://github.com/simonw/django/blob/signed/docs/ref/request-response.txt#L561 > > Most of the code lives in django.utils.signed (the low-level signing > API) but I've also added a get_signed_cookie() method to HttpRequest > and a corresponding set_signed_cookie() method to HttpResponse: > > http://github.com/simonw/django/blob/signed/django/http/__init__.py#L84 > http://github.com/simonw/django/blob/signed/django/http/__init__.py#L406 > http://github.com/simonw/django/blob/signed/django/utils/signed.py > > The code has documentation and unit tests. The documentation isn't > 100% complete - I need to improve the explanation of what signing is > and why it is useful and document the new COOKIE_SIGNER_BACKEND > setting which allows users to swap in their own cookie signing > behaviour should they need to.
Hi Simon, Here are some initial review comments: * I'm not sure I like this being in django.utils. To me, it feels like something that should be in django.core - along with caching, serialization, etc, signing is a core piece of functionality that a website will need to implement. django.utils is full of necessary, but largely ancillary code. * get_cookie_signer() strikes me as something that should be a general utility in the signing module, not something buried in django/http. * django.http.raise_error should be RAISE_ERROR, so it's obvious it's a constant * I'm not sure I follow why we need to use base62 (and therefore provide our own base encoding method) in the Timestamp signer. * In the original mailing list thread, there was discussion about prefixing the use of SECRET_KEY with some extra data, like the module name. The 'salt' option is one way to achieve this, but I agree with Luke and Marty that there should be some mandatory salting. * The original mailing list thread (and the wiki) also contained discussion about how to deprecate/cycle SECRET_KEY values in the case a value was compromised. Having a working solution for this problem probably isn't necessary, but it would be good to know that > Most importantly though, the implementation has not yet been peer > reviewed by real cryptographers. With that in mind, would it be > appropriate to check this in before the 1.2 freeze? We would certainly > get the code reviewed before the final 1.2 release. I don't see why we can't add this for the 1.2 freeze. Having *any* common signing module would be a valuable addition. Even if the module isn't reviewed, there is a benefit to having it - if a flaw *is* found, a normal Django security update will fix the flaw for all end users. If we get the signing module reviewed, that just gives us a bonus feature for the benefit of advertising - i.e., not only do we have a common signing module, but it's been reviewed as a cryptographically secure signing module. Russ %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
