Verify key-signed skill installs against a public key - #6447
Conversation
samuv
changed the title
github-actions
Bot
added
size/XL
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## t3code/key-verify/02-lock-public-key #6447 +/- ##
=======================================================================
Coverage ? 78.08%
=======================================================================
Files ? 768
Lines ? 74528
Branches ? 0
=======================================================================
Hits ? 58194
Misses ? 16329
Partials ? 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
The install path now reaches key verification, but the online verifier does not bind the verified signature payload to the artifact digest being installed. CI is green, but this trust-boundary issue must be fixed before merge.
| http.StatusUnprocessableEntity, | ||
| ) | ||
| } | ||
| result, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, ref, digest, pubKeyPEM) |
There was a problem hiding this comment.
This newly activates VerifyOCIWithKey, but that method only verifies each retrieved bundle against b.DigestAlgo:b.DigestHex; for cosign this is the digest of the simple-signing payload layer, not the requested artifact digest. Unlike VerifyBundleOfflineWithKey, it never reconstructs signer.PayloadDigest(ref, digest) or otherwise checks that the signed payload names this artifact. A registry attacker can therefore copy artifact A's valid signature layer into artifact B's sha256-<B>.sig manifest and have B accepted under A's trusted key. Please bind each online candidate to the expected ref/digest before accepting it, and add a regression test that attaches A's signature to B and expects verification to fail. (CWE-345/CWE-347)
5201699 to
d9b0cb4
Compare
github-actions
Bot
added
size/XL
| keyAnchor, err := resolveKeyAnchor(opts, skillName, expected, expectUnsigned, catalogExpected) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This enables the public-key verification path, but a true first install of a key-signed artifact that omits --public-key still falls through the keyless path and reaches the unchanged keySignedInstallError, which says key-pair signatures cannot be verified and tells the user to republish keylessly. The skill push --key help and architecture docs repeat that now-obsolete guidance. Please make this fallback tell the user to retry with --public-key, update the push help/docs to describe the supported flow, and regenerate the CLI docs; otherwise the most likely missing-anchor error directs users away from the feature added here.
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Re-reviewed the current rebased head against its declared stacked base. The existing digest-binding blocker remains unresolved. I also found that the newly supported first-install flow still tells users who omit --public-key that key-pair signatures cannot be verified and should be republished keylessly; the push help and architecture docs repeat that stale guidance. Please direct them to --public-key and update the generated CLI/docs. CI has completed with no failing or pending checks.
d9b0cb4 to
7c97d12
Compare
The lock file stores a pinned key as single-line base64 DER SPKI because provenance values must be graphic and whitespace-free, so PEM armor cannot be stored verbatim. Nothing converted between the two forms yet, and the field shared the reference length bound, which is narrower than the key material it now has to hold. Add EncodePublicKey/DecodePublicKey as the single conversion between a cosign.pub file and the stored form. Encoding refuses a private key by its PEM label rather than by whatever a PKIX parse makes of its bytes: the result is sent over the API and written to the lock file, and neither is somewhere private material should reach by accident. Decoding re-validates rather than trusting its input, since the value arrives from a request body or a hand-editable file and is the artifact's only trust anchor. Give publicKey its own bound: an RSA-4096 SPKI encodes to 736 characters, so the 512 reference limit would reject a legitimate anchor rather than the oversized garbage these limits exist to stop. Run the syntactic field checks before the anchor's decode, so the length is bounded by a checked number instead of the allocation being sized off an unchecked one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
A skill pushed with `thv skill push --key` could be published but never installed project-scoped: the keyless path has no trust root to chain a key-pair signature to, and the signing key is recoverable from neither the artifact nor its bundle, so nothing could supply the anchor. The previous change made that refusal honest; this one gives it a way through. Add `--public-key` to `thv skill install`, carried to the service as encoded key material rather than as a path, since the server may be another process on another host where that path names nothing. The key is required on true first use and pinned into the lock entry, which supplies it on every install thereafter — trust on first use, with the anchor named explicitly because it cannot be observed. Dispatch is lock-first: the recorded expectation picks the verification path, never the artifact. Were the artifact allowed to select its own policy, a republished key-signed artifact could walk an entry out of the certificate identity it is pinned to. Every disagreement between a supplied key and the recorded state is refused rather than resolved by precedence, so a mistyped key cannot install as though it had been honored; v1 offers no in-place re-anchor, and the refusals say so. Report a key aimed at a keylessly-signed artifact as that, not as a failed signature — the mirror of the key-signed diagnosis, and the likeliest way to reach this path by mistake. Git and local-build installs refuse a public key outright: a commit signature is made with a certificate, and a local build has no registry signature at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
7c97d12 to
3b16c8d
Compare
github-actions
Bot
added
size/XL
2 participants
Summary
A skill pushed with
thv skill push --keycould be published but never installed project-scoped. The keyless path has no trust root to chain a cosign key-pair signature to, and the signing key is recoverable from neither the artifact nor its attached bundle — cosign's manifest defines no annotation carrying it — so nothing could supply the trust anchor. #6443 made that refusal honest and #6444 gave the lock file somewhere to record a key; this PR gives the install a way through.--public-keyonthv skill install. The CLI reads thecosign.pubfile and sends the encoded key material, not the path: the server may be a different process on a different host, where that path names nothing — or something else. Required on true first use, then pinned into the lock entry, which supplies it on every install thereafter. Trust on first use, with the anchor named explicitly because it cannot be observed.allow_signer_change— is an error. Silently preferring either anchor is how a mistyped--public-keyinstalls as though it had been honored. v1 offers no in-place re-anchor, and each refusal says so.ErrKeylessSigned, the mirror of Report key-signed artifacts as such at install #6443'sErrKeySigned: a key aimed at a keylessly-signed artifact is the likeliest way to reach this path by mistake, and a bare "signature verification failed" hides the remedy (drop the key).Part of #6442. Scoped to install;
--public-keyonupgrade/sync --adoptfollows in the next PR.Two fixes this exposed
publicKeyshared the 512-character reference bound, but an RSA-4096 SPKI encodes to 736 — the bound would have rejected a legitimate anchor rather than the oversized garbage it guards against. It now has its own.validateProvenanceran the anchor's base64 decode before the length check, sizing the allocation off an unchecked value from a hand-editable file. The syntactic checks now run first.Type of change
Changes
pkg/skills/verifier/publickey.goEncodePublicKey/DecodePublicKeybetweencosign.puband the stored base64 DER SPKIpkg/skills/verifier/oci.go,errors.goErrKeylessSignedand theonlyKeylessSigneddiagnosispkg/skills/skillsvc/verify.goresolveKeyAnchor, the key verification branch, conflict refusals, key-path classificationpkg/skills/skillsvc/install.gopkg/skills/lockfile/validation.gopublicKey; syntactic checks before the anchor decodepkg/skills/options.go,pkg/api/v1/skills*.go,pkg/skills/client/*PublicKeythrough the option, DTO, and clientcmd/thv/app/skill_install.go--public-keyflag and PEM readTest plan
task testpasses (pre-existing unrelated failure only:TestMCPGoClientInitializeAndPing, whose:8096is held by a stray process)task lint-fixclean for every file touched (the 6 remainingstaticcheckhits are pre-existing, in untouchedcmd/thv-operator/files)task docsregeneratedNew coverage: PEM↔base64 round-trip and its rejections (private key refused by label, multi-block, non-SPKI, over-bound); every arm of
resolveKeyAnchor; the entry guard; key-path error classification; the git and local-build refusals;onlyKeylessSignedincluding the invariant that it andonlyKeySignednever both hold.Does this introduce a user-facing change?
Yes.
thv skill install --public-key <cosign.pub>installs a skill signed withthv skill push --key. The key is recorded in the lock file and reused on later installs; supplying a conflicting one is refused rather than ignored.Special notes for reviewers
Why the key must be supplied rather than observed. A key-pair bundle carries no certificate and writes no Rekor entry, so there is nothing to read the signing key off. That makes key provenance a weaker claim than keyless — it says the holder of this key signed this artifact, nothing about who that holder is — which is why the anchor has to come from outside the artifact every time, and why
allow_signer_changecannot re-anchor it (re-recording "what was observed" would mean re-recording whatever the caller named).Wrong key and damaged signature are genuinely indistinguishable. The bundle records no key of its own to compare against, so the error names both causes instead of guessing.
allow_unsignedis not a remedy on any arm of the key path. An install that named a public key asked for that key to be enforced; recording an unsigned exception would file a false trust decision in the lock.This is ~420 lines across 12 files, a little over the repo's 400-line/10-file guideline. Splitting the
ErrKeylessSigneddiagnosis out would fit the cap but would ship the key path without the message for its most common failure. Happy to split if you'd prefer.Generated with Claude Code