Skip to content

Add Fulcio/Rekor keyless signing to SignOCI - #235

Open
samuv wants to merge 4 commits into
mainfrom
skills-keyless/c2-fulcio-rekor-sign
Open

Add Fulcio/Rekor keyless signing to SignOCI#235
samuv wants to merge 4 commits into
mainfrom
skills-keyless/c2-fulcio-rekor-sign

Conversation

@samuv

@samuv samuv commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

container/signer.Default.SignOCI could only sign with a file-based cosign key, returning ErrKeyRequired when none was given. That left the certificate-bearing attach and retrieval paths — fixed in #234 (PR C1) — without a producer: nothing in-tree could actually mint a keyless signature, so the layout could only be exercised with hand-built fixtures.

This adds the real keyless path. Part of the keyless push-signing work tracked as stacklok/toolhive#6307.

  • Options.IdentityToken selects the keyless flow. A single-use ephemeral key pair is minted, Fulcio issues a short-lived certificate binding it to the token's identity, and the signature is submitted to Rekor. Acquiring the token (ambient CI credentials, browser flow, device flow) stays entirely the caller's problem — this package never performs OAuth and only forwards the token it is handed.
  • A Rekor entry is mandatory, not optional. container/verifier's own policy for keyless bundles requires WithSignedCertificateTimestamps(1), WithTransparencyLog(1) and WithObserverTimestamps(1), so a bundle with no transparency-log entry would not verify under the policy this repo itself applies. Rekor v1 specifically: v2 issues proof-only entries and needs a separate timestamp authority, and the cosign bundle annotation has no field for a v2 inclusion proof.
  • Options.FulcioURL / Options.RekorURL override the public-good endpoints, defaulted at signing time so a zero-valued Options targets public-good.
  • Both Key and IdentityToken set is an error, not a precedence rule. The two produce trust material verifiers check in materially different ways (a bare public key versus a Fulcio identity plus the transparency log), so resolving it silently would attach a signature the caller cannot verify the way it expects. New sentinel ErrAmbiguousSigningMethod.
  • ErrKeyRequired's message now names both options. Phrased in terms of the Options fields, since this package does not know CLI flag names.

attachCosignSignature and container/verifier are untouched — C1 already handles cert-bearing bundles correctly, and the keyless path calls it with exactly the same arguments as the key path.

Where defaulting happens, and why the URLs are hardcoded

Both documented on Options and the constants. Defaulting is applied inside keylessBundleOptions at signing time. The endpoints are hardcoded rather than derived from a trusted root because reading them means fetching a root (a TUF round trip, or the point-in-time snapshot embedded in container/verifier) purely to learn a URL, and the URI is only reachable by type-asserting root.CertificateAuthority — a single-method Verify interface — to the concrete *root.FulcioCertificateAuthority. More importantly the trusted root is verification material: keying signing endpoints off it would let a trust-root refresh silently redirect where signing requests, and the identity token in them, are sent.

Type of change

  • New feature (non-breaking change which adds functionality)

Changes

File Change
container/signer/signer.go Options.IdentityToken / FulcioURL / RekorURL; signingMaterial + keylessBundleOptions; ErrAmbiguousSigningMethod; updated ErrKeyRequired
container/signer/keyless_test.go In-process Fulcio + CT log + Rekor v1 over httptest, doubling as the verification trust root; keyless tests
go.mod Test-only imports promoted from indirect to direct (certificate-transparency-go, rekor, go-openapi/runtime, swag/conv, json-canonicalization). No new modules, go.sum unchanged

Test plan

  • Unit tests pass (task test, full suite, race detector)
  • Lint passes (task lint, 0 issues)
  • go build ./container/... and go vet ./container/...
  • Existing key-signed tests pass unmodified (TestSignOCIRoundTrip, TestSignOCIEncryptedKey, TestSignOCIAcceptsCosignCLIKeyFormat, the C1 attach tests, …)

This PR is network-free. Real Sigstore-staging E2E coverage is separate follow-on work, not this PR.

How the Fulcio/Rekor interaction is tested without the network

testSigstore is a complete Sigstore deployment running in-process behind httptest servers that speak the actual HTTP contracts sigstore-go's clients drive — POST /api/v2/signingCert and POST /api/v1/log/entries. Servers, not stubs: mocking sigstore-go's CertificateProvider / RekorClient interfaces would have left the part most likely to break — that SignOCI wires the ephemeral key, identity token and endpoint URLs into real requests — untested.

The material it issues is cryptographically genuine, which is what lets the same object also implement root.TrustedMaterial and serve as the verification trust root:

  • Fulcio verifies the caller's proof of possession before issuing. That check only passes if SignOCI signed the token's subject with the very key it asked to have certified.
  • Certificates carry a real embedded SCT. The certificate is issued twice on purpose: an embedded SCT signs the pre-certificate's TBS, so the pre-certificate must exist before the SCT can be computed, and the final certificate is reissued from the same template with the SCT appended.
  • Rekor unmarshals the proposed entry through Rekor's own type registry, which cryptographically validates the submitted signature against the entry's public key and artifact hash exactly as the real log does, then canonicalizes it and signs a real SignedEntryTimestamp and single-leaf checkpoint.

Coverage:

Test What it pins
TestSignOCIKeylessRoundTrip Attach → verifier.RetrieveBundlesHasCertificate() is true
TestSignOCIKeylessBundleVerifies Passes verifier.VerifyBundle with DefaultVerifierOptions() against the test trust root; identity read back and re-pinned; a different identity is rejected
TestSignOCIKeylessRejectedByUnrelatedTrustRoot Guards the above against passing for the wrong reason — a second, independent deployment rejects the bundle
TestSignOCIKeylessAttachesCertificateAndTlogAnnotations Certificate and Rekor bundle annotations, and the SAN/issuer in the cert
TestSignOCIKeylessReSignAppendsForNewIdentity C1's identity dedupe against real Fulcio-issued certs: same identity dedupes, different identity appends
TestSignOCIKeylessSurfacesFulcioFailure A rejected token fails signing rather than falling back to another layout
TestSignOCIRejectsAmbiguousSigningMethod, TestErrKeyRequiredNamesBothSigningMethods The two error paths

I mutation-tested the two assertions that could quietly stop covering anything, and confirmed both fail when the fake stops doing its job: dropping the SCT extension yields only able to verify 0 SCT entries; unable to meet threshold of 1, and corrupting the SignedEntryTimestamp yields not enough verified log entries from transparency log: 0 < 1.

Special notes for reviewers

  • The go.mod churn is only indirect → direct promotion for test-only imports; go.sum is unchanged.
  • sign.BundleOptions.TrustedRoot is deliberately left unset, so sign.Bundle does not verify the bundle it just produced. It would need a trusted root for the target deployment, which this package cannot scope to a caller's custom FulcioURL/RekorURL without a TUF fetch. Verification stays the caller's step through container/verifier.
  • The fixture identity token is unsigned on purpose. Real Fulcio verifies the token against its issuer; sigstore-go only base64-decodes the claims to recover the subject. Signing the fixture would imply a check that does not happen here.

Generated with Claude Code

rdimitrov
rdimitrov previously approved these changes Aug 13, 2026
samuv and others added 3 commits August 14, 2026 12:08
SignOCI could only sign with a file-based cosign key, hard-failing
without one, so the certificate-bearing attach and retrieval paths had
no producer: nothing in-tree could actually mint a keyless signature.

Options now takes an OIDC identity token, which selects the keyless
flow: a single-use ephemeral key pair, a Fulcio certificate binding it
to the token's identity, and a Rekor v1 transparency-log entry. The
entry is mandatory rather than optional, because the verification
policy container/verifier applies to keyless bundles requires one.
FulcioURL and RekorURL override the public-good endpoints, defaulted at
signing time. Supplying both a key and a token is rejected instead of
resolved by precedence: the two produce trust material verifiers check
in materially different ways, so choosing silently would attach a
signature the caller cannot verify the way it expects.

Tests stand up a complete in-process Sigstore deployment over httptest
— Fulcio, a Certificate Transparency log, and Rekor v1 — speaking the
real HTTP APIs, so no test reaches the public-good or staging
instances. Its material is cryptographically genuine, which lets the
same deployment serve as the verification trust root: a bundle it signs
is checked end to end under DefaultVerifierOptions, embedded SCT and
signed entry timestamp included, rather than merely inspected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Re-signing with an identity that already signed the artifact made
attachCosignSignature a no-op, but SignOCI still returned the freshly
(and never attached) built bundle — every signing operation is
randomised, so its signature did not match the sole registry layer,
breaking the "one signature, two representations" contract from the
dedupe side. SignOCI now retrieves and returns the bundle actually on
the registry when nothing was appended.

Bundle validation also ran after attachCosignSignature, so a
malformed-but-parseable Fulcio/Rekor response could mutate the
registry and only then fail, returning an error despite having
already signed. Validation now runs first.

Separately, sigstore-go's Fulcio client builds its HTTP request
without attaching the caller's context, so a canceled SignOCI
previously stayed blocked until the client's own timeout; and
sigstore/rekor's response parsing dereferences several
Verification/InclusionProof fields unconditionally, so a
well-formed-but-incomplete Rekor response reliably panics the process.
Both are guarded by running sign.Bundle on its own goroutine and
racing it against ctx.Done(), with a recover() converting a panic into
a returned error instead of taking down a long-lived signing process.
bundleMatchesSigner's keyless branch matched on certificate identity
(SAN + OIDC issuer) alone, unlike attach's own dedupe check
(signedByIdentity), which treats identity as a candidate rather than
a verdict and additionally requires the layer's signature to verify.
A same-identity layer whose signature doesn't verify against the
current payload — corrupt, or signed over a different one — could
therefore be selected ahead of a genuinely valid layer when
RetrieveBundles returns it first, making SignOCI return a bundle
whose signature does not match its own PayloadDigest.

Also documents that Result.Bundle's JSON shape differs by path (a
fresh attach serializes as bundle v0.3, a dedupe match is the v0.1
reconstruction RetrieveBundles builds from OCI annotations) and that
IdentityToken is sent to FulcioURL in the clear over a non-HTTPS
endpoint.
@samuv
samuv force-pushed the skills-keyless/c2-fulcio-rekor-sign branch from 72c783d to 697e3d4 Compare August 14, 2026 10:08

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel review (spec vs stacklok/toolhive#6307, repo standards, six specialist reviewers — every technical claim verified against the pinned deps on disk: sigstore-go v1.3.0, sigstore v1.10.9, rekor v1.5.3).

Spec axis — clean within this PR's claimed scope. Keyless flow, ErrAmbiguousSigningMethod, signing-time defaulting, mandatory Rekor v1 entry (verified: container/verifier requires SCT=1/tlog=1/observer=1), unchanged key path — all confirmed against the diff. Still open against #6307 (correctly deferred, not done here): CLI/push-API credential plumbing, Sigstore-staging E2E, stock cosign verify interop.

Standards — CLAUDE.md Boundaries says never modify go.mod dependencies without being asked; the 5 indirect→direct promotions are test-only and correct (go.sum unchanged), but please confirm an approver is fine with them. Also: table-driven tests for new functions — TestKeylessBundleOptionsDefaultToPublicGood is the clear candidate.

Domain — the likely blocking finding: keyless dedupe verifies the layer's signature against the key in its own cert, never chaining that cert to the configured Fulcio root (inline on cosign_attach.go). A registry writer can preload a self-signed same-identity layer, making signing a silent no-op that returns attacker material as success. Also inline: no HTTPS/loopback validation on FulcioURL/RekorURL (bearer token; networking.ValidateEndpointURL exists); Fulcio-vs-Rekor failure stage unidentifiable in errors; goroutine+recover wrapper scoped wider than the two verified upstream defects (pkg/sign/certificate.go:175-207, pkg/tle/tle.go:33-44 — claims confirmed).

Verified non-findings: hardcoded-endpoint rationale holds (root.CertificateAuthority exposes only Verify); Rekor-v1-only holds (v2 needs a TSA per pkg/sign/signer.go:171-189); no IdentityToken leakage into local errors; unsigned-JWT parsing is test-only; v0.3/v0.1 Result.Bundle shape variance round-trips through container/verifier (pinned by TestSignOCIKeylessRoundTrip).

Comment on lines +453 to +464
return sign.BundleOptions{
Context: ctx,
CertificateProvider: sign.NewFulcio(&sign.FulcioOptions{
BaseURL: fulcioURL,
Retries: keylessRetries,
}),
CertificateProviderOptions: &sign.CertificateProviderOptions{IDToken: opts.IdentityToken},
TransparencyLogs: []sign.Transparency{
sign.NewRekor(&sign.RekorOptions{
BaseURL: rekorURL,
Retries: keylessRetries,
Version: rekorAPIVersionV1,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High — cross-confirmed] No scheme validation before the bearer token goes out. opts.FulcioURL flows into sign.NewFulcio unchecked; sigstore-go v1.3.0 attaches Authorization: Bearer <IdentityToken> at pkg/sign/certificate.go:178-187 for any URL — so FulcioURL: "http://…" (non-loopback) sends the OIDC token in the clear (CWE-319 / OWASP A02:2021). The caveat is documented on IdentityToken's godoc, but someone configuring the URL reads that field's docs, which don't mention it.

Related (verified): the defaults apply independently, so setting only FulcioURL pairs a private Fulcio with public Rekor, producing a bundle that verifies under no standard trust root — nothing rejects or documents that pairing.

Suggest validating both endpoints here after defaulting — the repo already has networking.ValidateEndpointURL (networking/utilities.go:186) — requiring HTTPS except literal loopback (which your tests need). keylessBundleOptions would return an error; a table-driven test could cover http/file/missing-host vs HTTPS/loopback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: keylessBundleOptions now validates both FulcioURL and RekorURL with networking.ValidateEndpointURL (HTTPS required except loopback) before either is used to build sign.BundleOptions, so a non-loopback plain-HTTP endpoint is rejected before the bearer token is ever attached to a request. Regression test: TestKeylessBundleOptionsRejectsInsecureEndpoint. Also moved the caveat onto FulcioURL/RekorURL's own doc comments instead of leaving it only on IdentityToken's.

return false, fmt.Errorf("pushing signature manifest: %w", err)
}
return nil
return true, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High — likely the blocking one] The keyless dedupe path (just above, in signedByIdentity) verifies the layer's signature against the key inside the layer's own certificate — but nothing checks that certificate chains to the configured Fulcio root (CWE-345). A registry writer can mint a self-signed cert carrying the victim's SAN and OIDC-issuer extension (both public by design), sign the known simple-signing payload, and push it as a .sig layer. Dedupe then accepts it, attach is skipped, and bundleMatchesSigner (signer.go:373-404) repeats the same untrusted checks to return the attacker's bundle as a successful Result. Downstream verification still rejects the self-signed cert, so it isn't a forgery — but SignOCI reports success while the legitimate Fulcio-backed signature is silently never attached (signing-pipeline bypass / DoS).

Options: verify a candidate's full bundle against trusted material before accepting the no-op, or conservatively always append on the keyless path. Test: preload the tag with a self-signed same-identity layer that signature-verifies, assert SignOCI still appends.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: dedupe for cert-bearing layers now verifies the full chain of trust via verifier.VerifyBundle/root.TrustedMaterial before comparing SAN+issuer, rather than trusting a self-consistent signature over the layer's own (attacker-controlled) certificate. New shared helper keylessLayerTrusted (used by both attach's dedupe and bundleMatchesSigner) does: (1) confirm digest match, (2) run full bundle verification against the configured trust material, (3) only then compare the verified certificate's SAN/issuer. A self-signed cert with a matching SAN/issuer can no longer pass — TestAttachCosignSignatureCertDedupeRequiresChainOfTrust is the regression test (mutation-verified: reverting the chain check to always-true makes it fail). In production this verifies against verifier.OfflineTrustedMaterial() (the embedded production Sigstore root); tests inject an in-process CA as trust material via a new newDefaultForTest constructor, since they can't chain to the real production root.

Comment on lines +447 to +456
fulcioURL = DefaultFulcioURL
}
rekorURL := opts.RekorURL
if rekorURL == "" {
rekorURL = DefaultRekorURL
}
return sign.BundleOptions{
Context: ctx,
CertificateProvider: sign.NewFulcio(&sign.FulcioOptions{
BaseURL: fulcioURL,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium — cross-confirmed] This wrapper is scoped wider than the two verified upstream defects. I checked both claims and they're accurate: sigstore-go's Fulcio client builds its request with http.NewRequest (caller ctx never attached; default 30s client timeout — pkg/sign/certificate.go:175-207), and rekor nil-derefs Verification.InclusionProof (pkg/tle/tle.go:33-44,88-96). But this now wraps every sign.Bundle call, including file-key signing, so an unrelated panic in payload signing / keypair code / bundle assembly is reclassified as a Fulcio-or-Rekor network error with the stack trace discarded. And the ctx race abandons the Fulcio request until its own timeout rather than fixing cancellation.

Suggest narrowing: a RoundTripper that clones each request onto the caller's context for the Fulcio transport; panic-recover only around the Rekor adapter; apply the wrapper only on the keyless path; and file both defects upstream so this can be deleted when fixed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: split signBundle into a synchronous key-signed path (no goroutine, no recover — a panic there is this package's own bug and should propagate as one) and a keyless-only keylessSignBundle that keeps the goroutine+recover+ctx-race workaround scoped to the actual untrusted-network-response problem it exists for.

TransparencyLogs: []sign.Transparency{
sign.NewRekor(&sign.RekorOptions{
BaseURL: rekorURL,
Retries: keylessRetries,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High — cross-confirmed] The Fulcio-vs-Rekor failure stage is unidentifiable from these errors. "building sigstore bundle: %w" only relays sigstore-go's text (unstable dependency wording), and the recover path says "a Fulcio or Rekor response could not be processed" — an operator can't distinguish token-rejected, Fulcio-unreachable, Rekor-down, or malformed-response without guessing, and a consumer can't branch programmatically on the stage. Suggest wrapping with stage context (requesting signing certificate from Fulcio (%s): %w / submitting to Rekor (%s): %w) — and if applicable, exported stage sentinels or a typed operation error so errors.Is/errors.As works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: keylessSignBundle now calls opts.CertificateProvider.GetCertificate directly first, tagging any failure as "requesting signing certificate from Fulcio: %w", then hands sign.Bundle the already-fetched cert via a cachedCertificate provider so its internal call is a cache hit rather than a second Fulcio round trip — leaving Rekor submission as the only remaining fallible step, tagged "submitting to Rekor: %w". Verified safe by reading sign.Bundle's source for this project's specific BundleOptions (no TimestampAuthorities, no TrustedRoot) to confirm no other fallible step exists between cert-acquisition and Rekor submission. TestSignOCIKeylessSurfacesFulcioFailure and the new TestSignOCIKeylessSurfacesRekorFailure each assert the error names the correct stage and not the other.

Comment thread container/signer/signer.go Outdated
Comment on lines +113 to +115
// tests) endpoint sends it in the clear.
IdentityToken string
// FulcioURL overrides the certificate authority for keyless signing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] "zero Options value targets the public-good deployment" misstates the zero-value contract — bare Options{} fails with ErrKeyRequired; public-good defaulting only happens when IdentityToken is set and the URLs are empty. Suggest: "Empty means DefaultFulcioURL when keyless signing is selected" (same for RekorURL below). Also worth stating on SignOCI/Options that keyless performs outbound network egress to public-good services by default — a posture change from "no key ⇒ always error" that air-gapped consumers should see before runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the doc comment — it now says empty FulcioURL/RekorURL default to the public-good instances only when keyless signing is selected (IdentityToken set), not that a bare zero-value Options{} targets them (that still fails with ErrKeyRequired). Also added the outbound-egress-by-default note.

Comment thread container/signer/signer.go Outdated
Comment on lines +305 to +351
keychain authn.Keychain,
ref, digestStr string,
payload []byte,
pb *protobundle.Bundle,
pub crypto.PublicKey,
attached bool,
) ([]byte, error) {
if attached {
bun, err := verifybundle.NewBundle(pb)
if err != nil {
return nil, fmt.Errorf("finalizing sigstore bundle: %w", err)
}
raw, err := bun.MarshalJSON()
if err != nil {
return nil, fmt.Errorf("serializing sigstore bundle: %w", err)
}
return raw, nil
}
return previouslyAttachedBundleJSON(ctx, keychain, ref, digestStr, payload, pb, pub)
}

// previouslyAttachedBundleJSON retrieves the signature layer that made this
// signing operation a no-op and returns its serialized bundle — the one
// genuinely on the registry, as opposed to the freshly (but never attached)
// built pb.
func previouslyAttachedBundleJSON(
ctx context.Context,
keychain authn.Keychain,
ref, digestStr string,
payload []byte,
pb *protobundle.Bundle,
pub crypto.PublicKey,
) ([]byte, error) {
full := ref
if !strings.Contains(ref, "@") {
full = ref + "@" + digestStr
}
bundles, err := verifier.RetrieveBundles(ctx, full, keychain)
if err != nil {
return nil, fmt.Errorf("retrieving previously attached signature: %w", err)
}
cert, err := certMaterialFromBundle(pb)
if err != nil {
return nil, err
}
for _, b := range bundles {
if bundleMatchesSigner(b, payload, cert, pub) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] The dedupe round-trip could return the matched bundle instead of rediscovering it. attachCosignSignature found the matching layer and verified it, but returns only a bool; this path then re-reads the registry through verifier.RetrieveBundles and re-runs identity+signature matching below. Two costs: a second registry read against state that may have changed since attach's read, and signer now imports container/verifier in production code — coupling write-side to the full verification discovery API (a verifier-side discovery change can break signing). Prefer returning the matched layer/bundle (or descriptor) from the attach side; if a shared codec for the cosign annotations is needed between the two packages, keep it narrow and internal. Layering discussion — flagging as judgement, not mechanical.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left this as-is for now — happy to take it as a follow-up if you'd rather have it in this PR. Two things about it: pb's signature can't be reused directly regardless of what attach returns, since every signing operation is randomized (fresh ephemeral key + Fulcio cert for keyless, ECDSA's own nonce for key re-sign) — that's the existing comment on resultBundleJSON. What your suggestion would actually save is the second verifier.RetrieveBundles registry read: keylessAlreadySigned already walks the bundles looking for a match, so it could return the matched verifier.Bundle instead of a bool, and previouslyAttachedBundleJSON could reuse it instead of re-listing. I didn't make that change here to keep this pass scoped to the chain-of-trust fix and the other findings above; the container/verifier import stays either way since dedupe itself needs to verify against it.

Comment thread container/signer/signer.go Outdated
Comment on lines +373 to +404
func bundleMatchesSigner(b verifier.Bundle, payload []byte, cert *certMaterial, pub crypto.PublicKey) bool {
if b.Parsed == nil {
return false
}
msgSig := b.Parsed.GetMessageSignature()
if msgSig == nil || len(msgSig.GetSignature()) == 0 {
return false
}
if cert != nil {
existing, err := certMaterialFromBundle(b.Parsed.Bundle)
if err != nil || existing == nil {
return false
}
if existing.summary.SubjectAlternativeName != cert.summary.SubjectAlternativeName ||
existing.summary.Issuer != cert.summary.Issuer {
return false
}
existingCert, err := x509.ParseCertificate(existing.certDER)
if err != nil {
return false
}
sigVerifier, err := signature.LoadVerifier(existingCert.PublicKey, crypto.SHA256)
if err != nil {
return false
}
return sigVerifier.VerifySignature(bytes.NewReader(msgSig.GetSignature()), bytes.NewReader(payload)) == nil
}
sigVerifier, err := signature.LoadVerifier(pub, crypto.SHA256)
if err != nil {
return false
}
return sigVerifier.VerifySignature(bytes.NewReader(msgSig.GetSignature()), bytes.NewReader(payload)) == nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Identity-match + cert-signature verification now lives in two places — here in bundleMatchesSigner and in signedByIdentity (cosign_attach.go). They must stay in lockstep (the comment above this func says it "mirrors" the attach side) — one shared helper would make the next change to dedupe semantics single-site. Low only because it's two sites today (Rule of Three).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's resolved as a side effect of the chain-of-trust fix: keylessLayerTrusted is now the single shared helper for cert-bearing identity+chain verification, called from both keylessAlreadySigned (cosign_attach.go) and bundleMatchesSigner (signer.go). The two call sites still exist because they're matching against different things (attach matches against the registry's existing layers; the dedupe-result path re-derives the same match after attach reports a no-op), but the verification logic itself is single-site now.

NotAfter: now.Add(time.Hour),
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
BasicConstraintsValid: true,
IsCA: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Info] Verified reuse opportunity: the pinned sigstore-go@v1.3.0 ships pkg/testing/ca.GenerateRootCa() and GenerateFulcioIntermediate (pkg/testing/ca/ca.go:814,841) — the ~40-line root/intermediate fixture in newTestSigstore could delegate to those. Separately, claimsFromToken's manual split/base64/JSON decode could be jwt.NewParser().ParseUnverified with jwt.RegisteredClaims (golang-jwt/v5 is already a direct dep). Test-only, take or leave.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left as-is — noted, and it's a legitimate simplification, but swapping the fixture for ca.GenerateRootCa()/jwt.NewParser in the same pass as the chain-of-trust rework risked masking whether a test failure came from the fixture change or the actual fix. Fine as a separate cleanup follow-up if you'd like it filed as an issue.

Verify the full Sigstore chain of trust before treating a cert-bearing
layer as a dedupe match, instead of comparing certificate SAN and
issuer alone: those fields live inside the certificate but are public,
attacker-controlled input, so a self-signed cert with a matching SAN
and issuer could previously be mistaken for a genuinely signed layer.
Dedupe now calls into the existing verifier to confirm the certificate
chains to a trusted root before comparing identity.

Split keyless signing into distinct Fulcio and Rekor stages so errors
say which one failed, instead of a single generic "building sigstore
bundle" wrapper. Fulcio's certificate is now fetched once directly and
handed to sign.Bundle through a small cache so its internal call is a
no-op, keeping Rekor submission as the only remaining fallible step.

Narrow the context-cancellation race and panic recovery to only the
keyless path, since the key-signed path has no network calls to race
or untrusted responses to guard against.

Validate FulcioURL and RekorURL with the shared HTTPS/loopback check
used elsewhere in this module, rejecting plain-HTTP endpoints outside
loopback rather than sending signing material over an unencrypted
connection.

Add a per-instance trust-material injection seam (newDefaultForTest)
so tests can exercise real chain-of-trust verification against an
in-process CA instead of the production Sigstore root, without a
global variable that would be unsafe under parallel tests.
@samuv

samuv commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the domain findings from the panel review (replies inline on each):

  • Keyless dedupe now verifies chain of trust (the likely-blocking finding) instead of trusting SAN+issuer over a self-consistent signature — a self-signed same-identity cert can no longer be accepted as an existing signature.
  • FulcioURL/RekorURL scheme validation via the shared networking.ValidateEndpointURL (HTTPS required except loopback), so the bearer token never goes out over plain HTTP.
  • Fulcio vs. Rekor failure stage is now identifiable in errors (cert-acquisition is fetched directly and cached into sign.Bundle, isolating Rekor submission as the only remaining fallible step).
  • recover()/ctx-race wrapper narrowed to the keyless path only — the key-signed path is now a plain synchronous call.
  • Corrected the Options.FulcioURL/IdentityToken doc comments' zero-value contract.

On the two "Standards" notes in the review body:

  • The 5 indirect→direct go.mod promotions are unchanged by this round of fixes — still test-only (driven by keyless_test.go's direct imports), go.sum is untouched. Flagging again for an approver's explicit sign-off per CLAUDE.md's dependency-change boundary, since I can't self-approve that.
  • Didn't convert TestKeylessBundleOptionsDefaultToPublicGood to table-driven — it asserts several distinct properties of one configuration rather than varying inputs, so a table wouldn't reduce duplication there. TestKeylessBundleOptionsRejectsInsecureEndpoint (new) does exercise multiple input variations sequentially; happy to make that one tabular if you'd prefer.

Also note: the branch was rebased onto main after the review to pick up the golang.org/x/mod CVE fix (#236), so the diff base moved — the content at the review's commit (697e3d4) and this branch's current pre-fix commit are identical, only the fixes above are new.

Verification: go build/go vet clean, task lint 0 issues, container/signer full suite with -race green, and task test (whole module) green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants