summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--ironic/api/controllers/v1/node.py99
-rw-r--r--ironic/db/sqlalchemy/api.py2
-rw-r--r--ironic/tests/unit/api/controllers/v1/test_node.py75
-rw-r--r--releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml20
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>`_.