fix(orchestration): use base64 encoding to prevent quote stripping in docker exec

This commit consolidates several improvements and fixes to shell command
execution within orchestration:

- Use base64 encoding for `docker exec` commands to prevent argument
  parsing and quote stripping issues.
- Centralize base64 shell wrapping into utility functions.
- Apply `shlex.quote` to docker run/exec flags and shell wrapping.
- Use `printf` instead of `echo` for known_hosts population.
- Add comprehensive tests for shell utilities and orchestration.
- Update documentation on shell execution and security guidelines.

Author: jlapenna

DEVELOPERS.md

@@ -114,6 +114,37 @@ from sparkrun.orchestration.ssh import run_remote_script
 result = run_remote_script(host, script_string, timeout=120, **ssh_kwargs)
 ```
 
+### Shell Execution & Security
+
+Sparkrun frequently dynamically generates bash scripts and Docker commands that interpolate user-provided inputs (like container names, image names, or environment variables). To prevent shell injection and handle spaces/special characters, you MUST adhere to the following rules:
+
+1. **Python `shlex.quote`**: When building commands in Python (e.g., `docker run` flags), wrap all interpolated values with `shlex.quote`:
+   ```python
+   import shlex
+   cmd = f"docker run --name {shlex.quote(container_name)} {shlex.quote(image)}"
+   ```
+
+2. **Base64 Command Wrapping**: When passing complex commands (especially those with nested quotes or JSON) into `bash -c` or over SSH, use the `b64_encode_cmd` and `b64_wrap_bash` utilities from `sparkrun.utils.shell`:
+   ```python
+   from sparkrun.utils.shell import b64_encode_cmd
+   b64_cmd = b64_encode_cmd("vllm serve --hf-overrides '{\"rope\": \"yarn\"}'")
+   # The bash script should decode and execute this:
+   # printf '%s' '{b64_cmd}' | base64 -d -- | bash --noprofile --norc
+   ```
+
+3. **Use `printf` instead of `echo`**: Inside generated bash scripts (`.sh` files), never use `echo` to output interpolated Python variables. If a variable starts with a hyphen (e.g., `-n`), `echo` may interpret it as a flag. Instead, use `printf` with a format string:
+   ```bash
+   # DANGEROUS: echo "Launching {container_name}"
+   # SAFE:
+   printf "Launching %%s\n" "{container_name}"
+   ```
+   *Note: In Python string formatting (used to populate the scripts), `%` must be escaped as `%%`.*
+
+4. **Environment Variables**: When exporting variables in generated bash scripts, quote the interpolated value using `shlex.quote` in Python and omit quotes in the bash script:
+   ```python
+   env_lines.append(f"export MY_VAR={shlex.quote(val)}")
+   ```
+
 ### Runtime Architecture
 
 All runtimes extend `RuntimePlugin` (`runtimes/base.py`):

src/sparkrun/cli/_export.py

@@ -233,9 +233,6 @@ def _render_install_script(slug, recipe_yaml, cluster_yaml, cluster_name, user_h
     """Render user-level install script (no sudo needed)."""
     service_dir = "%s/.config/sparkrun/services/%s" % (user_home, slug)
     clusters_dir = "%s/.config/sparkrun/clusters" % user_home
-    # Escape single quotes in YAML content for heredoc safety
-    recipe_yaml_escaped = recipe_yaml.replace("'", "'\\''")
-    cluster_yaml_escaped = cluster_yaml.replace("'", "'\\''")
     return textwrap.dedent("""\
         #!/usr/bin/env bash
         set -euo pipefail
@@ -262,16 +259,14 @@ def _render_install_script(slug, recipe_yaml, cluster_yaml, cluster_name, user_h
         service_dir=service_dir,
         clusters_dir=clusters_dir,
         cluster_name=cluster_name,
-        recipe_yaml=recipe_yaml_escaped,
-        cluster_yaml=cluster_yaml_escaped,
+        recipe_yaml=recipe_yaml,
+        cluster_yaml=cluster_yaml,
     )
 
 
 def _render_sudo_install_script(slug, unit_contents):
     """Render sudo install script (writes unit file, enables service)."""
     unit_path = "/etc/systemd/system/sparkrun-%s.service" % slug
-    # Escape single quotes in unit contents for heredoc safety
-    unit_escaped = unit_contents.replace("'", "'\\''")
     return textwrap.dedent("""\
         #!/usr/bin/env bash
         set -euo pipefail
@@ -289,7 +284,7 @@ def _render_sudo_install_script(slug, unit_contents):
     """).format(
         unit_path=unit_path,
         slug=slug,
-        unit_contents=unit_escaped,
+        unit_contents=unit_contents,
     )
 
 

src/sparkrun/cli/_run.py

@@ -34,6 +34,7 @@
 @click.argument("recipe_name", type=RECIPE_NAME)
 @host_options
 @recipe_override_options
+@click.option("--name", "cluster_id_override", default=None, help="Override deterministic cluster ID (static container name)")
 @click.option("--solo", is_flag=True, help="Force single-node mode", hidden=True)
 @click.option("--port", type=int, default=None, help="Override serve port")
 @click.option("--served-model-name", default=None, help="Override served model name")
@@ -73,6 +74,7 @@ def run(
     hosts,
     hosts_file,
     cluster_name,
+    cluster_id_override,
     solo,
     port,
     tensor_parallel,
@@ -250,6 +252,26 @@ def run(
     if restart_policy:
         cli_executor_opts["restart_policy"] = restart_policy
 
+    # Also extract executor-specific keys from -o/--option overrides
+    executor_keys = {
+        "auto_remove",
+        "restart_policy",
+        "privileged",
+        "gpus",
+        "ipc",
+        "shm_size",
+        "network",
+        "user",
+        "security_opt",
+        "cap_add",
+        "ulimit",
+        "devices",
+        "memory_limit",
+    }
+    for key in list(overrides.keys()):
+        if key in executor_keys:
+            cli_executor_opts[key] = overrides.pop(key)
+
     # --- Diagnostics setup ---
     diag = None
     if diagnostics_path:
@@ -302,6 +324,7 @@ def run(
             dashboard=dashboard,
             init_port=init_port,
             topology=cluster_cfg.topology,
+            cluster_id_override=cluster_id_override,
             executor_config=cli_executor_opts,
             rootless=not rootful,
             auto_user=not rootful,

src/sparkrun/core/launcher.py

@@ -75,6 +75,7 @@ def launch_inference(
     dashboard: bool = False,
     init_port: int | None = None,
     topology: str | None = None,
+    cluster_id_override: str | None = None,
     # Executor config (dict for config chain layering)
     executor_config: dict | None = None,
     # note: transition to rootless by default
@@ -178,7 +179,7 @@ def launch_inference(
         serve_port = int(config_chain.get("port") or 8000)
 
     # Derive deterministic cluster_id from recipe + (trimmed) hosts
-    cluster_id = generate_cluster_id(recipe, host_list, overrides=overrides)
+    cluster_id = cluster_id_override or generate_cluster_id(recipe, host_list, overrides=overrides)
 
     # Resolve container image
     container_image = runtime.resolve_container(recipe, overrides)

src/sparkrun/core/recipe.py

@@ -533,18 +533,36 @@ def render_command(self, config_chain: Variables) -> str | None:
 
         Returns None if no command template is defined.
         """
+        import re
+
         if not self.command:
             return None
 
         rendered = self.command.strip()
 
+        def _protect_braces(m):
+            if m.group(1):
+                return "\x01"
+            elif m.group(2):
+                return "\x02"
+            return m.group(0)
+
+        # Protect double braces before applying vpd substitution.
+        # We match {{ or }} or {something}, replacing only the double braces
+        # with placeholders. This ensures that standard Python format-string
+        # escaping is respected and survives iterative substitution.
+        rendered = re.sub(r"(\{\{)|(\}\})|\{[^}]*\}", _protect_braces, rendered)
+
         # Use vpd arg_substitute for {placeholder} replacement
         # Iterate to handle nested substitutions
         last = None
         while last != rendered:
             last = rendered
             rendered = arg_substitute(rendered, config_chain)
 
+        # Restore protected double braces as literal single braces
+        rendered = rendered.replace("\x01", "{").replace("\x02", "}")
+
         # Fix trailing spaces after backslash line-continuations.
         # ``\<space><newline>`` → ``\<newline>``
         rendered = _TRAILING_SPACE_CONTINUATION_RE.sub("\\\n", rendered)

src/sparkrun/orchestration/docker.py

@@ -35,9 +35,10 @@ def docker_exec_cmd(
         parts.append("-d")
     if env:
         for key, value in sorted(env.items()):
-            parts.extend(["-e", f"{key}={value}"])
-    escaped_cmd = command.replace("'", "'\\''")
-    parts.extend([shlex.quote(container_name), "bash", "-c", "'%s'" % escaped_cmd])
+            parts.extend(["-e", shlex.quote(f"{key}={value}")])
+    from sparkrun.utils.shell import b64_wrap_bash
+
+    parts.extend([shlex.quote(container_name), "bash", "-c", shlex.quote(b64_wrap_bash(command))])
     return " ".join(parts)
 
 

src/sparkrun/orchestration/executor.py

@@ -10,11 +10,16 @@
 from __future__ import annotations
 
 import logging
+import shlex
 from abc import ABC, abstractmethod
 from dataclasses import dataclass
 
 from scitrera_app_framework.util import ext_parse_bool
 
+from sparkrun.scripts import read_script
+from sparkrun.utils import merge_env
+from sparkrun.utils.shell import b64_encode_cmd
+
 logger = logging.getLogger(__name__)
 
 # Default executor settings for DGX Spark GPU workloads.
@@ -78,7 +83,9 @@ def from_chain(cls, chain) -> ExecutorConfig:
         # still means "not set" and should fall back.
         def _get(key):
             v = chain.get(key)
-            return v if v is not None else EXECUTOR_DEFAULTS.get(key)
+            val = v if v is not None else EXECUTOR_DEFAULTS.get(key)
+            logger.debug("ExecutorConfig resolve: %s=%r (from chain: %r)", key, val, v)
+            return val
 
         return cls(
             auto_remove=ext_parse_bool(_get("auto_remove")),
@@ -209,8 +216,6 @@ def generate_launch_script(
 
         Absorbs ``scripts.py::generate_container_launch_script``.
         """
-        from sparkrun.utils import merge_env
-        from sparkrun.scripts import read_script
 
         all_env = merge_env(nccl_env, env)
         cleanup = self.stop_cmd(container_name)
@@ -243,21 +248,23 @@ def generate_exec_serve_script(
 
         Absorbs ``scripts.py::generate_exec_serve_script``.
         """
-        from sparkrun.scripts import read_script
 
         env_exports = ""
         if env:
             for key, value in sorted(env.items()):
-                env_exports += "export %s='%s'; " % (key, value)
+                env_exports += "export %s=%s; " % (key, shlex.quote(str(value)))
+
+        full_cmd = "%s%s" % (env_exports, serve_command)
 
-        escaped_cmd = serve_command.replace("'", "'\\''")
-        full_cmd = "%s%s" % (env_exports, escaped_cmd)
+        # Base64 encode the command to avoid all bash string-escaping/quoting bugs
+        # when passing it into `docker exec ... bash -c "..."`
+        b64_cmd = b64_encode_cmd(full_cmd)
 
         script_name = "exec_serve_detached.sh" if detached else "exec_serve_foreground.sh"
         template = read_script(script_name)
         return template.format(
-            container_name=container_name,
-            full_cmd=full_cmd,
+            container_name=shlex.quote(container_name),
+            b64_cmd=b64_cmd,
         )
 
     def generate_ray_head_script(
@@ -275,8 +282,6 @@ def generate_ray_head_script(
 
         Absorbs ``scripts.py::generate_ray_head_script``.
         """
-        from sparkrun.utils import merge_env
-        from sparkrun.scripts import read_script
 
         all_env = merge_env({"RAY_memory_monitor_refresh_ms": "0"}, nccl_env, env)
 
@@ -314,8 +319,6 @@ def generate_ray_worker_script(
 
         Absorbs ``scripts.py::generate_ray_worker_script``.
         """
-        from sparkrun.utils import merge_env
-        from sparkrun.scripts import read_script
 
         all_env = merge_env({"RAY_memory_monitor_refresh_ms": "0"}, nccl_env, env)
 
@@ -356,7 +359,6 @@ def generate_node_script(
 
         Absorbs ``base.py::_generate_node_script``.
         """
-        from sparkrun.utils import merge_env
 
         all_env = merge_env(nccl_env, env)
         cleanup = self.stop_cmd(container_name)
@@ -374,19 +376,24 @@ def generate_node_script(
             "#!/bin/bash\n"
             "set -uo pipefail\n"
             "\n"
-            "echo 'Cleaning up existing container: %(name)s'\n"
+            "printf 'Cleaning up existing container: %%s\\n' %(name)s\n"
             "%(cleanup)s\n"
             "\n"
-            "echo 'Launching %(label)s: %(name)s'\n"
+            "printf 'Launching %%s: %%s\\n' %(label)s %(name)s\n"
             "%(run_cmd)s\n"
             "\n"
             "# Verify container started\n"
             "sleep 1\n"
             "if docker ps --format '{{.Names}}' | grep -q '^%(name)s$'; then\n"
-            "    echo 'Container %(name)s launched successfully'\n"
+            "    printf 'Container %%s launched successfully\\n' %(name)s\n"
             "else\n"
-            "    echo 'ERROR: Container %(name)s failed to start' >&2\n"
+            "    printf 'ERROR: Container %%s failed to start\\n' %(name)s >&2\n"
             "    docker logs %(name)s 2>&1 | tail -20 || true\n"
             "    exit 1\n"
             "fi\n"
-        ) % {"name": container_name, "cleanup": cleanup, "run_cmd": run, "label": label}
+        ) % {
+            "name": shlex.quote(container_name),
+            "cleanup": cleanup,
+            "run_cmd": run,
+            "label": shlex.quote(label),
+        }