From 060fc497ac11375f83161a673de836fbfe2249d0 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 20 Apr 2023 17:09:43 +0200 Subject: [PATCH] Add tests to check attachments are secure A vulnerability was detected in Cinder that could allow users to access other people's volumes. The solution was to limit some of the operations on attached volumes to only OpenStack services. This patch adds some negative tests to check that a user cannot directly call Cinder to detach a volume, force detach it, terminate its connection, or delete its attachment. Depends-On: I612905a1bf4a1706cce913c0d8a6df7a240d599a Relates-Bug: #2004555 Change-Id: Ice6532ce7943e9a9363e443515946865541d09c2 --- .../api/volume/admin/test_volumes_actions.py | 6 +- tempest/api/volume/test_volumes_negative.py | 250 ++++++++++++++++++ .../lib/api_schema/response/volume/volumes.py | 1 + .../services/volume/v3/attachments_client.py | 7 + .../lib/services/volume/v3/volumes_client.py | 13 +- 5 files changed, 273 insertions(+), 4 deletions(-) diff --git a/tempest/api/volume/admin/test_volumes_actions.py b/tempest/api/volume/admin/test_volumes_actions.py index ecddfbaf6..b6e9f321b 100644 --- a/tempest/api/volume/admin/test_volumes_actions.py +++ b/tempest/api/volume/admin/test_volumes_actions.py @@ -83,7 +83,7 @@ class VolumesActionsTest(base.BaseVolumeAdminTest): server_id = self.create_server()['id'] volume_id = self.create_volume()['id'] - # Attach volume + # Request Cinder to map & export volume (it's not attached to instance) self.volumes_client.attach_volume( volume_id, instance_uuid=server_id, @@ -101,7 +101,9 @@ class VolumesActionsTest(base.BaseVolumeAdminTest): waiters.wait_for_volume_resource_status(self.volumes_client, volume_id, 'error') - # Force detach volume + # The force detach volume calls works because the volume is not really + # connected to the instance (it is safe), otherwise it would be + # rejected for security reasons (bug #2004555). self.admin_volume_client.force_detach_volume( volume_id, connector=None, attachment_id=attachment['attachment_id']) diff --git a/tempest/api/volume/test_volumes_negative.py b/tempest/api/volume/test_volumes_negative.py index d9b8430a2..2a47b0528 100644 --- a/tempest/api/volume/test_volumes_negative.py +++ b/tempest/api/volume/test_volumes_negative.py @@ -14,6 +14,7 @@ # under the License. import io +from unittest import mock from tempest.api.volume import base from tempest.common import utils @@ -345,3 +346,252 @@ class VolumesNegativeTest(base.BaseVolumeTest): self.assertRaises(lib_exc.BadRequest, self.create_volume, imageRef=image['id']) + + +class CommonVolumeConnectionSecure(base.BaseVolumeTest): + def _call_with_fake_service_token(self, valid_token, + client, method_name, *args, **kwargs): + """Call client method with non-service service token + + Add a service token header that can be a valid normal user token (which + won't have the service role) or an invalid token altogether. + """ + original_raw_request = client.raw_request + + def raw_request(url, method, headers=None, body=None, chunked=False, + log_req_body=None): + token = headers['X-Auth-Token'] + if not valid_token: + token = token[:-1] + ('a' if token[-1] != 'a' else 'b') + headers['X-Service-Token'] = token + return original_raw_request(url, method, headers=headers, + body=body, chunked=chunked, + log_req_body=log_req_body) + + client_method = getattr(client, method_name) + with mock.patch.object(client, 'raw_request', raw_request): + return client_method(*args, **kwargs) + + def _create_instance_and_attach_volume(self): + server_id = self.create_server()['id'] + volume_id = self.create_volume()['id'] + self.attach_volume(server_id, volume_id) + attachment_id = self.volumes_client.show_volume( + volume_id)['volume']['attachments'][0]['attachment_id'] + return server_id, volume_id, attachment_id + + +class AttachedVolumesSecureNegativeTest(CommonVolumeConnectionSecure): + """Tests to verify that volume attachments are secure (bug #2004555). + + A vulnerability was detected in Cinder that could allow users to access + other people's volumes. + + The solution was to limit some of the operations on attached volumes to + only OpenStack services. + + Errors returned by users making the REST API calls depend on how the + services are configured and what call we are making: + + Possible status code errors are: + + - 403 Forbidden: Policy is not allowing the call + - 401 Unauthorized: Token is not valid + - 409 Conflict: Cinder code is not allowing the call + """ + credentials = ['primary', 'admin'] + + @classmethod + def setup_clients(cls): + super().setup_clients() + cls.admin_volume_client = cls.os_admin.volumes_client_latest + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('c266ab09-9b7f-4eb7-a5cb-1abdc681c72e') + def test_force_detach_volume_rejected(self): + """Test user call to force detach volume is rejected (bug #2004555)""" + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + + # Reset volume's status to error + self.admin_volume_client.reset_volume_status(vol_id, + status='error') + waiters.wait_for_volume_resource_status(self.volumes_client, + vol_id, 'error') + # For the cleanup, we need to reset the volume status to in-use before + # the other cleanup steps try to detach it. + self.addCleanup(waiters.wait_for_volume_resource_status, + self.volumes_client, vol_id, 'in-use') + self.addCleanup(self.admin_volume_client.reset_volume_status, + vol_id, status='in-use') + + self.assertRaises( + (lib_exc.Forbidden, lib_exc.Conflict), + self.admin_volume_client.force_detach_volume, + vol_id, connector=None, + attachment_id=att_id) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('61d3f8c2-8cbb-4ef6-8e25-31986a3a2b73') + def test_force_detach_volume_rejected_incorrect_service_token(self): + """Test faking of service token on call to force detach volume + + Rejection of the user request call to force detach cannot be + circumvented by providing a fake service token or a service token that + doesn't have the service role (bug #2004555) + """ + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + + # Reset volume's status to error + self.admin_volume_client.reset_volume_status(vol_id, status='error') + waiters.wait_for_volume_resource_status(self.volumes_client, + vol_id, 'error') + # For the cleanup, we need to reset the volume status to in-use before + # the other cleanup steps try to detach it. + self.addCleanup(waiters.wait_for_volume_resource_status, + self.volumes_client, vol_id, 'in-use') + self.addCleanup(self.admin_volume_client.reset_volume_status, + vol_id, status='in-use') + + for valid_token in (True, False): + valid_exceptions = [lib_exc.Forbidden, lib_exc.Conflict] + if not valid_token: + valid_exceptions.append(lib_exc.Unauthorized) + self.assertRaises( + tuple(valid_exceptions), + self._call_with_fake_service_token, + valid_token, + self.admin_volume_client, + 'force_detach_volume', + vol_id, connector=None, + attachment_id=att_id) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('dac4da00-6b3c-403c-9562-0614e2750d41') + def test_detach_volume_rejected(self): + """Test user call to detach volume is rejected (bug #2004555)""" + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + self.assertRaises( + (lib_exc.Forbidden, lib_exc.Conflict), + self.volumes_client.detach_volume, + vol_id) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('9098110d-42d6-4a5a-b556-3824db202775') + def test_detach_volume_rejected_incorrect_service_token(self): + """Test faking of service token on call to force detach volume + + Rejection of the user request call to force detach cannot be + circumvented by providing a fake service token or a service token that + doesn't have the service role (bug #2004555) + """ + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + for valid_token in (True, False): + valid_exceptions = [lib_exc.Forbidden, lib_exc.Conflict] + if not valid_token: + valid_exceptions.append(lib_exc.Unauthorized) + self.assertRaises( + tuple(valid_exceptions), + self._call_with_fake_service_token, + valid_token, + self.volumes_client, + 'detach_volume', + vol_id) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('a0371eaa-0fae-4d1f-bbc9-8ce6463ced10') + def test_terminate_connection_rejected(self): + """Test user call to detach volume is rejected (bug #2004555)""" + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + self.assertRaises( + (lib_exc.Forbidden, lib_exc.Conflict), + self.volumes_client.terminate_connection, + vol_id, connector={}) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('6ac8bf9c-f0e8-4516-9255-6facfec7e6e2') + def test_terminate_connection_rejected_incorrect_service_token(self): + """Test faking of service token on call to force detach volume + + Rejection of the user request call to force detach cannot be + circumvented by providing a fake service token or a service token that + doesn't have the service role (bug #2004555) + """ + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + for valid_token in (True, False): + valid_exceptions = [lib_exc.Forbidden, lib_exc.Conflict] + if not valid_token: + valid_exceptions.append(lib_exc.Unauthorized) + self.assertRaises( + tuple(valid_exceptions), + self._call_with_fake_service_token, + valid_token, + self.volumes_client, + 'terminate_connection', + vol_id, connector={}) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('b13ca216-6a03-4c06-924c-21307b624263') + def test_detach_mismatch(self): + """Test user call to detach with mismatch is rejected (bug #2004555)""" + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + vol_id2 = self.create_volume()['id'] + self.attach_volume(vm_id, vol_id2) + att_id2 = self.volumes_client.show_volume( + vol_id2)['volume']['attachments'][0]['attachment_id'] + + self.assertRaises( + (lib_exc.Forbidden, lib_exc.BadRequest), + self.volumes_client.detach_volume, + vol_id, attachment_id=att_id2) + + +class AttachmentsSecureNegativeTest(CommonVolumeConnectionSecure): + """Tests to verify that volume attachments are secure (bug #2004555).""" + volume_min_microversion = '3.27' + volume_max_microversion = 'latest' + + @classmethod + def setup_clients(cls): + super().setup_clients() + cls.attachments_client = cls.os_primary.attachments_client_latest + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('13c4007f-dd23-487b-9800-f93f3379c106') + def test_delete_attachment_rejected(self): + """Test user call to detach volume is rejected (bug #2004555)""" + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + self.assertRaises( + (lib_exc.Forbidden, lib_exc.Conflict), + self.attachments_client.delete_attachment, + att_id) + + @decorators.attr(type=['negative']) + @utils.services('compute') + @decorators.idempotent_id('ff7992e6-3043-4ba3-ae95-4bac5ee4fb68') + def test_delete_attachment_rejected_incorrect_service_token(self): + """Test faking of service token on call to force detach volume + + Rejection of the user request call to force detach cannot be + circumvented by providing a fake service token or a service token that + doesn't have the service role (bug #2004555) + """ + vm_id, vol_id, att_id = self._create_instance_and_attach_volume() + for valid_token in (True, False): + valid_exceptions = [lib_exc.Forbidden, lib_exc.Conflict] + if not valid_token: + valid_exceptions.append(lib_exc.Unauthorized) + self.assertRaises( + tuple(valid_exceptions), + self._call_with_fake_service_token, + valid_token, + self.attachments_client, + 'delete_attachment', + att_id) diff --git a/tempest/lib/api_schema/response/volume/volumes.py b/tempest/lib/api_schema/response/volume/volumes.py index 4f445263c..900e5ef00 100644 --- a/tempest/lib/api_schema/response/volume/volumes.py +++ b/tempest/lib/api_schema/response/volume/volumes.py @@ -295,6 +295,7 @@ show_volume_summary = { attach_volume = {'status_code': [202]} set_bootable_volume = {'status_code': [200]} detach_volume = {'status_code': [202]} +terminate_connection = {'status_code': [202]} reserve_volume = {'status_code': [202]} unreserve_volume = {'status_code': [202]} extend_volume = {'status_code': [202]} diff --git a/tempest/lib/services/volume/v3/attachments_client.py b/tempest/lib/services/volume/v3/attachments_client.py index 5e448f715..303341e87 100644 --- a/tempest/lib/services/volume/v3/attachments_client.py +++ b/tempest/lib/services/volume/v3/attachments_client.py @@ -26,3 +26,10 @@ class AttachmentsClient(base_client.BaseClient): body = json.loads(body) self.expected_success(200, resp.status) return rest_client.ResponseBody(resp, body) + + def delete_attachment(self, attachment_id): + """Delete volume attachment.""" + url = "attachments/%s" % (attachment_id) + resp, body = self.delete(url) + self.expected_success(200, resp.status) + return rest_client.ResponseBody(resp, body) diff --git a/tempest/lib/services/volume/v3/volumes_client.py b/tempest/lib/services/volume/v3/volumes_client.py index ad8bd7181..1265baf6a 100644 --- a/tempest/lib/services/volume/v3/volumes_client.py +++ b/tempest/lib/services/volume/v3/volumes_client.py @@ -205,14 +205,23 @@ class VolumesClient(base_client.BaseClient): self.validate_response(schema.set_bootable_volume, resp, body) return rest_client.ResponseBody(resp, body) - def detach_volume(self, volume_id): + def detach_volume(self, volume_id, **kwargs): """Detaches a volume from an instance.""" - post_body = json.dumps({'os-detach': {}}) + post_body = json.dumps({'os-detach': kwargs}) url = 'volumes/%s/action' % (volume_id) resp, body = self.post(url, post_body) self.validate_response(schema.detach_volume, resp, body) return rest_client.ResponseBody(resp, body) + def terminate_connection(self, volume_id, connector): + """Detaches a volume from an instance.""" + post_body = json.dumps( + {'os-terminate_connection': {'connector': connector}}) + url = 'volumes/%s/action' % (volume_id) + resp, body = self.post(url, post_body) + self.validate_response(schema.terminate_connection, resp, body) + return rest_client.ResponseBody(resp, body) + def reserve_volume(self, volume_id): """Reserves a volume.""" post_body = json.dumps({'os-reserve': {}}) -- 2.38.1