From f9eb7abe8240abad59b895cb41dd9e69124b33a2 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 6 May 2021 09:38:10 -0700 Subject: [PATCH] Secure RBAC - Efficent node santiziation An investigation of performance issues in Ironic revealed that the policy checking was performing extra un-needed work which performed excess computational overhead when parsing the result data. In this specific case, the Secure RBAC work added some additional policy checks around individual the fields. Change-Id: I77b6e0e6c721f2ff1f8b9f511acde97fcdb21a39 Story: 2008885 Task: 42432 (cherry picked from commit 6cd6457479803ecd1d9ec271a868a565fab244e8) --- ironic/api/controllers/v1/node.py | 44 ++++++++++++------- .../unit/api/controllers/v1/test_node.py | 35 +++++++++++++++ ...tization-performance-dc7886952144bb04.yaml | 7 +++ 3 files changed, 70 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index ba36d261c6..7c8b868476 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1366,6 +1366,9 @@ def node_sanitize(node, fields): if lessee: target_dict['node.lessee'] = lessee + # Scrub the dictionary's contents down to what was requested. + api_utils.sanitize_dict(node, fields) + # NOTE(tenbrae): the 'show_password' policy setting name exists for # legacy purposes and can not be changed. Changing it will # cause upgrade problems for any operators who have @@ -1387,40 +1390,48 @@ def node_sanitize(node, fields): evaluate_additional_policies = not policy.check_policy( "baremetal:node:get:filter_threshold", target_dict, cdict) + + node_keys = node.keys() + if evaluate_additional_policies: # NOTE(TheJulia): The net effect of this is that by default, # at least matching common/policy.py defaults. is these should # be stripped out. - if not policy.check("baremetal:node:get:last_error", - target_dict, cdict): + if ('last_error' in node_keys + and not policy.check("baremetal:node:get:last_error", + target_dict, cdict)): # Guard the last error from being visible as it can contain # hostnames revealing infrastucture internal details. node['last_error'] = ('** Value Redacted - Requires ' 'baremetal:node:get:last_error ' 'permission. **') - if not policy.check("baremetal:node:get:reservation", - target_dict, cdict): + if ('reservation' in node_keys + and not policy.check("baremetal:node:get:reservation", + target_dict, cdict)): # Guard conductor names from being visible. node['reservation'] = ('** Redacted - requires baremetal:' 'node:get:reservation permission. **') - if not policy.check("baremetal:node:get:driver_internal_info", - target_dict, cdict): + if ('driver_internal_info' in node_keys + and not policy.check("baremetal:node:get:driver_internal_info", + target_dict, cdict)): # Guard conductor names from being visible. node['driver_internal_info'] = { 'content': '** Redacted - Requires baremetal:node:get:' 'driver_internal_info permission. **'} - if not policy.check("baremetal:node:get:driver_info", - target_dict, cdict): + + if 'driver_info' in node_keys: + if (evaluate_additional_policies + and not policy.check("baremetal:node:get:driver_info", + target_dict, cdict)): # Guard infrastructure intenral details from being visible. node['driver_info'] = { 'content': '** Redacted - requires baremetal:node:get:' 'driver_info permission. **'} + if not show_driver_secrets: + node['driver_info'] = strutils.mask_dict_password( + node['driver_info'], "******") - if not show_driver_secrets and node.get('driver_info'): - node['driver_info'] = strutils.mask_dict_password( - node['driver_info'], "******") - - if not show_instance_secrets and node.get('instance_info'): + if not show_instance_secrets and 'instance_info' in node_keys: node['instance_info'] = strutils.mask_dict_password( node['instance_info'], "******") # NOTE(tenbrae): agent driver may store a swift temp_url on the @@ -1434,11 +1445,12 @@ def node_sanitize(node, fields): if node.get('driver_internal_info', {}).get('agent_secret_token'): node['driver_internal_info']['agent_secret_token'] = "******" - update_state_in_older_versions(node) + if 'provision_state' in node_keys: + # Update legacy state data for provision state, but only if + # the key is present. + update_state_in_older_versions(node) hide_fields_in_newer_versions(node) - api_utils.sanitize_dict(node, fields) - show_states_links = ( api_utils.allow_links_node_states_and_driver_properties()) show_portgroups = api_utils.allow_portgroups_subcontrollers() diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 302da10551..05b22c42b9 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -17,6 +17,7 @@ import datetime from http import client as http_client import json import os +import sys import tempfile from unittest import mock from urllib import parse as urlparse @@ -141,6 +142,40 @@ class TestListNodes(test_api_base.BaseApiTest): self.assertNotIn('lessee', data['nodes'][0]) self.assertNotIn('network_data', data['nodes'][0]) + @mock.patch.object(policy, 'check', autospec=True) + @mock.patch.object(policy, 'check_policy', autospec=True) + def test_one_field_specific_santization(self, mock_check_policy, + mock_check): + py_ver = sys.version_info + if py_ver.major == 3 and py_ver.minor == 6: + self.skipTest('Test fails to work on python 3.6 when ' + 'matching mock.ANY.') + obj_utils.create_test_node(self.context, + chassis_id=self.chassis.id, + last_error='meow') + mock_check_policy.return_value = False + data = self.get_json( + '/nodes?fields=uuid,provision_state,maintenance,instance_uuid,' + 'last_error', + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertIn('uuid', data['nodes'][0]) + self.assertIn('provision_state', data['nodes'][0]) + self.assertIn('maintenance', data['nodes'][0]) + self.assertIn('instance_uuid', data['nodes'][0]) + self.assertNotIn('driver_info', data['nodes'][0]) + mock_check_policy.assert_has_calls([ + mock.call('baremetal:node:get:filter_threshold', + mock.ANY, mock.ANY)]) + mock_check.assert_has_calls([ + mock.call('is_admin', mock.ANY, mock.ANY), + mock.call('show_password', mock.ANY, mock.ANY), + mock.call('show_instance_secrets', mock.ANY, mock.ANY), + # Last error is populated above and should trigger a check. + mock.call('baremetal:node:get:last_error', mock.ANY, mock.ANY), + mock.call().__bool__(), + mock.call().__bool__(), + ]) + def test_get_one(self): node = obj_utils.create_test_node(self.context, chassis_id=self.chassis.id) diff --git a/releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml b/releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml new file mode 100644 index 0000000000..af3751237b --- /dev/null +++ b/releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes sub-optimal Ironic API performance where Secure RBAC related + field level policy checks were executing without first checking + if there were field results. This helps improve API performance when + only specific columns have been requested by the API consumer.