feat(attest-sbom): add kosli attest sbom - #1168
Conversation
|
Claude finished @AlexKantor87's task in 3m 47s —— View job Review:
|
| Check | Result |
|---|---|
go build ./... |
pass |
go vet ./... |
pass |
go mod tidy + git diff --exit-code go.mod go.sum |
clean |
Findings
Both are in cmd/kosli/root.go — the shared machinery this PR touches — and neither is a live bug.
1. viperAppliedFlags[f.Name] = true is still unconditional (root.go:854). The reply to the last round said it had been moved to record only "after Set returns without error". It hasn't: the assignment sits outside the if err != nil block, so a refused Set is still recorded as applied-from-config. Unreachable today (bindErr aborts the run at root.go:727 before any PreRunE reads the map), so this is the invariant not matching the doc comment on line 777 rather than a defect. One-line fix suggested inline.
2. bindFlags's doc comment is now orphaned (root.go:775-781). The new var and helper were inserted between that comment and the function, so lines 775-776 read as the opening of the viperAppliedFlags comment and godoc attaches the block to the var. bindFlags ends up undocumented.
Nothing else new surfaced. The findings from the earlier rounds all check out as addressed in the tree as it stands: file-bytes carries the hashed bytes end to end, os.Stat precedes os.Open, both reserved annotation keys have their own case, --attachments is hidden and the prose no longer names it, and the default: case in createMultipartRequestBody closes the silent-skip. I re-ran the "every Type spelling" check behind that last one — the Type: "env" hit is PolicyPayload.Type in testHelpers.go:609, not a FormItem, so the conclusion holds and no existing caller is caught by the new error.
One observation, not a request: sbom.ProcessSBOMFile now has no production caller (attestSbom.go uses ProcessSBOM), so the 35-case parser suite in internal/sbom exercises a wrapper the product no longer calls. It's a two-line delegate, so the risk is negligible — worth knowing rather than changing.
Worth calling out
- The read → hash → parse → upload path is now genuinely single-pass. Handing
requestsaFileBytesinstead of a path is the right shape for the invariant, andTestSbomUploadsTheBytesItHashedasserts it at the seam that matters:sha256ofform[1].Content.Dataequals bothoriginal_fingerprintandsbom_sha256, with a literal digest as an anchor so an empty buffer can't pass. - The fifo test earns its keep. The
selectwith a timeout makes the stat-before-open ordering a red run rather than a package hang, which is the rare case of a test that catches a regression a plain assertion couldn't. - Passing the bytes as a parameter instead of a field turns "the body cannot be built before the file is read" into a compile error rather than a runtime guard. That's the stronger version of the fix that was asked for.
- The
--attachmentssource attribution is careful in the direction that's easy to get wrong: it stays silent when the flag was typed, because the env value was never applied, and both branches have a test.
CI
PR checks is green; Main was still running at review time. The round-trip test's deliberate red is well argued in the PR body, and the stated plan — hold as draft until server#6863 deploys, so main never carries a failing required check — is the right resolution.
· branch claude/1162-attest-sbom-command
74c192f to
534a1fc
Compare
534a1fc to
00f824c
Compare
Reports a software bill of materials to a trail or an artifact as its own system attestation type, so large SBOMs no longer have to be squeezed into a custom type or attached to a generic attestation. Slice 2 of #1162, building on the reader in internal/sbom. The bytes are passed from loadSbom to the request builder as a value rather than held on the options struct, so a body cannot be built before the file has been read: that would be a well-formed request carrying no attachment and a fingerprint of nothing. The command reads the file once and everything comes from that one buffer: the SHA-256 that becomes original_fingerprint and the sbom_sha256 annotation, the parsed summary (format, creation time, tools, subject, package count) that becomes attestation_data, and the upload itself. The uploader is handed the bytes, through a new file-bytes form item in internal/requests, rather than a path it would open and read a second time. That is what makes the recorded checksum describe the bytes the server receives even for a file a build is still writing. The file is uploaded as it is, so a user can verify the checksum by hand. That only holds for exactly one attachment: two or more are tarred and gzipped before upload. So --attachments is refused and hidden from help, since with --sbom-file required the pair can never be satisfied. A value can still arrive from KOSLI_ATTACHMENTS or a config file without anyone typing the flag, and the flag is no longer in help to be found, so in that case the refusal names the source. bindFlags now records which flags it filled in from viper; cobra marks typed and configured flags Changed alike, so that record is the only way to tell them apart and to avoid blaming a variable that was never applied. A gzipped input is refused for the same reason as a second attachment: the format is read from the bytes. internal/requests refuses a form item of unknown type instead of skipping it, since a skipped part would post a body claiming a checksum for an attachment that is not there. The path is stat'd before it is opened, because open(2) on a fifo with no writer blocks and a check after it would never run; a directory gets its own message since it is what tab-completion stopping short produces. The read is bounded by an io.LimitReader one byte past a 9MB ceiling, chosen below the API's 10MB request limit to leave room for the JSON payload. annotate builds a new map rather than writing into payload.Annotations, because that field aliases the --annotate flag map that the reserved-key check reads; writing into it would make a second run of the same options fail for a key the user never supplied. Tests pin the fixture's real digest rather than matching the shape of a hash, assert that the bytes handed to the uploader hash to the recorded fingerprint, cover both reserved keys and the caller's own annotations surviving the merge, and assert both sides of the size boundary. TestAttestSbomRoundTrip reports to a real server and fails until the server carries the sbom type; that is the intended signal and it is not skipped. The empty-flag audit gains an attest sbom entry, generated by teaching bootstrap.py the fixture path. bootstrap.py now writes the indentation and trailing newline the committed spec.json already uses, so a regeneration no longer rewrites the whole file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
00f824c to
8bcd8d5
Compare
Slice 2 of #1162. Adds the
kosli attest sbomcommand, using the reader merged in #1165.Replaces #1166, which went through five review rounds and eight commits. This is the same code squashed to one commit, with one deliberate change: the end-to-end test is no longer skipped, so CI is red on purpose until the server side is on staging. See "Why CI is red".
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, and since--sbom-fileis required the pair can never be satisfied:--attachmentsis refused and hidden from--help.An already-compressed file is also rejected, because the format and the summary have to be read out of it.
Why the uploader is handed bytes, not a path
The reader loads the whole file in one go, so the size has to be limited somewhere, and the checksum has to describe what the server receives. Two earlier versions each fixed half of this and moved the race one step: first the file was stat'd and then opened separately (a file still being written grows in between), then it was read once but the uploader was given the path, which
internal/requestsopens and reads again when it builds the request body.Now there is one read. The path is stat'd first (a fifo with no writer blocks on
open, so a type check after opening would never run), then read through anio.LimitReadercapped one byte past the ceiling, and that same buffer produces the fingerprint, the parsed summary and the upload.internal/requestsgains afile-bytesform item for exactly this. A test hands the built request body tosha256and checks it equalsoriginal_fingerprint; the round-trip against a local server shows the storedevidence_archive_fingerprintequal to the fixture's digest.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. The margin is a guess at a small payload, not a bound:
--user-datarides in the same body and can push the total past 10MB on its own. 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
TestAttestSbomRoundTripreports an SBOM to a real server. CI runs against the current staging server image, which does not know what ansbomattestation is until https://github.com/kosli-dev/server/pull/6863 merges and deploys, so the POST comes backSystem attestation type 'sbom' does not exist.That failure is accurate and is left in place. The previous PR skipped this test to get a green tick, which made CI say "nothing is broken" while the one test that proves the command works was not running. Green here should mean the feature works, so this stays red until it does.
Everything else in the file passes today: the
--dry-runcases prove the payload the command builds, the error cases need no server, and the packaging test asserts local logic only.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
The full CLI suite, locally, against a server image built from the branch behind server#6863. That server knows the
sbomtype, so the round-trip test runs for real there: 2667 tests, two failures, bothTestAttestGitlabPRCommandTestSuiteand both reproduced identically on a clean clone ofmain, so they are the environment and not this change.The stored attestation was then read back from the server's database.
original_fingerprint, thesbom_sha256annotation andevidence_archive_fingerprintall equal the SHA-256 of the fixture on disk, which is the whole promise of the command.Also run:
go build ./...,go vet ./...,gofmt,golangci-linton the changed packages, the empty-flag audit for this command (all 25 flags refuse an empty value), andkosli attest sbom --help.Every guard has been mutation-tested: each of the size comparison, the read limit, the directory check, the gzip rejection, the mutual exclusion with
--attachments, both reserved annotation keys, the annotation merge, the fingerprint, and the bytes handed to the uploader turns a test red when broken.How to run the suite locally on an arm64 Mac is written up in the AlexKantor tracker under
process/cli-local-test-environment.md.🤖 Generated with Claude Code