Introduce kernel parameters allowing to specify password hashing algorithm and options

Postponing on #3530235

CI images has both now

constructor only changes looks ok but it require container rebuild so needs update hook to request it, otherwise looks ready

Thanks! Added an update hook.

Thank you! CR is ready and the code too

Looks good to me, added a question about the update hook though.

Thanks for working on this issue. As I said on 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active ,

• I think this issue is the right way to go. Let's make it easy to use something other than bcrypt.
• I think we should implement hook_requirements. We could do that as part of this issue or on #3536662.

@longwave mentioned #2951046 to me. I am adding that as a related issue. Once that issue gets in, we can use something like !php/const PASSWORD_ARGON2ID in services.yml.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I think requirements exposition of available hashes should be done in 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active

This issue exactly about to add support of argon and make it configurable, also soduim open doors to fast encrypted storage

Left a minor question on the MR, fine to self RTBC

% ./vendor/bin/drush updb
 -------- ------------------ ------------- ------------------------------------
  Module   Update ID          Type          Description
 -------- ------------------ ------------- ------------------------------------
  system   password_kernel_   post-update   Rebuild the container after adding
           parameters                       new kernel parameters.
 -------- ------------------ ------------- ------------------------------------


 ┌ Do you wish to run the specified pending updates? ───────────┐
 │ Yes                                                          │
 └──────────────────────────────────────────────────────────────┘

>  [notice] Update started: system_post_update_password_kernel_parameters
>  [notice] Update completed: system_post_update_password_kernel_parameters
 [success] Finished performing updates.

larowlan → credited akalata → .

larowlan → credited drumm → .

larowlan → credited neclimdul → .

larowlan → credited ram4nd → .

larowlan → credited xjm → .

@catch mentioned on slack

don't think we need the empty post update on 3530186 because the default deployment identifier is based on \Drupal::version() and that's used in the container cache key. Doesn't really matter except it's less updates to remove when the time comes later. Mentioning in here because it went around a couple of times in the issue already

Sorry for the runaround @znerol - can we remove that?

We discussed this on the security team triage call in relation to 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active - in particular that issue's reintroduction of the legacy phpass hashing for users with passwords longer than 72 bytes where the bcypt algorithm is in use.

We decided we would instead prefer to move forward with this issue rather than what is effectively backwards with the reuse of phpass.

We decided we would prefer to have the hook_requirements from that issue added here that:

1. Shows a warning if the site only has bcrypt hashing available - detailing the 72 char/null byte risk with links to PHP.net regarding argon2
2. Shows an error if the site has argon2 available but it isn't the default hashing algorithm

I've manually tested that changing the hashing algorithm with existing users successfully rehashes their password in the new algorithm when they next login.

I will mark the other issue as postponed on this one in the meantime.

We also decided to re-purpose the php_password contrib module as a BC layer as the changes in this MR can't be backported.

So this issue here will fix 11.3/10.6 and the contrib module will decorate the core service to bring this functionality to stable branches. The contrib module will have a forward compatibility layer that will be a no-op if it detects the container param added here - which would indicate the module is no longer needed as the site has updated to a version with this feature.

With this issue and the contrib module ready, we will likely issue a PSA

Crediting those involved in the discussion.

Removed post update hook

I think this needs:

We decided we would prefer to have the hook_requirements from that issue added here that:

Shows a warning if the site only has bcrypt hashing available - detailing the 72 char/null byte risk with links to PHP.net regarding argon2
Shows an error if the site has argon2 available but it isn't the default hashing algorithm

from #24.

forwards compatibility layer is ready for review here https://git.drupalcode.org/project/php_password/-/merge_requests/6/diffs

Added the requirement checks.

Should we add a config and a proper UI to make these requirement checks more actionable? Maybe an additional Password fieldset in admin/config/people/accounts?

I think transition to new hash should be done in follow-up, moreover old passwords makes no sense when Argon2 available

@andypost:

What transition do you mean?

Passwords should be rehashed automatically when users log in with their passwords. The Q&A for the Password Compatiblity module → apply to this situation, too.

From #24 here:

I've manually tested that changing the hashing algorithm with existing users successfully rehashes their password in the new algorithm when they next login.

@znerol:

Should we add a config and a proper UI to make these requirement checks more actionable?

I have been thinking that this is something that would be managed through settings.yml, not the UI. In fact, I like the idea of relying on something in the filesystem. Otherwise, XSS can be exploited to change the password hashing algorithm.

If you still want to provide a configuration form, then let's have a followup issue for further discussion.

By the time this ships, every single site which upgrades will generate a requirements warning. The requirements warning points to a CR which contains example code for a service provider class. The example code for the service provider class is there because I think we shouldn't instruct people to manually add a container parameter for this. The reason I think so is that it is safer to use the \PASSWORD_* constants instead of let people attempt to hard code the exact password algorithm into a yml file by hand.

Regarding the UI: This could be just a password_argon2 module with its own requirements check and a service provider which adds the password.algoritm: argon2id container parameter.

How is it easier to use the \PASSWORD_* constants? Using drush php:

> password_algos()
= [
    "2y",
    "argon2i",
    "argon2id",
  ]

I think we should let people set a hashing algorithm in services.yml.

Have we considered what to do if the service parameter is invalid? Do we fall back to PASSWORD_DEFAULT? Is there a requirements check (for the status page)?

In the MR. There is a requirements check for unknown / misspelled password algorithms.

When an algorithm is configured which isn't available on the system, attempting to create new password hash will result in a fatal:

Fatal error: Uncaught ValueError: password_hash(): Argument #2 ($algo) must be a valid password hashing algorithm in Command line code on line 1

Verifying existing hashes still works (assuming the hashing algorithm is available on the system).

I updated the change record to add an example of setting the parameters via a services.yml file
Fixed a couple of typos in the MR.
Going to try to simplify the hook_requirements a bit

Moved the requirements checking logic to a new private method which enabled me to reduce the cyclic complexity/nesting.

As this was just moving things around I think I'm still ok to mark this as RTBC. So doing so.

https://www.drupal.org/project/php_password/releases/3.0.0 → has been released with the forward compat layer

+1 RTBC as no code change just move and s/know/known test change

With the change record, is password.options really required? It looks like it defaults to empty array anyway and could be omitted.

Also should some documentation be going into default.services.yml? The issue summary mentions this but there's no changes in the MR.

Updated the CR to make it clear that password.options is optional.
Added docs to core.services.yml and default.services.yml

The link in the message is to a change record. Should that be to a documentation page instead?

I think we should consider removing the hook_requirements() from here altogether, and sorting that out in 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active where we have multiple options that might not require it (at least on all sites, we still might end up with something similar on some sites).

I hadn't read the other issue, 📌 Properly handle that PHP bcrypt passwords are truncated to 72 bytes Active , until now. I like the suggestion in #43.

@catch that would basically reverse the outcome from the security team triage call (#24). I'd rather try to go into a direction which doesn't send us running around in circles.

Added a draft MR with an Password Argon2 module. This is implementing the idea from #32.