Opened 13 years ago
Closed 13 years ago
#16182 closed Bug (wontfix)
TimestampSigner should use a more precise timestamp
Reported by: | floguy | Owned by: | floguy |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Generating a secure cookie using the TimestampSigner within the same second and the same data actually generates precisely the same cookie.
I'm currently developing a cookie-based session backend, and this line keeps failing as a result, because cycling the cookie isn't really cycling the cookie: https://code.djangoproject.com/browser/django/trunk/django/contrib/sessions/tests.py#L153
I've attached a small patch that fixes the problem by increasing the precision.
Attachments (3)
Change History (13)
by , 13 years ago
Attachment: | 16182-increased-signing-precision.diff added |
---|
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Patch looks good, it applies without problem. The tests for signing and sessions pass.
comment:2 by , 13 years ago
I've just updated the patch. This new patch adds tests, and actually improves the code's testability in general. Instead of monkeypatching time.time, we pass the time function as an argument to the TimestampSigner class.
by , 13 years ago
Attachment: | 16182-increased-signing-precision-2.diff added |
---|
comment:3 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Looks good to me. Only possible improvement could be renaming self.time to self.time_func (to avoid ambiguity), but that's a very minor change and something the committer can decide on, if needed.
by , 13 years ago
Attachment: | 16182-increased-signing-precision-3.diff added |
---|
Modified patch to use time_func
comment:6 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Triage Stage: | Ready for checkin → Design decision needed |
I see this is already committed, but I'd like to raise an objection anyway. The timestamp signer was deliberately only using second-level precision. Multiplying by 10,000 significantly increases the length of our sig that we're storing in the already very limited 4095 bytes of the cookie space. We don't want to take up another couple of those when a user can be storing data there.
I'll go into more detail in #16199, but signed cookies like this don't need this functionality for rotation. We're not invalidating any data on the backend, and a previously signed cookie is still technically valid until it expires even if we've rotated it on the users end.
comment:7 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Indeed, unfortunately I wasn't involved in the discussion during the DjangoCon sprint that led to committing this and I agree with Paul's argumentation. Reverting seems like the only option to me.
comment:8 by , 13 years ago
Given the arguments against, I'm inclined to agree; even with the new code, it's possible to make two cookies the same, so we're not eliminating any timing problems, merely reducing their probability (and possibly hiding some nasty bugs).
comment:10 by , 13 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
Changeset [16426] reverts this commit, and means I'll re-close this ticket as wontfix.
Increased precision for TimestampSigner