From be3c153d56e7f94a128438adcc5c717c50336bed Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
Date: Wed, 16 Jun 2021 10:26:21 -0700
Subject: [PATCH] Fix node detail instance_uuid request handling
The instance_uuid handling on the detailed node information
endpoint of the api (/v1/nodes/detail?instance_uuid=<uuid>),
which is used by services such as Nova for explicit node status
lookups, previously had special conditional logic surrounding it
which skipped the inclusion of the API requestor project-id, from
being incorporated into the database query.
Ultimately, this allowed an authenticated user to obtain a partially
redacted node entry where sensitive informational fields were scrubbed
from the response payload.
With this fix, queries for an explicit instance_uuid now follow the
standard path inside the Ironic API to the database which includes
inclusion of a requestor Project-ID if required by configured policy.
Change-Id: I9bfa5a54e02c8a1e9c8cad6b9acdbad6ab62bef3
Story: 2008976
Task: 42620
---
ironic/api/controllers/v1/node.py | 99 ++++++++-----------
ironic/db/sqlalchemy/api.py | 2 +-
.../unit/api/controllers/v1/test_node.py | 75 ++++++++++++++
...stance-info-behavior-1375914a30621eca.yaml | 20 ++++
4 files changed, 138 insertions(+), 58 deletions(-)
create mode 100644 releasenotes/notes/correct-detailed-instance-info-behavior-1375914a30621eca.yaml
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index da7c3e5d25..760ef82611 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -1761,75 +1761,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
- 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 = objects.Node.list(api.request.context, limit, marker_obj,
+ sort_key=sort_key, sort_dir=sort_dir,
+ filters=filters)
- nodes = self._get_nodes_by_instance(instance_uuid)
+ # Special filtering on results based on conductor field
+ if conductor:
+ nodes = self._filter_by_conductor(nodes, conductor)
- # NOTE(rloo) if limit==1 and len(nodes)==1 (see
- # Collection.has_next()), a 'next' link will
- # be generated, which we don't want.
- 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
+ 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 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
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 6f027c96af..297b30d277 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 085623796a..3408bc3b6e 100644
--- a/ironic/tests/unit/api/controllers/v1/test_node.py
+++ b/ironic/tests/unit/api/controllers/v1/test_node.py
@@ -755,6 +755,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 0000000000..6d98cca559
--- /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>`_.