diff --git a/.superpowers/sdd/task-3-report.md b/.superpowers/sdd/task-3-report.md index 571dbbd8..154f23ad 100644 --- a/.superpowers/sdd/task-3-report.md +++ b/.superpowers/sdd/task-3-report.md @@ -47,3 +47,11 @@ that neither secret values nor bundle/file metadata are inherited by the Pi chil The rotation helper retains its old/new scratch-file CLI contract; smoke tests keep those files outside Compose and mount only the bundle. + +## Whole-branch review fixes + +- `core-entrypoint.sh` validates `THT_SECRETS_FILE` fail-closed before optional lookups; malformed, + duplicate, unknown, oversized, or overlong bundles stop startup with sanitized diagnostics. +- Runtime password files are cleaned after child exit via signal forwarding and `wait`, rather + than being orphaned by `exec`. +- The shell loader accepts CRLF bundles (Windows/Notepad) consistently with the TypeScript loader. diff --git a/deploy/vector/secret-policy.sh b/deploy/vector/secret-policy.sh index 09f34823..50c22c34 100755 --- a/deploy/vector/secret-policy.sh +++ b/deploy/vector/secret-policy.sh @@ -23,12 +23,11 @@ read_secret_file() { cat "$1" } -# Read one value from the deployment bundle without putting the bundle itself in -# a service environment. The parser is deliberately strict: one KEY=VALUE per -# line, no duplicate keys, no unknown syntax, and no whitespace in credentials. -read_bundle_secret() { +# Validate the bundle without printing any value. Keep this parser aligned with +# the backend loader: comments/blank lines are allowed, while syntax, allowlist, +# duplicates, empty values, file size, and line size are fail-closed. +validate_bundle() { bundle_path=$1 - bundle_key=$2 if [ -L "$bundle_path" ] || [ ! -f "$bundle_path" ] || [ ! -r "$bundle_path" ] || [ ! -s "$bundle_path" ]; then echo "secret bundle must be a readable, non-empty regular file" >&2 return 2 @@ -38,20 +37,46 @@ read_bundle_secret() { /run/secrets/*:444|/run/secrets/*:400|/run/secrets/*:600|*:600|*:400) ;; *) echo "secret bundle must have mode 0600 or stricter (Docker secrets may be 0444)" >&2; return 2 ;; esac - case "$bundle_key" in - THT_[A-Z0-9_]*|PI_PROVIDER_API_KEY) ;; - *) echo "invalid secret bundle key" >&2; return 2 ;; - esac - value=$(awk -v wanted="$bundle_key" ' + size=$(stat -c '%s' "$bundle_path" 2>/dev/null || stat -f '%z' "$bundle_path" 2>/dev/null) || return 2 + if [ "$size" -gt 65536 ]; then + echo "secret bundle exceeds the 64KiB limit" >&2 + return 2 + fi + awk ' + { sub(/\r$/, "", $0) } + length($0) > 16384 { exit 9 } /^[[:space:]]*$/ || /^[[:space:]]*#/ { next } /^[A-Z][A-Z0-9_]*=/ { key=$0; sub(/=.*/, "", key) val=$0; sub(/^[^=]*=/, "", val) if (key !~ /^(THT_MODEL_API_KEY|THT_DWH_API_KEY|THT_VEC_API_KEY|THT_VEC_WRITE_API_KEY|THT_CA|THT_SSL_CA|THT_VECTOR_BOOTSTRAP_PASSWORD|THT_VECTOR_MIGRATOR_PASSWORD|THT_VECTOR_READER_PASSWORD|THT_VECTOR_WRITER_PASSWORD|PI_PROVIDER_API_KEY)$/) exit 6 - if (val == "") exit 7 - if (++seen[key] > 1) exit 8 + if (val == "" || ++seen[key] > 1) exit 7 + next + } + { exit 4 } + ' "$bundle_path" || { + echo "secret bundle syntax is invalid" >&2 + return 2 + } +} + +# Read one value from the deployment bundle without putting the bundle itself in +# a service environment. Values selected for credentials must contain no spaces. +read_bundle_secret() { + bundle_path=$1 + bundle_key=$2 + validate_bundle "$bundle_path" || return + case "$bundle_key" in + THT_[A-Z0-9_]*|PI_PROVIDER_API_KEY) ;; + *) echo "invalid secret bundle key" >&2; return 2 ;; + esac + value=$(awk -v wanted="$bundle_key" ' + { sub(/\r$/, "", $0) } + /^[[:space:]]*$/ || /^[[:space:]]*#/ { next } + /^[A-Z][A-Z0-9_]*=/ { + key=$0; sub(/=.*/, "", key) + val=$0; sub(/^[^=]*=/, "", val) if (key == wanted) { - if (found) exit 3 found=1; print val } next diff --git a/docker/core-entrypoint.sh b/docker/core-entrypoint.sh index 900b4f40..1039e542 100755 --- a/docker/core-entrypoint.sh +++ b/docker/core-entrypoint.sh @@ -36,8 +36,23 @@ cleanup_secret_tmp() { } trap cleanup_secret_tmp EXIT HUP INT TERM +run_child() { + "$@" & + child_pid=$! + forward_signal() { kill -TERM "$child_pid" 2>/dev/null || true; } + trap forward_signal HUP INT TERM + wait "$child_pid" + status=$? + trap - HUP INT TERM + return "$status" +} + bundle=${THT_SECRETS_FILE:-} -if [ -n "$bundle" ] && [ -r "$bundle" ]; then +if [ -n "$bundle" ]; then + if ! validate_bundle "$bundle" >/dev/null 2>&1; then + echo "THT_SECRETS_FILE points to an invalid secret bundle" >&2 + exit 2 + fi # REST adapters consume these values while the harness is running. They are # loaded here (before `tht` starts), not only in the backend/Pi process. load_bundle_env() { @@ -76,21 +91,21 @@ fi case "${1:-server}" in server) shift || true - exec node /app/backend/dist/server.js "$@" + run_child node /app/backend/dist/server.js "$@" ;; doctor) shift - exec tht doctor "$@" + run_child tht doctor "$@" ;; preprocess) shift - exec tht preprocess "$@" + run_child tht preprocess "$@" ;; tht) shift - exec tht "$@" + run_child tht "$@" ;; *) - exec tht "$@" + run_child tht "$@" ;; esac diff --git a/scripts/test-container-deployment.sh b/scripts/test-container-deployment.sh index 819b933b..9aa907cc 100755 --- a/scripts/test-container-deployment.sh +++ b/scripts/test-container-deployment.sh @@ -74,6 +74,22 @@ if grep -q 'must-not-leak' "$tmp/legacy-model.err"; then echo "legacy model credential leaked through entrypoint diagnostics" >&2 exit 1 fi +printf 'THT_VECTOR_READER_PASSWORD=one\nTHT_VECTOR_READER_PASSWORD=two\n' >"$tmp/invalid-bundle" +chmod 0600 "$tmp/invalid-bundle" +if THT_SECRETS_FILE="$tmp/invalid-bundle" ./docker/core-entrypoint.sh doctor \ + >"$tmp/invalid-bundle.out" 2>"$tmp/invalid-bundle.err"; then + echo "entrypoint accepted an invalid secret bundle" >&2 + exit 1 +fi +grep -q 'THT_SECRETS_FILE points to an invalid secret bundle' "$tmp/invalid-bundle.err" +if grep -q 'THT_VECTOR_READER_PASSWORD' "$tmp/invalid-bundle.err"; then + echo "invalid bundle diagnostics leaked key material" >&2 + exit 1 +fi +before_tmp=$(find "${TMPDIR:-/tmp}" -maxdepth 1 -type d -name 'thothii-secrets.*' -print | sort) +THT_SECRETS_FILE="$bundle" ./docker/core-entrypoint.sh doctor >/dev/null 2>&1 || true +after_tmp=$(find "${TMPDIR:-/tmp}" -maxdepth 1 -type d -name 'thothii-secrets.*' -print | sort) +test "$before_tmp" = "$after_tmp" if grep -Eq 'THT_VECTOR_(BOOTSTRAP|MIGRATOR|READER|WRITER)_PASSWORD_FILE|target: vector_(bootstrap|migrator|reader|writer)_password|dwh_api_key|model_api_key|THT_[A-Z0-9_]+_SECRET_FILE' "$tmp/production.yaml"; then echo "production external config contains local direct vector secrets" >&2 exit 1 diff --git a/scripts/test-vector-secret-policy.sh b/scripts/test-vector-secret-policy.sh index 8369ed68..fc43fc49 100755 --- a/scripts/test-vector-secret-policy.sh +++ b/scripts/test-vector-secret-policy.sh @@ -16,11 +16,14 @@ printf 'owner-readonly' >"$tmp/readonly" printf 'too-open' >"$tmp/open" printf '# comment\n\nTHT_VECTOR_READER_PASSWORD=reader\nTHT_VECTOR_WRITER_PASSWORD=writer\n' >"$tmp/bundle" printf 'THT_VECTOR_READER_PASSWORD=reader\nTHT_VECTOR_WRITER_PASSWORD=writer\nTHT_DWH_API_KEY=one\nTHT_DWH_API_KEY=two\n' >"$tmp/duplicate-bundle" +printf 'THT_VECTOR_READER_PASSWORD=reader\r\nTHT_VECTOR_WRITER_PASSWORD=writer\r\n' >"$tmp/crlf-bundle" +awk 'BEGIN { printf "THT_VECTOR_READER_PASSWORD="; for (i = 1; i <= 16385; i++) printf "x"; print "" }' >"$tmp/long-line-bundle" +awk 'BEGIN { for (i = 1; i <= 70000; i++) print "# filler" }' >"$tmp/large-bundle" chmod 0600 "$tmp/valid" chmod 0444 "$tmp/docker" chmod 0400 "$tmp/readonly" chmod 0640 "$tmp/open" -chmod 0600 "$tmp/bundle" "$tmp/duplicate-bundle" +chmod 0600 "$tmp/bundle" "$tmp/duplicate-bundle" "$tmp/crlf-bundle" "$tmp/long-line-bundle" "$tmp/large-bundle" for invalid in empty newline space; do if validate_secret_file "$tmp/$invalid" "$invalid" >/dev/null 2>&1; then @@ -40,9 +43,16 @@ if validate_secret_file "$tmp/open" open >/dev/null 2>&1; then fi test "$(read_secret_file "$tmp/valid" valid)" = "safe-quoted-'-dollar-$" test "$(read_bundle_secret "$tmp/bundle" THT_VECTOR_READER_PASSWORD)" = reader +test "$(read_bundle_secret "$tmp/crlf-bundle" THT_VECTOR_READER_PASSWORD)" = reader if read_bundle_secret "$tmp/duplicate-bundle" THT_VECTOR_READER_PASSWORD >/dev/null 2>&1; then echo "secret policy accepted a duplicate unrelated bundle key" >&2 exit 1 fi +for invalid_bundle in long-line-bundle large-bundle; do + if read_bundle_secret "$tmp/$invalid_bundle" THT_VECTOR_READER_PASSWORD >/dev/null 2>&1; then + echo "secret policy accepted oversized $invalid_bundle" >&2 + exit 1 + fi +done echo "shared vector secret policy contracts passed."