Remove the usage of shell=True when running commands using subprocess (#1443)

* fix(bandit b602): remove the usage of shell=True when running commands using subprocess

Signed-off-by: Pant, Akshay <[email protected]>

* fix(execute): keyword argument name

Signed-off-by: Pant, Akshay <[email protected]>

* fix(execute): value type in dict

Signed-off-by: Pant, Akshay <[email protected]>

* fix(dockerize): command args

Signed-off-by: Pant, Akshay <[email protected]>

---------

Signed-off-by: Pant, Akshay <[email protected]>

Author: theakshaypant

openfl/interface/workspace.py

@@ -404,7 +404,7 @@ def dockerize_(context, save: bool, rebuild: bool, enclave_key: str, base_image:
     options = " ".join(options)
     logging.info(f"Using base image: {base_image}")
     if enclave_key is None:
-        _execute("openssl genrsa -out key.pem -3 3072")
+        _execute(["openssl", "genrsa", "-out", "key.pem", "-3", "3072"])
         enclave_key = os.path.abspath("key.pem")
         logging.info(f"Generated new enclave key: {enclave_key}")
     else:
@@ -414,28 +414,27 @@ def dockerize_(context, save: bool, rebuild: bool, enclave_key: str, base_image:
         logging.info(f"Using enclave key: {enclave_key}")
 
     logging.info("Building workspace image")
-    ws_image_build_cmd = (
-        "DOCKER_BUILDKIT=1 docker build {options} "
-        "--build-arg WORKSPACE_NAME={workspace_name} "
-        "--secret id=signer-key,src={enclave_key} "
-        "-t {image_name} "
-        "-f {dockerfile} "
-        "{build_context}"
-    ).format(
-        options=options,
-        image_name=workspace_name,
-        workspace_name=workspace_name,
-        enclave_key=enclave_key,
-        dockerfile=os.path.join(SITEPACKS, "openfl-docker", "Dockerfile.workspace"),
-        build_context=".",
-    )
-    _execute(ws_image_build_cmd)
+    ws_image_build_cmd = [
+        "docker",
+        "build",
+        *options.split(),
+        "--build-arg",
+        f"WORKSPACE_NAME={workspace_name}",
+        "--secret",
+        f"id=signer-key,src={enclave_key}",
+        "-t",
+        workspace_name,
+        "-f",
+        os.path.join(SITEPACKS, "openfl-docker", "Dockerfile.workspace"),
+        ".",
+    ]
+    _execute(ws_image_build_cmd, env_var={"DOCKER_BUILDKIT": "1"})
 
     # Export workspace as tarball (optional)
     if save:
         logging.info("Saving workspace docker image...")
-        save_image_cmd = "docker save {image_name} -o {image_name}.tar"
-        _execute(save_image_cmd.format(image_name=workspace_name))
+        save_image_cmd = ["docker", "save", workspace_name, "-o", f"{workspace_name}.tar"]
+        _execute(save_image_cmd)
         logging.info(f"Docker image saved to file: {workspace_name}.tar")
 
 
@@ -455,7 +454,7 @@ def apply_template_plan(prefix, template):
     Plan.dump(prefix / "plan" / "plan.yaml", template_plan.config)
 
 
-def _execute(cmd: str, verbose=True) -> None:
+def _execute(cmd: str, env_var: dict = {}, verbose=True) -> None:
     """Executes `cmd` as a subprocess
 
     Args:
@@ -468,7 +467,7 @@ def _execute(cmd: str, verbose=True) -> None:
         `stdout` of the command as list of messages
     """
     logging.info(f"Executing: {cmd}")
-    process = subprocess.Popen(cmd, shell=True, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
+    process = subprocess.Popen(cmd, env=env_var, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
     stdout_log = []
     for line in process.stdout:
         msg = line.rstrip().decode("utf-8")

openfl/utilities/ca/ca.py

@@ -53,11 +53,20 @@ def get_token(name, ca_url, ca_path="."):
     root_crt = step_config_dir / "certs" / "root_ca.crt"
     try:
         token = subprocess.check_output(
-            f"{step_path} ca token {name} "
-            f"--key {priv_json} --root {root_crt} "
-            f"--password-file {pass_file} "
-            f"--ca-url {ca_url}",
-            shell=True,
+            [
+                step_path,
+                "ca",
+                "token",
+                name,
+                "--key",
+                priv_json,
+                "--root",
+                root_crt,
+                "--password-file",
+                pass_file,
+                "--ca-url",
+                ca_url,
+            ]
         )
     except subprocess.CalledProcessError as exc:
         logger.error("Error code %s: %s", exc.returncode, exc.output)
@@ -131,9 +140,21 @@ def certify(name, cert_path: Path, token_with_cert, ca_path: Path):
     with open(f"{cert_path}/root_ca.crt", mode="wb") as file:
         file.write(root_certificate)
     check_call(
-        f"{step_path} ca certificate {name} {cert_path}/{name}.crt "
-        f"{cert_path}/{name}.key --kty EC --curve P-384 -f --token {token}",
-        shell=True,
+        [
+            step_path,
+            "ca",
+            "certificate",
+            name,
+            f"{cert_path}/{name}.crt",
+            f"{cert_path}/{name}.key",
+            "--kty",
+            "EC",
+            "--curve",
+            "P-384",
+            "-f",
+            "--token",
+            token,
+        ]
     )
 
 
@@ -186,7 +207,7 @@ def run_ca(step_ca, pass_file, ca_json):
     """
     if _check_kill_process("step-ca", confirmation=True):
         logger.info("Up CA server")
-        check_call(f"{step_ca} --password-file {pass_file} {ca_json}", shell=True)
+        check_call([step_ca, "--password-file", pass_file, ca_json])
 
 
 def _check_kill_process(pstring, confirmation=False):
@@ -202,11 +223,22 @@ def _check_kill_process(pstring, confirmation=False):
     """
     pids = []
     proc = subprocess.Popen(
-        f"ps ax | grep {pstring} | grep -v grep",
-        shell=True,
+        ["ps", "ax"],
+        stdout=subprocess.PIPE,
+    )
+    grep_proc = subprocess.Popen(
+        ["grep", pstring],
+        stdin=proc.stdout,
+        stdout=subprocess.PIPE,
+    )
+    proc.stdout.close()
+    grep_proc_2 = subprocess.Popen(
+        ["grep", "-v", "grep"],
+        stdin=grep_proc.stdout,
         stdout=subprocess.PIPE,
     )
-    text = proc.communicate()[0].decode("utf-8")
+    grep_proc.stdout.close()
+    text = grep_proc_2.communicate()[0].decode("utf-8")
 
     for line in text.splitlines():
         fields = line.split()
@@ -249,21 +281,38 @@ def _create_ca(ca_path: Path, ca_url: str, password: str):
     shutil.rmtree(step_config_dir, ignore_errors=True)
     name = ca_url.split(":")[0]
     check_call(
-        f"{step_path} ca init --name name --dns {name} "
-        f"--address {ca_url}  --provisioner prov "
-        f"--password-file {pki_dir}/pass_file",
-        shell=True,
+        [
+            step_path,
+            "ca",
+            "init",
+            "--name",
+            "name",
+            "--dns",
+            name,
+            "--address",
+            ca_url,
+            "--provisioner",
+            "prov",
+            "--password-file",
+            f"{pki_dir}/pass_file",
+        ]
     )
 
-    check_call(f"{step_path} ca provisioner remove prov --all", shell=True)
+    check_call([step_path, "ca", "provisioner", "remove", "prov", "--all"])
+
     check_call(
-        f"{step_path} crypto jwk create {step_config_dir}/certs/pub.json "
-        f"{step_config_dir}/secrets/priv.json --password-file={pki_dir}/pass_file",
-        shell=True,
+        [
+            step_path,
+            "crypto",
+            "jwk",
+            "create",
+            f"{step_config_dir}/certs/pub.json",
+            f"{step_config_dir}/secrets/priv.json",
+            f"--password-file={pki_dir}/pass_file",
+        ]
     )
     check_call(
-        f"{step_path} ca provisioner add provisioner {step_config_dir}/certs/pub.json",
-        shell=True,
+        [step_path, "ca", "provisioner", "add", "provisioner", f"{step_config_dir}/certs/pub.json"]
     )