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.

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

[RFC] Symfony Security rework tracking issue #30914

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

Comments

@curry684
Copy link
Contributor

@curry684 curry684 commented Apr 6, 2019

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 (#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 (#28868 (comment))
  • “Sudo mode” (trigger a new authentication before sensitive operations, like GitHub) (#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 (#30914 (comment)) (#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. (#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) (#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) (#39802)
  • "I forgot my password" (implemented as make::reset-password)
  • Email validation on registration (tracked in symfony/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? (#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

@quentinus95
Copy link
Contributor

@quentinus95 quentinus95 commented Apr 8, 2019

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 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 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 vudaltsov commented Apr 8, 2019

@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
Copy link
Contributor

@teohhanhui teohhanhui commented Apr 11, 2019

Relevant comment on password rehashing: #31019 (comment)

@curry684
Copy link
Contributor Author

@curry684 curry684 commented Apr 12, 2019

@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.

@curry684
Copy link
Contributor Author

@curry684 curry684 commented Apr 12, 2019

Hmm interesting, thanks 👍

@rpkamp
Copy link
Contributor

@rpkamp rpkamp commented May 25, 2019

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

@Guikingone Guikingone commented May 28, 2019

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

@stephanvierkant stephanvierkant commented Jun 3, 2019

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

@chalasr
Copy link
Member

@chalasr chalasr commented Jun 11, 2019

I'm going to work on the JWT part.

@ro0NL
Copy link
Contributor

@ro0NL 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

@jkufner
Copy link

@jkufner jkufner commented Oct 13, 2019

Immutable Value Objects

The use of Doctrine for persistence tends to influence API designs in subtle ways that complicate the use of persistence layers based on different concepts (e.g., CQS). Please make sure that you avoid as much assumptions on persistence layers as possible, even for reading.

It would be nice to make a few explicit statements, for example that Tokens are immutable, or that UserInterface is an immutable value object, not only a DTO (the difference is in equivalence definition: value objects equal when they contain the same value, but DTO does not define such a thing).

When I was implementing my custom security component, I found the tokens are a good Idea, but their workflow is a bit confusing and complicated. This may be only a matter of documentation, but some clarification would help.

Another thing is the UserInterface interface. I think that the whole concept of a User entity in the security component is flawed (and the result of the Doctrine influence I mentioned in the beginning). There should not be a User in the first place. The UserInterface::getPassword() method assumes the User entity knows its (hashed) password; that may not be the case at all (e.g., SSO or LDAP). Instead, there should be a UserIdentity entity — a value object that identifies the user (or a principal in general). Then there should be a service that answers the questions of the authentication component, like "Is this password for this user identity valid?" or "Does the user of this identity have this role?", or even better "Does the user of this identity fulfill these constraints?". Such a separation will make the whole security component more robust and versatile. Moreover, it will concentrate the decissions in one place so that the code will be easier to understand and more secure (no User::eraseCredentials() method needed because the passwords never leave the question answering service and the tokens).

@vudaltsov
Copy link
Contributor

@vudaltsov vudaltsov commented Oct 14, 2019

@jkufner , fully agree. In fact Doctrine does not make you break encapsulation. I do DDD with absolutely no getters and setters and still map my aggregates successfully with Doctrine, since it works with properties directly. So what you say can be achieved with Doctrine as a possible persistence manager as well.

@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Oct 17, 2019

CSRF: #34004

@hktr92
Copy link

@hktr92 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.

@hkdobrev
Copy link
Contributor

@hkdobrev hkdobrev commented Mar 23, 2020

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 linaori commented Mar 24, 2020

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

@wouterj
Copy link
Member

@wouterj wouterj commented Apr 22, 2020

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

  • 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) (implemented by #35997)

@curry684
Copy link
Contributor Author

@curry684 curry684 commented Apr 22, 2020

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 wouterj commented Apr 22, 2020

@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!

@jrushlow
Copy link
Contributor

@jrushlow 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 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 curry684 commented May 6, 2020

Cheers, updated OP!

@jrushlow
Copy link
Contributor

@jrushlow jrushlow commented May 29, 2020

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

@curry684 curry684 commented May 30, 2020

@jrushlow checked off 😄

@wouterj
Copy link
Member

@wouterj wouterj commented Jun 8, 2021

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 Jun 8, 2021
@stof
Copy link
Member

@stof 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 curry684 commented Jun 8, 2021

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 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 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 pbowyer commented Jun 9, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet