Caching of decrypted data #46

svendecabooter · 2016-03-28T13:50:44Z

First draft to tackle the caching of encrypted data issue (see #17).

This PR:

adds a setting to the field storage field_encrypt settings to decide whether this particular encrypted field should be excluded from (render) caching.
adds a first attempt at defining tests to test the cacheability of the encrypted data.
implements hook_entity_view to set the cache max-age setting to 0 conditionally.

nerdstein · 2016-03-28T14:24:47Z

config/schema/field_encrypt.schema.yml

@@ -14,6 +14,9 @@ field.storage.*.*.third_party.field_encrypt:
 encryption_profile:
 type: string
 label: 'Encryption profile'
+ cache_exclude:
+ type: boolean
+ label: 'Exclude from cache'


This is a bit ambiguous. We're only focusing on render cache, right?

I think it's broader than just render cache. When caching the entity object itself, unencrypted field data also shouldn't be stored in the (database) cache...

I think uncacheable would be a better name. It'd also fit in better with the terminology in Drupal core.

… should be excluded from caching.

nerdstein · 2016-03-28T14:42:24Z

src/FieldEncryptProcessEntities.php

+
+ // If cache_exclude is set, set caching max-age to 0.
+ if ($storage->getThirdPartySetting('field_encrypt', 'cache_exclude', TRUE) == TRUE) {
+ $build[$field->getName()]['#cache']['max-age'] = 0;


svendecabooter · 2016-03-28T14:48:39Z

TODO:

Figure out if this approach is OK.
Figure out how exactly to test the other caching layers defined by Wim

…an be extended when it lands in Encrypt module.

…ed in entity cache.

svendecabooter · 2016-04-08T15:19:50Z

Extra logic and test added to fix caching on entity level, in 'cache.entity'.

wimleers · 2016-04-11T15:35:24Z

field_encrypt.module

+ $form['field_encrypt']['field_encrypt']['cache_exclude'] = [
+ '#type' => 'checkbox',
+ '#title' => t('Exclude from cache'),
+ '#description' => t('Exclude this field from being cached. This will make sure your unencrypted data will not be exposed in the cache, but could have impact on your performance.'),


s/could have impact/will have a negative impact/

weitzman · 2016-04-11T16:54:03Z

I haven't studied all the commits here, but I wonder why a field would decrypt during entity load. I would decrypyt at render time and mark the render array as max-age=0. That way we dont have to worry about entity cache, as it stores encrypted value.

wimleers · 2016-04-12T13:24:07Z

@weitzman Great idea. But I'm not entirely sure that's going to be possible. If field values are encrypted at all times except rendering, then I can imagine that anything relying on field data won't work correctly: Views, Rules …

This is also not how the module works currently, at least as far as I can tell based on https://raw.githubusercontent.com/d8-contrib-modules/field_encrypt/master/documentation/Field%20Encrypt%20Architecture.png.

I wish https://github.com/d8-contrib-modules/field_encrypt/blob/master/readme.md explained the architecture in detail, and especially the rationale for going with this approach. It's sparsely documented.

Which is alsow hy I had to ask #17 (comment) in the first place.

nerdstein · 2016-04-12T13:28:55Z

@wimleers - I'm happy to elaborate on the architecture in greater detail if you can be more clear on gaps you have found. Do you want to know what hooks are being used? How we're storing things? Please share some of what you wish to see and I'll add it.

And, yes, any sort of metadata or data used in the processing of other entities (blocks, views, etc) will break if we only do rendering. In fact, we tried it.

wimleers · 2016-04-12T13:30:37Z

Great, could you document in the README then why you did not go that way? That'd help address concerns that anybody evaluating this module will have.

wimleers · 2016-04-12T13:34:34Z

@nerdstein And yes, it would be great if you could comprehensively document how this module deals with:

decrypting
encrypting
preventing decrypted information from being accessed and therefore leaked, especially by contrib/custom modules
related to prior point: which caveats to pay attention to as a contrib/custom module developer
how this module guarantees that decrypted content is NEVER cached, and why it must never be cached — or perhaps in which cases caching is allowed

i.e. any and all aspects affecting encryption, decryption and passing around decrypted data need to be documented. And actually… also how we're guaranteeing that the decryption keys are stored safely.

EDIT:
To clarify, I think such comprehensive documentation is crucial in a system as flexible as Drupal, where anybody can override anything on any level. Just one weak spot, and this module is rendered ineffective. That's why for a module like this, you need to have comprehensive (architecture) documentation and test coverage proving all expected weak spots are in fact protected.

nerdstein · 2016-04-12T13:36:38Z

I will do so for the module as it is right now - but I'm very much open to adjusting the architecture if there is a better way to do this. That's precisely why we're asking for your contributions.

Our previous attempts to be more comprehensive with this may have been off, but it might not have been implemented correctly either.

If we have to make some architectural adjustments for this module - we should do it now.

svendecabooter · 2016-04-13T16:56:04Z

@wimleers: I have updated the code based on your feedback to set the cache properties on the EntityType.

This works locally, but somehow the test case seems to cache the entity where it shouldn't. Still need to figure out why that assertion is failing exactly, as I can't reproduce it outside of the test.

… changed

svendecabooter · 2016-04-22T09:01:47Z

In the latest commit I have taken an alternative approach to determine which entity types should be uncacheable. This seems to work better, and now the test cases are all passing.

Feedback welcome.

nerdstein · 2016-04-26T13:00:57Z

field_encrypt.module

@@ -84,6 +88,19 @@ function field_encrypt_form_alter(&$form, FormStateInterface $form_state, $form_
 ],
 );

+ // Add setting to decide if field should be excluded from cache.
+ $form['field_encrypt']['field_encrypt']['uncacheable'] = [


nerdstein · 2016-04-26T13:06:13Z

Looks good to me! I'll be merging and i'll be doing extensive testing next week

svendecabooter added 5 commits March 28, 2016 11:32

Add "cache_exclude" setting to field encryption settings.

47ce238

Move common test logic to FieldEncryptTestBase class.

66c2035

Move common test logic to FieldEncryptTestBase class.

7db135c

Fix test assertion

6856066

Add cache tests

d287f43

nerdstein reviewed Mar 28, 2016
View reviewed changes

Conditionally set field caching max-age to 0 on encrypted fields that…

3586edb

… should be excluded from caching.

nerdstein reviewed Mar 28, 2016
View reviewed changes

svendecabooter mentioned this pull request Mar 28, 2016

Evaluate caching of decrypted data #17

Open

Fix method doc

df03c07

svendecabooter added 4 commits March 28, 2016 16:50

remove debugging code

a1cae40

Make base test class abstract

f59c44c

Make test setUp more compatible with EncryptTestBase, so this class c…

a4e2fb9

…an be extended when it lands in Encrypt module.

Make sure entities with fields marked for cache exclusion aren't cach…

7d5e055

…ed in entity cache.

wimleers reviewed Apr 11, 2016
View reviewed changes

svendecabooter added 2 commits April 13, 2016 13:51

Rename "cache_exclude" to "uncacheable", to better match Drupal core

342bb7e

Mark entity types uncacheable if they have uncacheable fields.

f8af20e

Set cacheability options on entity types when field_encrypt config is…

8ffc5da

… changed

nerdstein reviewed Apr 26, 2016
View reviewed changes

nerdstein merged commit f5ea590 into d8-contrib-modules:master Apr 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caching of decrypted data #46

Caching of decrypted data #46

svendecabooter commented Mar 28, 2016

nerdstein Mar 28, 2016

svendecabooter Mar 28, 2016

wimleers Apr 11, 2016

nerdstein Mar 28, 2016

svendecabooter commented Mar 28, 2016

svendecabooter commented Apr 8, 2016

wimleers Apr 11, 2016

weitzman commented Apr 11, 2016

wimleers commented Apr 12, 2016

nerdstein commented Apr 12, 2016

wimleers commented Apr 12, 2016

wimleers commented Apr 12, 2016

nerdstein commented Apr 12, 2016

svendecabooter commented Apr 13, 2016

svendecabooter commented Apr 22, 2016

nerdstein Apr 26, 2016

nerdstein commented Apr 26, 2016

Caching of decrypted data #46

Caching of decrypted data #46

Conversation

svendecabooter commented Mar 28, 2016

nerdstein Mar 28, 2016

Choose a reason for hiding this comment

svendecabooter Mar 28, 2016

Choose a reason for hiding this comment

wimleers Apr 11, 2016

Choose a reason for hiding this comment

nerdstein Mar 28, 2016

Choose a reason for hiding this comment

svendecabooter commented Mar 28, 2016

svendecabooter commented Apr 8, 2016

wimleers Apr 11, 2016

Choose a reason for hiding this comment

weitzman commented Apr 11, 2016

wimleers commented Apr 12, 2016

nerdstein commented Apr 12, 2016

wimleers commented Apr 12, 2016

wimleers commented Apr 12, 2016

nerdstein commented Apr 12, 2016

svendecabooter commented Apr 13, 2016

svendecabooter commented Apr 22, 2016

nerdstein Apr 26, 2016

Choose a reason for hiding this comment

nerdstein commented Apr 26, 2016