Integrate with the key module for access and secret keys

I suggest that we not restrict the settings form to only the 'file' provider, because that prevents custom key implementations, that may be just as secure and also may use files. I ran into this because we store all or keys in a predefined place outside the document root, that varies depending on which environment (dev, test, live) the code is running on. I didn't roll a patch. Just remove the two #key_filters lines.

'#key_filters' => ['provider' => 'file'],

Thanks for the patch, was about to suggest doing this :)

I agree with @douggreen on #3, we should not limit the key provider to just file. The suggestion on #4 to use a setting for allowed key providers sounds interesting, but also perhaps too convoluted.

I believe that if your custom KeyProvider plugin uses a storage_method that is also "file" it should show up with this filter.

Unfortunately this is not the case, using the 'provider' key on the filter limits by provider only. Other options could be 'type', or 'type_group', and unfortunately all of these would be discarding custom providers.
You can see how all this works on Drupal\key\Element\KeySelect and Drupal\key\KeyRepository.

In my opinion the security concerns with storing key values in configuration should be tackled within key module itself, perhaps via means of informing the user of these concerns.

In reality what we would need is to be able to filter by storage_method. Opened #3013279: Add ability to filter list of keys based on key storage_method for this.

Yeah I think if we can review and get #3013279: Add ability to filter list of keys based on key storage_method in, then we can update this patch which will get us both secured selections and the flexibility of using different key providers :)

Great news, #3013279: Add ability to filter list of keys based on key storage_method was committed so we should be able to filter by storage_method using the latest release of key module: https://www.drupal.org/project/key/releases/8.x-1.8

+++ b/src/Form/SettingsForm.php
@@ -108,6 +108,50 @@ class SettingsForm extends ConfigFormBase {
+        '#key_filters' => ['provider' => 'file'],

Lets use 'storage_method' => 'file' here instead.

Addressing #9

Nice work guys, I'm reviewing today. I'll tell you something soon.

Hi, i was reviewing it, and I have new proposals to add:
- Inject 'module_handler' service in SettingsForm
- Remove ':key_collection' => $key_collection_url in key_access_key and key_secret_key form elements, becuase is not being used (SettingsForm).
- We are using only file storage method, I think that is important that the user knows it, we should add this filter to form field descriptions.
- We should update README.txt with this new feature.
- We can add new @todo for 8.x-3.0-beta1 (in this release we will remove all backward compatibility with access_key and secret_key in database storage) to create a method to get the keys, and we have only one way to get them and avoid checkings in different places.

Thanks @jansete for the review!

- Inject 'module_handler' service in SettingsForm

Done. Injected it also on S3fsService while I was at it.

- Remove ':key_collection' => $key_collection_url in key_access_key and key_secret_key form elements, becuase is not being used (SettingsForm).

Nice catch, done.

- We can add new @todo for 8.x-3.0-beta1 (in this release we will remove all backward compatibility with access_key and secret_key in database storage) to create a method to get the keys, and we have only one way to get them and avoid checkings in different places.

Not entirely sure what you mean here: what should the @todo say? where should we place it? Do you mean to create a follow up or?

Thanks @malcolm_p for the explanation.

For now, I've created a follow up here #3054540: Move the logic checking config, settings, and keys into a separate method please kindly review the summary & title as needed.

Setting to needs work due to that and adding the @todo to the patch.

Also needs work for this mentioned on #12:

We should update README.txt with this new feature.

I rerolled this for the alpha15 release and added a small blurb of help text to the README.txt to try and address the comment in #12

#17 looks good to me, adding the @todos here requested on #12. Anything else we need to do here?

I'm bumping the priority here, this is imho a security issue since people are probably committing their credentials to VCS.

One big item, and a few nits.

1. The Key module does not store keys on a per-module basis as you can see by this function in \Drupal\key\KeyRepository:

     /**
      * {@inheritdoc}
      */
     public function getKeys(array $key_ids = NULL) {
       return $this->entityTypeManager->getStorage('key')->loadMultiple($key_ids);
     }
   

   I cannot recommend highly enough that you prefix the keys with the module name. I would also make them a constant since they're being used all over the place among other instances of the word 'key' which will be confusing for anyone looking at the code for the first time.

2. +++ b/s3fs.install
   @@ -41,6 +41,28 @@ function s3fs_requirements($phase) {
   +    // @TODO Work on https://www.drupal.org/project/s3fs/issues/3054540 after 8.x-3.0-beta1 removes all backward compatibility with access_key and secret_key in database storage
   

   Restrict lines to 80 chars where at all possible

3. +++ b/src/Form/SettingsForm.php
   @@ -111,6 +145,47 @@ class SettingsForm extends ConfigFormBase {
   +    // @TODO Work on https://www.drupal.org/project/s3fs/issues/3054540 after 8.x-3.0-beta1 removes all backward compatibility with access_key and secret_key in database storage
   

   Comment over 80 chars

4. +++ b/src/S3fsService.php
   @@ -183,6 +197,27 @@ class S3fsService implements S3fsServiceInterface {
   +        // @TODO Work on https://www.drupal.org/project/s3fs/issues/3054540 after 8.x-3.0-beta1 removes all backward compatibility with access_key and secret_key in database storage
   

   Over 80 chars.

Regarding #4:
It sounds like the only advantages we will gain to using the keys module is that

• We will know the secret is in a file somewhere because Keys module will have loaded it for us.
• An admin can change which key file they are using via the GUI instead of needing to modify settings.php. They will still need access to the file system to place keys.

Looking at what I intend to develop for #3200292: [META] Support authentication other than InstanceProfile or Access Keys I believe we would gain the ability to know the config is in a file, and we will gain the ability for an admin to change the config inside the GUI by changing which config file. We we would not get a dropdown box, instead only receiving a text field, however that burden exists in the initial configuration of the Keys module too. Additional an admin would need to know how to write an AWS Profile file instead of just putting a single string in a file.

Perhaps if we were using more of the Keys module I could see more value, at the moment to me it feels like this is trying to add a module integration just to complete something that could be handled by adding a field for a file path for the access/private key or some documentation (I've recently committed a change to the README reminding about not storing keys inside the docroot).

There is note to a security concern that came up, I'm not aware off hand which incident this was, I could guess it was a config export, a settings.php leak, or a database leak revealing secret triggering some concerns but I'm too new to this project to know the exact specifics.

Regarding #20

I'm bumping the priority here, this is imho a security issue since people are probably committing their credentials to VCS.

Do we have any data that this change would significantly reduce key stored in version control? I could see an admin easily placing a file in version control as placing it in settings.php.

Is there something I'm missing on why integration to Keys would be better?

Rerolled patch for alpha17.

Hi @cmlara,

Perhaps if we were using more of the Keys module I could see more value

I can't speak for everyone of course, but Keys does open the door for more then just storing keys in files. It also can store keys externally (i.e. Lockr) and can be extended to support other storage methods we haven't thought of yet. That said, maybe a compromise would be to do this as a sub module so that we're not adding dependencies to the main module?

Do we have any data that this change would significantly reduce key stored in version control? I could see an admin easily placing a file in version control as placing it in settings.php.

Personally it makes it a little easier because I can set git ignores up to exclude anything put into our sites keys folders whereas trying to exclude the settings.php file gets more complicated. Obviously doesn't stop someone from deliberately adding the keys, but it's something.

Setting needs work for further discussion. The readme portion looks to need a rewrite as the file was re-done in Alpha17 the context of the addition no longer makes sense. Noticed a dual use MessengerTrait in the reroll as well.

Thanks for providing your input @jacobbell84.

I can't speak for everyone of course, but Keys does open the door for more then just storing keys in files. It also can store keys externally (i.e. Lockr) and can be extended to support other storage methods we haven't thought of yet.

As it currently exists I believe this patch doesn't allow for that as it appears to be locked specifically to just the file storage by '#key_filters' => ['storage_method' => 'file'],. This would open to other modules but only if they declare they use file storage which I'm not sure Lockr or other similar plugins would declare if they use many external systems like databases.

The use of non-file storage on keys is what looks the most interesting to me personally for the Key's module value. This is why I wonder what is the background history I'm missing that it was suggested we not trust an administrators choice on where they put the credentials as a core requirement of this patch as currently built (comments #3-#10).

I can set git ignores up to exclude anything put into our sites keys folders whereas trying to exclude the settings.php file gets more complicated.

This is where I generally suggest something like include('/path/to/keys/secretconfig.php') or $value = file_get_contents('/path/to/secret') or similar in settings.php that way the settings.php is safe and the secrets are in a more controlled location that can be ignored in the repo, or often in my own deployments isn't even inside the path of files deployed by the VCS and is controlled by external means (even if that is sometimes as low tech as writing the file one time and leaving it)

As to submoduling: I'm not really sure off hand how easy this would be to split off. It may be doable I just haven't looked that closely at it. That question also has me wondering what jansete's meaning was when he suggested " to create a method to get the keys, and we have only one way to get them and avoid checkings in different places." if he meant standardizing all the authentication to one function as it appears the sub issue of #3054540: Move the logic checking config, settings, and keys into a separate method makes it seem or if he was referring to not having as many configuration methods available so there are not multiple ways to provide the same data (in which case we would want to standardize on one single method that can provide secret keys and not sub module it)

Our CI environment enforces schema. I've added key to the module schema definition.

Fixing the previous patch.

Version back to dev as this is a feature request.

Back to needs work per #27 concerns raised about patch that have not yet been addressed.

Additionally this is needs work for additional discussion on patch about implementation details.

Patches need to be against dev to test and apply, especially with how much of the code is being worked on right now clearing up known issues in the code.

Please see #3206162: 4.x Version which aims to implement this, and provide feedback if you have any.

Oops, I meant to make that a related issue, not parent.

Here is a re-rolled version of the patch based on latest Dev.

Changed names around to try and make the source code a little easier to follow per suggestions above.

This re-roll does NOT limit the configuration to FILE only keys in order to allow other 3rd party key providers to function. If Key's module ever gets a method to allow all methods EXCEPT a method that would solve this concern. I can see some users wanting to use the Keys module environment provider instead of settings.php with environment variables.

Looking at this I don't think this code will work with multivalue keys, I'm not sure if there is a way to exclude those from the selector list.

No interdiff due to the rebase.

Opinions?

Rebase and limited to Authentication key type to avoid the multivalue key problem.