Browse Source

Merge "Secure RBAC - Efficent node santiziation" into stable/wallaby

changes/35/795135/1
Zuul 1 week ago
committed by Gerrit Code Review
parent
commit
925ebb825f
3 changed files with 70 additions and 16 deletions
  1. +28
    -16
      ironic/api/controllers/v1/node.py
  2. +35
    -0
      ironic/tests/unit/api/controllers/v1/test_node.py
  3. +7
    -0
      releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml

+ 28
- 16
ironic/api/controllers/v1/node.py 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:
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()


+ 35
- 0
ironic/tests/unit/api/controllers/v1/test_node.py 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)


+ 7
- 0
releasenotes/notes/secure-rbac-policy-sanitization-performance-dc7886952144bb04.yaml 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.

Loading…
Cancel
Save