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 6cd6457479)
This commit is contained in:
Julia Kreger 2021-05-06 09:38:10 -07:00
parent c7af969389
commit f9eb7abe82
3 changed files with 70 additions and 16 deletions

View File

@ -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 and node.get('driver_info'):
if not show_driver_secrets:
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'] = "******"
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()

View File

@ -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)

View File

@ -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.