Verify plugin signatures at install time - #6397
Open
samuv wants to merge 1 commit into
Open
Conversation
Project-scoped plugin installs now verify artifact signatures before anything is extracted or recorded (RFC THV-0080): OCI artifacts through the Sigstore keyless flow, git commits through gitsign verification, both against the identity recorded in the project's lock file. On first use the observed identity is recorded (trust on first use); later installs enforce it inside the verifier, which plugins reuse from pkg/skills/verifier so the pinned ref/runner checks come along too. Verification runs under the per-plugin mutex so concurrent first installs cannot race their TOFU anchors, and is scoped to installs that record lock state — including the plugins lock feature gate, since a disabled lock file has nowhere to anchor trust. Unsigned artifacts are rejected unless the caller sets allow_unsigned, which records an explicit "unsigned: true" exception in the lock entry; an entry locked to a signer identity refuses unsigned or local-build replacements outright. Lock-driven operations (sync restores, upgrade re-pins) honor the trust state the entry already records — a lock diff converting provenance to unsigned is therefore a reviewable trust downgrade, called out in the code. Unlike skills, a local-store upgrade deliberately clears resolvedReference, so ExpectedCanonicalName joins the lock-driven markers. Verified installs persist the Sigstore bundle with the DB record for offline re-verification during sync. The unsigned exception reaches the service from every surface: the CLI flag, the HTTP client DTO (without which the flag would silently never reach the server — pinned by a round-trip test), and the API request type. Failures classify to typed reasons via errors.Is on the verifier's sentinels. Part of #6300. Signed-off-by: Samuele Verzi <samu@stacklok.com>
samuv
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
jhrozek,
rdimitrov and
reyortiz3
as code owners
August 20, 2026 13:18
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## plugins-sig/06-bundle #6397 +/- ##
=========================================================
- Coverage 77.70% 77.68% -0.02%
=========================================================
Files 750 751 +1
Lines 72580 72711 +131
=========================================================
+ Hits 56396 56485 +89
- Misses 16179 16221 +42
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Plugins reach the same trust boundary skills crossed in #6129: a project's
toolhive.lock.yamlrecords what a plugin is, but nothing checks who published the artifact being installed under that name. A plugin contributes hooks, agents, and MCP servers to the client that loads it, so an unverified substitution at install time is executed, not just stored.This is PR7 of Stack 2 (#6300), stacked on #6396 (which added the
sigstore_bundlecolumn and surfaced the git commit signature/payload). It is the plugin mirror of the skills change in #6129, and reusespkg/skills/verifierwholesale — so plugins inherit #6315's ref/runner enforcement for free — andpkg/skills/lockfile.Entry, whoseProvenance/Unsignedfields already exist. No lockfile schema changes.VerifyOCI; git installs verify the commit signature/payload viaVerifyGit; local-store and layer-data installs have no registry signature and are an unsigned trust decision. Verification runs before extraction or DB recording, under the existing per-plugin mutex, so concurrent first installs cannot race their TOFU anchors.GetPlugin): the first verified install records the observed identity intoprovenance:, later installs pass it into the verifier and fail typed on mismatch. An entry locked to a signer identity refuses unsigned or local-build replacements outright.--allow-unsigned. Unsigned artifacts are rejected with a typed 403 unless the caller opts in, which recordsunsigned: truein the lock entry. Lock-driven operations (sync restores, upgrade re-pins) honor the trust state the entry already records instead of demanding the flag again; entries with no recorded trust state restored by sync record as unsigned.InstalledPluginrecord (PR6's field) for PR8's offline re-verify. Unsigned installs store NULL.plugins.InstallOptions.AllowUnsignedplus internalProvenance/Unsigned/SigstoreBundle, threaded through the API DTO, the Go HTTP client DTO, and a--allow-unsignedflag onthv ai-plugin install.pluginsvc.servicegains an injectablesigVerifierdefaulting toverifier.NewDefault(images.NewCompositeKeychain()).Scoping follows the
TOOLHIVE_PLUGINS_LOCK_ENABLEDgate the plugin lock service already uses: verification applies exactly where lock recording does. User-scope installs are untouched, and the feature stays inert on main until the stack lands.Part of #6300.
Type of change
Test plan
task test)task lint-fix)task test-e2e)New unit coverage in
pkg/plugins/pluginsvc/verify_test.gomirrors the skills suite: TOFU recording (and that the recorded identity reaches the verifier on the second install), unsigned rejection with no lock entry or DB record left behind, the--allow-unsignedexception, signer-mismatch rejection leaving the prior pin intact, locked-unsigned reinstall demanding the flag again, a lock-driven sync restore honoring recorded trust, a local build refused against a locked signer, bundle persisted on the DB record, and both failure classifiers distinguishing a provenance-field mismatch from a signer change. The DTO boundaries are pinned by round-trip tests inpkg/plugins/clientandpkg/api/v1, and the CLI flag by a registration test.E2E:
test/e2e/cli_plugins_lock_test.gogains the two install-verification cases (rejected without the flag /unsigned: truerecorded with it); its existing installs now passallow_unsignedbecause the suite runs with the lock gate on and publishes unsigned artifacts. Not run locally — this suite needs a built binary and is covered in CI.One pre-existing unrelated failure on the base branch:
TestMCPGoClientInitializeAndPinginpkg/transport/proxy/streamable(verified failing onplugins-sig/06-bundlewithout these changes).API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Does this introduce a user-facing change?
Yes. With
TOOLHIVE_PLUGINS_LOCK_ENABLED=true, a project-scopedthv ai-plugin installof an unsigned plugin now fails with a 403 instead of installing;--allow-unsigned(API:allow_unsigned) records the exception in the lock file. Once a plugin's signer identity is recorded, a later install signed by a different identity is refused. User-scoped installs are unchanged, and the whole behavior stays behind the rollout gate.Special notes for reviewers
verify.go:lockDrivenInstallalso treatsExpectedCanonicalNameas a lock-driven marker. A plugin upgrade off the local store deliberately clearsLockResolvedReferenceso sync restores by digest, so the skills pair of markers alone would misread it as a fresh user install and demand a flag the upgrade API cannot pass.verifyLocalInstalllets a lock-driven restore of an entry with no recorded trust state proceed and recordunsigned, the same allowanceisAllowedUnsignedalready makes for OCI and git — otherwise pre-verification lock entries pinned to the local store become unrestorable.InstallOptions.Provenanceis*lockfile.Provenance, not a plugin-side mirror ofskills.ProvenanceInfo. Plugins have no API-facing provenance type yet, so the skills conversion pair would exist only to be round-tripped — a place for recorded trust data to get silently dropped.verifier.Result.ToLockProvenance()covers it. If a later PR surfaces provenance onPluginInfo, the conversion arrives with the consumer that needs it.TestInstallAndRegister_LockSnapshotFailureRollsBackDBis renamed toTestInstall_UnreadableLockFileAbortsBeforeMutating. Verification reads the lock entry's trust state before extraction, so an unloadable lock file now fails there rather than atinstallAndRegister's snapshot. That snapshot'sLoadguard is kept as defense against a rewrite racing the window, but it is no longer reachable from a test — the renamed test asserts the stronger property that nothing is extracted or recorded at all.AllowSignerChange, PR9),sync --checkre-verification (PR8), push signing (PR10).🤖 Generated with Claude Code