From bf644e82742a6d09afc32901937d785ef40bfcae Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 14 Nov 2024 14:27:24 -0800 Subject: [PATCH] Fix policy checks added with runbooks In the runbooks change, I43555ef72cb882adcada2ed875fda40eed0dd034, new policies were added for a user sending a list of service steps or clean steps to the API. This was done with the generic check_policy helper, however the helper does not understand how to populate the ``node`` mapping data to enable RBAC rule value matching. Doing so requires a special node policy checker method. As such, the policy checker was changed, and additional tests were added. One final note, strucutrally the new policies were being checked *after* we stated to do state verification of the request. RBAC checks should be performed upfront... which also eases the burden of testing the RBAC model. Accordingly, the policy checks were moved together in the provision state logic. Closes-Bug: 2086823 Change-Id: I18c56cb4becf9e6181689ddc0f1c7433327a3aa6 --- ironic/api/controllers/v1/node.py | 28 +++++-- .../unit/api/test_rbac_project_scoped.yaml | 74 +++++++++++++++++++ ...sion-state-subpolicy-13ae3ef7497d20c1.yaml | 12 +++ 3 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/fix-set-provision-state-subpolicy-13ae3ef7497d20c1.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 0dde80f1fe..f85a40761a 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1206,11 +1206,32 @@ class NodeStatesController(rest.RestController): api_utils.check_allow_management_verbs(target) + # Secondary RBAC policy checking... + # done first because RBAC policies must be asserted in order + # of visibility to can I make a change upfront, before diving + # into if the change is valid to make. + if not runbook: + if clean_steps: + api_utils.check_owner_policy( + 'node', + 'baremetal:node:set_provision_state:clean_steps', + rpc_node['owner'], rpc_node['lessee'], + conceal_node=False) + if service_steps: + api_utils.check_owner_policy( + 'node', + 'baremetal:node:set_provision_state:service_steps', + rpc_node['owner'], rpc_node['lessee'], + conceal_node=False) + if (target in (ir_states.ACTIVE, ir_states.REBUILD) and rpc_node.maintenance): raise exception.NodeInMaintenance(op=_('provisioning'), node=rpc_node.uuid) + # This code does upfront state sanity checking, in that we're past + # basic RBAC policy checking, and we're shifting gears to content + # validation. m = ir_states.machine.copy() m.initialize(rpc_node.provision_state) if not m.is_actionable_event(ir_states.VERBS.get(target, target)): @@ -1236,13 +1257,6 @@ class NodeStatesController(rest.RestController): clean_steps, service_steps, disable_ramdisk = self._handle_runbook( rpc_node, target, runbook, clean_steps, service_steps ) - else: - if clean_steps: - api_utils.check_policy( - 'baremetal:node:set_provision_state:clean_steps') - if service_steps: - api_utils.check_policy( - 'baremetal:node:set_provision_state:service_steps') if clean_steps and target != ir_states.VERBS['clean']: msg = (_('"clean_steps" is only valid when setting target ' diff --git a/ironic/tests/unit/api/test_rbac_project_scoped.yaml b/ironic/tests/unit/api/test_rbac_project_scoped.yaml index 9dd9dad1df..32782b1cf5 100644 --- a/ironic/tests/unit/api/test_rbac_project_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_project_scoped.yaml @@ -1369,6 +1369,80 @@ service_cannot_change_provision_state: body: *provision_body assert_status: 404 +# The following tests fail on lock checking, which is fine, +# as it demonstrates we're past API RBAC policy checking... +# And these checks center around secondary policies, so the +# what we send is more important. +owner_member_can_set_provision_state_clean: + path: '/v1/nodes/{owner_node_ident}/states/provision' + method: put + headers: *owner_member_headers + body: &provision_body_clean_steps + target: clean + clean_steps: + - interface: deploy + step: update_firmware + args: + foo: bar + priority: 99 + assert_status: 409 + +owner_reader_cannot_set_provision_state_clean: + path: '/v1/nodes/{owner_node_ident}/states/provision' + method: put + headers: *owner_reader_headers + body: *provision_body_clean_steps + assert_status: 403 + +lessee_admin_can_set_provision_state_clean: + path: '/v1/nodes/{lessee_node_ident}/states/provision' + method: put + headers: *lessee_admin_headers + body: *provision_body_clean_steps + assert_status: 409 + +lessee_member_cannot_set_provision_state_clean: + path: '/v1/nodes/{lessee_node_ident}/states/provision' + method: put + headers: *lessee_member_headers + body: *provision_body_clean_steps + assert_status: 403 + +owner_member_can_set_provision_state_service: + path: '/v1/nodes/{owner_node_ident}/states/provision' + method: put + headers: *owner_member_headers + body: &provision_body_service_steps + target: service + clean_steps: + - interface: deploy + step: update_firmware + args: + foo: bar + priority: 99 + assert_status: 409 + +owner_reader_cannot_set_provision_state_service: + path: '/v1/nodes/{owner_node_ident}/states/provision' + method: put + headers: *owner_reader_headers + body: *provision_body_service_steps + assert_status: 403 + +lessee_admin_can_set_provision_state_service: + path: '/v1/nodes/{lessee_node_ident}/states/provision' + method: put + headers: *lessee_admin_headers + body: *provision_body_service_steps + assert_status: 409 + +lessee_member_cannot_set_provision_state_service: + path: '/v1/nodes/{lessee_node_ident}/states/provision' + method: put + headers: *lessee_member_headers + body: *provision_body_service_steps + assert_status: 403 + # Raid configuration owner_admin_can_set_raid_config: diff --git a/releasenotes/notes/fix-set-provision-state-subpolicy-13ae3ef7497d20c1.yaml b/releasenotes/notes/fix-set-provision-state-subpolicy-13ae3ef7497d20c1.yaml new file mode 100644 index 0000000000..18cda7501a --- /dev/null +++ b/releasenotes/notes/fix-set-provision-state-subpolicy-13ae3ef7497d20c1.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Fixes newly added policy rules, + ``baremetal:node:set_provision_state:clean_steps`` and + ``baremetal:node:set_provision_state:service_steps``which impacted + ``project scoped`` users utilizing the ``2024.2`` release of Ironic + where they were attempting to invoke ``service`` or ``clean`` + provision state commands. + This was due to a misunderstanding of the correct policy checker to + invoke, and additional testing has been added around these functions + to ensure they work as expected moving forward.