Comments

Cameron Tod created an issue. See original summary.

cameron tod’s picture

Issue tags: +Security
naveenvalecha’s picture

@Cameron Tod,
can we move the password storage to the settings.php instead of configuration ?
That way we can access it via Settings final class.
can we also show a warning on the status page i.e. hook_requirements if site is on http protocol.

cameron tod’s picture

I think we can offer that option, but to satisfy Akamai's concerns I think we'd still need to encrypt the value.

barrett’s picture

It seems rather heavy handed to mandate the use of Field Encrypt (especially since their D8 is beta and their D7 is largely unsupported). What's the approach you have in mind for this?

When the issue has come up previously (see #2569845: Store akamai password encrypted for example), there have been some efforts at it but the consensus of users was that we needed Akamai to implement an API-key style authentication rather than username and password structures.

I am glad to see Akamai finally being active in regards to the module, though.

cameron tod’s picture

I would much prefer Akamai had an API key based auth, but I'd need to open a dialogue with them to understand if that is possible and/or on their roadmap.

My initial thoughts were to use the Key module, write a plugin that provided encrypted keys, and apply that to the user pass, using $settings['drupal_hash_salt'] as an encryption key. That is based on quick survey of what's available and stable in d8 rather than a rigorous analysis or PoC.

If I can achieve 2 way encryption using native D8 code I think that's preferable, but I need to dig in to understand what's available. A quick look suggests maybe not.

synedra’s picture

The credentials for the new OPEN versions of the API (including the one that you need to use for the fast invalidate) use the OPEN authentication mechanism, not a user/password. As with OAuth there are 4 pieces of information needed to make a successful call:
1) Hostname (different for each credential set)
2) Client token
3) Client secret
4) Access token

On a standard server setup, these are stored in an INI configuration file in the ~/.edgerc directory.

The edgegrid-php library uses these credentials appropriately. Davey Shafik is working on a similar Wordpress plugin which will do the same thing.

Thanks,
Kirsten

cameron tod’s picture

StatusFileSize
new164.2 KB

Here's a screenshot of the config screen. Everything in 8.x-3.x is based on the OPEN APIs (specifically https://github.com/akamai-open/AkamaiOPEN-edgegrid-php) We actually don't use Akamai user/pass at all which I didn't really pick up on as your primary concern earlier:

config screen

Is this still something you need encrypted/accessed only via HTTPs?

dshafik’s picture

Given the difficulty of securing encryption keys for encrypted data, I think the best option is to allow the following mechanisms (which is my goal for WordPress also):

  1. Point to local .edgerc file and INI section
  2. Upload .edgerc file and store locally, and specify INI section. Require TLS.
  3. Use environment variables, specifically the following four elements:
    • AKAMAI_EDGEGRID_HOST
    • AKAMAI_EDGEGRID_CLIENT_TOKEN
    • AKAMAI_EDGEGRID_CLIENT_SECRET
    • AKAMAI_EDGEGRID_ACCESS_TOKEN
    • These environment variables should be set by whatever deployment tools are being used, as is common practice these days.

I think standardizing on this mechanism across multiple projects will be great for everyone.

dshafik’s picture

@Cameron Tod

The API credentials should be treated the same as login credentials, see my previous comment on my recommendations for handling this.

Thanks,

cameron tod’s picture

It's tough for us to require environment variables, as that breaks a Drupal principle of allowing people to admin sites on environments where they may not be able to create them for whatever reason (PaaS, shared hosting, lack of knowledge, etc). That's not to say its out of the question but I just need to think it through.

I've been playing around with https://github.com/defuse/php-encryption as a means to encrypt the credentials, but to encrypt securely we'd need to save a key somewhere. If that is not saved in private files, we're just moving the problem around (though we would have encrypted values in the DB, the key would be available over the public web). We could mandate use of private files, but in D8 that requires setting up some things on the server - at which point you might as well store the .edgerc directly, or even set envvars.

I am still evaluating - but thanks for all the suggestions so far!

cameron tod’s picture

Created followup for upload of .edgerc files: #2744253: Allow users to upload .edgerc file to specify credentials

cameron tod’s picture

Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new31.82 KB

Here's where I'm at so far.

- Credentials are now provided by a plugin service
- Credentials are now encrypted using openssl, utilising a secure generated initialisation vector, Drupal's hash_salt (stored in code, not in the db), and AES-256-OFB.

If credentials are to be stored in code (settings.php), then there is a standalone credentials plugin for that.

Moving credentials to a plugin should make it easier to support different ways of supplying creds, like file or envvar based.

I just need to write some tests for the new plugins and alter the existing tests to supply credentials via the new approach.

cameron tod’s picture

+++ b/src/CredentialsFactory.php
@@ -0,0 +1,22 @@
+    return new $plugin_class(\Drupal::configFactory());

Woops, need to inject this

cashwilliams’s picture

I feel like the solution should be to provide the ability to use the drupal.org/project/key module that is built to do exactly this type of thing. It is built so users can choose what storage method they'd like to store key, including the ability to store offsite using HSM etc.

cameron tod’s picture

I think that's probably best. Implementing my own encryption is asking for bugs down the road. I am also a bit reluctant to add so much extra code when its probably un-needed. I will take another look.

WidgetsBurritos’s picture

Status: Needs review » Needs work

At the very least this needs to be rerolled, but I'd be interested in testing out the Key module integration here as well.

WidgetsBurritos’s picture

Assigned: cameron tod » Unassigned
Status: Needs work » Needs review
Issue tags: +ContributionWeekend2019
StatusFileSize
new9.41 KB

Here is a proof of concept using the key module. Still need to figure out what to do from a testing perspective, but it at least shows what it would take to use the key module.

It is worth noting this issue with the key module: #2980072: Encrypt key value

We should probably help push that issue along as well.

Status: Needs review » Needs work

The last submitted patch, 18: akamai--2741225--use-key-module--18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

WidgetsBurritos’s picture

Status: Needs work » Needs review
StatusFileSize
new11.12 KB
new5.06 KB

Here is an updated patch that helps facilitate some of the optional dependency injection.

I haven't started to address any of the test failures yet.

Status: Needs review » Needs work

The last submitted patch, 20: akamai--2741225--use-key-module--20.patch, failed testing. View results

WidgetsBurritos’s picture

Status: Needs work » Needs review
StatusFileSize
new13.66 KB
new2.41 KB

This patch takes the test out for the database method as we've removed it. I haven't added tests for the key integration yet. That probably should exist as either a Functional or Kernel test.

Status: Needs review » Needs work

The last submitted patch, 22: akamai--2741225--use-key-module--22.patch, failed testing. View results

WidgetsBurritos’s picture

StatusFileSize
new13.76 KB
new1.92 KB

Wow ignore that last patch. I had a rebase glitch there. Let's try that again.

WidgetsBurritos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: akamai--2741225--use-key-module--24.patch, failed testing. View results

WidgetsBurritos’s picture

Status: Needs work » Needs review

I believe this will resolve the remaining test errors. It required a bit of reworking of the AkamaiAuthentication class to inject the messenger and key repository services into them.

WidgetsBurritos’s picture

StatusFileSize
new11.51 KB
new20.92 KB
WidgetsBurritos’s picture

Status: Needs review » Needs work

Based on feedback during ContributionWeekend2019:

  1. +++ b/src/AkamaiAuthentication.php
    @@ -36,18 +41,37 @@ class AkamaiAuthentication extends Authentication {
    +      }
    

    What about else here?

  2. +++ b/src/Form/ConfigForm.php
    @@ -55,11 +63,19 @@ class ConfigForm extends ConfigFormBase {
    +    }
    

    Instead of doing all this, let's create a custom service that can handle the optional service and the variety of options (e.g. edgerc or key module)

WidgetsBurritos’s picture

StatusFileSize
new40.45 KB
new11.78 KB

This patch contains a new akamai.key_provider service which acts as a wrapper around the key module's key.repository service. It's not exactly what was suggested in #29.2, as the .edgerc handling is handled by the external akamai libraries, so I limited the scope of what I focused on to just creating a wrapper around the key.repository. In regard to #29.1, we don't actually need to worry about this case as $auth is just empty, but all the functionality fails gracefully already.

WidgetsBurritos’s picture

Status: Needs work » Needs review
StatusFileSize
new28.03 KB

Consolidating multiple commits from #30

Status: Needs review » Needs work

The last submitted patch, 31: akamai--2741225--use-key-module--31.patch, failed testing. View results

WidgetsBurritos’s picture

Status: Needs work » Needs review
StatusFileSize
new544 bytes
new28.36 KB

Switching to composer-based test dependencies instead of info file.

rocketeerbkw’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and discussed at 2019 Austin Contribution Weekend. Removing the built-in config store and allowing users to instead use they key module to protect API keys seems like a good compromise approach. Latest patch LGTM.

  • WidgetsBurritos committed 9ee7b17 on 8.x-3.x
    Issue #2741225 by WidgetsBurritos, Cameron Tod, dshafik, Barrett,...
WidgetsBurritos’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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