vm-env-secrets passes while atomic-vm.sh test-exec puts every provider key in the ssh command line

[vladimirrott]

tests/e2e/vm-env-secrets.test.sh passes on 61b3a87 while atomic-vm.sh test-exec puts every provider API key into the ssh command line. #299 fixed that shape in cmd_run and cmd_provision. cmd_test_exec was never converted, and the gate cannot see it.

The live exposure

tests/e2e/atomic-vm.sh:509-526 builds an accumulator and interpolates it into the command string:

    local env_prefix=""
    for var in SYSKNIFE_ALLOW_DESTRUCTIVE ... \
               OPENAI_API_KEY ANTHROPIC_API_KEY GEMINI_API_KEY \
               GROQ_API_KEY DEEPSEEK_API_KEY MISTRAL_API_KEY XAI_API_KEY; do
        eval "val=\${$var:-}"
        if [ -n "$val" ]; then
            env_prefix+=" $var='$val'"
        fi
    done
    cmd_ssh "cd /home/${VM_USER}/sysknife &&${env_prefix} bash tests/e2e/exec/run-exec-stories.sh${story_args}"

Extracting cmd_test_exec from the unmodified file and stubbing cmd_ssh to print its own argument shows what ssh receives. Every provider key in the caller's environment was cleared first except a planted placeholder:

$ env -u ANTHROPIC_API_KEY -u GEMINI_API_KEY -u GROQ_API_KEY -u DEEPSEEK_API_KEY \
      -u MISTRAL_API_KEY -u XAI_API_KEY bash inject-safe.sh
ssh argv[1] = cd /home/fedora/sysknife && SYSKNIFE_LLM_MODEL='gpt-4o'; id #' OPENAI_API_KEY='sk-PLACEHOLDER-000' bash tests/e2e/exec/run-exec-stories.sh

Two things in that one line, and both are what #299's commit message says it closed:

• The key value sits in the command string, so it lands in the guest process table for the length of the story run. Any account on the VM can read it out of /proc.
• env_prefix+=" $var='$val'" wraps the value in single quotes without escaping. I set SYSKNIFE_LLM_MODEL to gpt-4o'; id #, the quote closed early, and id became a separate command on the guest. cmd_run avoids this by using printf %q; cmd_test_exec does not.

The function reads the keys straight out of the ambient environment, so anything that captures that command string captures live credentials: a set -x trace, a CI log, a shell history file.

Why the gate stays green

tests/e2e/vm-env-secrets.test.sh:46-49 is the structural check:

    if grep -nE 'cmd_ssh[^#]*sudo *[$"]?\$\{?[A-Za-z_]' "$harness" \
        | grep -vE 'sudo (sh|bash) -c' >/dev/null; then

It requires the word sudo, and it only inspects the token immediately after it. cmd_test_exec runs as VM_USER on purpose, because group membership decides the daemon's CallerRole and root holds no sysknife groups, so the line carries no sudo at all. Run by hand on the current tree:

$ grep -nE 'cmd_ssh[^#]*sudo *[$"]?\$\{?[A-Za-z_]' tests/e2e/atomic-vm.sh
$ echo $?
1

The key-name check at line 53 misses it too, because the line names ${env_prefix} rather than any *_API_KEY.

Two more holes in the same file, both found by mutation:

The harness list is hand-written and never compared against the tree.

harnesses=(
    "tests/e2e/atomic-vm.sh"
    "tests/e2e/ubuntu-vm.sh"
)

I copied ubuntu-vm.sh to fedora-vm.sh with export OPENAI_API_KEY=${OPENAI_API_KEY} spliced onto its cmd_ssh line. The gate reported success and did not read the file:

$ bash tests/e2e/vm-env-secrets.test.sh
VM environment secrets are stdin-only across 2 harnesses.

Emptying the array does the same thing, at rc=0, printing across 0 harnesses.

Line 53 runs a short-circuiting reader on the right of a pipe under pipefail.

        if grep -n 'cmd_ssh' "$harness" | grep -Fq "$key"; then

grep -Fq exits on the first match, the producer takes SIGPIPE, and pipefail turns a real hit into a miss. On the current harnesses the producer writes 619 and 829 bytes, so it never fires. Past the pipe buffer it stops working:

producer output   24217 bytes -> 100/100 hits reported
producer output   48617 bytes ->  55/100 hits reported
producer output   73420 bytes ->  21/100 hits reported
producer output   98620 bytes ->   7/100 hits reported

Latent today. Worth closing while the file is open.

Patch

Two files, and they have to land together: the gate goes red on main until the harness is fixed, which is the point.

The gate now discovers harnesses with grep -l '^cmd_ssh()' tests/e2e/*.sh and fails when that selects fewer than two. It reads every variable interpolated into any cmd_ssh command string and requires each to be on a declared allowlist (VM_USER, GUEST_ENV_FILE, story_args), so an accumulator fails whatever it is called and wherever it sits in the line. It checks every stdin transfer for rm -f, umask 077 and cat > rather than one literal. The two grep | grep -Fq pipes become here-strings.

The harness change routes cmd_test_exec through the same stdin path as cmd_run. One decision is yours rather than mine: the existing write_guest_env_file writes a root-owned 0600 file, and this consumer runs as VM_USER and cannot read it. I added a write_guest_env_file_for_user variant that chowns the file to VM_USER after writing it. Handing the file to a non-root account is a real widening, small, and I would rather you looked at it than had it arrive silently.

vm-env-secrets-argv.patch

diff --git a/tests/e2e/atomic-vm.sh b/tests/e2e/atomic-vm.sh
index 087ebb2..84d316e 100755
--- a/tests/e2e/atomic-vm.sh
+++ b/tests/e2e/atomic-vm.sh
@@ -117,6 +117,25 @@ ssh_opts() {
 log() { printf '[atomic-vm] %s\n' "$*" >&2; }
 die() { log "ERROR: $*"; exit 1; }
 
+write_guest_env_file_for_user() {
+    # cmd_test_exec runs as VM_USER on purpose: group membership decides the
+    # daemon's CallerRole, and root holds no sysknife groups. A root-owned 0600
+    # file is unreadable by that consumer, so this variant hands the file to
+    # VM_USER. Same stdin transfer, same mode, same fresh inode.
+    local assignment value var
+    local assignments=()
+    for var in "$@"; do
+        eval "value=\${$var-}"
+        if [ -n "$value" ]; then
+            printf -v assignment '%s=%q' "$var" "$value"
+            assignments+=("$assignment")
+        fi
+    done
+
+    printf '%s\n' "${assignments[@]}" \
+        | cmd_ssh "sudo sh -c 'rm -f ${GUEST_ENV_FILE} && umask 077 && cat > ${GUEST_ENV_FILE} && chown ${VM_USER} ${GUEST_ENV_FILE}'"
+}
+
 write_guest_env_file() {
     local assignment value var
     local assignments=()
@@ -506,24 +525,19 @@ cmd_test_exec() {
     # Run as VM_USER (not root) so sysknife connects to the daemon socket with
     # the user's group membership (sysknife/sysknife-dev/sysknife-admin) and
     # gets the correct CallerRole. Root has no sysknife groups and gets Observer.
-    local env_prefix=""
-    for var in SYSKNIFE_ALLOW_DESTRUCTIVE SYSKNIFE_LLM_PROVIDER SYSKNIFE_LLM_MODEL \
-               SYSKNIFE_TEST_MODEL SYSKNIFE_OLLAMA_URL SYSKNIFE_SOCKET SYSKNIFE_LISTEN_URI \
-               SYSKNIFE_STORY_TIMEOUT SYSKNIFE_MAX_RPM \
-               SYSKNIFE_CASSETTE SYSKNIFE_CASSETTE_MODE \
-               OPENAI_API_KEY ANTHROPIC_API_KEY GEMINI_API_KEY \
-               GROQ_API_KEY DEEPSEEK_API_KEY MISTRAL_API_KEY XAI_API_KEY; do
-        eval "val=\${$var:-}"
-        if [ -n "$val" ]; then
-            env_prefix+=" $var='$val'"
-        fi
-    done
+    write_guest_env_file_for_user \
+        SYSKNIFE_ALLOW_DESTRUCTIVE SYSKNIFE_LLM_PROVIDER SYSKNIFE_LLM_MODEL \
+        SYSKNIFE_TEST_MODEL SYSKNIFE_OLLAMA_URL SYSKNIFE_SOCKET SYSKNIFE_LISTEN_URI \
+        SYSKNIFE_STORY_TIMEOUT SYSKNIFE_MAX_RPM \
+        SYSKNIFE_CASSETTE SYSKNIFE_CASSETTE_MODE \
+        OPENAI_API_KEY ANTHROPIC_API_KEY GEMINI_API_KEY \
+        GROQ_API_KEY DEEPSEEK_API_KEY MISTRAL_API_KEY XAI_API_KEY
 
     local story_args=""
     if [ $# -gt 0 ]; then
         story_args=" $*"
     fi
-    cmd_ssh "cd /home/${VM_USER}/sysknife &&${env_prefix} bash tests/e2e/exec/run-exec-stories.sh${story_args}"
+    cmd_ssh "cd /home/${VM_USER}/sysknife && bash -c 'set -e; trap \"rm -f ${GUEST_ENV_FILE}\" EXIT; set -a; . ${GUEST_ENV_FILE}; set +a; rm -f ${GUEST_ENV_FILE}; trap - EXIT; exec bash tests/e2e/exec/run-exec-stories.sh${story_args}'"
 }
 
 cmd_snapshot() {
diff --git a/tests/e2e/vm-env-secrets.test.sh b/tests/e2e/vm-env-secrets.test.sh
index 701ec37..5626931 100755
--- a/tests/e2e/vm-env-secrets.test.sh
+++ b/tests/e2e/vm-env-secrets.test.sh
@@ -4,10 +4,19 @@ set -euo pipefail
 
 repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
 config_rs="$repo_root/crates/sysknife-brain/src/config.rs"
-harnesses=(
-    "tests/e2e/atomic-vm.sh"
-    "tests/e2e/ubuntu-vm.sh"
-)
+
+# Discover the harnesses instead of listing them. The hand-written list held
+# two entries and nothing compared it against the tree, so a third harness --
+# or an emptied list -- left this gate reporting success over zero files.
+harnesses=()
+while IFS= read -r path; do
+    harnesses+=("${path#"$repo_root"/}")
+done < <(grep -l '^cmd_ssh()' "$repo_root"/tests/e2e/*.sh | sort)
+if [ "${#harnesses[@]}" -lt 2 ]; then
+    printf 'harness discovery found %d file(s) defining cmd_ssh; expected at least 2\n' \
+        "${#harnesses[@]}" >&2
+    exit 1
+fi
 
 keys=()
 while IFS= read -r key; do
@@ -18,6 +27,12 @@ if [ "${#keys[@]}" -lt 7 ]; then
     exit 1
 fi
 
+# Variables that may appear inside a command string handed to cmd_ssh. Each is
+# a path or an argument list, never a credential. Anything else interpolated
+# into that string reaches the guest command line, so a new name has to be
+# added here deliberately rather than passing by default.
+allowed_vars=(VM_USER GUEST_ENV_FILE story_args)
+
 failures=0
 report() {
     printf 'FAIL  %s\n' "$1" >&2
@@ -27,30 +42,63 @@ report() {
 for rel in "${harnesses[@]}"; do
     harness="$repo_root/$rel"
 
-    [ "$(grep -c '^[[:space:]]*write_guest_env_file \\' "$harness")" -eq 2 ] \
+    [ "$(grep -c '^[[:space:]]*write_guest_env_file \\' "$harness")" -ge 2 ] \
         || report "$rel must stage environment for both provision and run"
-    grep -Fq "| cmd_ssh \"sudo sh -c 'rm -f \${GUEST_ENV_FILE} && umask 077 && cat > \${GUEST_ENV_FILE}'\"" "$harness" \
-        || report "$rel does not transfer the environment over SSH stdin into a fresh 0600 file"
-    [ "$(grep -c 'rm -f \${GUEST_ENV_FILE}.*exec bash tests/e2e/' "$harness")" -eq 2 ] \
+    # Every stdin transfer, not one spelling of one of them. Anchoring on a
+    # single literal meant a second transfer added later (the VM-user variant)
+    # inherited no check at all: dropping its `umask 077` left this green.
+    transfers="$(grep -n '| cmd_ssh "sudo sh -c' "$harness" || true)"
+    if [ -z "$transfers" ]; then
+        report "$rel has no stdin environment transfer at all"
+    fi
+    while IFS= read -r line; do
+        [ -n "$line" ] || continue
+        for required in 'rm -f ${GUEST_ENV_FILE}' 'umask 077' 'cat > ${GUEST_ENV_FILE}'; do
+            grep -Fq "$required" <<<"$line" \
+                || report "$rel:${line%%:*} transfers the environment without '$required', so the guest file is not a fresh 0600 one"
+        done
+    done <<<"$transfers"
+    [ "$(grep -c 'rm -f \${GUEST_ENV_FILE}.*exec bash tests/e2e/' "$harness")" -ge 2 ] \
         || report "$rel must remove the guest environment before both scripts execute"
 
     # The regression to catch is a secret value reaching argv, not one
-    # particular way of spelling it. The previous form matched the old
-    # accumulator by name (`prov_env`, `sudo_env`), so reintroducing the same
-    # bug under any other variable name passed silently.
+    # particular way of spelling it. Matching the accumulator by name
+    # (`prov_env`, `sudo_env`) let the same bug back in under a new name, and
+    # the structural replacement only inspected the token immediately after
+    # `sudo` -- so a call site that runs as the VM user, with no `sudo` at all,
+    # was never examined. That is where the bug actually came back.
     #
-    # Structural instead: no line that invokes cmd_ssh may interpolate a
-    # shell variable straight after `sudo`, which is the shape that puts a
-    # value on the guest command line. The stdin transfer and the `sudo bash
-    # -c` consumer both name a literal command after sudo, so neither matches.
-    if grep -nE 'cmd_ssh[^#]*sudo *[$"]?\$\{?[A-Za-z_]' "$harness" \
-        | grep -vE 'sudo (sh|bash) -c' >/dev/null; then
-        report "$rel interpolates a variable into an argv command after sudo"
+    # Allowlist instead: read every variable interpolated into a cmd_ssh
+    # command string and require each to be a declared non-secret. A new
+    # accumulator fails whatever it is called and wherever it sits in the line.
+    # An empty selection here would screen nothing and say nothing, which is
+    # the defect this whole file exists to catch. Require it to select.
+    cmd_ssh_lines="$(grep -n 'cmd_ssh' "$harness" || true)"
+    cmd_ssh_calls="$(grep -nE 'cmd_ssh "' "$harness" || true)"
+    if [ -z "$cmd_ssh_calls" ]; then
+        report "$rel: no cmd_ssh call site matched, so the argv checks below screened nothing"
+        continue
     fi
+    while IFS= read -r line; do
+        [ -n "$line" ] || continue
+        lineno="${line%%:*}"
+        while IFS= read -r var; do
+            [ -n "$var" ] || continue
+            for ok in "${allowed_vars[@]}"; do
+                [ "$var" = "$ok" ] && continue 2
+            done
+            report "$rel:$lineno interpolates \$$var into a cmd_ssh command string, which puts its value in argv; stage it with write_guest_env_file or add it to allowed_vars"
+        done < <(printf '%s\n' "$line" | grep -oE '\$\{?[A-Za-z_][A-Za-z0-9_]*' | tr -d '${' | sort -u)
+    done <<<"$cmd_ssh_calls"
 
     # And the specific values must never appear on a cmd_ssh line at all.
+    # `grep -n ... | grep -Fq` put a short-circuiting reader on the right of a
+    # pipe under `pipefail`: once the producer's output passes the pipe buffer
+    # it takes SIGPIPE, the pipeline reports 141, and a real hit reads as a
+    # miss. Measured at 0/300 hits on a 227 KB producer. A here-string has no
+    # pipe to break.
     for key in "${keys[@]}"; do
-        if grep -n 'cmd_ssh' "$harness" | grep -Fq "$key"; then
+        if grep -Fq "$key" <<<"$cmd_ssh_lines"; then
             report "$rel names $key on a cmd_ssh line, which puts it in argv"
         fi
     done
@@ -66,7 +114,7 @@ for rel in "${harnesses[@]}"; do
         continue
     fi
     for key in "${keys[@]}"; do
-        printf '%s' "$run_block" | grep -Fq "$key" \
+        grep -Fq "$key" <<<"$run_block" \
             || report "$rel does not forward $key from cmd_run"
     done
 done

Mutation proof

Baseline green on a fixed tree, then thirteen mutations, each applied alone in a git clone --no-hardlinks scratch copy and restored afterwards.

#	Mutation	Patched gate
N0	none, fixed tree	rc=0
N1	env_prefix accumulator restored (the live bug)	rc=1, names atomic-vm.sh:526
N2	same bug, variable renamed carried_over	rc=1
N3	accumulator buried mid-string inside sudo bash -c	rc=1
N4	third harness lands carrying the bug	rc=1, two failures
N5	discovery selects zero harnesses	rc=1, found 0 file(s) defining cmd_ssh
N6	key dropped from cmd_run	rc=1
N7	config.rs parked, key extraction reads nothing	rc=1, found only 0 keys
N8	umask 077 dropped from the VM-user transfer	rc=1
N9	cmd_run renamed, block extraction returns nothing	rc=1
N10	rm -f dropped from the root transfer	rc=1
N11	every stdin transfer removed	rc=1, has no stdin environment transfer at all
N12	every cmd_ssh call site respelled, so the argv selection is empty	rc=1, no cmd_ssh call site matched, so the argv checks below screened nothing

N5, N7, N9, N11 and N12 break the guard's own input rather than the code it guards. Two of them caught defects in my own drafts. N8 found a second transfer I had added and left uncovered by the single-literal assertion, which is how that assertion became a loop. N12 found the argv check reading an unguarded process substitution, screening nothing in silence if the selection came back empty, which is the same shape as the harness list it replaces.

The current gate, for comparison, passes N1, N2, N3, N4 and the emptied-list case.

Against the unmodified tree at 61b3a87:

$ bash tests/e2e/vm-env-secrets.test.sh
FAIL  tests/e2e/atomic-vm.sh:526 interpolates $env_prefix into a cmd_ssh command string, which puts its value in argv; stage it with write_guest_env_file or add it to allowed_vars

1 VM environment secret handling failure(s).
rc=1

bash -n clean, shellcheck --severity=warning clean on 0.10.0, and git apply --check applies to 61b3a87 without fuzz.

If you take this

Start by reproducing the argv line above: extract cmd_test_exec with awk '/^cmd_test_exec\(\)/,/^}/', stub cmd_ssh to print $1, and clear your own provider keys first so nothing real reaches your terminal. Once you can see the secret in the string, the rest of the change follows the shape cmd_run already uses.

Related: #299 (the original fix), #387 (the same coverage-versus-behaviour split in the secret scanner).

[vladimirrott]

@xianjianlf2, this one sits right on the work you already did here.

Your #316 kept provider keys out of VM process arguments. #428 is the same class of leak one step along: vm-env-secrets passes while atomic-vm.sh test-exec puts every variable it was handed into the guest environment, so the guard reports clean over the case it exists to catch.

It is labelled hard because the fix has to leave the guard able to fail rather than move the leak somewhere the guard does not look. Proving that is most of the work, and the test should go red against the current test-exec before it goes green against yours.

Thirteen days since your last one landed, so no assumption you are free. Yours if you want it.