Integrate with the key module, if available

I have checked the code of the key module for the D7 version, and it seems to me that it stores the decryption key in either a database or in a file. If the database is compromised, then it is possible to run PHP code (either through activating some code path that runs eval(), or by manipulating the serialized objects in the database). If PHP code can run, then the unlock key is readable.

I have found multiple tables in D8 that do store data with serialize(). If the class and all its data can be manipulated, it is very likely that at least one class exists that can be used to exploit the website (e.g. saving a php file, or reading a file). I am sure that given enough time and motivation, you can pretty much do anything with a Drupal site if you start manipulating objects in its database.

With that being said, I am all for making the module safer, and if you make a patch that does an optional integration with the key module, I am willing to merge it for D7, and I am sure that the same can be arranged for D8.

The reason to do this is so that the secret value can be stored in a secure key, outside of the document root, and outside of the version control repo. Attached is a D8 patch.

Updated patch
* simpler
* fixes bug on API form, referencing getDrupalSecretKey()
* creates the subscription on entity create instead of form submit to handle adding the api client as exported config, but not adding the subscription as exported config, so that the site url can be adjusted when moving config between dev/qa/live.

Oops, part of that patch was deleted, here's another update.

New patch without moving the creation of the subscription --- I'm no longer certain that needs to be done, and if it does, it should be in a separate patch.

Updated patch removed references to client project, and adds dependency injection to the Form.

Added key-integration tag to track integration efforts across contrib.

Updated patch for latest dev

updated patch to latest head

use key_select instead of select.

In reply to the initial comment on this thread by @tamasd, the key module supports different key providers, one of them is a file key providers, which does not store anything in the database. Key module also uses a plugin system for creating your own key provider storage. These are the reasons to use key module for storing keys. It gives sites the capability to store keys outside the database in a secure area.

Hi,

Looks like the patch that you have provided forces the module to use the key module if that's enabled.
It shouldn't be forced, it should be optional to use it even if it's enabled.

Also please provide a readme how to set up a key for the module initially.

Creating patch #21 to apply #18 patch changes for D9 compatible module version 3.0.0.

Patch #21 applies OK and gives me the option to use a defined key. But I can't get authentication to succeed.

I suspect I am not formatting my secrets.json file correctly.

I'm trying this.

{"secret_key":"[my_key_here]"}

Is "secret_key" wrong? Is there a a special key name that is expected?

I'm definitely using the right credentials as I can get things to work if I add them the usual way. But I really need to secure the secret key, so I'm hoping I can get this to work.

Thanks for working on this!

A colleague figured out what I was doing wrong. In case it helps someone else, the trick is that the file with the API key should just have the api key in it and nothing else (no brackets or quotes or anything). I was wrongly thinking it had to be structured json.

1. +++ b/src/Entity/BrightcoveAPIClient.php
   @@ -175,6 +177,27 @@ class BrightcoveAPIClient extends ConfigEntityBase implements BrightcoveAPIClien
   +  public function getSecretKeyValue($secret_key = NULL) {
   

   The getter shouldn't determine the actual value based on the argument.

   This method should know how to load the secret key.

2. +++ b/src/Entity/BrightcoveAPIClient.php
   @@ -175,6 +177,27 @@ class BrightcoveAPIClient extends ConfigEntityBase implements BrightcoveAPIClien
   +    if ($secret_key && \Drupal::moduleHandler()->moduleExists('key')) {
   

   Hidden dependency in entity, services should be added via dependency injection.

   Since Drupal entities are not injectables the next closest thing is the storage, but this probably should be handled by a service.

3. +++ b/src/Entity/BrightcoveAPIClient.php
   @@ -175,6 +177,27 @@ class BrightcoveAPIClient extends ConfigEntityBase implements BrightcoveAPIClien
   +      $key_entity = Key::load($secret_key);
   

   Hidden dependency, also entities should be loaded through their storage.

4. +++ b/src/Form/BrightcoveAPIClientForm.php
   @@ -191,13 +195,18 @@ class BrightcoveAPIClientForm extends EntityForm {
   +    if ($this->moduleHandler->moduleExists('key')) {
   

   It should be selectable.

   Like if someone would want to use key module and exist it could be selected, but we should let the option open for providing the key directly in configuration.

   Also if the credentials can be stored in the key module I would rather move the Account ID, Client ID and Client Secret to the Key entity as they are belong together.

5. +++ b/src/Form/BrightcoveAPIClientForm.php
   @@ -191,13 +195,18 @@ class BrightcoveAPIClientForm extends EntityForm {
   +      $form['secret_key']['#type'] = 'key_select';
   

   I wonder doing this would hurt the config schema or not, can you please validate?

To be honest I would rather refactor the client handler part of the module as a service, it's not ideal to have it on the entity.