Honour a pinned cosign key on plugin sync and upgrade

cmd/thv/app/ai_plugin_upgrade_test.go

@@ -93,3 +93,62 @@ func TestPluginUpgradeAllowSignerChangeFlag(t *testing.T) {
 	assert.True(t, aiPluginUpgradeAllowSignerChange,
 		"the flag must bind to the variable threaded into plugins.UpgradeOptions")
 }
+
+// TestPrintPluginUpgradeResultSignerRendering pins the distinction the
+// blocked-outcome line draws between a candidate that changed signer and one
+// that has no signer at all. The renderer infers "unsigned" from an absent
+// NewSignerIdentity, so that field is load-bearing: a keyless candidate that
+// reaches this point unnamed is printed as unsigned, telling the user the
+// artifact carries no signature when it carries one they have not approved.
+//
+//nolint:paralleltest // Test captures os.Stdout which cannot be done in parallel
+func TestPrintPluginUpgradeResultSignerRendering(t *testing.T) {
+	tests := []struct {
+		name       string
+		outcome    plugins.UpgradeOutcome
+		wantOutput string
+	}{
+		{
+			name: "a key-to-keyless move names the identity it moved to",
+			outcome: plugins.UpgradeOutcome{
+				Name:              "keyed-plugin",
+				Status:            plugins.UpgradeStatusSignerChangeBlocked,
+				NewSignerIdentity: "ci@example.com",
+			},
+			wantOutput: "keyed-plugin: signer change blocked (candidate is ci@example.com;" +
+				" use --allow-signer-change)\n",
+		},
+		{
+			name: "only a candidate with no identity is called unsigned",
+			outcome: plugins.UpgradeOutcome{
+				Name:   "keyless-plugin",
+				Status: plugins.UpgradeStatusSignerChangeBlocked,
+			},
+			wantOutput: "keyless-plugin: signer change blocked (candidate is unsigned;" +
+				" use --allow-signer-change)\n",
+		},
+		{
+			// The keyed guard routes an unsigned candidate here instead, so
+			// the flag above is never suggested for one.
+			name: "an unsigned candidate under a pinned key reports the rejection",
+			outcome: plugins.UpgradeOutcome{
+				Name:   "keyed-plugin",
+				Status: plugins.UpgradeStatusFailed,
+				Reason: plugins.FailureReasonUnsignedRejected,
+				Error:  "candidate is unsigned, and this entry is pinned to a cosign public key",
+			},
+			wantOutput: "keyed-plugin: failed [unsigned-rejected]: candidate is unsigned," +
+				" and this entry is pinned to a cosign public key\n",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			output := captureStdout(t, func() {
+				require.NoError(t, printPluginUpgradeResult(
+					&plugins.UpgradeResult{Outcomes: []plugins.UpgradeOutcome{tc.outcome}},
+					FormatText, false))
+			})
+			assert.Equal(t, tc.wantOutput, output)
+		})
+	}
+}

pkg/plugins/pluginsvc/sync.go

@@ -404,13 +404,14 @@ func (s *service) syncUnlockedInstall(
 }
 
 // verifyStoredSignature re-verifies the Sigstore bundle stored with an
-// installed plugin against the identity its lock entry records — entirely
-// offline, via the embedded trust root, so sync never contacts a registry to
-// decide whether an entry is current. An entry recorded unsigned has nothing
-// to verify. A recorded identity with no stored bundle fails closed for OCI
-// installs (the bundle should exist); git installs never store a bundle —
-// their signature lives on the commit and is re-verified when content is
-// re-resolved.
+// installed plugin against the trust anchor its lock entry records — a
+// certificate identity, or a cosign public key for a key-pinned entry —
+// entirely offline, via the embedded trust root, so sync never contacts a
+// registry to decide whether an entry is current. An entry recorded unsigned
+// has nothing to verify. A recorded anchor with no stored bundle fails closed
+// for OCI installs (the bundle should exist); git installs never store a
+// bundle — their signature lives on the commit and is re-verified when
+// content is re-resolved.
 //
 // An entry carrying neither a signer identity nor unsigned: true is reported
 // as drift rather than accepted. Every install and adoption path records
@@ -443,12 +444,36 @@ func (s *service) verifyStoredSignature(entry lockfile.Entry, pl plugins.Install
 		if !strings.Contains(entry.Digest, ":") {
 			return nil // git install: no stored bundle by design
 		}
-		return fmt.Errorf("%w: lock entry records signer %q but no bundle is stored",
-			verifier.ErrSignatureInvalid, entry.Provenance.SignerIdentity)
+		return fmt.Errorf("%w: lock entry is pinned to %s but no bundle is stored",
+			verifier.ErrSignatureInvalid, lockedAnchorDescription(entry.Provenance))
+	}
+	if entry.Provenance.PublicKey != "" {
+		return s.verifyStoredKeySignature(entry, pl)
 	}
 	return s.artifactVerifier().VerifyBundleOffline(pl.SigstoreBundle, entry.Digest, entry.Provenance)
 }
 
+// verifyStoredKeySignature re-verifies a key-signed stored bundle against the
+// public key its lock entry pins. Without this the keyless path rejects the
+// entry outright, and sync reads that as drift it can heal by reinstalling —
+// so a key-pinned plugin reported as modified on every run and never settled,
+// while --check failed permanently on a project that was in fact intact.
+//
+// What the signature is checked against is the lock entry's digest — the
+// artifact the project is pinned to. A cosign signature covers a
+// simple-signing payload rather than the artifact, but that payload is stored
+// with the bundle, so it is recovered from there and checked to name this
+// digest. Nothing is rebuilt from a reference: a payload reconstructed from a
+// reference verifies against whatever that reference claims, which is exactly
+// the check a signature lifted from another artifact passes.
+func (s *service) verifyStoredKeySignature(entry lockfile.Entry, pl plugins.InstalledPlugin) error {
+	pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey)
+	if err != nil {
+		return fmt.Errorf("%w: lock entry's pinned %s", verifier.ErrSignatureInvalid, err.Error())
+	}
+	return s.artifactVerifier().VerifyBundleOfflineWithKey(pl.SigstoreBundle, entry.Digest, pubKeyPEM)
+}
+
 // adoptLocked writes a lock entry for an existing, unmanaged project-scope
 // install, pinning its current on-disk state and assuming the per-plugin lock
 // is already held. The install's own Reference is used as Source: an adopted
@@ -533,11 +558,34 @@ func (s *service) adoptLocked(ctx context.Context, opts plugins.SyncOptions, pl
 // exists its identity is back-filled into the lock entry, and when it does not
 // adopting is the same trust decision as an unsigned install — it records an
 // explicit unsigned exception, which the caller must have opted into.
+//
+// A key-signed bundle is the one case with no landing place: it reveals no
+// identity to back-fill and does not carry the key that would anchor it, so it
+// is refused rather than recorded as unsigned.
 func (s *service) adoptionTrust(
 	opts plugins.SyncOptions, pl plugins.InstalledPlugin,
 ) (*lockfile.Provenance, bool, error) {
 	if len(pl.SigstoreBundle) > 0 {
 		result, err := s.artifactVerifier().ResultFromBundle(pl.SigstoreBundle, pl.Digest)
+		if errors.Is(err, verifier.ErrKeySigned) {
+			// Adoption back-fills trust from what the bundle itself reveals,
+			// and a key-signed bundle reveals nothing: the key is not in it,
+			// and sync takes no --public-key to supply one. Recording the
+			// install as unsigned instead would be a lie about a signed
+			// artifact, so the honest move is to send it through the one
+			// path that can anchor it.
+			return nil, false, httperr.WithCode(
+				fmt.Errorf("%w: plugin %q is signed with a cosign key pair, so adopting it cannot"+
+					" record a trust anchor — the key is carried neither by the artifact nor by its"+
+					" bundle. Install it project-scoped against the key instead, which verifies the"+
+					" signature and pins it: `thv ai-plugin install %s --scope project --public-key"+
+					" <path-or-base64>` (add --project-root if you are not in the project directory;"+
+					" --public-key applies only project-scoped, and --allow-unsigned is not a"+
+					" substitute because the artifact is signed)",
+					err, pl.Metadata.Name, pl.Metadata.Name),
+				http.StatusForbidden,
+			)
+		}
 		if err != nil {
 			return nil, false, fmt.Errorf("verifying stored bundle for adoption: %w", err)
 		}

pkg/plugins/pluginsvc/sync_verify_test.go

@@ -4,13 +4,15 @@
 package pluginsvc
 
 import (
+	"net/http"
 	"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"
@@ -311,3 +313,191 @@ func TestSyncMigratesUnrecordedTrustEntry(t *testing.T) {
 	assert.True(t, migrated.Unsigned, "the exception the user asked for must be recorded")
 	assert.Nil(t, migrated.Provenance)
 }
+
+// keyPinnedSyncFixture installs the fixture plugin, then rewrites its lock
+// entry into the shape a key-verified OCI install leaves behind — pinned to
+// testPublicKeyB64, with a stored bundle to re-verify offline. The install
+// itself goes through the local path because what is under test is sync's
+// re-verification, not how the entry came to be pinned.
+func keyPinnedSyncFixture(t *testing.T, mv verifier.Verifier) (*service, string) {
+	t.Helper()
+
+	svc, projectRoot := newLockTestService(t, WithVerifier(mv))
+	installTestPlugin(t, svc, projectRoot, validLockDigest())
+	inner := svc.(*service) //nolint:forcetypeassert
+
+	entry, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin")
+	require.True(t, ok)
+	entry.Unsigned = false
+	entry.Provenance = &lockfile.Provenance{PublicKey: testPublicKeyB64}
+	entry.ResolvedReference = "ghcr.io/org/my-plugin@" + validLockDigest()
+	require.NoError(t, lockfile.UpsertPluginEntry(mustOpenRoot(t, projectRoot), entry))
+
+	stored, err := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)
+	require.NoError(t, err)
+	stored.SigstoreBundle = []byte(`{"bundle":true}`)
+	require.NoError(t, inner.store.Update(t.Context(), stored))
+
+	return inner, projectRoot
+}
+
+// TestVerifyStoredSignature_KeyPinnedEntry covers the branch that keeps a
+// key-pinned project from reporting drift forever. The keyless path refuses a
+// key-pinned entry outright, and sync reads a refusal as drift it can heal by
+// reinstalling — so before this branch existed the plugin was reported
+// modified on every run and --check failed permanently on an intact project.
+func TestVerifyStoredSignature_KeyPinnedEntry(t *testing.T) {
+	t.Parallel()
+
+	keyPEM, err := verifier.DecodePublicKey(testPublicKeyB64)
+	require.NoError(t, err)
+	bundle := []byte(`{"bundle":true}`)
+
+	t.Run("verifies against the pinned key, not the keyless path", func(t *testing.T) {
+		t.Parallel()
+		entry := keyedLockEntry("keyed-plugin")
+		mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+		mv.EXPECT().VerifyBundleOfflineWithKey(bundle, entry.Digest, keyPEM).Return(nil)
+		// Not merely "the key path was taken": reaching the keyless path at
+		// all is the bug, and it fails closed in a way that looks like drift.
+		mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+
+		svc := &service{sigVerifier: mv}
+		require.NoError(t, svc.verifyStoredSignature(entry, plugins.InstalledPlugin{SigstoreBundle: bundle}))
+	})
+
+	t.Run("a real verification failure still propagates", func(t *testing.T) {
+		t.Parallel()
+		mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+		mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()).
+			Return(verifier.ErrSignatureInvalid)
+
+		svc := &service{sigVerifier: mv}
+		require.ErrorIs(t,
+			svc.verifyStoredSignature(keyedLockEntry("keyed-plugin"),
+				plugins.InstalledPlugin{SigstoreBundle: bundle}),
+			verifier.ErrSignatureInvalid)
+	})
+
+	t.Run("an undecodable pinned key fails closed", func(t *testing.T) {
+		t.Parallel()
+		entry := keyedLockEntry("keyed-plugin")
+		entry.Provenance = &lockfile.Provenance{PublicKey: "not-base64!!"}
+		mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+		mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+
+		svc := &service{sigVerifier: mv}
+		require.ErrorIs(t,
+			svc.verifyStoredSignature(entry, plugins.InstalledPlugin{SigstoreBundle: bundle}),
+			verifier.ErrSignatureInvalid)
+	})
+
+	t.Run("a missing bundle names the key anchor, not an empty signer", func(t *testing.T) {
+		t.Parallel()
+		svc := &service{sigVerifier: verifiermocks.NewMockVerifier(gomock.NewController(t))}
+		err := svc.verifyStoredSignature(keyedLockEntry("keyed-plugin"), plugins.InstalledPlugin{})
+		require.ErrorIs(t, err, verifier.ErrSignatureInvalid)
+		assert.Contains(t, err.Error(), "a cosign public key")
+		assert.NotContains(t, err.Error(), `signer ""`,
+			"a key entry records no signer identity; the keyless phrasing named an empty string")
+	})
+}
+
+// TestSync_KeyPinnedEntrySettles is the end-to-end statement of the bug: a
+// key-pinned project must report as intact. Before the key branch existed
+// `sync --check` failed permanently on a project nothing was wrong with, and
+// an apply reinstalled the plugin on every single run.
+//
+//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test
+func TestSync_KeyPinnedEntrySettles(t *testing.T) {
+	keyPEM, err := verifier.DecodePublicKey(testPublicKeyB64)
+	require.NoError(t, err)
+
+	mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+	mv.EXPECT().VerifyBundleOfflineWithKey([]byte(`{"bundle":true}`), validLockDigest(), keyPEM).
+		AnyTimes().Return(nil)
+	// The keyless verifier cannot check a key-pair bundle; routing there is
+	// what produced the permanent drift report.
+	mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+
+	inner, projectRoot := keyPinnedSyncFixture(t, mv)
+
+	checked, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"my-plugin"}, checked.AlreadyCurrent)
+	assert.Empty(t, checked.Drifted, "--check must succeed on an intact key-pinned project")
+	assert.Empty(t, checked.Failed)
+
+	// Apply mode must agree: an entry that verifies is not repaired.
+	applied, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"my-plugin"}, applied.AlreadyCurrent)
+	assert.Empty(t, applied.Installed, "a settled key-pinned entry must not be reinstalled")
+}
+
+// TestSync_AdoptRefusesKeySignedInstall covers the one place a key-signed
+// artifact has no path through: adoption back-fills trust from what the stored
+// bundle reveals, and a key-pair bundle reveals no identity and does not carry
+// the key. Recording it as unsigned instead would file a false trust decision
+// about an artifact that IS signed, so the refusal has to name the route that
+// can anchor it.
+//
+//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test
+func TestSync_AdoptRefusesKeySignedInstall(t *testing.T) {
+	for _, allowUnsigned := range []bool{false, true} {
+		mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+		mv.EXPECT().ResultFromBundle(gomock.Any(), gomock.Any()).
+			AnyTimes().Return(nil, verifier.ErrKeySigned)
+		mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).
+			AnyTimes().Return(nil)
+
+		svc, projectRoot := newLockTestService(t, WithVerifier(mv))
+		installTestPlugin(t, svc, projectRoot, validLockDigest())
+		inner := svc.(*service) //nolint:forcetypeassert
+
+		// Strip it back to the unmanaged state a pre-lock-tracking install is
+		// in, but leave a stored bundle so adoption has something to read.
+		require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin"))
+		legacy, err := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)
+		require.NoError(t, err)
+		legacy.Managed = false
+		legacy.Reference = "ghcr.io/org/my-plugin:v1"
+		legacy.SigstoreBundle = []byte(`{"bundle":true}`)
+		require.NoError(t, inner.store.Update(t.Context(), legacy))
+
+		result, err := inner.Sync(t.Context(), plugins.SyncOptions{
+			ProjectRoot: projectRoot, Adopt: true, AllowUnsigned: allowUnsigned,
+		})
+		require.NoError(t, err)
+		require.Len(t, result.Failed, 1, "allow_unsigned=%v", allowUnsigned)
+		assert.Equal(t, plugins.FailureReasonKeySigned, result.Failed[0].Reason,
+			"--allow-unsigned is not a substitute: the artifact is signed (allow_unsigned=%v)", allowUnsigned)
+		assert.Contains(t, result.Failed[0].Error, "--scope project --public-key",
+			"the refusal must name the path that can anchor it, not merely refuse")
+
+		_, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin")
+		assert.False(t, ok, "a refused adoption must write nothing")
+	}
+}
+
+// TestAdoptionTrust_KeySignedIsForbidden pins the status code and wrapped
+// sentinel directly, which the Sync-level test only sees through the typed
+// failure reason.
+func TestAdoptionTrust_KeySignedIsForbidden(t *testing.T) {
+	t.Parallel()
+
+	mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+	mv.EXPECT().ResultFromBundle(gomock.Any(), gomock.Any()).Return(nil, verifier.ErrKeySigned)
+
+	svc := &service{sigVerifier: mv}
+	_, _, err := svc.adoptionTrust(
+		plugins.SyncOptions{AllowUnsigned: true},
+		plugins.InstalledPlugin{
+			SigstoreBundle: []byte(`{"bundle":true}`),
+			Metadata:       plugins.PluginMetadata{Name: "keyed-plugin"},
+		})
+
+	require.ErrorIs(t, err, verifier.ErrKeySigned)
+	assert.Equal(t, http.StatusForbidden, httperr.Code(err))
+	assert.Contains(t, err.Error(), "--allow-unsigned is not a substitute")
+}