From 680f27f9b8d954edc69c08f025a2de62958c54c1 Mon Sep 17 00:00:00 2001 From: Twangboy Date: Fri, 21 Feb 2025 09:51:35 -0700 Subject: [PATCH] Fix stacktrace with unmapped account names --- changelog/66637.fixed.md | 4 + salt/modules/win_file.py | 5 +- salt/utils/win_dacl.py | 78 +++++++++++-------- .../utils/win_dacl/test_get_name.py | 22 ++++-- 4 files changed, 71 insertions(+), 38 deletions(-) create mode 100644 changelog/66637.fixed.md diff --git a/changelog/66637.fixed.md b/changelog/66637.fixed.md new file mode 100644 index 00000000000..12b6759245f --- /dev/null +++ b/changelog/66637.fixed.md @@ -0,0 +1,4 @@ +Fixes an issue when getting account names using the get_name function in the +win_dacl.py salt util. Capability SIDs return ``None``. SIDs for deleted +accounts return the SID. SIDs for domain accounts where the system is not +connected to the domain return the SID. diff --git a/salt/modules/win_file.py b/salt/modules/win_file.py index a2e4d0124dc..5c3130539d9 100644 --- a/salt/modules/win_file.py +++ b/salt/modules/win_file.py @@ -494,13 +494,14 @@ def get_group(path, follow_symlinks=True): def uid_to_user(uid): """ - Convert a uid to a user name + Convert a User ID (uid) to a username Args: uid (str): The user id to lookup Returns: - str: The name of the user + str: The name of the user. The ``uid`` will be returned if there is no + corresponding username CLI Example: diff --git a/salt/utils/win_dacl.py b/salt/utils/win_dacl.py index 346dac41de5..0a628736ec9 100644 --- a/salt/utils/win_dacl.py +++ b/salt/utils/win_dacl.py @@ -125,7 +125,7 @@ should match what you see when you look at the properties for an object. - subfolders_only: Applies to all containers beneath this object - files_only: Applies to all file objects beneath this object - .. NOTE:: + .. note:: 'applies to' properties can only be modified on directories. Files will always be ``this_folder_only``. @@ -883,10 +883,7 @@ def dacl(obj_name=None, obj_type="file"): """ # Get the principal from the sid (object sid) sid = win32security.ConvertSidToStringSid(ace[2]) - try: - principal = get_name(sid) - except CommandExecutionError: - principal = sid + principal = get_name(sid) # Get the ace type ace_type = self.ace_type[ace[0][0]] @@ -1194,14 +1191,17 @@ def get_name(principal): principal (str): Find the Normalized name based on this. Can be a PySID object, a SID - string, or a user name in any capitalization. + string, or a username in any capitalization. .. note:: - Searching based on the user name can be slow on hosts connected + Searching based on the username can be slow on hosts connected to large Active Directory domains. Returns: - str: The name that corresponds to the passed principal + str: The username that corresponds to the passed principal. If there is + no corresponding username, the string SID will be returned. + Capability SIDs will return ``None``. + Usage: @@ -1246,7 +1246,7 @@ def get_name(principal): name = f"NT Service\\{name}" return name - except (pywintypes.error, TypeError) as exc: + except pywintypes.error as exc: # Microsoft introduced the concept of Capability SIDs in Windows 8 # https://docs.microsoft.com/en-us/windows/security/identity-protection/access-control/security-identifiers#capability-sids # https://support.microsoft.com/en-us/help/4502539/some-sids-do-not-resolve-into-friendly-names @@ -1254,11 +1254,26 @@ def get_name(principal): # These types of SIDs do not resolve, so we'll just ignore them for this # All capability SIDs begin with `S-1-15-3`, so we'll only throw an # error when the sid does not begin with `S-1-15-3` - if not str_sid.startswith("S-1-15-3"): - message = f'Error resolving "{principal}"' - if type(exc) == pywintypes.error: - win_error = win32api.FormatMessage(exc.winerror).rstrip("\n") - message = f"{message}: {win_error}" + # 1332: No mapping between account names and security IDs was done + if exc.winerror == 1332: + # Capability SID, return None + if str_sid.startswith("S-1-15-3"): + log.debug("Name mapping not available for capability SID: %s", str_sid) + return None + + # User does not exist on the system or is on a disconnected domain + # Return the SID + else: + log.debug( + f"Could not resolve SID: %s\nThe user has either been removed" + " from the system or is a domain user and the system is not" + " connected to the domain" + ) + return str_sid + + # Some other unknown error + else: + message = f'Error resolving "{principal}: {exc.strerror}"' log.exception(message) raise CommandExecutionError(message, exc) @@ -2242,13 +2257,19 @@ def _check_perms(obj_name, obj_type, new_perms, access_mode, ret, test_mode=Fals cur_perms = get_permissions(obj_name=obj_name, obj_type=obj_type) changes = {} for user in new_perms: - applies_to_text = "" # Check that user exists: - try: - user_name = get_name(principal=user) - except CommandExecutionError: + user_name = get_name(principal=user) + # username will be the SID if there is no corresponding username + if user_name == get_sid_string(principal=user): ret["comment"].append( - '{} Perms: User "{}" missing from Target System'.format( + "{} Perms: Could not find a corresponding username for: {}".format( + access_mode.capitalize(), user + ) + ) + continue + if user_name is None: + ret["comment"].append( + "{} Perms: Skipping Capability SID: {}".format( access_mode.capitalize(), user ) ) @@ -2471,7 +2492,7 @@ def check_perms( log.debug("Resetting permissions for %s", obj_name) cur_perms = get_permissions(obj_name=obj_name, obj_type=obj_type) for user_name in cur_perms["Not Inherited"]: - # case insensitive dictionary search + # case-insensitive dictionary search if user_name not in {get_name(k) for k in (grant_perms or {})}: if "grant" in cur_perms["Not Inherited"][user_name]: ret["changes"].setdefault("remove_perms", {}) @@ -2489,7 +2510,7 @@ def check_perms( ret["changes"]["remove_perms"].update( {user_name: cur_perms["Not Inherited"][user_name]} ) - # case insensitive dictionary search + # case-insensitive dictionary search if user_name not in {get_name(k) for k in (deny_perms or {})}: if "deny" in cur_perms["Not Inherited"][user_name]: ret["changes"].setdefault("remove_perms", {}) @@ -2541,7 +2562,7 @@ def check_perms( log.debug("Resetting permissions for %s", obj_name) cur_perms = get_permissions(obj_name=obj_name, obj_type=obj_type) for user_name in cur_perms["Not Inherited"]: - # case insensitive dictionary search + # case-insensitive dictionary search if user_name not in {get_name(k) for k in (grant_perms or {})}: if "grant" in cur_perms["Not Inherited"][user_name]: rm_permissions( @@ -2550,7 +2571,7 @@ def check_perms( ace_type="grant", obj_type=obj_type, ) - # case insensitive dictionary search + # case-insensitive dictionary search if user_name not in {get_name(k) for k in (deny_perms or {})}: if "deny" in cur_perms["Not Inherited"][user_name]: rm_permissions( @@ -2582,14 +2603,9 @@ def _set_perms(obj_dacl, obj_type, new_perms, cur_perms, access_mode): ret = {} for user in new_perms: # Check that user exists: - try: - user_name = get_name(user) - except CommandExecutionError: - log.debug( - '%s Perms: User "%s" missing from Target System', - access_mode.capitalize(), - user, - ) + user_name = get_name(user) + # We want to skip unmapped usernames + if user_name == get_sid_string(user): continue # Get applies_to diff --git a/tests/pytests/functional/utils/win_dacl/test_get_name.py b/tests/pytests/functional/utils/win_dacl/test_get_name.py index f35c1336ec4..3f3f1e33901 100644 --- a/tests/pytests/functional/utils/win_dacl/test_get_name.py +++ b/tests/pytests/functional/utils/win_dacl/test_get_name.py @@ -9,9 +9,11 @@ import pytest import salt.exceptions import salt.utils.win_dacl +from tests.support.mock import patch # Third-party libs try: + import pywintypes import win32security HAS_WIN32 = True @@ -84,12 +86,22 @@ def test_get_name_capability_sid(): assert salt.utils.win_dacl.get_name(sid_obj) is None -def test_get_name_error(): +def test_get_name_unmapped_sid(): """ - Test get_name with an un mapped SID, should throw a CommandExecutionError + Test get_name with an un mapped SID, should return the passed sid """ test_sid = "S-1-2-3-4" sid_obj = win32security.ConvertStringSidToSid(test_sid) - with pytest.raises(salt.exceptions.CommandExecutionError) as exc: - salt.utils.win_dacl.get_name(sid_obj) - assert "No mapping between account names" in exc.value.message + assert salt.utils.win_dacl.get_name(sid_obj) == test_sid + + +def test_get_name_error(): + """ + Test get_name with an unexpected error, should throw a CommandExecutionError + """ + test_sid = "S-1-2-3-4" + sid_obj = win32security.ConvertStringSidToSid(test_sid) + with patch("win32security.LookupAccountSid", side_effect=pywintypes.error): + with pytest.raises(salt.exceptions.CommandExecutionError) as exc: + salt.utils.win_dacl.get_name(sid_obj) + assert "Error resolving" in exc.value.message