Browse Source

fix: Redact secondhand substitutions of sensitive data

This patch set ensures that documents that substitute data from
encrypted document sources are themselves redacted, assuming that
cleartext-secrets=true. Note that this redaction fix only applies
to the substitution dest/src paths. The data section is already
being correctly redacted for secondhand sources.

Change-Id: I6ce16a109628259b2cc8132cd9db63261b5dbace
Felipe Monteiro 5 months ago
parent
commit
47ade1f0da

+ 2
- 3
deckhand/common/document.py View File

@@ -173,9 +173,8 @@ class DocumentDict(dict):
173 173
         return [DocumentDict(d) for d in documents]
174 174
 
175 175
     @classmethod
176
-    def redact(cls, input):
177
-        return hashlib.sha256(json.dumps(input)
178
-                              .encode('utf-8')).hexdigest()
176
+    def redact(cls, field):
177
+        return hashlib.sha256(json.dumps(field).encode('utf-8')).hexdigest()
179 178
 
180 179
 
181 180
 def document_dict_representer(dumper, data):

+ 14
- 14
deckhand/common/utils.py View File

@@ -393,23 +393,23 @@ def redact_document(document):
393 393
     """
394 394
     d = _to_document(document)
395 395
     if d.is_encrypted:
396
-        document['data'] = document_dict.redact(d.data)
397
-        # FIXME(felipemonteiro): This block should be out-dented by 4 spaces
398
-        # because cleartext documents that substitute from encrypted documents
399
-        # should be subject to this redaction as well. However, doing this
400
-        # will result in substitution failures; the solution is to add a
401
-        # helper to :class:`deckhand.common.DocumentDict` that checks whether
402
-        # its metadata.substitutions is redacted - if so, skips substitution.
403
-        if d.substitutions:
404
-            subs = d.substitutions
405
-            for s in subs:
406
-                s['src']['path'] = document_dict.redact(s['src']['path'])
407
-                s['dest']['path'] = document_dict.redact(s['dest']['path'])
408
-            document['metadata']['substitutions'] = subs
409
-    return document
396
+        d.data = document_dict.redact(d.data)
397
+        for s in d.substitutions:
398
+            s['src']['path'] = document_dict.redact(s['src']['path'])
399
+            s['dest']['path'] = document_dict.redact(s['dest']['path'])
400
+    return d
410 401
 
411 402
 
412 403
 def redact_documents(documents):
404
+    """Redact sensitive data for each document in ``documents``.
405
+
406
+    Sensitive data includes ``data``, ``substitutions[n].src.path``, and
407
+    ``substitutions[n].dest.path`` fields.
408
+
409
+    :param list[dict] documents: List of documents whose data to redact.
410
+    :returns: Documents with redacted sensitive data.
411
+    :rtype: list[dict]
412
+    """
413 413
     return [redact_document(d) for d in documents]
414 414
 
415 415
 

+ 14
- 1
deckhand/control/common.py View File

@@ -148,6 +148,18 @@ def invalidate_cache_data():
148 148
 
149 149
 
150 150
 def get_rendered_docs(revision_id, cleartext_secrets=False, **filters):
151
+    """Helper for retrieving rendered documents for ``revision_id``.
152
+
153
+    Retrieves raw documents from DB, renders them, and returns rendered result
154
+    set.
155
+
156
+    :param int revision_id: Revision ID whose documents to render.
157
+    :param bool cleartext_secrets: Whether to show unencrypted data as
158
+        cleartext.
159
+    :param filters: Filters used for retrieving raw documents from DB.
160
+    :returns: List of rendered documents.
161
+    :rtype: list[dict]
162
+    """
151 163
     data = _retrieve_documents_for_rendering(revision_id, **filters)
152 164
     documents = document_wrapper.DocumentDict.from_list(data)
153 165
     encryption_sources = _resolve_encrypted_data(documents)
@@ -158,7 +170,8 @@ def get_rendered_docs(revision_id, cleartext_secrets=False, **filters):
158 170
         return engine.render(
159 171
             revision_id,
160 172
             documents,
161
-            encryption_sources=encryption_sources)
173
+            encryption_sources=encryption_sources,
174
+            cleartext_secrets=cleartext_secrets)
162 175
     except (errors.BarbicanClientException,
163 176
             errors.BarbicanServerException,
164 177
             errors.InvalidDocumentLayer,

+ 7
- 2
deckhand/engine/layering.py View File

@@ -384,7 +384,8 @@ class DocumentLayering(object):
384 384
                  documents,
385 385
                  validate=True,
386 386
                  fail_on_missing_sub_src=True,
387
-                 encryption_sources=None):
387
+                 encryption_sources=None,
388
+                 cleartext_secrets=False):
388 389
         """Contructor for ``DocumentLayering``.
389 390
 
390 391
         :param layering_policy: The document with schema
@@ -405,6 +406,9 @@ class DocumentLayering(object):
405 406
             actual unecrypted data. If encrypting data with Barbican, the
406 407
             reference will be a Barbican secret reference.
407 408
         :type encryption_sources: dict
409
+        :param cleartext_secrets: Whether to show unencrypted data as
410
+            cleartext.
411
+        :type cleartext_secrets: bool
408 412
 
409 413
         :raises LayeringPolicyNotFound: If no LayeringPolicy was found among
410 414
             list of ``documents``.
@@ -482,7 +486,8 @@ class DocumentLayering(object):
482 486
         self.secrets_substitution = secrets_manager.SecretsSubstitution(
483 487
             substitution_sources,
484 488
             encryption_sources=encryption_sources,
485
-            fail_on_missing_sub_src=fail_on_missing_sub_src)
489
+            fail_on_missing_sub_src=fail_on_missing_sub_src,
490
+            cleartext_secrets=cleartext_secrets)
486 491
 
487 492
         self._sorted_documents = self._topologically_sort_documents(
488 493
             substitution_sources)

+ 6
- 2
deckhand/engine/render.py View File

@@ -24,7 +24,8 @@ __all__ = ('render',
24 24
            'validate_render')
25 25
 
26 26
 
27
-def render(revision_id, documents, encryption_sources=None):
27
+def render(revision_id, documents, encryption_sources=None,
28
+           cleartext_secrets=False):
28 29
     """Render revision documents for ``revision_id`` using raw ``documents``.
29 30
 
30 31
     :param revision_id: Key used for caching rendered documents by.
@@ -37,6 +38,8 @@ def render(revision_id, documents, encryption_sources=None):
37 38
         actual unecrypted data. If encrypting data with Barbican, the
38 39
         reference will be a Barbican secret reference.
39 40
     :type encryption_sources: dict
41
+    :param cleartext_secrets: Whether to show unencrypted data as cleartext.
42
+    :type cleartext_secrets: bool
40 43
     :returns: Rendered documents for ``revision_id``.
41 44
     :rtype: List[dict]
42 45
 
@@ -49,7 +52,8 @@ def render(revision_id, documents, encryption_sources=None):
49 52
         revision_id,
50 53
         documents,
51 54
         encryption_sources=encryption_sources,
52
-        validate=False)
55
+        validate=False,
56
+        cleartext_secrets=cleartext_secrets)
53 57
 
54 58
 
55 59
 def validate_render(revision_id, rendered_documents, validator):

+ 70
- 44
deckhand/engine/secrets_manager.py View File

@@ -20,7 +20,7 @@ from oslo_log import log as logging
20 20
 import six
21 21
 
22 22
 from deckhand.barbican.driver import BarbicanDriver
23
-from deckhand.common import document as document_wrapper
23
+from deckhand.common.document import DocumentDict as dd
24 24
 from deckhand.common import utils
25 25
 from deckhand import errors
26 26
 
@@ -38,7 +38,7 @@ class SecretsManager(object):
38 38
 
39 39
     @staticmethod
40 40
     def requires_encryption(document):
41
-        clazz = document_wrapper.DocumentDict
41
+        clazz = dd
42 42
         if not isinstance(document, clazz):
43 43
             document = clazz(document)
44 44
         return document.is_encrypted
@@ -64,8 +64,8 @@ class SecretsManager(object):
64 64
         # TODO(fmontei): Look into POSTing Deckhand metadata into Barbican's
65 65
         # Secrets Metadata API to make it easier to track stale secrets from
66 66
         # prior revisions that need to be deleted.
67
-        if not isinstance(secret_doc, document_wrapper.DocumentDict):
68
-            secret_doc = document_wrapper.DocumentDict(secret_doc)
67
+        if not isinstance(secret_doc, dd):
68
+            secret_doc = dd(secret_doc)
69 69
 
70 70
         if secret_doc.is_encrypted:
71 71
             payload = cls.barbican_driver.create_secret(secret_doc)
@@ -100,8 +100,8 @@ class SecretsManager(object):
100 100
         :returns: None
101 101
 
102 102
         """
103
-        if not isinstance(document, document_wrapper.DocumentDict):
104
-            document = document_wrapper.DocumentDict(document)
103
+        if not isinstance(document, dd):
104
+            document = dd(document)
105 105
 
106 106
         secret_ref = document.data
107 107
         if document.is_encrypted and document.has_barbican_ref:
@@ -116,7 +116,7 @@ class SecretsSubstitution(object):
116 116
     """Class for document substitution logic for YAML files."""
117 117
 
118 118
     __slots__ = ('_fail_on_missing_sub_src', '_substitution_sources',
119
-                 '_encryption_sources')
119
+                 '_encryption_sources', '_cleartext_secrets')
120 120
 
121 121
     _insecure_reg_exps = (
122 122
         re.compile(r'^.* is not of type .+$'),
@@ -169,7 +169,9 @@ class SecretsSubstitution(object):
169 169
         return self._encryption_sources[secret_ref]
170 170
 
171 171
     def __init__(self, substitution_sources=None,
172
-                 fail_on_missing_sub_src=True, encryption_sources=None):
172
+                 fail_on_missing_sub_src=True,
173
+                 encryption_sources=None,
174
+                 cleartext_secrets=False):
173 175
         """SecretSubstitution constructor.
174 176
 
175 177
         This class will automatically detect documents that require
@@ -187,6 +189,9 @@ class SecretsSubstitution(object):
187 189
             actual unecrypted data. If encrypting data with Barbican, the
188 190
             reference will be a Barbican secret reference.
189 191
         :type encryption_sources: dict
192
+        :param cleartext_secrets: Whether to show unencrypted data as
193
+            cleartext.
194
+        :type cleartext_secrets: bool
190 195
         """
191 196
 
192 197
         # This maps a 2-tuple of (schema, name) to a document from which the
@@ -196,14 +201,15 @@ class SecretsSubstitution(object):
196 201
         self._substitution_sources = {}
197 202
         self._encryption_sources = encryption_sources or {}
198 203
         self._fail_on_missing_sub_src = fail_on_missing_sub_src
204
+        self._cleartext_secrets = cleartext_secrets
199 205
 
200 206
         if isinstance(substitution_sources, dict):
201 207
             self._substitution_sources = substitution_sources
202 208
         else:
203 209
             self._substitution_sources = dict()
204 210
             for document in substitution_sources:
205
-                if not isinstance(document, document_wrapper.DocumentDict):
206
-                    document = document_wrapper.DocumentDict(document)
211
+                if not isinstance(document, dd):
212
+                    document = dd(document)
207 213
                 if document.schema and document.name:
208 214
                     self._substitution_sources.setdefault(
209 215
                         (document.schema, document.name), document)
@@ -235,6 +241,43 @@ class SecretsSubstitution(object):
235 241
                             src_doc.name, dest_doc.schema, dest_doc.layer,
236 242
                             dest_doc.name)
237 243
 
244
+    def _substitute_one(self, document, src_doc, src_secret, dest_path,
245
+                        dest_pattern, dest_recurse=None):
246
+        dest_recurse = dest_recurse or {}
247
+        exc_message = ''
248
+        try:
249
+            substituted_data = utils.jsonpath_replace(
250
+                document['data'], src_secret, dest_path,
251
+                pattern=dest_pattern, recurse=dest_recurse)
252
+            if (isinstance(document['data'], dict) and
253
+                    isinstance(substituted_data, dict)):
254
+                document['data'].update(substituted_data)
255
+            elif substituted_data:
256
+                document['data'] = substituted_data
257
+            else:
258
+                exc_message = (
259
+                    'Failed to create JSON path "%s" in the '
260
+                    'destination document [%s, %s] %s. '
261
+                    'No data was substituted.' % (
262
+                        dest_path, document.schema,
263
+                        document.layer, document.name))
264
+        except Exception as e:
265
+            LOG.error('Unexpected exception occurred '
266
+                      'while attempting '
267
+                      'substitution using '
268
+                      'source document [%s, %s] %s '
269
+                      'referenced in [%s, %s] %s. Details: %s',
270
+                      src_doc.schema, src_doc.name, src_doc.layer,
271
+                      document.schema, document.layer,
272
+                      document.name,
273
+                      six.text_type(e))
274
+            exc_message = six.text_type(e)
275
+        finally:
276
+            if exc_message:
277
+                self._handle_unknown_substitution_exc(
278
+                    exc_message, src_doc, document)
279
+        return document
280
+
238 281
     def substitute_all(self, documents):
239 282
         """Substitute all documents that have a `metadata.substitutions` field.
240 283
 
@@ -259,8 +302,8 @@ class SecretsSubstitution(object):
259 302
             documents = [documents]
260 303
 
261 304
         for document in documents:
262
-            if not isinstance(document, document_wrapper.DocumentDict):
263
-                document = document_wrapper.DocumentDict(document)
305
+            if not isinstance(document, dd):
306
+                document = dd(document)
264 307
             # If the document has substitutions include it.
265 308
             if document.substitutions:
266 309
                 documents_to_substitute.append(document)
@@ -322,43 +365,26 @@ class SecretsSubstitution(object):
322 365
                     dest_pattern = each_dest_path.get('pattern', None)
323 366
                     dest_recurse = each_dest_path.get('recurse', {})
324 367
 
368
+                    # If the source document is encrypted and cleartext_secrets
369
+                    # is False, then redact the substitution metadata in the
370
+                    # destination document to prevent reverse-engineering of
371
+                    # where the sensitive data came from.
372
+                    if src_doc.is_encrypted and not self._cleartext_secrets:
373
+                        sub['src']['path'] = dd.redact(src_path)
374
+                        sub['dest']['path'] = dd.redact(dest_path)
375
+
325 376
                     LOG.debug('Substituting from schema=%s layer=%s name=%s '
326 377
                               'src_path=%s into dest_path=%s, dest_pattern=%s',
327 378
                               src_schema, src_doc.layer, src_name, src_path,
328 379
                               dest_path, dest_pattern)
329 380
 
330
-                    try:
331
-                        exc_message = ''
332
-                        substituted_data = utils.jsonpath_replace(
333
-                            document['data'], src_secret, dest_path,
334
-                            pattern=dest_pattern, recurse=dest_recurse)
335
-                        if (isinstance(document['data'], dict) and
336
-                                isinstance(substituted_data, dict)):
337
-                            document['data'].update(substituted_data)
338
-                        elif substituted_data:
339
-                            document['data'] = substituted_data
340
-                        else:
341
-                            exc_message = (
342
-                                'Failed to create JSON path "%s" in the '
343
-                                'destination document [%s, %s] %s. '
344
-                                'No data was substituted.' % (
345
-                                    dest_path, document.schema,
346
-                                    document.layer, document.name))
347
-                    except Exception as e:
348
-                        LOG.error('Unexpected exception occurred '
349
-                                  'while attempting '
350
-                                  'substitution using '
351
-                                  'source document [%s, %s] %s '
352
-                                  'referenced in [%s, %s] %s. Details: %s',
353
-                                  src_schema, src_name, src_doc.layer,
354
-                                  document.schema, document.layer,
355
-                                  document.name,
356
-                                  six.text_type(e))
357
-                        exc_message = six.text_type(e)
358
-                    finally:
359
-                        if exc_message:
360
-                            self._handle_unknown_substitution_exc(
361
-                                exc_message, src_doc, document)
381
+                    document = self._substitute_one(
382
+                        document,
383
+                        src_doc=src_doc,
384
+                        src_secret=src_secret,
385
+                        dest_path=dest_path,
386
+                        dest_pattern=dest_pattern,
387
+                        dest_recurse=dest_recurse)
362 388
 
363 389
         yield document
364 390
 

+ 20
- 6
deckhand/tests/unit/control/test_rendered_documents_controller.py View File

@@ -16,6 +16,7 @@ import yaml
16 16
 
17 17
 import mock
18 18
 
19
+from deckhand.common.document import DocumentDict as dd
19 20
 from deckhand.control import revision_documents
20 21
 from deckhand.engine import secrets_manager
21 22
 from deckhand import errors
@@ -200,6 +201,10 @@ class TestRenderedDocumentsController(test_base.BaseControllerTest):
200 201
 class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest):
201 202
 
202 203
     def _test_list_rendered_documents(self, cleartext_secrets):
204
+        """Validates that destination document that substitutes from an
205
+        encrypted document is appropriately redacted when ``cleartext_secrets``
206
+        is True.
207
+        """
203 208
         rules = {
204 209
             'deckhand:list_cleartext_documents': '@',
205 210
             'deckhand:list_encrypted_documents': '@',
@@ -215,6 +220,7 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest):
215 220
         certificate_data = 'sample-certificate'
216 221
         certificate_ref = ('http://127.0.0.1/key-manager/v1/secrets/%s'
217 222
                            % test_utils.rand_uuid_hex())
223
+        redacted_data = dd.redact(certificate_ref)
218 224
 
219 225
         doc1 = {
220 226
             'data': certificate_data,
@@ -228,6 +234,11 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest):
228 234
                     'layer': 'site'}, 'storagePolicy': 'encrypted',
229 235
                 'replacement': False}}
230 236
 
237
+        original_substitutions = [
238
+            {'dest': {'path': '.'},
239
+             'src': {'schema': 'deckhand/Certificate/v1',
240
+                     'name': 'example-cert', 'path': '.'}}
241
+        ]
231 242
         doc2 = {'data': {}, 'schema': 'example/Kind/v1',
232 243
                 'name': 'deckhand-global', 'layer': 'global',
233 244
                 'metadata': {
@@ -236,10 +247,8 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest):
236 247
                     'layeringDefinition': {'abstract': False,
237 248
                                            'layer': 'global'},
238 249
                     'name': 'deckhand-global',
239
-                    'schema': 'metadata/Document/v1', 'substitutions': [
240
-                        {'dest': {'path': '.'},
241
-                         'src': {'schema': 'deckhand/Certificate/v1',
242
-                                 'name': 'example-cert', 'path': '.'}}],
250
+                    'schema': 'metadata/Document/v1',
251
+                    'substitutions': original_substitutions,
243 252
                     'replacement': False}}
244 253
 
245 254
         payload = [layering_policy, doc1, doc2]
@@ -279,10 +288,15 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest):
279 288
             self.assertTrue(all(map(lambda x: x['data'] == certificate_data,
280 289
                                 rendered_documents)))
281 290
         else:
282
-            # Expected redacted data for both documents to be returned -
291
+            # Expect redacted data for both documents to be returned -
283 292
             # because the destination document should receive redacted data.
284
-            self.assertTrue(all(map(lambda x: x['data'] != certificate_data,
293
+            self.assertTrue(all(map(lambda x: x['data'] == redacted_data,
285 294
                                 rendered_documents)))
295
+            destination_doc = next(iter(filter(
296
+                lambda x: x['metadata']['name'] == 'deckhand-global',
297
+                rendered_documents)))
298
+            substitutions = destination_doc['metadata']['substitutions']
299
+            self.assertNotEqual(original_substitutions, substitutions)
286 300
 
287 301
     def test_list_rendered_documents_cleartext_secrets_true(self):
288 302
         self._test_list_rendered_documents(cleartext_secrets=True)

+ 4
- 2
deckhand/tests/unit/engine/test_secrets_manager.py View File

@@ -176,7 +176,8 @@ class TestSecretsSubstitution(test_base.TestDbBase):
176 176
         self.secrets_factory = factories.DocumentSecretFactory()
177 177
 
178 178
     def _test_doc_substitution(self, document_mapping, substitution_sources,
179
-                               expected_data, encryption_sources=None):
179
+                               expected_data, encryption_sources=None,
180
+                               cleartext_secrets=True):
180 181
         payload = self.document_factory.gen_test(document_mapping,
181 182
                                                  global_abstract=False)
182 183
         bucket_name = test_utils.rand_name('bucket')
@@ -188,7 +189,8 @@ class TestSecretsSubstitution(test_base.TestDbBase):
188 189
 
189 190
         secret_substitution = secrets_manager.SecretsSubstitution(
190 191
             encryption_sources=encryption_sources,
191
-            substitution_sources=substitution_sources)
192
+            substitution_sources=substitution_sources,
193
+            cleartext_secrets=cleartext_secrets)
192 194
         substituted_docs = list(secret_substitution.substitute_all(documents))
193 195
         self.assertIn(expected_document, substituted_docs)
194 196
 

Loading…
Cancel
Save