openstack/glance
Code Issues Proposed changes

Check download_image policy in the API

Browse Source 
Note that this adds a clause to the download exception handler which
returns 404 if the user can't download the image via policy, but leaves
the existing internal forbidden->403 handler because of how image
property protections work.

Also Cache middleware do not have any influence of policy refactoring.

Partially-Implements: blueprint policy-refactor

Change-Id: I060f22145d2915bfd0b0b3efc2912c24e89b9a30
This commit is contained in:
Abhishek Kekane
2021-08-13 20:31:28 +00:00
parent cb3369002a
commit 6ffd80f9c0
6 changed files with 95 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -31,6 +31,7 @@ import webob
from glance.api.common import size_checked_iter
from glance.api import policy
from glance.api.v2 import policy as api_policy
from glance.common import exception
from glance.common import utils
from glance.common import wsgi
@@ -95,25 +96,11 @@ class CacheFilter(wsgi.Middleware):
if image_id != 'detail':
return (version, method, image_id)
def _enforce(self, req, action, target=None):
def _enforce(self, req, image):
"""Authorize an action against our policies"""
if target is None:
target = {}
if isinstance(target, policy.ImageTarget):
# The _get_v2_image_metadata() method returns an instance of
# ImageTarget(), which isn't mutable and we need to make sure we
# update the project_id attribute of the target to match the owner.
# If the target isn't of dict() type already, we should explicitly
# handle that here. We take a similar approach in the policy layer
# (glance.api.policy) to make sure we build an accurate image
# target.
target = dict(target)
target['project_id'] = target.get('owner', None)
try:
self.policy.enforce(req.context, action, target)
except exception.Forbidden as e:
LOG.debug("User not permitted to perform '%s' action", action)
raise webob.exc.HTTPForbidden(explanation=e.msg, request=req)
api_pol = api_policy.ImageAPIPolicy(req.context, image,
self.policy)
api_pol.download_image()
def _get_v2_image_metadata(self, request, image_id):
"""
@@ -128,7 +115,7 @@ class CacheFilter(wsgi.Middleware):
# _process_v2_request call.
request.environ['api.cache.image'] = image
return policy.ImageTarget(image)
return (image, policy.ImageTarget(image))
except exception.NotFound as e:
raise webob.exc.HTTPNotFound(explanation=e.msg, request=request)
@@ -160,23 +147,23 @@ class CacheFilter(wsgi.Middleware):
return None
method = getattr(self, '_get_%s_image_metadata' % version)
image_metadata = method(request, image_id)
image, metadata = method(request, image_id)
# Deactivated images shall not be served from cache
if image_metadata['status'] == 'deactivated':
if metadata['status'] == 'deactivated':
return None
try:
self._enforce(request, 'download_image', target=image_metadata)
except exception.Forbidden:
return None
# NOTE(abhishekk): This means image is present in cache and before
# request is coming to API we are enforcing this check in the
# middleware
self._enforce(request, image)
LOG.debug("Cache hit for image '%s'", image_id)
image_iterator = self.get_from_cache(image_id)
method = getattr(self, '_process_%s_request' % version)
try:
return method(request, image_id, image_iterator, image_metadata)
return method(request, image_id, image_iterator, metadata)
except exception.ImageNotFound:
msg = _LE("Image cache contained image file for image '%s', "
"however the database did not contain metadata for "
@@ -290,16 +277,20 @@ class CacheFilter(wsgi.Middleware):
LOG.error(_LE("Checksum header is missing."))
# fetch image_meta on the basis of version
image_metadata = None
image = None
if version:
method = getattr(self, '_get_%s_image_metadata' % version)
image_metadata = method(resp.request, image_id)
image, metadata = method(resp.request, image_id)
# 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', target=image_metadata)
# FIXME(abhishekk): This policy check here is not necessary as this
# will hit only during first image download i.e. while image is not
# present in cache. We already enforced same check in API layer and
# enforcing same check here again makes no sense.
self._enforce(resp.request, image)
resp.app_iter = self.cache.get_caching_iter(image_id, image_checksum,
resp.app_iter)

8
glance/api/v2/image_data.py
View File

@@ -443,13 +443,19 @@ class ImageDataController(object):
self._restore(image_repo, image)
def download(self, req, image_id):
image_repo = self.gateway.get_repo(req.context)
image_repo = self.gateway.get_repo(
req.context, authorization_layer=False)
try:
image = image_repo.get(image_id)
if image.status == 'deactivated' and not req.context.is_admin:
msg = _('The requested image has been deactivated. '
'Image data download is forbidden.')
raise exception.Forbidden(message=msg)
# NOTE(abhishekk): This is the right place to verify whether
# user has permission to download the image or not.
api_pol = api_policy.ImageAPIPolicy(req.context, image,
self.policy)
api_pol.download_image()
except exception.NotFound as e:
raise webob.exc.HTTPNotFound(explanation=e.msg)
except exception.Forbidden as e:

3
glance/api/v2/policy.py
View File

@@ -156,6 +156,9 @@ class ImageAPIPolicy(APIPolicyBase):
if not CONF.enforce_secure_rbac:
check_is_image_mutable(self._context, self._image)
def download_image(self):
self._enforce('download_image')
class MetadefAPIPolicy(APIPolicyBase):
def __init__(self, context, md_resource=None, target=None, enforcer=None):

47
glance/tests/functional/v2/test_images_api_policy.py
View File

@@ -294,3 +294,50 @@ class TestImagesPolicy(functional.SynchronousAPIBase):
# Make sure upload returns 204 because even though we can not
# see the image, we can upload data to it
self._create_and_upload(expected_code=204)
def test_image_download(self):
# NOTE(abhishekk): These tests are running without cache middleware
self.start_server()
image_id = self._create_and_upload()
# First make sure we can download image
path = '/v2/images/%s/file' % image_id
response = self.api_get(path)
self.assertEqual(200, response.status_code)
self.assertEqual('IMAGEDATA', response.text)
# Now disable download permissions, but allow get_image
self.set_policy_rules({
'get_image': '',
'download_image': '!'
})
# Make sure download returns 403 because we can see the image,
# just not download it
response = self.api_get(path)
self.assertEqual(403, response.status_code)
# Now disable download permissions, including get_image
self.set_policy_rules({
'get_image': '!',
'download_image': '!',
})
# Make sure download returns 404 because we can not see nor
# download it
response = self.api_get(path)
self.assertEqual(404, response.status_code)
# Now allow download, but disallow get_image, just to prove that
# you do not need get_image in order to be granted download, and
# that we only use it for error code determination if
# permission is denied.
self.set_policy_rules({
'get_image': '!',
'download_image': ''})
# Make sure download returns 200 because even though we can not
# see the image, we can download it
response = self.api_get(path)
self.assertEqual(200, response.status_code)
self.assertEqual('IMAGEDATA', response.text)

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

@@ -292,7 +292,8 @@ class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
"""
def fake_get_v2_image_metadata(*args, **kwargs):
return {'status': 'active', 'properties': {}}
image = ImageStub(image_id, request.context.project_id)
return image, {'status': 'active', 'properties': {}}
image_id = 'test1'
request = webob.Request.blank('/v2/images/%s/file' % image_id)
@@ -301,7 +302,10 @@ class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
cache_filter = ProcessRequestTestCacheFilter()
cache_filter._get_v2_image_metadata = fake_get_v2_image_metadata
enforcer = self._enforcer_from_rules({'download_image': '!'})
enforcer = self._enforcer_from_rules({
'get_image': '',
'download_image': '!'
})
cache_filter.policy = enforcer
self.assertRaises(webob.exc.HTTPForbidden,
cache_filter.process_request, request)
@@ -320,12 +324,13 @@ class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
image = ImageStub(image_id, request.context.project_id,
extra_properties=extra_properties)
request.environ['api.cache.image'] = image
return glance.api.policy.ImageTarget(image)
return image, glance.api.policy.ImageTarget(image)
enforcer = self._enforcer_from_rules({
"restricted":
"not ('test_1234':%(x_test_key)s and role:_member_)",
"download_image": "role:admin or rule:restricted"
"download_image": "role:admin or rule:restricted",
"get_image": ""
})
request = webob.Request.blank('/v2/images/test1/file')
@@ -351,7 +356,7 @@ class TestCacheMiddlewareProcessRequest(base.IsolatedUnitTest):
image = ImageStub(image_id, request.context.project_id,
extra_properties=extra_properties)
request.environ['api.cache.image'] = image
return glance.api.policy.ImageTarget(image)
return image, glance.api.policy.ImageTarget(image)
request = webob.Request.blank('/v2/images/test1/file')
request.context = context.RequestContext(roles=['member'])
@@ -396,7 +401,7 @@ class TestCacheMiddlewareProcessResponse(base.IsolatedUnitTest):
image = test_policy.ImageStub(
image_id, extra_properties=extra_properties)
request.environ['api.cache.image'] = image
return glance.api.policy.ImageTarget(image)
return image, glance.api.policy.ImageTarget(image)
cache_filter = ProcessRequestTestCacheFilter()
cache_filter._fetch_request_info = fake_fetch_request_info
@@ -434,7 +439,7 @@ class TestCacheMiddlewareProcessResponse(base.IsolatedUnitTest):
image = ImageStub(image_id, request.context.project_id,
extra_properties=extra_properties)
request.environ['api.cache.image'] = image
return glance.api.policy.ImageTarget(image)
return image, glance.api.policy.ImageTarget(image)
cache_filter = ProcessRequestTestCacheFilter()
cache_filter._fetch_request_info = fake_fetch_request_info

6
glance/tests/unit/v2/test_v2_policy.py
View File

@@ -260,6 +260,12 @@ class APIImagePolicy(APIPolicyBase):
self.policy.upload_image()
self.assertFalse(m.called)
def test_download_image(self):
self.policy.download_image()
self.enforcer.enforce.assert_called_once_with(self.context,
'download_image',
mock.ANY)
class TestMetadefAPIPolicy(APIPolicyBase):
def setUp(self):