From 580459fbe807940932cff4d4570f0ffcb58a7b5b Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 27 Jul 2026 15:23:13 +0200 Subject: [PATCH] Add provenance and unsigned fields to the skills lock schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First PR of the Sigstore stack (RFC THV-0080 Stack 2, tracked in #5899): the lock file gains a typed provenance block recording the verified signer identity — the trust-on-first-use anchor later installs, syncs, and upgrades are checked against — and an unsigned flag recording an explicit --allow-unsigned trust decision. The two are mutually exclusive and validated with the same syntactic constraints as resolvedReference (graphic characters, bounded length), since the lock file is hand-editable and these values feed the identity policy future verifications enforce. The fields graduate from the Extra inline round-trip map into typed schema under version 1, exactly the path the forward-compat design reserved for them; that test's fixture moves to a still-unknown field. InstallOptions gains the AllowUnsigned option and the internal ProvenanceInfo/SigstoreBundle carriers the verification step (a later PR) will populate. No behavior changes yet — nothing produces or consumes these fields until the verifier lands. --- pkg/skills/lockfile/lockfile.go | 30 +++++++- pkg/skills/lockfile/lockfile_test.go | 99 ++++++++++++++++++++++++-- pkg/skills/lockfile/validation.go | 46 ++++++++++++ pkg/skills/lockfile/validation_test.go | 63 ++++++++++++++++ pkg/skills/options.go | 30 ++++++++ 5 files changed, 261 insertions(+), 7 deletions(-) diff --git a/pkg/skills/lockfile/lockfile.go b/pkg/skills/lockfile/lockfile.go index 6709491b00..dca6b24214 100644 --- a/pkg/skills/lockfile/lockfile.go +++ b/pkg/skills/lockfile/lockfile.go @@ -60,14 +60,25 @@ type Entry struct { // ContentDigest is a deterministic SHA-256 dirhash of the materialized // skill file set, used for on-disk integrity verification. ContentDigest string `yaml:"contentDigest,omitempty"` + // Provenance records the verified signer identity of the installed + // artifact — the trust-on-first-use anchor later installs, syncs, and + // upgrades are checked against. Nil for entries recorded before + // verification existed and for unsigned entries (see Unsigned); an + // entry never carries both. + Provenance *Provenance `yaml:"provenance,omitempty"` + // Unsigned is true when the artifact carried no signature and the user + // explicitly accepted that with --allow-unsigned. It is a recorded + // trust decision, not an omission: replacing an unsigned entry with a + // signed one clears it, and a signed entry never becomes unsigned + // without the same explicit flag. + Unsigned bool `yaml:"unsigned,omitempty"` // RequiredBy lists parent skill names for transitively materialized // dependencies (skills declared via toolhive.requires). RequiredBy []string `yaml:"requiredBy,omitempty"` // Explicit is true when the user directly installed this skill; explicit // entries are exempt from cascade removal when RequiredBy becomes empty. Explicit bool `yaml:"explicit,omitempty"` - // Extra round-trips fields this binary does not know about (e.g. the - // Sigstore provenance fields a future schema adds under version 1), so a + // Extra round-trips fields this binary does not know about, so a // Load→modify→Save cycle by an older binary preserves rather than strips // them. It applies per entry: an entry this binary rewrites (reinstall, // upgrade) is built fresh, so its unknown fields — which described the @@ -75,6 +86,21 @@ type Entry struct { Extra map[string]any `yaml:",inline"` } +// Provenance is the Sigstore signer identity recorded for a verified entry. +type Provenance struct { + // SignerIdentity is the certificate subject identity: for GitHub + // Actions certificates, the workflow path relative to the repository; + // otherwise the certificate SAN verbatim (a URI, email, or SPIFFE ID). + SignerIdentity string `yaml:"signerIdentity"` + // CertIssuer is the OIDC issuer that authenticated the signer. + CertIssuer string `yaml:"certIssuer"` + // RepositoryURI is the source repository from the Fulcio certificate + // extensions, when present. + RepositoryURI string `yaml:"repositoryUri,omitempty"` + // SigstoreURL is the Sigstore instance the signature chains to. + SigstoreURL string `yaml:"sigstoreUrl,omitempty"` +} + // Lockfile is the parsed contents of a project's toolhive.lock.yaml. type Lockfile struct { // Version is the lock file schema version. diff --git a/pkg/skills/lockfile/lockfile_test.go b/pkg/skills/lockfile/lockfile_test.go index 7d270b6ab7..0f04c779f5 100644 --- a/pkg/skills/lockfile/lockfile_test.go +++ b/pkg/skills/lockfile/lockfile_test.go @@ -137,9 +137,9 @@ func TestUnknownFieldsSurviveLoadModifySave(t *testing.T) { " - name: signed-skill\n" + " source: ghcr.io/org/signed-skill:1.0.0\n" + " digest: " + ociDigest(1) + "\n" + - " provenance:\n" + - " signerIdentity: dev@example.com\n" + - " certIssuer: https://accounts.example.com\n" + " attestations:\n" + + " - predicateType: https://slsa.dev/provenance/v1\n" + + " digest: " + ociDigest(3) + "\n" require.NoError(t, os.WriteFile(path, []byte(futureLock), 0o644)) // Load, touch an unrelated entry, save — the classic older-binary write. @@ -152,14 +152,14 @@ func TestUnknownFieldsSurviveLoadModifySave(t *testing.T) { data, err := os.ReadFile(path) //nolint:gosec // fixed test path require.NoError(t, err) saved := string(data) - assert.Contains(t, saved, "signerIdentity: dev@example.com", "unknown entry fields must survive a Load->Save cycle") + assert.Contains(t, saved, "predicateType: https://slsa.dev/provenance/v1", "unknown entry fields must survive a Load->Save cycle") assert.Contains(t, saved, "futureTopLevelField: keep-me", "unknown top-level fields must survive a Load->Save cycle") loaded, err := Load(root) require.NoError(t, err) signed, ok := loaded.Get("signed-skill") require.True(t, ok) - assert.Contains(t, signed.Extra, "provenance") + assert.Contains(t, signed.Extra, "attestations") } func TestSaveRejectsInvalidLockfile(t *testing.T) { @@ -385,3 +385,92 @@ func TestConcurrentUpsertEntryDoesNotLoseUpdates(t *testing.T) { func skillNameForIndex(i int) string { return "skill-" + string(rune('a'+i/26)) + string(rune('a'+i%26)) } + +// TestProvenanceRoundTrip covers the Stack-2 schema: a verified entry's +// provenance block and an unsigned entry's exception flag both survive +// Save -> Load intact under schema version 1. +func TestProvenanceRoundTrip(t *testing.T) { + t.Parallel() + root := testRoot(t) + + lf := &Lockfile{Version: CurrentVersion} + signed := Entry{ + Name: "signed-skill", + Source: "ghcr.io/org/signed-skill", + ResolvedReference: "ghcr.io/org/signed-skill:latest", + Digest: ociDigest(1), + Provenance: &Provenance{ + SignerIdentity: "/.github/workflows/release.yml", + CertIssuer: "https://token.actions.githubusercontent.com", + RepositoryURI: "https://github.com/org/signed-skill", + SigstoreURL: "https://rekor.sigstore.dev", + }, + Explicit: true, + } + unsigned := Entry{ + Name: "unsigned-skill", + Source: "unsigned-skill", + Digest: ociDigest(2), + Unsigned: true, + Explicit: true, + } + lf.Upsert(signed) + lf.Upsert(unsigned) + require.NoError(t, lf.Save(root)) + + loaded, err := Load(root) + require.NoError(t, err) + + gotSigned, ok := loaded.Get("signed-skill") + require.True(t, ok) + assert.Equal(t, signed.Provenance, gotSigned.Provenance) + assert.False(t, gotSigned.Unsigned) + assert.Empty(t, gotSigned.Extra, "a typed provenance block must not leak into the inline Extra map") + + gotUnsigned, ok := loaded.Get("unsigned-skill") + require.True(t, ok) + assert.True(t, gotUnsigned.Unsigned) + assert.Nil(t, gotUnsigned.Provenance) +} + +// TestProvenanceGraduatesFromExtraMap: before this schema change, a +// provenance block written by a newer binary round-tripped through the +// Extra inline map. Now that the field is typed, loading such a file must +// parse it into Entry.Provenance — not duplicate it in Extra, which would +// make Save produce a colliding key. +func TestProvenanceGraduatesFromExtraMap(t *testing.T) { + t.Parallel() + root := testRoot(t) + path, err := root.Path() + require.NoError(t, err) + + handWritten := "" + + "version: 1\n" + + "skills:\n" + + " - name: signed-skill\n" + + " source: ghcr.io/org/signed-skill\n" + + " digest: " + ociDigest(1) + "\n" + + " provenance:\n" + + " signerIdentity: dev@example.com\n" + + " certIssuer: https://accounts.example.com\n" + + " explicit: true\n" + require.NoError(t, os.WriteFile(path, []byte(handWritten), 0o644)) + + loaded, err := Load(root) + require.NoError(t, err) + entry, ok := loaded.Get("signed-skill") + require.True(t, ok) + require.NotNil(t, entry.Provenance, "provenance must parse into the typed field") + assert.Equal(t, "dev@example.com", entry.Provenance.SignerIdentity) + assert.NotContains(t, entry.Extra, "provenance", "the typed field must not also appear in Extra") + + // A Save after modification must not produce duplicate keys. + require.NoError(t, UpsertEntry(root, Entry{ + Name: "other-skill", Source: "other-skill", Digest: ociDigest(2), + })) + reloaded, err := Load(root) + require.NoError(t, err) + entry, ok = reloaded.Get("signed-skill") + require.True(t, ok) + assert.Equal(t, "dev@example.com", entry.Provenance.SignerIdentity) +} diff --git a/pkg/skills/lockfile/validation.go b/pkg/skills/lockfile/validation.go index 3b4429e936..a66e3f46ec 100644 --- a/pkg/skills/lockfile/validation.go +++ b/pkg/skills/lockfile/validation.go @@ -142,6 +142,52 @@ func validateEntry(entry Entry) error { return fmt.Errorf("entry %q: resolvedReference: %w", entry.Name, err) } } + if entry.Provenance != nil && entry.Unsigned { + return fmt.Errorf("entry %q: provenance and unsigned are mutually exclusive"+ + " — an entry is either a verified signature or a recorded unsigned exception", entry.Name) + } + if entry.Provenance != nil { + if err := validateProvenance(entry.Provenance); err != nil { + return fmt.Errorf("entry %q: provenance: %w", entry.Name, err) + } + } + return nil +} + +// validateProvenance syntactically constrains a provenance block. Like +// resolvedReference, these values are hand-editable and feed the identity +// policy that future verifications are checked against, so they must be +// well-formed graphic strings of bounded length. Validation is purely +// syntactic — whether the identity is trustworthy is the verifier's job. +func validateProvenance(p *Provenance) error { + if p.SignerIdentity == "" { + return errors.New("signerIdentity is required") + } + if p.CertIssuer == "" { + return errors.New("certIssuer is required") + } + fields := map[string]string{ + "signerIdentity": p.SignerIdentity, + "certIssuer": p.CertIssuer, + "repositoryUri": p.RepositoryURI, + "sigstoreUrl": p.SigstoreURL, + } + for name, value := range fields { + if value == "" { + continue + } + if len(value) > maxReferenceLength { + return fmt.Errorf("%s exceeds %d characters", name, maxReferenceLength) + } + if strings.TrimSpace(value) != value { + return fmt.Errorf("%s has leading or trailing whitespace", name) + } + for _, r := range value { + if !unicode.IsGraphic(r) || unicode.IsSpace(r) { + return fmt.Errorf("%s contains non-graphic or whitespace character %q", name, r) + } + } + } return nil } diff --git a/pkg/skills/lockfile/validation_test.go b/pkg/skills/lockfile/validation_test.go index 548d111bfb..a54430d0db 100644 --- a/pkg/skills/lockfile/validation_test.go +++ b/pkg/skills/lockfile/validation_test.go @@ -183,6 +183,69 @@ func TestValidateLockfile(t *testing.T) { }}, wantErr: "requiredBy cycle", }, + { + name: "valid provenance", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "signed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + SignerIdentity: "dev@example.com", + CertIssuer: "https://accounts.example.com", + }}, + }}, + }, + { + name: "provenance and unsigned are mutually exclusive", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "conflicted", Source: "s", Digest: validSHA256Digest, Unsigned: true, Provenance: &Provenance{ + SignerIdentity: "dev@example.com", + CertIssuer: "https://accounts.example.com", + }}, + }}, + wantErr: "mutually exclusive", + }, + { + name: "provenance missing signer identity", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "signed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + CertIssuer: "https://accounts.example.com", + }}, + }}, + wantErr: "signerIdentity is required", + }, + { + name: "provenance missing cert issuer", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "signed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + SignerIdentity: "dev@example.com", + }}, + }}, + wantErr: "certIssuer is required", + }, + { + name: "provenance with control characters rejected", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "signed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + SignerIdentity: "dev@example.com\x1b[31m", + CertIssuer: "https://accounts.example.com", + }}, + }}, + wantErr: "non-graphic", + }, + { + name: "provenance field too long rejected", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "signed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + SignerIdentity: strings.Repeat("a", maxReferenceLength+1), + CertIssuer: "https://accounts.example.com", + }}, + }}, + wantErr: "exceeds", + }, + { + name: "unsigned exception alone is valid", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "unsigned", Source: "s", Digest: validSHA256Digest, Unsigned: true}, + }}, + }, { name: "requiredBy diamond is not a cycle", lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ diff --git a/pkg/skills/options.go b/pkg/skills/options.go index 5f58e533fd..f861c031b9 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -31,6 +31,12 @@ type InstallOptions struct { ProjectRoot string `json:"project_root,omitempty"` // Group is the group name to add the skill to after installation. Group string `json:"group,omitempty"` + // AllowUnsigned permits installing a project-scoped skill whose artifact + // carries no Sigstore signature. Without it, unsigned artifacts are + // rejected; with it, the lock entry records the exception as + // "unsigned: true". Skill content is AI-executed instructions, so this + // is an explicit per-install trust decision, never a default. + AllowUnsigned bool `json:"allow_unsigned,omitempty"` // LayerData is the tar.gz content from an OCI layer. Internal use only — NOT exposed via HTTP API. LayerData []byte `json:"-"` // Reference is the full OCI reference (e.g. ghcr.io/org/skill:v1). @@ -70,6 +76,30 @@ type InstallOptions struct { // normal "same digest means content is already correct" fast path must // not apply. Internal use only — NOT exposed via HTTP API. SyncRestore bool `json:"-"` + // Provenance carries the verified signer identity established during + // install-time verification, for recording into the lock entry. Set by + // the verification step, nil when the artifact is unsigned or + // verification did not run. Internal use only — NOT exposed via HTTP API. + Provenance *ProvenanceInfo `json:"-"` + // SigstoreBundle is the serialized Sigstore bundle backing Provenance, + // persisted alongside the install record so sync can re-verify offline. + // Internal use only — NOT exposed via HTTP API. + SigstoreBundle []byte `json:"-"` +} + +// ProvenanceInfo is the verified signer identity of an installed artifact, +// the in-memory mirror of the lock file's provenance block. +type ProvenanceInfo struct { + // SignerIdentity is the certificate subject identity (workflow path for + // GitHub Actions certificates, SAN verbatim otherwise). + SignerIdentity string `json:"-"` + // CertIssuer is the OIDC issuer that authenticated the signer. + CertIssuer string `json:"-"` + // RepositoryURI is the source repository from the certificate + // extensions, when present. + RepositoryURI string `json:"-"` + // SigstoreURL is the Sigstore instance the signature chains to. + SigstoreURL string `json:"-"` } // InstallResult contains the outcome of an Install operation.