From 2786769de5cb3c92e10e97dbee1a849e2e46aa16 Mon Sep 17 00:00:00 2001 From: Doug Aaser Date: Tue, 24 Sep 2019 14:42:11 +0000 Subject: [PATCH] Fix encrypted doc rendering This patchset fixes a bug where Deckhand was failing to perform substitution and layering on document sets where all the documents had a storagePolicy of encrypted. Deckhand would attempt to substitute from an encrypted source document, but when that document marked as encrypted, it fails because the source doc had been redacted. The behavior now goes as follows: - Resolve Barbican references before layering and substitution have been performed so that the prior two operations don't attempt to operate on a Barbican reference - After substitution, redact the destination document if it is marked as encrypted - Now, after substition, we can redact the rest of the documents and substitutions Change-Id: I725775d554c9eed2692fc6203c416a7119646680 --- deckhand/common/utils.py | 18 ++- deckhand/control/common.py | 3 - deckhand/control/revision_documents.py | 7 + deckhand/engine/layering.py | 53 ++++--- deckhand/engine/secrets_manager.py | 19 ++- .../test_rendered_documents_controller.py | 150 +++++++++++++++++- 6 files changed, 210 insertions(+), 40 deletions(-) diff --git a/deckhand/common/utils.py b/deckhand/common/utils.py index 2645e43d..362b99f6 100644 --- a/deckhand/common/utils.py +++ b/deckhand/common/utils.py @@ -394,13 +394,17 @@ def redact_document(document): :returns: Document with redacted data. :rtype: dict """ - d = _to_document(document) - if d.is_encrypted: - d.data = document_dict.redact(d.data) - for s in d.substitutions: - s['src']['path'] = document_dict.redact(s['src']['path']) - s['dest']['path'] = document_dict.redact(s['dest']['path']) - return d + doc = _to_document(document) + if doc.is_encrypted: + doc.data = document_dict.redact(doc.data) + for sub in doc.substitutions: + sub['src']['path'] = document_dict.redact(sub['src']['path']) + if isinstance(sub['dest'], list): + for dest in sub['dest']: + dest['path'] = document_dict.redact(dest['path']) + else: + sub['dest']['path'] = document_dict.redact(sub['dest']['path']) + return doc def redact_documents(documents): diff --git a/deckhand/control/common.py b/deckhand/control/common.py index b741f20b..b33bc395 100644 --- a/deckhand/control/common.py +++ b/deckhand/control/common.py @@ -23,7 +23,6 @@ import six from deckhand.barbican import cache as barbican_cache from deckhand.common import document as document_wrapper -from deckhand.common import utils from deckhand.db.sqlalchemy import api as db_api from deckhand import engine from deckhand.engine import cache as engine_cache @@ -164,8 +163,6 @@ def get_rendered_docs(revision_id, cleartext_secrets=False, **filters): documents = document_wrapper.DocumentDict.from_list(data) encryption_sources = _resolve_encrypted_data(documents) - if not cleartext_secrets: - documents = utils.redact_documents(documents) try: return engine.render( revision_id, diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 122ced04..5a4fc014 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -55,6 +55,8 @@ class RevisionDocumentsResource(api_base.BaseResource): sort_by = req.params.pop('sort', None) limit = req.params.pop('limit', None) cleartext_secrets = req.get_param_as_bool('cleartext-secrets') + if cleartext_secrets is None: + cleartext_secrets = True req.params.pop('cleartext-secrets', None) filters = req.params.copy() @@ -114,6 +116,8 @@ class RenderedDocumentsResource(api_base.BaseResource): filters['metadata.storagePolicy'].append('encrypted') cleartext_secrets = req.get_param_as_bool('cleartext-secrets') + if cleartext_secrets is None: + cleartext_secrets = True req.params.pop('cleartext-secrets', None) rendered_documents, cache_hit = common.get_rendered_docs( revision_id, cleartext_secrets, **filters) @@ -137,6 +141,9 @@ class RenderedDocumentsResource(api_base.BaseResource): limit = req.params.pop('limit', None) user_filters = req.params.copy() + if not cleartext_secrets: + rendered_documents = utils.redact_documents(rendered_documents) + rendered_documents = [ d for d in rendered_documents if utils.deepfilter( d, **user_filters)] diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index d2d199a6..4160c8e3 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -623,6 +623,24 @@ class DocumentLayering(object): if doc.is_control: continue + # Retrieve the encrypted data for the document if its + # data has been encrypted so that future references use the actual + # secret payload, rather than the Barbican secret reference. + if doc.is_encrypted and doc.has_barbican_ref: + encrypted_data = self.secrets_substitution\ + .get_unencrypted_data( + secret_ref=doc.data, + src_doc=doc, + dest_doc=doc + ) + if not doc.is_abstract: + doc.data = encrypted_data + self.secrets_substitution.update_substitution_sources( + meta=doc.meta, + data=encrypted_data + ) + self._documents_by_index[doc.meta].data = encrypted_data + LOG.debug("Rendering document %s:%s:%s", *doc.meta) if doc.parent_selector: @@ -633,15 +651,15 @@ class DocumentLayering(object): parent = self._documents_by_index[parent_meta] if doc.actions: - rendered_data = parent + rendered_doc = parent # Apply each action to the current document. for action in doc.actions: LOG.debug('Applying action %s to document with ' 'schema=%s, layer=%s, name=%s.', action, *doc.meta) try: - rendered_data = self._apply_action( - action, doc, rendered_data) + rendered_doc = self._apply_action( + action, doc, rendered_doc) except Exception: with excutils.save_and_reraise_exception(): try: @@ -649,10 +667,10 @@ class DocumentLayering(object): doc, parent, action) except Exception: # nosec pass - doc.data = rendered_data.data + doc.data = rendered_doc.data self.secrets_substitution.update_substitution_sources( - doc.meta, rendered_data.data) - self._documents_by_index[doc.meta] = rendered_data + doc.meta, rendered_doc.data) + self._documents_by_index[doc.meta] = rendered_doc else: LOG.debug( 'Skipped layering for document [%s, %s] %s which ' @@ -664,7 +682,7 @@ class DocumentLayering(object): # inherit from it, but only update the document's data if concrete. if doc.substitutions: try: - substituted_data = list( + substituted_doc = list( self.secrets_substitution.substitute_all(doc)) except Exception: with excutils.save_and_reraise_exception(): @@ -672,25 +690,14 @@ class DocumentLayering(object): self._log_data_for_substitution_failure(doc) except Exception: # nosec pass - if substituted_data: - rendered_data = substituted_data[0] + if substituted_doc: + rendered_doc = substituted_doc[0] # Update the actual document data if concrete. - doc.data = rendered_data.data + doc.data = rendered_doc.data if not doc.has_replacement: self.secrets_substitution.update_substitution_sources( - doc.meta, rendered_data.data) - self._documents_by_index[doc.meta] = rendered_data - # Otherwise, retrieve the encrypted data for the document if its - # data has been encrypted so that future references use the actual - # secret payload, rather than the Barbican secret reference. - elif doc.is_encrypted and doc.has_barbican_ref: - encrypted_data = self.secrets_substitution\ - .get_unencrypted_data(doc.data, doc, doc) - if not doc.is_abstract: - doc.data = encrypted_data - self.secrets_substitution.update_substitution_sources( - doc.meta, encrypted_data) - self._documents_by_index[doc.meta] = encrypted_data + doc.meta, rendered_doc.data) + self._documents_by_index[doc.meta] = rendered_doc # NOTE: Since the child-replacement is always prioritized, before # other children, as soon as the child-replacement layers with the diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index a4c8f5c8..6ee035ee 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -247,13 +247,13 @@ class SecretsSubstitution(object): exc_message = '' try: substituted_data = utils.jsonpath_replace( - document['data'], src_secret, dest_path, + document.data, src_secret, dest_path, pattern=dest_pattern, recurse=dest_recurse) - if (isinstance(document['data'], dict) and + if (isinstance(document.data, dict) and isinstance(substituted_data, dict)): - document['data'].update(substituted_data) + document.data.update(substituted_data) elif substituted_data: - document['data'] = substituted_data + document.data = substituted_data else: exc_message = ( 'Failed to create JSON path "%s" in the ' @@ -313,6 +313,7 @@ class SecretsSubstitution(object): for d in documents_to_substitute])) for document in documents_to_substitute: + redact_dest = False LOG.debug('Checking for substitutions for document [%s, %s] %s.', *document.meta) for sub in document.substitutions: @@ -337,6 +338,9 @@ class SecretsSubstitution(object): LOG.warning(message) continue + if src_doc.is_encrypted: + redact_dest = True + # If the data is a dictionary, retrieve the nested secret # via jsonpath_parse, else the secret is the primitive/string # stored in the data section itself. @@ -391,6 +395,13 @@ class SecretsSubstitution(object): dest_pattern=dest_pattern, dest_recurse=dest_recurse) + # If we just substituted from an encrypted document + # into a cleartext document, we need to redact the + # dest document as well so the secret stays hidden + if (not document.is_encrypted and redact_dest and not + self._cleartext_secrets): + document.storage_policy = 'encrypted' + yield document def update_substitution_sources(self, meta, data): diff --git a/deckhand/tests/unit/control/test_rendered_documents_controller.py b/deckhand/tests/unit/control/test_rendered_documents_controller.py index 0ed4c88f..9a022c21 100644 --- a/deckhand/tests/unit/control/test_rendered_documents_controller.py +++ b/deckhand/tests/unit/control/test_rendered_documents_controller.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re import yaml import mock @@ -220,7 +221,7 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest): certificate_data = 'sample-certificate' certificate_ref = ('http://127.0.0.1/key-manager/v1/secrets/%s' % test_utils.rand_uuid_hex()) - redacted_data = dd.redact(certificate_ref) + redacted_data = dd.redact(certificate_data) doc1 = { 'data': certificate_data, @@ -305,8 +306,151 @@ class TestRenderedDocumentsControllerRedaction(test_base.BaseControllerTest): self._test_list_rendered_documents(cleartext_secrets=False) -class TestRenderedDocumentsControllerNegative( - test_base.BaseControllerTest): +class TestRenderedDocumentsControllerEncrypted(test_base.BaseControllerTest): + + def test_substitution_rendered_documents_all_encrypted(self): + """Validates that substitution still functions correctly when all + the documents have a storagePolicy of encrypted + """ + rules = { + 'deckhand:list_cleartext_documents': '@', + 'deckhand:list_encrypted_documents': '@', + 'deckhand:create_cleartext_documents': '@', + 'deckhand:create_encrypted_documents': '@' + } + + self.policy.set_rules(rules) + + doc_factory = factories.DocumentFactory(1, [1]) + + layering_policy = doc_factory.gen_test({})[0] + layering_policy['data']['layerOrder'] = ['global', 'site'] + password_data = 'sample-super-secret-password' + original_data = 'http://admin:PASSWORD@service-name:8080/v1' + doc1_ref = ('http://127.0.0.1/key-manager/v1/secrets/%s' + % test_utils.rand_uuid_hex()) + doc2_ref = ('http://127.0.0.1/key-manager/v1/secrets/%s' + % test_utils.rand_uuid_hex()) + dest_data = { + 'url': original_data + } + subbed_data = { + 'url': re.sub('PASSWORD', password_data, original_data) + } + redacted_password = dd.redact(password_data) + redacted_data = dd.redact(subbed_data) + + original_substitutions = [ + { + 'dest': { + 'path': '.url', + 'pattern': 'PASSWORD' + }, + 'src': { + 'schema': 'deckhand/Passphrase/v1', + 'name': 'example-password', + 'path': '.' + } + } + ] + + # source + doc1 = { + 'data': password_data, + 'schema': 'deckhand/Passphrase/v1', + 'name': 'example-password', + 'layer': 'site', + 'metadata': { + 'labels': { + 'site': 'site1' + }, + 'storagePolicy': 'encrypted', + 'layeringDefinition': { + 'abstract': False, + 'layer': 'site' + }, + 'name': 'example-password', + 'schema': 'metadata/Document/v1', + 'replacement': False + } + } + + # destination + doc2 = { + 'data': dest_data, + 'schema': 'example/Kind/v1', + 'name': 'deckhand-global', + 'layer': 'global', + 'metadata': { + 'labels': { + 'global': 'global1' + }, + 'storagePolicy': 'encrypted', + 'layeringDefinition': { + 'abstract': False, + 'layer': 'global' + }, + 'name': 'deckhand-global', + 'schema': 'metadata/Document/v1', + 'substitutions': original_substitutions, + 'replacement': False + } + } + + payload = [layering_policy, doc1, doc2] + + # Create both documents and mock out SecretsManager.create to return + # a fake Barbican ref. + with mock.patch.object( # noqa + secrets_manager.SecretsManager, 'create', + side_effect=[doc1_ref, doc2_ref] + ): + resp = self.app.simulate_put( + '/api/v1.0/buckets/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) + self.assertEqual(200, resp.status_code) + revision_id = list(yaml.safe_load_all(resp.text))[0]['status'][ + 'revision'] + + # Retrieve rendered documents and simulate a Barbican lookup by + # returning both the password and the destination data + with mock.patch( # noqa + 'deckhand.control.common._resolve_encrypted_data', + return_value={ + doc1_ref: password_data, + doc2_ref: dest_data + } + ): + resp = self.app.simulate_get( + '/api/v1.0/revisions/%s/rendered-documents' % revision_id, + headers={'Content-Type': 'application/x-yaml'}, + params={ + 'metadata.name': ['example-password', 'deckhand-global'], + 'cleartext-secrets': False + }, + params_csv=False) + + self.assertEqual(200, resp.status_code) + rendered_documents = list(yaml.safe_load_all(resp.text)) + self.assertEqual(2, len(rendered_documents)) + + # Expect redacted data for all documents to be returned - + # because the destination documents should receive redacted data. + data = list(map(lambda x: x['data'], rendered_documents)) + self.assertTrue(redacted_password in data) + self.assertTrue(redacted_data in data) + + # Expect the substitutions to be redacted since both docs are + # marked as encrypted + destination_doc = next(iter(filter( + lambda x: x['metadata']['name'] == 'deckhand-global', + rendered_documents))) + substitutions = destination_doc['metadata']['substitutions'] + self.assertNotEqual(original_substitutions, substitutions) + + +class TestRenderedDocumentsControllerNegative(test_base.BaseControllerTest): def test_rendered_documents_fail_schema_validation(self): """Validates that when fully rendered documents fail basic schema