From 49bff4c09acfdf195f40f911236b9ac5c6def3c5 Mon Sep 17 00:00:00 2001 From: Saad Zia Date: Thu, 28 May 2026 10:51:32 +0300 Subject: [PATCH 1/2] Security: Fix sensitive properties returned on volume targets Ensure that volume target properties are properly redacted when the user lacks the 'baremetal:volume:view_target_properties' policy in patch() similar to get_one(). Additionally, Ironic Core Security determined that the RBAC reliant control over who can see properties was insufficient, especially in standalone use cases, and such the field handling is now consistent with other sensitive field usage. Assisted-By: Claude Opus 4.6 Related-Bug: 2154564 Related-Bug: 2155049 Change-Id: I8366088731aa34da0e99941924c222ffed455c8b Signed-off-by: Saad Zia Signed-off-by: Julia Kreger --- ironic/api/controllers/v1/volume_target.py | 20 ++- .../api/controllers/v1/test_volume_target.py | 166 ++++++++++++++++++ ...sensitive-properties-041818005cac008f.yaml | 21 +++ 3 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/volume-target-redact-sensitive-properties-041818005cac008f.yaml diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index 21ebcad7a1..e7f477c85e 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -14,6 +14,7 @@ from http import client as http_client +from oslo_utils import strutils from oslo_utils import uuidutils from pecan import rest @@ -90,11 +91,18 @@ def convert_with_links(rpc_target, fields=None, sanitize=True): if not sanitize: return target - api_utils.sanitize_dict(target, fields) + target_sanitize(target, fields) return target +def target_sanitize(target, fields=None): + api_utils.sanitize_dict(target, fields) + if target.get('properties'): + target['properties'] = strutils.mask_dict_password( + target['properties'], "******") + + def list_convert_with_links(rpc_targets, limit, url, fields=None, detail=None, **kwargs): if detail: @@ -106,7 +114,7 @@ def list_convert_with_links(rpc_targets, limit, url, fields=None, limit=limit, url=url, fields=fields, - sanitize_func=api_utils.sanitize_dict, + sanitize_func=target_sanitize, **kwargs ) @@ -401,9 +409,15 @@ def patch(self, target_uuid, patch): new_target = api.request.rpcapi.update_volume_target( context, rpc_target, topic) - api_target = convert_with_links(new_target) notify.emit_end_notification(context, new_target, 'update', node_uuid=rpc_node.uuid) + + cdict = api.request.context.to_policy_values() + if not policy.check_policy('baremetal:volume:view_target_properties', + cdict, cdict): + self._redact_target_properties(new_target) + + api_target = convert_with_links(new_target) return api_target @METRICS.timer('VolumeTargetsController.delete') diff --git a/ironic/tests/unit/api/controllers/v1/test_volume_target.py b/ironic/tests/unit/api/controllers/v1/test_volume_target.py index 038f3cb58a..af33e2d18a 100644 --- a/ironic/tests/unit/api/controllers/v1/test_volume_target.py +++ b/ironic/tests/unit/api/controllers/v1/test_volume_target.py @@ -29,6 +29,7 @@ from ironic.api.controllers.v1 import notification_utils from ironic.api.controllers.v1 import utils as api_utils from ironic.common import exception +from ironic.common import policy from ironic.conductor import rpcapi from ironic import objects from ironic.objects import fields as obj_fields @@ -319,6 +320,91 @@ def test_sort_key_invalid(self): self.assertEqual('application/json', response.content_type) self.assertIn(invalid_key, response.json['error_message']) + @mock.patch.object(policy, 'check_policy', autospec=True) + def test_get_one_redact_properties(self, mock_check_policy): + """Properties are redacted when policy denies access.""" + target = obj_utils.create_test_volume_target( + self.context, node_id=self.node.id) + + def check_policy_side_effect(rule, target_dict, creds, + *args, **kwargs): + if rule == 'baremetal:volume:view_target_properties': + return False + return True + + mock_check_policy.side_effect = check_policy_side_effect + data = self.get_json('/volume/targets/%s' % target.uuid, + headers=self.headers) + self.assertIn('properties', data) + self.assertIn('redacted_contents', data['properties']) + self.assertNotIn('target_iqn', data['properties']) + + @mock.patch.object(policy, 'check_policy', autospec=True) + def test_get_one_no_redact_properties(self, mock_check_policy): + """Properties are visible when policy allows access.""" + target = obj_utils.create_test_volume_target( + self.context, node_id=self.node.id) + + def check_policy_side_effect(rule, target_dict, creds, + *args, **kwargs): + if rule == 'baremetal:volume:view_target_properties': + return True + return True + + mock_check_policy.side_effect = check_policy_side_effect + data = self.get_json('/volume/targets/%s' % target.uuid, + headers=self.headers) + self.assertIn('properties', data) + self.assertEqual({'target_iqn': 'iqn.foo'}, + data['properties']) + + @mock.patch.object(policy, 'check_policy', autospec=True) + def test_detail_redact_properties(self, mock_check_policy): + """Properties are redacted in detail listing.""" + obj_utils.create_test_volume_target( + self.context, node_id=self.node.id) + + def check_policy_side_effect(rule, target_dict, creds, + *args, **kwargs): + if rule == 'baremetal:volume:view_target_properties': + return False + return True + + mock_check_policy.side_effect = check_policy_side_effect + data = self.get_json('/volume/targets?detail=True', + headers=self.headers) + props = data['targets'][0]['properties'] + self.assertIn('redacted_contents', props) + self.assertNotIn('target_iqn', props) + + def test_get_one_masks_secret_properties(self): + """Password-like values in properties are always masked.""" + props = {'target_iqn': 'iqn.foo', + 'auth_password': 'supersecret'} + obj_utils.create_test_volume_target( + self.context, node_id=self.node.id, + properties=props) + data = self.get_json('/volume/targets/%s' + % dbutils.get_test_volume_target()['uuid'], + headers=self.headers) + self.assertEqual('iqn.foo', + data['properties']['target_iqn']) + self.assertEqual('******', + data['properties']['auth_password']) + + def test_detail_masks_secret_properties(self): + """Password-like values in properties are masked in listings.""" + props = {'target_iqn': 'iqn.foo', + 'auth_password': 'supersecret'} + obj_utils.create_test_volume_target( + self.context, node_id=self.node.id, + properties=props) + data = self.get_json('/volume/targets?detail=True', + headers=self.headers) + result_props = data['targets'][0]['properties'] + self.assertEqual('iqn.foo', result_props['target_iqn']) + self.assertEqual('******', result_props['auth_password']) + @mock.patch.object(api_utils, 'get_rpc_node', autospec=True) def test_get_all_by_node_name_ok(self, mock_get_rpc_node): # GET /v1/volume/targets specifying node_name - success @@ -727,6 +813,86 @@ def test_remove_uuid(self, mock_upd): self.assertTrue(response.json['error_message']) self.assertFalse(mock_upd.called) + @mock.patch.object(policy, 'check_policy', autospec=True) + @mock.patch.object(notification_utils, '_emit_api_notification', + autospec=True) + def test_patch_redact_properties(self, mock_notify, mock_check_policy, + mock_upd): + """Properties are redacted in PATCH response.""" + props = {'target_iqn': 'iqn.secret'} + mock_upd.return_value = self.target + mock_upd.return_value.properties = props + + def check_policy_side_effect(rule, target_dict, creds, + *args, **kwargs): + if rule == 'baremetal:volume:view_target_properties': + return False + return True + + mock_check_policy.side_effect = check_policy_side_effect + response = self.patch_json('/volume/targets/%s' + % self.target.uuid, + [{'path': '/extra/foo', + 'value': 'bar', + 'op': 'add'}], + headers=self.headers) + self.assertEqual(http_client.OK, response.status_code) + self.assertIn('redacted_contents', + response.json['properties']) + self.assertNotIn('target_iqn', + response.json['properties']) + mock_notify.assert_has_calls( + [mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.INFO, + obj_fields.NotificationStatus.START, + node_uuid=self.node.uuid), + mock.call(mock.ANY, mock.ANY, 'update', + obj_fields.NotificationLevel.INFO, + obj_fields.NotificationStatus.END, + node_uuid=self.node.uuid)]) + + @mock.patch.object(policy, 'check_policy', autospec=True) + def test_patch_no_redact_properties(self, mock_check_policy, + mock_upd): + """Properties are visible in PATCH response when allowed.""" + props = {'target_iqn': 'iqn.secret'} + mock_upd.return_value = self.target + mock_upd.return_value.properties = props + + def check_policy_side_effect(rule, target_dict, creds, + *args, **kwargs): + if rule == 'baremetal:volume:view_target_properties': + return True + return True + + mock_check_policy.side_effect = check_policy_side_effect + response = self.patch_json('/volume/targets/%s' + % self.target.uuid, + [{'path': '/extra/foo', + 'value': 'bar', + 'op': 'add'}], + headers=self.headers) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual(props, response.json['properties']) + + def test_patch_masks_secret_properties(self, mock_upd): + """Password-like values in properties are masked in PATCH.""" + props = {'target_iqn': 'iqn.foo', + 'auth_password': 'supersecret'} + mock_upd.return_value = self.target + mock_upd.return_value.properties = props + response = self.patch_json('/volume/targets/%s' + % self.target.uuid, + [{'path': '/extra/foo', + 'value': 'bar', + 'op': 'add'}], + headers=self.headers) + self.assertEqual(http_client.OK, response.status_code) + self.assertEqual('iqn.foo', + response.json['properties']['target_iqn']) + self.assertEqual('******', + response.json['properties']['auth_password']) + class TestPost(test_api_base.BaseApiTest): headers = {api_base.Version.string: str(api_v1.max_version())} diff --git a/releasenotes/notes/volume-target-redact-sensitive-properties-041818005cac008f.yaml b/releasenotes/notes/volume-target-redact-sensitive-properties-041818005cac008f.yaml new file mode 100644 index 0000000000..5d1a04b424 --- /dev/null +++ b/releasenotes/notes/volume-target-redact-sensitive-properties-041818005cac008f.yaml @@ -0,0 +1,21 @@ +--- +security: + - | + The volume target ``properties`` field can contain sensitive iSCSI + connection details, including CHAP authentication credentials. A + contributor identified that the ``baremetal:volume:view_target_properties`` + RBAC policy check was not applied to ``PATCH`` responses. During review, + the ironic core security team determined that RBAC policy enforcement + alone was insufficient since standalone deployments do not have RBAC + support. The fix was updated in line with code reviewer feedback to + add secondary scrubbing of the ``properties`` field in all cases using + ``mask_dict_password``, ensuring password-like values are masked in all + API responses unconditionally. +fixes: + - | + The ``PATCH`` endpoint for volume targets now applies the + ``baremetal:volume:view_target_properties`` RBAC policy check to its + response payload, consistent with the ``GET`` endpoints. Additionally, + the ``properties`` field is now scrubbed using ``mask_dict_password`` + to mask any password-like keys, providing defense-in-depth independent + of RBAC policy enforcement. From 7c00fdb8822a066fe7dd22d0360a1615486eb807 Mon Sep 17 00:00:00 2001 From: Riccardo Pittau Date: Mon, 8 Jun 2026 09:15:33 +0200 Subject: [PATCH 2/2] Fix kernel parameter parsing for quoted values and whitespace The kernel parameter validator introduced in commit c6c91d649 has three bugs in the KernelParameterTransformer class: 1. The quoted_value() method tries to access items[0].value but items[0] is already a plain string (returned by value_with_spaces transformer), not a lark Token. This causes all quoted parameter values (e.g. sshkey="ssh-rsa ... user@host") to fail with "'str' object has no attribute 'value'". 2. KernelParameter.__str__() uses self.value.value directly, bypassing ParameterValue.__str__() which wraps space-containing values in quotes. This breaks roundtrip rendering of quoted values. 3. parse() does not strip surrounding whitespace from the input, causing failures when config values have trailing spaces or newlines from template expansion or oslo.config. Assisted-By: Claude Opus 4.6 Change-Id: I9a398d557c3f87c070ffa10e317f0df613b98b6b Signed-off-by: Riccardo Pittau (cherry picked from commit a58fe7d548e3b874b7538d6272b2a73f3a0d4b94) (cherry picked from commit 87807ea7ce1424293ae6443de61fd4847a34619f) (cherry picked from commit 1908af6cd4ef02f9e33ba4ed6e8ffff8e0afbf79) --- ironic/common/kernel_parameters.py | 7 +- .../unit/common/test_kernel_parameters.py | 68 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/ironic/common/kernel_parameters.py b/ironic/common/kernel_parameters.py index ca0cc924ef..2af70d418e 100644 --- a/ironic/common/kernel_parameters.py +++ b/ironic/common/kernel_parameters.py @@ -66,7 +66,7 @@ class KernelParameter: def __str__(self): if len(self.value.value) > 0: - return f"{self.key.key}={self.value.value}" + return f"{self.key}={self.value}" return self.key.key @@ -100,7 +100,7 @@ def __str__(self): def parse(cls, command_line: str): try: cmd_line, init_args = \ - _divide_command_line_by_init_args(command_line) + _divide_command_line_by_init_args(command_line.strip()) tree = KernelParameterParser.parse(cmd_line) kcl = KernelParameterTransformer().transform(tree) return KernelCommandLine(kcl.parameters, init_args) @@ -143,8 +143,7 @@ def value(self, items): return ParameterValue(items[0]) def quoted_value(self, items): - # Strip " characters from literal. - return items[0].value[1:-1] + return items[0] def bare_value(self, items): return items[0].value diff --git a/ironic/tests/unit/common/test_kernel_parameters.py b/ironic/tests/unit/common/test_kernel_parameters.py index f64f46eb51..83e1f1d738 100644 --- a/ironic/tests/unit/common/test_kernel_parameters.py +++ b/ironic/tests/unit/common/test_kernel_parameters.py @@ -205,6 +205,54 @@ def test_sanitize_kernel_command_line( )], }, "some init args") ), + annotate( + "Quoted value with spaces (sshkey pattern)", + 'sshkey="ssh-rsa AAAAB3NzaC1yc2E= root@host"', + kp.KernelCommandLine({ + 'sshkey': [kp.KernelParameter( + kp.ParameterKey('sshkey'), + kp.ParameterValue( + 'ssh-rsa AAAAB3NzaC1yc2E= root@host'), + )], + }, "") + ), + annotate( + "Quoted value with SSH key and following parameters", + ('nofb nomodeset vga=normal ipa-insecure=1 ' + 'sshkey="ssh-rsa AAAAB3NzaC+/C1yc2E= root@host.example.com" ' + 'rd.net.timeout.carrier=30 ip=dhcp'), + kp.KernelCommandLine({ + 'nofb': [kp.KernelParameter( + kp.ParameterKey('nofb'), + kp.ParameterValue(''), + )], + 'nomodeset': [kp.KernelParameter( + kp.ParameterKey('nomodeset'), + kp.ParameterValue(''), + )], + 'vga': [kp.KernelParameter( + kp.ParameterKey('vga'), + kp.ParameterValue('normal'), + )], + 'ipa-insecure': [kp.KernelParameter( + kp.ParameterKey('ipa-insecure'), + kp.ParameterValue('1'), + )], + 'sshkey': [kp.KernelParameter( + kp.ParameterKey('sshkey'), + kp.ParameterValue( + 'ssh-rsa AAAAB3NzaC+/C1yc2E= root@host.example.com'), + )], + 'rd.net.timeout.carrier': [kp.KernelParameter( + kp.ParameterKey('rd.net.timeout.carrier'), + kp.ParameterValue('30'), + )], + 'ip': [kp.KernelParameter( + kp.ParameterKey('ip'), + kp.ParameterValue('dhcp'), + )], + }, "") + ), ) @unpack def test_kernel_command_line_parsing( @@ -228,3 +276,23 @@ def test_invalid_kernel_command_lines_fail_to_parse( self.assertRaises(InvalidParameterValue, kp.KernelCommandLine.parse, command_line) + + @data( + annotate( + "trailing space", + "quiet ro ", + ), + annotate( + "leading space", + " quiet ro", + ), + annotate( + "leading and trailing spaces", + " quiet ro ", + ), + ) + @unpack + def test_parse_strips_surrounding_whitespace(self, command_line: str): + result = kp.KernelCommandLine.parse(command_line) + self.assertEqual('quiet', list(result.parameters.keys())[0]) + self.assertEqual('ro', list(result.parameters.keys())[1])