From 301d8f580f4ce6a4f6089f877c0f200ff4ff633b Mon Sep 17 00:00:00 2001 From: Ghanshyam Maan Date: Thu, 28 Aug 2025 03:19:27 +0000 Subject: [PATCH] Fix glance service policy rule Glance service APIs are default to 'service_roles: service' - https://github.com/openstack/glance/blob/6c33a667a9f5ddce07b6131f4a5cb7460a4bdf17/glance/policies/base.py#L116 The issue here is the service token, which is sent from the service for the user token expiry case but glance uses that service token (keystonemiddleware sets the service token roles in Requestcontext in 'service_roles' field) for RBAC, which is not correct. The OpenStack services communicate with each other by passing the user token and service token wrapped in keystoneauth's ServiceTokenAuthWrapper. The only purpose of passing the service token is for long-running operations and in case the user token gets expired. For RBAC, we need to check if a user token has the 'service' role or not. Service needs to load the configured user auth plugin (where the user should have the 'service' role) from keystoneauth and pass that to the other services (for example, cinder change depends-on) and glance will use that user role to verify the policy permission. To fix that, we need to make the service APIs default to ``role:service`` and not `service_role`:`service`. This commit does one more change. Cinder does not have the way to configure the glance service user, we are adding the new config in this release. For backward compatibility, we need to allow admin access in service policy rule. In future release (after one SLURP release), we cna remove the admin access. Closes-Bug: #2121622 Co-Authored-By: : Sean Mooney Change-Id: I50909e6bdb3227ca99b7eba642546da791f9552a Signed-off-by: Sean Mooney Signed-off-by: Ghanshyam Maan --- glance/policies/base.py | 7 +- glance/tests/functional/v2/test_images.py | 81 +++++++++++++++---- .../service-api-default-eae660940ed97010.yaml | 6 ++ 3 files changed, 79 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/service-api-default-eae660940ed97010.yaml diff --git a/glance/policies/base.py b/glance/policies/base.py index ff27d20301..d00e7b1128 100644 --- a/glance/policies/base.py +++ b/glance/policies/base.py @@ -113,7 +113,12 @@ rules = [ policy.RuleDefault(name='context_is_admin', check_str='role:admin', description='Defines the rule for the is_admin:True ' 'check.'), - policy.RuleDefault(name='service_api', check_str='service_roles:service', + # TODO(gmaan): The admin role access is added for backward compatibility + # because some services does not send the service role token to glance. + # Remove the admin role access from service rule in 2026.2 or later(after + # 2026.1 SLURP release). + policy.RuleDefault(name='service_api', + check_str='role:service or role:admin', description='Default rule for the service-to-service ' 'API.'), ] diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py index 038b7800fe..27f2e33317 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -2465,7 +2465,7 @@ class TestImagesSingleStore(functional.SynchronousAPIBase): ) # Get locations of `queued` image - headers = self._headers({'X-Service-Roles': 'service'}) + headers = self._headers({'X-Roles': 'service'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(200, response.status_code, response.text) @@ -2493,14 +2493,28 @@ class TestImagesSingleStore(functional.SynchronousAPIBase): delay_sec=0.2, start_delay_sec=1) - # Get Locations not allowed for any other user - headers = self._headers({'X-Roles': 'admin,member'}) + self.set_policy_rules({ + 'get_image': 'role:member', + 'fetch_image_location': 'role:service or role:admin', + }) + + # Get Locations not allowed for any non-admin or non-service user + headers = self._headers({'X-Roles': 'member'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(http.FORBIDDEN, response.status_code, response.text) - # Get Locations allowed only for service user - headers = self._headers({'X-Service-Roles': 'service'}) + # TODO(gmaan): For backward compatibility, we are allowing admin + # user to access service only rules, but once we remove that + # access, we need to assert here that the admin cannot access the + # service only rules. + headers = self._headers({'X-Roles': 'admin'}) + path = '/v2/images/%s/locations' % image_id + response = self.api_get(path, headers=headers) + self.assertEqual(200, response.status_code, response.text) + + # Get Locations allowed for service user + headers = self._headers({'X-Roles': 'service'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(200, response.status_code, response.text) @@ -2515,15 +2529,30 @@ class TestImagesSingleStore(functional.SynchronousAPIBase): # Upload some image data image_data = b'ZZZZZ' self.api_methods.upload_and_verify(image_id, image_data) + self.set_policy_rules({ + 'get_image': 'role:member', + 'fetch_image_location': 'role:service or role:admin', + }) - # Get Locations not allowed for any other user - headers = self._headers({'X-Roles': 'admin,member'}) + # Get Locations not allowed for any non-admin or non-service user + headers = self._headers({'X-Roles': 'member'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(http.FORBIDDEN, response.status_code, response.text) + # TODO(gmaan): For backward compatibility, we are allowing admin + # user to access service only rules, but once we remove that + # access, we need to assert here that the admin cannot access the + # service only rules. + headers = self._headers({'X-Roles': 'admin'}) + path = '/v2/images/%s/locations' % image_id + response = self.api_get(path, headers=headers) + self.assertEqual(200, response.status_code, response.text) + output = jsonutils.loads(response.text) + self.assertTrue(output[0]['url']) + # Get Locations allowed only for service user - headers = self._headers({'X-Service-Roles': 'service'}) + headers = self._headers({'X-Roles': 'service'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(200, response.status_code, response.text) @@ -4588,13 +4617,20 @@ class TestStoreWeight(functional.SynchronousAPIBase): class TestMultipleBackendsLocationApi(functional.SynchronousAPIBase): def setUp(self): super(TestMultipleBackendsLocationApi, self).setUp() - self.start_server() for i in range(3): ret = test_utils.start_http_server("foo_image_id%d" % i, "foo_image%d" % i) setattr(self, 'http_server%d' % i, ret[1]) setattr(self, 'http_port%d' % i, ret[2]) + self.policy = policy.Enforcer(suppress_deprecation_warnings=True) + self.start_server() + + def start_server(self): + with mock.patch.object(policy, 'Enforcer') as mock_enf: + mock_enf.return_value = self.policy + super(TestMultipleBackendsLocationApi, self).start_server() + def setup_stores(self): pass @@ -4918,7 +4954,7 @@ class TestMultipleBackendsLocationApi(functional.SynchronousAPIBase): self.assertEqual('queued', image['status']) # Get location of `queued` image - headers = self._headers({'X-Service-Roles': 'service'}) + headers = self._headers({'X-Roles': 'service'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(200, response.status_code, response.text) @@ -4946,14 +4982,31 @@ class TestMultipleBackendsLocationApi(functional.SynchronousAPIBase): delay_sec=0.2, start_delay_sec=1, multistore=True) - # Get Locations not allowed for any other user - headers = self._headers({'X-Roles': 'admin,member'}) + self.policy.set_rules( + oslo_policy.policy.Rules.from_dict({ + 'get_image': 'role:member', + 'fetch_image_location': 'role:service or role:admin'}), + overwrite=True) + + # Get Locations not allowed for any non-admin or non-service user + headers = self._headers({'X-Roles': 'member'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(http.FORBIDDEN, response.status_code, response.text) - # Get Locations allowed only for service user - headers = self._headers({'X-Service-Roles': 'service'}) + # TODO(gmaan): For backward compatibility, we are allowing admin + # user to access service only rules, but once we remove that + # access, we need to assert here that the admin cannot access the + # service only rules. + headers = self._headers({'X-Roles': 'admin'}) + path = '/v2/images/%s/locations' % image_id + response = self.api_get(path, headers=headers) + self.assertEqual(200, response.status_code, response.text) + output = jsonutils.loads(response.text) + self.assertTrue(output[0]['url']) + + # Get Locations allowed for service user + headers = self._headers({'X-Roles': 'service'}) path = '/v2/images/%s/locations' % image_id response = self.api_get(path, headers=headers) self.assertEqual(200, response.status_code, response.text) diff --git a/releasenotes/notes/service-api-default-eae660940ed97010.yaml b/releasenotes/notes/service-api-default-eae660940ed97010.yaml new file mode 100644 index 0000000000..e1448a92a2 --- /dev/null +++ b/releasenotes/notes/service-api-default-eae660940ed97010.yaml @@ -0,0 +1,6 @@ +--- +deprecations: + - | + The ``add_image_location`` and ``fetch_image_location`` APIs policy rule + default value ``role:admin or role:service`` is deprecated and will be + changed to ``role:service`` in future release.