Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[fixes #22] Per-user keyrings, session invalidation #26

Merged
merged 3 commits into from
Aug 24, 2022
Merged

[fixes #22] Per-user keyrings, session invalidation #26

merged 3 commits into from
Aug 24, 2022

Conversation

david-batranu
Copy link
Member

Generate a Keyring for each user id, this makes it possible to invalidate the keyring on logout, meaning other user sessions using the same authentication token will become invalid.

The change applies only to new sessions, existing sessions will still be able to validate with the _secret keyring.

This protects against session stealing (copying the __ac cookie) and creates a working server-side log-out.

I require some input regarding:

  • I only did manual browser testing. I'm not sure how to run the tests.
  • I don't know if this affects other parts of Plone.
  • Should I make this optional? Session invalidation on log-out is considered a security best-practice nowadays.

@david-batranu
[refs plone#22] Per-user keyrings, session invalidation
d6c7d5a 
Generate a Keyring for each user id, this makes it possible to
invalidate the keyring on logout, meaning other user sessions using
the same authentication token will become invalid.

This protects against session stealing (copying the __ac cookie) and
creates a working server-side log-out.
@mister-roboto
Copy link

@david-batranu thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@david-batranu
Copy link
Member Author

@jenkins-plone-org please run jobs

@david-batranu
Copy link
Member Author

Fixed issues found by Jenkins, added tests for the session invalidation on log-out and also made it optional, enabling/disabling can be done via the acl_users/session/manage_secret ZMI tab (Manage secrets).

Would appreciate some feedback.

@avoinea avoinea requested review from ale-rt, dataflake, jensens and mauritsvanrees and removed request for dataflake April 4, 2022 14:39
ale-rt
ale-rt reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a nice new feature to me and I am fine to merge.
Thanks for your work!
I would wait for some opinion coming from the @plone/security-team before merging.

jensens
jensens requested changes Apr 6, 2022
View reviewed changes
Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a great feature. Like!
Just one minor complaint, see comment.

@david-batranu
[refs plone#22] Trying to remove IDisableCSRFProtection
4114ee0 
Tests now pass without IDisableCSRFProtection, unknown why.
mauritsvanrees
mauritsvanrees approved these changes Aug 24, 2022
View reviewed changes
Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High time that we merge this. LGTM.

@mauritsvanrees mauritsvanrees merged commit e10c43a into plone:master Aug 24, 2022
ale-rt
ale-rt reviewed Aug 24, 2022
View reviewed changes
news/26.feature.txt
@@ -0,0 +1 @@
Creating per-user keyrings in order to have session invalidation on log-out (server-side logout). [david-batranu]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the file .txt extension. it has to be just .feature to be properly picked up by towncrier.

plone/session/plugins/session.py
@@ -17,6 +18,7 @@
from zope.component import getUtility
from zope.component import queryUtility
from zope.interface import implementer
from zope.interface import alsoProvides
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this imported can be removed, right?

@ale-rt
Copy link
Member

ale-rt commented Aug 24, 2022

I noticed I had a pending review on this... I will fix it myself as a punishment...

@ale-rt
Copy link
Member

ale-rt commented Aug 24, 2022

I did that on #31

@mamico mamico mentioned this pull request Feb 8, 2025
Merged
@mamico mamico mentioned this pull request Feb 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ale-rt ale-rt ale-rt left review comments

@jensens jensens jensens approved these changes

@mauritsvanrees mauritsvanrees mauritsvanrees approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@david-batranu @mister-roboto @ale-rt @jensens @mauritsvanrees