Skip to content

Commit a50bfbf

Browse files
Zhi Yan Liuopenstack-gerrit
Zhi Yan Liu
authored and
openstack-gerrit
committed
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>
1 parent 0059c1d commit a50bfbf

File tree

4 files changed

+199
-24
lines changed

4 files changed

+199
-24
lines changed

‎glance/api/middleware/cache.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import webob
3030

3131
from glance.api.common import size_checked_iter
32+
from glance.api import policy
3233
from glance.api.v1 import images
3334
from glance.common import exception
3435
from glance.common import utils
@@ -54,6 +55,7 @@ class CacheFilter(wsgi.Middleware):
5455
def __init__(self, app):
5556
self.cache = image_cache.ImageCache()
5657
self.serializer = images.ImageSerializer()
58+
self.policy = policy.Enforcer()
5759
LOG.info(_("Initialized image cache middleware"))
5860
super(CacheFilter, self).__init__(app)
5961

@@ -91,6 +93,13 @@ def _match_request(request):
9193
else:
9294
return (version, method, image_id)
9395

96+
def _enforce(self, req, action):
97+
"""Authorize an action against our policies"""
98+
try:
99+
self.policy.enforce(req.context, action, {})
100+
except exception.Forbidden as e:
101+
raise webob.exc.HTTPForbidden(explanation=unicode(e), request=req)
102+
94103
def process_request(self, request):
95104
"""
96105
For requests for an image file, we check the local image
@@ -110,6 +119,11 @@ def process_request(self, request):
110119
if request.method != 'GET' or not self.cache.is_cached(image_id):
111120
return None
112121

122+
try:
123+
self._enforce(request, 'download_image')
124+
except webob.exc.HTTPForbidden:
125+
return None
126+
113127
LOG.debug(_("Cache hit for image '%s'"), image_id)
114128
image_iterator = self.get_from_cache(image_id)
115129
method = getattr(self, '_process_%s_request' % version)
@@ -230,6 +244,13 @@ def _process_GET_response(self, resp, image_id):
230244
if not image_checksum:
231245
LOG.error(_("Checksum header is missing."))
232246

247+
# NOTE(zhiyan): image_cache return a generator object and set to
248+
# response.app_iter, it will be called by eventlet.wsgi later.
249+
# So we need enforce policy firstly but do it by application
250+
# since eventlet.wsgi could not catch webob.exc.HTTPForbidden and
251+
# return 403 error to client then.
252+
self._enforce(resp.request, 'download_image')
253+
233254
resp.app_iter = self.cache.get_caching_iter(image_id, image_checksum,
234255
resp.app_iter)
235256
return resp

‎glance/common/wsgi.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,10 @@ def __call__(self, req):
376376
return response
377377
response = req.get_response(self.application)
378378
response.request = req
379-
return self.process_response(response)
379+
try:
380+
return self.process_response(response)
381+
except webob.exc.HTTPException as e:
382+
return e
380383

381384

382385
class Debug(Middleware):

‎glance/tests/functional/test_cache_middleware.py

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,121 @@ def test_cache_remote_image(self):
217217

218218
self.stop_servers()
219219

220+
@skip_if_disabled
221+
def test_cache_middleware_trans_v1_without_download_image_policy(self):
222+
"""
223+
Ensure the image v1 API image transfer applied 'download_image'
224+
policy enforcement.
225+
"""
226+
self.cleanup()
227+
self.start_servers(**self.__dict__.copy())
228+
229+
# Add an image and verify a 200 OK is returned
230+
image_data = "*" * FIVE_KB
231+
headers = minimal_headers('Image1')
232+
path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
233+
http = httplib2.Http()
234+
response, content = http.request(path, 'POST', headers=headers,
235+
body=image_data)
236+
self.assertEqual(response.status, 201)
237+
data = json.loads(content)
238+
self.assertEqual(data['image']['checksum'],
239+
hashlib.md5(image_data).hexdigest())
240+
self.assertEqual(data['image']['size'], FIVE_KB)
241+
self.assertEqual(data['image']['name'], "Image1")
242+
self.assertEqual(data['image']['is_public'], True)
243+
244+
image_id = data['image']['id']
245+
246+
# Verify image not in cache
247+
image_cached_path = os.path.join(self.api_server.image_cache_dir,
248+
image_id)
249+
self.assertFalse(os.path.exists(image_cached_path))
250+
251+
rules = {"context_is_admin": "role:admin", "default": "",
252+
"download_image": "!"}
253+
self.set_policy_rules(rules)
254+
255+
# Grab the image
256+
path = "http://%s:%d/v1/images/%s" % ("127.0.0.1", self.api_port,
257+
image_id)
258+
http = httplib2.Http()
259+
response, content = http.request(path, 'GET')
260+
self.assertEqual(response.status, 403)
261+
262+
# Now, we delete the image from the server and verify that
263+
# the image cache no longer contains the deleted image
264+
path = "http://%s:%d/v1/images/%s" % ("127.0.0.1", self.api_port,
265+
image_id)
266+
http = httplib2.Http()
267+
response, content = http.request(path, 'DELETE')
268+
self.assertEqual(response.status, 200)
269+
270+
self.assertFalse(os.path.exists(image_cached_path))
271+
272+
self.stop_servers()
273+
274+
@skip_if_disabled
275+
def test_cache_middleware_trans_v2_without_download_image_policy(self):
276+
"""
277+
Ensure the image v2 API image transfer applied 'download_image'
278+
policy enforcement.
279+
"""
280+
self.cleanup()
281+
self.start_servers(**self.__dict__.copy())
282+
283+
# Add an image and verify success
284+
path = "http://%s:%d/v2/images" % ("0.0.0.0", self.api_port)
285+
http = httplib2.Http()
286+
headers = {'content-type': 'application/json'}
287+
image_entity = {
288+
'name': 'Image1',
289+
'visibility': 'public',
290+
'container_format': 'bare',
291+
'disk_format': 'raw',
292+
}
293+
response, content = http.request(path, 'POST',
294+
headers=headers,
295+
body=json.dumps(image_entity))
296+
self.assertEqual(response.status, 201)
297+
data = json.loads(content)
298+
image_id = data['id']
299+
300+
path = "http://%s:%d/v2/images/%s/file" % ("0.0.0.0", self.api_port,
301+
image_id)
302+
headers = {'content-type': 'application/octet-stream'}
303+
image_data = "*" * FIVE_KB
304+
response, content = http.request(path, 'PUT',
305+
headers=headers,
306+
body=image_data)
307+
self.assertEqual(response.status, 204)
308+
309+
# Verify image not in cache
310+
image_cached_path = os.path.join(self.api_server.image_cache_dir,
311+
image_id)
312+
self.assertFalse(os.path.exists(image_cached_path))
313+
314+
rules = {"context_is_admin": "role:admin", "default": "",
315+
"download_image": "!"}
316+
self.set_policy_rules(rules)
317+
318+
# Grab the image
319+
http = httplib2.Http()
320+
response, content = http.request(path, 'GET')
321+
self.assertEqual(response.status, 403)
322+
323+
# Now, we delete the image from the server and verify that
324+
# the image cache no longer contains the deleted image
325+
path = "http://%s:%d/v2/images/%s" % ("0.0.0.0", self.api_port,
326+
image_id)
327+
http = httplib2.Http()
328+
response, content = http.request(path, 'DELETE')
329+
self.assertEqual(response.status, 204)
330+
331+
self.assertFalse(os.path.exists(image_cached_path))
332+
333+
self.stop_servers()
334+
220335

221336
class BaseCacheManageMiddlewareTest(object):
222337

‎glance/tests/unit/test_cache_middleware.py

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
from glance import context
2323
import glance.db.sqlalchemy.api as db
2424
import glance.registry.client.v1.api as registry
25-
from glance.tests import utils
25+
from glance.tests.unit import base
26+
from glance.tests.unit import utils as unit_test_utils
2627

2728

2829
class TestCacheMiddlewareURLMatching(testtools.TestCase):
@@ -87,13 +88,20 @@ def get_caching_iter(self, image_id, image_checksum, app_iter):
8788
self.image_checksum = image_checksum
8889

8990
self.cache = DummyCache()
91+
self.policy = unit_test_utils.FakePolicyEnforcer()
9092

9193

92-
class TestCacheMiddlewareChecksumVerification(testtools.TestCase):
94+
class TestCacheMiddlewareChecksumVerification(base.IsolatedUnitTest):
95+
def setUp(self):
96+
super(TestCacheMiddlewareChecksumVerification, self).setUp()
97+
self.context = context.RequestContext(is_admin=True)
98+
self.request = webob.Request.blank('')
99+
self.request.context = self.context
100+
93101
def test_checksum_v1_header(self):
94102
cache_filter = ChecksumTestCacheFilter()
95103
headers = {"x-image-meta-checksum": "1234567890"}
96-
resp = webob.Response(headers=headers)
104+
resp = webob.Response(request=self.request, headers=headers)
97105
cache_filter._process_GET_response(resp, None)
98106

99107
self.assertEqual("1234567890", cache_filter.cache.image_checksum)
@@ -104,14 +112,14 @@ def test_checksum_v2_header(self):
104112
"x-image-meta-checksum": "1234567890",
105113
"Content-MD5": "abcdefghi"
106114
}
107-
resp = webob.Response(headers=headers)
115+
resp = webob.Response(request=self.request, headers=headers)
108116
cache_filter._process_GET_response(resp, None)
109117

110118
self.assertEqual("abcdefghi", cache_filter.cache.image_checksum)
111119

112120
def test_checksum_missing_header(self):
113121
cache_filter = ChecksumTestCacheFilter()
114-
resp = webob.Response()
122+
resp = webob.Response(request=self.request)
115123
cache_filter._process_GET_response(resp, None)
116124

117125
self.assertEqual(None, cache_filter.cache.image_checksum)
@@ -143,14 +151,10 @@ def get_image_size(self, image_id):
143151
pass
144152

145153
self.cache = DummyCache()
154+
self.policy = unit_test_utils.FakePolicyEnforcer()
146155

147156

148-
class TestCacheMiddlewareProcessRequest(utils.BaseTestCase):
149-
def setUp(self):
150-
super(TestCacheMiddlewareProcessRequest, self).setUp()
151-
self.stubs = stubout.StubOutForTesting()
152-
self.addCleanup(self.stubs.UnsetAll)
153-
157+
class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
154158
def test_v1_deleted_image_fetch(self):
155159
"""
156160
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):
182186

183187
image_id = 'test1'
184188
request = webob.Request.blank('/v1/images/%s' % image_id)
189+
request.context = context.RequestContext()
185190

186191
cache_filter = ProcessRequestTestCacheFilter()
187192
self.stubs.Set(cache_filter, '_process_v1_request',
@@ -307,23 +312,32 @@ def fake_image_tag_get_all(context, image_id, session=None):
307312
self.assertEqual(response.headers['Content-Length'],
308313
'123456789')
309314

315+
def test_process_request_without_download_image_policy(self):
316+
"""
317+
Test for cache middleware skip processing when request
318+
context has not 'download_image' role.
319+
"""
320+
image_id = 'test1'
321+
request = webob.Request.blank('/v1/images/%s' % image_id)
322+
request.context = context.RequestContext()
310323

311-
class TestProcessResponse(utils.BaseTestCase):
312-
def setUp(self):
313-
super(TestProcessResponse, self).setUp()
314-
self.stubs = stubout.StubOutForTesting()
324+
cache_filter = ProcessRequestTestCacheFilter()
315325

316-
def tearDown(self):
317-
super(TestProcessResponse, self).tearDown()
318-
self.stubs.UnsetAll()
326+
rules = {'download_image': '!'}
327+
self.set_policy_rules(rules)
328+
cache_filter.policy = glance.api.policy.Enforcer()
319329

330+
self.assertEqual(None, cache_filter.process_request(request))
331+
332+
333+
class TestCacheMiddlewareProcessResponse(base.IsolatedUnitTest):
320334
def test_process_v1_DELETE_response(self):
321335
image_id = 'test1'
322336
request = webob.Request.blank('/v1/images/%s' % image_id)
323337
request.context = context.RequestContext()
324338
cache_filter = ProcessRequestTestCacheFilter()
325339
headers = {"x-image-meta-deleted": True}
326-
resp = webob.Response(headers=headers)
340+
resp = webob.Response(request=request, headers=headers)
327341
actual = cache_filter._process_DELETE_response(resp, image_id)
328342
self.assertEqual(actual, resp)
329343

@@ -335,7 +349,7 @@ def test_get_status_code(self):
335349
self.assertEqual(200, actual)
336350

337351
def test_process_response(self):
338-
def fake_fetch_request_info():
352+
def fake_fetch_request_info(*args, **kwargs):
339353
return ('test1', 'GET')
340354

341355
cache_filter = ProcessRequestTestCacheFilter()
@@ -344,6 +358,28 @@ def fake_fetch_request_info():
344358
request = webob.Request.blank('/v1/images/%s' % image_id)
345359
request.context = context.RequestContext()
346360
headers = {"x-image-meta-deleted": True}
347-
resp1 = webob.Response(headers=headers)
348-
actual = cache_filter.process_response(resp1)
349-
self.assertEqual(actual, resp1)
361+
resp = webob.Response(request=request, headers=headers)
362+
actual = cache_filter.process_response(resp)
363+
self.assertEqual(actual, resp)
364+
365+
def test_process_response_without_download_image_policy(self):
366+
"""
367+
Test for cache middleware raise webob.exc.HTTPForbidden directly
368+
when request context has not 'download_image' role.
369+
"""
370+
def fake_fetch_request_info(*args, **kwargs):
371+
return ('test1', 'GET')
372+
373+
cache_filter = ProcessRequestTestCacheFilter()
374+
cache_filter._fetch_request_info = fake_fetch_request_info
375+
rules = {'download_image': '!'}
376+
self.set_policy_rules(rules)
377+
cache_filter.policy = glance.api.policy.Enforcer()
378+
379+
image_id = 'test1'
380+
request = webob.Request.blank('/v1/images/%s' % image_id)
381+
request.context = context.RequestContext()
382+
resp = webob.Response(request=request)
383+
self.assertRaises(webob.exc.HTTPForbidden,
384+
cache_filter.process_response, resp)
385+
self.assertEqual([''], resp.app_iter)

0 commit comments

Comments
 (0)