diff --git a/changelog/54910.fixed.md b/changelog/54910.fixed.md new file mode 100644 index 00000000000..dbec1be61c3 --- /dev/null +++ b/changelog/54910.fixed.md @@ -0,0 +1 @@ +Stop run_winexe_command leaking the plaintext password and doubling the winexe prefix in its logged command diff --git a/salt/utils/cloud.py b/salt/utils/cloud.py index c32bbdbb534..e31a9e940c1 100644 --- a/salt/utils/cloud.py +++ b/salt/utils/cloud.py @@ -978,10 +978,10 @@ def run_winexe_command(cmd, args, host, username, password, port=445): Run a command remotely via the winexe executable """ creds = f"-U '{username}%{password}' //{host}" - logging_creds = f"-U '{username}%XXX-REDACTED-XXX' //{host}" cmd = f"winexe {creds} {cmd} {args}" - logging_cmd = f"winexe {logging_creds} {cmd} {args}" - return win_cmd(cmd, logging_command=logging_cmd) + # Only log the program name, never the arguments -- they carry the + # credentials. This mirrors how salt.modules.cmdmod logs commands. + return win_cmd(cmd, logging_command="winexe") def run_psexec_command(cmd, args, host, username, password, port=445): diff --git a/tests/pytests/unit/utils/test_cloud.py b/tests/pytests/unit/utils/test_cloud.py index f3288dbfef8..df8fc119c0d 100644 --- a/tests/pytests/unit/utils/test_cloud.py +++ b/tests/pytests/unit/utils/test_cloud.py @@ -810,3 +810,73 @@ def test_userdata_template(): "renderer": "jinja", } assert cloud.userdata_template(opts=opts, vm_={}, userdata="test") == "True" + + +def test_run_winexe_command_logs_program_name_only_54910(): + """ + The logging_command handed to win_cmd must be only the program name, so the + credentials (and every other argument) are never logged (issue #54910). + This mirrors how salt.modules.cmdmod logs commands. Before, cmd was + reassigned to the full credential-bearing command line before the logged + string was built, leaking the plaintext password into the logs. + """ + # Production-exact argument shape: wait_for_winexe() calls + # run_winexe_command("sc", "query winexesvc", host, username, password, port) + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + logging_command = win_cmd_mock.call_args.kwargs["logging_command"] + # Only the program name is logged -- no arguments at all. + assert logging_command == "winexe" + # The plaintext password, the username, the host and the remote command + # (all carried as arguments) must be absent from the logged string. + assert "s3cr3t-pw" not in logging_command + assert "Administrator" not in logging_command + assert "query winexesvc" not in logging_command + + +def test_run_winexe_command_executes_correct_command_54910(): + """ + Must-not-regress sibling: the actually executed command was already + correct before the fix and must stay correct. This assertion passes both + with and without the swap, guarding against a change that repairs the + logging string but corrupts the real command line. + """ + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + executed_command = win_cmd_mock.call_args.args[0] + assert ( + executed_command + == "winexe -U 'Administrator%s3cr3t-pw' //1.1.1.1 sc query winexesvc" + ) + + +def test_run_winexe_command_returns_win_cmd_result(): + """ + run_winexe_command should propagate win_cmd's return value to its caller + unchanged (wait_for_winexe relies on the return code to detect success). + """ + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + result = cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + assert result == 0 + win_cmd_mock.assert_called_once()