-
Notifications
You must be signed in to change notification settings - Fork 25
INTPYTHON-676 Add optional signing of cache data #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Thank you @linted ! Looks like
|
django_mongodb_backend/cache.py
Outdated
|
||
def dumps(self, obj): | ||
# For better incr() and decr() atomicity, don't pickle integers. | ||
# Using type() rather than isinstance() matches only integers and not | ||
# subclasses like bool. | ||
if type(obj) is int: # noqa: E721 | ||
return obj | ||
return pickle.dumps(obj, self.protocol) | ||
return obj if self.signer is None else self.signer.sign(b64encode(obj)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't need to be b64encoded. On line 29/30, you can first unsign, and then check if the type after unsigning comes back as an int. If it does, then you can just return the int.
django_mongodb_backend/cache.py
Outdated
if self.signer is not None: | ||
data = b64decode(self.signer.unsign(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.signer is not None: | |
data = b64decode(self.signer.unsign(data)) | |
if self.signer is not None: | |
if type(unsigned_data := self.signer.unsign(data)) is int: | |
data = unsigned_data | |
else: | |
data = b64decode(unsigned_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this when I was originally writing the function, but 1234
is valid base64 and int
. So, it would be ambiguous as to which was actually being stored. I fixed the issue by simply not signing int
values. I do not believe this to introduce any real problems and solves the $inc
test cases which were failing.
except (BadSignature, TypeError): | ||
self.delete(row["key"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding why we're catching BadSignature
and TypeError
issues. What would trigger this and why would it be okay to delete a row in these instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the data in the cache collection is tampered with or somehow the HMAC secret key or salt is changed, a BadSignature
error is thrown. I added error handling here to delete the row otherwise the exception will DOS the page until the cache entry gets culled. TypeError
can be thrown if you switch from ENABLE_SIGNING=True
to ENABLE_SIGNING=False
without clearing all cache entries. It's highly unlikely that will ever happen in prod, but I was able to get that error while in changing settings in debug mode.
try: | ||
results[keys_map[row["key"]]] = self.serializer.loads(row["value"]) | ||
except (BadSignature, TypeError): | ||
self.delete(row["key"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better behavior than silently deleting bad data (which could happen in what circumstances besides an attacker putting malicious data in the cache?) could be to raise (or at least log) SuspiciousOperation
. There is some precedent in Django for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explained in a above comment the cases I was able to create where these two exceptions were thrown. If we do not delete the entry, won't we just create a DOS of the affected page until the cache entry is culled?
I do agree that this probably shouldn't be silent, but throwing an error here will stop the request from being handled, generate a 500, and require the request to be resent. I think that is probably ok if we delete the offending cache entry so only one request would be affected by the issue. What do you think?
…ce signed values contain characters which won't allow for conversion to int and ints wouldn't pass signature validation before being unpickled
Add HMAC signing of pickled cache data. This implementation uses Django's built in HMAC signer to avoid introducing new libraries. HMAC will introduce some overhead to performance, but for small and medium sized cache entries the impact should be minimal. The feature is easily disabled by setting
"ENABLE_SIGNING" = False
within theCACHE
configuration.Introduced two new cache config options:
ENABLE_SIGNING
- boolean value to turn HMAC signing on or off. Defaults to True (on)SALT
- optional string to salt HMAC signatures with