CVE-2023-34049

@@ -0,0 +1,2 @@
+Fix CVE-2023-34049 by ensuring we do not use a predictable name for the script and correctly check returncode of scp command.
+This only impacts salt-ssh users using the pre-flight option.

@@ -11,9 +11,11 @@
 import logging
 import multiprocessing
 import os
+import pathlib
 import queue
 import re
 import shlex
+import shutil
 import subprocess
 import sys
 import tarfile
@@ -467,7 +469,14 @@ def key_deploy(self, host, ret):
             if target.get("passwd", False) or self.opts["ssh_passwd"]:
                 self._key_deploy_run(host, target, False)
             return ret
-        if ret[host].get("stderr", "").count("Permission denied"):
+        stderr = ret[host].get("stderr", "")
+        # -failed to upload file- is detecting scp errors
+        # Errors to ignore when Permission denied is in the stderr. For example
+        # scp can get a permission denied on the target host, but they where
+        # able to accurate authenticate against the box
+        ignore_err = ["failed to upload file"]
+        check_err = [x for x in ignore_err if stderr.count(x)]
+        if "Permission denied" in stderr and not check_err:
             target = self.targets[host]
             # permission denied, attempt to auto deploy ssh key
             print(
@@ -1007,11 +1016,32 @@ def run_ssh_pre_flight(self):
         """
         Run our pre_flight script before running any ssh commands
         """
-        script = os.path.join(tempfile.gettempdir(), self.ssh_pre_file)
-
-        self.shell.send(self.ssh_pre_flight, script)
+        with tempfile.NamedTemporaryFile() as temp:
+            # ensure we use copyfile to not copy the file attributes
+            # we want to ensure we use the perms set by the secure
+            # NamedTemporaryFile
+            try:
+                shutil.copyfile(self.ssh_pre_flight, temp.name)
+            except OSError as err:
+                return (
+                    "",
+                    f"Could not copy pre flight script {self.ssh_pre_flight} to temporary path",
+                    1,
+                )
+            target_script = f".{pathlib.Path(temp.name).name}"
+            log.trace(f"Copying the pre flight script {self.ssh_pre_file} to target")
+            stdout, stderr, retcode = self.shell.send(temp.name, target_script)
+            if retcode != 0:
+                # We could not copy the script to the target
+                log.error(
+                    f"Could not copy the pre flight script {self.ssh_pre_file} to target"
+                )
+                return stdout, stderr, retcode
 
-        return self.execute_script(script, script_args=self.ssh_pre_flight_args)
+            log.trace(f"Executing the pre flight script {self.ssh_pre_file} on target")
+            return self.execute_script(
+                target_script, script_args=self.ssh_pre_flight_args
+            )
 
     def check_thin_dir(self):
         """
@@ -1388,18 +1418,20 @@ def shim_cmd(self, cmd_str, extension="py"):
             return self.shell.exec_cmd(cmd_str)
 
         # Write the shim to a temporary file in the default temp directory
-        with tempfile.NamedTemporaryFile(
-            mode="w+b", prefix="shim_", delete=False
-        ) as shim_tmp_file:
+        with tempfile.NamedTemporaryFile(mode="w+b", delete=False) as shim_tmp_file:
             shim_tmp_file.write(salt.utils.stringutils.to_bytes(cmd_str))
 
         # Copy shim to target system, under $HOME/.<randomized name>
-        target_shim_file = ".{}.{}".format(
-            binascii.hexlify(os.urandom(6)).decode("ascii"), extension
-        )
+        target_shim_file = f".{pathlib.Path(shim_tmp_file.name).name}"
+
         if self.winrm:
             target_shim_file = saltwinshell.get_target_shim_file(self, target_shim_file)
-        self.shell.send(shim_tmp_file.name, target_shim_file, makedirs=True)
+        stdout, stderr, retcode = self.shell.send(
+            shim_tmp_file.name, target_shim_file, makedirs=True
+        )
+        if retcode != 0:
+            log.error(f"Could not copy the shim script to target")
+            return stdout, stderr, retcode
 
         # Remove our shim file
         try:

@@ -1,6 +1,5 @@
-import os
+import logging
 import re
-import tempfile
 from textwrap import dedent
 
 import pytest
@@ -16,6 +15,8 @@
 from salt.client import ssh
 from tests.support.mock import MagicMock, call, patch
 
+log = logging.getLogger(__name__)
+
 
 @pytest.fixture
 def opts(tmp_path):
@@ -59,7 +60,7 @@ def test_single_opts(opts, target):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     assert single.shell._ssh_opts() == ""
@@ -87,7 +88,7 @@ def test_run_with_pre_flight(opts, target, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("Success", "", 0)
@@ -122,7 +123,7 @@ def test_run_with_pre_flight_with_args(opts, target, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("Success", "foobar", 0)
@@ -156,7 +157,7 @@ def test_run_with_pre_flight_stderr(opts, target, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("", "Error running script", 1)
@@ -190,7 +191,7 @@ def test_run_with_pre_flight_script_doesnot_exist(opts, target, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("Success", "", 0)
@@ -224,7 +225,7 @@ def test_run_with_pre_flight_thin_dir_exists(opts, target, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("", "", 0)
@@ -242,6 +243,39 @@ def test_run_with_pre_flight_thin_dir_exists(opts, target, tmp_path):
         assert ret == cmd_ret
 
 
+def test_run_ssh_pre_flight(opts, target, tmp_path):
+    """
+    test Single.run_ssh_pre_flight function
+    """
+    target["ssh_pre_flight"] = str(tmp_path / "script.sh")
+    single = ssh.Single(
+        opts,
+        opts["argv"],
+        "localhost",
+        mods={},
+        fsclient=None,
+        thin=salt.utils.thin.thin_path(opts["cachedir"]),
+        mine=False,
+        **target,
+    )
+
+    cmd_ret = ("Success", "", 0)
+    mock_flight = MagicMock(return_value=cmd_ret)
+    mock_cmd = MagicMock(return_value=cmd_ret)
+    patch_flight = patch("salt.client.ssh.Single.run_ssh_pre_flight", mock_flight)
+    patch_cmd = patch("salt.client.ssh.Single.cmd_block", mock_cmd)
+    patch_exec_cmd = patch(
+        "salt.client.ssh.shell.Shell.exec_cmd", return_value=("", "", 1)
+    )
+    patch_os = patch("os.path.exists", side_effect=[True])
+
+    with patch_os, patch_flight, patch_cmd, patch_exec_cmd:
+        ret = single.run()
+        mock_cmd.assert_called()
+        mock_flight.assert_called()
+        assert ret == cmd_ret
+
+
 def test_execute_script(opts, target, tmp_path):
     """
     test Single.execute_script()
@@ -255,7 +289,7 @@ def test_execute_script(opts, target, tmp_path):
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
         winrm=False,
-        **target
+        **target,
     )
 
     exp_ret = ("Success", "", 0)
@@ -273,7 +307,7 @@ def test_execute_script(opts, target, tmp_path):
         ] == mock_cmd.call_args_list
 
 
-def test_shim_cmd(opts, target):
+def test_shim_cmd(opts, target, tmp_path):
     """
     test Single.shim_cmd()
     """
@@ -287,29 +321,32 @@ def test_shim_cmd(opts, target):
         mine=False,
         winrm=False,
         tty=True,
-        **target
+        **target,
     )
 
     exp_ret = ("Success", "", 0)
     mock_cmd = MagicMock(return_value=exp_ret)
     patch_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_cmd)
     patch_send = patch("salt.client.ssh.shell.Shell.send", return_value=("", "", 0))
     patch_rand = patch("os.urandom", return_value=b"5\xd9l\xca\xc2\xff")
+    tmp_file = tmp_path / "tmp_file"
+    mock_tmp = MagicMock()
+    patch_tmp = patch("tempfile.NamedTemporaryFile", mock_tmp)
+    mock_tmp.return_value.__enter__.return_value.name = tmp_file
 
-    with patch_cmd, patch_rand, patch_send:
+    with patch_cmd, patch_tmp, patch_send:
         ret = single.shim_cmd(cmd_str="echo test")
         assert ret == exp_ret
         assert [
-            call("/bin/sh '.35d96ccac2ff.py'"),
-            call("rm '.35d96ccac2ff.py'"),
+            call(f"/bin/sh '.{tmp_file.name}'"),
+            call(f"rm '.{tmp_file.name}'"),
         ] == mock_cmd.call_args_list
 
 
-def test_run_ssh_pre_flight(opts, target, tmp_path):
+def test_shim_cmd_copy_fails(opts, target, caplog):
     """
-    test Single.run_ssh_pre_flight
+    test Single.shim_cmd() when copying the file fails
     """
-    target["ssh_pre_flight"] = str(tmp_path / "script.sh")
     single = ssh.Single(
         opts,
         opts["argv"],
@@ -320,24 +357,205 @@ def test_run_ssh_pre_flight(opts, target, tmp_path):
         mine=False,
         winrm=False,
         tty=True,
-        **target
+        **target,
     )
 
-    exp_ret = ("Success", "", 0)
-    mock_cmd = MagicMock(return_value=exp_ret)
+    ret_cmd = ("Success", "", 0)
+    mock_cmd = MagicMock(return_value=ret_cmd)
     patch_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_cmd)
-    patch_send = patch("salt.client.ssh.shell.Shell.send", return_value=exp_ret)
-    exp_tmp = os.path.join(
-        tempfile.gettempdir(), os.path.basename(target["ssh_pre_flight"])
+    ret_send = ("", "General error in file copy", 1)
+    patch_send = patch("salt.client.ssh.shell.Shell.send", return_value=ret_send)
+    patch_rand = patch("os.urandom", return_value=b"5\xd9l\xca\xc2\xff")
+
+    with patch_cmd, patch_rand, patch_send:
+        ret = single.shim_cmd(cmd_str="echo test")
+        assert ret == ret_send
+        assert "Could not copy the shim script to target" in caplog.text
+        mock_cmd.assert_not_called()
+
+
+def test_run_ssh_pre_flight_no_connect(opts, target, tmp_path, caplog):
+    """
+    test Single.run_ssh_pre_flight when you
+    cannot connect to the target
+    """
+    pre_flight = tmp_path / "script.sh"
+    pre_flight.write_text("")
+    target["ssh_pre_flight"] = str(pre_flight)
+    single = ssh.Single(
+        opts,
+        opts["argv"],
+        "localhost",
+        mods={},
+        fsclient=None,
+        thin=salt.utils.thin.thin_path(opts["cachedir"]),
+        mine=False,
+        winrm=False,
+        tty=True,
+        **target,
+    )
+    mock_exec_cmd = MagicMock(return_value=("", "", 1))
+    patch_exec_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_exec_cmd)
+    tmp_file = tmp_path / "tmp_file"
+    mock_tmp = MagicMock()
+    patch_tmp = patch("tempfile.NamedTemporaryFile", mock_tmp)
+    mock_tmp.return_value.__enter__.return_value.name = tmp_file
+    ret_send = (
+        "",
+        "ssh: connect to host 192.168.1.186 port 22: No route to host\nscp: Connection closed\n",
+        255,
+    )
+    send_mock = MagicMock(return_value=ret_send)
+    patch_send = patch("salt.client.ssh.shell.Shell.send", send_mock)
+
+    with caplog.at_level(logging.TRACE):
+        with patch_send, patch_exec_cmd, patch_tmp:
+            ret = single.run_ssh_pre_flight()
+    assert f"Copying the pre flight script {pre_flight.name}" in caplog.text
+    assert (
+        f"Could not copy the pre flight script {pre_flight.name} to target"
+        in caplog.text
+    )
+    assert ret == ret_send
+    assert send_mock.call_args_list[0][0][0] == tmp_file
+    target_script = send_mock.call_args_list[0][0][1]
+    assert re.search(r".[a-z0-9]+", target_script)
+    mock_exec_cmd.assert_not_called()
+
+
+def test_run_ssh_pre_flight_permission_denied(opts, target, tmp_path):
+    """
+    test Single.run_ssh_pre_flight when you
+    cannot copy script to the target due to
+    a permission denied error
+    """
+    pre_flight = tmp_path / "script.sh"
+    pre_flight.write_text("")
+    target["ssh_pre_flight"] = str(pre_flight)
+    single = ssh.Single(
+        opts,
+        opts["argv"],
+        "localhost",
+        mods={},
+        fsclient=None,
+        thin=salt.utils.thin.thin_path(opts["cachedir"]),
+        mine=False,
+        winrm=False,
+        tty=True,
+        **target,
+    )
+    mock_exec_cmd = MagicMock(return_value=("", "", 1))
+    patch_exec_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_exec_cmd)
+    tmp_file = tmp_path / "tmp_file"
+    mock_tmp = MagicMock()
+    patch_tmp = patch("tempfile.NamedTemporaryFile", mock_tmp)
+    mock_tmp.return_value.__enter__.return_value.name = tmp_file
+    ret_send = (
+        "",
+        'scp: dest open "/tmp/preflight.sh": Permission denied\nscp: failed to upload file /etc/salt/preflight.sh to /tmp/preflight.sh\n',
+        255,
     )
+    send_mock = MagicMock(return_value=ret_send)
+    patch_send = patch("salt.client.ssh.shell.Shell.send", send_mock)
 
-    with patch_cmd, patch_send:
+    with patch_send, patch_exec_cmd, patch_tmp:
         ret = single.run_ssh_pre_flight()
-        assert ret == exp_ret
-        assert [
-            call("/bin/sh '{}'".format(exp_tmp)),
-            call("rm '{}'".format(exp_tmp)),
-        ] == mock_cmd.call_args_list
+    assert ret == ret_send
+    assert send_mock.call_args_list[0][0][0] == tmp_file
+    target_script = send_mock.call_args_list[0][0][1]
+    assert re.search(r".[a-z0-9]+", target_script)
+    mock_exec_cmd.assert_not_called()
+
+
+def test_run_ssh_pre_flight_connect(opts, target, tmp_path, caplog):
+    """
+    test Single.run_ssh_pre_flight when you
+    can connect to the target
+    """
+    pre_flight = tmp_path / "script.sh"
+    pre_flight.write_text("")
+    target["ssh_pre_flight"] = str(pre_flight)
+    single = ssh.Single(
+        opts,
+        opts["argv"],
+        "localhost",
+        mods={},
+        fsclient=None,
+        thin=salt.utils.thin.thin_path(opts["cachedir"]),
+        mine=False,
+        winrm=False,
+        tty=True,
+        **target,
+    )
+    ret_exec_cmd = ("", "", 1)
+    mock_exec_cmd = MagicMock(return_value=ret_exec_cmd)
+    patch_exec_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_exec_cmd)
+    tmp_file = tmp_path / "tmp_file"
+    mock_tmp = MagicMock()
+    patch_tmp = patch("tempfile.NamedTemporaryFile", mock_tmp)
+    mock_tmp.return_value.__enter__.return_value.name = tmp_file
+    ret_send = (
+        "",
+        "\rroot@192.168.1.187's password: \n\rpreflight.sh 0%    0 0.0KB/s   --:-- ETA\rpreflight.sh 100%   20     2.7KB/s   00:00 \n",
+        0,
+    )
+    send_mock = MagicMock(return_value=ret_send)
+    patch_send = patch("salt.client.ssh.shell.Shell.send", send_mock)
+
+    with caplog.at_level(logging.TRACE):
+        with patch_send, patch_exec_cmd, patch_tmp:
+            ret = single.run_ssh_pre_flight()
+
+    assert f"Executing the pre flight script {pre_flight.name} on target" in caplog.text
+    assert ret == ret_exec_cmd
+    assert send_mock.call_args_list[0][0][0] == tmp_file
+    target_script = send_mock.call_args_list[0][0][1]
+    assert re.search(r".[a-z0-9]+", target_script)
+    mock_exec_cmd.assert_called()
+
+
+def test_run_ssh_pre_flight_shutil_fails(opts, target, tmp_path):
+    """
+    test Single.run_ssh_pre_flight when cannot
+    copyfile with shutil
+    """
+    pre_flight = tmp_path / "script.sh"
+    pre_flight.write_text("")
+    target["ssh_pre_flight"] = str(pre_flight)
+    single = ssh.Single(
+        opts,
+        opts["argv"],
+        "localhost",
+        mods={},
+        fsclient=None,
+        thin=salt.utils.thin.thin_path(opts["cachedir"]),
+        mine=False,
+        winrm=False,
+        tty=True,
+        **target,
+    )
+    ret_exec_cmd = ("", "", 1)
+    mock_exec_cmd = MagicMock(return_value=ret_exec_cmd)
+    patch_exec_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_exec_cmd)
+    tmp_file = tmp_path / "tmp_file"
+    mock_tmp = MagicMock()
+    patch_tmp = patch("tempfile.NamedTemporaryFile", mock_tmp)
+    mock_tmp.return_value.__enter__.return_value.name = tmp_file
+    send_mock = MagicMock()
+    mock_shutil = MagicMock(side_effect=IOError("Permission Denied"))
+    patch_shutil = patch("shutil.copyfile", mock_shutil)
+    patch_send = patch("salt.client.ssh.shell.Shell.send", send_mock)
+
+    with patch_send, patch_exec_cmd, patch_tmp, patch_shutil:
+        ret = single.run_ssh_pre_flight()
+
+    assert ret == (
+        "",
+        f"Could not copy pre flight script {pre_flight} to temporary path",
+        1,
+    )
+    mock_exec_cmd.assert_not_called()
+    send_mock.assert_not_called()
 
 
 @pytest.mark.skip_on_windows(reason="SSH_PY_SHIM not set on windows")
@@ -355,7 +573,7 @@ def test_cmd_run_set_path(opts, target):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     ret = single._cmd_str()
@@ -376,7 +594,7 @@ def test_cmd_run_not_set_path(opts, target):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     ret = single._cmd_str()
@@ -395,7 +613,7 @@ def test_cmd_block_python_version_error(opts, target):
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
         winrm=False,
-        **target
+        **target,
     )
     mock_shim = MagicMock(
         return_value=(("", "ERROR: Unable to locate appropriate python command\n", 10))
@@ -434,7 +652,9 @@ def test_run_with_pre_flight_args(opts, target, test_opts, tmp_path):
     and script successfully runs
     """
     opts["ssh_run_pre_flight"] = True
-    target["ssh_pre_flight"] = str(tmp_path / "script.sh")
+    pre_flight_script = tmp_path / "script.sh"
+    pre_flight_script.write_text("")
+    target["ssh_pre_flight"] = str(pre_flight_script)
 
     if test_opts[0] is not None:
         target["ssh_pre_flight_args"] = test_opts[0]
@@ -448,22 +668,23 @@ def test_run_with_pre_flight_args(opts, target, test_opts, tmp_path):
         fsclient=None,
         thin=salt.utils.thin.thin_path(opts["cachedir"]),
         mine=False,
-        **target
+        **target,
     )
 
     cmd_ret = ("Success", "", 0)
     mock_cmd = MagicMock(return_value=cmd_ret)
     mock_exec_cmd = MagicMock(return_value=("", "", 0))
     patch_cmd = patch("salt.client.ssh.Single.cmd_block", mock_cmd)
     patch_exec_cmd = patch("salt.client.ssh.shell.Shell.exec_cmd", mock_exec_cmd)
-    patch_shell_send = patch("salt.client.ssh.shell.Shell.send", return_value=None)
+    patch_shell_send = patch(
+        "salt.client.ssh.shell.Shell.send", return_value=("", "", 0)
+    )
     patch_os = patch("os.path.exists", side_effect=[True])
 
     with patch_os, patch_cmd, patch_exec_cmd, patch_shell_send:
-        ret = single.run()
-        assert mock_exec_cmd.mock_calls[0].args[
-            0
-        ] == "/bin/sh '/tmp/script.sh'{}".format(expected_args)
+        single.run()
+        script_args = mock_exec_cmd.mock_calls[0].args[0]
+        assert re.search(r"\/bin\/sh '.[a-z0-9]+", script_args)
 
 
 @pytest.mark.slow_test

@@ -339,3 +339,113 @@ def test_extra_filerefs(tmp_path, opts):
     with patch("salt.roster.get_roster_file", MagicMock(return_value=roster)):
         ssh_obj = client._prep_ssh(**ssh_opts)
         assert ssh_obj.opts.get("extra_filerefs", None) == "salt://foobar"
+
+
+def test_key_deploy_permission_denied_scp(tmp_path, opts):
+    """
+    test "key_deploy" function when
+    permission denied authentication error
+    when attempting to use scp to copy file
+    to target
+    """
+    host = "localhost"
+    passwd = "password"
+    usr = "ssh-usr"
+    opts["ssh_user"] = usr
+    opts["tgt"] = host
+
+    ssh_ret = {
+        host: {
+            "stdout": "\rroot@192.168.1.187's password: \n\rroot@192.168.1.187's password: \n\rroot@192.168.1.187's password: \n",
+            "stderr": "Permission denied, please try again.\nPermission denied, please try again.\nroot@192.168.1.187: Permission denied (publickey,gssapi-keyex,gssapi-with-micimport pudb; pu.dbassword).\nscp: Connection closed\n",
+            "retcode": 255,
+        }
+    }
+    key_run_ret = {
+        "localhost": {
+            "jid": "20230922155652279959",
+            "return": "test",
+            "retcode": 0,
+            "id": "test",
+            "fun": "cmd.run",
+            "fun_args": ["echo test"],
+        }
+    }
+    patch_roster_file = patch("salt.roster.get_roster_file", MagicMock(return_value=""))
+    with patch_roster_file:
+        client = ssh.SSH(opts)
+    patch_input = patch("builtins.input", side_effect=["y"])
+    patch_getpass = patch("getpass.getpass", return_value=["password"])
+    mock_key_run = MagicMock(return_value=key_run_ret)
+    patch_key_run = patch("salt.client.ssh.SSH._key_deploy_run", mock_key_run)
+    with patch_input, patch_getpass, patch_key_run:
+        ret = client.key_deploy(host, ssh_ret)
+    assert mock_key_run.call_args_list[0][0] == (
+        host,
+        {"passwd": [passwd], "host": host, "user": usr},
+        True,
+    )
+    assert ret == key_run_ret
+    assert mock_key_run.call_count == 1
+
+
+def test_key_deploy_permission_denied_file_scp(tmp_path, opts):
+    """
+    test "key_deploy" function when permission denied
+    due to not having access to copy the file to the target
+    We do not want to deploy the key, because this is not
+    an authentication to the target error.
+    """
+    host = "localhost"
+    passwd = "password"
+    usr = "ssh-usr"
+    opts["ssh_user"] = usr
+    opts["tgt"] = host
+
+    mock_key_run = MagicMock(return_value=False)
+    patch_key_run = patch("salt.client.ssh.SSH._key_deploy_run", mock_key_run)
+
+    ssh_ret = {
+        "localhost": {
+            "stdout": "",
+            "stderr": 'scp: dest open "/tmp/preflight.sh": Permission denied\nscp: failed to upload file /etc/salt/preflight.sh to /tmp/preflight.sh\n',
+            "retcode": 1,
+        }
+    }
+    patch_roster_file = patch("salt.roster.get_roster_file", MagicMock(return_value=""))
+    with patch_roster_file:
+        client = ssh.SSH(opts)
+    ret = client.key_deploy(host, ssh_ret)
+    assert ret == ssh_ret
+    assert mock_key_run.call_count == 0
+
+
+def test_key_deploy_no_permission_denied(tmp_path, opts):
+    """
+    test "key_deploy" function when no permission denied
+    is returned
+    """
+    host = "localhost"
+    passwd = "password"
+    usr = "ssh-usr"
+    opts["ssh_user"] = usr
+    opts["tgt"] = host
+
+    mock_key_run = MagicMock(return_value=False)
+    patch_key_run = patch("salt.client.ssh.SSH._key_deploy_run", mock_key_run)
+    ssh_ret = {
+        "localhost": {
+            "jid": "20230922161937998385",
+            "return": "test",
+            "retcode": 0,
+            "id": "test",
+            "fun": "cmd.run",
+            "fun_args": ["echo test"],
+        }
+    }
+    patch_roster_file = patch("salt.roster.get_roster_file", MagicMock(return_value=""))
+    with patch_roster_file:
+        client = ssh.SSH(opts)
+    ret = client.key_deploy(host, ssh_ret)
+    assert ret == ssh_ret
+    assert mock_key_run.call_count == 0

@@ -1,132 +0,0 @@
-"""
-Test for ssh_pre_flight roster option
-"""
-
-import os
-
-import pytest
-
-import salt.utils.files
-from tests.support.case import SSHCase
-from tests.support.runtests import RUNTIME_VARS
-
-
-class SSHPreFlightTest(SSHCase):
-    """
-    Test ssh_pre_flight roster option
-    """
-
-    def setUp(self):
-        super().setUp()
-        self.roster = os.path.join(RUNTIME_VARS.TMP, "pre_flight_roster")
-        self.data = {
-            "ssh_pre_flight": os.path.join(RUNTIME_VARS.TMP, "ssh_pre_flight.sh")
-        }
-        self.test_script = os.path.join(
-            RUNTIME_VARS.TMP, "test-pre-flight-script-worked.txt"
-        )
-
-    def _create_roster(self, pre_flight_script_args=None):
-        data = dict(self.data)
-        if pre_flight_script_args:
-            data["ssh_pre_flight_args"] = pre_flight_script_args
-
-        self.custom_roster(self.roster, data)
-
-        with salt.utils.files.fopen(data["ssh_pre_flight"], "w") as fp_:
-            fp_.write("touch {}".format(self.test_script))
-
-    @pytest.mark.slow_test
-    def test_ssh_pre_flight(self):
-        """
-        test ssh when ssh_pre_flight is set
-        ensure the script runs successfully
-        """
-        self._create_roster()
-        assert self.run_function("test.ping", roster_file=self.roster)
-
-        assert os.path.exists(self.test_script)
-
-    @pytest.mark.slow_test
-    def test_ssh_run_pre_flight(self):
-        """
-        test ssh when --pre-flight is passed to salt-ssh
-        to ensure the script runs successfully
-        """
-        self._create_roster()
-        # make sure we previously ran a command so the thin dir exists
-        self.run_function("test.ping", wipe=False)
-        assert not os.path.exists(self.test_script)
-
-        assert self.run_function(
-            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
-        )
-        assert os.path.exists(self.test_script)
-
-    @pytest.mark.slow_test
-    def test_ssh_run_pre_flight_args(self):
-        """
-        test ssh when --pre-flight is passed to salt-ssh
-        to ensure the script runs successfully passing some args
-        """
-        self._create_roster(pre_flight_script_args="foobar test")
-        # make sure we previously ran a command so the thin dir exists
-        self.run_function("test.ping", wipe=False)
-        assert not os.path.exists(self.test_script)
-
-        assert self.run_function(
-            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
-        )
-        assert os.path.exists(self.test_script)
-
-    @pytest.mark.slow_test
-    def test_ssh_run_pre_flight_args_prevent_injection(self):
-        """
-        test ssh when --pre-flight is passed to salt-ssh
-        and evil arguments are used in order to produce shell injection
-        """
-        injected_file = os.path.join(RUNTIME_VARS.TMP, "injection")
-        self._create_roster(
-            pre_flight_script_args="foobar; echo injected > {}".format(injected_file)
-        )
-        # make sure we previously ran a command so the thin dir exists
-        self.run_function("test.ping", wipe=False)
-        assert not os.path.exists(self.test_script)
-        assert not os.path.isfile(injected_file)
-
-        assert self.run_function(
-            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
-        )
-
-        assert not os.path.isfile(
-            injected_file
-        ), "File injection suceeded. This shouldn't happend"
-
-    @pytest.mark.slow_test
-    def test_ssh_run_pre_flight_failure(self):
-        """
-        test ssh_pre_flight when there is a failure
-        in the script.
-        """
-        self._create_roster()
-        with salt.utils.files.fopen(self.data["ssh_pre_flight"], "w") as fp_:
-            fp_.write("exit 2")
-
-        ret = self.run_function(
-            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
-        )
-        assert ret["retcode"] == 2
-
-    def tearDown(self):
-        """
-        make sure to clean up any old ssh directories
-        """
-        files = [
-            self.roster,
-            self.data["ssh_pre_flight"],
-            self.test_script,
-            os.path.join(RUNTIME_VARS.TMP, "injection"),
-        ]
-        for fp_ in files:
-            if os.path.exists(fp_):
-                os.remove(fp_)

@@ -0,0 +1,309 @@
+"""
+Test for ssh_pre_flight roster option
+"""
+
+import grp
+import os
+import pathlib
+import pwd
+import shutil
+import subprocess
+
+import pytest
+import yaml
+from saltfactories.utils import random_string
+
+import salt.utils.files
+
+
+def _custom_roster(roster_file, roster_data):
+    with salt.utils.files.fopen(roster_file, "r") as fp:
+        data = salt.utils.yaml.safe_load(fp)
+    for key, item in roster_data.items():
+        data["localhost"][key] = item
+    with salt.utils.files.fopen(roster_file, "w") as fp:
+        yaml.safe_dump(data, fp)
+
+
+@pytest.fixture
+def _create_roster(salt_ssh_roster_file, tmp_path):
+    ret = {}
+    ret["roster"] = salt_ssh_roster_file
+    ret["data"] = {"ssh_pre_flight": str(tmp_path / "ssh_pre_flight.sh")}
+    ret["test_script"] = str(tmp_path / "test-pre-flight-script-worked.txt")
+    ret["thin_dir"] = tmp_path / "thin_dir"
+
+    with salt.utils.files.fopen(salt_ssh_roster_file, "r") as fp:
+        data = salt.utils.yaml.safe_load(fp)
+    pre_flight_script = ret["data"]["ssh_pre_flight"]
+    data["localhost"]["ssh_pre_flight"] = pre_flight_script
+    data["localhost"]["thin_dir"] = str(ret["thin_dir"])
+    with salt.utils.files.fopen(salt_ssh_roster_file, "w") as fp:
+        yaml.safe_dump(data, fp)
+
+    with salt.utils.files.fopen(pre_flight_script, "w") as fp:
+        fp.write("touch {}".format(ret["test_script"]))
+
+    yield ret
+    if ret["thin_dir"].exists():
+        shutil.rmtree(ret["thin_dir"])
+
+
+@pytest.mark.slow_test
+def test_ssh_pre_flight(salt_ssh_cli, caplog, _create_roster):
+    """
+    test ssh when ssh_pre_flight is set
+    ensure the script runs successfully
+    """
+    ret = salt_ssh_cli.run("test.ping")
+    assert ret.returncode == 0
+
+    assert pathlib.Path(_create_roster["test_script"]).exists()
+
+
+@pytest.mark.slow_test
+def test_ssh_run_pre_flight(salt_ssh_cli, _create_roster):
+    """
+    test ssh when --pre-flight is passed to salt-ssh
+    to ensure the script runs successfully
+    """
+    # make sure we previously ran a command so the thin dir exists
+    ret = salt_ssh_cli.run("test.ping")
+    assert pathlib.Path(_create_roster["test_script"]).exists()
+
+    # Now remeove the script to ensure pre_flight doesn't run
+    # without --pre-flight
+    pathlib.Path(_create_roster["test_script"]).unlink()
+
+    assert salt_ssh_cli.run("test.ping").returncode == 0
+    assert not pathlib.Path(_create_roster["test_script"]).exists()
+
+    # Now ensure
+    ret = salt_ssh_cli.run(
+        "test.ping",
+        "--pre-flight",
+    )
+    assert ret.returncode == 0
+    assert pathlib.Path(_create_roster["test_script"]).exists()
+
+
+@pytest.mark.slow_test
+def test_ssh_run_pre_flight_args(salt_ssh_cli, _create_roster):
+    """
+    test ssh when --pre-flight is passed to salt-ssh
+    to ensure the script runs successfully passing some args
+    """
+    _custom_roster(salt_ssh_cli.roster_file, {"ssh_pre_flight_args": "foobar test"})
+    # Create pre_flight script that accepts args
+    test_script = _create_roster["test_script"]
+    test_script_1 = pathlib.Path(test_script + "-foobar")
+    test_script_2 = pathlib.Path(test_script + "-test")
+    with salt.utils.files.fopen(_create_roster["data"]["ssh_pre_flight"], "w") as fp:
+        fp.write(
+            f"""
+        touch {str(test_script)}-$1
+        touch {str(test_script)}-$2
+        """
+        )
+    ret = salt_ssh_cli.run("test.ping")
+    assert ret.returncode == 0
+    assert test_script_1.exists()
+    assert test_script_2.exists()
+    pathlib.Path(test_script_1).unlink()
+    pathlib.Path(test_script_2).unlink()
+
+    ret = salt_ssh_cli.run("test.ping")
+    assert ret.returncode == 0
+    assert not test_script_1.exists()
+    assert not test_script_2.exists()
+
+    ret = salt_ssh_cli.run(
+        "test.ping",
+        "--pre-flight",
+    )
+    assert ret.returncode == 0
+    assert test_script_1.exists()
+    assert test_script_2.exists()
+
+
+@pytest.mark.slow_test
+def test_ssh_run_pre_flight_args_prevent_injection(
+    salt_ssh_cli, _create_roster, tmp_path
+):
+    """
+    test ssh when --pre-flight is passed to salt-ssh
+    and evil arguments are used in order to produce shell injection
+    """
+    injected_file = tmp_path / "injection"
+    _custom_roster(
+        salt_ssh_cli.roster_file,
+        {"ssh_pre_flight_args": f"foobar; echo injected > {str(injected_file)}"},
+    )
+    # Create pre_flight script that accepts args
+    test_script = _create_roster["test_script"]
+    test_script_1 = pathlib.Path(test_script + "-echo")
+    test_script_2 = pathlib.Path(test_script + "-foobar;")
+    with salt.utils.files.fopen(_create_roster["data"]["ssh_pre_flight"], "w") as fp:
+        fp.write(
+            f"""
+        touch {str(test_script)}-$1
+        touch {str(test_script)}-$2
+        """
+        )
+
+    # make sure we previously ran a command so the thin dir exists
+    ret = salt_ssh_cli.run("test.ping")
+    assert ret.returncode == 0
+    assert test_script_1.exists()
+    assert test_script_2.exists()
+    test_script_1.unlink()
+    test_script_2.unlink()
+    assert not injected_file.is_file()
+
+    ret = salt_ssh_cli.run(
+        "test.ping",
+        "--pre-flight",
+    )
+    assert ret.returncode == 0
+
+    assert test_script_1.exists()
+    assert test_script_2.exists()
+    assert not pathlib.Path(
+        injected_file
+    ).is_file(), "File injection suceeded. This shouldn't happend"
+
+
+@pytest.mark.flaky(max_runs=4)
+@pytest.mark.slow_test
+def test_ssh_run_pre_flight_failure(salt_ssh_cli, _create_roster):
+    """
+    test ssh_pre_flight when there is a failure
+    in the script.
+    """
+    with salt.utils.files.fopen(_create_roster["data"]["ssh_pre_flight"], "w") as fp_:
+        fp_.write("exit 2")
+
+    ret = salt_ssh_cli.run(
+        "test.ping",
+        "--pre-flight",
+    )
+    assert ret.data["retcode"] == 2
+
+
+@pytest.fixture
+def account():
+    username = random_string("test-account-", uppercase=False)
+    with pytest.helpers.create_account(username=username) as account:
+        yield account
+
+
+@pytest.mark.slow_test
+def test_ssh_pre_flight_script(salt_ssh_cli, caplog, _create_roster, tmp_path, account):
+    """
+    Test to ensure user cannot create and run a script
+    with the expected pre_flight script path on target.
+    """
+    try:
+        script = pathlib.Path.home() / "hacked"
+        tmp_preflight = pathlib.Path("/tmp", "ssh_pre_flight.sh")
+        tmp_preflight.write_text(f"touch {script}")
+        os.chown(tmp_preflight, account.info.uid, account.info.gid)
+        ret = salt_ssh_cli.run("test.ping")
+        assert not script.is_file()
+        assert ret.returncode == 0
+        assert ret.stdout == '{\n"localhost": true\n}\n'
+    finally:
+        for _file in [script, tmp_preflight]:
+            if _file.is_file():
+                _file.unlink()
+
+
+def demote(user_uid, user_gid):
+    def result():
+        # os.setgid does not remove group membership, so we remove them here so they are REALLY non-root
+        os.setgroups([])
+        os.setgid(user_gid)
+        os.setuid(user_uid)
+
+    return result
+
+
+@pytest.mark.slow_test
+def test_ssh_pre_flight_perms(salt_ssh_cli, caplog, _create_roster, account):
+    """
+    Test to ensure standard user cannot run pre flight script
+    on target when user sets wrong permissions (777) on
+    ssh_pre_flight script.
+    """
+    try:
+        script = pathlib.Path("/tmp", "itworked")
+        preflight = pathlib.Path("/ssh_pre_flight.sh")
+        preflight.write_text(f"touch {str(script)}")
+        tmp_preflight = pathlib.Path("/tmp", preflight.name)
+
+        _custom_roster(salt_ssh_cli.roster_file, {"ssh_pre_flight": str(preflight)})
+        preflight.chmod(0o0777)
+        run_script = pathlib.Path("/run_script")
+        run_script.write_text(
+            f"""
+        x=1
+        while [ $x -le 200000 ]; do
+            SCRIPT=`bash {str(tmp_preflight)} 2> /dev/null; echo $?`
+            if [ ${{SCRIPT}} == 0 ]; then
+                break
+            fi
+            x=$(( $x + 1 ))
+        done
+        """
+        )
+        run_script.chmod(0o0777)
+        # pylint: disable=W1509
+        ret = subprocess.Popen(
+            ["sh", f"{run_script}"],
+            preexec_fn=demote(account.info.uid, account.info.gid),
+            stdout=None,
+            stderr=None,
+            stdin=None,
+            universal_newlines=True,
+        )
+        # pylint: enable=W1509
+        ret = salt_ssh_cli.run("test.ping")
+        assert ret.returncode == 0
+
+        # Lets make sure a different user other than root
+        # Didn't run the script
+        assert os.stat(script).st_uid != account.info.uid
+        assert script.is_file()
+    finally:
+        for _file in [script, preflight, tmp_preflight, run_script]:
+            if _file.is_file():
+                _file.unlink()
+
+
+@pytest.mark.slow_test
+def test_ssh_run_pre_flight_target_file_perms(salt_ssh_cli, _create_roster, tmp_path):
+    """
+    test ssh_pre_flight to ensure the target pre flight script
+    has the correct perms
+    """
+    perms_file = tmp_path / "perms"
+    with salt.utils.files.fopen(_create_roster["data"]["ssh_pre_flight"], "w") as fp_:
+        fp_.write(
+            f"""
+        SCRIPT_NAME=$0
+        stat -L -c "%a %G %U" $SCRIPT_NAME > {perms_file}
+        """
+        )
+
+    ret = salt_ssh_cli.run(
+        "test.ping",
+        "--pre-flight",
+    )
+    assert ret.returncode == 0
+    with salt.utils.files.fopen(perms_file) as fp:
+        data = fp.read()
+    assert data.split()[0] == "600"
+    uid = os.getuid()
+    gid = os.getgid()
+    assert data.split()[1] == grp.getgrgid(gid).gr_name
+    assert data.split()[2] == pwd.getpwuid(uid).pw_name