Fix os-volumes-attachments policy to be admin_or_owner
os-volumes-attachments API policy is default to admin_or_owner[1] but API is allowed for everyone. We can see the test trying with other project context can access the API - https://review.opendev.org/#/c/709929/1/nova/tests/unit/policies/test_volumes.py@84 This is because API does not pass the server project_id in policy target, impact APIs: index: https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/api/openstack/compute/volumes.py#L282 show: https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/api/openstack/compute/volumes.py#L307 create: https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/api/openstack/compute/volumes.py#L337 delete: https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/api/openstack/compute/volumes.py#L440 And if no target is passed then, policy.py add the default targets which is nothing but context.project_id (allow for everyone try to access) - https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/policy.py#L191 [1]https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/policies/volumes_attachments.py#L21 Closes-bug: #1864776 Change-Id: Iff0d8024ee1faeaecb44d717bd870bcd32c8d99c
This commit is contained in:
@@ -279,9 +279,9 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
def index(self, req, server_id):
|
def index(self, req, server_id):
|
||||||
"""Returns the list of volume attachments for a given instance."""
|
"""Returns the list of volume attachments for a given instance."""
|
||||||
context = req.environ['nova.context']
|
context = req.environ['nova.context']
|
||||||
context.can(va_policies.POLICY_ROOT % 'index')
|
|
||||||
|
|
||||||
instance = common.get_instance(self.compute_api, context, server_id)
|
instance = common.get_instance(self.compute_api, context, server_id)
|
||||||
|
context.can(va_policies.POLICY_ROOT % 'index',
|
||||||
|
target={'project_id': instance.project_id})
|
||||||
|
|
||||||
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
|
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
|
||||||
context, instance.uuid)
|
context, instance.uuid)
|
||||||
@@ -304,10 +304,11 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
def show(self, req, server_id, id):
|
def show(self, req, server_id, id):
|
||||||
"""Return data about the given volume attachment."""
|
"""Return data about the given volume attachment."""
|
||||||
context = req.environ['nova.context']
|
context = req.environ['nova.context']
|
||||||
context.can(va_policies.POLICY_ROOT % 'show')
|
instance = common.get_instance(self.compute_api, context, server_id)
|
||||||
|
context.can(va_policies.POLICY_ROOT % 'show',
|
||||||
|
target={'project_id': instance.project_id})
|
||||||
|
|
||||||
volume_id = id
|
volume_id = id
|
||||||
instance = common.get_instance(self.compute_api, context, server_id)
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
|
bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
|
||||||
@@ -334,7 +335,9 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
def create(self, req, server_id, body):
|
def create(self, req, server_id, body):
|
||||||
"""Attach a volume to an instance."""
|
"""Attach a volume to an instance."""
|
||||||
context = req.environ['nova.context']
|
context = req.environ['nova.context']
|
||||||
context.can(va_policies.POLICY_ROOT % 'create')
|
instance = common.get_instance(self.compute_api, context, server_id)
|
||||||
|
context.can(va_policies.POLICY_ROOT % 'create',
|
||||||
|
target={'project_id': instance.project_id})
|
||||||
|
|
||||||
volume_id = body['volumeAttachment']['volumeId']
|
volume_id = body['volumeAttachment']['volumeId']
|
||||||
device = body['volumeAttachment'].get('device')
|
device = body['volumeAttachment'].get('device')
|
||||||
@@ -342,8 +345,6 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
delete_on_termination = body['volumeAttachment'].get(
|
delete_on_termination = body['volumeAttachment'].get(
|
||||||
'delete_on_termination', False)
|
'delete_on_termination', False)
|
||||||
|
|
||||||
instance = common.get_instance(self.compute_api, context, server_id)
|
|
||||||
|
|
||||||
if instance.vm_state in (vm_states.SHELVED,
|
if instance.vm_state in (vm_states.SHELVED,
|
||||||
vm_states.SHELVED_OFFLOADED):
|
vm_states.SHELVED_OFFLOADED):
|
||||||
_check_request_version(req, '2.20', 'attach_volume',
|
_check_request_version(req, '2.20', 'attach_volume',
|
||||||
@@ -395,7 +396,9 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
@validation.schema(volumes_schema.update_volume_attachment)
|
@validation.schema(volumes_schema.update_volume_attachment)
|
||||||
def update(self, req, server_id, id, body):
|
def update(self, req, server_id, id, body):
|
||||||
context = req.environ['nova.context']
|
context = req.environ['nova.context']
|
||||||
context.can(va_policies.POLICY_ROOT % 'update')
|
instance = common.get_instance(self.compute_api, context, server_id)
|
||||||
|
context.can(va_policies.POLICY_ROOT % 'update',
|
||||||
|
target={'project_id': instance.project_id})
|
||||||
|
|
||||||
old_volume_id = id
|
old_volume_id = id
|
||||||
try:
|
try:
|
||||||
@@ -416,8 +419,6 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
# NotFound response if that is not existent.
|
# NotFound response if that is not existent.
|
||||||
raise exc.HTTPBadRequest(explanation=e.format_message())
|
raise exc.HTTPBadRequest(explanation=e.format_message())
|
||||||
|
|
||||||
instance = common.get_instance(self.compute_api, context, server_id)
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
self.compute_api.swap_volume(context, instance, old_volume,
|
self.compute_api.swap_volume(context, instance, old_volume,
|
||||||
new_volume)
|
new_volume)
|
||||||
@@ -437,12 +438,13 @@ class VolumeAttachmentController(wsgi.Controller):
|
|||||||
def delete(self, req, server_id, id):
|
def delete(self, req, server_id, id):
|
||||||
"""Detach a volume from an instance."""
|
"""Detach a volume from an instance."""
|
||||||
context = req.environ['nova.context']
|
context = req.environ['nova.context']
|
||||||
context.can(va_policies.POLICY_ROOT % 'delete')
|
instance = common.get_instance(self.compute_api, context, server_id,
|
||||||
|
expected_attrs=['device_metadata'])
|
||||||
|
context.can(va_policies.POLICY_ROOT % 'delete',
|
||||||
|
target={'project_id': instance.project_id})
|
||||||
|
|
||||||
volume_id = id
|
volume_id = id
|
||||||
|
|
||||||
instance = common.get_instance(self.compute_api, context, server_id,
|
|
||||||
expected_attrs=['device_metadata'])
|
|
||||||
if instance.vm_state in (vm_states.SHELVED,
|
if instance.vm_state in (vm_states.SHELVED,
|
||||||
vm_states.SHELVED_OFFLOADED):
|
vm_states.SHELVED_OFFLOADED):
|
||||||
_check_request_version(req, '2.20', 'detach_volume',
|
_check_request_version(req, '2.20', 'detach_volume',
|
||||||
|
|||||||
@@ -62,7 +62,8 @@ IMAGE_UUID = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'
|
|||||||
|
|
||||||
def fake_get_instance(self, context, instance_id, expected_attrs=None,
|
def fake_get_instance(self, context, instance_id, expected_attrs=None,
|
||||||
cell_down_support=False):
|
cell_down_support=False):
|
||||||
return fake_instance.fake_instance_obj(context, **{'uuid': instance_id})
|
return fake_instance.fake_instance_obj(
|
||||||
|
context, id=1, uuid=instance_id, project_id=context.project_id)
|
||||||
|
|
||||||
|
|
||||||
def fake_get_volume(self, context, id):
|
def fake_get_volume(self, context, id):
|
||||||
@@ -1512,6 +1513,8 @@ class TestVolumeAttachPolicyEnforcementV21(test.NoDBTestCase):
|
|||||||
self.controller = volumes_v21.VolumeAttachmentController()
|
self.controller = volumes_v21.VolumeAttachmentController()
|
||||||
self.req = fakes.HTTPRequest.blank('')
|
self.req = fakes.HTTPRequest.blank('')
|
||||||
|
|
||||||
|
self.stub_out('nova.compute.api.API.get', fake_get_instance)
|
||||||
|
|
||||||
def _common_policy_check(self, rules, rule_name, func, *arg, **kwarg):
|
def _common_policy_check(self, rules, rule_name, func, *arg, **kwarg):
|
||||||
self.policy.set_rules(rules)
|
self.policy.set_rules(rules)
|
||||||
exc = self.assertRaises(
|
exc = self.assertRaises(
|
||||||
|
|||||||
Reference in New Issue
Block a user