Merge "Redfish: Consider password part of the session cache"

doc/source/admin/drivers/redfish.rst

@@ -87,8 +87,18 @@ field:
                          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``.
+                         as ``[redfish]auth_type``. Most operators should not
+                         need to leverage this setting. Session based
+                         authentication should generally be used in most
+                         cases as it prevents re-authentication every time
+                         a background task checks in with the BMC.
 
+.. note::
+   The ``redfish_address``, ``redfish_username``, ``redfish_password``,
+   and ``redfish_verify_ca`` fields, if changed, will trigger a new session
+   to be establsihed and cached with the BMC. The ``redfish_auth_type`` field
+   will only be used for the creation of a new cached session, or should
+   one be rejected by the BMC.
 
 The ``baremetal node create`` command can be used to enroll
 a node with the ``redfish`` driver. For example:
@@ -620,6 +630,44 @@ Eject Virtual Media
 
     "boot_device (optional)", "body", "string", "Type of the device to eject (all devices by default)"
 
+Internal Session Cache
+======================
+
+The ``redfish`` hardware type, and derived interfaces, utilizes a built-in
+session cache which prevents Ironic from re-authenticating every time
+Ironic attempts to connect to the BMC for any reason.
+
+This consists of cached connectors objects which are used and tracked by
+a unique consideration of ``redfish_username``, ``redfish_password``,
+``redfish_verify_ca``, and finally ``redfish_address``. Changing any one
+of those values will trigger a new session to be created.
+The ``redfish_system_id`` value is explicitly not considered as Redfish
+has a model of use of one BMC to many systems, which is also a model
+Ironic supports.
+
+The session cache default size is ``1000`` sessions per conductor.
+If you are operating a deployment with a larger number of Redfish
+BMCs, it is advised that you do appropriately tune that number.
+This can be tuned via the API service configuration file,
+``[redfish]connection_cache_size``.
+
+Session Cache Expiration
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+By default, sessions remain cached for as long as possible in
+memory, as long as they have not experienced an authentication,
+connection, or other unexplained error.
+
+Under normal circumstances, the sessions will only be rolled out
+of the cache in order of oldest first when the cache becomes full.
+There is no time based expiration to entries in the session cache.
+
+Of course, the cache is only in memory, and restarting the
+``ironic-conductor`` will also cause the cache to be rebuilt
+from scratch. If this is due to any persistent connectivity issue,
+this may be sign of an unexpected condition, and please consider
+contacting the Ironic developer community for assistance.
+
 .. _Redfish: http://redfish.dmtf.org/
 .. _Sushy: https://opendev.org/openstack/sushy
 .. _TLS: https://en.wikipedia.org/wiki/Transport_Layer_Security

ironic/drivers/modules/redfish/utils.py

@@ -15,6 +15,7 @@
 #    under the License.
 
 import collections
+import hashlib
 import os
 from urllib import parse as urlparse
 
@@ -198,17 +199,31 @@ class SessionCache(object):
     _sessions = collections.OrderedDict()
 
     def __init__(self, driver_info):
+        # Hash the password in the data structure, so we can
+        # include it in the session key.
+        # NOTE(TheJulia): Multiplying the address by 4, to ensure
+        # we meet a minimum of 16 bytes for salt.
+        pw_hash = hashlib.pbkdf2_hmac(
+            'sha512',
+            driver_info.get('password').encode('utf-8'),
+            str(driver_info.get('address') * 4).encode('utf-8'), 40)
         self._driver_info = driver_info
+        # Assemble the session key and append the hashed password to it,
+        # which forces new sessions to be established when the saved password
+        # is changed, just like the username, or address.
         self._session_key = tuple(
             self._driver_info.get(key)
             for key in ('address', 'username', 'verify_ca')
-        )
+        ) + (pw_hash.hex(),)
 
     def __enter__(self):
         try:
             return self.__class__._sessions[self._session_key]
-
         except KeyError:
+            LOG.debug('A cached redfish session for Redfish endpoint '
+                      '%(endpoint)s was not detected, initiating a session.',
+                      {'endpoint': self._driver_info['address']})
+
         auth_type = self._driver_info['auth_type']
 
         auth_class = self.AUTH_CLASSES[auth_type]
@@ -229,6 +244,8 @@ class SessionCache(object):
 
         if CONF.redfish.connection_cache_size:
             self.__class__._sessions[self._session_key] = conn
+            # Save a secure hash of the password into memory, so if we
+            # observe it change, we can detect the session is no longer valid.
 
             if (len(self.__class__._sessions)
                     > CONF.redfish.connection_cache_size):

ironic/tests/unit/drivers/modules/redfish/test_utils.py

@@ -252,6 +252,7 @@ class RedfishUtilsAuthTestCase(db_base.DbTestCase):
         redfish_utils.get_system(self.node)
         redfish_utils.get_system(self.node)
         self.assertEqual(1, mock_sushy.call_count)
+        self.assertEqual(len(redfish_utils.SessionCache._sessions), 1)
 
     @mock.patch.object(sushy, 'Sushy', autospec=True)
     def test_ensure_new_session_address(self, mock_sushy):
@@ -269,6 +270,21 @@ class RedfishUtilsAuthTestCase(db_base.DbTestCase):
         redfish_utils.get_system(self.node)
         self.assertEqual(2, mock_sushy.call_count)
 
+    @mock.patch.object(sushy, 'Sushy', autospec=True)
+    def test_ensure_new_session_password(self, mock_sushy):
+        d_info = self.node.driver_info
+        d_info['redfish_username'] = 'foo'
+        d_info['redfish_password'] = 'bar'
+        self.node.driver_info = d_info
+        self.node.save()
+        redfish_utils.get_system(self.node)
+        d_info['redfish_password'] = 'foo'
+        self.node.driver_info = d_info
+        self.node.save()
+        redfish_utils.SessionCache._sessions = collections.OrderedDict()
+        redfish_utils.get_system(self.node)
+        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)

releasenotes/notes/redfish_consider_password_in_session_cache-1fa84234db179053.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixes an issue where the Redfish session cache would continue using an
+    old session when a password for a Redfish BMC was changed. Now the old
+    session will not be found in this case, and a new session will be created
+    with the latest credential information available.