From 67db6bec29cb4f792ab6eead79b11b08c34637ba Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 13 Aug 2026 18:28:15 +0200 Subject: [PATCH 1/4] Add Fulcio/Rekor keyless signing to SignOCI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- container/signer/keyless_test.go | 838 +++++++++++++++++++++++++++++++ container/signer/signer.go | 186 ++++++- go.mod | 10 +- 3 files changed, 1008 insertions(+), 26 deletions(-) create mode 100644 container/signer/keyless_test.go diff --git a/container/signer/keyless_test.go b/container/signer/keyless_test.go new file mode 100644 index 0000000..3b94c07 --- /dev/null +++ b/container/signer/keyless_test.go @@ -0,0 +1,838 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package signer + +import ( + "bytes" + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" + "encoding/base64" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "math/big" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/cyberphone/json-canonicalization/go/src/webpki.org/jsoncanonicalizer" + "github.com/go-openapi/runtime" + "github.com/go-openapi/swag/conv" + ct "github.com/google/certificate-transparency-go" + cttls "github.com/google/certificate-transparency-go/tls" + ctx509 "github.com/google/certificate-transparency-go/x509" + ctx509util "github.com/google/certificate-transparency-go/x509util" + "github.com/google/go-containerregistry/pkg/registry" + "github.com/sigstore/rekor/pkg/generated/models" + "github.com/sigstore/rekor/pkg/types" + // Blank import registers the Rekor entry type the fake log unmarshals + // and canonicalizes. + _ "github.com/sigstore/rekor/pkg/types/hashedrekord/v0.0.1" + rekorutil "github.com/sigstore/rekor/pkg/util" + fulciocert "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/sigstore/sigstore-go/pkg/root" + "github.com/sigstore/sigstore-go/pkg/tlog" + "github.com/sigstore/sigstore/pkg/cryptoutils" + "github.com/sigstore/sigstore/pkg/signature" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/container/verifier" +) + +// Identity fixtures for the keyless tests: the OIDC claims the fake Fulcio +// stamps into the certificates it issues. +const ( + testKeylessSAN = "signer@example.com" + testKeylessIssuer = "https://oidc.example.com" +) + +// testSigstore is a complete Sigstore deployment running in-process: a Fulcio +// certificate authority (root + intermediate), a Certificate Transparency log, +// and a Rekor v1 transparency log, exposed over httptest servers speaking the +// real HTTP APIs sigstore-go's sign.Fulcio and sign.Rekor clients drive. +// +// It exists because these are unit tests: they must never reach the public-good +// or staging Sigstore instances. Mocking sigstore-go's own client interfaces +// instead would leave the part most likely to break — that SignOCI wires the +// ephemeral key, identity token, and endpoint URLs into real requests — untested. +// So the fakes are servers, not stubs, and the material they produce is +// cryptographically genuine: certificates chain to the test root, carry a real +// embedded SCT, and Rekor entries carry a real SignedEntryTimestamp. That is +// what lets the same object serve as the verification trust root +// (testSigstore implements root.TrustedMaterial), so a bundle this deployment +// signs is verified end to end under the project's real policy rather than +// merely inspected. +type testSigstore struct { + rootCert *x509.Certificate + intermediateCert *x509.Certificate + intermediateKey *ecdsa.PrivateKey + ctlogKey *ecdsa.PrivateKey + rekorKey *ecdsa.PrivateKey + + fulcioURL string + rekorURL string + + // serial hands out unique certificate serial numbers, so signing the same + // artifact twice yields distinguishable certificates as real Fulcio does. + serial atomic.Int64 + // logIndex hands out increasing Rekor log indices. + logIndex atomic.Int64 +} + +var _ root.TrustedMaterial = (*testSigstore)(nil) + +// newTestSigstore mints the deployment's keys and starts its Fulcio and Rekor +// servers, registering cleanup on t. +func newTestSigstore(t *testing.T) *testSigstore { + t.Helper() + + rootKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + now := time.Now() + rootTmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "toolhive-test-sigstore-root"}, + NotBefore: now.Add(-time.Hour), + NotAfter: now.Add(time.Hour), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + BasicConstraintsValid: true, + IsCA: true, + } + rootCert := createTestCertificate(t, rootTmpl, rootTmpl, &rootKey.PublicKey, rootKey) + + // Fulcio issues from an intermediate, and the SCT verification path needs + // the leaf's issuer in the chain, so the test CA mirrors that shape rather + // than signing leaves directly off the root. + intermediateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + intermediateTmpl := &x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: pkix.Name{CommonName: "toolhive-test-fulcio-intermediate"}, + NotBefore: now.Add(-time.Hour), + NotAfter: now.Add(time.Hour), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + BasicConstraintsValid: true, + IsCA: true, + } + intermediateCert := createTestCertificate(t, intermediateTmpl, rootCert, &intermediateKey.PublicKey, rootKey) + + ctlogKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + rekorKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + s := &testSigstore{ + rootCert: rootCert, + intermediateCert: intermediateCert, + intermediateKey: intermediateKey, + ctlogKey: ctlogKey, + rekorKey: rekorKey, + } + + fulcioMux := http.NewServeMux() + fulcioMux.HandleFunc("POST /api/v2/signingCert", s.handleSigningCert) + fulcio := httptest.NewServer(fulcioMux) + t.Cleanup(fulcio.Close) + s.fulcioURL = fulcio.URL + + rekorMux := http.NewServeMux() + rekorMux.HandleFunc("POST /api/v1/log/entries", s.handleCreateLogEntry) + rekor := httptest.NewServer(rekorMux) + t.Cleanup(rekor.Close) + s.rekorURL = rekor.URL + + return s +} + +func createTestCertificate( + t *testing.T, tmpl, parent *x509.Certificate, pub any, parentKey crypto.Signer, +) *x509.Certificate { + t.Helper() + der, err := x509.CreateCertificate(rand.Reader, tmpl, parent, pub, parentKey) + require.NoError(t, err) + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + return cert +} + +// identityToken builds a syntactically valid, unsigned OIDC ID token carrying +// the fixture identity. Nothing verifies its signature: real Fulcio does that, +// and sigstore-go only base64-decodes the claims to recover the subject it +// proves possession of. An unsigned token is therefore the honest fixture — +// signing it would imply a check that does not happen here. +func identityToken(t *testing.T, subject string) string { + t.Helper() + enc := func(v any) string { + raw, err := json.Marshal(v) + require.NoError(t, err) + return base64.RawURLEncoding.EncodeToString(raw) + } + header := enc(map[string]string{"alg": "none", "typ": "JWT"}) + claims := enc(map[string]any{ + "iss": testKeylessIssuer, + "sub": subject, + "email": subject, + "email_verified": true, + "aud": "sigstore", + "exp": time.Now().Add(time.Hour).Unix(), + }) + return header + "." + claims + ".unsigned" +} + +// claimsFromToken recovers the subject and issuer from an ID token's claims, +// standing in for the verification real Fulcio performs against the issuer. +func claimsFromToken(token string) (subject, issuer string, err error) { + parts := strings.Split(token, ".") + if len(parts) < 2 { + return "", "", fmt.Errorf("identity token is not a JWT") + } + raw, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return "", "", fmt.Errorf("decoding identity token claims: %w", err) + } + var claims struct { + Issuer string `json:"iss"` + Subject string `json:"sub"` + Email string `json:"email"` + } + if err := json.Unmarshal(raw, &claims); err != nil { + return "", "", fmt.Errorf("parsing identity token claims: %w", err) + } + subject = claims.Email + if subject == "" { + subject = claims.Subject + } + if subject == "" || claims.Issuer == "" { + return "", "", fmt.Errorf("identity token carries no subject or issuer") + } + return subject, claims.Issuer, nil +} + +// fulcioSigningCertRequest is Fulcio's v2 signing-certificate request body, as +// sign.Fulcio serializes it. +type fulcioSigningCertRequest struct { + PublicKeyRequest struct { + PublicKey struct { + Algorithm string `json:"algorithm"` + Content string `json:"content"` + } `json:"publicKey"` + ProofOfPossession string `json:"proofOfPossession"` + } `json:"publicKeyRequest"` +} + +// handleSigningCert implements Fulcio's POST /api/v2/signingCert: it +// authenticates the bearer identity token, checks the caller's proof of +// possession of the submitted public key, and returns a certificate chain whose +// leaf binds that key to the token's identity. +// +// The proof-of-possession check is the point of doing this over HTTP at all: it +// only passes if SignOCI signed the token's subject with the very key it asked +// to have certified, which is exactly the wiring a stub could not exercise. +func (s *testSigstore) handleSigningCert(w http.ResponseWriter, r *http.Request) { + token := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + if token == "" { + http.Error(w, "missing bearer identity token", http.StatusUnauthorized) + return + } + subject, issuer, err := claimsFromToken(token) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + var req fulcioSigningCertRequest + if err := json.NewDecoder(io.LimitReader(r.Body, 1<<20)).Decode(&req); err != nil { + http.Error(w, "malformed signing certificate request", http.StatusBadRequest) + return + } + pub, err := cryptoutils.UnmarshalPEMToPublicKey([]byte(req.PublicKeyRequest.PublicKey.Content)) + if err != nil { + http.Error(w, "unparseable public key", http.StatusBadRequest) + return + } + pop, err := base64.StdEncoding.DecodeString(req.PublicKeyRequest.ProofOfPossession) + if err != nil { + http.Error(w, "unparseable proof of possession", http.StatusBadRequest) + return + } + popVerifier, err := signature.LoadVerifier(pub, crypto.SHA256) + if err != nil { + http.Error(w, "unusable public key", http.StatusBadRequest) + return + } + if err := popVerifier.VerifySignature(bytes.NewReader(pop), bytes.NewReader([]byte(subject))); err != nil { + http.Error(w, "proof of possession does not verify", http.StatusBadRequest) + return + } + + leafDER, err := s.issueLeafCertificate(pub, subject, issuer) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + chain := make([]string, 0, 3) + for _, der := range [][]byte{leafDER, s.intermediateCert.Raw, s.rootCert.Raw} { + chain = append(chain, certificateAnnotation(der)) + } + // Fulcio's v2 signingCert returns 200, not 201, and sign.Fulcio treats + // anything else as a failure. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "signedCertificateEmbeddedSct": map[string]any{ + "chain": map[string]any{"certificates": chain}, + }, + }) +} + +// issueLeafCertificate issues a Fulcio-shaped code-signing certificate for pub: +// the subject in a SAN, the OIDC issuer in the Fulcio issuer extension, and a +// Certificate Transparency SCT embedded in the certificate. +// +// The certificate is created twice on purpose. An embedded SCT signs the +// PRE-certificate's TBS — the certificate as it would be without the SCT +// extension — so the pre-certificate has to exist before the SCT can be +// computed, and the final certificate is then reissued from the same template +// with the SCT appended. A verifier reverses this by stripping the SCT +// extension back off, which recovers byte-identical TBS bytes only because +// nothing else about the template changed between the two issuances. +func (s *testSigstore) issueLeafCertificate(pub crypto.PublicKey, subject, issuer string) ([]byte, error) { + issuerExt, err := asn1.Marshal(issuer) + if err != nil { + return nil, fmt.Errorf("encoding OIDC issuer extension: %w", err) + } + now := time.Now() + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(s.serial.Add(1)), + EmailAddresses: []string{subject}, + NotBefore: now.Add(-time.Minute), + NotAfter: now.Add(10 * time.Minute), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + ExtraExtensions: []pkix.Extension{ + {Id: fulciocert.OIDIssuerV2, Value: issuerExt}, + }, + } + + precertDER, err := x509.CreateCertificate(rand.Reader, tmpl, s.intermediateCert, pub, s.intermediateKey) + if err != nil { + return nil, fmt.Errorf("issuing pre-certificate: %w", err) + } + precert, err := x509.ParseCertificate(precertDER) + if err != nil { + return nil, fmt.Errorf("parsing pre-certificate: %w", err) + } + + sctExt, err := s.signedCertificateTimestampExtension(precert) + if err != nil { + return nil, err + } + tmpl.ExtraExtensions = append(tmpl.ExtraExtensions, *sctExt) + + leafDER, err := x509.CreateCertificate(rand.Reader, tmpl, s.intermediateCert, pub, s.intermediateKey) + if err != nil { + return nil, fmt.Errorf("issuing signing certificate: %w", err) + } + return leafDER, nil +} + +// signedCertificateTimestampExtension logs precert to the test CT log and +// returns the SCT list extension to embed in the final certificate, in the +// RFC 6962 §3.3 encoding sigstore-go's SCT verification parses back. +func (s *testSigstore) signedCertificateTimestampExtension(precert *x509.Certificate) (*pkix.Extension, error) { + logID, err := testLogID(s.ctlogKey.Public()) + if err != nil { + return nil, err + } + timestamp := uint64(time.Now().UnixMilli()) //nolint:gosec // test timestamp, always positive + sct := ct.SignedCertificateTimestamp{ + SCTVersion: ct.V1, + LogID: ct.LogID{KeyID: logID}, + Timestamp: timestamp, + } + input, err := ct.SerializeSCTSignatureInput(sct, ct.LogEntry{ + Leaf: ct.MerkleTreeLeaf{ + Version: ct.V1, + LeafType: ct.TimestampedEntryLeafType, + TimestampedEntry: &ct.TimestampedEntry{ + Timestamp: timestamp, + EntryType: ct.PrecertLogEntryType, + PrecertEntry: &ct.PreCert{ + IssuerKeyHash: sha256.Sum256(s.intermediateCert.RawSubjectPublicKeyInfo), + TBSCertificate: precert.RawTBSCertificate, + }, + }, + }, + }) + if err != nil { + return nil, fmt.Errorf("serializing SCT signature input: %w", err) + } + digest := sha256.Sum256(input) + sig, err := s.ctlogKey.Sign(rand.Reader, digest[:], crypto.SHA256) + if err != nil { + return nil, fmt.Errorf("signing SCT: %w", err) + } + sct.Signature = ct.DigitallySigned{ + Algorithm: cttls.SignatureAndHashAlgorithm{Hash: cttls.SHA256, Signature: cttls.ECDSA}, + Signature: sig, + } + + sctList, err := ctx509util.MarshalSCTsIntoSCTList([]*ct.SignedCertificateTimestamp{&sct}) + if err != nil { + return nil, fmt.Errorf("marshaling SCT list: %w", err) + } + raw, err := cttls.Marshal(*sctList) + if err != nil { + return nil, fmt.Errorf("encoding SCT list: %w", err) + } + // The extension value is the TLS-encoded SCT list wrapped in an ASN.1 + // OCTET STRING, per RFC 6962 §3.3. + wrapped, err := asn1.Marshal(raw) + if err != nil { + return nil, fmt.Errorf("wrapping SCT list: %w", err) + } + return &pkix.Extension{Id: asn1.ObjectIdentifier(ctx509.OIDExtensionCTSCT), Value: wrapped}, nil +} + +// handleCreateLogEntry implements Rekor v1's POST /api/v1/log/entries. The +// proposed entry is unmarshaled with 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 +// canonicalized, signed into a SignedEntryTimestamp, and returned. +func (s *testSigstore) handleCreateLogEntry(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(io.LimitReader(r.Body, 1<<20)) + if err != nil { + http.Error(w, "unreadable proposed entry", http.StatusBadRequest) + return + } + proposed, err := models.UnmarshalProposedEntry(bytes.NewReader(body), runtime.JSONConsumer()) + if err != nil { + http.Error(w, "malformed proposed entry", http.StatusBadRequest) + return + } + entryImpl, err := types.UnmarshalEntry(proposed) + if err != nil { + http.Error(w, "invalid proposed entry: "+err.Error(), http.StatusBadRequest) + return + } + canonical, err := entryImpl.Canonicalize(r.Context()) + if err != nil { + http.Error(w, "uncanonicalizable entry", http.StatusInternalServerError) + return + } + + entry, err := s.logEntry(r.Context(), canonical) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // The client reads the created entry out of the payload map by the ETag + // header, so the two must agree on the entry's UUID. + uuid := hex.EncodeToString(entry.leafHash) + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", uuid) + w.Header().Set("Location", "/api/v1/log/entries/"+uuid) + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(models.LogEntry{uuid: entry.anon}) +} + +// testLogEntry is a Rekor log entry the fake log has just integrated. +type testLogEntry struct { + anon models.LogEntryAnon + leafHash []byte +} + +// logEntry integrates a canonicalized entry body into the fake log: it signs +// the entry timestamp (the inclusion promise the cosign bundle annotation +// carries) and a single-leaf checkpoint (the inclusion proof). +func (s *testSigstore) logEntry(ctx context.Context, canonical []byte) (*testLogEntry, error) { + signer, err := signature.LoadECDSASignerVerifier(s.rekorKey, crypto.SHA256) + if err != nil { + return nil, fmt.Errorf("loading rekor signer: %w", err) + } + logID, err := testLogID(s.rekorKey.Public()) + if err != nil { + return nil, err + } + logIDHex := hex.EncodeToString(logID[:]) + integratedTime := time.Now().Unix() + logIndex := s.logIndex.Add(1) + encodedBody := base64.StdEncoding.EncodeToString(canonical) + + // The SignedEntryTimestamp covers the canonicalized JCS form of the + // {body, integratedTime, logIndex, logID} payload — the shape + // sigstore-go's tlog.VerifySET reconstructs and checks. + payload, err := json.Marshal(tlog.RekorPayload{ + Body: encodedBody, + IntegratedTime: integratedTime, + LogIndex: logIndex, + LogID: logIDHex, + }) + if err != nil { + return nil, fmt.Errorf("marshaling rekor payload: %w", err) + } + canonicalPayload, err := jsoncanonicalizer.Transform(payload) + if err != nil { + return nil, fmt.Errorf("canonicalizing rekor payload: %w", err) + } + set, err := signer.SignMessage(bytes.NewReader(canonicalPayload)) + if err != nil { + return nil, fmt.Errorf("signing entry timestamp: %w", err) + } + + // A one-leaf Merkle tree: the RFC 6962 leaf hash is also the root hash, + // and the inclusion proof needs no sibling hashes. + leafHash := sha256.Sum256(append([]byte{0x00}, canonical...)) + checkpoint, err := rekorutil.CreateAndSignCheckpoint(ctx, testRekorHostname, testRekorTreeID, 1, leafHash[:], signer) + if err != nil { + return nil, fmt.Errorf("signing checkpoint: %w", err) + } + rootHashHex := hex.EncodeToString(leafHash[:]) + + return &testLogEntry{ + leafHash: leafHash[:], + anon: models.LogEntryAnon{ + Body: encodedBody, + IntegratedTime: &integratedTime, + LogID: &logIDHex, + LogIndex: &logIndex, + Verification: &models.LogEntryAnonVerification{ + SignedEntryTimestamp: set, + InclusionProof: &models.InclusionProof{ + LogIndex: conv.Pointer(int64(0)), + TreeSize: conv.Pointer(int64(1)), + RootHash: &rootHashHex, + Hashes: []string{}, + Checkpoint: conv.Pointer(string(checkpoint)), + }, + }, + }, + }, nil +} + +// testRekorHostname and testRekorTreeID form the checkpoint origin +// (" - "). The trailing numeric tree ID is load-bearing: +// sigstore-go tells a Rekor v1 signed tree head from a v2 checkpoint by that +// suffix, and a v1 entry misread as v2 fails inclusion verification. +const ( + testRekorHostname = "rekor.localhost" + testRekorTreeID = int64(1) +) + +// testLogID is a transparency log's ID: the SHA-256 of its PKIX-encoded public +// key. It keys both the trusted root's log maps and the SCT's LogID field. +func testLogID(pub crypto.PublicKey) ([sha256.Size]byte, error) { + der, err := x509.MarshalPKIXPublicKey(pub) + if err != nil { + return [sha256.Size]byte{}, fmt.Errorf("marshaling log public key: %w", err) + } + return sha256.Sum256(der), nil +} + +// TimestampingAuthorities returns none: this deployment has no RFC 3161 +// timestamp authority, so signature time comes from the Rekor entry's +// integrated time — the same observer-timestamp source a public-good cosign +// keyless signature relies on. +func (*testSigstore) TimestampingAuthorities() []root.TimestampingAuthority { + return nil +} + +func (s *testSigstore) FulcioCertificateAuthorities() []root.CertificateAuthority { + return []root.CertificateAuthority{&root.FulcioCertificateAuthority{ + Root: s.rootCert, + Intermediates: []*x509.Certificate{s.intermediateCert}, + ValidityPeriodStart: s.rootCert.NotBefore, + ValidityPeriodEnd: s.rootCert.NotAfter, + URI: s.fulcioURL, + }} +} + +func (s *testSigstore) RekorLogs() map[string]*root.TransparencyLog { + return s.transparencyLog(s.rekorKey.Public(), s.rekorURL) +} + +func (s *testSigstore) CTLogs() map[string]*root.TransparencyLog { + return s.transparencyLog(s.ctlogKey.Public(), s.fulcioURL) +} + +func (*testSigstore) transparencyLog(pub crypto.PublicKey, baseURL string) map[string]*root.TransparencyLog { + logID, err := testLogID(pub) + if err != nil { + panic(err) + } + now := time.Now() + return map[string]*root.TransparencyLog{ + hex.EncodeToString(logID[:]): { + BaseURL: baseURL, + ID: logID[:], + ValidityPeriodStart: now.Add(-time.Hour), + ValidityPeriodEnd: now.Add(time.Hour), + HashFunc: crypto.SHA256, + PublicKey: pub, + SignatureHashFunc: crypto.SHA256, + }, + } +} + +// PublicKeyVerifier returns an error: this deployment issues certificates, so +// nothing signed against it should be verified as a bare long-lived key. +func (*testSigstore) PublicKeyVerifier(hint string) (root.TimeConstrainedVerifier, error) { + return nil, fmt.Errorf("test sigstore has no long-lived public keys (hint %q)", hint) +} + +// signKeylessArtifact pushes an artifact to an in-process registry and signs it +// keylessly against s, returning the artifact reference and the signing result. +func signKeylessArtifact(t *testing.T, s *testSigstore) (ref, digestStr string, res *Result) { + t.Helper() + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + host := strings.TrimPrefix(reg.URL, "http://") + + ref, digestStr = pushTestArtifact(t, host) + res, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + IdentityToken: identityToken(t, testKeylessSAN), + FulcioURL: s.fulcioURL, + RekorURL: s.rekorURL, + }) + require.NoError(t, err, "keyless signing against the test Fulcio and Rekor must succeed") + return ref, digestStr, res +} + +// TestSignOCIKeylessRoundTrip is the contract for the keyless flow: signing +// with an identity token produces a certificate-bearing signature that +// container/verifier discovers from the registry and classifies as keyless. +// Before this existed, SignOCI hard-failed without a key, so the +// certificate-bearing attach path C1 added had no producer. +func TestSignOCIKeylessRoundTrip(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + ref, digestStr, res := signKeylessArtifact(t, s) + require.NotEmpty(t, res.Bundle) + require.NotEmpty(t, res.PayloadDigest) + + // The returned bundle carries Fulcio and Rekor material, not a key hint. + bundles, err := verifier.RetrieveBundles(t.Context(), ref, nil) + require.NoError(t, err, "the attached keyless signature must be retrievable") + require.Len(t, bundles, 1) + assert.True(t, bundles[0].HasCertificate(), + "a keyless signature must round-trip through the registry as certificate-bearing") + + // The bundle signs the simple-signing payload digest, which is what the + // attached layer's own digest is — the cosign convention. + expectedDigest, err := PayloadDigest(ref, digestStr) + require.NoError(t, err) + assert.Equal(t, expectedDigest, res.PayloadDigest) + assert.Equal(t, strings.TrimPrefix(expectedDigest, "sha256:"), bundles[0].DigestHex) +} + +// TestSignOCIKeylessBundleVerifies is the test that matters: a bundle this +// package signs keylessly must satisfy the project's own keyless verification +// policy — DefaultVerifierOptions, i.e. an embedded SCT, a transparency-log +// entry, and an observer timestamp — against the deployment that issued it, +// with the signer identity bound into the policy. Anything less would prove +// only that the annotations parse, not that the signature is trustworthy. +func TestSignOCIKeylessBundleVerifies(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + ref, _, _ := signKeylessArtifact(t, s) + + bundles, err := verifier.RetrieveBundles(t.Context(), ref, nil) + require.NoError(t, err) + require.Len(t, bundles, 1) + + opts, err := verifier.DefaultVerifierOptions() + require.NoError(t, err) + + // Trust on first use: the chain of trust is verified, and the identity is + // read back out of the result. + result, err := verifier.VerifyBundle(bundles[0], s, nil, opts...) + require.NoError(t, err, "a keyless bundle must verify under the default policy") + + identity, err := verifier.IdentityFromResult(result) + require.NoError(t, err) + assert.Equal(t, testKeylessSAN, identity.SignerIdentity) + assert.Equal(t, testKeylessIssuer, identity.CertIssuer) + + // Pinning that identity must still verify, and a different one must not. + _, err = verifier.VerifyBundle(bundles[0], s, &identity, opts...) + require.NoError(t, err, "the recorded identity must re-verify") + + _, err = verifier.VerifyBundle(bundles[0], s, &verifier.Identity{ + SignerIdentity: "someone-else@example.com", + CertIssuer: testKeylessIssuer, + }, opts...) + require.ErrorIs(t, err, verifier.ErrVerificationFailed, + "a bundle must not verify against an identity that did not sign it") +} + +// TestSignOCIKeylessRejectedByUnrelatedTrustRoot guards against the test above +// passing for the wrong reason: the bundle must verify because the deployment +// that issued its certificate is trusted, not because the policy is lax. A +// second, independent deployment must reject it. +func TestSignOCIKeylessRejectedByUnrelatedTrustRoot(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + other := newTestSigstore(t) + + ref, _, _ := signKeylessArtifact(t, s) + bundles, err := verifier.RetrieveBundles(t.Context(), ref, nil) + require.NoError(t, err) + require.Len(t, bundles, 1) + + opts, err := verifier.DefaultVerifierOptions() + require.NoError(t, err) + _, err = verifier.VerifyBundle(bundles[0], other, nil, opts...) + require.ErrorIs(t, err, verifier.ErrVerificationFailed, + "a bundle must not verify against a trust root that did not issue its certificate") +} + +// TestSignOCIKeylessAttachesCertificateAndTlogAnnotations pins the annotations +// the keyless path writes. container/verifier classifies a layer as key-signed +// whenever the certificate annotation is absent, and cannot establish signature +// time without the transparency-log annotation, so both are load-bearing rather +// than informational. +func TestSignOCIKeylessAttachesCertificateAndTlogAnnotations(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + ref, digestStr, _ := signKeylessArtifact(t, s) + + layers := sigLayers(t, ref, digestStr) + require.Len(t, layers, 1) + assert.NotEmpty(t, layers[0].Annotations[annotationCosignSignature]) + + pemCert := layers[0].Annotations[annotationCosignCertificate] + require.NotEmpty(t, pemCert, "the keyless path must attach the certificate annotation") + cert, err := parseLeafCertificate([]byte(pemCert)) + require.NoError(t, err) + summary, err := fulciocert.SummarizeCertificate(cert) + require.NoError(t, err) + assert.Equal(t, testKeylessSAN, summary.SubjectAlternativeName) + assert.Equal(t, testKeylessIssuer, summary.Issuer) + + require.NotEmpty(t, layers[0].Annotations[annotationCosignBundle], + "a Rekor entry is mandatory for the keyless flow, so its annotation must be written") +} + +// TestSignOCIKeylessReSignAppendsForNewIdentity exercises the dedupe path with +// real Fulcio-issued certificates: keyless signing mints a fresh key and +// certificate every run, so re-signing as the same identity must dedupe on +// identity rather than on key or certificate bytes, while a different identity +// must append. +func TestSignOCIKeylessReSignAppendsForNewIdentity(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + host := strings.TrimPrefix(reg.URL, "http://") + ref, digestStr := pushTestArtifact(t, host) + + signAs := func(subject string) { + t.Helper() + _, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + IdentityToken: identityToken(t, subject), + FulcioURL: s.fulcioURL, + RekorURL: s.rekorURL, + }) + require.NoError(t, err) + } + + signAs(testKeylessSAN) + require.Len(t, sigLayers(t, ref, digestStr), 1) + + signAs(testKeylessSAN) + assert.Len(t, sigLayers(t, ref, digestStr), 1, + "re-signing as the same identity must not append a second layer") + + signAs("other@example.com") + assert.Len(t, sigLayers(t, ref, digestStr), 2, + "a different signer identity must append its own layer") +} + +// TestSignOCIKeylessSurfacesFulcioFailure proves a rejected identity token +// fails signing rather than silently falling back to some other layout. +func TestSignOCIKeylessSurfacesFulcioFailure(t *testing.T) { + t.Parallel() + + fulcio := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "invalid identity token", http.StatusUnauthorized) + })) + t.Cleanup(fulcio.Close) + + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + + _, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + IdentityToken: identityToken(t, testKeylessSAN), + FulcioURL: fulcio.URL, + // Unreachable, and must stay unreached: Fulcio fails first. + RekorURL: "http://127.0.0.1:1", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "building sigstore bundle") +} + +// TestSignOCIRejectsAmbiguousSigningMethod documents the precedence choice: +// supplying both a key and an identity token is a caller error, not a +// preference to be resolved silently. +func TestSignOCIRejectsAmbiguousSigningMethod(t *testing.T) { + t.Parallel() + keyPath, _ := writeTestKey(t) + + _, err := NewDefault(nil).SignOCI(t.Context(), testRef, "sha256:"+strings.Repeat("ab", 32), Options{ + Key: keyPath, + IdentityToken: identityToken(t, testKeylessSAN), + }) + require.ErrorIs(t, err, ErrAmbiguousSigningMethod) +} + +// TestErrKeyRequiredNamesBothSigningMethods keeps the "nothing provided" error +// actionable: it must point at both ways to sign, or a caller reading it will +// conclude a private key is the only option. +func TestErrKeyRequiredNamesBothSigningMethods(t *testing.T) { + t.Parallel() + + _, err := NewDefault(nil).SignOCI(t.Context(), testRef, "sha256:abc", Options{}) + require.ErrorIs(t, err, ErrKeyRequired) + assert.Contains(t, err.Error(), "Key") + assert.Contains(t, err.Error(), "IdentityToken") +} + +// TestKeylessBundleOptionsDefaultToPublicGood pins where URL defaulting +// happens: at signing time, so a zero-valued Options targets the public-good +// deployment and a caller-supplied URL is used verbatim. +func TestKeylessBundleOptionsDefaultToPublicGood(t *testing.T) { + t.Parallel() + + assert.Equal(t, "https://fulcio.sigstore.dev", DefaultFulcioURL) + assert.Equal(t, "https://rekor.sigstore.dev", DefaultRekorURL) + + opts := keylessBundleOptions(t.Context(), Options{IdentityToken: "token"}) + require.NotNil(t, opts.CertificateProvider, "keyless signing must go through Fulcio") + require.Len(t, opts.TransparencyLogs, 1, + "keyless signing must submit to exactly one transparency log") + require.NotNil(t, opts.CertificateProviderOptions) + assert.Equal(t, "token", opts.CertificateProviderOptions.IDToken) + assert.Nil(t, opts.TrustedRoot, + "signing must not verify against a trusted root it cannot scope to the target deployment") +} diff --git a/container/signer/signer.go b/container/signer/signer.go index cd204c9..643d63e 100644 --- a/container/signer/signer.go +++ b/container/signer/signer.go @@ -1,16 +1,23 @@ // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. // SPDX-License-Identifier: Apache-2.0 -// Package signer signs OCI artifacts with Sigstore, following the -// cosign key-pair convention: a "simple signing" payload binding the -// artifact's manifest digest is signed with the user's key, attached to the -// registry as a cosign signature manifest (the "sha256-.sig" tag), and -// returned as a serialized Sigstore bundle for durable storage and offline -// re-verification. +// Package signer signs OCI artifacts with Sigstore, following the cosign +// convention: a "simple signing" payload binding the artifact's manifest +// digest is signed, attached to the registry as a cosign signature manifest +// (the "sha256-.sig" tag), and returned as a serialized Sigstore bundle +// for durable storage and offline re-verification. +// +// Both cosign signing methods are supported, selected by [Options]: a +// file-based key pair ([Options.Key]) or keyless signing against Fulcio and +// Rekor with an OIDC identity token ([Options.IdentityToken]). Acquiring that +// token — ambient CI credentials, an interactive browser flow, a device +// flow — is entirely the caller's concern; this package never performs OAuth +// and only ever forwards the token it is handed to Fulcio. // // It is the signing counterpart to [github.com/stacklok/toolhive-core/container/verifier]: -// a bundle produced here verifies through that package's -// [verifier.VerifyBundleWithKey], and the attached manifest is the layout +// a key-signed bundle produced here verifies through that package's +// [verifier.VerifyBundleWithKey] and a keyless one through +// [verifier.VerifyBundle], and the attached manifest is the layout // [verifier.RetrieveBundles] reconstructs from. package signer @@ -26,18 +33,86 @@ import ( "github.com/sigstore/sigstore-go/pkg/sign" ) -// ErrKeyRequired indicates no signing key was provided. Keyless (OIDC) -// signing is not implemented yet, so a cosign private key is the only -// supported signing method. Callers exposing a CLI should wrap this with -// the flag the user is expected to pass. -var ErrKeyRequired = errors.New("signing key required: a cosign private key is needed to sign") +// Public-good Sigstore endpoints, used when Options leaves the corresponding +// URL empty. +// +// These are hardcoded rather than derived from a trusted root, deliberately. +// The trusted root does carry the deployment's URIs, but reading them means +// fetching a root (a TUF network 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. Signing +// against a different deployment is instead an explicit choice, made through +// Options. +const ( + // DefaultFulcioURL is the public-good Fulcio certificate authority. + DefaultFulcioURL = "https://fulcio.sigstore.dev" + // DefaultRekorURL is the public-good Rekor transparency log. + DefaultRekorURL = "https://rekor.sigstore.dev" +) + +// rekorAPIVersionV1 selects the Rekor v1 API in sign.RekorOptions.Version +// (sigstore-go accepts sign.RekorAPIVersions but keeps the version constants +// unexported). v1 is deliberate, not incidental: v2 issues proof-only entries +// and requires a separate timestamp authority, and the cosign bundle +// annotation this package attaches has no field for a v2 inclusion proof — +// only the v1 inclusion promise (a SignedEntryTimestamp) fits. See +// rekorBundleAnnotation. +const rekorAPIVersionV1 uint32 = 1 + +// keylessRetries is how many times a Fulcio or Rekor call is retried on a +// retryable (5xx, 429) response before signing fails. +const keylessRetries = 3 -// Options configures OCI signing. +// ErrKeyRequired indicates no signing method was provided: signing needs +// either a cosign private key or an OIDC identity token for the keyless +// flow. Callers exposing a CLI should wrap this with the flags the user is +// expected to pass. +var ErrKeyRequired = errors.New( + "no signing method provided: set Options.Key to a cosign private key, " + + "or Options.IdentityToken to an OIDC identity token for keyless signing") + +// ErrAmbiguousSigningMethod indicates both Options.Key and +// Options.IdentityToken were set. Rather than pick one, signing fails: the +// two produce materially different trust material — a bundle verifiers check +// against a bare public key, versus one they check against a Fulcio identity +// and the transparency log — so silently choosing would attach a signature +// the caller cannot verify the way it expects. +var ErrAmbiguousSigningMethod = errors.New( + "conflicting signing methods: set exactly one of Options.Key or Options.IdentityToken") + +// Options configures OCI signing. Exactly one signing method must be set: +// Key for the cosign key-pair flow, or IdentityToken for keyless signing. +// FulcioURL and RekorURL apply to keyless signing only and are ignored for +// the key-pair flow. type Options struct { // Key is the path to a cosign PEM-encoded private key file. An // encrypted key is decrypted with the COSIGN_PASSWORD environment // variable, matching the cosign CLI. Key string + // IdentityToken is a raw OIDC ID token (a JWT, as a plain string) + // identifying the signer to Fulcio. Setting it 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. Obtaining the token is the caller's + // responsibility — see the package doc. + IdentityToken string + // FulcioURL overrides the certificate authority for keyless signing. + // Empty means DefaultFulcioURL; the default is applied at signing time, + // so a zero Options value targets the public-good deployment. + FulcioURL string + // RekorURL overrides the transparency log for keyless signing. Empty + // means DefaultRekorURL, applied at signing time as with FulcioURL. + // Only the Rekor v1 API is supported — see rekorAPIVersionV1. + // + // A Rekor entry is not optional: the verification policy this project's + // container/verifier applies to keyless bundles requires a + // transparency-log entry, so a bundle signed without one would not + // verify. + RekorURL string } // Result is the outcome of a signing operation. @@ -66,7 +141,8 @@ type Signer interface { SignOCI(ctx context.Context, ref, digest string, opts Options) (*Result, error) } -// Default implements Signer with file-based cosign keys. +// Default implements Signer for both cosign signing methods: file-based key +// pairs and keyless signing via Fulcio and Rekor. type Default struct { keychain authn.Keychain } @@ -91,11 +167,13 @@ func NewDefault(keychain authn.Keychain) *Default { // verifier reconstructing the bundle from the registry manifest (or // re-verifying the stored bundle offline) therefore checks exactly the // signature that was attached — one signature, two representations. +// +// The signing method comes from opts: a file key, or keyless via Fulcio and +// Rekor. The two differ only in where the signing key and its trust material +// come from — everything downstream of sign.Bundle, including the attached +// manifest layout, is shared. func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Options) (*Result, error) { - if opts.Key == "" { - return nil, ErrKeyRequired - } - keypair, err := loadKeypair(opts.Key) + keypair, bundleOpts, err := signingMaterial(ctx, opts) if err != nil { return nil, err } @@ -106,8 +184,11 @@ func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Optio } // sign.Bundle invokes the keypair's SignData over the payload and - // records both the signature and the payload digest in the bundle. - pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, sign.BundleOptions{Context: ctx}) + // records both the signature and the payload digest in the bundle. On + // the keyless path it additionally calls Fulcio for a certificate over + // the ephemeral key and submits the signature to Rekor, folding both + // into the bundle's verification material. + pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, bundleOpts) if err != nil { return nil, fmt.Errorf("building sigstore bundle: %w", err) } @@ -133,6 +214,69 @@ func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Optio return &Result{Bundle: raw, PayloadDigest: payloadDigest(payload)}, nil } +// signingMaterial resolves opts into the key pair to sign with and the +// sign.Bundle options that establish its trust material, rejecting an absent +// or ambiguous signing method. Both branches return a sign.Keypair, which is +// the whole reason SignOCI needs no other knowledge of which flow is in play. +func signingMaterial(ctx context.Context, opts Options) (sign.Keypair, sign.BundleOptions, error) { + switch { + case opts.Key != "" && opts.IdentityToken != "": + return nil, sign.BundleOptions{}, ErrAmbiguousSigningMethod + case opts.Key != "": + keypair, err := loadKeypair(opts.Key) + if err != nil { + return nil, sign.BundleOptions{}, err + } + return keypair, sign.BundleOptions{Context: ctx}, nil + case opts.IdentityToken != "": + // A fresh key pair per signature is the point of keyless signing: + // the private key never outlives the call, and the certificate + // Fulcio issues over it is what carries identity forward. + keypair, err := sign.NewEphemeralKeypair(nil) + if err != nil { + return nil, sign.BundleOptions{}, fmt.Errorf("generating ephemeral signing key: %w", err) + } + return keypair, keylessBundleOptions(ctx, opts), nil + default: + return nil, sign.BundleOptions{}, ErrKeyRequired + } +} + +// keylessBundleOptions builds the sign.Bundle options for the keyless flow: +// Fulcio as the certificate provider for the ephemeral key, and Rekor v1 as +// the transparency log. +// +// TrustedRoot is deliberately left unset. sign.Bundle would otherwise verify +// the bundle it just produced, which needs a trusted root for the target +// deployment — a TUF fetch this package has no way to scope to a caller's +// custom FulcioURL/RekorURL. Verification is the caller's step, through +// container/verifier, against the root it decides to trust. +func keylessBundleOptions(ctx context.Context, opts Options) sign.BundleOptions { + fulcioURL := opts.FulcioURL + if fulcioURL == "" { + fulcioURL = DefaultFulcioURL + } + rekorURL := opts.RekorURL + if rekorURL == "" { + rekorURL = DefaultRekorURL + } + 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, + }), + }, + } +} + // PayloadDigest returns the digest of the simple-signing payload that a // signature over the artifact at ref pinned to digestStr covers. // diff --git a/go.mod b/go.mod index 31316e1..6218b6a 100644 --- a/go.mod +++ b/go.mod @@ -31,12 +31,17 @@ require ( ) require ( + github.com/cyberphone/json-canonicalization v0.0.0-20241213102144-19d51d7fe467 + github.com/go-openapi/runtime v0.33.0 + github.com/go-openapi/swag/conv v0.27.3 github.com/golang-jwt/jwt/v5 v5.3.1 + github.com/google/certificate-transparency-go v1.3.3 github.com/lestrrat-go/httprc/v3 v3.0.6 github.com/lestrrat-go/jwx/v3 v3.2.0 github.com/prometheus/client_golang v1.24.1 github.com/secure-systems-lab/go-securesystemslib v0.11.0 github.com/shirou/gopsutil/v4 v4.26.7 + github.com/sigstore/rekor v1.5.3 github.com/sigstore/sigstore v1.10.9 go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.45.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.45.0 @@ -67,7 +72,6 @@ require ( github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/coreos/go-oidc/v3 v3.20.0 // indirect - github.com/cyberphone/json-canonicalization v0.0.0-20241213102144-19d51d7fe467 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1 // indirect github.com/digitorus/pkcs7 v0.0.0-20230818184609-3a137a874352 // indirect @@ -84,13 +88,11 @@ require ( github.com/go-openapi/jsonpointer v1.0.0 // indirect github.com/go-openapi/jsonreference v1.0.0 // indirect github.com/go-openapi/loads v0.25.0 // indirect - github.com/go-openapi/runtime v0.33.0 // indirect github.com/go-openapi/runtime/server-middleware v0.30.0 // indirect github.com/go-openapi/spec v0.22.9 // indirect github.com/go-openapi/strfmt v0.27.0 // indirect github.com/go-openapi/swag v0.26.1 // indirect github.com/go-openapi/swag/cmdutils v0.27.0 // indirect - github.com/go-openapi/swag/conv v0.27.3 // indirect github.com/go-openapi/swag/fileutils v0.27.3 // indirect github.com/go-openapi/swag/jsonname v0.26.1 // indirect github.com/go-openapi/swag/jsonutils v0.27.3 // indirect @@ -104,7 +106,6 @@ require ( github.com/go-openapi/validate v0.26.1 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/goccy/go-json v0.10.6 // indirect - github.com/google/certificate-transparency-go v1.3.3 // indirect github.com/google/jsonschema-go v0.4.3 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect @@ -134,7 +135,6 @@ require ( github.com/segmentio/asm v1.2.1 // indirect github.com/segmentio/encoding v0.5.4 // indirect github.com/shibumi/go-pathspec v1.3.0 // indirect - github.com/sigstore/rekor v1.5.3 // indirect github.com/sigstore/rekor-tiles/v2 v2.3.0 // indirect github.com/sigstore/timestamp-authority/v2 v2.1.3 // indirect github.com/sirupsen/logrus v1.9.4 // indirect From 181b7e2ef78a33088bf34825ec38be20a21e0fcf Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 13 Aug 2026 19:30:41 +0200 Subject: [PATCH 2/4] Fix dedupe fidelity, mutation ordering, ctx, and Rekor panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- container/signer/cosign_attach.go | 33 +++--- container/signer/keyless_test.go | 162 ++++++++++++++++++++++++++ container/signer/signer.go | 185 +++++++++++++++++++++++++++--- container/signer/signer_test.go | 34 ++++-- 4 files changed, 374 insertions(+), 40 deletions(-) diff --git a/container/signer/cosign_attach.go b/container/signer/cosign_attach.go index bdf7617..ede7d3c 100644 --- a/container/signer/cosign_attach.go +++ b/container/signer/cosign_attach.go @@ -127,6 +127,13 @@ func parseManifestDigest(digestStr string) (digest.Digest, error) { // pub is the public key used for the key-signed dedupe check below — it is // separate from pb because the key-signed bundle itself carries no key // material, only a hint. +// +// The returned bool reports whether a new layer was actually appended: +// false means the artifact was already signed by this identity/key and +// attachCosignSignature was a no-op. This matters to the caller — pb's +// signature is randomised per call, so when nothing was appended, pb does +// NOT represent what is now on the registry; only the pre-existing layer +// does. See SignOCI's use of this to decide what to return as Result.Bundle. func attachCosignSignature( ctx context.Context, keychain authn.Keychain, @@ -134,28 +141,28 @@ func attachCosignSignature( payload []byte, pb *protobundle.Bundle, pub crypto.PublicKey, -) error { +) (bool, error) { ref, err := name.ParseReference(imageRef) if err != nil { - return fmt.Errorf("parsing image reference: %w", err) + return false, fmt.Errorf("parsing image reference: %w", err) } d, err := parseManifestDigest(digestStr) if err != nil { - return err + return false, err } msgSig := pb.GetMessageSignature() if msgSig == nil || len(msgSig.GetSignature()) == 0 { - return errors.New("bundle carries no message signature to attach") + return false, errors.New("bundle carries no message signature to attach") } cert, err := certMaterialFromBundle(pb) if err != nil { - return err + return false, err } h, err := v1.NewHash(d.String()) if err != nil { - return fmt.Errorf("parsing digest hash: %w", err) + return false, fmt.Errorf("parsing digest hash: %w", err) } sigTag := ref.Context().Tag(fmt.Sprint(h.Algorithm, "-", h.Hex, ".sig")) remoteOpts := []remote.Option{remote.WithAuthFromKeychain(keychain), remote.WithContext(ctx)} @@ -167,21 +174,21 @@ func attachCosignSignature( // on the next push. This mirrors cosign's own append behaviour. base, err := existingSignatureImage(sigTag, remoteOpts) if err != nil { - return err + return false, err } already, err := alreadySigned(base, payload, pub, cert) if err != nil { - return err + return false, err } if already { // Re-signing with the same key/identity is a no-op; pushing // repeatedly must not grow the manifest without bound. - return nil + return false, nil } annotations, err := signatureAnnotations(msgSig.GetSignature(), cert) if err != nil { - return err + return false, err } layer := static.NewLayer(payload, mediaTypeCosignSimpleSigningV1JSON) @@ -191,14 +198,14 @@ func attachCosignSignature( MediaType: mediaTypeCosignSimpleSigningV1JSON, }) if err != nil { - return fmt.Errorf("building signature manifest: %w", err) + return false, fmt.Errorf("building signature manifest: %w", err) } img = mutate.MediaType(img, types.OCIManifestSchema1) if err := remote.Write(sigTag, img, remoteOpts...); err != nil { - return fmt.Errorf("pushing signature manifest: %w", err) + return false, fmt.Errorf("pushing signature manifest: %w", err) } - return nil + return true, nil } // alreadySigned reports whether base already carries a signature layer diff --git a/container/signer/keyless_test.go b/container/signer/keyless_test.go index 3b94c07..daefbaf 100644 --- a/container/signer/keyless_test.go +++ b/container/signer/keyless_test.go @@ -41,6 +41,7 @@ import ( // and canonicalizes. _ "github.com/sigstore/rekor/pkg/types/hashedrekord/v0.0.1" rekorutil "github.com/sigstore/rekor/pkg/util" + verifybundle "github.com/sigstore/sigstore-go/pkg/bundle" fulciocert "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/root" "github.com/sigstore/sigstore-go/pkg/tlog" @@ -836,3 +837,164 @@ func TestKeylessBundleOptionsDefaultToPublicGood(t *testing.T) { assert.Nil(t, opts.TrustedRoot, "signing must not verify against a trusted root it cannot scope to the target deployment") } + +// TestSignOCIKeylessDedupeReturnsAttachedBundle is the regression test for +// the "same signature, two representations" contract on the dedupe path. +// Re-signing with the same identity makes attachCosignSignature a no-op +// (TestSignOCIKeylessReSignAppendsForNewIdentity is its append-side +// sibling), but every keyless signing operation mints a fresh ephemeral +// key, Fulcio certificate, and Rekor entry — so the freshly built bundle's +// own signature never matches whatever is actually on the registry when +// nothing gets appended. SignOCI must return the bundle that IS attached, +// not the one it built in memory and then discarded. +func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + + opts := Options{IdentityToken: identityToken(t, testKeylessSAN), FulcioURL: s.fulcioURL, RekorURL: s.rekorURL} + first, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + require.NoError(t, err) + + second, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + require.NoError(t, err) + + require.Len(t, sigLayers(t, ref, digestStr), 1, + "re-signing the same identity must not append a duplicate layer") + + // Compare the underlying cryptographic material, not the raw JSON: + // sign.Bundle's own output (what first.Bundle would be, and what + // second.Bundle would WRONGLY be without the fix) is bundle media type + // v0.3, while verifier.RetrieveBundles always reconstructs the classic + // v0.1 shape from OCI annotations — a real, pre-existing difference in + // serialization shape that exists regardless of dedupe, so it is not + // the right thing to assert byte-identical. + firstSig, firstCert := decodeBundleSignatureAndCert(t, first.Bundle) + secondSig, secondCert := decodeBundleSignatureAndCert(t, second.Bundle) + assert.Equal(t, firstSig, secondSig, + "the dedupe path must return the signature actually attached, not a freshly built, never-written one") + assert.Equal(t, firstCert, secondCert, "the dedupe path must return the certificate actually attached") + + // Guard against the equality above passing for the wrong reason (e.g. + // signing happening to be deterministic in this fixture): a THIRD call + // must also converge on that exact same material, not merely produce + // "some" pair that happens to match. + third, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + require.NoError(t, err) + thirdSig, thirdCert := decodeBundleSignatureAndCert(t, third.Bundle) + assert.Equal(t, firstSig, thirdSig) + assert.Equal(t, firstCert, thirdCert) +} + +// decodeBundleSignatureAndCert extracts the message signature and leaf +// certificate DER from a serialized sigstore bundle, for comparing the +// underlying cryptographic material across two bundles independent of +// their JSON shape. +func decodeBundleSignatureAndCert(t *testing.T, raw []byte) (sig, certDER []byte) { + t.Helper() + var bun verifybundle.Bundle + require.NoError(t, bun.UnmarshalJSON(raw)) + msgSig := bun.GetMessageSignature() + require.NotNil(t, msgSig) + cert, err := certMaterialFromBundle(bun.Bundle) + require.NoError(t, err) + require.NotNil(t, cert) + return msgSig.GetSignature(), cert.certDER +} + +// TestSignOCIRespectsContextCancellation is the regression test for a +// canceled ctx not stopping an in-flight Fulcio call. sigstore-go's Fulcio +// client (pkg/sign.Fulcio.GetCertificate, sigstore-go v1.3.0) builds its +// HTTP request with http.NewRequest, not http.NewRequestWithContext, and +// only consults ctx between retries — once a request is actually in +// flight, canceling ctx has no effect on it; SignOCI would otherwise block +// until Fulcio's own client-level timeout (30s by default). signBundle +// races ctx.Done() against the call instead, so SignOCI must return +// promptly on cancellation regardless of what the dependency does. +func TestSignOCIRespectsContextCancellation(t *testing.T) { + t.Parallel() + + block := make(chan struct{}) + fulcioMux := http.NewServeMux() + fulcioMux.HandleFunc("POST /api/v2/signingCert", func(http.ResponseWriter, *http.Request) { + <-block // never responds within the test's lifetime + }) + fulcio := httptest.NewServer(fulcioMux) + // Cleanup order matters: httptest.Server.Close blocks until in-flight + // requests complete, so the handler must be unblocked FIRST. t.Cleanup + // runs LIFO, so registering fulcio.Close before closing block makes + // close(block) run first. + t.Cleanup(fulcio.Close) + t.Cleanup(func() { close(block) }) + + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := NewDefault(nil).SignOCI(ctx, ref, digestStr, Options{ + IdentityToken: identityToken(t, testKeylessSAN), + FulcioURL: fulcio.URL, + RekorURL: "http://127.0.0.1:0", // unreachable; never consulted before ctx fires + }) + elapsed := time.Since(start) + + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Less(t, elapsed, 2*time.Second, + "SignOCI must return promptly on ctx cancellation, not block for the underlying HTTP client's own timeout") +} + +// TestSignOCIRecoversFromMalformedRekorResponse is the regression test for +// a nil-pointer panic reachable through untrusted network input. +// sigstore/rekor's own response parsing (pkg/tle.GenerateTransparencyLogEntry, +// reached via sigstore-go's Rekor v1 client) dereferences +// Verification.InclusionProof unconditionally, and a syntactically +// well-formed Rekor response that simply omits the optional "verification" +// object reaches that dereference. SignOCI's signBundle wrapper must +// recover this into a returned error: signing happens for the lifetime of +// a long-lived `thv serve` process, so an unrecovered panic here would +// take the whole process down over one bad response from a misconfigured +// (not necessarily malicious) Rekor deployment. +func TestSignOCIRecoversFromMalformedRekorResponse(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + fulcioMux := http.NewServeMux() + fulcioMux.HandleFunc("POST /api/v2/signingCert", s.handleSigningCert) + fulcio := httptest.NewServer(fulcioMux) + t.Cleanup(fulcio.Close) + + // A syntactically valid Rekor response — 201 Created, ETag identifying + // the entry, a decodable body — that simply has no "verification" key. + // LogID must be present and valid hex for GenerateTransparencyLogEntry + // to reach as far as the Verification dereference. + rekorMux := http.NewServeMux() + rekorMux.HandleFunc("POST /api/v1/log/entries", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", "test-entry-uuid") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"test-entry-uuid":{` + + `"logID":"` + strings.Repeat("ab", 32) + `",` + + `"logIndex":1,"integratedTime":1700000000,"body":"e30="}}`)) + }) + rekor := httptest.NewServer(rekorMux) + t.Cleanup(rekor.Close) + + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + + // If this panics unrecovered, the whole test BINARY aborts, not just + // this test — which is precisely the failure mode being guarded against. + _, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + IdentityToken: identityToken(t, testKeylessSAN), + FulcioURL: fulcio.URL, + RekorURL: rekor.URL, + }) + require.Error(t, err, "a malformed Rekor response must surface as an error, never as a crash") +} diff --git a/container/signer/signer.go b/container/signer/signer.go index 643d63e..e36353b 100644 --- a/container/signer/signer.go +++ b/container/signer/signer.go @@ -22,15 +22,22 @@ package signer import ( + "bytes" "context" + "crypto" "crypto/sha256" "encoding/hex" "errors" "fmt" + "strings" "github.com/google/go-containerregistry/pkg/authn" + protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" verifybundle "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/sigstore/sigstore-go/pkg/sign" + "github.com/sigstore/sigstore/pkg/signature" + + "github.com/stacklok/toolhive-core/container/verifier" ) // Public-good Sigstore endpoints, used when Options leaves the corresponding @@ -183,37 +190,185 @@ func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Optio return nil, err } - // sign.Bundle invokes the keypair's SignData over the payload and - // records both the signature and the payload digest in the bundle. On - // the keyless path it additionally calls Fulcio for a certificate over - // the ephemeral key and submits the signature to Rekor, folding both - // into the bundle's verification material. - pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, bundleOpts) + pb, err := signBundle(ctx, payload, keypair, bundleOpts) if err != nil { - return nil, fmt.Errorf("building sigstore bundle: %w", err) + return nil, err } msgSig := pb.GetMessageSignature() if msgSig == nil || len(msgSig.GetSignature()) == 0 { return nil, errors.New("signing produced no message signature") } - if err := attachCosignSignature( - ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey(), - ); err != nil { - return nil, fmt.Errorf("attaching signature manifest: %w", err) + // Validated BEFORE the registry is touched: a malformed-but-parseable + // response from Fulcio/Rekor could otherwise attach successfully and + // only THEN fail validation below, returning an error despite having + // already mutated the registry. + if _, err := verifybundle.NewBundle(pb); err != nil { + return nil, fmt.Errorf("validating sigstore bundle: %w", err) } - bun, err := verifybundle.NewBundle(pb) + attached, err := attachCosignSignature(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey()) if err != nil { - return nil, fmt.Errorf("finalizing sigstore bundle: %w", err) + return nil, fmt.Errorf("attaching signature manifest: %w", err) } - raw, err := bun.MarshalJSON() + + raw, err := resultBundleJSON(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey(), attached) if err != nil { - return nil, fmt.Errorf("serializing sigstore bundle: %w", err) + return nil, err } return &Result{Bundle: raw, PayloadDigest: payloadDigest(payload)}, nil } +// signBundle runs sign.Bundle on its own goroutine so SignOCI honors ctx +// cancellation even though it cannot always rely on the underlying +// dependency to: sigstore-go's Fulcio client (pkg/sign.Fulcio.GetCertificate, +// as of sigstore-go v1.3.0) builds its HTTP request with http.NewRequest, +// not http.NewRequestWithContext, and only consults ctx between retries — +// once a request is actually in flight, a canceled ctx has no effect until +// the HTTP client's own timeout fires (30s by default). Racing ctx.Done() +// against the call lets a canceled SignOCI return promptly regardless; the +// abandoned goroutine still exits on its own once the underlying call +// completes or times out, so nothing leaks unbounded. +// +// The same boundary recovers a panic, for a related but distinct reason: +// sigstore/rekor's own response parsing (pkg/tle.GenerateTransparencyLogEntry, +// reached via sigstore-go's Rekor v1 client) dereferences several +// Verification/InclusionProof response fields unconditionally, and a +// well-formed JSON response that simply omits that object — a misconfigured +// or non-standard Rekor deployment, not necessarily a hostile one — reaches +// that dereference and panics. SignOCI is this package's public API surface +// and, per the package doc, signing happens for the lifetime of a long-lived +// `thv serve` process; an unrecovered panic here would take the whole +// process down over a single bad network response. Recovering only around +// this specific call (not, say, wrapping SignOCI's own logic) keeps the +// recover() scoped to untrusted external input, not to bugs in this +// package's own code. +func signBundle(ctx context.Context, payload []byte, keypair sign.Keypair, opts sign.BundleOptions) (*protobundle.Bundle, error) { + type signOutcome struct { + pb *protobundle.Bundle + err error + } + outcome := make(chan signOutcome, 1) + go func() { + defer func() { + if r := recover(); r != nil { + outcome <- signOutcome{ + err: fmt.Errorf("signing failed unexpectedly (a Fulcio or Rekor response could not be processed): %v", r), + } + } + }() + pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, opts) + outcome <- signOutcome{pb: pb, err: err} + }() + select { + case <-ctx.Done(): + return nil, ctx.Err() + case res := <-outcome: + if res.err != nil { + return nil, fmt.Errorf("building sigstore bundle: %w", res.err) + } + return res.pb, nil + } +} + +// resultBundleJSON returns the serialized bundle that accurately reflects +// what is now on the registry, per attachCosignSignature's attached result. +// +// When attached is true, pb IS what was just written — "one signature, two +// representations," as documented on SignOCI. When attached is false, +// attachCosignSignature detected this identity/key had already signed the +// artifact and left the pre-existing layer alone: pb was built but never +// written anywhere, and every signing operation is randomised (a fresh +// ephemeral key and Fulcio certificate for keyless; ECDSA's own randomised +// nonce for a key re-sign), so pb's signature does not match what is +// actually attached. Returning pb in that case would violate the same +// contract from the other direction — the caller would hold a +// self-consistent bundle that nonetheless does not correspond to the +// artifact's actual registry state. The genuinely attached bundle is +// retrieved and returned instead. +func resultBundleJSON( + ctx context.Context, + 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) { + return b.Raw, nil + } + } + return nil, errors.New("previously attached signature not found among the artifact's signature layers") +} + +// bundleMatchesSigner reports whether b is the layer this signing operation +// deduped against: the same certificate identity for keyless (SAN + OIDC +// issuer — never certificate bytes, which are fresh every keyless run), or +// a signature over payload that verifies with pub for the key-signed flow +// (never signature bytes, which ECDSA randomises on every call). +func bundleMatchesSigner(b verifier.Bundle, payload []byte, cert *certMaterial, pub crypto.PublicKey) bool { + if b.Parsed == nil { + return false + } + if cert != nil { + existing, err := certMaterialFromBundle(b.Parsed.Bundle) + if err != nil || existing == nil { + return false + } + return existing.summary.SubjectAlternativeName == cert.summary.SubjectAlternativeName && + existing.summary.Issuer == cert.summary.Issuer + } + msgSig := b.Parsed.GetMessageSignature() + if msgSig == nil || len(msgSig.GetSignature()) == 0 { + return false + } + sigVerifier, err := signature.LoadVerifier(pub, crypto.SHA256) + if err != nil { + return false + } + return sigVerifier.VerifySignature(bytes.NewReader(msgSig.GetSignature()), bytes.NewReader(payload)) == nil +} + // signingMaterial resolves opts into the key pair to sign with and the // sign.Bundle options that establish its trust material, rejecting an absent // or ambiguous signing method. Both branches return a sign.Keypair, which is diff --git a/container/signer/signer_test.go b/container/signer/signer_test.go index 028bc17..cbe4555 100644 --- a/container/signer/signer_test.go +++ b/container/signer/signer_test.go @@ -705,7 +705,8 @@ func TestAttachCosignSignatureCertBearingRoundTrip(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", true) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil)) + _, attachErr1 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + require.NoError(t, attachErr1) layers := sigLayers(t, ref, digestStr) require.Len(t, layers, 1) @@ -801,7 +802,8 @@ func TestAttachCosignSignatureCertBearingWithoutTlog(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil)) + _, attachErr2 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + require.NoError(t, attachErr2) layers := sigLayers(t, ref, digestStr) require.Len(t, layers, 1) @@ -869,7 +871,7 @@ func TestRekorBundleAnnotationRejectsProofOnlyEntry(t *testing.T) { TlogEntries: []*protorekor.TransparencyLogEntry{entry}, }, } - err = attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + _, err = attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) require.Error(t, err) assert.Contains(t, err.Error(), "inclusion promise") } @@ -898,13 +900,15 @@ func TestAttachCosignSignatureCertDedupeIgnoresUnverifiableExistingLayer(t *test // one about to be signed, but under the same certificate/identity — the // same SAN+issuer match signedByIdentity's dedupe would otherwise trust. badPb := certBearingBundle(t, otherPayload, "https://example.com/signer", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, badPb, nil)) + _, attachErr3 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, badPb, nil) + require.NoError(t, attachErr3) require.Len(t, sigLayers(t, ref, digestStr), 1) // Now attach a genuinely valid signature over the real payload, same // identity. It must NOT be deduped away by the mismatched layer above. goodPb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, goodPb, nil)) + _, attachErr4 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, goodPb, nil) + require.NoError(t, attachErr4) assert.Len(t, sigLayers(t, ref, digestStr), 2, "an existing same-identity layer whose signature does not verify must not suppress a valid re-sign") } @@ -926,7 +930,8 @@ func TestAttachCosignSignatureCertBearingWithTlogAnnotatesBundle(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", true) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil)) + _, attachErr5 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + require.NoError(t, attachErr5) layers := sigLayers(t, ref, digestStr) require.Len(t, layers, 1) @@ -961,7 +966,8 @@ func TestAttachCosignSignatureCertDedupeSameIdentity(t *testing.T) { for range 3 { pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil)) + _, attachErr6 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + require.NoError(t, attachErr6) } assert.Len(t, sigLayers(t, ref, digestStr), 1, @@ -984,16 +990,19 @@ func TestAttachCosignSignatureCertAppendsDifferentIdentity(t *testing.T) { require.NoError(t, err) pbA := certBearingBundle(t, payload, "https://example.com/signer-a", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbA, nil)) + _, attachErr7 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbA, nil) + require.NoError(t, attachErr7) require.Len(t, sigLayers(t, ref, digestStr), 1) pbB := certBearingBundle(t, payload, "https://example.com/signer-b", "https://issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbB, nil)) + _, attachErr8 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbB, nil) + require.NoError(t, attachErr8) assert.Len(t, sigLayers(t, ref, digestStr), 2, "a different keyless signer SAN must not be deduped away") pbC := certBearingBundle(t, payload, "https://example.com/signer-a", "https://other-issuer.example.com", false) - require.NoError(t, attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbC, nil)) + _, attachErr9 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbC, nil) + require.NoError(t, attachErr9) assert.Len(t, sigLayers(t, ref, digestStr), 3, "a different OIDC issuer for the same SAN must not be deduped away") } @@ -1018,8 +1027,9 @@ func TestAttachCosignSignatureKeySignedDedupeUnaffected(t *testing.T) { for range 3 { pb := keySignedBundle(t, payload, priv) - require.NoError(t, attachCosignSignature( - t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, priv.Public())) + _, attachErr10 := attachCosignSignature( + t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, priv.Public()) + require.NoError(t, attachErr10) } layers := sigLayers(t, ref, digestStr) From 697e3d44a49d1e8d6be7650cec9951c53a45bbe7 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 11:27:55 +0200 Subject: [PATCH 3/4] Verify signature, not just identity, when selecting a dedupe match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- container/signer/keyless_test.go | 76 ++++++++++++++++++++++++++++++++ container/signer/signer.go | 53 ++++++++++++++++++---- 2 files changed, 120 insertions(+), 9 deletions(-) diff --git a/container/signer/keyless_test.go b/container/signer/keyless_test.go index daefbaf..3e5879f 100644 --- a/container/signer/keyless_test.go +++ b/container/signer/keyless_test.go @@ -34,6 +34,7 @@ import ( cttls "github.com/google/certificate-transparency-go/tls" ctx509 "github.com/google/certificate-transparency-go/x509" ctx509util "github.com/google/certificate-transparency-go/x509util" + "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/registry" "github.com/sigstore/rekor/pkg/generated/models" "github.com/sigstore/rekor/pkg/types" @@ -904,6 +905,81 @@ func decodeBundleSignatureAndCert(t *testing.T, raw []byte) (sig, certDER []byte return msgSig.GetSignature(), cert.certDER } +// TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer covers the same +// class of bug TestAttachCosignSignatureCertDedupeIgnoresUnverifiableExistingLayer +// (signer_test.go) fixed on the attach side, but for bundleMatchesSigner — +// the SEPARATE function that decides what SignOCI RETURNS when it dedupes. +// Identity match (SAN + OIDC issuer) alone is not enough to select a layer: +// a same-identity layer whose signature does not verify against THIS +// payload (corrupt, or signed over a different one) must be skipped in +// favor of one that does. Skipping this check would let SignOCI return a +// Result.Bundle whose signature does not match the PayloadDigest it also +// returns — a caller could never successfully verify it offline. +func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + opts := Options{IdentityToken: identityToken(t, testKeylessSAN), FulcioURL: s.fulcioURL, RekorURL: s.rekorURL} + + payload, err := SimpleSigningPayload(ref, digestStr) + require.NoError(t, err) + + // A genuinely Fulcio-signed bundle for the SAME identity, but over a + // DIFFERENT payload — obtained by signing (and attaching, at an + // unrelated sig tag) against a digest that need not correspond to any + // real content; SimpleSigningPayload only needs the two strings. + otherDigestStr := "sha256:" + strings.Repeat("cd", 32) + otherPayload, err := SimpleSigningPayload(ref, otherDigestStr) + require.NoError(t, err) + otherResult, err := NewDefault(nil).SignOCI(t.Context(), ref, otherDigestStr, opts) + require.NoError(t, err) + var otherBun verifybundle.Bundle + require.NoError(t, otherBun.UnmarshalJSON(otherResult.Bundle)) + + // Attach that bundle directly at the REAL target's sig tag, as the + // first layer — same identity as opts, signature over otherPayload, + // which will not verify against payload. + attached, err := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, otherBun.Bundle, nil) + require.NoError(t, err) + require.True(t, attached) + require.Len(t, sigLayers(t, ref, digestStr), 1) + + // A real sign+attach for the actual target appends the genuinely valid + // layer second — attach's own dedupe (already correct) skips the + // damaged layer above since it doesn't verify, so this appends rather + // than deduping. + valid, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + require.NoError(t, err) + require.Len(t, sigLayers(t, ref, digestStr), 2, "the damaged layer must not have deduped the valid signing") + validSig, validCert := decodeBundleSignatureAndCert(t, valid.Bundle) + + // Re-sign once more with the same identity: this dedupes (the valid + // layer from above verifies), so SignOCI must return THAT layer's + // material — not the damaged layer that happens to be first in + // RetrieveBundles' iteration order. + redundant, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + require.NoError(t, err) + require.Len(t, sigLayers(t, ref, digestStr), 2, "the third call must dedupe against the valid layer, not append again") + + redundantSig, redundantCert := decodeBundleSignatureAndCert(t, redundant.Bundle) + assert.Equal(t, validSig, redundantSig, + "the dedupe path must select the layer that verifies against payload, not the damaged one") + assert.Equal(t, validCert, redundantCert) + + // Confirm the returned bundle is actually self-consistent: its own + // signature verifies against its own PayloadDigest, not otherPayload. + sigVerifier, err := signature.LoadVerifier(func() crypto.PublicKey { + cert, parseErr := x509.ParseCertificate(redundantCert) + require.NoError(t, parseErr) + return cert.PublicKey + }(), crypto.SHA256) + require.NoError(t, err) + assert.NoError(t, sigVerifier.VerifySignature(bytes.NewReader(redundantSig), bytes.NewReader(payload)), + "the returned bundle's signature must verify against the payload its own PayloadDigest names") +} + // TestSignOCIRespectsContextCancellation is the regression test for a // canceled ctx not stopping an in-flight Fulcio call. sigstore-go's Fulcio // client (pkg/sign.Fulcio.GetCertificate, sigstore-go v1.3.0) builds its diff --git a/container/signer/signer.go b/container/signer/signer.go index e36353b..2c1cb40 100644 --- a/container/signer/signer.go +++ b/container/signer/signer.go @@ -26,6 +26,7 @@ import ( "context" "crypto" "crypto/sha256" + "crypto/x509" "encoding/hex" "errors" "fmt" @@ -106,6 +107,10 @@ type Options struct { // short-lived certificate binding it to the token's identity, and the // signature is submitted to Rekor. Obtaining the token is the caller's // responsibility — see the package doc. + // + // This token is sent as a bearer credential to FulcioURL. A caller + // setting FulcioURL to anything other than an HTTPS (or loopback, for + // tests) endpoint sends it in the clear. IdentityToken string // FulcioURL overrides the certificate authority for keyless signing. // Empty means DefaultFulcioURL; the default is applied at signing time, @@ -126,6 +131,15 @@ type Options struct { type Result struct { // Bundle is the serialized Sigstore bundle, for durable storage and // later offline re-verification. + // + // Its JSON shape is not stable across calls for the same identity: a + // freshly attached signature serializes as sign.Bundle's own bundle + // media type (v0.3), while a signature this call deduped against (see + // SignOCI's "one signature, two representations" doc) is reconstructed + // by verifier.RetrieveBundles from the classic cosign annotations — + // the v0.1 shape, carrying an inclusion promise rather than a proof. + // Both verify identically; a caller comparing stored bundles byte-for- + // byte across calls, or inspecting mediaType, will see this difference. Bundle []byte // PayloadDigest is the ":" digest of the simple-signing // payload the bundle actually signs. @@ -342,25 +356,46 @@ func previouslyAttachedBundleJSON( } // bundleMatchesSigner reports whether b is the layer this signing operation -// deduped against: the same certificate identity for keyless (SAN + OIDC -// issuer — never certificate bytes, which are fresh every keyless run), or -// a signature over payload that verifies with pub for the key-signed flow +// deduped against: for keyless, the same certificate identity (SAN + OIDC +// issuer — never certificate bytes, which are fresh every keyless run) AND +// a signature that verifies against THAT certificate's own key over +// payload; for key-signed, a signature over payload that verifies with pub // (never signature bytes, which ECDSA randomises on every call). +// +// The cryptographic check on the keyless branch matters, not just the +// identity match: signedByIdentity (the attach-side dedupe this mirrors) +// treats identity as a candidate, not a verdict — the layer must still +// verify against its own certificate before dedupe accepts it, so that a +// same-identity layer whose signature happens not to verify (corrupt, or +// signed over a different payload) doesn't get selected here instead of a +// genuinely valid layer. Skipping this check would let SignOCI return a +// Result.Bundle whose signature does not match its own PayloadDigest. 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 } - return existing.summary.SubjectAlternativeName == cert.summary.SubjectAlternativeName && - existing.summary.Issuer == cert.summary.Issuer - } - msgSig := b.Parsed.GetMessageSignature() - if msgSig == nil || len(msgSig.GetSignature()) == 0 { - 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 { From a692b11ceb54580e966d22a5273e8c1d7c194fd5 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 16:08:30 +0200 Subject: [PATCH 4/4] Require chain-of-trust for keyless dedupe and harden signing errors 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. --- container/signer/cosign_attach.go | 159 ++++++++++-------- container/signer/keyless_test.go | 86 ++++++++-- container/signer/signer.go | 262 +++++++++++++++++++++--------- container/signer/signer_test.go | 46 ++++-- 4 files changed, 379 insertions(+), 174 deletions(-) diff --git a/container/signer/cosign_attach.go b/container/signer/cosign_attach.go index ede7d3c..7d3a1fb 100644 --- a/container/signer/cosign_attach.go +++ b/container/signer/cosign_attach.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "crypto" + "crypto/sha256" "crypto/x509" "encoding/base64" "encoding/hex" @@ -30,7 +31,11 @@ import ( protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" protorekor "github.com/sigstore/protobuf-specs/gen/pb-go/rekor/v1" fulciocert "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/sigstore/sigstore-go/pkg/root" + "github.com/sigstore/sigstore-go/pkg/verify" "github.com/sigstore/sigstore/pkg/signature" + + "github.com/stacklok/toolhive-core/container/verifier" ) const ( @@ -141,6 +146,7 @@ func attachCosignSignature( payload []byte, pb *protobundle.Bundle, pub crypto.PublicKey, + tm root.TrustedMaterial, ) (bool, error) { ref, err := name.ParseReference(imageRef) if err != nil { @@ -177,7 +183,7 @@ func attachCosignSignature( return false, err } - already, err := alreadySigned(base, payload, pub, cert) + already, err := alreadySigned(ctx, keychain, imageRef, digestStr, base, payload, pub, cert, tm) if err != nil { return false, err } @@ -208,14 +214,18 @@ func attachCosignSignature( return true, nil } -// alreadySigned reports whether base already carries a signature layer -// equivalent to the one about to be attached, dispatching to the dedupe -// check that matches the signature's layout: identity comparison for a -// certificate-bearing (keyless) signature, key comparison otherwise. See -// signedByIdentity and signedByKey for why these must be different checks. -func alreadySigned(base v1.Image, payload []byte, pub crypto.PublicKey, cert *certMaterial) (bool, error) { +// alreadySigned reports whether the artifact already carries a signature +// layer equivalent to the one about to be attached, dispatching to the +// dedupe check that matches the signature's layout: a chain-verified +// identity check for a certificate-bearing (keyless) signature, key +// comparison against base's raw layers otherwise. See keylessAlreadySigned +// and signedByKey for why these must be different checks. +func alreadySigned( + ctx context.Context, keychain authn.Keychain, imageRef, digestStr string, + base v1.Image, payload []byte, pub crypto.PublicKey, cert *certMaterial, tm root.TrustedMaterial, +) (bool, error) { if cert != nil { - return signedByIdentity(base, payload, cert.summary) + return keylessAlreadySigned(ctx, keychain, imageRef, digestStr, payload, cert, tm) } return signedByKey(base, payload, pub) } @@ -310,78 +320,87 @@ func signedByKey(img v1.Image, payload []byte, pub crypto.PublicKey) (bool, erro return false, nil } -// signedByIdentity reports whether img already carries a certificate-bearing -// signature layer from the same signer identity as summary that ALSO -// cryptographically verifies against payload — i.e. whether this keyless -// signer has already produced a valid signature over this exact artifact. -// Identity match alone is not enough to dedupe on: a layer whose signature -// does not verify against its own certificate is not "already signed", -// merely damaged, and treating it as equivalent would let a corrupted or -// mismatched existing layer permanently block a legitimate re-sign from -// ever attaching a valid signature. This is the keyless counterpart to -// signedByKey: comparing certificate bytes, or the signature itself, would -// never dedupe, because keyless signing mints a fresh ephemeral keypair and -// certificate on every run. Identity — certificate SAN plus OIDC issuer — -// is what stays stable across runs, and is also what a verifier's trust -// policy binds to (see container/verifier's identityPolicyOption), so it is -// the right notion of "already signed by this signer", checked alongside — -// never instead of — the cryptographic check. -func signedByIdentity(img v1.Image, payload []byte, summary fulciocert.Summary) (bool, error) { - manifest, err := img.Manifest() +// keylessAlreadySigned reports whether the artifact at imageRef/digestStr +// already carries a genuinely trusted keyless signature from cert's +// identity over payload. See keylessLayerTrusted for why identity match +// alone (certificate SAN plus OIDC issuer — the keyless counterpart to +// signedByKey's public-key comparison, since keyless signing mints a fresh +// ephemeral keypair and certificate on every run) is NOT enough to dedupe +// on: unlike signedByKey's pub, which the caller supplies as ground truth, +// the SAN and issuer extensions are attacker-visible fields inside the +// certificate itself, so a self-signed certificate can claim any identity — +// this must additionally chain-verify. +// +// This re-reads the registry through verifier.RetrieveBundles rather than +// scanning the existing signature manifest's raw layers directly (as the +// key-signed path does): only RetrieveBundles' reconstruction carries the +// certificate and transparency-log material coreverifier.VerifyBundle +// needs, and rebuilding that here would duplicate container/verifier's own +// read side. A failure here — including "nothing attached yet" — is +// treated as not-yet-signed rather than propagated: at worst this appends a +// redundant layer, which append-never-replace already makes safe, and that +// is a better failure mode than blocking a legitimate sign over a +// transient read problem. +func keylessAlreadySigned( + ctx context.Context, keychain authn.Keychain, imageRef, digestStr string, + payload []byte, cert *certMaterial, tm root.TrustedMaterial, +) (bool, error) { + full := imageRef + if !strings.Contains(imageRef, "@") { + full = imageRef + "@" + digestStr + } + bundles, err := verifier.RetrieveBundles(ctx, full, keychain) if err != nil { - return false, fmt.Errorf("reading signature manifest layers: %w", err) + return false, nil //nolint:nilerr // see doc comment: a read failure here means "append", not "fail the sign" } - for _, l := range manifest.Layers { - pemCert := l.Annotations[annotationCosignCertificate] - if pemCert == "" { - continue - } - existing, err := parseLeafCertificate([]byte(pemCert)) - if err != nil { - // A layer we cannot parse is not one of ours; leave it alone - // rather than failing the whole push over someone else's - // malformed annotation. - continue - } - existingSummary, err := fulciocert.SummarizeCertificate(existing) - if err != nil { - continue - } - if existingSummary.SubjectAlternativeName != summary.SubjectAlternativeName || - existingSummary.Issuer != summary.Issuer { - continue - } - if verifies, _ := certLayerVerifies(l, existing, payload); verifies { + opts, err := verifier.DefaultVerifierOptions() + if err != nil { + return false, nil //nolint:nilerr // embedded verifier options; failure here is not expected in practice + } + for _, b := range bundles { + if b.HasCertificate() && keylessLayerTrusted(b, tm, opts, payload, cert.summary) { return true, nil } - // Same identity, but this layer's signature does not verify against - // its own certificate and payload — it is corrupt or stale, not a - // prior valid signing. Fall through rather than dedupe on it, so a - // new, valid signature still gets appended. } return false, nil } -// certLayerVerifies reports whether layer's attached signature annotation -// cryptographically verifies against cert's public key and payload. A -// decode or verifier-construction failure is reported as "does not verify" -// rather than propagated: the annotation is registry-supplied data, and the -// caller's fallback (treat as not-yet-signed, append a new layer) is -// already the correct response to a layer that fails to check out. -func certLayerVerifies(layer v1.Descriptor, cert *x509.Certificate, payload []byte) (bool, error) { - encoded := layer.Annotations[annotationCosignSignature] - if encoded == "" { - return false, errors.New("layer carries a certificate but no signature annotation") - } - raw, err := base64.StdEncoding.DecodeString(encoded) - if err != nil { - return false, fmt.Errorf("decoding signature annotation: %w", err) +// keylessLayerTrusted reports whether bundle b is a genuine, chain-verified +// Fulcio/Rekor signature from summary's identity over payload — not merely +// a certificate carrying a matching SAN/issuer extension. Both fields are +// public, unauthenticated data embedded in the certificate: anyone with +// registry write access can mint a self-signed certificate carrying a +// victim's SAN and OIDC-issuer extension, sign the known simple-signing +// payload with the matching (self-generated) key, and push it as a +// signature layer. A check that only confirms internal self-consistency +// (the signature verifies against ITS OWN certificate's key) would accept +// that forgery as "already signed by this identity" and silently skip +// attaching the real, Fulcio-backed signature — a signing-pipeline bypass, +// not a forged verification (a real consumer's install/verify step still +// rejects the self-signed certificate on chain-of-trust), but a bypass +// nonetheless. This instead runs the exact chain-of-trust and +// transparency-log verification a real consumer's install/verify step +// applies — container/verifier's embedded trust root and +// DefaultVerifierOptions — so only a genuinely Fulcio-issued, Rekor-logged +// certificate can dedupe. +// +// The digest check runs before the (comparatively expensive) chain +// verification: a chain-valid signature over some OTHER payload from this +// same identity says nothing about whether THIS payload has already been +// signed, and checking it first also avoids wasted verification work. +func keylessLayerTrusted( + b verifier.Bundle, tm root.TrustedMaterial, opts []verify.VerifierOption, payload []byte, summary fulciocert.Summary, +) bool { + sum := sha256.Sum256(payload) + if b.DigestAlgo != "sha256" || b.DigestHex != hex.EncodeToString(sum[:]) { + return false } - sigVerifier, err := signature.LoadVerifier(cert.PublicKey, crypto.SHA256) - if err != nil { - return false, fmt.Errorf("loading verifier for existing certificate: %w", err) + vr, err := verifier.VerifyBundle(b, tm, nil, opts...) + if err != nil || vr.Signature == nil || vr.Signature.Certificate == nil { + return false } - return sigVerifier.VerifySignature(bytes.NewReader(raw), bytes.NewReader(payload)) == nil, nil + return vr.Signature.Certificate.SubjectAlternativeName == summary.SubjectAlternativeName && + vr.Signature.Certificate.Issuer == summary.Issuer } // certMaterial is the certificate and transparency-log evidence for a diff --git a/container/signer/keyless_test.go b/container/signer/keyless_test.go index 3e5879f..0bb6f7b 100644 --- a/container/signer/keyless_test.go +++ b/container/signer/keyless_test.go @@ -59,6 +59,7 @@ import ( const ( testKeylessSAN = "signer@example.com" testKeylessIssuer = "https://oidc.example.com" + testFakeIDToken = "token" ) // testSigstore is a complete Sigstore deployment running in-process: a Fulcio @@ -750,7 +751,7 @@ func TestSignOCIKeylessReSignAppendsForNewIdentity(t *testing.T) { signAs := func(subject string) { t.Helper() - _, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + _, err := newDefaultForTest(nil, s).SignOCI(t.Context(), ref, digestStr, Options{ IdentityToken: identityToken(t, subject), FulcioURL: s.fulcioURL, RekorURL: s.rekorURL, @@ -771,7 +772,9 @@ func TestSignOCIKeylessReSignAppendsForNewIdentity(t *testing.T) { } // TestSignOCIKeylessSurfacesFulcioFailure proves a rejected identity token -// fails signing rather than silently falling back to some other layout. +// fails signing rather than silently falling back to some other layout, and +// that the error names Fulcio as the stage that failed — not Rekor, which +// the RekorURL below proves was never even reached. func TestSignOCIKeylessSurfacesFulcioFailure(t *testing.T) { t.Parallel() @@ -791,7 +794,7 @@ func TestSignOCIKeylessSurfacesFulcioFailure(t *testing.T) { RekorURL: "http://127.0.0.1:1", }) require.Error(t, err) - assert.Contains(t, err.Error(), "building sigstore bundle") + assert.Contains(t, err.Error(), "requesting signing certificate from Fulcio") } // TestSignOCIRejectsAmbiguousSigningMethod documents the precedence choice: @@ -829,16 +832,38 @@ func TestKeylessBundleOptionsDefaultToPublicGood(t *testing.T) { assert.Equal(t, "https://fulcio.sigstore.dev", DefaultFulcioURL) assert.Equal(t, "https://rekor.sigstore.dev", DefaultRekorURL) - opts := keylessBundleOptions(t.Context(), Options{IdentityToken: "token"}) + opts, err := keylessBundleOptions(t.Context(), Options{IdentityToken: testFakeIDToken}) + require.NoError(t, err) require.NotNil(t, opts.CertificateProvider, "keyless signing must go through Fulcio") require.Len(t, opts.TransparencyLogs, 1, "keyless signing must submit to exactly one transparency log") require.NotNil(t, opts.CertificateProviderOptions) - assert.Equal(t, "token", opts.CertificateProviderOptions.IDToken) + assert.Equal(t, testFakeIDToken, opts.CertificateProviderOptions.IDToken) assert.Nil(t, opts.TrustedRoot, "signing must not verify against a trusted root it cannot scope to the target deployment") } +// TestKeylessBundleOptionsRejectsInsecureEndpoint is the regression test for +// the identity token going out as a bearer credential: a non-HTTPS, +// non-loopback FulcioURL or RekorURL must be rejected before either is +// ever contacted, since sigstore-go's Fulcio client attaches the token to +// its request unconditionally, regardless of scheme. +func TestKeylessBundleOptionsRejectsInsecureEndpoint(t *testing.T) { + t.Parallel() + + _, err := keylessBundleOptions(t.Context(), Options{IdentityToken: testFakeIDToken, FulcioURL: "http://fulcio.example.com"}) + require.Error(t, err, "a non-loopback plain-HTTP FulcioURL must be rejected") + + _, err = keylessBundleOptions(t.Context(), Options{IdentityToken: testFakeIDToken, RekorURL: "http://rekor.example.com"}) + require.Error(t, err, "a non-loopback plain-HTTP RekorURL must be rejected") + + // Loopback stays permitted — this is what every keyless test in this + // package relies on for its fake Fulcio/Rekor servers. + opts, err := keylessBundleOptions(t.Context(), Options{IdentityToken: testFakeIDToken, FulcioURL: "http://127.0.0.1:12345"}) + require.NoError(t, err, "loopback HTTP must remain permitted for tests") + require.NotNil(t, opts.CertificateProvider) +} + // TestSignOCIKeylessDedupeReturnsAttachedBundle is the regression test for // the "same signature, two representations" contract on the dedupe path. // Re-signing with the same identity makes attachCosignSignature a no-op @@ -856,10 +881,11 @@ func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) { ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) opts := Options{IdentityToken: identityToken(t, testKeylessSAN), FulcioURL: s.fulcioURL, RekorURL: s.rekorURL} - first, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + signer := newDefaultForTest(nil, s) + first, err := signer.SignOCI(t.Context(), ref, digestStr, opts) require.NoError(t, err) - second, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + second, err := signer.SignOCI(t.Context(), ref, digestStr, opts) require.NoError(t, err) require.Len(t, sigLayers(t, ref, digestStr), 1, @@ -882,7 +908,7 @@ func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) { // signing happening to be deterministic in this fixture): a THIRD call // must also converge on that exact same material, not merely produce // "some" pair that happens to match. - third, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + third, err := signer.SignOCI(t.Context(), ref, digestStr, opts) require.NoError(t, err) thirdSig, thirdCert := decodeBundleSignatureAndCert(t, third.Bundle) assert.Equal(t, firstSig, thirdSig) @@ -922,6 +948,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { t.Cleanup(reg.Close) ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) opts := Options{IdentityToken: identityToken(t, testKeylessSAN), FulcioURL: s.fulcioURL, RekorURL: s.rekorURL} + signer := newDefaultForTest(nil, s) payload, err := SimpleSigningPayload(ref, digestStr) require.NoError(t, err) @@ -933,7 +960,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { otherDigestStr := "sha256:" + strings.Repeat("cd", 32) otherPayload, err := SimpleSigningPayload(ref, otherDigestStr) require.NoError(t, err) - otherResult, err := NewDefault(nil).SignOCI(t.Context(), ref, otherDigestStr, opts) + otherResult, err := signer.SignOCI(t.Context(), ref, otherDigestStr, opts) require.NoError(t, err) var otherBun verifybundle.Bundle require.NoError(t, otherBun.UnmarshalJSON(otherResult.Bundle)) @@ -941,7 +968,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { // Attach that bundle directly at the REAL target's sig tag, as the // first layer — same identity as opts, signature over otherPayload, // which will not verify against payload. - attached, err := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, otherBun.Bundle, nil) + attached, err := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, otherBun.Bundle, nil, nil) require.NoError(t, err) require.True(t, attached) require.Len(t, sigLayers(t, ref, digestStr), 1) @@ -950,7 +977,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { // layer second — attach's own dedupe (already correct) skips the // damaged layer above since it doesn't verify, so this appends rather // than deduping. - valid, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + valid, err := signer.SignOCI(t.Context(), ref, digestStr, opts) require.NoError(t, err) require.Len(t, sigLayers(t, ref, digestStr), 2, "the damaged layer must not have deduped the valid signing") validSig, validCert := decodeBundleSignatureAndCert(t, valid.Bundle) @@ -959,7 +986,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) { // layer from above verifies), so SignOCI must return THAT layer's // material — not the damaged layer that happens to be first in // RetrieveBundles' iteration order. - redundant, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, opts) + redundant, err := signer.SignOCI(t.Context(), ref, digestStr, opts) require.NoError(t, err) require.Len(t, sigLayers(t, ref, digestStr), 2, "the third call must dedupe against the valid layer, not append again") @@ -1025,6 +1052,41 @@ func TestSignOCIRespectsContextCancellation(t *testing.T) { "SignOCI must return promptly on ctx cancellation, not block for the underlying HTTP client's own timeout") } +// TestSignOCIKeylessSurfacesRekorFailure is TestSignOCIKeylessSurfacesFulcioFailure's +// complement: Fulcio succeeds (a real certificate is issued), Rekor fails, +// and the error must name Rekor as the stage — not Fulcio, and not the +// generic "building sigstore bundle" wording this used to have before +// keylessSignBundle could tell the two stages apart. +func TestSignOCIKeylessSurfacesRekorFailure(t *testing.T) { + t.Parallel() + s := newTestSigstore(t) + + fulcioMux := http.NewServeMux() + fulcioMux.HandleFunc("POST /api/v2/signingCert", s.handleSigningCert) + fulcio := httptest.NewServer(fulcioMux) + t.Cleanup(fulcio.Close) + + // 400, not 503/429: those are retried with backoff (keylessRetries), which + // would make this test slow without changing what it proves. + rekor := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "malformed request", http.StatusBadRequest) + })) + t.Cleanup(rekor.Close) + + reg := httptest.NewServer(registry.New()) + t.Cleanup(reg.Close) + ref, digestStr := pushTestArtifact(t, strings.TrimPrefix(reg.URL, "http://")) + + _, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{ + IdentityToken: identityToken(t, testKeylessSAN), + FulcioURL: fulcio.URL, + RekorURL: rekor.URL, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "submitting to Rekor") + assert.NotContains(t, err.Error(), "Fulcio") +} + // TestSignOCIRecoversFromMalformedRekorResponse is the regression test for // a nil-pointer panic reachable through untrusted network input. // sigstore/rekor's own response parsing (pkg/tle.GenerateTransparencyLogEntry, diff --git a/container/signer/signer.go b/container/signer/signer.go index 2c1cb40..7436aa4 100644 --- a/container/signer/signer.go +++ b/container/signer/signer.go @@ -26,7 +26,6 @@ import ( "context" "crypto" "crypto/sha256" - "crypto/x509" "encoding/hex" "errors" "fmt" @@ -35,10 +34,13 @@ import ( "github.com/google/go-containerregistry/pkg/authn" protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" verifybundle "github.com/sigstore/sigstore-go/pkg/bundle" + "github.com/sigstore/sigstore-go/pkg/root" "github.com/sigstore/sigstore-go/pkg/sign" + "github.com/sigstore/sigstore-go/pkg/verify" "github.com/sigstore/sigstore/pkg/signature" "github.com/stacklok/toolhive-core/container/verifier" + "github.com/stacklok/toolhive-core/networking" ) // Public-good Sigstore endpoints, used when Options leaves the corresponding @@ -108,13 +110,18 @@ type Options struct { // signature is submitted to Rekor. Obtaining the token is the caller's // responsibility — see the package doc. // - // This token is sent as a bearer credential to FulcioURL. A caller - // setting FulcioURL to anything other than an HTTPS (or loopback, for - // tests) endpoint sends it in the clear. + // This token is sent as a bearer credential to FulcioURL, which is why + // FulcioURL and RekorURL are validated to be HTTPS (loopback excepted, + // for tests) before either is contacted — see keylessBundleOptions. IdentityToken string // FulcioURL overrides the certificate authority for keyless signing. - // Empty means DefaultFulcioURL; the default is applied at signing time, - // so a zero Options value targets the public-good deployment. + // Empty means DefaultFulcioURL. Defaulting happens at signing time and + // only when IdentityToken selects the keyless flow — a zero Options + // value alone still errors with ErrKeyRequired, it does not sign + // against the public-good deployment by default. Once IdentityToken IS + // set, though, an empty FulcioURL/RekorURL is a deliberate posture + // change from "no key ⇒ always error": keyless signing then performs + // outbound network egress to public-good Sigstore services by default. FulcioURL string // RekorURL overrides the transparency log for keyless signing. Empty // means DefaultRekorURL, applied at signing time as with FulcioURL. @@ -166,6 +173,17 @@ type Signer interface { // pairs and keyless signing via Fulcio and Rekor. type Default struct { keychain authn.Keychain + // trustedMaterial resolves the trust material keyless dedupe checks an + // existing layer's certificate against (see keylessLayerTrusted). Nil in + // every production Default — keylessTrustedMaterial falls back to + // verifier.OfflineTrustedMaterial, the embedded public-good Sigstore + // root, which is what DefaultFulcioURL/DefaultRekorURL actually issue + // against. It exists as a field (not a hardcoded call) only so tests + // signing against a synthetic Fulcio/Rekor deployment — one that, by + // construction, cannot chain to the embedded production root — can + // supply their own trust material and exercise dedupe actually firing, + // not just the fail-closed path. See newDefaultForTest. + trustedMaterial func() (root.TrustedMaterial, error) } var _ Signer = (*Default)(nil) @@ -180,6 +198,25 @@ func NewDefault(keychain authn.Keychain) *Default { return &Default{keychain: keychain} } +// newDefaultForTest creates a signer whose keyless dedupe check verifies +// against tm instead of the embedded production Sigstore root — for tests +// signing against a synthetic Fulcio/Rekor deployment that cannot chain to +// the real one. Production code always uses NewDefault. +func newDefaultForTest(keychain authn.Keychain, tm root.TrustedMaterial) *Default { + d := NewDefault(keychain) + d.trustedMaterial = func() (root.TrustedMaterial, error) { return tm, nil } + return d +} + +// keylessTrustedMaterial resolves the trust material for keyless dedupe +// verification — see the doc comment on Default.trustedMaterial. +func (d *Default) keylessTrustedMaterial() (root.TrustedMaterial, error) { + if d.trustedMaterial != nil { + return d.trustedMaterial() + } + return verifier.OfflineTrustedMaterial() +} + // SignOCI signs the artifact following the cosign convention: the signature // is computed over the simple-signing payload (which embeds the manifest // digest, binding the signature to the artifact), the SAME signature is @@ -204,7 +241,7 @@ func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Optio return nil, err } - pb, err := signBundle(ctx, payload, keypair, bundleOpts) + pb, err := signBundle(ctx, payload, keypair, bundleOpts, opts.IdentityToken != "") if err != nil { return nil, err } @@ -221,43 +258,81 @@ func (d *Default) SignOCI(ctx context.Context, ref, digestStr string, opts Optio return nil, fmt.Errorf("validating sigstore bundle: %w", err) } - attached, err := attachCosignSignature(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey()) + tm, err := d.keylessTrustedMaterial() + if err != nil { + return nil, fmt.Errorf("loading trusted material: %w", err) + } + + attached, err := attachCosignSignature(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey(), tm) if err != nil { return nil, fmt.Errorf("attaching signature manifest: %w", err) } - raw, err := resultBundleJSON(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey(), attached) + raw, err := resultBundleJSON(ctx, d.keychain, ref, digestStr, payload, pb, keypair.GetPublicKey(), tm, attached) if err != nil { return nil, err } return &Result{Bundle: raw, PayloadDigest: payloadDigest(payload)}, nil } -// signBundle runs sign.Bundle on its own goroutine so SignOCI honors ctx -// cancellation even though it cannot always rely on the underlying -// dependency to: sigstore-go's Fulcio client (pkg/sign.Fulcio.GetCertificate, -// as of sigstore-go v1.3.0) builds its HTTP request with http.NewRequest, -// not http.NewRequestWithContext, and only consults ctx between retries — -// once a request is actually in flight, a canceled ctx has no effect until -// the HTTP client's own timeout fires (30s by default). Racing ctx.Done() -// against the call lets a canceled SignOCI return promptly regardless; the -// abandoned goroutine still exits on its own once the underlying call -// completes or times out, so nothing leaks unbounded. +// signBundle builds the sigstore bundle for payload. For the key-signed +// path (keyless false) this is a direct, synchronous call: no network is +// involved, so neither of the two problems keylessSignBundle exists to work +// around applies, and a panic here would be this package's own bug — it +// should propagate as one, not be reclassified as a Fulcio or Rekor +// problem with the real stack trace discarded. +func signBundle( + ctx context.Context, payload []byte, keypair sign.Keypair, opts sign.BundleOptions, keyless bool, +) (*protobundle.Bundle, error) { + if !keyless { + pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, opts) + if err != nil { + return nil, fmt.Errorf("building sigstore bundle: %w", err) + } + return pb, nil + } + return keylessSignBundle(ctx, payload, keypair, opts) +} + +// keylessSignBundle runs the keyless flow on its own goroutine so SignOCI +// honors ctx cancellation even though it cannot always rely on the +// underlying dependency to: sigstore-go's Fulcio client +// (pkg/sign.Fulcio.GetCertificate, as of sigstore-go v1.3.0) builds its HTTP +// request with http.NewRequest, not http.NewRequestWithContext, and only +// consults ctx between retries — once a request is actually in flight, a +// canceled ctx has no effect until the HTTP client's own timeout fires (30s +// by default). Racing ctx.Done() against the call lets a canceled SignOCI +// return promptly regardless; the abandoned goroutine still exits on its +// own once the underlying call completes or times out, so nothing leaks +// unbounded. +// +// The same goroutine boundary recovers a panic, for a related but distinct +// reason: sigstore/rekor's own response parsing +// (pkg/tle.GenerateTransparencyLogEntry, reached via sigstore-go's Rekor v1 +// client) dereferences several Verification/InclusionProof response fields +// unconditionally, and a well-formed JSON response that simply omits that +// object — a misconfigured or non-standard Rekor deployment, not +// necessarily a hostile one — reaches that dereference and panics. SignOCI +// is this package's public API surface and, per the package doc, signing +// happens for the lifetime of a long-lived `thv serve` process; an +// unrecovered panic here would take the whole process down over a single +// bad network response. Scoping this boundary to the keyless path only +// (rather than wrapping signBundle's key-signed branch too, as an earlier +// version of this function did) keeps recover() scoped to untrusted +// external input, not to bugs in this package's own code. // -// The same boundary recovers a panic, for a related but distinct reason: -// sigstore/rekor's own response parsing (pkg/tle.GenerateTransparencyLogEntry, -// reached via sigstore-go's Rekor v1 client) dereferences several -// Verification/InclusionProof response fields unconditionally, and a -// well-formed JSON response that simply omits that object — a misconfigured -// or non-standard Rekor deployment, not necessarily a hostile one — reaches -// that dereference and panics. SignOCI is this package's public API surface -// and, per the package doc, signing happens for the lifetime of a long-lived -// `thv serve` process; an unrecovered panic here would take the whole -// process down over a single bad network response. Recovering only around -// this specific call (not, say, wrapping SignOCI's own logic) keeps the -// recover() scoped to untrusted external input, not to bugs in this -// package's own code. -func signBundle(ctx context.Context, payload []byte, keypair sign.Keypair, opts sign.BundleOptions) (*protobundle.Bundle, error) { +// Fulcio and Rekor failures are distinguished by calling +// opts.CertificateProvider directly first — the exact call sign.Bundle +// would otherwise make internally, but with its error tagged as a Fulcio +// failure before sign.Bundle ever sees it — then handing sign.Bundle the +// already-fetched certificate via cachedCertificate, so its own internal +// GetCertificate call is a cache hit rather than a second Fulcio round +// trip. With this configuration (no TimestampAuthorities, no TrustedRoot), +// the only network call remaining inside sign.Bundle at that point is the +// Rekor submission, so a failure there is unambiguously tagged as Rekor's. +func keylessSignBundle( + ctx context.Context, payload []byte, keypair sign.Keypair, opts sign.BundleOptions, +) (*protobundle.Bundle, error) { type signOutcome struct { pb *protobundle.Bundle err error @@ -271,20 +346,40 @@ func signBundle(ctx context.Context, payload []byte, keypair sign.Keypair, opts } } }() - pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, opts) - outcome <- signOutcome{pb: pb, err: err} + fulcio := opts.CertificateProvider + certDER, err := fulcio.GetCertificate(ctx, keypair, opts.CertificateProviderOptions) + if err != nil { + outcome <- signOutcome{err: fmt.Errorf("requesting signing certificate from Fulcio: %w", err)} + return + } + cachedOpts := opts + cachedOpts.CertificateProvider = &cachedCertificate{certDER: certDER} + pb, err := sign.Bundle(&sign.PlainData{Data: payload}, keypair, cachedOpts) + if err != nil { + outcome <- signOutcome{err: fmt.Errorf("submitting to Rekor: %w", err)} + return + } + outcome <- signOutcome{pb: pb} }() select { case <-ctx.Done(): return nil, ctx.Err() case res := <-outcome: - if res.err != nil { - return nil, fmt.Errorf("building sigstore bundle: %w", res.err) - } - return res.pb, nil + return res.pb, res.err } } +// cachedCertificate is a sign.CertificateProvider that returns an +// already-fetched certificate without making another network call — see +// keylessSignBundle's doc comment for why. +type cachedCertificate struct { + certDER []byte +} + +func (c *cachedCertificate) GetCertificate(context.Context, sign.Keypair, *sign.CertificateProviderOptions) ([]byte, error) { + return c.certDER, nil +} + // resultBundleJSON returns the serialized bundle that accurately reflects // what is now on the registry, per attachCosignSignature's attached result. // @@ -307,6 +402,7 @@ func resultBundleJSON( payload []byte, pb *protobundle.Bundle, pub crypto.PublicKey, + tm root.TrustedMaterial, attached bool, ) ([]byte, error) { if attached { @@ -320,7 +416,7 @@ func resultBundleJSON( } return raw, nil } - return previouslyAttachedBundleJSON(ctx, keychain, ref, digestStr, payload, pb, pub) + return previouslyAttachedBundleJSON(ctx, keychain, ref, digestStr, payload, pb, pub, tm) } // previouslyAttachedBundleJSON retrieves the signature layer that made this @@ -334,6 +430,7 @@ func previouslyAttachedBundleJSON( payload []byte, pb *protobundle.Bundle, pub crypto.PublicKey, + tm root.TrustedMaterial, ) ([]byte, error) { full := ref if !strings.Contains(ref, "@") { @@ -347,8 +444,18 @@ func previouslyAttachedBundleJSON( if err != nil { return nil, err } + var opts []verify.VerifierOption + if cert != nil { + // Only needed for the keyless chain-verification branch below; + // loading it unconditionally would be wasted work on the + // key-signed path, which never reaches keylessLayerTrusted. + opts, err = verifier.DefaultVerifierOptions() + if err != nil { + return nil, fmt.Errorf("loading verifier options: %w", err) + } + } for _, b := range bundles { - if bundleMatchesSigner(b, payload, cert, pub) { + if bundleMatchesSigner(b, tm, opts, payload, cert, pub) { return b.Raw, nil } } @@ -356,47 +463,32 @@ func previouslyAttachedBundleJSON( } // bundleMatchesSigner reports whether b is the layer this signing operation -// deduped against: for keyless, the same certificate identity (SAN + OIDC -// issuer — never certificate bytes, which are fresh every keyless run) AND -// a signature that verifies against THAT certificate's own key over -// payload; for key-signed, a signature over payload that verifies with pub -// (never signature bytes, which ECDSA randomises on every call). +// deduped against: for keyless, a genuine, chain-verified Fulcio/Rekor +// signature from cert's identity over payload (see keylessLayerTrusted — +// identity match alone is not enough, since SAN and OIDC issuer are +// attacker-visible fields inside the certificate itself); for key-signed, a +// signature over payload that verifies with pub (never signature bytes, +// which ECDSA randomises on every call). // -// The cryptographic check on the keyless branch matters, not just the -// identity match: signedByIdentity (the attach-side dedupe this mirrors) -// treats identity as a candidate, not a verdict — the layer must still -// verify against its own certificate before dedupe accepts it, so that a -// same-identity layer whose signature happens not to verify (corrupt, or -// signed over a different payload) doesn't get selected here instead of a -// genuinely valid layer. Skipping this check would let SignOCI return a -// Result.Bundle whose signature does not match its own PayloadDigest. -func bundleMatchesSigner(b verifier.Bundle, payload []byte, cert *certMaterial, pub crypto.PublicKey) bool { +// This mirrors keylessAlreadySigned's keyless check exactly (both call +// keylessLayerTrusted) because they must agree: attach's dedupe decision +// and this selection must treat the same layer as "already signed" or not, +// or SignOCI could report success while returning material that doesn't +// correspond to what attach actually decided. +func bundleMatchesSigner( + b verifier.Bundle, tm root.TrustedMaterial, opts []verify.VerifierOption, + payload []byte, cert *certMaterial, pub crypto.PublicKey, +) bool { if b.Parsed == nil { return false } + if cert != nil { + return b.HasCertificate() && keylessLayerTrusted(b, tm, opts, payload, cert.summary) + } 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 @@ -426,7 +518,11 @@ func signingMaterial(ctx context.Context, opts Options) (sign.Keypair, sign.Bund if err != nil { return nil, sign.BundleOptions{}, fmt.Errorf("generating ephemeral signing key: %w", err) } - return keypair, keylessBundleOptions(ctx, opts), nil + bundleOpts, err := keylessBundleOptions(ctx, opts) + if err != nil { + return nil, sign.BundleOptions{}, err + } + return keypair, bundleOpts, nil default: return nil, sign.BundleOptions{}, ErrKeyRequired } @@ -441,7 +537,7 @@ func signingMaterial(ctx context.Context, opts Options) (sign.Keypair, sign.Bund // deployment — a TUF fetch this package has no way to scope to a caller's // custom FulcioURL/RekorURL. Verification is the caller's step, through // container/verifier, against the root it decides to trust. -func keylessBundleOptions(ctx context.Context, opts Options) sign.BundleOptions { +func keylessBundleOptions(ctx context.Context, opts Options) (sign.BundleOptions, error) { fulcioURL := opts.FulcioURL if fulcioURL == "" { fulcioURL = DefaultFulcioURL @@ -450,6 +546,18 @@ func keylessBundleOptions(ctx context.Context, opts Options) sign.BundleOptions if rekorURL == "" { rekorURL = DefaultRekorURL } + // IdentityToken goes out as an Authorization: Bearer header to fulcioURL + // (sigstore-go's Fulcio client, unconditionally). A non-HTTPS, + // non-loopback URL sends it in the clear — validated here, after + // defaulting, so both the caller-supplied and the default case are + // covered by one check. Loopback stays permitted for tests, which need + // a plain-HTTP fake Fulcio/Rekor. + if err := networking.ValidateEndpointURL(fulcioURL); err != nil { + return sign.BundleOptions{}, fmt.Errorf("FulcioURL: %w", err) + } + if err := networking.ValidateEndpointURL(rekorURL); err != nil { + return sign.BundleOptions{}, fmt.Errorf("RekorURL: %w", err) + } return sign.BundleOptions{ Context: ctx, CertificateProvider: sign.NewFulcio(&sign.FulcioOptions{ @@ -464,7 +572,7 @@ func keylessBundleOptions(ctx context.Context, opts Options) sign.BundleOptions Version: rekorAPIVersionV1, }), }, - } + }, nil } // PayloadDigest returns the digest of the simple-signing payload that a diff --git a/container/signer/signer_test.go b/container/signer/signer_test.go index cbe4555..06c10fa 100644 --- a/container/signer/signer_test.go +++ b/container/signer/signer_test.go @@ -705,7 +705,7 @@ func TestAttachCosignSignatureCertBearingRoundTrip(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", true) - _, attachErr1 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + _, attachErr1 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil, nil) require.NoError(t, attachErr1) layers := sigLayers(t, ref, digestStr) @@ -802,7 +802,7 @@ func TestAttachCosignSignatureCertBearingWithoutTlog(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - _, attachErr2 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + _, attachErr2 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil, nil) require.NoError(t, attachErr2) layers := sigLayers(t, ref, digestStr) @@ -871,7 +871,7 @@ func TestRekorBundleAnnotationRejectsProofOnlyEntry(t *testing.T) { TlogEntries: []*protorekor.TransparencyLogEntry{entry}, }, } - _, err = attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + _, err = attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "inclusion promise") } @@ -900,14 +900,14 @@ func TestAttachCosignSignatureCertDedupeIgnoresUnverifiableExistingLayer(t *test // one about to be signed, but under the same certificate/identity — the // same SAN+issuer match signedByIdentity's dedupe would otherwise trust. badPb := certBearingBundle(t, otherPayload, "https://example.com/signer", "https://issuer.example.com", false) - _, attachErr3 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, badPb, nil) + _, attachErr3 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, badPb, nil, nil) require.NoError(t, attachErr3) require.Len(t, sigLayers(t, ref, digestStr), 1) // Now attach a genuinely valid signature over the real payload, same // identity. It must NOT be deduped away by the mismatched layer above. goodPb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - _, attachErr4 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, goodPb, nil) + _, attachErr4 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, goodPb, nil, nil) require.NoError(t, attachErr4) assert.Len(t, sigLayers(t, ref, digestStr), 2, "an existing same-identity layer whose signature does not verify must not suppress a valid re-sign") @@ -930,7 +930,7 @@ func TestAttachCosignSignatureCertBearingWithTlogAnnotatesBundle(t *testing.T) { require.NoError(t, err) pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", true) - _, attachErr5 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) + _, attachErr5 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil, nil) require.NoError(t, attachErr5) layers := sigLayers(t, ref, digestStr) @@ -954,7 +954,22 @@ func TestAttachCosignSignatureCertBearingWithTlogAnnotatesBundle(t *testing.T) { // does for the key-signed path) can never dedupe a keyless re-sign. Three // "runs" by the same signer identity — same SAN, same OIDC issuer, but a // brand new certificate and key each time — must collapse to one layer. -func TestAttachCosignSignatureCertDedupeSameIdentity(t *testing.T) { +// TestAttachCosignSignatureCertDedupeRequiresChainOfTrust is the regression +// test for a security fix: identity match (certificate SAN + OIDC issuer) +// alone must NOT dedupe a keyless signature. Both fields are ordinary, +// attacker-visible data embedded in a certificate — certBearingBundle's +// fixture certs are self-signed, exactly the shape a forged certificate +// carrying a victim's identity would take, and their signatures ARE +// internally self-consistent (each verifies against its own certificate's +// key). Before this fix, that self-consistency alone was enough to dedupe: +// this identical call, repeated three times, appended nothing after the +// first — a signing-pipeline bypass, since a registry writer's forged, +// self-signed, same-identity layer would be silently treated as "already +// signed," and the real caller's own signing operation would be skipped. +// The fix requires the existing layer's certificate to chain-verify against +// a trusted root before accepting a dedupe match; a self-signed certificate +// can never satisfy that, so this must now append every time. +func TestAttachCosignSignatureCertDedupeRequiresChainOfTrust(t *testing.T) { t.Parallel() reg := httptest.NewServer(registry.New()) t.Cleanup(reg.Close) @@ -966,12 +981,13 @@ func TestAttachCosignSignatureCertDedupeSameIdentity(t *testing.T) { for range 3 { pb := certBearingBundle(t, payload, "https://example.com/signer", "https://issuer.example.com", false) - _, attachErr6 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil) - require.NoError(t, attachErr6) + _, attachErr := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, nil, nil) + require.NoError(t, attachErr) } - assert.Len(t, sigLayers(t, ref, digestStr), 1, - "re-signing with the same keyless identity must not append a duplicate layer") + assert.Len(t, sigLayers(t, ref, digestStr), 3, + "a self-signed certificate — even with a matching SAN and OIDC issuer, both attacker-visible fields — "+ + "must never dedupe without chain-of-trust verification") } // TestAttachCosignSignatureCertAppendsDifferentIdentity covers the other @@ -990,18 +1006,18 @@ func TestAttachCosignSignatureCertAppendsDifferentIdentity(t *testing.T) { require.NoError(t, err) pbA := certBearingBundle(t, payload, "https://example.com/signer-a", "https://issuer.example.com", false) - _, attachErr7 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbA, nil) + _, attachErr7 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbA, nil, nil) require.NoError(t, attachErr7) require.Len(t, sigLayers(t, ref, digestStr), 1) pbB := certBearingBundle(t, payload, "https://example.com/signer-b", "https://issuer.example.com", false) - _, attachErr8 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbB, nil) + _, attachErr8 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbB, nil, nil) require.NoError(t, attachErr8) assert.Len(t, sigLayers(t, ref, digestStr), 2, "a different keyless signer SAN must not be deduped away") pbC := certBearingBundle(t, payload, "https://example.com/signer-a", "https://other-issuer.example.com", false) - _, attachErr9 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbC, nil) + _, attachErr9 := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pbC, nil, nil) require.NoError(t, attachErr9) assert.Len(t, sigLayers(t, ref, digestStr), 3, "a different OIDC issuer for the same SAN must not be deduped away") @@ -1028,7 +1044,7 @@ func TestAttachCosignSignatureKeySignedDedupeUnaffected(t *testing.T) { for range 3 { pb := keySignedBundle(t, payload, priv) _, attachErr10 := attachCosignSignature( - t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, priv.Public()) + t.Context(), authn.DefaultKeychain, ref, digestStr, payload, pb, priv.Public(), nil) require.NoError(t, attachErr10) }