From e415dec296906c506cd2acb05bbb1911e0c2487f Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 7 Sep 2026 16:29:19 +0200 Subject: [PATCH 1/4] Honour a pinned cosign key on plugin sync/upgrade Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/sync.go | 64 ++++- pkg/plugins/pluginsvc/sync_verify_test.go | 190 ++++++++++++++ pkg/plugins/pluginsvc/upgrade.go | 56 ++++ pkg/plugins/pluginsvc/upgrade_verify_test.go | 253 +++++++++++++++++++ 4 files changed, 554 insertions(+), 9 deletions(-) diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 5236f7e63c..9976cec93e 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,32 @@ 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 with `thv ai-plugin install --public-key` instead, which"+ + " verifies the signature and pins the key (--allow-unsigned is not a substitute:"+ + " the artifact is signed)", + err, 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..86ebd11658 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, "thv ai-plugin install --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..ec40dde51d 100644 --- a/pkg/plugins/pluginsvc/upgrade.go +++ b/pkg/plugins/pluginsvc/upgrade.go @@ -231,6 +231,9 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, // signature cannot be verified at all, or its certificate's repository ref // or runner class differs from what is recorded. Returns true when blocked. // +// A key-pinned entry has no identity to probe and is handed to +// guardKeyedSignerChange instead. +// // 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 // ref rotate to any other tag ref, reasoning that a release workflow signs @@ -251,6 +254,9 @@ func (s *service) guardSignerChange( latest resolvedLatest, outcome *plugins.UpgradeOutcome, ) bool { + if entry.Provenance.PublicKey != "" { + return s.guardKeyedSignerChange(ctx, entry, latest, outcome) + } probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) switch { case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned): @@ -275,6 +281,56 @@ func (s *service) guardSignerChange( return false } +// guardKeyedSignerChange is guardSignerChange for an entry pinned to a cosign +// public key. There is no identity to probe for and compare — a key-pair +// bundle carries no certificate — so the pinned key is applied to the +// candidate directly: verifying against it IS the evidence that the signer +// has not changed, and the upgrade then proceeds on the anchor the entry +// already records. No new key is accepted here; the lock supplies it. +// +// The candidate is verified over OCI unconditionally, with no git arm: the +// lock schema refuses publicKey on a git entry (a commit signature is checked +// against a certificate, never a key), so a key-pinned entry is always an OCI +// one and latest.commitPayload is always empty here. +// +// The blocked arms are split by whether --allow-signer-change would actually +// help, because the caller is told to use it. It does for a candidate that +// moved to keyless signing or dropped its signature: dropping the recorded +// key and re-verifying is exactly the key-to-keyless move resolveKeyAnchor +// supports. It does NOT for a candidate signed by a different key — that +// needs an in-place re-anchor, which v1 does not offer — so that arm reports +// a failure naming the route that works instead of a remedy that does not. +func (s *service) guardKeyedSignerChange( + ctx context.Context, + entry lockfile.Entry, + latest resolvedLatest, + outcome *plugins.UpgradeOutcome, +) bool { + pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey) + if err != nil { + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = plugins.FailureReasonUnknown + outcome.Error = fmt.Errorf("lock entry's pinned %w", err).Error() + return true + } + _, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, latest.ref, latest.digest, pubKeyPEM) + switch { + case verifyErr == nil: + return false + case errors.Is(verifyErr, verifier.ErrKeylessSigned), errors.Is(verifyErr, verifier.ErrUnsigned): + outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + return true + default: + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = plugins.FailureReasonSignatureInvalid + outcome.Error = 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; uninstall the plugin and"+ + " reinstall it with `thv ai-plugin install --public-key`)", verifyErr).Error() + return true + } +} + // 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 diff --git a/pkg/plugins/pluginsvc/upgrade_verify_test.go b/pkg/plugins/pluginsvc/upgrade_verify_test.go index fdc774a943..bd013dc843 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -4,14 +4,19 @@ package pluginsvc import ( + "context" "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 +549,251 @@ func TestUpgrade_OversizedCommitMaterialFailsRatherThanBlocks(t *testing.T) { assert.Equal(t, plugins.FailureReasonUnknown, outcome.Reason) assert.Contains(t, outcome.Error, "over the") } + +// TestGuardKeyedSignerChange covers the guard for a key-pinned entry. The +// keyless guard probes for a certificate identity to compare, which a +// key-pair bundle does not have — so before this branch a key-pinned entry +// could never be upgraded at all: probeCandidateSigner returned ErrKeySigned +// and the plan failed with a signature error, where a policy decision was +// intended. +// +// The blocked arms are split by whether the remedy the CLI prints actually +// works. "signer change blocked" tells the user to pass --allow-signer-change, +// which drops the recorded key and re-verifies keylessly — a real fix when the +// candidate moved to keyless signing or lost its signature, and a dead end +// when it is simply signed by a different key. +func TestGuardKeyedSignerChange(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 + wantBlocked bool + wantStatus plugins.UpgradeStatus + wantReason plugins.FailureReason + wantErrText string + }{ + { + name: "verifying against the pinned key is the evidence the signer is unchanged", + wantBlocked: false, + }, + { + name: "a candidate that moved to keyless signing is a signer change", + verifyErr: verifier.ErrKeylessSigned, + wantBlocked: true, + wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + }, + { + name: "a candidate that lost its signature is a signer change", + verifyErr: verifier.ErrUnsigned, + wantBlocked: true, + wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + }, + { + name: "a different key is a failure, since allow_signer_change cannot re-anchor", + verifyErr: verifier.ErrSignatureInvalid, + wantBlocked: true, + wantStatus: plugins.UpgradeStatusFailed, + wantReason: plugins.FailureReasonSignatureInvalid, + wantErrText: "reinstall it with `thv ai-plugin install --public-key`", + }, + } + 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) + // The keyless probe must not run: it is what produced the + // unhelpful diagnosis this branch exists to replace. + mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + svc := &service{sigVerifier: mv} + outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} + blocked := svc.guardSignerChange( + t.Context(), keyedLockEntry("keyed-plugin"), latest, &outcome) + + assert.Equal(t, tc.wantBlocked, blocked) + if !tc.wantBlocked { + return + } + assert.Equal(t, tc.wantStatus, outcome.Status) + assert.Equal(t, tc.wantReason, outcome.Reason) + if tc.wantErrText != "" { + assert.Contains(t, outcome.Error, tc.wantErrText) + } + if tc.wantStatus == plugins.UpgradeStatusSignerChangeBlocked { + assert.Empty(t, outcome.NewSignerIdentity, + "a key-signed candidate has no identity to report, and inventing one would"+ + " print a signer the artifact never claimed") + } + }) + } +} + +// TestGuardKeyedSignerChange_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. +func TestGuardKeyedSignerChange_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} + outcome := plugins.UpgradeOutcome{Name: entry.Name} + require.True(t, svc.guardSignerChange(t.Context(), entry, resolvedLatest{ + ref: "ghcr.io/org/keyed-plugin:v2", + digest: "sha256:" + strings.Repeat("c", 64), + }, &outcome)) + assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) + assert.Equal(t, plugins.FailureReasonUnknown, outcome.Reason) + assert.NotEqual(t, plugins.UpgradeStatusSignerChangeBlocked, 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)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes().Return(&verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil) + 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) + + 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) +} From 6f288f844161a134e4c7ed7e00f115d8e5325017 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 8 Sep 2026 14:15:30 +0200 Subject: [PATCH 2/4] Split unsigned from keyless in keyed upgrades A key-pinned entry whose candidate lost its signature was reported as a signer change, whose remedy is --allow-signer-change. That override bypasses the guard but not verification, and upgrade has no unsigned-consent flag to pair with it, so the install failed unsigned-rejected one step later. Report the rejection directly and name the route that works. A candidate that moved to keyless signing is still a signer change, but it now carries the identity it moved to: the CLI infers "unsigned" from an absent NewSignerIdentity, so an unnamed one was rendered as the one thing it is not. Signed-off-by: Samuele Verzi --- cmd/thv/app/ai_plugin_upgrade_test.go | 59 +++++++++++++++++ pkg/plugins/pluginsvc/upgrade.go | 61 +++++++++++++++--- pkg/plugins/pluginsvc/upgrade_verify_test.go | 66 +++++++++++++++++--- 3 files changed, 168 insertions(+), 18 deletions(-) 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/upgrade.go b/pkg/plugins/pluginsvc/upgrade.go index ec40dde51d..3d8ebff7b2 100644 --- a/pkg/plugins/pluginsvc/upgrade.go +++ b/pkg/plugins/pluginsvc/upgrade.go @@ -293,13 +293,17 @@ func (s *service) guardSignerChange( // against a certificate, never a key), so a key-pinned entry is always an OCI // one and latest.commitPayload is always empty here. // -// The blocked arms are split by whether --allow-signer-change would actually -// help, because the caller is told to use it. It does for a candidate that -// moved to keyless signing or dropped its signature: dropping the recorded +// The arms are split by whether --allow-signer-change would actually help, +// because a blocked outcome is what tells the caller to reach for it. It +// helps for a candidate that moved to keyless signing: dropping the recorded // key and re-verifying is exactly the key-to-keyless move resolveKeyAnchor -// supports. It does NOT for a candidate signed by a different key — that -// needs an in-place re-anchor, which v1 does not offer — so that arm reports -// a failure naming the route that works instead of a remedy that does not. +// supports. It does NOT help for a candidate that dropped its signature — +// the override re-verifies from scratch, and upgrade carries no +// unsigned-consent flag to pair with it, so the install it waves through +// fails unsigned-rejected anyway — nor for a candidate signed by a different +// key, which needs an in-place re-anchor v1 does not offer. Both of those +// report a failure naming the route that works instead of a remedy that +// leads nowhere. func (s *service) guardKeyedSignerChange( ctx context.Context, entry lockfile.Entry, @@ -317,8 +321,16 @@ func (s *service) guardKeyedSignerChange( switch { case verifyErr == nil: return false - case errors.Is(verifyErr, verifier.ErrKeylessSigned), errors.Is(verifyErr, verifier.ErrUnsigned): - outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + case errors.Is(verifyErr, verifier.ErrKeylessSigned): + return s.blockKeyToKeylessChange(ctx, entry, latest, outcome) + case errors.Is(verifyErr, verifier.ErrUnsigned): + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = plugins.FailureReasonUnsignedRejected + outcome.Error = 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, uninstall it and reinstall it with"+ + " `thv ai-plugin install --allow-unsigned`", verifyErr).Error() return true default: outcome.Status = plugins.UpgradeStatusFailed @@ -331,6 +343,39 @@ func (s *service) guardKeyedSignerChange( } } +// blockKeyToKeylessChange reports a candidate that moved from key-pair to +// keyless signing, naming the identity it moved to. The identity costs a +// second verification because VerifyOCIWithKey cannot report one — it was +// asked about a key — and an unnamed signer change is worse than it looks: +// the CLI renders a blocked outcome with no identity as "unsigned", so +// leaving the field empty here would describe a signed candidate as +// unsigned, which is the opposite of what the guard just established. +// +// A probe that fails is a failure rather than a signer change. The candidate +// carries a keyless signature that does not verify, and --allow-signer-change +// re-verifies rather than skipping verification, so calling this blocked +// would advise a flag that cannot get past it either. +func (s *service) blockKeyToKeylessChange( + ctx context.Context, + entry lockfile.Entry, + latest resolvedLatest, + outcome *plugins.UpgradeOutcome, +) bool { + probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) + if probeErr != nil { + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = classifySignatureError(probeErr) + if outcome.Reason == "" { + outcome.Reason = plugins.FailureReasonUnknown + } + outcome.Error = probeErr.Error() + return true + } + outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + outcome.NewSignerIdentity = probe.SignerIdentity + return true +} + // 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 diff --git a/pkg/plugins/pluginsvc/upgrade_verify_test.go b/pkg/plugins/pluginsvc/upgrade_verify_test.go index bd013dc843..e4d3b8f96a 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -5,6 +5,7 @@ package pluginsvc import ( "context" + "fmt" "net/http" "strings" "testing" @@ -575,9 +576,11 @@ func TestGuardKeyedSignerChange(t *testing.T) { tests := []struct { name string verifyErr error + probe *verifier.Result wantBlocked bool wantStatus plugins.UpgradeStatus wantReason plugins.FailureReason + wantSigner string wantErrText string }{ { @@ -587,14 +590,22 @@ func TestGuardKeyedSignerChange(t *testing.T) { { name: "a candidate that moved to keyless signing is a signer change", verifyErr: verifier.ErrKeylessSigned, + probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, wantBlocked: true, wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + wantSigner: "ci@example.com", }, { - name: "a candidate that lost its signature is a signer change", + // --allow-signer-change bypasses this guard but not verification, + // and upgrade has no --allow-unsigned to pair with it, so calling + // this a signer change would advertise a route that dead-ends in + // unsigned-rejected one step later. + name: "a candidate that lost its signature is an unsigned rejection, not a signer change", verifyErr: verifier.ErrUnsigned, wantBlocked: true, - wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + wantStatus: plugins.UpgradeStatusFailed, + wantReason: plugins.FailureReasonUnsignedRejected, + wantErrText: "reinstall it with `thv ai-plugin install --allow-unsigned`", }, { name: "a different key is a failure, since allow_signer_change cannot re-anchor", @@ -611,9 +622,15 @@ func TestGuardKeyedSignerChange(t *testing.T) { mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) mv.EXPECT().VerifyOCIWithKey(gomock.Any(), latest.ref, latest.digest, keyPEM). Return(nil, tc.verifyErr) - // The keyless probe must not run: it is what produced the - // unhelpful diagnosis this branch exists to replace. - mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + // The keyless probe runs only to name the identity a + // key-to-keyless move landed on. Every other arm decides without + // it, and must not pay for a second verification. + probeCalls := 0 + if tc.probe != nil { + probeCalls = 1 + } + mv.EXPECT().VerifyOCI(gomock.Any(), latest.ref, latest.digest, gomock.Nil()). + Times(probeCalls).Return(tc.probe, nil) svc := &service{sigVerifier: mv} outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} @@ -629,15 +646,44 @@ func TestGuardKeyedSignerChange(t *testing.T) { if tc.wantErrText != "" { assert.Contains(t, outcome.Error, tc.wantErrText) } - if tc.wantStatus == plugins.UpgradeStatusSignerChangeBlocked { - assert.Empty(t, outcome.NewSignerIdentity, - "a key-signed candidate has no identity to report, and inventing one would"+ - " print a signer the artifact never claimed") - } + // 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.wantSigner, outcome.NewSignerIdentity) }) } } +// TestGuardKeyedSignerChange_KeylessProbeFailure keeps a candidate whose +// keyless signature does not verify out of the blocked bucket. Blocked tells +// the caller to re-run with --allow-signer-change, which skips this guard but +// still verifies, so the flag cannot get such a candidate installed either. +func TestGuardKeyedSignerChange_KeylessProbeFailure(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), + } + + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey(gomock.Any(), latest.ref, latest.digest, keyPEM). + Return(nil, verifier.ErrKeylessSigned) + mv.EXPECT().VerifyOCI(gomock.Any(), latest.ref, latest.digest, gomock.Nil()). + Return(nil, fmt.Errorf("probing: %w", verifier.ErrSignatureInvalid)) + + svc := &service{sigVerifier: mv} + outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} + require.True(t, svc.guardSignerChange( + t.Context(), keyedLockEntry("keyed-plugin"), latest, &outcome)) + + assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) + assert.Equal(t, plugins.FailureReasonSignatureInvalid, outcome.Reason) + assert.Empty(t, outcome.NewSignerIdentity) +} + // TestGuardKeyedSignerChange_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. From aed8c3a1abcf09b81a2a4a1b13955bff86e4a305 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 8 Sep 2026 15:18:41 +0200 Subject: [PATCH 3/4] Probe keyless before failing a keyed candidate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerifyOCIWithKey reports ErrKeylessSigned only when every attached bundle is keyless, so an artifact mid-migration — a valid keyless bundle beside a stale key-pair one — arrived as ErrSignatureInvalid and was sent to uninstall-and-reinstall. It is the supported key-to-keyless transition, so verify the candidate keylessly before naming a diagnosis. Restrict the signature-invalid verdict to ErrSignatureInvalid. Registry, transport, and context failures say nothing about a signature, and advising an uninstall over a network blip is destructive. Narrow --allow-signer-change per entry. It is project-wide, so needing it for one plugin dropped the pin on every key-pinned plugin beside it, and resolveKeyAnchor then sent a still-key-signed candidate through keyless verification, which fails ErrKeySigned. An entry whose candidate still verifies against its pin keeps it. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/upgrade.go | 146 ++++++++++++++----- pkg/plugins/pluginsvc/upgrade_verify_test.go | 99 +++++++++++-- 2 files changed, 196 insertions(+), 49 deletions(-) diff --git a/pkg/plugins/pluginsvc/upgrade.go b/pkg/plugins/pluginsvc/upgrade.go index 3d8ebff7b2..f2dad792d5 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, } } @@ -207,9 +214,14 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, // 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 := opts.AllowSignerChange + if entry.Provenance != nil { + if !opts.AllowSignerChange { + if blocked := s.guardSignerChange(ctx, entry, latest, &outcome); blocked { + return upgradePlan{entry: entry, outcome: outcome} + } + } else if entry.Provenance.PublicKey != "" { + allowSignerChange = !s.candidateMatchesPinnedKey(ctx, entry, latest) } } @@ -222,7 +234,13 @@ 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, + } } // guardSignerChange probes the candidate artifact's signer identity and @@ -318,12 +336,12 @@ func (s *service) guardKeyedSignerChange( return true } _, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, latest.ref, latest.digest, pubKeyPEM) - switch { - case verifyErr == nil: + if verifyErr == nil { return false - case errors.Is(verifyErr, verifier.ErrKeylessSigned): - return s.blockKeyToKeylessChange(ctx, entry, latest, outcome) - case errors.Is(verifyErr, verifier.ErrUnsigned): + } + 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. outcome.Status = plugins.UpgradeStatusFailed outcome.Reason = plugins.FailureReasonUnsignedRejected outcome.Error = fmt.Errorf("candidate is unsigned, and this entry is pinned to a cosign"+ @@ -332,47 +350,95 @@ func (s *service) guardKeyedSignerChange( " move this plugin to an unsigned artifact, uninstall it and reinstall it with"+ " `thv ai-plugin install --allow-unsigned`", verifyErr).Error() return true - default: - outcome.Status = plugins.UpgradeStatusFailed - outcome.Reason = plugins.FailureReasonSignatureInvalid - outcome.Error = 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; uninstall the plugin and"+ - " reinstall it with `thv ai-plugin install --public-key`)", verifyErr).Error() - return true } + return s.classifyKeyedCandidate(ctx, entry, latest, verifyErr, outcome) } -// blockKeyToKeylessChange reports a candidate that moved from key-pair to -// keyless signing, naming the identity it moved to. The identity costs a -// second verification because VerifyOCIWithKey cannot report one — it was -// asked about a key — and an unnamed signer change is worse than it looks: -// the CLI renders a blocked outcome with no identity as "unsigned", so -// leaving the field empty here would describe a signed candidate as -// unsigned, which is the opposite of what the guard just established. +// candidateMatchesPinnedKey reports whether the candidate still verifies +// against the public key the entry pins. +// +// It exists because --allow-signer-change is a project-wide flag applied per +// entry. The override authorizes dropping a recorded anchor when the +// artifact has genuinely moved off it — not ignoring an anchor the artifact +// still satisfies. resolveKeyAnchor drops the recorded key whenever the flag +// is set, so forwarding it here would push a still-key-signed candidate +// through keyless verification and fail it with ErrKeySigned. That is +// reachable without asking for it: needing the override for one plugin in a +// multi-plugin project would otherwise break every key-pinned plugin +// alongside it. // -// A probe that fails is a failure rather than a signer change. The candidate -// carries a keyless signature that does not verify, and --allow-signer-change -// re-verifies rather than skipping verification, so calling this blocked -// would advise a flag that cannot get past it either. -func (s *service) blockKeyToKeylessChange( +// An undecodable pin reports false. There is nothing to preserve, and the +// override is exactly the escape hatch for an entry whose anchor can no +// longer be applied. +func (s *service) candidateMatchesPinnedKey( + ctx context.Context, entry lockfile.Entry, latest resolvedLatest, +) bool { + pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey) + if err != nil { + return false + } + _, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, latest.ref, latest.digest, pubKeyPEM) + return verifyErr == nil +} + +// classifyKeyedCandidate decides what a candidate that failed verification +// against the pinned key actually is, by verifying it keylessly before +// committing to a diagnosis. A keyless bundle that verifies means the +// artifact moved to keyless signing: a signer change --allow-signer-change +// authorizes, reported with the identity it moved to. +// +// The 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 alongside a stale +// key-pair one — comes back as ErrSignatureInvalid instead. Deciding on the +// keyed error alone would call that a damaged signature and send the caller +// to uninstall-and-reinstall, when the override resolves it: dropping the +// pin lets ordinary keyless verification accept the valid bundle. Only the +// probe tells the two apart. +// +// Naming the identity is load-bearing rather than cosmetic. The CLI renders +// a blocked outcome carrying no NewSignerIdentity as "unsigned", so a +// keyless candidate left unnamed is reported as the one thing the guard has +// just established it is not. +func (s *service) classifyKeyedCandidate( ctx context.Context, entry lockfile.Entry, latest resolvedLatest, + verifyErr error, outcome *plugins.UpgradeOutcome, ) bool { - probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) - if probeErr != nil { + if probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest); probeErr == nil { + outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + outcome.NewSignerIdentity = probe.SignerIdentity + return true + } else 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. outcome.Status = plugins.UpgradeStatusFailed outcome.Reason = classifySignatureError(probeErr) if outcome.Reason == "" { outcome.Reason = plugins.FailureReasonUnknown } - outcome.Error = probeErr.Error() + outcome.Error = fmt.Errorf("candidate dropped key-pair signing for keyless, but its keyless"+ + " signature does not verify: %w", probeErr).Error() + return true + } + + outcome.Status = plugins.UpgradeStatusFailed + if errors.Is(verifyErr, verifier.ErrSignatureInvalid) { + outcome.Reason = plugins.FailureReasonSignatureInvalid + outcome.Error = 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; uninstall the plugin and"+ + " reinstall it with `thv ai-plugin install --public-key`)", verifyErr).Error() return true } - outcome.Status = plugins.UpgradeStatusSignerChangeBlocked - outcome.NewSignerIdentity = probe.SignerIdentity + // A registry, transport, or context failure says nothing about the + // signature. Calling it signature-invalid would advise uninstalling a + // working plugin over a network blip. + outcome.Reason = plugins.FailureReasonUnknown + outcome.Error = fmt.Errorf("verifying candidate against the pinned cosign public key: %w", + verifyErr).Error() return true } @@ -450,7 +516,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 e4d3b8f96a..cfd9fab75e 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -577,14 +577,18 @@ func TestGuardKeyedSignerChange(t *testing.T) { name string verifyErr error probe *verifier.Result + probeErr error + noProbe bool wantBlocked bool wantStatus plugins.UpgradeStatus wantReason plugins.FailureReason wantSigner string wantErrText string + wantNoText string }{ { name: "verifying against the pinned key is the evidence the signer is unchanged", + noProbe: true, wantBlocked: false, }, { @@ -602,6 +606,7 @@ func TestGuardKeyedSignerChange(t *testing.T) { // unsigned-rejected one step later. name: "a candidate that lost its signature is an unsigned rejection, not a signer change", verifyErr: verifier.ErrUnsigned, + noProbe: true, wantBlocked: true, wantStatus: plugins.UpgradeStatusFailed, wantReason: plugins.FailureReasonUnsignedRejected, @@ -610,11 +615,37 @@ func TestGuardKeyedSignerChange(t *testing.T) { { name: "a different key is a failure, since allow_signer_change cannot re-anchor", verifyErr: verifier.ErrSignatureInvalid, + probeErr: verifier.ErrKeySigned, wantBlocked: true, wantStatus: plugins.UpgradeStatusFailed, wantReason: plugins.FailureReasonSignatureInvalid, wantErrText: "reinstall it with `thv ai-plugin install --public-key`", }, + { + // 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 key-to-keyless + // transition, and must not be sent to uninstall-and-reinstall. + name: "a mixed keyless and stale-key artifact is the supported transition", + verifyErr: verifier.ErrSignatureInvalid, + probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, + wantBlocked: true, + wantStatus: plugins.UpgradeStatusSignerChangeBlocked, + wantSigner: "ci@example.com", + }, + { + // A registry or transport failure says nothing about the + // signature, so it must not advise uninstalling the plugin. + name: "an operational verifier failure is not a signature verdict", + verifyErr: fmt.Errorf("fetching signatures: %w", context.DeadlineExceeded), + probeErr: context.DeadlineExceeded, + wantBlocked: true, + wantStatus: plugins.UpgradeStatusFailed, + wantReason: plugins.FailureReasonUnknown, + wantErrText: "context deadline exceeded", + wantNoText: "uninstall", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -622,15 +653,15 @@ func TestGuardKeyedSignerChange(t *testing.T) { mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) mv.EXPECT().VerifyOCIWithKey(gomock.Any(), latest.ref, latest.digest, keyPEM). Return(nil, tc.verifyErr) - // The keyless probe runs only to name the identity a - // key-to-keyless move landed on. Every other arm decides without - // it, and must not pay for a second verification. - probeCalls := 0 - if tc.probe != nil { - probeCalls = 1 + // 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, nil) + Times(probeCalls).Return(tc.probe, tc.probeErr) svc := &service{sigVerifier: mv} outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} @@ -646,6 +677,10 @@ func TestGuardKeyedSignerChange(t *testing.T) { if tc.wantErrText != "" { assert.Contains(t, outcome.Error, tc.wantErrText) } + if tc.wantNoText != "" { + assert.NotContains(t, outcome.Error, tc.wantNoText, + "an operational failure must not carry a destructive remedy") + } // 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. @@ -817,8 +852,18 @@ func TestUpgrade_KeyPinnedEntryVerifiesAgainstPinnedKey(t *testing.T) { //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().Return(&verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil) + 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()). @@ -832,7 +877,7 @@ func TestUpgrade_AllowSignerChangeMovesKeyPinnedEntryToKeyless(t *testing.T) { outcome := upgradePlugins(t, inner, plugins.UpgradeOptions{ ProjectRoot: projectRoot, AllowSignerChange: true, }) - assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status) + assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, "error: %s", outcome.Error) entry, ok := loadPluginLockEntry(t, projectRoot) require.True(t, ok) @@ -843,3 +888,39 @@ func TestUpgrade_AllowSignerChangeMovesKeyPinnedEntryToKeyless(t *testing.T) { "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) +} From 5b1fc32cea16555f719a02622b5910f15101cbdb Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 9 Sep 2026 09:38:52 +0200 Subject: [PATCH 4/4] Refuse to drop a key pin on an unproven mismatch A failed keyed verification was treated as evidence the candidate had moved off its pinned key. Under a project-wide --allow-signer-change that is enough to drop the pin, so a registry, transport, or context fault let an artifact still carrying a valid signature by the pinned key be re-anchored to keyless. Only the verifier's own signature verdicts settle the question, so an operational failure now stops the upgrade before the keyless probe runs, in both modes. Measure the candidate once and share the verdict between the guard and the override, so the two cannot disagree about what it is. Name a reinstall a caller can run. install requires the plugin argument and defaults to user scope, where --public-key is rejected outright and --allow-unsigned records nothing, so the bare flag was refused before it verified anything. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/sync.go | 10 +- pkg/plugins/pluginsvc/sync_verify_test.go | 2 +- pkg/plugins/pluginsvc/upgrade.go | 368 ++++++++++++------- pkg/plugins/pluginsvc/upgrade_verify_test.go | 281 ++++++++------ 4 files changed, 401 insertions(+), 260 deletions(-) diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 9976cec93e..52c0a061b1 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -577,10 +577,12 @@ func (s *service) adoptionTrust( 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 with `thv ai-plugin install --public-key` instead, which"+ - " verifies the signature and pins the key (--allow-unsigned is not a substitute:"+ - " the artifact is signed)", - err, pl.Metadata.Name), + " 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, ) } diff --git a/pkg/plugins/pluginsvc/sync_verify_test.go b/pkg/plugins/pluginsvc/sync_verify_test.go index 86ebd11658..3dde436a73 100644 --- a/pkg/plugins/pluginsvc/sync_verify_test.go +++ b/pkg/plugins/pluginsvc/sync_verify_test.go @@ -472,7 +472,7 @@ func TestSync_AdoptRefusesKeySignedInstall(t *testing.T) { 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, "thv ai-plugin install --public-key", + 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") diff --git a/pkg/plugins/pluginsvc/upgrade.go b/pkg/plugins/pluginsvc/upgrade.go index f2dad792d5..e8822afaeb 100644 --- a/pkg/plugins/pluginsvc/upgrade.go +++ b/pkg/plugins/pluginsvc/upgrade.go @@ -203,26 +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. - allowSignerChange := opts.AllowSignerChange - if entry.Provenance != nil { - if !opts.AllowSignerChange { - if blocked := s.guardSignerChange(ctx, entry, latest, &outcome); blocked { - return upgradePlan{entry: entry, outcome: outcome} - } - } else if entry.Provenance.PublicKey != "" { - allowSignerChange = !s.candidateMatchesPinnedKey(ctx, entry, latest) - } + 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}) @@ -243,15 +226,58 @@ func (s *service) planUpgrade(ctx context.Context, opts plugins.UpgradeOptions, } } +// 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 // 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. // -// A key-pinned entry has no identity to probe and is handed to -// guardKeyedSignerChange instead. -// // 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 // ref rotate to any other tag ref, reasoning that a release workflow signs @@ -272,9 +298,6 @@ func (s *service) guardSignerChange( latest resolvedLatest, outcome *plugins.UpgradeOutcome, ) bool { - if entry.Provenance.PublicKey != "" { - return s.guardKeyedSignerChange(ctx, entry, latest, outcome) - } probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) switch { case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned): @@ -299,147 +322,208 @@ func (s *service) guardSignerChange( return false } -// guardKeyedSignerChange is guardSignerChange for an entry pinned to a cosign -// public key. There is no identity to probe for and compare — a key-pair -// bundle carries no certificate — so the pinned key is applied to the -// candidate directly: verifying against it IS the evidence that the signer -// has not changed, and the upgrade then proceeds on the anchor the entry -// already records. No new key is accepted here; the lock supplies it. +// recordKeyedVerdict turns a keyed verdict into a plan outcome and reports +// whether the upgrade stops here. // -// The candidate is verified over OCI unconditionally, with no git arm: the -// lock schema refuses publicKey on a git entry (a commit signature is checked -// against a certificate, never a key), so a key-pinned entry is always an OCI -// one and latest.commitPayload is always empty 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 arms are split by whether --allow-signer-change would actually help, -// because a blocked outcome is what tells the caller to reach for it. It -// helps for a candidate that moved to keyless signing: dropping the recorded -// key and re-verifying is exactly the key-to-keyless move resolveKeyAnchor -// supports. It does NOT help for a candidate that dropped its signature — -// the override re-verifies from scratch, and upgrade carries no -// unsigned-consent flag to pair with it, so the install it waves through -// fails unsigned-rejected anyway — nor for a candidate signed by a different -// key, which needs an in-place re-anchor v1 does not offer. Both of those -// report a failure naming the route that works instead of a remedy that -// leads nowhere. -func (s *service) guardKeyedSignerChange( - ctx context.Context, - entry lockfile.Entry, - latest resolvedLatest, - outcome *plugins.UpgradeOutcome, +// 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 { - pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey) - if err != nil { - outcome.Status = plugins.UpgradeStatusFailed - outcome.Reason = plugins.FailureReasonUnknown - outcome.Error = fmt.Errorf("lock entry's pinned %w", err).Error() - return true - } - _, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, latest.ref, latest.digest, pubKeyPEM) - if verifyErr == nil { + switch verdict.kind { + case keyedPinHolds: return false - } - 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. + case keyedMovedToKeyless: + if allowSignerChange { + return false + } + outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + outcome.NewSignerIdentity = verdict.identity + return true + case keyedUndecided: outcome.Status = plugins.UpgradeStatusFailed - outcome.Reason = plugins.FailureReasonUnsignedRejected - outcome.Error = 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, uninstall it and reinstall it with"+ - " `thv ai-plugin install --allow-unsigned`", verifyErr).Error() + outcome.Reason = verdict.reason + outcome.Error = verdict.err return true } - return s.classifyKeyedCandidate(ctx, entry, latest, verifyErr, outcome) + 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 } -// candidateMatchesPinnedKey reports whether the candidate still verifies -// against the public key the entry pins. +// judgeKeyedCandidate measures a candidate against the public key its entry +// pins, verifying it keylessly when the key does not apply. // -// It exists because --allow-signer-change is a project-wide flag applied per -// entry. The override authorizes dropping a recorded anchor when the -// artifact has genuinely moved off it — not ignoring an anchor the artifact -// still satisfies. resolveKeyAnchor drops the recorded key whenever the flag -// is set, so forwarding it here would push a still-key-signed candidate -// through keyless verification and fail it with ErrKeySigned. That is -// reachable without asking for it: needing the override for one plugin in a -// multi-plugin project would otherwise break every key-pinned plugin -// alongside it. +// 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. // -// An undecodable pin reports false. There is nothing to preserve, and the -// override is exactly the escape hatch for an entry whose anchor can no -// longer be applied. -func (s *service) candidateMatchesPinnedKey( +// 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, -) bool { +) keyedVerdict { pubKeyPEM, err := verifier.DecodePublicKey(entry.Provenance.PublicKey) if err != nil { - return false + 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) - return verifyErr == nil + 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), + } } -// classifyKeyedCandidate decides what a candidate that failed verification -// against the pinned key actually is, by verifying it keylessly before -// committing to a diagnosis. A keyless bundle that verifies means the -// artifact moved to keyless signing: a signer change --allow-signer-change -// authorizes, reported with the identity it moved to. -// -// The 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 alongside a stale -// key-pair one — comes back as ErrSignatureInvalid instead. Deciding on the -// keyed error alone would call that a damaged signature and send the caller -// to uninstall-and-reinstall, when the override resolves it: dropping the -// pin lets ordinary keyless verification accept the valid bundle. Only the -// probe tells the two apart. +// conclusiveKeyedMismatch reports whether a failed keyed verification +// actually settled that the pinned key does not apply to the candidate. // -// Naming the identity is load-bearing rather than cosmetic. The CLI renders -// a blocked outcome carrying no NewSignerIdentity as "unsigned", so a -// keyless candidate left unnamed is reported as the one thing the guard has -// just established it is not. -func (s *service) classifyKeyedCandidate( - ctx context.Context, - entry lockfile.Entry, - latest resolvedLatest, - verifyErr error, - outcome *plugins.UpgradeOutcome, -) bool { - if probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest); probeErr == nil { - outcome.Status = plugins.UpgradeStatusSignerChangeBlocked - outcome.NewSignerIdentity = probe.SignerIdentity - return true - } else if errors.Is(verifyErr, verifier.ErrKeylessSigned) { +// 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. - outcome.Status = plugins.UpgradeStatusFailed - outcome.Reason = classifySignatureError(probeErr) - if outcome.Reason == "" { - outcome.Reason = plugins.FailureReasonUnknown + if reason := classifySignatureError(probeErr); reason != "" { + return reason } - outcome.Error = fmt.Errorf("candidate dropped key-pair signing for keyless, but its keyless"+ - " signature does not verify: %w", probeErr).Error() - return true + return plugins.FailureReasonUnknown } + return plugins.FailureReasonSignatureInvalid +} - outcome.Status = plugins.UpgradeStatusFailed - if errors.Is(verifyErr, verifier.ErrSignatureInvalid) { - outcome.Reason = plugins.FailureReasonSignatureInvalid - outcome.Error = 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; uninstall the plugin and"+ - " reinstall it with `thv ai-plugin install --public-key`)", verifyErr).Error() - return true +// 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() } - // A registry, transport, or context failure says nothing about the - // signature. Calling it signature-invalid would advise uninstalling a - // working plugin over a network blip. - outcome.Reason = plugins.FailureReasonUnknown - outcome.Error = fmt.Errorf("verifying candidate against the pinned cosign public key: %w", - verifyErr).Error() - return true +} + +// 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 diff --git a/pkg/plugins/pluginsvc/upgrade_verify_test.go b/pkg/plugins/pluginsvc/upgrade_verify_test.go index cfd9fab75e..a04aa3fe1d 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -551,19 +551,11 @@ func TestUpgrade_OversizedCommitMaterialFailsRatherThanBlocks(t *testing.T) { assert.Contains(t, outcome.Error, "over the") } -// TestGuardKeyedSignerChange covers the guard for a key-pinned entry. The -// keyless guard probes for a certificate identity to compare, which a -// key-pair bundle does not have — so before this branch a key-pinned entry -// could never be upgraded at all: probeCandidateSigner returned ErrKeySigned -// and the plan failed with a signature error, where a policy decision was -// intended. -// -// The blocked arms are split by whether the remedy the CLI prints actually -// works. "signer change blocked" tells the user to pass --allow-signer-change, -// which drops the recorded key and re-verifies keylessly — a real fix when the -// candidate moved to keyless signing or lost its signature, and a dead end -// when it is simply signed by a different key. -func TestGuardKeyedSignerChange(t *testing.T) { +// 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) @@ -574,77 +566,103 @@ func TestGuardKeyedSignerChange(t *testing.T) { } tests := []struct { - name string - verifyErr error - probe *verifier.Result - probeErr error - noProbe bool - wantBlocked bool - wantStatus plugins.UpgradeStatus - wantReason plugins.FailureReason - wantSigner string - wantErrText string - wantNoText string + 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, - wantBlocked: false, + 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"}, - wantBlocked: true, - wantStatus: plugins.UpgradeStatusSignerChangeBlocked, - wantSigner: "ci@example.com", + 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 this guard but not verification, - // and upgrade has no --allow-unsigned to pair with it, so calling - // this a signer change would advertise a route that dead-ends in - // unsigned-rejected one step later. - name: "a candidate that lost its signature is an unsigned rejection, not a signer change", + // --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, - wantBlocked: true, - wantStatus: plugins.UpgradeStatusFailed, + wantKind: keyedUndecided, wantReason: plugins.FailureReasonUnsignedRejected, - wantErrText: "reinstall it with `thv ai-plugin install --allow-unsigned`", + 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, - wantBlocked: true, - wantStatus: plugins.UpgradeStatusFailed, + wantKind: keyedUndecided, wantReason: plugins.FailureReasonSignatureInvalid, - wantErrText: "reinstall it with `thv ai-plugin install --public-key`", + wantErrText: "--scope project --public-key", + wantBlocked: true, wantBlockedOverride: true, + wantStatus: plugins.UpgradeStatusFailed, }, { - // 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 key-to-keyless - // transition, and must not be sent to uninstall-and-reinstall. - name: "a mixed keyless and stale-key artifact is the supported transition", - verifyErr: verifier.ErrSignatureInvalid, - probe: &verifier.Result{Signed: true, SignerIdentity: "ci@example.com"}, - wantBlocked: true, - wantStatus: plugins.UpgradeStatusSignerChangeBlocked, - wantSigner: "ci@example.com", + 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, }, { - // A registry or transport failure says nothing about the - // signature, so it must not advise uninstalling the plugin. + // 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), - probeErr: context.DeadlineExceeded, - wantBlocked: true, - wantStatus: plugins.UpgradeStatusFailed, + 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 { @@ -664,65 +682,45 @@ func TestGuardKeyedSignerChange(t *testing.T) { Times(probeCalls).Return(tc.probe, tc.probeErr) svc := &service{sigVerifier: mv} - outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} - blocked := svc.guardSignerChange( - t.Context(), keyedLockEntry("keyed-plugin"), latest, &outcome) + verdict := svc.judgeKeyedCandidate(t.Context(), keyedLockEntry("keyed-plugin"), latest) - assert.Equal(t, tc.wantBlocked, blocked) - if !tc.wantBlocked { - return - } - assert.Equal(t, tc.wantStatus, outcome.Status) - assert.Equal(t, tc.wantReason, outcome.Reason) + 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, outcome.Error, tc.wantErrText) + assert.Contains(t, verdict.err, tc.wantErrText) } if tc.wantNoText != "" { - assert.NotContains(t, outcome.Error, tc.wantNoText, + assert.NotContains(t, verdict.err, tc.wantNoText, "an operational failure must not carry a destructive remedy") } - // 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.wantSigner, outcome.NewSignerIdentity) - }) - } -} - -// TestGuardKeyedSignerChange_KeylessProbeFailure keeps a candidate whose -// keyless signature does not verify out of the blocked bucket. Blocked tells -// the caller to re-run with --allow-signer-change, which skips this guard but -// still verifies, so the flag cannot get such a candidate installed either. -func TestGuardKeyedSignerChange_KeylessProbeFailure(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), + 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) + } + }) } - - mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) - mv.EXPECT().VerifyOCIWithKey(gomock.Any(), latest.ref, latest.digest, keyPEM). - Return(nil, verifier.ErrKeylessSigned) - mv.EXPECT().VerifyOCI(gomock.Any(), latest.ref, latest.digest, gomock.Nil()). - Return(nil, fmt.Errorf("probing: %w", verifier.ErrSignatureInvalid)) - - svc := &service{sigVerifier: mv} - outcome := plugins.UpgradeOutcome{Name: "keyed-plugin"} - require.True(t, svc.guardSignerChange( - t.Context(), keyedLockEntry("keyed-plugin"), latest, &outcome)) - - assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) - assert.Equal(t, plugins.FailureReasonSignatureInvalid, outcome.Reason) - assert.Empty(t, outcome.NewSignerIdentity) } -// TestGuardKeyedSignerChange_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. -func TestGuardKeyedSignerChange_UndecodablePinnedKey(t *testing.T) { +// 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") @@ -732,14 +730,19 @@ func TestGuardKeyedSignerChange_UndecodablePinnedKey(t *testing.T) { mv.EXPECT().VerifyOCI(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) svc := &service{sigVerifier: mv} - outcome := plugins.UpgradeOutcome{Name: entry.Name} - require.True(t, svc.guardSignerChange(t.Context(), entry, resolvedLatest{ + verdict := svc.judgeKeyedCandidate(t.Context(), entry, resolvedLatest{ ref: "ghcr.io/org/keyed-plugin:v2", digest: "sha256:" + strings.Repeat("c", 64), - }, &outcome)) - assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) - assert.Equal(t, plugins.FailureReasonUnknown, outcome.Reason) - assert.NotEqual(t, plugins.UpgradeStatusSignerChangeBlocked, outcome.Status) + }) + 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 — @@ -924,3 +927,55 @@ func TestUpgrade_AllowSignerChangeKeepsSameKeyPin(t *testing.T) { "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") +}