Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow early setting of hash_salt #4697

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

janusman
Copy link
Contributor

@janusman janusman commented Aug 3, 2023

Motivation
This would allow #4291 so that a previously-set hash salt (for example, from the Acquia require line, or from the new "early settings" file) to be used instead of the BLT-defined one.

Proposed changes
Only set hash salt from the salt.txt file if $settings['hash_salt'] is empty (unset or empty string).

Alternatives considered
Acquia customers need to manually edit their settings.php to re-set a unique hash_salt because BLT "undoes" that unique-hashsalt-setting done in the acquia require line logic.

Testing steps
Set a hash_salt value in settings.php before the BLT require line, and then check with

drush --uri=[example.com](http://example.com/) php-eval 'echo \Drupal\Core\Site\Settings::getHashSalt();'

that the hash_salt is respected instead of set to the salt.txt value.

@danepowell
Copy link
Contributor

@janusman are you sure this is backwards-compatible? My worry is that if Acquia Cloud sets a default hash salt and BLT users take this update, the hash salt will suddenly change from the BLT-defined salt to the Cloud-defined salt. This would be very bad in production (users passwords and who knows what else would be reset).

@janusman
Copy link
Contributor Author

janusman commented Aug 23, 2023

The hash_salt, as per the default.settings.php documentation, says it's a "Salt for one-time login links, cancel links, form tokens, etc" ... so, no passwords would need to be changed.

On the other hand, the need for different hash salts per Drupal multisite instance is because we see sharing the same hash_salt can lead to problems including PHP fatal errors for missing code (Plugins, Classes, etc) among others. I wish I could recite from memory exactly what it is about the hash_salt value that impacts this, but I can't, I need to go research it :) I vaguely recall it may have to do with the class loader and keeping that in APCu storage? (admittedly only more important if you run APCu.. but that is a standard recommendation IIUC).

I can say that I have changed this value on various sites without apparent repercussion (other than fixing the above issues).

@danepowell
Copy link
Contributor

I strongly suspect that documentation is incomplete. Drupal passwords are hashed and salted (at least since ~D7), so where does that salt come from if not this setting? And wouldn't that imply that changing the salt breaks logins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants