Strengthen signature-to-artifact binding in container/verifier

container/signer/cosign_attach.go

@@ -7,7 +7,6 @@ import (
 	"bytes"
 	"context"
 	"crypto"
-	"crypto/sha256"
 	"crypto/x509"
 	"encoding/base64"
 	"encoding/hex"
@@ -77,8 +76,9 @@ type cosignImage struct {
 // digest — is what gets signed, per the cosign convention: a verifier
 // recovers the payload from the signature manifest's layer, checks the
 // signature over it, and reads the bound manifest digest out of it.
-// Exported because offline re-verification of a stored key-signed bundle
-// must reconstruct exactly these bytes to check the signature's binding.
+// Exported so callers can reproduce and inspect the exact bytes a signature
+// covers; re-verifying a stored [Result.Bundle] does not need it, since the
+// stored form carries the payload itself.
 func SimpleSigningPayload(imageRef, digestStr string) ([]byte, error) {
 	ref, err := name.ParseReference(imageRef)
 	if err != nil {
@@ -92,7 +92,9 @@ func SimpleSigningPayload(imageRef, digestStr string) ([]byte, error) {
 		Critical: cosignCritical{
 			Identity: cosignIdentity{DockerReference: ref.Context().Name()},
 			Image:    cosignImage{DockerManifestDigest: d.String()},
-			Type:     "cosign container image signature",
+			// The verifier refuses a payload carrying any other type, so
+			// this must stay the value it checks for.
+			Type: verifier.CosignSignatureType,
 		},
 	}
 	return json.Marshal(payload)
@@ -384,15 +386,17 @@ func keylessAlreadySigned(
 // DefaultVerifierOptions — so only a genuinely Fulcio-issued, Rekor-logged
 // certificate can dedupe.
 //
-// The digest check runs before the (comparatively expensive) chain
+// The payload 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.
+// Comparing payload bytes — rather than the bundle's artifact digest, which
+// every signature on this artifact shares — is what makes it a check about
+// this signature at all.
 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[:]) {
+	if !bytes.Equal(b.SimpleSigningPayload, payload) {
 		return false
 	}
 	vr, err := verifier.VerifyBundle(b, tm, nil, opts...)

container/signer/keyless_test.go

@@ -42,7 +42,6 @@ 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"
@@ -637,12 +636,19 @@ func TestSignOCIKeylessRoundTrip(t *testing.T) {
 	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.
+	// PayloadDigest names the blob the signature covers — the attached
+	// layer's own digest, per 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)
+
+	// The retrieved bundle, though, binds the ARTIFACT digest: that is what
+	// a caller has, and the payload it carries is what ties the two
+	// together.
+	assert.Equal(t, strings.TrimPrefix(digestStr, "sha256:"), bundles[0].DigestHex)
+	expectedPayload, err := SimpleSigningPayload(ref, digestStr)
+	require.NoError(t, err)
+	assert.Equal(t, expectedPayload, bundles[0].SimpleSigningPayload)
 }
 
 // TestSignOCIKeylessBundleVerifies is the test that matters: a bundle this
@@ -898,8 +904,8 @@ func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) {
 	// 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)
+	firstSig, firstCert := decodeBundleSignatureAndCert(t, first.Bundle, digestStr)
+	secondSig, secondCert := decodeBundleSignatureAndCert(t, second.Bundle, digestStr)
 	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")
@@ -910,7 +916,7 @@ func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) {
 	// "some" pair that happens to match.
 	third, err := signer.SignOCI(t.Context(), ref, digestStr, opts)
 	require.NoError(t, err)
-	thirdSig, thirdCert := decodeBundleSignatureAndCert(t, third.Bundle)
+	thirdSig, thirdCert := decodeBundleSignatureAndCert(t, third.Bundle, digestStr)
 	assert.Equal(t, firstSig, thirdSig)
 	assert.Equal(t, firstCert, thirdCert)
 }
@@ -919,13 +925,13 @@ func TestSignOCIKeylessDedupeReturnsAttachedBundle(t *testing.T) {
 // 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) {
+func decodeBundleSignatureAndCert(t *testing.T, raw []byte, artifactDigest string) (sig, certDER []byte) {
 	t.Helper()
-	var bun verifybundle.Bundle
-	require.NoError(t, bun.UnmarshalJSON(raw))
-	msgSig := bun.GetMessageSignature()
+	stored, err := verifier.DecodeStoredBundle(raw, artifactDigest)
+	require.NoError(t, err)
+	msgSig := stored.Parsed.GetMessageSignature()
 	require.NotNil(t, msgSig)
-	cert, err := certMaterialFromBundle(bun.Bundle)
+	cert, err := certMaterialFromBundle(stored.Parsed.Bundle)
 	require.NoError(t, err)
 	require.NotNil(t, cert)
 	return msgSig.GetSignature(), cert.certDER
@@ -962,13 +968,13 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) {
 	require.NoError(t, err)
 	otherResult, err := signer.SignOCI(t.Context(), ref, otherDigestStr, opts)
 	require.NoError(t, err)
-	var otherBun verifybundle.Bundle
-	require.NoError(t, otherBun.UnmarshalJSON(otherResult.Bundle))
+	otherStored, err := verifier.DecodeStoredBundle(otherResult.Bundle, otherDigestStr)
+	require.NoError(t, err)
 
 	// 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, nil)
+	attached, err := attachCosignSignature(t.Context(), authn.DefaultKeychain, ref, digestStr, otherPayload, otherStored.Parsed.Bundle, nil, nil)
 	require.NoError(t, err)
 	require.True(t, attached)
 	require.Len(t, sigLayers(t, ref, digestStr), 1)
@@ -980,7 +986,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) {
 	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)
+	validSig, validCert := decodeBundleSignatureAndCert(t, valid.Bundle, digestStr)
 
 	// Re-sign once more with the same identity: this dedupes (the valid
 	// layer from above verifies), so SignOCI must return THAT layer's
@@ -990,7 +996,7 @@ func TestSignOCIKeylessDedupeSkipsUnverifiableExistingLayer(t *testing.T) {
 	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)
+	redundantSig, redundantCert := decodeBundleSignatureAndCert(t, redundant.Bundle, digestStr)
 	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)

container/signer/signer.go

@@ -139,24 +139,34 @@ type Result struct {
 	// Bundle is the serialized Sigstore bundle, for durable storage and
 	// later offline re-verification.
 	//
+	// Re-verify it against the ARTIFACT digest — the same digest passed to
+	// SignOCI — with [verifier.VerifyBundleOffline] or
+	// [verifier.VerifyBundleOfflineWithKey]. Bundle is not bare Sigstore
+	// bundle JSON: it wraps the bundle together with the simple-signing
+	// payload the signature covers, because that payload is the only thing
+	// tying a cosign signature to an artifact. See
+	// [verifier.StoredBundleMediaType] for the shape, and
+	// [verifier.DecodeStoredBundle] to unwrap it.
+	//
 	// 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
+	// freshly attached signature wraps 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 "<algorithm>:<hex>" digest of the simple-signing
-	// payload the bundle actually signs.
+	// payload the signature covers.
 	//
-	// This is deliberately surfaced because it is NOT the artifact digest
-	// passed to SignOCI. Following the cosign convention, the signature
-	// covers a payload that *embeds* the artifact digest rather than the
-	// digest itself, so verifying Bundle offline requires this value —
-	// passing the artifact digest to a bundle verifier will always fail.
-	// See [PayloadDigest] to recompute it from a reference and digest alone.
+	// It is NOT the artifact digest passed to SignOCI: following the cosign
+	// convention, the signature covers a payload that *embeds* the artifact
+	// digest rather than the digest itself. Verifying Bundle does not
+	// require it — Bundle carries the payload, and the verifier entry points
+	// take the artifact digest — so this is informational: it identifies the
+	// blob in the attached signature manifest that this signature signs. See
+	// [PayloadDigest] to recompute it from a reference and digest alone.
 	PayloadDigest string
 }
 
@@ -414,7 +424,16 @@ func resultBundleJSON(
 		if err != nil {
 			return nil, fmt.Errorf("serializing sigstore bundle: %w", err)
 		}
-		return raw, nil
+		// Persist the payload with the bundle. The signature covers the
+		// payload, and only the payload names the artifact — a bundle
+		// stored without it can be verified but no longer bound, which is
+		// the difference between "someone signed this artifact" and
+		// "someone signed something". See verifier.StoredBundleMediaType.
+		stored, err := verifier.EncodeStoredBundle(raw, payload)
+		if err != nil {
+			return nil, fmt.Errorf("serializing sigstore bundle: %w", err)
+		}
+		return stored, nil
 	}
 	return previouslyAttachedBundleJSON(ctx, keychain, ref, digestStr, payload, pb, pub, tm)
 }
@@ -578,9 +597,14 @@ func keylessBundleOptions(ctx context.Context, opts Options) (sign.BundleOptions
 // PayloadDigest returns the digest of the simple-signing payload that a
 // signature over the artifact at ref pinned to digestStr covers.
 //
-// Consumers verifying a stored bundle generally hold only the reference and
-// the artifact digest — not the [Result] from signing — so this is the
-// supported way to recover the value a bundle verifier needs.
+// It is informational, for inspecting or cross-referencing the blob inside an
+// attached cosign signature manifest. Verification does not need it and must
+// not be given it: every verifier entry point — including
+// [verifier.VerifyBundleOffline] and [verifier.VerifyBundleOfflineWithKey] —
+// takes the ARTIFACT's own manifest digest, the same digestStr passed here,
+// and recovers the payload from the stored bundle itself. Passing this value
+// where an artifact digest is expected fails with
+// [verifier.ErrSignatureArtifactMismatch].
 func PayloadDigest(imageRef, digestStr string) (string, error) {
 	payload, err := SimpleSigningPayload(imageRef, digestStr)
 	if err != nil {

container/signer/signer_test.go

@@ -36,7 +36,6 @@ import (
 	protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1"
 	protocommon "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
 	protorekor "github.com/sigstore/protobuf-specs/gen/pb-go/rekor/v1"
-	"github.com/sigstore/sigstore-go/pkg/bundle"
 	fulciocert "github.com/sigstore/sigstore-go/pkg/fulcio/certificate"
 	"github.com/sigstore/sigstore/pkg/cryptoutils"
 	"github.com/stretchr/testify/assert"
@@ -92,14 +91,16 @@ func pushTestArtifact(t *testing.T, registryHost string) (ref string, digestStr
 // verifyKeyBundle verifies a signing result against the public key, through
 // the sibling verifier package's real entry point.
 //
-// It uses res.PayloadDigest rather than recomputing a digest, which is the
-// point: a consumer holding only the Result must be able to verify it. When
-// this helper had to rebuild the simple-signing payload itself, the test
-// passed while the public API was unusable as documented.
-func verifyKeyBundle(t *testing.T, res *Result, pubPEM []byte) error {
+// It passes the ARTIFACT digest — the same value handed to SignOCI — which is
+// the point: a consumer holding the Result and the digest it asked to sign
+// must be able to verify it, without knowing that cosign signatures cover a
+// payload rather than the artifact. When this helper had to rebuild the
+// simple-signing payload itself, the test passed while the public API was
+// unusable as documented.
+func verifyKeyBundle(t *testing.T, res *Result, artifactDigest string, pubPEM []byte) error {
 	t.Helper()
 	require.NotEmpty(t, res.PayloadDigest, "a signing result must carry the digest it signed")
-	_, err := verifier.VerifyBundleOfflineWithKey(res.Bundle, res.PayloadDigest, pubPEM)
+	_, err := verifier.VerifyBundleOfflineWithKey(res.Bundle, artifactDigest, pubPEM)
 	return err
 }
 
@@ -116,12 +117,12 @@ func TestSignOCIRoundTrip(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, raw.Bundle)
 
-	// The returned bundle verifies against the signing key over the
-	// simple-signing payload digest.
+	// The returned bundle verifies against the signing key, bound to the
+	// artifact digest.
 	payload, err := SimpleSigningPayload(ref, digestStr)
 	require.NoError(t, err)
 	payloadDigest := sha256.Sum256(payload)
-	require.NoError(t, verifyKeyBundle(t, raw, pubPEM),
+	require.NoError(t, verifyKeyBundle(t, raw, digestStr, pubPEM),
 		"the returned bundle must verify against the signing key")
 
 	// The attached signature manifest reconstructs to the SAME signature:
@@ -151,9 +152,13 @@ func TestSignOCIRoundTrip(t *testing.T) {
 		"the signature manifest layer must be the exact signed payload")
 
 	// The annotation signature matches the bundle's message signature.
-	parsed := &bundle.Bundle{}
-	require.NoError(t, parsed.UnmarshalJSON(raw.Bundle))
-	bundleSig := parsed.Bundle.GetMessageSignature().GetSignature()
+	// Result.Bundle is the stored form (bundle + payload), so it is unwrapped
+	// through the verifier rather than parsed as a bare sigstore bundle.
+	stored, err := verifier.DecodeStoredBundle(raw.Bundle, digestStr)
+	require.NoError(t, err)
+	assert.Equal(t, payload, stored.SimpleSigningPayload,
+		"the stored bundle must carry the exact payload the signature covers")
+	bundleSig := stored.Parsed.GetMessageSignature().GetSignature()
 	annotationSig, err := base64.StdEncoding.DecodeString(layer.Annotations[annotationCosignSignature])
 	require.NoError(t, err)
 	assert.Equal(t, bundleSig, annotationSig,
@@ -173,7 +178,7 @@ func TestSignOCIRejectsWrongKeyVerification(t *testing.T) {
 	raw, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{Key: keyPath})
 	require.NoError(t, err)
 
-	require.Error(t, verifyKeyBundle(t, raw, otherPub),
+	require.Error(t, verifyKeyBundle(t, raw, digestStr, otherPub),
 		"a different key must not verify the bundle")
 }
 
@@ -398,7 +403,7 @@ func TestSignOCIAcceptsCosignCLIKeyFormat(t *testing.T) {
 			raw, err := NewDefault(nil).SignOCI(t.Context(), ref, digestStr, Options{Key: keyPath})
 			require.NoError(t, err, "a key from `cosign generate-key-pair` must be usable")
 
-			require.NoError(t, verifyKeyBundle(t, raw, pubPEM))
+			require.NoError(t, verifyKeyBundle(t, raw, digestStr, pubPEM))
 		})
 	}
 }
@@ -461,8 +466,8 @@ func TestSignOCIAppendsRatherThanReplacingSignatures(t *testing.T) {
 
 	// Both signatures must still verify — the manifest carries two distinct
 	// annotations, one per key.
-	require.NoError(t, verifyKeyBundle(t, rawA, pubA))
-	require.NoError(t, verifyKeyBundle(t, rawB, pubB))
+	require.NoError(t, verifyKeyBundle(t, rawA, digestStr, pubA))
+	require.NoError(t, verifyKeyBundle(t, rawB, digestStr, pubB))
 
 	sigs := map[string]bool{}
 	for _, l := range layers {
@@ -512,19 +517,26 @@ func TestResultCarriesTheDigestItSigned(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.NotEqual(t, digestStr, res.PayloadDigest,
-		"the signed digest is the payload's, not the artifact's — if these ever match, the contract changed")
-
-	// The artifact digest must NOT verify the bundle. This is the mistake
-	// the previous signature invited.
+		"the signed blob is the payload, not the artifact — if these ever match, the contract changed")
+
+	// The ARTIFACT digest verifies the stored bundle. This is the contract:
+	// a caller holds the digest it asked to have signed, and that is the
+	// value the verifier takes. Reaching it requires the stored bundle to
+	// carry the payload, so that the signature can be checked against what
+	// it actually covers AND the payload's claim about which artifact it
+	// covers can be checked against digestStr.
 	_, err = verifier.VerifyBundleOfflineWithKey(res.Bundle, digestStr, pubPEM)
-	require.Error(t, err, "the artifact digest must not verify a payload-bound bundle")
+	require.NoError(t, err, "the artifact digest must verify the stored bundle")
 
-	// The digest the Result reports must.
-	_, err = verifier.VerifyBundleOfflineWithKey(res.Bundle, res.PayloadDigest, pubPEM)
-	require.NoError(t, err)
+	// A different artifact's digest must not, even though the bundle's own
+	// signature is perfectly valid: the payload names this artifact.
+	otherArtifact := "sha256:" + strings.Repeat("ab", 32)
+	_, err = verifier.VerifyBundleOfflineWithKey(res.Bundle, otherArtifact, pubPEM)
+	require.ErrorIs(t, err, verifier.ErrSignatureArtifactMismatch,
+		"a bundle must not verify against an artifact its payload does not name")
 
-	// And PayloadDigest recomputes the same value from ref + digest alone,
-	// which is all a later consumer has.
+	// PayloadDigest stays available and still identifies the signed blob in
+	// the attached signature manifest, recomputable from ref + digest alone.
 	recomputed, err := PayloadDigest(ref, digestStr)
 	require.NoError(t, err)
 	assert.Equal(t, res.PayloadDigest, recomputed)