Merge "Redfish: Consider password part of the session cache" into stable/yoga

4 files changed, 111 insertions, 23 deletions

diff --git a/doc/source/admin/drivers/redfish.rst b/doc/source/admin/drivers/redfish.rst
index 17d705ec5..bcf62b26a 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:
@@ -611,6 +621,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.