From b20cc91e6f5c0a84965b0498c75b2ad40f361361 Mon Sep 17 00:00:00 2001 From: Pranali Deore Date: Fri, 10 Feb 2023 13:48:13 +0000 Subject: [PATCH] Remove deprecated ``enforce_secure_rbac`` option As per the revised SRBAC community goals, glance service is now switching to new defaults by default hence removing the deprecated ``enforce_secure_rbac`` option which is no longer needed. The ``enforce_secure_rbac`` option was introduced EXPERIMENTAL in Wallaby release for operators to opt into enforcing authorization based on common RBAC personas. Related blueprint secure-rbac Change-Id: I273527c85d30c1c09c086c73c892aaa6d127df6b --- glance/api/policy.py | 10 --- glance/api/v2/image_members.py | 3 +- glance/api/v2/policy.py | 21 ++++--- glance/cmd/api.py | 8 --- glance/common/config.py | 24 -------- glance/common/wsgi_app.py | 12 ---- glance/tests/functional/__init__.py | 13 ++-- glance/tests/unit/common/test_wsgi_app.py | 35 ----------- glance/tests/unit/test_policy.py | 23 ------- glance/tests/unit/v2/test_v2_policy.py | 61 ++++++++++++------- ...-enforce-secure-rbac-ec9a0249870460c2.yaml | 17 ++++++ 11 files changed, 77 insertions(+), 150 deletions(-) create mode 100644 releasenotes/notes/remove-enforce-secure-rbac-ec9a0249870460c2.yaml diff --git a/glance/api/policy.py b/glance/api/policy.py index d92d4df6d7..17043eb41a 100644 --- a/glance/api/policy.py +++ b/glance/api/policy.py @@ -25,7 +25,6 @@ from oslo_policy import policy from glance.common import exception from glance.domain import proxy -from glance.i18n import _LW from glance import policies @@ -64,15 +63,6 @@ class Enforcer(policy.Enforcer): if suppress_deprecation_warnings: self.suppress_deprecation_warnings = True self.register_defaults(policies.list_rules()) - if CONF.enforce_secure_rbac and CONF.oslo_policy.enforce_new_defaults: - LOG.warning(_LW( - "Deploying glance with secure RBAC personas enabled via " - "`glance-api.conf [DEFAULT] enforce_secure_rbac=True` and " - "`glance-api.conf [oslo_policy] enforce_new_defaults=True` " - "is marked as EXPERIMENTAL in Wallaby. The status of this " - "feature will graduate to SUPPORTED as glance adopts more " - "personas, specifically for system-scope." - )) def add_rules(self, rules): """Add new rules to the Rules object""" diff --git a/glance/api/v2/image_members.py b/glance/api/v2/image_members.py index 7c7b8d5cdb..97f5708f3f 100644 --- a/glance/api/v2/image_members.py +++ b/glance/api/v2/image_members.py @@ -82,7 +82,8 @@ class ImageMembersController(object): # NOTE(abhishekk): Ownership check only needs to performed while # adding new members to image owner = image.owner - if not CONF.enforce_secure_rbac and not context.is_admin: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope) and not context.is_admin: if ownership_check == 'create': if owner is None or owner != context.owner: message = _("You are not permitted to create image " diff --git a/glance/api/v2/policy.py b/glance/api/v2/policy.py index 8ad4046478..bce036cb51 100644 --- a/glance/api/v2/policy.py +++ b/glance/api/v2/policy.py @@ -225,7 +225,8 @@ class ImageAPIPolicy(APIPolicyBase): self._enforce('delete_image_location') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def get_image_location(self): @@ -247,7 +248,8 @@ class ImageAPIPolicy(APIPolicyBase): raise if 'visibility' in self._target: self._enforce_visibility(self._target['visibility']) - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_admin_or_same_owner(self._context, self._target) def get_image(self): @@ -260,14 +262,16 @@ class ImageAPIPolicy(APIPolicyBase): self._enforce('delete_image') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def upload_image(self): self._enforce('upload_image') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def download_image(self): @@ -277,21 +281,24 @@ class ImageAPIPolicy(APIPolicyBase): self._enforce('modify_image') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def deactivate_image(self): self._enforce('deactivate') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def reactivate_image(self): self._enforce('reactivate') # TODO(danms): Remove this legacy fallback when secure RBAC # replaces the legacy policy. - if not CONF.enforce_secure_rbac: + if not (CONF.oslo_policy.enforce_new_defaults or + CONF.oslo_policy.enforce_scope): check_is_image_mutable(self._context, self._image) def copy_image(self): diff --git a/glance/cmd/api.py b/glance/cmd/api.py index fa5b38d1e6..90511b510f 100644 --- a/glance/cmd/api.py +++ b/glance/cmd/api.py @@ -108,14 +108,6 @@ def main(): host=CONF.bind_host ) - if CONF.enforce_secure_rbac != CONF.oslo_policy.enforce_new_defaults: - fail_message = ( - "[DEFAULT] enforce_secure_rbac does not match " - "[oslo_policy] enforce_new_defaults. Please set both to " - "True to enable secure RBAC personas. Otherwise, make sure " - "both are False.") - raise exception.ServerError(fail_message) - # NOTE(danms): Configure system-wide threading model to use eventlet glance.async_.set_threadpool_model('eventlet') diff --git a/glance/common/config.py b/glance/common/config.py index cff15ab1d4..b118380057 100644 --- a/glance/common/config.py +++ b/glance/common/config.py @@ -593,30 +593,6 @@ Related options: Related options: * [DEFAULT]/node_staging_uri""")), - cfg.BoolOpt('enforce_secure_rbac', default=True, - deprecated_for_removal=True, - deprecated_reason=_(""" -This option has been introduced to require operators to opt into enforcing -authorization based on common RBAC personas, which is EXPERIMENTAL as of the -Wallaby release. This behavior will be the default and STABLE in a future -release, allowing this option to be removed. -"""), - deprecated_since='Wallaby', - help=_(""" -Enforce API access based on common persona definitions used across OpenStack. -Enabling this option formalizes project-specific read/write operations, like -creating private images or updating the status of shared image, behind the -`member` role. It also formalizes a read-only variant useful for -project-specific API operations, like listing private images in a project, -behind the `reader` role. - -Operators should take an opportunity to understand glance's new image policies, -audit assignments in their deployment, and update permissions using the default -roles in keystone (e.g., `admin`, `member`, and `reader`). - -Related options: - * [oslo_policy]/enforce_new_defaults -""")), cfg.StrOpt('worker_self_reference_url', default=None, help=_(""" diff --git a/glance/common/wsgi_app.py b/glance/common/wsgi_app.py index 2a3168e0dc..ab1f121701 100644 --- a/glance/common/wsgi_app.py +++ b/glance/common/wsgi_app.py @@ -22,7 +22,6 @@ import osprofiler.initializer from glance.api import common import glance.async_ from glance.common import config -from glance.common import exception from glance.common import store_utils from glance import housekeeping from glance.i18n import _ @@ -80,16 +79,6 @@ def _setup_os_profiler(): host=CONF.bind_host) -def _validate_policy_enforcement_configuration(): - if CONF.enforce_secure_rbac != CONF.oslo_policy.enforce_new_defaults: - fail_message = ( - "[DEFAULT] enforce_secure_rbac does not match " - "[oslo_policy] enforce_new_defaults. Please set both to " - "True to enable secure RBAC personas. Otherwise, make sure " - "both are False.") - raise exception.ServerError(fail_message) - - def drain_workers(): # NOTE(danms): If there are any other named pools that we need to # drain before exit, they should be in this list. @@ -155,5 +144,4 @@ def init_app(): run_staging_cleanup() _setup_os_profiler() - _validate_policy_enforcement_configuration() return config.load_paste_app('glance-api') diff --git a/glance/tests/functional/__init__.py b/glance/tests/functional/__init__.py index a17c67c395..2573c3e22b 100644 --- a/glance/tests/functional/__init__.py +++ b/glance/tests/functional/__init__.py @@ -408,9 +408,7 @@ class ApiServer(Server): self.image_location_quota = 2 self.disable_path = None - secure_rbac = bool(os.getenv('OS_GLANCE_TEST_RBAC_DEFAULTS')) - self.enforce_secure_rbac = secure_rbac - self.enforce_new_defaults = secure_rbac + self.enforce_new_defaults = True self.needs_database = True default_sql_connection = SQLITE_CONN_TEMPLATE % self.test_dir @@ -456,7 +454,6 @@ image_location_quota=%(image_location_quota)s location_strategy=%(location_strategy)s allow_additional_image_properties = True node_staging_uri=%(node_staging_uri)s -enforce_secure_rbac=%(enforce_secure_rbac)s [oslo_policy] policy_file = %(policy_file)s policy_default_rule = %(policy_default_rule)s @@ -585,7 +582,6 @@ class ApiServerForMultipleBackend(Server): self.image_location_quota = 2 self.disable_path = None - self.enforce_secure_rbac = True self.enforce_new_defaults = True self.needs_database = True @@ -629,7 +625,6 @@ image_location_quota=%(image_location_quota)s location_strategy=%(location_strategy)s allow_additional_image_properties = True enabled_backends=file1:file,file2:file,file3:file -enforce_secure_rbac=%(enforce_secure_rbac)s [oslo_policy] policy_file = %(policy_file)s policy_default_rule = %(policy_default_rule)s @@ -1620,9 +1615,9 @@ class SynchronousAPIBase(test_utils.BaseTestCase): self.api = config.load_paste_app(root_app, conf_file=self.paste_config) - secure_rbac = bool(os.getenv('OS_GLANCE_TEST_RBAC_DEFAULTS')) - self.config(enforce_secure_rbac=secure_rbac) - self.config(enforce_new_defaults=secure_rbac, + self.config(enforce_new_defaults=True, + group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') def _headers(self, custom_headers=None): diff --git a/glance/tests/unit/common/test_wsgi_app.py b/glance/tests/unit/common/test_wsgi_app.py index 5b7cb82129..87afc40d7d 100644 --- a/glance/tests/unit/common/test_wsgi_app.py +++ b/glance/tests/unit/common/test_wsgi_app.py @@ -19,7 +19,6 @@ from unittest import mock from glance.api import common from glance.api.v2 import cached_images import glance.async_ -from glance.common import exception from glance.common import wsgi_app from glance.tests import utils as test_utils @@ -91,40 +90,6 @@ class TestWsgiAppInit(test_utils.BaseTestCase): wsgi_app.drain_workers() self.assertIsNone(cached_images.WORKER) - @mock.patch('glance.common.config.load_paste_app') - @mock.patch('glance.async_.set_threadpool_model') - @mock.patch('glance.common.wsgi_app._get_config_files') - def test_policy_enforcement_kills_service_if_misconfigured( - self, mock_load_app, mock_set, mock_config_files): - self.config(enforce_new_defaults=True, group='oslo_policy') - self.config(enforce_secure_rbac=False) - self.assertRaises(exception.ServerError, wsgi_app.init_app) - - self.config(enforce_new_defaults=False, group='oslo_policy') - self.config(enforce_secure_rbac=True) - self.assertRaises(exception.ServerError, wsgi_app.init_app) - - @mock.patch('glance.common.config.load_paste_app') - @mock.patch('glance.async_.set_threadpool_model') - @mock.patch('glance.common.wsgi_app._get_config_files') - def test_policy_enforcement_valid_truthy_configuration( - self, mock_load_app, mock_set, mock_config_files): - self.config(enforce_new_defaults=True, group='oslo_policy') - self.config(enforce_secure_rbac=True) - self.assertTrue(wsgi_app.init_app()) - - @mock.patch('glance.common.config.load_paste_app') - @mock.patch('glance.async_.set_threadpool_model') - @mock.patch('glance.common.wsgi_app._get_config_files') - def test_policy_enforcement_valid_falsy_configuration( - self, mock_load_app, mock_set, mock_config_files): - # This is effectively testing the default values, but we're doing that - # to make sure nothing bad happens at runtime in the default case when - # validating policy enforcement configuration. - self.config(enforce_new_defaults=False, group='oslo_policy') - self.config(enforce_secure_rbac=False) - self.assertTrue(wsgi_app.init_app()) - @mock.patch('glance.async_._THREADPOOL_MODEL', new=None) @mock.patch('glance.common.config.load_paste_app') @mock.patch('glance.common.wsgi_app._get_config_files') diff --git a/glance/tests/unit/test_policy.py b/glance/tests/unit/test_policy.py index 9f26d0e297..b372685924 100644 --- a/glance/tests/unit/test_policy.py +++ b/glance/tests/unit/test_policy.py @@ -394,29 +394,6 @@ class TestPolicyEnforcer(base.IsolatedUnitTest): enforcer.check(context, 'foo', {}) mock_enforcer.assert_called_once_with('foo', {}, context) - def test_ensure_experimental_warning_is_logged_for_secure_rbac(self): - self.config(enforce_new_defaults=True, group='oslo_policy') - self.config(enforce_secure_rbac=True) - expected_log_string = ( - "Deploying glance with secure RBAC personas enabled via " - "`glance-api.conf [DEFAULT] enforce_secure_rbac=True` and " - "`glance-api.conf [oslo_policy] enforce_new_defaults=True` " - "is marked as EXPERIMENTAL in Wallaby. The status of this " - "feature will graduate to SUPPORTED as glance adopts more " - "personas, specifically for system-scope." - ) - with mock.patch.object(glance.api.policy, 'LOG') as mock_log: - glance.api.policy.Enforcer( - suppress_deprecation_warnings=True) - mock_log.warning.assert_called_once_with(expected_log_string) - - def test_ensure_experimental_warning_is_not_logged_for_legacy_rbac(self): - self.config(enforce_new_defaults=False, group='oslo_policy') - with mock.patch.object(glance.api.policy, 'LOG') as mock_log: - glance.api.policy.Enforcer( - suppress_deprecation_warnings=True) - mock_log.warning.assert_not_called() - class TestPolicyEnforcerNoFile(base.IsolatedUnitTest): diff --git a/glance/tests/unit/v2/test_v2_policy.py b/glance/tests/unit/v2/test_v2_policy.py index ae04c7de84..18b65e66c1 100644 --- a/glance/tests/unit/v2/test_v2_policy.py +++ b/glance/tests/unit/v2/test_v2_policy.py @@ -142,7 +142,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_delete_locations_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True @@ -166,8 +167,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.delete_locations() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=True and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.delete_locations() self.assertFalse(m.called) @@ -235,7 +238,8 @@ class APIImagePolicy(APIPolicyBase): generic_target) def test_add_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') self.context.is_admin = False self.policy = policy.ImageAPIPolicy(self.context, {'owner': 'else'}, @@ -252,7 +256,8 @@ class APIImagePolicy(APIPolicyBase): # Make sure we are not calling the legacy handler if # secure_rbac is being used. We won't fail the check because # our enforcer is a mock, just make sure we don't call that handler. - self.config(enforce_secure_rbac=True) + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_admin_or_same_owner') as m: self.policy.add_image() m.assert_not_called() @@ -287,8 +292,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_delete_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) - + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True self.context.owner = 'someuser' @@ -311,8 +316,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.delete_image() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=True and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.delete_image() self.assertFalse(m.called) @@ -324,7 +331,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_upload_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True @@ -348,8 +356,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.upload_image() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=True and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.upload_image() self.assertFalse(m.called) @@ -367,7 +377,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_modify_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True @@ -391,8 +402,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.modify_image() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=True and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.modify_image() self.assertFalse(m.called) @@ -404,7 +417,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_deactivate_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True @@ -428,8 +442,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.deactivate_image() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=Truei and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.deactivate_image() self.assertFalse(m.called) @@ -441,7 +457,8 @@ class APIImagePolicy(APIPolicyBase): mock.ANY) def test_reactivate_image_falls_back_to_legacy(self): - self.config(enforce_secure_rbac=False) + self.config(enforce_new_defaults=False, group='oslo_policy') + self.config(enforce_scope=False, group='oslo_policy') # As admin, image is mutable even if owner does not match self.context.is_admin = True @@ -465,8 +482,10 @@ class APIImagePolicy(APIPolicyBase): self.policy.reactivate_image() m.assert_called_once_with(self.context, self.image) - # Make sure we are not checking it if enforce_secure_rbac=True - self.config(enforce_secure_rbac=True) + # Make sure we are not checking it if enforce_new_defaults=True and + # enforce_scope=True + self.config(enforce_new_defaults=True, group='oslo_policy') + self.config(enforce_scope=True, group='oslo_policy') with mock.patch('glance.api.v2.policy.check_is_image_mutable') as m: self.policy.reactivate_image() self.assertFalse(m.called) diff --git a/releasenotes/notes/remove-enforce-secure-rbac-ec9a0249870460c2.yaml b/releasenotes/notes/remove-enforce-secure-rbac-ec9a0249870460c2.yaml new file mode 100644 index 0000000000..f40f46ed53 --- /dev/null +++ b/releasenotes/notes/remove-enforce-secure-rbac-ec9a0249870460c2.yaml @@ -0,0 +1,17 @@ +--- +upgrade: + - | + As per the revised SRBAC community goals, glance service is switching to + new defaults by default in Antelope cycle, hence removing the deprecated + ``enforce_secure_rbac`` option which is no longer needed. + The ``enforce_secure_rbac`` option was introduced EXPERIMENTAL in Wallaby + release for operators to opt into enforcing authorization based on common + RBAC personas. + + Now operator can control the scope and new defaults flag with the below + config options in + ``glance-api.conf`` file:: + + [oslo_policy] + enforce_new_defaults=True + enforce_scope=True \ No newline at end of file