Repository navigation
imagecache: Verify layer content against its diffID before it enters the pool - #2279
Conversation
| // A remote layer's DiffID() is read from this same config, so | ||
| // the claim is checked against the content at unpack instead. | ||
| diffID := cfgFile.RootFS.DiffIDs[i] | ||
| dir, err := s.ensureLayer(gctx, diffID, layer) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – ensureLayer shares one flight per diffID, but each caller brings its own blob and only the leader's blob is read. Now that a content mismatch is an error, an image that lies about a common base layer's diffID fails every honest pull of that diffID running at the same time. The pool stays clean, but this lets one image break other images' pulls on the node. One fix is to retry once with the caller's own layer when layerSF.Do returns an error with shared set.
58db03b to
f1e10e9
Compare
|
igooch: can we get this one in? Seems close, do you need ben to review again? |
d86293b to
23c71ca
Compare
|
Greg Castle (@destijl) close, but it now depends on #1509 as the fix for Ben's comment needs #1509's pins. Could you or Benjamin Elder (@BenTheElder) review #1509? |
23c71ca to
0f0d1e4
Compare
0f0d1e4 to
bd55c41
Compare
Bowei Du (bowei)
added
area/security
fb2c437 to
15e2186
Compare
ced85b3 to
ce851eb
Compare
…the pool The layer pool is keyed by diffID and shared by every image on a node, but the diffID comes from the image config, which the image's author controls. Nothing checked that a layer's content matched it: the old comparison read a remote layer's DiffID() from the same config. So the first image to unpack a diffID decided what every later image with that diffID mounted, and an image could claim a common base layer's diffID to plant its own content under it. The compressed digest was not checked either. go-containerregistry verifies a blob against the manifest digest only when the stream reaches EOF, and untar stops at the tar end marker, before EOF. Unpack now hashes the uncompressed stream as it reads, drains it to EOF so the compressed digest check runs, and requires the hash to match the claimed diffID before the layer is renamed into the pool. On a mismatch the temp tree is discarded and the pull fails. The recorded size counts only what untar read, so trailing bytes cannot inflate the size GC accounts for. A mismatch is the image's, so it is not retried with each candidate credential. A layer's flight is now keyed by its blob as well as its diffID. A caller that joins a flight gets the leader's result, so with a diffID key an image lying about a common layer's diffID would fail every honest pull of that diffID in flight. With the blob in the key the lying image leads, and fails, only its own flight. Pulls of the same blob still share one. This relies on pins to keep eviction off a pull's layers; the flight is only deduplication. The on-disk layout is unchanged, so the layout version stays at 1. Layers unpacked before this change were never verified and stay in the pool until eviction retires them.
ce851eb to
9a3c261
Compare
|
#1509 merged with an outstanding nit today, reviewing this one again |
| A layer already in the pool is reused without re-hashing, because it | ||
| was checked when it was written. The reusing image's own blob is never | ||
| downloaded, so it is never checked. Layers unpacked by an atelet that | ||
| lacked this check stay in the pool until eviction retires them. |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This keeps trusting every layer a pre-fix atelet unpacked, and eviction never retires a layer that images keep using, so a poisoned base layer can outlive the fix. The issue thread settled on bumping the layout version so New refuses the old pool. #2392 also moves layoutVersion to "2", so whichever lands second needs "3".
There was a problem hiding this comment.
Technically we don't do back-compat yet, but we do very soon, and bumping this integer is ~free.
There was a problem hiding this comment.
this can technically be handled in a trivial follow-up, let's merge the bulk of it.
| diffID := cfgFile.RootFS.DiffIDs[i] | ||
| dir, err := s.ensureLayer(gctx, diffID, layer) | ||
| if err != nil { | ||
| return fmt.Errorf("while unpacking layer %s: %w", diffID, err) |
There was a problem hiding this comment.
🤖 question 🟢 – Should an errDiffIDMismatch be logged here at WARN with the image digest? Today the only trace is this error string returned through the Run/Restore RPC, and ate.imagecache.requests counts it as a plain error. It is the one signal of a lying image or a tampering registry.
There was a problem hiding this comment.
Added. pullWith logs the mismatch with ref, digest, and diffID. I left the metric alone as a new error.type value needs a registry change, so that's better as its own PR.
Layers in a version 1 pool were never hashed against their diffID, and eviction never retires a layer that images keep using. Bump the layout version so atelet refuses the old pool, and document how to rebuild a node's image cache. Log a diffID mismatch at WARN with the image. The RPC error was its only trace.
…gent-substrate#2979) Follow-up to agent-substrate#2279. @BenTheElder's last review asked for these, and they were pushed to agent-substrate#2279 before it merged, but main got the earlier commit. - Layout version moves to 2. Layers in a version 1 pool may not have been hashed against their diffID, and eviction never retires a layer that images keep using, so atelet refuses the old pool. - `pullWith` logs a diffID mismatch at WARN with ref, digest, and diffID. The RPC error was its only trace. ## Breaking change After an upgrade or rollback, atelet refuses the existing image cache. Before 1.0 a minor upgrade is a reinstall, so the upgrade runbook does not change. agent-substrate#2392 also moves the layout version to 2. Whichever lands second takes 3. Written with Claude Code.
Fixes #2082.
The bug
The layer pool is keyed by diffID and shared by every image on a node, but nothing checked a layer's content against its diffID:
DiffID()with the config's diffID, but go-containerregistry reads thatDiffID()from the same config, so the check could never fail.The fix
Breaking change
docs/upgrade.md).docs/upgrade.mdtext here matches Run containers as the image's USER and WORKDIR, with layer file owners kept #2392's.Not in this PR
A node that already has a verified layer reuses it without downloading the new image's blob, so an image whose blob doesn't match its claimed diffID can still succeed there. The same gap lets an image mount any layer already on the node by claiming its diffID, without a credential for the registry that served it. Checking each image's own blob on reuse is the follow-up.