Verify plugin signatures at install time - #6397
Open
samuv wants to merge 2 commits into
Open
Conversation
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 @@
## main #6397 +/- ##
========================================
Coverage 77.85% 77.85%
========================================
Files 760 761 +1
Lines 73043 73175 +132
========================================
+ Hits 56865 56971 +106
- Misses 16173 16199 +26
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 tasks
samuv
force-pushed
the
plugins-sig/07-install-verify
branch
from
August 25, 2026 12:59
4fb33d9 to
85bbebe
Compare
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>
The verifier interface gained a ProvenanceExpectation wrapper on main (#6420) so it can tell a strict lock pin from the independently-optional catalog constraints. The rebase merged textually clean but stopped compiling; plugins only ever present a lock expectation today, and NewLockExpectation(nil) is nil, so the trust-on-first-use case is unchanged. Part of #6300. Signed-off-by: Samuele Verzi <samu@stacklok.com>
samuv
force-pushed
the
plugins-sig/07-install-verify
branch
from
August 25, 2026 13:34
05f8f15 to
69fc618
Compare
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), built on the now-merged #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.Carried-over review from #6396
@jhrozek raised two points on the groundwork PR and deferred both to "the PR that populates the column" — this one. Status:
Binding the bundle to the digest it covers — addressed structurally, the same way skills does it.
installFromOCIsetsopts.Digest = pulledDigest.String(), passes that exact digest toverifyOCIInstall, and takesresult.Bundlefrom theVerifyOCIcall that verified it;buildInstalledPluginthen writesDigestandSigstoreBundlefrom the sameopts. No path pairs a stale bundle with a new digest, because every install — including upgrade re-pins and sync restores — re-pulls and re-verifies rather than reusing a stored bundle. The upgrade hazard specifically called out is therefore not reachable:applyUpgradeinstalls viapinGitReference/pinOCIReferenceoutput, which re-entersinstallFromGit/installFromOCIand re-verifies. ThreadingSigstoreBundlethroughbuildInstalledPluginalso fixes the third (minor) point — a re-install no longer nulls an existing bundle.Size ceiling on the bundle and the git payload/signature — deliberately deferred to PR8, not forgotten. PR8 is the PR that reads these bytes back (
VerifyBundleOfflineduringsync --check), so the cap and the code that depends on it land together and are testable in one place. Nothing in this stack has shipped a user-reachable unsigned-by-default path in the meantime: the whole feature stays behindTOOLHIVE_PLUGINS_LOCK_ENABLED. The intended shape is a reject-not-truncate bound enforced at capture time inverify.go, before the bytes reachInstallOptions;maxCompressedLayerSize(pkg/plugins/pluginsvc/content.go:26) is the precedent for the constant, though a bundle is a few KB in practice so a much tighter ceiling is more useful than mirroring 50 MB.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.mainand picked up a semantic (not textual) conflict: Check first skill install against catalog-declared provenance #6420 changedVerifyOCI/VerifyGitto take a*verifier.ProvenanceExpectationso they can distinguish a strict lock pin from independently-optional catalog constraints. CommitAdapt plugin verify to ProvenanceExpectationwraps the lock provenance withverifier.NewLockExpectation, which returns nil for nil input — so the trust-on-first-use path is unchanged. Plugins present only a lock expectation today; the catalog-provenance mirror of Check first skill install against catalog-declared provenance #6420 is a later PR in this stack.AllowSignerChange, PR9),sync --checkre-verification and the bundle/payload size caps (PR8), push signing (PR10).🤖 Generated with Claude Code