From e2c3ca4b0ffa332058011d70f0c8b7181b09a1de Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 20 Aug 2026 15:17:08 +0200 Subject: [PATCH 1/2] Verify plugin signatures at install time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cmd/thv/app/ai_plugin_install.go | 26 +- cmd/thv/app/ai_plugin_install_test.go | 22 ++ docs/cli/thv_ai-plugin_install.md | 1 + docs/server/docs.go | 4 + docs/server/swagger.json | 4 + docs/server/swagger.yaml | 6 + pkg/api/v1/plugins.go | 15 +- pkg/api/v1/plugins_test.go | 35 ++ pkg/api/v1/plugins_types.go | 4 + pkg/plugins/client/client.go | 15 +- pkg/plugins/client/client_test.go | 26 ++ pkg/plugins/client/dto.go | 3 + pkg/plugins/options.go | 31 ++ pkg/plugins/pluginsvc/install.go | 11 + pkg/plugins/pluginsvc/install_extraction.go | 21 +- pkg/plugins/pluginsvc/install_git.go | 17 +- pkg/plugins/pluginsvc/install_oci.go | 13 + pkg/plugins/pluginsvc/lock.go | 9 + pkg/plugins/pluginsvc/lock_test.go | 208 ++++++----- pkg/plugins/pluginsvc/service.go | 11 + pkg/plugins/pluginsvc/sync.go | 3 + pkg/plugins/pluginsvc/sync_test.go | 40 ++- pkg/plugins/pluginsvc/upgrade_test.go | 52 +-- pkg/plugins/pluginsvc/verify.go | 287 +++++++++++++++ pkg/plugins/pluginsvc/verify_test.go | 375 ++++++++++++++++++++ test/e2e/cli_plugins_lock_test.go | 68 +++- 26 files changed, 1132 insertions(+), 175 deletions(-) create mode 100644 cmd/thv/app/ai_plugin_install_test.go create mode 100644 pkg/plugins/pluginsvc/verify.go create mode 100644 pkg/plugins/pluginsvc/verify_test.go diff --git a/cmd/thv/app/ai_plugin_install.go b/cmd/thv/app/ai_plugin_install.go index 3d4996b428..e69fa50b59 100644 --- a/cmd/thv/app/ai_plugin_install.go +++ b/cmd/thv/app/ai_plugin_install.go @@ -10,11 +10,12 @@ import ( ) var ( - aiPluginInstallScope string - aiPluginInstallClientsRaw string - aiPluginInstallForce bool - aiPluginInstallProjectRoot string - aiPluginInstallGroup string + aiPluginInstallScope string + aiPluginInstallClientsRaw string + aiPluginInstallForce bool + aiPluginInstallProjectRoot string + aiPluginInstallGroup string + aiPluginInstallAllowUnsigned bool ) var aiPluginInstallCmd = &cobra.Command{ @@ -44,6 +45,8 @@ func init() { &aiPluginInstallProjectRoot, "project-root", "", "Project root path for project-scoped installs", ) aiPluginInstallCmd.Flags().StringVar(&aiPluginInstallGroup, "group", "", "Group to add the plugin to after installation") + aiPluginInstallCmd.Flags().BoolVar(&aiPluginInstallAllowUnsigned, "allow-unsigned", false, + "Allow installing a project-scoped plugin without a verified signature (recorded in the lock file)") } func aiPluginInstallCmdFunc(cmd *cobra.Command, args []string) error { @@ -55,12 +58,13 @@ func aiPluginInstallCmdFunc(cmd *cobra.Command, args []string) error { } _, err = c.Install(cmd.Context(), plugins.InstallOptions{ - Name: args[0], - Scope: plugins.Scope(aiPluginInstallScope), - Clients: parseSkillInstallClients(aiPluginInstallClientsRaw), - Force: aiPluginInstallForce, - ProjectRoot: projectRoot, - Group: aiPluginInstallGroup, + Name: args[0], + Scope: plugins.Scope(aiPluginInstallScope), + Clients: parseSkillInstallClients(aiPluginInstallClientsRaw), + Force: aiPluginInstallForce, + ProjectRoot: projectRoot, + Group: aiPluginInstallGroup, + AllowUnsigned: aiPluginInstallAllowUnsigned, }) if err != nil { return formatAIPluginError("install plugin", err) diff --git a/cmd/thv/app/ai_plugin_install_test.go b/cmd/thv/app/ai_plugin_install_test.go new file mode 100644 index 0000000000..8b16ec1689 --- /dev/null +++ b/cmd/thv/app/ai_plugin_install_test.go @@ -0,0 +1,22 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAIPluginInstallAllowUnsignedFlag pins the flag that carries the +// unsigned-install trust decision: an install of a project-scoped plugin +// without a verified signature is rejected unless the user opts in here. +func TestAIPluginInstallAllowUnsignedFlag(t *testing.T) { + t.Parallel() + + flag := aiPluginInstallCmd.Flags().Lookup("allow-unsigned") + require.NotNil(t, flag, "thv ai-plugin install must expose --allow-unsigned") + assert.Equal(t, "false", flag.DefValue, "unsigned installs must never be the default") +} diff --git a/docs/cli/thv_ai-plugin_install.md b/docs/cli/thv_ai-plugin_install.md index bb4c04e4b6..bb5636c179 100644 --- a/docs/cli/thv_ai-plugin_install.md +++ b/docs/cli/thv_ai-plugin_install.md @@ -25,6 +25,7 @@ thv ai-plugin install [plugin-name] [flags] ### Options ``` + --allow-unsigned Allow installing a project-scoped plugin without a verified signature (recorded in the lock file) --clients string Comma-separated target client apps (e.g. claude-code,codex), or "all" for every available client --force Overwrite existing plugin directory --group string Group to add the plugin to after installation diff --git a/docs/server/docs.go b/docs/server/docs.go index 54cee99290..0d4b3b00e5 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -3366,6 +3366,10 @@ const docTemplate = `{ "pkg_api_v1.installPluginRequest": { "description": "Request to install a plugin", "properties": { + "allow_unsigned": { + "description": "AllowUnsigned permits installing a project-scoped plugin without a\nverified signature; the exception is recorded in the project's lock\nfile.", + "type": "boolean" + }, "clients": { "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every plugin-supporting client.\nOmitting this field installs to all available clients.", "items": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index e44f4aeb4b..c1c54fb684 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -3359,6 +3359,10 @@ "pkg_api_v1.installPluginRequest": { "description": "Request to install a plugin", "properties": { + "allow_unsigned": { + "description": "AllowUnsigned permits installing a project-scoped plugin without a\nverified signature; the exception is recorded in the project's lock\nfile.", + "type": "boolean" + }, "clients": { "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every plugin-supporting client.\nOmitting this field installs to all available clients.", "items": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index fa19d25384..8f27de29ff 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -3197,6 +3197,12 @@ components: pkg_api_v1.installPluginRequest: description: Request to install a plugin properties: + allow_unsigned: + description: |- + AllowUnsigned permits installing a project-scoped plugin without a + verified signature; the exception is recorded in the project's lock + file. + type: boolean clients: description: |- Clients lists target client identifiers (e.g., "claude-code"), diff --git a/pkg/api/v1/plugins.go b/pkg/api/v1/plugins.go index 311b462073..82ffbed010 100644 --- a/pkg/api/v1/plugins.go +++ b/pkg/api/v1/plugins.go @@ -125,13 +125,14 @@ func (s *PluginsRoutes) installPlugin(w http.ResponseWriter, r *http.Request) er } result, err := s.pluginService.Install(r.Context(), plugins.InstallOptions{ - Name: req.Name, - Version: req.Version, - Scope: req.Scope, - ProjectRoot: req.ProjectRoot, - Clients: req.Clients, - Force: req.Force, - Group: req.Group, + Name: req.Name, + Version: req.Version, + Scope: req.Scope, + ProjectRoot: req.ProjectRoot, + Clients: req.Clients, + Force: req.Force, + Group: req.Group, + AllowUnsigned: req.AllowUnsigned, }) if err != nil { return err diff --git a/pkg/api/v1/plugins_test.go b/pkg/api/v1/plugins_test.go index d621e994b0..a920d96a78 100644 --- a/pkg/api/v1/plugins_test.go +++ b/pkg/api/v1/plugins_test.go @@ -677,3 +677,38 @@ func TestPluginsInstallLocationHeader(t *testing.T) { assert.Equal(t, http.StatusCreated, rec.Code) assert.Equal(t, "/api/v1beta/plugins/my-plugin", rec.Header().Get("Location")) } + +// TestPluginsInstallCarriesAllowUnsigned pins the API-side half of the +// unsigned-install exception: a request body that sets allow_unsigned must +// reach the service as InstallOptions.AllowUnsigned, or the flag dies at the +// handler and every caller is told to pass the flag it already passed. +func TestPluginsInstallCarriesAllowUnsigned(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockSvc := plugmocks.NewMockPluginService(ctrl) + + mockSvc.EXPECT().Install(gomock.Any(), plugins.InstallOptions{ + Name: "my-plugin", + Scope: plugins.ScopeProject, + ProjectRoot: "/tmp/project", + AllowUnsigned: true, + }).Return(&plugins.InstallResult{ + Plugin: plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Scope: plugins.ScopeProject, + Status: plugins.InstallStatusInstalled, + }, + }, nil) + + router := chi.NewRouter() + router.Mount("/", PluginsRouter(mockSvc)) + + body := `{"name":"my-plugin","scope":"project","project_root":"/tmp/project","allow_unsigned":true}` + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusCreated, rec.Code) +} diff --git a/pkg/api/v1/plugins_types.go b/pkg/api/v1/plugins_types.go index bb9e5fd48b..e85d24df5b 100644 --- a/pkg/api/v1/plugins_types.go +++ b/pkg/api/v1/plugins_types.go @@ -31,6 +31,10 @@ type installPluginRequest struct { Clients []string `json:"clients,omitempty"` // Force allows overwriting unmanaged plugin directories Force bool `json:"force,omitempty"` + // AllowUnsigned permits installing a project-scoped plugin without a + // verified signature; the exception is recorded in the project's lock + // file. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` // Group is the group name to add the plugin to after installation Group string `json:"group,omitempty"` } diff --git a/pkg/plugins/client/client.go b/pkg/plugins/client/client.go index 8a58982478..c0f7e79f75 100644 --- a/pkg/plugins/client/client.go +++ b/pkg/plugins/client/client.go @@ -206,13 +206,14 @@ func (c *Client) List(ctx context.Context, opts plugins.ListOptions) ([]plugins. // Install installs a plugin from a remote source. func (c *Client) Install(ctx context.Context, opts plugins.InstallOptions) (*plugins.InstallResult, error) { body := installRequest{ - Name: opts.Name, - Version: opts.Version, - Scope: opts.Scope, - ProjectRoot: opts.ProjectRoot, - Clients: opts.Clients, - Force: opts.Force, - Group: opts.Group, + Name: opts.Name, + Version: opts.Version, + Scope: opts.Scope, + ProjectRoot: opts.ProjectRoot, + Clients: opts.Clients, + Force: opts.Force, + Group: opts.Group, + AllowUnsigned: opts.AllowUnsigned, } var resp installResponse diff --git a/pkg/plugins/client/client_test.go b/pkg/plugins/client/client_test.go index e87a11f524..5647b1b3d7 100644 --- a/pkg/plugins/client/client_test.go +++ b/pkg/plugins/client/client_test.go @@ -999,3 +999,29 @@ func TestCallerCancellationIsNeitherSentinel(t *testing.T) { assert.NotErrorIs(t, err, ErrRequestTimeout) assert.NotErrorIs(t, err, ErrServerUnreachable) } + +// TestInstallCarriesAllowUnsigned round-trips the unsigned exception through +// the client's request body — without this, the CLI flag silently never +// reaches the server (every --allow-unsigned install would 403 telling the +// user to pass the flag they passed). +func TestInstallCarriesAllowUnsigned(t *testing.T) { + t.Parallel() + + var got installRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.NoError(t, json.NewDecoder(r.Body).Decode(&got)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(installResponse{}) + })) + t.Cleanup(srv.Close) + + _, err := newTestClient(t, srv).Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + Scope: plugins.ScopeProject, + ProjectRoot: "/tmp/project", + AllowUnsigned: true, + }) + require.NoError(t, err) + assert.True(t, got.AllowUnsigned, "allow_unsigned must reach the server") +} diff --git a/pkg/plugins/client/dto.go b/pkg/plugins/client/dto.go index bf3f2ee593..f5070abd12 100644 --- a/pkg/plugins/client/dto.go +++ b/pkg/plugins/client/dto.go @@ -15,6 +15,9 @@ type installRequest struct { Clients []string `json:"clients,omitempty"` Force bool `json:"force,omitempty"` Group string `json:"group,omitempty"` + // AllowUnsigned mirrors plugins.InstallOptions.AllowUnsigned; without + // it here the CLI flag would silently never reach the server. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` } type validateRequest struct { diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index b3118da912..f7327371bc 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -7,6 +7,7 @@ import ( "context" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) // ListOptions configures the behavior of the List operation. Alias for @@ -31,6 +32,13 @@ type InstallOptions struct { ProjectRoot string `json:"project_root,omitempty"` // Group is the group name to add the plugin to after installation. Group string `json:"group,omitempty"` + // AllowUnsigned permits installing a project-scoped plugin whose + // artifact carries no Sigstore signature. Without it, unsigned + // artifacts are rejected; with it, the lock entry records the exception + // as "unsigned: true". A plugin contributes hooks, agents, and MCP + // servers to the client that loads it, so this is an explicit + // per-install trust decision, never a default. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` // LayerData is the tar.gz content from an OCI layer. Internal use only — NOT exposed via HTTP API. LayerData []byte `json:"-"` // Reference is the full OCI reference (e.g. ghcr.io/org/plugin:v1). @@ -71,6 +79,24 @@ type InstallOptions struct { // to equal this value before any install mutation. Used by Sync/Upgrade so a // lock entry cannot be repaired under a different canonical identity. ExpectedCanonicalName string `json:"-"` + // Unsigned records the trust decision that this install proceeded + // without a verified signature (via AllowUnsigned). Set internally by + // install-time verification; recorded as `unsigned: true` in the lock + // entry. Internal use only — NOT exposed via HTTP API. + Unsigned bool `json:"-"` + // Provenance carries the verified signer identity established during + // install-time verification, for recording into the lock entry. Set by + // the verification step, nil when the artifact is unsigned or + // verification did not run. Unlike skills.InstallOptions, this is the + // lock file's own shape: plugins have no API-facing provenance type to + // convert through yet, and a conversion pair that exists only to be + // round-tripped is a place for recorded trust data to get dropped. + // Internal use only — NOT exposed via HTTP API. + Provenance *lockfile.Provenance `json:"-"` + // SigstoreBundle is the serialized Sigstore bundle backing Provenance, + // persisted alongside the install record so sync can re-verify offline. + // Internal use only — NOT exposed via HTTP API. + SigstoreBundle []byte `json:"-"` } // InstallResult contains the outcome of an Install operation. @@ -165,6 +191,11 @@ const ( FailureReasonSignerMismatch = skills.FailureReasonSignerMismatch FailureReasonUnsignedRejected = skills.FailureReasonUnsignedRejected FailureReasonUnknown = skills.FailureReasonUnknown + + // FailureReasonProvenanceFieldMismatch means the artifact verifies + // against the recorded signer, but a pinned certificate field (the + // repository ref or runner environment) no longer matches. + FailureReasonProvenanceFieldMismatch = skills.FailureReasonProvenanceFieldMismatch ) // UpgradeOptions configures a lock-file upgrade. Alias for diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index 9f7130e945..afa26185b3 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -132,6 +132,17 @@ func (s *service) installByName( } } + // Local-store artifacts and raw layer data carry no registry signature + // to verify — installing them project-scoped is an unsigned trust + // decision that must be explicit. + if shouldVerifyInstall(opts, scope) { + decision, verifyErr := verifyLocalInstall(opts, opts.Name) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(&opts, decision) + } + result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { return nil, err diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 20170e11ab..105c163c29 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -619,16 +619,17 @@ func buildInstalledPlugin( Version: opts.Version, Description: opts.Description, }, - Scope: scope, - ProjectRoot: opts.ProjectRoot, - Reference: opts.Reference, - Tag: opts.Tag, - Digest: opts.Digest, - Status: plugins.InstallStatusInstalled, - InstalledAt: time.Now().UTC(), - Clients: clients, - Components: opts.Components, - Dependencies: opts.Dependencies, + Scope: scope, + ProjectRoot: opts.ProjectRoot, + Reference: opts.Reference, + Tag: opts.Tag, + Digest: opts.Digest, + Status: plugins.InstallStatusInstalled, + InstalledAt: time.Now().UTC(), + Clients: clients, + Components: opts.Components, + Dependencies: opts.Dependencies, + SigstoreBundle: opts.SigstoreBundle, } } diff --git a/pkg/plugins/pluginsvc/install_git.go b/pkg/plugins/pluginsvc/install_git.go index 39dde0f0ed..2a4b60af10 100644 --- a/pkg/plugins/pluginsvc/install_git.go +++ b/pkg/plugins/pluginsvc/install_git.go @@ -50,9 +50,9 @@ func (s *service) installFromGit( gitURL := opts.Name - // head carries the resolved commit's hash plus its (unverified) gitsign - // signature and signed payload. Only the hash is consumed today; the - // signature and payload are carried for install-time verification. + // head carries the resolved commit's hash (the install digest) plus its + // gitsign signature and the payload it covers, which install-time + // verification checks below before anything is written. files, manifest, head, err := s.cloneAndCollectPlugin(ctx, gitRef) if err != nil { return nil, httperr.WithCode( @@ -105,6 +105,17 @@ func (s *service) installFromGit( defer unlock() } + // Verify the commit signature before anything is written or recorded. + // This runs under the per-plugin lock so concurrent first installs + // cannot both read an absent lock entry and race their TOFU anchors. + if shouldVerifyInstall(opts, scope) { + decision, verifyErr := s.verifyGitInstall(ctx, opts, manifest.Name, head.Payload, head.Signature) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(&opts, decision) + } + result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { return nil, err diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index 6a4c549bda..a08ebaa626 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -121,6 +121,19 @@ func (s *service) installFromOCI( defer unlock() } + // Verify the artifact signature before anything is extracted or + // recorded; the decision (verified identity or explicit unsigned + // exception) travels on opts into the DB record and lock entry. This + // runs under the per-plugin lock so concurrent first installs cannot + // both read an absent lock entry and race their TOFU anchors. + if shouldVerifyInstall(opts, scope) { + decision, verifyErr := s.verifyOCIInstall(ctx, opts, pluginConfig.Name, ociRef, opts.Digest) + if verifyErr != nil { + return nil, verifyErr + } + applyDecisionToOpts(&opts, decision) + } + result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { return nil, err diff --git a/pkg/plugins/pluginsvc/lock.go b/pkg/plugins/pluginsvc/lock.go index 02b5ec234c..ab128d6150 100644 --- a/pkg/plugins/pluginsvc/lock.go +++ b/pkg/plugins/pluginsvc/lock.go @@ -53,6 +53,8 @@ func (s *service) recordLockState( ResolvedReference: resolvedReference, Digest: pl.Digest, ContentDigest: contentDigest, + Provenance: opts.Provenance, + Unsigned: opts.Unsigned, }); err != nil { return pl, fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) } @@ -75,6 +77,11 @@ type lockEntryInput struct { ResolvedReference string Digest string ContentDigest string + // Provenance is the verified signer identity to record, nil for + // unsigned or unverified entries. + Provenance *lockfile.Provenance + // Unsigned records the explicit unsigned-install exception. + Unsigned bool } // recordLockEntry upserts a single plugins: entry into projectRoot's lock @@ -94,6 +101,8 @@ func recordLockEntry(projectRoot string, in lockEntryInput) error { ResolvedReference: in.ResolvedReference, Digest: in.Digest, ContentDigest: in.ContentDigest, + Provenance: in.Provenance, + Unsigned: in.Unsigned, Explicit: true, } existing, exists := lf.GetPlugin(in.Name) diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index a9cbb40273..eb74eba2d1 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -70,7 +70,7 @@ func (*extractingAdapter) ScopeSupport() plugins.ScopeSupport { return plugins.ScopeSupport{} } -func newLockTestService(t *testing.T, enableGate bool) (plugins.PluginService, string) { +func newLockTestService(t *testing.T, enableGate bool, extra ...Option) (plugins.PluginService, string) { t.Helper() if enableGate { t.Setenv(plugins.LockFileEnvVar, "true") @@ -91,12 +91,12 @@ func newLockTestService(t *testing.T, enableGate bool) (plugins.PluginService, s home := t.TempDir() // Claude Code RelPath is empty; IsClientInstalled checks ~/.claude.json. require.NoError(t, os.WriteFile(filepath.Join(home, ".claude.json"), []byte("{}"), 0o644)) - svc := New( + opts := append([]Option{ WithStore(sqlite.NewPluginStore(db)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), WithClientManager(client.NewTestClientManagerWithHome(home)), - ) - return svc, projectRoot + }, extra...) + return New(opts...), projectRoot } func mustOpenRoot(t *testing.T, projectRoot string) lockfile.Root { @@ -125,12 +125,13 @@ func installTestPlugin(t *testing.T, svc plugins.PluginService, projectRoot, dig t.Helper() const name = "my-plugin" result, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: name, - LayerData: makePluginLayerData(t, name), - Digest: digest, - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: name, + LayerData: makePluginLayerData(t, name), + AllowUnsigned: true, + Digest: digest, + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) return result @@ -171,11 +172,12 @@ func TestInstallUserScope_DoesNotWriteLock(t *testing.T) { svc, projectRoot := newLockTestService(t, true) result, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeUser, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeUser, + Clients: []string{"claude-code"}, }) require.NoError(t, err) assert.False(t, result.Plugin.Managed) @@ -210,12 +212,13 @@ func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) _, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err, "install must fail when the lock file cannot be written") assert.Equal(t, http.StatusInternalServerError, httperr.Code(err)) @@ -273,12 +276,13 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { } _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), - Digest: validLockDigestAlt(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + AllowUnsigned: true, + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err, "reinstall must fail when marking the record managed fails") assert.Contains(t, err.Error(), "db update unavailable") @@ -327,12 +331,13 @@ func TestInstallProjectScope_RollbackCompensationErrorIsJoined(t *testing.T) { } _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), - Digest: validLockDigestAlt(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + AllowUnsigned: true, + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err) assert.Contains(t, err.Error(), "recording plugin in project lock file", @@ -366,12 +371,13 @@ func TestInstallProjectScope_RollbackKeepsPreExistingGroupMembership(t *testing. t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) _, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err, "install must fail when the lock entry cannot be written") // gomock verifies no gm.Update ran: rollback did not touch the @@ -646,12 +652,13 @@ func TestInstall_MaterializeFailureAfterExtractRemovesTree(t *testing.T) { } _, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err) assert.Contains(t, err.Error(), "marketplace write failed") @@ -692,12 +699,13 @@ func TestInstallProjectScope_LockWriteFailureRemovesGroupMembership(t *testing.T inner.groupManager = gm _, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err) assert.Empty(t, members, "a failed fresh install must not leave the plugin in the group") @@ -724,12 +732,13 @@ func TestInstallUpgrade_SecondClientFailureRestoresRegistration(t *testing.T) { ) _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) @@ -739,12 +748,13 @@ func TestInstallUpgrade_SecondClientFailureRestoresRegistration(t *testing.T) { assert.Contains(t, string(before), "my-plugin@toolhive") _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), - Digest: validLockDigestAlt(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"codex"}, + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + AllowUnsigned: true, + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"codex"}, }) require.Error(t, err) assert.Contains(t, err.Error(), "disk full") @@ -787,12 +797,13 @@ func TestUninstall_PartialDematerializeRestoresAllClients(t *testing.T) { ) _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code", "codex"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code", "codex"}, }) require.NoError(t, err) @@ -836,13 +847,14 @@ func TestInstallFresh_LockWriteFailureRestoresPreexistingTree(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, - Force: true, + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, }) require.Error(t, err) @@ -922,13 +934,14 @@ func TestInstallFresh_RollbackDoesNotRegisterUnmanagedTree(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, - Force: true, + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, }) require.Error(t, err) @@ -940,23 +953,34 @@ func TestInstallFresh_RollbackDoesNotRegisterUnmanagedTree(t *testing.T) { "rollback must not register a tree that was unregistered at snapshot time") } +// TestInstall_UnreadableLockFileAbortsBeforeMutating covers an unreadable +// lock file on a project-scope install. Install-time verification reads the +// entry's trust state before anything is extracted, so an unloadable lock +// file now fails there — earlier than installAndRegister's own snapshot, +// which keeps its Load guard only against a rewrite racing that window. +// Failing closed is the point: no DB record, no files, no lock entry. +// //nolint:paralleltest // uses t.Setenv via newLockTestService -func TestInstallAndRegister_LockSnapshotFailureRollsBackDB(t *testing.T) { +func TestInstall_UnreadableLockFileAbortsBeforeMutating(t *testing.T) { svc, projectRoot := newLockTestService(t, true) - // A lock path that is a directory makes Load fail after extraction. + // A lock path that is a directory makes Load fail. require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) _, err := svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.Error(t, err) - assert.Contains(t, err.Error(), "loading lock file") + assert.Contains(t, err.Error(), "reading lock trust state") + + _, statErr := os.Stat(filepath.Join(projectRoot, ".claude", "plugins", "my-plugin")) + assert.True(t, os.IsNotExist(statErr), "nothing may be extracted before the lock file can be read") info, infoErr := svc.Info(t.Context(), plugins.InfoOptions{ Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, diff --git a/pkg/plugins/pluginsvc/service.go b/pkg/plugins/pluginsvc/service.go index 9cca4a87a7..0a4b20e3ff 100644 --- a/pkg/plugins/pluginsvc/service.go +++ b/pkg/plugins/pluginsvc/service.go @@ -19,6 +19,7 @@ import ( "github.com/stacklok/toolhive/pkg/git" "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/verifier" "github.com/stacklok/toolhive/pkg/storage" ) @@ -131,6 +132,15 @@ func WithPluginLookup(pl PluginLookup) Option { } } +// WithVerifier sets the signature verifier used for install-time +// verification. Defaults to the Sigstore verifier with the composite +// registry keychain. +func WithVerifier(v verifier.Verifier) Option { + return func(s *service) { + s.sigVerifier = v + } +} + // pluginLock provides per-plugin mutual exclusion keyed by scope/name/projectRoot. // Entries are never evicted. This is acceptable because the number of distinct // plugins on a single machine is expected to remain small (< 1000). The key @@ -188,6 +198,7 @@ type service struct { pluginLookup PluginLookup gitClient git.Client clientManager *client.ClientManager + sigVerifier verifier.Verifier } // New creates a new plugin service and returns it as a plugins.PluginService. diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index e5f747710e..53fe9c4dae 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -527,6 +527,9 @@ func classifySyncFailure(err error) plugins.FailureReason { if errors.Is(err, errLockWrite) { return plugins.FailureReasonLockWriteFailed } + if reason := classifySignatureError(err); reason != "" { + return reason + } switch httperr.Code(err) { case http.StatusNotFound: return plugins.FailureReasonDigestMissing diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 335f5598dd..08adb64410 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -52,7 +52,12 @@ func (c *redirectGitClient) Cleanup(ctx context.Context, repoInfo *git.Repositor return c.inner.Cleanup(ctx, repoInfo) } -func newGitLockTestService(t *testing.T, repoDir string) (plugins.PluginService, string) { +// newGitLockTestService builds a lock-enabled service whose git installs +// resolve to repoDir. Test repo commits are unsigned, so it defaults to a +// verifier that reports every commit as signed by a fixed test identity — +// tests about verification itself pass their own WithVerifier via extra +// (later options win). +func newGitLockTestService(t *testing.T, repoDir string, extra ...Option) (plugins.PluginService, string) { t.Helper() t.Setenv(plugins.LockFileEnvVar, "true") @@ -69,13 +74,14 @@ func newGitLockTestService(t *testing.T, repoDir string) (plugins.PluginService, home := t.TempDir() // Claude Code RelPath is empty; IsClientInstalled checks ~/.claude.json. require.NoError(t, os.WriteFile(filepath.Join(home, ".claude.json"), []byte("{}"), 0o644)) - svc := New( + opts := append([]Option{ WithStore(sqlite.NewPluginStore(db)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), WithClientManager(client.NewTestClientManagerWithHome(home)), WithGitClient(&redirectGitClient{dir: repoDir, inner: git.NewDefaultGitClient()}), - ) - return svc, projectRoot + WithVerifier(alwaysSignedVerifier(t)), + }, extra...) + return New(opts...), projectRoot } func pluginOnDiskPath(projectRoot, name string) string { @@ -603,12 +609,13 @@ func TestSync_LocalUpgradeRoundTripRestoresExactDigest(t *testing.T) { inner.ociStore = ociStore _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) @@ -663,12 +670,13 @@ func TestSync_LocalStorePinDigestMissing(t *testing.T) { inner.ociStore = ociStore _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) diff --git a/pkg/plugins/pluginsvc/upgrade_test.go b/pkg/plugins/pluginsvc/upgrade_test.go index ee99cbbb68..95b2db27bd 100644 --- a/pkg/plugins/pluginsvc/upgrade_test.go +++ b/pkg/plugins/pluginsvc/upgrade_test.go @@ -219,12 +219,13 @@ func TestUpgrade_PlainNameResolvesLocalStoreWithoutRegistry(t *testing.T) { inner.pluginLookup = lookup _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) lookup.n = 0 @@ -252,12 +253,13 @@ func TestUpgrade_AppliesSameNameLocalTagWithoutRegistry(t *testing.T) { inner.pluginLookup = lookup _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) lookup.n = 0 @@ -308,12 +310,13 @@ func TestUpgrade_AppliesDifferentlyNamedLocalTagWithoutRegistry(t *testing.T) { inner.pluginLookup = lookup _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) lookup.n = 0 @@ -350,12 +353,13 @@ func TestUpgrade_PlainNameFallsBackToRegistryWhenLocalMisses(t *testing.T) { inner.ociStore = ociStore _, err = svc.Install(t.Context(), plugins.InstallOptions{ - Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), - Digest: validLockDigest(), - Scope: plugins.ScopeProject, - ProjectRoot: projectRoot, - Clients: []string{"claude-code"}, + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + AllowUnsigned: true, + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, }) require.NoError(t, err) diff --git a/pkg/plugins/pluginsvc/verify.go b/pkg/plugins/pluginsvc/verify.go new file mode 100644 index 0000000000..15daf0b915 --- /dev/null +++ b/pkg/plugins/pluginsvc/verify.go @@ -0,0 +1,287 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/container/images" + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/skills/verifier" +) + +// artifactVerifier returns the configured signature verifier, defaulting to +// the Sigstore verifier with the composite registry keychain. Plugins reuse +// the skills verifier wholesale: the Sigstore policy, the trust-on-first-use +// contract, and the pinned ref/runner enforcement are the same trust model +// applied to a different artifact type. +func (s *service) artifactVerifier() verifier.Verifier { + if s.sigVerifier != nil { + return s.sigVerifier + } + return verifier.NewDefault(images.NewCompositeKeychain()) +} + +// shouldVerifyInstall reports whether install-time signature verification +// applies: project-scope installs that record lock state. The lock file is +// where trust decisions live, so verification is scoped to exactly the +// installs that write one — including the feature gate, since a disabled +// lock file has nowhere to anchor trust on first use. +func shouldVerifyInstall(opts plugins.InstallOptions, scope plugins.Scope) bool { + return scope == plugins.ScopeProject && opts.ProjectRoot != "" && plugins.LockFileFeatureEnabled() +} + +// provenanceDecision is the outcome of install-time verification: either a +// verified identity (with the bundle backing it) or an explicit unsigned +// exception. +type provenanceDecision struct { + provenance *lockfile.Provenance + unsigned bool + bundle []byte +} + +// applyDecisionToOpts records the verification outcome on the install +// options, from where it flows into the installed-plugin record and the lock +// entry. +func applyDecisionToOpts(opts *plugins.InstallOptions, decision *provenanceDecision) { + if decision == nil { + return + } + opts.Provenance = decision.provenance + opts.Unsigned = decision.unsigned + opts.SigstoreBundle = decision.bundle +} + +// verifyOCIInstall verifies the signature of the OCI artifact at ref/digest +// before anything is extracted or recorded. The identity expected by the +// lock file (if any) is enforced inside the verifier's Sigstore policy; +// trust on first use records whatever identity verification observes. +func (s *service) verifyOCIInstall( + ctx context.Context, + opts plugins.InstallOptions, + pluginName, ref, digest string, +) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, pluginName) + if err != nil { + return nil, err + } + if expectUnsigned { + return unsignedLockedDecision(opts, pluginName) + } + + result, verifyErr := s.artifactVerifier().VerifyOCI(ctx, ref, digest, expected) + if verifyErr != nil { + if isAllowedUnsigned(verifyErr, opts, expected) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, classifyInstallVerifyError(verifyErr, pluginName, expected) + } + return &provenanceDecision{provenance: result.ToLockProvenance(), bundle: result.Bundle}, nil +} + +// verifyGitInstall verifies the gitsign signature on the resolved commit +// before anything is written or recorded. +func (s *service) verifyGitInstall( + ctx context.Context, + opts plugins.InstallOptions, + pluginName string, + payload []byte, + signature string, +) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, pluginName) + if err != nil { + return nil, err + } + if expectUnsigned { + return unsignedLockedDecision(opts, pluginName) + } + + result, verifyErr := s.artifactVerifier().VerifyGit(ctx, payload, []byte(signature), expected) + if verifyErr != nil { + if isAllowedUnsigned(verifyErr, opts, expected) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, classifyInstallVerifyError(verifyErr, pluginName, expected) + } + return &provenanceDecision{provenance: result.ToLockProvenance(), bundle: result.Bundle}, nil +} + +// verifyLocalInstall handles installs sourced from the local OCI store or +// raw layer data: there is no registry signature to verify, so the install +// is an unsigned trust decision. An entry already locked to a signer +// identity refuses a local replacement outright — swapping a verified +// artifact for a local build is exactly the substitution the lock exists to +// catch. +func verifyLocalInstall(opts plugins.InstallOptions, pluginName string) (*provenanceDecision, error) { + expected, expectUnsigned, err := expectedLockTrust(opts.ProjectRoot, pluginName) + if err != nil { + return nil, err + } + if expected != nil { + return nil, httperr.WithCode( + fmt.Errorf("plugin %q is locked to signer %q; a local build cannot satisfy it", + pluginName, expected.SignerIdentity), + http.StatusForbidden, + ) + } + if expectUnsigned { + return unsignedLockedDecision(opts, pluginName) + } + // A lock-driven restore of an entry with no recorded trust state + // materializes what install once accepted, so it records unsigned + // rather than demanding a flag the operation has no way to pass — + // the same allowance isAllowedUnsigned makes for OCI and git. + if !opts.AllowUnsigned && !lockDrivenInstall(opts) { + return nil, httperr.WithCode( + fmt.Errorf("local build for %q is unsigned; set allow_unsigned (--allow-unsigned) to record an exception", + pluginName), + http.StatusForbidden, + ) + } + return &provenanceDecision{unsigned: true}, nil +} + +// unsignedLockedDecision handles installs of entries the lock file already +// marks unsigned. A lock-driven operation honors the recorded decision (the +// lock file IS the policy it restores); a fresh user-driven install must +// repeat the explicit exception. +// +// SECURITY: this early return means an entry marked unsigned is installed +// without consulting the verifier at all — by design, but it makes a lock +// diff that converts `provenance:` to `unsigned: true` a trust DOWNGRADE +// that sync will honor silently. For plugins the blast radius is the whole +// client: an unsigned plugin can ship hooks and MCP servers the client +// loads. That conversion is exactly what lock file review must catch; it +// cannot happen without a lock file edit. +func unsignedLockedDecision(opts plugins.InstallOptions, pluginName string) (*provenanceDecision, error) { + if opts.AllowUnsigned || lockDrivenInstall(opts) { + return &provenanceDecision{unsigned: true}, nil + } + return nil, httperr.WithCode( + fmt.Errorf("plugin %q is locked as unsigned; set allow_unsigned (--allow-unsigned) to reinstall it", pluginName), + http.StatusForbidden, + ) +} + +// expectedLockTrust reads the trust state recorded in projectRoot's lock +// file for pluginName: the expected signer identity (nil on first use — the +// TOFU case), or that the entry was recorded unsigned. +func expectedLockTrust(projectRoot, pluginName string) (*lockfile.Provenance, bool, error) { + if projectRoot == "" { + return nil, false, nil + } + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return nil, false, fmt.Errorf("reading lock trust state for %q: %w", pluginName, err) + } + lf, err := lockfile.Load(root) + if err != nil { + return nil, false, fmt.Errorf("reading lock trust state for %q: %w", pluginName, err) + } + entry, ok := lf.GetPlugin(pluginName) + if !ok { + return nil, false, nil + } + if entry.Unsigned { + return nil, true, nil + } + return entry.Provenance, false, nil +} + +// isAllowedUnsigned reports whether a verification failure is the unsigned +// case AND the caller may proceed: either the explicit --allow-unsigned +// exception, or a sync restore of an entry with no recorded trust state +// (entries created before verification existed) — a restore materializes +// what install once accepted, and the outcome is recorded as unsigned so +// the trust state stops being ambiguous. An entry locked to a signer +// identity is never replaceable by an unsigned artifact. +func isAllowedUnsigned(verifyErr error, opts plugins.InstallOptions, expected *lockfile.Provenance) bool { + if !errors.Is(verifyErr, verifier.ErrUnsigned) || expected != nil { + return false + } + return opts.AllowUnsigned || lockDrivenInstall(opts) +} + +// lockDrivenInstall reports whether this install materializes an existing +// lock entry rather than making a new trust decision: sync restores and +// upgrade re-pins (all set internal-only options no HTTP caller can reach). +// Such operations honor the trust state the entry already records. +// +// ExpectedCanonicalName is part of the test where skills gets by with the +// other two: a plugin upgrade off the local store deliberately clears +// LockResolvedReference so sync restores by digest, so those two markers +// alone would misread it as a fresh user install and demand a flag the +// upgrade API has no way to pass. +func lockDrivenInstall(opts plugins.InstallOptions) bool { + return opts.SyncRestore || opts.LockResolvedReference != "" || opts.ExpectedCanonicalName != "" +} + +// classifyInstallVerifyError maps a verifier failure to the HTTP-coded +// error surfaced by the install API — always a 403; the allowed-unsigned +// path is handled before this is called. +func classifyInstallVerifyError( + verifyErr error, + pluginName string, + expected *lockfile.Provenance, +) error { + switch { + case errors.Is(verifyErr, verifier.ErrUnsigned): + if expected != nil { + return httperr.WithCode( + fmt.Errorf("plugin %q is locked to signer %q but the artifact is unsigned", + pluginName, expected.SignerIdentity), + http.StatusForbidden, + ) + } + return httperr.WithCode( + fmt.Errorf("unsigned plugin %q rejected; set allow_unsigned (--allow-unsigned) to record an exception", + pluginName), + http.StatusForbidden, + ) + // Checked before the broader ErrSignerMismatch case: pinnedFieldMismatch + // wraps both, so a ref/runner-only mismatch would otherwise be + // misreported as a signer-identity change below. + case errors.Is(verifyErr, verifier.ErrProvenanceFieldMismatch): + return httperr.WithCode( + fmt.Errorf("plugin %q's certificate no longer matches its pinned provenance: %w", pluginName, verifyErr), + http.StatusForbidden, + ) + case errors.Is(verifyErr, verifier.ErrSignerMismatch): + return httperr.WithCode( + fmt.Errorf("signer identity mismatch for %q: %w"+ + " (if the signer change is intended, remove the plugin's lock entry and reinstall)", + pluginName, verifyErr), + http.StatusForbidden, + ) + default: + return httperr.WithCode( + fmt.Errorf("signature verification failed for %q: %w", pluginName, verifyErr), + http.StatusForbidden, + ) + } +} + +// classifySignatureError maps verifier sentinels to typed failure reasons +// for sync/upgrade results. Returns "" when err is not a signature failure. +func classifySignatureError(err error) plugins.FailureReason { + switch { + // Checked before the broader ErrSignerMismatch case for the same reason + // as classifyInstallVerifyError above. + case errors.Is(err, verifier.ErrProvenanceFieldMismatch): + return plugins.FailureReasonProvenanceFieldMismatch + case errors.Is(err, verifier.ErrSignerMismatch): + return plugins.FailureReasonSignerMismatch + case errors.Is(err, verifier.ErrUnsigned): + return plugins.FailureReasonUnsignedRejected + case errors.Is(err, verifier.ErrSignatureInvalid): + return plugins.FailureReasonSignatureInvalid + default: + return "" + } +} diff --git a/pkg/plugins/pluginsvc/verify_test.go b/pkg/plugins/pluginsvc/verify_test.go new file mode 100644 index 0000000000..d47d480d4c --- /dev/null +++ b/pkg/plugins/pluginsvc/verify_test.go @@ -0,0 +1,375 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "fmt" + "net/http" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/skills/verifier" + verifiermocks "github.com/stacklok/toolhive/pkg/skills/verifier/mocks" +) + +const ( + testSignerIdentity = "/.github/workflows/release.yml" + testCertIssuer = "https://token.actions.githubusercontent.com" +) + +func signedResult() *verifier.Result { + return &verifier.Result{ + Signed: true, + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + RepositoryURI: "https://github.com/org/repo", + SigstoreURL: "https://rekor.sigstore.dev", + Bundle: []byte(`{"bundle":true}`), + } +} + +// alwaysSignedVerifier reports every artifact as signed by the fixed test +// identity, so tests exercising lock/sync/upgrade mechanics don't trip +// install-time verification. +func alwaysSignedVerifier(t *testing.T) verifier.Verifier { + t.Helper() + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + return mv +} + +// loadPluginLockEntry reads the fixture plugin's lock entry from projectRoot. +func loadPluginLockEntry(t *testing.T, projectRoot string) (lockfile.Entry, bool) { + t.Helper() + return readLockfile(t, projectRoot).GetPlugin("my-plugin") +} + +// gitInstall installs the fixture git plugin project-scoped. +func gitInstall(t *testing.T, svc plugins.PluginService, projectRoot string, mutate func(*plugins.InstallOptions)) error { + t.Helper() + opts := plugins.InstallOptions{ + Name: gitPluginRef, + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + } + if mutate != nil { + mutate(&opts) + } + _, err := svc.Install(t.Context(), opts) + return err +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_TOFURecordsProvenance(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + // First install: no lock entry yet — trust on first use, nil expected. + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(signedResult(), nil) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, nil)) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance, "TOFU must record the observed identity") + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) + assert.Equal(t, testCertIssuer, entry.Provenance.CertIssuer) + assert.False(t, entry.Unsigned) + + stored, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + assert.Equal(t, []byte(`{"bundle":true}`), stored.InstalledPlugin.SigstoreBundle, + "the bundle must be persisted with the install record for offline re-verification") + + // Second install: the recorded identity must flow into the verifier as + // the expected identity. + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ any, _, _ []byte, expected *lockfile.Provenance) (*verifier.Result, error) { + require.NotNil(t, expected, "the second install must enforce the recorded identity") + assert.Equal(t, testSignerIdentity, expected.SignerIdentity) + return signedResult(), nil + }) + require.NoError(t, gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.Force = true })) +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_UnsignedRejectedWithoutFlag(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + err := gitInstall(t, svc, projectRoot, nil) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + + _, ok := loadPluginLockEntry(t, projectRoot) + assert.False(t, ok, "a rejected install must not write a lock entry") + _, err = svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "a rejected install must not create a DB record") +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_UnsignedAcceptedWithFlag(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, + func(o *plugins.InstallOptions) { o.AllowUnsigned = true })) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + assert.True(t, entry.Unsigned, "the unsigned exception must be recorded") + assert.Nil(t, entry.Provenance) + + stored, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + assert.Nil(t, stored.InstalledPlugin.SigstoreBundle, "an unsigned install stores no bundle") +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_SignerMismatchRejectedAndLockIntact(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(signedResult(), nil) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, nil)) + + // The re-install is signed by someone else: the verifier reports a + // mismatch (the expected identity was bound into its policy). + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, verifier.ErrSignerMismatch) + err := gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.Force = true }) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + + // The prior trusted state is untouched. + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_LockedUnsignedRequiresFlagAgain(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, + func(o *plugins.InstallOptions) { o.AllowUnsigned = true })) + + // Reinstall without the flag: the locked unsigned exception does not + // silently renew — the verifier is not even consulted (the mock has no + // second expectation, so a call would fail the test). + err := gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.Force = true }) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) +} + +// TestInstallVerification_LockDrivenInstallHonorsRecordedTrust proves a sync +// restore of an unsigned-locked entry does not demand the flag again: the +// lock file already records the decision the restore is materializing. +// +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestInstallVerification_LockDrivenInstallHonorsRecordedTrust(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Nil()). + Return(nil, verifier.ErrUnsigned) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, + func(o *plugins.InstallOptions) { o.AllowUnsigned = true })) + + // Drop the install so sync has to restore it from the lock entry. + inner := svc.(*service) //nolint:forcetypeassert + require.NoError(t, os.RemoveAll(pluginOnDiskPath(projectRoot, "my-plugin"))) + require.NoError(t, inner.store.Delete(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)) + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Installed, "the restore must not demand allow_unsigned") + assert.Empty(t, result.Failed) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + assert.True(t, entry.Unsigned, "the restore keeps the recorded trust state") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallVerification_UserScopeSkipsVerification(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + // The mock has no expectations: any verifier call fails the test. + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + + svc, _ := newGitLockTestService(t, repoDir, WithVerifier(mv)) + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, + Scope: plugins.ScopeUser, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) +} + +// TestInstallVerification_GateDisabledSkipsVerification pins the feature-gate +// scoping: with the plugins lock file off there is nowhere to anchor trust on +// first use, so verification must not run at all. +// +//nolint:paralleltest // uses t.Setenv +func TestInstallVerification_GateDisabledSkipsVerification(t *testing.T) { + svc, projectRoot := newLockTestService(t, false, WithVerifier( + verifiermocks.NewMockVerifier(gomock.NewController(t)), // no expectations + )) + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err, "an unsigned local install must not be rejected while the gate is off") +} + +func TestVerifyLocalInstall(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opts plugins.InstallOptions + entry *lockfile.Entry + wantErr bool + unsigned bool + }{ + { + name: "no flag rejected", + opts: plugins.InstallOptions{}, + wantErr: true, + }, + { + name: "flag records unsigned", + opts: plugins.InstallOptions{AllowUnsigned: true}, + unsigned: true, + }, + { + name: "locked identity refuses local replacement even with flag", + opts: plugins.InstallOptions{AllowUnsigned: true}, + entry: &lockfile.Entry{ + Name: "local-plugin", + Source: "example.com/org/local-plugin", + ResolvedReference: "example.com/org/local-plugin:v1", + Digest: "sha256:" + strings.Repeat("a", 64), + Provenance: &lockfile.Provenance{ + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + }, + }, + wantErr: true, + }, + { + name: "locked unsigned honored with flag", + opts: plugins.InstallOptions{AllowUnsigned: true}, + entry: &lockfile.Entry{ + Name: "local-plugin", + Source: "example.com/org/local-plugin", + ResolvedReference: "example.com/org/local-plugin:v1", + Digest: "sha256:" + strings.Repeat("a", 64), + Unsigned: true, + }, + unsigned: true, + }, + { + name: "lock-driven upgrade of a local build needs no flag", + opts: plugins.InstallOptions{ExpectedCanonicalName: "local-plugin"}, + unsigned: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + projectRoot := makeProjectRoot(t) + if tc.entry != nil { + require.NoError(t, lockfile.UpsertPluginEntry(mustOpenRoot(t, projectRoot), *tc.entry)) + } + opts := tc.opts + opts.ProjectRoot = projectRoot + + decision, err := verifyLocalInstall(opts, "local-plugin") + if tc.wantErr { + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + return + } + require.NoError(t, err) + assert.Equal(t, tc.unsigned, decision.unsigned) + assert.Nil(t, decision.provenance) + }) + } +} + +func TestClassifySignatureError(t *testing.T) { + t.Parallel() + assert.Equal(t, plugins.FailureReasonSignerMismatch, classifySignatureError(verifier.ErrSignerMismatch)) + assert.Equal(t, plugins.FailureReasonUnsignedRejected, classifySignatureError(verifier.ErrUnsigned)) + assert.Equal(t, plugins.FailureReasonSignatureInvalid, classifySignatureError(verifier.ErrSignatureInvalid)) + assert.Equal(t, plugins.FailureReason(""), classifySignatureError(assert.AnError)) + + // A pinned ref/runner mismatch satisfies errors.Is against BOTH + // ErrSignerMismatch and ErrProvenanceFieldMismatch (see + // verifier.pinnedFieldMismatch) — the more specific reason must win, or + // every version bump on a ref-pinned plugin would misreport as a + // publisher change rather than a provenance-field change. + fieldMismatch := fmt.Errorf("%w: %w: locked to repository ref, but the artifact carries a different one", + verifier.ErrSignerMismatch, verifier.ErrProvenanceFieldMismatch) + assert.Equal(t, plugins.FailureReasonProvenanceFieldMismatch, classifySignatureError(fieldMismatch)) +} + +// TestClassifyInstallVerifyErrorDistinguishesProvenanceField covers the +// install-time (403) classification alongside TestClassifySignatureError's +// sync/upgrade coverage: a pinned ref/runner mismatch must not be reported +// to the operator as a signer-identity change. +func TestClassifyInstallVerifyErrorDistinguishesProvenanceField(t *testing.T) { + t.Parallel() + + fieldMismatch := fmt.Errorf("%w: %w: locked to repository ref, but the artifact carries a different one", + verifier.ErrSignerMismatch, verifier.ErrProvenanceFieldMismatch) + err := classifyInstallVerifyError(fieldMismatch, "some-plugin", &lockfile.Provenance{SignerIdentity: testSignerIdentity}) + assert.Contains(t, err.Error(), "no longer matches its pinned provenance", + "a provenance-field mismatch must lead with the field-specific wording, not the identity one") + assert.NotContains(t, err.Error(), "signer identity mismatch for", + "the identity-specific phrasing (distinct from ErrSignerMismatch's own wrapped message text) must not appear") + + identityMismatch := classifyInstallVerifyError( + verifier.ErrSignerMismatch, "some-plugin", &lockfile.Provenance{SignerIdentity: testSignerIdentity}) + assert.Contains(t, identityMismatch.Error(), "signer identity mismatch for", + "a genuine signer-identity mismatch keeps its existing wording") +} diff --git a/test/e2e/cli_plugins_lock_test.go b/test/e2e/cli_plugins_lock_test.go index 4fc2b3201e..ecb107d672 100644 --- a/test/e2e/cli_plugins_lock_test.go +++ b/test/e2e/cli_plugins_lock_test.go @@ -50,6 +50,55 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", return exitErr.ExitCode() } + Describe("project-scoped install signature verification", func() { + It("rejects an unsigned install without allow_unsigned and writes no lock entry", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-unsigned-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "An unsigned plugin that must be rejected") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusForbidden)) + + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + _, ok := lf.GetPlugin(pluginName) + Expect(ok).To(BeFalse(), "a rejected install must not write a lock entry") + }) + + It("records the unsigned exception in the lock entry when allow_unsigned is set", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-unsigned-allowed-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "An unsigned plugin installed by exception") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + entry, ok := lf.GetPlugin(pluginName) + Expect(ok).To(BeTrue()) + Expect(entry.Unsigned).To(BeTrue(), "the unsigned exception must be recorded in the lock entry") + Expect(entry.Provenance).To(BeNil()) + }) + }) + Describe("thv ai-plugin sync --check", func() { It("exits 0 when the project matches its lock file", func() { projectRoot := makeE2EProjectRoot() @@ -60,7 +109,8 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A clean plugin for CLI exit code testing") installResp := installPlugin(apiServer, installPluginE2ERequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -79,7 +129,8 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A drifted plugin for CLI exit code testing") installResp := installPlugin(apiServer, installPluginE2ERequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -122,7 +173,8 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A plugin whose registry will vanish") installResp := installPlugin(apiServer, installPluginE2ERequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -148,7 +200,8 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A plugin for the fresh-clone gate") installResp := installPlugin(apiServer, installPluginE2ERequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -178,7 +231,8 @@ var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "The original description") installResp := installPlugin(apiServer, installPluginE2ERequest{ - Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, }) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -219,6 +273,10 @@ type installPluginE2ERequest struct { Scope string `json:"scope,omitempty"` ProjectRoot string `json:"project_root,omitempty"` Clients []string `json:"clients,omitempty"` + // AllowUnsigned records the unsigned-install exception. The plugins + // published by these tests are unsigned, so project-scoped installs + // need it — the rejection path itself is covered below. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` } func installPlugin(server *e2e.Server, req installPluginE2ERequest) *http.Response { From 69fc61828a8331c26e75d91c7cf05ccfd855dabc Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 25 Aug 2026 15:10:27 +0200 Subject: [PATCH 2/2] Adapt plugin verify to ProvenanceExpectation 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 --- pkg/plugins/pluginsvc/verify.go | 9 +++++++-- pkg/plugins/pluginsvc/verify_test.go | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/plugins/pluginsvc/verify.go b/pkg/plugins/pluginsvc/verify.go index 15daf0b915..3ad0fcfb11 100644 --- a/pkg/plugins/pluginsvc/verify.go +++ b/pkg/plugins/pluginsvc/verify.go @@ -75,7 +75,11 @@ func (s *service) verifyOCIInstall( return unsignedLockedDecision(opts, pluginName) } - result, verifyErr := s.artifactVerifier().VerifyOCI(ctx, ref, digest, expected) + // The verifier takes a ProvenanceExpectation so it can distinguish a + // strict lock pin from the independently-optional catalog constraints + // added in #6420. Plugins only ever present a lock expectation today; + // NewLockExpectation(nil) is nil, preserving the TOFU case. + result, verifyErr := s.artifactVerifier().VerifyOCI(ctx, ref, digest, verifier.NewLockExpectation(expected)) if verifyErr != nil { if isAllowedUnsigned(verifyErr, opts, expected) { return &provenanceDecision{unsigned: true}, nil @@ -102,7 +106,8 @@ func (s *service) verifyGitInstall( return unsignedLockedDecision(opts, pluginName) } - result, verifyErr := s.artifactVerifier().VerifyGit(ctx, payload, []byte(signature), expected) + result, verifyErr := s.artifactVerifier().VerifyGit( + ctx, payload, []byte(signature), verifier.NewLockExpectation(expected)) if verifyErr != nil { if isAllowedUnsigned(verifyErr, opts, expected) { return &provenanceDecision{unsigned: true}, nil diff --git a/pkg/plugins/pluginsvc/verify_test.go b/pkg/plugins/pluginsvc/verify_test.go index d47d480d4c..21b6c7b1b9 100644 --- a/pkg/plugins/pluginsvc/verify_test.go +++ b/pkg/plugins/pluginsvc/verify_test.go @@ -100,9 +100,9 @@ func TestInstallVerification_TOFURecordsProvenance(t *testing.T) { // Second install: the recorded identity must flow into the verifier as // the expected identity. mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn(func(_ any, _, _ []byte, expected *lockfile.Provenance) (*verifier.Result, error) { + DoAndReturn(func(_ any, _, _ []byte, expected *verifier.ProvenanceExpectation) (*verifier.Result, error) { require.NotNil(t, expected, "the second install must enforce the recorded identity") - assert.Equal(t, testSignerIdentity, expected.SignerIdentity) + assert.Equal(t, verifier.NewLockExpectation(entry.Provenance), expected) return signedResult(), nil }) require.NoError(t, gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.Force = true }))