fix: the spool is bounded, and its bound no longer depends on the cluster being healthy (ADR 0042)

[kfeofantov]

The last audit finding whose victim is the customer's cluster rather than this product. Three things were wrong with the spool's bound.

1. The age cap was conditional on the cluster being healthy

sweep was private and called from exactly one place — the end of WriteUsageSnapshot — because it rode the snapshot cadence "so no extra timer exists".

But WriteUsageSnapshot runs only when there are usage records, and there are none when the kubelet cannot be polled: nodes/proxy withheld, every kubelet unreachable, the poll loop stuck. On exactly those clusters nothing was ever swept, the 24h cap never applied, and the spool grew without limit while the agent looked healthy.

A bound that stops working on an unhealthy cluster is not a bound. And the second timer that comment was avoiding did not need creating — the periodic flush loop was already there.

2. Age bounds nothing an adversary controls

WriteOOMKill writes one file per (finishedAt, namespace, pod, container, restartCount). The restart count is in the name, so every restart is a new file rather than an overwrite. Nothing acknowledges payloads yet, so nothing removes them before the 24h mark, and the controller's emptyDir had no sizeLimit.

A pod OOM-killing in a tight loop therefore consumes the node's ephemeral storage — and a node that runs out makes the kubelet start evicting pods. Other workloads' pods, on a node this agent is a guest of.

The irony is local: the restart journal was deliberately windowed with the reasoning that "a crash loop must not put the spool's file count under its own control". That reasoning was never applied to the OOM path beside it.

3. A caller-supplied string reached a filename

WriteProfile built its name from key.Workload, which is ownerReferences[].name — a field the API server validates for non-emptiness and nothing else: no DNS-1123, no character set. filepath.Join resolves ...

Checked in both directions. With the sanitizer removed, the traversal tests fail on five of eight inputs:

--- FAIL: TestAnOwnerReferenceNameCannotEscapeTheSpool/"../../../../tmp/pwn"
--- FAIL: .../"x/../../../../etc/cron.d/evil"
--- FAIL: .../"..\\..\\windows"
    payload landed at "profile-acme-..\..\windows-server-deadbeef-100-160.json", which is not a plain filename
--- FAIL: TestWriteRefusesANameThatIsNotAPlainFilename
    write("../escape.json") was allowed

Worth stating precisely rather than dramatically: the write creates no intermediate directories, so it lands only where the whole parent path already exists and is writable. In the shipped chart readOnlyRootFilesystem: true makes most targets fail with EROFS — a real mitigation, and an accident of an unrelated setting rather than anything this package did.

What changes

• Sweep is exported and called unconditionally from the agent's own periodic loop.
• 512 MiB and 20000 files, oldest-first after the age cutoff. Both, because the smallest payload is a few hundred bytes and a byte budget alone permits millions of files — enough to exhaust inodes and make every sweep's listing expensive. Constants, not values: the spool is always an emptyDir and no setting changes that (ADR 0026).
• emptyDir: {sizeLimit: 1Gi}, deliberately above the agent's own 512 MiB budget. The ordering is the point — the agent's bound is the one meant to act, the kubelet's is what holds if it ever fails. The chart test asserts it as a relation to the constant, not as a number, so the two cannot drift into the wrong order.
• Every caller-supplied filename component goes through fileToken, not just the one known to be attacker-controlled. Which Kubernetes fields are validated is not this package's business to track, and "safe because that field happens to be DNS-1123" is the reasoning that made this a finding.
• write refuses a name that is not a plain filename. It catches nothing today; it is the choke point every payload passes through, and the next caller will not read the ADR.

Named, not solved

A crash-looping pod can still push useful payloads out of the spool: it produces OOM payloads faster than anything else is written, and oldest-first eviction drops older usage windows first. The data is loss-harmless, and the alternative — ranking payload kinds against each other during eviction — is a policy with no obvious right answer and no honest way to test it. Windowing OOM events the way restarts are windowed would remove the pressure at its source and changes a payload's shape, so it belongs in its own record.

Verification

go test -race ./..., make lint (0 issues), make chart-lint, go vet -tags e2e ./... clean.

make policy-e2e PASS 63.74s against the rebuilt image — the controller still writes every payload with the sweep now running each tick.

docs/adr/0042-the-spool-is-bounded.md amends 0007 §4 ("Retention is time-bounded, not size-unbounded"), with the mirror header. docs/security.md §6 Storage now states the three bounds in order, and says plainly that the earlier version of this did not work.

🤖 Generated with Claude Code