Enforce recorded ref and runner on skill verification

docs/arch/12-skills-system.md

@@ -331,6 +331,8 @@ RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080
 
 **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`.
 
+The Sigstore policy alone is not the whole guarantee: its SAN match deliberately leaves the signing workflow's git ref unpinned (`(@.*)?$`), so identity alone is satisfied by "the right workflow, on any branch." Two additional certificate fields — the git ref the signing workflow ran on and the runner class it executed in (`repositoryRef:`/`runnerEnvironment:` in `provenance:`) — are enforced separately, after the Sigstore policy succeeds, against the certificate's Fulcio extensions. An entry recorded before these fields existed, or a certificate that carries neither (a signer outside CI), is unconstrained on that field — never a wildcard match once something IS recorded. Install, sync, and upgrade all require the recorded ref and runner class to match exactly, with no automatic allowance for any kind of change, ref rotation included: an earlier version of this guard let a recorded tag ref rotate to any other tag ref automatically, reasoning that a release workflow signs each version on its own tag, but review found that this let a candidate signed from an attacker's own tag on the same repository (e.g. `refs/tags/attacker-release`) replace a pinned tag just as easily, since nothing tied the candidate's tag to the version actually being upgraded to. A ref or runner-class change of any shape is now blocked exactly like a genuine signer-identity change, and needs the same explicit `--allow-signer-change` to proceed and re-record it.
+
 What is still trusted on faith, deliberately and visibly:
 
 - **Unsigned skills** install only with an explicit `--allow-unsigned`, recorded as `unsigned: true` in the lock entry. That entry is a standing exception: lock-driven operations (sync restores, upgrade re-pins) honor it without re-asking.

pkg/skills/options.go

@@ -290,6 +290,12 @@ const (
 	// FailureReasonSignerMismatch means the artifact verifies, but against
 	// an identity other than the one recorded in the lock file.
 	FailureReasonSignerMismatch FailureReason = "signer-mismatch"
+	// FailureReasonProvenanceFieldMismatch means the artifact verifies
+	// against the recorded signer identity and issuer, but its
+	// certificate's repository ref or runner environment differs from what
+	// is pinned — a narrower case than FailureReasonSignerMismatch, whose
+	// remediation (--allow-signer-change) is nonetheless the same.
+	FailureReasonProvenanceFieldMismatch FailureReason = "provenance-field-mismatch"
 	// FailureReasonUnsignedRejected means the artifact is unsigned and the
 	// operation did not permit unsigned installs.
 	FailureReasonUnsignedRejected FailureReason = "unsigned-rejected"

pkg/skills/skillsvc/upgrade.go

@@ -175,8 +175,23 @@ func (s *service) planUpgrade(ctx context.Context, opts skills.UpgradeOptions, e
 
 // 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, or
-// its signature cannot be verified at all. Returns true when blocked.
+// 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.
+//
+// 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
+// other tag ref, reasoning that a release workflow signs each version on
+// its own tag — but that also let a candidate signed from an attacker's OWN
+// tag (e.g. "refs/tags/attacker-release") on the SAME repository replace a
+// pinned tag, since nothing tied the candidate's tag to the specific
+// version actually being upgraded to. Binding it correctly would need the
+// resolved release source's own tag, which the git resolver does not
+// surface at all (only the resolved commit hash) — so an OCI-only partial
+// fix would leave git-sourced skills with the identical hole. Every ref
+// change — tag or branch, git or OCI — therefore blocks here exactly like a
+// genuine signer-identity change, and needs the same explicit
+// --allow-signer-change override. See stacklok/toolhive#6315 review.
 func (s *service) guardSignerChange(
 	ctx context.Context,
 	entry lockfile.Entry,
@@ -197,14 +212,33 @@ func (s *service) guardSignerChange(
 		outcome.Error = probeErr.Error()
 		return true
 	case probe.SignerIdentity != entry.Provenance.SignerIdentity ||
-		probe.CertIssuer != entry.Provenance.CertIssuer:
+		probe.CertIssuer != entry.Provenance.CertIssuer ||
+		runnerEnvironmentChanged(probe, entry.Provenance) ||
+		repositoryRefChanged(probe, entry.Provenance):
 		outcome.Status = skills.UpgradeStatusSignerChangeBlocked
 		outcome.NewSignerIdentity = probe.SignerIdentity
 		return true
 	}
 	return false
 }
 
+// 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
+// empty, as do certificates that carry no such extension.
+func runnerEnvironmentChanged(probe *verifier.Result, recorded *lockfile.Provenance) bool {
+	return recorded.RunnerEnvironment != "" && probe.RunnerEnvironment != recorded.RunnerEnvironment
+}
+
+// repositoryRefChanged reports whether the candidate's certificate ref
+// differs from the one recorded, with the same absent-means-unconstrained
+// rule as runnerEnvironmentChanged. Unlike the runner class, no ref value
+// is treated as an automatically allowed rotation — see guardSignerChange's
+// doc comment for why.
+func repositoryRefChanged(probe *verifier.Result, recorded *lockfile.Provenance) bool {
+	return recorded.RepositoryRef != "" && probe.RepositoryRef != recorded.RepositoryRef
+}
+
 // probeCandidateSigner verifies the candidate artifact chain-of-trust-only
 // (nil expected identity) and returns the observed identity. Git candidates
 // are re-resolved at the pinned commit to obtain the signature material.

pkg/skills/skillsvc/upgrade_verify_test.go

@@ -69,6 +69,222 @@ func signerChangeFixture(
 	return svc, projectRoot
 }
 
+// TestUpgrade_RefChangeRequiresAllowSignerChange covers the ref-pinning
+// guard's current shape: ANY ref change — including a plausible-looking
+// tag-to-tag release rotation — blocks the upgrade exactly like a genuine
+// signer-identity change, and the existing --allow-signer-change override
+// is what re-pins it.
+//
+// An earlier version of this guard let a recorded tag ref rotate to any
+// other tag ref automatically, on the theory that a release workflow signs
+// each version on its own tag. Panel review on stacklok/toolhive#6315 found
+// that this let a candidate signed from an attacker's OWN tag on the same
+// repository (e.g. "refs/tags/attacker-release") replace a pinned tag,
+// since nothing tied the candidate's tag to the specific version actually
+// being upgraded to — binding it correctly would need the resolved release
+// source's own tag, which the git resolver never surfaces (only the
+// resolved commit hash), so a fix scoped to OCI would have left git-sourced
+// skills with the identical hole. The automatic allowance was removed
+// rather than patched per-format.
+//
+//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel
+func TestUpgrade_RefChangeRequiresAllowSignerChange(t *testing.T) {
+	const (
+		installedRef = "refs/tags/v0.1.0"
+		releaseRef   = "refs/tags/v0.2.0"
+	)
+	gr, fx := newGitResolverMock(t)
+	fx.register("repin-skill", gitSkill("repin-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, expected *lockfile.Provenance) (*verifier.Result, error) {
+			calls++
+			if calls == 1 {
+				return refSignedResult(installedRef), nil // initial install (TOFU)
+			}
+			candidate := refSignedResult(releaseRef)
+			if expected == nil {
+				return candidate, nil // the upgrade's plan-time signer probe
+			}
+			if expected.RepositoryRef != "" && expected.RepositoryRef != candidate.RepositoryRef {
+				return nil, verifier.ErrSignerMismatch
+			}
+			return candidate, nil
+		})
+	mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+	svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv))
+	ref, _ := gitRef("repin-skill")
+	_, err := svc.Install(t.Context(), skills.InstallOptions{
+		Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"},
+	})
+	require.NoError(t, err)
+	entry, ok := readLockfile(t, projectRoot).Get("repin-skill")
+	require.True(t, ok)
+	require.NotNil(t, entry.Provenance)
+	require.Equal(t, installedRef, entry.Provenance.RepositoryRef, "the install must pin the observed ref")
+
+	fx.register("repin-skill", gitSkillVersion("repin-skill"))
+
+	// Without the override, even a tag-shaped rotation is blocked.
+	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,
+		"a ref change has no automatic allowance, even a plausible release-tag rotation")
+
+	entry, ok = readLockfile(t, projectRoot).Get("repin-skill")
+	require.True(t, ok)
+	require.NotNil(t, entry.Provenance)
+	assert.Equal(t, installedRef, entry.Provenance.RepositoryRef, "a blocked upgrade must not touch the lock")
+
+	// With the explicit override, it proceeds and re-pins the new ref —
+	// the same mechanism a genuine signer-identity change already uses.
+	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)
+
+	entry, ok = readLockfile(t, projectRoot).Get("repin-skill")
+	require.True(t, ok)
+	require.NotNil(t, entry.Provenance)
+	assert.Equal(t, releaseRef, entry.Provenance.RepositoryRef,
+		"the override must re-record the new ref, so the next install enforces it")
+}
+
+func TestRepositoryRefChanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		probe    string
+		recorded string
+		want     bool
+	}{
+		{name: "same ref", probe: "refs/tags/v0.1.0", recorded: "refs/tags/v0.1.0"},
+		{name: "entry recorded none is unconstrained", probe: "refs/heads/attacker"},
+		{name: "tag rotation blocked", probe: "refs/tags/v0.2.0", recorded: "refs/tags/v0.1.0", want: true},
+		{name: "branch change blocked", probe: "refs/heads/attacker", recorded: "refs/heads/main", want: true},
+		{name: "candidate carrying none blocked", recorded: "refs/tags/v0.1.0", want: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tc.want, repositoryRefChanged(
+				&verifier.Result{RepositoryRef: tc.probe},
+				&lockfile.Provenance{RepositoryRef: tc.recorded}))
+		})
+	}
+}
+
+// TestUpgrade_RefTransitionBlocked is the regression test for the ref-pin
+// guard: guardSignerChange must reject ANY ref change without an explicit
+// --allow-signer-change, and critically, the transition must never reach
+// applyUpgrade's install call at all, so the lock stays untouched. Before
+// the original fix, every upgrade unconditionally cleared the expected ref
+// with no prior check, so a candidate signed by the same identity, issuer,
+// and runner from a different branch would pass and silently replace the
+// locked ref — the exact substitution this PR's ref pinning exists to
+// catch. See TestUpgrade_RefChangeRequiresAllowSignerChange for why even a
+// plausible tag-to-tag rotation is included, not just an obviously
+// suspicious branch change.
+//
+//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel
+func TestUpgrade_RefTransitionBlocked(t *testing.T) {
+	tests := []struct {
+		name        string
+		lockedRef   string
+		candidate   string
+		description string
+	}{
+		{
+			name: "attacker branch", lockedRef: "refs/heads/main", candidate: "refs/heads/attacker",
+			description: "same identity, issuer, and runner, signed from a different branch",
+		},
+		{
+			name: "candidate lost its ref extension", lockedRef: "refs/tags/v0.1.0", candidate: "",
+			description: "a certificate that stopped carrying a ref extension must not silently unpin one",
+		},
+		{
+			name: "plausible tag rotation", lockedRef: "refs/tags/v0.1.0", candidate: "refs/tags/v0.2.0",
+			description: "a tag-to-tag rotation has no automatic allowance either — see the test above for why",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gr, fx := newGitResolverMock(t)
+			fx.register("ref-guarded-skill", gitSkill("ref-guarded-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, expected *lockfile.Provenance) (*verifier.Result, error) {
+					calls++
+					if calls == 1 {
+						return refSignedResult(tc.lockedRef), nil // initial install (TOFU)
+					}
+					candidate := refSignedResult(tc.candidate)
+					if expected == nil {
+						return candidate, nil // the upgrade's plan-time signer probe
+					}
+					// A blocked transition must never reach here: applyUpgrade
+					// is only called when guardSignerChange did not block.
+					t.Fatalf("install-time verification must not run for a blocked ref transition: %s", tc.description)
+					return nil, nil
+				})
+			mv.EXPECT().VerifyBundleOffline(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+			svc, projectRoot := newLockTestService(t, gr, WithVerifier(mv))
+			ref, _ := gitRef("ref-guarded-skill")
+			_, err := svc.Install(t.Context(), skills.InstallOptions{
+				Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"},
+			})
+			require.NoError(t, err)
+
+			fx.register("ref-guarded-skill", gitSkillVersion("ref-guarded-skill"))
+			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, tc.description)
+
+			entry, ok := readLockfile(t, projectRoot).Get("ref-guarded-skill")
+			require.True(t, ok)
+			require.NotNil(t, entry.Provenance)
+			assert.Equal(t, tc.lockedRef, entry.Provenance.RepositoryRef,
+				"a blocked transition must leave the locked ref untouched")
+		})
+	}
+}
+
+func TestRunnerEnvironmentChanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		probe    string
+		recorded string
+		want     bool
+	}{
+		{name: "same runner class", probe: testRunnerEnvironment, recorded: testRunnerEnvironment},
+		{name: "entry recorded none is unconstrained", probe: "self-hosted"},
+		{name: "runner class change blocked", probe: "self-hosted", recorded: testRunnerEnvironment, want: true},
+		{name: "candidate carrying none blocked", recorded: testRunnerEnvironment, want: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tc.want, runnerEnvironmentChanged(
+				&verifier.Result{RunnerEnvironment: tc.probe},
+				&lockfile.Provenance{RunnerEnvironment: tc.recorded}))
+		})
+	}
+}
+
 //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel
 func TestUpgrade_SignerChangeBlocked(t *testing.T) {
 	svc, projectRoot := signerChangeFixture(t, func() (*verifier.Result, error) {