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/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/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/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]) 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.