From 30d28e3c9fef10f504e76af10d60f41210e2ae2b Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Fri, 17 Jan 2025 15:58:25 -0600 Subject: [PATCH] Add unit test for role_manager FULL_ADMIN allowlist (#15427) This commit adds a private API endpoint to dump current role manager contents and adds a unit test to verify that the methods explicitly granted to the FULL_ADMIN role have not changed. This provides coverage for a regression that granted virt write methods in STIG mode to users with FULL_ADMIN privilege. --- .../plugins/account_/privilege_roles.py | 28 ++++++++++++++++++- src/middlewared/middlewared/plugins/cron.py | 2 +- .../plugins/init_shutdown_script.py | 4 +-- tests/unit/test_role_manager.py | 26 +++++++++++++++++ 4 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/middlewared/middlewared/plugins/account_/privilege_roles.py b/src/middlewared/middlewared/plugins/account_/privilege_roles.py index 2b4ddadfc7ccb..e603a93cf42fd 100644 --- a/src/middlewared/middlewared/plugins/account_/privilege_roles.py +++ b/src/middlewared/middlewared/plugins/account_/privilege_roles.py @@ -1,7 +1,9 @@ +from copy import deepcopy + from middlewared.api.current import PrivilegeRoleEntry from middlewared.role import ROLES -from middlewared.service import Service, filterable_api_method, filter_list +from middlewared.service import Service, filterable_api_method, filter_list, private class PrivilegeService(Service): @@ -38,3 +40,27 @@ async def roles(self, filters, options): ] return filter_list(roles, filters, options) + + @private + async def dump_role_manager(self): + """ + Private method for CI in order to dump current information in role manager + This is consumed in tests/unit/test_role_manager.py + + And possibly more tests + """ + + # deepcopy is okay here because this should basically never be called and + # we'd rather err on side of paranoia + method_resources = deepcopy(self.middleware.role_manager.methods.resources) + method_allowlists = deepcopy(self.middleware.role_manager.methods.allowlists_for_roles) + + event_resources = deepcopy(self.middleware.role_manager.events.resources) + event_allowlists = deepcopy(self.middleware.role_manager.events.allowlists_for_roles) + + return { + 'method_resources': method_resources, + 'method_allowlists': method_allowlists, + 'event_resources': event_resources, + 'event_allowlists': event_allowlists + } diff --git a/src/middlewared/middlewared/plugins/cron.py b/src/middlewared/middlewared/plugins/cron.py index 68a0d7b7df27b..44117bc38fef4 100644 --- a/src/middlewared/middlewared/plugins/cron.py +++ b/src/middlewared/middlewared/plugins/cron.py @@ -188,7 +188,7 @@ async def do_delete(self, id_): return response - @api_method(CronJobRunArgs, CronJobRunResult, roles=['FULL_ADMIN']) + @api_method(CronJobRunArgs, CronJobRunResult, roles=['SYSTEM_CRON_WRITE']) @job(lock=lambda args: f'cron_job_run_{args[0]}', logs=True, lock_queue_size=1) def run(self, job, id_, skip_disabled): """ diff --git a/src/middlewared/middlewared/plugins/init_shutdown_script.py b/src/middlewared/middlewared/plugins/init_shutdown_script.py index 112b43a75ec76..36756572bc55a 100644 --- a/src/middlewared/middlewared/plugins/init_shutdown_script.py +++ b/src/middlewared/middlewared/plugins/init_shutdown_script.py @@ -39,7 +39,7 @@ class Config: entry = InitShutdownScriptEntry role_prefix = 'SYSTEM_CRON' - @api_method(InitShutdownScriptCreateArgs, InitShutdownScriptCreateResult, roles=['FULL_ADMIN']) + @api_method(InitShutdownScriptCreateArgs, InitShutdownScriptCreateResult) async def do_create(self, data): """ Create an initshutdown script task. @@ -68,7 +68,7 @@ async def do_create(self, data): ) return await self.get_instance(data['id']) - @api_method(InitShutdownScriptUpdateArgs, InitShutdownScriptUpdateResult, roles=['FULL_ADMIN']) + @api_method(InitShutdownScriptUpdateArgs, InitShutdownScriptUpdateResult) async def do_update(self, id_, data): """ Update initshutdown script task of `id`. diff --git a/tests/unit/test_role_manager.py b/tests/unit/test_role_manager.py index d62d75f52adf9..9048d6f800217 100644 --- a/tests/unit/test_role_manager.py +++ b/tests/unit/test_role_manager.py @@ -2,6 +2,7 @@ from middlewared.role import RoleManager, ROLES from middlewared.utils import security +from truenas_api_client import Client FAKE_METHODS = [ @@ -18,6 +19,19 @@ WRITE_METHODS = frozenset(['fakemethod3', 'fakemethod6']) READONLY_ADMIN_METHODS = ALL_METHODS - WRITE_METHODS FULL_ADMIN_STIG = ALL_METHODS - FAKE_METHODS_NOSTIG +EXPECTED_FA_RESOURCES = frozenset({ + 'user.verify_twofactor_token', + 'failover.reboot.other_node', + 'truenas.accept_eula', + 'filesystem.put', + 'truenas.set_production', + 'system.shutdown', + 'config.reset', + 'system.reboot', + 'config.upload', + 'filesystem.get', + 'config.save', +}) @pytest.fixture(scope='module') @@ -93,3 +107,15 @@ def test__role_manager_reject_already_registered(tmp_role_manager): with pytest.raises(ValueError, match='is already registered in this role manager'): tmp_role_manager.register_method(method_name='canary.update', roles=['VM_WRITE']) + + +def test__check_readonly_role(): + """ + We _really_ shouldn't be directly assigning resources to FULL_ADMIN. The reason for this + is that it provides no granularity for restricting what FA can do when STIG is enabled. + Mostly we don't want methods like "virt.global.update" being populated here. + """ + with Client() as c: + method_allowlists = c.call('privilege.dump_role_manager')['method_allowlists'] + fa_resources = set([entry['resource'] for entry in method_allowlists['FULL_ADMIN']]) + assert fa_resources == EXPECTED_FA_RESOURCES