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

@@ -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:
+        nodes = objects.Node.list(api.request.context, limit, marker_obj,
-            # NOTE(rloo) if instance_uuid is specified, the other query
+                                  sort_key=sort_key, sort_dir=sort_dir,
-            # parameters are ignored. Since there can be at most one node that
+                                  filters=filters)
-            # has this instance_uuid, we do not want to generate a 'next' link.
 
-            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
+        parameters = {'sort_key': sort_key, 'sort_dir': sort_dir}
-            # Collection.has_next()), a 'next' link will
+        if associated:
-            # be generated, which we don't want.
+            parameters['associated'] = associated
-            limit = 0
+        if maintenance:
-        else:
+            parameters['maintenance'] = maintenance
-            possible_filters = {
+        if retired:
-                'maintenance': maintenance,
+            parameters['retired'] = retired
-                '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
+        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.
 

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',

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)

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>`_.