openstack/glance
Code Issues Proposed changes

Adding 'download_image' policy enforcement to image cache middleware

Browse Source 
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>
This commit is contained in:
Zhi Yan Liu 2013-10-07 11:44:33 +08:00 committed by Gerrit Code Review
parent 0059c1d8aa
commit a50bfbf490
4 changed files with 199 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
glance/api/middleware/cache.py
View File

@ -29,6 +29,7 @@ import re
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 @@ class CacheFilter(wsgi.Middleware):
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 @@ class CacheFilter(wsgi.Middleware):
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 @@ class CacheFilter(wsgi.Middleware):
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

5
glance/common/wsgi.py
View File

@ -376,7 +376,10 @@ class Middleware(object):
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):

115
glance/tests/functional/test_cache_middleware.py
View File

@ -217,6 +217,121 @@ class BaseCacheMiddlewareTest(object):
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):

82
glance/tests/unit/test_cache_middleware.py
View File

@ -22,7 +22,8 @@ from glance.common import exception
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 @@ class ChecksumTestCacheFilter(glance.api.middleware.cache.CacheFilter):
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 @@ class TestCacheMiddlewareChecksumVerification(testtools.TestCase):
"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 @@ class ProcessRequestTestCacheFilter(glance.api.middleware.cache.CacheFilter):
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 @@ class TestCacheMiddlewareProcessRequest(utils.BaseTestCase):
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 @@ class TestCacheMiddlewareProcessRequest(utils.BaseTestCase):
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 @@ class TestProcessResponse(utils.BaseTestCase):
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 @@ class TestProcessResponse(utils.BaseTestCase):
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)