Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cmd/thv/app/skill_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ func printSkillInfoText(info *skills.SkillInfo) {
_, _ = fmt.Fprintf(w, "Name:\t%s\n", info.Metadata.Name)
_, _ = fmt.Fprintf(w, "Version:\t%s\n", info.Metadata.Version)
switch {
// Checked before the identity cases: a key-pinned entry has no signer
// identity and no cert issuer, so those would render as empty values and
// read exactly like an untracked install.
case info.Provenance != nil && info.Provenance.PublicKey != "":
_, _ = fmt.Fprintf(w, "Signed by:\t(cosign key pair)\n")
_, _ = fmt.Fprintf(w, "Public key:\t%s\n", info.Provenance.PublicKey)
case info.Provenance != nil && info.Provenance.Provisional:
_, _ = fmt.Fprintf(w, "Signed by:\t%s (provisional)\n", info.Provenance.SignerIdentity)
_, _ = fmt.Fprintf(w, "Cert issuer:\t%s\n", info.Provenance.CertIssuer)
Expand Down
4 changes: 4 additions & 0 deletions cmd/thv/app/skill_install.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ func printInstallTrust(result *skills.InstallResult) {
}
name := result.Skill.Metadata.Name
switch {
// Before the identity cases: a key-pinned install has no signer identity
// to name, and "signed by " with nothing after it is worse than silence.
case result.Provenance != nil && result.Provenance.PublicKey != "":
fmt.Printf("Installed %s (signed by a cosign key pair; the pinned public key is in the lock file)\n", name)
case result.Provenance != nil && result.Provenance.Provisional:
fmt.Printf("Installed %s (signed by %s; verification provisional — see lock file)\n",
name, result.Provenance.SignerIdentity)
Expand Down
4 changes: 4 additions & 0 deletions docs/arch/12-skills-system.md
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,10 @@ Catalog constraints the skill verifier cannot enforce are refused rather than si

The Sigstore policy alone is not the whole guarantee: its SAN match deliberately leaves the signing workflow's git ref unpinned (`(@.*)?$`), so identity alone is satisfied by "the right workflow, on any branch." Two additional certificate fields — the git ref the signing workflow ran on and the runner class it executed in (`repositoryRef:`/`runnerEnvironment:` in `provenance:`) — are enforced separately, after the Sigstore policy succeeds, against the certificate's Fulcio extensions. An entry recorded before these fields existed, or a certificate that carries neither (a signer outside CI), is unconstrained on that field — never a wildcard match once something IS recorded. Install, sync, and upgrade all require the recorded ref and runner class to match exactly, with no automatic allowance for any kind of change, ref rotation included: an earlier version of this guard let a recorded tag ref rotate to any other tag ref automatically, reasoning that a release workflow signs each version on its own tag, but review found that this let a candidate signed from an attacker's own tag on the same repository (e.g. `refs/tags/attacker-release`) replace a pinned tag just as easily, since nothing tied the candidate's tag to the version actually being upgraded to. A ref or runner-class change of any shape is now blocked exactly like a genuine signer-identity change, and needs the same explicit `--allow-signer-change` to proceed and re-record it.

A `provenance:` block records exactly one trust anchor. Keyless entries record a certificate identity (`signerIdentity:` plus `certIssuer:`, optionally narrowed by the certificate-derived fields above). Key-pair entries record `publicKey:` instead — the base64 DER SPKI form of the cosign public key — and must leave every certificate field empty, since a key-pair signature carries none. The two are mutually exclusive: they are checked by different policies against different trust roots, so an entry carrying both would not say which applies.

The full key is stored rather than a digest of it because the key is recoverable from neither the artifact nor the stored bundle — cosign's signature manifest defines no annotation carrying it — so a digest would have nothing to hash at verification time. That also makes the field safe to add without a schema version bump: a build predating it sees an entry with no `signerIdentity`, reports it as required, and fails the whole lock file closed rather than treating the entry as unpinned.

What is still trusted on faith, deliberately and visibly:

- **Unsigned skills** install only with an explicit `--allow-unsigned`, recorded as `unsigned: true` in the lock entry. That entry is a standing exception: lock-driven operations (sync restores, upgrade re-pins) honor it without re-asking.
Expand Down
4 changes: 4 additions & 0 deletions docs/server/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions docs/server/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions docs/server/swagger.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 22 additions & 1 deletion pkg/skills/lockfile/lockfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,13 @@ type Entry struct {
Extra map[string]any `yaml:",inline"`
}

// Provenance is the Sigstore signer identity recorded for a verified entry.
// Provenance is the Sigstore trust anchor recorded for a verified entry.
//
// Exactly one anchor is recorded. Keyless (Fulcio) entries record a
// certificate identity — SignerIdentity plus CertIssuer, optionally narrowed
// by the certificate-derived fields below. Key-pair (cosign) entries record
// PublicKey instead and leave every certificate field empty, because a
// key-pair signature carries no certificate to derive them from.
type Provenance struct {
// SignerIdentity is the certificate subject identity: for GitHub
// Actions certificates, the workflow path relative to the repository;
Expand All @@ -117,6 +123,21 @@ type Provenance struct {
RunnerEnvironment string `yaml:"runnerEnvironment,omitempty"`
// SigstoreURL is the Sigstore instance the signature chains to.
SigstoreURL string `yaml:"sigstoreUrl,omitempty"`
// PublicKey is the base64-encoded DER SPKI form of the cosign public key
// a key-pair-signed entry is pinned to — the PEM body with its armor and
// line breaks removed, since a lock value may not contain whitespace
// (see validateProvenance).
//
// The full key is stored, not a digest of it, because the key is
// recoverable from neither the artifact nor the stored bundle: cosign's
// signature manifest defines no annotation carrying it, so there would
// be nothing to hash at verification time. This value is therefore the
// entry's only trust anchor, and re-verification depends on it.
//
// Mutually exclusive with SignerIdentity/CertIssuer. Whether the value
// parses as a usable key is the verifier's concern; validation here is
// syntactic, as for every other field.
PublicKey string `yaml:"publicKey,omitempty"`
// Provisional marks provenance whose verification has a documented
// gap — currently git-commit signatures, verified for signature and
// certificate chain but without transparency-log proof of signing
Expand Down
136 changes: 126 additions & 10 deletions pkg/skills/lockfile/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package lockfile

import (
"crypto/x509"
"encoding/base64"
"errors"
"fmt"
"strings"
Expand Down Expand Up @@ -154,15 +156,79 @@ func validateEntry(entry Entry) error {
if err := validateResolvedReference(entry.ResolvedReference); err != nil {
return fmt.Errorf("entry %q: resolvedReference: %w", entry.Name, err)
}
if err := validateDigestKind(entry); err != nil {
return fmt.Errorf("entry %q: %w", entry.Name, err)
}
}
if err := validateEntryTrust(entry); err != nil {
return fmt.Errorf("entry %q: %w", entry.Name, err)
}
return nil
}

// validateEntryTrust checks the entry's trust fields: that it records at most
// one of a signature and an unsigned exception, that the provenance block is
// well-formed, and that its anchor suits the kind of artifact the entry
// restores. Separated from the rest of validateEntry because these checks
// need the whole entry — the anchor's fitness depends on the source — where
// validateProvenance below sees only the provenance block.
func validateEntryTrust(entry Entry) error {
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)
return errors.New("provenance and unsigned are mutually exclusive" +
" — an entry is either a verified signature or a recorded unsigned exception")
}
if entry.Provenance == nil {
return nil
}
if err := validateProvenance(entry.Provenance); err != nil {
return fmt.Errorf("provenance: %w", err)
}
// A cosign key pair signs an OCI artifact, while a git entry's signature
// lives on the commit and is always certificate-based. A key-pinned git
// entry pins an anchor no verification of that entry could ever use.
if entry.Provenance.PublicKey != "" && entryIsGitSource(entry) {
return errors.New("provenance: publicKey is only valid for an OCI artifact;" +
" a git commit signature is verified against a certificate, not a key")
}
if entry.Provenance != nil {
if err := validateProvenance(entry.Provenance); err != nil {
return fmt.Errorf("entry %q: provenance: %w", entry.Name, err)
return nil
}

// entryIsGitSource classifies an entry the way the restore path does: from
// resolvedReference, which is the field buildPinnedReference dispatches on.
// Reading the deciding field — rather than inferring the kind from the digest
// beside it — is what keeps this classification from drifting away from the
// code that acts on it. An entry recording no resolved reference is
// classified by its digest form, the only signal left.
func entryIsGitSource(entry Entry) bool {
if entry.ResolvedReference != "" {
return gitresolver.IsGitReference(entry.ResolvedReference)
}
return !strings.HasPrefix(entry.Digest, ContentDigestPrefix)
}

// validateDigestKind rejects an entry whose digest form contradicts the
// source it is restored from. Restore dispatches on resolvedReference but
// pins from digest, so a git reference paired with an OCI digest yields
// "git://host/repo@sha256:..." — a reference no fetch can satisfy, and one
// whose malformedness surfaces only once the fetch is attempted. The install
// path cannot write such a pair (a git install records a bare commit hash, an
// OCI install a prefixed manifest digest); a hand edit or a botched merge
// resolution can, which is why the lock boundary is where it belongs.
//
// Callers apply this only to an entry that records a resolved reference:
// without one there is no second field to disagree with.
func validateDigestKind(entry Entry) error {
ociDigest := strings.HasPrefix(entry.Digest, ContentDigestPrefix)
if entryIsGitSource(entry) {
if ociDigest {
return errors.New("digest is an OCI manifest digest but resolvedReference is a git reference;" +
" a git entry pins a full commit hash")
}
return nil
}
if !ociDigest {
return fmt.Errorf("digest is a git commit hash but resolvedReference is an OCI reference;"+
" an OCI entry pins %q + 64 hex chars", ContentDigestPrefix)
}
return nil
}
Expand All @@ -173,11 +239,8 @@ func validateEntry(entry Entry) error {
// 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")
if err := validateProvenanceAnchor(p); err != nil {
return err
}
fields := map[string]string{
"signerIdentity": p.SignerIdentity,
Expand All @@ -186,6 +249,7 @@ func validateProvenance(p *Provenance) error {
"repositoryRef": p.RepositoryRef,
"runnerEnvironment": p.RunnerEnvironment,
"sigstoreUrl": p.SigstoreURL,
"publicKey": p.PublicKey,
}
for name, value := range fields {
if value == "" {
Expand All @@ -206,6 +270,58 @@ func validateProvenance(p *Provenance) error {
return nil
}

// validateProvenanceAnchor enforces that an entry records exactly one trust
// anchor: a keyless certificate identity, or a cosign public key. The two are
// verified by different policies against different trust roots, so an entry
// carrying both would not say which applies, and an entry carrying neither
// pins nothing at all.
//
// Rejecting a key-pinned entry outright is also what makes this field safe to
// add without a schema version bump: a build that predates PublicKey sees an
// entry with no signerIdentity, reports it as required, and fails the whole
// lock file closed rather than silently treating the entry as unpinned.
func validateProvenanceAnchor(p *Provenance) error {
keyed := p.PublicKey != ""
identified := p.SignerIdentity != "" || p.CertIssuer != ""
switch {
case keyed && identified:
return errors.New("publicKey and signerIdentity/certIssuer are mutually exclusive" +
" — an entry is pinned to either a cosign key or a keyless certificate identity")
case keyed:
// The remaining fields are all read off a Fulcio certificate, which a
// key-pair signature does not have. Populated here they would pin
// constraints that no verification could ever check.
for name, value := range map[string]string{
"repositoryUri": p.RepositoryURI,
"repositoryRef": p.RepositoryRef,
"runnerEnvironment": p.RunnerEnvironment,
"sigstoreUrl": p.SigstoreURL,
} {
if value != "" {
return fmt.Errorf("%s cannot be set on a publicKey-pinned entry"+
" — it is read from a certificate, and a key-pair signature has none", name)
}
}
der, err := base64.StdEncoding.DecodeString(p.PublicKey)
Comment thread
samuv marked this conversation as resolved.
if err != nil {
return fmt.Errorf("publicKey is not valid base64: %w", err)
}
// Decoding proves the encoding, not the content. This value is the
// entry's only trust anchor, so a blob that is merely well-encoded
// would be accepted here and then fail deep inside verification, long
// after the lock file stopped being the obvious suspect.
if _, err := x509.ParsePKIXPublicKey(der); err != nil {
return fmt.Errorf("publicKey is not a DER SPKI public key: %w", err)
}
return nil
case p.SignerIdentity == "":
return errors.New("signerIdentity is required")
case p.CertIssuer == "":
return errors.New("certIssuer is required")
}
return nil
}

// validateResolvedReference syntactically constrains the resolvedReference
// field. Sync fetches from this value without re-resolving Source, and the
// lock file is hand-editable, so a value that is not a plausible git:// or
Expand Down
Loading
Loading