Skip to content

[Security] Add event to inspect authenticated token before it becomes effective #37359

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

scheb
Copy link
Contributor

@scheb scheb commented Jun 19, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR n/a

Hello there, I'm the author of scheb/two-factor-bundle, which extends Symfony's security layer with two-factor authentication. I've been closely following the recent changes by @wouterj to rework the security layer with "authenticators" (great work!). While I managed to make my bundle work with authenticators, I see some limitations in the security layer that I'd like to address to make such extensions easier to implement.

This PR adds a new event, which is disapatched right after the authenticated token has been created by the authenticator, to "announce" it to the application before it becomes effective to the security system. The event works similar to ResponseEvent, but for security token. It allows listeners to inspect the new token before it becomes effective and - most importantly - apply modifications to it. So components other than the authenticator will be able to influence how the security token looks like, that will be set to the security layer on successful authentication.

Why would you want to do this? Of course I'm looking at this from the 2fa perspective. To make 2fa work, it's necessary to prevent a newly created authenticated token from becoming visible to the security system and therefore exposing its privileges/roles. This is done by replacing the authenticated token with a temporary "TwoFactorToken". Currently I'm doing this through dependency injection, getting all the registered authenticators and decorating them with my own token-exchange logic. This is not very clean and overly complicated, but it works. Adding this event as a hook-in point would allow for a much cleaner integration for any component that wants to have a saying in how the security token should look like.

@scheb scheb force-pushed the token-created-event branch from cb254ae to 37c6f0c Compare June 19, 2020 15:23
@chalasr chalasr modified the milestones: 5.1, next Jun 19, 2020
@scheb scheb force-pushed the token-created-event branch 2 times, most recently from 1192e19 to c469334 Compare June 23, 2020 14:50
@scheb scheb marked this pull request as ready for review June 23, 2020 15:04
@scheb scheb force-pushed the token-created-event branch from c469334 to bab908d Compare June 23, 2020 15:06
@fabpot
Copy link
Member

fabpot commented Aug 12, 2020

Thank you @scheb.

@fabpot fabpot force-pushed the token-created-event branch from 363db1c to 2030964 Compare August 12, 2020 16:06
@fabpot fabpot merged commit 31c194f into symfony:master Aug 12, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
nicolas-grekas added a commit that referenced this pull request Dec 23, 2020
…ecurity events (scheb)

This PR was squashed before being merged into the 5.1 branch.

Discussion
----------

[Security] Fix event propagation for globally registered security events

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

When new authenticator security is enabled, the `AuthenticatorManager` is using its own firewall-specific event dispatcher. To allow security events being listened to on the global level, `RegisterGlobalSecurityEventListenersPass` is there to automatically add globally registered event listeners to the firewall-specific event dispatchers.

`RegisterGlobalSecurityEventListenersPass` contains a list of events that are propagated, but unfortunately this list is incomplete as there are other events in `AuthenticatorManager` that would need too be propagated. So I added the missing (older) security events. These older events may also be registered by their name, rather than the FQN of the class, so I've also added those.

As this is targeting 5.1, I'll file another PR for the `AuthenticationTokenCreatedEvent` that was introduced in 5.2, as soon as this change was merged into 5.x.

On a note, I feel this "whitelist" approach to propagate security events to the global dispatcher isn't that great, because it's prone to error. Additional security events may be added in the future and adding these to `RegisterGlobalSecurityEventListenersPass` can easily be missed. When I added `AuthenticationTokenCreatedEvent` in PR #37359 I wasn't aware of this propagation mechanic existed and also no one reviewing the PR noticed it.

Additional changes:
- Typo fix :)
- The `array_uintersect` in `RegisterGlobalSecurityEventListenersPassTest` wasn't implemented correctly *

\* That function's behavior is really odd and easy to be used in the wrong way. The callback function isn't intended to return true/false for matching items, but return -1/0/1 like sorting functions. The tests seemingly only worked by chance as returning true/false is doing pretty much the opposite of what the callback function is supposed to do.

Commits
-------

1675864 [Security] Fix event propagation for globally registered security events
@scheb scheb deleted the token-created-event branch December 28, 2020 11:40
wouterj added a commit that referenced this pull request Apr 16, 2021
…vent (scheb)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Security] Add passport to AuthenticationTokenCreatedEvent

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT

This is a follow-up to my previous PR #37359, which added `AuthenticationTokenCreatedEvent` to the new authenticator-based security system to inspect the security token before it becomes effective to the security system. It **adds the passport** that was used to generate that token to the event, so that it can be inspected as well.

Reasoning:
1) It makes the event more aligned with other security events (which are also providing the passport)
2) I see valid use-cases when you'd want to look into the passport/badges to decide if you'd want to make modifications to the security token. @Seldaek mentioned to me in scheb/2fa#74 that he'd like to have the ability to add a badge from his custom authenticator class, which then influences 2fa being triggered or not. Having the passport in the event would make that a straight forward task.

I would like to add this to Symfony 5.3, since @wouterj plans to stabilize the authenticator security system for that release, so I believe this is worth adding it now rather than later. The constructor change could be considered a BC break, but since authenticator system is experimental, I believe it's fair to make that change now before declaring it "stable".

Commits
-------

74196e0 Add passport to AuthenticationTokenCreatedEvent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants