Add container/signer for cosign key-based OCI signing - #230
Conversation
Sigstore verification lives here already; signing lived in toolhive, so the two halves of one trust model sat in different repositories and the signer's round-trip test had to hand-roll its own verifier to check its own output. Port the package from toolhive next to container/verifier. The move is mechanical — it imported nothing from toolhive, and every dependency was already a direct one here. The round-trip test now verifies through VerifyBundleOfflineWithKey rather than a reimplementation, so it exercises the path a consumer actually takes.
Signing could not read any key produced by the cosign CLI, and the tests did not catch it: the fixtures used cryptoutils, which writes PKCS#8 standard encryption under the same "ENCRYPTED SIGSTORE PRIVATE KEY" label cosign uses for a payload sealed with scrypt and nacl/secretbox. Found by verifying a real thv-signed artifact with the stock cosign CLI, which is the interop claim this package's documentation makes.
|
Pushed a second commit: the ported package could not read keys from Found by signing a real artifact with Both formats are accepted now, with a wrong password reported as such instead of as an ASN.1 parse error. New tests build fixtures with The same fix is in stacklok/toolhive#6139, which ships first; this keeps the two in step so the eventual move does not regress it. |
container/signer links sigstore-go's signing entry point, which pulls in golang.org/x/crypto/openpgp transitively and trips GO-2026-5932. The advisory has no fixed version and never will — the package is deprecated by design. Adopt the same exclusion, with the same justification, that stacklok/toolhive already applies for the identical dependency, and fail the job on any vulnerability that is not on the list.
|
Third commit, and this one touches a security workflow so it deserves explicit attention. Adding It is genuinely introduced by this PR — stacklok/toolhive already excludes this exact advisory with the same justification, because it consumes the same sigstore packages. Core had no exclusion mechanism at all — the action's result was the whole gate — so I added one modelled on toolhive's: parse the JSON findings, ignore only documented IDs, and fail on anything else. Net effect is stricter than before for every other advisory, since the job now explicitly errors on unexcluded findings rather than relying on the action's exit code alone. Worth a reviewer's judgement on whether you want that mechanism here, or would rather solve it another way. The alternative I can see is not linking |
"Remove when sigstore drops the dependency" is not something anyone can check. Rekor already made the change — sigstore/rekor#2883, merged after v1.5.3 — so the trigger is a release containing it, and the note now says so along with the full import chain and why the traces are init-only.
|
Tightened the exclusion's justification after digging into your question about swapping in ProtonMail's library. Short answer: we cannot, and Nothing here imports openpgp — But the question surfaced something useful: rekor has already done it. sigstore/rekor#2883 migrated to So the exclusion's expiry is now a real condition rather than "when sigstore drops the dependency": bump rekor past v1.5.3 once a release contains the migration. The comment says that, plus the import chain and the fact that every govulncheck trace is package-init reachability rather than a call — which is the part that actually makes the exclusion defensible. Tracked in #231, and stacklok/toolhive#6287 for the same exclusion there. |
… digest Three review findings on #230. Signing built the manifest from empty.Image and wrote it to the .sig tag, so a second signer silently deleted the first signer's trust material. Append to the existing manifest instead, treating only a genuine "absent" answer from the registry as empty. Re-signing with the same key is a no-op, detected by verifying existing layers against the public key — comparing signature bytes cannot work, because ECDSA is randomised. The vulnerability gate could pass on a broken scan: `|| true` covered the whole pipeline, so a jq parse error, an empty file, or a missing file all produced an empty finding list indistinguishable from a clean result. Make parse failures fatal, tolerate only grep's no-match status, and fail when the scanner errored without reporting anything. SignOCI returned bytes whose signed message is the simple-signing payload digest, not the artifact digest callers pass in — so the documented offline-verification flow could not work from the public API alone. Return both, and add PayloadDigest for consumers holding only a reference and a digest.
The trailing `|| true` covered the whole pipeline, so a jq parse error, an empty output file, or a missing one all produced an empty finding list — indistinguishable from a clean scan, and the gate reported success. Verified against malformed, empty, and missing output: all three passed before, all three now fail. A genuinely clean scan and a real unexcluded finding both keep their previous verdicts. Found in review of stacklok/toolhive-core#230, whose equivalent step was modelled on this one.
Signing built the manifest from empty.Image and wrote it to the .sig tag, so signing an artifact a second time deleted the first signature — trust material that may belong to another signer. This PR makes signing the default on push, which makes the overwrite reachable. Append to the existing manifest, treating only a genuine "absent" answer from the registry as empty. Re-signing with the same key stays a no-op, detected by verifying existing layers against the public key: ECDSA is randomised, so comparing signature bytes never matches. Found in review of stacklok/toolhive-core#230, which carries the same fix for the copy being moved there.
Signing built the manifest from empty.Image and wrote it to the .sig tag, so signing an artifact a second time deleted the first signature — trust material that may belong to another signer. This PR makes signing the default on push, which makes the overwrite reachable. Append to the existing manifest, treating only a genuine "absent" answer from the registry as empty. Re-signing with the same key stays a no-op, detected by verifying existing layers against the public key: ECDSA is randomised, so comparing signature bytes never matches. Found in review of stacklok/toolhive-core#230, which carries the same fix for the copy being moved there.
* Sign pushes by default and remove the lock feature gate The final RFC THV-0080 piece: thv skill push signs the pushed artifact with the provided cosign key (attaching the signature manifest next to it, so installs can verify), and pushing unsigned requires an explicit --no-sign — a failed signing fails the push rather than silently publishing unsigned. With signing on publish and verification on consume both in place, the TOOLHIVE_SKILLS_LOCK_ENABLED gate comes out: the lock file, sync, upgrade, dependency materialization, and signature verification are now standard behavior for project-scoped skills. The architecture document's trust-model section graduates from the drift-detection honesty note to the verified model — TOFU semantics, the explicit allow_unsigned / allow_signer_change escape hatches, the provisional git marker, and what deliberately remains trusted on faith. Closes #5899. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Push E2E artifacts with an explicit no-sign Signed-by-default pushes turned every E2E push into a 400: the suite has no signing infrastructure, so its pushes carry the explicit no_sign decision — matching the allow_unsigned exceptions its project-scoped installs already record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Accept the key format cosign generate-key-pair writes thv skill push --key could not read any key produced by the cosign CLI. It failed with an opaque ASN.1 error, and no test caught it: the fixtures built keys with cryptoutils, which writes PKCS#8 standard encryption under the same "ENCRYPTED SIGSTORE PRIVATE KEY" label that cosign uses for a payload sealed with scrypt and nacl/secretbox. Same label, different bytes. Decrypt the cosign sealing when the label says so, falling back to the PKCS#8 form, and report a wrong password as such rather than as a parse failure. Also document where COSIGN_PASSWORD is read: signing happens in the API server, so the variable belongs in that process's environment, not the shell running the CLI. * Append signatures instead of replacing them Signing built the manifest from empty.Image and wrote it to the .sig tag, so signing an artifact a second time deleted the first signature — trust material that may belong to another signer. This PR makes signing the default on push, which makes the overwrite reachable. Append to the existing manifest, treating only a genuine "absent" answer from the registry as empty. Re-signing with the same key stays a no-op, detected by verifying existing layers against the public key: ECDSA is randomised, so comparing signature bytes never matches. Found in review of stacklok/toolhive-core#230, which carries the same fix for the copy being moved there. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Sigstore verification already lives here, in
container/verifier. Signing lived in toolhive (pkg/skills/signer), so the two halves of one trust model sat in different repositories — with a visible cost: the signer's round-trip test could not use the real verifier, and hand-rolled a ~20-line equivalent instead, carrying the comment "used directly here until the core release this stack's verifier PR consumes".This ports the package to
container/signer, next to its counterpart. It was written to be moved and that held up:go.modis untouchedcontainer/verifierdoes not reference the signerWhat it does: signs an OCI artifact following the cosign key-pair convention — a simple-signing payload binding the manifest digest is signed with the user's key, attached to the registry as a cosign signature manifest at the
sha256-<hex>.sigtag, and returned as a serialized Sigstore bundle for durable storage and offline re-verification. Keyless (OIDC) signing is not implemented;ErrKeyRequiredis returned instead.Three small generalizations, since this is now a library rather than skills-specific code:
container/verifierErrKeyRequiredno longer names a--keyCLI flag that does not exist at this layer; its doc comment tells CLI callers to wrap it with their own flag name--keyflag" now say "caller input (typically a CLI flag)"Test plan
task test)task lint-fix)All nine existing tests ported and passing, plus the whole
container/...tree green.The round-trip test is now meaningfully stronger. It previously verified signer output against a locally reconstructed verifier; it now calls
verifier.VerifyBundleOfflineWithKey, so sign → verify is proven against the entry point consumers actually use, in one repository:Coverage retained: round trip against a live in-process registry, wrong-key rejection, encrypted-key handling via
COSIGN_PASSWORD, missing-key error, digest normalization, payload/digest binding, keypair metadata, andresolveKeyPathpath vetting.Notes for reviewers
Nothing is removed from toolhive in this PR. Its
pkg/skills/signerstays until this lands and ships in a release, because an in-flight toolhive PR (stacklok/toolhive#6139) still modifies that package. Deleting it now would conflict. The toolhive-side removal is a follow-up gated on both.resolveKeyPathis worth a look — it is the sanitization barrier for a user-supplied key path (absolute, cleaned, symlinks resolved, regular-file checked) and carries the//nolint:gosecthat depends on it. It was written to satisfy CodeQL's path-injection analysis in toolhive; flag it if this repo wants a different approach.Generated with Claude Code