diff options
| -rw-r--r-- | ironic/api/controllers/v1/node.py | 99 | ||||
| -rw-r--r-- | ironic/db/sqlalchemy/api.py | 2 | ||||
| -rw-r--r-- | ironic/tests/unit/api/controllers/v1/test_node.py | 75 | ||||
| -rw-r--r-- | releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml | 20 |
4 files changed, 138 insertions, 58 deletions
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 7c8b86847..c0daa1945 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1758,75 +1758,60 @@ class NodesController(rest.RestController): # The query parameters for the 'next' URL parameters = {} + possible_filters = { + 'maintenance': maintenance, + 'chassis_uuid': chassis_uuid, + 'associated': associated, + 'provision_state': provision_state, + 'driver': driver, + 'resource_class': resource_class, + 'fault': fault, + 'conductor_group': conductor_group, + 'owner': owner, + 'lessee': lessee, + 'project': project, + 'description_contains': description_contains, + 'retired': retired, + 'instance_uuid': instance_uuid + } + filters = {} + for key, value in possible_filters.items(): + if value is not None: + filters[key] = value + + nodes = objects.Node.list(api.request.context, limit, marker_obj, + sort_key=sort_key, sort_dir=sort_dir, + filters=filters) + + # Special filtering on results based on conductor field + if conductor: + nodes = self._filter_by_conductor(nodes, conductor) + + parameters = {'sort_key': sort_key, 'sort_dir': sort_dir} + if associated: + parameters['associated'] = associated + if maintenance: + parameters['maintenance'] = maintenance + if retired: + parameters['retired'] = retired + if detail is not None: + parameters['detail'] = detail if instance_uuid: - # NOTE(rloo) if instance_uuid is specified, the other query - # parameters are ignored. Since there can be at most one node that - # has this instance_uuid, we do not want to generate a 'next' link. - - nodes = self._get_nodes_by_instance(instance_uuid) - # NOTE(rloo) if limit==1 and len(nodes)==1 (see # Collection.has_next()), a 'next' link will # be generated, which we don't want. + # NOTE(TheJulia): This is done after the query as + # instance_uuid is a unique constraint in the DB + # and we cannot pass a limit of 0 to sqlalchemy + # and expect a response. limit = 0 - else: - possible_filters = { - 'maintenance': maintenance, - 'chassis_uuid': chassis_uuid, - 'associated': associated, - 'provision_state': provision_state, - 'driver': driver, - 'resource_class': resource_class, - 'fault': fault, - 'conductor_group': conductor_group, - 'owner': owner, - 'lessee': lessee, - 'project': project, - 'description_contains': description_contains, - 'retired': retired, - } - filters = {} - for key, value in possible_filters.items(): - if value is not None: - filters[key] = value - - nodes = objects.Node.list(api.request.context, limit, marker_obj, - sort_key=sort_key, sort_dir=sort_dir, - filters=filters) - - # Special filtering on results based on conductor field - if conductor: - nodes = self._filter_by_conductor(nodes, conductor) - - parameters = {'sort_key': sort_key, 'sort_dir': sort_dir} - if associated: - parameters['associated'] = associated - if maintenance: - parameters['maintenance'] = maintenance - if retired: - parameters['retired'] = retired - - if detail is not None: - parameters['detail'] = detail return node_list_convert_with_links(nodes, limit, url=resource_url, fields=fields, **parameters) - def _get_nodes_by_instance(self, instance_uuid): - """Retrieve a node by its instance uuid. - - It returns a list with the node, or an empty list if no node is found. - """ - try: - node = objects.Node.get_by_instance_uuid(api.request.context, - instance_uuid) - return [node] - except exception.InstanceNotFound: - return [] - def _check_names_acceptable(self, names, error_msg): """Checks all node 'name's are acceptable, it does not return a value. diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 2029d8942..df7267776 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -316,7 +316,7 @@ class Connection(api.Connection): _NODE_QUERY_FIELDS = {'console_enabled', 'maintenance', 'retired', 'driver', 'resource_class', 'provision_state', 'uuid', 'id', 'fault', 'conductor_group', - 'owner', 'lessee'} + 'owner', 'lessee', 'instance_uuid'} _NODE_IN_QUERY_FIELDS = {'%s_in' % field: field for field in ('uuid', 'provision_state')} _NODE_NON_NULL_FILTERS = {'associated': 'instance_uuid', diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 05b22c42b..d5a7bfdd7 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -736,6 +736,81 @@ class TestListNodes(test_api_base.BaseApiTest): self.assertIn('retired_reason', data['nodes'][0]) self.assertIn('network_data', data['nodes'][0]) + def test_detail_instance_uuid(self): + instance_uuid = '6eccd391-961c-4da5-b3c5-e2fa5cfbbd9d' + node = obj_utils.create_test_node( + self.context, + instance_uuid=instance_uuid) + data = self.get_json( + '/nodes/detail?instance_uuid=%s' % instance_uuid, + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(1, len(data['nodes'])) + self.assertEqual(node.uuid, data['nodes'][0]["uuid"]) + expected_fields = [ + 'name', 'driver', 'driver_info', 'extra', 'chassis_uuid', + 'reservation', 'maintenance', 'console_enabled', + 'target_power_state', 'target_provision_state', + 'provision_updated_at', 'inspection_finished_at', + 'inspection_started_at', 'raid_config', 'target_raid_config', + 'network_interface', 'resource_class', 'owner', 'lessee', + 'storage_interface', 'traits', 'automated_clean', + 'conductor_group', 'protected', 'protected_reason', + 'retired', 'retired_reason', 'allocation_uuid', 'network_data' + ] + + for field in expected_fields: + self.assertIn(field, data['nodes'][0]) + for field in api_utils.V31_FIELDS: + self.assertIn(field, data['nodes'][0]) + # never expose the chassis_id + self.assertNotIn('chassis_id', data['nodes'][0]) + self.assertNotIn('allocation_id', data['nodes'][0]) + # no pagination marker should be present + self.assertNotIn('next', data) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_instance_uuid_project_not_match(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + instance_uuid = '6eccd391-961c-4da5-b3c5-e2fa5cfbbd9d' + requestor_uuid = '46c0bf8a-846d-49a5-9724-5a61a5efa6bf' + obj_utils.create_test_node( + self.context, + owner='97879042-c0bf-4216-882a-66a7cbf2bd74', + instance_uuid=instance_uuid) + data = self.get_json( + '/nodes/detail?instance_uuid=%s' % instance_uuid, + headers={'X-Project-ID': requestor_uuid, + api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(0, len(data['nodes'])) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_instance_uuid_project_match(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + instance_uuid = '6eccd391-961c-4da5-b3c5-e2fa5cfbbd9d' + requestor_uuid = '46c0bf8a-846d-49a5-9724-5a61a5efa6bf' + node = obj_utils.create_test_node( + self.context, + owner=requestor_uuid, + instance_uuid=instance_uuid) + data = self.get_json( + '/nodes/detail?instance_uuid=%s' % instance_uuid, + headers={'X-Project-ID': requestor_uuid, + api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(1, len(data['nodes'])) + # Assert we did get the node and it matched. + self.assertEqual(node.uuid, data['nodes'][0]["uuid"]) + self.assertEqual(node.owner, data['nodes'][0]["owner"]) + def test_detail_using_query(self): node = obj_utils.create_test_node(self.context, chassis_id=self.chassis.id) diff --git a/releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml b/releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml new file mode 100644 index 000000000..6d98cca55 --- /dev/null +++ b/releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml @@ -0,0 +1,20 @@ +--- +security: + - | + Fixes an issue with the ``/v1/nodes/detail`` endpoint where an + authenticated user could explicitly ask for an ``instance_uuid`` lookup + and the associated node would be returned to the user with sensitive + fields redacted in the result payload if the user did not explicitly have + ``owner`` or ``lessee`` permissions over the node. This is considered a + low-impact low-risk issue as it requires the API consumer to already know + the UUID value of the associated instance, and the returned information + is mainly metadata in nature. More information can be found in + `Storyboard story 2008976 <https://storyboard.openstack.org/#!/story/2008976>`_. +fixes: + - | + Fixes an issue with the ``/v1/nodes/detail`` endpoint where requests + for an explicit ``instance_uuid`` match would not follow the standard + query handling path and thus not be filtered based on policy determined + access level and node level ``owner`` or ``lessee`` fields appropriately. + Additional information can be found in + `story 2008976 <https://storyboard.openstack.org/#!/story/2008976>`_. |