From 140f8e6403bc4975b3c02ae22279c04f33eab29f Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Wed, 11 May 2016 18:45:54 +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. Added new policy 'set_unrestricted_location' that allows to specify any location for selected users. Closes-Bug: #1546507 Change-Id: I8d2aea50dbd26a5a01a50c2bd9fe3d92f6e41026 --- etc/policy.json | 1 + glance/api/policy.py | 9 +++++ glance/api/v1/images.py | 8 ++++ glance/common/utils.py | 14 +++++++ 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 | 56 ++++++++++++++++++++++++++++ glance/tests/unit/v2/test_images_resource.py | 9 +++++ 9 files changed, 112 insertions(+) diff --git a/etc/policy.json b/etc/policy.json index f49bc08..e02ab2b 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/policy.py b/glance/api/policy.py index 96f99c7..ebba3b3 100644 --- a/glance/api/policy.py +++ b/glance/api/policy.py @@ -23,6 +23,7 @@ from oslo_log import log as logging from oslo_policy import policy from glance.common import exception +from glance.common import utils import glance.domain.proxy from glance.i18n import _ @@ -163,6 +164,14 @@ class ImageProxy(glance.domain.proxy.Image): set([loc['url'] for loc in new_locations])): self.policy.enforce(self.context, 'delete_image_location', self.target) + for location in new_locations: + if not (self.policy.check( + self.context, 'set_unrestricted_location', {}) + or utils.validate_external_url( + location['url'], self.image_id)): + msg = (_("Setting location %s that refers to another image" + " is forbidden") % location['url']) + raise exception.Forbidden(message=msg) self.image.locations = new_locations def delete(self): diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py index 4b60c87..f70035c 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/common/utils.py b/glance/common/utils.py index 84c50eb..7b2a0b8 100644 --- a/glance/common/utils.py +++ b/glance/common/utils.py @@ -686,3 +686,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/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 721e090..3a5464f 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -2863,6 +2863,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 e1b252a..eab4501 100644 --- a/glance/tests/unit/utils.py +++ b/glance/tests/unit/utils.py @@ -246,6 +246,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 f4d6bd3..d6d29c9 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(403, 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(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 3cfc4dd..91a7935 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1571,6 +1571,15 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(2, len(output.locations)) self.assertEqual(new_location, output.locations[1]) + def test_update_add_invalid_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.HTTPBadRequest, + 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': {}} -- 1.9.1