Replace custom password hashing library with PHP password_hash()

Our implementation has diverged from phpass due to using sha512 primitive so the hashes are no longer compatible, so I don't think we can make a direct switch to this library (without extending it in a fairly non-trivial way).

If we are going to switch/improve implementations, we should use the php 5.5 built in password hashing API, together with the backward compatibility wrapper (which supports all PHP versions D8 supports). This is stronger than our sha512 based phpass (for the same hashing workload time) due to bcrypt usage, and also an agreed standard, rather than a defacto one, so encourages broader comparability between web frameworks/platforms.

See discussion at #1201444-39: Strenghten password hashing mechanism.
https://wiki.php.net/rfc/password_hash
https://github.com/ircmaxell/password_compat
http://blog.nic0.me/post/63180966453/php-5-5-0s-password-hash-api-a-deep...

I agree with #2. If we're going to change our password hashing logic at all, it makes the most sense to just go straight to the PHP 5.5 API with a known BC layer written by the same author. No sense reinventing a wheel that is already right there in front of us.

Totally +1. Re-titling accordingly.

Just the aspect that our code still has "Phpass" in its name is a very terrible joke.

Implementing this is easy. But we need a plan for migrating those custom hashed passwords from D7. :-/

Let's start with a patch. It's just a very rough approach. I ran no tests yet and the D7>D8 password rehashing is still something that needs attention (I wrote the D6>D8 migration for user password rehash).

Removed the D7 password service injection.

Oops! Wrong patch, sorry.

You're starting with a patch for PHP 5.5 ? or you're doing this for PHP 5.5 + the fallback for PHP 5.4 ?

New version. Still under heavy development.

@killua99, yes I'm trying that approach.

• Added unit test for the new password hashing class.
• Renamed \Drupal\Core\Password\Password to more meaningful \Drupal\Core\Password\PhpPassword

Still @todo:

• Fix the password rehashing when importing from D7.

Some fixes on D7 hashed password detection when migrating.

Note:

1. The migration from D7 hashed passwords has no test coverage now because that part should belong to Migrate and Migrate Drupal modules. Also the D7 password hashing is more a migration task. However, I covered here the migration but without testing. We'll test that when D7->D8 migration will be merged in the main repository from the sandbox. Any bugs will be fixed in that stage but, at least conceptually, I fixed the issue here.
2. I attached also the patch without the external BC library and Composer generated code for review-ability reasons.

Reviews are welcomed.

I cancelled manually the test on replace_d7_password-1845004-16-wo-library-and-composer-gen.patch. Forgot to name it in the right way.

Added test for first time login after the password has been migrated from D6 or D7 and fixed the rehashing from D7 (that has a bug in previous patch).

Switching back to the previous title. It is better.

Tagging.

The patch that doesn't have the library "password.php" mean is ready for PHP 5.5 ?

@killua99, no. That one is a simplified version for reviewers.

1. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -0,0 +1,104 @@
   +    // Prevent DoS attacks by refusing to hash large passwords.
   +    if (strlen($password) > 512) {
   +      return FALSE;
   +    }
   

   I'd like to have this vetted by the security team; length-limited passwords are usually regarded with derision by most security gurus, although in this case it may be logical if the number is big enough.

   If it is kept, move the magic value to a class constant rather than make it inline?

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -0,0 +1,104 @@
   +   */
   +  public function check($password, UserInterface $account) {
   +    $stored_hash = $account->getPassword();
   +
   +    // Password migrated from Drupal 7.
   +    if (substr($stored_hash, 0, 5) == 'D7$S$') {
   +      $salt = substr($stored_hash, 2, 12);
   +      $stored_hash = substr($stored_hash, 14);
   +      $password = $this->drupal7Password->crypt('sha512', $password, $salt);
   +    }
   +    // MD5 migrated password (Drupal 6).
   +    elseif (substr($stored_hash, 0, 2) == 'U$') {
   +      $stored_hash = substr($stored_hash, 1);
   +      $password = md5($password);
   +    }
   +
   +    return password_verify($password, $stored_hash);
   +  }
   

   If I'm reading this correctly, we don't rehash an old password. That is, a user with an ancient D6 password will still have that password in the DB until they change it, rather than it being updated to the more secure modern hash at login. Is that true? Don't we want to upgrade all of those old hashes? (I don't know what we did with this in D7 so I may be barking up the wrong tree here.)

3. +++ b/core/modules/migrate/src/MigratePassword.php
   @@ -8,12 +8,13 @@
   + * Replaces the original 'password' service in order to prefix the MD5 or
   + * Drupal 7 re-hashed passwords. The new salted hash will be recreated on first
   + * login similarly to the D6->D7 upgrade path.
   

   So it is supposed to happen, but I don't see it happening above. Am I just missing it, or do we need more docs?

4. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -125,22 +123,64 @@ function testPasswordRehashOnLogin() {
   +  /**
   +   * Test MD5 (Drupal 6) and Drupal 7 passwords rehashing.
   +   */
   +  public function testMigratedPasswordRehashing() {
   

   Split this into 2 separate tests. They have little to do with each other.

5. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -172,4 +212,22 @@ function assertFailedLogin($account, $flood_trigger = NULL) {
   +  /**
   +   * Updates the hashed user password bypassing the API. We want to set an
   +   * already hashed password.
   +   *
   

   Invalid docblock. One line please.

   The "We want..." sentence can just be moved to its own line, after an extra linebreak.

@Crell: The password length check was a security issue a while ago: #2378703: Port denial of service fixes from SA-CORE-2014-006 to Drupal 8 It's just ported over from the current implementation.

@Crell,

If I'm reading this correctly, we don't rehash an old password. That is, a user with an ancient D6 password will still have that password in the DB until they change it, rather than it being updated to the more secure modern hash at login. Is that true?

Short answer: No. In every moment we have D8 hashed passwords in the DB.

Here's how it works:

What passwords are rehashed?

Password rehashing can be offered ONLY if the users are migrated using the migrate in core. Otherwise the migrate manager should take care of its own custom rehashing policy.

What happens during migration?

When the destination of the migration is a Drupal 8 user —­ \Drupal\migrate\Plugin\migrate\destination\EntityUser that class swaps the main password hasher with \Drupal\migrate\MigratePassword. This one has the main hasher injected in the constructor so he can use the D8 hashing mechanism. The service swapping is handled by \Drupal\migrate\MigrateServiceProvider that is magically (yes, we're still using magic!) called.

What \Drupal\migrate\MigratePassword does?

• For D6: is hashing (with D8 service) the already MD5 hashed password and prepend the "U" character to flag that it's a D6. We need that later. So responding to your question: Yes, we are rehashing the already md5 hashed D6 passwords with the new hash..
• D7: The D7 hashed password is rehashed with the D8 new hash but before that we are saving the salt. After that we are prefixing the D8 hash with "D7" and the Drupal 7 salt. Why we need the salt? If we don't store that we cannot check later, on user first login, that he can authenticate against D7.

Now we're safe. In order to break the passwords a hacker needs to break the D8 new hash that is based on password_hash().

Rehashing the password

When a user authenticates for the first time we also check the hashed prefix

• If starts with "U$", it's a D6 hashed password. We MD5 the plain password (sent by user), and verify against our hash (but without leading "U")
• If starts with "D7$" it's a D7. We extract the salt and pass it with the plain password to the Drupal 7 Hasher Service. Having the D7 hash we test it with D8 stored hash (only the relevant part)

EDIT (ADD): After a successful authenticating we simply use the plain pass and recreate the hash with D8 hasher.

Few small fixes and a more compact patch.

Maybe this should be a motivation for 5.5 as the minimum supported version?

@pwolanin, I fully agree :)

Thanks, Claudiu, that helps. I don't see the "reset on first login" logic, though. Is that just elsewhere outside this patch's context?

I wouldn't be opposed to requiring 5.5, but I don't know that it simplifies any code here. The BC library from ircmaxell is a straight drop in, and most of the code here is around old passwords. What would be the 5.5 benefit here, specifically? (This would be a catch and/or Alex decision either way.)

Added a comment discussed on IRC. Needs... English language review :)

+++ b/core/lib/Drupal/Core/Password/PhpPassword.php
@@ -93,6 +93,14 @@ public function check($password, UserInterface $account) {
+    //   during the migration. In this case the rehashed legacy hash is prefixed
+    //   to indicate and old-Drupal hash and will not comply the expected
+    //   password_needs_rehash() format.

"... will not comply WITH the expected..."

English looks good otherwise. ;-)

As soon as the bot is happy with it, so am I. Thanks, Claudiu!

1. +++ b/core/core.services.yml
   @@ -5,6 +5,7 @@ parameters:
   +  password_hash_cost: 10
   
   @@ -751,13 +752,14 @@ services:
    # @todo increase by 1 every Drupal version in order to counteract increases in
   -# the speed and power of computers available to crack the hashes. The current
   -# password hashing method was introduced in Drupal 7 with a log2 count of 15.
   

   So why back to 10? There does not seem to be any discussion of this on the issue.

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -0,0 +1,117 @@
   +  /**
   +   * Maximum password length.
   +   */
   +  const PASSWORD_MAX_LENGTH = 512;
   

   Lets put this on PasswordInterface

3. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -0,0 +1,117 @@
   +    // Prevent DoS attacks by refusing to hash large passwords.
   +    if (strlen($password) > static::PASSWORD_MAX_LENGTH) {
   +      return FALSE;
   +    }
   

   From one of the blogpost's linked: Anthony Ferrara (@ircmaxell) was kind enough to point out that bcrypt actually limits passwords to 72 characters internally, so it’s impossible for someone to take down your server by sending very long passwords. He also provided me with a StackOverflow link with more information on the matter.

   I guess it is okay to leave this in.

4. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -15,6 +15,7 @@
   +use Drupal\Core\Password\PhpassHashedPassword;
   

   Not used

5. +++ b/core/modules/user/src/Tests/UserLoginTest.php
   @@ -8,7 +8,8 @@
   +use Drupal\user\Entity\User;
   

   Not used.

@Crell - you are assuming that the library is correct, etc. it would be nice to reduce the number of code forks and we shouldn't ship Drupal 8 supporting php 5.4 anyhow.

I'm not sure I'm happy with the all the code changes. Can you explain:

-  protected function crypt($algo, $password, $setting) {
+  public function crypt($algo, $password, $setting) {

Also - the flag to enable MD5 is a bit odd and seems overkill - you should just read the algorithm from the setting string?

pwolanin: Well both the C function and the BC lib were written by ircmaxell, in parallel, so I am assuming that yes, they are compatible as advertised.

@alexpott

#37.1

So why back to 10? There does not seem to be any discussion of this on the issue.

That is not the same parameter as in D7 hashing. So we are not downgrading the complexity. This is the cost of password_hash() new function (see http://php.net/manual/en/function.password-hash.php). I used the default value for this option as it's used by the PHP function. It's true that there was only one discussion on that. I asked Anthony Ferrara (@ircmaxell) what is his opinion about that. He answered me: "It's enough". Other opinions would be valuable. Just keep in mind that the increase with 1 of this value doubles the time needed for a hash. Once I tested with 15 and I felt the delay.

#37.2 and #37.3: You're right. It's stated in PHP documentation:

Caution
Using the PASSWORD_BCRYPT for the algo parameter, will result in the password parameter being truncated to a maximum length of 72 characters.

Moved the constant to interface. In fact is useless now. Even somebody will use a password between 72 and 512, he will not be aware that only the first 72 chars were used. In fact this can lead to the next hypothetical and fun situation: A user is entering a password > 72 but he can login with a different password as long the first 72 chars are correct. Shouldn't we add a constraint to user password field limiting the length to 72?

@pwolanin

I'm not sure I'm happy with the all the code changes. Can you explain:
- protected function crypt($algo, $password, $setting) {
+ public function crypt($algo, $password, $setting) {

Yes, sure. When a D7 user logs in for the first time in D8 we need to hash his plain password first with D7 hash. But we need to use the same salt as it has been used when originally his password was hashed in D7. That's why, after the "D7" prefix we are adding the D7 salt. When we are hashing with D7 algorithm we need to be able to pass the salt (extracted from the prefix) to the hashing engine of D7. Unfortunately the only method on the interface (public) is PhpassHashedPassword::hash(). But that method doesn't take the salt as parameter. We need access to a method that can accept the salt. The straight way seems to me making PhpassHashedPassword::crypt() public so that I can access the hashing mechanism from outside and pass the salt that I want (not a random generated one).

Also - the flag to enable MD5 is a bit odd and seems overkill - you should just read the algorithm from the setting string?

That I inherited from D6 > D7 migration. Why is "U"? I have no idea. I can replace it with "D6" but in this case we have maybe to be more flexible because we can use it to rehash from Joomla, and also other old systems that uses MD5. Open to discussion.

Tagging

Needs reroll.

Reroll.

ignore interdiff... I was considering using lazy but doesn't seem needed. during upload it disappeared from the files list though so I couldn't remove it.

#37, #38 were addressed, so back to RTBC (per #36)

reroll

Back to RTBC.

+1 to getting back to standard phpass passwords.

Few questions though:

1. This doesn't remove PhpassHashedPassword.php at all - so we're adding the compat library but not actually getting rid of any of our custom code? Nor is any of it marked deprecated

2. Issue needs a beta evaluation. Lack of interoperability is a (minor) bug so I think this issue is fine just for that, but could use documenting

3. Found a couple of nits in the patch.

1. +++ b/core/core.services.yml
   @@ -749,13 +750,16 @@ services:
   +  # The first argument of the hashing service (constructor of PhpPassword) is
   +  # the 'cost' option of password_hash(). In Drupal 8 the 'cost' has the default
   +  # value used by password_hash() which is 10. Future versions of Drupal may
   +  # increase this value in order to counteract increases in the speed and power
   +  # of computers available to crack the hashes. Note that an increase of 1 will
   +  # double the time needed for password hashing.
      password:
   +    class: Drupal\Core\Password\PhpPassword
   +    arguments: ['%password_hash_cost%', '@drupal7_password']
   +  drupal7_password:
        class: Drupal\Core\Password\PhpassHashedPassword
        arguments: [16]
      accept_header_matcher:
   

   This says the cost value is 10, but we set it to 16.

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -0,0 +1,112 @@
   +    // - The password is a Drupal 6 or 7 password and it has been rehashed
   +    //   during the migration. In this case the rehashed legacy hash is prefixed
   +    //   to indicate and old-Drupal hash and will not comply with the expected
   +    //   password_needs_rehash() format.
   +    // - The parameters of hashing engine were changed. For example the
   +    //   parameter 'password_hash_cost' (the hashing cost) has been increased in
   

   typo: an old Drupal

@catch,

#54.1 : Right. We need to keep the mechanisms that makes possible the rehashing of a plain password to Drupal 6 and Drupal 7 hashes. Otherwise we cannot rehash the passwords imported (migrated) from D6 and 7. While with D6 it's easy (because the hashing mechanism is md5()), we need to keep the code that make possible to hash a D7 plain password and that is PhpassHashedPassword.php . Please read #26 for a detailed explanation.

#54.2: Added.

#54.3.1: Those two values are different things. "16" is a D7 hashing mechanism parameter called "password stretching iteration count", referred also as "countLog2" in code. "10" is something different, related to the new hasher we are implementing, and is called cost. You can see in core.services.yml that we have 2 different services: password (the new D8 hasher) and drupal7_password (the D7 hasher — that we still need for migrate). "16" is allocated to drupal7_password and "10" is the cost allocated to password.

#54.3.2: Fixed.

OK that makes sense for the 7.x passwords. I think we should add additional docs to that class to explain why it's there now, and possibly rename it though?

Also all the password hashing services are good candidates to mark as lazy.

@catch,

(..) I think we should add additional docs to that class to explain why it's there now (...)

OK.

(..) and possibly rename it though?

First I renamed but then I preferred to keep the patch footprint small. Renaming will make a bigger patch. Maybe as a followup?

Also all the password hashing services are good candidates to mark as lazy.

OK. Let's see if passes the tests.

@catch,

The problem with making LAZY the 2 services is that they need \Drupal\user\UserInterface (thank you @dawehner for clarifying me this!). Unfortunately proxies of module interfaces don't work! Also, conceptually, it is wrong that a class in \Drupal\Core depends on an interface that is in a module (\Drupal\user).

That said, and taking other comments in this issues (@catch: rename the D7 hasher class, etc.), I decided to clean up more and remove \Drupal\user\UserInterface from \Drupal\Core\Password\PasswordInterface. The methods that are requesting now the user are PasswordInterface::check() and PasswordInterface::userNeedsNewHash(). In fact we don't need the user object to be passed to the hashing engine. We need only the hashed password extracted from the user.

It's true that passing the entire user object was made with this assumption: by extending the class somebody is able to swap the mechanism and use other backend to lookup for the hash. (eg. use the uid from the account object and query an external backend). But this assumption is wrong because this interface should not be in business of abstracting the authentication authority. To switch to other authentication authority (credential database) the developer should implement the \Drupal\user\UserAuthInterface. The shipped implementation \Drupal\user\UserAuth is the place where the user database is queried. PasswordInterface must be responsible only for hashing. This is resolved now in the patch.

But after I dropped the reference to \Drupal\user\UserInterface in \Drupal\Core\Password\PasswordInterface I found out that there’s no more Drupal bootstrapped context for this interface. So, yes, this interface defines a now Component. The new namespaced name is \Drupal\Component\Password\PasswordInterface.

I also performed some docs cleanup and the LAZY stuff just works. Method ::userNeedsNewHash() is renamed to ::needsRehash().

Forgot two occurrences...

Read the comment and interdiff from #61.

That said, and taking other comments in this issues (@catch: rename the D7 hasher class, etc.), I decided to clean up more and remove \Drupal\user\UserInterface from \Drupal\Core\Password\PasswordInterface. The methods that are requesting now the user are PasswordInterface::check() and PasswordInterface::userNeedsNewHash(). In fact we don't need the user object to be passed to the hashing engine. We need only the hashed password extracted from the user.

I like that solution! It allows you to reuse the service in more places!

This now introduces a couple of API changes ... so tagging.

1. +++ b/core/core.services.yml
   @@ -749,15 +750,20 @@ services:
   +  drupal7_password:
   +    class: Drupal\Component\Password\Drupal7Password
   

   Mh, so afaik in components, there should be no Drupal specifics, what about not naming the Class Drupal7Passsword but rather keep the name PhpassHashedPasword?

2. +++ b/core/lib/Drupal/Component/Password/Drupal7Password.php
   @@ -2,21 +2,25 @@
   -class PhpassHashedPassword implements PasswordInterface {
   +class Drupal7Password implements PasswordInterface {
   
   @@ -45,7 +49,7 @@ class PhpassHashedPassword implements PasswordInterface {
   -   * Constructs a new phpass password hashing instance.
   +   * Constructs a new password hashing instance.
   
   @@ -61,13 +65,13 @@ function __construct($countLog2) {
   -   * @param String $input
   +   * @param string $input
       *   The string containing bytes to encode.
   -   * @param Integer $count
   +   * @param int $count
       *   The number of characters (bytes) to encode.
       *
   -   * @return String
   -   *   Encoded string
   +   * @return string
   +   *   Encoded string.
       */
   
   @@ -96,7 +100,8 @@ protected function base64Encode($input, $count) {
   -   * Generates a random base 64-encoded salt prefixed with settings for the hash.
   +   * Generates a random base 64-encoded salt prefixed with settings for the
   +   * hash.
       *
       * Proper use of salts may defeat a number of attacks, including:
   
   @@ -104,7 +109,7 @@ protected function base64Encode($input, $count) {
   -   * @return String
   +   * @return string
       *   A 12 character string containing the iteration count and a random salt.
   

   Do you mind trying to not clean everything up? Don't fix the docs here, please, it makes the patch way bigger than actually needed. Thank you

3. +++ b/core/lib/Drupal/Component/Password/Drupal7Password.php
   @@ -201,68 +206,69 @@ protected function crypt($algo, $password, $setting) {
   -  public function userNeedsNewHash(UserInterface $account) {
   +  public function needsRehash($hash) {
        // Check whether this was an updated password.
   -    if ((substr($account->getPassword(), 0, 3) != '$S$') || (strlen($account->getPassword()) != static::HASH_LENGTH)) {
   +    if ((substr($hash, 0, 3) != '$S$') || (strlen($hash) != static::HASH_LENGTH)) {
          return TRUE;
        }
        // Ensure that $count_log2 is within set bounds.
        $count_log2 = $this->enforceLog2Boundaries($this->countLog2);
        // Check whether the iteration count used differs from the standard number.
   -    return ($this->getCountLog2($account->getPassword()) !== $count_log2);
   +    return ($this->getCountLog2($hash) !== $count_log2);
      }
   

   So yeah, the only really needed change is this hunk, I think

4. +++ b/core/lib/Drupal/Component/Password/PhpPassword.php
   @@ -0,0 +1,108 @@
   +  /**
   +   * The algorithmic cost that should be used.
   +   *
   +   * @var int
   +   */
   +  protected $cost;
   

   Can we point to some documentation, what this means? Is it scaling linear?

5. +++ b/core/lib/Drupal/Component/Password/PhpPassword.php
   @@ -0,0 +1,108 @@
   +    // Password migrated from Drupal 7.
   +    if (substr($hash, 0, 5) == 'D7$S$') {
   +      $salt = substr($hash, 2, 12);
   +      $hash = substr($hash, 14);
   +      $password = $this->drupal7Password->crypt('sha512', $password, $salt);
   +    }
   

   So yeah I'd argue that this bit is specific to Drupal, so it should not be a component.

The API change makes a lot of sense and also the refactoring into a component.

Can we point to some documentation, what this means? Is it scaling linear?

Yes, in fact the parameter documentation of the old D7 generator has an appropriate comment. Perhaps copy that over. Another option would be to point to the PHP documentation, however that one references the crypt docs.

Btw. I do not see any reason why we would want to lower the default cost to 10. Is there any evidence that either the D7 value was to high or that the security of the new implementation with algorithmic cost 10 actually matches the security of the old implementation?

Do you mind trying to not clean everything up? Don't fix the docs here, please, it makes the patch way bigger than actually needed. Thank you

Please no. This patch was RTBC several times and the doc changes never were rejected before. So let's not throw away work which is already done.

So yeah I'd argue that this bit is specific to Drupal, so it should not be a component.

How about introducing a core password service which wraps around both (the d7 and the new password service) in order to ensure backwards compatibility on migrated sites?

Please no. This patch was RTBC several times and the doc changes never were rejected before. So let's not throw away work which is already done.

Sorry, well fair

Btw. I do not see any reason why we would want to lower the default cost to 10. Is there any evidence that either the D7 value was to high or that the security of the new implementation with algorithmic cost 10 actually matches the security of the old implementation?

Yes I also think we should set this to 16 unless there's a reason not to.

Timing the old vs. new password hashing ought to give us an idea how computationally expensive they are compared to each other.

@catch, @znerol,

I responded to this question back in #41 & #55. There's no decrease, they are simply different parameters. Also, i discussed with Anthony Ferrara / @ircmaxell (the developer of https://github.com/ircmaxell/password_compat — the PHP < 5.5 compatibility library) about the value of cost. The "cost = 10" is the default value used by password_hash() and he said "It's enough". Keep in mind that an increase of this value with 1 doubles the computation time for hashing.

Given that those are two completely different implementation, the easiest way would be to benchmark the old implementation vs the new one and come up with a cost value that leads to roughly comparable results. That would be a net increase in security, as the new implementation is expected to be much faster. Just @ircmaxell choosing 10 as the default value is a good reason enough for us to chose the same value.

@dawehner

#64.1, #64.5: Ok, ok. For me this is behaving like a component because it doesn't need a Drupal bootstrapped context but, it's true, smells as Drupal a lot. So, back to \Drupal\Core. But now, because is core, I kept the name :)

#64.2, #64.3: Thank you @znerol, @dawehner :)

#64.4: Added.

There's no decrease, they are simply different parameters.

The cost parameter of the PHP password hash function is exactly the same as the count_log2 parameter in our implementation. The only thing which changed is the hashing function (sha256 vs blowfish). We need to ensure that the amount of work necessary to crack passwords does not decrease. I highly suspect that 2^10 rounds of blowfish is much cheaper than 2^15 (drupal 7) rounds of sha256.

Keep in mind that an increase of this value with 1 doubles the computation time for hashing.

It is important to understand that this is excatly what we want. The parameter needs to be set as high as tolerable.

1. +++ b/core/core.services.yml
   similarity index 80%
   rename from core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   
   rename from core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   rename to core/lib/Drupal/Core/Password/Drupal7Password.php
   
   rename to core/lib/Drupal/Core/Password/Drupal7Password.php
   index 17cafdd..92bdd9d 100644
   
   +++ b/core/lib/Drupal/Core/Password/Drupal7Password.php
   @@ -2,21 +2,25 @@
     * @see http://www.openwall.com/phpass/
   

   @catch can we not rename this. Technically its the D7 hash algorithm but really its the forked phpass implementation.

2. +++ b/core/lib/Drupal/Core/Password/Drupal7Password.php
   @@ -45,7 +49,7 @@ class PhpassHashedPassword implements PasswordInterface {
   -   * Constructs a new phpass password hashing instance.
   +   * Constructs a new password hashing instance.
   
   @@ -61,13 +65,13 @@ function __construct($countLog2) {
   -   * @param String $input
   +   * @param string $input
   ...
   -   * @param Integer $count
   +   * @param int $count
   ...
   -   * @return String
   -   *   Encoded string
   +   * @return string
   +   *   Encoded string.
   
   @@ -96,7 +100,8 @@ protected function base64Encode($input, $count) {
   -   * Generates a random base 64-encoded salt prefixed with settings for the hash.
   +   * Generates a random base 64-encoded salt prefixed with settings for the
   +   * hash.
   

   And other cleanups. I second dawehner on leaving this out. It has not been rtbc'd it was added in #61

@catch can we not rename this. Technically its the D7 hash algorithm but really its the forked phpass implementation.

Yeah it's not a massive deal for me. However some indication of what it's there for now would be good. Eventually we should be able to remove this code - although it'll need to be there for 7-9 migrated password hashes so not before Drupal 10.

@znerol,

#71: I'm not gonna disagree with you. Then I expect an agreed value for the cost. I do not know in detail about hashing algorithms. I only had a discussion with @ircmaxell and his input was that 10 is OK.

@neclimdul,

#72.1: I think that Drupal7Password tells better what is that. PhpassHashedPassword was diverged from PhPass. So it's not PhpPass.

#72.2: Oh, please :(

#1203852-4: Increase hashing iterations for D7 and D8. wall time. 50-100ms

10 actually falls between this but it varies from the original implementation depending on various factors(its a different algorithm).

php 5.6 on laptop:
d7:     double(109.3887)
new-10: double(67.6392)
new-11: double(134.5462)
new-12: double(272.3537)

php 7 on laptop:
d7:     float(60.3045) 
new-10: float(71.7845) 
new-11: float(135.4712) 
new-12: float(272.6089)

php 5.4 in on-premise vm:
d7:     double(141.0043)
new-10: double(76.7628)
new-11: double(154.0343)
new-12: double(307.1231)

php 5.5 in AWS:
d7:     float(67.8245)
new-10: float(71.4158)
new-11: float(142.5454)
new-12: float(284.3316)

From this, I think 10 is actually in our target range and fine. I'm open to more review from other people in the security team though so I will mention this to them.

@neclimdul, thank you for benchmarking. I'm just confused a little looking in the script. For D8 you are using every time the same cost: new PhpPassword(10, $d7hash);?

The question is not really how fast it is in PHP really, but how fast could you make some optimized cracking code.

Looks like at least 11 would be safer from your numbers,

also I would consider this wrong (patch needs work):

+    // Password migrated from Drupal 7.
+    if (substr($hash, 0, 5) == 'D7$S$') {

Just leave it as $S$

Unless there is a crypt hash with id $S$ that we might be consuming. I don't see any collision with http://php.net/manual/en/function.crypt.php

I think the whole wrapping logic of the D7 class is flawed - we should just have one implementation that has a case statement on the hash indetifier

We should also set PASSWORD_BCRYPT as the algorithm, or we run the risk that a DB dump from Drupal 8 on PHP 7.1 will not work on Drupal 8 on PHP 5.6. Then we can look for the expected "$2y$" identifier

@pwolanin,

also I would consider this wrong (patch needs work):
+ // Password migrated from Drupal 7.
+ if (substr($hash, 0, 5) == 'D7$S$') {
Just leave it as $S$

No, that is not wrong. That is not a D7 hash but a D7 hash over-hashed with D8 hash and prefixed with 'D7' in the moment of migration (see \Drupal\migrate\MigratePassword). That should remain as it is (or we can replace the "D7" prefix with something else more meaningful?). Please read #26 to understand how the D6 and D7 passwords are migrated and rehashed.

I'm OK with cost = 11 but keep in mind that the count_log2 in D7 is now 15. In D8 was already increased to 16. So, I think 10 is acceptable now but can be increased later, in future releases.

Re #78

I think the whole wrapping logic of the D7 class is flawed - we should just have one implementation that has a case statement on the hash indetifier

We need the D7 hashing mechanism in D8. Why? We need to rehash the plain password when the user logs in for the very first time. But before rehashing, we need to authenticate the user (is he a valid user with a valid password?). In that moment we have his password hashed with D7 service and over-hashed with D8 (on migrate). So, we need the D7 hashing service to validate his plain entered password. I don't say we cannot mess up everything in a single service. We can, but that will be very ugly. In the patch we have a very clean, separated logic for the 2 hashers. In fact they are 2 implementations of the same interface — and that sounds good.

I think re-hashing the hashed 7 password with the 8 algorithm is pointless work. I do not think that should go in core. It also makes the D7 -> D8 migration much slower than it needs to be.

I'd rather see here a small class that routes to a checker pased on hash ID.

Overall, I basically think the approach of patch is confusing and needlessly complex since the PhpPassword class needs to have the other instance injected and needs to know how to delegate to it.

If you need to support old + new, there should be a "routing" class that does that. The PhpPassword class should stand alone and not know anything about hashes other than php bcrypt.

@pwolanin,

Should D8 store MD5 hashed passwords (from D6)? Of course not! We need to align the security of those imported/migrated passwords to D8 standards. If, for some reasons, the database data leaks, we have to make sure that the hashes are strong enough.

The same applies to D7 > D8. Why keeping in backend passwords hashed with different algorithms? After migrating all users, in D8 we will have only D8 hashed passwords. Yes, some of them will be rehashed when the user will send us the plain pass for the first time. But in every moment we are aligned to password_hash() standards in terms of computation effort needed to compute the hash.

Setting the PASSWORD_BCRYPT as explicit algo sounds good. Let's see other opinions on this, but to me this would assure that we are consistent across different PHP versions.

claudiu: I think the point is that D6 passwords are too weak of a hash, so wrapping those in another hash is sensible, but the D7 passwords are already securely hashed, just with a different algorithm; wrapping those doesn't improve security of the old passwords.

I'd say it would also considerably lower the barrier of this getting committed if the switch could happen on existing D8 site without having to manually re-hash all existing passwords first. Just because we don't officially support an upgrade path yet doesn't mean we have to try hard to break existing sites ;)

Wondering what exactly would happen on an D8 site that already has users migrated from D6 with re-hashed passwords. (Purely theoretical question of course. I of course wouldn't have such a site).

@Crell, in both cases (wrapping the D7 hash or not) we need the D7 hashing service. And, conceptually, each (D7 & D8 service) should be implementations of an interface. Well, not over-hashing the already D7 hashes is a point. I don't see what is the gain. I started from the D6>D7 model but, yes, it's true that D7 hashes are stronger.

@Berdir, I'm not sure I understood well. The patch doesn't propose a manual rehash. What would lower the committing barrier? Sorry.

@Crell, in both cases (wrapping the D7 hash or not) we need the D7 hashing service. And, conceptually, each (D7 & D8 service) should be implementations of an interface. Well, not over-hashing the already D7 hashes is a point. I don't see what is the gain. I started from the D6>D7 model but, yes, it's true that D7 hashes are stronger.

@Berdir, I'm not sure I understood well. The patch doesn't propose a manual rehash. What would lower the committing barrier? Sorry.

We don't need a D7 hashing service, we just need the D7 hashing code as a utility class that can compare an existing hash to an incoming plaintext (or already md5'd) password

re #76, the hash changes the iterations

re #87, Berdir is pointing out that d8 sites in the wild have phpass password and this patch would leave the sites broken.

Talking to pwolanin earlier today he said he'd be happy with a class containing delegation logic and classes for the hashing algorithms. This patch implements that. This has a nice side effect that you could trivially use only the php_password implementation on new installs. Also, I set it up to support parse phpass hashes so existing sites don't break which leads to the next point.

I've also kicked the can with this patch and removed all D7 migration related code(other then tests that are going to fail). Sorry about the test I just ran out of steam and wanted to get some thoughts on this.

Let me explain why I did this.
1) We don't have a migration path for D7 users so we're guessing.
2) There seems to be strong feelings that the D7 hashed passwords are strong enough for now which would mean we can just store them as is.

We can handle those two concerns in follow ups.

Conceptually:

1. I'm fine with providing an update path for 8.0.x sites. I was thinking on this when I started the patch but, because we were not on RC, I applied the rule "no update path till RC". But here the update path is trivial — we just keep the D7 hashes as they are.
2. I'm OK with importing the D7 hashes as they are — no over-hashing with D8 hash on migrate. But on first user successful login the plain password (sent by user) will be rehashed and replaced in the backend.
3. I don't like the class architecture proposed in #90. Now we have 3 services. The complain was that we are defining 2 services. I don't see any reason to have that wrapper. Injecting the D7 service into the main service is just fine. Now everything is more complex without a good reason. Also I don't like the naming based on Drupal version. Of course, Drupal7Password can be an exception but we can keep it as PhpassHashedPassword. Renaming this was only a matter of telling that this is not really a PhPass algo.

#92.1 right on
#92.2 ditto
#92.3 this seems to be the solution proposed b znerol and pwolanin. What were the service concerns?

I'm also open to better names. I literally went "Man don't know what to call this... Whatever this will work for now ." legacy delegator or something along those lines?

re: services - I don't wee why there ever needs to be more than 1 service registered. A service is something that may be swapped, right?

Let me take a shot at further simplifying.

Ok, rewrite to 1 service and hack the phpass class down to some minimal static methods.

We should probably generate a few Drupal 7 hashes and migrated Drupal 6 -> 7 hashes as test fixtures for a unit test since this code can no longer generate new Drupal 7 hashes.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -7,21 +7,13 @@
-class PhpassHashedPassword implements PasswordInterface {
...
+class PhpassHashedPassword  {

So, we are not implementing PasswordInterface anymore. Now we have 3 classes but only one service. I really don't understand what's the gain here. Now the whole logic lost its simplicity.

There is absolutely no reason for the back-compatibility code to implement the whole interface.

There is no reason for multiple services wither. We just need 1 service. If you do not want to support old/migrated/imported hashes, you would just replace the main service and use \Drupal\Core\Password\PhpPassword

1. +++ b/core/lib/Drupal/Core/Password/MultiFormatPassword.php
   @@ -0,0 +1,79 @@
   +        return PhpassHashedPassword::check('sha512', $password, $hash);
   ...
   +        return PhpassHashedPassword::check('md5', $password, $hash);
   ...
   +        return $this->phpPassword->check($password, $hash);
   

   This isn't consistent...

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   index 17cafdd..15b8246 100644
   --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   
   --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   

   Catch requested this change

3. +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   @@ -69,7 +41,7 @@ function __construct($countLog2) {
   -  protected function base64Encode($input, $count) {
   +  protected static function base64Encode($input, $count) {
   
   @@ -158,23 +88,22 @@ protected function enforceLog2Boundaries($count_log2) {
   -  protected function crypt($algo, $password, $setting) {
   -    // Prevent DoS attacks by refusing to hash large passwords.
   +  protected static function crypt($algo, $password, $setting) {
   
   @@ -204,65 +133,28 @@ protected function crypt($algo, $password, $setting) {
   -  public function getCountLog2($setting) {
   +  public static function getCountLog2($setting) {
   ...
   -  public function check($password, UserInterface $account) {
   ...
   +  public static function check($algo, $password, $stored_hash) {
   

   Why static?

4. +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
   @@ -204,65 +133,28 @@ protected function crypt($algo, $password, $setting) {
   -  public function userNeedsNewHash(UserInterface $account) {
   -    // Check whether this was an updated password.
   -    if ((substr($account->getPassword(), 0, 3) != '$S$') || (strlen($account->getPassword()) != static::HASH_LENGTH)) {
   -      return TRUE;
   -    }
   -    // Ensure that $count_log2 is within set bounds.
   -    $count_log2 = $this->enforceLog2Boundaries($this->countLog2);
   -    // Check whether the iteration count used differs from the standard number.
   -    return ($this->getCountLog2($account->getPassword()) !== $count_log2);
   

   completely gone? The interface is broken.

re #94 I think that's too limiting a definition. Swapped out or dependency. Possibly other reasons.

re #98
Again I disagree.
1) the code is more then a backwards compatibility service, its a working implementation. It could be used to integrate with phpBB for example.
2) Logic is consistent and clear
3) You can still swap out the implementation with the 3 service approach.

Reroll and test fixes for #90.

sorry. testbot.

re #101

I still don't understand why we need 3 classes. We have the main hashing service but we want to be able to rehash from D6 and D7. For that we need to keep the old hashing engines. With D6 it's easy because the hasher is in PHP, it's md5(). With D7 we have to keep the old hashing service as it is and make it available for the main service. But how? By injecting the D7 hashing service into the main service. Simple and clean.

So, I disagree that maintaining the full D7 (crufty and unused) implementation of the interface is useful. That's making core harder to maintain.

I also think the 3 services setup makes the logic here needlessly complex. You can swap the one service if you need something different.

@claudiu.cristea - the code to check an existing D7 hash can be 100% static. There is no reason to inject a service. This is not something you would every swap out.

So, I disagree that maintaining the full D7 (crufty and unused) implementation of the interface is useful. That's making core harder to maintain.

It is used and will need to be maintained regardless in all discussed approaches because we'll have migrated d7 passwords and old d8 passwords. We're talking about a handful of additional methods that will largely match their ported d7 counterparts.

I also think the 3 services setup makes the logic here needlessly complex. You can swap the one service if you need something different.

The password service could have 100 dependencies and if you replace 'password' its replaced and the dependencies go unused. I changed to the '.' separated values for the service names to clarify this.

@claudiu.cristea - the code to check an existing D7 hash can be 100% static. There is no reason to inject a service. This is not something you would every swap out.

Being static really depends on keeping the interface on the d7 class. We don't want to mix static and global state obviously.

But again I don't agree with "no reason to inject." The PhPass implementation is a dependency therefore we should inject it. Making it static just hides that fact that it is a dependency which is not necessarily a good thing. For one, it makes certain forms of unit testing very difficult.

I'm not sure I see why static code (within the same namespace) could make unit testing harder.

If so then let's put the remaining code into the single "routing" class.

DI makes sense where you might inject a different implementation using the same interface. That's not true here as far as I can tell. If core is going to use the php password implementation, then it's ok to simply instantiate an instance.

Via pwolanin:

A service is something that may be swapped, right?

Incorrect. A service is a stateless object that does something self-contained. If using a DIC correctly they can (almost) always be swapped out, but that is not the only reason to use them. "I don't know why you would swap this out" is not in any way a reason that something should not be a service. There may be other reasons to not makg something a service, but that is not one of them.

I'm not sure I see why static code (within the same namespace) could make unit testing harder.

Because a static call always means code that you cannot ever not use. It means that the smallest "unit" that can be tested is the aggregate of the service in question AND the class it's statically calling... and anything else that calls. That chain can get long very quickly if you're not careful. It's a very slippery slope. (I view every single "static utility class" we have in core in D8 as a self-inflicted bug for this reason. We should fix that in Drupal 9, or even 8.x.) While in this specific case one could argue that the chain doesn't go very far, that's not the point. If you cannot test the D8 password logic without running D7 password logic, there is something seriously wrong with the definition of "unit" and you need to fix your code.

That said, there is also no reason, strictly speaking, why the D7 password class needs to implement the same interface as the D8 password class. There are extremely few people that would be using D7's logic on D8; I would go as far as arguing that is a case we don't even want to support. (People running beta sites knew what they were getting into; Worst case, you tell everyone to use the password-request feature and you're done. kthxbye.)

The problem with having the D7 hasher be a dependency of the D8 hasher is that we then need to load it on every single login. That's code that will *never* run on a site installed fresh, and will run less and less on an upgrade site over time. The 95% case is that the D7 hasher is not needed, so we don't need to spend the time on it.

@Crell - so take a look at my patch - I assume the static code is only loaded if you ever hit the relevant branch in the case statement?

I also agree that we should not be supporting/enabling people to use the old algorithm if we are moving to a new and more standard one.

Also - I'm remembering now that we made the interface take an account object on purpose so we could authenticate using some scheme other than local password hash comparison if desired. There doesn't seem to be any justification for this API change in the issue summary.

Given that this feature could be added to 8.1, etc, with no migration path needed, I'm really questioning why we need to do it for 8.0.0.

Also - I'm remembering now that we made the interface take an account object on purpose so we could authenticate using some scheme other than local password hash comparison if desired.

That is wrong as architecture. This service should do only one thing: provide hashing service. The same thing is now implemented in PHP >= 5.5.0 by the suite of password_*() functions. If you want to swap the authentication authority that should be done by implementing \Drupal\user\UserAuthInterface. The shipped implementation \Drupal\user\UserAuth is the place where the backend is queried for the stored hash. And that is correct. Also we cannot make the services "lazy" because #2408371: Proxies of module interfaces don't work.

I'm on the fence as to whether this is a feature request (because the goal of the issue is to add interoperability) or a task (because why?). It's definitely not a bug, so changing that claim in the beta eval.

I'm also inclined to agree with @pwolanin that this should be postponed in either case due to its disruption. Even with migration paths and BC this is a risky change to introduce during the beta and liable to add technical debt, so my perspective is that the disruption outweighs any impact in terms of interoperability or potential security hardening.

I'm happy to discuss disruption.

First, I think we can roll back the method name changes and user object to string change if that's a blocker. Those are the real "API" changes and they where done to support the lazy tag on the service. Really we are probably not going to gain a lot from a lazy proxy because we don't do anything in the constructors of these classes so the cost of instantiating them over their proxies is probably negligible.

Also, I think some of the other disruptions are a bit overstated.

1) migration path. Technically there is not update hook or migration code. Earlier d8 hashes and d7 hashes get rehashed the same way we would rehash them if we upped the cost value. Its is just using a new hashing library for the new hash.
2) Technical debt. Why technically adding any code is technical debt, really we are trying to removing technical debt by deprecating a hashing library we maintain internally in favor of one maintained by PHP.

xjm: Is the disruption greater in beta than in 8.1? "We changed all your passwords" seems like a more invasive thing to do in a point release than in beta...

So, claudiu.cristea makes a valid point about this interface being the wrong place to manage external auth.

Perhaps that's a bug and I'd support changing just the interface to take the plain hash so we could revisit the rest of this in 8.1.

@Crell - the stored hash is only changed when a user logs in, so it's not really disruptive.

Wow! Could the patch differentiate between users logging in via the regular interface vis a vis a backend login such as Services ? Where I work, Drupal is integrated via https://www.drupal.org/project/ldap, so there is no way the password can be changed upstream.

@giorgio79 - you'd probalby be working at a higher level like \Drupal\user\UserAuthInterface to handle local vs remote users?

Based on discussion with catch, created this issue to split out just the interface change: #2503083: Simplify PasswordInterface so it's not coupled to UserInterface

Note: We are going with 5.5 now so I think we shoudl no longer provide a wrapper here.

@dawehner, right. Changing the title and IS.

Oops! That auto-complete :)

We should fix the interface in the other issue and postpone changing the code for 8.1

Agree with postponing but, as @Crell said in #114, beta is a better moment to make this switch than 8.1.x. Moving back to 8.0.x.

One of the reasons for going 5.5 is to avoid pulling the rug out from under people in a point release. The same argument applies here. Let's do it now for 8.0.0.

@claudiu.cristea @Crell The Beta Evaluation could be improved to explain the need to get this in now.

Updating issue summary accordingly.

Here's a patch built on top of #2503083: Simplify PasswordInterface so it's not coupled to UserInterface. The patch is based on #70 but with the next changes:

• No more compatibility library due to #2296557: [policy] Require PHP 5.5
• D7 hashes are not over-hashed with D8 hash when migrating but only rehashed on first successful login.
• We explicitly use the PASSWORD_BCRYPT algo. See #78.
• The public method ::check() of PasswordInterface was renamed to ::verify() to be consistent with PHP 5.5 password functions naming
  

  PHP 5.5 function	PasswordInterface method
  password_hash()	::hash()
  password_verify()	::verify()
  password_needs_rehash()	::needsRehash()

There's a patch built on top of #2503083: Simplify PasswordInterface so it's not coupled to UserInterface and a non-testing relative patch. Anyway the test will fail because the bot is still on PHP 5.4.

Just to run the test. I will switch it back later.

Simplify PasswordInterface so it's not coupled to UserInterface Is in.

Yes, but is still postponed on #2508231: Raise minimum required version of PHP to 5.5.9.

@Crell - I don't understand your argument about point releases.

Also, I still don't think the last patch is the right approach (there is no need to add a service, and the code to make D7 hashes should be removed), and as a feature request this should still wait for 8.1

It was wrongly changed to Feature request. It's a Task because:

1. No new functionality is added!
2. Removes the 100% custom hashing engine that is hard to maintain and Drupal should not be in the business of developing/maintaining a password hashing library.
3. The actual mechanism provides no interoperability.
4. This is an effect of #2508231: Raise minimum required version of PHP to 5.5.9. We are moving to PHP >=5.5.9, right? Then why keeping code that is doing things already covered better by PHP? We are just taking advantage of what PHP offer, no new feature.

What (I think) @Crell pointed out in #124 and I agree is that this switch introduces a kind of disruption. The passwords will be rehashed in the backend on first successful login. Better to do this on BETA phase not, later between 8.0.x and 8.1.x, when will affect (maybe) one million of websites. Even the transition of passwords seems smooth let's take this precaution. Doing the switch now will affect about 6,000 websites (according to https://www.drupal.org/project/usage/drupal, most of them not in production in this moment.

Sorry, but I disagree with most of those points, and in #111 the core committer agreed it was a new feature and should be postponed to 8.1

A release manager has already stated their opinion on this. Unless there are new facts, we should go with that decision.

@claudiu.cristea: Would you be so kind and post the documentation fixes as a separate issue?

Thanks everyone!

@catch and I chatted about this issue a bit more this morning. We decided that we should consider this issue a task, and that there is long-term value in making the change before 9.x without blocking this on feature thaw. We also agreed that it would be better still to postpone this change to 8.1.x (or any minor), even if it's slightly more work to update core then, because we are in a late beta and should be restricting changes now.

@catch also suggested that we could open a separate issue now just for prefixing the 8.x hashes.

@xjm - not sure I follow. I don't think there is any new prefixing that's needed?

Removes the 100% custom hashing engine that is hard to maintain and Drupal should not be in the business of developing/maintaining a password hashing library.

If we could actually remove it that would be great, but we can't do that until the release that drops core support for upgrading from Drupal 7, which unless something changes with migrate/core cycles is going to be Drupal 10. This just means making the change in a Drupal 8 release, any Drupal 8 release, before 7.x support is dropped. Since we need the library to handle Drupal 7 hashes anyway.

And even when we drop support for upgrading from Drupal 7, there will be some users who did not log in for a long time, and still have old password hashes.

Those users would then not be able to log in at all, and would have to reset their passwords.

Since this is a problem with every Drupal 7 site, a release or two of Drupal 8 sites doesn't make a huge difference here IMO. We're still going to end up re-hashing potentially billions of passwords in the wild over the next 6-7 years either way.

for 8.0 releases I guess we can just use https://www.drupal.org/project/php_password

@neclimdul, nice. But I wonder what happens if the admin uninstalls that module :)

No one can log in. The module should probably take some pain's to make that exceptionally clear but that's the decision we made. Its not really any different from any other module that decided to implement PasswordInterface and provide a password hashing replacement.

Filed #2511806: Fix documentation in password hashing class

8.1.x has come and gone, guess this is no longer postponed :)

try to re-roll #127

@slasher13, the approach from #127 is no more actual. We cannot go with that solution anymore. The issue is assigned to me, meaning I'll come with a proposal shortly.

Because we missed this on 8.0.x BETA release, now we have to keep some BC. Updated the IS accordingly.

No intediff has been provided as is not relevant.

+++ b/core/lib/Drupal/Core/Password/PasswordInterface.php
@@ -33,21 +33,30 @@ public function hash($password);
+   *
+   * @todo Rename this method to verify() in Drupal 9.0.x, to match the naming
+   *   of corresponding PHP >=5.5.0 password hashing function password_verify().
+   *

We can make this change in 8.x if we add a new interface extending the old interface and deprecate the old method. Doesn't need to happen here, but maybe open an issue for that and remove the 9.x @todo? Need to get into the habit of this for #2822727: [policy, no patch] Adopt a continuous API upgrade path for major version changes.

Any particular reason for the Legacy rename, is it just to make it clear it shouldn't be used by itself?

@catch, but I'm not very happy. Looks like a BC break in UserAuth. If a module has swapped that service, it will expect a PasswordInterface but will get a PasswordHashInterface.