fix: reject image references that cannot be reported safely - #102
Conversation
2e81af6 to
c6b29ff
Compare
|
Rebased onto images, err := processor.ProcessFile(filePath)
// Not `continue`: a file that failed on one document may still have
// yielded images from the others...
failures += logErrors(logger, err)
failures += printImages(stdout, logger, filePath, images)So document-level failures (#103) and unreportable image values (this PR) both count toward the exit code, and neither discards what the other found. Worth checking the case only the combination produces — a stream with a malformed document and a bad image reference: $ kir both.yaml
registry.k8s.io/nginx-slim:0.8
after-the-break:1.0Exit 1, both failures named separately, and both reportable images survive — including the one from the document that also held the bad reference, and the one after the unparseable document. Re-ran the full checklist after the rebase: Generated by Claude Code |
c6b29ff to
75640bd
Compare
kir's stdout is a contract — one image per line, normally piped straight into a scanner's arguments — but every byte of it comes from a manifest kir did not write, and nothing checked those bytes. An image value holding a line break forged an extra entry in the list. One holding escape sequences could make a terminal display a registry the scanner was never given. One starting with a dash reached the scanner as an option rather than an operand. Values no registry could serve at all — an empty tag, an unrendered Helm template, an unexpanded shell variable — were reported as images. All of it passed with exit 0. Such a value is now reported on stderr and counted against the exit code, the same shape ADR 0008 gives a malformed document, while the other images in the document are still printed. Validation is delegated to distribution/reference, the canonical parser, rather than to hand-written rules: kir then accepts exactly what a registry client would, so anything it admits is pullable and anything it refuses was never an image. Only validation is borrowed, not normalisation — kir reports what the manifest said, so "nginx" stays "nginx". The parser quotes the offending value back in its own message, so that message is escaped rather than interpolated: a reference carrying escape sequences must not repaint the terminal it is reported on.
75640bd to
e4aa27c
Compare
Validates image references before reporting them, so
kir's output can be trusted by whatever consumes it.fix:, so merging bumps the version.Problem
kir's stdout is a contract: one image per line, normally piped straight into a scanner's arguments. Every byte of it comes from a manifestkirdid not write, and nothing checked those bytes. Againstmaster, all of this printed with exit 0:^[[2K^Mis erase-line plus carriage return: an operator readsregistry.io/trusted:safewhile the scanner is handed thenginx:1.0…value.xargs syftpasses--platform=linux/amd64as an option, so a manifest author reaches into the scanner's argv.Change
A new
imagerefpackage holds the rule;cmdapplies it where stdout, stderr and the exit code meet. An unreportable value gets ADR 0008's treatment — named on stderr, counted against the exit code — without discarding the images beside it that were fine.Validation is delegated to
distribution/reference, the canonical parser.kirthen accepts exactly what a registry client would, so anything it admits is pullable and anything it refuses was never an image — which is what makes refusing safe.Two deliberate limits on what's borrowed:
kirreports what the manifest said, songinxstaysnginxrather than becomingdocker.io/library/nginx.strconv.Quote.Why not hand-written rules
An earlier revision of this PR did exactly that — reject control characters, whitespace, a leading dash, empty — on the theory that a full grammar risks rejecting an unusual-but-legitimate reference, and that a dropped image is worse than a bad one printed. I tested that theory against the library and it didn't hold:
ParseNormalizedNamednginx:/{{.Values.image}}/$IMAGENo over-rejection on anything real, and the hand-written version had a genuine hole. The library is strictly better here.
One trap worth knowing
While comparing them I saw the library reject every digest-pinned reference with
unsupported digest algorithm, and nearly concluded it over-rejects. The cause was my test harness missingimport _ "crypto/sha256"— go-digest resolves sha256 only when that hash is linked in.So
imageref.gocarries that blank import. To be precise about what it does and doesn't buy, all measured rather than assumed:kirlinkscrypto/sha256today anyway, through the Kubernetes libraries, so the import changes nothing at present — digest-pinned images work without it.imagerefself-sufficient: with it,crypto/sha256is in the package's own dependency closure; without it, it isn't. That matters because the candidate answers to Dynamically findPodSpecin manifests #26 dropclient-go, which is what currently links the hash.crypto/sha256whateverimagerefimports, so removing the import keeps the suite green. It's held in place by a comment instead, which is stated as such rather than dressed up as a guard.Cost
Two new modules:
github.com/distribution/referencev0.6.0 andgithub.com/opencontainers/go-digestv1.0.0 (4go.sumlines). Neither was already in the tree — I checkedgo list -m allandgo mod graph— so this is a genuine addition, and it cuts slightly against #84's goal of shrinking the dependency set. Both are small and canonical, but the trade is real and yours to weigh.Tests
imageref: 14 rejection cases and 12 references that must keep passing, including two digest-pinned ones.imageref: a test asserting the error message carries no raw\x1b,\ror\n— the parser embeds the value, so this pins the escaping.cmd: drivesRunend to end and asserts stdout carries only the reportable image, exit is 1, and stderr has no raw escape byte.Hostile inputs live in Go source rather than an approvals fixture, per AGENTS.md — a checked-in
.yamlwould have its escapes and trailing whitespace normalised, and the golden would stop guarding anything.Every existing golden passes unchanged, including #103's
TestFailure/PartialStream. Verified the two fixes compose: a stream with both a malformed document and a bad reference reports each failure separately, exits 1, and still prints both reportable images.Checklist
gofmt -l .empty ·go vet ./...clean ·go test ./...and-racegreen ·go mod tidyno drift on the committed tree.