Redfish: Consider password part of the session cache

Previously, when a password change occured in ironic,
the session would not be invalidated, and this, in theory,
could lead to all sorts of issues with the old password
still being re-used for authentication.

In a large environment where credentials for BMCs may not
be centralized, this can quickly lead to repeated account
lockout experiences for the BMC service account.

Anyhow, now we consider it in tracking the sessions, so
when the saved password is changed, a new session is
established, and the old session is eventually expired out
of the cache.

Change-Id: I49e1907b89a9096aa043424b205e7bd390ed1a2f

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,43 +199,59 @@ 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:
-            auth_type = self._driver_info['auth_type']
+            LOG.debug('A cached redfish session for Redfish endpoint '
+                      '%(endpoint)s was not detected, initiating a session.',
+                      {'endpoint': self._driver_info['address']})
 
-            auth_class = self.AUTH_CLASSES[auth_type]
+        auth_type = self._driver_info['auth_type']
 
-            authenticator = auth_class(
+        auth_class = self.AUTH_CLASSES[auth_type]
-                username=self._driver_info['username'],
-                password=self._driver_info['password']
-            )
 
-            sushy_params = {'verify': self._driver_info['verify_ca'],
+        authenticator = auth_class(
-                            'auth': authenticator}
+            username=self._driver_info['username'],
-            if 'root_prefix' in self._driver_info:
+            password=self._driver_info['password']
-                sushy_params['root_prefix'] = self._driver_info['root_prefix']
+        )
-            conn = sushy.Sushy(
-                self._driver_info['address'],
-                **sushy_params
-            )
 
-            if CONF.redfish.connection_cache_size:
+        sushy_params = {'verify': self._driver_info['verify_ca'],
-                self.__class__._sessions[self._session_key] = conn
+                        'auth': authenticator}
+        if 'root_prefix' in self._driver_info:
+            sushy_params['root_prefix'] = self._driver_info['root_prefix']
+        conn = sushy.Sushy(
+            self._driver_info['address'],
+            **sushy_params
+        )
 
-                if (len(self.__class__._sessions)
+        if CONF.redfish.connection_cache_size:
-                        > CONF.redfish.connection_cache_size):
+            self.__class__._sessions[self._session_key] = conn
-                    self._expire_oldest_session()
+            # Save a secure hash of the password into memory, so if we
+            # observe it change, we can detect the session is no longer valid.
 
-            return conn
+            if (len(self.__class__._sessions)
+                    > CONF.redfish.connection_cache_size):
+                self._expire_oldest_session()
+
+        return conn
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         # NOTE(etingof): perhaps this session token is no good

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.