Adding 'download_image' policy enforcement to image cache middleware

Currently image cache middleware not care 'download_image' policy, the
enforcement caused user receive empty content but with HTTP 200 code
rather than 403 when client attempt to download image using v2 API. And
the real Forbidden exception be logged in glance-api log which image
application action raised. The end user is confused by this behavior.

Fixes bug: 1235378

Change-Id: Ibaa7ccf8613ee3cce4cb6a72e3206a2c94122222
Signed-off-by: Zhi Yan Liu <zhiyanl@cn.ibm.com>

glance/api/middleware/cache.py

@@ -29,6 +29,7 @@
 import webob
 
 from glance.api.common import size_checked_iter
+from glance.api import policy
 from glance.api.v1 import images
 from glance.common import exception
 from glance.common import utils
@@ -54,6 +55,7 @@ class CacheFilter(wsgi.Middleware):
     def __init__(self, app):
         self.cache = image_cache.ImageCache()
         self.serializer = images.ImageSerializer()
+        self.policy = policy.Enforcer()
         LOG.info(_("Initialized image cache middleware"))
         super(CacheFilter, self).__init__(app)
 
@@ -91,6 +93,13 @@ def _match_request(request):
             else:
                 return (version, method, image_id)
 
+    def _enforce(self, req, action):
+        """Authorize an action against our policies"""
+        try:
+            self.policy.enforce(req.context, action, {})
+        except exception.Forbidden as e:
+            raise webob.exc.HTTPForbidden(explanation=unicode(e), request=req)
+
     def process_request(self, request):
         """
         For requests for an image file, we check the local image
@@ -110,6 +119,11 @@ def process_request(self, request):
         if request.method != 'GET' or not self.cache.is_cached(image_id):
             return None
 
+        try:
+            self._enforce(request, 'download_image')
+        except webob.exc.HTTPForbidden:
+            return None
+
         LOG.debug(_("Cache hit for image '%s'"), image_id)
         image_iterator = self.get_from_cache(image_id)
         method = getattr(self, '_process_%s_request' % version)
@@ -230,6 +244,13 @@ def _process_GET_response(self, resp, image_id):
         if not image_checksum:
             LOG.error(_("Checksum header is missing."))
 
+        # NOTE(zhiyan): image_cache return a generator object and set to
+        # response.app_iter, it will be called by eventlet.wsgi later.
+        # So we need enforce policy firstly but do it by application
+        # since eventlet.wsgi could not catch webob.exc.HTTPForbidden and
+        # return 403 error to client then.
+        self._enforce(resp.request, 'download_image')
+
         resp.app_iter = self.cache.get_caching_iter(image_id, image_checksum,
                                                     resp.app_iter)
         return resp

glance/common/wsgi.py

@@ -376,7 +376,10 @@ def __call__(self, req):
             return response
         response = req.get_response(self.application)
         response.request = req
-        return self.process_response(response)
+        try:
+            return self.process_response(response)
+        except webob.exc.HTTPException as e:
+            return e
 
 
 class Debug(Middleware):

glance/tests/functional/test_cache_middleware.py

@@ -217,6 +217,121 @@ def test_cache_remote_image(self):
 
         self.stop_servers()
 
+    @skip_if_disabled
+    def test_cache_middleware_trans_v1_without_download_image_policy(self):
+        """
+        Ensure the image v1 API image transfer applied 'download_image'
+        policy enforcement.
+        """
+        self.cleanup()
+        self.start_servers(**self.__dict__.copy())
+
+        # Add an image and verify a 200 OK is returned
+        image_data = "*" * FIVE_KB
+        headers = minimal_headers('Image1')
+        path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'POST', headers=headers,
+                                         body=image_data)
+        self.assertEqual(response.status, 201)
+        data = json.loads(content)
+        self.assertEqual(data['image']['checksum'],
+                         hashlib.md5(image_data).hexdigest())
+        self.assertEqual(data['image']['size'], FIVE_KB)
+        self.assertEqual(data['image']['name'], "Image1")
+        self.assertEqual(data['image']['is_public'], True)
+
+        image_id = data['image']['id']
+
+        # Verify image not in cache
+        image_cached_path = os.path.join(self.api_server.image_cache_dir,
+                                         image_id)
+        self.assertFalse(os.path.exists(image_cached_path))
+
+        rules = {"context_is_admin": "role:admin", "default": "",
+                 "download_image": "!"}
+        self.set_policy_rules(rules)
+
+        # Grab the image
+        path = "http://%s:%d/v1/images/%s" % ("127.0.0.1", self.api_port,
+                                              image_id)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 403)
+
+        # Now, we delete the image from the server and verify that
+        # the image cache no longer contains the deleted image
+        path = "http://%s:%d/v1/images/%s" % ("127.0.0.1", self.api_port,
+                                              image_id)
+        http = httplib2.Http()
+        response, content = http.request(path, 'DELETE')
+        self.assertEqual(response.status, 200)
+
+        self.assertFalse(os.path.exists(image_cached_path))
+
+        self.stop_servers()
+
+    @skip_if_disabled
+    def test_cache_middleware_trans_v2_without_download_image_policy(self):
+        """
+        Ensure the image v2 API image transfer applied 'download_image'
+        policy enforcement.
+        """
+        self.cleanup()
+        self.start_servers(**self.__dict__.copy())
+
+        # Add an image and verify success
+        path = "http://%s:%d/v2/images" % ("0.0.0.0", self.api_port)
+        http = httplib2.Http()
+        headers = {'content-type': 'application/json'}
+        image_entity = {
+            'name': 'Image1',
+            'visibility': 'public',
+            'container_format': 'bare',
+            'disk_format': 'raw',
+        }
+        response, content = http.request(path, 'POST',
+                                         headers=headers,
+                                         body=json.dumps(image_entity))
+        self.assertEqual(response.status, 201)
+        data = json.loads(content)
+        image_id = data['id']
+
+        path = "http://%s:%d/v2/images/%s/file" % ("0.0.0.0", self.api_port,
+                                                   image_id)
+        headers = {'content-type': 'application/octet-stream'}
+        image_data = "*" * FIVE_KB
+        response, content = http.request(path, 'PUT',
+                                         headers=headers,
+                                         body=image_data)
+        self.assertEqual(response.status, 204)
+
+        # Verify image not in cache
+        image_cached_path = os.path.join(self.api_server.image_cache_dir,
+                                         image_id)
+        self.assertFalse(os.path.exists(image_cached_path))
+
+        rules = {"context_is_admin": "role:admin", "default": "",
+                 "download_image": "!"}
+        self.set_policy_rules(rules)
+
+        # Grab the image
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 403)
+
+        # Now, we delete the image from the server and verify that
+        # the image cache no longer contains the deleted image
+        path = "http://%s:%d/v2/images/%s" % ("0.0.0.0", self.api_port,
+                                              image_id)
+        http = httplib2.Http()
+        response, content = http.request(path, 'DELETE')
+        self.assertEqual(response.status, 204)
+
+        self.assertFalse(os.path.exists(image_cached_path))
+
+        self.stop_servers()
+
 
 class BaseCacheManageMiddlewareTest(object):
 

glance/tests/unit/test_cache_middleware.py

@@ -22,7 +22,8 @@
 from glance import context
 import glance.db.sqlalchemy.api as db
 import glance.registry.client.v1.api as registry
-from glance.tests import utils
+from glance.tests.unit import base
+from glance.tests.unit import utils as unit_test_utils
 
 
 class TestCacheMiddlewareURLMatching(testtools.TestCase):
@@ -87,13 +88,20 @@ def get_caching_iter(self, image_id, image_checksum, app_iter):
                 self.image_checksum = image_checksum
 
         self.cache = DummyCache()
+        self.policy = unit_test_utils.FakePolicyEnforcer()
 
 
-class TestCacheMiddlewareChecksumVerification(testtools.TestCase):
+class TestCacheMiddlewareChecksumVerification(base.IsolatedUnitTest):
+    def setUp(self):
+        super(TestCacheMiddlewareChecksumVerification, self).setUp()
+        self.context = context.RequestContext(is_admin=True)
+        self.request = webob.Request.blank('')
+        self.request.context = self.context
+
     def test_checksum_v1_header(self):
         cache_filter = ChecksumTestCacheFilter()
         headers = {"x-image-meta-checksum": "1234567890"}
-        resp = webob.Response(headers=headers)
+        resp = webob.Response(request=self.request, headers=headers)
         cache_filter._process_GET_response(resp, None)
 
         self.assertEqual("1234567890", cache_filter.cache.image_checksum)
@@ -104,14 +112,14 @@ def test_checksum_v2_header(self):
             "x-image-meta-checksum": "1234567890",
             "Content-MD5": "abcdefghi"
         }
-        resp = webob.Response(headers=headers)
+        resp = webob.Response(request=self.request, headers=headers)
         cache_filter._process_GET_response(resp, None)
 
         self.assertEqual("abcdefghi", cache_filter.cache.image_checksum)
 
     def test_checksum_missing_header(self):
         cache_filter = ChecksumTestCacheFilter()
-        resp = webob.Response()
+        resp = webob.Response(request=self.request)
         cache_filter._process_GET_response(resp, None)
 
         self.assertEqual(None, cache_filter.cache.image_checksum)
@@ -143,14 +151,10 @@ def get_image_size(self, image_id):
                 pass
 
         self.cache = DummyCache()
+        self.policy = unit_test_utils.FakePolicyEnforcer()
 
 
-class TestCacheMiddlewareProcessRequest(utils.BaseTestCase):
-    def setUp(self):
-        super(TestCacheMiddlewareProcessRequest, self).setUp()
-        self.stubs = stubout.StubOutForTesting()
-        self.addCleanup(self.stubs.UnsetAll)
-
+class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
     def test_v1_deleted_image_fetch(self):
         """
         Test for determining that when an admin tries to download a deleted
@@ -182,6 +186,7 @@ def fake_process_v1_request(request, image_id, image_iterator):
 
         image_id = 'test1'
         request = webob.Request.blank('/v1/images/%s' % image_id)
+        request.context = context.RequestContext()
 
         cache_filter = ProcessRequestTestCacheFilter()
         self.stubs.Set(cache_filter, '_process_v1_request',
@@ -307,23 +312,32 @@ def fake_image_tag_get_all(context, image_id, session=None):
         self.assertEqual(response.headers['Content-Length'],
                          '123456789')
 
+    def test_process_request_without_download_image_policy(self):
+        """
+        Test for cache middleware skip processing when request
+        context has not 'download_image' role.
+        """
+        image_id = 'test1'
+        request = webob.Request.blank('/v1/images/%s' % image_id)
+        request.context = context.RequestContext()
 
-class TestProcessResponse(utils.BaseTestCase):
-    def setUp(self):
-        super(TestProcessResponse, self).setUp()
-        self.stubs = stubout.StubOutForTesting()
+        cache_filter = ProcessRequestTestCacheFilter()
 
-    def tearDown(self):
-        super(TestProcessResponse, self).tearDown()
-        self.stubs.UnsetAll()
+        rules = {'download_image': '!'}
+        self.set_policy_rules(rules)
+        cache_filter.policy = glance.api.policy.Enforcer()
 
+        self.assertEqual(None, cache_filter.process_request(request))
+
+
+class TestCacheMiddlewareProcessResponse(base.IsolatedUnitTest):
     def test_process_v1_DELETE_response(self):
         image_id = 'test1'
         request = webob.Request.blank('/v1/images/%s' % image_id)
         request.context = context.RequestContext()
         cache_filter = ProcessRequestTestCacheFilter()
         headers = {"x-image-meta-deleted": True}
-        resp = webob.Response(headers=headers)
+        resp = webob.Response(request=request, headers=headers)
         actual = cache_filter._process_DELETE_response(resp, image_id)
         self.assertEqual(actual, resp)
 
@@ -335,7 +349,7 @@ def test_get_status_code(self):
         self.assertEqual(200, actual)
 
     def test_process_response(self):
-        def fake_fetch_request_info():
+        def fake_fetch_request_info(*args, **kwargs):
             return ('test1', 'GET')
 
         cache_filter = ProcessRequestTestCacheFilter()
@@ -344,6 +358,28 @@ def fake_fetch_request_info():
         request = webob.Request.blank('/v1/images/%s' % image_id)
         request.context = context.RequestContext()
         headers = {"x-image-meta-deleted": True}
-        resp1 = webob.Response(headers=headers)
-        actual = cache_filter.process_response(resp1)
-        self.assertEqual(actual, resp1)
+        resp = webob.Response(request=request, headers=headers)
+        actual = cache_filter.process_response(resp)
+        self.assertEqual(actual, resp)
+
+    def test_process_response_without_download_image_policy(self):
+        """
+        Test for cache middleware raise webob.exc.HTTPForbidden directly
+        when request context has not 'download_image' role.
+        """
+        def fake_fetch_request_info(*args, **kwargs):
+            return ('test1', 'GET')
+
+        cache_filter = ProcessRequestTestCacheFilter()
+        cache_filter._fetch_request_info = fake_fetch_request_info
+        rules = {'download_image': '!'}
+        self.set_policy_rules(rules)
+        cache_filter.policy = glance.api.policy.Enforcer()
+
+        image_id = 'test1'
+        request = webob.Request.blank('/v1/images/%s' % image_id)
+        request.context = context.RequestContext()
+        resp = webob.Response(request=request)
+        self.assertRaises(webob.exc.HTTPForbidden,
+                          cache_filter.process_response, resp)
+        self.assertEqual([''], resp.app_iter)