From 005f21c0df8634563f1e7eac7bb1a6b00df9ea4f Mon Sep 17 00:00:00 2001 From: Duc Truong Date: Wed, 15 Feb 2023 20:40:24 +0000 Subject: [PATCH] Fix auth_protocol and priv_protocol for SNMP v3 SNMP driver was using the wrong dictionary key to retrieve auth_protocol and priv_protocol from driver info. As a result, the SNMP client was created with empty strings for both those fields. Any nodes configured to use SNMP v3 with those fields failed because the SNMP driver was unable to perform power related operations due to authentication error. - Use correct keys for snmp auth_protocol and priv_protocol when creating SNMP client - Sanitize snmp auth_key and priv_key in API results Story: 2010613 Task: 47535 Change-Id: I5efd3c9f79a021f1a8e613c3d13b6596a7972672 --- ironic/api/controllers/v1/node.py | 10 ++++ ironic/drivers/modules/snmp.py | 4 +- .../unit/api/controllers/v1/test_node.py | 58 +++++++++++++++++++ .../tests/unit/drivers/modules/test_snmp.py | 35 +++++++++++ .../notes/bug-2010613-3ab1f32aaa776f28.yaml | 7 +++ 5 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/bug-2010613-3ab1f32aaa776f28.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index d21b075c81..5bf0a4981d 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1600,6 +1600,10 @@ def node_sanitize(node, fields, cdict=None, node['driver_info'] = strutils.mask_dict_password( node['driver_info'], "******") + _mask_fields(node['driver_info'], + ['snmp_auth_key', 'snmp_priv_key'], + "******") + if not show_instance_secrets and 'instance_info' in node_keys: node['instance_info'] = strutils.mask_dict_password( node['instance_info'], "******") @@ -1663,6 +1667,12 @@ def _node_sanitize_extended(node, node_keys, target_dict, cdict): 'driver_internal_info permission. **'} +def _mask_fields(dictionary, fields, secret): + for field in fields: + if dictionary.get(field): + dictionary[field] = secret + + def node_list_convert_with_links(nodes, limit, url, fields=None, **kwargs): cdict = api.request.context.to_policy_values() target_dict = dict(cdict) diff --git a/ironic/drivers/modules/snmp.py b/ironic/drivers/modules/snmp.py index d544d5687b..435d24b789 100644 --- a/ironic/drivers/modules/snmp.py +++ b/ironic/drivers/modules/snmp.py @@ -392,9 +392,9 @@ def _get_client(snmp_info): snmp_info.get("read_community"), snmp_info.get("write_community"), snmp_info.get("user"), - snmp_info.get("auth_proto"), + snmp_info.get("auth_protocol"), snmp_info.get("auth_key"), - snmp_info.get("priv_proto"), + snmp_info.get("priv_protocol"), snmp_info.get("priv_key"), snmp_info.get("context_engine_id"), snmp_info.get("context_name")) diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index d56652b1e2..45ef4d26ff 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -844,6 +844,64 @@ class TestListNodes(test_api_base.BaseApiTest): self.assertIn('retired_reason', data['nodes'][0]) self.assertIn('network_data', data['nodes'][0]) + def test_detail_snmpv3(self): + driver_info = { + 'snmp_version': 3, + 'snmp_user': 'test-user', + 'snmp_auth_protocol': 'sha', + 'snmp_auth_key': 'test-auth-key', + 'snmp_priv_protocol': 'aes', + 'snmp_priv_key': 'test-priv-key' + } + sanitized_driver_info = driver_info.copy() + sanitized_driver_info['snmp_auth_key'] = '******' + sanitized_driver_info['snmp_priv_key'] = '******' + + node = obj_utils.create_test_node(self.context, + chassis_id=self.chassis.id, + driver_info=driver_info) + data = self.get_json( + '/nodes/detail', + headers={api_base.Version.string: str(api_v1.max_version())}) + self.assertEqual(node.uuid, data['nodes'][0]["uuid"]) + self.assertIn('name', data['nodes'][0]) + self.assertIn('driver', data['nodes'][0]) + self.assertIn('driver_info', data['nodes'][0]) + self.assertEqual(sanitized_driver_info, + data['nodes'][0]['driver_info']) + self.assertIn('extra', data['nodes'][0]) + self.assertIn('properties', data['nodes'][0]) + self.assertIn('chassis_uuid', data['nodes'][0]) + self.assertIn('reservation', data['nodes'][0]) + self.assertIn('maintenance', data['nodes'][0]) + self.assertIn('console_enabled', data['nodes'][0]) + self.assertIn('target_power_state', data['nodes'][0]) + self.assertIn('target_provision_state', data['nodes'][0]) + self.assertIn('provision_updated_at', data['nodes'][0]) + self.assertIn('inspection_finished_at', data['nodes'][0]) + self.assertIn('inspection_started_at', data['nodes'][0]) + self.assertIn('raid_config', data['nodes'][0]) + self.assertIn('target_raid_config', data['nodes'][0]) + self.assertIn('network_interface', data['nodes'][0]) + self.assertIn('resource_class', data['nodes'][0]) + for field in api_utils.V31_FIELDS: + self.assertIn(field, data['nodes'][0]) + self.assertIn('storage_interface', data['nodes'][0]) + self.assertIn('traits', data['nodes'][0]) + self.assertIn('conductor_group', data['nodes'][0]) + self.assertIn('automated_clean', data['nodes'][0]) + self.assertIn('protected', data['nodes'][0]) + self.assertIn('protected_reason', data['nodes'][0]) + self.assertIn('owner', data['nodes'][0]) + self.assertIn('lessee', data['nodes'][0]) + # never expose the chassis_id + self.assertNotIn('chassis_id', data['nodes'][0]) + self.assertNotIn('allocation_id', data['nodes'][0]) + self.assertIn('allocation_uuid', data['nodes'][0]) + self.assertIn('retired', data['nodes'][0]) + self.assertIn('retired_reason', data['nodes'][0]) + self.assertIn('network_data', data['nodes'][0]) + def test_detail_instance_uuid(self): instance_uuid = '6eccd391-961c-4da5-b3c5-e2fa5cfbbd9d' node = obj_utils.create_test_node( diff --git a/ironic/tests/unit/drivers/modules/test_snmp.py b/ironic/tests/unit/drivers/modules/test_snmp.py index 5391d7ac59..e1b6fc1dfb 100644 --- a/ironic/tests/unit/drivers/modules/test_snmp.py +++ b/ironic/tests/unit/drivers/modules/test_snmp.py @@ -45,6 +45,41 @@ class SNMPClientTestCase(base.TestCase): self.oid = (1, 3, 6, 1, 1, 1, 0) self.value = 'value' + @mock.patch.object(pysnmp, 'SnmpEngine', autospec=True) + def test__get_client(self, mock_snmpengine): + driver_info = db_utils.get_test_snmp_info( + snmp_address=self.address, + snmp_port=self.port, + snmp_user='test-user', + snmp_auth_protocol='sha', + snmp_auth_key='test-auth-key', + snmp_priv_protocol='aes', + snmp_priv_key='test-priv-key', + snmp_context_engine_id='test-engine-id', + snmp_context_name='test-context-name', + snmp_version='3') + node = obj_utils.get_test_node( + self.context, + driver_info=driver_info) + info = snmp._parse_driver_info(node) + + client = snmp._get_client(info) + + mock_snmpengine.assert_called_once_with() + self.assertEqual(self.address, client.address) + self.assertEqual(int(self.port), client.port) + self.assertEqual(snmp.SNMP_V3, client.version) + self.assertNotIn('read_community', client.__dict__) + self.assertNotIn('write_community', client.__dict__) + self.assertEqual('test-user', client.user) + self.assertEqual(pysnmp.usmHMACSHAAuthProtocol, client.auth_proto) + self.assertEqual('test-auth-key', client.auth_key) + self.assertEqual(pysnmp.usmAesCfb128Protocol, client.priv_proto) + self.assertEqual('test-priv-key', client.priv_key) + self.assertEqual('test-engine-id', client.context_engine_id) + self.assertEqual('test-context-name', client.context_name) + self.assertEqual(mock_snmpengine.return_value, client.snmp_engine) + @mock.patch.object(pysnmp, 'SnmpEngine', autospec=True) def test___init__(self, mock_snmpengine): client = snmp.SNMPClient(self.address, self.port, snmp.SNMP_V1) diff --git a/releasenotes/notes/bug-2010613-3ab1f32aaa776f28.yaml b/releasenotes/notes/bug-2010613-3ab1f32aaa776f28.yaml new file mode 100644 index 0000000000..89769bfb86 --- /dev/null +++ b/releasenotes/notes/bug-2010613-3ab1f32aaa776f28.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + [`bug 2010613 `_] + Fixes issue with SNMP v3 auth protocol and priv protocol set in + driver info not being retrieved correctly when a SNMP client is + initialized.