From 59b5b66c8ed95a509223c834b271ecbe21e42e2f Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Wed, 19 Sep 2018 16:24:51 +0200 Subject: [PATCH] Add configurable Redfish client authentication Adds 'driver_info/redfish_auth_type' option for Redfish HTTP client to chose one of the following authentication methods - 'basic', 'session' and 'auto'. The latter first tries 'session' and falls back to 'basic' if session authentication is not supported by the Redfish BMC. Default is set in ironic config (defaulted to 'auto'). Also bumped sushy requirement to 1.3.0+ Change-Id: I11bf8413bdb3f2d7f632495bb20a71a165554b27 Story: 2003813 Task: 26565 --- doc/source/admin/drivers/redfish.rst | 8 ++ ironic/conf/redfish.py | 9 +- ironic/drivers/modules/redfish/utils.py | 50 ++++++++-- .../drivers/modules/redfish/test_utils.py | 96 ++++++++++++++++--- ...dd-redfish-auth-type-5fe78071b528e53b.yaml | 10 ++ 5 files changed, 150 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/add-redfish-auth-type-5fe78071b528e53b.yaml diff --git a/doc/source/admin/drivers/redfish.rst b/doc/source/admin/drivers/redfish.rst index bcfb683214..db61d4ccc5 100644 --- a/doc/source/admin/drivers/redfish.rst +++ b/doc/source/admin/drivers/redfish.rst @@ -77,6 +77,14 @@ field: driver will use for verification. To disable verifying TLS_, set this to False. This is optional. +- ``redfish_auth_type``: Redfish HTTP client authentication method. Can be + "basic", "session" or "auto". + The "auto" mode first tries "session" and falls back + to "basic" if session authentication is not supported + by the Redfish BMC. Default is set in ironic config + as ``[redfish]auth_type``. + + The ``openstack baremetal node create`` command can be used to enroll a node with the ``redfish`` driver. For example: diff --git a/ironic/conf/redfish.py b/ironic/conf/redfish.py index aa57514140..a49f2e5d1b 100644 --- a/ironic/conf/redfish.py +++ b/ironic/conf/redfish.py @@ -36,7 +36,14 @@ opts = [ 'BMC connections (obtained through Redfish Session ' 'Service). This option caps the maximum number of ' 'connections to maintain. The value of `0` disables ' - 'client connection caching completely.')) + 'client connection caching completely.')), + cfg.StrOpt('auth_type', + choices=[('basic', _('Use HTTP basic authentication')), + ('session', _('Use HTTP session authentication')), + ('auto', _('Try HTTP session authentication first, ' + 'fall back to basic HTTP authentication'))], + default='auto', + help=_('Redfish HTTP client authentication method.')) ] diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py index 5cc8ff45bd..69be609188 100644 --- a/ironic/drivers/modules/redfish/utils.py +++ b/ironic/drivers/modules/redfish/utils.py @@ -62,7 +62,11 @@ OPTIONAL_PROPERTIES = { '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') + 'directory. Defaults to True. Optional'), + 'redfish_auth_type': _('Redfish HTTP client authentication method. Can be ' + '"basic", "session" or "auto". If not set, the ' + 'default value is taken from Ironic ' + 'configuration as ``[redfish]auth_type`` option.') } COMMON_PROPERTIES = REQUIRED_PROPERTIES.copy() @@ -142,16 +146,33 @@ def parse_driver_info(node): 'to a file/directory, not "%(value)s"') % {'value': verify_ca, 'node': node.uuid}) + auth_type = driver_info.get('redfish_auth_type', CONF.redfish.auth_type) + if auth_type not in ('basic', 'session', 'auto'): + raise exception.InvalidParameterValue( + _('Invalid value "%(value)s" set in ' + 'driver_info/redfish_auth_type on node %(node)s. ' + 'The value should be one of "basic", "session" or "auto".') % + {'value': auth_type, 'node': node.uuid}) + return {'address': address, 'system_id': system_id, 'username': driver_info.get('redfish_username'), 'password': driver_info.get('redfish_password'), 'verify_ca': verify_ca, + 'auth_type': auth_type, 'node_uuid': node.uuid} class SessionCache(object): """Cache of HTTP sessions credentials""" + AUTH_CLASSES = {} + if sushy: + AUTH_CLASSES.update( + basic=sushy.auth.BasicAuth, + session=sushy.auth.SessionAuth, + auto=sushy.auth.SessionOrBasicAuth + ) + _sessions = collections.OrderedDict() def __init__(self, driver_info): @@ -166,11 +187,19 @@ class SessionCache(object): return self.__class__._sessions[self._session_key] except KeyError: + auth_type = self._driver_info['auth_type'] + + auth_class = self.AUTH_CLASSES[auth_type] + + authenticator = auth_class( + username=self._driver_info['username'], + password=self._driver_info['password'] + ) + conn = sushy.Sushy( self._driver_info['address'], - username=self._driver_info['username'], - password=self._driver_info['password'], - verify=self._driver_info['verify_ca'] + verify=self._driver_info['verify_ca'], + auth=authenticator ) if CONF.redfish.connection_cache_size: @@ -206,7 +235,6 @@ def get_system(node): :raises: RedfishError if the System is not registered in Redfish """ driver_info = parse_driver_info(node) - address = driver_info['address'] system_id = driver_info['system_id'] @retrying.retry( @@ -229,9 +257,12 @@ def get_system(node): # retrying on them except sushy.exceptions.ConnectionError as e: LOG.warning('For node %(node)s, got a connection error from ' - 'Redfish at address "%(address)s" when fetching ' - 'System "%(system)s". Error: %(error)s', - {'system': system_id, 'address': address, + 'Redfish at address "%(address)s" using auth type ' + '"%(auth_type)s" when fetching System "%(system)s". ' + 'Error: %(error)s', + {'system': system_id, + 'address': driver_info['address'], + 'auth_type': driver_info['auth_type'], 'node': node.uuid, 'error': e}) raise exception.RedfishConnectionError(node=node.uuid, error=e) @@ -241,4 +272,5 @@ def get_system(node): with excutils.save_and_reraise_exception(): LOG.error('Failed to connect to Redfish at %(address)s for ' 'node %(node)s. Error: %(error)s', - {'address': address, 'node': node.uuid, 'error': e}) + {'address': driver_info['address'], + 'node': node.uuid, 'error': e}) diff --git a/ironic/tests/unit/drivers/modules/redfish/test_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_utils.py index daa78d4538..9614c17405 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_utils.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_utils.py @@ -51,6 +51,7 @@ class RedfishUtilsTestCase(db_base.DbTestCase): 'username': 'username', 'password': 'password', 'verify_ca': True, + 'auth_type': 'auto', 'node_uuid': self.node.uuid } @@ -140,6 +141,20 @@ class RedfishUtilsTestCase(db_base.DbTestCase): 'The value should be a Boolean', redfish_utils.parse_driver_info, self.node) + def test_parse_driver_info_valid_auth_type(self): + for value in 'basic', 'session', 'auto': + self.node.driver_info['redfish_auth_type'] = value + response = redfish_utils.parse_driver_info(self.node) + self.parsed_driver_info['auth_type'] = value + self.assertEqual(self.parsed_driver_info, response) + + def test_parse_driver_info_invalid_auth_type(self): + for value in 'BasiC', 'SESSION', 'Auto': + self.node.driver_info['redfish_auth_type'] = value + self.assertRaisesRegex(exception.InvalidParameterValue, + 'The value should be one of ', + redfish_utils.parse_driver_info, self.node) + @mock.patch.object(sushy, 'Sushy', autospec=True) @mock.patch('ironic.drivers.modules.redfish.utils.' 'SessionCache._sessions', {}) @@ -190,17 +205,6 @@ class RedfishUtilsTestCase(db_base.DbTestCase): mock_sleep.assert_called_with( redfish_utils.CONF.redfish.connection_retry_interval) - @mock.patch.object(sushy, 'Sushy', autospec=True) - @mock.patch('ironic.drivers.modules.redfish.utils.' - 'SessionCache._sessions', {}) - def test_auth_auto(self, mock_sushy): - redfish_utils.get_system(self.node) - mock_sushy.assert_called_with( - self.parsed_driver_info['address'], - username=self.parsed_driver_info['username'], - password=self.parsed_driver_info['password'], - verify=True) - @mock.patch.object(sushy, 'Sushy', autospec=True) @mock.patch('ironic.drivers.modules.redfish.utils.' 'SessionCache._sessions', {}) @@ -226,8 +230,21 @@ class RedfishUtilsTestCase(db_base.DbTestCase): self.assertEqual(2, mock_sushy.call_count) @mock.patch.object(sushy, 'Sushy', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache.AUTH_CLASSES', autospec=True) @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache._sessions', collections.OrderedDict()) + def test_ensure_basic_session_caching(self, mock_auth, mock_sushy): + self.node.driver_info['redfish_auth_type'] = 'basic' + mock_session_or_basic_auth = mock_auth['auto'] + redfish_utils.get_system(self.node) + mock_sushy.assert_called_with( + mock.ANY, verify=mock.ANY, + auth=mock_session_or_basic_auth.return_value, + ) + self.assertEqual(len(redfish_utils.SessionCache._sessions), 1) + + @mock.patch.object(sushy, 'Sushy', autospec=True) def test_expire_old_sessions(self, mock_sushy): cfg.CONF.set_override('connection_cache_size', 10, 'redfish') for num in range(20): @@ -238,8 +255,8 @@ class RedfishUtilsTestCase(db_base.DbTestCase): self.assertEqual(len(redfish_utils.SessionCache._sessions), 10) @mock.patch.object(sushy, 'Sushy', autospec=True) - @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache._sessions', - collections.OrderedDict()) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache._sessions', {}) def test_disabled_sessions_cache(self, mock_sushy): cfg.CONF.set_override('connection_cache_size', 0, 'redfish') for num in range(2): @@ -248,3 +265,56 @@ class RedfishUtilsTestCase(db_base.DbTestCase): self.assertEqual(mock_sushy.call_count, 2) self.assertEqual(len(redfish_utils.SessionCache._sessions), 0) + + @mock.patch.object(sushy, 'Sushy', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache.AUTH_CLASSES', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache._sessions', {}) + def test_auth_auto(self, mock_auth, mock_sushy): + redfish_utils.get_system(self.node) + mock_session_or_basic_auth = mock_auth['auto'] + mock_session_or_basic_auth.assert_called_with( + username=self.parsed_driver_info['username'], + password=self.parsed_driver_info['password'] + ) + mock_sushy.assert_called_with( + self.parsed_driver_info['address'], + auth=mock_session_or_basic_auth.return_value, + verify=True) + + @mock.patch.object(sushy, 'Sushy', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache.AUTH_CLASSES', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache._sessions', {}) + def test_auth_session(self, mock_auth, mock_sushy): + self.node.driver_info['redfish_auth_type'] = 'session' + mock_session_auth = mock_auth['session'] + redfish_utils.get_system(self.node) + mock_session_auth.assert_called_with( + username=self.parsed_driver_info['username'], + password=self.parsed_driver_info['password'] + ) + mock_sushy.assert_called_with( + mock.ANY, verify=mock.ANY, + auth=mock_session_auth.return_value + ) + + @mock.patch.object(sushy, 'Sushy', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache.AUTH_CLASSES', autospec=True) + @mock.patch('ironic.drivers.modules.redfish.utils.' + 'SessionCache._sessions', {}) + def test_auth_basic(self, mock_auth, mock_sushy): + self.node.driver_info['redfish_auth_type'] = 'basic' + mock_basic_auth = mock_auth['basic'] + redfish_utils.get_system(self.node) + mock_basic_auth.assert_called_with( + username=self.parsed_driver_info['username'], + password=self.parsed_driver_info['password'] + ) + sushy.Sushy.assert_called_with( + mock.ANY, verify=mock.ANY, + auth=mock_basic_auth.return_value + ) diff --git a/releasenotes/notes/add-redfish-auth-type-5fe78071b528e53b.yaml b/releasenotes/notes/add-redfish-auth-type-5fe78071b528e53b.yaml new file mode 100644 index 0000000000..60cc818577 --- /dev/null +++ b/releasenotes/notes/add-redfish-auth-type-5fe78071b528e53b.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + Adds the ``[redfish]auth_type`` ironic configuration option for the + ``redfish`` hardware type that is used to choose one of the following + authentication methods:: ``basic``, ``session`` and ``auto``. + The ``auto`` setting first tries ``session`` method and falls back to + ``basic`` if session authentication is not supported by the Redfish BMC. + The default is ``auto``. This configuration option can be overridden + on a per-node basis by the ``driver_info/redfish_auth_type`` option.