From c99aa716c580e7b6300ad1a165af929dea533d52 Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Thu, 17 Nov 2016 14:45:11 +0300 Subject: [PATCH] Prevent setting locations to other images Currently user may specify a custom location for his image, that links to another image file. After that if user deletes his image glance will delete another image data too. 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. For rbd locations there is an exception: all direct rbd urls "rbd://image_id" are forbidden. Snap-styled rbd urls "rbd://fsid/pool/image/snapshot" comply with the general rules. Added new policy 'set_restricted_location' that allows to specify any location for selected users. Change-Id: I5327c810edea426da0e7ff623e3179deef78124e Closes-Bug: #1546507 --- etc/policy.json | 1 + glance/api/v1/images.py | 8 ++ glance/api/v2/images.py | 12 ++- glance/common/utils.py | 31 ++++++++ glance/tests/etc/policy.json | 1 + glance/tests/functional/v2/test_images.py | 10 +++ glance/tests/unit/utils.py | 4 + glance/tests/unit/v1/test_api.py | 115 +++++++++++++++++++++++++++ glance/tests/unit/v2/test_images_resource.py | 23 ++++++ 9 files changed, 203 insertions(+), 2 deletions(-) diff --git a/etc/policy.json b/etc/policy.json index 0a058c1..d010a1d 100644 --- a/etc/policy.json +++ b/etc/policy.json @@ -8,6 +8,7 @@ "get_images": "", "modify_image": "", "publicize_image": "role:admin", + "set_unrestricted_location": "role:admin", "copy_from": "", "download_image": "", diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py index 45628d9..ab0bdfc 100644 --- a/glance/api/v1/images.py +++ b/glance/api/v1/images.py @@ -818,6 +818,14 @@ class Controller(controller.BaseController): else: if location: self._validate_image_for_activation(req, image_id, image_meta) + if not (self.policy.check( + req.context, 'set_unrestricted_location', {}) + or utils.validate_external_url(location, image_id)): + msg = (_("Setting location %s that refers to another image" + " is forbidden") % location) + upload_utils.safe_kill(req, image_meta['id'], 'queued') + raise HTTPForbidden(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 cd52476..b65f80f 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -198,7 +198,7 @@ class ImagesController(object): value = change['value'] json_schema_version = change.get('json_schema_version', 10) if path_root == 'locations': - self._do_add_locations(image, path[1], value) + self._do_add_locations(req, image, path[1], value) else: if ((hasattr(image, path_root) or path_root in image.extra_properties) @@ -287,12 +287,20 @@ class ImagesController(object): raise webob.exc.HTTPBadRequest( explanation=encodeutils.exception_to_unicode(ve)) - def _do_add_locations(self, image, path_pos, value): + def _do_add_locations(self, req, image, path_pos, value): if CONF.show_multiple_locations == False: msg = _("It's not allowed to add locations if locations are " "invisible.") raise webob.exc.HTTPForbidden(explanation=msg) + if not (self.policy.check( + req.context, 'set_unrestricted_location', {}) + or utils.validate_external_url( + value['url'], image.image_id)): + msg = (_("Setting location %s that refers to another image" + " is forbidden") % value['url']) + raise exception.Forbidden(message=msg) + if image.status not in ('active', 'queued'): msg = _("It's not allowed to add locations if image status is " "%s.") % image.status diff --git a/glance/common/utils.py b/glance/common/utils.py index 4e06979..735269b 100644 --- a/glance/common/utils.py +++ b/glance/common/utils.py @@ -672,3 +672,34 @@ 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 + + # All http(s) locations are good + if url.startswith('http'): + return True + + # For all non-rbd locations we just need to check that link contains + # image id + prefix = 'rbd://' + if not url.startswith(prefix) and image_id in url: + return True + + # There are 2 types of 'rbd' urls: + # 'rbd://image' and 'rbd://fsid/pool/image/snapshot' + # First one is forbidden, second is okay + pieces = url[len(prefix):].split('/') + if len(pieces) == 4 and pieces[3] == image_id: + return True + return False diff --git a/glance/tests/etc/policy.json b/glance/tests/etc/policy.json index 41665e9..e0d6372 100644 --- a/glance/tests/etc/policy.json +++ b/glance/tests/etc/policy.json @@ -9,6 +9,7 @@ "get_images": "", "modify_image": "", "publicize_image": "", + "set_unrestricted_location": "role:admin", "copy_from": "", "download_image": "", diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py index cec1a7f..44071c7 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -2866,6 +2866,16 @@ class TestImages(functional.FunctionalTest): self.assertEqual(10, image['size']) def test_update_locations_with_restricted_sources(self): + rules = {'context_is_admin': 'role:admin', + 'default': '', + 'add_image': '', + 'get_image': '', + 'modify_image': '', + 'upload_image': '', + 'delete_image': '', + 'set_unrestricted_location': '', + 'download_image': '!'} + self.set_policy_rules(rules) self.api_server.show_multiple_locations = True self.start_servers(**self.__dict__.copy()) # Create an image diff --git a/glance/tests/unit/utils.py b/glance/tests/unit/utils.py index 83636a0..cc27118 100644 --- a/glance/tests/unit/utils.py +++ b/glance/tests/unit/utils.py @@ -234,6 +234,10 @@ class FakePolicyEnforcer(object): if self.rules.get(action) is False: raise exception.Forbidden() + def check(self, _ctxt, action, target=None, **kwargs): + """Check if a rule for given action is set to false.""" + return self.rules.get(action) + def set_rules(self, rules): self.rules = rules diff --git a/glance/tests/unit/v1/test_api.py b/glance/tests/unit/v1/test_api.py index a51ae86..81b826f 100644 --- a/glance/tests/unit/v1/test_api.py +++ b/glance/tests/unit/v1/test_api.py @@ -524,6 +524,49 @@ class TestGlanceAPI(base.IsolatedUnitTest): res = req.get_response(self.api) self.assertEqual(http_client.BAD_REQUEST, 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_direct_rbd_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(403, 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_snap_styled_rbd_location( + self, mock_reserve, mock_validate): + mock_reserve.return_value = { + 'id': 'this_image_id', + 'location': 'rbd://cluster_id/pool/image/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://cluster_id/pool/' + 'image/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(403, res.status_int) + def test_create_with_empty_copy_from(self): fixture_headers = { 'x-glance-api-copy-from': '', @@ -1282,6 +1325,78 @@ class TestGlanceAPI(base.IsolatedUnitTest): res = req.get_response(self.api) self.assertEqual(http_client.BAD_REQUEST, 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_update_add_direct_rbd_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(403, 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_update_add_snap_styled_rbd_location( + self, mock_update_meta, mock_get_size, mock_update_store_acls, + mock_validate): + mock_get_size.return_value = 1000 + 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) + res_body = jsonutils.loads(res.body)['image'] + self.assertEqual('queued', res_body['status']) + image_id = res_body['id'] + location = 'rbd://%s/pool/image/another_image_id' % image_id + mock_update_meta.return_value = {'id': 'this_image_id', + 'location': location} + + with mock.patch.object(glance.api.v1.images.Controller, + '_external_source', + return_value=location): + 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(403, 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 8ee1d1f..a92b7dd 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1654,6 +1654,25 @@ class TestImagesController(base.IsolatedUnitTest): self.assertRaises(webob.exc.HTTPConflict, self.controller.update, request, '1', changes) + def test_update_add_direct_rbd_location(self): + self.config(show_multiple_locations=True) + 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.HTTPForbidden, + self.controller.update, request, UUID1, changes) + + def test_update_add_snap_styled_rbd_location(self): + self.config(show_multiple_locations=True) + new_location = {'url': 'rbd://%s/pool/image/another_image_id' % UUID1, + 'metadata': {}} + request = unit_test_utils.get_fake_request() + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + self.assertRaises(webob.exc.HTTPForbidden, + self.controller.update, request, UUID1, changes) + def test_update_add_locations_insertion(self): self.config(show_multiple_locations=True) new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} @@ -1667,6 +1686,8 @@ class TestImagesController(base.IsolatedUnitTest): def test_update_add_locations_list(self): self.config(show_multiple_locations=True) + rules = {"set_unrestricted_location": True} + self.policy.set_rules(rules) request = unit_test_utils.get_fake_request() changes = [{'op': 'add', 'path': ['locations', '-'], 'value': {'url': 'foo', 'metadata': {}}}] @@ -1675,6 +1696,8 @@ class TestImagesController(base.IsolatedUnitTest): def test_update_add_locations_invalid(self): self.config(show_multiple_locations=True) + rules = {"set_unrestricted_location": True} + self.policy.set_rules(rules) request = unit_test_utils.get_fake_request() changes = [{'op': 'add', 'path': ['locations', '-'], 'value': {'url': 'unknow://foo', 'metadata': {}}}] -- 2.7.4