diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index 3741219e88..e2244056bf 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -415,7 +415,7 @@ oci_tags table (reserved; not currently populated) RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080-skills-lock-file.md) adds a project-level `toolhive.lock.yaml`, committed at the project root, that pins the exact content of every project-scoped skill install — the same guarantee `package-lock.json`, `Cargo.lock`, and `go.sum` provide elsewhere. Two teammates (or a CI runner) cloning the same repo restore identical skill content via `thv skill sync`, rather than whatever the source currently resolves to. -**Trust model, stated plainly:** project-scoped installs are verified against Sigstore signatures, and the lock file records the trust decisions those verifications produce. On first install of a signed skill the observed signer identity is recorded (trust on first use) as the entry's `provenance:` block and **displayed to the user**; every later install, sync, and upgrade enforces that identity *inside* the Sigstore verification policy — OCI artifacts through their attached signature bundles, git commits through gitsign signature-and-chain verification (currently `provisional: true`: the transparency-log proof of signing time is not yet validated, so the replay window is unbounded until that lands). Sync additionally re-verifies each entry's stored signature bundle offline (embedded trust root, no network) before counting it current, and upgrade refuses to move to an artifact signed by a different identity — or unsigned — without an explicit `--allow-signer-change`. +**Trust model, stated plainly:** project-scoped installs are verified against Sigstore signatures, and the lock file records the trust decisions those verifications produce. On first install of a signed skill the observed signer identity is recorded (trust on first use) as the entry's `provenance:` block and **displayed to the user**; every later install, sync, and upgrade enforces that identity *inside* the Sigstore verification policy — OCI artifacts through their attached signature bundles, git commits through gitsign signature-and-chain verification (currently `provisional: true`: the transparency-log proof of signing time is not yet validated, so the replay window is unbounded until that lands). Sync additionally re-verifies each entry's stored signature bundle offline (embedded trust root, no network) before counting it current, and upgrade refuses to move to an artifact signed by a different identity without an explicit `--allow-signer-change`. An unsigned candidate is not a signer change but a failure (`unsigned-rejected`): upgrade has no unsigned-consent flag, and `--allow-signer-change` re-verifies from scratch, which an unsigned artifact still fails, so the failure names the project-scoped reinstall with `--allow-unsigned` that records the exception explicitly. **First install need not be blind.** When a skill resolves through the registry/catalog (a plain name like `thv skill install code-review`, not a direct OCI or git reference) and that catalog entry declares an expected `provenance` (`toolhive-core`'s `registry.Skill.Provenance`, RFC THV-0080 follow-up [#6310](https://github.com/stacklok/toolhive/issues/6310)), the very first project-scoped install is checked against it instead of unconditionally trusting whatever the artifact's signature says. Each non-empty supported field constrains independently; an empty or absent catalog provenance preserves ordinary trust on first use. For multi-signed OCI artifacts, every cryptographically valid signature is considered until one satisfies all declared constraints. A lock entry, including a legacy entry with no recorded trust state, always takes precedence over the catalog — the catalog only fills the gap before any trust decision has been recorded. User-scoped installs do not use project lock-file verification and therefore do not apply catalog provenance. diff --git a/docs/arch/14-plugins-system.md b/docs/arch/14-plugins-system.md index be8ddd0433..efd36f3df0 100644 --- a/docs/arch/14-plugins-system.md +++ b/docs/arch/14-plugins-system.md @@ -185,6 +185,11 @@ installing (OCI candidates are still fetched to compare digests). `--allow-ref-change` permits a repository move, while `--allow-signer-change` permits supported trust transitions. Neither option silently replaces a pinned cosign public key with an arbitrary different key. +An unsigned candidate under an entry that records a signer is not a trust +transition but a failure (`unsigned-rejected`): upgrade has no unsigned-consent +flag, and `--allow-signer-change` re-verifies from scratch, which an unsigned +artifact still fails, so the failure names the project-scoped reinstall with +`--allow-unsigned` that records the exception explicitly. ## Storage diff --git a/pkg/plugins/pluginsvc/upgrade.go b/pkg/plugins/pluginsvc/upgrade.go index e8822afaeb..110e8250df 100644 --- a/pkg/plugins/pluginsvc/upgrade.go +++ b/pkg/plugins/pluginsvc/upgrade.go @@ -253,7 +253,10 @@ func (s *service) resolveSignerPolicy( outcome *plugins.UpgradeOutcome, ) (allowSignerChange, blocked bool) { if entry.Provenance == nil { - return opts.AllowSignerChange, false + // No recorded signer, so there is no signer change to authorize: + // forwarding the override would only clear the install's + // expectations for an entry that has none to clear. + return false, false } if entry.Provenance.PublicKey != "" { // A keyed entry is measured against its pin whether or not the @@ -266,7 +269,7 @@ func (s *service) resolveSignerPolicy( } return opts.AllowSignerChange && verdict.kind == keyedMovedToKeyless, false } - if !opts.AllowSignerChange && s.guardSignerChange(ctx, entry, latest, outcome) { + if s.guardSignerChange(ctx, entry, latest, opts.AllowSignerChange, outcome) { return false, true } return opts.AllowSignerChange, false @@ -274,9 +277,16 @@ func (s *service) resolveSignerPolicy( // guardSignerChange probes the candidate artifact's signer identity and // fills outcome when the upgrade must not proceed: the candidate is signed -// by a different identity (or unsigned) versus the recorded provenance, its -// signature cannot be verified at all, or its certificate's repository ref -// or runner class differs from what is recorded. Returns true when blocked. +// by a different identity versus the recorded provenance, its signature +// cannot be verified at all, it is unsigned, or its certificate's repository +// ref or runner class differs from what is recorded. Returns true when +// blocked. Only a differing identity or provenance field is reported as a +// signer change, and only that arm is waived by allowSignerChange; the rest +// are failures in both modes, because --allow-signer-change would not +// resolve them — it re-verifies from scratch, which an unsigned or +// unverifiable candidate fails just the same. Running the guard under the +// override is what keeps a project-wide flag from waving those through to +// an install that then either refuses them or, worse, records them. // // The repository ref has NO automatic allowance for a tag-shaped rotation. // An earlier version of the skills guard this mirrors let a recorded tag @@ -296,12 +306,25 @@ func (s *service) guardSignerChange( ctx context.Context, entry lockfile.Entry, latest resolvedLatest, + allowSignerChange bool, outcome *plugins.UpgradeOutcome, ) bool { probe, probeErr := s.probeCandidateSigner(ctx, entry.Name, latest) switch { case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned): - outcome.Status = plugins.UpgradeStatusSignerChangeBlocked + // Not a signer change, because the remedy a signer change names + // cannot resolve it: --allow-signer-change re-verifies from scratch + // and re-records what it observes, and an unsigned artifact fails + // that verification exactly as it fails this one. Upgrade has no + // unsigned-consent flag, so the only way onto an unsigned artifact + // is the reinstall that records the exception explicitly. + outcome.Status = plugins.UpgradeStatusFailed + outcome.Reason = plugins.FailureReasonUnsignedRejected + outcome.Error = fmt.Errorf("candidate is unsigned, and this entry is pinned to a signer"+ + " identity: %w. Upgrade has no unsigned-consent flag, and --allow-signer-change is not"+ + " one — it re-verifies from scratch, which an unsigned artifact still fails. To move"+ + " this plugin to an unsigned artifact, reinstall it: %s", + probeErr, projectReinstallCommand(entry, "--allow-unsigned")).Error() return true case probeErr != nil: outcome.Status = plugins.UpgradeStatusFailed @@ -315,6 +338,11 @@ func (s *service) guardSignerChange( probe.CertIssuer != entry.Provenance.CertIssuer || runnerEnvironmentChanged(probe, entry.Provenance) || repositoryRefChanged(probe, entry.Provenance): + // The one arm the override resolves: it re-verifies from scratch + // and re-records the identity actually observed. + if allowSignerChange { + return false + } outcome.Status = plugins.UpgradeStatusSignerChangeBlocked outcome.NewSignerIdentity = probe.SignerIdentity return true diff --git a/pkg/plugins/pluginsvc/upgrade_verify_test.go b/pkg/plugins/pluginsvc/upgrade_verify_test.go index a04aa3fe1d..66471bcbc9 100644 --- a/pkg/plugins/pluginsvc/upgrade_verify_test.go +++ b/pkg/plugins/pluginsvc/upgrade_verify_test.go @@ -133,14 +133,30 @@ func TestUpgrade_SignerChangeBlocked(t *testing.T) { } //nolint:paralleltest // serial: real sqlite + on-disk client materialization per test -func TestUpgrade_UnsignedCandidateBlockedAgainstSignedEntry(t *testing.T) { +func TestUpgrade_UnsignedCandidateRejectedAgainstSignedEntry(t *testing.T) { svc, projectRoot := signerChangeFixture(t, func() (*verifier.Result, error) { return nil, verifier.ErrUnsigned }) - outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{ProjectRoot: projectRoot}) - assert.Equal(t, plugins.UpgradeStatusSignerChangeBlocked, outcome.Status) - assert.Empty(t, outcome.NewSignerIdentity, "an unsigned candidate has no identity to report") + // A signer change is resolved by --allow-signer-change, which re-verifies + // from scratch and fails on an unsigned artifact just the same, and + // upgrade has no unsigned-consent flag — so this is a failure in both + // modes, naming the reinstall that actually records the exception. + for _, override := range []bool{false, true} { + outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{ + ProjectRoot: projectRoot, AllowSignerChange: override, + }) + assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status, "allow_signer_change=%v", override) + assert.Equal(t, plugins.FailureReasonUnsignedRejected, outcome.Reason, "allow_signer_change=%v", override) + assert.Contains(t, outcome.Error, "--scope project --allow-unsigned", + "the remedy must be the reinstall that records the exception") + assert.Empty(t, outcome.NewSignerIdentity, "an unsigned candidate has no identity to report") + } + + entry, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.False(t, entry.Unsigned, "no unsigned exception may be recorded without --allow-unsigned") } // TestUpgrade_ProvenanceFieldDivergenceBlocked covers the fields beyond the @@ -361,6 +377,41 @@ func TestUpgrade_UnaffectedEntrySkipsSignerProbe(t *testing.T) { "the guard must not probe an entry that records no signer identity") } +// TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride is the +// override-mode twin of TestUpgrade_UnaffectedEntrySkipsSignerProbe: an entry +// the lock records as unsigned has no signer to change, so a project-wide +// --allow-signer-change must leave it exactly as a plain upgrade would rather +// than clearing expectUnsigned and refusing the artifact the lock accepts. +// +//nolint:paralleltest // serial: real sqlite + on-disk client materialization per test +func TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + + calls := 0 + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes(). + DoAndReturn(func(_ any, _, _ []byte, _ *verifier.ProvenanceExpectation) (*verifier.Result, error) { + calls++ + return nil, verifier.ErrUnsigned + }) + mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil) + + svc, projectRoot := newGitLockTestService(t, repoDir, WithVerifier(mv)) + require.NoError(t, gitInstall(t, svc, projectRoot, func(o *plugins.InstallOptions) { o.AllowUnsigned = true })) + require.Equal(t, 1, calls, "only the install itself consulted the verifier") + + addPluginRepoCommit(t, repoDir, "# hello unsigned") + outcome := upgradePlugins(t, svc, plugins.UpgradeOptions{ProjectRoot: projectRoot, AllowSignerChange: true}) + assert.Equal(t, plugins.UpgradeStatusUpgraded, outcome.Status, "error: %s", outcome.Error) + assert.Equal(t, 1, calls, "an entry with no recorded signer is never probed, override or not") + + after, ok := loadPluginLockEntry(t, projectRoot) + require.True(t, ok) + assert.True(t, after.Unsigned, "the recorded unsigned decision is preserved") + assert.Nil(t, after.Provenance) +} + func TestRepositoryRefChanged(t *testing.T) { t.Parallel() @@ -543,7 +594,7 @@ func TestUpgrade_OversizedCommitMaterialFailsRatherThanBlocks(t *testing.T) { Provenance: &lockfile.Provenance{SignerIdentity: testSignerIdentity}, }, resolvedLatest{commitPayload: make([]byte, maxSignatureBlobSize+1), commitSignature: "sig"}, - &outcome) + false, &outcome) assert.True(t, blocked, "an unusable probe must stop the upgrade") assert.Equal(t, plugins.UpgradeStatusFailed, outcome.Status) diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go index a9ac172e20..6bce802942 100644 --- a/pkg/skills/skillsvc/upgrade.go +++ b/pkg/skills/skillsvc/upgrade.go @@ -227,7 +227,10 @@ func (s *service) resolveSignerPolicy( outcome *skills.UpgradeOutcome, ) (allowSignerChange, blocked bool) { if entry.Provenance == nil { - return opts.AllowSignerChange, false + // No recorded signer, so there is no signer change to authorize: + // forwarding the override would only clear the install's + // expectations for an entry that has none to clear. + return false, false } if entry.Provenance.PublicKey != "" { // A keyed entry is measured against its pin whether or not the @@ -240,7 +243,7 @@ func (s *service) resolveSignerPolicy( } return opts.AllowSignerChange && verdict.kind == keyedMovedToKeyless, false } - if !opts.AllowSignerChange && s.guardSignerChange(ctx, entry, newRef, newDigest, outcome) { + if s.guardSignerChange(ctx, entry, newRef, newDigest, opts.AllowSignerChange, outcome) { return false, true } return opts.AllowSignerChange, false @@ -248,9 +251,16 @@ func (s *service) resolveSignerPolicy( // guardSignerChange probes the candidate artifact's signer identity and // fills outcome when the upgrade must not proceed: the candidate is signed -// by a different identity (or unsigned) versus the recorded provenance, its -// signature cannot be verified at all, or its certificate's repository ref -// or runner class differs from what is recorded. Returns true when blocked. +// by a different identity versus the recorded provenance, its signature +// cannot be verified at all, it is unsigned, or its certificate's repository +// ref or runner class differs from what is recorded. Returns true when +// blocked. Only a differing identity or provenance field is reported as a +// signer change, and only that arm is waived by allowSignerChange; the rest +// are failures in both modes, because --allow-signer-change would not +// resolve them — it re-verifies from scratch, which an unsigned or +// unverifiable candidate fails just the same. Running the guard under the +// override is what keeps a project-wide flag from waving those through to +// an install that then either refuses them or, worse, records them. // // The repository ref has NO automatic allowance for a tag-shaped rotation. // An earlier version of this guard let a recorded tag ref rotate to any @@ -269,12 +279,25 @@ func (s *service) guardSignerChange( ctx context.Context, entry lockfile.Entry, newRef, newDigest string, + allowSignerChange bool, outcome *skills.UpgradeOutcome, ) bool { probe, probeErr := s.probeCandidateSigner(ctx, newRef, newDigest) switch { case probeErr != nil && errors.Is(probeErr, verifier.ErrUnsigned): - outcome.Status = skills.UpgradeStatusSignerChangeBlocked + // Not a signer change, because the remedy a signer change names + // cannot resolve it: --allow-signer-change re-verifies from scratch + // and re-records what it observes, and an unsigned artifact fails + // that verification exactly as it fails this one. Upgrade has no + // unsigned-consent flag, so the only way onto an unsigned artifact + // is the reinstall that records the exception explicitly. + outcome.Status = skills.UpgradeStatusFailed + outcome.Reason = skills.FailureReasonUnsignedRejected + outcome.Error = fmt.Errorf("candidate is unsigned, and this entry is pinned to a signer"+ + " identity: %w. Upgrade has no unsigned-consent flag, and --allow-signer-change is not"+ + " one — it re-verifies from scratch, which an unsigned artifact still fails. To move"+ + " this skill to an unsigned artifact, reinstall it: %s", + probeErr, projectReinstallCommand(entry, "--allow-unsigned")).Error() return true case probeErr != nil: outcome.Status = skills.UpgradeStatusFailed @@ -288,6 +311,11 @@ func (s *service) guardSignerChange( probe.CertIssuer != entry.Provenance.CertIssuer || runnerEnvironmentChanged(probe, entry.Provenance) || repositoryRefChanged(probe, entry.Provenance): + // The one arm the override resolves: it re-verifies from scratch + // and re-records the identity actually observed. + if allowSignerChange { + return false + } outcome.Status = skills.UpgradeStatusSignerChangeBlocked outcome.NewSignerIdentity = probe.SignerIdentity return true diff --git a/pkg/skills/skillsvc/upgrade_verify_test.go b/pkg/skills/skillsvc/upgrade_verify_test.go index 0a1e99fdec..ba291715df 100644 --- a/pkg/skills/skillsvc/upgrade_verify_test.go +++ b/pkg/skills/skillsvc/upgrade_verify_test.go @@ -308,17 +308,39 @@ func TestUpgrade_SignerChangeBlocked(t *testing.T) { assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) } +// TestUpgrade_UnsignedCandidateRejectedAgainstSignedEntry: an entry pinned to +// a signer identity whose candidate lost its signature is a failure, not a +// signer change. The signer-change remedy is --allow-signer-change, which +// re-verifies from scratch and fails on an unsigned artifact just the same, +// and upgrade has no unsigned-consent flag — so reporting it as a change +// would send the caller into a second refusal. The failure names the +// reinstall that actually records the exception. +// //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestUpgrade_UnsignedCandidateBlockedAgainstSignedEntry(t *testing.T) { +func TestUpgrade_UnsignedCandidateRejectedAgainstSignedEntry(t *testing.T) { svc, projectRoot := signerChangeFixture(t, func() (*verifier.Result, error) { return nil, verifier.ErrUnsigned }) - result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert - require.NoError(t, err) - require.Len(t, result.Outcomes, 1) - assert.Equal(t, skills.UpgradeStatusSignerChangeBlocked, result.Outcomes[0].Status) - assert.Empty(t, result.Outcomes[0].NewSignerIdentity, "an unsigned candidate has no identity to report") + for _, override := range []bool{false, true} { + result, err := svc.(*service).Upgrade(t.Context(), //nolint:forcetypeassert + skills.UpgradeOptions{ProjectRoot: projectRoot, AllowSignerChange: override}) + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + outcome := result.Outcomes[0] + assert.Equal(t, skills.UpgradeStatusFailed, outcome.Status, "allow_signer_change=%v", override) + assert.Equal(t, skills.FailureReasonUnsignedRejected, outcome.Reason, "allow_signer_change=%v", override) + assert.Contains(t, outcome.Error, "--scope project --allow-unsigned", + "the remedy must be the reinstall that records the exception") + assert.Empty(t, outcome.NewSignerIdentity, "an unsigned candidate has no identity to report") + } + + // Nothing installed, lock unchanged in either mode. + entry, ok := loadLockEntry(t, projectRoot, "guarded-skill") + require.True(t, ok) + require.NotNil(t, entry.Provenance) + assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) + assert.False(t, entry.Unsigned, "no unsigned exception may be recorded without --allow-unsigned") } //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel @@ -361,6 +383,57 @@ func TestUpgrade_SignerChangePreviewParity(t *testing.T) { assert.Equal(t, testSignerIdentity, entry.Provenance.SignerIdentity) } +// TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride: an entry the +// lock records as unsigned has no signer to change, so a project-wide +// --allow-signer-change must leave it exactly as a plain upgrade would. +// Forwarding the override to its install would clear expectUnsigned and +// re-verify from scratch, refusing the unsigned artifact the lock already +// accepts — or, before isAllowedUnsigned stopped granting under the override, +// silently re-recording it. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverride(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("unsigned-skill", gitSkill("unsigned-skill")) + + calls := 0 + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyGit(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes(). + DoAndReturn(func(_ any, _, _ []byte, _ *verifier.ProvenanceExpectation) (*verifier.Result, error) { + calls++ + return nil, verifier.ErrUnsigned + }) + + svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv)) + ref, _ := gitRef("unsigned-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, AllowUnsigned: true, + }) + require.NoError(t, err) + entry, ok := loadLockEntry(t, projectRoot, "unsigned-skill") + require.True(t, ok) + require.True(t, entry.Unsigned) + require.Nil(t, entry.Provenance) + require.Equal(t, 1, calls, "only the install itself consulted the verifier") + + fx.register("unsigned-skill", gitSkillVersion("unsigned-skill")) + result, err := svc.(*service).Upgrade(t.Context(), //nolint:forcetypeassert + skills.UpgradeOptions{ProjectRoot: projectRoot, AllowSignerChange: true}) + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusUpgraded, result.Outcomes[0].Status, + "error: %s", result.Outcomes[0].Error) + assert.Equal(t, 1, calls, "an entry with no recorded signer is never probed, override or not") + + after, ok := loadLockEntry(t, projectRoot, "unsigned-skill") + require.True(t, ok) + assert.True(t, after.Unsigned, "the recorded unsigned decision is preserved") + assert.Nil(t, after.Provenance) + assert.Equal(t, result.Outcomes[0].NewDigest, after.Digest) +} + // TestJudgeKeyedCandidate covers the measurement of a candidate against a // key-pinned entry, and what each mode does with the result. The keyless // guard probes for a certificate identity to compare, which a key-pair bundle diff --git a/pkg/skills/skillsvc/verify.go b/pkg/skills/skillsvc/verify.go index 4e9526fe3f..d28eb39435 100644 --- a/pkg/skills/skillsvc/verify.go +++ b/pkg/skills/skillsvc/verify.go @@ -460,7 +460,13 @@ func isAllowedUnsigned(verifyErr error, opts skills.InstallOptions, expected *lo if !errors.Is(verifyErr, verifier.ErrUnsigned) || expected != nil { return false } - return opts.AllowUnsigned || lockDrivenInstall(opts) + // A lock-driven install honors the trust state the entry records — but + // under allow_signer_change the caller has just cleared that state to + // re-verify from scratch, so a nil expectation here says nothing about + // what the entry recorded. Granting on it would let the signer-change + // override double as unsigned consent, converting a signer-pinned entry + // into an unsigned one with no --allow-unsigned ever given. + return opts.AllowUnsigned || (lockDrivenInstall(opts) && !opts.AllowSignerChange) } // lockDrivenInstall reports whether this install materializes an existing diff --git a/pkg/skills/skillsvc/verify_test.go b/pkg/skills/skillsvc/verify_test.go index b07c01fb91..fabafa573d 100644 --- a/pkg/skills/skillsvc/verify_test.go +++ b/pkg/skills/skillsvc/verify_test.go @@ -1128,6 +1128,27 @@ func TestIsAllowedUnsignedRejectsKeySigned(t *testing.T) { "the genuine unsigned case must still be allowed through") } +// TestIsAllowedUnsignedNotGrantedUnderSignerChangeOverride: the lock-driven +// grant exists for entries the lock already records as unsigned or as +// predating verification. allow_signer_change clears the expectation to +// re-verify from scratch, so a nil expectation under it is not evidence of +// either — and honoring the grant there would turn the signer-change +// override into unsigned consent. +func TestIsAllowedUnsignedNotGrantedUnderSignerChangeOverride(t *testing.T) { + t.Parallel() + lockDriven := skills.InstallOptions{LockResolvedReference: "example.com/org/s:v2"} + assert.True(t, isAllowedUnsigned(verifier.ErrUnsigned, lockDriven, nil), + "precondition: a plain lock-driven install honors a recorded unsigned state") + + lockDriven.AllowSignerChange = true + assert.False(t, isAllowedUnsigned(verifier.ErrUnsigned, lockDriven, nil), + "the override must not double as --allow-unsigned") + + lockDriven.AllowUnsigned = true + assert.True(t, isAllowedUnsigned(verifier.ErrUnsigned, lockDriven, nil), + "an explicit --allow-unsigned still grants") +} + // TestCatalogInstallNamesKeySignedArtifact covers the second route to // classification. A first install resolved from a catalog entry that declares // provenance is classified by classifyCatalogVerifyError, not