From 8a862495d2067a4404a6234ee5baa796b7ae1c58 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sat, 30 May 2026 12:14:57 +0400 Subject: [PATCH 1/3] fix(docker): stage security-report script via tempfile, avoid ARG_MAX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Pulumi `command:local:Command` security-report resource invokes its `Create` field via `/bin/sh -c ""`, which means the entire generated script body counts against the kernel's ARG_MAX (typically 128 KB on Linux). `buildSecurityReportScript` composes the script from the merged scan-results.json (Trivy + Grype). On an image carrying a fresh Ubuntu base — e.g. chrome-base-derived images used by browser-automation services — the merged scan can exceed 5,000 findings, producing script bodies in the 150-200 KB range. Every deploy on those images fails with: error: fork/exec /bin/sh: argument list too long error: update failed Observed in integrail/baas v2026.5.2 → test (run 26629920604) on 2026-05-29: 5,025 merged findings → 152 KB Create body → kernel rejected the fork before SC's report renderer ever ran. ## Fix Stage the generated script to a deterministic tempfile and invoke `bash ` instead of inlining. The Create argv stays under a few hundred bytes regardless of how many vulnerabilities the report enumerates. - New helper `stageSecurityReportScript(resourceName, script)`: - Writes to `$TMPDIR/sc-security-report--.sh`. - Path is deterministic on `(resourceName, script)` so Pulumi sees no spurious drift between runs of an otherwise-unchanged resource. - Filename sanitised — resource names may contain registry hostnames with `:` / `/`, which break tempfile paths. - Caller in `build_and_push.go` uses the staged path; if staging fails (any filesystem error), falls back to inlining the script — preserves prior behaviour for short reports where ARG_MAX is not a concern. ## Tests 5 new in `pkg/clouds/pulumi/docker/security_report_test.go`: - `RoundTrip` — content written + read back matches exactly. - `DeterministicPath` — same inputs => same path (no Pulumi drift). - `DifferentScriptDifferentPath` — different content => different path (collision avoidance). - `SanitizesResourceName` — `:` and `/` removed from basename. - `HandlesLargeScript` — 256 KB script body roundtrips; path stays small. Direct regression test for the ARG_MAX class of failure. All 5 pass. Full `go test ./pkg/clouds/pulumi/docker/...` clean; no existing tests touched. `go vet` clean. ## Out of scope - VEX-based scan-result trimming (consumed-side fix at the scanner layer): tracked in integrail/baas#138 with a `.vex/` document produced by integrail/everworker#2554's canonical pattern. That shrinks the input to this renderer; this commit makes the renderer robust to large inputs in the first place. The two fixes are complementary and both worth shipping — VEX reduces noise across the pipeline (DefectDojo, etc), tempfile-staging hardens this specific step. Signed-off-by: Dmitrii Creed --- pkg/clouds/pulumi/docker/build_and_push.go | 16 ++- pkg/clouds/pulumi/docker/security_report.go | 44 ++++++++ .../pulumi/docker/security_report_test.go | 106 ++++++++++++++++++ 3 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 pkg/clouds/pulumi/docker/security_report_test.go diff --git a/pkg/clouds/pulumi/docker/build_and_push.go b/pkg/clouds/pulumi/docker/build_and_push.go index 22255aa9..90892673 100644 --- a/pkg/clouds/pulumi/docker/build_and_push.go +++ b/pkg/clouds/pulumi/docker/build_and_push.go @@ -223,10 +223,22 @@ func executeSecurityOperations(ctx *sdk.Context, stack api.Stack, dockerImage *d if scanGate != nil { reportDeps = append(reportDeps, scanGate) } - _, err = local.NewCommand(ctx, fmt.Sprintf("security-report-%s", imageName), &local.CommandArgs{ + reportResourceName := fmt.Sprintf("security-report-%s", imageName) + _, err = local.NewCommand(ctx, reportResourceName, &local.CommandArgs{ + // Stage the report script to a tempfile and invoke it by path — + // keeps the Create argv under ARG_MAX regardless of how many + // vulnerabilities the merged scan-results.json enumerates. On any + // filesystem error the helper returns "" and we fall back to + // inlining the script (preserves prior behaviour for short + // reports). See stageSecurityReportScript for the full rationale. Create: securityImageRef.ApplyT(func(img string) string { commentOutput := resolveCommentOutputPath(security, imageName) - return buildSecurityReportScript(img, imageName, security, commentOutput) + script := buildSecurityReportScript(img, imageName, security, commentOutput) + path, err := stageSecurityReportScript(reportResourceName, script) + if err != nil || path == "" { + return script + } + return fmt.Sprintf("bash %s", shellQuote(path)) }).(sdk.StringOutput), }, sdk.DependsOn(reportDeps)) if err != nil { diff --git a/pkg/clouds/pulumi/docker/security_report.go b/pkg/clouds/pulumi/docker/security_report.go index 38b3ac73..652e50b5 100644 --- a/pkg/clouds/pulumi/docker/security_report.go +++ b/pkg/clouds/pulumi/docker/security_report.go @@ -1,13 +1,57 @@ package docker import ( + "crypto/sha256" + "encoding/hex" "fmt" + "os" "path/filepath" + "regexp" "strings" "github.com/simple-container-com/api/pkg/api" ) +// sanitizeForFilenameRe replaces any char outside the safelist with `_`, used +// when composing a tempfile name from a Pulumi resource name (which may +// contain registry hostnames, `:`, `/`, etc.). +var sanitizeForFilenameRe = regexp.MustCompile(`[^A-Za-z0-9._-]`) + +// stageSecurityReportScript writes the dynamically-built report script to a +// deterministic tempfile under $TMPDIR and returns the path. +// +// Why: the script is composed from the merged scan-results.json (Trivy + +// Grype) — which on an image carrying a fresh Ubuntu base can run into +// thousands of CVEs. The Pulumi `command:local:Command` resource invokes its +// `Create` field via `/bin/sh -c ""`, which means the script content +// counts against the kernel's ARG_MAX (typically 128 KB on Linux). On a +// chrome-base-derived image we observed 5,025 merged findings producing a +// >150 KB script body, and every deploy failed with: +// +// error: fork/exec /bin/sh: argument list too long +// error: update failed +// +// Staging the script to a tempfile and returning a short `bash ` +// invocation keeps the Create argv well under ARG_MAX regardless of how many +// vulnerabilities the report enumerates. +// +// Path is deterministic on (resourceName, script-content) — same inputs +// produce the same path, so Pulumi doesn't see spurious "drift" between +// runs that would otherwise re-trigger the resource on no-op refreshes. +// +// On any filesystem error, returns the empty string and the error; callers +// should fall back to inlining the script (preserves prior behaviour for +// short reports where ARG_MAX is not a concern). +func stageSecurityReportScript(resourceName, script string) (string, error) { + sum := sha256.Sum256([]byte(resourceName + "\x00" + script)) + safeName := sanitizeForFilenameRe.ReplaceAllString(resourceName, "_") + path := filepath.Join(os.TempDir(), fmt.Sprintf("sc-security-report-%s-%s.sh", safeName, hex.EncodeToString(sum[:8]))) + if err := os.WriteFile(path, []byte(script), 0o600); err != nil { + return "", err + } + return path, nil +} + // buildSecurityReportScript generates a shell script that reads scan results and // outputs a unified security summary to the console, $GITHUB_STEP_SUMMARY, and // an optional markdown file for PR comments. diff --git a/pkg/clouds/pulumi/docker/security_report_test.go b/pkg/clouds/pulumi/docker/security_report_test.go new file mode 100644 index 00000000..f6159682 --- /dev/null +++ b/pkg/clouds/pulumi/docker/security_report_test.go @@ -0,0 +1,106 @@ +package docker + +import ( + "os" + "strings" + "testing" + + . "github.com/onsi/gomega" +) + +func TestStageSecurityReportScript_RoundTrip(t *testing.T) { + RegisterTestingT(t) + + script := "echo hello\nprintf 'world\\n'\n" + path, err := stageSecurityReportScript("security-report-baas", script) + defer os.Remove(path) + + Expect(err).NotTo(HaveOccurred()) + Expect(path).NotTo(BeEmpty()) + Expect(path).To(HavePrefix(os.TempDir())) + Expect(path).To(HaveSuffix(".sh")) + + got, err := os.ReadFile(path) + Expect(err).NotTo(HaveOccurred()) + Expect(string(got)).To(Equal(script)) +} + +// stageSecurityReportScript must produce the same path on identical inputs so +// Pulumi sees no drift between runs of an otherwise-unchanged resource. +func TestStageSecurityReportScript_DeterministicPath(t *testing.T) { + RegisterTestingT(t) + + script := "REPORT=\"\"\nprintf '%b' \"$REPORT\"\n" + p1, err := stageSecurityReportScript("security-report-api", script) + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(p1) + + p2, err := stageSecurityReportScript("security-report-api", script) + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(p2) + + Expect(p1).To(Equal(p2)) +} + +// Same resource name + different script content => different paths. +func TestStageSecurityReportScript_DifferentScriptDifferentPath(t *testing.T) { + RegisterTestingT(t) + + p1, err := stageSecurityReportScript("security-report-api", "echo one\n") + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(p1) + + p2, err := stageSecurityReportScript("security-report-api", "echo two\n") + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(p2) + + Expect(p1).NotTo(Equal(p2)) +} + +// Resource names may contain `:` or `/` (registry URLs, etc.). The tempfile +// path must be filesystem-safe. +func TestStageSecurityReportScript_SanitizesResourceName(t *testing.T) { + RegisterTestingT(t) + + path, err := stageSecurityReportScript( + "security-report-471112843480.dkr.ecr.us-west-2.amazonaws.com/baas-ecr:tag", + "echo ok\n", + ) + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(path) + + // No raw `:` or `/` should leak into the basename. + base := path[strings.LastIndex(path, "/")+1:] + Expect(base).NotTo(ContainSubstring(":")) +} + +// Reality check: the original ARG_MAX failure was a ~150 KB inlined script. +// A staged invocation `bash ` is well under the kernel limit +// (typically 128 KB on Linux). This test asserts the contract — that the +// helper produces a short path that the caller can compose into a short +// Create command, regardless of the script body size. +func TestStageSecurityReportScript_HandlesLargeScript(t *testing.T) { + RegisterTestingT(t) + + // Build a script body that exceeds typical Linux ARG_MAX (128 KB) by + // roughly 2× — the size class the original failure landed at on a + // chrome-base-derived image (5,025 merged CVEs producing ~150 KB). + const targetBytes = 256 * 1024 + var sb strings.Builder + for sb.Len() < targetBytes { + sb.WriteString("REPORT=\"${REPORT}| HIGH | CVE-XXXX-YYYY | pkg | x.y | - |\\n\"\n") + } + largeScript := sb.String() + Expect(len(largeScript)).To(BeNumerically(">", 256*1024)) + + path, err := stageSecurityReportScript("security-report-large", largeScript) + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(path) + + // Path itself stays small. + Expect(len(path)).To(BeNumerically("<", 256)) + + got, err := os.ReadFile(path) + Expect(err).NotTo(HaveOccurred()) + Expect(len(got)).To(Equal(len(largeScript))) +} From 691e15b3606fef2745d92f868d190e80d76985da Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sat, 30 May 2026 15:55:54 +0400 Subject: [PATCH 2/3] fix(review): atomic rename, sh-not-bash, filename cap, log fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups from independent code reviews on this PR (codex + claude opus, identical findings on the bash issue, opus caught the atomicity gap, codex caught the filename-length gap). ## 1. `bash` → `sh` (codex + opus, both P0) `build_and_push.go:241` invoked the staged script via `bash`. SC can land on Alpine-based Pulumi runners where `bash` isn't installed — exit 127 before the report script ever runs. The generated script uses only POSIX constructs (`printf`, `jq`, parameter expansion) so `sh` works equally well. Switched. ## 2. Atomic write via `os.CreateTemp` + `os.Rename` (opus P0) `security_report.go`: previous `os.WriteFile` truncates-then-streams. Concurrent deploys with *different* script content but the *same* sanitised prefix could interleave a write with bash reading the file. Same-content collisions were already safe (idempotent overwrite), but different-content collisions weren't. Now writes to a unique `os.CreateTemp` file in `$TMPDIR` and `os.Rename`s to the deterministic final path. Rename is atomic on the same filesystem (both files in `$TMPDIR`), so readers never observe a partial file from another writer. ## 3. Cap resource-name component of the basename (codex P0) `security_report.go`: a long resource name (registry hostname + slash-separated path + tag, easily 200+ chars from SC's naming convention) could push the sanitised basename past NAME_MAX (255 bytes on Linux). `os.WriteFile` returned ENAMETOOLONG → caller fell back to inlining the full script → reintroduced the exact ARG_MAX failure this helper exists to fix. Added `maxSafeNameLen = 64` cap on the resource-name component. Hash (16 hex chars) carries collision avoidance; the human-readable prefix is purely for debuggability. ## 4. Log fallback errors via `ctx.Log.Warn` (opus P0) `build_and_push.go`: previously the staging error was silently discarded — operators wouldn't see *why* the deploy fell back to inlining (and potentially hit ARG_MAX). Now logs a warn through the Pulumi context with the underlying error, while still falling back gracefully (preserves prior behaviour for short reports). ## Test additions - `TestStageSecurityReportScript_CapsLongResourceName` — 640-char resource name produces a basename < 255 bytes. - `TestStageSecurityReportScript_ConcurrentWriters` — 24 goroutines hammering the same final path; every observed read sees complete, correct content (regression guard for the atomicity fix). All 7 tests pass. Full `go test ./pkg/clouds/pulumi/docker/...` clean. `go vet` clean. Signed-off-by: Dmitrii Creed --- pkg/clouds/pulumi/docker/build_and_push.go | 26 +++++--- pkg/clouds/pulumi/docker/security_report.go | 61 +++++++++++++++++-- .../pulumi/docker/security_report_test.go | 60 +++++++++++++++++- 3 files changed, 133 insertions(+), 14 deletions(-) diff --git a/pkg/clouds/pulumi/docker/build_and_push.go b/pkg/clouds/pulumi/docker/build_and_push.go index 90892673..01343f3a 100644 --- a/pkg/clouds/pulumi/docker/build_and_push.go +++ b/pkg/clouds/pulumi/docker/build_and_push.go @@ -227,18 +227,30 @@ func executeSecurityOperations(ctx *sdk.Context, stack api.Stack, dockerImage *d _, err = local.NewCommand(ctx, reportResourceName, &local.CommandArgs{ // Stage the report script to a tempfile and invoke it by path — // keeps the Create argv under ARG_MAX regardless of how many - // vulnerabilities the merged scan-results.json enumerates. On any - // filesystem error the helper returns "" and we fall back to - // inlining the script (preserves prior behaviour for short - // reports). See stageSecurityReportScript for the full rationale. + // vulnerabilities the merged scan-results.json enumerates. Invoked + // via `sh` (not `bash`) because the generated script only uses + // POSIX constructs (`printf`, `jq`, parameter expansion) and SC + // can land on Alpine-based runners where `bash` is absent. + // + // On any filesystem error the helper returns "" and we fall back + // to inlining the script (preserves prior behaviour for short + // reports). The staging error is logged via ctx.Log.Warn so + // operators can investigate — silent fallback would re-introduce + // the exact ARG_MAX failure this helper exists to fix. Create: securityImageRef.ApplyT(func(img string) string { commentOutput := resolveCommentOutputPath(security, imageName) script := buildSecurityReportScript(img, imageName, security, commentOutput) - path, err := stageSecurityReportScript(reportResourceName, script) - if err != nil || path == "" { + path, stageErr := stageSecurityReportScript(reportResourceName, script) + if stageErr != nil || path == "" { + if stageErr != nil { + _ = ctx.Log.Warn( + fmt.Sprintf("security-report: failed to stage script for %q (falling back to inline, large reports may hit ARG_MAX): %v", imageName, stageErr), + &sdk.LogArgs{}, + ) + } return script } - return fmt.Sprintf("bash %s", shellQuote(path)) + return fmt.Sprintf("sh %s", shellQuote(path)) }).(sdk.StringOutput), }, sdk.DependsOn(reportDeps)) if err != nil { diff --git a/pkg/clouds/pulumi/docker/security_report.go b/pkg/clouds/pulumi/docker/security_report.go index 652e50b5..ec7499d3 100644 --- a/pkg/clouds/pulumi/docker/security_report.go +++ b/pkg/clouds/pulumi/docker/security_report.go @@ -17,6 +17,16 @@ import ( // contain registry hostnames, `:`, `/`, etc.). var sanitizeForFilenameRe = regexp.MustCompile(`[^A-Za-z0-9._-]`) +// maxSafeNameLen caps the resource-name component of the staged-script +// basename so the full filename stays under common NAME_MAX limits (255 +// bytes on Linux, 255 chars on macOS). A long ECR-derived image name + +// stack + service can easily push the unsanitised resource name past 200 +// chars — at which point os.WriteFile returns ENAMETOOLONG and the caller +// falls back to inlining the full script, reintroducing the ARG_MAX +// failure this helper exists to avoid. +const maxSafeNameLen = 64 +const stagedScriptPrefix = "sc-security-report-" + // stageSecurityReportScript writes the dynamically-built report script to a // deterministic tempfile under $TMPDIR and returns the path. // @@ -31,25 +41,64 @@ var sanitizeForFilenameRe = regexp.MustCompile(`[^A-Za-z0-9._-]`) // error: fork/exec /bin/sh: argument list too long // error: update failed // -// Staging the script to a tempfile and returning a short `bash ` +// Staging the script to a tempfile and returning a short `sh ` // invocation keeps the Create argv well under ARG_MAX regardless of how many // vulnerabilities the report enumerates. // +// ## Atomicity +// +// Writes through `os.CreateTemp + os.Rename` so concurrent deploys with +// different script content but colliding final paths (extremely unlikely +// given the 64-bit hash, but possible) can't observe a half-written file +// from another writer. `os.Rename` is atomic on the same filesystem; both +// the unique stage file and the final path live in `$TMPDIR`. +// +// ## Path stability +// // Path is deterministic on (resourceName, script-content) — same inputs // produce the same path, so Pulumi doesn't see spurious "drift" between // runs that would otherwise re-trigger the resource on no-op refreshes. // +// ## Fallback contract +// // On any filesystem error, returns the empty string and the error; callers // should fall back to inlining the script (preserves prior behaviour for -// short reports where ARG_MAX is not a concern). +// short reports where ARG_MAX is not a concern) AND log the staging error +// so operators can investigate — silent fallback would re-introduce the +// exact failure mode this helper exists to fix. func stageSecurityReportScript(resourceName, script string) (string, error) { sum := sha256.Sum256([]byte(resourceName + "\x00" + script)) safeName := sanitizeForFilenameRe.ReplaceAllString(resourceName, "_") - path := filepath.Join(os.TempDir(), fmt.Sprintf("sc-security-report-%s-%s.sh", safeName, hex.EncodeToString(sum[:8]))) - if err := os.WriteFile(path, []byte(script), 0o600); err != nil { - return "", err + if len(safeName) > maxSafeNameLen { + safeName = safeName[:maxSafeNameLen] + } + finalPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s%s-%s.sh", stagedScriptPrefix, safeName, hex.EncodeToString(sum[:8]))) + + tmpFile, err := os.CreateTemp(os.TempDir(), stagedScriptPrefix+"stage-*.sh.tmp") + if err != nil { + return "", fmt.Errorf("create stage tempfile: %w", err) + } + tmpPath := tmpFile.Name() + cleanup := func() { _ = os.Remove(tmpPath) } + if _, err := tmpFile.Write([]byte(script)); err != nil { + _ = tmpFile.Close() + cleanup() + return "", fmt.Errorf("write stage tempfile: %w", err) + } + if err := tmpFile.Chmod(0o600); err != nil { + _ = tmpFile.Close() + cleanup() + return "", fmt.Errorf("chmod stage tempfile: %w", err) + } + if err := tmpFile.Close(); err != nil { + cleanup() + return "", fmt.Errorf("close stage tempfile: %w", err) + } + if err := os.Rename(tmpPath, finalPath); err != nil { + cleanup() + return "", fmt.Errorf("rename stage tempfile to %s: %w", finalPath, err) } - return path, nil + return finalPath, nil } // buildSecurityReportScript generates a shell script that reads scan results and diff --git a/pkg/clouds/pulumi/docker/security_report_test.go b/pkg/clouds/pulumi/docker/security_report_test.go index f6159682..cd0b0754 100644 --- a/pkg/clouds/pulumi/docker/security_report_test.go +++ b/pkg/clouds/pulumi/docker/security_report_test.go @@ -2,7 +2,9 @@ package docker import ( "os" + "path/filepath" "strings" + "sync" "testing" . "github.com/onsi/gomega" @@ -74,8 +76,64 @@ func TestStageSecurityReportScript_SanitizesResourceName(t *testing.T) { Expect(base).NotTo(ContainSubstring(":")) } +// Long resource names (registry hostname + slash-separated path + tag +// produced by SC's naming convention) can push the unsanitised resource +// name well past NAME_MAX (255 bytes on Linux). The helper must cap the +// resource-name component so the final basename stays under the limit — +// otherwise ENAMETOOLONG would trip the fallback path and reintroduce +// the ARG_MAX failure for the large reports this helper exists to fix. +func TestStageSecurityReportScript_CapsLongResourceName(t *testing.T) { + RegisterTestingT(t) + + long := strings.Repeat("very-long-resource-name-segment.", 20) // ~640 chars + path, err := stageSecurityReportScript(long, "echo ok\n") + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(path) + + base := filepath.Base(path) + Expect(len(base)).To(BeNumerically("<", 255)) +} + +// `os.Rename` is atomic on the same filesystem. Concurrent writers +// targeting the same final path (same resource name + same script +// content => same path) must not produce a partially-written file +// observable by a reader. We can't directly assert atomicity, but we +// can run many writers in parallel and assert every observed final +// file is complete + correct. +func TestStageSecurityReportScript_ConcurrentWriters(t *testing.T) { + RegisterTestingT(t) + + const script = "REPORT=\"value\"\nprintf '%s\\n' \"$REPORT\"\n" + const n = 24 + + var wg sync.WaitGroup + wg.Add(n) + paths := make([]string, n) + errs := make([]error, n) + for i := 0; i < n; i++ { + go func(i int) { + defer wg.Done() + paths[i], errs[i] = stageSecurityReportScript("security-report-concurrent", script) + }(i) + } + wg.Wait() + + for i, p := range paths { + Expect(errs[i]).NotTo(HaveOccurred()) + Expect(p).NotTo(BeEmpty()) + got, err := os.ReadFile(p) + Expect(err).NotTo(HaveOccurred()) + Expect(string(got)).To(Equal(script), "writer %d observed truncated/interleaved content", i) + } + // All paths identical (deterministic). + for i := 1; i < n; i++ { + Expect(paths[i]).To(Equal(paths[0])) + } + defer os.Remove(paths[0]) +} + // Reality check: the original ARG_MAX failure was a ~150 KB inlined script. -// A staged invocation `bash ` is well under the kernel limit +// A staged invocation `sh ` is well under the kernel limit // (typically 128 KB on Linux). This test asserts the contract — that the // helper produces a short path that the caller can compose into a short // Create command, regardless of the script body size. From 1f747c70cad74edb8369cf8d26abe3a632da754d Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sat, 30 May 2026 16:24:35 +0400 Subject: [PATCH 3/3] fix(review): TTL sweep + stat short-circuit (should-fix follow-ups) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two should-fix items from the codex + opus review pass, folded into this PR. ## 1. TTL sweep (opus #2 should-fix) On a long-lived self-hosted CI runner, `/tmp/sc-security-report-*.sh` files accumulate indefinitely — every deploy stages a new file, nothing cleans them up. Hundreds per day × months → directory growth unbounded. Added `sweepStaleStagedScripts(maxAge)` that scans `$TMPDIR` for prefix-matching files and removes anything older than `maxAge`. Called once per process via `sync.Once` from `stageSecurityReportScript` — first stage in a deploy sweeps prior runs, subsequent stages in the same `pulumi up` no-op. Threshold: 24h. Long enough that a Pulumi resource's Delete (which inspects the original Create text) can still find the file during normal teardown windows. Best-effort: any FS error during sweep is silently ignored — failure here would be cleanup of prior runs, not the critical path of this deploy. ## 2. Stat short-circuit (opus #4 nit) Pulumi's ApplyT callback re-fires on every `preview`/`up`. The previous implementation rewrote the file (via atomic rename) every time, even when nothing had changed — mtime churn on no-op refreshes. Added an `os.Stat(finalPath)` guard: if the deterministic-path file already exists, return it without rewriting. Refreshes mtime via `os.Chtimes` so the TTL sweep doesn't garbage-collect actively- referenced files between preview and apply. The hash-derived path guarantees content matches when the path exists (collision probability ~2^-64), so the short-circuit is safe without a content read. ## Tests - `TestStageSecurityReportScript_StatShortCircuit` — backdates the staged file, re-stages with identical content, asserts mtime was refreshed AND file size unchanged (no rewrite happened). - `TestSweepStaleStagedScripts` — creates a young + old file with the staged prefix, runs sweep with 1h cutoff, asserts young survives and old is reaped. All 9 tests in the file pass. Full package + `go vet` clean. ## Not in scope - `bash` → `sh` fallback path for the *inlined* script (the inline-script case only runs when staging fails — already exceptional, doesn't compound the Alpine concern). - Shared `integrail-deployer-bot` IAM-key blast radius — operational concern, not an SC code change. Signed-off-by: Dmitrii Creed --- pkg/clouds/pulumi/docker/security_report.go | 60 +++++++++++++++++++ .../pulumi/docker/security_report_test.go | 57 ++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/pkg/clouds/pulumi/docker/security_report.go b/pkg/clouds/pulumi/docker/security_report.go index ec7499d3..5bf87e26 100644 --- a/pkg/clouds/pulumi/docker/security_report.go +++ b/pkg/clouds/pulumi/docker/security_report.go @@ -8,6 +8,8 @@ import ( "path/filepath" "regexp" "strings" + "sync" + "time" "github.com/simple-container-com/api/pkg/api" ) @@ -27,6 +29,42 @@ var sanitizeForFilenameRe = regexp.MustCompile(`[^A-Za-z0-9._-]`) const maxSafeNameLen = 64 const stagedScriptPrefix = "sc-security-report-" +// stagedScriptMaxAge is the lifetime ceiling for staged scripts in TMPDIR. +// On a long-lived CI runner with hundreds of deploys per day, the staged +// files would accumulate indefinitely otherwise. 24h is long enough that +// a Pulumi resource's Delete (which inspects Create text) can still find +// the file during normal teardown windows. +const stagedScriptMaxAge = 24 * time.Hour + +// sweepOnce coalesces the TTL sweep so multiple stages in a single +// process (e.g., a stack with several images) don't each scan TMPDIR. +var sweepOnce sync.Once + +// sweepStaleStagedScripts removes stagedScriptPrefix-named files in TMPDIR +// older than `maxAge`. Best-effort — any error is silently ignored, since +// failure here would be cleanup of prior runs, not the critical path. +func sweepStaleStagedScripts(maxAge time.Duration) { + dir := os.TempDir() + entries, err := os.ReadDir(dir) + if err != nil { + return + } + cutoff := time.Now().Add(-maxAge) + for _, entry := range entries { + name := entry.Name() + if !strings.HasPrefix(name, stagedScriptPrefix) { + continue + } + info, err := entry.Info() + if err != nil { + continue + } + if info.ModTime().Before(cutoff) { + _ = os.Remove(filepath.Join(dir, name)) + } + } +} + // stageSecurityReportScript writes the dynamically-built report script to a // deterministic tempfile under $TMPDIR and returns the path. // @@ -66,7 +104,18 @@ const stagedScriptPrefix = "sc-security-report-" // short reports where ARG_MAX is not a concern) AND log the staging error // so operators can investigate — silent fallback would re-introduce the // exact failure mode this helper exists to fix. +// +// ## Idempotence +// +// Pulumi's ApplyT callback re-fires on every `up` / `preview`, even when +// the script content hasn't changed. The hash-derived path is stable, so +// we `Stat` first and skip the write if a file already exists at that +// path — both cheaper (no rewrite, no mtime churn) and a no-op for +// readers concurrent with our re-entry. func stageSecurityReportScript(resourceName, script string) (string, error) { + // First call per process: clear out stale stages from prior deploys. + sweepOnce.Do(func() { sweepStaleStagedScripts(stagedScriptMaxAge) }) + sum := sha256.Sum256([]byte(resourceName + "\x00" + script)) safeName := sanitizeForFilenameRe.ReplaceAllString(resourceName, "_") if len(safeName) > maxSafeNameLen { @@ -74,6 +123,17 @@ func stageSecurityReportScript(resourceName, script string) (string, error) { } finalPath := filepath.Join(os.TempDir(), fmt.Sprintf("%s%s-%s.sh", stagedScriptPrefix, safeName, hex.EncodeToString(sum[:8]))) + // Hash-derived path uniquely identifies the (resourceName, script) + // tuple. If the file already exists, it was written by an earlier + // stage of this process or a prior `pulumi up` — trust the hash and + // skip the rewrite. Refresh mtime so the TTL sweep doesn't garbage- + // collect the file while it's still actively referenced. + if _, err := os.Stat(finalPath); err == nil { + now := time.Now() + _ = os.Chtimes(finalPath, now, now) + return finalPath, nil + } + tmpFile, err := os.CreateTemp(os.TempDir(), stagedScriptPrefix+"stage-*.sh.tmp") if err != nil { return "", fmt.Errorf("create stage tempfile: %w", err) diff --git a/pkg/clouds/pulumi/docker/security_report_test.go b/pkg/clouds/pulumi/docker/security_report_test.go index cd0b0754..b66f394f 100644 --- a/pkg/clouds/pulumi/docker/security_report_test.go +++ b/pkg/clouds/pulumi/docker/security_report_test.go @@ -6,6 +6,7 @@ import ( "strings" "sync" "testing" + "time" . "github.com/onsi/gomega" ) @@ -76,6 +77,62 @@ func TestStageSecurityReportScript_SanitizesResourceName(t *testing.T) { Expect(base).NotTo(ContainSubstring(":")) } +// Pulumi's ApplyT callback re-fires on every preview/up. The helper must +// short-circuit when the deterministic path already exists — no rewrite, +// no mtime churn beyond the explicit refresh. Asserts: +// 1. Second call returns identical path. +// 2. mtime was refreshed by the second call (so TTL sweep won't garbage- +// collect actively-referenced files between preview and apply). +// 3. File size unchanged (no rewrite happened — content was already correct). +func TestStageSecurityReportScript_StatShortCircuit(t *testing.T) { + RegisterTestingT(t) + + const script = "echo first\n" + first, err := stageSecurityReportScript("security-report-shortcircuit", script) + Expect(err).NotTo(HaveOccurred()) + defer os.Remove(first) + + firstInfo, err := os.Stat(first) + Expect(err).NotTo(HaveOccurred()) + firstSize := firstInfo.Size() + + // Backdate so we can detect whether the second call refreshed mtime. + old := time.Now().Add(-12 * time.Hour) + Expect(os.Chtimes(first, old, old)).To(Succeed()) + + second, err := stageSecurityReportScript("security-report-shortcircuit", script) + Expect(err).NotTo(HaveOccurred()) + Expect(second).To(Equal(first)) + + secondInfo, err := os.Stat(second) + Expect(err).NotTo(HaveOccurred()) + Expect(secondInfo.Size()).To(Equal(firstSize), "stat short-circuit should not rewrite file") + Expect(secondInfo.ModTime()).To(BeTemporally(">", old.Add(time.Hour)), "mtime should have been refreshed by stat short-circuit") +} + +// The TTL sweep removes prefix-matching files older than the threshold. +// Asserts a young file survives + an old file gets reaped. +func TestSweepStaleStagedScripts(t *testing.T) { + RegisterTestingT(t) + + dir := os.TempDir() + young := filepath.Join(dir, stagedScriptPrefix+"sweep-young-"+strings.Repeat("a", 8)+".sh") + old := filepath.Join(dir, stagedScriptPrefix+"sweep-old-"+strings.Repeat("b", 8)+".sh") + Expect(os.WriteFile(young, []byte("echo young\n"), 0o600)).To(Succeed()) + Expect(os.WriteFile(old, []byte("echo old\n"), 0o600)).To(Succeed()) + defer os.Remove(young) + defer os.Remove(old) + + // Backdate the old one well past the sweep threshold (1h). + pastCutoff := time.Now().Add(-2 * time.Hour) + Expect(os.Chtimes(old, pastCutoff, pastCutoff)).To(Succeed()) + + sweepStaleStagedScripts(1 * time.Hour) + + Expect(young).To(BeAnExistingFile(), "young file should survive sweep") + Expect(old).NotTo(BeAnExistingFile(), "old file should be reaped by sweep") +} + // Long resource names (registry hostname + slash-separated path + tag // produced by SC's naming convention) can push the unsanitised resource // name well past NAME_MAX (255 bytes on Linux). The helper must cap the