Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Security] Rework the remember me system#40145

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
chalasr merged 1 commit intosymfony:5.xfromwouterj:issue-39308/remember-me
Apr 11, 2021

Conversation

wouterj
Copy link
Member

@wouterjwouterj commentedFeb 10, 2021•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsFixes part of#39308
LicenseMIT
Doc PRtbd

As I said in#39308, I want to change the remember me system in Symfony 5.3. The remember me system has a couple big "problems":

  1. It's hardwired into some Security classes likeContextListener. TheRememberMeFactory adds asetRememberMe() method call to the DI config and the context listener calls methods on this. This is very coupled, instead of the decoupled nature of the rest of security.
  2. Conditional conditions are combined with cookie creation in one class. This is especially hard in e.g. 2FA (where setting the cookie should be done after 2FA is completed, which is currently near impossible as it's directly bound to the conditional of being called after logging in).

The changes

  • The first commits harden the current functional test suite of remember me, to avoid breaking it.
  • I discovered a lot of similarity between remember me tokens and login links. That's why I've extracted the shared logic into a genericSignatureHasher in the 3rd commit.
  • I then remodelledRememberMeAuthenticator to the login link system, which I think improves a lot and at least improves problem (2) - as the conditionals (RememberMeAuthenticator) is split from the cookie creation (RememberMeHandlerInterface).
  • Finally, I added a new event (TokenDeauthenticatedEvent) to theContextListener to avoid direct coupling - solving problem (1).

This removes any usage of remember me services, which can be deprecated along with the rest of the security system.

Usage

As with the authenticator manager:Nothing changes in the configuration

Usage of persistent token providers has been improved. First, configuration is provided (setting up services is no longer needed):

# beforeservices:Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider:autowire:truesecurity:firewalls:main:remember_me:# ...token_provider:'Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider'# aftersecurity:firewalls:main:remember_me:# ...token_provider:doctrine:true

Furthermore, a schema listener is created. Whenever the doctrine token provider is used,make:migration/doctrine:schema:update will automatically create the required table.

Some advanced usage of Remember me is also improved a lot (there is no real "before" here, consider looking at scheb/2fa to get an idea of the before). A few use-cases I took into account:

  • If you ever need toprogrammatically create a remember me cookie, you can autowireRememberMeHandlerInterface and usecreateRememberMeCookie($user). This will make sure the remember me cookie is set on the final response (using theResponseListener)
  • TheRememberMeListener previously was responsible for both determining if a cookie must be set and setting the cookie. This is now split in 2 listeners (checking is done byRememberMeConditionsListener). IfRememberMeBadge is enabled, the cookie is set and otherwise it isn't. This allows e.g. SchebTwoFactorBundle to create a listener that catches whether remember me was requested, but suppress it until the 2nd factor is completed.

Todo

  • Update UPGRADE and CHANGELOG
  • Show before/after examples
  • Investigate the conditional event registering ofContextListener. This forces to inject both the firewall and the global event dispatcher at the moment.
  • Make sure old remember me tokens still function. As remember me tokens are long lived, we may need to provide backwards compatibility for at least Symfony 6.x.Update: it was decided to not include this for now:[Security] Rework the remember me system #40145 (comment)

cc@scheb@weaverryan as you both initiated this PR by sharing the problems with the current design.

chalasr, sstok, and GromNaN reacted with heart emoji
@stof
Copy link
Member

stof commentedFeb 10, 2021•
edited
Loading

  • This removes the capability ofPersistentTokenRememberMeService. Do we want to reintroduce this, or do we think that this is secure enough as-is (considering the same ideas as with the login link).

PersistentTokenRememberMeService is the only way to be able to invalidate tokens. And unlike login links, the remember tokens cannot be short-lived as that would defeat their purpose. So I suspect that removing the feature might not be OK (even though it was never properly documented and so requires digging into the code to discover it, making it an advanced features).

@nicolas-grekasnicolas-grekas added this to the5.x milestoneFeb 10, 2021
@wouterj
Copy link
MemberAuthor

PersistentTokenRememberMeService is the only way to be able to invalidate tokens. And unlike login links, the remember tokens cannot be short-lived as that would defeat their purpose. So I suspect that removing the feature might not be OK (even though it was never properly documented and so requires digging into the code to discover it, making it an advanced features).

You can invalidate tokens by including properties in the signature. If any signature property changes, all previously made remember me tokens are invalidated. The difference being short-lived login links vs long-lived remember me tokens is an interesting one! I see the current persistent tokens are using the logic as explained inhttps://stackoverflow.com/a/244907/1149495 I'll see if I can find other examples/references about persistent remember me cookies and whether using signature properties can fulfill this (or we need to reintroduce a persistent token handler).

@weaverryan
Copy link
Member

PersistentTokenRememberMeService is the only way to be able to invalidate tokens

As Wouter said, the signature properties could be used to invalidate tokens... but it could only be used to invalidateall tokens (e.g. by adding somelastLogoutAt property toUser and having that in the signature properties). If I logged in at work and at home (using remember me on both) and then logged out at work, there would not be a way to fully invalidate JUST the remember me cookie for my work computer. We would only be able to invalidateall current remember me tokens. This is actually a UX problem with removingPersistentTokenRememberMeService for companies that want a more secure setup.

However, I will also say that the new system will make it easier for "everyone else" to have a more secure remember me system: we can document how to setup a system that would truly invalidate remember me tokens (via the signature properties) on log out (though, again, it would invalidate all remember me tokens for that user).

Copy link
Member

@weaverryanweaverryan 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 incredibly promising!

@wouterjwouterjforce-pushed theissue-39308/remember-me branch 3 times, most recently fromdf166fa to3a7445eCompareFebruary 17, 2021 19:07
@wouterj
Copy link
MemberAuthor

PR and description updated:

  • I've reintroduced a persistent remember me implementation. I also tried to improve its use (default config + schema listener for automatic migration creation)
  • I've reimplemented the conditions to set a remember me cookie (request parameter) and added a special way to opt-out.
  • I've added lots of tests, CHANGELOG and PHPdoc.

@wouterjwouterjforce-pushed theissue-39308/remember-me branch 4 times, most recently from2e591bc to3a6b88cCompareApril 2, 2021 20:51
Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Thank you.
I did my best reviewing this. I just had some minor comments and suggestions.

@wouterjwouterjforce-pushed theissue-39308/remember-me branch 2 times, most recently from5edac9b toe26d6c4CompareApril 8, 2021 09:46
Copy link
Member

@weaverryanweaverryan 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 ready to go 👍

@wouterjwouterjforce-pushed theissue-39308/remember-me branch frome26d6c4 to1174f00CompareApril 11, 2021 11:58
@wouterj
Copy link
MemberAuthor

Thanks for the detailed review@chalasr :)

@wouterjwouterjforce-pushed theissue-39308/remember-me branch from1174f00 to232abe9CompareApril 11, 2021 12:01
@chalasrchalasrforce-pushed theissue-39308/remember-me branch from232abe9 to1567041CompareApril 11, 2021 12:47
@chalasr
Copy link
Member

Thank you@wouterj.

@chalasrchalasr merged commitb40eac2 intosymfony:5.xApr 11, 2021
@scheb
Copy link
Contributor

@wouterj Congratulations for having this merged! 🥳 Thank you for all of your work and thank you to everyone who gave feedback on this thread. I believe it's a great step forward!

chalasr, weaverryan, Nyholm, and sanderdlm reacted with heart emoji

@wouterjwouterj deleted the issue-39308/remember-me branchApril 11, 2021 13:18
@fabpotfabpot mentioned this pull requestApr 18, 2021
@PhilETaylor
Copy link
Contributor

cross postingscheb/2fa#75 for visibility :-) where a live app, upgraded tov5.3.0-BETA1 (locally, dont worry) now has a problem with the decorator and interface type hints.

@scheb
Copy link
Contributor

cross postingscheb/2fa#75 for visibility :-) where a live app, upgraded tov5.3.0-BETA1 (locally, dont worry) now has a problem with the decorator and interface type hints.

As I mentioned onscheb/2fa#75, just to let the people here know, that's nothing to worry about since I've been in the loop from the beginning of this PR here and I have an (unfinished) branch preparred to make scheb/2fa-bundle compatible with Symfony 5.3. Whoever wants to test Symfony 5.3 with 2fa-bundle is welcome to use that branch.

@PhilETaylor
Copy link
Contributor

Sorry :-) was too eager to test ... thanks everyone for the hard work - this looks amazing!

@zerkms
Copy link
Contributor

What was the reason to serialiseuserFqcn in the cookie? It's not used anywhere and leaks application implementation details for no good reason. The same forexpires.

@derrabus
Copy link
Member

@zerkms Would you mind opening a new issue for this topic? Only few people monitor comments of already closed PRs.

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

@stofstofstof left review comments

@ro0NLro0NLro0NL left review comments

@schebschebscheb left review comments

@NyholmNyholmNyholm left review comments

@chalasrchalasrchalasr approved these changes

@weaverryanweaverryanweaverryan approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.

13 participants
@wouterj@stof@weaverryan@fabpot@derrabus@chalasr@scheb@PhilETaylor@zerkms@ro0NL@Nyholm@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp