diff --git a/cmd/thv/app/ai_plugin_upgrade_test.go b/cmd/thv/app/ai_plugin_upgrade_test.go index 84bff9a41f..f20f5e1cc8 100644 --- a/cmd/thv/app/ai_plugin_upgrade_test.go +++ b/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) + }) + } +} diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 5236f7e63c..52c0a061b1 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/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"+ + " ` (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) } diff --git a/pkg/plugins/pluginsvc/sync_verify_test.go b/pkg/plugins/pluginsvc/sync_verify_test.go index 6ee4c45947..3dde436a73 100644 --- a/pkg/plugins/pluginsvc/sync_verify_test.go +++ b/pkg/plugins/pluginsvc/sync_verify_test.go @@ -4,6 +4,7 @@ package pluginsvc import ( + "net/http" "strings" "testing" @@ -11,6 +12,7 @@ import ( "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") +} diff --git a/pkg/plugins/pluginsvc/upgrade.go b/pkg/plugins/pluginsvc/upgrade.go index 8ef530ae19..e8822afaeb 100644 --- a/pkg/plugins/pluginsvc/upgrade.go +++ b/pkg/plugins/pluginsvc/upgrade.go @@ -133,6 +133,12 @@ type upgradePlan struct { pinnedRef string // set only when the upgrade needs installing resolvedRef string // the resolved reference to record as ResolvedReference layerData []byte // set when the new content was resolved from the local OCI store + // allowSignerChange is opts.AllowSignerChange narrowed to this entry. The + // flag is project-wide, but honoring it per entry is not: forwarding it + // for a key-pinned entry whose candidate still verifies against the pin + // drops the key and sends a key-signed artifact through keyless + // verification, which refuses it. + allowSignerChange bool } // resolvedLatest is the current state of a lock entry's Source, before any @@ -180,11 +186,12 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, // local digest). outcome.Status = plugins.UpgradeStatusUpgraded return upgradePlan{ - entry: entry, - outcome: outcome, - pinnedRef: entry.Name, - resolvedRef: latest.ref, - layerData: latest.layerData, + entry: entry, + outcome: outcome, + pinnedRef: entry.Name, + resolvedRef: latest.ref, + layerData: latest.layerData, + allowSignerChange: opts.AllowSignerChange, } } @@ -196,21 +203,9 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, } } - // Signer-change guard: when the entry records a signer identity, the - // candidate must verify with the same one before the upgrade is even - // planned. Verification happens here (plan-only modes stay - // install-free) with a nil expected identity so a differing signer is - // reported as a change rather than a bare failure; blocked plans carry - // no pinnedRef, exactly like ref changes. - // - // Local-store candidates never reach this point — they returned above - // with layerData set — because they carry no signature to probe. - // verifyLocalInstall refuses them outright at install time when the - // entry is locked to a signer, which is the stronger check. - if entry.Provenance != nil && !opts.AllowSignerChange { - if blocked := s.guardSignerChange(ctx, entry, latest, &outcome); blocked { - return upgradePlan{entry: entry, outcome: outcome} - } + allowSignerChange, blocked := s.resolveSignerPolicy(ctx, opts, entry, latest, &outcome) + if blocked { + return upgradePlan{entry: entry, outcome: outcome} } pinnedRef, err := buildPinnedReference(lockfile.Entry{ResolvedReference: latest.ref, Digest: latest.digest}) @@ -222,7 +217,59 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, } outcome.Status = plugins.UpgradeStatusUpgraded - return upgradePlan{entry: entry, outcome: outcome, pinnedRef: pinnedRef, resolvedRef: latest.ref} + return upgradePlan{ + entry: entry, + outcome: outcome, + pinnedRef: pinnedRef, + resolvedRef: latest.ref, + allowSignerChange: allowSignerChange, + } +} + +// resolveSignerPolicy applies the signer-change guard to one entry, and +// reports both whether the plan stops here and whether --allow-signer-change +// applies to THIS entry. +// +// The guard runs at plan time so plan-only modes stay install-free, probing +// with a nil expected identity so a differing signer is reported as a change +// rather than a bare failure; blocked plans carry no pinnedRef, exactly like +// ref changes. +// +// The returned flag is not simply opts.AllowSignerChange. The override is +// project-wide but authorizes dropping an anchor the artifact has genuinely +// moved off — not ignoring one it still satisfies. Narrowing it per entry is +// what stops a plugin that needs the override from unpinning every +// key-pinned plugin beside it. +// +// Local-store candidates never reach this point — they returned earlier with +// layerData set — because they carry no signature to probe. +// verifyLocalInstall refuses them outright at install time when the entry is +// locked to a signer, which is the stronger check. +func (s *service) resolveSignerPolicy( + ctx context.Context, + opts plugins.UpgradeOptions, + entry lockfile.Entry, + latest resolvedLatest, + outcome *plugins.UpgradeOutcome, +) (allowSignerChange, blocked bool) { + if entry.Provenance == nil { + return opts.AllowSignerChange, false + } + if entry.Provenance.PublicKey != "" { + // A keyed entry is measured against its pin whether or not the + // override was passed; only what a genuine move authorizes differs. + // Measuring once keeps the two modes from disagreeing about what the + // candidate is. + verdict := s.judgeKeyedCandidate(ctx, entry, latest) + if recordKeyedVerdict(verdict, opts.AllowSignerChange, outcome) { + return false, true + } + return opts.AllowSignerChange && verdict.kind == keyedMovedToKeyless, false + } + if !opts.AllowSignerChange && s.guardSignerChange(ctx, entry, latest, outcome) { + return false, true + } + return opts.AllowSignerChange, false } // guardSignerChange probes the candidate artifact's signer identity and @@ -275,6 +322,210 @@ func (s *service) guardSignerChange( return false } +// recordKeyedVerdict turns a keyed verdict into a plan outcome and reports +// whether the upgrade stops here. +// +// A genuine key-to-keyless move is the one case the two modes treat +// differently: without the override it is blocked so the caller can decide, +// and with the override it proceeds so the pin can be dropped. Everything +// else is identical in both modes — in particular an undecided candidate +// stops the upgrade either way, so the override can never convert a failed +// measurement into permission to re-anchor. +// +// The identity on a blocked move is load-bearing rather than cosmetic: the +// CLI renders a blocked outcome carrying no NewSignerIdentity as "unsigned", +// so leaving it empty would report a keyless candidate as the one thing the +// measurement just established it is not. +func recordKeyedVerdict( + verdict keyedVerdict, allowSignerChange bool, outcome *plugins.UpgradeOutcome, +) bool { + switch verdict.kind { + case keyedPinHolds: + return false + case keyedMovedToKeyless: + if allowSignerChange { + return false + } + outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + outcome.NewSignerIdentity = verdict.identity + return true + case keyedUndecided: + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = verdict.reason + outcome.Error = verdict.err + return true + } + return false +} + +// keyedVerdictKind is what a candidate turned out to be when measured +// against the cosign public key its lock entry pins. +type keyedVerdictKind int + +const ( + // keyedPinHolds: the candidate verifies against the pinned key, so the + // signer demonstrably has not changed. + keyedPinHolds keyedVerdictKind = iota + // keyedMovedToKeyless: the pinned key conclusively does not apply AND a + // keyless signature verifies. This is the only genuine key-to-keyless + // move, and the only state in which dropping the pin is justified. + keyedMovedToKeyless + // keyedUndecided: no conclusion could be reached — the candidate carries + // nothing, carries something that verifies neither way, or verification + // could not be completed at all. + keyedUndecided +) + +// keyedVerdict is the measurement of a candidate against a pinned key, +// together with the outcome to record when it is not usable. +// +// One measurement serves both callers because they ask the same question and +// differ only in what a genuine move authorizes: the guard blocks it, and +// --allow-signer-change drops the pin for it. Sharing the verdict is what +// keeps those two from disagreeing about what the candidate is. +type keyedVerdict struct { + kind keyedVerdictKind + identity string // observed keyless identity, keyedMovedToKeyless only + reason plugins.FailureReason // keyedUndecided only + err string // keyedUndecided only +} + +// judgeKeyedCandidate measures a candidate against the public key its entry +// pins, verifying it keylessly when the key does not apply. +// +// The keyless probe is not only for the all-keyless case. VerifyOCIWithKey +// reports ErrKeylessSigned only when EVERY attached bundle carries a +// certificate, so an artifact mid-migration — a valid keyless bundle beside +// a stale key-pair one — comes back as ErrSignatureInvalid instead. Deciding +// on the keyed error alone would call that a damaged signature, when it is +// the supported transition. Only the probe tells the two apart. +// +// SECURITY: an operational failure is never a conclusion. A registry, +// transport, or context error says nothing about which key signed the +// artifact, so it yields keyedUndecided and the upgrade fails. Treating it +// as "the pin no longer applies" would let a transient network fault stand +// in as evidence that a plugin moved off its key — and under a project-wide +// --allow-signer-change that is enough to drop the pin and re-anchor an +// artifact that still carries a perfectly valid signature by the pinned key. +// An anchor may only be dropped on a conclusive mismatch plus a keyless +// signature that actually verifies. +func (s *service) judgeKeyedCandidate( + ctx context.Context, entry lockfile.Entry, latest resolvedLatest, +) keyedVerdict { + pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey) + if err != nil { + return keyedVerdict{ + kind: keyedUndecided, + reason: plugins.FailureReasonUnknown, + err: fmt.Errorf("lock entry's pinned %w", err).Error(), + } + } + + _, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, latest.ref, latest.digest, pubKeyPEM) + if verifyErr == nil { + return keyedVerdict{kind: keyedPinHolds} + } + if errors.Is(verifyErr, verifier.ErrUnsigned) { + // Nothing is attached at all, so there is no keyless bundle to find + // and no signer change to authorize. + return keyedVerdict{ + kind: keyedUndecided, + reason: plugins.FailureReasonUnsignedRejected, + err: fmt.Errorf("candidate is unsigned, and this entry is pinned to a cosign public"+ + " key: %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", + verifyErr, projectReinstallCommand(entry, "--allow-unsigned")).Error(), + } + } + + if !conclusiveKeyedMismatch(verifyErr) { + // The pin could not be evaluated, so nothing about the candidate has + // been established — least of all that it moved off the key. Stop + // before the keyless probe: a candidate carrying BOTH a still-valid + // pinned-key signature and a valid keyless one would otherwise be + // re-anchored to keyless on the strength of a transient fault. + return keyedVerdict{ + kind: keyedUndecided, + reason: plugins.FailureReasonUnknown, + err: fmt.Errorf("verifying candidate against the pinned cosign public key: %w", + verifyErr).Error(), + } + } + + probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) + if probeErr == nil { + return keyedVerdict{kind: keyedMovedToKeyless, identity: probe.SignerIdentity} + } + return keyedVerdict{ + kind: keyedUndecided, + reason: keyedFailureReason(verifyErr, probeErr), + err: keyedFailureMessage(entry, verifyErr, probeErr), + } +} + +// conclusiveKeyedMismatch reports whether a failed keyed verification +// actually settled that the pinned key does not apply to the candidate. +// +// Only the verifier's own signature verdicts do. ErrKeylessSigned means +// every attached bundle is keyless; ErrSignatureInvalid means a bundle was +// checked against the key and did not pass. A registry, transport, or +// context error means the question was never answered, and must not be read +// as an answer. +func conclusiveKeyedMismatch(verifyErr error) bool { + return errors.Is(verifyErr, verifier.ErrKeylessSigned) || + errors.Is(verifyErr, verifier.ErrSignatureInvalid) +} + +// keyedFailureReason classifies a candidate that verifies neither against the +// pinned key nor keylessly. Only conclusive mismatches reach here — an +// operational failure was already reported as undecided, because calling one +// signature-invalid would attach a destructive remedy to a network blip. +func keyedFailureReason(verifyErr, probeErr error) plugins.FailureReason { + if errors.Is(verifyErr, verifier.ErrKeylessSigned) { + // Every bundle is keyless, so the pinned key never had one to check + // and the keyless verdict is the whole diagnosis. + if reason := classifySignatureError(probeErr); reason != "" { + return reason + } + return plugins.FailureReasonUnknown + } + return plugins.FailureReasonSignatureInvalid +} + +// keyedFailureMessage explains a candidate that verifies neither way, naming +// a remedy only where one exists. +func keyedFailureMessage(entry lockfile.Entry, verifyErr, probeErr error) string { + switch { + case errors.Is(verifyErr, verifier.ErrKeylessSigned): + return fmt.Errorf("candidate dropped key-pair signing for keyless, but its keyless"+ + " signature does not verify: %w", probeErr).Error() + default: + return fmt.Errorf("candidate does not verify against the cosign public key this entry is"+ + " pinned to — either it was signed with a different key or the signature is damaged:"+ + " %w (re-anchoring to a new key is not supported in place; reinstall it: %s)", + verifyErr, projectReinstallCommand(entry, "--public-key ")).Error() + } +} + +// projectReinstallCommand renders a reinstall the caller can actually run. +// +// Naming the flag alone is not enough to act on. `thv ai-plugin install` +// requires the plugin argument, and it defaults to --scope user, where +// validateInstallPublicKey rejects --public-key outright and --allow-unsigned +// records nothing — a lock entry's trust anchor only exists project-scoped. +// So the bare flag would be refused before it verified anything. +func projectReinstallCommand(entry lockfile.Entry, flag string) string { + source := entry.Source + if source == "" { + source = entry.Name + } + return fmt.Sprintf("`thv ai-plugin uninstall %s --scope project` then"+ + " `thv ai-plugin install %s --scope project %s`"+ + " (add --project-root if you are not in the project directory)", + entry.Name, source, flag) +} + // runnerEnvironmentChanged reports whether the candidate's runner class // differs from the one recorded. An entry that recorded none is // unconstrained — lock entries written before the field existed have it @@ -349,7 +600,7 @@ func (s *service) applyUpgrade(ctx context.Context, opts plugins.UpgradeOptions, Clients: clients, LockSource: plan.entry.Source, LockResolvedReference: lockResolved, - AllowSignerChange: opts.AllowSignerChange, + AllowSignerChange: plan.allowSignerChange, ExpectedCanonicalName: plan.entry.Name, }); err != nil { outcome := plan.outcome diff --git a/pkg/plugins/pluginsvc/upgrade_verify_test.go b/pkg/plugins/pluginsvc/upgrade_verify_test.go index fdc774a943..a04aa3fe1d 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -4,14 +4,20 @@ package pluginsvc import ( + "context" + "fmt" "net/http" + "strings" "testing" + godigest "github.com/opencontainers/go-digest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "github.com/stacklok/toolhive-core/httperr" + ociplugins "github.com/stacklok/toolhive-core/oci/plugins" + ocimocks "github.com/stacklok/toolhive-core/oci/plugins/mocks" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills/lockfile" "github.com/stacklok/toolhive/pkg/skills/verifier" @@ -544,3 +550,432 @@ func TestUpgrade_OversizedCommitMaterialFailsRatherThanBlocks(t *testing.T) { assert.Equal(t, plugins.FailureReasonUnknown, outcome.Reason) assert.Contains(t, outcome.Error, "over the") } + +// TestJudgeKeyedCandidate measures a candidate against a pinned key and then +// runs the verdict through both modes. One measurement serves the guard and +// the --allow-signer-change override, so the table asserts what each mode +// does with it: only a genuine key-to-keyless move differs. +func TestJudgeKeyedCandidate(t *testing.T) { + t.Parallel() + + keyPEM, err := verifier.DecodePublicKey(testPublicKeyB64) + require.NoError(t, err) + latest := resolvedLatest{ + ref: "ghcr.io/org/keyed-plugin:v2", + digest: "sha256:" + strings.Repeat("c", 64), + } + + tests := []struct { + name string + verifyErr error + probe *verifier.Result + probeErr error + noProbe bool + + wantKind keyedVerdictKind + wantIdentity string + wantReason plugins.FailureReason + wantErrText string + wantNoText string + + // what each mode does with the verdict + wantBlocked bool + wantBlockedOverride bool + wantStatus plugins.UpgradeStatus + }{ + { + name: "verifying against the pinned key is the evidence the signer is unchanged", + noProbe: true, + wantKind: keyedPinHolds, + wantBlocked: false, wantBlockedOverride: false, + }, + { + name: "a candidate that moved to keyless signing is a signer change", + verifyErr: verifier.ErrKeylessSigned, + probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, + wantKind: keyedMovedToKeyless, + wantIdentity: "ci@example.com", + // Blocked without the override, permitted with it: this is the + // one transition --allow-signer-change exists to authorize. + wantBlocked: true, wantBlockedOverride: false, + wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + }, + { + // VerifyOCIWithKey reports ErrKeylessSigned only when EVERY + // bundle is keyless, so an artifact mid-migration — valid + // keyless bundle beside a stale key-pair one — arrives as + // ErrSignatureInvalid. It is still the supported transition. + name: "a mixed keyless and stale-key artifact is the supported transition", + verifyErr: verifier.ErrSignatureInvalid, + probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, + wantKind: keyedMovedToKeyless, + wantIdentity: "ci@example.com", + wantBlocked: true, wantBlockedOverride: false, + wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + }, + { + // --allow-signer-change bypasses the guard but not verification, + // and upgrade has no --allow-unsigned to pair with it. + name: "a candidate that lost its signature is an unsigned rejection", + verifyErr: verifier.ErrUnsigned, + noProbe: true, + wantKind: keyedUndecided, + wantReason: plugins.FailureReasonUnsignedRejected, + wantErrText: "--scope project --allow-unsigned", + wantBlocked: true, wantBlockedOverride: true, + wantStatus: plugins.UpgradeStatusFailed, + }, + { + name: "a different key is a failure, since allow_signer_change cannot re-anchor", + verifyErr: verifier.ErrSignatureInvalid, + probeErr: verifier.ErrKeySigned, + wantKind: keyedUndecided, + wantReason: plugins.FailureReasonSignatureInvalid, + wantErrText: "--scope project --public-key", + wantBlocked: true, wantBlockedOverride: true, + wantStatus: plugins.UpgradeStatusFailed, + }, + { + name: "an all-keyless candidate whose signature is broken is a failure", + verifyErr: verifier.ErrKeylessSigned, + probeErr: verifier.ErrSignatureInvalid, + wantKind: keyedUndecided, + wantReason: plugins.FailureReasonSignatureInvalid, + wantErrText: "dropped key-pair signing for keyless", + wantBlocked: true, wantBlockedOverride: true, + wantStatus: plugins.UpgradeStatusFailed, + }, + { + // SECURITY: an operational failure is not evidence about which + // key signed the artifact, so it must not read as "the pin no + // longer applies" — under the override that would be enough to + // drop the pin. The probe is given a result that WOULD succeed + // and asserted never called: an artifact can carry a valid + // keyless signature beside a still-valid pinned-key one, so + // consulting it here would re-anchor on a transient fault. + name: "an operational verifier failure is not a signature verdict", + verifyErr: fmt.Errorf("fetching signatures: %w", context.DeadlineExceeded), + probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, + noProbe: true, + wantKind: keyedUndecided, + wantReason: plugins.FailureReasonUnknown, + wantErrText: "context deadline exceeded", + wantNoText: "uninstall", + wantBlocked: true, wantBlockedOverride: true, + wantStatus: plugins.UpgradeStatusFailed, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), latest.ref, latest.digest, keyPEM). + Return(nil, tc.verifyErr) + // A keyed success and a bare unsigned artifact decide without a + // probe; every other arm must establish what the candidate + // actually carries before naming a diagnosis. + probeCalls := 1 + if tc.noProbe { + probeCalls = 0 + } + mv.EXPECT().VerifyOCI(gomock.Any(), latest.ref, latest.digest, gomock.Nil()). + Times(probeCalls).Return(tc.probe, tc.probeErr) + + svc := &service{sigVerifier: mv} + verdict := svc.judgeKeyedCandidate(t.Context(), keyedLockEntry("keyed-plugin"), latest) + + assert.Equal(t, tc.wantKind, verdict.kind) + assert.Equal(t, tc.wantIdentity, verdict.identity) + assert.Equal(t, tc.wantReason, verdict.reason) + if tc.wantErrText != "" { + assert.Contains(t, verdict.err, tc.wantErrText) + } + if tc.wantNoText != "" { + assert.NotContains(t, verdict.err, tc.wantNoText, + "an operational failure must not carry a destructive remedy") + } + + for _, override := range []bool{false, true} { + outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} + blocked := recordKeyedVerdict(verdict, override, &outcome) + want := tc.wantBlocked + if override { + want = tc.wantBlockedOverride + } + assert.Equal(t, want, blocked, "allow_signer_change=%v", override) + if !blocked { + continue + } + assert.Equal(t, tc.wantStatus, outcome.Status) + // The CLI renders a blocked outcome carrying no identity as + // "unsigned", so a keyless candidate that arrives unnamed is + // reported as the one thing it demonstrably is not. + assert.Equal(t, tc.wantIdentity, outcome.NewSignerIdentity) + } + }) + } +} + +// TestJudgeKeyedCandidate_UndecodablePinnedKey fails the plan rather than +// treating a corrupt anchor as a signer change: the lock file is +// hand-editable and the entry cannot be evaluated at all, which is not the +// same claim. It must not become permission to drop the pin either. +func TestJudgeKeyedCandidate_UndecodablePinnedKey(t *testing.T) { + t.Parallel() + + entry := keyedLockEntry("keyed-plugin") + entry.Provenance = &lockfile.Provenance{PublicKey: "not-base64!!"} + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + svc := &service{sigVerifier: mv} + verdict := svc.judgeKeyedCandidate(t.Context(), entry, resolvedLatest{ + ref: "ghcr.io/org/keyed-plugin:v2", + digest: "sha256:" + strings.Repeat("c", 64), + }) + assert.Equal(t, keyedUndecided, verdict.kind) + assert.Equal(t, plugins.FailureReasonUnknown, verdict.reason) + + for _, override := range []bool{false, true} { + outcome := plugins.UpgradeOutcome{Name: entry.Name} + require.True(t, recordKeyedVerdict(verdict, override, &outcome), + "allow_signer_change must not rescue an anchor that cannot be read") + assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) + } +} + +// keyPinnedUpgradeFixture installs an OCI plugin against testPublicKeyB64 — +// so the lock entry is genuinely key-pinned by the install path rather than +// hand-written — and publishes a second version so an upgrade is planned. The +// returned function switches which digest the tag resolves to; callers move it +// before upgrading. +// +// The registry client is mocked but the OCI store is real: Pull only reports +// which digest a reference names, and the content is read back out of the +// store, so both the guard and the install exercise their real code paths. +func keyPinnedUpgradeFixture( + t *testing.T, mv verifier.Verifier, +) (*service, string, func()) { + t.Helper() + + svc, projectRoot := newLockTestService(t, WithVerifier(mv)) + ociStore, err := ociplugins.NewStore(tempDir(t)) + require.NoError(t, err) + + d1 := buildTestPlugin(t, ociStore, "my-plugin", "1.0.0") + d2 := buildTestPlugin(t, ociStore, "my-plugin", "2.0.0") + tagged := d1 + + reg := ocimocks.NewMockRegistryClient(gomock.NewController(t)) + reg.EXPECT().Pull(gomock.Any(), ociStore, gomock.Any()).AnyTimes(). + DoAndReturn(func(_ context.Context, _ *ociplugins.Store, ref string) (godigest.Digest, error) { + // A digest-pinned reference names its own artifact; the tag + // resolves to whatever is currently published under it. + if _, after, found := strings.Cut(ref, "@"); found { + return godigest.Digest(after), nil + } + return tagged, nil + }) + + inner := svc.(*service) //nolint:forcetypeassert + inner.ociStore = ociStore + inner.registry = reg + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "ghcr.io/org/my-plugin:v1", + PublicKey: testPublicKeyB64, + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + require.Equal(t, testPublicKeyB64, entry.Provenance.PublicKey, + "precondition: the install must have pinned the key") + + return inner, projectRoot, func() { tagged = d2 } +} + +// TestUpgrade_KeyPinnedEntryVerifiesAgainstPinnedKey proves the whole keyed +// upgrade, not just the guard: the candidate is checked against the pinned +// key, and the install applyUpgrade then performs is checked against it too. +// That install carries no key of its own — resolveKeyAnchor reads the anchor +// back out of the lock — so a regression that dropped the pin would surface +// here as a keyless verification call rather than as a wrong result. +// +//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test +func TestUpgrade_KeyPinnedEntryVerifiesAgainstPinnedKey(t *testing.T) { + keyPEM, err := verifier.DecodePublicKey(testPublicKeyB64) + require.NoError(t, err) + + keyCalls := 0 + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(keyPEM)). + AnyTimes(). + DoAndReturn(func(_ context.Context, _, _ string, _ []byte) (*verifier.Result, error) { + keyCalls++ + return &verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil + }) + mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + // Neither the guard nor the install may fall back to the keyless path. + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + inner, projectRoot, publishV2 := keyPinnedUpgradeFixture(t, mv) + installCalls := keyCalls + publishV2() + + outcome := upgradePlugins(t, inner, plugins.UpgradeOptions{ProjectRoot: projectRoot}) + assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, + "a candidate that verifies against the pinned key must not be blocked") + + assert.Equal(t, 2, keyCalls-installCalls, + "the signer guard and the install applyUpgrade performs must each verify against the key") + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Equal(t, testPublicKeyB64, entry.Provenance.PublicKey, + "the upgrade must leave the entry pinned to the same key") + assert.Empty(t, entry.Provenance.SignerIdentity, + "a key-pair bundle carries no certificate identity to record") + assert.Equal(t, outcome.NewDigest, entry.Digest) +} + +// TestUpgrade_AllowSignerChangeMovesKeyPinnedEntryToKeyless covers the one +// supported way off a key: --allow-signer-change drops the recorded key and +// re-verifies keylessly, so the entry ends up anchored to the identity that +// was actually observed. This is why a keyless candidate blocks as a signer +// change rather than failing — the remedy the CLI prints genuinely works. +// +//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test +func TestUpgrade_AllowSignerChangeMovesKeyPinnedEntryToKeyless(t *testing.T) { + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + // The install pins the key; the candidate has since moved to keyless + // signing, which is what makes the override applicable at all. + installed := false + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes(). + DoAndReturn(func(_ any, _, _ string, _ []byte) (*verifier.Result, error) { + if !installed { + installed = true + return &verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil + } + return nil, verifier.ErrKeylessSigned + }) + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + + inner, projectRoot, publishV2 := keyPinnedUpgradeFixture(t, mv) + publishV2() + + outcome := upgradePlugins(t, inner, plugins.UpgradeOptions{ + ProjectRoot: projectRoot, AllowSignerChange: true, + }) + assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, "error: %s", outcome.Error) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Empty(t, entry.Provenance.PublicKey, + "the override drops the recorded key rather than keeping a pin it did not enforce") + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity, + "the entry must be re-anchored to the identity actually observed") + assert.Equal(t, testCertIssuer, entry.Provenance.CertIssuer) +} + +// TestUpgrade_AllowSignerChangeKeepsSameKeyPin is the multi-plugin case: +// --allow-signer-change is a project-wide flag, so needing it for one plugin +// must not silently unpin another. The override authorizes dropping a +// recorded key only when the candidate actually moved off it; a candidate +// still signed by the pinned key keeps the pin, because dropping it sends a +// key-signed artifact through keyless verification, which refuses it. +// +//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test +func TestUpgrade_AllowSignerChangeKeepsSameKeyPin(t *testing.T) { + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(&verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil) + // What a key-signed artifact really answers when verified keylessly. + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil, verifier.ErrKeySigned) + mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + + inner, projectRoot, publishV2 := keyPinnedUpgradeFixture(t, mv) + publishV2() + + outcome := upgradePlugins(t, inner, plugins.UpgradeOptions{ + ProjectRoot: projectRoot, AllowSignerChange: true, + }) + assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, "error: %s", outcome.Error) + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Equal(t, testPublicKeyB64, entry.Provenance.PublicKey, + "a candidate that still verifies against the pinned key stays pinned to it") + assert.Empty(t, entry.Provenance.SignerIdentity) +} + +// TestUpgrade_OperationalKeyedErrorDoesNotDropPin is the fail-closed case for +// a project-wide --allow-signer-change. A registry or transport fault during +// planning says nothing about which key signed the candidate, so it must not +// stand in as evidence that the plugin moved off its pinned key. If it did, +// a transient fault would be enough to drop the pin and re-anchor an artifact +// that still carries a valid signature by that very key. +// +//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test +func TestUpgrade_OperationalKeyedErrorDoesNotDropPin(t *testing.T) { + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + installed := false + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes(). + DoAndReturn(func(_ any, _, _ string, _ []byte) (*verifier.Result, error) { + if !installed { + installed = true + return &verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil + } + return nil, fmt.Errorf("fetching signatures: %w", context.DeadlineExceeded) + }) + // The candidate also carries a perfectly good keyless signature, so a + // probe would succeed. That is the trap: the artifact still carries a + // valid pinned-key signature too, and only the transient keyed fault + // makes it look like it moved. The probe must never be reached. + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(signedResult(), nil) + mv.EXPECT().VerifyBundleOfflineWithKey(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(nil) + + inner, projectRoot, publishV2 := keyPinnedUpgradeFixture(t, mv) + before, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + publishV2() + + outcome := upgradePlugins(t, inner, plugins.UpgradeOptions{ + ProjectRoot: projectRoot, AllowSignerChange: true, + }) + assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) + assert.Equal(t, plugins.FailureReasonUnknown, outcome.Reason) + assert.NotContains(t, outcome.Error, "uninstall", + "a transport fault must not advise uninstalling the plugin") + + after, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, after.Provenance) + assert.Equal(t, testPublicKeyB64, after.Provenance.PublicKey, "the pin must survive a transport fault") + assert.Empty(t, after.Provenance.SignerIdentity, "nothing may be re-anchored to keyless") + assert.Equal(t, before.Digest, after.Digest, "a failed plan must not re-pin the entry") +}