acquia / blt

Acquia's toolset for automating Drupal 8 and 9 development, testing, and deployment.
https://docs.acquia.com/blt/
GNU General Public License v2.0
442 stars 394 forks source link

Allow early setting of hash_salt #4697

Open janusman opened 1 year ago

janusman commented 1 year ago

Motivation This would allow acquia#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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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?