8000 [RFC] Symfony Security rework tracking issue · Issue #30914 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC] Symfony Security rework tracking issue #30914

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
Closed
24 of 35 tasks
curry684 opened this issue Apr 6, 2019 · 42 comments
Closed
24 of 35 tasks

[RFC] Symfony Security rework tracking issue #30914

curry684 opened this issue Apr 6, 2019 · 42 comments
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@curry684
Copy link
Contributor
curry684 commented Apr 6, 2019 •
edited by wouterj
Loading

After discussions at EU FOSSA Hackathon we have some ideas on how to rework the Security component. I'm writing this "tracker issue" to gather up the info and choices made thus far.

The biggest functional issues have been discussed frequently over the years (#10316 and others) that the authentication makes some shortcut assumptions no longer valid in 2019:

  • The object of security considerations is not always a user, it may be a device, a computer, an API key, or any number of other abstract things. In the security world the concept of "something that can participate in security questions" is called a Principal.
  • There should be a functional separation between the Principal performing an action and the Context he is in while doing so. Authorization decisions should be based on the presented Context. This solves many practical issues like impersonation and "act as company".
  • Principals may be authenticated by various means, and be oblivious themselves on the subject. Principals should therefore not expose functionality to get passwords or roles as they may not have any. Tokens handle authentication.

The Symfony Security component was originally based on the Java Spring Security framework, but has deviated over the years. Today Spring Security has both fixed some structural issues that Symfony has (the notion of principals versus users and no central assumption that passwords and roles exist) and has some features that are currently lacking or hacky at best (structural support for OpenID/OAuth, support for 2FA). Conclusion for now is that most of the current Security component, especially authorization, is mostly fine and does not need a lot of work, and it wouldn't be all that hard to fix the authentication parts, and more strongly decouple authentication and authorization, by taking some current inspiration from Spring again.

Once these changes are propagated a lot of other features related to authentication should be easier to implement. Wishlists have been assembled:

Authentication

  • User impersonation (including multiple impersonation without having to log out) not affected by Security changes
  • Social login (login in the app using Google, Facebook, GitHub, Twitter, etc.) and generic OAuth support ([WIP][Security] OAuth2 component #31952 (comment))
  • Login throttling (limit the number of failed login attempts over a period of time)
  • Simultaneous session limiting (e.g. each user can login only from one device at the same time)
  • Two-factor (or multi-factor) authentication ([Security] Native support for 2FA #28868 (comment))
  • “Sudo mode” (trigger a new authentication before sensitive operations, like GitHub) ([Security] Add "sudo mode" #33955)
  • Native JWT implementation if some package is available (firebase/php-jwt ?)
  • CSRF on router level
  • User checkers (allow to make checks after user has logged in to block the log in if needed)
  • LDAP (authenticate against a LDAP server) not affected by Security changes
  • Support for third-party authentication services (e.g. Auth0: https://auth0.com/)
  • Support for SAML 2.0 (authentication and federation mechanism)
  • Support pre-authentication mechanisms (e.g. X.509 certificates, Request-Header authentication, etc.)
  • Have a centralized token invalidation mechanism ([RFC] Symfony Security rework tracking issue #30914 (comment)) ([Security] Rework the remember me system #40145)
  • Easy to add new authentication mechanisms (as Guard currently offers)
  • Easy customization of provided authentication mechanisms (especially form_login, which is the most complex and most used one): through events/hooks? Or implement those as Guards, we can easily decorate/override?
  • More events: at least having an event before a login is attempted, which could be used to implement custom rate-limiting mechanism
  • Constant timing user login for non-existent user (to protect leaking existence of a user) - see https://github.com/paragonie/airship/blob/8f04f071c414c3893cf66311839d20a343af1237/src/Engine/Security/Authentication.php#L161-L168 for technical details

Authorization

  • "Access control" to set permissions over web site sections using regexps not affected by Security changes
  • Native @Security annotation to set the permissions of controller classes and/or actions not affected by Security changes
  • Grant or deny permissions based on trust levels, such as whether 2FA is enabled or "sudo mode" is required
  • Having an opinionated way to set up object-level control mechanism (CRUD). Voters tend to be a little bit too general in my opinion, making necessary to set up per project conventions (ie, you have to check for “EDIT” attribute…), which can’t be enforced programmatically now.

Users

Dropping the notion of users at the lowest level must not mean DX suffers and developers need to write a lot of boilerplate again. In fact directly implementing PrincipalInterface should likely be considered an advanced subject, and we should provide a layer with abstract and stock implementation of common user-based scenarios:

  • Allow to represent application users with a User class (bonus: help you create it with make:user). not affected by Security changes
  • Allow to get a User object very easily from a Twig template. not affected by Security changes
  • Allow to easily differentiate anonymous users and logged in users. ([Security] Removed anonymous in the new security system #36574 (comment))
  • “Remember Me” feature (access to the app without having to log in again; and allow to differentiate those users from both anonymous users and users who logged in for real) ([Security] Rework the remember me system #40145)
  • Roles (to have different kinds of users: normal users, admins, etc.) not affected by Security changes
  • Allow to hash user passwords (Bcrypt, Argon2i, etc. but not old algorithms with custom salts) (
  • Password rehashing (to upgrade from Bcrypt to Argon2i transparently) ([Security] Extract password hashing from security-core - with proper wording #39802)
  • "I forgot my password" (implemented as make::reset-password)
  • Email validation on registration (tracked in Verification system after registration maker-bundle#542)
  • Allow to log in users programmatically in an easy way (e.g. $security->login(‘username’))
  • Make it trivial to test protected resources (logging in a user in tests should be very easy)
  • Password validators (check if password is too simple, if it has been compromised before, etc.)
  • Have minimal requirements/assumptions about User class from Security component, so decoupling is easier (which is a pretty popular topic, actually). Only requiring username/password, likely? ([Security] Decouple passwords from UserInterface #40267)

Non-wishlist

  • As we are committed to using voters for authorization, and do not want to have/support 2 mechanisms serving similar or mostly identical purposes we will not implement ACLs again

/cc @wouterj @derrabus @linaori @sstok
This was referenced Apr 6, 2019
Closed
Closed
Closed
@nicolas-grekas nicolas-grekas pinned this issue Apr 7, 2019
@nicolas-grekas nicolas-grekas changed the title [WiP] Symfony Security rework tracking issue [RFC] Symfony Security rework tracking issue Apr 7, 2019
@nicolas-grekas nicolas-grekas added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Apr 7, 2019
@curry684 curry684 mentioned this issue Apr 7, 2019
Closed
@kaznovac kaznovac mentioned this issue Apr 7, 2019
Closed
@quentinus95
Copy link
Contributor

Following your comment on #28868, I wanted to add what I wrote there about authentication.

The current implementation considers authentication as a binary state (authenticated / not authenticated), and we use additional roles and rules (IS_AUTHENTICATED_FULLY) to determine the amount of trust we give to the authenticated principal.

The main issue is that we use authorization to describe something purely related to authentication. Something interesting could be to change this idea of binary status and replace it with a level of trust (e.g. an authentication through a form_login gives you 100 points). Having a 2FA form / using the remember me feature / having multiple factors could have an impact on this level of trust (e.g. using login_form + 2fa gives you a total of 150 points, using remember me gives you a total of 80 points, ...).

We could use this mecanism to ease authentication controls by defining, for instance, a minimal level of trust either using firewalls or path based security rules. This would give a lot of flexibility to craft advanced authentication mecanisms.

@curry684
Copy link
Contributor Author
curry684 commented Apr 8, 2019

@quentinus95 interesting thoughts. You should however remember that the process of authentication itself is handled by Tokens, so the process of how an authentication chain ends up at a certain level of trust should be handled there. This is even already being improved by the proposal at #30765.

In a nutshell right now we're pretty limited with the RememberMeToken granting IS_AUTHENTICATED_REMEMBERED, and just about every other one ends up with IS_AUTHENTICATED_FULLY. We'll need more granular control there to indeed implement some of the scenarios you describe and are also in some form in the to-do list of this RFC, specifically

  • Require 2FA to be enabled for the user for certain highly dangerous actions
  • Implement "sudo mode", requiring reauthentication to temporarily get into an "elevated" authentication

@curry684
Copy link
Contributor Author
curry684 commented Apr 8, 2019

Updated RFC with some minor things:

  • Added trust levels to authorization section
  • Renamed Identity to Context to comply with Spring terminology and to reduce confusion with .NET which implement Identity a bit differently)
  • Clarified object level permission description to explicitly mention CRUD, there should be some easily usable extension to Voters to implement CRUD functionality at object/entity levels

@vudaltsov
Copy link
Contributor
vudaltsov commented Apr 8, 2019 •
edited
Loading

@curry684 , this is a great list, thank you!

I have some thoughts:

  • Manual authentication. Useful when authenticating user right after he has signed up. This is a common question which is usually solved by copying https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/Security/LoginManager.php and a hackish compiler pass for remember me into the project.

  • A centralized token invalidation mechanism. It seems that tokens should not validate themselves — it's not flexible and extensible. Consider a scenario, when some security issue happens at the website, and you need to invalidate all logins after a fix. Currently there's no easy way to deactivate RememberMeTokens. Once in Slack I proposed to add a TokenValidatorInterface.

    interface TokenValidatorInterface
{
    public function supports(TokenInterface $token): bool;

    public function isValid(TokenInterface $token/*, $someOtherArgs */): bool;
}

    By the way, the code from AbstractToken::hasUserChanged() could be implemented under such an interface easily.

@teohhanhui teohhanhui mentioned this issue Apr 11, 2019
Merged
@teohhanhui
Copy link
Contributor

Relevant comment on password rehashing: #31019 (comment)

@curry684
Copy link
Contributor Author

@vudaltsov first one is already on the list as manual authentication, second one added, thanks!

@teohhanhui given that in that issue "the framework" has made a choice on the subject I'm not seeing an RFC item there. It's of course a valid discussion but not something to have here right now I think.

@teohhanhui
Copy link
Contributor

@curry684 Not the PR, but my comment. Well, I should just put it here:

https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/reference/html5/#pe-dpe
https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/api/org/springframework/security/crypto/password/DelegatingPasswordEncoder.html
https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/api/org/springframework/security/crypto/factory/PasswordEncoderFactories.html#createDelegatingPasswordEncoder--

@curry684
Copy link
Contributor Author

Hmm interesting, thanks 👍

@teohhanhui teohhanhui mentioned this issue Apr 17, 2019
Closed
@chalasr chalasr mentioned this issue Apr 18, 2019
Closed
@chalasr chalasr mentioned this issue Apr 26, 2019
Merged
@rpkamp
Copy link
Contributor
rpkamp commented May 25, 2019 •
edited
Loading

For login throttling you could also consider device cookies: https://www.owasp.org/index.php/Slow_Down_Online_Guessing_Attacks_with_Device_Cookies

That would need some state about logins saved somewhere, but I suppose implementing the functionality and leaving an interface for the user to implement storage -possibly with adding defaults in e.g. DoctrineBridge- might work.

@Guikingone
Copy link
Contributor

Hi 👋

I was thinking about the OAuth provider, what about using the HttpClient and provide a default system similar to what is available in PHPLeague: https://github.com/thephpleague/oauth2-client ? 🤔

Any idea?

@stephanvierkant
Copy link
Contributor

Is this the right issue to note that \Symfony\Component\Security\Core\User\UserInterface::getSalt() makes no sense with modern hashing functions?

This was referenced Jun 7, 2019
Closed
Closed
@chalasr
Copy link
Member
chalasr commented Jun 11, 2019

I'm going to work on the JWT part.

@chalasr chalasr mentioned this issue Jul 31, 2019
Merged
@ro0NL
Copy link
Contributor
ro0NL commented Jul 31, 2019

Is this the right issue to note that \Symfony\Component\Security\Core\User\UserInterface::getSalt() makes no sense with modern hashing functions?

for this id like to propose a new contract, as we're heading for encodePassword($pass, null) and have this useless arg

@hktr92
Copy link
hktr92 commented Oct 24, 2019

I have an internal web app at work that requires impersonation chaining. the current impl supports only one impersonation and there are issues when you impersonate yourself (you shouldn't be allowed to do so). so it might be useful to be in the wild, but defaulting to allow a single impersonation in the chain.

I'll try to experim this feature internally (and bug fixing) and if it were to be implemented in the base code, i'll be willing to implement it in the component.

@wouterj wouterj mentioned this issue Dec 1, 2019
Merged
@hkdobrev
Copy link
Contributor

Could we have new Symfony contracts for the token interface and the user interface if applicable? What do you think?

@linaori
Copy link
Contributor
linaori 8000 commented Mar 24, 2020

@hkdobrev see #35237, unless there's a very good reason, I don't think it will happen

@scheb scheb mentioned this issue Mar 24, 2020
Closed
@wouterj
Copy link
Member
wouterj commented Apr 22, 2020 •
edited
Loading

With the merge of the new experimental security (ref #33558), the following boxes can be ticked 😃

Authentication

  • User checkers (allow to make checks after user has logged in to block the log in if needed)
  • Support pre-authentication mechanisms (e.g. X.509 certificates, Request-Header authentication, etc.)
  • Easy to add new authentication mechanisms (as Guard currently offers)
  • Easy customization of provided authentication mechanisms
  • More events: at least having an event before a login is attempted, which could be used to implement custom rate-limiting mechanism

Users

@curry684
Copy link
Contributor Author
curry684 commented Apr 22, 2020 •
edited
Loading

Nice work! I don't think you need me to physically tick them right? edit: ticked them off in case you do 😉

@wouterj
Copy link
Member
wouterj commented Apr 22, 2020 •
edited
Loading

@curry684 I don't have special permissions for the symfony/symfony repository. So either you or a core team member can tick them.

edit: thanks a lot for the quick response!

@simonberger simonberger mentioned this issue Apr 25, 2020
Merged
@jrushlow
Copy link
Contributor
jrushlow commented May 5, 2020

From the original post:

"I forgot my password" (provide a default implementation for that or simplify a lot its implementation) (Note: currently under review in the MakerBundle)

This feature has been implemented in the maker bundle make:reset-password using symfonycasts/reset-password-bundle

https://github.com/symfonycasts/reset-password-bundle

@jrushlow
Copy link
Contributor
jrushlow commented May 5, 2020

Another user related function would be email verification. This is also a WIP and should be ready soon.

symfony/maker-bundle#542

@curry684
Copy link
Contributor Author
curry684 commented May 6, 2020

Cheers, updated OP!

@jrushlow
Copy link
Contributor

Another user related function would be email verification. This is also a WIP and should be ready soon.

symfony/maker-bundle#542

@curry684 We just merged this in a few moments ago - another check mark on the list

@curry684
Copy link
Contributor Author

@jrushlow checked off 😄

@wouterj wouterj mentioned this issue Jun 4, 2020
Closed
@simonberger simonberger mentioned this issue Jun 14, 2020
Closed
@fabpot fabpot unpinned this issue Jul 24, 2020
@wouterj wouterj mentioned this issue Dec 4, 2020
Closed
@symfony symfony deleted a comment from mynameisbogdan Feb 23, 2021
@symfony symfony deleted a comment from wouterj Feb 23, 2021
@wouterj
Copy link
Member
wouterj commented Jun 8, 2021 •
edited
Loading

So after a long ride, I think it's time to close this issue. Starting with this issue, the security component has received lots of attention the past 2 years. I'm happy to see that almost all items on this list are ticked off. The remaining ones are either on the list for 5.4 in specialized issues or already fixed by a community bundle (which we may or may not merge in Symfony one day).

Thanks a lot for everyone that has in some way been part of this long journey!

@wouterj wouterj closed this as completed Jun 8, 2021
@stof
Copy link
Member
stof commented Jun 8, 2021

Regarding password validators, I think we can consider it covered too:

  • compromised passwords are covered in the core
  • min length is covered in the core
  • password complexity requirements are explicitly discouraged by the latest NIST password guidelines, so I'm fine with not having them in the core. For projects that still want to use them, there is https://packagist.org/packages/rollerworks/password-strength-bundle

@curry684
Copy link
Contributor Author
curry684 commented Jun 8, 2021 •
edited
Loading

It's been a happy ride indeed 👍

See you next FOSSA event 😉

password complexity requirements are explicitly discouraged by the latest NIST password guidelines

@stof is the discouragement explicit and complete now? I'm fully aware requiring 'complex passwords' has been discouraged for good reasons for several years in most policies, however to my knowledge it has always been recommended to require some basic level of entropy by disallowing dictionary words, excessively short passwords and common 'canned' passwords like qwerty, nimda or 1234.

@wouterj
Copy link
Member
wouterj commented Jun 8, 2021

@curry684 see https://pages.nist.gov/800-63-3/sp800-63b.html#memsecretver

When processing requests to establish and change memorized secrets, verifiers SHALL compare the prospective secrets against a list that contains values known to be commonly-used, expected, or compromised. [...]
Verifiers SHOULD offer guidance to the subscriber, such as a password-strength meter [Meters], to assist the user in choosing a strong memorized secret. [...]
Verifiers SHOULD NOT impose other composition rules (e.g., requiring mixtures of different character types or prohibiting consecutively repeated characters) for memorized secrets. [...]

@curry684
Copy link
Contributor Author
curry684 commented Jun 9, 2021

From the same link I was actually right:

Verifiers SHALL require subscriber-chosen memorized secrets to be at least 8 characters in length.
[...]
When processing requests to establish and change memorized secrets, verifiers SHALL compare the prospective secrets against a list that contains values known to be commonly-used, expected, or compromised. For example, the list MAY include, but is not limited to:

Passwords obtained from previous breach corpuses.
Dictionary words.
Repetitive or sequential characters (e.g. ‘aaaaaa’, ‘1234abcd’).
Context-specific words, such as the name of the service, the username, and derivatives thereof.

If the chosen secret is found in the list, the CSP or verifier SHALL advise the subscriber that they need to select a different secret, SHALL provide the reason for rejection, and SHALL require the subscriber to choose a different value.

Given that we cover minimum length and breach lists I agree it's enough and the rest is optionally pluggable. The confusion was in what 'complexity' entails, I consider disallowing dictionary words a complexity thing.

@pbowyer
Copy link
Contributor
pbowyer commented Jun 9, 2021

Thank you to everyone who's contributed, it's great to have these improvements in Symfony!

@chalasr chalasr mentioned this issue May 21, 2022
Closed
@Spomky Spomky mentioned this issue May 15, 2023
Merged
@Spomky Spomky mentioned this issue Nov 1, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0