From ac8b711e0f0829992666998b4a653b8f8c3787be Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Fri, 26 Apr 2024 12:03:45 -0600 Subject: [PATCH 1/3] Fix issues with the cmd module on Windows stderr is no longer piped to stdout by default for cmd.run Scripts are called using the -File paramter for powershell.exe stderr is cleared if it contains CLIXML (only for encoded commands) cmd.powershell now accepts lists as commands Makes sure returned JSON data is valid before trying to load it Strips whitespace from the stdout in win_runas --- changelog/61166.fixed.md | 6 + salt/modules/cmdmod.py | 111 ++++++--- salt/utils/win_runas.py | 5 +- .../functional/modules/cmd/test_powershell.py | 221 +++++++++++++----- tests/pytests/unit/modules/test_cmdmod.py | 126 ++++++---- tests/support/pytest/helpers.py | 5 + tools/precommit/docstrings.py | 1 - 7 files changed, 334 insertions(+), 141 deletions(-) create mode 100644 changelog/61166.fixed.md diff --git a/changelog/61166.fixed.md b/changelog/61166.fixed.md new file mode 100644 index 000000000000..1c2cdafff292 --- /dev/null +++ b/changelog/61166.fixed.md @@ -0,0 +1,6 @@ +Fixes multiple issues with the cmd module on Windows. ``stderr`` is no +longer piped to ``stdout`` by default on ``cmd.run``. Scripts are called +using the ``-File`` parameter to the ``powershell.exe`` binary. ``CLIXML`` +data in stderr is now removed (only applies to encoded commands). Commands can +now be sent to ``cmd.powershell`` as a list. Makes sure JSON data returned is +valid. Strips whitespace from the return when using ``runas``. diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index d603a69ba772..135312e06900 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -256,32 +256,39 @@ def _check_avail(cmd): return bret and wret -def _prep_powershell_cmd(shell, cmd, stack, encoded_cmd): +def _prep_powershell_cmd(win_shell, cmd, encoded_cmd): """ - Prep cmd when shell is powershell + Prep cmd when shell is powershell. If we were called by script(), then fake + out the Windows shell to run a Powershell script. Otherwise, just run a + Powershell command. """ + # Find the full path to the shell + win_shell = salt.utils.path.which(win_shell) - # If this is running on Windows wrap - # the shell in quotes in case there are - # spaces in the paths. - if salt.utils.platform.is_windows(): - shell = f'"{shell}"' + if not win_shell: + raise CommandExecutionError("PowerShell binary not found") + + new_cmd = [win_shell, "-NonInteractive", "-NoProfile", "-ExecutionPolicy", "Bypass"] # extract_stack() returns a list of tuples. # The last item in the list [-1] is the current method. # The third item[2] in each tuple is the name of that method. - if stack[-2][2] == "script": - cmd = ( - "{} -NonInteractive -NoProfile -ExecutionPolicy Bypass -Command {}".format( - shell, cmd - ) - ) + stack = traceback.extract_stack(limit=3) + if stack[-3][2] == "script": + # If this is cmd.script, then we're running a file + new_cmd.extend(["-File", f"{cmd}"]) elif encoded_cmd: - cmd = f"{shell} -NonInteractive -NoProfile -EncodedCommand {cmd}" + new_cmd.extend(["-EncodedCommand", f"{cmd}"]) else: - cmd = f'{shell} -NonInteractive -NoProfile -Command "{cmd}"' + # Strip whitespace + if isinstance(cmd, str): + cmd = cmd.strip() + elif isinstance(cmd, list): + cmd = " ".join(cmd).strip() + new_cmd.extend(["-Command", f"& {{{cmd}}}"]) - return cmd + log.debug(new_cmd) + return new_cmd def _run( @@ -384,19 +391,7 @@ def _run( # The powershell core binary is "pwsh" # you can also pass a path here as long as the binary name is one of the two if any(word in shell.lower().strip() for word in ["powershell", "pwsh"]): - # Strip whitespace - if isinstance(cmd, str): - cmd = cmd.strip() - elif isinstance(cmd, list): - cmd = " ".join(cmd).strip() - cmd = cmd.replace('"', '\\"') - - # If we were called by script(), then fakeout the Windows - # shell to run a Powershell script. - # Else just run a Powershell command. - stack = traceback.extract_stack(limit=2) - - cmd = _prep_powershell_cmd(shell, cmd, stack, encoded_cmd) + cmd = _prep_powershell_cmd(shell, cmd, encoded_cmd) # munge the cmd and cwd through the template (cmd, cwd) = _render_cmd(cmd, cwd, template, saltenv, pillarenv, pillar_override) @@ -809,6 +804,9 @@ def _run( _log_cmd(cmd), ) + # Encoded commands dump CLIXML data in stderr. It's not an actual error + if encoded_cmd and "CLIXML" in err: + err = "" if rstrip: if out is not None: out = out.rstrip() @@ -1055,6 +1053,7 @@ def run( ignore_retcode=False, saltenv=None, use_vt=False, + redirect_stderr=False, bg=False, password=None, encoded_cmd=False, @@ -1190,6 +1189,12 @@ def run( :param bool use_vt: Use VT utils (saltstack) to stream the command output more interactively to the console and the logs. This is experimental. + :param bool redirect_stderr: If set to ``True``, then stderr will be + redirected to stdout. This is helpful for cases where obtaining both + the retcode and output is desired. + + .. versionadded:: 3006.9 + :param bool encoded_cmd: Specify if the supplied command is encoded. Only applies to shell 'powershell' and 'pwsh'. @@ -1301,6 +1306,7 @@ def run( salt '*' cmd.run cmd='sed -e s/=/:/g' """ python_shell = _python_shell_default(python_shell, kwargs.get("__pub_jid", "")) + stderr = subprocess.STDOUT if redirect_stderr else subprocess.PIPE ret = _run( cmd, runas=runas, @@ -1309,7 +1315,7 @@ def run( python_shell=python_shell, cwd=cwd, stdin=stdin, - stderr=subprocess.STDOUT, + stderr=stderr, env=env, clean_env=clean_env, prepend_path=prepend_path, @@ -4057,6 +4063,9 @@ def powershell( else: python_shell = True + if isinstance(cmd, list): + cmd = " ".join(cmd) + # Append PowerShell Object formatting # ConvertTo-JSON is only available on PowerShell 3.0 and later psversion = shell_info("powershell")["psversion"] @@ -4085,7 +4094,7 @@ def powershell( encoded_cmd = False # Retrieve the response, while overriding shell with 'powershell' - response = run( + response = run_stdout( cmd, cwd=cwd, stdin=stdin, @@ -4113,9 +4122,8 @@ def powershell( **kwargs, ) - # Sometimes Powershell returns an empty string, which isn't valid JSON - if response == "": - response = "{}" + response = _prep_powershell_json(response) + try: return salt.utils.json.loads(response) except Exception: # pylint: disable=broad-except @@ -4419,10 +4427,16 @@ def powershell_all( else: python_shell = True + if isinstance(cmd, list): + cmd = " ".join(cmd) + # Append PowerShell Object formatting - cmd += " | ConvertTo-JSON" - if depth is not None: - cmd += f" -Depth {depth}" + # ConvertTo-JSON is only available on PowerShell 3.0 and later + psversion = shell_info("powershell")["psversion"] + if salt.utils.versions.version_cmp(psversion, "2.0") == 1: + cmd += " | ConvertTo-JSON" + if depth is not None: + cmd += f" -Depth {depth}" if encode_cmd: # Convert the cmd to UTF-16LE without a BOM and base64 encode. @@ -4474,6 +4488,8 @@ def powershell_all( response["result"] = [] return response + stdoutput = _prep_powershell_json(stdoutput) + # If we fail to parse stdoutput we will raise an exception try: result = salt.utils.json.loads(stdoutput) @@ -4492,9 +4508,30 @@ def powershell_all( else: # result type is list so the force_list param has no effect response["result"] = result + + # Encoded commands dump CLIXML data in stderr. It's not an actual error + if "CLIXML" in response["stderr"]: + response["stderr"] = "" + return response +def _prep_powershell_json(text): + """ + Try to fix the output from OutputTo-JSON in powershell commands to make it + valid JSON + """ + # An empty string just needs to be an empty quote + if text == "": + text = '""' + else: + # Raw text needs to be quoted + starts_with = ['"', "{", "["] + if not any(text.startswith(x) for x in starts_with): + text = f'"{text}"' + return text + + def run_bg( cmd, cwd=None, diff --git a/salt/utils/win_runas.py b/salt/utils/win_runas.py index b50bc01ddb98..fc8c9c82be5f 100644 --- a/salt/utils/win_runas.py +++ b/salt/utils/win_runas.py @@ -52,6 +52,9 @@ def __virtual__(): def split_username(username): + """ + Splits out the username from the domain name and returns both. + """ domain = "." user_name = username if "@" in username: @@ -234,7 +237,7 @@ def runas(cmdLine, username, password=None, cwd=None): fd_out = msvcrt.open_osfhandle(stdout_read.handle, os.O_RDONLY | os.O_TEXT) with os.fdopen(fd_out, "r") as f_out: stdout = f_out.read() - ret["stdout"] = stdout + ret["stdout"] = stdout.strip() # Read standard error fd_err = msvcrt.open_osfhandle(stderr_read.handle, os.O_RDONLY | os.O_TEXT) diff --git a/tests/pytests/functional/modules/cmd/test_powershell.py b/tests/pytests/functional/modules/cmd/test_powershell.py index f072a967e814..4eb29ffc987c 100644 --- a/tests/pytests/functional/modules/cmd/test_powershell.py +++ b/tests/pytests/functional/modules/cmd/test_powershell.py @@ -1,10 +1,7 @@ -import base64 - import pytest import salt.modules.cmdmod as cmdmod import salt.utils.path -import salt.utils.stringutils pytestmark = [ pytest.mark.windows_whitelisted, @@ -23,83 +20,189 @@ def shell(request): return request.param -def test_powershell(shell): +@pytest.fixture(scope="module") +def account(): + with pytest.helpers.create_account() as _account: + yield _account + + +@pytest.mark.parametrize( + "cmd, expected, encode_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ('Write-Output "Encoded Foo"', "Encoded Foo", True), + (["Write-Output", '"Encoded Foo"'], "Encoded Foo", True), + ], +) +def test_powershell(shell, cmd, expected, encode_cmd): """ Test cmd.powershell """ - ret = cmdmod.powershell("Write-Output foo", shell=shell) - assert ret == "foo" - - -def test_powershell_encode_cmd(shell): + ret = cmdmod.powershell(cmd=cmd, encode_cmd=encode_cmd, shell=shell) + assert ret == expected + + +@pytest.mark.parametrize( + "cmd, expected, encode_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ('Write-Output "Encoded Foo"', "Encoded Foo", True), + (["Write-Output", '"Encoded Foo"'], "Encoded Foo", True), + ], +) +def test_powershell_runas(shell, account, cmd, expected, encode_cmd): """ - Test cmd.powershell with encode_cmd + Test cmd.powershell with runas """ - ret = cmdmod.powershell('Write-Output "encoded foo"', encode_cmd=True, shell=shell) - assert ret == "encoded foo" - - -def test_powershell_all(shell): + ret = cmdmod.powershell( + cmd=cmd, + encode_cmd=encode_cmd, + shell=shell, + runas=account.username, + password=account.password, + ) + assert ret == expected + + +@pytest.mark.parametrize( + "cmd, expected, encode_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ('Write-Output "Encoded Foo"', "Encoded Foo", True), + (["Write-Output", '"Encoded Foo"'], "Encoded Foo", True), + ], +) +def test_powershell_all(shell, cmd, expected, encode_cmd): """ - Test cmd.powershell_all + Test cmd.powershell_all. `encode_cmd` takes the passed command and encodes + it. Different from encoded_command where it's receiving an already encoded + command """ - ret = cmdmod.powershell_all("Write-Output foo", shell=shell) + ret = cmdmod.powershell_all(cmd=cmd, encode_cmd=encode_cmd, shell=shell) assert isinstance(ret["pid"], int) assert ret["retcode"] == 0 assert ret["stderr"] == "" - assert ret["result"] == "foo" - - -def test_powershell_all_encode_cmd(shell): + assert ret["result"] == expected + + +@pytest.mark.parametrize( + "cmd, expected, encode_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ('Write-Output "Encoded Foo"', "Encoded Foo", True), + (["Write-Output", '"Encoded Foo"'], "Encoded Foo", True), + ], +) +def test_powershell_all_runas(shell, account, cmd, expected, encode_cmd): """ - Test cmd.powershell_all with encode_cmd + Test cmd.powershell_all with runas. `encode_cmd` takes the passed command + and encodes it. Different from encoded_command where it's receiving an + already encoded command """ ret = cmdmod.powershell_all( - 'Write-Output "encoded foo"', encode_cmd=True, shell=shell + cmd=cmd, + encode_cmd=encode_cmd, + shell=shell, + runas=account.username, + password=account.password, ) assert isinstance(ret["pid"], int) assert ret["retcode"] == 0 assert ret["stderr"] == "" - assert ret["result"] == "encoded foo" - - -def test_cmd_run_all_powershell_list(): + assert ret["result"] == expected + + +@pytest.mark.parametrize( + "cmd, expected, encoded_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ( + "VwByAGkAdABlAC0ASABvAHMAdAAgACcARQBuAGMAbwBkAGUAZAAgAEYAbwBvACcA", + "Encoded Foo", + True, + ), + ], +) +def test_cmd_run_all_powershell(shell, cmd, expected, encoded_cmd): + """ + Ensure that cmd.run_all supports running shell='powershell' """ - Ensure that cmd.run_all supports running shell='powershell' with cmd passed - as a list + ret = cmdmod.run_all(cmd=cmd, shell=shell, encoded_cmd=encoded_cmd) + assert ret["stdout"] == expected + + +@pytest.mark.parametrize( + "cmd, expected, encoded_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ( + "VwByAGkAdABlAC0ASABvAHMAdAAgACcARQBuAGMAbwBkAGUAZAAgAEYAbwBvACcA", + "Encoded Foo", + True, + ), + ], +) +def test_cmd_run_all_powershell_runas(shell, account, cmd, expected, encoded_cmd): + """ + Ensure that cmd.run_all with runas supports running shell='powershell' """ ret = cmdmod.run_all( - cmd=["Write-Output", "salt"], python_shell=False, shell="powershell" + cmd=cmd, + shell=shell, + encoded_cmd=encoded_cmd, + runas=account.username, + password=account.password, ) - assert ret["stdout"] == "salt" - - -def test_cmd_run_all_powershell_string(): + assert ret["stdout"] == expected + + +@pytest.mark.parametrize( + "cmd, expected, encoded_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ( + "VwByAGkAdABlAC0ASABvAHMAdAAgACcARQBuAGMAbwBkAGUAZAAgAEYAbwBvACcA", + "Encoded Foo", + True, + ), + ], +) +def test_cmd_run_encoded_cmd(shell, cmd, expected, encoded_cmd): """ - Ensure that cmd.run_all supports running shell='powershell' with cmd passed - as a string + Ensure that cmd.run supports running shell='powershell' """ - ret = cmdmod.run_all( - cmd="Write-Output salt", python_shell=False, shell="powershell" + ret = cmdmod.run(cmd=cmd, shell=shell, encoded_cmd=encoded_cmd) + assert ret == expected + + +@pytest.mark.parametrize( + "cmd, expected, encoded_cmd", + [ + ("Write-Output Foo", "Foo", False), + (["Write-Output", "Foo"], "Foo", False), + ( + "VwByAGkAdABlAC0ASABvAHMAdAAgACcARQBuAGMAbwBkAGUAZAAgAEYAbwBvACcA", + "Encoded Foo", + True, + ), + ], +) +def test_cmd_run_encoded_cmd_runas(shell, account, cmd, expected, encoded_cmd): + """ + Ensure that cmd.run with runas supports running shell='powershell' + """ + ret = cmdmod.run( + cmd=cmd, + shell=shell, + encoded_cmd=encoded_cmd, + runas=account.username, + password=account.password, ) - assert ret["stdout"] == "salt" - - -def test_cmd_run_encoded_cmd(shell): - cmd = "Write-Output 'encoded command'" - cmd = f"$ProgressPreference='SilentlyContinue'; {cmd}" - cmd_utf16 = cmd.encode("utf-16-le") - encoded_cmd = base64.standard_b64encode(cmd_utf16) - encoded_cmd = salt.utils.stringutils.to_str(encoded_cmd) - ret = cmdmod.run(cmd=encoded_cmd, shell=shell, encoded_cmd=True) - assert ret == "encoded command" - - -def test_cmd_run_all_encoded_cmd(shell): - cmd = "Write-Output 'encoded command'" - cmd = f"$ProgressPreference='SilentlyContinue'; {cmd}" - cmd_utf16 = cmd.encode("utf-16-le") - encoded_cmd = base64.standard_b64encode(cmd_utf16) - encoded_cmd = salt.utils.stringutils.to_str(encoded_cmd) - ret = cmdmod.run_all(cmd=encoded_cmd, shell=shell, encoded_cmd=True) - assert ret["stdout"] == "encoded command" + assert ret == expected diff --git a/tests/pytests/unit/modules/test_cmdmod.py b/tests/pytests/unit/modules/test_cmdmod.py index a0e283c4ef91..26bf03ef21c5 100644 --- a/tests/pytests/unit/modules/test_cmdmod.py +++ b/tests/pytests/unit/modules/test_cmdmod.py @@ -310,7 +310,7 @@ def test_powershell_empty(): mock_run = {"pid": 1234, "retcode": 0, "stderr": "", "stdout": ""} with patch("salt.modules.cmdmod._run", return_value=mock_run): ret = cmdmod.powershell("Set-ExecutionPolicy RemoteSigned") - assert ret == {} + assert ret == "" def test_is_valid_shell_windows(): @@ -1052,57 +1052,97 @@ def test_runas_env_sudo_group(bundled): ) +def test_prep_powershell_cmd_no_powershell(): + with pytest.raises(CommandExecutionError): + cmdmod._prep_powershell_cmd( + win_shell="unk_bin", cmd="Some-Command", encoded_cmd=False + ) + + def test_prep_powershell_cmd(): """ Tests _prep_powershell_cmd returns correct cmd """ - with patch("salt.utils.platform.is_windows", MagicMock(return_value=False)): - stack = [["", "", ""], ["", "", ""], ["", "", ""]] - ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=False - ) - assert ret == 'powershell -NonInteractive -NoProfile -Command "$PSVersionTable"' - + stack = [["", "", ""], ["", "", ""], ["", "", ""], ["", "", ""]] + with patch("traceback.extract_stack", return_value=stack), patch( + "salt.utils.path.which", return_value="C:\\powershell.exe" + ): ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=True + win_shell="powershell", cmd="$PSVersionTable", encoded_cmd=False ) - assert ( - ret - == "powershell -NonInteractive -NoProfile -EncodedCommand $PSVersionTable" - ) - - stack = [["", "", ""], ["", "", "script"], ["", "", ""]] + expected = [ + "C:\\powershell.exe", + "-NonInteractive", + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-Command", + "& {$PSVersionTable}", + ] + assert ret == expected + + +def test_prep_powershell_cmd_encoded(): + """ + Tests _prep_powershell_cmd returns correct cmd when encoded_cmd=True + """ + stack = [["", "", ""], ["", "", ""], ["", "", ""], ["", "", ""]] + # This is the encoded command for 'Write-Host "Encoded HOLO"' + e_cmd = "VwByAGkAdABlAC0ASABvAHMAdAAgACIARQBuAGMAbwBkAGUAZAAgAEgATwBMAE8AIgA=" + with patch("traceback.extract_stack", return_value=stack), patch( + "salt.utils.path.which", return_value="C:\\powershell.exe" + ): ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=False - ) - assert ( - ret - == "powershell -NonInteractive -NoProfile -ExecutionPolicy Bypass -Command $PSVersionTable" + win_shell="powershell", cmd=e_cmd, encoded_cmd=True ) - - with patch("salt.utils.platform.is_windows", MagicMock(return_value=True)): - stack = [["", "", ""], ["", "", ""], ["", "", ""]] - + expected = [ + "C:\\powershell.exe", + "-NonInteractive", + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-EncodedCommand", + f"{e_cmd}", + ] + assert ret == expected + + +def test_prep_powershell_cmd_script(): + """ + Tests _prep_powershell_cmd returns correct cmd when called from cmd.script + """ + stack = [["", "", ""], ["", "", "script"], ["", "", ""], ["", "", ""]] + script = r"C:\some\script.ps1" + with patch("traceback.extract_stack", return_value=stack), patch( + "salt.utils.path.which", return_value="C:\\powershell.exe" + ): ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=False - ) - assert ( - ret == '"powershell" -NonInteractive -NoProfile -Command "$PSVersionTable"' + win_shell="powershell", cmd=script, encoded_cmd=False ) + expected = [ + "C:\\powershell.exe", + "-NonInteractive", + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-File", + script, + ] + assert ret == expected - ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=True - ) - assert ( - ret - == '"powershell" -NonInteractive -NoProfile -EncodedCommand $PSVersionTable' - ) - stack = [["", "", ""], ["", "", "script"], ["", "", ""]] - ret = cmdmod._prep_powershell_cmd( - shell="powershell", cmd="$PSVersionTable", stack=stack, encoded_cmd=False - ) - assert ( - ret - == '"powershell" -NonInteractive -NoProfile -ExecutionPolicy Bypass -Command $PSVersionTable' - ) +@pytest.mark.parametrize( + "text, expected", + [ + ("", '""'), # Should quote an empty string + ("Foo", '"Foo"'), # Should quote a string + ('["foo", "bar"]', '["foo", "bar"]'), # Should leave unchanged + ('{"foo": "bar"}', '{"foo": "bar"}'), # Should leave unchanged + ], +) +def test_prep_powershell_json(text, expected): + """ + Make sure the output is valid json + """ + result = cmdmod._prep_powershell_json(text) + assert result == expected diff --git a/tests/support/pytest/helpers.py b/tests/support/pytest/helpers.py index 07fa7a67b024..5e0f4cf3259b 100644 --- a/tests/support/pytest/helpers.py +++ b/tests/support/pytest/helpers.py @@ -329,6 +329,11 @@ def __enter__(self): ret = self.sminion.functions.user.add(self.username) assert ret is True self._delete_account = True + if salt.utils.platform.is_windows(): + log.debug("Configuring system account: %s", self) + ret = self.sminion.functions.user.update( + self.username, password_never_expires=True + ) if salt.utils.platform.is_darwin() or salt.utils.platform.is_windows(): password = self.password else: diff --git a/tools/precommit/docstrings.py b/tools/precommit/docstrings.py index 665b171fd64e..dbf47e72b5aa 100644 --- a/tools/precommit/docstrings.py +++ b/tools/precommit/docstrings.py @@ -361,7 +361,6 @@ "machine_get_machinestate_tuple", ], "salt/utils/win_osinfo.py": ["get_os_version_info"], - "salt/utils/win_runas.py": ["split_username"], "salt/utils/yamldumper.py": [ "represent_undefined", "represent_ordereddict", From 03bb18a4546d35e3e5d2631d130fbd55e5313b55 Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Wed, 8 May 2024 08:43:12 -0600 Subject: [PATCH 2/3] Make redirect_true default to True --- salt/modules/cmdmod.py | 4 ++-- tests/pytests/functional/modules/cmd/test_powershell.py | 2 +- tests/pytests/functional/modules/test_win_useradd.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 135312e06900..1324b0df125c 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -1053,7 +1053,7 @@ def run( ignore_retcode=False, saltenv=None, use_vt=False, - redirect_stderr=False, + redirect_stderr=True, bg=False, password=None, encoded_cmd=False, @@ -1191,7 +1191,7 @@ def run( :param bool redirect_stderr: If set to ``True``, then stderr will be redirected to stdout. This is helpful for cases where obtaining both - the retcode and output is desired. + the retcode and output is desired. Default is ``True`` .. versionadded:: 3006.9 diff --git a/tests/pytests/functional/modules/cmd/test_powershell.py b/tests/pytests/functional/modules/cmd/test_powershell.py index 4eb29ffc987c..5b98bc5b70f9 100644 --- a/tests/pytests/functional/modules/cmd/test_powershell.py +++ b/tests/pytests/functional/modules/cmd/test_powershell.py @@ -178,7 +178,7 @@ def test_cmd_run_encoded_cmd(shell, cmd, expected, encoded_cmd): """ Ensure that cmd.run supports running shell='powershell' """ - ret = cmdmod.run(cmd=cmd, shell=shell, encoded_cmd=encoded_cmd) + ret = cmdmod.run(cmd=cmd, shell=shell, encoded_cmd=encoded_cmd, redirect_stderr=False) assert ret == expected diff --git a/tests/pytests/functional/modules/test_win_useradd.py b/tests/pytests/functional/modules/test_win_useradd.py index 01f4a71e349f..8f3a6e907e79 100644 --- a/tests/pytests/functional/modules/test_win_useradd.py +++ b/tests/pytests/functional/modules/test_win_useradd.py @@ -320,13 +320,13 @@ def test_setpassword_int(user, account_int): ("logonscript", "\\\\server\\script.cmd", "", None), ("expiration_date", "3/19/2024", "", "2024-03-19 00:00:00"), ("expiration_date", "Never", "", None), - ("expired", True, "", None), - ("expired", False, "", None), ("account_disabled", True, "", None), ("account_disabled", False, "", None), ("unlock_account", True, "account_locked", False), ("password_never_expires", True, "", None), ("password_never_expires", False, "", None), + ("expired", True, "", None), + ("expired", False, "", None), ("disallow_change_password", True, "", None), ("disallow_change_password", False, "", None), ], From 8f33dc50a2b74b1a2598a722d1726daf9b36c942 Mon Sep 17 00:00:00 2001 From: Shane Lee Date: Thu, 9 May 2024 10:52:00 -0600 Subject: [PATCH 3/3] Fix some tests --- changelog/61166.fixed.md | 11 ++- salt/modules/cmdmod.py | 17 ++-- salt/modules/win_dsc.py | 13 ++- tests/integration/modules/test_cmdmod.py | 36 -------- .../functional/modules/cmd/test_powershell.py | 5 +- .../functional/modules/cmd/test_script.py | 87 +++++++++++++++++++ .../functional/modules/test_win_useradd.py | 3 + tests/pytests/unit/modules/test_cmdmod.py | 4 +- 8 files changed, 122 insertions(+), 54 deletions(-) create mode 100644 tests/pytests/functional/modules/cmd/test_script.py diff --git a/changelog/61166.fixed.md b/changelog/61166.fixed.md index 1c2cdafff292..f197c324c9e9 100644 --- a/changelog/61166.fixed.md +++ b/changelog/61166.fixed.md @@ -1,6 +1,5 @@ -Fixes multiple issues with the cmd module on Windows. ``stderr`` is no -longer piped to ``stdout`` by default on ``cmd.run``. Scripts are called -using the ``-File`` parameter to the ``powershell.exe`` binary. ``CLIXML`` -data in stderr is now removed (only applies to encoded commands). Commands can -now be sent to ``cmd.powershell`` as a list. Makes sure JSON data returned is -valid. Strips whitespace from the return when using ``runas``. +Fixes multiple issues with the cmd module on Windows. Scripts are called using +the ``-File`` parameter to the ``powershell.exe`` binary. ``CLIXML`` data in +stderr is now removed (only applies to encoded commands). Commands can now be +sent to ``cmd.powershell`` as a list. Makes sure JSON data returned is valid. +Strips whitespace from the return when using ``runas``. diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 1324b0df125c..c92a4aa41958 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -276,16 +276,21 @@ def _prep_powershell_cmd(win_shell, cmd, encoded_cmd): stack = traceback.extract_stack(limit=3) if stack[-3][2] == "script": # If this is cmd.script, then we're running a file - new_cmd.extend(["-File", f"{cmd}"]) + # You might be tempted to use -File here instead of -Command + # The problem with using -File is that any arguments that contain + # powershell commands themselves will not be evaluated + # See GitHub issue #56195 + new_cmd.append("-Command") + if isinstance(cmd, list): + cmd = " ".join(cmd) + new_cmd.append(f"& {cmd.strip()}") elif encoded_cmd: new_cmd.extend(["-EncodedCommand", f"{cmd}"]) else: # Strip whitespace - if isinstance(cmd, str): - cmd = cmd.strip() - elif isinstance(cmd, list): - cmd = " ".join(cmd).strip() - new_cmd.extend(["-Command", f"& {{{cmd}}}"]) + if isinstance(cmd, list): + cmd = " ".join(cmd) + new_cmd.extend(["-Command", f"& {{{cmd.strip()}}}"]) log.debug(new_cmd) return new_cmd diff --git a/salt/modules/win_dsc.py b/salt/modules/win_dsc.py index 997a72fd787e..0ef4c2a2b9ac 100644 --- a/salt/modules/win_dsc.py +++ b/salt/modules/win_dsc.py @@ -450,7 +450,9 @@ def get_config(): raise config = dict() - if raw_config: + if not raw_config: + raise CommandExecutionError("Not Configured") + else: # Does this Configuration contain a single resource if "ConfigurationName" in raw_config: # Load the single resource @@ -606,11 +608,13 @@ def test_config(): """ cmd = "Test-DscConfiguration" try: - _pshell(cmd, ignore_retcode=True) + result = _pshell(cmd, ignore_retcode=True) except CommandExecutionError as exc: if "Current configuration does not exist" in exc.info["stderr"]: raise CommandExecutionError("Not Configured") raise + if not result: + raise CommandExecutionError("Not Configured") return True @@ -635,11 +639,14 @@ def get_config_status(): "Type, Mode, RebootRequested, NumberofResources" ) try: - return _pshell(cmd, ignore_retcode=True) + result = _pshell(cmd, ignore_retcode=True) except CommandExecutionError as exc: if "No status information available" in exc.info["stderr"]: raise CommandExecutionError("Not Configured") raise + if not result: + raise CommandExecutionError("Not Configured") + return result def get_lcm_config(): diff --git a/tests/integration/modules/test_cmdmod.py b/tests/integration/modules/test_cmdmod.py index c13d31b527bc..4f2a72c45608 100644 --- a/tests/integration/modules/test_cmdmod.py +++ b/tests/integration/modules/test_cmdmod.py @@ -596,39 +596,3 @@ def test_windows_env_handling(self): ).splitlines() self.assertIn("abc=123", out) self.assertIn("ABC=456", out) - - @pytest.mark.slow_test - @pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") - def test_windows_powershell_script_args(self): - """ - Ensure that powershell processes inline script in args - """ - val = "i like cheese" - args = ( - '-SecureString (ConvertTo-SecureString -String "{}" -AsPlainText -Force)' - " -ErrorAction Stop".format(val) - ) - script = "salt://issue-56195/test.ps1" - ret = self.run_function( - "cmd.script", [script], args=args, shell="powershell", saltenv="base" - ) - self.assertEqual(ret["stdout"], val) - - @pytest.mark.slow_test - @pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") - @pytest.mark.skip_if_binaries_missing("pwsh") - def test_windows_powershell_script_args_pwsh(self): - """ - Ensure that powershell processes inline script in args with powershell - core - """ - val = "i like cheese" - args = ( - '-SecureString (ConvertTo-SecureString -String "{}" -AsPlainText -Force)' - " -ErrorAction Stop".format(val) - ) - script = "salt://issue-56195/test.ps1" - ret = self.run_function( - "cmd.script", [script], args=args, shell="pwsh", saltenv="base" - ) - self.assertEqual(ret["stdout"], val) diff --git a/tests/pytests/functional/modules/cmd/test_powershell.py b/tests/pytests/functional/modules/cmd/test_powershell.py index 5b98bc5b70f9..f8913db0493d 100644 --- a/tests/pytests/functional/modules/cmd/test_powershell.py +++ b/tests/pytests/functional/modules/cmd/test_powershell.py @@ -15,6 +15,7 @@ def shell(request): This will run the test on powershell and powershell core (pwsh). If powershell core is not installed that test run will be skipped """ + if request.param == "pwsh" and salt.utils.path.which("pwsh") is None: pytest.skip("Powershell 7 Not Present") return request.param @@ -178,7 +179,9 @@ def test_cmd_run_encoded_cmd(shell, cmd, expected, encoded_cmd): """ Ensure that cmd.run supports running shell='powershell' """ - ret = cmdmod.run(cmd=cmd, shell=shell, encoded_cmd=encoded_cmd, redirect_stderr=False) + ret = cmdmod.run( + cmd=cmd, shell=shell, encoded_cmd=encoded_cmd, redirect_stderr=False + ) assert ret == expected diff --git a/tests/pytests/functional/modules/cmd/test_script.py b/tests/pytests/functional/modules/cmd/test_script.py new file mode 100644 index 000000000000..c272835f0bf6 --- /dev/null +++ b/tests/pytests/functional/modules/cmd/test_script.py @@ -0,0 +1,87 @@ +import pytest + +import salt.utils.path + +pytestmark = [ + pytest.mark.core_test, + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture(scope="module") +def cmd(modules): + return modules.cmd + + +@pytest.fixture(params=["powershell", "pwsh"]) +def shell(request): + """ + This will run the test on powershell and powershell core (pwsh). If + powershell core is not installed that test run will be skipped + """ + if request.param == "pwsh" and salt.utils.path.which("pwsh") is None: + pytest.skip("Powershell 7 Not Present") + return request.param + + +@pytest.fixture(scope="module") +def account(): + with pytest.helpers.create_account() as _account: + yield _account + + +@pytest.fixture +def issue_56195(state_tree): + contents = """ + [CmdLetBinding()] + Param( + [SecureString] $SecureString + ) + $Credential = New-Object System.Net.NetworkCredential("DummyId", $SecureString) + $Credential.Password + """ + with pytest.helpers.temp_file("test.ps1", contents, state_tree / "issue-56195"): + yield + + +@pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") +def test_windows_script_args_powershell(cmd, shell, issue_56195): + """ + Ensure that powershell processes an inline script with args where the args + contain powershell that needs to be rendered + """ + password = "i like cheese" + args = ( + "-SecureString (ConvertTo-SecureString -String '{}' -AsPlainText -Force)" + " -ErrorAction Stop".format(password) + ) + script = "salt://issue-56195/test.ps1" + + ret = cmd.script(source=script, args=args, shell="powershell", saltenv="base") + + assert ret["stdout"] == password + + +@pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") +def test_windows_script_args_powershell_runas(cmd, shell, account, issue_56195): + """ + Ensure that powershell processes an inline script with args where the args + contain powershell that needs to be rendered + """ + password = "i like cheese" + args = ( + "-SecureString (ConvertTo-SecureString -String '{}' -AsPlainText -Force)" + " -ErrorAction Stop".format(password) + ) + script = "salt://issue-56195/test.ps1" + + ret = cmd.script( + source=script, + args=args, + shell="powershell", + saltenv="base", + runas=account.username, + password=account.password, + ) + + assert ret["stdout"] == password diff --git a/tests/pytests/functional/modules/test_win_useradd.py b/tests/pytests/functional/modules/test_win_useradd.py index 8f3a6e907e79..5e33ce36bd48 100644 --- a/tests/pytests/functional/modules/test_win_useradd.py +++ b/tests/pytests/functional/modules/test_win_useradd.py @@ -333,6 +333,9 @@ def test_setpassword_int(user, account_int): ) def test_update_str(user, value_name, new_value, info_field, expected, account_str): setting = {value_name: new_value} + # You can't expire an account if the password never expires + if value_name == "expired": + setting.update({"password_never_expires": not new_value}) ret = user.update(account_str.username, **setting) assert ret is True ret = user.info(account_str.username) diff --git a/tests/pytests/unit/modules/test_cmdmod.py b/tests/pytests/unit/modules/test_cmdmod.py index 26bf03ef21c5..cfc031fc0635 100644 --- a/tests/pytests/unit/modules/test_cmdmod.py +++ b/tests/pytests/unit/modules/test_cmdmod.py @@ -1125,8 +1125,8 @@ def test_prep_powershell_cmd_script(): "-NoProfile", "-ExecutionPolicy", "Bypass", - "-File", - script, + "-Command", + f"& {script}", ] assert ret == expected