From 4d08db5b6d42323ac1958ef3b7417d875e7bea8c Mon Sep 17 00:00:00 2001 From: Stuart McLaren Date: Tue, 11 Aug 2015 10:37:09 +0000 Subject: [PATCH] Prevent image status being directly modified via v1 Users shouldn't be able to change an image's status directly via the v1 API. Some existing consumers of Glance set the x-image-meta-status header in requests to the Glance API, eg: https://github.com/openstack/nova/blob/master/plugins/xenserver/xenapi/etc/xapi.d/plugins/glance#L184 We should try to prevent users setting 'status' via v1, but without breaking existing benign API calls such as these. I've adopted the following approach (which has some prior art in 'protected properties'). If a PUT request is received which contains an x-image-meta-status header: * The user provided status is ignored if it matches the current image status (this prevents benign calls such as the nova one above from breaking). The usual code (eg 200) will be returned. * If the user provided status doesn't match the current image status (ie there is a real attempt to change the value) 403 will be returned. This will break any calls which currently intentionally change the status. APIImpact Closes-bug: 1482371 Change-Id: I44fadf32abb57c962b67467091c3f51c1ccc25e6 --- glance/api/v1/__init__.py | 3 + glance/api/v1/images.py | 9 +++ glance/tests/functional/v1/test_api.py | 89 ++++++++++++++++++++++ .../integration/legacy_functional/test_v1_api.py | 2 + 4 files changed, 103 insertions(+) diff --git a/glance/api/v1/__init__.py b/glance/api/v1/__init__.py index 74de9aa1411d8e926770b67f7d851cf14e794414..9306bbb4fe78f77a26bb539c717fdfd2b38767c8 100644 --- a/glance/api/v1/__init__.py +++ b/glance/api/v1/__init__.py @@ -21,3 +21,6 @@ SUPPORTED_PARAMS = ('limit', 'marker', 'sort_key', 'sort_dir') # Metadata which only an admin can change once the image is active ACTIVE_IMMUTABLE = ('size', 'checksum') + +# Metadata which cannot be changed (irrespective of the current image state) +IMMUTABLE = ('status',) diff --git a/glance/api/v1/images.py b/glance/api/v1/images.py index 7de66a97d29666698ba339bed0221191acc998de..d7732c0c8f3991867eefd44cb08eb09a4116292b 100644 --- a/glance/api/v1/images.py +++ b/glance/api/v1/images.py @@ -58,6 +58,7 @@ _LW = i18n._LW SUPPORTED_PARAMS = glance.api.v1.SUPPORTED_PARAMS SUPPORTED_FILTERS = glance.api.v1.SUPPORTED_FILTERS ACTIVE_IMMUTABLE = glance.api.v1.ACTIVE_IMMUTABLE +IMMUTABLE = glance.api.v1.IMMUTABLE CONF = cfg.CONF CONF.import_opt('disk_formats', 'glance.common.config', group='image_format') @@ -939,6 +940,14 @@ class Controller(controller.BaseController): request=req, content_type="text/plain") + for key in IMMUTABLE: + if (image_meta.get(key) is not None and + image_meta.get(key) != orig_image_meta.get(key)): + msg = _("Forbidden to modify '%s' of image.") % key + raise HTTPForbidden(explanation=msg, + request=req, + content_type="text/plain") + # The default behaviour for a PUT /images/ is to # override any properties that were previously set. This, however, # leads to a number of issues for the common use case where a caller diff --git a/glance/tests/functional/v1/test_api.py b/glance/tests/functional/v1/test_api.py index 7e076719ac2fc2289bfecdb696432acb8cb4987a..941a5868bb962715ee3e3bbb8f67a4eb049e736e 100644 --- a/glance/tests/functional/v1/test_api.py +++ b/glance/tests/functional/v1/test_api.py @@ -761,3 +761,92 @@ class TestApi(functional.FunctionalTest): self.assertEqual(404, response.status) self.stop_servers() + + def test_status_cannot_be_manipulated_directly(self): + self.cleanup() + self.start_servers(**self.__dict__.copy()) + headers = minimal_headers('Image1') + + # Create a 'queued' image + http = httplib2.Http() + headers = {'Content-Type': 'application/octet-stream', + 'X-Image-Meta-Disk-Format': 'raw', + 'X-Image-Meta-Container-Format': 'bare'} + path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port) + response, content = http.request(path, 'POST', headers=headers, + body=None) + self.assertEqual(201, response.status) + image = jsonutils.loads(content)['image'] + self.assertEqual('queued', image['status']) + + # Ensure status of 'queued' image can't be changed + path = "http://%s:%d/v1/images/%s" % ("127.0.0.1", self.api_port, + image['id']) + http = httplib2.Http() + headers = {'X-Image-Meta-Status': 'active'} + response, content = http.request(path, 'PUT', headers=headers) + self.assertEqual(403, response.status) + response, content = http.request(path, 'HEAD') + self.assertEqual(200, response.status) + self.assertEqual('queued', response['x-image-meta-status']) + + # We allow 'setting' to the same status + http = httplib2.Http() + headers = {'X-Image-Meta-Status': 'queued'} + response, content = http.request(path, 'PUT', headers=headers) + self.assertEqual(200, response.status) + response, content = http.request(path, 'HEAD') + self.assertEqual(200, response.status) + self.assertEqual('queued', response['x-image-meta-status']) + + # Make image active + http = httplib2.Http() + headers = {'Content-Type': 'application/octet-stream'} + response, content = http.request(path, 'PUT', headers=headers, + body='data') + self.assertEqual(200, response.status) + image = jsonutils.loads(content)['image'] + self.assertEqual('active', image['status']) + + # Ensure status of 'active' image can't be changed + http = httplib2.Http() + headers = {'X-Image-Meta-Status': 'queued'} + response, content = http.request(path, 'PUT', headers=headers) + self.assertEqual(403, response.status) + response, content = http.request(path, 'HEAD') + self.assertEqual(200, response.status) + self.assertEqual('active', response['x-image-meta-status']) + + # We allow 'setting' to the same status + http = httplib2.Http() + headers = {'X-Image-Meta-Status': 'active'} + response, content = http.request(path, 'PUT', headers=headers) + self.assertEqual(200, response.status) + response, content = http.request(path, 'HEAD') + self.assertEqual(200, response.status) + self.assertEqual('active', response['x-image-meta-status']) + + # Create a 'queued' image, ensure 'status' header is ignored + http = httplib2.Http() + path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port) + headers = {'Content-Type': 'application/octet-stream', + 'X-Image-Meta-Status': 'active'} + response, content = http.request(path, 'POST', headers=headers, + body=None) + self.assertEqual(201, response.status) + image = jsonutils.loads(content)['image'] + self.assertEqual('queued', image['status']) + + # Create an 'active' image, ensure 'status' header is ignored + http = httplib2.Http() + path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port) + headers = {'Content-Type': 'application/octet-stream', + 'X-Image-Meta-Disk-Format': 'raw', + 'X-Image-Meta-Status': 'queued', + 'X-Image-Meta-Container-Format': 'bare'} + response, content = http.request(path, 'POST', headers=headers, + body='data') + self.assertEqual(201, response.status) + image = jsonutils.loads(content)['image'] + self.assertEqual('active', image['status']) + self.stop_servers() diff --git a/glance/tests/integration/legacy_functional/test_v1_api.py b/glance/tests/integration/legacy_functional/test_v1_api.py index 8d83494c531865036c83f779acf1815a6a04a3f1..bf7486a1db7a1c8c6b7fbaed125892757afdc058 100644 --- a/glance/tests/integration/legacy_functional/test_v1_api.py +++ b/glance/tests/integration/legacy_functional/test_v1_api.py @@ -358,6 +358,8 @@ class TestApi(base.ApiTest): path = "/v1/images" response, content = self.http.request(path, 'POST', headers=headers) self.assertEqual(201, response.status) + image = jsonutils.loads(content)['image'] + self.assertEqual('active', image['status']) # 2. HEAD image-location # Verify image size is zero and the status is active -- 2.5.0