From 45be8e1c620c50f3cbca76f561945200a8843bc8 Mon Sep 17 00:00:00 2001

From: Stuart McLaren <[email protected]>

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

(cherry picked from commit 4d08db5b6d42323ac1958ef3b7417d875e7bea8c)

(cherry picked from commit 9beca533f42ae1fc87418de0c360e19bc59b24b5)

---

 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 +

 test-requirements.txt                              |  5 ++

 5 files changed, 108 insertions(+)

diff --git a/glance/api/v1/__init__.py b/glance/api/v1/__init__.py

index 74de9aa..9306bbb 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 746f8cd..f976f9d 100644

--- a/glance/api/v1/images.py

+++ b/glance/api/v1/images.py

@@ -56,6 +56,7 @@ _LW = gettextutils._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')

@@ -895,6 +896,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/<IMAGE_ID> 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 1486fb3..ad54005 100644

--- a/glance/tests/functional/v1/test_api.py

+++ b/glance/tests/functional/v1/test_api.py

@@ -765,3 +765,92 @@ class TestApi(functional.FunctionalTest):

         self.assertEqual(200, 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 66455a2..0e5b339 100644

--- a/glance/tests/integration/legacy_functional/test_v1_api.py

+++ b/glance/tests/integration/legacy_functional/test_v1_api.py

@@ -357,6 +357,8 @@ class TestApi(base.ApiTest):

         path = "/v1/images"

         response, content = self.http.request(path, 'POST', headers=headers)

         self.assertEqual(response.status, 201)

+        image = jsonutils.loads(content)['image']

+        self.assertEqual('active', image['status'])

         # 2. HEAD image-location

         # Verify image size is zero and the status is active

diff --git a/test-requirements.txt b/test-requirements.txt

index 6d435f2..97affae 100644

--- a/test-requirements.txt

+++ b/test-requirements.txt

@@ -29,3 +29,8 @@ xattr>=0.4

 # Documentation

 oslosphinx>=2.2.0  # Apache-2.0

+

+# Gate is failing because of an older version of oslo.vmware is installing

+# PyYAML 3.11. Adding this line here will help moving this patch forward and

+# fixing Glance's stable/juno gate

+PyYAML<=3.10,>=3.1.0

-- 

1.9.1