[3006.x] Fix issues with the cmd module on Windows

changelog/61166.fixed.md

@@ -0,0 +1,5 @@
+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``.

salt/modules/cmdmod.py

@@ -256,32 +256,44 @@ 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
+        # 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:
-        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, list):
+            cmd = " ".join(cmd)
+        new_cmd.extend(["-Command", f"& {{{cmd.strip()}}}"])
 
-    return cmd
+    log.debug(new_cmd)
+    return new_cmd
 
 
 def _run(
@@ -384,19 +396,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 +809,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 +1058,7 @@ def run(
     ignore_retcode=False,
     saltenv=None,
     use_vt=False,
+    redirect_stderr=True,
     bg=False,
     password=None,
     encoded_cmd=False,
@@ -1190,6 +1194,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. Default is ``True``
+
+        .. versionadded:: 3006.9
+
     :param bool encoded_cmd: Specify if the supplied command is encoded.
         Only applies to shell 'powershell' and 'pwsh'.
 
@@ -1301,6 +1311,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 +1320,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 +4068,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 +4099,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 +4127,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 +4432,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 +4493,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 +4513,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,

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():

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)

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)