[Security] The road to a stable authenticator-based authentication #39308
Comments
wouterj
added
the
Help wanted
|
I've been working quite a bit with custom security implementations in Symfony. I'll be happy to review or discuss things. I agree with the rough ideas you presented. I also see that there are some more things todo with integrating with common authentication strategies. Most of which might just be documentation though.. |
|
The idea of introducing trust levels is awesome. I also use something like that in one of my older projects where permissions (or roles) are bound to trust levels. So a user can only have a certain permission if the trust level is high enough. This can help avoiding that a user can accidently get access to something he/she may never be allowed to. So it would be great to have trust levels built into Symfony core. A nice concept that can be very helpful for complex security requirements. |
|
Removing credential-related methods from the user interface is also a good idea. For example I have a system where the user "handle" and password are stored in a separate table and not directly in the user table. So this would also help to make such implementations less ugly. Sure there still needs to be a way to access that credentials. |
|
See #40267 for the password related part. |
|
About revisiting |
…asr) This PR was merged into the 5.3-dev branch. Discussion ---------- [Security] Decouple passwords from UserInterface | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | #23081, helps with #39308 | License | MIT | Doc PR | todo This PR addresses a long-standing issue of the Security component: UserInterface is coupled to passwords. It does it by moving the `getPassword()` method from `UserInterface` to a `PasswordAuthenticatedUserInterface`, and the `getSalt()` method to a `LegacyPasswordAuthenticatedUserInterface`. Steps: - In 5.3, we add the new interface and, at places where password-based authentication happens, trigger deprecation notices when a `UserInterface` object does not implement the new interface(s). The UserInterface is kept as-is until 6.0. - In 6.0, we can remove the methods from `UserInterface` as well as support for using password authentication with user objects not implementing the new interface(s). As a side-effect, some password-related interfaces (`UserPasswordHasherInterface` and `PasswordUpgraderInterface`) must change their signatures to type-hint against the new interface. That is done in a BC way, which is to make the concerned methods virtual until 6.0, with deprecation notices triggered from callers and concrete implementations. Benefits: In 6.0, applications that use password-less authentication (e.g. login links) won't need to write no-op `getPassword()` and `getSalt()` in order to fulfil the `UserInterface` contract. For applications that do use password-based authentication, they will need to opt-in explicitly by implementing the relevant interface(s). This build on great discussions with @wouterj and @nicolas-grekas, and it is part of the overall rework of the Security component. Commits ------- 2764225 [Security] Decouple passwords from UserInterface
…serIdentifier() (wouterj) This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Security] Rename UserInterface::getUsername() to getUserIdentifier() | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix part of #39308 | License | MIT | Doc PR | tbd This continues the great work by @chalasr in #40267 and rounds up the `UserInterface` cleanup. The `getUsername()` has been a point of confusion for many years. In today's applications, many domains no longer have a username, but instead rely on a user's email address or the like. Even more, this username has to be unique for all Security functionality to work correctly - so it's more confusing in complex applications relying on e.g. "username+company" to be unique. **This PR proposes to rename the method to `getUserIdentifier()`**, to more clearly indicate the goal of the method (note: I'm open for any other suggestion). For BC, the `getUsername()` method is deprecated in 5.3 and `getUserIdentifier()` will be added to the `UserInterface` in 6.0. PHPdocs are used to improve type-hinting for 5.3 & 5.4. Both authentication managers (the legacy and experimental one) check the authenticated user object and trigger a deprecation if `getUserIdentifier()` is not implemented. * [x] **judge whether we need to have better deprecations for calling `getUsername()` in the application code.** Consistent with these changes, I've also done the same BC change for `TokenInterface::getUsername()`. * [x] **do the same for remember me's `PersistentTokenInterface::getUsername()`** * [x] **also rename `UserProviderInterface::loadUserByUsername()` & `UsernameNotFoundException`** * [x] **also rename `UserLoaderInterface::loadUserByUsername()`** I'm very much looking forward for feedback and help for this important, but BC-heavy, rename. 😃 Commits ------- 8afd7a3 [Security] Rename UserInterface::getUsername() to getUserIdentifier()
|
Sorry for spamming this issue again, but there is still a question that I'd like to get answered before the next Symfony version is released. Are the token roles and the user roles the same thing?
|
This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Security] Rework the remember me system | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fixes part of #39308 | License | MIT | Doc PR | tbd 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** like `ContextListener`. The `RememberMeFactory` adds a `setRememberMe()` 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 generic `SignatureHasher` in the 3rd commit. * I then remodelled `RememberMeAuthenticator` 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 the `ContextListener` 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): ```yaml # before services: Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider: autowire: true security: firewalls: main: remember_me: # ... token_provider: 'Symfony\Bridge\Doctrine\Security\RememberMe\DoctrineTokenProvider' # after security: 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 to **programmatically create a remember me cookie**, you can autowire `RememberMeHandlerInterface` and use `createRememberMeCookie($user)`. This will make sure the remember me cookie is set on the final response (using the `ResponseListener`) * The `RememberMeListener` 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 by `RememberMeConditionsListener`). If `RememberMeBadge` 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 --- * [x] Update UPGRADE and CHANGELOG * [x] Show before/after examples * [x] Investigate the conditional event registering of `ContextListener`. 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: #40145 (comment) cc `@scheb` `@weaverryan` as you both initiated this PR by sharing the problems with the current design. Commits ------- 1567041 [Security] Rework the remember me system
… (chalasr) This PR was merged into the 5.3-dev branch. Discussion ---------- [Security] Deprecate the old authentication mechanisms | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | no | New feature? | no | Deprecations? | yes/ | Tickets | #39308 | License | MIT | Doc PR | todo Now that the authenticator system proven working well and is considered stable, we can deprecate the old authentication listeners as well as the Guard component (+ integrations). Commits ------- 0bb3964 [Security] Deprecate the old authentication mechanisms
|
I think that we should answer this question before releasing Symfony 6.0
|
|
@wouterj The upcoming version 5.4 it's ending its development phase, but this issue is still open, and my previous question is still unanswered. I'd like to know your opinion on this topic. Thanks in advance.
|
|
Note that development phase isn't yet ended, but will end in a couple days/1 or 2 weeks. Anyway: I would say cases where token roles and user roles are not the same are indications of roles being "misused" (no judgement here, many applications misuse roles in some place). However, it is a use-case apparently. In any case, as I understand it, it is possible to have different token and user roles (by using |
|
My two cents on that: User roles and token roles are not the same. User roles are the roles that a user account has configured. That's just user account data. Token roles are the roles that are in effect in a given security context, it's the source of truth for security/access matters. Yes, for most cases the list of roles will be identical. Though there may be business rules involved, making the token roles different from the roles that are configured for the user. For example, my |
|
Alright, rephrase: Token roles should usually be a subset of User roles If this is not the case, you are probably using roles for something differently than what they are designed for. We however don't need to force people into our way of thinking imho: it's fine if people get it working (and they apparently do). |
|
Thank you for this issue. |
After the keynote, many people were wondering how long the new system will remain "experimental". I've been thinking about this for the past weeks and I want to work hard to make 5.3 the first stable release of the new system.
This would give us 1 release (5.4) to add features and fixes that people find when migrating to the new system. In 6.0, the old system will be removed and thus the new system must fill all gaps of the old system. While I highly recommend people to test the new security system, I can completely understand people not to upgrade their large production applications to an experimental feature.
This brings an important deadline, as it means the upcoming 6 months are the last months we can "fix security" while breaking BC. Personally, I've 3 security goals left that I think may result in BC breaks:
UserInterfaceRefactor the remember me system, see ☑️ #40145
Goal: Have the system work automatically and extracting the storage (persistent token, etc) from the logic that determines if remember me should be used (mostly
AbstractRememberMeServices)The current remember me system requires lots of manual interactions (calling
loginSuccess(),loginFailure(),autoLogin()on exactly the correct cases every time). We should use the new events (and probably add a few new ones) in order to let it run automatically at the correct cases.Also, currently it is almost impossible to customize when remember me should be activated (e.g. in 2fa situations). The
AbstractRememberMeServiceslogic should be extracted from the services and into a new class to allow customization.For some background on this topic:
At some knowledge of authentication factors (sudo mode)
Goal: Add the idea of "authentication factors" (i.e. being authenticated using 1 factor may not be considered "fully authenticated" if 2fa is required)
We already have https://github.com/scheb/2fa as a great solution for mfa in Symfony. I don't think we should replace it, but there are some things that we still have to refactor in Symfony to make the bundle work nicer (instead of working around the current limitations). @weaverryan, @scheb and I have had a discussion about this on Slack with this as some sort of conclusion: https://gist.github.com/weaverryan/d828ef860b1119862cf3828d016b8f41 This is anything but a final conclusion, but it can be used as a reference for the work on this subject.
Revisit
UserInterface, see ☑️ #40267 & #40403Goal: remove credential related methods (
getSalt(),getPassword(),eraseCredentials()) from the mainUserInterfaceThese methods are becoming "outdated" (salts are autogenerated in bcrypt/libsodium and passwordless authentication is becoming more and more common). There still is a need to have a "security user" (or principal). Ideally, we also rename
getUsername()to a more generic method (getIdentifier()or the like). This change does require to remain BC as it's also used in the old system (or can we somehow have a new-system only user interface?)I would be interested to connect with people that can help with these tasks. At the same time, I'm also interested in peoples opinion on the ideas of this issue.
If you have great idea for Security, please note that there is a larger meta issue: #30914 This issue is a subset of that issue, to cover only the changes that require the current component to still be "experimental".
The text was updated successfully, but these errors were encountered: