From 0313ce26b5b6550df64bf80690794be8b57e11da Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Mon, 29 Jan 2024 13:47:46 -0800 Subject: [PATCH] Fix service role support Turns out the service role support doesn't quite work, because you could not enumerate nodes regardless of node owner or lessee in order to enable services like Nova to enumerate nodes to be able to schedule upon them, or networking-baremetal to enumerate ports in update mapping in Neutron. So this change enables permissions to be modified to allow service project users with the service role to enumerate the list of resources, and grants rights similar to "system scoped members" to the service project's users with the "service" role which aligns with update actions to provision/unprovision nodes. Adds some additional rbac testing to ensure we appropriately covered these access rights. Closes-Bug: 2051592 Change-Id: I2b4bcc748b6e43e4215dc45137becce301349032 --- ironic/common/context.py | 7 + ironic/common/policy.py | 34 ++- ironic/conf/default.py | 36 +++ ironic/tests/unit/api/test_acl.py | 13 +- .../unit/api/test_rbac_system_scoped.yaml | 224 ++++++++++++++++++ ...ect-service-role-fix-e4d1a8c23856926a.yaml | 41 ++++ 6 files changed, 343 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/service-project-service-role-fix-e4d1a8c23856926a.yaml diff --git a/ironic/common/context.py b/ironic/common/context.py index 5c985d11d8..596107a47d 100644 --- a/ironic/common/context.py +++ b/ironic/common/context.py @@ -12,9 +12,13 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_config import cfg from oslo_context import context +CONF = cfg.CONF + + class RequestContext(context.RequestContext): """Extends security contexts from the oslo.context library.""" @@ -44,6 +48,9 @@ class RequestContext(context.RequestContext): 'project_name': self.project_name, 'is_public_api': self.is_public_api, }) + if (CONF.rbac_service_role_elevated_access + and CONF.rbac_service_project_name is not None): + policy_values['config.service_project_name'] = CONF.rbac_service_project_name # noqa return policy_values def ensure_thread_contain_context(self): diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 4bca847c84..b8af77e9f6 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -51,15 +51,22 @@ SYSTEM_ADMIN = 'role:admin and system_scope:all' # authorization that system administrators typically have. This persona, or # check string, typically isn't used by default, but it's existence it useful # in the event a deployment wants to offload some administrative action from -# system administrator to system members -SYSTEM_MEMBER = 'role:member and system_scope:all' +# system administrator to system members. +# The rule:service_role match here is to enable an elevated level of API +# access for a specialized service role and users with appropriate +# service role access. +SYSTEM_MEMBER = '(role:member and system_scope:all) or rule:service_role' # noqa -# Generic policy check string for read-only access to system-level resources. -# This persona is useful for someone who needs access for auditing or even -# support. These uses are also able to view project-specific resources where -# applicable (e.g., listing all volumes in the deployment, regardless of the -# project they belong to). -SYSTEM_READER = '(role:reader and system_scope:all) or (role:service and system_scope:all)' # noqa +# Generic policy check string for read-only access to system-level +# resources. This persona is useful for someone who needs access +# for auditing or even support. These uses are also able to view +# project-specific resources where applicable (e.g., listing all +# volumes in the deployment, regardless of the project they belong to). +# The rule:service_role match here is to enable an elevated level of API +# access for a specialized service role and users with appropriate +# role access, specifically because 'service" role is outside of the RBAC +# model defaults and does not imply reader access. +SYSTEM_READER = '(role:reader and system_scope:all) or (role:service and system_scope:all) or rule:service_role' # noqa # This check string is reserved for actions that require the highest level of # authorization on a project or resources within the project (e.g., setting the @@ -98,7 +105,7 @@ PROJECT_SERVICE = ('role:service and project_id:%(node.owner)s') # administrator should be able to delete any baremetal host in the deployment, # a project member should only be able to delete hosts in their project). SYSTEM_OR_PROJECT_MEMBER = ( - '(' + SYSTEM_MEMBER + ') or (' + PROJECT_MEMBER + ')' + '(' + SYSTEM_MEMBER + ') or (' + PROJECT_MEMBER + ') or (' + SYSTEM_SERVICE + ')' # noqa ) SYSTEM_OR_PROJECT_READER = ( '(' + SYSTEM_READER + ') or (' + PROJECT_READER + ') or (' + PROJECT_SERVICE + ')' # noqa @@ -210,6 +217,13 @@ default_policies = [ policy.RuleDefault('show_instance_secrets', '!', description='Show or mask secrets within instance information in API responses'), # noqa + # NOTE(TheJulia): This is a special rule to allow customization of the + # service role check. The config.service_project_name is a reserved + # target check field which is loaded from configuration to the + # check context in ironic/common/context.py. + policy.RuleDefault('service_role', + 'role:service and project_name:%(config.service_project_name)s', # noqa + description='Rule to match service role usage with a service project, delineated as a separate rule to enable customization.'), # noqa # Roles likely to be overridden by operator # TODO(TheJulia): Lets nuke demo from high orbit. policy.RuleDefault('is_member', @@ -484,7 +498,7 @@ node_policies = [ policy.DocumentedRuleDefault( name='baremetal:node:list_all', check_str=SYSTEM_READER, - scope_types=['system'], + scope_types=['system', 'project'], description='Retrieve multiple Node records', operations=[{'path': '/nodes', 'method': 'GET'}, {'path': '/nodes/detail', 'method': 'GET'}], diff --git a/ironic/conf/default.py b/ironic/conf/default.py index df104fc915..261922980a 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -445,6 +445,40 @@ webserver_opts = [ 'being accessed.')), ] +rbac_opts = [ + cfg.BoolOpt('rbac_service_role_elevated_access', + default=False, + help=_('Enable elevated access for users with service role ' + 'belonging to the \'rbac_service_project_name\' ' + 'project when using default policy. The default ' + 'setting of disabled causes all service role ' + 'requests to be scoped to the project the service ' + 'account belongs to.')), + cfg.StrOpt('rbac_service_project_name', + default='service', + help=_('The project name utilized for Role Based Access ' + 'Control checks for the reserved `service` project. ' + 'This project is utilized for services to have ' + 'accounts for cross-service communication. Often ' + 'these accounts require higher levels of access, and ' + 'effectively this permits accounts from the service ' + 'to not be restricted to project scoping ' + 'of responses. i.e. The service project user with a ' + '`service` role will be able to see nodes across all ' + 'projects, similar to System scoped access. If not ' + 'set to a value, and all service role access will ' + 'be filtered matching an `owner` or `lessee`, if ' + 'applicable. If an operator wishes to make behavior ' + 'visible for all service role users across ' + 'all projects, then a custom policy must be used ' + 'to override the default "service_role" rule. ' + 'It should be noted that the value of "service" ' + 'is a default convention for OpenStack deployments, ' + 'but the requsite access and details around ' + 'end configuration are largely up to an operator ' + 'if they are doing an OpenStack deployment manually.')), +] + def list_opts(): _default_opt_lists = [ @@ -461,6 +495,7 @@ def list_opts(): service_opts, utils_opts, webserver_opts, + rbac_opts ] full_opt_list = [] for options in _default_opt_lists: @@ -482,3 +517,4 @@ def register_opts(conf): conf.register_opts(service_opts) conf.register_opts(utils_opts) conf.register_opts(webserver_opts) + conf.register_opts(rbac_opts) diff --git a/ironic/tests/unit/api/test_acl.py b/ironic/tests/unit/api/test_acl.py index ded1d0b2b5..df13752dc0 100644 --- a/ironic/tests/unit/api/test_acl.py +++ b/ironic/tests/unit/api/test_acl.py @@ -77,12 +77,14 @@ class TestACLBase(base.BaseApiTest): def _fake_process_request(self, request, auth_token_request): pass - def _test_request(self, path, params=None, headers=None, method='get', + def _test_request(self, path, params=None, headers=None, method='get', # noqa: C901, E501 body=None, assert_status=None, assert_dict_contains=None, assert_list_length=None, deprecated=None, - self_manage_nodes=True): + self_manage_nodes=True, + enable_service_project=False, + service_project='service'): path = path.format(**self.format_data) self.mock_auth.side_effect = self._fake_process_request @@ -92,6 +94,13 @@ class TestACLBase(base.BaseApiTest): 'project_admin_can_manage_own_nodes', False, 'api') + if enable_service_project: + cfg.CONF.set_override('rbac_service_role_elevated_access', True) + if service_project != 'service': + # Enable us to sort of gracefully test a name variation + # with existing ddt test modeling. + cfg.CONF.set_override('rbac_service_project_name', + service_project) # always request the latest api version version = api_versions.max_version_string() diff --git a/ironic/tests/unit/api/test_rbac_system_scoped.yaml b/ironic/tests/unit/api/test_rbac_system_scoped.yaml index 919c7c1cbe..65ea2cf0a8 100644 --- a/ironic/tests/unit/api/test_rbac_system_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_system_scoped.yaml @@ -27,6 +27,11 @@ values: X-Auth-Token: 'baremetal-service-token' X-Roles: service OpenStack-System-Scope: all + service_project_headers: &service_project_headers + X-Auth-Toke: 'service-project-token' + X-Roles: 'service' + X-Project-Name: 'service' + X-Project-ID: c11111111111111111111111111111 owner_project_id: &owner_project_id '{owner_project_id}' other_project_id: &other_project_id '{other_project_id}' node_ident: &node_ident '{node_ident}' @@ -112,6 +117,34 @@ nodes_get_service: nodes: 3 assert_status: 200 +nodes_get_service_project: + path: '/v1/nodes' + method: get + headers: *service_project_headers + assert_list_length: + nodes: 3 + assert_status: 200 + enable_service_project: true + +nodes_get_service_project_disabled: + path: '/v1/nodes' + method: get + headers: *service_project_headers + assert_list_length: + nodes: 0 + assert_status: 200 + enable_service_project: false + +nodes_get_service_project_admin: + path: '/v1/nodes' + method: get + headers: *service_project_headers + assert_list_length: + nodes: 0 + assert_status: 200 + service_project: admin + enable_service_project: true + nodes_get_other_admin: path: '/v1/nodes' method: get @@ -200,6 +233,21 @@ nodes_node_ident_patch_member: body: *extra_patch assert_status: 503 +nodes_node_ident_patch_service: + path: '/v1/nodes/{node_ident}' + method: patch + headers: *service_headers + body: *extra_patch + assert_status: 503 + +nodes_node_ident_patch_service_project: + path: '/v1/nodes/{node_ident}' + method: patch + headers: *service_project_headers + body: *extra_patch + assert_status: 503 + enable_service_project: true + nodes_node_ident_patch_reader: path: '/v1/nodes/{node_ident}' method: patch @@ -248,6 +296,19 @@ nodes_validate_get_member: headers: *scoped_member_headers assert_status: 503 +nodes_validate_get_service: + path: '/v1/nodes/{node_ident}/validate' + method: get + headers: *service_headers + assert_status: 503 + +nodes_validate_get_service_project: + path: '/v1/nodes/{node_ident}/validate' + method: get + headers: *service_project_headers + assert_status: 503 + enable_service_project: true + nodes_validate_get_reader: path: '/v1/nodes/{node_ident}/validate' method: get @@ -815,6 +876,14 @@ nodes_vifs_post_service: assert_status: 503 body: *vif_body +nodes_vifs_post_service_project: + path: '/v1/nodes/{node_ident}/vifs' + method: post + headers: *service_project_headers + assert_status: 503 + body: *vif_body + enable_service_project: true + # This calls the conductor, hence not status 403. nodes_vifs_node_vif_ident_delete_admin: path: '/v1/nodes/{node_ident}/vifs/{vif_ident}' @@ -1004,6 +1073,26 @@ nodes_portgroups_get_reader: headers: *reader_headers assert_status: 200 +nodes_portgroups_get_service: + path: '/v1/nodes/{node_ident}/portgroups' + method: get + headers: *service_headers + assert_status: 200 + +nodes_portgroups_get_service_project: + path: '/v1/nodes/{node_ident}/portgroups' + method: get + headers: *service_project_headers + assert_status: 200 + enable_service_project: true + +nodes_portgroups_get_service_project: + path: '/v1/nodes/{node_ident}/portgroups' + method: get + headers: *service_project_headers + assert_status: 404 + enable_service_project: false + nodes_portgroups_detail_get_admin: path: '/v1/nodes/{node_ident}/portgroups/detail' method: get @@ -1022,6 +1111,25 @@ nodes_portgroups_detail_get_reader: headers: *reader_headers assert_status: 200 +nodes_portgroups_detail_get_service: + path: '/v1/nodes/{node_ident}/portgroups/detail' + method: get + headers: *service_headers + assert_status: 200 + +nodes_portgroups_detail_get_service_project: + path: '/v1/nodes/{node_ident}/portgroups/detail' + method: get + headers: *service_project_headers + assert_status: 200 + enable_service_project: true + +nodes_portgroups_detail_get_service_project: + path: '/v1/nodes/{node_ident}/portgroups/detail' + method: get + headers: *service_project_headers + assert_status: 404 + # Ports - https://docs.openstack.org/api-ref/baremetal/#ports-ports ports_get_admin: @@ -1029,6 +1137,33 @@ ports_get_admin: method: get headers: *admin_headers assert_status: 200 + assert_list_length: + ports: 1 + +ports_get_service: + path: '/v1/ports' + method: get + headers: *service_headers + assert_status: 200 + assert_list_length: + ports: 1 + +ports_get_service_project: + path: '/v1/ports' + method: get + headers: *service_project_headers + assert_status: 200 + assert_list_length: + ports: 1 + enable_service_project: true + +ports_get_service_project_disabled: + path: '/v1/ports' + method: get + headers: *service_project_headers + assert_status: 200 + assert_list_length: + ports: 0 ports_get_member: path: '/v1/ports' @@ -1258,6 +1393,14 @@ volume_get_service: headers: *service_headers assert_status: 200 +volume_get_service_project: + path: '/v1/volume' + method: get + headers: *service_project_headers + assert_status: 200 + assert_list_length: + enable_service_project: true + # Volume connectors volume_connectors_get_admin: @@ -1284,6 +1427,24 @@ volume_connectors_get_service: headers: *service_headers assert_status: 200 +volume_connectors_get_service_project: + path: '/v1/volume/connectors' + method: get + headers: *service_project_headers + assert_status: 200 + assert_list_length: + connectors: 1 + enable_service_project: true + +volume_connectors_get_service_project_disable: + path: '/v1/volume/connectors' + method: get + headers: *service_project_headers + assert_status: 200 + assert_list_length: + connectors: 0 + enable_service_project: false + # NOTE(TheJulia): This ends up returning a 400 due to the # UUID not already being in ironic. volume_connectors_post_admin: @@ -1319,6 +1480,14 @@ volume_connectors_post_service: assert_status: 201 body: *volume_connector_body +volume_connectors_post_service_project: + path: '/v1/volume/connectors' + method: post + headers: *service_project_headers + assert_status: 201 + body: *volume_connector_body + enable_service_project: true + volume_volume_connector_id_get_admin: path: '/v1/volume/connectors/{volume_connector_ident}' method: get @@ -1443,6 +1612,53 @@ volume_targets_post_member: boot_index: 2 volume_id: 'test-id2' +volume_targets_post_service: + path: '/v1/volume/targets' + method: post + headers: *service_headers + assert_status: 201 + body: + node_uuid: 1be26c0b-03f2-4d2e-ae87-c02d7f33c123 + volume_type: iscsi + boot_index: 2 + volume_id: 'test-id2' + +volume_targets_post_service_project: + path: '/v1/volume/targets' + method: post + headers: *service_project_headers + assert_status: 201 + body: + node_uuid: 1be26c0b-03f2-4d2e-ae87-c02d7f33c123 + volume_type: iscsi + boot_index: 2 + volume_id: 'test-id2' + enable_service_project: true + +volume_targets_post_service_project_disabled: + path: '/v1/volume/targets' + method: post + headers: *service_project_headers + assert_status: 403 + body: + node_uuid: 1be26c0b-03f2-4d2e-ae87-c02d7f33c123 + volume_type: iscsi + boot_index: 2 + volume_id: 'test-id2' + +volume_targets_post_service_project_admin: + path: '/v1/volume/targets' + method: post + headers: *service_project_headers + assert_status: 403 + body: + node_uuid: 1be26c0b-03f2-4d2e-ae87-c02d7f33c123 + volume_type: iscsi + boot_index: 2 + volume_id: 'test-id2' + service_project: admin + enable_service_project: true + volume_targets_post_reader: path: '/v1/volume/targets' method: post @@ -1507,6 +1723,14 @@ volume_volume_target_id_patch_service: headers: *service_headers assert_status: 503 +volume_volume_target_id_patch_service: + path: '/v1/volume/targets/{volume_target_ident}' + method: patch + body: *volume_target_patch + headers: *service_project_headers + assert_status: 503 + enable_service_project: true + volume_volume_target_id_delete_admin: path: '/v1/volume/targets/{volume_target_ident}' method: delete diff --git a/releasenotes/notes/service-project-service-role-fix-e4d1a8c23856926a.yaml b/releasenotes/notes/service-project-service-role-fix-e4d1a8c23856926a.yaml new file mode 100644 index 0000000000..ef91114b6d --- /dev/null +++ b/releasenotes/notes/service-project-service-role-fix-e4d1a8c23856926a.yaml @@ -0,0 +1,41 @@ +--- +fixes: + - | + Provides a fix for ``service`` role support to enable the use + case where a dedicated service project is used for cloud service + operation to facilitate actions as part of the operation of the + cloud infrastructure. + + OpenStack clouds can take a variety of configuration models + for service accounts. It is now possible to utilize the + ``[DEFAULT] rbac_service_role_elevated_access`` setting to + enable users with a ``service`` role in a dedicated ``service`` + project to act upon the API similar to a "System" scoped + "Member" where resources regardless of ``owner`` or ``lessee`` + settings are available. This is needed to enable synchronization + processes, such as ``nova-compute`` or the ``networking-baremetal`` + ML2 plugin to perform actions across the whole of an Ironic + deployment, if desirable where a "System" scoped user is also + undesirable. + + This functionality can be tuned to utilize a customized project + name aside from the default convention ``service``, for example + ``baremetal`` or ``admin``, utilizing the + ``[DEFAULT] rbac_service_project_name`` setting. + + Operators can alternatively entirely override the + ``service_role`` RBAC policy rule, if so desired, however + Ironic feels the default is both reasonable and delineates + sufficiently for the variety of Role Based Access Control + usage cases which can exist with a running Ironic deployment. +upgrades: + - | + This version of ironic includes an opt-in fix to the Role Based Access + Control logic where the "service" role in a "service" project is + able to be granted elevated access to the API surface of Ironic such that + all baremetal nodes are visible to that API consumer. This is for + deployments which have not moved to a "System" scoped user for connecting + to ironic for services like ``nova-compute`` and the + ``networking-baremetal`` Neutron plugin, and where it is desirable for + those services to be able to operate across the whole of the Ironic + deployment.