Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#46 | password-hashing-2165549.46.patch | 7.61 KB | larowlan |
#46 | interdiff.txt | 634 bytes | larowlan |
#36 | drupal8.password-hash-it.36.patch | 5.52 KB | sun |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedHere'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:
Comment #2
pwolanin CreditAttribution: pwolanin commentedAdd a simple test case to insure a web request instantiates the kernel with the 'testing' environment.
Comment #5
pwolanin CreditAttribution: pwolanin commentedSo, 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.
Comment #6
larowlanRelated: #2016629: Refactor bootstrap to better utilize the kernel
Comment #7
dawehnerYou could provide some test module which also has a ServiceProvider, that alters back the number and checks whether the correct number appears.
Comment #8
BerdirWe 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.
Comment #9
damiankloip CreditAttribution: damiankloip commentedYeas, 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?
Comment #12
BerdirThe 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.
Comment #13
pwolanin CreditAttribution: pwolanin commented@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?
Comment #14
sunI 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?
Comment #15
pwolanin CreditAttribution: pwolanin commentedThe 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?
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedRelated: #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?
Comment #17
BerdirWe already have that in the form of the testing profile, which is what the latest patch is now doing?
Comment #18
sunThe 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.
Comment #19
BerdirRight, 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.
Comment #20
sunWait — 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.
Comment #21
BerdirThere 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).
Comment #22
sun#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.
Comment #23
sunAdding/changing some properties, so as to ensure that this issue appears in the appropriate lists/views.
Comment #24
damiankloip CreditAttribution: damiankloip commentedMe 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.
Comment #25
sunHere'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.
Comment #26
sunmmm, needs another override in
WebTestBase::setUp()
, because that invokes the non-interactive installer, which resetsSettings
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?
Comment #28
pwolanin CreditAttribution: pwolanin commentedIt 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.
Comment #29
damiankloip CreditAttribution: damiankloip commentedBut 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?
Comment #30
pwolanin CreditAttribution: pwolanin commented@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.
Comment #31
sunIt'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:
↓
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 theSettings
that are leaking from the test runner/parent site environment into the test :-(Comment #32
sunCreated #2209461: Settings from test runner leak into all tests
Comment #33
pwolanin CreditAttribution: pwolanin commentedShould we set the default at install time, rather than having sites suddenly experience more load on Jan 1 of some years?
Comment #34
sun@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.
Comment #35
pwolanin CreditAttribution: pwolanin commented@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.
Comment #36
sunApparently, 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:
Settings
/settings.php. (same facility as previous)Based on that, I'd suggest the following:
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).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.Comment #37
pwolanin CreditAttribution: pwolanin commentedThis looks like progress in the right direction. I really don't think the default should be in the yml file.
Comment #38
sunGlad 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.
Comment #39
BerdirThere's a test somewhere that verifies the service definition can be altered to change setting. That should use the setting now ...
Comment #40
dawehnerWe could just pass along the settings object via DI
Comment #41
larowlanRe-roll and fixes #40
Comment #43
pwolanin CreditAttribution: pwolanin commentedI don't think we should inject settings as a service - sun and I are discussing that in another issue.
Comment #44
dawehnerI 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.
Comment #45
pwolanin CreditAttribution: pwolanin commented@dawehner. We can, but the current Settings class/object is pretty wonky, and it's not a singleton
Comment #46
larowlanfixes failing test
Comment #47
sunTo clarify on the injection problem of
Settings
:Settings
is injected might get serialized and stored in state/cache.Settings
class that was injected will be serialized with the service (if it is assigned to a class property).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:
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 wantSettings
to be).Comment #48
pwolanin CreditAttribution: pwolanin commentedOk, so let's stop injecting in anywhere?
If so, we shoudl make that Settings::get() creates an instance if it's missing?