diff --git a/changelog/41347.added.md b/changelog/41347.added.md new file mode 100644 index 00000000000..18c65f55806 --- /dev/null +++ b/changelog/41347.added.md @@ -0,0 +1,9 @@ +Added ``shadow.verify_password`` to ``salt.modules.win_shadow``, which +validates a Windows user's password via ``LogonUser`` with +``LOGON32_LOGON_NETWORK`` (Microsoft's recommended approach per +`KB180548 `_) without +creating an interactive session. If the check causes an account lockout, +the account is automatically unlocked. Updated ``user.present`` on Windows +to use ``shadow.verify_password`` so the password is only changed when it +differs from the current value, matching the idempotent behaviour on other +platforms. diff --git a/salt/modules/win_shadow.py b/salt/modules/win_shadow.py index 8dc42ef1708..6e1242bc357 100644 --- a/salt/modules/win_shadow.py +++ b/salt/modules/win_shadow.py @@ -6,9 +6,29 @@ minion, and it is using a different module (or gives an error similar to *'shadow.info' is not available*), see :ref:`here `. + +:depends: + - pywintypes + - win32security + - winerror """ +import logging + import salt.utils.platform +import salt.utils.win_runas +from salt.exceptions import CommandExecutionError + +log = logging.getLogger(__name__) + +try: + import pywintypes + import win32security + import winerror + + HAS_WIN32 = True +except ImportError: + HAS_WIN32 = False # Define the module's virtual name __virtualname__ = "shadow" @@ -18,9 +38,11 @@ def __virtual__(): """ Only works on Windows systems """ - if salt.utils.platform.is_windows(): - return __virtualname__ - return (False, "Module win_shadow: module only works on Windows systems.") + if not salt.utils.platform.is_windows(): + return (False, "Module win_shadow: module only works on Windows systems.") + if not HAS_WIN32: + return (False, "Module win_shadow: Missing Win32 modules") + return __virtualname__ def info(name): @@ -150,3 +172,121 @@ def set_password(name, password): salt '*' shadow.set_password root mysecretpassword """ return __salt__["user.update"](name=name, password=password) + + +def verify_password(name, password): + """ + Verify the password for a Windows user account by attempting a network + logon. This uses ``LOGON32_LOGON_NETWORK`` which does not create an + interactive session and typically does not generate audit log events. + + .. note:: + This is Microsoft's documented recommended method for validating + credentials on Windows. There is no equivalent of ``/etc/shadow`` on + Windows — the NT hash stored in the SAM database is inaccessible even + to SYSTEM at runtime. ``LogonUser`` with ``LOGON32_LOGON_NETWORK`` is + the only supported approach. + + See `How to validate user credentials on Microsoft operating systems + `_ + + .. warning:: + A wrong password will increment the account's bad-logon counter. If + the counter reaches the lockout threshold, the account will be locked. + This function detects that situation and automatically unlocks the + account if the lockout was caused by this call (i.e. the account was + not already locked beforehand). If the account was already locked, + a ``CommandExecutionError`` is raised because the password cannot be + verified in that state. + + If the logon attempt causes the account to become locked (i.e. the bad + password pushed the counter over the threshold), the account is + automatically unlocked — but only if it was not already locked before + this call. + + Args: + + name (str): The username to verify. Accepts plain names (local + accounts), UPN format (``user@domain``), or down-level format + (``DOMAIN\\user``). + + password (str): The password to verify. + + Returns: + bool: ``True`` if the password is correct (or correct but the account + has some other restriction such as being disabled or expired). + ``False`` if the password is wrong. + + Raises: + CommandExecutionError: If the account is locked (cannot verify) or an + unexpected error occurs. + + CLI Example: + + .. code-block:: bash + + salt '*' shadow.verify_password + """ + user_name, domain = salt.utils.win_runas.split_username(name) + + pre_info = __salt__["user.info"](name) + pre_locked = pre_info.get("account_locked", False) if pre_info else False + + try: + handle = win32security.LogonUser( + user_name, + domain, + password, + win32security.LOGON32_LOGON_NETWORK, + win32security.LOGON32_PROVIDER_DEFAULT, + ) + except pywintypes.error as exc: + if exc.winerror in ( + winerror.ERROR_LOGON_FAILURE, + winerror.ERROR_WRONG_PASSWORD, + ): + # Wrong password. If our attempt pushed the account into lockout, + # undo it — but only if the account was not already locked. + if not pre_locked: + post_info = __salt__["user.info"](name) + if post_info and post_info.get("account_locked", False): + log.debug( + "shadow.verify_password: password check locked account %s, " + "unlocking", + name, + ) + __salt__["user.update"](name, unlock_account=True) + log.debug("shadow.verify_password: password is not valid: %s", exc.strerror) + return False + + # These errors occur after a successful credential check — the password + # is correct but some other account restriction prevents logon. + if exc.winerror in ( + winerror.ERROR_ACCOUNT_DISABLED, + winerror.ERROR_ACCOUNT_EXPIRED, + winerror.ERROR_PASSWORD_EXPIRED, + winerror.ERROR_PASSWORD_MUST_CHANGE, + winerror.ERROR_ACCOUNT_RESTRICTION, + winerror.ERROR_INVALID_LOGON_HOURS, + winerror.ERROR_INVALID_WORKSTATION, + winerror.ERROR_LOGON_NOT_GRANTED, + winerror.ERROR_LOGON_TYPE_NOT_GRANTED, + ): + log.debug( + "shadow.verify_password: password is valid (restricted: %s)", + exc.strerror, + ) + return True + + if exc.winerror == winerror.ERROR_ACCOUNT_LOCKED_OUT: + msg = f"shadow.verify_password: account '{name}' is locked, cannot verify password" + log.debug(msg) + raise CommandExecutionError(msg) + + msg = f"shadow.verify_password: unexpected error {exc.winerror}: {exc.strerror}" + log.debug(msg) + raise CommandExecutionError(msg) + else: + handle.Close() + log.debug("shadow.verify_password: password is valid") + return True diff --git a/salt/states/user.py b/salt/states/user.py index 06ef1ffc176..d6678d73bdf 100644 --- a/salt/states/user.py +++ b/salt/states/user.py @@ -221,6 +221,10 @@ def _changes( ): change["password_lock"] = password_lock elif "shadow.info" in __salt__ and salt.utils.platform.is_windows(): + if password and not empty_password and enforce_password: + if "shadow.verify_password" in __salt__: + if not __salt__["shadow.verify_password"](name, password): + change["passwd"] = password if ( expire and expire != -1 @@ -709,6 +713,7 @@ def present( # Make changes + _passwd_changed = "passwd" in changes and not empty_password if "passwd" in changes: del changes["passwd"] if not empty_password: @@ -826,6 +831,8 @@ def _change_homedir(name, val): ret["changes"][key] = "XXX-REDACTED-XXX" else: ret["changes"][key] = spost[key] + if salt.utils.platform.is_windows() and _passwd_changed: + ret["changes"]["passwd"] = "XXX-REDACTED-XXX" if __grains__["kernel"] in ("OpenBSD", "FreeBSD") and lcpost != lcpre: ret["changes"]["loginclass"] = lcpost if ret["changes"]: diff --git a/tests/pytests/functional/modules/test_win_shadow.py b/tests/pytests/functional/modules/test_win_shadow.py new file mode 100644 index 00000000000..1996cc852d5 --- /dev/null +++ b/tests/pytests/functional/modules/test_win_shadow.py @@ -0,0 +1,71 @@ +import pytest +from saltfactories.utils import random_string + +pytestmark = [ + pytest.mark.destructive_test, + pytest.mark.skip_unless_on_windows, + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture(scope="module") +def shadow(modules): + return modules.shadow + + +@pytest.fixture(scope="module") +def lgpo(modules): + return modules.lgpo + + +@pytest.fixture(scope="module") +def user(modules): + return modules.user + + +@pytest.fixture +def account(modules): + _username = random_string("test-shadow-", uppercase=False) + with pytest.helpers.create_account(username=_username) as acct: + yield acct + + +@pytest.fixture +def lockout_threshold_one(lgpo): + """ + Temporarily set the account lockout threshold to 1 so that a single bad + password attempt locks the account. Restores the original value on teardown. + """ + original = lgpo.get_policy("LockoutThreshold", "machine") + lgpo.set_computer_policy("LockoutThreshold", 1) + try: + yield + finally: + lgpo.set_computer_policy("LockoutThreshold", original) + + +def test_verify_password_correct(shadow, account): + """ + verify_password returns True when the correct password is supplied. + """ + assert shadow.verify_password(account.username, account.password) is True + + +def test_verify_password_wrong(shadow, account): + """ + verify_password returns False when the wrong password is supplied. + """ + assert shadow.verify_password(account.username, "definitely-wrong-pw!") is False + + +def test_verify_password_unlocks_account_on_lockout( + shadow, user, account, lockout_threshold_one +): + """ + When a wrong password locks the account, verify_password should + automatically unlock it and still return False. + """ + result = shadow.verify_password(account.username, "definitely-wrong-pw!") + assert result is False + info = user.info(account.username) + assert info["account_locked"] is False diff --git a/tests/pytests/functional/states/test_user.py b/tests/pytests/functional/states/test_user.py index fb6cf0b5015..f54b1f637ff 100644 --- a/tests/pytests/functional/states/test_user.py +++ b/tests/pytests/functional/states/test_user.py @@ -552,3 +552,53 @@ def test_user_present_no_groups(modules, states, username, user_present_groups, user_info = modules.user.info(username) assert user_info assert user_info["groups"] == [username, *user_present_groups] + + +# --------------------------------------------------------------------------- +# Windows password tests +# --------------------------------------------------------------------------- + + +@pytest.fixture +def account_with_password(states, username): + """Create a test account via user.present with a known password.""" + ret = states.user.present(name=username, password="P@ssW0rd!") + assert ret.result is True + yield username + + +@pytest.mark.skip_unless_on_windows +def test_win_user_present_same_password(states, account_with_password): + """ + Running user.present with the same password a second time should be a + no-op: result True, no changes, comment contains 'up to date'. + """ + ret = states.user.present(name=account_with_password, password="P@ssW0rd!") + assert ret.result is True + assert ret.changes == {} + assert "up to date" in ret.comment + + +@pytest.mark.skip_unless_on_windows +def test_win_user_present_new_password_test_mode(states, account_with_password): + """ + Running user.present with a different password and test=True should show + a pending passwd change in the comment without applying it. + """ + ret = states.user.present( + name=account_with_password, password="N3wP@ssW0rd!", test=True + ) + assert ret.result is None + assert ret.changes == {} + assert "passwd: XXX-REDACTED-XXX" in ret.comment + + +@pytest.mark.skip_unless_on_windows +def test_win_user_present_new_password(states, account_with_password): + """ + Running user.present with a different password should change it and + report the change as passwd: XXX-REDACTED-XXX. + """ + ret = states.user.present(name=account_with_password, password="N3wP@ssW0rd!") + assert ret.result is True + assert ret.changes == {"passwd": "XXX-REDACTED-XXX"} diff --git a/tests/pytests/unit/modules/test_win_shadow.py b/tests/pytests/unit/modules/test_win_shadow.py index dbac3807172..0edddc2a2f9 100644 --- a/tests/pytests/unit/modules/test_win_shadow.py +++ b/tests/pytests/unit/modules/test_win_shadow.py @@ -7,12 +7,37 @@ import pytest import salt.modules.win_shadow as win_shadow +from salt.exceptions import CommandExecutionError from tests.support.mock import MagicMock, patch +pytestmark = [ + pytest.mark.skip_unless_on_windows, + pytest.mark.windows_whitelisted, +] + @pytest.fixture def configure_loader_modules(): - return {win_shadow: {"__salt__": {"user.update": MagicMock(return_value=True)}}} + return { + win_shadow: { + "__salt__": { + "user.update": MagicMock(return_value=True), + "user.info": MagicMock(return_value={"account_locked": False}), + } + } + } + + +def _win_error(code): + """Return a pywintypes.error with the given winerror code.""" + import pywintypes + + return pywintypes.error(code, "LogonUser", f"winerror {code}") + + +# --------------------------------------------------------------------------- +# Existing tests +# --------------------------------------------------------------------------- def test_info(): @@ -47,3 +72,158 @@ def test_set_password(): win_shadow.__salt__, {"cmd.run_all": mock_cmd, "user.info": mock_user_info} ): assert win_shadow.set_password("root", "mysecretpassword") + + +# --------------------------------------------------------------------------- +# verify_password unit tests +# --------------------------------------------------------------------------- + + +def test_verify_password_valid(): + """LogonUser succeeds — password is correct.""" + mock_handle = MagicMock() + with patch("win32security.LogonUser", return_value=mock_handle): + assert win_shadow.verify_password("testuser", "correct") is True + mock_handle.Close.assert_called_once() + + +def test_verify_password_wrong_password(): + """LogonUser raises ERROR_LOGON_FAILURE — password is wrong, no lockout.""" + import winerror + + mock_post_info = MagicMock(return_value={"account_locked": False}) + with patch.dict(win_shadow.__salt__, {"user.info": mock_post_info}): + with patch( + "win32security.LogonUser", + side_effect=_win_error(winerror.ERROR_LOGON_FAILURE), + ): + assert win_shadow.verify_password("testuser", "wrong") is False + + +def test_verify_password_causes_lockout(): + """ + LogonUser raises ERROR_LOGON_FAILURE and the account is now locked. + The function should unlock it and return False. + """ + import winerror + + user_info_responses = [ + {"account_locked": False}, # pre_info + {"account_locked": True}, # post_info (our call caused lockout) + ] + mock_user_info = MagicMock(side_effect=user_info_responses) + mock_user_update = MagicMock(return_value=True) + + with patch.dict( + win_shadow.__salt__, + {"user.info": mock_user_info, "user.update": mock_user_update}, + ): + with patch( + "win32security.LogonUser", + side_effect=_win_error(winerror.ERROR_LOGON_FAILURE), + ): + result = win_shadow.verify_password("testuser", "wrong") + + assert result is False + mock_user_update.assert_called_once_with("testuser", unlock_account=True) + + +def test_verify_password_pre_locked(): + """ + Account was already locked before the call — must raise CommandExecutionError. + """ + import winerror + + with patch( + "win32security.LogonUser", + side_effect=_win_error(winerror.ERROR_ACCOUNT_LOCKED_OUT), + ): + with pytest.raises(CommandExecutionError): + win_shadow.verify_password("testuser", "anypass") + + +def test_verify_password_wrong_password_pre_locked_no_unlock(): + """ + Account was already locked (ERROR_LOGON_FAILURE path). + The function must NOT call user.update because it was pre-locked. + """ + import winerror + + mock_pre_info = MagicMock(return_value={"account_locked": True}) + mock_user_update = MagicMock(return_value=True) + + with patch.dict( + win_shadow.__salt__, + {"user.info": mock_pre_info, "user.update": mock_user_update}, + ): + with patch( + "win32security.LogonUser", + side_effect=_win_error(winerror.ERROR_LOGON_FAILURE), + ): + result = win_shadow.verify_password("testuser", "wrong") + + assert result is False + mock_user_update.assert_not_called() + + +@pytest.mark.parametrize( + "error_code", + [ + "ERROR_ACCOUNT_DISABLED", + "ERROR_ACCOUNT_EXPIRED", + "ERROR_PASSWORD_EXPIRED", + "ERROR_PASSWORD_MUST_CHANGE", + "ERROR_ACCOUNT_RESTRICTION", + "ERROR_INVALID_LOGON_HOURS", + "ERROR_INVALID_WORKSTATION", + "ERROR_LOGON_NOT_GRANTED", + "ERROR_LOGON_TYPE_NOT_GRANTED", + ], +) +def test_verify_password_restriction_errors_return_true(error_code): + """ + Errors that occur after a successful credential check (the password is + correct but some other restriction prevents logon) must return True. + """ + import winerror + + code = getattr(winerror, error_code) + with patch("win32security.LogonUser", side_effect=_win_error(code)): + assert win_shadow.verify_password("testuser", "correct") is True + + +def test_verify_password_unknown_error(): + """An unrecognised winerror code must raise CommandExecutionError.""" + with patch("win32security.LogonUser", side_effect=_win_error(9999)): + with pytest.raises(CommandExecutionError): + win_shadow.verify_password("testuser", "anypass") + + +def test_verify_password_upn_format(): + """UPN-format names (user@domain) are split correctly.""" + mock_handle = MagicMock() + with patch("win32security.LogonUser", return_value=mock_handle) as mock_logon: + win_shadow.verify_password("alice@corp.example", "pass") + args = mock_logon.call_args[0] + assert args[0] == "alice" + assert args[1] == "corp.example" + + +def test_verify_password_downlevel_format(): + """Down-level names (DOMAIN\\user) are split correctly.""" + mock_handle = MagicMock() + with patch("win32security.LogonUser", return_value=mock_handle) as mock_logon: + win_shadow.verify_password("CORP\\alice", "pass") + args = mock_logon.call_args[0] + assert args[0] == "alice" + assert args[1] == "CORP" + + +def test_verify_password_local_format(): + """Plain local names use '.' as the domain.""" + mock_handle = MagicMock() + with patch("win32security.LogonUser", return_value=mock_handle) as mock_logon: + win_shadow.verify_password("alice", "pass") + args = mock_logon.call_args[0] + assert args[0] == "alice" + assert args[1] == "."