Skip to content

Check first skill install against catalog-declared provenance - #6420

Merged
samuv merged 6 commits into
mainfrom
skills-catalog-provenance
Aug 24, 2026
Merged

samuv merged 6 commits into
mainfrom
skills-catalog-provenance

Conversation

@samuv

@samuv samuv commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #6310. Trust-on-first-use had no expected identity to check a
skill's very first install against — the verifier only recorded whatever
signer identity it observed, never validated it against anything. Every
install after that was enforced against the recorded lock entry, but the
first one was taken on faith.

toolhive-core v0.0.42 added Provenance to registry.Skill
(stacklok/toolhive-core#247, #248, #251) so a catalog entry can declare who
is expected to sign a given skill. This wires that into install-time
verification:

  • 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
    the resolved entry declares a provenance, it becomes the expected
    identity for verification — but only on a true first install. A lock
    entry, once one exists, always takes precedence over the catalog; the
    catalog only fills the gap before any trust decision has been recorded.
  • An absent or empty catalog provenance is unconstrained, matching every
    other provenance field's "absent means unconstrained" contract — most
    catalog entries won't declare one for a while, and that must not block
    installs.
  • A catalog-declared attestation constraint is refused outright rather
    than silently skipped: pkg/skills/verifier has no attestation-comparison
    logic today (unlike toolhive-core's own generic Sigstore.VerifyServer
    path used for MCP servers), so honoring the identity fields while quietly
    ignoring attestation would let a catalog author believe a guarantee is
    enforced when it isn't.

Also bumps toolhive-core to v0.0.42 as a separate commit (required for the
Skill.Provenance field) — that release also migrated toolhive-core's cel
package to the renamed cel.dev/cel-go module, which broke the two CEL call
sites in this repo (pkg/auth/awssts, pkg/authserver/server/tokenexchange)
that pass it EnvOptions from the old github.com/google/cel-go path. Fixed
as a mechanical import-path swap (confirmed Variable/MapType/StringType/
DynType all exist identically in the new module) in the same commit as the
bump, since the two are inseparable — any bump to v0.0.42 breaks the build
without it.

Type of change

  • New feature

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

New tests: provenanceInfoFromCatalog's conversion (nil, fully-populated,
and attestation-refused cases), plus two install-flow tests —
TestInstallVerification_CatalogProvenanceUsedOnFirstUse (a registry entry
with no prior lock entry gets its declared identity enforced instead of
blind TOFU) and TestInstallVerification_LockEntryTakesPrecedenceOverCatalog
(once a lock entry exists, it wins over a catalog that has since changed
its declared identity).

task test passes for pkg/skills/..., pkg/auth/..., and
pkg/authserver/.... Ran the full suite once too: three failures appeared
(pkg/transport/proxy/streamable — a documented pre-existing port-8096
conflict from a stray unrelated process in this environment;
pkg/vmcp/server's TestForwarding_Logging_RealBackend and
TestIntegration_SSEGetConnectionSurvivesWriteTimeout — both timing-based
integration tests, confirmed passing when rerun in isolation, i.e. flakes
under full-suite resource contention, not a regression from this change).
go build ./... and go vet ./... are clean across the whole module.

Changes

File Change
go.mod, go.sum Bump toolhive-core to v0.0.42
pkg/auth/awssts/role_mapper.go, pkg/authserver/server/tokenexchange/multi_issuer_validator.go Migrate CEL EnvOption import to cel.dev/cel-go/cel (mechanical, required by the bump)
pkg/skills/options.go Add InstallOptions.CatalogProvenance (internal-only)
pkg/skills/skillsvc/registry.go Carry the catalog entry's Provenance through registryResolveResult
pkg/skills/skillsvc/install.go Convert and attach it in installFromResolvedRegistry
pkg/skills/skillsvc/verify.go provenanceInfoFromCatalog conversion (fail-closed on attestation); fall back to it in verifyOCIInstall/verifyGitInstall only on true first use
docs/arch/12-skills-system.md Document the catalog-provenance fallback
docs/server/* Regenerated via task docstoolhive-core's Skill type (exposed through the registry API) gained the provenance field along with the version bump

Does this introduce a user-facing change?

Yes, two ways:

  • A skill installed by name (not a direct OCI/git reference) whose catalog
    entry declares an expected provenance is now checked against it on
    first install. Previously any correctly-signed artifact was accepted and
    its observed identity recorded unconditionally. No change for entries
    with no declared provenance (the common case today), and no change to
    any already-locked skill.
  • The registry API's Skill schema (GET /registry/{name}/v0.1/x/dev.toolhive/skills
    and friends) gains an optional provenance field, inherited directly
    from the toolhive-core bump — additive and backward compatible.

Special notes for reviewers

Three commits: the dependency bump + CEL fix, the feature, then a
task docs regeneration I initially missed (CI's Swagger check caught it —
bumping toolhive-core alone was enough to change the registry API's
generated schema, since Skill gained the field independent of anything
this PR's own code touches). The CEL fix is unrelated to this issue but
unavoidable — any PR bumping to toolhive-core v0.0.42 breaks the build
without it, so it travels with the bump rather than as a separate PR.

@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Aug 24, 2026
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.09524% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.77%. Comparing base (dd0ea20) to head (d4edb31).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/skills/verifier/oci.go 52.94% 8 Missing ⚠️
pkg/skills/skillsvc/verify.go 93.75% 4 Missing ⚠️
pkg/skills/verifier/git.go 0.00% 2 Missing ⚠️
pkg/skills/verifier/offline.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6420      +/-   ##
==========================================
+ Coverage   77.71%   77.77%   +0.05%     
==========================================
  Files         759      759              
  Lines       72889    72979      +90     
==========================================
+ Hits        56646    56756     +110     
+ Misses      16238    16218      -20     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed size/L Large PR: 600-999 lines changed labels Aug 24, 2026
@samuv samuv self-assigned this Aug 24, 2026
rdimitrov
rdimitrov previously approved these changes Aug 24, 2026
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/L Large PR: 600-999 lines changed size/XL Extra large PR: 1000+ lines changed labels Aug 24, 2026
samuv added 6 commits August 24, 2026 20:18
Needed for the Skill.Provenance field (#6310 prerequisite) landing
in the next commit. v0.0.42 also migrated toolhive-core's cel
package to the renamed cel.dev/cel-go module; update the two
callers that pass it EnvOptions to match, or the build breaks.

Signed-off-by: Samuele Verzi <samu@stacklok.com>
Trust-on-first-use had nothing to verify a skill's very first
install against — the observed signer identity was simply recorded,
never checked. When a skill resolves through the registry/catalog
and that entry declares an expected provenance, use it as the
first-install expectation instead of blind trust; a lock entry,
once one exists, still always wins.

A catalog-declared attestation constraint is refused rather than
silently skipped: the skill verifier has no attestation-comparison
logic today, so honoring identity while ignoring attestation would
let a catalog author believe a guarantee is enforced when it isn't.

Closes #6310

Signed-off-by: Samuele Verzi <samu@stacklok.com>
toolhive-core's Skill type gained a Provenance field in the v0.0.42
bump, and it's exposed through the registry API — task docs was
missed for this in the prior commit.

Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
@samuv
samuv force-pushed the skills-catalog-provenance branch from e1c5ca1 to d4edb31 Compare August 24, 2026 18:18
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Aug 24, 2026
@samuv
samuv merged commit 69e922b into main Aug 24, 2026
48 checks passed
@samuv
samuv deleted the skills-catalog-provenance branch August 24, 2026 19:57
samuv added a commit that referenced this pull request Aug 25, 2026
The verifier interface gained a ProvenanceExpectation wrapper on main
(#6420) so it can tell a strict lock pin from the independently-optional
catalog constraints. The rebase merged textually clean but stopped
compiling; plugins only ever present a lock expectation today, and
NewLockExpectation(nil) is nil, so the trust-on-first-use case is
unchanged.

Part of #6300.

Signed-off-by: Samuele Verzi <samu@stacklok.com>
samuv added a commit that referenced this pull request Aug 25, 2026
The verifier interface gained a ProvenanceExpectation wrapper on main
(#6420) so it can tell a strict lock pin from the independently-optional
catalog constraints. The rebase merged textually clean but stopped
compiling; plugins only ever present a lock expectation today, and
NewLockExpectation(nil) is nil, so the trust-on-first-use case is
unchanged.

Part of #6300.

Signed-off-by: Samuele Verzi <samu@stacklok.com>
samuv added a commit that referenced this pull request Aug 26, 2026
* Verify plugin signatures at install time

Project-scoped plugin installs now verify artifact signatures before
anything is extracted or recorded (RFC THV-0080): OCI artifacts through
the Sigstore keyless flow, git commits through gitsign verification,
both against the identity recorded in the project's lock file. On first
use the observed identity is recorded (trust on first use); later
installs enforce it inside the verifier, which plugins reuse from
pkg/skills/verifier so the pinned ref/runner checks come along too.
Verification runs under the per-plugin mutex so concurrent first
installs cannot race their TOFU anchors, and is scoped to installs that
record lock state — including the plugins lock feature gate, since a
disabled lock file has nowhere to anchor trust.

Unsigned artifacts are rejected unless the caller sets allow_unsigned,
which records an explicit "unsigned: true" exception in the lock entry;
an entry locked to a signer identity refuses unsigned or local-build
replacements outright. Lock-driven operations (sync restores, upgrade
re-pins) honor the trust state the entry already records — a lock diff
converting provenance to unsigned is therefore a reviewable trust
downgrade, called out in the code. Unlike skills, a local-store upgrade
deliberately clears resolvedReference, so ExpectedCanonicalName joins
the lock-driven markers. Verified installs persist the Sigstore bundle
with the DB record for offline re-verification during sync.

The unsigned exception reaches the service from every surface: the CLI
flag, the HTTP client DTO (without which the flag would silently never
reach the server — pinned by a round-trip test), and the API request
type. Failures classify to typed reasons via errors.Is on the
verifier's sentinels.

Part of #6300.

Signed-off-by: Samuele Verzi <samu@stacklok.com>

* Adapt plugin verify to ProvenanceExpectation

The verifier interface gained a ProvenanceExpectation wrapper on main
(#6420) so it can tell a strict lock pin from the independently-optional
catalog constraints. The rebase merged textually clean but stopped
compiling; plugins only ever present a lock expectation today, and
NewLockExpectation(nil) is nil, so the trust-on-first-use case is
unchanged.

Part of #6300.

Signed-off-by: Samuele Verzi <samu@stacklok.com>

---------

Signed-off-by: Samuele Verzi <samu@stacklok.com>
@github-actions github-actions Bot mentioned this pull request Aug 26, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Catalog-supplied expected identity, so first install is not trust-on-first-use

2 participants