Skip to content

Verify plugin signatures at install time - #6397

Open
samuv wants to merge 1 commit into
plugins-sig/06-bundlefrom
plugins-sig/07-install-verify
Open

Verify plugin signatures at install time#6397
samuv wants to merge 1 commit into
plugins-sig/06-bundlefrom
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), stacked on #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.

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.
  • Not in scope, per the stack plan: signer-change override (AllowSignerChange, PR9), sync --check re-verification (PR8), push signing (PR10).

🤖 Generated with Claude Code

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 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.64516% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.68%. Comparing base (662d3a5) to head (4fb33d9).

Files with missing lines Patch % Lines
pkg/plugins/pluginsvc/verify.go 76.19% 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                    @@
##           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.
📢 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.

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