From ca15b7afb9bab5d27e1f5e5fc2ed136ad03d83c7 Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Thu, 18 Feb 2016 04:45:58 +0300 Subject: [PATCH] Prevent setting locations to external images Currently user may specify a custom location for his image, that links to external image. After that if user deletes his image glance will delete external image data. To prevent this behavior from now it's only allowed to specify locations, that contain current image id or external http stores, where data can't be deleted. Closes-Bug: #1546507 Change-Id: I8d2aea50dbd26a5a01a50c2bd9fe3d92f6e41026 --- glance/api/v1/images.py | 5 +++ glance/api/v2/images.py | 4 ++ glance/common/utils.py | 14 +++++++ glance/tests/unit/v1/test_api.py | 56 ++++++++++++++++++++++++++++ glance/tests/unit/v2/test_images_resource.py | 8 ++++ 5 files changed, 87 insertions(+) diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py index f924763..7671fef 100644 --- a/glance/api/v1/images.py +++ b/glance/api/v1/images.py @@ -810,6 +810,11 @@ class Controller(controller.BaseController): else: if location: self._validate_image_for_activation(req, image_id, image_meta) + if not utils.validate_external_url(location, image_id): + msg = (_("Setting location %s to external image is" + " forbidden") % location) + raise HTTPBadRequest(explanation=msg, request=req, + content_type="text/plain") image_size_meta = image_meta.get('size') if image_size_meta: try: diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index cdcabaa..39c7bd1 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -288,6 +288,10 @@ class ImagesController(object): explanation=encodeutils.exception_to_unicode(ve)) def _do_add_locations(self, image, path_pos, value): + if not utils.validate_external_url(value['url'], image.image_id): + msg = (_("Setting location %s to external image is" + " forbidden") % value) + raise webob.exc.HTTPBadRequest(explanation=msg) pos = self._get_locations_op_pos(path_pos, len(image.locations), True) if pos is None: diff --git a/glance/common/utils.py b/glance/common/utils.py index 2b8b6f1..9a64107 100644 --- a/glance/common/utils.py +++ b/glance/common/utils.py @@ -635,3 +635,17 @@ def evaluate_filter_op(value, operator, threshold): msg = _("Unable to filter on a unknown operator.") raise exception.InvalidFilterOperatorValue(msg) + + +def validate_external_url(url, image_id): + """ + Check that given url can be assigned to an image. + + :param url: link that need to be validated + :param image_id: ID of image we want to assign the url + :return: True if url can be assigned to image + """ + # NOTE(mfedosin): this function checks, that given url is not a link + # to another active image data. + # See bug #1546507 + return url.startswith('http') or image_id in url diff --git a/glance/tests/unit/v1/test_api.py b/glance/tests/unit/v1/test_api.py index f4d6bd3..bfafd28 100644 --- a/glance/tests/unit/v1/test_api.py +++ b/glance/tests/unit/v1/test_api.py @@ -523,6 +523,26 @@ class TestGlanceAPI(base.IsolatedUnitTest): res = req.get_response(self.api) self.assertEqual(400, res.status_int) + @mock.patch.object(glance.api.v1.images.Controller, + '_validate_image_for_activation') + @mock.patch.object(glance.api.v1.images.Controller, '_reserve') + def test_create_with_invalid_location(self, mock_reserve, mock_validate): + mock_reserve.return_value = {'id': 'this_image_id', + 'location': 'rbd://another_image_id'} + fixture_headers = { + 'x-image-meta-name': 'bogus', + 'x-image-meta-disk-format': 'qcow2', + 'x-image-meta-container-format': 'bare', + 'x-image-meta-location': 'rbd://another_image_id', + } + req = webob.Request.blank("/images") + req.method = 'POST' + for k, v in six.iteritems(fixture_headers): + req.headers[k] = v + + res = req.get_response(self.api) + self.assertEqual(400, res.status_int) + def test_create_with_empty_copy_from(self): fixture_headers = { 'x-glance-api-copy-from': '', @@ -1280,6 +1300,42 @@ class TestGlanceAPI(base.IsolatedUnitTest): res = req.get_response(self.api) self.assertEqual(400, res.status_int) + @mock.patch.object(glance.api.v1.images.Controller, + '_validate_image_for_activation') + @mock.patch.object(glance.api.v1.images.Controller, 'update_store_acls') + @mock.patch.object(glance.api.v1.images.Controller, '_get_size') + @mock.patch.object(registry, 'update_image_metadata') + def test_add_invalid_location( + self, mock_update_meta, mock_get_size, mock_update_store_acls, + mock_validate): + mock_get_size.return_value = 1000 + location = 'rbd://another_image_id' + mock_update_meta.return_value = {'id': 'this_image_id', + 'location': location} + fixture_headers = { + 'x-image-meta-name': 'bogus', + 'x-image-meta-disk-format': 'qcow2', + 'x-image-meta-container-format': 'bare', + } + req = webob.Request.blank("/images") + req.method = 'POST' + for k, v in six.iteritems(fixture_headers): + req.headers[k] = v + res = req.get_response(self.api) + self.assertEqual(201, res.status_int) + + with mock.patch.object(glance.api.v1.images.Controller, + '_external_source', + return_value=location): + res_body = jsonutils.loads(res.body)['image'] + self.assertEqual('queued', res_body['status']) + image_id = res_body['id'] + req = webob.Request.blank("/images/%s" % image_id) + req.method = 'PUT' + req.headers['x-image-meta-location'] = location + res = req.get_response(self.api) + self.assertEqual(400, res.status_int) + def test_create_image_with_nonexistent_location_url(self): # Ensure HTTP 404 response returned when try to create # image with non-existent http location URL. diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index dad24bc..c0d50e3 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1549,6 +1549,14 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(2, len(output.locations)) self.assertEqual(new_location, output.locations[1]) + def test_update_add_invalid_location(self): + new_location = {'url': 'rbd://another_image_id', 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.update, request, UUID1, changes) + def test_update_add_locations_insertion(self): new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} request = unit_test_utils.get_fake_request() -- 1.9.1