Skip to content

Verify plugin signatures at install time - #6397

Open
samuv wants to merge 2 commits into
mainfrom
plugins-sig/07-install-verify
Open

Verify plugin signatures at install time#6397
samuv wants to merge 2 commits into
mainfrom
plugins-sig/07-install-verify

Conversation

@samuv

@samuv samuv commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Plugins reach the same trust boundary skills crossed in #6129: a project's toolhive.lock.yaml records 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_bundle column and surfaced the git commit signature/payload). It is the plugin mirror of the skills change in #6129, and reuses pkg/skills/verifier wholesale — so plugins inherit #6315's ref/runner enforcement for free — and pkg/skills/lockfile.Entry, whose Provenance/Unsigned fields already exist. No lockfile schema changes.

  • Verify before mutating. OCI installs verify the artifact at its resolved ref/digest via VerifyOCI; git installs verify the commit signature/payload via VerifyGit; 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.
  • TOFU + lock enforcement. The expected trust state is read from the lock entry (GetPlugin): the first verified install records the observed identity into provenance:, 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 records unsigned: true in 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.
  • Bundle persistence. A verified install stores the returned Sigstore bundle on the InstalledPlugin record (PR6's field) for PR8's offline re-verify. Unsigned installs store NULL.
  • Wired end to end. plugins.InstallOptions.AllowUnsigned plus internal Provenance/Unsigned/SigstoreBundle, threaded through the API DTO, the Go HTTP client DTO, and a --allow-unsigned flag on thv ai-plugin install. pluginsvc.service gains an injectable sigVerifier defaulting to verifier.NewDefault(images.NewCompositeKeychain()).

Scoping follows the TOOLHIVE_PLUGINS_LOCK_ENABLED gate 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

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • E2E tests (task test-e2e)

New unit coverage in pkg/plugins/pluginsvc/verify_test.go mirrors 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-unsigned exception, 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 in pkg/plugins/client and pkg/api/v1, and the CLI flag by a registration test.

E2E: test/e2e/cli_plugins_lock_test.go gains the two install-verification cases (rejected without the flag / unsigned: true recorded with it); its existing installs now pass allow_unsigned because 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: TestMCPGoClientInitializeAndPing in pkg/transport/proxy/streamable (verified failing on plugins-sig/06-bundle without these changes).

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label 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-scoped thv ai-plugin install of 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:

  1. Binding the bundle to the digest it covers — addressed structurally, the same way skills does it. installFromOCI sets opts.Digest = pulledDigest.String(), passes that exact digest to verifyOCIInstall, and takes result.Bundle from the VerifyOCI call that verified it; buildInstalledPlugin then writes Digest and SigstoreBundle from the same opts. 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: applyUpgrade installs via pinGitReference/pinOCIReference output, which re-enters installFromGit/installFromOCI and re-verifies. Threading SigstoreBundle through buildInstalledPlugin also fixes the third (minor) point — a re-install no longer nulls an existing bundle.

  2. 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 (VerifyBundleOffline during sync --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 behind TOOLHIVE_PLUGINS_LOCK_ENABLED. The intended shape is a reject-not-truncate bound enforced at capture time in verify.go, before the bytes reach InstallOptions; 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

  • Size. ~390 net non-test, non-generated lines, just under the 400-line guideline (skills' equivalent was ~470). The trust logic is deliberately kept in one PR rather than split — a verifier without its lock enforcement, or enforcement without the flag that relieves it, is not independently reviewable or safely mergeable.
  • Two deliberate divergences from the skills mirror, both commented in verify.go:
    1. lockDrivenInstall also treats ExpectedCanonicalName as a lock-driven marker. A plugin upgrade off the local store deliberately clears LockResolvedReference so 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.
    2. verifyLocalInstall lets a lock-driven restore of an entry with no recorded trust state proceed and record unsigned, the same allowance isAllowedUnsigned already makes for OCI and git — otherwise pre-verification lock entries pinned to the local store become unrestorable.
  • InstallOptions.Provenance is *lockfile.Provenance, not a plugin-side mirror of skills.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 on PluginInfo, the conversion arrives with the consumer that needs it.
  • TestInstallAndRegister_LockSnapshotFailureRollsBackDB is renamed to TestInstall_UnreadableLockFileAbortsBeforeMutating. Verification reads the lock entry's trust state before extraction, so an unloadable lock file now fails there rather than at installAndRegister's snapshot. That snapshot's Load guard 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.
  • Rebase note. After Store plugin sigstore bundles, carry git signature #6396 merged, this branch was rebased onto main and picked up a semantic (not textual) conflict: Check first skill install against catalog-declared provenance #6420 changed VerifyOCI/VerifyGit to take a *verifier.ProvenanceExpectation so they can distinguish a strict lock pin from independently-optional catalog constraints. Commit Adapt plugin verify to ProvenanceExpectation wraps the lock provenance with verifier.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.
  • Not in scope, per the stack plan: signer-change override (AllowSignerChange, PR9), sync --check re-verification and the bundle/payload size caps (PR8), push signing (PR10).

🤖 Generated with Claude Code

@samuv samuv self-assigned this Aug 20, 2026
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Aug 20, 2026
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.76923% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.85%. Comparing base (7f15a63) to head (69fc618).

Files with missing lines Patch % Lines
pkg/plugins/pluginsvc/verify.go 76.41% 25 Missing ⚠️
pkg/plugins/pluginsvc/install_oci.go 20.00% 4 Missing ⚠️
pkg/plugins/pluginsvc/sync.go 50.00% 1 Missing ⚠️
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.
📢 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.

Base automatically changed from plugins-sig/06-bundle to main August 25, 2026 12:59
@samuv
samuv force-pushed the plugins-sig/07-install-verify branch from 4fb33d9 to 85bbebe Compare August 25, 2026 12:59
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Aug 25, 2026
samuv added 2 commits August 25, 2026 15:34
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
samuv force-pushed the plugins-sig/07-install-verify branch from 05f8f15 to 69fc618 Compare August 25, 2026 13:34
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant