Shared module-proxy cache: cleanup config ignored, cache unbounded, traversal untested

[luthermonson]

Summary

Follow-up to the container-breakout review (#124, #125, #126, #127). The open question was whether the dependency caches are keyed per-job or shared across jobs, and if shared, whether one job can influence what a later job downloads.

Answer: shared across every job on the node, by design — and that's the right design. The integrity story mostly holds up, but it holds up for a reason that isn't written down anywhere, and there are two concrete defects.

Scope note: only the Go module proxy exists on main (pkg/proxies/go/). Cargo and Composer proxies are on the unmerged feat/cargo-composer-cache branch; whatever is decided here should land there before it merges, since the same shape applies.

How it's keyed

One goproxy.Proxy per daemon, listening on the bridge gateway (cmd/ephemerd/main.go:442-448), with a single cache dir at <data-dir>/cache/gomod. Every container gets GOPROXY=http://<gateway>:<port>,direct (pkg/proxies/go/goproxy.go:126-130).

The cache key is the request path and nothing else (goproxy.go:273-278):

clean := strings.TrimPrefix(urlPath, "/")
h := sha256.Sum256([]byte(clean))
prefix := fmt.Sprintf("%x", h[:2])
return filepath.Join(p.cfg.CacheDir, prefix, filepath.FromSlash(clean))

No job, repo, or owner identity participates. A module fetched by a job for repo A is served to a job for repo B.

Why that is not (currently) a poisoning primitive

Worth recording explicitly, because it is load-bearing and undocumented:

1. Jobs cannot write the cache. They speak HTTP; only the daemon writes files. The package comment already says this and it is accurate.
2. Content always comes from upstream. cacheAndServe fetches p.cfg.Upstream + r.URL.Path and stores that response under the key derived from that same path (goproxy.go:160-181). Key and content cannot disagree — a job can choose which key gets populated, but not what goes in it.
3. Go authenticates modules itself. go.sum plus the checksum database is what actually makes a shared module cache safe, and sumdb requests are correctly passed straight through, never cached (goproxy.go:138-142). A mismatched .zip fails verification in the client.
4. Mutable endpoints aren't cached — /@v/list and /@latest pass through (goproxy.go:145-148), so a job can't pin another job's view of "latest".

Point 3 is the whole ballgame, and it's an assumption about the client, not a property of this proxy. A job that sets GOFLAGS=-mod=mod, GONOSUMDB=*, GOPRIVATE=*, or GONOSUMCHECK=1 opts itself out — that only harms itself, but it means the proxy must never be described as "safe because it verifies", since it verifies nothing.

Defect 1 — cleanup = false is silently ignored

cmd/ephemerd/main.go:437-440:

cleanup := cfg.ModuleProxy.Cleanup
if !cleanup {
    cleanup = true
}

Cleanup is a plain bool (pkg/config/config.go:383, documented "wipe cache on shutdown (default true)"). This collapses to cleanup = true unconditionally: false is indistinguishable from unset, and the branch that was meant to supply the default instead overrides the operator.

Consequences: the entire module cache is wiped on every daemon stop, so it never survives a restart or an ephemerd_version bump — and an operator who explicitly sets cleanup = false gets no signal that their setting did nothing.

Fix is the established pattern in this repo: make it *bool and add ResolvedCleanup() defaulting to true, mirroring DindConfig.AllowPrivileged / RuntimeConfig.AllowNewPrivileges.

Defect 2 — the cache is unbounded

There is no size cap, no eviction, and no prune pass. Compare DindConfig.CachePruneInterval / CacheMaxAge (config.go:266-277), which the image cache has.

Because the cache is shared and any job can request arbitrarily many module versions, one job can fill the node's disk and degrade every subsequent job on that node. This is the only cross-job impact I can actually construct, and it is availability, not integrity.

Defect 1 currently masks this — the wipe-on-shutdown means the cache is bounded by daemon uptime in practice. Fixing Defect 1 without adding eviction would turn a masked problem into a live one, so these should land together.

Gap — path traversal in the cache key is untested

cachePath joins the decoded r.URL.Path under the cache dir. filepath.Join cleans, so a .. segment traverses; the sha256 prefix doesn't help, since it's derived from the same attacker-influenced string.

In practice http.ServeMux cleans unclean paths and 301s before the handler runs, and on the read path http.ServeFile independently rejects r.URL.Path containing ... So I could not construct an escape. But the write path's safety rests entirely on ServeMux's redirect behavior, which is an implicit dependency on net/http internals with no test pinning it.

Worth a table test hitting the handler directly with .., encoded %2e%2e%2f, absolute-ish paths, and a very long path, asserting the resulting cachePath stays under CacheDir. Cheap, and it converts an implicit assumption into a checked one.

Suggested order

1. Traversal test (pure test, no behavior change).
2. Size cap + eviction for the module cache.
3. *bool fix for cleanup, landing with (2).
4. Apply the same three to cargo/composer before feat/cargo-composer-cache merges.

Per-job cache keying is explicitly not recommended — it would destroy the hit rate the proxy exists for, and the sharing is not where the risk is.