diff --git a/cmd/thv/app/skill_info.go b/cmd/thv/app/skill_info.go index b17939c0b3..a4e5d89380 100644 --- a/cmd/thv/app/skill_info.go +++ b/cmd/thv/app/skill_info.go @@ -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) diff --git a/cmd/thv/app/skill_install.go b/cmd/thv/app/skill_install.go index 1d958265f9..455db3b273 100644 --- a/cmd/thv/app/skill_install.go +++ b/cmd/thv/app/skill_install.go @@ -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) diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index dabadacbde..01baec6316 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -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. diff --git a/docs/server/docs.go b/docs/server/docs.go index 18b35f48ef..d8b0e15b09 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1826,6 +1826,10 @@ const docTemplate = `{ "description": "Provisional marks provenance with a documented verification gap\n(git signatures until transparency-log validation lands).", "type": "boolean" }, + "public_key": { + "description": "PublicKey is the base64-encoded DER SPKI cosign public key a\nkey-pair-signed entry is pinned to. Set only when SignerIdentity and\nCertIssuer are empty: the two anchors are mutually exclusive.", + "type": "string" + }, "repository_ref": { "description": "RepositoryRef is the git ref the signing workflow ran on, from Fulcio\ncertificate extension 1.3.6.1.4.1.57264.1.14. Empty means\nunconstrained, matching lock files written before the field existed.", "type": "string" diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 99e7e9c61f..725bd6952c 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1819,6 +1819,10 @@ "description": "Provisional marks provenance with a documented verification gap\n(git signatures until transparency-log validation lands).", "type": "boolean" }, + "public_key": { + "description": "PublicKey is the base64-encoded DER SPKI cosign public key a\nkey-pair-signed entry is pinned to. Set only when SignerIdentity and\nCertIssuer are empty: the two anchors are mutually exclusive.", + "type": "string" + }, "repository_ref": { "description": "RepositoryRef is the git ref the signing workflow ran on, from Fulcio\ncertificate extension 1.3.6.1.4.1.57264.1.14. Empty means\nunconstrained, matching lock files written before the field existed.", "type": "string" diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index f2bb14b123..f452f2ffd8 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1916,6 +1916,12 @@ components: Provisional marks provenance with a documented verification gap (git signatures until transparency-log validation lands). type: boolean + public_key: + description: |- + PublicKey is the base64-encoded DER SPKI cosign public key a + key-pair-signed entry is pinned to. Set only when SignerIdentity and + CertIssuer are empty: the two anchors are mutually exclusive. + type: string repository_ref: description: |- RepositoryRef is the git ref the signing workflow ran on, from Fulcio diff --git a/pkg/skills/lockfile/lockfile.go b/pkg/skills/lockfile/lockfile.go index 9a9f9a1b79..a3bde864b2 100644 --- a/pkg/skills/lockfile/lockfile.go +++ b/pkg/skills/lockfile/lockfile.go @@ -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; @@ -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 diff --git a/pkg/skills/lockfile/validation.go b/pkg/skills/lockfile/validation.go index 2dd8abe284..da9f28e766 100644 --- a/pkg/skills/lockfile/validation.go +++ b/pkg/skills/lockfile/validation.go @@ -4,6 +4,8 @@ package lockfile import ( + "crypto/x509" + "encoding/base64" "errors" "fmt" "strings" @@ -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 } @@ -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, @@ -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 == "" { @@ -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) + 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 diff --git a/pkg/skills/lockfile/validation_test.go b/pkg/skills/lockfile/validation_test.go index 7ae75adac6..547b79e608 100644 --- a/pkg/skills/lockfile/validation_test.go +++ b/pkg/skills/lockfile/validation_test.go @@ -18,6 +18,11 @@ var ( validContentDigest = ContentDigestPrefix + validSHA256Hex ) +// testPublicKeyB64 is a real P-256 public key in the base64 DER SPKI +// form a key-pinned lock entry stores. It must genuinely parse: validation +// rejects a value that merely decodes as base64. +const testPublicKeyB64 = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExlVDpbnOEv2fH3gS8n7UCHS9Gs0wKxIPR5EAcl8F1jSxlxAV/pll0NsSiuAK95Ws4Fpkn+5QkdVKNXy7LHgb2A==" + func TestValidateDigest(t *testing.T) { t.Parallel() @@ -202,6 +207,79 @@ func TestValidateLockfile(t *testing.T) { }}, wantErr: "mutually exclusive", }, + { + name: "publicKey-pinned provenance accepted without an identity", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + PublicKey: testPublicKeyB64, + }}, + }}, + }, + { + name: "publicKey and a certificate identity are mutually exclusive", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "both", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + SignerIdentity: "dev@example.com", + CertIssuer: "https://accounts.example.com", + PublicKey: testPublicKeyB64, + }}, + }}, + wantErr: "mutually exclusive", + }, + { + name: "certificate-derived fields rejected on a publicKey-pinned entry", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + PublicKey: testPublicKeyB64, + RepositoryRef: "refs/tags/v1", + }}, + }}, + wantErr: "read from a certificate", + }, + { + name: "publicKey must decode to a DER SPKI key, not merely to bytes", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + // Valid base64 of ASCII text: well-encoded, but not a key. + PublicKey: "bm90LWEta2V5LWp1c3QtdGV4dA==", + }}, + }}, + wantErr: "not a DER SPKI public key", + }, + { + // No resolvedReference, so the entry is classified by the only signal + // left: a bare commit hash means a git entry, whose signature lives + // on the commit and is verified against a certificate, never a key. + name: "publicKey rejected on a git entry classified by its digest", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: strings.Repeat("a", 40), Provenance: &Provenance{ + PublicKey: testPublicKeyB64, + }}, + }}, + wantErr: "only valid for an OCI artifact", + }, + { + // The same rejection reached through the resolved reference, with a + // digest that agrees with it: the anchor check fires on its own + // account here, not as a side effect of the two fields disagreeing. + name: "publicKey rejected on a git entry classified by its resolved reference", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validGitSHA1, + ResolvedReference: "git://github.com/org/repo@main#skills/keyed", + Provenance: &Provenance{PublicKey: testPublicKeyB64}, + }, + }}, + wantErr: "only valid for an OCI artifact", + }, + { + name: "publicKey must be base64", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + PublicKey: "-----BEGIN PUBLIC KEY-----", + }}, + }}, + wantErr: "not valid base64", + }, { name: "provenance missing signer identity", lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ @@ -279,6 +357,68 @@ func TestValidateLockfile(t *testing.T) { {Name: "unsigned", Source: "s", Digest: validSHA256Digest, Unsigned: true}, }}, }, + { + // The reported bypass: restore dispatches on resolvedReference, so + // classifying the entry by its digest alone let a git entry carry an + // OCI-only key anchor and pushed the malformed trust decision out to + // fetch time. + name: "publicKey rejected on a git entry whose digest is written in OCI form", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, + ResolvedReference: "git://github.com/org/repo@main#skills/keyed", + Provenance: &Provenance{PublicKey: testPublicKeyB64}, + }, + }}, + wantErr: "resolvedReference is a git reference", + }, + { + // The disagreement is malformed on its own account, key anchor or + // not: buildPinnedReference would splice the OCI digest into a git + // reference and produce "git://github.com/org/repo@sha256:...". + name: "git resolvedReference with an OCI digest rejected without any provenance", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "mixed", Source: "s", Digest: validSHA256Digest, + ResolvedReference: "git://github.com/org/repo@main#skills/mixed"}, + }}, + wantErr: "a git entry pins a full commit hash", + }, + { + name: "OCI resolvedReference with a git commit hash digest rejected", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "mixed", Source: "s", Digest: validGitSHA1, + ResolvedReference: "ghcr.io/org/mixed:1.0.0"}, + }}, + wantErr: "an OCI entry pins", + }, + { + // Plugins are a separate graph validated by the same per-entry + // checks; the "plugins:" prefix is what attributes the failure to + // the right half of the file. + name: "plugin git resolvedReference with an OCI digest rejected", + lf: Lockfile{Version: CurrentVersion, Plugins: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, + ResolvedReference: "git://github.com/org/repo@main#plugins/keyed", + Provenance: &Provenance{PublicKey: testPublicKeyB64}, + }, + }}, + wantErr: "plugins: entry \"keyed\": digest is an OCI manifest digest", + }, + { + name: "git resolvedReference with a matching commit hash is valid", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "gitted", Source: "s", Digest: validGitSHA1, + ResolvedReference: "git://github.com/org/repo@main#skills/gitted"}, + }}, + }, + { + name: "OCI resolvedReference with a matching manifest digest is valid", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "ocied", Source: "s", Digest: validSHA256Digest, + ResolvedReference: "ghcr.io/org/ocied:1.0.0", + Provenance: &Provenance{PublicKey: testPublicKeyB64}, + }, + }}, + }, { 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 931a35b776..899b9a908b 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -127,6 +127,10 @@ type ProvenanceInfo struct { RunnerEnvironment string `json:"runner_environment,omitempty"` // SigstoreURL is the Sigstore instance the signature chains to. SigstoreURL string `json:"sigstore_url,omitempty"` + // PublicKey is the base64-encoded DER SPKI cosign public key a + // key-pair-signed entry is pinned to. Set only when SignerIdentity and + // CertIssuer are empty: the two anchors are mutually exclusive. + PublicKey string `json:"public_key,omitempty"` // Provisional marks provenance with a documented verification gap // (git signatures until transparency-log validation lands). Provisional bool `json:"provisional,omitempty"` diff --git a/pkg/skills/skillsvc/verify.go b/pkg/skills/skillsvc/verify.go index acf695a582..6f441e838b 100644 --- a/pkg/skills/skillsvc/verify.go +++ b/pkg/skills/skillsvc/verify.go @@ -377,6 +377,7 @@ func provenanceInfoFromLock(p *lockfile.Provenance) *skills.ProvenanceInfo { RepositoryRef: p.RepositoryRef, RunnerEnvironment: p.RunnerEnvironment, SigstoreURL: p.SigstoreURL, + PublicKey: p.PublicKey, Provisional: p.Provisional, } } @@ -394,6 +395,7 @@ func provenanceInfoToLock(p *skills.ProvenanceInfo) *lockfile.Provenance { RepositoryRef: p.RepositoryRef, RunnerEnvironment: p.RunnerEnvironment, SigstoreURL: p.SigstoreURL, + PublicKey: p.PublicKey, Provisional: p.Provisional, } } diff --git a/pkg/skills/skillsvc/verify_test.go b/pkg/skills/skillsvc/verify_test.go index 0061dc03e4..2b1c143633 100644 --- a/pkg/skills/skillsvc/verify_test.go +++ b/pkg/skills/skillsvc/verify_test.go @@ -31,6 +31,11 @@ const ( testRunnerEnvironment = "github-hosted" ) +// testPublicKeyB64 is a real P-256 public key in the base64 DER SPKI +// form a key-pinned lock entry stores. It must genuinely parse: validation +// rejects a value that merely decodes as base64. +const testPublicKeyB64 = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExlVDpbnOEv2fH3gS8n7UCHS9Gs0wKxIPR5EAcl8F1jSxlxAV/pll0NsSiuAK95Ws4Fpkn+5QkdVKNXy7LHgb2A==" + func signedResult() *verifier.Result { return &verifier.Result{ Signed: true, @@ -910,7 +915,12 @@ func TestVerifyLocalInstall(t *testing.T) { func TestProvenanceConversionsPreserveEveryField(t *testing.T) { t.Parallel() - locked := &lockfile.Provenance{ + // Two fixtures rather than one, because the anchors are mutually + // exclusive (see validateProvenanceAnchor): a single struct with every + // field populated would encode a lock state that validation rejects, and + // make it the canonical example. Coverage is asserted across the pair, so + // a newly added field still has to appear in one of them. + keyless := &lockfile.Provenance{ SignerIdentity: testSignerIdentity, CertIssuer: testCertIssuer, RepositoryURI: "https://github.com/org/repo", @@ -919,11 +929,15 @@ func TestProvenanceConversionsPreserveEveryField(t *testing.T) { SigstoreURL: "https://rekor.sigstore.dev", Provisional: true, } - requireAllFieldsSet(t, locked) + keyed := &lockfile.Provenance{PublicKey: testPublicKeyB64} + requireEveryFieldCovered(t, keyless, keyed) - info := provenanceInfoFromLock(locked) - requireAllFieldsSet(t, info) - assert.Equal(t, locked, provenanceInfoToLock(info)) + keylessInfo := provenanceInfoFromLock(keyless) + keyedInfo := provenanceInfoFromLock(keyed) + requireEveryFieldCovered(t, keylessInfo, keyedInfo) + assert.Equal(t, keyless, provenanceInfoToLock(keylessInfo)) + assert.Equal(t, keyed, provenanceInfoToLock(keyedInfo), + "a key-pinned entry must survive the round trip as the only anchor it has") assert.Nil(t, provenanceInfoFromLock(nil)) assert.Nil(t, provenanceInfoToLock(nil)) @@ -941,16 +955,26 @@ func TestNormalizeCatalogProvenance(t *testing.T) { "a single supported constraint must not be discarded") } -// requireAllFieldsSet fails when any field of the struct pointed to by v holds -// its zero value, so a field added to one provenance shape without a matching -// line in the conversions is caught here rather than in production. -func requireAllFieldsSet(t *testing.T, v any) { +// requireEveryFieldCovered fails when a field of the struct type is zero in +// every one of the given values, so a field added to one provenance shape +// without a matching line in the conversions is caught here rather than in +// production. Values are checked as a set because mutually exclusive anchors +// cannot be represented in a single legal fixture. +func requireEveryFieldCovered(t *testing.T, values ...any) { t.Helper() - rv := reflect.ValueOf(v).Elem() - for i := range rv.NumField() { - assert.False(t, rv.Field(i).IsZero(), - "%s.%s is zero: wire it through the provenance conversions and this fixture", - rv.Type().Name(), rv.Type().Field(i).Name) + require.NotEmpty(t, values) + first := reflect.ValueOf(values[0]).Elem() + for i := range first.NumField() { + covered := false + for _, v := range values { + if !reflect.ValueOf(v).Elem().Field(i).IsZero() { + covered = true + break + } + } + assert.True(t, covered, + "%s.%s is zero in every fixture: wire it through the provenance conversions and a fixture", + first.Type().Name(), first.Type().Field(i).Name) } } diff --git a/pkg/skills/verifier/errors.go b/pkg/skills/verifier/errors.go index 03b5e4c1d4..3972e22f59 100644 --- a/pkg/skills/verifier/errors.go +++ b/pkg/skills/verifier/errors.go @@ -3,7 +3,10 @@ package verifier -import "errors" +import ( + "errors" + "fmt" +) var ( // ErrUnsigned indicates the artifact carries no Sigstore signature @@ -45,3 +48,12 @@ var ( // specifically. ErrProvenanceFieldMismatch = errors.New("certificate provenance field mismatch") ) + +// errKeyPinnedEntry is returned when a lock entry pinned to a cosign public +// key is handed to a keyless verification path. It wraps ErrSignatureInvalid +// so existing callers classify it as a verification failure — which it is, +// the entry cannot be verified this way — while the message names the real +// problem instead of surfacing sigstore's empty-identity complaint. +var errKeyPinnedEntry = fmt.Errorf( + "%w: entry is pinned to a cosign public key, which the keyless verification path cannot check", + ErrSignatureInvalid) diff --git a/pkg/skills/verifier/git.go b/pkg/skills/verifier/git.go index bdb34f34c5..c85f689838 100644 --- a/pkg/skills/verifier/git.go +++ b/pkg/skills/verifier/git.go @@ -39,6 +39,14 @@ func (*Default) VerifyGit( payload, signature []byte, expected *ProvenanceExpectation, ) (*Result, error) { + // A git commit signature is always certificate-based (gitsign), so a + // key-pinned expectation cannot apply to one and PublicKey would + // otherwise be silently ignored — yielding an unsigned or signer-mismatch + // diagnosis unrelated to the real problem. Lock validation rejects such + // an entry, but an expectation built in memory never passes through it. + if keyPinnedExpectation(expected) { + return nil, errKeyPinnedEntry + } if len(signature) == 0 { return nil, fmt.Errorf("%w: commit is not signed", ErrUnsigned) } diff --git a/pkg/skills/verifier/oci.go b/pkg/skills/verifier/oci.go index 1b072e8b9f..d42bcd7538 100644 --- a/pkg/skills/verifier/oci.go +++ b/pkg/skills/verifier/oci.go @@ -23,6 +23,9 @@ func (d *Default) VerifyOCI( imageRef, digest string, expected *ProvenanceExpectation, ) (*Result, error) { + if keyPinnedExpectation(expected) { + return nil, errKeyPinnedEntry + } bundles, err := d.retrieveBundles(ctx, imageRef, digest) if err != nil { return nil, err diff --git a/pkg/skills/verifier/offline.go b/pkg/skills/verifier/offline.go index ef4ceb406b..a90b29f653 100644 --- a/pkg/skills/verifier/offline.go +++ b/pkg/skills/verifier/offline.go @@ -19,6 +19,9 @@ import ( // inside the Sigstore policy; a mismatch is reported as ErrSignerMismatch, // any other verification failure as ErrSignatureInvalid. func (*Default) VerifyBundleOffline(bundleBytes []byte, digest string, expected *lockfile.Provenance) error { + if expected != nil && expected.PublicKey != "" { + return errKeyPinnedEntry + } if len(bundleBytes) == 0 { // Classified as an invalid signature because a recorded identity // with nothing backing it cannot be verified; the message leads diff --git a/pkg/skills/verifier/types.go b/pkg/skills/verifier/types.go index 10735119a5..09c04b8c9c 100644 --- a/pkg/skills/verifier/types.go +++ b/pkg/skills/verifier/types.go @@ -92,6 +92,18 @@ func (r *Result) ToLockProvenance() *lockfile.Provenance { } } +// keyPinnedExpectation reports whether expected pins a cosign public key +// rather than a certificate identity. Such an entry cannot be checked by the +// keyless policy at all, and its certificate fields are empty by construction +// — which sigstore rejects ("there must be subject alternative name +// criteria") rather than treating as match-anything, so the failure is closed +// either way. Detecting it up front replaces that opaque message with one +// naming the actual mismatch: the entry is key-pinned, and the caller reached +// for the keyless path. +func keyPinnedExpectation(expected *ProvenanceExpectation) bool { + return expected != nil && expected.locked != nil && expected.locked.PublicKey != "" +} + // expectedIdentity converts a lock expectation into the core Identity bound // into the Sigstore verification policy. Catalog expectations and nil (trust // on first use) yield nil, which core treats as chain-of-trust-only diff --git a/pkg/skills/verifier/verifier_test.go b/pkg/skills/verifier/verifier_test.go index 47173dfa0f..956ece3541 100644 --- a/pkg/skills/verifier/verifier_test.go +++ b/pkg/skills/verifier/verifier_test.go @@ -30,6 +30,11 @@ import ( ) // startTestRegistry runs an in-process OCI registry and returns its host. +// testPublicKeyB64 is a real P-256 public key in the base64 DER SPKI +// form a key-pinned lock entry stores. It must genuinely parse: validation +// rejects a value that merely decodes as base64. +const testPublicKeyB64 = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExlVDpbnOEv2fH3gS8n7UCHS9Gs0wKxIPR5EAcl8F1jSxlxAV/pll0NsSiuAK95Ws4Fpkn+5QkdVKNXy7LHgb2A==" + func startTestRegistry(t *testing.T) string { t.Helper() reg := httptest.NewServer(registry.New()) @@ -561,3 +566,49 @@ func TestVerifyOCIReportsKeySignedAgainstLockedIdentity(t *testing.T) { require.NotErrorIs(t, err, ErrSignerMismatch, "no certificate exists to compare, so reporting a signer mismatch would invent an observation") } + +// TestKeylessPathsRefuseKeyPinnedEntry covers the window this PR opens: the +// lock can now express a key-pinned entry, but nothing verifies one until the +// install path lands. Such an entry has empty certificate fields by +// construction, and sigstore rejects an all-empty identity ("there must be +// subject alternative name criteria") rather than treating it as +// match-anything — so the failure was already closed. What was missing was a +// message naming the actual problem instead of that one. +func TestKeylessPathsRefuseKeyPinnedEntry(t *testing.T) { + t.Parallel() + host := startTestRegistry(t) + ref, digest := pushTestArtifact(t, host) + keyed := &lockfile.Provenance{PublicKey: testPublicKeyB64} + d := NewDefault(nil) + + _, err := d.VerifyOCI(t.Context(), ref, digest, NewLockExpectation(keyed)) + require.ErrorIs(t, err, ErrSignatureInvalid, + "an entry that cannot be verified this way is a verification failure") + require.ErrorContains(t, err, "pinned to a cosign public key") + + // Refused before any registry access, so an unsigned artifact does not + // mask the misrouting as ErrUnsigned. + require.NotErrorIs(t, err, ErrUnsigned) + + offlineErr := d.VerifyBundleOffline([]byte("ignored"), digest, keyed) + require.ErrorIs(t, offlineErr, ErrSignatureInvalid) + require.ErrorContains(t, offlineErr, "pinned to a cosign public key", + "offline re-verification has the identical hazard") +} + +// TestVerifyGitRefusesKeyPinnedEntry covers the third keyless path. Lock +// validation rejects a key-pinned git entry, but an expectation assembled in +// memory never passes through it — and VerifyGit would otherwise ignore +// PublicKey entirely and report an unrelated unsigned diagnosis, since a git +// commit signature is always certificate-based. +func TestVerifyGitRefusesKeyPinnedEntry(t *testing.T) { + t.Parallel() + + expected := NewLockExpectation(&lockfile.Provenance{PublicKey: testPublicKeyB64}) + _, err := NewDefault(nil).VerifyGit(t.Context(), []byte("payload"), []byte("signature"), expected) + + require.ErrorIs(t, err, ErrSignatureInvalid) + require.ErrorContains(t, err, "pinned to a cosign public key") + require.NotErrorIs(t, err, ErrUnsigned, + "refused before the signature checks, so the misrouting is not masked as unsigned") +}