Skip to content

Sign pushes by default and remove the lock feature gate - #6139

Open
samuv wants to merge 3 commits into
skills-sig/13b-provenance-displayfrom
skills-sig/14-push-signing-ungate
Open

Sign pushes by default and remove the lock feature gate#6139
samuv wants to merge 3 commits into
skills-sig/13b-provenance-displayfrom
skills-sig/14-push-signing-ungate

Conversation

@samuv

@samuv samuv commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Note

Stack 2 of RFC THV-0080 — final PR (stack #6128, on top of #6137). Closes #5899 when it lands.

Summary

The last piece: signing on publish, and the feature gate comes out.

  • thv skill push signs by default: --key signs the pushed artifact with a cosign private key (the signature manifest is attached at the .sig tag, so project-scoped installs verify it; the flow the signer/verifier PRs round-trip in tests). Pushing unsigned requires an explicit --no-sign, and a failed signing fails the push — an artifact is never silently published unsigned. Threaded through CLI, client, and API.
  • TOOLHIVE_SKILLS_LOCK_ENABLED is removed: with verification on consume (installs, sync, upgrade) and signing on publish both in place, the lock file graduates from experimental. The gate, its env var, the "(experimental)" command markers, and every gated test-server setup are gone; the lock file, toolhive.requires materialization, sync, upgrade, and signature verification are now standard behavior for project-scoped skills. This is THE user-facing change of the stack.
  • Trust-model docs rewritten (docs/arch/12-skills-system.md): the "drift detection over a repository-editable file" honesty note graduates to the verified model — TOFU semantics with the identity displayed, --allow-unsigned / --allow-signer-change escape hatches, the provisional: git marker, and an explicit list of what deliberately remains trusted on faith (unsigned exceptions, the lock file as reviewable policy, the first-use anchor).

Type of change

  • New feature (non-breaking change which adds functionality)

Test plan

  • Unit tests pass locally (task test, full suite with -race)
  • Linting passes (task lint-fix, 0 issues)
  • New unit tests: push requires an explicit signing decision (400 before anything is pushed); push signs with the provided key pinned to the pushed digest; signing failure fails the push. Gate-dependent tests updated: the gated-server E2E blocks merge into the standard suites; the legacy-unmanaged adopt fixture now constructs pre-lock state directly instead of toggling the gate.
  • task docs regenerated.

Does this introduce a user-facing change?

Yes. The skills lock file feature is no longer gated: project-scoped installs write toolhive.lock.yaml, verify signatures (unsigned installs need --allow-unsigned), and thv skill sync/upgrade work without TOOLHIVE_SKILLS_LOCK_ENABLED. thv skill push now requires --key or an explicit --no-sign.

Special notes for reviewers

  • The signed round-trip interop check (cosign verify --key accepts our attached signature) is covered by the signer package's unit tests plus toolhive-core's key-signed retrieval tests; a manual cosign-CLI check against a real registry is worth doing once before release.
  • Follow-ups tracked on Track: Skills lock file + Sigstore signing (RFC THV-0080) #5899 at close: Rekor proof validation for git (removes provisional:), keyless/OIDC signing, catalog-supplied expected identity, and moving pkg/skills/signer to toolhive-core.

Generated with Claude Code

@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jul 29, 2026
@samuv samuv self-assigned this Jul 29, 2026
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.82%. Comparing base (b391771) to head (1c8d470).

Files with missing lines Patch % Lines
pkg/skills/signer/key.go 77.27% 3 Missing and 2 partials ⚠️
pkg/skills/skillsvc/build.go 93.33% 1 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           skills-sig/13b-provenance-display    #6139   +/-   ##
==================================================================
  Coverage                              72.81%   72.82%           
==================================================================
  Files                                    743      742    -1     
  Lines                                  77674    77708   +34     
==================================================================
+ Hits                                   56558    56589   +31     
- Misses                                 17146    17150    +4     
+ Partials                                3970     3969    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 29, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 64ff0fa to 3afb9eb Compare August 3, 2026 08:28
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 3, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 3afb9eb to 71a473b Compare August 3, 2026 09:07
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 3, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 71a473b to 762881c Compare August 3, 2026 13:41
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 3, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 762881c to caa6620 Compare August 3, 2026 14:11
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 3, 2026
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Aug 3, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from dbf984a to 454442d Compare August 4, 2026 08:34
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 4, 2026
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 454442d to 1c3289a Compare August 4, 2026 08:35
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Aug 4, 2026
samuv and others added 3 commits August 12, 2026 11:58
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>
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>
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.
@samuv
samuv force-pushed the skills-sig/14-push-signing-ungate branch from 1c3289a to 1c8d470 Compare August 12, 2026 10:09
@samuv
samuv marked this pull request as ready for review August 12, 2026 10:09
@github-actions github-actions Bot removed the size/M Medium PR: 300-599 lines changed label Aug 12, 2026
@samuv

samuv commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the updated #6137 branch and marked ready. The gate removal verifies clean: pkg/skills/feature_gate.go is gone, no LockFileFeatureEnabled / TOOLHIVE_SKILLS_LOCK_ENABLED / errExperimentalLockFeature references remain outside tests, and both (experimental) CLI markers are gone.

A shipping blocker this PR had

The plan for this stack promised one manual check that was never run: "signing round-trip interop — cosign verify --key accepts our attached signature". I ran it. It failed:

$ cosign generate-key-pair
$ thv skill push 127.0.0.1:15000/interop/demo:v1 --key cosign.key
Error: failed to push skill: signing pushed artifact: decoding signing key:
  asn1: structure error: tags don't match (16 vs {class:1 tag:27 ...}) pkcs8 @2

--key could not read any key produced by cosign generate-key-pair — the only realistic way a user obtains one. Since this PR makes signing the default on push, that would have shipped as a broken flag.

The cause is a collision of formats behind one label. cosign seals the PKCS#8 key with scrypt + nacl/secretbox (go-securesystemslib/encrypted); cryptoutils.MarshalPrivateKeyToEncryptedDER writes PKCS#8 standard encryption. Both land in a PEM block labelled ENCRYPTED SIGSTORE PRIVATE KEY, and loadKeypair only understood the second.

The tests missed it because the fixtures were built with cryptoutils — and TestSignOCIEncryptedKey even asserted in a comment that its fixture was "what cosign generate-key-pair produces". It wasn't. That comment is now corrected.

Fixed by decrypting the cosign sealing when the label indicates it, falling back to the PKCS#8 form so both keep working, and reporting a wrong password as a password problem instead of an ASN.1 parse failure.

Interop now proven

$ thv skill push 127.0.0.1:15000/interop/final:v1 --key cosign2.key
$ cosign verify --key cosign2.pub --insecure-ignore-tlog=true 127.0.0.1:15000/interop/final:v1

Verification for 127.0.0.1:15000/interop/final:v1 --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - The signatures were verified against the specified public key

Verified with both an empty-password key and a password-protected one, against a real registry, using the stock cosign CLI.

New regression tests build fixtures with encrypted.Encrypt — the same call cosign makes — so they fail without the fix. I confirmed that by reverting the change and watching them go red, rather than assuming.

One behaviour worth documenting, not fixing

COSIGN_PASSWORD has to be set on the thv serve process, not the shell running the CLI, because signing happens server-side. A password-protected key fails with the password in your shell and succeeds with it in the server's environment. I have documented it in the flag help and the Options.Key doc comment rather than changing the design — forwarding the passphrase over the API would mean putting a secret in a request body.

task test, task lint-fix, and task docs green at the branch tip.

Related: stacklok/toolhive-core#230 carries the same fix, since that PR moves this package into core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track: Skills lock file + Sigstore signing (RFC THV-0080)

2 participants