From 778d715b5f288245e8f59e5b626a0cd800286e43 Mon Sep 17 00:00:00 2001 From: ankit Date: Tue, 28 Jul 2020 07:23:25 +0000 Subject: [PATCH] Enhance certificate verification for ilo harware type This patch enhance certificate verification for hardware type ilo and ilo5 by adding attribute ilo_verify_ca which can take directory and bolean values apart from file. Change-Id: Ic48bf53097635498a8461be049ee5d2a50c6fe2a --- ironic/conf/ilo.py | 13 +++ ironic/drivers/modules/ilo/common.py | 107 ++++++++++++++---- ironic/tests/unit/conductor/test_manager.py | 3 +- .../unit/drivers/modules/ilo/test_common.py | 91 +++++++++++++-- ...fication-enhancement-8eefd541cfc2a9da.yaml | 12 ++ 5 files changed, 194 insertions(+), 32 deletions(-) create mode 100644 releasenotes/notes/ilo-certificate-verification-enhancement-8eefd541cfc2a9da.yaml diff --git a/ironic/conf/ilo.py b/ironic/conf/ilo.py index 5baaf3d4a4..8e5ca1d899 100644 --- a/ironic/conf/ilo.py +++ b/ironic/conf/ilo.py @@ -77,7 +77,20 @@ opts = [ '300GB SSD with default pattern "block" would take ' 'approx. 30 seconds to complete sanitize disk erase.')), cfg.StrOpt('ca_file', + deprecated_for_removal=True, + deprecated_reason=_('Its being replaced by new configuration ' + 'parameter "verify_ca".'), help=_('CA certificate file to validate iLO.')), + cfg.StrOpt('verify_ca', + default='True', + help=_('CA certificate to validate iLO. This can be either ' + 'a Boolean value, a path to a CA_BUNDLE file or ' + 'directory with certificates of trusted CAs. If set ' + 'to True the driver will verify the host certificates; ' + 'if False the driver will ignore verifying the SSL ' + 'certificate. If it\'s a path the driver will use the ' + 'specified certificate or one of the certificates in ' + 'the directory. Defaults to True.')), cfg.StrOpt('default_boot_mode', default='auto', choices=[('auto', _('based on boot mode settings on the ' diff --git a/ironic/drivers/modules/ilo/common.py b/ironic/drivers/modules/ilo/common.py index ef330c5c2b..f297a6b3cd 100644 --- a/ironic/drivers/modules/ilo/common.py +++ b/ironic/drivers/modules/ilo/common.py @@ -25,6 +25,7 @@ from ironic_lib import utils as ironic_utils from oslo_log import log as logging from oslo_utils import fileutils from oslo_utils import importutils +from oslo_utils import strutils from ironic.common import boot_devices from ironic.common import exception @@ -56,7 +57,17 @@ REQUIRED_PROPERTIES = { OPTIONAL_PROPERTIES = { 'client_port': _("port to be used for iLO operations. Optional."), 'client_timeout': _("timeout (in seconds) for iLO operations. Optional."), - 'ca_file': _("CA certificate file to validate iLO. Optional") + 'ca_file': _("CA certificate file to validate iLO. This " + "attibute is deprecated and will be removed in " + "future release. Optional"), + 'ilo_verify_ca': _("Either a Boolean value, a path to a CA_BUNDLE " + "file or directory with certificates of trusted " + "CAs. If set to True the driver will verify the " + "host certificates; if False the driver will ignore " + "verifying the SSL certificate. If it\'s a path the " + "driver will use the specified certificate or one of " + "the certificates in the directory. Defaults to True. " + "Optional") } SNMP_PROPERTIES = { @@ -255,22 +266,8 @@ def parse_driver_info(node): "The following required iLO parameters are missing from the " "node's driver_info: %s") % missing_info) - not_integers = [] - for param in OPTIONAL_PROPERTIES: - value = info.get(param, CONF.ilo.get(param)) - if param == "client_port": - d_info[param] = utils.validate_network_port(value, param) - elif param == "ca_file": - if value and not os.path.isfile(value): - raise exception.InvalidParameterValue(_( - '%(param)s "%(value)s" is not found.') % - {'param': param, 'value': value}) - d_info[param] = value - else: - try: - d_info[param] = int(value) - except ValueError: - not_integers.append(param) + optional_info = _parse_optional_driver_info(node) + d_info.update(optional_info) snmp_info = _parse_snmp_driver_info(info) if snmp_info: @@ -284,11 +281,6 @@ def parse_driver_info(node): if param == "console_port": d_info[param] = utils.validate_network_port(value, param) - if not_integers: - raise exception.InvalidParameterValue(_( - "The following iLO parameters from the node's driver_info " - "should be integers: %s") % not_integers) - return d_info @@ -335,6 +327,75 @@ def _parse_snmp_driver_info(info): return snmp_info +def _parse_optional_driver_info(node): + """Parses the optional driver_info parameters. + + :param node: an ironic Node object. + :returns: a dictionary containing information of optional properties. + :raises: InvalidParameterValue if any parameters are incorrect + """ + info = node.driver_info + optional_info = {} + not_integers = [] + + for param in OPTIONAL_PROPERTIES: + if param != 'ilo_verify_ca': + value = info.get(param, CONF.ilo.get(param)) + + if param == "client_port": + optional_info[param] = utils.validate_network_port(value, param) + elif param == "client_timeout": + try: + optional_info[param] = int(value) + except ValueError: + not_integers.append(param) + + if not_integers: + raise exception.InvalidParameterValue(_( + "The following iLO parameters from the node's driver_info " + "should be integers: %s") % not_integers) + + ca_file_value = info.get('ca_file', CONF.ilo.get('ca_file')) + verify_ca = info.get('ilo_verify_ca', CONF.ilo.get('verify_ca')) + + if ca_file_value: + LOG.warning("The `driver_info/ca_file` parameter is deprecated " + "in favor of `driver_info/ilo_verify_ca` parameter. " + "Please use `driver_info/ilo_verify_ca` instead of " + "`driver_info/ca_file` in node %(node)s " + "configuration.", {'node': node.uuid}) + + if not os.path.isfile(ca_file_value): + raise exception.InvalidParameterValue(_( + 'ca_file "%(value)s" is not found.') % + {'value': ca_file_value}) + optional_info['verify_ca'] = ca_file_value + else: + # Check if ilo_verify_ca is a Boolean or a file/directory + # in the file-system + if isinstance(verify_ca, str): + if not os.path.isdir(verify_ca) and not os.path.isfile(verify_ca): + try: + verify_ca = strutils.bool_from_string(verify_ca, + strict=True) + except ValueError: + raise exception.InvalidParameterValue( + _('Invalid value type set in driver_info/' + 'ilo_verify_ca on node %(node)s. ' + 'The value should be a Boolean or the path ' + 'to a file/directory, not "%(value)s"' + ) % {'value': verify_ca, 'node': node.uuid}) + elif not isinstance(verify_ca, bool): + raise exception.InvalidParameterValue( + _('Invalid value type set in driver_info/ilo_verify_ca ' + 'on node %(node)s. The value should be a Boolean ' + 'or the path to a file/directory, not "%(value)s"' + ) % {'value': verify_ca, 'node': node.uuid}) + optional_info['verify_ca'] = verify_ca + + return optional_info + + def get_ilo_object(node): """Gets an IloClient object from proliantutils library. @@ -369,7 +430,7 @@ def get_ilo_object(node): driver_info['ilo_password'], driver_info['client_timeout'], driver_info['client_port'], - cacert=driver_info.get('ca_file'), + cacert=driver_info['verify_ca'], snmp_credentials=info) return ilo_object diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 52a5e03a3c..898098245b 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -5832,7 +5832,8 @@ class ManagerTestProperties(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): 'ilo_deploy_iso', 'console_port', 'ilo_change_password', 'ca_file', 'snmp_auth_user', 'snmp_auth_prot_password', 'snmp_auth_priv_password', 'snmp_auth_protocol', - 'snmp_auth_priv_protocol', 'deploy_forces_oob_reboot'] + 'snmp_auth_priv_protocol', 'deploy_forces_oob_reboot', + 'ilo_verify_ca'] self._check_driver_properties("ilo", expected) def test_driver_properties_fail(self): diff --git a/ironic/tests/unit/drivers/modules/ilo/test_common.py b/ironic/tests/unit/drivers/modules/ilo/test_common.py index d5f486aba1..c743a62604 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_common.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_common.py @@ -83,7 +83,7 @@ class IloValidateParametersTestCase(BaseIloTest): self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) self.assertEqual(60, info['client_timeout']) self.assertEqual(443, info['client_port']) - self.assertEqual('/home/user/cafile.pem', info['ca_file']) + self.assertEqual('/home/user/cafile.pem', info['verify_ca']) self.assertEqual('user', info['snmp_auth_user']) self.assertEqual('1234', info['snmp_auth_prot_password']) self.assertEqual('4321', info['snmp_auth_priv_password']) @@ -99,6 +99,81 @@ class IloValidateParametersTestCase(BaseIloTest): self.assertEqual(60, info['client_timeout']) self.assertEqual(443, info['client_port']) + @mock.patch.object(os.path, 'isdir', autospec=True) + def test_parse_driver_info_path_verify_ca_dir(self, mock_isdir): + mock_isdir.return_value = True + fake_path = '/path/to/a/valid/CA' + self.node.driver_info['ilo_verify_ca'] = fake_path + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(fake_path, info['verify_ca']) + mock_isdir.assert_called_once_with(fake_path) + + @mock.patch.object(os.path, 'isfile', autospec=True) + def test_parse_driver_info_path_verify_ca_file(self, mock_isfile): + mock_isfile.return_value = True + fake_path = '/path/to/a/valid/CA.pem' + self.node.driver_info['ilo_verify_ca'] = fake_path + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(fake_path, info['verify_ca']) + mock_isfile.assert_called_once_with(fake_path) + + def test_parse_driver_info_verify_ca_default_value(self): + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(True, info['verify_ca']) + + def test_parse_driver_info_verify_ca_string_false(self): + self.config(verify_ca="False", group='ilo') + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(False, info['verify_ca']) + + def test_parse_driver_info_verify_ca_boolean_true(self): + self.config(verify_ca=True, group='ilo') + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(True, info['verify_ca']) + + def test_parse_driver_info_verify_ca_boolean_false(self): + self.config(verify_ca=False, group='ilo') + info = ilo_common.parse_driver_info(self.node) + self.assertEqual(INFO_DICT['ilo_address'], info['ilo_address']) + self.assertEqual(INFO_DICT['ilo_username'], info['ilo_username']) + self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) + self.assertEqual(60, info['client_timeout']) + self.assertEqual(443, info['client_port']) + self.assertEqual(False, info['verify_ca']) + + def test_parse_driver_info_invalid_value_verify_ca(self): + # Integers are not supported + self.node.driver_info['ilo_verify_ca'] = 123456 + self.assertRaisesRegex(exception.InvalidParameterValue, + 'Invalid value type', + ilo_common._parse_optional_driver_info, + self.node) + @mock.patch.object(os.path, 'isfile', return_value=True, autospec=True) def test_parse_driver_info_snmp_true_no_auth_priv_protocols(self, isFile_mock): @@ -113,7 +188,7 @@ class IloValidateParametersTestCase(BaseIloTest): self.assertEqual(INFO_DICT['ilo_password'], info['ilo_password']) self.assertEqual(60, info['client_timeout']) self.assertEqual(443, info['client_port']) - self.assertEqual('/home/user/cafile.pem', info['ca_file']) + self.assertEqual('/home/user/cafile.pem', info['verify_ca']) self.assertEqual('user', info['snmp_auth_user']) self.assertEqual('1234', info['snmp_auth_prot_password']) self.assertEqual('4321', info['snmp_auth_priv_password']) @@ -230,7 +305,7 @@ class IloCommonMethodsTestCase(BaseIloTest): def _test_get_ilo_object(self, ilo_client_mock, isFile_mock, ca_file=None): self.info['client_timeout'] = 600 self.info['client_port'] = 4433 - self.info['ca_file'] = ca_file + self.info['ilo_verify_ca'] = ca_file self.node.driver_info = self.info ilo_client_mock.return_value = 'ilo_object' returned_ilo_object = ilo_common.get_ilo_object(self.node) @@ -240,7 +315,7 @@ class IloCommonMethodsTestCase(BaseIloTest): self.info['ilo_password'], self.info['client_timeout'], self.info['client_port'], - cacert=self.info['ca_file'], + cacert=self.info['ilo_verify_ca'], snmp_credentials=None) self.assertEqual('ilo_object', returned_ilo_object) @@ -256,7 +331,7 @@ class IloCommonMethodsTestCase(BaseIloTest): 'snmp_inspection': True} d_info = {'client_timeout': 600, 'client_port': 4433, - 'ca_file': 'ca_file', + 'verify_ca': 'True', 'snmp_auth_user': 'user', 'snmp_auth_prot_password': '1234', 'snmp_auth_priv_password': '4321', @@ -272,15 +347,15 @@ class IloCommonMethodsTestCase(BaseIloTest): self.info['ilo_password'], self.info['client_timeout'], self.info['client_port'], - cacert=self.info['ca_file'], + cacert=self.info['verify_ca'], snmp_credentials=info) self.assertEqual('ilo_object', returned_ilo_object) def test_get_ilo_object_cafile(self): self._test_get_ilo_object(ca_file='/home/user/ilo.pem') - def test_get_ilo_object_no_cafile(self): - self._test_get_ilo_object() + def test_get_ilo_object_cafile_boolean(self): + self._test_get_ilo_object(ca_file=True) def test_update_ipmi_properties(self): with task_manager.acquire(self.context, self.node.uuid, diff --git a/releasenotes/notes/ilo-certificate-verification-enhancement-8eefd541cfc2a9da.yaml b/releasenotes/notes/ilo-certificate-verification-enhancement-8eefd541cfc2a9da.yaml new file mode 100644 index 0000000000..6323d8e6a4 --- /dev/null +++ b/releasenotes/notes/ilo-certificate-verification-enhancement-8eefd541cfc2a9da.yaml @@ -0,0 +1,12 @@ +--- +features: + - | + Adds a new configuration option ``[ilo]verify_ca`` and a new + ``driver_info`` parameter ``ilo_verify_ca`` to enhance certificate + verification for hardware type ilo and ilo5 which can take + directory and bolean values apart from file. +deprecations: + - The ``[ilo]ca_file`` configuration option is deprecated for + removal, please use ``[ilo]verify_ca`` instead which can take + directory and boolean values apart from file for certificate + verification.