From 021090516b589cbdb0d53d417ce91e60826ccfb1 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 24 Jan 2018 15:58:40 -0500 Subject: [PATCH] Improve validation error messages returned by Deckhand Currently the format of the validation error messages returned by the Deckhand Validation API is lacking. The new response is a dictionary with the following keys: * validation_schema: The schema body that was used to validate the document. * schema_path: The JSON path in the schema where the failure originated. * name: The document name. * schema: The document schema. * path: The JSON path in the document where the failure originated. * error_section: The "section" in the document above which the error originated (i.e. the dict in which ``path`` is found). * message: The error message returned by the ``jsonschema`` validator. This PS updates the document validation module and associated unit tests to return and verify the above format. Change-Id: I9ef1c36db85233cbfb866dea786228ef1416468c --- deckhand/engine/document_validation.py | 81 +++++++++++++++---- deckhand/engine/secrets_manager.py | 14 ++++ .../control/test_validations_controller.py | 61 ++++++++++---- .../unit/engine/test_document_validation.py | 25 ++++++ .../test_document_validation_negative.py | 5 ++ 5 files changed, 152 insertions(+), 34 deletions(-) diff --git a/deckhand/engine/document_validation.py b/deckhand/engine/document_validation.py index cae804ce..60ef4089 100644 --- a/deckhand/engine/document_validation.py +++ b/deckhand/engine/document_validation.py @@ -22,6 +22,7 @@ import six from deckhand.engine import document_wrapper from deckhand.engine.schema import base_schema from deckhand.engine.schema import v1_0 +from deckhand.engine.secrets_manager import SecretsSubstitution from deckhand import errors from deckhand import types from deckhand import utils @@ -131,7 +132,7 @@ class SchemaValidator(BaseValidator): :rtype: dict """ - schema_prefix, schema_version = get_schema_parts(document) + schema_prefix, schema_version = _get_schema_parts(document) matching_schemas = [] relevant_schemas = self._schema_map.get(schema_version, {}) for candidae_schema_prefix, schema in relevant_schemas.items(): @@ -194,9 +195,8 @@ class SchemaValidator(BaseValidator): 'Failed schema validation for document [%s] %s. ' 'Details: %s.', document.schema, document.name, error.message) - parent_path = root_path + '.'.join( - [six.text_type(x) for x in error.path]) - yield error.message, parent_path + yield _generate_validation_error_output( + schema_to_use, document, error, root_path) class DataSchemaValidator(SchemaValidator): @@ -218,7 +218,7 @@ class DataSchemaValidator(SchemaValidator): continue if 'data' not in data_schema: continue - schema_prefix, schema_version = get_schema_parts(data_schema, + schema_prefix, schema_version = _get_schema_parts(data_schema, 'metadata.name') class Schema(object): @@ -233,7 +233,7 @@ class DataSchemaValidator(SchemaValidator): LOG.info('Skipping schema validation for abstract document [%s]: ' '%s.', document.schema, document.name) return False - schema_prefix, schema_version = get_schema_parts(document) + schema_prefix, schema_version = _get_schema_parts(document) return schema_prefix in self._schema_map.get(schema_version, {}) def validate(self, document): @@ -328,7 +328,7 @@ class DocumentValidation(object): supported_schema_list = self._get_supported_schema_list() document_schema = None if not document.get('schema') else '/'.join( - get_schema_parts(document)) + _get_schema_parts(document)) if document_schema not in supported_schema_list: error_msg = ("The provided document schema %s is invalid. " "Supported schemas include: %s" % ( @@ -344,15 +344,10 @@ class DocumentValidation(object): for validator in self._validators: if validator.matches(document): - error_messages = validator.validate(document) - if error_messages: - for error_msg, error_path in error_messages: - result['errors'].append({ - 'schema': document.schema, - 'name': document.name, - 'message': error_msg, - 'path': error_path - }) + error_outputs = validator.validate(document) + if error_outputs: + for error_output in error_outputs: + result['errors'].append(error_output) if result['errors']: result.setdefault('status', 'failure') @@ -420,10 +415,62 @@ class DocumentValidation(object): return validations -def get_schema_parts(document, schema_key='schema'): +def _get_schema_parts(document, schema_key='schema'): schema_parts = utils.jsonpath_parse(document, schema_key).split('/') schema_prefix = '/'.join(schema_parts[:2]) schema_version = schema_parts[2] if schema_version.endswith('.0'): schema_version = schema_version[:-2] return schema_prefix, schema_version + + +def _generate_validation_error_output(schema, document, error, root_path): + """Returns a formatted output with necessary details for debugging why + a validation failed. + + The response is a dictionary with the following keys: + + * validation_schema: The schema body that was used to validate the + document. + * schema_path: The JSON path in the schema where the failure originated. + * name: The document name. + * schema: The document schema. + * path: The JSON path in the document where the failure originated. + * error_section: The "section" in the document above which the error + originated (i.e. the dict in which ``path`` is found). + * message: The error message returned by the ``jsonschema`` validator. + + :returns: Dictionary in the above format. + """ + path_to_error_in_document = root_path + '.'.join( + [str(x) for x in error.path]) + path_to_error_in_schema = '.' + '.'.join( + [str(x) for x in error.schema_path]) + + parent_path_to_error_in_document = '.'.join( + path_to_error_in_document.split('.')[:-1]) or '.' + try: + parent_error_section = utils.jsonpath_parse( + document, parent_path_to_error_in_document) + if 'data' in parent_error_section: + # NOTE(fmontei): Because validation is performed on fully rendered + # documents, it is necessary to omit the parts of the data section + # where substitution may have occurred to avoid exposing any + # secrets. While this may make debugging a few validation failures + # more difficult, it is a necessary evil. + SecretsSubstitution.sanitize_potential_secrets(document) + except Exception: + parent_error_section = ( + 'Failed to find parent section above where error occurred.') + + error_output = { + 'validation_schema': schema.schema, + 'schema_path': path_to_error_in_schema, + 'name': document.name, + 'schema': document.schema, + 'path': path_to_error_in_document, + 'error_section': parent_error_section, + 'message': error.message + } + + return error_output diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index fe4f81dc..3b5ec8cb 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -216,3 +216,17 @@ class SecretsSubstitution(object): substituted_docs.append(document) return substituted_docs + + @staticmethod + def sanitize_potential_secrets(document): + """Sanitize all secret data that may have been substituted into the + document. Uses references in ``document.substitutions`` to determine + which values to sanitize. Only meaningful to call this on post-rendered + documents. + + :param DocumentDict document: Document to sanitize. + """ + safe_message = 'Sanitized to avoid exposing secret.' + for sub in document.substitutions: + utils.jsonpath_replace(document['data'], safe_message, + sub['dest']['path']) diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 7b50f921..bed130ca 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py @@ -17,6 +17,7 @@ import yaml from oslo_config import cfg +from deckhand.engine.schema.v1_0 import document_schema from deckhand import factories from deckhand.tests import test_utils from deckhand.tests.unit.control import base as test_base @@ -555,19 +556,29 @@ class TestValidationsController(test_base.BaseControllerTest): self.assertEqual(expected_body, body) # Validate that both expected errors are present for validation. - expected_errors = [ - { - 'message': "'layeringDefinition' is a required property", - 'name': 'test_doc', - 'schema': 'example/foo/v1', - 'path': '.metadata' - }, { - 'message': "'fail' is not of type 'integer'", - 'name': 'test_doc', - 'schema': 'example/foo/v1', - 'path': '.data.a' - } - ] + expected_errors = [{ + 'error_section': { + 'data': {'a': 'fail'}, + 'metadata': {'labels': {'global': 'global1'}, + 'name': 'test_doc', + 'schema': 'metadata/Document/v1.0'}, + 'schema': 'example/foo/v1' + }, + 'name': 'test_doc', + 'path': '.metadata', + 'schema': 'example/foo/v1', + 'message': "'layeringDefinition' is a required property", + 'validation_schema': document_schema.schema, + 'schema_path': '.properties.metadata.required' + }, { + 'error_section': {'a': 'fail'}, + 'name': 'test_doc', + 'path': '.data.a', + 'schema': 'example/foo/v1', + 'message': "'fail' is not of type 'integer'", + 'validation_schema': schema_to_use, + 'schema_path': '.properties.a.type' + }] resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s/entries/0' % ( revision_id, types.DECKHAND_SCHEMA_VALIDATION), @@ -598,6 +609,15 @@ class TestValidationsController(test_base.BaseControllerTest): }, 'required': ['a'] } + expected_errors = [{ + 'error_section': {'a': 'fail'}, + 'name': 'test_doc', + 'path': '.data.a', + 'schema': 'example/foo/v1', + 'message': "'fail' is not of type 'integer'", + 'validation_schema': schema_to_use, + 'schema_path': '.properties.a.type' + }] data_schema = data_schema_factory.gen_test( metadata_name, data=schema_to_use) @@ -652,11 +672,15 @@ class TestValidationsController(test_base.BaseControllerTest): self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_errors = [{ - 'schema': 'example/foo/v1', + 'error_section': {'a': 'fail'}, 'name': 'test_doc', + 'path': '.data.a', + 'schema': 'example/foo/v1', 'message': "'fail' is not of type 'integer'", - 'path': '.data.a' + 'validation_schema': schema_to_use, + 'schema_path': '.properties.a.type' }] + self.assertIn('errors', body) self.assertEqual(expected_errors, body['errors']) @@ -704,10 +728,13 @@ class TestValidationsController(test_base.BaseControllerTest): self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_errors = [{ - 'schema': document['schema'], + 'error_section': document, 'name': document['metadata']['name'], + 'path': '.', + 'schema': document['schema'], 'message': "'data' is a required property", - 'path': '.' + 'validation_schema': document_schema.schema, + 'schema_path': '.required' }] self.assertIn('errors', body) self.assertEqual(expected_errors, body['errors']) diff --git a/deckhand/tests/unit/engine/test_document_validation.py b/deckhand/tests/unit/engine/test_document_validation.py index 53135061..29cf52c2 100644 --- a/deckhand/tests/unit/engine/test_document_validation.py +++ b/deckhand/tests/unit/engine/test_document_validation.py @@ -63,3 +63,28 @@ class TestDocumentValidation(engine_test_base.TestDocumentValidationBase): self.assertTrue(mock_log.info.called) self.assertIn("Skipping schema validation for abstract document", mock_log.info.mock_calls[0][1][0]) + + @mock.patch.object(document_validation, 'jsonschema', autospec=True) + def test_validation_failure_does_not_expose_secrets(self, mock_jsonschema): + m_args = mock.Mock() + mock_jsonschema.Draft4Validator(m_args).iter_errors.side_effect = [ + # Return empty list of errors for base schema validator and pretend + # that 1 error is returned for next validator. + [], [mock.Mock(path=[], schema_path=[])] + ] + test_document = self._read_data('sample_document') + for sub in test_document['metadata']['substitutions']: + substituted_data = utils.jsonpath_replace( + test_document['data'], 'scary-secret', sub['dest']['path']) + test_document['data'].update(substituted_data) + self.assertEqual( + 'scary-secret', utils.jsonpath_parse(test_document['data'], + sub['dest']['path'])) + + validations = document_validation.DocumentValidation( + test_document).validate_all() + + self.assertEqual(2, len(validations[0]['errors'])) + self.assertIn('Sanitized to avoid exposing secret.', + str(validations[0]['errors'][-1])) + self.assertNotIn('scary-secret.', str(validations[0]['errors'][-1])) diff --git a/deckhand/tests/unit/engine/test_document_validation_negative.py b/deckhand/tests/unit/engine/test_document_validation_negative.py index b335c26b..85bf59aa 100644 --- a/deckhand/tests/unit/engine/test_document_validation_negative.py +++ b/deckhand/tests/unit/engine/test_document_validation_negative.py @@ -44,6 +44,11 @@ class TestDocumentValidationNegative(test_base.TestDocumentValidationBase): self.assertEqual(types.DECKHAND_SCHEMA_VALIDATION, validations[-1]['name']) self.assertEqual(1, len(validations[-1]['errors'])) + + for key in ('name', 'schema', 'path', 'error_section', + 'validation_schema', 'schema_path', 'message'): + self.assertIn(key, validations[-1]['errors'][-1]) + self.assertEqual(expected['metadata']['name'], validations[-1]['errors'][-1]['name']) self.assertEqual(expected['schema'],