diff --git a/cmd/thv/app/skill_install.go b/cmd/thv/app/skill_install.go index 455db3b273..11585d0578 100644 --- a/cmd/thv/app/skill_install.go +++ b/cmd/thv/app/skill_install.go @@ -5,11 +5,14 @@ package app import ( "fmt" + "os" + "path/filepath" "strings" "github.com/spf13/cobra" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/verifier" ) var ( @@ -19,6 +22,7 @@ var ( skillInstallProjectRoot string skillInstallGroup string skillInstallAllowUnsigned bool + skillInstallPublicKey string ) var skillInstallCmd = &cobra.Command{ @@ -46,6 +50,10 @@ func init() { skillInstallCmd.Flags().StringVar(&skillInstallGroup, "group", "", "Group to add the skill to after installation") skillInstallCmd.Flags().BoolVar(&skillInstallAllowUnsigned, "allow-unsigned", false, "Allow installing a project-scoped skill without a verified signature (recorded in the lock file)") + skillInstallCmd.Flags().StringVar(&skillInstallPublicKey, "public-key", "", + "Path to the cosign public key (cosign.pub) a key-pair-signed skill must verify against."+ + " Required the first time such a skill is installed project-scoped; the key is then pinned"+ + " in the lock file and reused automatically") } func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { @@ -56,6 +64,11 @@ func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { return err } + publicKey, err := readInstallPublicKey(skillInstallPublicKey) + if err != nil { + return err + } + result, err := c.Install(cmd.Context(), skills.InstallOptions{ Name: args[0], Scope: skills.Scope(skillInstallScope), @@ -64,6 +77,7 @@ func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { ProjectRoot: projectRoot, Group: skillInstallGroup, AllowUnsigned: skillInstallAllowUnsigned, + PublicKey: publicKey, }) if err != nil { return formatSkillError("install skill", err) @@ -73,6 +87,28 @@ func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { return nil } +// readInstallPublicKey turns a --public-key file path into the encoded key +// material the API carries. The CLI reads the file rather than forwarding its +// path because the server is a separate process, possibly on another host, +// where that path names nothing — or something else. +func readInstallPublicKey(path string) (string, error) { + if path == "" { + return "", nil + } + cleanPath := filepath.Clean(path) + // #nosec G304 - the path is a CLI flag the user chose; reading the file + // they named is the operation. + pemBytes, err := os.ReadFile(cleanPath) + if err != nil { + return "", fmt.Errorf("read public key: %w", err) + } + encoded, err := verifier.EncodePublicKey(pemBytes) + if err != nil { + return "", fmt.Errorf("read public key %s: %w", cleanPath, err) + } + return encoded, nil +} + // printInstallTrust shows the trust state the install recorded — RFC // THV-0080 wants the pinned identity displayed prominently, not discovered // weeks later inside a signer-mismatch error. diff --git a/cmd/thv/app/skill_push.go b/cmd/thv/app/skill_push.go index 8b9d79abc7..b6ff3d6665 100644 --- a/cmd/thv/app/skill_push.go +++ b/cmd/thv/app/skill_push.go @@ -35,10 +35,10 @@ func init() { skillPushCmd.Flags().StringVar(&skillPushKey, "key", "", "Path to a cosign private key to sign the pushed artifact. "+ "Encrypted keys are decrypted with COSIGN_PASSWORD read from the 'thv serve' process, "+ - "which performs the signing. NOTE: ToolHive cannot verify key-pair signatures at "+ - "install time, so a project-scoped install of the result is refused and "+ - "--allow-unsigned does not override it — use keyless signing for artifacts that "+ - "need to be installable") + "which performs the signing. Consumers installing the result project-scoped must pass "+ + "--public-key with the matching cosign public key the first time; distribute it "+ + "alongside the artifact. Keyless signing needs no such out-of-band step, since the "+ + "signer identity is verifiable from the artifact itself") skillPushCmd.Flags().StringVar(&skillPushIdentityToken, "identity-token", "", "OIDC identity token (or a path to a file containing one) for keyless signing. "+ "Mutually exclusive with --key. If omitted, one is acquired automatically: from the "+ diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index 01baec6316..c063c0d6fc 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -430,7 +430,15 @@ What is still trusted on faith, deliberately and visibly: Publishing is signed by default: `thv skill push` requires `--key` (a cosign private key), an OIDC identity token for keyless signing (supplied with `--identity-token` or acquired automatically), or an explicit `--no-sign`. Either signing path attaches the signature manifest next to the artifact, and the bundle is retrievable at install. See [Publishing](#3-publishing) for the full ladder. -Only the **keyless** path produces an installable artifact. Install-time verification checks the keyless (Fulcio) trust root, and a cosign key pair carries no certificate to chain to it — nor is the signing public key recoverable from the artifact, since the cosign manifest defines no annotation for it. A project-scoped install of a `--key`-signed artifact is therefore refused, and `--allow-unsigned` does **not** override the refusal: the artifact *is* signed, so it never produces the unsigned verdict that exception applies to. Tracked as [#6442](https://github.com/stacklok/toolhive/issues/6442). +Both paths produce an installable artifact, but they differ in what the consumer must supply. A cosign key pair carries no certificate to chain to the keyless (Fulcio) trust root, and the signing public key is recoverable neither from the artifact nor from the attached bundle — the cosign manifest defines no annotation for it. So the key has to arrive from outside the artifact: a project-scoped install of a `--key`-signed skill requires `--public-key` on first use, which verifies the signature and pins that key in the lock entry as `publicKey:`. Later installs read it back from the lock and need no flag. + +`--allow-unsigned` does **not** substitute for the key, in either direction: the artifact *is* signed, so it never produces the unsigned verdict that exception applies to. Nor does `--allow-signer-change` re-anchor an entry to a new key — a key-pair bundle carries no identity to observe, so honoring one would re-anchor on the strength of the caller having named it. Re-anchoring means removing the lock entry and reinstalling. + +Dispatch between the two paths is decided by the **lock entry, never the artifact**: a `publicKey:` pin selects the key path, a certificate pin the keyless one. Letting the artifact choose would let a republished key-signed artifact walk out of the identity its entry is pinned to. A supplied key that disagrees with what the entry pins is refused up front rather than silently ignored. + +Verifying a key-pair signature binds it to the artifact explicitly. The signature covers cosign's simple-signing payload, and signature manifests are discovered by a tag derived from the digest being verified — so attachment proves nothing about which artifact a signature describes. The payload digest is reconstructed from the requested reference and each candidate must sign exactly those bytes, which is what stops one artifact's signature from being replayed onto another by copying its signature layer into that artifact's `.sig` manifest (`bundleSignsPayload`). + +Scope for v1 (issue [#6442](https://github.com/stacklok/toolhive/issues/6442)): install only. `--public-key` is not yet accepted on `upgrade` or `sync --adopt`, and the plugins surface does not accept it at all yet. ### Schema diff --git a/docs/cli/thv_skill_install.md b/docs/cli/thv_skill_install.md index 268829aadc..842d687a7f 100644 --- a/docs/cli/thv_skill_install.md +++ b/docs/cli/thv_skill_install.md @@ -31,6 +31,7 @@ thv skill install [skill-name] [flags] --group string Group to add the skill to after installation -h, --help help for install --project-root string Project root path for project-scoped installs + --public-key string Path to the cosign public key (cosign.pub) a key-pair-signed skill must verify against. Required the first time such a skill is installed project-scoped; the key is then pinned in the lock file and reused automatically --scope string Installation scope (user, project) (default "user") ``` diff --git a/docs/cli/thv_skill_push.md b/docs/cli/thv_skill_push.md index 754a6a0ddd..39779c6fa5 100644 --- a/docs/cli/thv_skill_push.md +++ b/docs/cli/thv_skill_push.md @@ -26,7 +26,7 @@ thv skill push [reference] [flags] ``` -h, --help help for push --identity-token string OIDC identity token (or a path to a file containing one) for keyless signing. Mutually exclusive with --key. If omitted, one is acquired automatically: from the ambient CI OIDC token when running with id-token: write permission, otherwise via an interactive browser sign-in - --key string Path to a cosign private key to sign the pushed artifact. Encrypted keys are decrypted with COSIGN_PASSWORD read from the 'thv serve' process, which performs the signing. NOTE: ToolHive cannot verify key-pair signatures at install time, so a project-scoped install of the result is refused and --allow-unsigned does not override it — use keyless signing for artifacts that need to be installable + --key string Path to a cosign private key to sign the pushed artifact. Encrypted keys are decrypted with COSIGN_PASSWORD read from the 'thv serve' process, which performs the signing. Consumers installing the result project-scoped must pass --public-key with the matching cosign public key the first time; distribute it alongside the artifact. Keyless signing needs no such out-of-band step, since the signer identity is verifiable from the artifact itself --no-sign Push without signing (consumers will need an explicit unsigned exception to install project-scoped) ``` diff --git a/docs/server/docs.go b/docs/server/docs.go index d8b0e15b09..0ded5bafcc 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -3308,6 +3308,10 @@ const docTemplate = `{ "description": "ProjectRoot is the project root path for project-scoped installs", "type": "string" }, + "public_key": { + "description": "PublicKey is the base64-encoded DER SPKI cosign public key the artifact\nmust verify against, for artifacts signed with a cosign key pair rather\nthan keylessly. Required the first time such an artifact is installed\nproject-scoped, and pinned in the lock file from then on.", + "type": "string" + }, "scope": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.Scope" }, diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 725bd6952c..626fcb4185 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -3301,6 +3301,10 @@ "description": "ProjectRoot is the project root path for project-scoped installs", "type": "string" }, + "public_key": { + "description": "PublicKey is the base64-encoded DER SPKI cosign public key the artifact\nmust verify against, for artifacts signed with a cosign key pair rather\nthan keylessly. Required the first time such an artifact is installed\nproject-scoped, and pinned in the lock file from then on.", + "type": "string" + }, "scope": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.Scope" }, diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index f452f2ffd8..b8d5f384f6 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -3108,6 +3108,13 @@ components: project_root: description: ProjectRoot is the project root path for project-scoped installs type: string + public_key: + description: |- + PublicKey is the base64-encoded DER SPKI cosign public key the artifact + must verify against, for artifacts signed with a cosign key pair rather + than keylessly. Required the first time such an artifact is installed + project-scoped, and pinned in the lock file from then on. + type: string scope: $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.Scope' version: diff --git a/go.mod b/go.mod index 9a4e5eca2a..6ab587a2c8 100644 --- a/go.mod +++ b/go.mod @@ -258,7 +258,7 @@ require ( github.com/sergi/go-diff v1.4.0 // indirect github.com/sethvargo/go-retry v0.3.0 // indirect github.com/shibumi/go-pathspec v1.3.0 // indirect - github.com/sigstore/protobuf-specs v0.5.2 // indirect + github.com/sigstore/protobuf-specs v0.5.2 github.com/sigstore/rekor v1.5.4 // indirect github.com/sigstore/rekor-tiles/v2 v2.3.0 // indirect github.com/sigstore/sigstore v1.10.9 diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index 1e316aec70..173a0b4e1f 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -129,6 +129,7 @@ func (s *SkillsRoutes) installSkill(w http.ResponseWriter, r *http.Request) erro Force: req.Force, Group: req.Group, AllowUnsigned: req.AllowUnsigned, + PublicKey: req.PublicKey, }) if err != nil { return err diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 71bd6e254f..051d2df587 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -35,6 +35,11 @@ type installSkillRequest struct { // verified signature; the exception is recorded in the project's lock // file. AllowUnsigned bool `json:"allow_unsigned,omitempty"` + // PublicKey is the base64-encoded DER SPKI cosign public key the artifact + // must verify against, for artifacts signed with a cosign key pair rather + // than keylessly. Required the first time such an artifact is installed + // project-scoped, and pinned in the lock file from then on. + PublicKey string `json:"public_key,omitempty"` // Group is the group name to add the skill to after installation Group string `json:"group,omitempty"` } diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index 909d26b5d8..3f333489c2 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -212,6 +212,7 @@ func (c *Client) Install(ctx context.Context, opts skills.InstallOptions) (*skil Force: opts.Force, Group: opts.Group, AllowUnsigned: opts.AllowUnsigned, + PublicKey: opts.PublicKey, } var resp installResponse diff --git a/pkg/skills/client/dto.go b/pkg/skills/client/dto.go index 62f39a4fec..800595957b 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -18,6 +18,10 @@ type installRequest struct { // AllowUnsigned mirrors skills.InstallOptions.AllowUnsigned; without it // here the CLI flag would silently never reach the server. AllowUnsigned bool `json:"allow_unsigned,omitempty"` + // PublicKey mirrors skills.InstallOptions.PublicKey: the base64 DER SPKI + // the CLI encoded from the --public-key file, since a path would not + // resolve on a server in another process or on another host. + PublicKey string `json:"public_key,omitempty"` } type validateRequest struct { diff --git a/pkg/skills/lockfile/validation.go b/pkg/skills/lockfile/validation.go index da9f28e766..0e16032b05 100644 --- a/pkg/skills/lockfile/validation.go +++ b/pkg/skills/lockfile/validation.go @@ -134,6 +134,14 @@ func findRequiredByCycle(entries []Entry) []string { // from a corrupted or hostile lock file reaching the fetch path. const maxReferenceLength = 512 +// MaxEncodedPublicKeyLength bounds the base64 DER SPKI in a provenance block. +// Key material needs more room than an identifier: the largest key cosign can +// import, RSA-4096, encodes to 736 characters, so the reference bound would +// reject a legitimate anchor rather than the oversized garbage these limits +// exist to stop. Exported because the same value must bound the field before +// it is ever written — see verifier.EncodePublicKey. +const MaxEncodedPublicKeyLength = 1024 + func validateEntry(entry Entry) error { if err := skills.ValidateSkillName(entry.Name); err != nil { return fmt.Errorf("entry name: %w", err) @@ -239,9 +247,6 @@ func validateDigestKind(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 err := validateProvenanceAnchor(p); err != nil { - return err - } fields := map[string]string{ "signerIdentity": p.SignerIdentity, "certIssuer": p.CertIssuer, @@ -255,8 +260,8 @@ func validateProvenance(p *Provenance) error { if value == "" { continue } - if len(value) > maxReferenceLength { - return fmt.Errorf("%s exceeds %d characters", name, maxReferenceLength) + if limit := provenanceFieldLimit(name); len(value) > limit { + return fmt.Errorf("%s exceeds %d characters", name, limit) } if strings.TrimSpace(value) != value { return fmt.Errorf("%s has leading or trailing whitespace", name) @@ -267,7 +272,20 @@ func validateProvenance(p *Provenance) error { } } } - return nil + // Ordered after the syntactic checks so publicKey is length-bounded before + // it is base64-decoded: this file is hand-editable, and decoding first + // would size the allocation off a value that has not been checked yet. + return validateProvenanceAnchor(p) +} + +// provenanceFieldLimit returns the length bound for one provenance field. +// Every field is a short identifier except publicKey, which carries encoded +// key material and needs the wider bound. +func provenanceFieldLimit(name string) int { + if name == "publicKey" { + return MaxEncodedPublicKeyLength + } + return maxReferenceLength } // validateProvenanceAnchor enforces that an entry records exactly one trust diff --git a/pkg/skills/lockfile/validation_test.go b/pkg/skills/lockfile/validation_test.go index 547b79e608..416e5dff56 100644 --- a/pkg/skills/lockfile/validation_test.go +++ b/pkg/skills/lockfile/validation_test.go @@ -275,7 +275,10 @@ func TestValidateLockfile(t *testing.T) { name: "publicKey must be base64", lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ - PublicKey: "-----BEGIN PUBLIC KEY-----", + // Graphic and whitespace-free, so the syntactic checks pass + // and the base64 decode is genuinely what rejects it. PEM + // armor would be caught earlier, by its embedded spaces. + PublicKey: "-----BEGINPUBLICKEY-----", }}, }}, wantErr: "not valid base64", @@ -419,6 +422,32 @@ func TestValidateLockfile(t *testing.T) { }, }}, }, + { + // Key material needs more room than an identifier: an RSA-4096 + // SPKI encodes to 736 characters, so the reference bound would + // reject a legitimate anchor rather than the garbage it guards + // against. Reaching the SPKI parse at this length proves the + // narrower bound is not the one being applied. + name: "publicKey is bounded above the reference limit", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + PublicKey: strings.Repeat("A", maxReferenceLength+4), + }}, + }}, + wantErr: "not a DER SPKI public key", + }, + { + // The length must be rejected before the value is base64-decoded, + // so the allocation is bounded by a checked number rather than by + // whatever the lock file happens to contain. + name: "publicKey beyond its own bound rejected before decoding", + lf: Lockfile{Version: CurrentVersion, Skills: []Entry{ + {Name: "keyed", Source: "s", Digest: validSHA256Digest, Provenance: &Provenance{ + PublicKey: strings.Repeat("A", MaxEncodedPublicKeyLength+4), + }}, + }}, + wantErr: "exceeds", + }, { 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 899b9a908b..44769a922c 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -40,6 +40,15 @@ type InstallOptions struct { // "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"` + // PublicKey is the base64-encoded DER SPKI cosign public key a + // project-scoped install must verify the artifact against, for artifacts + // signed with a cosign key pair rather than keylessly. Required on true + // first use of such an artifact — the signing key is recoverable from + // neither the artifact nor its bundle, so nothing else can supply the + // trust anchor — and pinned into the lock entry, which supplies it on + // every install thereafter. A value that conflicts with what the lock + // already pins is rejected, never ignored. + PublicKey string `json:"public_key,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). diff --git a/pkg/skills/skillsvc/install.go b/pkg/skills/skillsvc/install.go index bf911e7dc7..da47cbe528 100644 --- a/pkg/skills/skillsvc/install.go +++ b/pkg/skills/skillsvc/install.go @@ -47,6 +47,14 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski // the same lock key and DB record. opts.ProjectRoot = projectRoot + // Checked here, before any resolve or fetch work: this is the only entry + // point a caller-supplied public key arrives through, and rejecting it now + // means a key that could never be used is reported as bad input rather + // than as a verification failure after the artifact has been pulled. + if err := validateInstallPublicKey(opts, scope); err != nil { + return nil, err + } + // When the caller supplies `version` separately and the name is a tag-less // OCI-like reference (contains '/' but no ':' or '@'), splice the version // in as the tag. Without this, parseOCIReference + qualifiedOCIRef would diff --git a/pkg/skills/skillsvc/verify.go b/pkg/skills/skillsvc/verify.go index 6f441e838b..4e9526fe3f 100644 --- a/pkg/skills/skillsvc/verify.go +++ b/pkg/skills/skillsvc/verify.go @@ -33,6 +33,28 @@ func shouldVerifyInstall(opts skills.InstallOptions, scope skills.Scope) bool { return scope == skills.ScopeProject && opts.ProjectRoot != "" } +// validateInstallPublicKey rejects a supplied public key before any resolve +// or fetch work begins. Both checks exist so the key is never accepted and +// then quietly unused: an install that does not verify would drop it on the +// floor, and a malformed one would otherwise surface as a verification +// failure deep in the install, long after the input that caused it. +func validateInstallPublicKey(opts skills.InstallOptions, scope skills.Scope) error { + if opts.PublicKey == "" { + return nil + } + if !shouldVerifyInstall(opts, scope) { + return httperr.WithCode( + errors.New("public_key (--public-key) applies to project-scoped installs, which are the ones"+ + " whose trust anchor a lock file records; this install would verify nothing"), + http.StatusBadRequest, + ) + } + if _, err := verifier.DecodePublicKey(opts.PublicKey); err != nil { + return httperr.WithCode(fmt.Errorf("public_key: %w", err), http.StatusBadRequest) + } + return nil +} + // provenanceDecision is the outcome of install-time verification: either a // verified identity (with the bundle backing it) or an explicit unsigned // exception. @@ -61,6 +83,12 @@ func applyDecisionToOpts(opts *skills.InstallOptions, decision *provenanceDecisi // takes its place if the install resolved from a registry entry that // declared one (RFC THV-0080 follow-up #6310) — otherwise trust on first // use records whatever identity verification observes. +// +// An entry pinned to a cosign public key, or a first install that supplies +// one, takes the key path instead. Which path runs is decided by the lock +// file and the caller, never by what the artifact turns out to carry: letting +// the artifact select its own verification policy would let a republished +// key-signed artifact walk out of the identity its entry is pinned to. func (s *service) verifyOCIInstall( ctx context.Context, opts skills.InstallOptions, @@ -79,6 +107,13 @@ func (s *service) verifyOCIInstall( catalogExpected = normalizeCatalogProvenance(opts.CatalogProvenance) verifierExpected = verifier.NewCatalogExpectation(catalogExpected) } + keyAnchor, err := resolveKeyAnchor(opts, skillName, expected, expectUnsigned, catalogExpected) + if err != nil { + return nil, err + } + if keyAnchor != "" { + return s.verifyOCIInstallWithKey(ctx, keyAnchor, skillName, ref, digest) + } if opts.AllowSignerChange { // The signer-change guard was explicitly overridden: verify the // chain of trust only and re-record whatever identity is observed. @@ -106,6 +141,168 @@ func (s *service) verifyOCIInstall( }, nil } +// resolveKeyAnchor decides which cosign public key, if any, this install +// verifies against, returning "" for the ordinary keyless path. +// +// Dispatch is lock-first: a key-pinned entry selects the key path using the +// key the LOCK records, so a supplied key can confirm that pin but never +// replace it. A supplied key is itself the anchor only on true first use, +// where nothing is recorded yet and the key is the only thing that can supply +// one. +// +// Every disagreement between the supplied key and the recorded trust state is +// an error rather than a precedence rule. Silently preferring one of two +// conflicting anchors is how a mistyped --public-key installs as though it had +// been honored — and a caller who names a trust anchor has said they want it +// enforced, so the honest answer to "that is not the anchor here" is to stop. +func resolveKeyAnchor( + opts skills.InstallOptions, + skillName string, + expected *lockfile.Provenance, + expectUnsigned bool, + catalogExpected *regtypes.Provenance, +) (string, error) { + supplied := opts.PublicKey + locked := "" + if expected != nil { + locked = expected.PublicKey + } + + if opts.AllowSignerChange { + // The override re-verifies from scratch and re-records what it + // observes. For a key there is nothing to observe — a key-pair bundle + // carries no identity — so honoring a key here would mean re-anchoring + // to whatever key the caller named, on the strength of the caller + // having named it. That is the in-place re-anchor v1 deliberately does + // not offer. Without a key the override drops the recorded one and + // takes the keyless path, which is the supported key-to-keyless move. + if supplied != "" { + return "", httperr.WithCode( + fmt.Errorf("skill %q: a public key cannot be combined with allow_signer_change;"+ + " re-anchoring an entry to a different key is not supported —"+ + " uninstall the skill and reinstall it with the new key", skillName), + http.StatusBadRequest, + ) + } + return "", nil + } + + switch { + case locked != "" && supplied != "" && supplied != locked: + return "", keyAnchorConflict(skillName, + "is pinned to a different cosign public key than the one supplied") + case locked != "": + return locked, nil + case supplied == "": + return "", nil + // A key was supplied and the entry is not key-pinned. Each remaining case + // already records an anchor the key would have to displace. + case expected != nil: + return "", keyAnchorConflict(skillName, + fmt.Sprintf("is pinned to signer %q, and a cosign key pair carries no certificate identity"+ + " that could satisfy it", expected.SignerIdentity)) + case expectUnsigned: + return "", keyAnchorConflict(skillName, + "is recorded as an explicit unsigned exception, which a public key cannot upgrade in place") + case catalogExpected != nil: + return "", httperr.WithCode( + fmt.Errorf("skill %q: its catalog entry declares a certificate identity, which a"+ + " cosign key-pair signature carries none of; refusing to install under a public key"+ + " and silently drop that constraint", skillName), + http.StatusForbidden, + ) + default: + return supplied, nil + } +} + +// keyAnchorConflict reports a supplied public key that contradicts the trust +// state the lock file already records. The remedy is the same for all of them +// — v1 has no in-place re-anchor path — so it is stated once here. +func keyAnchorConflict(skillName, problem string) error { + return httperr.WithCode( + fmt.Errorf("skill %q %s; to install it under a different trust anchor,"+ + " uninstall the skill and reinstall it", skillName, problem), + http.StatusForbidden, + ) +} + +// lockedAnchorDescription names the trust anchor an entry records, for error +// messages that report an artifact failing to satisfy it. A key-pinned entry +// has no signer identity, and rendering it as one would print an empty pin +// and read as a bug in the lock file rather than a refusal. +func lockedAnchorDescription(expected *lockfile.Provenance) string { + if expected.PublicKey != "" { + return "a cosign public key" + } + return fmt.Sprintf("signer %q", expected.SignerIdentity) +} + +// verifyOCIInstallWithKey verifies the artifact against a cosign public key +// and records that key as the entry's trust anchor. +// +// Unlike the keyless path there is nothing to observe: a key-pair bundle +// carries no certificate, so the provenance recorded is the key that was +// checked rather than an identity read off the artifact. That makes this a +// weaker claim than keyless provenance — it says the holder of this key signed +// this artifact, and nothing about who that holder is — which is why the key +// has to come from outside the artifact every time. +func (s *service) verifyOCIInstallWithKey( + ctx context.Context, + encodedKey, skillName, ref, digest string, +) (*provenanceDecision, error) { + // Re-decoded rather than carried down from validateInstallPublicKey: this + // key may instead have come from the lock file, and internal callers reach + // the install path without passing that entry check at all. + pubKeyPEM, err := verifier.DecodePublicKey(encodedKey) + if err != nil { + return nil, httperr.WithCode( + fmt.Errorf("skill %q: pinned %w", skillName, err), + http.StatusUnprocessableEntity, + ) + } + result, verifyErr := s.artifactVerifier().VerifyOCIWithKey(ctx, ref, digest, pubKeyPEM) + if verifyErr != nil { + return nil, classifyKeyVerifyError(verifyErr, skillName) + } + return &provenanceDecision{ + provenance: &skills.ProvenanceInfo{PublicKey: encodedKey}, + bundle: result.Bundle, + }, nil +} + +// classifyKeyVerifyError maps a key-pair verification failure to the 403 the +// install API surfaces. allow_unsigned is deliberately not consulted on any +// arm: an install that named a public key asked for that key to be enforced, +// and the unsigned exception answers a different question. +func classifyKeyVerifyError(verifyErr error, skillName string) error { + switch { + case errors.Is(verifyErr, verifier.ErrUnsigned): + return httperr.WithCode( + fmt.Errorf("skill %q must verify against a cosign public key, but the artifact"+ + " carries no signature material at all", skillName), + http.StatusForbidden, + ) + case errors.Is(verifyErr, verifier.ErrKeylessSigned): + return httperr.WithCode( + fmt.Errorf("skill %q: %w; install it without a public key so its certificate identity"+ + " is verified and pinned instead", skillName, verifyErr), + http.StatusForbidden, + ) + default: + // The wrong key and a corrupt signature are indistinguishable here, + // and saying so is more useful than picking one: the bundle records no + // key of its own, so the only fact available is that this key does not + // verify this signature. + return httperr.WithCode( + fmt.Errorf("skill %q does not verify against the cosign public key it is checked against"+ + " — either the key is not the one that signed it, or the signature is damaged: %w", + skillName, verifyErr), + http.StatusForbidden, + ) + } +} + // verifyGitInstall verifies the gitsign signature on the resolved commit // before anything is written or recorded. See verifyOCIInstall for the // catalog-provenance fallback on true first use. @@ -120,6 +317,17 @@ func (s *service) verifyGitInstall( if err != nil { return nil, err } + // Refused rather than ignored, for the same reason the lock file refuses + // to store a key on a git entry: a commit signature is made with a Fulcio + // certificate, so there is no operation here a public key could take part + // in. + if opts.PublicKey != "" { + return nil, httperr.WithCode( + fmt.Errorf("skill %q is installed from git, whose commit signature is verified against a"+ + " certificate; a cosign public key cannot verify it", skillName), + http.StatusBadRequest, + ) + } var catalogExpected *regtypes.Provenance verifierExpected := verifier.NewLockExpectation(expected) if !lockEntryExists { @@ -165,10 +373,20 @@ func verifyLocalInstall(opts skills.InstallOptions, skillName string) (*provenan if err != nil { return nil, err } + // A local build has no registry signature material at all, so a public key + // would have nothing to check. Saying so beats accepting the key and then + // recording the install as unsigned anyway. + if opts.PublicKey != "" { + return nil, httperr.WithCode( + fmt.Errorf("skill %q is a local build, which carries no registry signature for a"+ + " cosign public key to verify", skillName), + http.StatusBadRequest, + ) + } if expected != nil { return nil, httperr.WithCode( - fmt.Errorf("skill %q is locked to signer %q; a local build cannot satisfy it", - skillName, expected.SignerIdentity), + fmt.Errorf("skill %q is locked to %s; a local build cannot satisfy it", + skillName, lockedAnchorDescription(expected)), http.StatusForbidden, ) } @@ -297,7 +515,7 @@ func classifyInstallVerifyError( // deliberately no remedy here: the artifact IS signed, and recording it // as an unsigned exception would file a false trust decision in the lock. case errors.Is(verifyErr, verifier.ErrKeySigned): - return keySignedInstallError(skillName, verifyErr) + return keySignedInstallError(skillName, verifyErr, expected) default: return httperr.WithCode( fmt.Errorf("signature verification failed for %q: %w", skillName, verifyErr), @@ -320,7 +538,7 @@ func classifyCatalogVerifyError(verifyErr error, skillName string) error { // at all. The catalog constraint is beside the point, so this reports the // same diagnosis and remedy the non-catalog route does. if errors.Is(verifyErr, verifier.ErrKeySigned) { - return keySignedInstallError(skillName, verifyErr) + return keySignedInstallError(skillName, verifyErr, nil) } return httperr.WithCode( fmt.Errorf("skill %q does not match its catalog-declared provenance: %w", skillName, verifyErr), @@ -328,17 +546,38 @@ func classifyCatalogVerifyError(verifyErr error, skillName string) error { ) } -// keySignedInstallError reports a key-signed artifact identically wherever it -// is detected. A lock-constrained install and a catalog-constrained first -// install reach classification by different routes, but neither could verify -// the artifact and both have the same remedy, so the wording is shared rather -// than duplicated — including the note that allow_unsigned is not a way out, -// since the artifact IS signed and recording it as an unsigned exception -// would file a false trust decision in the lock. -func keySignedInstallError(skillName string, verifyErr error) error { +// keySignedInstallError reports a key-signed artifact that the keyless path +// could not verify. A lock-constrained install and a catalog-constrained +// first install both land here, but the remedy is not the same, so it is +// chosen from what the entry already pins rather than stated generically. +// +// With no anchor recorded, this is a first install of a key-signed artifact +// and --public-key is exactly the missing input. With a keyless identity +// pinned, --public-key is NOT the remedy: resolveKeyAnchor rejects a key +// against a certificate-pinned entry, because a key pair carries no identity +// that could satisfy it. Naming the flag there would send the caller into a +// conflict error one step later — the most likely way to reach this arm is +// also the one where the obvious advice is wrong. +// +// Either way allow_unsigned is no way out: the artifact IS signed, and +// recording it as an unsigned exception would file a false trust decision in +// the lock. +func keySignedInstallError(skillName string, verifyErr error, expected *lockfile.Provenance) error { + if expected != nil { + return httperr.WithCode( + fmt.Errorf("skill %q: %w, but its lock entry is pinned to keyless signer %q;"+ + " a key pair carries no certificate identity that could satisfy that pin, so"+ + " supplying a public key is refused rather than allowed to displace it."+ + " Remove the lock entry and reinstall with --public-key to anchor it to the key"+ + " (allow_unsigned does not apply — the artifact is signed)", + skillName, verifyErr, expected.SignerIdentity), + http.StatusForbidden, + ) + } return httperr.WithCode( - fmt.Errorf("skill %q: %w; re-publish it with keyless signing"+ - " (allow_unsigned does not apply — the artifact is signed)", + fmt.Errorf("skill %q: %w; re-run the install with --public-key pointing at the cosign"+ + " public key it was signed with, and that key is pinned in the lock file for"+ + " subsequent installs (allow_unsigned does not apply — the artifact is signed)", skillName, verifyErr), http.StatusForbidden, ) diff --git a/pkg/skills/skillsvc/verify_test.go b/pkg/skills/skillsvc/verify_test.go index 2b1c143633..0e8ee89abe 100644 --- a/pkg/skills/skillsvc/verify_test.go +++ b/pkg/skills/skillsvc/verify_test.go @@ -841,6 +841,8 @@ func TestVerifyLocalInstall(t *testing.T) { opts skills.InstallOptions entry *lockfile.Entry wantErr bool + wantCode int + wantMsg string unsigned bool }{ { @@ -867,6 +869,34 @@ func TestVerifyLocalInstall(t *testing.T) { }, }, wantErr: true, + wantMsg: `locked to signer "` + testSignerIdentity + `"`, + }, + { + // A key-pinned entry has no signer identity, so the refusal must + // name the anchor it does have. Rendering it as `signer ""` reads + // as a corrupt lock file rather than as the refusal it is. + name: "locked public key refuses local replacement without a blank signer", + opts: skills.InstallOptions{AllowUnsigned: true}, + entry: &lockfile.Entry{ + Name: "local-skill", + Source: "example.com/org/local-skill", + ResolvedReference: "example.com/org/local-skill:v1", + Digest: "sha256:" + strings.Repeat("a", 64), + Provenance: &lockfile.Provenance{PublicKey: testPublicKeyB64}, + }, + wantErr: true, + wantMsg: "locked to a cosign public key", + }, + { + // Bad input, not a policy refusal: a local build carries no + // registry signature for any key to check, so accepting the key + // and then recording the install as unsigned would answer a + // question the caller did not ask. + name: "public key rejected as input for a local build", + opts: skills.InstallOptions{AllowUnsigned: true, PublicKey: testPublicKeyB64}, + wantErr: true, + wantCode: http.StatusBadRequest, + wantMsg: "carries no registry signature", }, { name: "locked unsigned honored with flag", @@ -899,7 +929,14 @@ func TestVerifyLocalInstall(t *testing.T) { decision, err := verifyLocalInstall(opts, "local-skill") if tc.wantErr { require.Error(t, err) - assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + wantCode := tc.wantCode + if wantCode == 0 { + wantCode = http.StatusForbidden + } + assert.Equal(t, wantCode, httperr.Code(err)) + if tc.wantMsg != "" { + assert.Contains(t, err.Error(), tc.wantMsg) + } return } require.NoError(t, err) @@ -1021,18 +1058,51 @@ func TestClassifyInstallVerifyErrorDistinguishesProvenanceField(t *testing.T) { // reported as a verification failure, and must say plainly that allow_unsigned // is no remedy — the artifact is signed, so recording an unsigned exception // would file a false trust decision in the lock. +// +// The remedy differs by what the entry pins, so it is asserted per case: the +// generic wording that once told every caller to republish keylessly now +// directs a first install to the flag that verifies it instead. func TestClassifyInstallVerifyErrorNamesKeySigned(t *testing.T) { t.Parallel() err := classifyInstallVerifyError(verifier.ErrKeySigned, "some-skill", nil) assert.Contains(t, err.Error(), "cosign key pair") - assert.Contains(t, err.Error(), "re-publish it with keyless signing", - "the message must state the remedy, not merely the refusal") + assert.Contains(t, err.Error(), "--public-key", + "a first install of a key-signed artifact is exactly what --public-key is for") assert.Contains(t, err.Error(), "allow_unsigned does not apply") + assert.NotContains(t, err.Error(), "re-publish it with keyless signing", + "republishing is no longer the remedy — the artifact is verifiable as signed") assert.NotContains(t, err.Error(), "signature verification failed for", "the generic invalid-signature wording is the misdiagnosis this replaces") } +// TestClassifyInstallVerifyErrorKeySignedAgainstKeylessPin covers the arm +// where the obvious advice is wrong: the entry pins a certificate identity, so +// resolveKeyAnchor refuses a supplied key rather than letting it displace the +// pin. Telling this caller to pass --public-key would walk them into that +// conflict one step later, so the message has to name the pin and the fact +// that re-anchoring means removing the entry. +func TestClassifyInstallVerifyErrorKeySignedAgainstKeylessPin(t *testing.T) { + t.Parallel() + + err := classifyInstallVerifyError( + verifier.ErrKeySigned, "some-skill", &lockfile.Provenance{SignerIdentity: testSignerIdentity}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + assert.Contains(t, err.Error(), testSignerIdentity, + "the pinned identity is the reason the key is refused; naming it explains the refusal") + assert.Contains(t, err.Error(), "Remove the lock entry and reinstall", + "re-anchoring is deliberately not offered in place — say what does work") + assert.Contains(t, err.Error(), "allow_unsigned does not apply") + + // The supplied key really is refused against a certificate pin, so the + // message is not merely cautious wording. + _, anchorErr := resolveKeyAnchor( + skills.InstallOptions{PublicKey: testPublicKeyB64}, "some-skill", + &lockfile.Provenance{SignerIdentity: testSignerIdentity}, false, nil) + require.Error(t, anchorErr) +} + // TestClassifySignatureErrorNamesKeySigned keeps the sync/upgrade failure // reason distinct from signature-invalid for the same reason. func TestClassifySignatureErrorNamesKeySigned(t *testing.T) { @@ -1086,8 +1156,326 @@ func TestCatalogInstallNamesKeySignedArtifact(t *testing.T) { require.Error(t, err) assert.Equal(t, http.StatusForbidden, httperr.Code(err)) - assert.Contains(t, err.Error(), "re-publish it with keyless signing") + assert.Contains(t, err.Error(), "--public-key") assert.Contains(t, err.Error(), "allow_unsigned does not apply") assert.NotContains(t, err.Error(), "does not match its catalog-declared provenance", "a key-signed artifact was never compared against the catalog constraint") } + +// keyedLockEntry is a lock entry pinned to testPublicKeyB64 — the shape a +// previous key-verified install leaves behind. +func keyedLockEntry(name string) lockfile.Entry { + return lockfile.Entry{ + Name: name, + Source: "example.com/org/" + name, + ResolvedReference: "example.com/org/" + name + ":v1", + Digest: "sha256:" + strings.Repeat("b", 64), + Provenance: &lockfile.Provenance{PublicKey: testPublicKeyB64}, + } +} + +// TestVerifyOCIInstall_KeyPathDispatch covers the two ways the key path is +// reached and the fact that reaching it excludes the keyless one. Dispatch is +// lock-first by design: were the artifact allowed to select the policy, a +// republished key-signed artifact could walk an entry out of the certificate +// identity it is pinned to. +func TestVerifyOCIInstall_KeyPathDispatch(t *testing.T) { + t.Parallel() + + keyPEM, err := verifier.DecodePublicKey(testPublicKeyB64) + require.NoError(t, err) + + tests := []struct { + name string + entry *lockfile.Entry + opts skills.InstallOptions + }{ + { + name: "first use verifies against the supplied key", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + }, + { + name: "pinned entry verifies against the locked key with no flag", + entry: ptrTo(keyedLockEntry("keyed-skill")), + }, + { + name: "supplied key that agrees with the pin is accepted", + entry: ptrTo(keyedLockEntry("keyed-skill")), + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + projectRoot := makeProjectRoot(t) + if tc.entry != nil { + writeLockEntry(t, projectRoot, *tc.entry) + } + mv := verifiermocks.NewMockVerifier(gomock.NewController(t)) + mv.EXPECT().VerifyOCIWithKey( + gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(keyPEM)). + Return(&verifier.Result{Signed: true, Bundle: []byte(`{"bundle":true}`)}, nil) + + opts := tc.opts + opts.ProjectRoot = projectRoot + svc := &service{sigVerifier: mv} + decision, err := svc.verifyOCIInstall( + t.Context(), opts, "keyed-skill", "example.com/org/keyed-skill:v1", + "sha256:"+strings.Repeat("b", 64)) + + require.NoError(t, err) + require.NotNil(t, decision.provenance) + // The key is recorded, and nothing else is: a key-pair bundle + // carries no certificate, so there is no identity to observe and + // inventing one would file provenance the artifact never asserted. + assert.Equal(t, &skills.ProvenanceInfo{PublicKey: testPublicKeyB64}, decision.provenance) + assert.Equal(t, []byte(`{"bundle":true}`), decision.bundle) + assert.False(t, decision.unsigned) + }) + } +} + +func ptrTo[T any](v T) *T { return &v } + +// TestResolveKeyAnchor pins the conflict rules. Every disagreement between a +// supplied key and the recorded trust state is an error rather than a +// precedence rule, because silently preferring either one is how a mistyped +// --public-key installs as though it had been honored. +func TestResolveKeyAnchor(t *testing.T) { + t.Parallel() + + const otherKeyB64 = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEZ7Bd5Kk7GAOI1PoQFvY6Sw+9zL3fVX" + + "Bqz0mAo0hVW1nQz4Vv9pQmT2yqXqL7NqRk5FvPQZ8DdcW0xTn3Yg6ZBw==" + identityPin := &lockfile.Provenance{SignerIdentity: testSignerIdentity, CertIssuer: testCertIssuer} + keyPin := &lockfile.Provenance{PublicKey: testPublicKeyB64} + + tests := []struct { + name string + opts skills.InstallOptions + expected *lockfile.Provenance + expectUnsigned bool + catalog *regtypes.Provenance + want string + wantCode int + wantMsg string + }{ + {name: "nothing supplied, nothing pinned: keyless"}, + {name: "identity pin, no key: keyless", expected: identityPin}, + {name: "key pin selects the locked key", expected: keyPin, want: testPublicKeyB64}, + { + name: "supplied key confirms the locked key", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + expected: keyPin, + want: testPublicKeyB64, + }, + { + name: "supplied key that differs from the pin is refused", + opts: skills.InstallOptions{PublicKey: otherKeyB64}, + expected: keyPin, + wantCode: http.StatusForbidden, + wantMsg: "pinned to a different cosign public key", + }, + { + name: "supplied key against an identity pin is refused", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + expected: identityPin, + wantCode: http.StatusForbidden, + wantMsg: "carries no certificate identity", + }, + { + name: "supplied key cannot upgrade a recorded unsigned exception", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + expectUnsigned: true, + wantCode: http.StatusForbidden, + wantMsg: "unsigned exception", + }, + { + // The catalog declares a certificate identity; installing under a + // key would satisfy none of it, so honoring the key would drop the + // constraint without saying so. + name: "supplied key against a catalog identity constraint is refused", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + catalog: ®types.Provenance{SignerIdentity: testSignerIdentity}, + wantCode: http.StatusForbidden, + wantMsg: "catalog entry declares a certificate identity", + }, + { + name: "first use adopts the supplied key", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + want: testPublicKeyB64, + }, + { + // The override re-records whatever it observes, and a key-pair + // bundle offers nothing to observe — so honoring a key here would + // re-anchor on the caller's say-so alone. v1 has no such path. + name: "allow_signer_change with a key is refused", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64, AllowSignerChange: true}, + expected: keyPin, + wantCode: http.StatusBadRequest, + wantMsg: "cannot be combined with allow_signer_change", + }, + { + // key -> keyless is the one supported transition: the candidate is + // chain-verifiable, so the override drops the pinned key and lets + // the keyless path record what it observes. + name: "allow_signer_change without a key drops the pinned key", + opts: skills.InstallOptions{AllowSignerChange: true}, + expected: keyPin, + want: "", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyAnchor(tc.opts, "some-skill", tc.expected, tc.expectUnsigned, tc.catalog) + if tc.wantCode != 0 { + require.Error(t, err) + assert.Equal(t, tc.wantCode, httperr.Code(err)) + assert.Contains(t, err.Error(), tc.wantMsg) + assert.Empty(t, got, "a refused anchor must not also be returned") + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +// TestValidateInstallPublicKey covers the entry guard: a key that this install +// could never use is bad input, reported before any artifact is fetched rather +// than as a verification failure afterwards. +func TestValidateInstallPublicKey(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opts skills.InstallOptions + scope skills.Scope + wantMsg string + }{ + { + name: "no key is always fine", + opts: skills.InstallOptions{}, + scope: skills.ScopeUser, + }, + { + name: "project scope with a valid key", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64, ProjectRoot: "/tmp/project"}, + scope: skills.ScopeProject, + }, + { + // User-scope installs are not lock-managed, so verification never + // runs and the key would be accepted and then dropped. + name: "user scope rejects a key it would never use", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + scope: skills.ScopeUser, + wantMsg: "applies to project-scoped installs", + }, + { + name: "project scope without a root rejects a key", + opts: skills.InstallOptions{PublicKey: testPublicKeyB64}, + scope: skills.ScopeProject, + wantMsg: "applies to project-scoped installs", + }, + { + name: "malformed base64 rejected", + opts: skills.InstallOptions{PublicKey: "not!base64", ProjectRoot: "/tmp/project"}, + scope: skills.ScopeProject, + wantMsg: "not valid base64", + }, + { + // Well-encoded is not well-formed. This value decodes cleanly and + // is not a key, which is exactly the input that would otherwise + // fail deep inside verification with the lock file as the suspect. + name: "valid base64 that is not a public key rejected", + opts: skills.InstallOptions{PublicKey: "aGVsbG8gd29ybGQ=", ProjectRoot: "/tmp/project"}, + scope: skills.ScopeProject, + wantMsg: "not a DER SPKI public key", + }, + { + name: "oversized key rejected before decoding", + opts: skills.InstallOptions{ + PublicKey: strings.Repeat("A", lockfile.MaxEncodedPublicKeyLength+1), + ProjectRoot: "/tmp/project", + }, + scope: skills.ScopeProject, + wantMsg: "exceeding the", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := validateInstallPublicKey(tc.opts, tc.scope) + if tc.wantMsg == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err), + "a key this install cannot use is bad input, not a policy refusal") + assert.Contains(t, err.Error(), tc.wantMsg) + }) + } +} + +// TestClassifyKeyVerifyError pins the diagnoses the key path reports. None of +// them mention allow_unsigned: an install that named a public key asked for +// that key to be enforced, and the unsigned exception answers a different +// question entirely. +func TestClassifyKeyVerifyError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + wantMsg string + }{ + { + name: "unsigned artifact", + err: verifier.ErrUnsigned, + wantMsg: "carries no signature material at all", + }, + { + // The likeliest mistake: a key aimed at an artifact that was + // signed keylessly. The remedy is to drop the key, which a bare + // "verification failed" would never suggest. + name: "keyless artifact names the right remedy", + err: verifier.ErrKeylessSigned, + wantMsg: "install it without a public key", + }, + { + // Wrong key and damaged signature are genuinely indistinguishable: + // the bundle records no key of its own to compare against. + name: "verification failure names both possible causes", + err: verifier.ErrSignatureInvalid, + wantMsg: "either the key is not the one that signed it, or the signature is damaged", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := classifyKeyVerifyError(tc.err, "keyed-skill") + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) + assert.Contains(t, err.Error(), tc.wantMsg) + assert.NotContains(t, err.Error(), "allow_unsigned") + }) + } +} + +// TestVerifyGitInstall_RefusesPublicKey guards the git side of the same rule +// the lock file enforces on key-pinned git entries: a commit signature is made +// with a Fulcio certificate, so a public key has no operation to take part in. +func TestVerifyGitInstall_RefusesPublicKey(t *testing.T) { + t.Parallel() + + svc := &service{sigVerifier: verifiermocks.NewMockVerifier(gomock.NewController(t))} + _, err := svc.verifyGitInstall( + t.Context(), + skills.InstallOptions{ProjectRoot: makeProjectRoot(t), PublicKey: testPublicKeyB64}, + "git-skill", []byte("payload"), "signature") + + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) + assert.Contains(t, err.Error(), "a cosign public key cannot verify it") +} diff --git a/pkg/skills/verifier/errors.go b/pkg/skills/verifier/errors.go index 3972e22f59..d6ea3ca0ab 100644 --- a/pkg/skills/verifier/errors.go +++ b/pkg/skills/verifier/errors.go @@ -19,12 +19,17 @@ var ( // identity other than the expected one. ErrSignerMismatch = errors.New("signer identity mismatch") // ErrKeySigned indicates the artifact carries only cosign key-pair - // signatures, which install-time verification cannot check: the keyless - // (Fulcio) trust root has nothing to chain them to, and the signing - // public key is recoverable neither from the artifact nor from the - // attached bundle — cosign's manifest defines no annotation carrying it, - // and the reconstructed bundle holds a fixed placeholder hint in its - // place. + // signatures, so the keyless (Fulcio) trust root has nothing to chain + // them to. Verifying them needs the signing public key supplied from + // outside the artifact, because it is recoverable neither from the + // artifact nor from the attached bundle — cosign's manifest defines no + // annotation carrying it, and the reconstructed bundle holds a fixed + // placeholder hint in its place. + // + // The text states the situation and prescribes no remedy: this sentinel + // is shared by the skills and plugins surfaces, which do not offer the + // same ones, and the right advice depends on what the entry already pins + // (see keySignedInstallError). Callers wrap it with their own. // // Deliberately NOT wrapping ErrSignatureInvalid, unlike // ErrProvenanceFieldMismatch below: the signature may be perfectly @@ -32,7 +37,18 @@ var ( // misclassification this sentinel exists to end. That narrowing cannot // fail open, because no caller treats ErrSignatureInvalid as permission // to proceed — it only selects a failure reason. - ErrKeySigned = errors.New("artifact is signed with a cosign key pair, which cannot be verified at install time") + ErrKeySigned = errors.New("artifact is signed with a cosign key pair, not keylessly") + // ErrKeylessSigned is the mirror of ErrKeySigned: every signature on the + // artifact carries a Fulcio certificate, so a cosign public key is the + // wrong trust anchor to check it with. Reported when a caller supplies a + // key for an artifact that was signed keylessly — the likeliest way to + // reach the key path by mistake, and one whose remedy (drop the key) is + // invisible in a bare "signature verification failed". + // + // Deliberately NOT wrapping ErrSignatureInvalid, for the same reason + // ErrKeySigned does not: the signature is intact, and there is a real + // verification path for it. + ErrKeylessSigned = errors.New("artifact is signed keylessly, not with a cosign key pair") // ErrProvenanceFieldMismatch indicates the signature verifies against // the expected signer identity and issuer, but a certificate field the // Sigstore policy cannot itself express — the repository ref or runner diff --git a/pkg/skills/verifier/oci.go b/pkg/skills/verifier/oci.go index d42bcd7538..ed489f1ae8 100644 --- a/pkg/skills/verifier/oci.go +++ b/pkg/skills/verifier/oci.go @@ -12,6 +12,7 @@ import ( "github.com/sigstore/sigstore-go/pkg/root" "github.com/sigstore/sigstore-go/pkg/verify" + "github.com/stacklok/toolhive-core/container/signer" coreverifier "github.com/stacklok/toolhive-core/container/verifier" ) @@ -58,18 +59,72 @@ func (d *Default) VerifyOCIWithKey( if err != nil { return nil, err } + expectedPayload, err := signer.PayloadDigest(imageRef, digest) + if err != nil { + return nil, fmt.Errorf("reconstructing the signed payload for %s: %w", digest, err) + } var lastErr error + boundCandidates := 0 for _, b := range bundles { + // Discovery is by tag — the ".sig" manifest is found by naming it + // after this artifact's digest — so being attached here is not + // evidence of being about this artifact. Only the payload says that. + if !bundleSignsPayload(b, expectedPayload) { + continue + } + boundCandidates++ if _, verifyErr := coreverifier.VerifyBundleWithKey(b, pubKeyPEM); verifyErr != nil { lastErr = verifyErr continue } return resultFromKey(b.Raw), nil } + // Only after every bundle failed: a key-signed bundle that verifies wins + // regardless of what else is attached to the artifact. Reported ahead of + // the generic failure so aiming a key at a keylessly-signed artifact is + // named as the mistake it is rather than reported as a bad signature. + if onlyKeylessSigned(bundles) { + return nil, ErrKeylessSigned + } + if boundCandidates == 0 { + return nil, fmt.Errorf("%w: signature material is attached to this artifact but none of it"+ + " signs this artifact — the signed payload names a different repository or digest", + ErrSignatureInvalid) + } return nil, wrapInvalid(lastErr) } +// bundleSignsPayload reports whether b's signature covers the simple-signing +// payload for the artifact under verification. +// +// core binds a candidate to the digest of the layer the candidate came from +// (RetrieveBundles records it as DigestAlgo/DigestHex), which proves the +// signature covers that blob intact but says nothing about which artifact the +// blob describes. The artifact is named only inside the payload, as a +// repository and a manifest digest, so agreeing with a payload digest +// reconstructed from the requested reference is what ties the two together. +// +// Without this, a valid signature is transplantable: copying artifact A's +// signature layer into "sha256-.sig" makes it discoverable as B's, and it +// still verifies — B is then accepted under whatever key legitimately signed +// A. Comparing digests rather than parsing the payload keeps the check on the +// bytes that were actually signed; a payload edited to name B no longer +// hashes to the layer digest core verified the signature against. +// +// REMOVE THIS when the toolhive-core dependency moves past v0.0.42. +// toolhive-core#263 fixes the same gap at the source and inverts the contract +// this relies on: Bundle.DigestHex becomes the ARTIFACT digest rather than the +// payload digest, so the comparison below turns into artifact-vs-payload and +// can never hold. That fails closed, not open, and the sign-then-verify round +// trip in TestVerifyOCIWithKeyRoundTrip fails with it — so a bump surfaces as +// a red test rather than as silently disabled verification. The fix then is to +// delete this helper and let core's ErrSignatureArtifactMismatch do the work, +// NOT to loosen the comparison. +func bundleSignsPayload(b coreverifier.Bundle, expectedPayload string) bool { + return b.DigestAlgo+":"+b.DigestHex == expectedPayload +} + // verifyKeylessBundles verifies bundles until one passes the keyless policy // and its source-specific provenance expectation, returning its result or // nil with the most useful verification error. @@ -249,6 +304,22 @@ func onlyKeySigned(bundles []coreverifier.Bundle) bool { return true } +// onlyKeylessSigned reports whether every retrieved bundle carries a Fulcio +// certificate — the keyless layout, which no supplied public key can verify. +// The inverse of onlyKeySigned, and an empty slice is excluded for the same +// reason. +func onlyKeylessSigned(bundles []coreverifier.Bundle) bool { + if len(bundles) == 0 { + return false + } + for _, b := range bundles { + if !b.HasCertificate() { + return false + } + } + return true +} + // signerMismatchError builds the ErrSignerMismatch error, naming both the // expected identity tuple and the identity the artifact actually verifies // with (when extractable). diff --git a/pkg/skills/verifier/publickey.go b/pkg/skills/verifier/publickey.go new file mode 100644 index 0000000000..8cf794d428 --- /dev/null +++ b/pkg/skills/verifier/publickey.go @@ -0,0 +1,80 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package verifier + +import ( + "bytes" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "errors" + "fmt" + + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// publicKeyPEMType is the PEM label a cosign public key carries. Cosign's +// private keys use their own labels, which is what makes checking this a +// cheap guard against encoding one by mistake. +const publicKeyPEMType = "PUBLIC KEY" + +// EncodePublicKey converts a cosign public key file's PEM contents into the +// single-line base64 DER SPKI form the API and the lock file carry. The lock +// file's provenance values must be graphic, whitespace-free strings, so PEM's +// armor and line breaks cannot be stored verbatim. +// +// This runs on the client side of the API deliberately: sending the file's +// path instead would name nothing on a server that is a different process, or +// on a different host, a different file entirely. +func EncodePublicKey(pemBytes []byte) (string, error) { + block, rest := pem.Decode(pemBytes) + if block == nil { + return "", errors.New("no PEM block found; expected a cosign public key file (cosign.pub)") + } + if len(bytes.TrimSpace(rest)) > 0 { + return "", errors.New("file contains more than one PEM block; expected exactly one public key") + } + // Checked before parsing, so a private key is refused by its own label + // rather than by whatever a PKIX parse makes of its bytes. The encoded + // result is sent to the server and written to the lock file, and neither + // is somewhere private key material should be able to reach by accident. + if block.Type != publicKeyPEMType { + return "", fmt.Errorf("PEM block is %q, expected %q; this flag takes the cosign public key,"+ + " not the signing key", block.Type, publicKeyPEMType) + } + if _, err := x509.ParsePKIXPublicKey(block.Bytes); err != nil { + return "", fmt.Errorf("not a DER SPKI public key: %w", err) + } + encoded := base64.StdEncoding.EncodeToString(block.Bytes) + // Rejected here rather than at the lock write, where the install has + // already fetched and verified the artifact and the error would arrive + // with nothing left to do about it. + if len(encoded) > lockfile.MaxEncodedPublicKeyLength { + return "", fmt.Errorf("encoded public key is %d characters, exceeding the %d a lock entry accepts", + len(encoded), lockfile.MaxEncodedPublicKeyLength) + } + return encoded, nil +} + +// DecodePublicKey converts the base64 DER SPKI form back into the PEM +// encoding the key verification APIs take. It re-validates rather than +// trusting its input: the value reaches here from an HTTP request body or a +// hand-editable lock file, and it is the only trust anchor the artifact will +// be checked against. +func DecodePublicKey(encoded string) ([]byte, error) { + // Bounded before decoding, so the allocation is capped before it is made + // rather than after. + if len(encoded) > lockfile.MaxEncodedPublicKeyLength { + return nil, fmt.Errorf("public key is %d characters, exceeding the %d maximum", + len(encoded), lockfile.MaxEncodedPublicKeyLength) + } + der, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("public key is not valid base64: %w", err) + } + if _, err := x509.ParsePKIXPublicKey(der); err != nil { + return nil, fmt.Errorf("public key is not a DER SPKI public key: %w", err) + } + return pem.EncodeToMemory(&pem.Block{Type: publicKeyPEMType, Bytes: der}), nil +} diff --git a/pkg/skills/verifier/publickey_test.go b/pkg/skills/verifier/publickey_test.go new file mode 100644 index 0000000000..9c635f64b6 --- /dev/null +++ b/pkg/skills/verifier/publickey_test.go @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package verifier + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "strings" + "testing" + + "github.com/sigstore/sigstore/pkg/cryptoutils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// TestPublicKeyRoundTrip is the contract between the two ends of the wire: the +// CLI encodes a PEM file, the service decodes it back to PEM to verify with. +// A conversion that is not an identity would verify against key material the +// operator never named. +func TestPublicKeyRoundTrip(t *testing.T) { + t.Parallel() + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + pubPEM, err := cryptoutils.MarshalPublicKeyToPEM(priv.Public()) + require.NoError(t, err) + + encoded, err := EncodePublicKey(pubPEM) + require.NoError(t, err) + // The lock file rejects whitespace and non-graphic runes, which is the + // whole reason PEM cannot be stored verbatim. + assert.NotContains(t, encoded, "\n") + assert.Equal(t, strings.TrimSpace(encoded), encoded) + + decoded, err := DecodePublicKey(encoded) + require.NoError(t, err) + assert.Equal(t, string(pubPEM), string(decoded)) +} + +func TestEncodePublicKeyRejections(t *testing.T) { + t.Parallel() + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + privPEM, err := cryptoutils.MarshalPrivateKeyToPEM(priv) + require.NoError(t, err) + pubPEM, err := cryptoutils.MarshalPublicKeyToPEM(priv.Public()) + require.NoError(t, err) + + tests := []struct { + name string + input []byte + wantMsg string + }{ + {name: "empty input", input: nil, wantMsg: "no PEM block found"}, + {name: "not PEM at all", input: []byte("just some text"), wantMsg: "no PEM block found"}, + { + // Sent to the server and written to the lock file, so refusing + // this by the block's own label — before anything parses its + // bytes — is what keeps private material out of both. + name: "private key refused by its label", + input: privPEM, + wantMsg: "takes the cosign public key", + }, + { + // Which of the two is the trust anchor? Guessing would pin one of + // them silently. + name: "two blocks are ambiguous", + input: append(append([]byte{}, pubPEM...), pubPEM...), + wantMsg: "more than one PEM block", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := EncodePublicKey(tc.input) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantMsg) + }) + } +} + +func TestDecodePublicKeyRejections(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + wantMsg string + }{ + {name: "not base64", input: "not!base64", wantMsg: "not valid base64"}, + { + // Decoding proves the encoding, not the content. This is the + // input that would otherwise fail deep inside verification, with + // the lock file no longer the obvious suspect. + name: "base64 of something that is not a key", + input: "aGVsbG8gd29ybGQ=", + wantMsg: "not a DER SPKI public key", + }, + { + // Bounded before decoding, so the allocation is capped before it + // is made rather than after. + name: "over the length bound", + input: strings.Repeat("A", lockfile.MaxEncodedPublicKeyLength+1), + wantMsg: "exceeding the", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := DecodePublicKey(tc.input) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantMsg) + }) + } +} + +// TestDecodePublicKeyAcceptsTheLockFixture ties the shared test constant to +// the real decoder: every key-pinned fixture in the tree depends on this value +// being a genuine SPKI key, not merely valid base64. +func TestDecodePublicKeyAcceptsTheLockFixture(t *testing.T) { + t.Parallel() + + pemBytes, err := DecodePublicKey(testPublicKeyB64) + require.NoError(t, err) + assert.Contains(t, string(pemBytes), "BEGIN PUBLIC KEY") +} diff --git a/pkg/skills/verifier/verifier_test.go b/pkg/skills/verifier/verifier_test.go index 956ece3541..8312b3e3d6 100644 --- a/pkg/skills/verifier/verifier_test.go +++ b/pkg/skills/verifier/verifier_test.go @@ -19,6 +19,9 @@ import ( "github.com/google/go-containerregistry/pkg/registry" "github.com/google/go-containerregistry/pkg/v1/random" "github.com/google/go-containerregistry/pkg/v1/remote" + protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" + protocommon "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1" + sgbundle "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/sigstore/sigstore/pkg/cryptoutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -137,6 +140,83 @@ func TestVerifyOCIWithKeyRejectsWrongKey(t *testing.T) { _, err = NewDefault(nil).VerifyOCIWithKey(t.Context(), ref, digest, otherPub) require.ErrorIs(t, err, ErrSignatureInvalid) + require.NotErrorIs(t, err, ErrKeylessSigned, + "the artifact IS key-signed; the key is simply the wrong one, and telling the caller to"+ + " drop the key would send them to a path that cannot verify it either") +} + +// pushTaggedTestArtifact pushes a random OCI image to the shared test +// repository under tag and returns its digest. Same repository as +// pushTestArtifact deliberately: a transplant within one repository is the +// case a repository-only binding would miss. +func pushTaggedTestArtifact(t *testing.T, host, tag string) string { + t.Helper() + img, err := random.Image(256, 1) + require.NoError(t, err) + parsed, err := name.ParseReference(host + "/test/skill:" + tag) + require.NoError(t, err) + require.NoError(t, remote.Write(parsed, img)) + d, err := img.Digest() + require.NoError(t, err) + return d.String() +} + +// transplantSignature republishes the signature manifest attached to +// fromDigest under the tag the verifier derives from toDigest, forging the +// only link discovery relies on. It needs no key and no write access to the +// signature itself — just the ability to push a tag to the repository. +func transplantSignature(t *testing.T, host, fromDigest, toDigest string) { + t.Helper() + sigTag := func(d string) name.Reference { + r, err := name.ParseReference(host + "/test/skill:" + strings.Replace(d, ":", "-", 1) + ".sig") + require.NoError(t, err) + return r + } + attached, err := remote.Image(sigTag(fromDigest)) + require.NoError(t, err, "the signed artifact must have a discoverable signature manifest to copy") + require.NoError(t, remote.Write(sigTag(toDigest), attached)) +} + +// TestVerifyOCIWithKeyRejectsTransplantedSignature covers the replay a +// digest-unbound key check would accept: the signature is genuine, the key is +// genuinely trusted, and the only false claim is which artifact the signature +// is about. +// +// Signatures are discovered by tag, derived from the digest being verified, so +// a registry-side attacker who can push a tag can make one artifact's +// signature appear to be another's. The signature still verifies — it covers +// its own payload intact — so nothing about the cryptography is disturbed. +// What must reject it is the payload naming the artifact it actually signed. +func TestVerifyOCIWithKeyRejectsTransplantedSignature(t *testing.T) { + t.Parallel() + host := startTestRegistry(t) + signedRef, signedDigest := pushTestArtifact(t, host) + pubPEM, _ := signArtifact(t, signedRef, signedDigest) + + // A second, unsigned artifact — the one the attacker wants accepted. + targetDigest := pushTaggedTestArtifact(t, host, "v2") + require.NotEqual(t, signedDigest, targetDigest) + repoRef := host + "/test/skill" + + // Before the transplant the target is simply unsigned. + _, err := NewDefault(nil).VerifyOCIWithKey(t.Context(), repoRef, targetDigest, pubPEM) + require.ErrorIs(t, err, ErrUnsigned) + + transplantSignature(t, host, signedDigest, targetDigest) + + // The transplanted signature is now discoverable as the target's, and + // verifies against the trusted key on its own terms. + _, err = NewDefault(nil).VerifyOCIWithKey(t.Context(), repoRef, targetDigest, pubPEM) + require.ErrorIs(t, err, ErrSignatureInvalid, + "a signature naming a different artifact must not verify this one") + require.ErrorContains(t, err, "none of it signs this artifact") + require.NotErrorIs(t, err, ErrUnsigned, + "signature material IS present; calling it unsigned would invite an --allow-unsigned override") + + // The signature still verifies for the artifact it was actually made for, + // so the check rejects the false binding rather than the signature. + _, err = NewDefault(nil).VerifyOCIWithKey(t.Context(), repoRef, signedDigest, pubPEM) + require.NoError(t, err) } func TestVerifyOCIUnsignedArtifact(t *testing.T) { @@ -612,3 +692,47 @@ func TestVerifyGitRefusesKeyPinnedEntry(t *testing.T) { require.NotErrorIs(t, err, ErrUnsigned, "refused before the signature checks, so the misrouting is not masked as unsigned") } + +// certBundle is a bundle carrying a signing certificate — the keyless layout. +// Only the verification material is populated: onlyKeylessSigned reads nothing +// else, and a fully-formed keyless bundle would need a live Fulcio. +func certBundle() coreverifier.Bundle { + return coreverifier.Bundle{Parsed: &sgbundle.Bundle{Bundle: &protobundle.Bundle{ + VerificationMaterial: &protobundle.VerificationMaterial{ + Content: &protobundle.VerificationMaterial_Certificate{ + Certificate: &protocommon.X509Certificate{RawBytes: []byte("der")}, + }, + }, + }}} +} + +// TestOnlyKeylessSigned is the mirror of the onlyKeySigned rule: a supplied +// public key cannot verify a Fulcio-certificate signature, and saying so beats +// reporting the intact signature as broken. A mixed artifact is excluded +// because one of its bundles genuinely is key-signed, so the generic failure +// is the honest answer there. +func TestOnlyKeylessSigned(t *testing.T) { + t.Parallel() + + keySigned := coreverifier.Bundle{} + tests := []struct { + name string + bundles []coreverifier.Bundle + want bool + }{ + {name: "no bundles", bundles: nil, want: false}, + {name: "all keyless", bundles: []coreverifier.Bundle{certBundle(), certBundle()}, want: true}, + {name: "all key-signed", bundles: []coreverifier.Bundle{keySigned}, want: false}, + {name: "mixed", bundles: []coreverifier.Bundle{certBundle(), keySigned}, want: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, onlyKeylessSigned(tc.bundles)) + // The two predicates must never both hold: they select opposite + // verification paths, and a bundle set answering yes to both would + // mean whichever is checked first decides the policy. + assert.False(t, onlyKeylessSigned(tc.bundles) && onlyKeySigned(tc.bundles)) + }) + } +}