diff --git a/salt/client/ssh/ssh_py_shim.py b/salt/client/ssh/ssh_py_shim.py index c0ce0fd7de5..4abc8cae4f8 100644 --- a/salt/client/ssh/ssh_py_shim.py +++ b/salt/client/ssh/ssh_py_shim.py @@ -239,12 +239,14 @@ def get_executable(): "python", ) for py_cmd in pycmds: - cmd = ( - py_cmd - + " -c \"import sys; sys.stdout.write('%s:%s' % (sys.version_info[0], sys.version_info[1]))\"" - ) stdout, _ = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True + [ + py_cmd, + "-c" + "import sys; sys.stdout.write('%s:%s' % (sys.version_info[0], sys.version_info[1]))", + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ).communicate() if sys.version_info[0] == 2 and sys.version_info[1] < 7: stdout = stdout.decode(get_system_encoding(), "replace").strip() diff --git a/salt/modules/archive.py b/salt/modules/archive.py index 2e9c977ce4d..d108172e9e8 100644 --- a/salt/modules/archive.py +++ b/salt/modules/archive.py @@ -203,12 +203,12 @@ def list_( else: if not salt.utils.path.which("tar"): raise CommandExecutionError("'tar' command not available") - if decompress_cmd is not None: + if decompress_cmd is not None and isinstance(decompress_cmd, str): # Guard against shell injection try: - decompress_cmd = " ".join( - [shlex.quote(x) for x in shlex.split(decompress_cmd)] - ) + decompress_cmd = [ + shlex.quote(x) for x in shlex.split(decompress_cmd) + ] except AttributeError: raise CommandExecutionError("Invalid CLI options") else: @@ -221,12 +221,11 @@ def list_( ) == 0 ): - decompress_cmd = "xz --decompress --stdout" + decompress_cmd = ["xz", "--decompress", "--stdout"] if decompress_cmd: decompressed = subprocess.Popen( - "{} {}".format(decompress_cmd, shlex.quote(cached)), - shell=True, + decompress_cmd + [shlex.quote(cached)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) diff --git a/salt/modules/virt.py b/salt/modules/virt.py index 09c1771601c..17e3ba7b9bc 100644 --- a/salt/modules/virt.py +++ b/salt/modules/virt.py @@ -656,19 +656,15 @@ def _libvirt_creds(): """ Returns the user and group that the disk images should be owned by """ - g_cmd = "grep ^\\s*group /etc/libvirt/qemu.conf" - u_cmd = "grep ^\\s*user /etc/libvirt/qemu.conf" + g_cmd = ["grep", "^\\s*group", "/etc/libvirt/qemu.conf"] + u_cmd = ["grep", "^\\s*user", "/etc/libvirt/qemu.conf"] try: - stdout = subprocess.Popen( - g_cmd, shell=True, stdout=subprocess.PIPE - ).communicate()[0] + stdout = subprocess.Popen(g_cmd, stdout=subprocess.PIPE).communicate()[0] group = salt.utils.stringutils.to_str(stdout).split('"')[1] except IndexError: group = "root" try: - stdout = subprocess.Popen( - u_cmd, shell=True, stdout=subprocess.PIPE - ).communicate()[0] + stdout = subprocess.Popen(u_cmd, stdout=subprocess.PIPE).communicate()[0] user = salt.utils.stringutils.to_str(stdout).split('"')[1] except IndexError: user = "root" @@ -5728,7 +5724,7 @@ def seed_non_shared_migrate(disks, force=False): # the target exists, check to see if it is compatible pre = salt.utils.yaml.safe_load( subprocess.Popen( - "qemu-img info arch", shell=True, stdout=subprocess.PIPE + ["qemu-img", "info", "arch"], stdout=subprocess.PIPE ).communicate()[0] ) if ( @@ -5740,11 +5736,9 @@ def seed_non_shared_migrate(disks, force=False): os.makedirs(os.path.dirname(fn_)) if os.path.isfile(fn_): os.remove(fn_) - cmd = "qemu-img create -f " + form + " " + fn_ + " " + size - subprocess.call(cmd, shell=True) + subprocess.call(["qemu-img", "create", "-f", form, fn_, size]) creds = _libvirt_creds() - cmd = "chown " + creds["user"] + ":" + creds["group"] + " " + fn_ - subprocess.call(cmd, shell=True) + subprocess.call(["chown", "{user}:{group}".format(**creds), fn_]) return True @@ -5979,7 +5973,7 @@ def _is_bhyve_hyper(): vmm_enabled = False try: stdout = subprocess.Popen( - sysctl_cmd, shell=True, stdout=subprocess.PIPE + ["sysctl", "hw.vmm.create"], stdout=subprocess.PIPE ).communicate()[0] vmm_enabled = len(salt.utils.stringutils.to_str(stdout).split('"')[1]) != 0 except IndexError: diff --git a/salt/pillar/libvirt.py b/salt/pillar/libvirt.py index f502580873b..072293df251 100644 --- a/salt/pillar/libvirt.py +++ b/salt/pillar/libvirt.py @@ -71,13 +71,27 @@ def gen_hyper_keys( with salt.utils.files.fopen(cainfo, "w+") as fp_: fp_.write("cn = salted\nca\ncert_signing_key") if not os.path.isfile(cakey): - subprocess.call("certtool --generate-privkey > {}".format(cakey), shell=True) + proc = subprocess.run( + ["certtool", "--generate-privkey"], + stdout=subprocess.PIPE, + universal_newlines=True, + check=True, + ) + with salt.utils.files.fopen(cakey, "w") as wfh: + wfh.write(proc.stdout) if not os.path.isfile(cacert): - cmd = ( - "certtool --generate-self-signed --load-privkey {} " - "--template {} --outfile {}" - ).format(cakey, cainfo, cacert) - subprocess.call(cmd, shell=True) + subprocess.call( + [ + "certtool", + "--generate-self-signed", + "--load-privkey", + cakey, + "--template", + cainfo, + "--outfile", + cacert, + ] + ) sub_dir = os.path.join(key_dir, minion_id) if not os.path.isdir(sub_dir): os.makedirs(sub_dir) @@ -98,14 +112,31 @@ def gen_hyper_keys( ) fp_.write(infodat) if not os.path.isfile(priv): - subprocess.call("certtool --generate-privkey > {}".format(priv), shell=True) + proc = subprocess.run( + ["certtool", "--generate-privkey"], + stdout=subprocess.PIPE, + universal_newlines=True, + check=True, + ) + with salt.utils.files.fopen(priv, "w") as wfh: + wfh.write(proc.stdout) if not os.path.isfile(cert): - cmd = ( - "certtool --generate-certificate --load-privkey {} " - "--load-ca-certificate {} --load-ca-privkey {} " - "--template {} --outfile {}" - ).format(priv, cacert, cakey, srvinfo, cert) - subprocess.call(cmd, shell=True) + subprocess.call( + [ + "certtool", + "--generate-certificate", + "--load-privkey", + priv, + "--load-ca-certificate", + cacert, + "--load-ca-privkey", + cakey, + "--template", + srvinfo, + "--outfile", + cert, + ] + ) if not os.path.isfile(clientinfo): with salt.utils.files.fopen(clientinfo, "w+") as fp_: infodat = salt.utils.stringutils.to_str( @@ -118,11 +149,28 @@ def gen_hyper_keys( ) fp_.write(infodat) if not os.path.isfile(cpriv): - subprocess.call("certtool --generate-privkey > {}".format(cpriv), shell=True) + proc = subprocess.run( + ["certtool", "--generate-privkey"], + stdout=subprocess.PIPE, + universal_newlines=True, + check=True, + ) + with salt.utils.files.fopen(cpriv, "w") as wfh: + wfh.write(proc.stdout) if not os.path.isfile(ccert): - cmd = ( - "certtool --generate-certificate --load-privkey {} " - "--load-ca-certificate {} --load-ca-privkey {} " - "--template {} --outfile {}" - ).format(cpriv, cacert, cakey, clientinfo, ccert) - subprocess.call(cmd, shell=True) + subprocess.call( + [ + "certtool", + "--generate-certificate", + "--load-privkey", + cpriv, + "--load-ca-certificate", + cacert, + "--load-ca-privkey", + cakey, + "--template", + clientinfo, + "--outfile", + ccert, + ] + ) diff --git a/salt/tops/ext_nodes.py b/salt/tops/ext_nodes.py index cca367fc818..1174747fefe 100644 --- a/salt/tops/ext_nodes.py +++ b/salt/tops/ext_nodes.py @@ -45,10 +45,17 @@ The above essentially is the same as a top.sls containing the following: - database """ import logging +import shlex import subprocess +import salt.utils.platform import salt.utils.yaml +if salt.utils.platform.is_windows(): + from salt.utils.win_functions import escape_argument as _cmd_quote +else: + _cmd_quote = shlex.quote + log = logging.getLogger(__name__) @@ -67,10 +74,20 @@ def top(**kwargs): """ if "id" not in kwargs["opts"]: return {} - cmd = "{} {}".format(__opts__["master_tops"]["ext_nodes"], kwargs["opts"]["id"]) - ndata = salt.utils.yaml.safe_load( - subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE).communicate()[0] + proc = subprocess.run( + [ + _cmd_quote(part) + for part in shlex.split( + __opts__["master_tops"]["ext_nodes"], + posix=salt.utils.platform.is_windows() is False, + ) + + [_cmd_quote(kwargs["opts"]["id"])] + ], + stdout=subprocess.PIPE, + check=True, + shell=True, # nosec ) + ndata = salt.utils.yaml.safe_load(proc.stdout) if not ndata: log.info("master_tops ext_nodes call did not return any data") ret = {} diff --git a/salt/utils/cloud.py b/salt/utils/cloud.py index 989109f8cf0..f827955b6c0 100644 --- a/salt/utils/cloud.py +++ b/salt/utils/cloud.py @@ -2535,8 +2535,7 @@ def remove_sshkey(host, known_hosts=None): else: log.debug("Removing ssh key for %s from known hosts file", host) - cmd = "ssh-keygen -R {}".format(host) - subprocess.call(cmd, shell=True) + subprocess.call(["ssh-keygen", "-R", host]) def wait_for_ip( diff --git a/salt/utils/network.py b/salt/utils/network.py index f8d50998e4e..144f9dc8505 100644 --- a/salt/utils/network.py +++ b/salt/utils/network.py @@ -896,15 +896,13 @@ def linux_interfaces(): ifconfig_path = None if ip_path else salt.utils.path.which("ifconfig") if ip_path: cmd1 = subprocess.Popen( - "{} link show".format(ip_path), - shell=True, + [ip_path, "link", "show"], close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ).communicate()[0] cmd2 = subprocess.Popen( - "{} addr show".format(ip_path), - shell=True, + [ip_path, "addr", "show"], close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -916,10 +914,7 @@ def linux_interfaces(): ) elif ifconfig_path: cmd = subprocess.Popen( - "{} -a".format(ifconfig_path), - shell=True, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + [ifconfig_path, "-a"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ).communicate()[0] ifaces = _interfaces_ifconfig(salt.utils.stringutils.to_str(cmd)) return ifaces @@ -1062,10 +1057,7 @@ def junos_interfaces(): """ ifconfig_path = salt.utils.path.which("ifconfig") cmd = subprocess.Popen( - "{} -a".format(ifconfig_path), - shell=True, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + [ifconfig_path, "-a"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ).communicate()[0] return _junos_interfaces_ifconfig(salt.utils.stringutils.to_str(cmd)) @@ -1082,10 +1074,7 @@ def netbsd_interfaces(): ifconfig_path = salt.utils.path.which("ifconfig") cmd = subprocess.Popen( - "{} -a".format(ifconfig_path), - shell=True, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, + [ifconfig_path, "-a"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ).communicate()[0] return _netbsd_interfaces_ifconfig(salt.utils.stringutils.to_str(cmd)) @@ -1227,8 +1216,10 @@ def _hw_addr_aix(iface): MAC address not available in through interfaces """ cmd = subprocess.Popen( - "entstat -d {} | grep 'Hardware Address'".format(iface), - shell=True, + ["grep", "Hardware Address"], + stdin=subprocess.Popen( + ["entstat", "-d", iface], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + ).stdout, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ).communicate()[0] diff --git a/salt/utils/pkg/rpm.py b/salt/utils/pkg/rpm.py index d1ef18e0d0e..ba600e106b1 100644 --- a/salt/utils/pkg/rpm.py +++ b/salt/utils/pkg/rpm.py @@ -53,8 +53,7 @@ def get_osarch(): """ if salt.utils.path.which("rpm"): ret = subprocess.Popen( - 'rpm --eval "%{_host_cpu}"', - shell=True, + ["rpm", "--eval", "%{_host_cpu}"], close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, diff --git a/salt/utils/win_update.py b/salt/utils/win_update.py index 147d7d04f10..c1cbb58bc22 100644 --- a/salt/utils/win_update.py +++ b/salt/utils/win_update.py @@ -1116,9 +1116,7 @@ class WindowsUpdateAgent: try: log.debug(cmd) - p = subprocess.Popen( - cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE - ) + p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) return p.communicate() except OSError as exc: diff --git a/setup.py b/setup.py index a5663305b2e..073109c3828 100755 --- a/setup.py +++ b/setup.py @@ -685,25 +685,25 @@ class TestCommand(Command): """ def run(self): - from subprocess import Popen + # This should either be removed or migrated to use nox + import subprocess self.run_command("build") build_cmd = self.get_finalized_command("build_ext") runner = os.path.abspath("tests/runtests.py") - test_cmd = sys.executable + " {}".format(runner) + test_cmd = [sys.executable, runner] if self.runtests_opts: - test_cmd += " {}".format(self.runtests_opts) + test_cmd.extend(self.runtests_opts.split()) print("running test") - test_process = Popen( + ret = subprocess.run( test_cmd, - shell=True, stdout=sys.stdout, stderr=sys.stderr, cwd=build_cmd.build_lib, + check=False, ) - test_process.communicate() - sys.exit(test_process.returncode) + sys.exit(ret.returncode) class Clean(clean): diff --git a/tests/unit/utils/test_pkg.py b/tests/unit/utils/test_pkg.py index b4a67b8e57c..3e360d78804 100644 --- a/tests/unit/utils/test_pkg.py +++ b/tests/unit/utils/test_pkg.py @@ -1,10 +1,6 @@ -# -*- coding: utf-8 -*- - -from __future__ import absolute_import, print_function, unicode_literals - import salt.utils.pkg from salt.utils.pkg import rpm -from tests.support.mock import MagicMock, patch +from tests.support.mock import ANY, MagicMock, patch from tests.support.unit import TestCase @@ -65,10 +61,9 @@ class PkgRPMTestCase(TestCase): with patch("salt.utils.pkg.rpm.subprocess", subprocess_mock): assert rpm.get_osarch() == "Z80" assert subprocess_mock.Popen.call_count == 2 # One within the mock - assert subprocess_mock.Popen.call_args[1]["close_fds"] - assert subprocess_mock.Popen.call_args[1]["shell"] - assert len(subprocess_mock.Popen.call_args_list) == 2 - assert subprocess_mock.Popen.call_args[0][0] == 'rpm --eval "%{_host_cpu}"' + subprocess_mock.Popen.assert_called_with( + ["rpm", "--eval", "%{_host_cpu}"], close_fds=True, stderr=ANY, stdout=ANY + ) @patch("salt.utils.path.which", MagicMock(return_value=False)) @patch("salt.utils.pkg.rpm.subprocess", MagicMock(return_value=False))