Call for feedback: django.utils.signed and signed cookies
Simon Willison
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.
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.
Cheers,
Simon
Luke Plant
> 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.
>
> 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.
Given that much of our existing code hasn't been reviewed by
cryptographers, I don't think that necessarily needs to hold up the
patch, but I agree it should be checked before 1.2 release.
A few quick comments, I haven't reviewed at length:
Rather than use 'settings.SECRET_KEY' as the default HMAC key,
shouldn't we add a prefix so that any usage of SECRET_KEY can't be
(potentially) used to attack other usages? We discussed this a while
back. The new messages system uses:
'django.contrib.messages' + settings.SECRET_KEY
for its HMAC key, which seems like a good convention to adopt. I don't
know how strictly necessary it is, but I don't think it can hurt.
I also think the "Cryptographic signing" documentation should clearly
note that developers should be very careful not to expose
functionality that would allow users to retrieve signatures for
arbitrary strings. If they do, it will allow any other system which
uses the same key for signing to be subverted.
And an entry in the 1.2 release notes is needed.
Luke
--
I never hated a man enough to give him his diamonds back. (Zsa Zsa
Gabor)
Luke Plant || http://lukeplant.me.uk/
Marty Alchin
> Rather than use 'settings.SECRET_KEY' as the default HMAC key,
> shouldn't we add a prefix so that any usage of SECRET_KEY can't be
> (potentially) used to attack other usages? We discussed this a while
> back. The new messages system uses:
>
> 'django.contrib.messages' + settings.SECRET_KEY
>
> for its HMAC key, which seems like a good convention to adopt. I don't
> know how strictly necessary it is, but I don't think it can hurt.
This was my concern as well, after the previous talks. The behavior
seems to be there, in the form of the `salt` argument, but I doubt
many people will use it. It might not be strictly necessary, but I
agree that it's probably a good practice that I would personally like
to see mandated by making that argument required, rather than
optional. It may seem like overkill, but if we're offering a way for
people to secure their applications, I think we should offer the most
secure option as the default, rather than simply the easiest. Of
course, the real-world importance of salting the hash would be best
addressed in the cryptographic review, but I agree that it's something
that should be noted prominently here, to make sure it gets a
once-over in said review.
> I also think the "Cryptographic signing" documentation should clearly
> note that developers should be very careful not to expose
> functionality that would allow users to retrieve signatures for
> arbitrary strings. If they do, it will allow any other system which
> uses the same key for signing to be subverted.
This is particularly important in the case of cookies, because it's
all to easy to simply change the name of a cookie and have it send an
authenticated value. That's why my original implementation also used
the cookie's name as an additional salt in the value. The effect is
essentially the same as using the `salt` argument explicitly, and I
think it's an acceptable shortcut for the cookie case. We're already
asking users to supply the key, which is basically a namespace anyway,
so why shouldn't we just automatically use that as salt for the hash?
That way, if `salt` does become required for the generic signing case,
it can still be left out from the cookie case, since the key can play
double-duty. If there's a concern with collision with other keys, we
could always prefix it with 'django_cookie_' or something before
applying it, but I don't think that'd be a necessary step.
-Marty
Russell Keith-Magee
> 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 %-)
Simon Willison
wrote:
> * 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.
I have to admit I don't 100% understand the difference between core
and utils - but I'm happy with moving it across to core if that makes
more sense.
> * get_cookie_signer() strikes me as something that should be a
> general utility in the signing module, not something buried in
> django/http.
I'd love to see this code factored out as per Marty's suggestion in
another thread. I wrote a backend just for signed cookies because
that's the one part of the implementation that's "baked in" to core
Django APIs - but I expect other bits to be baked in to things like
session handling / auth / form wizards etc over time, so being able to
over-ride the Signer used for those in the future will be important.
> * django.http.raise_error should be RAISE_ERROR, so it's obvious it's
> a constant
Good point, I'll fix that this evening.
> * I'm not sure I follow why we need to use base62 (and therefore
> provide our own base encoding method) in the Timestamp signer.
OK, you caught me. This is part of my evil scheme to sneak baseconv.py
in to Django. I'm using it for the signed cookie timestamp to make it
as small as possible (same reason I'm using a base64 of the HMAC hash
rather than the regular hexdigest) - since cookies add overhead to
every single HTTP request I figure anything we can do to make them
smaller is a bonus. I considered using a custom timestamp in seconds
since (2009, 12, 1) but that felt a little weird.
baseconv.py is actually a lot more important for some of the other
ways I use signed cookies. A great example is "recover my account /
forgotten password" e-mails. I like sending a signed URL for those (no
need to store any state) but it needs to be under 72 characters to fit
in an e-mail:
http://simonwillison.net/recover/b23/kfQJafvGyiofrdGnuthdxImIJw/
Where b23 is the base62 encoded user ID and the bit after it is a
signature.
URLs like that often make use of timestamped signatures (I like my
recover account URLs to expire after 24 hours) so again it's useful to
keep them short.
baseconv.py is also useful for things like URL shorteners e.g.
http://swtiny.eu/EZj
> * 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.
I'll look at that this evening.
> * 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
I've got a plan for that as far as signed cookies go - an optional
piece of middleware which looks for invalid signed cookies in the
request, checks them against an OLD_SECRET_KEYS list and upgrades them
if they can be decrypted that way. This can be implemented completely
separately from the current code, though the ideal implementation
would involve adding a few related methods to the Signer class.
Cheers,
Simon
Simon Willison
http://github.com/simonw/django/commit/802952bbb8b763e65ee545c6a8f39524b20e147c
"Use sha('signer' + secret_key + salt) to derive the key for use in
the
signature() method, addressing feedback from the django-developers
list"
The default signature() method now looks like this:
def signature(self, value, salt=''):
# Derive a new key from the SECRET_KEY, using the optional
salt
key = sha_constructor('signer' + self.key + salt).hexdigest()
return base64_hmac(value, key)
The secret key (self.key here) is now never used directly. Instead, a
sha1 hash of the salt 'signer' plus the secret key plus any additional
salt is used as the key for the signature. sha1 is used here as
protection against weird key length extension attacks (like the one
that affected the Flickr API recently).
http://github.com/simonw/django/commit/4ed44c2bce5000d6c78c3a26b84d08f636b3589c
"RAISE_ERROR now capitalised to emphasize that it is a constant"
http://github.com/simonw/django/commit/20f3a693b99ec6af0f91eecb31046e8a07dc7151
"Signed cookies now automatically include the name of the cookie as
part of the salt"
http://github.com/simonw/django/commit/68c52f0b995447d93bce1db486b23a27b918da73
"Moved get_cookie_signer in to utils.signed"
New patch is attached to the ticket. Is there anything else I need to
address before checking it in?
http://code.djangoproject.com/ticket/12417
Cheers,
Simon
Johannes Dollinger
r = len(s) % 4
pad = '=' * (r and 4 - r or 0)
I'd like some more kwargs to Signer and TimestampSigner. Mostly what's
in http://dpaste.com/136418/ (except the `separator` kwarg, which was
a bad idea as it depends on encode()): Signer(serializer=...) and
TimestampSigner(ttl=...).
__
Johannes
> --
>
> You received this message because you are subscribed to the Google
> Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com
> .
> To unsubscribe from this group, send email to django-develop...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
> .
>
>
SmileyChris
<johannes.dollin...@einfallsreich.net> wrote:
> There's a small bug in b64_decode(), the padding should be
> r = len(s) % 4
> pad = '=' * (r and 4 - r or 0)
Or even simpler:
pad = '=' * (-len(s) % 4)
Russell Keith-Magee
As far as the patch itself is concerned, looks good to me. My only
other request would be a serving of dogfood - if we're going to
include a signed cookie module, it would be nice to prove that it can
actually be used by actually using it.
Russ %-)
Alex Gaynor
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>
As an FYI Eric Florenzano has said that he asked a friend of his,
Matthew Dempsky, to take a look at this (from a security perspective),
he's the gent who found the security hole in djbdns.
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Simon Willison
<johannes.dollin...@einfallsreich.net> wrote:
> I'd like some more kwargs to Signer and TimestampSigner. Mostly what's
> a bad idea as it depends on encode()): Signer(serializer=...) and
> TimestampSigner(ttl=...).
The first few versions of the code had a bunch more stuff along the
lines of that dpaste (which expires in 6 days, so here's a gist copy
of it: http://gist.github.com/261572 )
After struggling with it for quite a while, I decided that having a
single class that handled signing, serialization and compression
wasn't the right approach - it was doing far too much. Instead, I
changed the design so the Signer's only responsibility was generating
signatures, appending them to strings and verifying that they were
correct. The encryption/serialization was extracted out and moved
directly in to the signed.dumps() and signed.loads() functions.
This solved a bunch of problems I was having with the code - too many
subclasses, confusing amounts of multiple inheritance for mixing
together different subclassed behaviours - and generally made
everything feel a lot more cohesive.
That's also why I added the BACKEND setting for setting a cookie
signing backend - that way, users with specific ideas about how their
signed cookies should work can write their own class that does pretty
much anything they like.
Is there any functionality in particular that you think should be
resurrected from the more-complex-signing-classes? I'm very keen on
keeping the serialization and compression stuff out of Signer.
Cheers,
Simon
Simon Willison
wrote:
> As far as the patch itself is concerned, looks good to me. My only
> other request would be a serving of dogfood - if we're going to
> include a signed cookie module, it would be nice to prove that it can
> actually be used by actually using it.
I'd love to see the cookie backend of the new django.contrib.messages
feature switch over to using get/set_signed_cookie. I'm happy to put
together the patch, or alternatively one of the people who worked on
that feature can do it.
Cheers,
Simon
Simon Willison
we've had it reviewed by real cryptographers. I'll aim to get it in
before the 1.2 beta feature freeze.
Cheers,
Simon
Johannes Dollinger
Am 22.12.2009 um 08:20 schrieb Simon Willison:
> On Dec 22, 12:52 am, Johannes Dollinger
> <johannes.dollin...@einfallsreich.net> wrote:
>> I'd like some more kwargs to Signer and TimestampSigner. Mostly
>> what's
>> inhttp://dpaste.com/136418/(except the `separator` kwarg, which was
>> a bad idea as it depends on encode()): Signer(serializer=...) and
>> TimestampSigner(ttl=...).
>
> The first few versions of the code had a bunch more stuff along the
> lines of that dpaste (which expires in 6 days, so here's a gist copy
> of it: http://gist.github.com/261572 )
Thanks.
> After struggling with it for quite a while, I decided that having a
> single class that handled signing, serialization and compression
> wasn't the right approach - it was doing far too much. Instead, I
> changed the design so the Signer's only responsibility was generating
> signatures, appending them to strings and verifying that they were
> correct. The encryption/serialization was extracted out and moved
> directly in to the signed.dumps() and signed.loads() functions.
>
> This solved a bunch of problems I was having with the code - too many
> subclasses, confusing amounts of multiple inheritance for mixing
> together different subclassed behaviours - and generally made
> everything feel a lot more cohesive.
>
> That's also why I added the BACKEND setting for setting a cookie
> signing backend - that way, users with specific ideas about how their
> signed cookies should work can write their own class that does pretty
> much anything they like.
>
> Is there any functionality in particular that you think should be
> resurrected from the more-complex-signing-classes? I'm very keen on
> keeping the serialization and compression stuff out of Signer.
I wanted to proposed to make dumps() and loads() methods on Signer
again. Now they are bound to TimestampSigner. If they were defined as
methods, you could reuse the functionality on subclasses. A mixin
might be overengineering as you would never use it without Signer and
it makes sense to have the feature available for all subclasses. Why
do you want to keep it out?
Useful additions:
* If sign() and unsign() take a `sep` kwarg, Signer.__init__() should
also take it. Better yet, only set it in __init__().
* TimestampSigner.__init__() needs a max_age/ttl kwarg.
__
Johannes