feat(registry/types): add signer-identity provenance to Skill - #247
Merged
Merged
Conversation
Adds an optional `Provenance` field to `registry.Skill`, so a skills catalog can declare who is expected to sign a skill. This is the prerequisite for checking a skill's signer identity on first install instead of trusting on first use (stacklok/toolhive#6310). Reuses the existing in-package `registry.Provenance` type rather than defining a parallel struct for skills. It already carries the same six semantic fields that toolhive's `lockfile.Provenance` needs, including `RepositoryRef` and `RunnerEnvironment`. The field is added directly to `Skill` (mirroring `ImageMetadata`) since `Skill` is the format `toolhive-catalog` serializes as `skill.json` directly, with no raw/converted split like MCP servers have. Absent provenance means unconstrained: most catalog entries won't have this for a while, and that must not break installs. It is an opt-in tightening per entry, not a requirement. The skill schema's `provenance` definition mirrors the server one in publisher-provided.schema.json, but deliberately omits its `format` keywords. `Provenance`'s five core fields have no `omitempty`, so a partially populated provenance serializes empty strings, and an empty string satisfies neither `uri` nor `hostname`. Keeping the formats would make a signer-identity-only entry -- the realistic catalog case -- fail validation. A local `$defs` is used because publisher-provided.schema.json is not in `referencedSchemas`, so a cross-file `$ref` would not resolve offline. `container/verifier.Sigstore.VerifyServer` is untouched: it is already generic over an OCI ref and a `*Provenance`, and works for skills as-is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
Review flagged that the new skill schema advertised `attestation` as an
enforceable constraint while the verifier can silently skip it:
if p.Attestation != nil && r.Statement != nil &&
p.Attestation.Predicate != nil && r.Statement.Predicate != nil {
When an expected attestation is set but the artifact carries none,
`r.Statement` is nil, the comparison is skipped, and the function falls
through to `return true`. A skill declaring an attestation would verify
successfully against a signature that has no attestation at all -- a
fail-open on a security constraint.
Rather than propagate that unsound contract to a new surface, the skill
provenance definition now rejects `attestation` outright. Deleting the
property alone would not have been enough: `additionalProperties` was
unset, so the key would still have validated and been silently dropped.
Setting `additionalProperties: false` makes it a loud error, and has the
side benefit of turning a misspelled constraint key into a failure rather
than a quietly missing guarantee:
skill schema validation failed: provenance: Additional property
attestation is not allowed
The Go field comment records the same limitation so it is discoverable
from the type, not only from a schema error.
The underlying verifier gap is deliberately left alone here. It predates
this change, it affects MCP servers today through the identical
`attestation` key in publisher-provided.schema.json, and closing it would
change verification outcomes for existing server catalog entries. That is
a cross-consumer behavior change that belongs in its own PR, with the
"expected attestation, actual attestation absent" regression test.
Nothing calls VerifyServer with a skill Provenance yet, so no skill path
is currently exposed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
rdimitrov
approved these changes
Aug 21, 2026
samuv
added a commit
that referenced
this pull request
Aug 21, 2026
…ce (#251) * feat(registry/types): allow attestation constraints on skill provenance #247 stripped `attestation` from the skill provenance contract because the verifier could silently skip it: an expected attestation was only compared when the artifact also carried one, so the constraint was dropped exactly when it mattered. #248 closed that gap -- `compareAttestation` now fails when the provenance asks for an attestation and the statement is absent. That removed the reason for the restriction but not the restriction, and left two comments on main asserting the opposite of what the code does. Both the schema description and the `Skill.Provenance` doc comment still claimed a declared attestation "would be silently skipped", and the Go comment's "until that gap is fixed" read as permanent. Re-add the key and correct both. The skill `verified_attestation` definition is deliberately stricter than the server one in publisher-provided.schema.json: `predicate` is typed as an object. The verifier normalizes an expected predicate through `structpb.NewStruct`, which requires `map[string]any`: string(str) -> err=expected predicate is string, want an object int(42) -> err=expected predicate is int, want an object []interface{}([a]) -> err=expected predicate is []interface {}, want an object map[string]any{k:v} -> ok A non-object predicate therefore mismatches every artifact. Untyped, the schema would accept a constraint that can never be satisfied and the failure would surface at install time as an unexplained verification failure. Typing it makes that a validation error at authoring time. `additionalProperties: false` is kept on both definitions -- it was added for typo protection rather than for the attestation restriction, and it now also catches a misspelled `predicatetype`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com> * docs(registry/types): clarify that an empty Attestation still constrains Review nit: "an empty one is not checked" contradicted the next sentence, since an empty Attestation is deliberately checked and requires the artifact to be attested. Scope the rule to empty strings and call out Attestation as the exception, noting that its own PredicateType and Predicate go back to the usual constrain-only-when-set behaviour. The schema's provenance description carried the same contradiction ("an absent or empty value leaves that dimension unconstrained", which is false for `attestation: {}`), so fix it there too. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com> --------- Signed-off-by: Samuele Verzi <samu@stacklok.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds an optional
Provenancefield toregistry.Skill, so a skills catalog can declare who is expected to sign a skill.This is the prerequisite for stacklok/toolhive#6310 ("Catalog-supplied expected identity, so first install is not trust-on-first-use").
This PR is
toolhive-coreonly. It does not wire anything up intoolhive— that's a follow-up PR there, once this ships in a release andtoolhive'sgo.modbumps to it. Same pattern as #234/#235 →toolhive'sskills-keyless/01-core-signer(stacklok/toolhive#6383).Design notes
Reuses the existing
registry.Provenance(registry/types/registry_types.go) rather than defining a parallel struct for skills. It already carries the same six semantic fields skills need, includingRepositoryRefandRunnerEnvironment— the two thattoolhive'slockfile.Provenanceadded for skills in its stacklok/toolhive#6309 follow-up. Two parallel structs for the same concept just drift.Added directly to
Skill, mirroringImageMetadata.Provenance. Servers keep provenance indirect for MCP-registry-format reasons (raw upstreamServerJSONnests it in a_metapublisher-extension blob, converted byServerJSONToImageMetadata), butSkillhas no such raw/converted split — it is the formattoolhive-catalogserializes asskill.json, field-for-field.container/verifier.Sigstore.VerifyServeris untouched. It's already generic over an OCI image ref and a*Provenance— nothing in its body or incompareBasePropertiesis server-specific — so it should work for skills unmodified. A rename to something generic (Verify, withVerifyServeras a deprecated alias) is tempting now that it's about to gain a non-server caller, butCLAUDE.md's "never change exported interface signatures without discussion" applies. Happy to open that as a separate issue if people want it; blast radius is small (3 non-test call sites).One deliberate divergence from the server schema
The skill schema's
provenance$defmirrors the server one inpublisher-provided.schema.json— same fields, descriptions, and examples — but omits itsformatkeywords (urionrepository_uri/cert_issuer,hostnameonsigstore_url).Provenance's five core fields have noomitempty, so a partially populated provenance marshals"repository_uri": "". gojsonschema rejects that:Keeping the formats would mean setting only
signer_identity— the realistic catalog case, and exactly the opt-in shape this field is for — failsValidate(). The reason is recorded in the$def's own description, and there are regression tests pinning it (only signer identity,empty provenance object).The alternative, adding
omitemptytoProvenance's json tags, would change serialization for MCP servers too, so I left it alone.A local
$defs/provenanceis used rather than a cross-file$ref, becausepublisher-provided.schema.jsonisn't inschema_validation.go'sreferencedSchemaspreload list and the ref wouldn't resolve offline.Tests
New
registry/types/skills_types_test.go, all table-driven:Provenanceomitted (asserted on the raw map, not just the decoded value), full / partial / with-attestation preserved.Validate()with provenance — absent, full, signer-identity-only, empty object, with attestation. None may fail.ValidateSkillBytes— raw JSON shapes the Go struct can't express (provenancenot an object,signer_identitynot a string,attestationnot an object) correctly rejected.Verification
task— green, 0 lint issues, all packages pass.task license-check— passes.registry/typescoverage: 81.1% (repo requires ≥70%).TestSkill_Validatepasses unmodified — this change is purely additive.No live Sigstore/network verification here; actually using
VerifyServeragainst a real signed skill happens in the follow-uptoolhivePR.Out of scope
toolhive— thepkg/skills/verifier/skillsvcwiring that passes a catalog-suppliedProvenanceinstead ofnil. That's Catalog-supplied expected identity, so first install is not trust-on-first-use toolhive#6310's remaining work, blocked on this merging + releasing + a go.mod bump.toolhive-catalog— populatingprovenancefor real entries (e.g.dockyard's/.github/workflows/build-skills.yml). Separate repo, only possible once this exists in a released version.dockyard— doesn't consume this verification path at all; signs independently with stockcosign.🤖 Generated with Claude Code