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

[Security] add password rehashing capabilities #31153

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@nicolas-grekas
Copy link
Member

commented Apr 17, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31139
License MIT
Doc PR -
Maker PR symfony/maker-bundle#389

This PR adds a new PasswordUpgraderInterface to migrate passwords to better hash algos when users log in.

The interface is implemented on relevant user-encoder in core. When Users are managed via Doctrine, the interface should be implemented on the app's UserRepository. This means the feature is opt-in. For new projects, symfony/maker-bundle#389 generates user repositories that provide the needed implementation, so that this feature works by default. Existing projects should add the new method to their repo.

Because it was needed, Guard's AuthenticatorInterface::checkCredentials() method now takes a PasswordUpgraderInterface as 3rd argument.

On the validation side, a new MigratingPasswordEncoder allows validating a hash using several algos.

TL;DR, this should provide state of the art password management, with an authentication layer that can validate old password hashes and turn them into new ones.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch 2 times, most recently from 880d7b6 to ff71522 Apr 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@weaverryan this would impact maker bundle, we should figure out how.

@linaori
Copy link
Contributor

left a comment

I think this is a good idea! I'm not too sure about the opt-in vs opt-out of #30955. The current implementation in this PR is really nice and I prefer it over an event listener. The issue I see here, is that the functionality is shifted from something you can hook into every authenticator via an event, to you must implement this in your authenticator for it to work. This means people will have to wait for upstream changes for every implementation. In this PR I only see it in the Dao authentication provider, meaning every other built-in is still lacking this feature.

TL;DR
While I really like the idea, I think it's something harder to -implement for/get control over by- vendors and applications

Show resolved Hide resolved src/Symfony/Component/Security/Core/User/UserProviderInterface.php Outdated
*/
public function needsRehash(string $encoded): bool
{
return $this->bestEncoder->needsRehash($encoded);

This comment has been minimized.

Copy link
@stof

stof Apr 18, 2019

Member

this implementation is not enough. We also need to rehash if any other encoder than the best one was actually validating it.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 7, 2019

Author Member

I don't understand why. To me, only the best encoder should be used. Can you elaborate?

This comment has been minimized.

Copy link
@stof

stof May 9, 2019

Member

would this encoder return true in this method if the hash was not generated by this encoder but by one of the extra encoders ? Or would it fail entirely or return false ?
There is 2 reasons to rehash a password:

  • the best encoder was already used, but it now has a stronger hashing and wants to rehash the password
  • a weaker encoder was used. In this case, we want to rehash, but this code expects the best encoder to detect that case when passing it the hash generated by the other encoder (and so potentially in a different format).

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 21, 2019

Author Member

would this encoder return true in this method if the hash was not generated by this encoder but by one of the extra encoders

Yes, absolutely! There cannot be any other return value in this situation. The encoder must return true only if it supports the hash of course - that's what implementations do also.

Show resolved Hide resolved src/Symfony/Component/Security/Core/Encoder/ChainPasswordEncoder.php Outdated
@weaverryan

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Description updated with a link to an issue on MakerBundle. Changes would be minimal there, but there would be some. Thanks for thinking about that.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Thanks for the comments, continuing my experiment (I still didn't run the code yet :) ):

  • maker bundle PR submitted, see symfony/maker-bundle#389
  • upgradePassword() is now split in a dedicated PasswordUpgraderInterface
  • applicable user providers now all implement PasswordUpgraderInterface
  • Guard's AuthenticatorInterface::checkCredentials() method now takes a PasswordUpgraderInterface as 3rd argument

If anyone is courageous enough to give this a try, please do :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch 3 times, most recently from 008ad54 to 846ad0e May 7, 2019

Show resolved Hide resolved src/Symfony/Component/Security/CHANGELOG.md Outdated
*/
public function needsRehash(string $encoded): bool
{
return $this->bestEncoder->needsRehash($encoded);

This comment has been minimized.

Copy link
@stof

stof May 9, 2019

Member

would this encoder return true in this method if the hash was not generated by this encoder but by one of the extra encoders ? Or would it fail entirely or return false ?
There is 2 reasons to rehash a password:

  • the best encoder was already used, but it now has a stronger hashing and wants to rehash the password
  • a weaker encoder was used. In this case, we want to rehash, but this code expects the best encoder to detect that case when passing it the hash generated by the other encoder (and so potentially in a different format).

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch from 846ad0e to 26057d3 May 21, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

PR ready for deeper review before I invest more time in adding tests and/or split in several PRs: the code together with symfony/maker-bundle#389 works well now.

@nicolas-grekas nicolas-grekas marked this pull request as ready for review May 21, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch from 26057d3 to 41bc2c0 May 21, 2019

}
$repository = $this->getRepository();
if ($repository instanceof PasswordUpgraderInterface) {

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 21, 2019

Contributor

A silent failure seems odd here. Maybe log something in debug that the user will not be saved because the repository doesn't support it? At least that way when people are trying to figure out why the feature doesn't appear to be working there is something in the logs to tell them why.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 21, 2019

Author Member

A hard failure would be even more odd: you don't want your users not being able to log in when password upgrades cannot happen. They must be opportunistic to me. Logging why not!

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 22, 2019

Contributor

I agree a hard failure would be incorrect here.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

I'm not sure anymore about the logger: it would be noisy for ppl that just don't want to implement password upgrades for some reason.
Better document this: if you need pwd upgrade, you must implement the interface.

throw new BadCredentialsException('The presented password is invalid.');
}
if ($this->userProvider instanceof PasswordUpgraderInterface && method_exists($encoder, 'needsRehash') && $encoder->needsRehash($user->getPassword())) {
$this->userProvider->upgradePassword($user, $encoder->encodePassword($presentedPassword, $user->getSalt()));

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 21, 2019

Contributor

This gives this method two responsibilies: checking the password and upgrading it if needed. Maybe this should be done at some higher -orchistration- level?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 21, 2019

Author Member

You mean the checkAuthentication method? I've no better idea, can you elaborate?

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 22, 2019

Contributor

Yes, the checkAuthentication method.
So instead of doing this inside the checkAuthentication do this in the code that calls checkAuthentication, so move it one level up to decouple the behaviours:

if ($provider->checkAuthentication(...)) {
    $provider->upgradePassword(...);
}

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

I'll have a look but I think what misses in your example is the encoder: the "if" must involve it before calling upgrade. Yet the encoder is internal detail of the provider.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

encoder is internal detail of the provider

I confirm - and so is the cleartext password btw. Same for guard.
Deadend to me.

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 22, 2019

Contributor

Seems to me it could be moved to \Symfony\Component\Security\Core\Authentication\Provider\UserAuthenticationProvider. That way it's part of the template method so all AuthenticationProviders can benefit from it, instead of being part of a particular implementation.

public function authenticate(TokenInterface $token)
{

    // ...

    try {
        $this->userChecker->checkPreAuth($user);
        $this->checkAuthentication($user, $token);
        if ($this instanceof PasswordUpgradingAuthenticationProvider) {
            // optional: if ($this->needsPasswordRehash($user, $token)) {
            $this->upgradeUserPassword($user, $token);
            // optional: }
        }
        $this->userChecker->checkPostAuth($user);
    } catch (BadCredentialsException $e) {
        if ($this->hideUserNotFoundExceptions) {
            throw new BadCredentialsException('Bad credentials.', 0, $e);
        }
        throw $e;
    }

and then \Symfony\Component\Security\Core\Authentication\Provider\DaoAuthenticationProvider would implement PasswordUpgradingAuthenticationProvider:

class DaoAuthenticationProvider extends UserAuthenticationProvider implements PasswordUpgradingAuthenticationProvider
{

   // ...

   public function upgradeUserPassword(UserInterface $user, UsernamePasswordToken $token): void
   {
      if ($this->userProvider instanceof PasswordUpgraderInterface && method_exists($encoder, 'needsRehash') && $encoder->needsRehash($user->getPassword())) {
          $this->userProvider->upgradePassword($user, $encoder->encodePassword($token->getCredentials(), $user->getSalt()));
      }
   }
}

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

optional: if ($this->needsPasswordRehash($user, $token)) {

yes but no :) this is not optional: this is part of the critical lifecycle of password migrations.
Not all auth providers have a concept of encoders. Only those should know and care about migrations. Said another way, your initial issue was SRP for the method, this proposal is moving the issue somewhere else. Let's say there is no SRP issue in the first place and everything is fine: checkAuthentication is the correct place to wire opportunistic password upgrades to me.

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 22, 2019

Contributor

My issue was with CQS, not SRP. My issue with SRP was in the naming of the ChainPasswordEncoder.

My issue here is still CQS, and I think it would be solved by my proposal.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

Sorry it wouldn't, see my objections about encoders. See also the guard interface.
Opportunistic password upgrades don't align to CQS here to me.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

But please submit a PR on my fork if I'm missing the point :)

Show resolved Hide resolved src/Symfony/Component/Security/Core/Encoder/ChainPasswordEncoder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Security/Core/User/InMemoryUserProvider.php Outdated

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch from 41bc2c0 to d0dd991 May 22, 2019

* Hashes passwords using the best available encoder.
* Validates them using a chain of encoders.
*
* /!\ Don't put a PlaintextPasswordEncoder in the list as that'd mean a leaked hash

This comment has been minimized.

Copy link
@rpkamp

rpkamp May 22, 2019

Contributor

Instead of (or: in addition to) this warning, why not put a check in the constructor that throws an exception when the PlaintextPasswordEncoder is passed?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Author Member

I wouldn't do this: technically one might have a reason to do so, we shouldn't close the door.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-rehash branch from d0dd991 to 6e0487b May 22, 2019

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 Jun 2, 2019

chalasr added a commit that referenced this pull request Jun 4, 2019

feature #31594 [Security] add PasswordEncoderInterface::needsRehash()…
… (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Security] add PasswordEncoderInterface::needsRehash()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Split from #31153, with tests.

Commits
-------

50590dc [Security] add PasswordEncoderInterface::needsRehash()

fabpot added a commit that referenced this pull request Jun 4, 2019

feature #31597 [Security] add MigratingPasswordEncoder (nicolas-grekas)
This PR was merged into the 4.4 branch.

Discussion
----------

[Security] add MigratingPasswordEncoder

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Split from #31153: the proposed `MigratingPasswordEncoder` is able to validate password using a chain of encoders, and encodes new them using the best-provided algorithm.

This chained encoder is used when the "auto" algorithm is configured. This is seamless for 4.3 app.

Commits
-------

765f14c [Security] add MigratingPasswordEncoder
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Continued in #31843

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:sec-rehash branch Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.