From f6a2b67eaf2f883fb2910dd3d48923786d7cd9e0 Mon Sep 17 00:00:00 2001 From: Fei Long Wang Date: Wed, 16 Nov 2016 14:33:58 +1300 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. Co-Authored-By: Mike Fedosin () Co-Authored-By: FeiLong Wang (flwang@catalyst.net.nz) Closes-Bug: #1546507 Change-Id: I5327c810edea426da0e7ff623e3179deef78124e --- etc/policy.json | 1 + glance/api/v1/images.py | 8 ++ glance/api/v2/images.py | 12 ++- glance/common/utils.py | 31 ++++++++ glance/db/simple/api.py | 31 ++++++++ glance/db/sqlalchemy/api.py | 25 ++++++ glance/location.py | 21 ++++- 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 ++++++ 12 files changed, 276 insertions(+), 6 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..4129138 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[2] == image_id: + return True + return False diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index fd3d5ee..41f6e7a 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -619,6 +619,37 @@ def image_location_delete(context, image_id, location_id, status, raise exception.NotFound(msg) +def image_locations_with_url(context, location): + locations = [] + try: + short_rbd_locations = [] + if location["url"].startswith("rbd://"): + pieces = location['url'][len("rbd://"):].split('/') + # NOTE(flwang): We can deduct the short rbd location based on the + # long one, but it's impossible to do it in reverse since there's + # no way to know the FSID. + if len(pieces) == 4: + for loc in DATA['locations']: + if loc['url'] == "rbd://" + pieces[2]: + short_rbd_locations.append(loc) + + for loc in DATA['locations']: + if loc['url'] == location['url']: + locations.append(loc) + keyfn = lambda x: x['created_at'] or '' + + if short_rbd_locations: + locations.extend(short_rbd_locations) + + locations.sort(key=keyfn) + return locations + except exception.NotFound: + msg = (_("No location found with url %(loc)s") % + dict(loc=location['url'])) + LOG.warn(msg) + return locations + + def _image_locations_set(context, image_id, locations): # NOTE(zhiyan): 1. Remove records from DB for deleted locations used_loc_ids = [loc['id'] for loc in locations if loc.get('id')] diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index 77ab684..9d548c1 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -938,6 +938,31 @@ def image_location_delete(context, image_id, location_id, status, raise exception.NotFound(msg) +def image_locations_with_url(context, location, session=None): + try: + session = session or get_session() + short_rbd_locations = [] + if location["url"].startswith("rbd://"): + pieces = location['url'][len("rbd://"):].split('/') + # NOTE(flwang): We can deduct the short rbd location based on the + # long one, but it's impossible to do it in reverse since there's + # no way to know the FSID. + if len(pieces) == 4: + short_rbd_locations = session.query(models.ImageLocation).filter_by( + value="rbd://" + pieces[2]).order_by(models.ImageLocation.created_at).all() + + locations = session.query(models.ImageLocation).filter_by( + value=location['url']).order_by(models.ImageLocation.created_at).all() + if short_rbd_locations: + locations.extend(short_rbd_locations) + return locations + except sa_orm.exc.NoResultFound: + msg = (_("No location found with url %(loc)s") % + dict(loc=location['url'])) + LOG.warn(msg) + return [] + + def _image_locations_set(context, image_id, locations, session=None): # NOTE(zhiyan): 1. Remove records from DB for deleted locations session = session or get_session() diff --git a/glance/location.py b/glance/location.py index f7f2250..273cbfb 100644 --- a/glance/location.py +++ b/glance/location.py @@ -400,10 +400,23 @@ class ImageProxy(glance.domain.proxy.Image): self.image.delete() if self.image.locations: for location in self.image.locations: - self.store_utils.delete_image_location_from_backend( - self.context, - self.image.image_id, - location) + locs = self.image.db_api.image_locations_with_url(self.context, + location) + # NOTE(flwang): 1. If current image is the "owner" of this + # location, the location can be deleted, no matter if there + # is another image using it. 2. If current image is not the + # "owner" of this location, the location's back end data can't + # be deleted. + # The locations list returned by function + # "image_locations_with_url" are sorted by the creation time + # of the location, because technically, the location owner is + # the image who created the location at the first time. + if (len(locs) > 0 and + locs[0].get('image_id') == self.image.image_id): + self.store_utils.delete_image_location_from_backend( + self.context, + self.image.image_id, + location) def set_data(self, data, size=None): if size is None: 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': {}}}] -- 1.9.1