feat(sbom): add kosli attest sbom - #1166
Conversation
Reports a software bill of materials as its own attestation type, using the reader added in #1165. The file is uploaded as it is, so the checksum recorded against it is the checksum of the file the customer supplied and they can verify it by hand. That is why only one file is allowed: the CLI tars and gzips two or more attachments, which would compress the SBOM and break that checksum. --sbom-file and --attachments are mutually exclusive for the same reason. Size is checked before the file is read. The reader loads the whole file in one go, so checking afterwards would pull a large file into memory before the friendlier error could be produced. The ceiling leaves room under the API's 10MB request limit, because the JSON payload is counted alongside the file. The format and the file checksum are recorded twice on purpose: inside attestation_data, where the server's schema can enforce them, and as annotations, which is where a reader sees them on the trail page. Both come from the same read of the same file, so they cannot disagree. A caller passing either annotation key gets an error rather than having their value silently replaced. The URL slug is the attestation family rather than the type. The server tells system types apart by type_name in the body.
|
Claude encountered an error after 6m 47s —— View job Review:
|
| Check | Result |
|---|---|
go build ./... |
clean |
go vet ./... |
clean |
go mod tidy + git diff go.mod go.sum |
no change |
I could not run the test suites — go test needs approval in this environment, and the command suites need the local server on localhost:8001 in any case. Everything below is from reading the code, plus checks I could run (sha256sum, grep over the audit tooling).
One thing I could verify directly: the pinned digest is correct. sha256sum cmd/kosli/testdata/sbom/cyclonedx.json → db09ef115d88e48a5ef553b21a88ccdc15b3df700e0b7c3736e2ef1024d26d9c, matching both original_fingerprint and sbom_sha256 in attestSbom_test.go:57,62.
Findings
1. hack/empty-flag-audit/spec.json has no attest sbom entry (inline) — the highest-value one, and a direct consequence of the last round's fix. Regenerating empty-flag-audit-coverage.json satisfied TestEmptyFlagAuditCoversEveryCommandAndFlag, but audit.py reads that same file and raises SystemExit for any covered command missing from spec.json (audit.py:650-688). The audit now refuses to run at all. Not CI-visible, which is exactly why it is worth catching here.
2. The cheap local file checks run behind the expensive work (inline) — --artifact-type docker fingerprints (possibly pulls) the image in CommonAttestationOptions.run before loadSbom discovers the path is a typo or a directory. Same reasoning that justified moving rejectReservedAnnotations to PreRunE. Note the hidden coupling flagged inline: a naive hoist of loadSbom silently drops both annotations, because processAnnotations replaces payload.Annotations wholesale.
3. The size ceiling's headroom is not what the comment claims (inline) — --user-data embeds an arbitrary JSON file into the same request body, so the 1MB margin can be consumed by something the guard never measures. Comment-level, not necessarily code.
Nit: attestSbomShortDesc ends with one trailing space; every sibling (attestSonar.go:33, attestSnyk.go:27, attestJira.go:40, …) uses two, which is a markdown hard break in the generated docs.
What is done well
The last three rounds landed real improvements and I could not find a way to break what they fixed:
loadSbomnow opens once, stats the handle, and reads throughio.LimitReader(file, max+1)— the bound is on memory rather than on a size measured earlier, andoriginal_fingerprint,sbom_sha256anddocumentprovably come from one buffer.TestSbomIsUploadedUncompressedis the right shape for the invariant the whole design rests on, and the two-attachment half is what keeps it from passing vacuously if packaging stopped happening. Thet.Cleanupon the tar is correct;getPathOfEvidenceFileToUploadremoves its own staging dir (cli_utils.go:684-688), so nothing leaks.- Pinning the fixture digest instead of
[a-f0-9]{64}is the difference between asserting the promise in--helpand asserting the shape of a hash. - Dropping the inert
--dry-runfrom the five error cases: those now exercise the path a real user hits. - The at-limit / over-limit pair, built from one
sizedSbomhelper withos.Truncate, pins the comparison as>rather than>=without allocating 9MB. ProcessSBOMas a bytes entry point withProcessSBOMFilerouted through it is the right factoring, and the pass-through hop is gone.
The two remaining reds (TestAttestSbomRoundTrip, plus the server type) are as the PR describes; the skip message now carries kosli-dev/server#6863, which makes the un-skip greppable.
· branch claude/1162-attest-sbom
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Addresses the automated review. The flag audit had no entry for this command, so a third test was failing for a reason unrelated to the server sequencing. Regenerated. The interesting half of this slice is now tested. Three dry-run cases assert the URL, the type name, the parsed format, the fingerprint, the package count and both annotations. They need a server for the flow and trail, but not one that knows the sbom type, so they stay green until the server side ships. Only the round trip is blocked now. The file was read twice, once to fingerprint and once to parse, so the recorded fingerprint could describe different bytes from the recorded summary. internal/sbom gains a bytes entry point and the command reads once. A path that is not a regular file is refused: a directory or a pipe reports size zero, walked past the size guard and was then read unbounded. The reserved annotation check moves to PreRunE. It needs nothing from the file, and a typo should not cost a repository walk and a pass over nine megabytes first. Test fixtures were byte-identical copies of ones in internal/sbom, and the gzip case duplicated an assertion made there. Dropped, and the two that remain are minimal documents. The oversize file is now made with truncate rather than by building nine megabytes of string. Every error golden here was wrong: errors from this command carry a "[kosli attest sbom flow= trail=]" prefix that I had not accounted for. All six were corrected against the command's actual output.
The six error goldens carried a "[kosli attest sbom flow=... trail=...]" prefix that the test harness never produces. The prefix comes from enrichError, which is only called from innerMain; the suite calls root.ExecuteC() directly, so the output is the bare message. The prefix was added last round after checking against the built binary, which does go through innerMain -- the wrong execution path to verify against. The size guard stat'd the path and then opened it again to read, so the bound it promised did not hold for a file still being written or a symlink repointed between the two calls. It is now one handle: stat it for IsRegular, then read through an io.LimitReader capped one byte past the limit and reject on the length actually read. The size rule lives in one place and the error message names the limit rather than an observed size that may already be stale. Also: - annotate no longer returns an error it cannot produce, now that the reserved-key rejection runs in PreRunE. - processSBOM is renamed ProcessSBOM, removing a pass-through with one caller. - attestSbomLongDesc names --attachments as unusable here, which the one-file rule implied but never said. - A file of exactly the limit is now tested from the accepting side, so the comparison is pinned as > rather than >=. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite runs against the current staging server image, which has no sbom system attestation type, so the POST comes back "System attestation type 'sbom' does not exist" and the job is red for a reason no change in this repo can fix. The skip states the condition instead. This is the only test that exercises the command against a real server, so un-skipping it is a precondition for merging, not optional cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The goldens matched the fingerprint against [a-f0-9]{64}, which is true
of the digest of anything: the tarred bundle, the payload, a copy with
the BOM stripped. The command's whole promise is that the recorded
checksum is the checksum of the file the customer supplied, and a test
on the shape of a hash cannot check that. Both goldens now pin the
fixture's real digest. Hashing different bytes turns them red; under the
old regex it did not.
Nothing tested the reason the command refuses a second attachment. One
attachment is uploaded as it is; two are tarred and gzipped while
sbom_sha256 still describes the uncompressed original, and nothing
downstream fails. The dry-run goldens cannot see it because a multipart
request logs only its JSON fields. There is now a direct test, which
also asserts that two attachments ARE packaged, so it cannot pass by
packaging having stopped altogether.
Also:
- Five error cases carried --dry-run that did nothing: each fails before
the request is built. It was not only noise. --dry-run is the mode
where those command lines exit 0 for a real user, so wantError held
only because the harness skips innerMain.
- A directory now says so, rather than reporting the generic
not-a-regular-file message a user then has to interpret. It is the
case reached by accident, when tab-completion stops a path short.
- The skip on the round-trip test names the server change it waits on,
so it is greppable and shows up in go test -v output.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| "trail": "string", | ||
| "user-data": "string" | ||
| }, | ||
| "attest sbom": { |
There was a problem hiding this comment.
Regenerating this file fixed the Go test and moved the breakage to the audit script. hack/empty-flag-audit/spec.json has no attest sbom entry (grep "attest sbom" hack/empty-flag-audit/spec.json → nothing), and audit.py reads this file to decide what it must cover:
# audit.py:650-688
def unaudited(spec):
covered = json.loads(COVERAGE.read_text())
for command, flags in sorted(covered.items()):
entry = spec.get(command)
if entry is None:
missing.append(f"{command} (no entry at all)")
...
def require_full_coverage(spec):
...
raise SystemExit(...)So the next ./hack/empty-flag-audit/audit.py exits with 1 command-and-flag combinations…: attest sbom (no entry at all) and writes no results at all. That is the script working as designed — its own message says "Add them to spec.json… If one of them is genuinely not worth auditing, say so in [the coverage file]" — but the second half of the pair is missing from this PR.
The entry is mechanical; attest snyk (spec.json:1045) is the closest template, because it is the existing example of a command whose baseline legitimately fails:
"attest sbom": {
"args": [],
"flags": {
"external-url": "probe=http://example.com",
"flow": "{flow}",
"name": "{name}",
"sbom-file": "cmd/kosli/testdata/sbom/cyclonedx.json",
"trail": "{trail}"
},
"baseline_ok": false,
"error": "System attestation type 'sbom' does not exist",
"flags_to_test": [ ...the 25 flags this file now lists... ],
"flag_values": { "sbom-file": "cmd/kosli/testdata/sbom/cyclonedx.json", ... }
}Recording baseline_ok: false with the server-type reason also parks the same sequencing fact in a second place that will need revisiting when kosli-dev/server#6863 lands, which seems right rather than redundant.
| return err | ||
| } | ||
|
|
||
| err = o.loadSbom() |
There was a problem hiding this comment.
The reserved-annotation check moved to PreRunE so a typo would not cost a repo walk and a pass over the file. The file's own cheap checks still sit behind the most expensive thing this command does:
CommonAttestationOptions.run → GetSha256Digest(args[0], …) (attestation.go:75-80), which for --artifact-type docker can pull and fingerprint an image. So
kosli attest sbom myimage --artifact-type docker --sbom-file typo.json …
pulls the image, then fails with failed to read SBOM file [typo.json]: no such file or directory. Same for the directory and oversize cases — the three errors that are pure local input.
Worth knowing before reordering, and worth a comment either way: loadSbom cannot simply move above line 179. annotate writes into o.payload.Annotations, and CommonAttestationOptions.run finishes by replacing that field wholesale (payload.Annotations, err = processAnnotations(o.annotations), attestation.go:116), so a hoisted loadSbom would have both annotations silently discarded. The nil-guard inside annotate makes it read as order-independent when it is not.
Two ways out, both small:
- split the cheap guards out (
open/Stat/IsDir/IsRegular, returning the open handle) and call that fromPreRunE, leaving the read andannotatewhere they are; or - leave the order and say in a comment on
annotatethat it must run afterprocessAnnotations.
The dry-run goldens would catch the dropped annotations, so this is about not setting the trap rather than about an undetected break.
| // The API rejects a request body over 10MB, and the JSON payload is counted | ||
| // alongside the file, so this leaves room for it rather than sitting on the | ||
| // limit. Lifting the ceiling needs direct-to-S3 upload, tracked separately. | ||
| const maxSbomFileBytes = 9 * 1024 * 1024 |
There was a problem hiding this comment.
The 1MB of headroom is sized against "the JSON payload", but the JSON payload is not bounded: --user-data embeds an arbitrary JSON file into the same body (payload.UserData, err = LoadJsonData(o.userDataFilePath), attestation.go:104), and --annotate, --external-url and the commit info ride along with it.
So --sbom-file 9mb.json --user-data 2mb.json passes this guard and still comes back as the bare 413 the guard exists to avoid — the one case where the friendlier local error is most wanted, since the user now has two files to suspect.
Not necessarily worth code: the fix would be measuring the marshalled payload after building it, which is a different check in a different place. But the comment currently reads as if the 10MB limit is accounted for, and it is only accounted for on the file side. One clause ("…the JSON payload, whose size --user-data can raise past this margin") would stop the next reader trusting it further than it goes.
1 participant
Slice 2 of #1162. Adds the
kosli attest sbomcommand, using the reader merged in #1165.Draft, and it cannot pass CI yet. See "Why CI is red" below. The reason is a sequencing one the ticket already sets out, not a problem with the change.
What it does
Reads the SBOM, records what it says about itself, and uploads the file as an attachment.
Why only one file
The file is uploaded as-is, so the checksum recorded against it is the checksum of the file the customer handed us. They can verify it by hand.
That only holds for a single file. The CLI tars and gzips two or more attachments before upload, which would compress the SBOM and break the checksum that was just recorded for it. So
--sbom-fileand--attachmentscannot be used together.An already-compressed file is also rejected, because the format and the summary have to be read out of it.
Why the read is bounded rather than the file measured
The reader loads the whole file in one go, so the size has to be limited somewhere. An earlier version stat'd the path and then opened it separately to read, which does not actually bound anything: a file a build is still writing grows between the two calls, and a symlink can be repointed at a bigger file.
It now opens the file once and reads through an
io.LimitReadercapped one byte past the ceiling, then rejects on the length actually read. One byte past, so a file of exactly the ceiling is accepted and anything larger is not.The ceiling is below the API's 10MB request limit rather than equal to it, because the JSON payload is counted against that limit alongside the file. Raising the ceiling needs direct-to-S3 upload, which is kosli-dev/server#6536.
Why the format and checksum are recorded twice
They go inside
attestation_data, where the server's schema can enforce them, and again as thesbom_formatandsbom_sha256annotations, which is where a reader sees them on the trail page.Holding one value in two places is usually a mistake. Here both come from the same read of the same file in the same pass, so they cannot drift apart: if the CLI is wrong they are wrong together rather than disagreeing.
Passing either key through
--annotateis an error rather than being silently overwritten.Why the URL says "system"
The slug is the attestation family, not the type. The server tells system types apart by
type_namein the body, which issbomhere.Why CI is red
One test reports an SBOM to a real Kosli server. CI runs against the current staging server image, which does not know what an
sbomattestation is until https://github.com/kosli-dev/server/pull/6863 merges and deploys, so the POST comes back "System attestation type 'sbom' does not exist".That test is skipped for now with the condition written into the skip message. Un-skipping it is a precondition for merging, not optional cleanup — it is the only test that exercises the command against a real server. Everything else in the file uses
--dry-runand passes today.The ticket already requires this order: the command must not be released until the server side is in production, because a released command pointed at a server that does not know the type fails for every customer who tries it.
What was run
Every test in the file except the skipped round-trip, through the same harness CI uses, with the environment the Makefile sets (
KOSLI_TESTS=trueplus the fake GitHub CI variables). All pass.That environment matters. Without those variables a
[warning] Repo information will not be reportedline lands ahead of every error message, and each exact-match golden fails on it. A barego testtherefore looks broken when nothing is.The full local stack still cannot run here: it wants a linux/amd64 server image, and building the server on an arm64 Mac produces an arm64 one the compose stack refuses. The tests that need a live server are the round-trip test, which is skipped anyway.
Also run:
go build ./...,go vet ./...,gofmt,golangci-linton the changed packages, andkosli attest sbom --help.Each new guard was mutation-tested — the size comparison flipped from
>to>=, the+1dropped from the read limit, the regular-file check removed, and the annotation call removed. Every one of those turns a test red, so none of the tests are decoration.