Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Here's a patch at makes an effort to convert the password.inc to PSR-0 syntax. There's more to do, since an effort to convert the actual password.inc file to use the PSR-0 version. I'm posting this to get feedback so I can know if on the right path.
Comment | File | Size | Author |
---|---|---|---|
#60 | 1463624-60-fix-password-hash-script.patch | 394 bytes | Tor Arne Thune |
#50 | 1463624-47-move-password-inc-to-dic-rebase.patch | 38 KB | znerol |
#50 | interdiff-47-rebase.txt | 2.32 KB | znerol |
#47 | interdiff.patch | 952 bytes | RobLoach |
#47 | 1463624.patch | 38.03 KB | RobLoach |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedAfter 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.
Comment #2
cweagansI 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.
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedCool. 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)
Comment #4
Crell CreditAttribution: Crell commentedNo 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.
Comment #5
marcingy CreditAttribution: marcingy commentedAfter 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.
Comment #7
marcingy CreditAttribution: marcingy commented#5: 1463624-5.patch queued for re-testing.
Comment #9
marcingy CreditAttribution: marcingy commentedNot sure I can run the tests locally and have just done a fresh install ideas welcome.....
Comment #10
Crell CreditAttribution: Crell commentedThe password hashing has to be available in core, not user.module, because it's needed for the installer.
Comment #11
marcingy CreditAttribution: marcingy commentedThanks, file location and where they are loaded moved.
Comment #13
marcingy CreditAttribution: marcingy commentedFix up tests
Comment #14
Crell CreditAttribution: Crell commentedNeeds to be PasswordInterface, with leading capital. (Same for the file name.)
string and int, not String and Integer.
string is lowercase.
Needs leading capital on class and file.
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.
string and int, not String and Integer.
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.
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.)
Comment #15
znerol CreditAttribution: znerol commentedRerolled on top of current head. Will attempt to resolve the issues from #14.
Comment #16
znerol CreditAttribution: znerol commentedComment #18
znerol CreditAttribution: znerol commentedChanges:
$count_log2
as an instance variable toPhpassHashedPassword
Drupal\Core\Password\PhpassFactory
capable of creating instances ofPhpassHashedPassword
retrieving$count_log2
fromsystem.password.phpass.count_log2
config.PasswordHashingTest
into aUnitTestBase
Before
PhpassHashedPassword
into a component,drupal_random_bytes
needs to become pluggable.Comment #19
znerol CreditAttribution: znerol commentedComment #20
znerol CreditAttribution: znerol commentedAdd upgrade-hook and test for
password_count_log2
.Comment #21
Crell CreditAttribution: Crell commentedI think this code is now sufficiently abstracted that it can move to Drupal\Component. Win!
Why? This is what the DIC is for.
No need for a DRUPAL prefix.
No need for this to be static. It doesn't change, so it can be a const.
$this->default_count_log2 needs to be defined explicitly. It also needs to be lowerCamelCase (eg, $defaultCountLog2)
Comment #22
Berdir> 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.
Comment #23
Berdir*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.
Comment #24
BerdirComment #25
Crell CreditAttribution: Crell commentedPerhaps 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.
Comment #26
Berdir@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.
Comment #27
Crell CreditAttribution: Crell commentedMagic 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).
Comment #28
znerol CreditAttribution: znerol commentedThe 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 (seeuser_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 (seeuser_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 theCoreBundle.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:
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?
Comment #29
znerol CreditAttribution: znerol commented@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.Comment #30
znerol CreditAttribution: znerol commentedDid some more work on that.
DRUPAL_
prefix from the constants$countLog2
as private instance variable$count_log2
parameter from thehash
function. The parameter was necessary during the D7 ugrade. Nowadays we'd just create a new instance ofPhpassHashedPassword
supplying the desired iteration count to its constructor.$count_log2
is also removed from non-public methodgenerateSalt()
.In #21 some other things were suggested. I'd like to explain why I ignored them.
Drupal\Component
because the code still has strong dependencies on core.generateSalt
calls intodrupal_random_bytes
.check
anduserNeedsNewHash
take a user object via$account
.PhpassFactory
, removesystem.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 thecountLog2
setting will result in the users passwords being rehashed when they log in. Crell suggested thatcountLog2
will not likely change during the life time of an installation. However according to the phpass docsTherefore I'd like to keep the factory class as long as the DIC is somewhat hard coded.
$ITOA64
as a static string because constants do not seem to understand index-accessThanks for the reviews so far. It would also be great to have someone from the security team taking a look at the patch.
Comment #31
znerol CreditAttribution: znerol commentedComment #32
BerdirWe usually use protected except there is an actual reason why something should not be available to a child class.
Comment #33
znerol CreditAttribution: znerol commentedComment #34
Crell CreditAttribution: Crell commentedI'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.
Comment #35
znerol CreditAttribution: znerol commentedFixed @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 toupdate_variables_to_config
. Also we'd need a way to alter the DIC during tests.Comment #36
znerol CreditAttribution: znerol commentedComment #38
Berdir@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:
And test bundle would then do something like this:
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.
Comment #39
znerol CreditAttribution: znerol commentedIn this case there are two kinds of update:
countLog2
by one before each release as suggested in the code-comments. This is indeed trivial.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.
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.
.The diff contains a rebase of #35, no other changes.
Comment #40
znerol CreditAttribution: znerol commentedComment #41
BerdirI 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.
Comment #42
znerol CreditAttribution: znerol commentedWin! Thanks @Berdir and @Crell for your support.
Comment #43
Crell CreditAttribution: Crell commentedNow that's just purdy. :-)
Comment #44
znerol CreditAttribution: znerol commentedForgot to turn on the hide-flag on the test module.
Comment #45
RobLoachI'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...
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.
Absolutely love the change to UnitTestBase instead of WebTestBase.
Comment #46
cweagansOpened #1845004: Replace custom password hashing library with PHP password_hash().
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!
Comment #47
RobLoachWas 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.
Comment #48
effulgentsia CreditAttribution: effulgentsia commented#47: 1463624.patch queued for re-testing.
Comment #50
znerol CreditAttribution: znerol commentedRerolled
Comment #51
znerol CreditAttribution: znerol commentedComment #52
cosmicdreams CreditAttribution: cosmicdreams commentedGiven the previous comments and the competency of the patch, I move to RTBC.
Comment #53
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #54
BerdirI think this needs a small change notice to tell people where they can find the functionality now.
Comment #55
znerol CreditAttribution: znerol commentedChange record draft: Site specific customizations of user password hashing.
Comment #56
znerol CreditAttribution: znerol commentedFollow up addressing #45 in #1850638: Add default parameter to PhpassHashedPassword constructor.
Comment #57
Crell CreditAttribution: Crell commentedYay!
Comment #59
chx CreditAttribution: chx commented#1965128: The password inc change notice is both broken and outdated
Comment #60
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThe include to password.inc in
password-hash.sh
got readded somewhere, but even with the line removed again the script fatals:Attaching a patch that at least removes the unwanted include.
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commented#2108573: password-hash.sh is broken