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
(cherry picked from commit c2ba869040f535e4bb83481d1fe9468e7e6991d8)

4 files changed, 111 insertions, 23 deletions

diff --git a/doc/source/admin/drivers/redfish.rst b/doc/source/admin/drivers/redfish.rst
index dd19f8bde..899ef98a2 100644
--- a/doc/source/admin/drivers/redfish.rst
+++ b/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
diff --git a/ironic/drivers/modules/redfish/utils.py b/ironic/drivers/modules/redfish/utils.py
index 40cf33bce..e85e2ec6a 100644
--- a/ironic/drivers/modules/redfish/utils.py
+++ b/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(
-                username=self._driver_info['username'],
-                password=self._driver_info['password']
-            )
+        auth_class = self.AUTH_CLASSES[auth_type]
 
-            sushy_params = {'verify': self._driver_info['verify_ca'],
-                            '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
-            )
+        authenticator = auth_class(
+            username=self._driver_info['username'],
+            password=self._driver_info['password']
+        )
+
+        sushy_params = {'verify': self._driver_info['verify_ca'],
+                        '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 CONF.redfish.connection_cache_size:
-                self.__class__._sessions[self._session_key] = conn
+        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):
-                    self._expire_oldest_session()
+            if (len(self.__class__._sessions)
+                    > CONF.redfish.connection_cache_size):
+                self._expire_oldest_session()
 
-            return conn
+        return conn
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         # NOTE(etingof): perhaps this session token is no good
diff --git a/ironic/tests/unit/drivers/modules/redfish/test_utils.py b/ironic/tests/unit/drivers/modules/redfish/test_utils.py
index ca8aba9da..01b7089c7 100644
--- a/ironic/tests/unit/drivers/modules/redfish/test_utils.py
+++ b/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):
@@ -270,6 +271,21 @@ class RedfishUtilsAuthTestCase(db_base.DbTestCase):
         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)
     @mock.patch('ironic.drivers.modules.redfish.utils.SessionCache._sessions',
diff --git a/releasenotes/notes/redfish_consider_password_in_session_cache-1fa84234db179053.yaml b/releasenotes/notes/redfish_consider_password_in_session_cache-1fa84234db179053.yaml
new file mode 100644
index 000000000..af48b88fa
--- /dev/null
+++ b/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.