Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

After talking about this patch on IRC it has been suggested that this patch be reworked with the following goals:

1. Keep the Interface for Password
2. Move the password code back into User.module and implement the interface there

This patch does not give proper consideration to the pluggable nature of the password.inc. That needs to be fixed as well.

cweagans’s picture

I think this is still important. We can throw it into the DI container, which will help to make it pluggable. I'm in favor of just moving the code to a class in Component and putting a tiny wrapper (similar to config() ) in common.inc.

cosmicdreams’s picture

Assigned: cosmicdreams » Unassigned

Cool. I'll see if I have some time to learn this approach by following that code. ETA for that work is next week though. If anyone can knock this out before me please do. (unassigning until I can pick this up)

Crell’s picture

No need for a wrapper function. Don't bother. Just OOP the code, stick it in the DIC, and call it a day. It's accessible via the Container.

marcingy’s picture

Title: Convert password to PSR-0 » Move password.inc in DIC
Status: Active » Needs review
FileSize
33.58 KB

After consideration I think this should be in the username space. Refractors the naming with the new class a fair amount. The password hashing tests work locally so think it is ready for the bot.

Status: Needs review » Needs work

The last submitted patch, 1463624-5.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

#5: 1463624-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1463624-5.patch, failed testing.

marcingy’s picture

Not sure I can run the tests locally and have just done a fresh install ideas welcome.....

Crell’s picture

The password hashing has to be available in core, not user.module, because it's needed for the installer.

marcingy’s picture

Title: Move password.inc in DIC » Move password.inc into DIC
Status: Needs work » Needs review
FileSize
33.59 KB

Thanks, file location and where they are loaded moved.

Status: Needs review » Needs work

The last submitted patch, 1463624-11.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
33.87 KB

Fix up tests

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Password/passwordInterface.php
@@ -0,0 +1,65 @@
+interface passwordInterface {

Needs to be PasswordInterface, with leading capital. (Same for the file name.)

+++ b/core/lib/Drupal/Core/Password/passwordInterface.php
@@ -0,0 +1,65 @@
+   * @param String $password
+   *   A plain-text password.
+   * @param Integer $count_log2

string and int, not String and Integer.

+++ b/core/lib/Drupal/Core/Password/passwordInterface.php
@@ -0,0 +1,65 @@
+   * @param String $password

string is lowercase.

+++ b/core/lib/Drupal/Core/Password/phpassHashedPassword.php
@@ -0,0 +1,255 @@
+class phpassHashedPassword implements passwordInterface {

Needs leading capital on class and file.

+++ b/core/lib/Drupal/Core/Password/phpassHashedPassword.php
@@ -0,0 +1,255 @@
+  /**
+   * Returns a string for mapping an int to the corresponding base 64 character.
+   */
+  protected function itoa64() {
+    return './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz';
+  }

Is there a reason this can't be a class constant, which would be a bit faster? It doesn't make a huge difference but it seems like a class constant fits the use case here better.

+++ b/core/lib/Drupal/Core/Password/phpassHashedPassword.php
@@ -0,0 +1,255 @@
+   * @param String $input
+   *   The string containing bytes to encode.
+   * @param Integer $count

string and int, not String and Integer.

+++ b/core/lib/Drupal/Core/Password/phpassHashedPassword.php
@@ -0,0 +1,255 @@
+    if (empty($count_log2)) {
+      // Use the standard iteration count.
+      $count_log2 = variable_get('password_count_log2', static::DRUPAL_HASH_COUNT);
+    }

Move this out. It should be a constructor parameter, not a variable_get() call. By eliminating these external hard dependencies, we can move the class to Drupal\Core, and then write real unit tests, not WebTests, which are more reliable and faster.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/PasswordHashingTest.php
@@ -30,6 +29,7 @@ function setUp() {
   function testPasswordHashing() {
+    $password_hasher = drupal_container()->get('password');
     // Set a log2 iteration count that is deliberately out of bounds to test
     // that it is corrected to be within bounds.
     variable_set('password_count_log2', 1);
@@ -37,26 +37,26 @@ function testPasswordHashing() {

Now that it's a class, for testing purposes we can use unit tests that ignore the container entirely. Instantiate the concrete class yourself, then test that. Then use a UnitTest, not WebTest, and everything gets way way faster. :-) (This is why we bother converting it to a class.)

znerol’s picture

Assigned: Unassigned » znerol
FileSize
33.9 KB

Rerolled on top of current head. Will attempt to resolve the issues from #14.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1463624-13-rebase.patch, failed testing.

znerol’s picture

Changes:

  • Add $count_log2 as an instance variable to PhpassHashedPassword
  • Introduce Drupal\Core\Password\PhpassFactory capable of creating instances of PhpassHashedPassword retrieving $count_log2 from system.password.phpass.count_log2 config.
  • Convert PasswordHashingTest into a UnitTestBase

Before PhpassHashedPassword into a component, drupal_random_bytes needs to become pluggable.

znerol’s picture

Status: Needs work » Needs review
znerol’s picture

Add upgrade-hook and test for password_count_log2.

Crell’s picture

+++ b/core/lib/Drupal/Core/Password/PasswordInterface.php
@@ -0,0 +1,65 @@
+
+/**
+ * @file
+ * Definition of Drupal\Core\Password\PasswordInterface.
+ *
+ * Secure password hashing functions for user authentication.
+ */
+
+namespace Drupal\Core\Password;

I think this code is now sufficiently abstracted that it can move to Drupal\Component. Win!

+++ b/core/lib/Drupal/Core/Password/PhpassFactory.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace Drupal\Core\Password;
+use Drupal\Core\Password\PhpassHashedPassword;
+
+class PhpassFactory {
+  public static function get($configfactory) {
+    $count_log2 = (int)$configfactory
+      ->get('system.password')
+      ->load()
+      ->get('phpass.count_log2');
+
+    return new PhpassHashedPassword($count_log2);
+  }
+}

Why? This is what the DIC is for.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -0,0 +1,250 @@
+  /**
+   * The minimum allowed log2 number of iterations for password stretching.
+   */
+  const DRUPAL_MIN_HASH_COUNT = 7;
+
+  /**
+   * The maximum allowed log2 number of iterations for password stretching.
+   */
+  const DRUPAL_MAX_HASH_COUNT = 30;
+
+  /**
+   * The expected (and maximum) number of characters in a hashed password.
+   */
+  const DRUPAL_HASH_LENGTH = 55;

No need for a DRUPAL prefix.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -0,0 +1,250 @@
+  /**
+   * Returns a string for mapping an int to the corresponding base 64 character.
+   */
+  static $ITOA64 = './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz';

No need for this to be static. It doesn't change, so it can be a const.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -0,0 +1,250 @@
+  function __construct($default_count_log2) {
+    $this->default_count_log2 = $default_count_log2;
+  }

$this->default_count_log2 needs to be defined explicitly. It also needs to be lowerCamelCase (eg, $defaultCountLog2)

Berdir’s picture

> Why? This is what the DIC is for.

If we pass this in directly in the bundle, then we need to access config there and then dump it into the container. Once that happened, then we can no longer just change the config, we also need to force a container re-compile. *If that is acceptable*, then we can just as well throw out the config completely and hardcode it in the container register call because if you want to change it, you can alter the container definition in your bundle.

This will at least affect the tests that do change the config and then expect that a new login request respects that config.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Password/PhpassFactory.phpundefined
+++ b/core/lib/Drupal/Core/Password/PhpassFactory.phpundefined
@@ -0,0 +1,15 @@

@@ -0,0 +1,15 @@
+<?php
+
+namespace Drupal\Core\Password;
+use Drupal\Core\Password\PhpassHashedPassword;
+
+class PhpassFactory {
+  public static function get($configfactory) {
+    $count_log2 = (int)$configfactory
+      ->get('system.password')
+      ->load()
+      ->get('phpass.count_log2');
+
+    return new PhpassHashedPassword($count_log2);

*If we keep this class (see my response above), then we need to format it correctly. @file, class and method docblocks.

So this is needs work either we, but we should discuss this first before fixing it.

Berdir’s picture

Status: Needs review » Needs work
Crell’s picture

Perhaps I don't fully understand the password system, but I thought it was something that you'd only be setting once per site and leaving it. It's not something that you'd be futzing with via the UI; you'd set it and forget. That's a perfect case for the DIC. If you're setting up an alternate password hasher, that's advanced enough that you can use the Container and tweak the container by whatever mechanism we end up endorsing. CMI doesn't need to enter the picture. That's for user-config, not sysadmin config.

Berdir’s picture

@Crell: Sounds good to to me. There is one test that checks what happens if the setting is changed, we'll need to find a solution for that one. Maybe a test module that defines a bundle that alters the definition or something like that?

Keeping that test makes sense IMHO because according to the current documentation of that varible/constant, the idea is that it's incremented for new major releases and the test verifies that we can magically update existing passwords for that.

Crell’s picture

Magic upgrading of passwords is an upgrade test feature, not a normal behavior feature. In that case, it should be fine to use the objects directly rather than going through the DIC. So if we switch default password libraries in Drupal 9 (which may actually be a good idea with the new movement toward becrypt/passwordlib), we can just grab the old object and new system directly, do a map, and call it a day. That's edge casey enough that it doesn't need to be something that works at runtime, because no sane person is doing it at runtime (if I understand the use case correctly).

znerol’s picture

The particular test is a problem-case anyway because it makes assumptions about the underlying hashing algorithm. Lets dig a bit into history in order to understand the current implementation.

During the development of Drupal 7 it was decided that a stronger hashing algorithm is needed for passwords stored in the database (#723802: convert to sha-256 and hmac from md5 and sha1). Currently when a user signs up on / logs into a D7 site, his password+salt is hashed 2^15 times using the SHA-512 algorithm (log2_count = 15). This is a quite cpu intensive process. However under normal circumstances this is not a problem because sign ups and logins are not very frequent operations.

In order to increase password security on sites upgraded from previous Drupal versions it is indeed necessary to rehash the existing password-hashes. In order to speed up the upgrade process, the rehashing is done with only 2^11 rounds (see user_update_7000).

In order that those two kinds of hashes can be distinguished, the strong version has the $S$ prefix while the password-hashes which were upgraded are prefixed with $U$. During login, after a user was authenticated successfully and his cleartext-password is still available, there is the uniqe chance to upgrade a $U$- into an $S$-hash (see user_needs_new_hash).

That said, it would be easily possible to devise a system, where the parameters for the hashing-algorithm could be altered during a sites lifetime. It is even possible to replace the hashing algorithm for new passwords entirely, as long as the previous one is still available on the site in order to check old existing password-hashes. I don't know if there is any site in the drupal universe with a non-standard password_count_log2 variable but I also do not see any reason why this settings should'nt be in config. On the other hand I expect that the CoreBundle.php itself will turn into a YAML sooner or later (see the chapter on the service container in the symfony book).

Disclaimer: I hope I got the facts right. I extracted them from the code and the docs.

Now to the architecture. I think we need to have the following parts in place:

  • A default password-hashing method used to create new passwords
  • A couple of password-hashing methods used to verify passwords. The method and its parameters (including log2_count etc.) can be selected on the basis of the hash-prefix.
  • A password-hash-policy service which decides during login whether a given hash still complies to the sites requirements. If it does'nt, the cleartext password is rehashed and stored.

The test mentioned above then just needs to verify if the login-method is triggering a rehash if the hash-policy service is demanding it.

What about the scope?

znerol’s picture

@Crell: To clear up the magic-update confusion. I think this is definitely a runtime-feature. After $log@_count++ during an upgrade, password-hashes are updated as users log into the new site.

znerol’s picture

Did some more work on that.

  • Remove the DRUPAL_ prefix from the constants
  • Declare $countLog2 as private instance variable
  • Remove $count_log2 parameter from the hash function. The parameter was necessary during the D7 ugrade. Nowadays we'd just create a new instance of PhpassHashedPassword supplying the desired iteration count to its constructor.
  • Therefore $count_log2 is also removed from non-public method generateSalt().
  • Updated comments and doc strings

In #21 some other things were suggested. I'd like to explain why I ignored them.

  • I did neither move the implementation nor the interface to Drupal\Component because the code still has strong dependencies on core.
    1. generateSalt calls into drupal_random_bytes.
    2. check and userNeedsNewHash take a user object via $account.
  • Abandon PhpassFactory, remove system.password config and instead fix the iteration count in the DIC. As long as there is no easy way to change the DIC during unit tests, we cannot run the test which ensures that incrementing the countLog2 setting will result in the users passwords being rehashed when they log in. Crell suggested that countLog2 will not likely change during the life time of an installation. However according to the phpass docs
    ... the number of iterations should not be hard-coded, but rather it should be configurable by an administrator for use when a new password is set (hashed), and it should be getting saved along with the hash (to allow the administrator to change the iteration count for newly set/changed passwords, yet not break support for previously-generated password hashes).

    Therefore I'd like to keep the factory class as long as the DIC is somewhat hard coded.

  • I left $ITOA64 as a static string because constants do not seem to understand index-access
    $ php -r 'static $X = "lorem"; var_dump($X[0]);'
    string(1) "l"
    $ php -r 'const X = "lorem"; print(X[0]);'
    PHP Parse error:  syntax error, unexpected '[' in Command line code on line 1
    

Thanks for the reviews so far. It would also be great to have someone from the security team taking a look at the patch.

znerol’s picture

Status: Needs work » Needs review
Berdir’s picture

+++ 1463624-29-move-password-inc-to-dic.patch	2012-11-13 18:28:42.000000000 +0100
@@ -447,10 +450,24 @@
 +  /**
++   * Specifies the number of times the hashing function will be applied when
++   * generating new password hashes. The number of times is calculated by
++   * raising 2 to the power of the given value.
++   */
++  private $countLog2;

We usually use protected except there is an actual reason why something should not be available to a child class.

znerol’s picture

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Password/PhpassFactory.php
@@ -0,0 +1,27 @@
+/**
+ * @file
+ * Definition of Drupal\Core\Password\PhpassFactory.
+ *
+ * Instantiate phpass password hashing algorithm with the parameters given in
+ * the system.password config.
+ */
+
+namespace Drupal\Core\Password;
+use Drupal\Core\Password\PhpassHashedPassword;
+
+class PhpassFactory {
+
+  /**
+   * Return an instance of Drupal\Core\Password\PhpassHashedPassword
+   */
+  public static function get($configfactory) {
+    $count_log2 = (int)$configfactory
+      ->get('system.password')
+      ->load()
+      ->get('phpass.count_log2');
+
+    return new PhpassHashedPassword($count_log2);
+  }
+}

I'm still not happy with this factory, but if the ID has to be stored in the Config system then I don't have an alternative right now, which makes me sad. The only alternative I can think of is making this an instantiated object and its own service, which I'm sure someone would push back on.

That said, the description in the @file docblock belongs on the class, not in the @file.

I think that's my last objection here, so once that's fixed I think we are RTBC.

Edit: Removed stray flotsam from dreditor.

znerol’s picture

Fixed @file and class docblocks.

@Crell: Password hashing iteration count is not the only case where it is difficult to decide where the value belongs to. For another example see #1799218: Convert taxonomy_override_selector and taxonomy_terms_per_page_admin variables to use configuration system. None of this configuration values should be modifiable from the UI. I agree with you that those settings should go into the DIC as soon as possible. But then again we'd have to modify values in the DIC during D7->D8 upgrade (aka, we'd need something like update_variables_to_dic analogous to update_variables_to_config. Also we'd need a way to alter the DIC during tests.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1463624-34-move-password-inc-to-dic.patch, failed testing.

Berdir’s picture

@znerol: If it's a DIC argument, then it's just code. No need for any kind of updates or helpers. D9 would simply change the code.

Note that it's possible to replace a container definition in a module bundle. Which means that we could have a user_test or similar test module (if there isn't one already) that replaces the argument service.

So the default definition would look like this:

    // Add password hashing service
    $container->register('password', 'Drupal\Core\Password\PhpassHashedPassword')
      ->addArgument(16);

And test bundle would then do something like this:

    // Add password hashing service
    $container->register('password', 'Drupal\Core\Password\PhpassHashedPassword')
      ->addArgument(20);

   // Or replacing the argument might work too, not sure what's better. Not sure about the exact method names.
   $container->getDefinition()->setArgument(0, 20);

Or something along those lines. So the test would then simply enable this module within the test, which should trigger a rebuild of the kernel, add this module and replace the definition on the next call.

znerol’s picture

Status: Needs review » Needs work
FileSize
37.57 KB

@znerol: If it's a DIC argument, then it's just code. No need for any kind of updates or helpers. D9 would simply change the code.

In this case there are two kinds of update:

  1. Increment the default countLog2 by one before each release as suggested in the code-comments. This is indeed trivial.
  2. Keeping the value of password_count_log2 (during the D7->D8 upgrade) when the admin decided that the default is not suitable for his/her site.

I did not dig too much into the DIC, so I do not know how complex case 2 will be if we'd set countLog2 there statically.

Note that it's possible to replace a container definition in a module bundle. Which means that we could have a user_test or similar test module (if there isn't one already) that replaces the argument service.

I may investigate into that later. Does the test-system support enabling a module in the middle of a test-method? Because IMHO that would be necessary to fix the test in question. E.g. if you look at PasswordHashingTest::testPasswordHashing we'd need to amend the DIC after // Increment the log2 iteration to MIN + 1..

  // Since the log2 setting hasn't changed and the user has a valid password,
  // user_needs_new_hash() should return FALSE.
  $this->assertFalse(user_needs_new_hash($account), t('User does not need a new hash.'));
  // Increment the log2 iteration to MIN + 1.
  variable_set('password_count_log2', DRUPAL_MIN_HASH_COUNT + 1);
  $this->assertTrue(user_needs_new_hash($account), t('User needs a new hash after incrementing the log2 count.'));

The diff contains a rebase of #35, no other changes.

znerol’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs work » Needs review

Keeping the value of password_count_log2 (during the D7->D8 upgrade) when the admin decided that the default is not suitable for his/her site.

I don't think this is necessary. If someone did this, then there is a very high chance that this happened in settings.php. And the way stuff is configured in settings.php will change anyway for many things, e.g. cache backend configuration. So we just need to make sure to document this as an API change (change notice).

Yes, enabling a module within a test should basically just work by calling module_enable(array('your_test_module')).

That module could then also serve as an example for site admins which could copy the code to a yet-to-be-created SiteBundle.php.

znerol’s picture

Win! Thanks @Berdir and @Crell for your support.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Now that's just purdy. :-)

znerol’s picture

Forgot to turn on the hide-flag on the test module.

RobLoach’s picture

I've noticed there's a PHP Password Library that provides much the same functionality as Drupal\Core\Password. This issue is simply to move password.inc to Drupal\Core\Password so that we can use the DIC, correct? If we move to adopt a third-party library for PHPass, that should probably be done in a separate issue.

Overall, I love how this patch is looking. Some notes...

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -80,6 +80,15 @@ public function build(ContainerBuilder $container) {
+    // 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.
+    $container->register('password', 'Drupal\Core\Password\PhpassHashedPassword')
+      ->addArgument(16);

Can we make the default constructor argument for $countLog2 = 16? Then we wouldn't need to addArgument here. The constructor injection here is fantastic though :-) . Does a default at all make sense here? RTBC either way.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/PasswordHashingTest.phpundefined
@@ -7,12 +7,13 @@
 /**
  * Unit tests for password hashing API.
  */
-class PasswordHashingTest extends WebTestBase {
+class PasswordHashingTest extends UnitTestBase {
   public static function getInfo() {

Absolutely love the change to UnitTestBase instead of WebTestBase.

cweagans’s picture

Status: Reviewed & tested by the community » Needs work

Opened #1845004: Replace custom password hashing library with PHP password_hash().

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -102,25 +103,32 @@ function testPerUserLoginFloodControl() {
+    // Change the required number of iterations by loading a test-module ¶
+    // containing the necessary container builder code and then verify that the ¶

Really like what's being done here so far. I really hate to be "that guy", but can you do a quick reroll without the trailing whitespace here?

Other than that, I couldn't see anything wrong with this, so RTBC after that change!

RobLoach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
38.03 KB
952 bytes

Was thinking about the 16 argument that we're passing in. This isn't anything we could build from VERSION, right? Still RTBC, just doing some brainstorming.

effulgentsia’s picture

#47: 1463624.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Dependency Injection (DI)

The last submitted patch, 1463624.patch, failed testing.

znerol’s picture

znerol’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Given the previous comments and the competency of the patch, I move to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Berdir’s picture

Title: Move password.inc into DIC » Change notice: Move password.inc into DIC
Priority: Normal » Critical
Status: Fixed » Active

I think this needs a small change notice to tell people where they can find the functionality now.

znerol’s picture

znerol’s picture

Crell’s picture

Title: Change notice: Move password.inc into DIC » Move password.inc into DIC
Priority: Critical » Normal
Status: Active » Fixed

Yay!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

chx’s picture

Tor Arne Thune’s picture

Assigned: znerol » Unassigned
Category: task » bug
Status: Closed (fixed) » Needs review
FileSize
394 bytes

The include to password.inc in password-hash.sh got readded somewhere, but even with the line removed again the script fatals:

PHP Fatal error:  Class 'Drupal' not found in /home/user/Public/Sites/drupal8/core/includes/bootstrap.inc on line 2128
PHP Stack trace:
PHP   1. {main}() /home/user/Public/Sites/drupal8/core/scripts/password-hash.sh:0
PHP   2. drupal_container() /home/user/Public/Sites/drupal8/core/scripts/password-hash.sh:85

Fatal error: Class 'Drupal' not found in /home/user/Public/Sites/drupal8/core/includes/bootstrap.inc on line 2128

Call Stack:
    0.0002     236536   1. {main}() /home/user/Public/Sites/drupal8/core/scripts/password-hash.sh:0
    0.0078     621312   2. drupal_container() /home/user/Public/Sites/drupal8/core/scripts/password-hash.sh:85

Attaching a patch that at least removes the unwanted include.

ParisLiakos’s picture

Category: bug » task
Status: Needs review » Closed (fixed)