Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Convert the instances of variable_set and variable_get to the state system.
Comment | File | Size | Author |
---|---|---|---|
#42 | 40-42-interdiff.txt | 1.07 KB | alexpott |
#42 | 1798734-private-key-state-42.patch | 2.84 KB | alexpott |
#40 | 1798734-private-key-state-40.patch | 2.72 KB | Berdir |
#37 | 1798734-private-key-state-37.patch | 2.72 KB | gdd |
#31 | drupal-private-key-1798734-1.patch | 2.72 KB | Berdir |
Comments
Comment #1
ACF CreditAttribution: ACF commentedReplaced variable_set and variable_get with state()->set and state()->get.
Comment #3
ACF CreditAttribution: ACF commented#1: 1798734-drupalprivatekey-drupal8-1.patch queued for re-testing.
Comment #5
ACF CreditAttribution: ACF commented#1: 1798734-drupalprivatekey-drupal8-1.patch queued for re-testing.
Comment #6
alexpottConfirmed with ACF that the failures was the entity translation test random failures.
Tested locally... works as expected. Thanks for the work!
Comment #7
chx CreditAttribution: chx commentedI think you found a controversial one.
If you create a secret dir based on private key and save it into CMI the other site won't recognize it because the private key isdifferent? We either need to document the hell out of this, or, better move it to CMI instead of state. Thoughts? I didn't move it to CNW to faciliate reviews / followups.
Comment #8
gddI guess it will depend on your use case. In many cases I can see not wanting to deploy your private key between servers, in which cases this should be in state. However there is probably cases where you would want to as well. For instance, you have signed files on one site, deploy them to a second site, then want to verify the signatures (similar to what we did in the early versions of CMI.) I would lean towards putting this in CMI to support that use case, however I'm unsure of the implications of that decision. Would love to see more discussion on this one.
Comment #9
leschekfm CreditAttribution: leschekfm commentedI don't know much about drupal_private_key and only stumbled over this issue, but would it make sense as there are two usecases, to make this also configurable? If a certain setting is set, use CMI and otherwise state?
Comment #10
longwaveShouldn't this be optionally set in settings.php (not CMI), where it can be manually deployed between servers if necessary, and if not found in settings.php then it should be state?
Comment #11
gddIf something needs to be deployed between servers, then it is not state by definition (state is environment-specific.) Right now, CMI can provide defaults that get overridden in settings.php if desired. After some thought I think I agree this should be moved into CMI rather than state. Not that hard to change, although it will need to be added to the default system settings yaml file.
Comment #12
leschekfm CreditAttribution: leschekfm commentedChanged the patch from state to CMI and added default value to the .yaml file.
Hope I've missed nothing :)
Comment #13
longwaveBut isn't CMI intended for export to version control, which is somewhere you maybe don't want a private key to be stored? This key seems like it should be environment specific, so therefore it is state - except for certain setups where the state on two or more servers is identical for convenience?
Comment #14
pcambra#12: 1798734-drupalprivatekey-drupal8-12.patch queued for re-testing.
Comment #16
pcambra#12: 1798734-drupalprivatekey-drupal8-12.patch queued for re-testing.
Comment #18
gdd#13: The key should not be environment specific. It is most often used for signing data like filenames and sensitive data. This data is meant to be deployed and thus the key that generated the signature must be deployed as well so that it is the same between sites.
Comment #19
znerol CreditAttribution: znerol commented@heyrocker: Looking up the references to drupal_get_private_key gives the following results:
The latter is called from a couple of functions. However I do not see any instance where
drupal_get_private_key
seems to be used for creating/verifying signatures of any kind. In the existing code basedrupal_private_key
is IMO clearly used in a site-specific way. I suggest to go back and implement it using the state system rather than as a config.Comment #20
gddIt is not used that way in core, however it is a common use case in site installations and contrib. We actually were using it in the config system initially when we were signing files as a security measure.
Comment #21
ACF CreditAttribution: ACF commentedComment #22
BerdirAgree that this is problematic. @heyrocker: What is use is a private key that is used for deployment security if he is deployed itself too? Sounds like such a usage would have to use something else in Drupal 8.
Comment #23
chx CreditAttribution: chx commentedDeployability is a lesser concern IMO because it never changes. If it never changes, however, why don't we get the installer to generate it into settings.php and use settings()->get() ?
Comment #24
BerdirThat's an interesting idea. That makes me wonder what the difference between the hash salt and the private key is exactly? Do we need to two separate private thingies in settings.php?
Looking at the usages of them:
- drupal_get_token() uses both
- WebTestBase::drupalGetToken() uses just the private key but that method is never used in core?
- update.module is especially interesting and uses "$site_key = drupal_hmac_base64($base_url, drupal_get_private_key());" in
update_process_fetch_task() but _update_manager_unique_identifier() uses drupal_get_hash_salt()?
- For ApcUniversalClassLoader, we use the hash salt
- user_pass_rehash() uses the hash salt.
Can't really see a pattern from that? ;)
Comment #25
znerol CreditAttribution: znerol commentedI think the reason for
drupal_get_hash_salt
is explained in #590656: Harden one-time login links against vulnerability from disclosure of SQL backups, or SQL 'SELECT' injection.Comment #26
pwolanin CreditAttribution: pwolanin commentedThe principle is that an important credential is best protected by splitting it so that you need access to both the DB and a file's contents.
So, e.g. form tokens are created by a combination of the DB value (session ID) and the "salt" from the file. The on-time link is created by a combination of the hashed password, login time, etc from the DB and the "salt" from the file.
Perhaps we need some renaming or more code comments to clarify the concepts, but in short I don't think that the Drupal private key should be part of the config system, unless you split it into 2 pieces (one of which remains in the DB) and continue to use the existing "salt" string for the other applications.
Comment #27
BerdirI think that #26 means that this must be state.
And we might want to explore in a follow up how to improve documentation and possibly usage of those two functions so that we document the recommendation to use them together (maybe even provide a single API function that reads from two different locations?)
Comment #28
chx CreditAttribution: chx commentedIt'd violate the semantics of state to put it in there -- share is never shared and it doesn't even make sense to share it in most cases.
It does sound like K-V store, though. And people needing to share it between environments can write the necessary ~one line of code script to copy it.
Comment #29
BerdirHm, so you are saying a custom keyvalue collection?
Comment #30
BerdirHere is a patch that changes it to state. Had that already before you commented. Using another collection would require a custom update function and test coverage. But could of course be done.
I still don't quite see why and when this would need to be shared? Except between environments of the same site ( development/production) and there it is a one time thing when they databases are copied initially anyway. Any other use case does not make sense to me for the *private* key, you'd never want to give that away to another site. If you want something like that, you'd create another key, possibly based on the private key?
Comment #31
BerdirComment #33
znerol CreditAttribution: znerol commentedRe: #26 Does
drupal_get_private_key
really add to the security or is it just a relict of ancient times? Given that there seems to be much confusion about its usage I argue it would be better to drop it.Comment #34
chx CreditAttribution: chx commented#29 yes. See #8 for sharing.
Comment #35
gdd#31: drupal-private-key-1798734-1.patch queued for re-testing.
Comment #37
gddRerolled for new update hook.
Ultimately I feel like I have come around on this and agree with #26 and #28 that this belongs in state. If this patch comes back green I think its RTBC.
Comment #38
BerdirThere's a much higher chance for the patch to come back green if the issue is set to needs review :)
Comment #40
BerdirUpdated the update function number.
Comment #42
alexpottWe need to migrate drupal private key earlier as this is used in the upgrade batch.
Comment #43
BerdirI think this is good to go.
- This in the database or some other persistent storage
- We lose the ability to override this in settings.php but according to #26, doing that would be wrong anyway.
- Has upgrade path and tests.
Comment #44
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.