From 854f059b82506133d5b0ad37ef57ed4f5635282a Mon Sep 17 00:00:00 2001 From: satoshi-sh Date: Mon, 7 Apr 2025 14:11:37 +0000 Subject: [PATCH] Improve is_image_available - Add 'community' visiblity alongside 'public' - Make the auth token check configurable with a new option 'allow_image_access_via_auth_token' (default: OFF in master, ON is stable) - Add testing for is_image_available Partial-Bug: #2099276 Change-Id: I10df3e8fd0091e70f3fb1bc19524aada296c13c1 --- ironic/common/glance_service/service_utils.py | 51 +++++++++++--- ironic/conf/default.py | 6 ++ .../tests/unit/common/test_glance_service.py | 68 +++++++++++++++++-- ...ccess_via_auth_token-1b5869f1c0999bea.yaml | 14 ++++ 4 files changed, 125 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/add_allow_image_access_via_auth_token-1b5869f1c0999bea.yaml diff --git a/ironic/common/glance_service/service_utils.py b/ironic/common/glance_service/service_utils.py index 54851f24c5..82c801307c 100644 --- a/ironic/common/glance_service/service_utils.py +++ b/ironic/common/glance_service/service_utils.py @@ -22,6 +22,7 @@ from oslo_utils import timeutils from oslo_utils import uuidutils from ironic.common import exception +from ironic.common import keystone from ironic.conf import CONF _IMAGE_ATTRIBUTES = ['size', 'disk_format', 'owner', @@ -34,6 +35,7 @@ _IMAGE_ATTRIBUTES = ['size', 'disk_format', 'owner', LOG = log.getLogger(__name__) +_GLANCE_SESSION = None def _extract_attributes(image): @@ -123,28 +125,40 @@ def is_image_available(context, image): This check is needed in case Nova and Glance are deployed without authentication turned on. """ + # NOTE: Any support for private/shared images in Ironic requires a secure + # way for ironic to know the original requester: + # - If we trust node[instance_info][project_id], we are susceptible to a + # node.owner stealing another project's private image by lying in + # instance_info. + # - As of 2025.1, the project_id attached to the auth context at this + # point is more likely to be the nova-computes service user rather + # than the original requester. This is a missing feature from the + # Ironic/Nova virt driver. + auth_token = getattr(context, 'auth_token', None) + conductor_project_id = get_conductor_project_id() image_visibility = getattr(image, 'visibility', None) image_owner = getattr(image, 'owner', None) image_id = getattr(image, 'id', 'unknown') is_admin = 'admin' in getattr(context, 'roles', []) - project_id = getattr(context, 'project_id', None) project = getattr(context, 'project', 'unknown') - # The presence of an auth token implies this is an authenticated - # request and we need not handle the noauth use-case. - if auth_token: + # If an auth token is present and the config allows access via auth token, + # allow image access. + if CONF.allow_image_access_via_auth_token and auth_token: # We return true here since we want the *user* request context to # be able to be used. return True - - if image_visibility == 'public': + # If the image visibility is public or community, allow access. + if image_visibility in ['public', 'community']: return True - - if project_id and image_owner == project_id: - return True - + # If the user is an admin and the config allows ignoring project checks for + # admin tasks, allow access. if is_admin and CONF.ignore_project_check_for_admin_tasks: return True + # If the image is private and the owner is the conductor project, + # allow access. + if image_visibility == 'private' and image_owner == conductor_project_id: + return True LOG.info( 'Access to %s owned by %s denied to requester %s', @@ -167,3 +181,20 @@ def is_glance_image(image_href): return False return (image_href.startswith('glance://') or uuidutils.is_uuid_like(image_href)) + + +def get_conductor_project_id(): + global _GLANCE_SESSION + if not _GLANCE_SESSION: + _GLANCE_SESSION = keystone.get_session('glance') + session = _GLANCE_SESSION + service_auth = keystone.get_auth('glance') + + try: + if service_auth and hasattr(service_auth, 'get_project_id'): + return service_auth.get_project_id(session) + elif hasattr(session, 'get_project_id') and session.auth: + return session.get_project_id() + except Exception as e: + LOG.debug("Error getting conductor project ID: %s", str(e)) + return None diff --git a/ironic/conf/default.py b/ironic/conf/default.py index 3646c15407..639273fa44 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -69,6 +69,12 @@ api_opts = [ default='/etc/ironic/htpasswd', help=_('Path to Apache format user authentication file used ' 'when auth_strategy=http_basic')), + cfg.BoolOpt('allow_image_access_via_auth_token', + default=False, + deprecated_for_removal=True, + help=_('If True, Ironic allows access to Glance images if an ' + 'auth_token is present in the request context.') + ), cfg.BoolOpt( 'ignore_project_check_for_admin_tasks', default=True, diff --git a/ironic/tests/unit/common/test_glance_service.py b/ironic/tests/unit/common/test_glance_service.py index f997dea64f..9d9344faf9 100644 --- a/ironic/tests/unit/common/test_glance_service.py +++ b/ironic/tests/unit/common/test_glance_service.py @@ -102,7 +102,8 @@ class TestGlanceImageService(base.TestCase): fixture = {'name': None, 'owner': None, 'properties': {}, - 'status': "active"} + 'status': "active", + 'visibility': "public"} fixture.update(kwargs) return openstack.image.v2.image.Image.new(**fixture) @@ -146,7 +147,7 @@ class TestGlanceImageService(base.TestCase): 'status': "active", 'tags': [], 'updated_at': None, - 'visibility': None, + 'visibility': "public", 'os_hash_algo': None, 'os_hash_value': None, } @@ -188,6 +189,7 @@ class TestGlanceImageService(base.TestCase): class MyGlanceStubClient(stubs.StubGlanceClient): """A client that fails the first time, then succeeds.""" + def get_image(self, image_id): if tries[0] == 0: tries[0] = 1 @@ -229,10 +231,12 @@ class TestGlanceImageService(base.TestCase): 'image contains no data', self.service.download, image_id) + @mock.patch.object(service_utils, '_GLANCE_SESSION', autospec=True) @mock.patch('os.sendfile', autospec=True) @mock.patch('os.path.getsize', autospec=True) @mock.patch('%s.open' % __name__, new=mock.mock_open(), create=True) - def test_download_file_url(self, mock_getsize, mock_sendfile): + def test_download_file_url(self, mock_getsize, mock_sendfile, + mock_serviceutils_glance): # NOTE: only in v2 API class MyGlanceStubClient(stubs.StubGlanceClient): @@ -241,8 +245,9 @@ class TestGlanceImageService(base.TestCase): s_tmpfname = '/whatever/source' def get_image(self, image_id): + direct_url = "file://%s" + self.s_tmpfname return type('GlanceTestDirectUrlMeta', (object,), - {'direct_url': 'file://%s' + self.s_tmpfname}) + dict(visibility='public', direct_url=direct_url)) stub_context = context.RequestContext(auth_token=True) stub_context.user_id = 'fake' @@ -251,6 +256,7 @@ class TestGlanceImageService(base.TestCase): stub_service = image_service.GlanceImageService(stub_client, context=stub_context) + mock_serviceutils_glance.return_value = stub_service image_id = uuidutils.generate_uuid() self.config(allowed_direct_url_schemes=['file'], group='glance') @@ -278,6 +284,7 @@ class TestGlanceImageService(base.TestCase): def test_client_forbidden_converts_to_imagenotauthed(self): class MyGlanceStubClient(stubs.StubGlanceClient): """A client that raises a Forbidden exception.""" + def get_image(self, image_id): raise openstack_exc.ForbiddenException() @@ -295,6 +302,7 @@ class TestGlanceImageService(base.TestCase): def test_client_notfound_converts_to_imagenotfound(self): class MyGlanceStubClient(stubs.StubGlanceClient): """A client that raises a NotFound exception.""" + def get_image(self, image_id): raise openstack_exc.NotFoundException() @@ -995,3 +1003,55 @@ class TestServiceUtils(base.TestCase): self.assertFalse(service_utils.is_glance_image(image_href)) image_href = None self.assertFalse(service_utils.is_glance_image(image_href)) + + +class TestIsImageAvailable(base.TestCase): + + def setUp(self): + super(TestIsImageAvailable, self).setUp() + self.image = mock.Mock() + self.context = context.RequestContext() + self.context.roles = [] + + def test_allow_access_via_auth_token_enabled(self): + self.context.auth_token = 'fake-token' + self.config(allow_image_access_via_auth_token=True) + self.assertTrue(service_utils.is_image_available( + self.context, self.image)) + + def test_allow_public_image(self): + self.image.visibility = 'public' + self.assertTrue(service_utils.is_image_available( + self.context, self.image)) + + def test_allow_community_image(self): + self.image.visibility = 'community' + self.assertTrue(service_utils.is_image_available( + self.context, self.image)) + + def test_allow_admin_if_config_enabled(self): + self.context.roles = ['admin'] + self.config(ignore_project_check_for_admin_tasks=True) + self.assertTrue(service_utils.is_image_available( + self.context, self.image)) + + def test_allow_private_image_owned_by_conductor(self): + self.image.visibility = 'private' + self.image.owner = service_utils.get_conductor_project_id() + self.assertTrue(service_utils.is_image_available( + self.context, self.image)) + + def test_deny_private_image_different_owner(self): + self.config(allow_image_access_via_auth_token=False) + self.config(ignore_project_check_for_admin_tasks=False) + + self.image.visibility = 'private' + self.image.owner = 'other-owner' + self.image.id = 'fake-id' + + self.context.project = 'test-project' + self.context.roles = [] + self.context.auth_token = None + + result = service_utils.is_image_available(self.context, self.image) + self.assertFalse(result) diff --git a/releasenotes/notes/add_allow_image_access_via_auth_token-1b5869f1c0999bea.yaml b/releasenotes/notes/add_allow_image_access_via_auth_token-1b5869f1c0999bea.yaml new file mode 100644 index 0000000000..1f1b9849f2 --- /dev/null +++ b/releasenotes/notes/add_allow_image_access_via_auth_token-1b5869f1c0999bea.yaml @@ -0,0 +1,14 @@ +--- +features: + - | + If `allow_image_access_via_auth_token` is set to `True`, Ironic allows access to + Glance images if an auth_token is present in the request context. +upgrade: + - | + CONF.allow_image_access_via_auth_token is set to `True` in this Ironic release. + OpenStack integrated operators should ensure images for Ironic use are using image + visibility "public" or "community" for the most reliable results. +deprecation: + - | + CONF.allow_image_access_via_auth_token is deprecated, and will be removed, + along with legacy image access logic, in or after the OpenStack 2026.2 release. \ No newline at end of file