Updated: Comment #0

Problem/Motivation

Currently we set the kernel environment to 'testing' in TestBase::rebuildContainer() but we don't use that information.

Proposed resolution

Set the environment to 'testing' any time we are withing a test request, and make tweaks to speed up Drupal, such as reducing the number of password hashes.

Remaining tasks

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
3.37 KB

Here's a first pass at the patch. I verified the tweak by adding the following to a web test and seeing that the value was 7:

    $p = \Drupal::service('password');
    $pass = $p->hash('test');
    debug($p->getCountLog2($pass));
pwolanin’s picture

FileSize
4.88 KB

Add a simple test case to insure a web request instantiates the kernel with the 'testing' environment.

The last submitted patch, 1: kernel-env-2165549-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: kernel-env-2165549-2.patch, failed testing.

pwolanin’s picture

So, amusingly those 2 fails are for tests looking for the correct number of password iterations. I'm not sure how to make those tests work while changing the number for all the rest.

larowlan’s picture

dawehner’s picture

You could provide some test module which also has a ServiceProvider, that alters back the number and checks whether the correct number appears.

Berdir’s picture

We already have that for a test that makes sure the number can be altered with that method. Not sure if it's reliable/what happens if two modules do the same. Also, this is testing the default behavior, so that wouldn't really make sense..

Could we maybe add a service provider to the testing profile? then that test could use standard.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
946 bytes

Yeas, we were talking about this briefly yesterday, modifying the CoreServiceProvider and friends is certainly the wrong approach IMO.

Profiles can have service providers too, so we could use one with the testing profile like so?

Status: Needs review » Needs work

The last submitted patch, 9: 2165549-9.patch, failed testing.

The last submitted patch, 9: 2165549-9.patch, failed testing.

Berdir’s picture

The expected user login test fail (which we should now be able to fix by using the standard profile there) + failed to write config rando fails.

pwolanin’s picture

@damiankloip, @Berdir - thanks. I figured the changing the CoreServiceProvider wasn't ideal, but I wasnt to see if it worked in case we wanted broader changes.

I still think it would be worth setting the kernel environment name?

sun’s picture

I still think it would be worth setting the kernel environment name?

I don't think that changing the kernel environment name in the site under test is desirable. The one and only purpose of web/integration tests is to test all functionality in an actual and real production environment.

The more manipulations are happening to a web test environment, the less accurate are the test results. The goal always was and still is to bring down the number of web test environment manipulations to +/- zero, because we want our tests to produce accurate and reliable results. Web test results are no longer reliable and trustworthy, if they are not, in fact, testing functionality in the exact same way like a real production site instance.


With that out of the way, do we have some real performance/bench numbers for how expensive the extra password hashing cycles are?

I can't imagine that to take more than a few milliseconds…? If so, I don't think that hacking the test environment to reduce the hashing cycles would be worth it in any way?

pwolanin’s picture

The hashing is designed to be slow and might add a couple seconds to every test setup (unless it does a lot of logins), but I'd agree it's not likely to be a large fraction of the overall test time.

Currently we only use the kernel name to decide where to put the container, so I don't really see it having bad side effects to set it based on being in a test?

Damien Tournoud’s picture

Related: #2168809: Do not add the Guzzle HTTP client request subscriber outside of test environment, which is another desirable modification of the site-under-test.

Would it be worth to factor those in a module that we only enable in the site-under-test?

Berdir’s picture

We already have that in the form of the testing profile, which is what the latest patch is now doing?

sun’s picture

The testing profile is not necessarily used by tests — we still have a dozen or more web tests that are using standard or minimal. Most of them shouldn't, but approx. a handful of them do so for legit reasons.

And of course, any installation profile is able to ship with tests for itself. The installation profile's functional code might issue HTTP requests, for which at least the Guzzle subscriber needs to kick in.

I'm not sure whether I like the idea of a simpletest_child.module, but the underlying consideration of aggregating all of these environment tweaks into a single spot is a good idea.

Instead of a module, I think I'd prefer a Drupal\Core component that groups and contains all necessary classes/subscribers, and whose service provider would be included in a hard-coded fashion by DrupalKernel if drupal_valid_test_ua() returns TRUE.

Berdir’s picture

Right, but as this issue shows, sometimes those alterations must not run. We want to make sure that we can test the default/standard.profile behavior without any test-specific alterations.

sun’s picture

but as this issue shows, sometimes those alterations must not run.

Wait — first we introduce a new hack for the site environment under test and then we introduce further complexity to prevent hacks from being executed under certain circumstances?

Unless I misunderstood, that makes little sense. Instead of solving problems, we're introducing new ones that need to be solved.

We should avoid all manipulations to the test site environment, unless they are strictly required (like the Guzzle subscriber). Every single manipulation that only occurs in a test environment defeats the whole point and purpose of web/integration tests.

Berdir’s picture

There is no further complexity, that test would then simply not use the testing profile.

However, I agree that I'm not sure if this is worth it. This is a follow-up issue of the one that did the hashing alteration for DUBT tests, where I know that it was worth it and we don't care that much about being as close as possible to the actual behavior (I tested with EntityFieldTest, 8 test methods, this took multiple seconds).

sun’s picture

#2168809: Do not add the Guzzle HTTP client request subscriber outside of test environment now has a patch that implements #18; i.e., it introduces Drupal\Core\Test as a component, and only conditionally adds the event subscriber to Guzzle.

The conditional registration is performed in CoreServiceProvider, so as guarantee that these changes are applied to every service container that is built for tests, regardless of whether the test is a web test or not.

Following this concept, we could try whether we can add the adjustment of this patch equally to CoreServiceProvider::registerTest().

As @Berdir pointed out that we're doing it for DUTB already, we might be able to remove/consolidate that code.

sun’s picture

Component: base system » simpletest.module
Issue tags: +Testing system, +Test suite performance

Adding/changing some properties, so as to ensure that this issue appears in the appropriate lists/views.

damiankloip’s picture

Me and sun just discussed this on IRC; The benefit of the current implementation is that this mod is only active when testing profile is used. So we can still use standard and get regular amount of passes. Sun suggested making a setting that we add to the test site settings. We can populate this with our low default for test performance, then any tests that want the standard value cal just override this. easy.

sun’s picture

Title: Use the kernel environment to make small tweaks to service definitions to speed testing » Reduce number of password hashing iterations in all tests to improve test performance
Status: Needs work » Needs review
FileSize
5.5 KB

Here's what I'd propose.

Since this issue is seemingly focusing on the single aspect of password hashing iterations from the beginning, I'm also adjusting the title accordingly. I'm aware that the OP just mentions it as one example, but as mentioned in #14 and following, I'm not able to imagine many other/further possibilities like this, because we need to ensure that the test environment actually resembles a regular environment.

sun’s picture

mmm, needs another override in WebTestBase::setUp(), because that invokes the non-interactive installer, which resets Settings on its own... :-/

For web tests, we could consider to write it out into the primed settings.php file that is written before the installer is invoked?

Status: Needs review » Needs work

The last submitted patch, 25: drupal8.password-hash-it.25.patch, failed testing.

pwolanin’s picture

It doesn't look correct to me to use the Drupal version number in here. We potentially want to change all Drupal versions to have the same value.

e.g. see #1203852: Increase hashing iterations for D7 and D8.

damiankloip’s picture

But if we are increasing this each Drupal version, doesn't this do exactly that? \Drupal::VERSION + 8 = 16 etc.. Unless I misunderstood the issue you referenced?

pwolanin’s picture

@damiankloip - This was really my very half-assed way of saying we should not fall too far behind Moore's law.

The point of referencing the Drupal version is just to remind us that we need to regularly increase it. There's not really any reason Drupal 8 should use 2x more hashes than Drupal 7 if they are both supported versions.

sun’s picture

It's PHP, we can calculate whatever we want. :-) In light of #1203852: Increase hashing iterations for D7 and D8., which appears to have retroactively increased the iterations for both D7 and D8, and actually suggests to increase it every 18-24 months, we can code up a time-based algorithm exactly like that:

  1. Drupal 7.0 was released on 2011-01-05, which we can simplify to 2011.
  2. In 2011, the iterations were 15.
  3. Increase the iterations by 1 every 2 years.

$default = (int) (15 + (date('Y') - 2011) / 2);

Perhaps I shouldn't have changed the hard-coded number in this patch... — but yeah, I wasn't aware of it, and when I encountered it, my first thought was "Huh? That requires some completely unnecessary manual maintenance." ;-)


UGH. It looks like the (totally missing) reset of Settings uncovered a ton of test failures that currently rely on the Settings that are leaking from the test runner/parent site environment into the test :-(

sun’s picture

pwolanin’s picture

Should we set the default at install time, rather than having sites suddenly experience more load on Jan 1 of some years?

sun’s picture

@pwolanin: Now you're confusing me :-)

In #1203852: Increase hashing iterations for D7 and D8., you argued that core should increase the default value periodically, so as to combat improvements in computing power. And we actually did so, retroactively, both for D7 and D8.

Storing a setting at install time conflicts with that idea.

I personally think that the argumentation for doing so is sound. It's a simple way to ensure that Drupal sites stay secure as time goes on.

If a site continues to be hosted on slow(er) machines for 4-8+ years, and if the performance impact is not acceptable for the site owner, then the proposed solution here allows to override the password hashing iterations via settings.php.

However, I think that the precondition of the same old site being hosted that long on the same old hardware is rather unlikely today. Already in ~2005, the general practice was that most websites got replaced with completely new ones every ~3 years, most commonly built by a new service provider, which in turn also means a new hosting provider + faster hardware. That was 2005, today is even more fast-paced, especially due to cloud hosting.

So in the end, I think the automated increase is a security improvement that only affects an edge-case of hosted sites to begin with. The computed default value much rather ensures that we do not forget to update this in new major versions of Drupal core.

pwolanin’s picture

@sun - I feel like there are competing interests in terms of this. Predictable behavior for an existing site, and protecting against improvements in the hardware of the attacker (not the site itself).

For e.g. D7 you would only get a new value if you do a code update. So, at least you could test it in advance. I think changing this based on the calendar is a bad/unexpected behavior.

For D8, having the behavior be stable over the lifetime of the site seems reasonable, and/or we could set it at install time and increase it in an update function. e.g. 8.0 to 8.1.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up
Related issues: +#1933712: Make PhpassHashedPassword log2 number maintainable
FileSize
5.52 KB

Apparently, I once created a dedicated issue for this: #1933712: Make PhpassHashedPassword log2 number maintainable ;-) — Closed it as a duplicate.

OK, so we indeed have competing interests:

  1. For site owners, the default iterations should only change upon updating the core code base. This default value lives either in code or in state.
  2. Site owners should be able to override the iterations with a custom value, in order to increase security, or in order to improve performance. This value may live in settings.php.
  3. Tests want to decrease the default iterations, so as to improve performance. This override lives in Settings/settings.php. (same facility as previous)
  4. Core wants to increase the default iterations over time in order to increase security. Albeit a version/time-based algorithm would be able to automatically increment it, that would conflict with 1). Therefore, a non-computed value has to be hard-coded and manually maintained in code over time.

Based on that, I'd suggest the following:

  1. Use Settings to retrieve the number of iterations (as in the latest patch here). This allows site owners to override the value in settings.php, while retaining a hard-coded default value. This resolves 1), 2), 3).
  2. Move the default value into the Settings call in the class constructor (as in the latest patch here), but remove the time/version-based algorithm and hard-code the default value instead. This resolves 4).

This does not make the default value in core any more or any less maintainable than it is now. The default value is merely moved from core.services.yml into the class constructor, so as to enable usage of Settings. The class constructor still accepts a custom value as parameter to facilitate unit testing of the class itself, but also to allow other code to instantiate the class with a custom value.

pwolanin’s picture

This looks like progress in the right direction. I really don't think the default should be in the yml file.

sun’s picture

Glad you like it. I agree that core.services.yml was/is a bad place for such a default setting.

I think this patch is actually ready. However, it contains the same/additional hash_salt fix from #2209461: Settings from test runner leak into all tests, so we likely want to commit that first.

Berdir’s picture

There's a test somewhere that verifies the service definition can be altered to change setting. That should use the setting now ...

dawehner’s picture

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -48,12 +53,18 @@ class PhpassHashedPassword implements PasswordInterface {
+  function __construct($countLog2 = NULL) {
...
+      $countLog2 = Settings::getSingleton()->get('password_hash_iterations', (int) \Drupal::VERSION + 8);

We could just pass along the settings object via DI

larowlan’s picture

FileSize
3.16 KB
7 KB

Re-roll and fixes #40

Status: Needs review » Needs work

The last submitted patch, 41: password-hashing-2165549.41.patch, failed testing.

pwolanin’s picture

I don't think we should inject settings as a service - sun and I are discussing that in another issue.

dawehner’s picture

I don't see a reason why you should not pass in the settings via the DI, if it is available. This gives you for example the knowledge of all the dependencies you might need if you want to use this class.

pwolanin’s picture

@dawehner. We can, but the current Settings class/object is pretty wonky, and it's not a singleton

larowlan’s picture

Status: Needs work » Needs review
FileSize
634 bytes
7.61 KB

fixes failing test

sun’s picture

To clarify on the injection problem of Settings:

  1. The service instance into which Settings is injected might get serialized and stored in state/cache.
  2. The instance of the Settings class that was injected will be serialized with the service (if it is assigned to a class property).
  3. Upon unserialization — typically in a different request/process — the service would operate with Settings that are not necessarily the actual settings of the current request/process.

cf. #2199795: Make the Settings class prevent serialization of actual settings

The problem does not seem to apply here, because the injected settings are not assigned to a class property. However, it is possible that the aforementioned issue is going to remove the exposure of Settings as a service.

Architecturally it does not look right to me that we are injecting a full configuration management service to retrieve configuration settings for a service, instead of just the configuration for the service. The configuration management service is not a dependency of the password hashing service, only the configuration value is.

IIRC, @dawehner once pointed me to a new feature of Symfony's DependencyInjection component that would allow to do exactly that directly in service declarations, along the lines of this:

services:
  password:
    ...
    arguments: ['@settings::get(password_hashing_iterations, 16)']

However, (1) that's not available to us because it requires an additional ExpressionSomething component, and (2) it would move back the hard-coded default value into core.services.yml file, whereas raw hard-coded primitives do not really look appropriate in service definitions.

Only marginally related, the diff in #2219009: Improve DX of Settings class clarifies that the vast majority of code directly accesses the Settings singleton. Only a very small fraction of classes get it injected. This inherently means that the settings service is not swappable (and I'm fairly sure that we do not want Settings to be).

pwolanin’s picture

Ok, so let's stop injecting in anywhere?

If so, we shoudl make that Settings::get() creates an instance if it's missing?

Status: Needs review » Needs work

The last submitted patch, 46: password-hashing-2165549.46.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.