Fix Cinder Integration fallout from CVE-2023-2088

In the recent change to cinder, to address CVE-2023-2088,
cinder changed the policy rules and behavior for unbinding,
or "detaching" a volume. This was because of a vulnerability
in compute nodes where a volume which was in use by a VM
could be detached outside of Nova, and nova wouldn't become
aware the volume was detached, and the volume could be accessible
to the next VM.

This vulnerability doesn't apply to bare metal operations as
volumes are attached to whole baremetal nodes with Ironic.

We now generate and use a service token when interacting with
Cinder which allows cinder to recognize "this request is
coming from a fellow OpenStack service", and by-pass
checking with Nova if the "instance" is managed by Nova,
or Not. This allows the volumes to be attached, and detached
as needed as part of the power operation flow and overall
set of lifecycle operations.

Related-Bug: 2004555
Closes-Bug: 2019892

Change-Id: Ib258bc9650496da989fc93b759b112d279c8b217
(cherry picked from commit 9c0b4c9)

devstack/lib/ironic

@@ -1625,7 +1625,7 @@ EOF
 function configure_ironic_api {
     iniset $IRONIC_CONF_FILE DEFAULT auth_strategy $IRONIC_AUTH_STRATEGY
     configure_keystone_authtoken_middleware $IRONIC_CONF_FILE ironic
-
+    iniset $IRONIC_CONF_FILE keystone_authtoken service_token_roles_required True
     if [[ "$IRONIC_USE_WSGI" == "True" ]]; then
         iniset $IRONIC_CONF_FILE oslo_middleware enable_proxy_headers_parsing True
     elif is_service_enabled tls-proxy; then

ironic/common/cinder.py

@@ -19,6 +19,7 @@
 from cinderclient.v3 import client
 from oslo_log import log
 
+from ironic.common import context as ironic_context
 from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import keystone
@@ -39,30 +40,67 @@ def _get_cinder_session():
     return _CINDER_SESSION
 
 
-def get_client(context):
+def get_client(context, auth_from_config=False):
     """Get a cinder client connection.
 
     :param context: request context,
                     instance of ironic.common.context.RequestContext
+    :param auth_from_config: (boolean) When True, use auth values from
+                             conf parameters
     :returns: A cinder client.
     """
     service_auth = keystone.get_auth('cinder')
     session = _get_cinder_session()
+    # Used the service cached session to get the endpoint
+    # because getting an endpoint requires auth!
+    endpoint = keystone.get_endpoint('cinder', session=session,
+                                     auth=service_auth)
+    project_id = None
+    if hasattr(service_auth, 'get_project_id'):
+        # Being moderately defensive so we don't have to mock this in
+        # every cinder unit test.
+        project_id = service_auth.get_project_id(session)
+    if project_id and project_id in str(endpoint):
+        # We've found the project ID in the endpoint URL due to the
+        # endpoint configuration, however this is not required in
+        # more modern versions of cinder, and if you attempt to access
+        # a resource with a project ID in the URL, and with a service
+        # token, the request gets a Bad Request response.
+        # This works starting at Cinder microversion 3.67 - Yoga.
+        endpoint = endpoint.replace("/%s" % project_id, "")
+
+    if not context:
+        context = ironic_context.RequestContext(auth_token=None)
+
+    user_auth = None
+    if CONF.cinder.auth_type != 'none' and context.auth_token:
+        user_auth = keystone.get_service_auth(
+            context, endpoint, service_auth,
+            only_service_auth=auth_from_config)
+
+    if auth_from_config:
+        # If we are here, then we've been requested to *only* use our supplied
+        sess = keystone.get_session('cinder', timeout=CONF.cinder.timeout,
+                                    auth=service_auth)
+    else:
+        sess = keystone.get_session('cinder', timeout=CONF.cinder.timeout,
+                                    auth=user_auth or service_auth)
+
+        # Re-determine the endpoint so we can work with versions prior to
+        # Yoga, becuase the endpoint, based upon configuration, may require
+        # project_id specific URLs.
+        if user_auth:
+            endpoint = keystone.get_endpoint('cinder', session=sess,
+                                             auth=user_auth)
 
-    # TODO(pas-ha) use versioned endpoint data to select required
-    # cinder api version
-    cinder_url = keystone.get_endpoint('cinder', session=session,
-                                       auth=service_auth)
-    # TODO(pas-ha) investigate possibility of passing a user context here,
-    # similar to what neutron/glance-related code does
     # NOTE(pas-ha) cinderclient has both 'connect_retries' (passed to
     # ksa.Adapter) and 'retries' (used in its subclass of ksa.Adapter) options.
     # The first governs retries on establishing the HTTP connection,
     # the second governs retries on OverLimit exceptions from API.
     # The description of [cinder]/retries fits the first,
     # so this is what we pass.
-    return client.Client(session=session, auth=service_auth,
-                         endpoint_override=cinder_url,
+    return client.Client(session=sess, auth=user_auth,
+                         endpoint_override=endpoint,
                          connect_retries=CONF.cinder.retries,
                          global_request_id=context.global_id)
 
@@ -134,17 +172,21 @@ def _create_metadata_dictionary(node, action):
     return {label: json.dumps(data)}
 
 
-def _init_client(task):
+def _init_client(task, auth_from_config=False):
     """Obtain cinder client and return it for use.
 
     :param task: TaskManager instance representing the operation.
+    :param auth_from_config: If we should source our authentication parameters
+                             from the configured service as opposed to request
+                             context.
 
     :returns: A cinder client.
     :raises: StorageError If an exception is encountered creating the client.
     """
     node = task.node
     try:
-        return get_client(task.context)
+        return get_client(task.context,
+                          auth_from_config=auth_from_config)
     except Exception as e:
         msg = (_('Failed to initialize cinder client for operations on node '
                  '%(uuid)s: %(err)s') % {'uuid': node.uuid, 'err': e})
@@ -238,8 +280,9 @@ def attach_volumes(task, volume_list, connector):
                    }]
        """
     node = task.node
+    LOG.debug('Initializing volume attach for node %(node)s.',
+              {'node': node.uuid})
     client = _init_client(task)
-
     connected = []
     for volume_id in volume_list:
         try:
@@ -367,8 +410,10 @@ def _handle_errors(msg):
             LOG.error(msg)
             raise exception.StorageError(msg)
 
-    client = _init_client(task)
+    client = _init_client(task, auth_from_config=False)
     node = task.node
+    LOG.debug('Initializing volume detach for node %(node)s.',
+              {'node': node.uuid})
 
     for volume_id in volume_list:
         try:

ironic/common/keystone.py

@@ -125,7 +125,8 @@ def get_endpoint(group, **adapter_kwargs):
     return result
 
 
-def get_service_auth(context, endpoint, service_auth):
+def get_service_auth(context, endpoint, service_auth,
+                     only_service_auth=False):
     """Create auth plugin wrapping both user and service auth.
 
     When properly configured and using auth_token middleware,
@@ -134,8 +135,25 @@ def get_service_auth(context, endpoint, service_auth):
 
     Ideally we would use the plugin provided by auth_token middleware
     however this plugin isn't serialized yet.
+
+    :param context: The RequestContext instance from which the user
+                    auth_token is extracted.
+    :param endpoint: The requested endpoint to be utilized.
+    :param service_auth: The service authenticaiton credentals to be
+                         used.
+    :param only_service_auth: Boolean, default False. When set to True,
+                              the resulting Service token pair is generated
+                              as if it originates from the user itself.
+                              Useful to cast admin level operations which are
+                              launched by Ironic itself, as opposed to user
+                              initiated requests.
+    :returns: Returns a service token via the ServiceTokenAuthWrapper
+              class.
     """
-    # TODO(pas-ha) use auth plugin from context when it is available
-    user_auth = token_endpoint.Token(endpoint, context.auth_token)
+    user_auth = None
+    if not only_service_auth:
+        user_auth = token_endpoint.Token(endpoint, context.auth_token)
+    else:
+        user_auth = service_auth
     return service_token.ServiceTokenAuthWrapper(user_auth=user_auth,
                                                  service_auth=service_auth)

ironic/tests/unit/common/test_cinder.py

@@ -58,8 +58,7 @@ def test__get_cinder_session(self, mock_keystone_session, mock_auth):
 @mock.patch('ironic.common.keystone.get_adapter', autospec=True)
 @mock.patch('ironic.common.keystone.get_service_auth', autospec=True,
             return_value=mock.sentinel.sauth)
-@mock.patch('ironic.common.keystone.get_auth', autospec=True,
-            return_value=mock.sentinel.auth)
+@mock.patch('ironic.common.keystone.get_auth', autospec=True)
 @mock.patch('ironic.common.keystone.get_session', autospec=True,
             return_value=mock.sentinel.session)
 @mock.patch.object(cinderclient.Client, '__init__', autospec=True,
@@ -74,8 +73,11 @@ def setUp(self):
         cinder._CINDER_SESSION = None
         self.context = context.RequestContext(global_request_id='global')
 
-    def _assert_client_call(self, init_mock, url, auth=mock.sentinel.auth):
-        cinder.get_client(self.context)
+    def _assert_client_call(self, init_mock, url, auth=mock.sentinel.auth,
+                            auth_from_config=None):
+        if not auth_from_config:
+            self.context.auth_token = 'meow'
+        cinder.get_client(self.context, auth_from_config=auth_from_config)
         init_mock.assert_called_once_with(
             mock.ANY,
             session=mock.sentinel.session,
@@ -86,15 +88,45 @@ def _assert_client_call(self, init_mock, url, auth=mock.sentinel.auth):
 
     def test_get_client(self, mock_client_init, mock_session, mock_auth,
                         mock_sauth, mock_adapter):
-
+        mock_auth.return_value = mock_auth_obj = mock.Mock()
+        mock_auth_obj.get_project_id.return_value = '1111'
+        mock_adapter.return_value = mock_adapter_obj = mock.Mock()
+        mock_adapter_obj.get_endpoint.side_effect = iter([
+            'cinder_url/1111',
+            'cinder_url'])
+        self._assert_client_call(mock_client_init, 'cinder_url',
+                                 auth=mock.sentinel.sauth)
+        mock_session.assert_has_calls([
+            mock.call('cinder'),
+            mock.call('cinder', timeout=1, auth=mock.sentinel.sauth)])
+        mock_auth.assert_called_once_with('cinder')
+        mock_adapter.assert_has_calls([
+            mock.call('cinder', session=mock.sentinel.session,
+                      auth=mock_auth_obj),
+
+            mock.call('cinder', session=mock.sentinel.session,
+                      auth=mock.sentinel.sauth)])
+        self.assertTrue(mock_sauth.called)
+
+    def test_get_client_service_token(self, mock_client_init, mock_session,
+                                      mock_auth, mock_sauth, mock_adapter):
+        mock_auth.return_value = mock_auth_obj = mock.Mock()
+        mock_auth_obj.get_project_id.return_value = '1111'
         mock_adapter.return_value = mock_adapter_obj = mock.Mock()
-        mock_adapter_obj.get_endpoint.return_value = 'cinder_url'
-        self._assert_client_call(mock_client_init, 'cinder_url')
-        mock_session.assert_called_once_with('cinder')
+        mock_adapter_obj.get_endpoint.side_effect = iter([
+            'cinder_url/1111',
+            'cinder_url'])
+        self._assert_client_call(
+            mock_client_init,
+            'cinder_url',
+            auth=None,
+            auth_from_config=True)
+        mock_session.assert_has_calls([
+            mock.call('cinder'),
+            mock.call('cinder', timeout=1, auth=mock_auth_obj)])
         mock_auth.assert_called_once_with('cinder')
-        mock_adapter.assert_called_once_with('cinder',
-                                             session=mock.sentinel.session,
-                                             auth=mock.sentinel.auth)
+        mock_adapter.assert_called_once_with(
+            'cinder', session=mock.sentinel.session, auth=mock_auth_obj)
         self.assertFalse(mock_sauth.called)
 
 

releasenotes/notes/cinder-2019892-6b5a9de5c5f05aa6.yaml

@@ -0,0 +1,16 @@
+---
+fixes:
+  - |
+    Fixes Ironic integration with Cinder because of changes which resulted as
+    part of the recent Security related fix in
+    `bug 2004555 <https://launchpad.net/bugs/2004555>`_. The work in Ironic
+    to track this fix was logged in
+    `bug 2019892 <https://bugs.launchpad.net/ironic/+bug/2019892>`_.
+    Ironic now sends a service token to Cinder, which allows for access
+    restrictions added as part of the original CVE-2023-2088
+    fix to be appropriately bypassed. Ironic was not vulnerable,
+    but the restrictions added as a result did impact Ironic's usage.
+    This is because Ironic volume attachments are not on a shared
+    "compute node", but instead mapped to the physical machines
+    and Ironic handles the attachment life-cycle after initial
+    attachment.