From d6dc519440b9c77856f3dc5efc9bd392285fb022 Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Wed, 3 Jul 2024 07:26:37 -0700 Subject: [PATCH] Fix issues with directory service cache build (#13968) This fixes the following issues: Directory service cache query could return duplicate entries Cache query result keys were out of sync with normal user.query and group.query Detection of id_type_both was broken Test coverage is added for the above through the following: We were already using set of names returned from cache. Compare set length with original list length. Grant privilege for user.query and group.query for our AD privilege tests. Since it's a limited user the output will pass through schema validation and raise error if they get out of sync with expected. Check id_type_both is set when using RID backend. There is still an outstanding issue that new schema has broken account queries for directory services as a limited user account. This will cause (2) to fail until the underlying schema issue is fixed. --- .../plugins/directoryservices_/util_cache.py | 23 +++++++++++++++---- src/middlewared/middlewared/plugins/idmap.py | 8 +++++-- src/middlewared/middlewared/utils/tdb.py | 5 +++- tests/api2/test_030_activedirectory.py | 13 ++++++++++- tests/api2/test_040_ad_user_group_cache.py | 22 ++++++++++++++---- 5 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/middlewared/middlewared/plugins/directoryservices_/util_cache.py b/src/middlewared/middlewared/plugins/directoryservices_/util_cache.py index 368f8bdb29bdc..6bcbeea5050f4 100644 --- a/src/middlewared/middlewared/plugins/directoryservices_/util_cache.py +++ b/src/middlewared/middlewared/plugins/directoryservices_/util_cache.py @@ -1,6 +1,5 @@ import enum import os -import typing from collections import defaultdict from collections.abc import Iterable @@ -33,7 +32,6 @@ from threading import Lock from uuid import uuid4 - # Update progress of job every nth user / group, we expect possibly hundreds to # a few thousand users and groups, but some edge cases where they number in # tens of thousands. Percentage complete is not updated when generating @@ -157,6 +155,8 @@ def _add_sid_info_to_entries( entry['id_type'] = idmap_entry['id_type'] if dom_by_sid: entry['domain_info'] = dom_by_sid[idmap_entry['sid'].rsplit('-', 1)[0]] + else: + entry['domain_info'] = None to_remove.reverse() for idx in to_remove: @@ -236,6 +236,10 @@ def fill_cache( ): # Now iterate members of 100 for insertion for u in users: + if u['domain_info']: + id_type_both = u['domain_info']['idmap_backend'] in ('AUTORID', 'RID') + else: + id_type_both = False user_data = u['nss'] entry = { @@ -257,11 +261,14 @@ def fill_cache( 'attributes': {}, 'groups': [], 'sshpubkey': None, + 'immutable': True, + 'two_factor_auth_configured': False, 'local': False, - 'id_type_both': u['id_type'] == 'BOTH', + 'id_type_both': id_type_both, 'nt_name': user_data.pw_name, 'smb': u['sid'] is not None, 'sid': u['sid'], + 'roles': [] } if user_count % LOG_CACHE_ENTRY_INTERVAL == 0: @@ -280,6 +287,11 @@ def fill_cache( dom_by_sid ): for g in groups: + if g['domain_info']: + id_type_both = g['domain_info']['idmap_backend'] in ('AUTORID', 'RID') + else: + id_type_both = False + group_data = g['nss'] entry = { 'id': BASE_SYNTHETIC_DATASTORE_ID + group_data.gr_gid, @@ -291,10 +303,11 @@ def fill_cache( 'sudo_commands_nopasswd': [], 'users': [], 'local': False, - 'id_type_both': g['id_type'] == 'BOTH', + 'id_type_both': id_type_both, 'nt_name': group_data.gr_name, 'smb': g['sid'] is not None, 'sid': g['sid'], + 'roles': [] } if group_count % LOG_CACHE_ENTRY_INTERVAL == 0: @@ -371,4 +384,4 @@ def query_cache_entries( options: dict ) -> list: with get_tdb_handle(DSCacheFile[id_type.name].value, CACHE_OPTIONS) as handle: - return filter_list(handle.entries(include_keys=False), filters, options) + return filter_list(handle.entries(include_keys=False, key_prefix='ID_'), filters, options) diff --git a/src/middlewared/middlewared/plugins/idmap.py b/src/middlewared/middlewared/plugins/idmap.py index 20b44d49f44be..1c78ccfa3c3af 100644 --- a/src/middlewared/middlewared/plugins/idmap.py +++ b/src/middlewared/middlewared/plugins/idmap.py @@ -607,7 +607,7 @@ async def idmap_conf_to_client_config(self, data): async def query(self, filters, options): extra = options.get("extra", {}) more_info = extra.get("additional_information", []) - ret = await super().query(filters, options) + ret = await super().query() if 'DOMAIN_INFO' in more_info: for entry in ret: try: @@ -622,7 +622,7 @@ async def query(self, filters, options): entry.update({'domain_info': domain_info}) - return ret + return filter_list(ret, filters, options) @accepts(Dict( 'idmap_domain_create', @@ -1188,6 +1188,9 @@ async def synthetic_user(self, passwd, sid): 'sshpubkey': None, 'local': False, 'id_type_both': id_type_both, + 'roles': [], + 'two_factor_auth_configured': False, + 'immutable': True, 'smb': True, 'sid': sid } @@ -1214,6 +1217,7 @@ async def synthetic_group(self, grp, sid): 'users': [], 'local': False, 'id_type_both': id_type_both, + 'roles': [], 'smb': True, 'sid': sid } diff --git a/src/middlewared/middlewared/utils/tdb.py b/src/middlewared/middlewared/utils/tdb.py index 451d0a6a18bc6..7684a81030926 100644 --- a/src/middlewared/middlewared/utils/tdb.py +++ b/src/middlewared/middlewared/utils/tdb.py @@ -204,7 +204,7 @@ def clear(self) -> None: """ self.hdl.clear() - def entries(self, include_keys: bool = True) -> Iterable[dict]: + def entries(self, include_keys: bool = True, key_prefix: str = None) -> Iterable[dict]: """ Iterate entries in TDB file: @@ -221,6 +221,9 @@ def entries(self, include_keys: bool = True) -> Iterable[dict]: if self.keys_null_terminated: tdb_key = tdb_key[:-1] + if key_prefix and not tdb_key.startswith(key_prefix): + continue + tdb_val = self.get(tdb_key) if include_keys: yield { diff --git a/tests/api2/test_030_activedirectory.py b/tests/api2/test_030_activedirectory.py index 6995af5f7ed64..04ce1fffa4f1b 100644 --- a/tests/api2/test_030_activedirectory.py +++ b/tests/api2/test_030_activedirectory.py @@ -340,6 +340,10 @@ def test_10_account_privilege_authentication(request, set_product_type): with active_directory(dns_timeout=15): call("system.general.update", {"ds_auth": True}) + nusers = call("user.query", [["local", "=", False]], {"count": True}) + assert nusers > 0 + ngroups = call("group.query", [["local", "=", False]], {"count": True}) + assert ngroups > 0 try: # RID 513 is constant for "Domain Users" domain_sid = call("idmap.domain_info", AD_DOMAIN.split(".")[0])['sid'] @@ -347,7 +351,11 @@ def test_10_account_privilege_authentication(request, set_product_type): "name": "AD privilege", "local_groups": [], "ds_groups": [f"{domain_sid}-513"], - "allowlist": [{"method": "CALL", "resource": "system.info"}], + "allowlist": [ + {"method": "CALL", "resource": "system.info"}, + {"method": "CALL", "resource": "user.query"}, + {"method": "CALL", "resource": "group.query"}, + ], "web_shell": False, }): with client(auth=(f"limiteduser@{AD_DOMAIN}", ADPASSWORD)) as c: @@ -357,6 +365,9 @@ def test_10_account_privilege_authentication(request, set_product_type): assert 'DIRECTORY_SERVICE' in me['account_attributes'] assert 'ACTIVE_DIRECTORY' in me['account_attributes'] + assert len(c.call("user.query", [["local", "=", False]])) == nusers + assert len(c.call("group.query", [["local", "=", False]])) == ngroups + assert "system.info" in methods assert "pool.create" not in methods diff --git a/tests/api2/test_040_ad_user_group_cache.py b/tests/api2/test_040_ad_user_group_cache.py index 220cbc246c7f7..00dbcb17c8f54 100644 --- a/tests/api2/test_040_ad_user_group_cache.py +++ b/tests/api2/test_040_ad_user_group_cache.py @@ -28,15 +28,21 @@ def do_ad_connection(request): if cache_fill_job['state'] == 'RUNNING': call('core.job_wait', cache_fill_job['id'], job=True) - users = set([x['username'] for x in call( + users = [x['username'] for x in call( 'user.query', [['local', '=', False]], - )]) + )] - groups = set([x['name'] for x in call( + set_users = set(users) + assert len(set_users) == len(users) + + groups = [x['name'] for x in call( 'group.query', [['local', '=', False]], - )]) + )] + + set_groups = set(groups) + assert len(set_groups) == len(groups) - yield ad | {'users': users, 'groups': groups} + yield ad | {'users': set_users, 'groups': set_groups} def get_ad_user_and_group(ad_connection): @@ -132,6 +138,12 @@ def test_check_lazy_initialization_of_users_and_groups_by_name(do_ad_connection) ad_user, ad_group = get_ad_user_and_group(do_ad_connection) + assert ad_user['id_type_both'] is True + assert ad_user['immutable'] is True + assert ad_user['local'] is False + assert ad_group['id_type_both'] is True + assert ad_group['local'] is False + cache_names = set([x['username'] for x in call( 'user.query', [['local', '=', False]], )])