From 9934ba9de67b519aad80ec3cac66c2fb8225f53a Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 14:04:39 +0200 Subject: [PATCH 1/2] feat(registry/types): add signer-identity provenance to Skill 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) Signed-off-by: Samuele Verzi --- registry/types/data/skill.schema.json | 68 ++++++ registry/types/skills_types.go | 5 + registry/types/skills_types_test.go | 298 ++++++++++++++++++++++++++ 3 files changed, 371 insertions(+) create mode 100644 registry/types/skills_types_test.go diff --git a/registry/types/data/skill.schema.json b/registry/types/data/skill.schema.json index 2550e7e..0cff246 100644 --- a/registry/types/data/skill.schema.json +++ b/registry/types/data/skill.schema.json @@ -85,6 +85,9 @@ "$ref": "#/$defs/skill_package" } }, + "provenance": { + "$ref": "#/$defs/provenance" + }, "metadata": { "type": "object", "description": "Official metadata from the SKILL.md file", @@ -168,6 +171,71 @@ } } }, + "provenance": { + "type": "object", + "description": "Expected signer identity for this skill, checked on first install instead of trust-on-first-use. Every field is optional: an absent or empty value leaves that dimension unconstrained. Mirrors the server provenance definition in publisher-provided.schema.json, but without 'format' keywords, because the Go Provenance struct serializes unset fields as empty strings and an empty string does not satisfy 'uri' or 'hostname'.", + "properties": { + "sigstore_url": { + "type": "string", + "description": "Sigstore TUF repository host for provenance verification", + "examples": [ + "tuf-repo.github.com", + "tuf-repo-cdn.sigstore.dev" + ] + }, + "repository_uri": { + "type": "string", + "description": "Repository URI used for provenance verification" + }, + "repository_ref": { + "type": "string", + "description": "Repository reference used for provenance verification" + }, + "signer_identity": { + "type": "string", + "description": "Identity of the signer for provenance verification", + "examples": [ + "/.github/workflows/build-skills.yml" + ] + }, + "runner_environment": { + "type": "string", + "description": "Build environment where the skill was built", + "examples": [ + "github-hosted", + "gitlab-hosted", + "self-hosted" + ] + }, + "cert_issuer": { + "type": "string", + "description": "Certificate issuer for provenance verification", + "examples": [ + "https://token.actions.githubusercontent.com" + ] + }, + "attestation": { + "$ref": "#/$defs/verified_attestation" + } + } + }, + "verified_attestation": { + "type": "object", + "description": "Verified attestation information", + "properties": { + "predicate_type": { + "type": "string", + "description": "Type of the attestation predicate", + "examples": [ + "https://slsa.dev/provenance/v0.2", + "https://slsa.dev/provenance/v1" + ] + }, + "predicate": { + "description": "Attestation predicate data" + } + } + }, "skill_repository": { "type": "object", "description": "Source repository metadata", diff --git a/registry/types/skills_types.go b/registry/types/skills_types.go index bf18994..c2d01b2 100644 --- a/registry/types/skills_types.go +++ b/registry/types/skills_types.go @@ -77,6 +77,11 @@ type Skill struct { Icons []SkillIcon `json:"icons,omitempty"` // Packages is the list of packages for the skill. Packages []SkillPackage `json:"packages,omitempty"` + // Provenance is the expected signer identity for this skill, checked on + // first install instead of trust-on-first-use. Absent means unconstrained + // — most catalog entries won't have this for a while, and that must not + // break installs; it's an opt-in tightening per entry, not a requirement. + Provenance *Provenance `json:"provenance,omitempty" yaml:"provenance,omitempty"` // Metadata is the official metadata of the skill as reported in the // SKILL.md file. Metadata map[string]any `json:"metadata,omitempty"` diff --git a/registry/types/skills_types_test.go b/registry/types/skills_types_test.go new file mode 100644 index 0000000..cf35bc0 --- /dev/null +++ b/registry/types/skills_types_test.go @@ -0,0 +1,298 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package registry + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +const ( + testSkillName = "commit" + testSigstoreURL = "tuf-repo.github.com" + testRepositoryURI = "https://github.com/stacklok/dockyard" + testRepositoryRef = "refs/heads/main" + testSignerIdentity = "/.github/workflows/build-skills.yml" + testRunnerEnv = "github-hosted" + testCertIssuer = "https://token.actions.githubusercontent.com" + testSkillIdentifier = "ghcr.io/stacklok/dockyard/skills/commit:1.0.0" + + testCaseFullProvenance = "fully populated provenance" +) + +// fullProvenance returns a Provenance with every field populated, matching the +// shape a catalog entry would declare for a skill built by a known workflow. +func fullProvenance() *Provenance { + return &Provenance{ + SigstoreURL: testSigstoreURL, + RepositoryURI: testRepositoryURI, + RepositoryRef: testRepositoryRef, + SignerIdentity: testSignerIdentity, + RunnerEnvironment: testRunnerEnv, + CertIssuer: testCertIssuer, + } +} + +// baseSkill returns a minimally valid Skill that individual tests can extend. +func baseSkill() *Skill { + return &Skill{ + Namespace: testNamespace, + Name: testSkillName, + Description: testLongDesc, + Version: testVersion, + Status: testStatusActive, + Packages: []SkillPackage{ + { + RegistryType: testRegistryType, + Identifier: testSkillIdentifier, + }, + }, + } +} + +// TestSkill_ProvenanceJSONRoundTrip verifies that Skill.Provenance survives a +// JSON round trip intact, and that a nil Provenance is omitted entirely rather +// than serialized as an explicit null. +func TestSkill_ProvenanceJSONRoundTrip(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + provenance *Provenance + // wantKey is whether the "provenance" key should appear in the JSON. + wantKey bool + }{ + { + name: "provenance unset is omitted", + provenance: nil, + wantKey: false, + }, + { + name: testCaseFullProvenance, + provenance: fullProvenance(), + wantKey: true, + }, + { + name: "partially populated provenance", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + }, + wantKey: true, + }, + { + name: "provenance with attestation", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + Attestation: &VerifiedAttestation{ + PredicateType: "https://slsa.dev/provenance/v1", + Predicate: map[string]any{"buildType": "workflow"}, + }, + }, + wantKey: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + skill := baseSkill() + skill.Provenance = tc.provenance + + data, err := json.Marshal(skill) + require.NoError(t, err) + + // Assert on the raw shape so omitempty behavior is pinned down, + // not just the decoded value. + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + _, hasKey := raw["provenance"] + assert.Equal(t, tc.wantKey, hasKey, "unexpected presence of provenance key in %s", data) + + var decoded Skill + require.NoError(t, json.Unmarshal(data, &decoded)) + assert.Equal(t, tc.provenance, decoded.Provenance) + assert.Equal(t, *skill, decoded) + }) + } +} + +// TestSkill_ProvenanceYAMLRoundTrip verifies the same omitempty and round-trip +// behavior for YAML, which the config-style consumers of the catalog use. +func TestSkill_ProvenanceYAMLRoundTrip(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + provenance *Provenance + wantKey bool + }{ + { + name: "provenance unset is omitted", + provenance: nil, + wantKey: false, + }, + { + name: testCaseFullProvenance, + provenance: fullProvenance(), + wantKey: true, + }, + { + name: "partially populated provenance", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + }, + wantKey: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + skill := baseSkill() + skill.Provenance = tc.provenance + + data, err := yaml.Marshal(skill) + require.NoError(t, err) + + var raw map[string]any + require.NoError(t, yaml.Unmarshal(data, &raw)) + _, hasKey := raw["provenance"] + assert.Equal(t, tc.wantKey, hasKey, "unexpected presence of provenance key in %s", data) + + var decoded Skill + require.NoError(t, yaml.Unmarshal(data, &decoded)) + assert.Equal(t, tc.provenance, decoded.Provenance) + }) + } +} + +// TestSkill_ValidateWithProvenance verifies that Validate accepts skills with +// provenance in every degree of population. Provenance is an opt-in tightening +// per catalog entry, so neither its absence nor a partially filled value may +// cause validation to fail. +func TestSkill_ValidateWithProvenance(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + provenance *Provenance + expectError bool + }{ + { + name: "no provenance", + provenance: nil, + }, + { + name: testCaseFullProvenance, + provenance: fullProvenance(), + }, + { + // The five core Provenance fields have no omitempty, so this + // serializes empty strings for everything but signer_identity. + // The schema must tolerate that. + name: "only signer identity", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + }, + }, + { + name: "empty provenance object", + provenance: &Provenance{}, + }, + { + name: "provenance with attestation", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + CertIssuer: testCertIssuer, + Attestation: &VerifiedAttestation{ + PredicateType: "https://slsa.dev/provenance/v1", + Predicate: map[string]any{"buildType": "workflow"}, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + skill := baseSkill() + skill.Provenance = tc.provenance + + err := skill.Validate() + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +// TestValidateSkillBytes_Provenance exercises the schema's provenance +// definition directly, including shapes the Go struct cannot express. +func TestValidateSkillBytes_Provenance(t *testing.T) { + t.Parallel() + + const skillPrefix = `{ + "namespace": "io.github.stacklok", + "name": "commit", + "description": "Extract text and tables from PDF files", + "version": "1.0.0"` + + tests := []struct { + name string + provenance string + expectError bool + }{ + { + name: "valid full provenance", + provenance: `{"sigstore_url":"tuf-repo.github.com","repository_uri":"https://github.com/stacklok/dockyard","repository_ref":"refs/heads/main","signer_identity":"/.github/workflows/build-skills.yml","runner_environment":"github-hosted","cert_issuer":"https://token.actions.githubusercontent.com"}`, //nolint:lll + }, + { + name: "valid partial provenance", + provenance: `{"signer_identity":"/.github/workflows/build-skills.yml"}`, + }, + { + name: "valid empty provenance object", + provenance: `{}`, + }, + { + name: "provenance must be an object", + provenance: `"not-an-object"`, + expectError: true, + }, + { + name: "signer_identity must be a string", + provenance: `{"signer_identity":42}`, + expectError: true, + }, + { + name: "attestation must be an object", + provenance: `{"attestation":"nope"}`, + expectError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + data := []byte(skillPrefix + `,"provenance":` + tc.provenance + "}") + err := ValidateSkillBytes(data) + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From 7dff469c76c0ecf15fb654bba1697df9fa474235 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 14:15:35 +0200 Subject: [PATCH 2/2] fix(registry/types): refuse attestation constraints on skill provenance 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) Signed-off-by: Samuele Verzi --- registry/types/data/skill.schema.json | 23 ++--------------- registry/types/skills_types.go | 6 +++++ registry/types/skills_types_test.go | 37 ++++++++++++++++++++++++--- 3 files changed, 42 insertions(+), 24 deletions(-) diff --git a/registry/types/data/skill.schema.json b/registry/types/data/skill.schema.json index 0cff246..b70a117 100644 --- a/registry/types/data/skill.schema.json +++ b/registry/types/data/skill.schema.json @@ -173,7 +173,8 @@ }, "provenance": { "type": "object", - "description": "Expected signer identity for this skill, checked on first install instead of trust-on-first-use. Every field is optional: an absent or empty value leaves that dimension unconstrained. Mirrors the server provenance definition in publisher-provided.schema.json, but without 'format' keywords, because the Go Provenance struct serializes unset fields as empty strings and an empty string does not satisfy 'uri' or 'hostname'.", + "description": "Expected signer identity for this skill, checked on first install instead of trust-on-first-use. Every field is optional: an absent or empty value leaves that dimension unconstrained. Mirrors the server provenance definition in publisher-provided.schema.json, with two deliberate differences. First, no 'format' keywords, because the Go Provenance struct serializes unset fields as empty strings and an empty string does not satisfy 'uri' or 'hostname'. Second, 'attestation' is not accepted and 'additionalProperties' is false: the verifier only compares an expected attestation when the artifact also carries one, so a declared attestation constraint would be silently skipped against a signature that has none. Rejecting the key keeps the contract fail-closed -- a constraint this path cannot enforce is refused loudly rather than ignored. The same strictness makes a misspelled constraint key an error instead of a silently dropped guarantee.", + "additionalProperties": false, "properties": { "sigstore_url": { "type": "string", @@ -213,26 +214,6 @@ "examples": [ "https://token.actions.githubusercontent.com" ] - }, - "attestation": { - "$ref": "#/$defs/verified_attestation" - } - } - }, - "verified_attestation": { - "type": "object", - "description": "Verified attestation information", - "properties": { - "predicate_type": { - "type": "string", - "description": "Type of the attestation predicate", - "examples": [ - "https://slsa.dev/provenance/v0.2", - "https://slsa.dev/provenance/v1" - ] - }, - "predicate": { - "description": "Attestation predicate data" } } }, diff --git a/registry/types/skills_types.go b/registry/types/skills_types.go index c2d01b2..5685aa8 100644 --- a/registry/types/skills_types.go +++ b/registry/types/skills_types.go @@ -81,6 +81,12 @@ type Skill struct { // first install instead of trust-on-first-use. Absent means unconstrained // — most catalog entries won't have this for a while, and that must not // break installs; it's an opt-in tightening per entry, not a requirement. + // + // Only the identity fields are honored. Setting Provenance.Attestation + // makes Validate fail: the verifier compares an expected attestation only + // when the artifact also carries one, so the constraint would be silently + // skipped against a signature that has none. Refusing it loudly keeps the + // contract fail-closed until that gap is fixed. Provenance *Provenance `json:"provenance,omitempty" yaml:"provenance,omitempty"` // Metadata is the official metadata of the skill as reported in the // SKILL.md file. diff --git a/registry/types/skills_types_test.go b/registry/types/skills_types_test.go index cf35bc0..a8f0684 100644 --- a/registry/types/skills_types_test.go +++ b/registry/types/skills_types_test.go @@ -86,6 +86,9 @@ func TestSkill_ProvenanceJSONRoundTrip(t *testing.T) { wantKey: true, }, { + // The shared Provenance type carries Attestation, so it still + // serializes here. Validate rejects it; see + // TestSkill_ValidateWithProvenance. name: "provenance with attestation", provenance: &Provenance{ SignerIdentity: testSignerIdentity, @@ -208,7 +211,11 @@ func TestSkill_ValidateWithProvenance(t *testing.T) { provenance: &Provenance{}, }, { - name: "provenance with attestation", + // The verifier only compares an expected attestation when the + // artifact also carries one, so this constraint would be silently + // skipped against a signature with no attestation. Reject it + // rather than let a catalog author believe it is enforced. + name: "attestation is refused, not silently ignored", provenance: &Provenance{ SignerIdentity: testSignerIdentity, CertIssuer: testCertIssuer, @@ -217,6 +224,15 @@ func TestSkill_ValidateWithProvenance(t *testing.T) { Predicate: map[string]any{"buildType": "workflow"}, }, }, + expectError: true, + }, + { + name: "empty attestation is still refused", + provenance: &Provenance{ + SignerIdentity: testSignerIdentity, + Attestation: &VerifiedAttestation{}, + }, + expectError: true, }, } @@ -276,8 +292,23 @@ func TestValidateSkillBytes_Provenance(t *testing.T) { expectError: true, }, { - name: "attestation must be an object", - provenance: `{"attestation":"nope"}`, + // Not "must be an object" -- the key is refused outright, because + // the verifier cannot enforce an attestation constraint against a + // signature that carries none. + name: "attestation key is rejected", + provenance: `{"attestation":{"predicate_type":"https://slsa.dev/provenance/v1"}}`, + expectError: true, + }, + { + // additionalProperties:false turns a typo into a loud failure + // instead of a silently dropped security constraint. + name: "misspelled constraint key is rejected", + provenance: `{"signer_identityy":"/.github/workflows/build-skills.yml"}`, + expectError: true, + }, + { + name: "unknown constraint key is rejected", + provenance: `{"expected_sbom":"whatever"}`, expectError: true, }, }