openstack/ironic
Code Issues Proposed changes

Fix Cinder Integration fallout from CVE-2023-2088

Browse Source 
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
This commit is contained in:
Julia Kreger 2023-05-11 11:09:28 -07:00
parent 912dcbbdc9
commit 9c0b4c90a1
6 changed files with 141 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
devstack/lib/ironic
View File

@ -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

71
ironic/common/cinder.py
View File

@ -19,6 +19,7 @@ from cinderclient import exceptions as cinder_exceptions
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 detach_volumes(task, volume_list, connector, allow_errors=False):
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:

24
ironic/common/keystone.py
View File

@ -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)

54
ironic/tests/unit/common/test_cinder.py
View File

@ -58,8 +58,7 @@ class TestCinderSession(base.TestCase):
@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 @@ class TestCinderClient(base.TestCase):
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 @@ class TestCinderClient(base.TestCase):
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.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=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_called_once_with('cinder',
session=mock.sentinel.session,
auth=mock.sentinel.auth)
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.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_auth_obj)
self.assertFalse(mock_sauth.called)

16
releasenotes/notes/cinder-2019892-6b5a9de5c5f05aa6.yaml Normal file
View File

@ -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.

13
zuul.d/project.yaml
View File

@ -32,13 +32,7 @@
- ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode:
voting: false
- ironic-tempest-bios-ipmi-direct-tinyipa
# Currently broken, fallout from OSSA-2023-003 which was a breaking
# change. Ironic needs to ensure we use a service token, not the
# admin token credentials, and we then may need to revise our tempest
# plugin once we have that sorted.
# bugs.launchpad.net/ironic/+bug/2019892
- ironic-tempest-bfv:
voting: false
- ironic-tempest-bfv
- ironic-tempest-ipa-partition-uefi-pxe-grub2
- metalsmith-integration-glance-centos8-legacy
# Non-voting jobs
@ -86,10 +80,7 @@
# seeming to be
# - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode
- ironic-tempest-bios-ipmi-direct-tinyipa
# NOTE(TheJulia): Disabled as of 20230516 until we can get the
# Cinder boot from volume integration sorted.
# https://bugs.launchpad.net/ironic/+bug/2019892
# - ironic-tempest-bfv
- ironic-tempest-bfv
- ironic-tempest-ipa-partition-uefi-pxe-grub2
- metalsmith-integration-glance-centos8-legacy
experimental: