Merge "Enhance certificate verification for ilo harware type"

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 '

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 = []
+    optional_info = _parse_optional_driver_info(node)
-    for param in OPTIONAL_PROPERTIES:
+    d_info.update(optional_info)
-        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)
 
     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
 

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):

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):
+    def test_get_ilo_object_cafile_boolean(self):
-        self._test_get_ilo_object()
+        self._test_get_ilo_object(ca_file=True)
 
     def test_update_ipmi_properties(self):
         with task_manager.acquire(self.context, self.node.uuid,

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.