Reject unsigned upgrade candidates in both modes

docs/arch/12-skills-system.md

@@ -415,7 +415,7 @@ oci_tags table (reserved; not currently populated)
 
 RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080-skills-lock-file.md) adds a project-level `toolhive.lock.yaml`, committed at the project root, that pins the exact content of every project-scoped skill install — the same guarantee `package-lock.json`, `Cargo.lock`, and `go.sum` provide elsewhere. Two teammates (or a CI runner) cloning the same repo restore identical skill content via `thv skill sync`, rather than whatever the source currently resolves to.
 
-**Trust model, stated plainly:** project-scoped installs are verified against Sigstore signatures, and the lock file records the trust decisions those verifications produce. On first install of a signed skill the observed signer identity is recorded (trust on first use) as the entry's `provenance:` block and **displayed to the user**; every later install, sync, and upgrade enforces that identity *inside* the Sigstore verification policy — OCI artifacts through their attached signature bundles, git commits through gitsign signature-and-chain verification (currently `provisional: true`: the transparency-log proof of signing time is not yet validated, so the replay window is unbounded until that lands). Sync additionally re-verifies each entry's stored signature bundle offline (embedded trust root, no network) before counting it current, and upgrade refuses to move to an artifact signed by a different identity — or unsigned — without an explicit `--allow-signer-change`.
+**Trust model, stated plainly:** project-scoped installs are verified against Sigstore signatures, and the lock file records the trust decisions those verifications produce. On first install of a signed skill the observed signer identity is recorded (trust on first use) as the entry's `provenance:` block and **displayed to the user**; every later install, sync, and upgrade enforces that identity *inside* the Sigstore verification policy — OCI artifacts through their attached signature bundles, git commits through gitsign signature-and-chain verification (currently `provisional: true`: the transparency-log proof of signing time is not yet validated, so the replay window is unbounded until that lands). Sync additionally re-verifies each entry's stored signature bundle offline (embedded trust root, no network) before counting it current, and upgrade refuses to move to an artifact signed by a different identity without an explicit `--allow-signer-change`. An unsigned candidate is not a signer change but a failure (`unsigned-rejected`): upgrade has no unsigned-consent flag, and `--allow-signer-change` re-verifies from scratch, which an unsigned artifact still fails, so the failure names the project-scoped reinstall with `--allow-unsigned` that records the exception explicitly.
 
 **First install need not be blind.** When a skill resolves through the registry/catalog (a plain name like `thv skill install code-review`, not a direct OCI or git reference) and that catalog entry declares an expected `provenance` (`toolhive-core`'s `registry.Skill.Provenance`, RFC THV-0080 follow-up [#6310](https://github.com/stacklok/toolhive/issues/6310)), the very first project-scoped install is checked against it instead of unconditionally trusting whatever the artifact's signature says. Each non-empty supported field constrains independently; an empty or absent catalog provenance preserves ordinary trust on first use. For multi-signed OCI artifacts, every cryptographically valid signature is considered until one satisfies all declared constraints. A lock entry, including a legacy entry with no recorded trust state, always takes precedence over the catalog — the catalog only fills the gap before any trust decision has been recorded. User-scoped installs do not use project lock-file verification and therefore do not apply catalog provenance.
 

docs/arch/14-plugins-system.md

@@ -185,6 +185,11 @@ installing (OCI candidates are still fetched to compare digests).
 `--allow-ref-change` permits a repository move, while
 `--allow-signer-change` permits supported trust transitions. Neither option
 silently replaces a pinned cosign public key with an arbitrary different key.
+An unsigned candidate under an entry that records a signer is not a trust
+transition but a failure (`unsigned-rejected`): upgrade has no unsigned-consent
+flag, and `--allow-signer-change` re-verifies from scratch, which an unsigned
+artifact still fails, so the failure names the project-scoped reinstall with
+`--allow-unsigned` that records the exception explicitly.
 
 ## Storage
 

pkg/plugins/pluginsvc/upgrade.go

@@ -253,7 +253,10 @@ func (s *service) resolveSignerPolicy(
 	outcome *plugins.UpgradeOutcome,
 ) (allowSignerChange, blocked bool) {
 	if entry.Provenance == nil {
-		return opts.AllowSignerChange, false
+		// No recorded signer, so there is no signer change to authorize:
+		// forwarding the override would only clear the install's
+		// expectations for an entry that has none to clear.
+		return false, false
 	}
 	if entry.Provenance.PublicKey != "" {
 		// A keyed entry is measured against its pin whether or not the
@@ -266,17 +269,24 @@ func (s *service) resolveSignerPolicy(
 		}
 		return opts.AllowSignerChange && verdict.kind == keyedMovedToKeyless, false
 	}
-	if !opts.AllowSignerChange && s.guardSignerChange(ctx, entry, latest, outcome) {
+	if s.guardSignerChange(ctx, entry, latest, opts.AllowSignerChange, outcome) {
 		return false, true
 	}
 	return opts.AllowSignerChange, false
 }
 
 // guardSignerChange probes the candidate artifact's signer identity and
 // fills outcome when the upgrade must not proceed: the candidate is signed
-// by a different identity (or unsigned) versus the recorded provenance, its
-// signature cannot be verified at all, or its certificate's repository ref
-// or runner class differs from what is recorded. Returns true when blocked.
+// by a different identity versus the recorded provenance, its signature
+// cannot be verified at all, it is unsigned, or its certificate's repository
+// ref or runner class differs from what is recorded. Returns true when
+// blocked. Only a differing identity or provenance field is reported as a
+// signer change, and only that arm is waived by allowSignerChange; the rest
+// are failures in both modes, because --allow-signer-change would not
+// resolve them — it re-verifies from scratch, which an unsigned or
+// unverifiable candidate fails just the same. Running the guard under the
+// override is what keeps a project-wide flag from waving those through to
+// an install that then either refuses them or, worse, records them.
 //
 // The repository ref has NO automatic allowance for a tag-shaped rotation.
 // An earlier version of the skills guard this mirrors let a recorded tag
@@ -296,12 +306,25 @@ func (s *service) guardSignerChange(
 	ctx context.Context,
 	entry lockfile.Entry,
 	latest resolvedLatest,
+	allowSignerChange bool,
 	outcome *plugins.UpgradeOutcome,
 ) bool {
 	probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest)
 	switch {
 	case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned):
-		outcome.Status = plugins.UpgradeStatusSignerChangeBlocked
+		// Not a signer change, because the remedy a signer change names
+		// cannot resolve it: --allow-signer-change re-verifies from scratch
+		// and re-records what it observes, and an unsigned artifact fails
+		// that verification exactly as it fails this one. Upgrade has no
+		// unsigned-consent flag, so the only way onto an unsigned artifact
+		// is the reinstall that records the exception explicitly.
+		outcome.Status = plugins.UpgradeStatusFailed
+		outcome.Reason = plugins.FailureReasonUnsignedRejected
+		outcome.Error = fmt.Errorf("candidate is unsigned, and this entry is pinned to a signer"+
+			" identity: %w. Upgrade has no unsigned-consent flag, and --allow-signer-change is not"+
+			" one — it re-verifies from scratch, which an unsigned artifact still fails. To move"+
+			" this plugin to an unsigned artifact, reinstall it: %s",
+			probeErr, projectReinstallCommand(entry, "--allow-unsigned")).Error()
 		return true
 	case probeErr != nil:
 		outcome.Status = plugins.UpgradeStatusFailed
@@ -315,6 +338,11 @@ func (s *service) guardSignerChange(
 		probe.CertIssuer != entry.Provenance.CertIssuer ||
 		runnerEnvironmentChanged(probe, entry.Provenance) ||
 		repositoryRefChanged(probe, entry.Provenance):
+		// The one arm the override resolves: it re-verifies from scratch
+		// and re-records the identity actually observed.
+		if allowSignerChange {
+			return false
+		}
 		outcome.Status = plugins.UpgradeStatusSignerChangeBlocked
 		outcome.NewSignerIdentity = probe.SignerIdentity
 		return true

pkg/plugins/pluginsvc/upgrade_verify_test.go

@@ -133,14 +133,30 @@ func TestUpgrade_SignerChangeBlocked(t *testing.T) {
 }
 
 //nolint:paralleltest // serial: real sqlite + on-disk client materialization per test
-func TestUpgrade_UnsignedCandidateBlockedAgainstSignedEntry(t *testing.T) {
+func TestUpgrade_UnsignedCandidateRejectedAgainstSignedEntry(t *testing.T) {
 	svc, projectRoot := signerChangeFixture(t, func() (*verifier.Result, error) {
 		return nil, verifier.ErrUnsigned
 	})
 
-	outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{ProjectRoot: projectRoot})
-	assert.Equal(t, plugins.UpgradeStatusSignerChangeBlocked, outcome.Status)
-	assert.Empty(t, outcome.NewSignerIdentity, "an unsigned candidate has no identity to report")
+	// A signer change is resolved by --allow-signer-change, which re-verifies
+	// from scratch and fails on an unsigned artifact just the same, and
+	// upgrade has no unsigned-consent flag — so this is a failure in both
+	// modes, naming the reinstall that actually records the exception.
+	for _, override := range []bool{false, true} {
+		outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{
+			ProjectRoot: projectRoot, AllowSignerChange: override,
+		})
+		assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status, "allow_signer_change=%v", override)
+		assert.Equal(t, plugins.FailureReasonUnsignedRejected, outcome.Reason, "allow_signer_change=%v", override)
+		assert.Contains(t, outcome.Error, "--scope project --allow-unsigned",
+			"the remedy must be the reinstall that records the exception")
+		assert.Empty(t, outcome.NewSignerIdentity, "an unsigned candidate has no identity to report")
+	}
+
+	entry, ok := loadPluginLockEntry(t, projectRoot)
+	require.True(t, ok)
+	require.NotNil(t, entry.Provenance)
+	assert.False(t, entry.Unsigned, "no unsigned exception may be recorded without --allow-unsigned")
 }
 
 // TestUpgrade_ProvenanceFieldDivergenceBlocked covers the fields beyond the
@@ -361,6 +377,41 @@ func TestUpgrade_UnaffectedEntrySkipsSignerProbe(t *testing.T) {
 		"the guard must not probe an entry that records no signer identity")
 }
 
+// TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride is the
+// override-mode twin of TestUpgrade_UnaffectedEntrySkipsSignerProbe: an entry
+// the lock records as unsigned has no signer to change, so a project-wide
+// --allow-signer-change must leave it exactly as a plain upgrade would rather
+// than clearing expectUnsigned and refusing the artifact the lock accepts.
+//
+//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test
+func TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride(t *testing.T) {
+	repoDir := createPluginTestRepo(t, "")
+
+	calls := 0
+	mv := verifiermocks.NewMockVerifier(gomock.NewController(t))
+	mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+		AnyTimes().
+		DoAndReturn(func(_ any, _, _ []byte, _ *verifier.ProvenanceExpectation) (*verifier.Result, error) {
+			calls++
+			return nil, verifier.ErrUnsigned
+		})
+	mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+	svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv))
+	require.NoError(t, gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.AllowUnsigned = true }))
+	require.Equal(t, 1, calls, "only the install itself consulted the verifier")
+
+	addPluginRepoCommit(t, repoDir, "# hello unsigned")
+	outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{ProjectRoot: projectRoot, AllowSignerChange: true})
+	assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, "error: %s", outcome.Error)
+	assert.Equal(t, 1, calls, "an entry with no recorded signer is never probed, override or not")
+
+	after, ok := loadPluginLockEntry(t, projectRoot)
+	require.True(t, ok)
+	assert.True(t, after.Unsigned, "the recorded unsigned decision is preserved")
+	assert.Nil(t, after.Provenance)
+}
+
 func TestRepositoryRefChanged(t *testing.T) {
 	t.Parallel()
 
@@ -543,7 +594,7 @@ func TestUpgrade_OversizedCommitMaterialFailsRatherThanBlocks(t *testing.T) {
 			Provenance: &lockfile.Provenance{SignerIdentity: testSignerIdentity},
 		},
 		resolvedLatest{commitPayload: make([]byte, maxSignatureBlobSize+1), commitSignature: "sig"},
-		&outcome)
+		false, &outcome)
 
 	assert.True(t, blocked, "an unusable probe must stop the upgrade")
 	assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status)

pkg/skills/skillsvc/upgrade.go

@@ -227,7 +227,10 @@ func (s *service) resolveSignerPolicy(
 	outcome *skills.UpgradeOutcome,
 ) (allowSignerChange, blocked bool) {
 	if entry.Provenance == nil {
-		return opts.AllowSignerChange, false
+		// No recorded signer, so there is no signer change to authorize:
+		// forwarding the override would only clear the install's
+		// expectations for an entry that has none to clear.
+		return false, false
 	}
 	if entry.Provenance.PublicKey != "" {
 		// A keyed entry is measured against its pin whether or not the
@@ -240,17 +243,24 @@ func (s *service) resolveSignerPolicy(
 		}
 		return opts.AllowSignerChange && verdict.kind == keyedMovedToKeyless, false
 	}
-	if !opts.AllowSignerChange && s.guardSignerChange(ctx, entry, newRef, newDigest, outcome) {
+	if s.guardSignerChange(ctx, entry, newRef, newDigest, opts.AllowSignerChange, outcome) {
 		return false, true
 	}
 	return opts.AllowSignerChange, false
 }
 
 // guardSignerChange probes the candidate artifact's signer identity and
 // fills outcome when the upgrade must not proceed: the candidate is signed
-// by a different identity (or unsigned) versus the recorded provenance, its
-// signature cannot be verified at all, or its certificate's repository ref
-// or runner class differs from what is recorded. Returns true when blocked.
+// by a different identity versus the recorded provenance, its signature
+// cannot be verified at all, it is unsigned, or its certificate's repository
+// ref or runner class differs from what is recorded. Returns true when
+// blocked. Only a differing identity or provenance field is reported as a
+// signer change, and only that arm is waived by allowSignerChange; the rest
+// are failures in both modes, because --allow-signer-change would not
+// resolve them — it re-verifies from scratch, which an unsigned or
+// unverifiable candidate fails just the same. Running the guard under the
+// override is what keeps a project-wide flag from waving those through to
+// an install that then either refuses them or, worse, records them.
 //
 // The repository ref has NO automatic allowance for a tag-shaped rotation.
 // An earlier version of this guard let a recorded tag ref rotate to any
@@ -269,12 +279,25 @@ func (s *service) guardSignerChange(
 	ctx context.Context,
 	entry lockfile.Entry,
 	newRef, newDigest string,
+	allowSignerChange bool,
 	outcome *skills.UpgradeOutcome,
 ) bool {
 	probe, probeErr := s.probeCandidateSigner(ctx, newRef, newDigest)
 	switch {
 	case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned):
-		outcome.Status = skills.UpgradeStatusSignerChangeBlocked
+		// Not a signer change, because the remedy a signer change names
+		// cannot resolve it: --allow-signer-change re-verifies from scratch
+		// and re-records what it observes, and an unsigned artifact fails
+		// that verification exactly as it fails this one. Upgrade has no
+		// unsigned-consent flag, so the only way onto an unsigned artifact
+		// is the reinstall that records the exception explicitly.
+		outcome.Status = skills.UpgradeStatusFailed
+		outcome.Reason = skills.FailureReasonUnsignedRejected
+		outcome.Error = fmt.Errorf("candidate is unsigned, and this entry is pinned to a signer"+
+			" identity: %w. Upgrade has no unsigned-consent flag, and --allow-signer-change is not"+
+			" one — it re-verifies from scratch, which an unsigned artifact still fails. To move"+
+			" this skill to an unsigned artifact, reinstall it: %s",
+			probeErr, projectReinstallCommand(entry, "--allow-unsigned")).Error()
 		return true
 	case probeErr != nil:
 		outcome.Status = skills.UpgradeStatusFailed
@@ -288,6 +311,11 @@ func (s *service) guardSignerChange(
 		probe.CertIssuer != entry.Provenance.CertIssuer ||
 		runnerEnvironmentChanged(probe, entry.Provenance) ||
 		repositoryRefChanged(probe, entry.Provenance):
+		// The one arm the override resolves: it re-verifies from scratch
+		// and re-records the identity actually observed.
+		if allowSignerChange {
+			return false
+		}
 		outcome.Status = skills.UpgradeStatusSignerChangeBlocked
 		outcome.NewSignerIdentity = probe.SignerIdentity
 		return true