Improvements to contrib.sessions #78

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
@crodjer

crodjer commented May 18, 2012

  • Check session expiry using the sigining framework
  • Extend the session key character set
  • Cleanup management command improvements
@ogier

This comment has been minimized.

Show comment
Hide comment
@ogier

ogier May 22, 2012

Contributor

Doesn't the fallback negate any security benefit to signing? If an attacker could break the old (insecure) mechanism, they can now break yours by triggering the old mechanism.

Contributor

ogier commented May 22, 2012

Doesn't the fallback negate any security benefit to signing? If an attacker could break the old (insecure) mechanism, they can now break yours by triggering the old mechanism.

@crodjer

This comment has been minimized.

Show comment
Hide comment
@crodjer

crodjer May 22, 2012

@ogier Yes, this can be an issue. With the compatibility workaround the
signing checks on sessions which were created before introduction of
the signing framework.

We could make this optional for users to keep compatibility with old
mechanism. Without this, the existing sessions will be reset.

On 01:50 -0700 / 22 May, Alex Ogier wrote:

Doesn't the fallback negate any security benefit to signing? If an attacker could break the old (insecure) mechanism, they can now break yours by triggering the old mechanism.


Reply to this email directly or view it on GitHub:
#78 (comment)

crodjer commented May 22, 2012

@ogier Yes, this can be an issue. With the compatibility workaround the
signing checks on sessions which were created before introduction of
the signing framework.

We could make this optional for users to keep compatibility with old
mechanism. Without this, the existing sessions will be reset.

On 01:50 -0700 / 22 May, Alex Ogier wrote:

Doesn't the fallback negate any security benefit to signing? If an attacker could break the old (insecure) mechanism, they can now break yours by triggering the old mechanism.


Reply to this email directly or view it on GitHub:
#78 (comment)

crodjer added some commits May 18, 2012

Add myself to authors
Signed-off-by: Rohan Jain <crodjer@gmail.com>
Check session expiry on the serve side
Use timed signer to check for expiration of session data. This is to
fix ticket #18194. The sessions based on file backend otherwise do not
expire, as far as the server is concerned.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
@crodjer

This comment has been minimized.

Show comment
Hide comment
@crodjer

crodjer May 28, 2012

To remove merge commits from the pull request, I did a rebase and re-added the commits in this pull request. In that process, I lost some comments on the commit "Check session expiry on the serve side", Line 79 . Here are those:

On 19:57 -0700 / 21 May, Sergiy Kuzmenko (@shelldweller) wrote:

2 compatibility issues:

  1. This will invalidate all existing sessions that were created the old way (and will likely throw an uncaught exception).
  2. Exception change for tempered data: SuspiciousOperation is implicitly replaced by BadSignature. (This might be the right thing to do but it must be documented).

On 12:02 +0530 / 22 May, Rohan Jain (@crodjer) wrote:

In case of an exception while unsigning existing sessions, we can fall
back to the previous decoding method. Added a commit for this in the
pull request.

crodjer commented May 28, 2012

To remove merge commits from the pull request, I did a rebase and re-added the commits in this pull request. In that process, I lost some comments on the commit "Check session expiry on the serve side", Line 79 . Here are those:

On 19:57 -0700 / 21 May, Sergiy Kuzmenko (@shelldweller) wrote:

2 compatibility issues:

  1. This will invalidate all existing sessions that were created the old way (and will likely throw an uncaught exception).
  2. Exception change for tempered data: SuspiciousOperation is implicitly replaced by BadSignature. (This might be the right thing to do but it must be documented).

On 12:02 +0530 / 22 May, Rohan Jain (@crodjer) wrote:

In case of an exception while unsigning existing sessions, we can fall
back to the previous decoding method. Added a commit for this in the
pull request.

crodjer added some commits May 18, 2012

Extend session key char set
Signed-off-by: Rohan Jain <crodjer@gmail.com>
Session cleanup management command improvements
Cleanup logic now lies in the backend. It will be executed based on
the currently set backend.
Adds a cleanup functionality for the file backend and db backend.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
Remove unused imports
Signed-off-by: Rohan Jain <crodjer@gmail.com>
Compatibility decoding of existing sessions
The existing sessions, which were not signed with the signing
framework is handled with the older decoding method.
Mark the session as modified so that it uses the new encoding method
for storing the data.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
Make compatibility with older mechanism optional
Don't enable compatibility with older mechanism by default as it
compromises with the security benefits of introducing signing
framework.

Signed-off-by: Rohan Jain <crodjer@gmail.com>
@ptone

This comment has been minimized.

Show comment
Hide comment
@ptone

ptone Oct 5, 2012

Member

There seem to be several tickets worth of stuff in here any references to open tickets?

This at least seems related to management stuff

https://code.djangoproject.com/ticket/18978

Member

ptone commented Oct 5, 2012

There seem to be several tickets worth of stuff in here any references to open tickets?

This at least seems related to management stuff

https://code.djangoproject.com/ticket/18978

@crodjer

This comment has been minimized.

Show comment
Hide comment
@crodjer

crodjer Oct 6, 2012

Yes and the initial commits here are for session expiry related issues: https://code.djangoproject.com/ticket/18194

crodjer commented Oct 6, 2012

Yes and the initial commits here are for session expiry related issues: https://code.djangoproject.com/ticket/18194

@ptone

This comment has been minimized.

Show comment
Hide comment
@ptone

ptone Oct 7, 2012

Member

There is some good work here - but please refactor it so that it is one pull request per ticket - and then cross reference the tickets and pulls to each other so that reviewers can find and connect them.

Thanks!

Member

ptone commented Oct 7, 2012

There is some good work here - but please refactor it so that it is one pull request per ticket - and then cross reference the tickets and pulls to each other so that reviewers can find and connect them.

Thanks!

@ptone ptone closed this Oct 7, 2012

@crodjer

This comment has been minimized.

Show comment
Hide comment
@crodjer

crodjer Oct 9, 2012

Sure, I'll do that as soon as possible.

Thanks

crodjer commented Oct 9, 2012

Sure, I'll do that as soon as possible.

Thanks

sztrovacsek pushed a commit to sztrovacsek/django that referenced this pull request Mar 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment