From a5f4d607a39fa07dcab5910228c68952d0dc0ab6 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 15:08:44 +0200 Subject: [PATCH 1/2] 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) Signed-off-by: Samuele Verzi --- registry/types/data/skill.schema.json | 25 ++++++++- registry/types/skills_types.go | 11 ++-- registry/types/skills_types_test.go | 78 +++++++++++++++++++-------- 3 files changed, 87 insertions(+), 27 deletions(-) diff --git a/registry/types/data/skill.schema.json b/registry/types/data/skill.schema.json index b70a117..390aeac 100644 --- a/registry/types/data/skill.schema.json +++ b/registry/types/data/skill.schema.json @@ -173,7 +173,7 @@ }, "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, 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.", + "description": "Expected provenance 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, 'additionalProperties' is false, so a misspelled constraint key is a loud error rather than a silently dropped guarantee.", "additionalProperties": false, "properties": { "sigstore_url": { @@ -214,6 +214,29 @@ "examples": [ "https://token.actions.githubusercontent.com" ] + }, + "attestation": { + "$ref": "#/$defs/verified_attestation" + } + } + }, + "verified_attestation": { + "type": "object", + "description": "Expected in-toto attestation. Declaring this key at all requires the artifact to carry an attestation: verification fails against a signature that has none. Both members are optional and constrain independently, so an empty object means 'must be attested, no constraint on what the attestation says'.", + "additionalProperties": false, + "properties": { + "predicate_type": { + "type": "string", + "description": "Expected predicate type. When set, it must equal the predicate type of the statement carried by the signature.", + "examples": [ + "https://slsa.dev/provenance/v0.2", + "https://slsa.dev/provenance/v1" + ] + }, + "predicate": { + "type": "object", + "description": "Expected predicate body, compared for deep equality against the statement predicate. Constrained to an object because an in-toto predicate is always one: the verifier normalizes the expected value through structpb before comparing, and a non-object expectation fails that step, so it could never match any artifact.", + "additionalProperties": true } } }, diff --git a/registry/types/skills_types.go b/registry/types/skills_types.go index 5685aa8..35c02e2 100644 --- a/registry/types/skills_types.go +++ b/registry/types/skills_types.go @@ -82,11 +82,12 @@ type Skill struct { // — 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. + // Each field constrains independently: an empty one is not checked. Setting + // Attestation requires the artifact to be attested at all, so verification + // fails against a signature carrying no statement. Attestation.Predicate + // must be a JSON object; anything else can never match, and Validate + // rejects it rather than letting it through as a constraint that silently + // fails every artifact. 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 a8f0684..83d026f 100644 --- a/registry/types/skills_types_test.go +++ b/registry/types/skills_types_test.go @@ -23,6 +23,7 @@ const ( testSkillIdentifier = "ghcr.io/stacklok/dockyard/skills/commit:1.0.0" testCaseFullProvenance = "fully populated provenance" + testPredicateType = "https://slsa.dev/provenance/v1" ) // fullProvenance returns a Provenance with every field populated, matching the @@ -86,14 +87,11 @@ 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, Attestation: &VerifiedAttestation{ - PredicateType: "https://slsa.dev/provenance/v1", + PredicateType: testPredicateType, Predicate: map[string]any{"buildType": "workflow"}, }, }, @@ -177,10 +175,12 @@ func TestSkill_ProvenanceYAMLRoundTrip(t *testing.T) { } } -// 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. +// TestSkill_ValidateWithProvenance verifies that Validate accepts provenance in +// every degree of population, since it is an opt-in tightening per catalog +// entry and neither its absence nor a partially filled value may fail +// validation. The exception is a constraint the verifier could never satisfy: +// a non-object attestation predicate is rejected here rather than shipped as an +// entry that fails against every artifact. func TestSkill_ValidateWithProvenance(t *testing.T) { t.Parallel() @@ -211,27 +211,48 @@ func TestSkill_ValidateWithProvenance(t *testing.T) { provenance: &Provenance{}, }, { - // 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", + name: "fully specified attestation", provenance: &Provenance{ SignerIdentity: testSignerIdentity, CertIssuer: testCertIssuer, Attestation: &VerifiedAttestation{ - PredicateType: "https://slsa.dev/provenance/v1", + PredicateType: testPredicateType, Predicate: map[string]any{"buildType": "workflow"}, }, }, - expectError: true, }, { - name: "empty attestation is still refused", + // Means "must be attested, no constraint on what the attestation + // says" -- the verifier still requires the artifact to carry a + // statement. + name: "empty attestation constrains only presence", provenance: &Provenance{ SignerIdentity: testSignerIdentity, Attestation: &VerifiedAttestation{}, }, + }, + { + name: "predicate type without predicate", + provenance: &Provenance{ + Attestation: &VerifiedAttestation{PredicateType: testPredicateType}, + }, + }, + { + // normalizeAttestationPredicate requires map[string]any, so a + // scalar predicate can never match any artifact. Reject it at + // authoring time instead of shipping a constraint that always + // fails. + name: "scalar predicate is rejected", + provenance: &Provenance{ + Attestation: &VerifiedAttestation{Predicate: "not-an-object"}, + }, + expectError: true, + }, + { + name: "array predicate is rejected", + provenance: &Provenance{ + Attestation: &VerifiedAttestation{Predicate: []any{"a", "b"}}, + }, expectError: true, }, } @@ -292,11 +313,26 @@ func TestValidateSkillBytes_Provenance(t *testing.T) { expectError: true, }, { - // 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"}}`, + name: "valid attestation constraint", + provenance: `{"attestation":{"predicate_type":"https://slsa.dev/provenance/v1","predicate":{"buildType":"workflow"}}}`, + }, + { + name: "valid empty attestation constraint", + provenance: `{"attestation":{}}`, + }, + { + name: "attestation must be an object", + provenance: `{"attestation":"nope"}`, + expectError: true, + }, + { + name: "attestation predicate must be an object", + provenance: `{"attestation":{"predicate":"not-an-object"}}`, + expectError: true, + }, + { + name: "misspelled attestation key is rejected", + provenance: `{"attestation":{"predicatetype":"x"}}`, expectError: true, }, { From 8476e9ddb71701dbeef3968ea2cdc54fc739d11c Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 15:15:09 +0200 Subject: [PATCH 2/2] 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) Signed-off-by: Samuele Verzi --- registry/types/data/skill.schema.json | 2 +- registry/types/skills_types.go | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/registry/types/data/skill.schema.json b/registry/types/data/skill.schema.json index 390aeac..251798e 100644 --- a/registry/types/data/skill.schema.json +++ b/registry/types/data/skill.schema.json @@ -173,7 +173,7 @@ }, "provenance": { "type": "object", - "description": "Expected provenance 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, 'additionalProperties' is false, so a misspelled constraint key is a loud error rather than a silently dropped guarantee.", + "description": "Expected provenance for this skill, checked on first install instead of trust-on-first-use. Every field is optional, and an empty string leaves that dimension unconstrained. The exception is 'attestation', where the key's presence is itself the constraint: see its own description. 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, 'additionalProperties' is false, so a misspelled constraint key is a loud error rather than a silently dropped guarantee.", "additionalProperties": false, "properties": { "sigstore_url": { diff --git a/registry/types/skills_types.go b/registry/types/skills_types.go index 35c02e2..f472513 100644 --- a/registry/types/skills_types.go +++ b/registry/types/skills_types.go @@ -82,12 +82,14 @@ type Skill struct { // — 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. // - // Each field constrains independently: an empty one is not checked. Setting - // Attestation requires the artifact to be attested at all, so verification - // fails against a signature carrying no statement. Attestation.Predicate - // must be a JSON object; anything else can never match, and Validate - // rejects it rather than letting it through as a constraint that silently - // fails every artifact. + // Each field constrains independently, and an empty string leaves that + // dimension unconstrained. Attestation is the exception: setting it at + // all, even to an empty struct, requires the artifact to be attested, so + // verification fails against a signature carrying no statement. Its own + // PredicateType and Predicate then follow the usual rule and constrain + // only when set. Predicate must be a JSON object; anything else can never + // match, and Validate rejects it rather than letting it through as a + // constraint that silently fails every artifact. Provenance *Provenance `json:"provenance,omitempty" yaml:"provenance,omitempty"` // Metadata is the official metadata of the skill as reported in the // SKILL.md file.