From 04ad3fa93b3fb5e46272e7fd252dedaecf38f77b Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 30 Mar 2018 19:14:33 +0100 Subject: [PATCH] Add validation for empty documents inside multi-document payload This is to add a helper function to base controller resource class for detecting whether a list of documents has any empty (None) or non-object entries in it (since all documents should be objects), resulting in a 400 Bad Request getting raised. This is to prevent the following stacktrace from occurring: File "/usr/local/lib/python3.5/dist-packages/falcon/api.py", line 244, in __call__ responder(req, resp, **params) File "./deckhand/policy.py", line 104, in handler return func(*args, **kwargs) File "./deckhand/control/buckets.py", line 58, in on_put documents, data_schemas, pre_validate=True) File "./deckhand/engine/document_validation.py", line 387, in __init__ raw_document[prop] = document.get(prop) AttributeError: 'NoneType' object has no attribute 'get' Change-Id: I76fc9b0d7662358f8b26b5bddf1187e92d1554de --- deckhand/control/base.py | 44 +++++++++++++++++++ deckhand/control/buckets.py | 14 +----- deckhand/control/revision_tags.py | 12 +---- deckhand/control/validations.py | 18 +------- .../unit/control/test_buckets_controller.py | 31 +++++++++++-- .../test_rendered_documents_controller.py | 4 +- .../test_revision_documents_controller.py | 4 +- .../test_revisions_rollback_controller.py | 4 +- 8 files changed, 83 insertions(+), 48 deletions(-) diff --git a/deckhand/control/base.py b/deckhand/control/base.py index 7cde446a..df26dbfe 100644 --- a/deckhand/control/base.py +++ b/deckhand/control/base.py @@ -12,10 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +import yaml + import falcon +from oslo_log import log as logging from deckhand import context +LOG = logging.getLogger(__name__) + class BaseResource(object): """Base resource class for implementing API resources.""" @@ -42,6 +47,45 @@ class BaseResource(object): resp.headers['Allow'] = ','.join(allowed_methods) resp.status = falcon.HTTP_200 + def from_yaml(self, req, expect_list=True, allow_empty=False): + """Reads and converts YAML-formatted request body into a dict or list + of dicts. + + :param req: Falcon Request object. + :param expect_list: Whether to expect a list or an object. + :param allow_empty: Whether the request body can be empty. + :returns: List of dicts if ``expect_list`` is True or else a dict. + """ + raw_data = req.stream.read(req.content_length or 0) + + if not allow_empty and not raw_data: + error_msg = ("The request body must not be empty.") + LOG.error(error_msg) + raise falcon.HTTPBadRequest(description=error_msg) + + try: + if expect_list: + data = list(yaml.safe_load_all(raw_data)) + else: + data = yaml.safe_load(raw_data) + except yaml.YAMLError as e: + error_msg = ("The request body must be properly formatted YAML. " + "Details: %s." % e) + LOG.error(error_msg) + raise falcon.HTTPBadRequest(description=error_msg) + + if expect_list: + bad_entries = [str(i + 1) for i, x in enumerate(data) + if not x or not isinstance(x, dict)] + if bad_entries: + error_msg = ( + "Expected a list of valid objects. Invalid entries " + "found at following indexes: %s." % ','.join(bad_entries)) + LOG.error(error_msg) + raise falcon.HTTPBadRequest(description=error_msg) + + return data + class DeckhandRequest(falcon.Request): context_type = context.RequestContext diff --git a/deckhand/control/buckets.py b/deckhand/control/buckets.py index 64ea0075..63d233c3 100644 --- a/deckhand/control/buckets.py +++ b/deckhand/control/buckets.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import yaml - import falcon from oslo_log import log as logging import six @@ -34,18 +32,10 @@ class BucketsResource(api_base.BaseResource): """API resource for realizing CRUD operations for buckets.""" view_builder = document_view.ViewBuilder() - secrets_mgr = secrets_manager.SecretsManager() @policy.authorize('deckhand:create_cleartext_documents') def on_put(self, req, resp, bucket_name=None): - document_data = req.stream.read(req.content_length or 0) - try: - documents = list(yaml.safe_load_all(document_data)) - except yaml.YAMLError as e: - error_msg = ("Could not parse the document into YAML data. " - "Details: %s." % e) - LOG.error(error_msg) - raise falcon.HTTPBadRequest(description=six.text_type(e)) + documents = self.from_yaml(req, expect_list=True, allow_empty=True) # NOTE: Must validate documents before doing policy enforcement, # because we expect certain formatting of the documents while doing @@ -86,7 +76,7 @@ class BucketsResource(api_base.BaseResource): for document in secret_documents: # TODO(fmontei): Move all of this to document validation directly. if document['metadata'].get('storagePolicy') == 'encrypted': - secret_data = self.secrets_mgr.create(document) + secret_data = secrets_manager.SecretsManager.create(document) document['data'] = secret_data def _create_revision_documents(self, bucket_name, documents, diff --git a/deckhand/control/revision_tags.py b/deckhand/control/revision_tags.py index ea6e54ff..8b5c47db 100644 --- a/deckhand/control/revision_tags.py +++ b/deckhand/control/revision_tags.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import yaml - import falcon from oslo_log import log as logging @@ -32,15 +30,7 @@ class RevisionTagsResource(api_base.BaseResource): @policy.authorize('deckhand:create_tag') def on_post(self, req, resp, revision_id, tag=None): """Creates a revision tag.""" - body = req.stream.read(req.content_length or 0) - - try: - tag_data = yaml.safe_load(body) - except yaml.YAMLError as e: - error_msg = ("Could not parse the request body into YAML data. " - "Details: %s." % e) - LOG.error(error_msg) - raise falcon.HTTPBadRequest(description=e) + tag_data = self.from_yaml(req, expect_list=False, allow_empty=True) try: resp_tag = db_api.revision_tag_create(revision_id, tag, tag_data) diff --git a/deckhand/control/validations.py b/deckhand/control/validations.py index 34bed512..1fa234d6 100644 --- a/deckhand/control/validations.py +++ b/deckhand/control/validations.py @@ -12,11 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -import yaml - import falcon from oslo_log import log as logging -import six from deckhand.control import base as api_base from deckhand.control.views import validation as validation_view @@ -34,19 +31,8 @@ class ValidationsResource(api_base.BaseResource): @policy.authorize('deckhand:create_validation') def on_post(self, req, resp, revision_id, validation_name): - validation_data = req.stream.read(req.content_length or 0) - try: - validation_data = yaml.safe_load(validation_data) - except yaml.YAMLError as e: - error_msg = ("Could not parse the validation into YAML data. " - "Details: %s." % e) - LOG.error(error_msg) - raise falcon.HTTPBadRequest(description=six.text_type(e)) - - if not validation_data: - error_msg = 'Validation payload must be provided.' - LOG.error(error_msg) - raise falcon.HTTPBadRequest(description=error_msg) + validation_data = self.from_yaml( + req, expect_list=False, allow_empty=False) if not all([validation_data.get(x) for x in ('status', 'validator')]): error_msg = 'Validation payload must contain keys: %s.' % ( diff --git a/deckhand/tests/unit/control/test_buckets_controller.py b/deckhand/tests/unit/control/test_buckets_controller.py index 336f7e49..27a234b4 100644 --- a/deckhand/tests/unit/control/test_buckets_controller.py +++ b/deckhand/tests/unit/control/test_buckets_controller.py @@ -17,7 +17,7 @@ import yaml import mock from oslo_config import cfg -from deckhand.control import buckets +from deckhand.engine import secrets_manager from deckhand import factories from deckhand.tests import test_utils from deckhand.tests.unit.control import base as test_base @@ -100,7 +100,7 @@ class TestBucketsController(test_base.BaseControllerTest): secrets_factory = factories.DocumentSecretFactory() payload = [secrets_factory.gen_test('Certificate', 'encrypted')] - with mock.patch.object(buckets.BucketsResource, 'secrets_mgr', + with mock.patch.object(secrets_manager, 'SecretsManager', autospec=True) as mock_secrets_mgr: mock_secrets_mgr.create.return_value = payload[0]['data'] _do_test(payload) @@ -115,7 +115,7 @@ class TestBucketsController(test_base.BaseControllerTest): data_schema_factory = factories.DataSchemaFactory() data_schema = data_schema_factory.gen_test(document['schema'], {}) - with mock.patch.object(buckets.BucketsResource, 'secrets_mgr', + with mock.patch.object(secrets_manager, 'SecretsManager', autospec=True) as mock_secrets_mgr: mock_secrets_mgr.create.return_value = document['data'] _do_test([document, data_schema]) @@ -217,6 +217,31 @@ class TestBucketsControllerNegative(test_base.BaseControllerTest): body=yaml.safe_dump_all([payload])) self.assertEqual(409, resp.status_code) + def test_bucket_with_empty_document_raises_bad_request(self): + rules = {'deckhand:create_cleartext_documents': '@'} + self.policy.set_rules(rules) + + # Verify that 400 is returned for empty entries. + bucket_name = test_utils.rand_name('bucket') + resp = self.app.simulate_put( + '/api/v1.0/buckets/%s/documents' % bucket_name, + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all([None, '', {}])) + self.assertEqual(400, resp.status_code) + self.assertRegex( + resp.text, + r'.*Invalid entries found at following indexes:\n.*1,2,3.') + + # Verify that 400 is returned for non-object entries. + resp = self.app.simulate_put( + '/api/v1.0/buckets/%s/documents' % bucket_name, + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all([{'foo': 'bar'}, 'foo', 5])) + self.assertEqual(400, resp.status_code) + self.assertRegex( + resp.text, + r'.*Invalid entries found at following indexes:\n.*2,3.') + class TestBucketsControllerNegativeRBAC(test_base.BaseControllerTest): """Test suite for validating negative RBAC scenarios for bucket diff --git a/deckhand/tests/unit/control/test_rendered_documents_controller.py b/deckhand/tests/unit/control/test_rendered_documents_controller.py index 29581d34..13211e9d 100644 --- a/deckhand/tests/unit/control/test_rendered_documents_controller.py +++ b/deckhand/tests/unit/control/test_rendered_documents_controller.py @@ -16,8 +16,8 @@ import yaml import mock -from deckhand.control import buckets from deckhand.control import revision_documents +from deckhand.engine import secrets_manager from deckhand import errors from deckhand import factories from deckhand.tests.unit.control import base as test_base @@ -276,7 +276,7 @@ class TestRenderedDocumentsControllerNegativeRBAC( 'encrypted') payload = [layering_policy, encrypted_document] - with mock.patch.object(buckets.BucketsResource, 'secrets_mgr', + with mock.patch.object(secrets_manager, 'SecretsManager', autospec=True) as mock_secrets_mgr: mock_secrets_mgr.create.return_value = payload[0]['data'] resp = self.app.simulate_put( diff --git a/deckhand/tests/unit/control/test_revision_documents_controller.py b/deckhand/tests/unit/control/test_revision_documents_controller.py index 9302b7ac..e52a89ea 100644 --- a/deckhand/tests/unit/control/test_revision_documents_controller.py +++ b/deckhand/tests/unit/control/test_revision_documents_controller.py @@ -16,7 +16,7 @@ import yaml import mock -from deckhand.control import buckets +from deckhand.engine import secrets_manager from deckhand import factories from deckhand.tests.unit.control import base as test_base @@ -62,7 +62,7 @@ class TestRevisionDocumentsControllerNegativeRBAC( # Create a document for a bucket. secrets_factory = factories.DocumentSecretFactory() payload = [secrets_factory.gen_test('Certificate', 'encrypted')] - with mock.patch.object(buckets.BucketsResource, 'secrets_mgr', + with mock.patch.object(secrets_manager, 'SecretsManager', autospec=True) as mock_secrets_mgr: mock_secrets_mgr.create.return_value = payload[0]['data'] resp = self.app.simulate_put( diff --git a/deckhand/tests/unit/control/test_revisions_rollback_controller.py b/deckhand/tests/unit/control/test_revisions_rollback_controller.py index 257f700e..d317d4da 100644 --- a/deckhand/tests/unit/control/test_revisions_rollback_controller.py +++ b/deckhand/tests/unit/control/test_revisions_rollback_controller.py @@ -16,7 +16,7 @@ import yaml import mock -from deckhand.control import buckets +from deckhand.engine import secrets_manager from deckhand import factories from deckhand.tests.unit.control import base as test_base @@ -59,7 +59,7 @@ class TestRevisionsRollbackControllerNegativeRBAC( secrets_factory = factories.DocumentSecretFactory() payload = [secrets_factory.gen_test('Certificate', 'encrypted')] - with mock.patch.object(buckets.BucketsResource, 'secrets_mgr', + with mock.patch.object(secrets_manager, 'SecretsManager', autospec=True) as mock_secrets_mgr: mock_secrets_mgr.create.return_value = payload[0]['data'] resp = self.app.simulate_put(