Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ACF’s picture

Status: Active » Needs review
FileSize
566 bytes

Replaced variable_set and variable_get with state()->set and state()->get.

Status: Needs review » Needs work
Issue tags: -State system

The last submitted patch, 1798734-drupalprivatekey-drupal8-1.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1798734-drupalprivatekey-drupal8-1.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
Issue tags: +State system
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed with ACF that the failures was the entity translation test random failures.

Tested locally... works as expected. Thanks for the work!

chx’s picture

Component: configuration system » base system
Status: Reviewed & tested by the community » Needs review
Issue tags: +Configuration system

I 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.

gdd’s picture

I 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.

leschekfm’s picture

I 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?

longwave’s picture

Shouldn'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?

gdd’s picture

Title: Convert drupal_private_key to the state system. » Convert drupal_private_key to the configuration system.
Status: Needs review » Needs work

If 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.

leschekfm’s picture

Status: Needs work » Needs review
FileSize
927 bytes

Changed the patch from state to CMI and added default value to the .yaml file.
Hope I've missed nothing :)

longwave’s picture

Status: Needs review » Needs work

But 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?

pcambra’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -State system

Status: Needs review » Needs work

The last submitted patch, 1798734-drupalprivatekey-drupal8-12.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Configuration system, +State system

The last submitted patch, 1798734-drupalprivatekey-drupal8-12.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review

#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.

znerol’s picture

@heyrocker: Looking up the references to drupal_get_private_key gives the following results:

  1. _update_process_fetch_task
      ...
      $site_key = drupal_hmac_base64($base_url, drupal_get_private_key());
      $url = _update_build_fetch_url($project, $site_key);
      $fetch_url_base = _update_get_fetch_url_base($project);
      ...
    
  2. drupal_get_token

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 base drupal_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.

gdd’s picture

It 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.

ACF’s picture

Assigned: ACF » Unassigned
Berdir’s picture

Agree 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.

chx’s picture

Deployability 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() ?

Berdir’s picture

That'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? ;)

znerol’s picture

pwolanin’s picture

Status: Needs review » Needs work

The 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.

Berdir’s picture

I 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?)

chx’s picture

It'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.

Berdir’s picture

Hm, so you are saying a custom keyvalue collection?

Berdir’s picture

Status: Needs work » Needs review

Here 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?

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-private-key-1798734-1.patch, failed testing.

znerol’s picture

Re: #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.

chx’s picture

#29 yes. See #8 for sharing.

gdd’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -State system

#31: drupal-private-key-1798734-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +State system

The last submitted patch, drupal-private-key-1798734-1.patch, failed testing.

gdd’s picture

Rerolled 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.

Berdir’s picture

Status: Needs work » Needs review

There's a much higher chance for the patch to come back green if the issue is set to needs review :)

Status: Needs review » Needs work

The last submitted patch, 1798734-private-key-state-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Updated the update function number.

Status: Needs review » Needs work

The last submitted patch, 1798734-private-key-state-40.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
1.07 KB

We need to migrate drupal private key earlier as this is used in the upgrade batch.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.