From f65e69651a70ad19a9b9a6f2d550f84c9695ebbe Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 13:49:55 -0700 Subject: [PATCH 1/3] fix(ci): retry transient base image pulls Signed-off-by: Apurv Kumaria --- .github/actions/base-image-resolver.sh | 117 +++++++++++++- test/base-image-resolver-helper.test.ts | 194 ++++++++++++++++++++++++ 2 files changed, 310 insertions(+), 1 deletion(-) diff --git a/.github/actions/base-image-resolver.sh b/.github/actions/base-image-resolver.sh index 9f0d8ad3e9..e4919cb474 100644 --- a/.github/actions/base-image-resolver.sh +++ b/.github/actions/base-image-resolver.sh @@ -17,8 +17,123 @@ resolver_glibc_ok() { && [[ "$(printf '%s\n%s\n' "$minimum" "$have" | sort -V | head -n 1)" == "$minimum" ]] } +resolver_sanitize_pull_diagnostic() { + local diagnostic="${1:-}" + printf '%s' "$diagnostic" \ + | LC_ALL=C tr -d '\000-\010\013\014\016-\037\177' \ + | tr '\n' ' ' \ + | sed -E \ + -e 's#([Hh][Tt][Tt][Pp][Ss]?://)[^/@[:space:]]+@#\1[redacted]@#g' \ + -e 's#([?&][^=[:space:]&]{1,80}=)[^&[:space:]]+#\1[redacted]#g' \ + -e 's#(([Pp][Rr][Oo][Xx][Yy]-)?[Aa][Uu][Tt][Hh][Oo][Rr][Ii][Zz][Aa][Tt][Ii][Oo][Nn]|[Xx]-[Aa][Pp][Ii]-[Kk][Ee][Yy]|[Xx]-[Aa][Uu][Tt][Hh]-[Tt][Oo][Kk][Ee][Nn]|[Cc][Oo][Oo][Kk][Ii][Ee]|[Ss][Ee][Tt]-[Cc][Oo][Oo][Kk][Ii][Ee])([[:space:]]*[:=][[:space:]]*([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn])?[[:space:]]*)[^[:space:],;]+#\1\3[redacted]#g' \ + -e 's#(([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn])[[:space:]]+)[^[:space:],;]+#\1[redacted]#g' \ + -e 's#(([Tt][Oo][Kk][Ee][Nn]|[Aa][Cc][Cc][Ee][Ss][Ss][_-][Tt][Oo][Kk][Ee][Nn]|[Rr][Ee][Ff][Rr][Ee][Ss][Hh][_-][Tt][Oo][Kk][Ee][Nn]|[Aa][Pp][Ii][_-]?[Kk][Ee][Yy]|[Cc][Ll][Ii][Ee][Nn][Tt][_-][Ss][Ee][Cc][Rr][Ee][Tt]|[Pp][Aa][Ss][Ss][Ww][Oo][Rr][Dd]|[Pp][Aa][Ss][Ss][Ww][Dd]|[Ss][Ee][Cc][Rr][Ee][Tt]|[Aa][Uu][Tt][Hh]|[Ss][Ii][Gg]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]|[Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Xx]-[Aa][Mm][Zz]-([Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Ss][Ee][Cc][Uu][Rr][Ii][Tt][Yy]-[Tt][Oo][Kk][Ee][Nn]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]))[[:space:]]*=[[:space:]]*)[^&[:space:],;]+#\1[redacted]#g' \ + -e 's#eyJ[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_.-]+#[redacted]#g' \ + -e 's#(gh[pousr]_|github_pat_)[A-Za-z0-9_]{16,}#[redacted]#g' \ + -e 's#(sk-|nvapi-|hf_)[A-Za-z0-9._-]{12,}#[redacted]#g' +} + +resolver_emit_pull_diagnostic() { + local diagnostic="$1" line line_count=0 + if [[ -z "$diagnostic" ]]; then + echo "docker pull: command failed without diagnostic output" >&2 + return 0 + fi + + while IFS= read -r line || [[ -n "$line" ]]; do + line_count=$((line_count + 1)) + if ((line_count > 20)); then + echo "docker pull: additional sanitized diagnostics omitted" >&2 + break + fi + printf 'docker pull: %.500s\n' "$line" >&2 + done <<<"$diagnostic" +} + +resolver_pull_diagnostic_is_transient() { + local diagnostic normalized + diagnostic="${1:-}" + normalized="$(printf '%s' "$diagnostic" | LC_ALL=C tr '[:upper:]' '[:lower:]')" + [[ -n "$normalized" ]] || return 1 + + # Deterministic failures take precedence even if a daemon appends a generic + # transport phrase to the same diagnostic. + if [[ "$normalized" =~ manifest[[:space:]]+(unknown|invalid) ]] \ + || [[ "$normalized" =~ no[[:space:]]+matching[[:space:]]+manifest ]] \ + || [[ "$normalized" =~ (manifest|repository|reference|name).*(not[[:space:]]+found|does[[:space:]]+not[[:space:]]+exist) ]] \ + || [[ "$normalized" =~ pull[[:space:]]+access[[:space:]]+denied ]] \ + || [[ "$normalized" =~ access[[:space:]]+denied ]] \ + || [[ "$normalized" =~ requested[[:space:]]+access.*denied ]] \ + || [[ "$normalized" =~ (^|[[:space:]])(denied:|forbidden([[:space:]:]|$)) ]] \ + || [[ "$normalized" =~ (unauthorized|authentication[[:space:]]+required|insufficient[_[:space:]-]+scope) ]] \ + || [[ "$normalized" =~ (http[^[:alnum:]]+[^[:space:]]*[[:space:]]+|status([[:space:]]+code)?[^0-9]{0,12})(401|403|404)([^0-9]|$) ]] \ + || [[ "$normalized" =~ (^|[^[:alnum:]])(401|403|404)[[:space:]]+(unauthorized|forbidden|not[[:space:]]+found) ]] \ + || [[ "$normalized" =~ invalid[[:space:]]+(reference|repository|tag) ]] \ + || [[ "$normalized" =~ (digest|checksum|integrity).*(invalid|mismatch|verification|does[[:space:]]+not[[:space:]]+match|failed) ]] \ + || [[ "$normalized" =~ (does[[:space:]]+not[[:space:]]+match|mismatch|unexpected).*(digest|checksum) ]] \ + || [[ "$normalized" =~ (failed|unable).*(verify|validate).*(digest|checksum|integrity) ]] \ + || [[ "$normalized" =~ (layer|content).*(verification[[:space:]]+failed|size[[:space:]]+validation[[:space:]]+failed) ]] \ + || [[ "$normalized" =~ (unsupported|incompatible)[[:space:]]+platform ]] \ + || [[ "$normalized" =~ no[[:space:]]+match[[:space:]]+for[[:space:]]+platform ]] \ + || [[ "$normalized" =~ does[[:space:]]+not[[:space:]]+match[[:space:]]+the[[:space:]]+specified[[:space:]]+platform ]]; then + return 1 + fi + + if [[ "$normalized" =~ (http[^[:alnum:]]+[^[:space:]]*[[:space:]]+|status([[:space:]]+code)?[^0-9]{0,12})(429|5[0-9][0-9])([^0-9]|$) ]] \ + || [[ "$normalized" =~ (too[[:space:]]+many[[:space:]]+requests|toomanyrequests|rate[[:space:]_-]*limit) ]] \ + || [[ "$normalized" =~ (bad[[:space:]]+gateway|service[[:space:]]+unavailable|gateway[[:space:]]+timeout|internal[[:space:]]+server[[:space:]]+error) ]] \ + || [[ "$normalized" =~ (tls[[:space:]]+handshake|i/o|connection)[[:space:]]+timeout ]] \ + || [[ "$normalized" =~ (client[.]timeout[[:space:]]+exceeded|connection[[:space:]]+timed[[:space:]]+out) ]] \ + || [[ "$normalized" =~ request[[:space:]]+(canceled|cancelled).*waiting[[:space:]]+for[[:space:]]+connection ]] \ + || [[ "$normalized" =~ (context[[:space:]]+)?deadline[[:space:]]+exceeded ]] \ + || [[ "$normalized" =~ connection[[:space:]]+(reset|refused|aborted|closed) ]] \ + || [[ "$normalized" =~ (network[[:space:]]+is[[:space:]]+unreachable|no[[:space:]]+route[[:space:]]+to[[:space:]]+host) ]] \ + || [[ "$normalized" =~ temporary[[:space:]]+failure[[:space:]]+in[[:space:]]+name[[:space:]]+resolution ]] \ + || [[ "$normalized" =~ (dial|proxyconnect)[[:space:]]+(tcp|udp) ]] \ + || [[ "$normalized" =~ lookup.*(no[[:space:]]+such[[:space:]]+host|server[[:space:]]+misbehaving) ]] \ + || [[ "$normalized" =~ (eai_again|etimedout|econnreset|econnrefused) ]] \ + || [[ "$normalized" =~ ((^|[[:space:]:])eof([[:space:]]|$)|unexpected[[:space:]]+eof|broken[[:space:]]+pipe|transport[[:space:]]+is[[:space:]]+closing) ]] \ + || [[ "$normalized" =~ failed[[:space:]]+to[[:space:]]+do[[:space:]]+request ]] \ + || [[ "$normalized" =~ temporar(il)?y[[:space:]]+unavailable ]]; then + return 0 + fi + return 1 +} + resolver_pull() { - docker pull "$1" >/dev/null 2>&1 + local ref="$1" raw_diagnostic diagnostic status attempt delay + + for attempt in 1 2 3; do + if raw_diagnostic="$(docker pull "$ref" 2>&1 >/dev/null)"; then + return 0 + else + status=$? + fi + + if ! diagnostic="$(resolver_sanitize_pull_diagnostic "$raw_diagnostic")"; then + echo "::error::Docker pull diagnostics could not be sanitized; refusing a local base-image fallback" >&2 + exit 75 + fi + resolver_emit_pull_diagnostic "$diagnostic" + + if ! resolver_pull_diagnostic_is_transient "$diagnostic"; then + return "$status" + fi + if ((attempt == 3)); then + echo "::error::Base-image pull failed with a transient registry or transport error after 3 attempts; refusing a local build fallback" >&2 + # The resolver actions intentionally treat ordinary pull failures as a + # missing candidate. Terminating the sourced action with EX_TEMPFAIL is + # therefore the only way to preserve this distinct failure at present. + exit 75 + fi + + delay="$attempt" + echo "::warning::Transient base-image pull failure; retrying attempt $((attempt + 1))/3 after ${delay}s" >&2 + if ! sleep "$delay"; then + echo "::error::Could not wait before retrying the transient base-image pull" >&2 + exit 75 + fi + done } resolver_repo_digest() { diff --git a/test/base-image-resolver-helper.test.ts b/test/base-image-resolver-helper.test.ts index ad702dc50d..2c11f47227 100644 --- a/test/base-image-resolver-helper.test.ts +++ b/test/base-image-resolver-helper.test.ts @@ -36,6 +36,13 @@ function fakeDocker(body: string) { return dir; } +function fakeSleep(dir: string) { + const executable = path.join(dir, "sleep"); + writeFileSync(executable, '#!/usr/bin/env bash\nset -eu\nprintf "%s\\n" "$1" >> "$SLEEP_LOG"\n', { + mode: 0o755, + }); +} + afterEach(() => { for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); @@ -237,3 +244,190 @@ resolver_try_candidates reject first second`); ); }); }); + +describe("base image pull recovery (#7140)", () => { + it("retries a transient transport failure and succeeds on the third attempt", () => { + const bin = fakeDocker(` +count="$(cat "$PULL_COUNT" 2>/dev/null || printf 0)" +count=$((count + 1)) +printf "%s\\n" "$count" > "$PULL_COUNT" +if ((count < 3)); then + printf "%s\\n" "failed to do request: net/http: TLS handshake timeout" >&2 + exit 1 +fi`); + const pullCount = path.join(bin, "pull-count"); + const sleepLog = path.join(bin, "sleep.log"); + + const result = run( + ` +sleep() { printf '%s\\n' "$1" >> "$SLEEP_LOG"; } +resolver_pull example:test`, + { + PATH: `${bin}:${process.env.PATH}`, + PULL_COUNT: pullCount, + SLEEP_LOG: sleepLog, + }, + ); + + expect(result.status, result.stderr).toBe(0); + expect(readFileSync(pullCount, "utf8")).toBe("3\n"); + expect(readFileSync(sleepLog, "utf8")).toBe("1\n2\n"); + expect(result.stderr).toContain("TLS handshake timeout"); + }); + + it("fails with a distinct status after exhausting transient registry retries", () => { + const secret = "registry-bearer-secret"; + const bin = fakeDocker(` +count="$(cat "$PULL_COUNT" 2>/dev/null || printf 0)" +count=$((count + 1)) +printf "%s\\n" "$count" > "$PULL_COUNT" +printf "%s\\n" "unexpected HTTP status: 503 Service Unavailable Authorization: Bearer $PULL_SECRET" >&2 +exit 1`); + const pullCount = path.join(bin, "pull-count"); + const sleepLog = path.join(bin, "sleep.log"); + + const result = run( + ` +sleep() { printf '%s\\n' "$1" >> "$SLEEP_LOG"; } +resolver_pull example:test +printf 'unreachable'`, + { + PATH: `${bin}:${process.env.PATH}`, + PULL_COUNT: pullCount, + PULL_SECRET: secret, + SLEEP_LOG: sleepLog, + }, + ); + + expect(result.status).toBe(75); + expect(result.stdout).not.toContain("unreachable"); + expect(readFileSync(pullCount, "utf8")).toBe("3\n"); + expect(readFileSync(sleepLog, "utf8")).toBe("1\n2\n"); + expect(result.stderr).toContain("503 Service Unavailable"); + expect(result.stderr).toContain("[redacted]"); + expect(result.stderr).not.toContain(secret); + }); + + it.each([ + ["missing manifest", "manifest unknown"], + ["missing manifest with transport text", "manifest unknown; service unavailable"], + ["missing repository", "repository does not exist or may require 'docker login'"], + ["authorization rejection", "unauthorized: authentication required"], + ["HTTP authorization rejection", "unexpected HTTP status: 403 Service Unavailable"], + ["digest mismatch", "downloaded layer does not match the expected digest"], + ["invalid reference", "invalid reference format"], + ["platform incompatibility", "no matching manifest for linux/arm64 in the manifest list"], + ["unclassified daemon rejection", "daemon policy rejected this pull"], + ])("does not retry a deterministic %s failure", (_name, diagnostic) => { + const bin = fakeDocker(` +count="$(cat "$PULL_COUNT" 2>/dev/null || printf 0)" +count=$((count + 1)) +printf "%s\\n" "$count" > "$PULL_COUNT" +printf "%s\\n" "$PULL_DIAGNOSTIC" >&2 +exit 1`); + const pullCount = path.join(bin, "pull-count"); + const sleepLog = path.join(bin, "sleep.log"); + + const result = run( + ` +sleep() { printf '%s\\n' "$1" >> "$SLEEP_LOG"; } +resolver_pull example:test`, + { + PATH: `${bin}:${process.env.PATH}`, + PULL_COUNT: pullCount, + PULL_DIAGNOSTIC: diagnostic, + SLEEP_LOG: sleepLog, + }, + ); + + expect(result.status).toBe(1); + expect(readFileSync(pullCount, "utf8")).toBe("1\n"); + expect(result.stderr).toContain(diagnostic); + expect(() => readFileSync(sleepLog, "utf8")).toThrow(); + }); + + it.each([ + ["transport timeout", "net/http: TLS handshake timeout"], + ["client timeout", "request canceled while waiting for connection (Client.Timeout exceeded)"], + ["network outage", "dial tcp: network is unreachable"], + ["HTTP server failure", "unexpected HTTP status: 502 Bad Gateway"], + ["HTTP throttling", "unexpected HTTP status: 429 Too Many Requests"], + ["registry rate limit", "toomanyrequests: rate limit exceeded"], + ])("positively classifies a transient %s diagnostic", (_name, diagnostic) => { + const result = run('resolver_pull_diagnostic_is_transient "$PULL_DIAGNOSTIC"', { + PULL_DIAGNOSTIC: diagnostic, + }); + + expect(result.status, result.stderr).toBe(0); + }); + + it("surfaces sanitized terminal diagnostics instead of suppressing stderr", () => { + const basicSecret = "registry-password"; + const foldedSecret = "folded-registry-secret"; + const querySecret = "registry-query-token"; + const bin = fakeDocker(` +printf "%s\\n" \ + "pull access denied at https://registry-user:$BASIC_SECRET@example.test/v2/image?token=$QUERY_SECRET" \ + "Authorization:" \ + " Bearer $FOLDED_SECRET" >&2 +exit 1`); + + const result = run("resolver_pull example:test", { + BASIC_SECRET: basicSecret, + FOLDED_SECRET: foldedSecret, + PATH: `${bin}:${process.env.PATH}`, + QUERY_SECRET: querySecret, + }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("pull access denied"); + expect(result.stderr).toContain("[redacted]"); + expect(result.stderr).not.toContain(basicSecret); + expect(result.stderr).not.toContain(foldedSecret); + expect(result.stderr).not.toContain(querySecret); + }); + + it("prevents an exhausted transient pull from triggering a local Hermes build", () => { + const bin = fakeDocker(` +printf "%s\\0" "$@" >> "$DOCKER_LOG" +printf "\\0" >> "$DOCKER_LOG" +if [[ "$1" == pull ]]; then + printf "%s\\n" "unexpected status code 503: Service Unavailable" >&2 + exit 1 +fi +if [[ "$1" == build ]]; then exit 42; fi +exit 1`); + fakeSleep(bin); + const dockerLog = path.join(bin, "docker.log"); + const githubEnv = path.join(bin, "github.env"); + const sleepLog = path.join(bin, "sleep.log"); + writeFileSync(githubEnv, ""); + const resolver = hermesAction.runs.steps.find( + (step) => step.name === "Resolve Hermes sandbox base image", + )?.run; + + const result = spawnSync("bash", ["--noprofile", "--norc", "-c", resolver ?? ""], { + cwd: repoRoot, + encoding: "utf8", + timeout: execTimeout(), + env: { + ...process.env, + DOCKER_LOG: dockerLog, + GITHUB_ACTION_PATH: path.join(repoRoot, ".github/actions/resolve-hermes-base-image"), + GITHUB_ENV: githubEnv, + PATH: `${bin}:${process.env.PATH}`, + SLEEP_LOG: sleepLog, + }, + }); + + expect(result.status).toBe(75); + expect(readFileSync(githubEnv, "utf8")).toBe(""); + expect(readFileSync(sleepLog, "utf8")).toBe("1\n2\n"); + const calls = readFileSync(dockerLog, "utf8") + .split("\0\0") + .filter(Boolean) + .map((call) => call.split("\0").filter(Boolean)); + expect(calls.filter((args) => args[0] === "pull")).toHaveLength(3); + expect(calls.some((args) => args[0] === "build")).toBe(false); + }); +}); From edbfac2547b927e3d40de51b0abdb5a3b7844846 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 14:39:46 -0700 Subject: [PATCH 2/3] fix(ci): harden pull retry diagnostics Signed-off-by: Apurv Kumaria --- .github/actions/base-image-resolver.sh | 119 ++++++++++++++++----- test/base-image-resolver-helper.test.ts | 134 ++++++++++++++++++++++-- 2 files changed, 217 insertions(+), 36 deletions(-) diff --git a/.github/actions/base-image-resolver.sh b/.github/actions/base-image-resolver.sh index e4919cb474..0cc7c3a494 100644 --- a/.github/actions/base-image-resolver.sh +++ b/.github/actions/base-image-resolver.sh @@ -17,37 +17,53 @@ resolver_glibc_ok() { && [[ "$(printf '%s\n%s\n' "$minimum" "$have" | sort -V | head -n 1)" == "$minimum" ]] } +RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT=65536 + resolver_sanitize_pull_diagnostic() { - local diagnostic="${1:-}" - printf '%s' "$diagnostic" \ - | LC_ALL=C tr -d '\000-\010\013\014\016-\037\177' \ - | tr '\n' ' ' \ + # Docker stderr is byte-bounded before this stream reaches the shell. Treat + # CR and LF as record boundaries while redacting complete sensitive headers, + # then flatten the records so untrusted text cannot create a GitHub command. + LC_ALL=C tr '\015' '\012' \ + | LC_ALL=C tr -d '\000-\011\013\014\016-\037\177-\237' \ + | LC_ALL=C awk ' + BEGIN { sensitive_continuation = 0 } + { + lower = tolower($0) + if (match(lower, /(^|[[:space:]])(proxy-authorization|authorization|cookie|set-cookie|x-registry-auth|x-registry-config|x-api-key|x-auth-token)[[:space:]]*[:=]/)) { + print substr($0, 1, RSTART + RLENGTH - 1) "[redacted]" + sensitive_continuation = 1 + next + } + if (sensitive_continuation && $0 ~ /^[[:space:]]+/) { + print "[redacted]" + next + } + sensitive_continuation = 0 + print + } + ' \ | sed -E \ -e 's#([Hh][Tt][Tt][Pp][Ss]?://)[^/@[:space:]]+@#\1[redacted]@#g' \ -e 's#([?&][^=[:space:]&]{1,80}=)[^&[:space:]]+#\1[redacted]#g' \ - -e 's#(([Pp][Rr][Oo][Xx][Yy]-)?[Aa][Uu][Tt][Hh][Oo][Rr][Ii][Zz][Aa][Tt][Ii][Oo][Nn]|[Xx]-[Aa][Pp][Ii]-[Kk][Ee][Yy]|[Xx]-[Aa][Uu][Tt][Hh]-[Tt][Oo][Kk][Ee][Nn]|[Cc][Oo][Oo][Kk][Ii][Ee]|[Ss][Ee][Tt]-[Cc][Oo][Oo][Kk][Ii][Ee])([[:space:]]*[:=][[:space:]]*([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn])?[[:space:]]*)[^[:space:],;]+#\1\3[redacted]#g' \ - -e 's#(([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn])[[:space:]]+)[^[:space:],;]+#\1[redacted]#g' \ + -e 's#(([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn]|[Nn][Ee][Gg][Oo][Tt][Ii][Aa][Tt][Ee])[[:space:]]+)[^[:space:],;]+#\1[redacted]#g' \ -e 's#(([Tt][Oo][Kk][Ee][Nn]|[Aa][Cc][Cc][Ee][Ss][Ss][_-][Tt][Oo][Kk][Ee][Nn]|[Rr][Ee][Ff][Rr][Ee][Ss][Hh][_-][Tt][Oo][Kk][Ee][Nn]|[Aa][Pp][Ii][_-]?[Kk][Ee][Yy]|[Cc][Ll][Ii][Ee][Nn][Tt][_-][Ss][Ee][Cc][Rr][Ee][Tt]|[Pp][Aa][Ss][Ss][Ww][Oo][Rr][Dd]|[Pp][Aa][Ss][Ss][Ww][Dd]|[Ss][Ee][Cc][Rr][Ee][Tt]|[Aa][Uu][Tt][Hh]|[Ss][Ii][Gg]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]|[Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Xx]-[Aa][Mm][Zz]-([Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Ss][Ee][Cc][Uu][Rr][Ii][Tt][Yy]-[Tt][Oo][Kk][Ee][Nn]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]))[[:space:]]*=[[:space:]]*)[^&[:space:],;]+#\1[redacted]#g' \ -e 's#eyJ[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_.-]+#[redacted]#g' \ -e 's#(gh[pousr]_|github_pat_)[A-Za-z0-9_]{16,}#[redacted]#g' \ - -e 's#(sk-|nvapi-|hf_)[A-Za-z0-9._-]{12,}#[redacted]#g' + -e 's#(sk-|nvapi-|hf_)[A-Za-z0-9._-]{12,}#[redacted]#g' \ + | tr '\012' ' ' } resolver_emit_pull_diagnostic() { - local diagnostic="$1" line line_count=0 + local diagnostic="$1" truncated="$2" if [[ -z "$diagnostic" ]]; then echo "docker pull: command failed without diagnostic output" >&2 - return 0 + else + printf 'docker pull: %.500s\n' "$diagnostic" >&2 fi - while IFS= read -r line || [[ -n "$line" ]]; do - line_count=$((line_count + 1)) - if ((line_count > 20)); then - echo "docker pull: additional sanitized diagnostics omitted" >&2 - break - fi - printf 'docker pull: %.500s\n' "$line" >&2 - done <<<"$diagnostic" + if [[ "$truncated" == 1 ]]; then + echo "docker pull: diagnostic truncated at ${RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT} bytes" >&2 + fi } resolver_pull_diagnostic_is_transient() { @@ -75,7 +91,12 @@ resolver_pull_diagnostic_is_transient() { || [[ "$normalized" =~ (layer|content).*(verification[[:space:]]+failed|size[[:space:]]+validation[[:space:]]+failed) ]] \ || [[ "$normalized" =~ (unsupported|incompatible)[[:space:]]+platform ]] \ || [[ "$normalized" =~ no[[:space:]]+match[[:space:]]+for[[:space:]]+platform ]] \ - || [[ "$normalized" =~ does[[:space:]]+not[[:space:]]+match[[:space:]]+the[[:space:]]+specified[[:space:]]+platform ]]; then + || [[ "$normalized" =~ does[[:space:]]+not[[:space:]]+match[[:space:]]+the[[:space:]]+specified[[:space:]]+platform ]] \ + || [[ "$normalized" =~ x509: ]] \ + || [[ "$normalized" =~ (certificate|cert).*(unknown[[:space:]]+authority|verif|expired|not[[:space:]]+yet[[:space:]]+valid|hostname|not[[:space:]]+valid|untrusted|self[[:space:]-]*signed) ]] \ + || [[ "$normalized" =~ tls:.*bad[[:space:]]+certificate ]] \ + || [[ "$normalized" =~ tls:.*failed[[:space:]]+to[[:space:]]+verify[[:space:]]+certificate ]] \ + || [[ "$normalized" =~ (http[^0-9]{0,20}|status([[:space:]]+code)?[^0-9]{0,12})4([01][0-9]|2[0-8]|[3-9][0-9])([^0-9]|$) ]]; then return 1 fi @@ -89,32 +110,74 @@ resolver_pull_diagnostic_is_transient() { || [[ "$normalized" =~ connection[[:space:]]+(reset|refused|aborted|closed) ]] \ || [[ "$normalized" =~ (network[[:space:]]+is[[:space:]]+unreachable|no[[:space:]]+route[[:space:]]+to[[:space:]]+host) ]] \ || [[ "$normalized" =~ temporary[[:space:]]+failure[[:space:]]+in[[:space:]]+name[[:space:]]+resolution ]] \ - || [[ "$normalized" =~ (dial|proxyconnect)[[:space:]]+(tcp|udp) ]] \ || [[ "$normalized" =~ lookup.*(no[[:space:]]+such[[:space:]]+host|server[[:space:]]+misbehaving) ]] \ || [[ "$normalized" =~ (eai_again|etimedout|econnreset|econnrefused) ]] \ || [[ "$normalized" =~ ((^|[[:space:]:])eof([[:space:]]|$)|unexpected[[:space:]]+eof|broken[[:space:]]+pipe|transport[[:space:]]+is[[:space:]]+closing) ]] \ - || [[ "$normalized" =~ failed[[:space:]]+to[[:space:]]+do[[:space:]]+request ]] \ || [[ "$normalized" =~ temporar(il)?y[[:space:]]+unavailable ]]; then return 0 fi return 1 } +resolver_capture_pull() ( + local ref="$1" diagnostic_file="" byte_count status truncated=0 + local temp_root="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" + + # mktemp creates the capture with mode 0600 under this restrictive umask. + # The 64 KiB prefix is sanitized before it enters the parent shell. + umask 077 + diagnostic_file="$(mktemp "${temp_root%/}/nemoclaw-docker-pull.XXXXXX")" || exit 74 + trap 'rm -f -- "$diagnostic_file"' EXIT + trap 'exit 129' HUP + trap 'exit 130' INT + trap 'exit 143' TERM + + if docker pull "$ref" >/dev/null 2>"$diagnostic_file"; then + status=0 + else + status=$? + fi + + byte_count="$(wc -c <"$diagnostic_file")" || exit 74 + if ((byte_count > RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT)); then + truncated=1 + fi + + printf '%s\n%s\n' "$status" "$truncated" + LC_ALL=C head -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" "$diagnostic_file" \ + | resolver_sanitize_pull_diagnostic +) + resolver_pull() { - local ref="$1" raw_diagnostic diagnostic status attempt delay + local ref="$1" capture payload diagnostic status truncated attempt delay for attempt in 1 2 3; do - if raw_diagnostic="$(docker pull "$ref" 2>&1 >/dev/null)"; then - return 0 - else - status=$? + if ! capture="$(resolver_capture_pull "$ref")"; then + echo "::error::Docker pull diagnostics could not be captured securely; refusing a local base-image fallback" >&2 + exit 75 fi - if ! diagnostic="$(resolver_sanitize_pull_diagnostic "$raw_diagnostic")"; then - echo "::error::Docker pull diagnostics could not be sanitized; refusing a local base-image fallback" >&2 + if [[ "$capture" != *$'\n'* ]]; then + echo "::error::Docker pull diagnostics returned an invalid status; refusing a local base-image fallback" >&2 exit 75 fi - resolver_emit_pull_diagnostic "$diagnostic" + status="${capture%%$'\n'*}" + payload="${capture#*$'\n'}" + truncated="${payload%%$'\n'*}" + if [[ "$payload" == *$'\n'* ]]; then + diagnostic="${payload#*$'\n'}" + else + diagnostic="" + fi + if [[ ! "$status" =~ ^[0-9]+$ ]] || ((status > 255)) || [[ ! "$truncated" =~ ^[01]$ ]]; then + echo "::error::Docker pull diagnostics returned invalid metadata; refusing a local base-image fallback" >&2 + exit 75 + fi + if ((status == 0)); then + return 0 + fi + + resolver_emit_pull_diagnostic "$diagnostic" "$truncated" if ! resolver_pull_diagnostic_is_transient "$diagnostic"; then return "$status" diff --git a/test/base-image-resolver-helper.test.ts b/test/base-image-resolver-helper.test.ts index 2c11f47227..84dcba0f84 100644 --- a/test/base-image-resolver-helper.test.ts +++ b/test/base-image-resolver-helper.test.ts @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { spawnSync } from "node:child_process"; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -317,6 +317,22 @@ printf 'unreachable'`, ["digest mismatch", "downloaded layer does not match the expected digest"], ["invalid reference", "invalid reference format"], ["platform incompatibility", "no matching manifest for linux/arm64 in the manifest list"], + ["HTTP bad request", "failed to do request: unexpected HTTP status: 400 Bad Request"], + ["HTTP request timeout", "failed to do request: unexpected HTTP status: 408 Request Timeout"], + ["HTTP client rejection", "failed to do request: unexpected HTTP status: 418 I'm a teapot"], + [ + "HTTP client closed request", + "failed to do request: unexpected HTTP status: 499 Client Closed", + ], + [ + "certificate verification rejection", + "failed to do request: x509: certificate signed by unknown authority; connection reset", + ], + [ + "TLS certificate rejection", + "failed to do request: remote error: tls: bad certificate; service unavailable", + ], + ["generic request rejection", "failed to do request"], ["unclassified daemon rejection", "daemon policy rejected this pull"], ])("does not retry a deterministic %s failure", (_name, diagnostic) => { const bin = fakeDocker(` @@ -361,38 +377,137 @@ resolver_pull example:test`, expect(result.status, result.stderr).toBe(0); }); - it("surfaces sanitized terminal diagnostics instead of suppressing stderr", () => { + it("redacts complete credential headers and prevents log-command injection", () => { const basicSecret = "registry-password"; + const cookieSecret = "session-cookie-secret"; + const cookieSecondSecret = "second-cookie-secret"; const foldedSecret = "folded-registry-secret"; + const negotiateSecret = "negotiate-registry-secret"; + const proxySecret = "proxy-registry-secret"; const querySecret = "registry-query-token"; + const registryAuthSecret = "registry-auth-secret"; + const registryConfigSecret = "registry-config-secret"; + const setCookieSecret = "set-cookie-secret"; const bin = fakeDocker(` -printf "%s\\n" \ +printf "%s\\r\\n" \ "pull access denied at https://registry-user:$BASIC_SECRET@example.test/v2/image?token=$QUERY_SECRET" \ + "Authorization: Negotiate $NEGOTIATE_SECRET" \ + "Proxy-Authorization: CustomScheme $PROXY_SECRET" \ + "Cookie: session=$COOKIE_SECRET; second=$COOKIE_SECOND_SECRET" \ + "Set-Cookie: session=$SET_COOKIE_SECRET; Secure; HttpOnly, second=also-secret" \ + "X-Registry-Auth: $REGISTRY_AUTH_SECRET" \ + "X-Registry-Config: {\\"auth\\":\\"$REGISTRY_CONFIG_SECRET\\"}" \ "Authorization:" \ - " Bearer $FOLDED_SECRET" >&2 + " Bearer $FOLDED_SECRET" \ + $'\\033[31m\\r::warning::forged-pull-command' >&2 exit 1`); const result = run("resolver_pull example:test", { BASIC_SECRET: basicSecret, + COOKIE_SECRET: cookieSecret, + COOKIE_SECOND_SECRET: cookieSecondSecret, FOLDED_SECRET: foldedSecret, + NEGOTIATE_SECRET: negotiateSecret, PATH: `${bin}:${process.env.PATH}`, + PROXY_SECRET: proxySecret, QUERY_SECRET: querySecret, + REGISTRY_AUTH_SECRET: registryAuthSecret, + REGISTRY_CONFIG_SECRET: registryConfigSecret, + SET_COOKIE_SECRET: setCookieSecret, }); expect(result.status).toBe(1); expect(result.stderr).toContain("pull access denied"); expect(result.stderr).toContain("[redacted]"); - expect(result.stderr).not.toContain(basicSecret); - expect(result.stderr).not.toContain(foldedSecret); - expect(result.stderr).not.toContain(querySecret); + for (const secret of [ + basicSecret, + cookieSecret, + cookieSecondSecret, + foldedSecret, + negotiateSecret, + proxySecret, + querySecret, + registryAuthSecret, + registryConfigSecret, + setCookieSecret, + ]) { + expect(result.stderr).not.toContain(secret); + } + expect(result.stderr).not.toContain("\u001b"); + expect(result.stderr).not.toContain("\n::warning::forged-pull-command"); }); - it("prevents an exhausted transient pull from triggering a local Hermes build", () => { + it.each([ + [ + "multiple Cookie values", + "Cookie: first=cookie-one; second=cookie-two; preferences=dark mode", + ["cookie-one", "cookie-two", "dark mode"], + ], + [ + "multiple Set-Cookie values", + "Set-Cookie: first=set-cookie-one; Secure, second=set-cookie-two; HttpOnly", + ["set-cookie-one", "set-cookie-two"], + ], + [ + "Docker registry authorization", + "X-Registry-Auth: registry-auth-value", + ["registry-auth-value"], + ], + [ + "Docker registry configuration", + 'X-Registry-Config: {"auths":{"registry.example":{"auth":"registry-config-value"}}}', + ["registry-config-value"], + ], + [ + "arbitrary authorization scheme", + "Authorization: Negotiate negotiate-value", + ["negotiate-value"], + ], + [ + "arbitrary proxy authorization scheme", + "Proxy-Authorization: CustomScheme proxy-value", + ["proxy-value"], + ], + [ + "folded authorization value", + "Authorization:\r\n\tNegotiate folded-negotiate-value", + ["folded-negotiate-value"], + ], + ])("redacts a complete %s", (_name, raw, secrets) => { + const result = run('printf "%s" "$RAW_DIAGNOSTIC" | resolver_sanitize_pull_diagnostic', { + RAW_DIAGNOSTIC: raw, + }); + + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("[redacted]"); + for (const secret of secrets) expect(result.stdout).not.toContain(secret); + }); + + it("bounds captured stderr and preserves a deterministic Docker status", () => { + const bin = fakeDocker(` +printf "%s\\n" "daemon policy rejected this pull" >&2 +head -c 100000 /dev/zero | tr '\\0' x >&2 +exit 42`); + + const result = run("resolver_pull example:test", { + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status).toBe(42); + expect(result.stderr).toContain("daemon policy rejected this pull"); + expect(result.stderr).toContain("diagnostic truncated at 65536 bytes"); + expect(result.stderr.length).toBeLessThan(1_000); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + + it("bounds exhausted transient output and prevents a local Hermes build", () => { const bin = fakeDocker(` printf "%s\\0" "$@" >> "$DOCKER_LOG" printf "\\0" >> "$DOCKER_LOG" if [[ "$1" == pull ]]; then printf "%s\\n" "unexpected status code 503: Service Unavailable" >&2 + head -c 100000 /dev/zero | tr '\\0' x >&2 exit 1 fi if [[ "$1" == build ]]; then exit 42; fi @@ -416,6 +531,7 @@ exit 1`); GITHUB_ACTION_PATH: path.join(repoRoot, ".github/actions/resolve-hermes-base-image"), GITHUB_ENV: githubEnv, PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, SLEEP_LOG: sleepLog, }, }); @@ -423,11 +539,13 @@ exit 1`); expect(result.status).toBe(75); expect(readFileSync(githubEnv, "utf8")).toBe(""); expect(readFileSync(sleepLog, "utf8")).toBe("1\n2\n"); + expect(result.stderr.match(/diagnostic truncated at 65536 bytes/g)).toHaveLength(3); const calls = readFileSync(dockerLog, "utf8") .split("\0\0") .filter(Boolean) .map((call) => call.split("\0").filter(Boolean)); expect(calls.filter((args) => args[0] === "pull")).toHaveLength(3); expect(calls.some((args) => args[0] === "build")).toBe(false); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); }); }); From 4edb0c05d21724fe2cd1274b6ba3ee877a2a79e3 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 17:22:27 -0700 Subject: [PATCH 3/3] fix(ci): bound pull diagnostic capture Signed-off-by: Apurv Kumaria --- .github/actions/base-image-resolver.sh | 187 ++++++++++++++++-- test/base-image-resolver-helper.test.ts | 249 +++++++++++++++++++++++- 2 files changed, 406 insertions(+), 30 deletions(-) diff --git a/.github/actions/base-image-resolver.sh b/.github/actions/base-image-resolver.sh index 0cc7c3a494..fe336cde4f 100644 --- a/.github/actions/base-image-resolver.sh +++ b/.github/actions/base-image-resolver.sh @@ -19,17 +19,39 @@ resolver_glibc_ok() { RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT=65536 -resolver_sanitize_pull_diagnostic() { +resolver_canonicalize_pull_diagnostic_records() { # Docker stderr is byte-bounded before this stream reaches the shell. Treat - # CR and LF as record boundaries while redacting complete sensitive headers, - # then flatten the records so untrusted text cannot create a GitHub command. - LC_ALL=C tr '\015' '\012' \ - | LC_ALL=C tr -d '\000-\011\013\014\016-\037\177-\237' \ + # CRLF, lone CR, and LF as record boundaries, remove terminal decoration, + # and remove other controls before any record-boundary security decisions. + LC_ALL=C sed -E \ + -e $'s/\033\\][^\007\033]*(\007|\033\\\\)//g' \ + -e $'s/\033\\[[0-?]*[ -\\/]*[@-~]//g' \ + | LC_ALL=C tr -d '\000-\010\013\014\016-\037\177-\377' \ | LC_ALL=C awk ' + BEGIN { cr = sprintf("%c", 13) } + { + if (length($0) == 0) { + print + next + } + count = split($0, records, cr) + last = count + if (count > 1 && records[count] == "" && substr($0, length($0), 1) == cr) { + last-- + } + for (i = 1; i <= last; i++) print records[i] + } + ' +} + +resolver_sanitize_canonical_pull_diagnostic() { + # Redact complete sensitive headers, then flatten the records so untrusted + # text cannot create a GitHub command. + LC_ALL=C awk ' BEGIN { sensitive_continuation = 0 } { lower = tolower($0) - if (match(lower, /(^|[[:space:]])(proxy-authorization|authorization|cookie|set-cookie|x-registry-auth|x-registry-config|x-api-key|x-auth-token)[[:space:]]*[:=]/)) { + if (match(lower, /(proxy-authorization|authorization|cookie|set-cookie|x-registry-auth|x-registry-config|x-api-key|x-auth-token)[[:space:]]*[:=]/)) { print substr($0, 1, RSTART + RLENGTH - 1) "[redacted]" sensitive_continuation = 1 next @@ -42,15 +64,21 @@ resolver_sanitize_pull_diagnostic() { print } ' \ - | sed -E \ + | LC_ALL=C tr '\011' ' ' \ + | LC_ALL=C sed -E \ -e 's#([Hh][Tt][Tt][Pp][Ss]?://)[^/@[:space:]]+@#\1[redacted]@#g' \ - -e 's#([?&][^=[:space:]&]{1,80}=)[^&[:space:]]+#\1[redacted]#g' \ + -e 's#([?&][^=[:space:]&]*=)[^&[:space:]]+#\1[redacted]#g' \ -e 's#(([Bb][Ee][Aa][Rr][Ee][Rr]|[Bb][Aa][Ss][Ii][Cc]|[Tt][Oo][Kk][Ee][Nn]|[Nn][Ee][Gg][Oo][Tt][Ii][Aa][Tt][Ee])[[:space:]]+)[^[:space:],;]+#\1[redacted]#g' \ -e 's#(([Tt][Oo][Kk][Ee][Nn]|[Aa][Cc][Cc][Ee][Ss][Ss][_-][Tt][Oo][Kk][Ee][Nn]|[Rr][Ee][Ff][Rr][Ee][Ss][Hh][_-][Tt][Oo][Kk][Ee][Nn]|[Aa][Pp][Ii][_-]?[Kk][Ee][Yy]|[Cc][Ll][Ii][Ee][Nn][Tt][_-][Ss][Ee][Cc][Rr][Ee][Tt]|[Pp][Aa][Ss][Ss][Ww][Oo][Rr][Dd]|[Pp][Aa][Ss][Ss][Ww][Dd]|[Ss][Ee][Cc][Rr][Ee][Tt]|[Aa][Uu][Tt][Hh]|[Ss][Ii][Gg]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]|[Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Xx]-[Aa][Mm][Zz]-([Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll]|[Ss][Ee][Cc][Uu][Rr][Ii][Tt][Yy]-[Tt][Oo][Kk][Ee][Nn]|[Ss][Ii][Gg][Nn][Aa][Tt][Uu][Rr][Ee]))[[:space:]]*=[[:space:]]*)[^&[:space:],;]+#\1[redacted]#g' \ -e 's#eyJ[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_.-]+#[redacted]#g' \ -e 's#(gh[pousr]_|github_pat_)[A-Za-z0-9_]{16,}#[redacted]#g' \ -e 's#(sk-|nvapi-|hf_)[A-Za-z0-9._-]{12,}#[redacted]#g' \ - | tr '\012' ' ' + | LC_ALL=C tr '\012' ' ' +} + +resolver_sanitize_pull_diagnostic() { + resolver_canonicalize_pull_diagnostic_records \ + | resolver_sanitize_canonical_pull_diagnostic } resolver_emit_pull_diagnostic() { @@ -58,15 +86,18 @@ resolver_emit_pull_diagnostic() { if [[ -z "$diagnostic" ]]; then echo "docker pull: command failed without diagnostic output" >&2 else + if ((${#diagnostic} > 500)); then + diagnostic="${diagnostic: -500}" + fi printf 'docker pull: %.500s\n' "$diagnostic" >&2 fi if [[ "$truncated" == 1 ]]; then - echo "docker pull: diagnostic truncated at ${RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT} bytes" >&2 + echo "docker pull: diagnostic truncated to final ${RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT} bytes" >&2 fi } -resolver_pull_diagnostic_is_transient() { +resolver_pull_diagnostic_is_deterministic() { local diagnostic normalized diagnostic="${1:-}" normalized="$(printf '%s' "$diagnostic" | LC_ALL=C tr '[:upper:]' '[:lower:]')" @@ -97,8 +128,17 @@ resolver_pull_diagnostic_is_transient() { || [[ "$normalized" =~ tls:.*bad[[:space:]]+certificate ]] \ || [[ "$normalized" =~ tls:.*failed[[:space:]]+to[[:space:]]+verify[[:space:]]+certificate ]] \ || [[ "$normalized" =~ (http[^0-9]{0,20}|status([[:space:]]+code)?[^0-9]{0,12})4([01][0-9]|2[0-8]|[3-9][0-9])([^0-9]|$) ]]; then - return 1 + return 0 fi + return 1 +} + +resolver_pull_diagnostic_is_transient() { + local diagnostic normalized + diagnostic="${1:-}" + normalized="$(printf '%s' "$diagnostic" | LC_ALL=C tr '[:upper:]' '[:lower:]')" + [[ -n "$normalized" ]] || return 1 + resolver_pull_diagnostic_is_deterministic "$diagnostic" && return 1 if [[ "$normalized" =~ (http[^[:alnum:]]+[^[:space:]]*[[:space:]]+|status([[:space:]]+code)?[^0-9]{0,12})(429|5[0-9][0-9])([^0-9]|$) ]] \ || [[ "$normalized" =~ (too[[:space:]]+many[[:space:]]+requests|toomanyrequests|rate[[:space:]_-]*limit) ]] \ @@ -119,33 +159,131 @@ resolver_pull_diagnostic_is_transient() { return 1 } +resolver_collect_pull_diagnostic_cleanup() { + if [[ -n "${counter_pid:-}" ]]; then + kill "$counter_pid" 2>/dev/null || true + wait "$counter_pid" 2>/dev/null || true + fi + if [[ -n "${collector_raw_file:-}" ]]; then + rm -f -- "$collector_raw_file" + fi +} + +resolver_collect_pull_diagnostic() ( + local collector_count_fifo="$1" collector_count_file="$2" + local collector_raw_file="$3" collector_diagnostic_file="$4" + local counter_pid="" pipeline_status=0 counter_status=0 byte_count + + set -o pipefail + trap resolver_collect_pull_diagnostic_cleanup EXIT + trap 'exit 129' HUP + trap 'exit 130' INT + trap 'exit 143' TERM + + LC_ALL=C wc -c <"$collector_count_fifo" >"$collector_count_file" & + counter_pid=$! + if LC_ALL=C tee "$collector_count_fifo" \ + | LC_ALL=C tail -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" >"$collector_raw_file"; then + pipeline_status=0 + else + pipeline_status=$? + fi + if wait "$counter_pid"; then + counter_status=0 + else + counter_status=$? + fi + counter_pid="" + ((pipeline_status == 0 && counter_status == 0)) || exit 74 + + byte_count="$(LC_ALL=C tr -d '[:space:]' <"$collector_count_file")" || exit 74 + [[ "$byte_count" =~ ^[0-9]+$ ]] || exit 74 + if ((byte_count > RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT)); then + # A bounded tail can begin inside a credential value. Drop that partial + # record and any folded continuations before sanitizing so an omitted + # header cannot expose its suffix. + if ! resolver_canonicalize_pull_diagnostic_records <"$collector_raw_file" \ + | LC_ALL=C awk ' + !discarded { + if (length($0) == 0) next + discarded = 1 + skip_continuations = 1 + next + } + skip_continuations && /^[ \t]/ { next } + { skip_continuations = 0; print } + ' \ + | resolver_sanitize_canonical_pull_diagnostic \ + | LC_ALL=C tail -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" >"$collector_diagnostic_file"; then + exit 74 + fi + elif ! resolver_sanitize_pull_diagnostic <"$collector_raw_file" \ + | LC_ALL=C tail -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" >"$collector_diagnostic_file"; then + exit 74 + fi + + rm -f -- "$collector_raw_file" + collector_raw_file="" +) + +resolver_capture_pull_cleanup() { + if [[ -n "${collector_pid:-}" ]]; then + kill "$collector_pid" 2>/dev/null || true + wait "$collector_pid" 2>/dev/null || true + fi + if [[ -n "${capture_dir:-}" ]]; then + rm -f -- "$diagnostic_file" "$raw_file" "$count_file" "$count_fifo" "$stderr_fifo" + rmdir -- "$capture_dir" 2>/dev/null || true + fi +} + resolver_capture_pull() ( - local ref="$1" diagnostic_file="" byte_count status truncated=0 + local ref="$1" capture_dir="" diagnostic_file raw_file count_file count_fifo stderr_fifo + local collector_pid="" byte_count collector_status status truncated=0 local temp_root="${RUNNER_TEMP:-${TMPDIR:-/tmp}}" - # mktemp creates the capture with mode 0600 under this restrictive umask. - # The 64 KiB prefix is sanitized before it enters the parent shell. + # The collector continuously drains Docker stderr, retains only its final + # 64 KiB in a mode-0700 directory, and removes the mode-0600 raw tail before + # any diagnostic enters the parent shell. umask 077 - diagnostic_file="$(mktemp "${temp_root%/}/nemoclaw-docker-pull.XXXXXX")" || exit 74 - trap 'rm -f -- "$diagnostic_file"' EXIT + capture_dir="$(mktemp -d "${temp_root%/}/nemoclaw-docker-pull.XXXXXX")" || exit 74 + diagnostic_file="$capture_dir/diagnostic" + raw_file="$capture_dir/raw-tail" + count_file="$capture_dir/byte-count" + count_fifo="$capture_dir/count.fifo" + stderr_fifo="$capture_dir/stderr.fifo" + trap resolver_capture_pull_cleanup EXIT trap 'exit 129' HUP trap 'exit 130' INT trap 'exit 143' TERM - if docker pull "$ref" >/dev/null 2>"$diagnostic_file"; then + mkfifo "$count_fifo" "$stderr_fifo" || exit 74 + resolver_collect_pull_diagnostic \ + "$count_fifo" "$count_file" "$raw_file" "$diagnostic_file" <"$stderr_fifo" & + collector_pid=$! + + if docker pull "$ref" >/dev/null 2>"$stderr_fifo"; then status=0 else status=$? fi - byte_count="$(wc -c <"$diagnostic_file")" || exit 74 + if wait "$collector_pid"; then + collector_status=0 + else + collector_status=$? + fi + collector_pid="" + ((collector_status == 0)) || exit 74 + + byte_count="$(LC_ALL=C tr -d '[:space:]' <"$count_file")" || exit 74 + [[ "$byte_count" =~ ^[0-9]+$ ]] || exit 74 if ((byte_count > RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT)); then truncated=1 fi printf '%s\n%s\n' "$status" "$truncated" - LC_ALL=C head -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" "$diagnostic_file" \ - | resolver_sanitize_pull_diagnostic + LC_ALL=C head -c "$RESOLVER_PULL_DIAGNOSTIC_BYTE_LIMIT" "$diagnostic_file" ) resolver_pull() { @@ -179,7 +317,14 @@ resolver_pull() { resolver_emit_pull_diagnostic "$diagnostic" "$truncated" + if resolver_pull_diagnostic_is_deterministic "$diagnostic"; then + return "$status" + fi if ! resolver_pull_diagnostic_is_transient "$diagnostic"; then + if [[ "$truncated" == 1 ]]; then + echo "::error::Truncated base-image pull diagnostics were not classifiable; refusing a local build fallback" >&2 + exit 75 + fi return "$status" fi if ((attempt == 3)); then diff --git a/test/base-image-resolver-helper.test.ts b/test/base-image-resolver-helper.test.ts index 84dcba0f84..536292c69b 100644 --- a/test/base-image-resolver-helper.test.ts +++ b/test/base-image-resolver-helper.test.ts @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { spawnSync } from "node:child_process"; -import { mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -377,10 +377,46 @@ resolver_pull example:test`, expect(result.status, result.stderr).toBe(0); }); + it.each([ + ["HTTP server failure", "unexpected HTTP status: 503 Service Unavailable"], + ["HTTP throttling", "unexpected HTTP status: 429 Too Many Requests"], + ["network failure", "connection reset by peer"], + ])("retries a terminal %s after more than 64 KiB of output", (_name, diagnostic) => { + const bin = fakeDocker(` +count="$(cat "$PULL_COUNT" 2>/dev/null || printf 0)" +count=$((count + 1)) +printf "%s\\n" "$count" > "$PULL_COUNT" +head -c 100000 /dev/zero | tr '\\0' x >&2 +printf "\\n%s\\n" "$PULL_DIAGNOSTIC" >&2 +exit 1`); + const pullCount = path.join(bin, "pull-count"); + + const result = run( + ` +sleep() { :; } +resolver_pull example:test`, + { + PATH: `${bin}:${process.env.PATH}`, + PULL_COUNT: pullCount, + PULL_DIAGNOSTIC: diagnostic, + RUNNER_TEMP: bin, + }, + ); + + expect(result.status).toBe(75); + expect(readFileSync(pullCount, "utf8")).toBe("3\n"); + expect(result.stderr).toContain(diagnostic); + expect(result.stderr.match(/diagnostic truncated to final 65536 bytes/g)).toHaveLength(3); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + it("redacts complete credential headers and prevents log-command injection", () => { + const ansiSecret = "ansi-registry-secret"; const basicSecret = "registry-password"; const cookieSecret = "session-cookie-secret"; const cookieSecondSecret = "second-cookie-secret"; + const crAuthorizationSecret = "cr-authorization-secret"; + const crProxySecret = "cr-proxy-secret"; const foldedSecret = "folded-registry-secret"; const negotiateSecret = "negotiate-registry-secret"; const proxySecret = "proxy-registry-secret"; @@ -388,6 +424,7 @@ resolver_pull example:test`, const registryAuthSecret = "registry-auth-secret"; const registryConfigSecret = "registry-config-secret"; const setCookieSecret = "set-cookie-secret"; + const verticalTabSecret = "vertical-tab-secret"; const bin = fakeDocker(` printf "%s\\r\\n" \ "pull access denied at https://registry-user:$BASIC_SECRET@example.test/v2/image?token=$QUERY_SECRET" \ @@ -400,12 +437,19 @@ printf "%s\\r\\n" \ "Authorization:" \ " Bearer $FOLDED_SECRET" \ $'\\033[31m\\r::warning::forged-pull-command' >&2 +printf "progress\\rAuthorization: CustomScheme %s\\r" "$CR_AUTHORIZATION_SECRET" >&2 +printf "progress\\rProxy-Authorization: CustomScheme %s\\r" "$CR_PROXY_SECRET" >&2 +printf "\\033[31mAuthorization: CustomScheme %s\\033[0m\\r\\n" "$ANSI_SECRET" >&2 +printf "progress\\vAuthorization: CustomScheme %s\\r\\n" "$VERTICAL_TAB_SECRET" >&2 exit 1`); const result = run("resolver_pull example:test", { + ANSI_SECRET: ansiSecret, BASIC_SECRET: basicSecret, COOKIE_SECRET: cookieSecret, COOKIE_SECOND_SECRET: cookieSecondSecret, + CR_AUTHORIZATION_SECRET: crAuthorizationSecret, + CR_PROXY_SECRET: crProxySecret, FOLDED_SECRET: foldedSecret, NEGOTIATE_SECRET: negotiateSecret, PATH: `${bin}:${process.env.PATH}`, @@ -414,15 +458,19 @@ exit 1`); REGISTRY_AUTH_SECRET: registryAuthSecret, REGISTRY_CONFIG_SECRET: registryConfigSecret, SET_COOKIE_SECRET: setCookieSecret, + VERTICAL_TAB_SECRET: verticalTabSecret, }); expect(result.status).toBe(1); expect(result.stderr).toContain("pull access denied"); expect(result.stderr).toContain("[redacted]"); for (const secret of [ + ansiSecret, basicSecret, cookieSecret, cookieSecondSecret, + crAuthorizationSecret, + crProxySecret, foldedSecret, negotiateSecret, proxySecret, @@ -430,6 +478,7 @@ exit 1`); registryAuthSecret, registryConfigSecret, setCookieSecret, + verticalTabSecret, ]) { expect(result.stderr).not.toContain(secret); } @@ -473,31 +522,213 @@ exit 1`); "Authorization:\r\n\tNegotiate folded-negotiate-value", ["folded-negotiate-value"], ], + [ + "folded arbitrary authorization value", + "Authorization:\r\n\tCustomScheme folded-custom-value", + ["folded-custom-value"], + ], + [ + "CR-only authorization after progress output", + "progress\rAuthorization: CustomScheme cr-only-authorization\r", + ["cr-only-authorization"], + ], + [ + "CR-only proxy authorization after progress output", + "progress\rProxy-Authorization: CustomScheme cr-only-proxy\r", + ["cr-only-proxy"], + ], + [ + "ANSI-colored arbitrary authorization value", + "\u001b[31mAuthorization: CustomScheme ansi-custom-value\u001b[0m", + ["ansi-custom-value"], + ], + [ + "control-separated arbitrary authorization value", + "progress\u000bAuthorization: CustomScheme control-custom-value", + ["control-custom-value"], + ], + [ + "control-interrupted authorization name", + "Authori\u000bzation: CustomScheme control-name-value", + ["control-name-value"], + ], + [ + "OSC-title-interrupted authorization name", + "Auth\u001b]0;registry\u0007orization: CustomScheme osc-title-value", + ["osc-title-value"], + ], + [ + "OSC-link-interrupted authorization name", + "Auth\u001b]8;;https://registry.example\u001b\\orization: CustomScheme osc-link-value", + ["osc-link-value"], + ], ])("redacts a complete %s", (_name, raw, secrets) => { - const result = run('printf "%s" "$RAW_DIAGNOSTIC" | resolver_sanitize_pull_diagnostic', { - RAW_DIAGNOSTIC: raw, - }); + const result = run( + 'set -o pipefail; printf "%s" "$RAW_DIAGNOSTIC" | resolver_sanitize_pull_diagnostic', + { + RAW_DIAGNOSTIC: raw, + }, + ); expect(result.status, result.stderr).toBe(0); expect(result.stdout).toContain("[redacted]"); for (const secret of secrets) expect(result.stdout).not.toContain(secret); }); + it("redacts a folded credential preceded by an invalid byte", () => { + const result = run( + `set -o pipefail +printf 'Authorization:\\r\\n\\377\\tCustomScheme invalid-fold-secret\\r\\n' | + resolver_sanitize_pull_diagnostic`, + ); + + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("[redacted]"); + expect(result.stdout).not.toContain("invalid-fold-secret"); + }); + + it.each([81, 4096])("redacts query values with a %i-byte key", (keyLength) => { + const secret = `long-query-secret-${keyLength}`; + const result = run( + 'set -o pipefail; printf "%s" "$RAW_DIAGNOSTIC" | resolver_sanitize_pull_diagnostic', + { + RAW_DIAGNOSTIC: `https://registry.example/v2/image?${"k".repeat(keyLength)}=${secret}`, + }, + ); + + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("[redacted]"); + expect(result.stdout).not.toContain(secret); + }); + + it("preserves a deterministic status with invalid diagnostic bytes under UTF-8", () => { + const bin = fakeDocker(` +printf 'manifest unknown \\377\\n' >&2 +exit 42`); + const result = run("resolver_pull example:test", { + LC_ALL: "C.UTF-8", + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status, result.stderr).toBe(42); + expect(result.stderr).toContain("manifest unknown"); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + it("bounds captured stderr and preserves a deterministic Docker status", () => { + const tailQuerySecret = "tail-query-secret"; + const longQueryKey = "k".repeat(4096); const bin = fakeDocker(` -printf "%s\\n" "daemon policy rejected this pull" >&2 head -c 100000 /dev/zero | tr '\\0' x >&2 +max_size=0 +for file in "$RUNNER_TEMP"/nemoclaw-docker-pull.*/*; do + if [[ -f "$file" ]]; then + size="$(wc -c < "$file")" + if ((size > max_size)); then max_size="$size"; fi + fi +done +printf "%s\\n" "$max_size" > "$OBSERVED_MAX" +printf "\\nhttps://registry.example/v2/image?%s=%s\\n" "$LONG_QUERY_KEY" "$TAIL_QUERY_SECRET" >&2 +printf "\\n%s\\n" "manifest unknown" >&2 exit 42`); + const observedMax = path.join(bin, "observed-max"); const result = run("resolver_pull example:test", { + LONG_QUERY_KEY: longQueryKey, + OBSERVED_MAX: observedMax, PATH: `${bin}:${process.env.PATH}`, RUNNER_TEMP: bin, + TAIL_QUERY_SECRET: tailQuerySecret, }); expect(result.status).toBe(42); - expect(result.stderr).toContain("daemon policy rejected this pull"); - expect(result.stderr).toContain("diagnostic truncated at 65536 bytes"); + expect(result.stderr).toContain("manifest unknown"); + expect(result.stderr).toContain("diagnostic truncated to final 65536 bytes"); + expect(result.stderr).not.toContain(tailQuerySecret); expect(result.stderr.length).toBeLessThan(1_000); + expect(Number(readFileSync(observedMax, "utf8"))).toBeLessThanOrEqual(65_536); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + + it("drops an ANSI-decorated folded credential after a truncated header", () => { + const foldedTailSecret = "folded-tail-secret"; + const bin = fakeDocker(` +printf "Authorization: " >&2 +head -c 100000 /dev/zero | tr '\\0' x >&2 +printf "\\r\\n\\033[31m\\tCustomScheme %s\\033[0m\\r\\nmanifest unknown\\n" "$FOLDED_TAIL_SECRET" >&2 +exit 42`); + + const result = run("resolver_pull example:test", { + FOLDED_TAIL_SECRET: foldedTailSecret, + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status, result.stderr).toBe(42); + expect(result.stderr).toContain("manifest unknown"); + expect(result.stderr).toContain("diagnostic truncated to final 65536 bytes"); + expect(result.stderr).not.toContain(foldedTailSecret); + expect(result.stderr).not.toContain("\u001b"); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + + it("drops an invalid-byte-prefixed folded credential after a truncated header", () => { + const foldedTailSecret = "invalid-folded-tail-secret"; + const bin = fakeDocker(` +printf "Authorization: " >&2 +head -c 100000 /dev/zero | tr '\\0' x >&2 +printf "\\r\\n\\377\\tCustomScheme %s\\r\\nmanifest unknown\\n" "$FOLDED_TAIL_SECRET" >&2 +exit 42`); + + const result = run("resolver_pull example:test", { + FOLDED_TAIL_SECRET: foldedTailSecret, + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status, result.stderr).toBe(42); + expect(result.stderr).toContain("manifest unknown"); + expect(result.stderr).toContain("diagnostic truncated to final 65536 bytes"); + expect(result.stderr).not.toContain(foldedTailSecret); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + + it("fails closed when a truncated terminal diagnostic is unclassified", () => { + const bin = fakeDocker(` +printf "%s\\n" "$@" >> "$DOCKER_LOG" +head -c 100000 /dev/zero | tr '\\0' x >&2 +printf "\\n%s\\n" "daemon policy rejected this pull" >&2 +exit 1`); + const dockerLog = path.join(bin, "docker.log"); + + const result = run("resolver_pull example:test", { + DOCKER_LOG: dockerLog, + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status).toBe(75); + expect(readFileSync(dockerLog, "utf8")).toBe("pull\nexample:test\n"); + expect(result.stderr).toContain("Truncated base-image pull diagnostics were not classifiable"); + expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); + }); + + it("fails closed and cleans up when the bounded collector fails", () => { + const bin = fakeDocker(` +printf "%s\\n" "connection reset by peer" >&2 +exit 1`); + writeFileSync(path.join(bin, "tail"), "#!/usr/bin/env bash\ncat >/dev/null\nexit 7\n", { + mode: 0o755, + }); + + const result = run("resolver_pull example:test", { + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: bin, + }); + + expect(result.status).toBe(75); + expect(result.stderr).toContain("could not be captured securely"); expect(readdirSync(bin).filter((name) => name.startsWith("nemoclaw-docker-pull."))).toEqual([]); }); @@ -506,8 +737,8 @@ exit 42`); printf "%s\\0" "$@" >> "$DOCKER_LOG" printf "\\0" >> "$DOCKER_LOG" if [[ "$1" == pull ]]; then - printf "%s\\n" "unexpected status code 503: Service Unavailable" >&2 head -c 100000 /dev/zero | tr '\\0' x >&2 + printf "\\n%s\\n" "unexpected status code 503: Service Unavailable" >&2 exit 1 fi if [[ "$1" == build ]]; then exit 42; fi @@ -539,7 +770,7 @@ exit 1`); expect(result.status).toBe(75); expect(readFileSync(githubEnv, "utf8")).toBe(""); expect(readFileSync(sleepLog, "utf8")).toBe("1\n2\n"); - expect(result.stderr.match(/diagnostic truncated at 65536 bytes/g)).toHaveLength(3); + expect(result.stderr.match(/diagnostic truncated to final 65536 bytes/g)).toHaveLength(3); const calls = readFileSync(dockerLog, "utf8") .split("\0\0") .filter(Boolean)