From f2739c8cdd24a87b31b96b7127318991977ad3b8 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Tue, 16 Jun 2026 19:46:18 +0200 Subject: [PATCH] fix(verifier): require signature verification, reject timestamp-only bundles VerifyBundle only verified the DSSE signature when the bundle carried an X.509 certificate. A cert-less bundle with a valid RFC3161 timestamp fell through to the timestamp path and was reported as verified, even though the signature was never checked against any key. Because the timestamp signs only the signature bytes and not the payload, a forged statement with arbitrary signature bytes plus a trusted-TSA timestamp passed verification. Make signature verification mandatory: a certificate-backed signature must verify before the timestamp is considered, and the timestamp now only validates the signing window. Public-key bundles are rejected as unsupported rather than falling through to the timestamp-only path. Cert-less bundles with no verifiable key are reported as missing material instead of verified. Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: 18d8fc72-9d5f-4bb9-9268-b0eee03058db --- .../testdata/bundle_with_publickey.json | 17 +++++ pkg/attestation/verifier/verifier.go | 74 +++++++++++-------- pkg/attestation/verifier/verifier_test.go | 48 ++++++++++-- 3 files changed, 103 insertions(+), 36 deletions(-) create mode 100644 pkg/attestation/verifier/testdata/bundle_with_publickey.json diff --git a/pkg/attestation/verifier/testdata/bundle_with_publickey.json b/pkg/attestation/verifier/testdata/bundle_with_publickey.json new file mode 100644 index 000000000..c98865d5c --- /dev/null +++ b/pkg/attestation/verifier/testdata/bundle_with_publickey.json @@ -0,0 +1,17 @@ +{ + "mediaType": "application/vnd.dev.sigstore.bundle+json;version=0.3", + "verificationMaterial": { + "publicKey": { + "hint": "some-key-hint" + } + }, + "dsseEnvelope": { + "payload": "eyJfdHlwZSI6Imh0dHBzOi8vaW4tdG90by5pby9TdGF0ZW1lbnQvdjEifQ==", + "payloadType": "application/vnd.in-toto+json", + "signatures": [ + { + "sig": "TUVVQ0lRQ1JjOFVmeFVSMnBkL2UxVW1pVmhkRE5BZUxVRi83ODZ2WGxCT1VWM0dJcUFJZ0NqOTROczZwbzFQSzJjTW95MzBpOVB3Smx5c1E0R1RmTGI4TnZ5WlB1ckk9" + } + ] + } +} diff --git a/pkg/attestation/verifier/verifier.go b/pkg/attestation/verifier/verifier.go index 1698ebed1..5d8ca6b0a 100644 --- a/pkg/attestation/verifier/verifier.go +++ b/pkg/attestation/verifier/verifier.go @@ -41,6 +41,11 @@ type TrustedRoot struct { var ErrMissingVerificationMaterial = errors.New("missing material") var ErrInvalidBundle = errors.New("invalid bundle") +// ErrUnsupportedVerificationMaterial indicates the bundle carries verification +// material we cannot verify a signature against (e.g. a bare public key with no +// trusted key set). It is treated as a verification failure, never ignored. +var ErrUnsupportedVerificationMaterial = errors.New("unsupported verification material") + func VerifyBundle(ctx context.Context, bundleBytes []byte, tr *TrustedRoot) error { if bundleBytes == nil { return ErrMissingVerificationMaterial @@ -55,7 +60,6 @@ func VerifyBundle(ctx context.Context, bundleBytes []byte, tr *TrustedRoot) erro // fix for old attestations attestation.FixSignatureInBundle(bundle) - hasVerificationMaterial := false sb := &sigstorebundle.Bundle{Bundle: bundle} vc, err := sb.VerificationContent() if err != nil { @@ -64,44 +68,52 @@ func VerifyBundle(ctx context.Context, bundleBytes []byte, tr *TrustedRoot) erro } } - if vc != nil && vc.Certificate() != nil { - hasVerificationMaterial = true - signingCert := vc.Certificate() - - akiSum := sha256.Sum256(signingCert.AuthorityKeyId) - aki := hex.EncodeToString(akiSum[:]) - chain, ok := tr.Keys[aki] - if !ok { - return fmt.Errorf("trusted root not found for signing key with AKI %s", aki) + // Signature verification is MANDATORY + switch { + case vc != nil && vc.Certificate() != nil: + if err := verifyCertSignature(ctx, bundle, vc.Certificate(), tr); err != nil { + return err } + case bundle.GetVerificationMaterial().GetPublicKey() != nil: + // Public-key bundles are not supported at this time + return fmt.Errorf("%w: public key verification material", ErrUnsupportedVerificationMaterial) + default: + // No certificate and no public key: nothing to verify the signature against. + return ErrMissingVerificationMaterial + } - verifier, err := cosign.ValidateAndUnpackCertWithChain(signingCert, chain, &cosign.CheckOpts{IgnoreSCT: true}) - if err != nil { - return fmt.Errorf("validating the certificate: %w", err) - } + // The signature has been verified against a trusted certificate. The timestamp + // (if present) only validates the signing window; it can never be the sole + // verification material. + if err := VerifyTimestamps(sb, tr); err != nil && !errors.Is(err, ErrMissingVerificationMaterial) { + return fmt.Errorf("could not verify timestamps: %w", err) + } - dsseVerifier, err := dsse.NewEnvelopeVerifier(&sigdsee.VerifierAdapter{SignatureVerifier: verifier}) - if err != nil { - return fmt.Errorf("creating DSSE verifier: %w", err) - } + return nil +} - _, err = dsseVerifier.Verify(ctx, attestation.DSSEEnvelopeFromBundle(bundle)) - if err != nil { - return fmt.Errorf("validating the DSSE envelope: %w", err) - } +// verifyCertSignature validates the signing certificate against the trusted root +// chain and verifies the DSSE envelope signature with the certificate's key. +func verifyCertSignature(ctx context.Context, bundle *protobundle.Bundle, signingCert *x509.Certificate, tr *TrustedRoot) error { + akiSum := sha256.Sum256(signingCert.AuthorityKeyId) + aki := hex.EncodeToString(akiSum[:]) + chain, ok := tr.Keys[aki] + if !ok { + return fmt.Errorf("trusted root not found for signing key with AKI %s", aki) } - // Even with no cert (using a local key), we can still validate the timestamp - if err = VerifyTimestamps(sb, tr); err != nil { - if !errors.Is(err, ErrMissingVerificationMaterial) { - return fmt.Errorf("could not verify timestamps: %w", err) - } - } else { - hasVerificationMaterial = true + verifier, err := cosign.ValidateAndUnpackCertWithChain(signingCert, chain, &cosign.CheckOpts{IgnoreSCT: true}) + if err != nil { + return fmt.Errorf("validating the certificate: %w", err) } - if !hasVerificationMaterial { - return ErrMissingVerificationMaterial + dsseVerifier, err := dsse.NewEnvelopeVerifier(&sigdsee.VerifierAdapter{SignatureVerifier: verifier}) + if err != nil { + return fmt.Errorf("creating DSSE verifier: %w", err) + } + + if _, err := dsseVerifier.Verify(ctx, attestation.DSSEEnvelopeFromBundle(bundle)); err != nil { + return fmt.Errorf("validating the DSSE envelope: %w", err) } return nil diff --git a/pkg/attestation/verifier/verifier_test.go b/pkg/attestation/verifier/verifier_test.go index a2e795bef..6da3d9d47 100644 --- a/pkg/attestation/verifier/verifier_test.go +++ b/pkg/attestation/verifier/verifier_test.go @@ -23,9 +23,12 @@ import ( "os" "testing" + protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" + sigstorebundle "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/sigstore/sigstore/pkg/cryptoutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/encoding/protojson" ) func TestVerifyBundle(t *testing.T) { @@ -42,6 +45,9 @@ func TestVerifyBundle(t *testing.T) { roots *TrustedRoot bundle string expectErr string + // expectSentinel, when set, is asserted with errors.Is in addition to + // (or instead of) the substring match. + expectSentinel error }{ { name: "invalid bundle, but still verifiable", @@ -54,10 +60,11 @@ func TestVerifyBundle(t *testing.T) { bundle: "testdata/bundle_valid.json", }, { - name: "valid bundle without verification material", - roots: roots, - bundle: "testdata/bundle_valid_nomaterial.json", - expectErr: "missing material", + name: "valid bundle without verification material", + roots: roots, + bundle: "testdata/bundle_valid_nomaterial.json", + expectErr: "missing material", + expectSentinel: ErrMissingVerificationMaterial, }, { name: "corrupted bundle", @@ -71,6 +78,26 @@ func TestVerifyBundle(t *testing.T) { bundle: "testdata/dsse_envelope.json", expectErr: "invalid bundle", }, + { + // a cert-less bundle carrying only a timestamp must never be reported as verified. + // It is rejected at the mandatory-signature gate before timestamp validation runs, + // so the timestamp can never be the deciding factor. + name: "timestamp-only bundle (no signing key) is rejected", + roots: roots, + bundle: "testdata/bundle_with_bad_timestamp.json", + expectErr: "missing material", + expectSentinel: ErrMissingVerificationMaterial, + }, + { + // public-key bundles have no trusted key + // set to verify against and must fail rather than fall through to + // the timestamp-only path. + name: "public key bundle is rejected as unsupported", + roots: roots, + bundle: "testdata/bundle_with_publickey.json", + expectErr: "unsupported verification material", + expectSentinel: ErrUnsupportedVerificationMaterial, + }, } for _, tc := range cases { @@ -81,6 +108,10 @@ func TestVerifyBundle(t *testing.T) { if tc.expectErr != "" { assert.Error(t, err) assert.Contains(t, err.Error(), tc.expectErr) + if tc.expectSentinel != nil { + assert.True(t, errors.Is(err, tc.expectSentinel), + "expected %v, got: %v", tc.expectSentinel, err) + } return } assert.NoError(t, err) @@ -118,9 +149,16 @@ func TestVerifyTimestamps_TypedErrors(t *testing.T) { bundleBytes, err := os.ReadFile("testdata/bundle_with_bad_timestamp.json") require.NoError(t, err) + // VerifyTimestamps is exercised directly: the bad-timestamp fixture is a + // cert-less bundle, which VerifyBundle now rejects at the mandatory-signature + // gate before timestamp validation would run. + bundle := new(protobundle.Bundle) + require.NoError(t, protojson.Unmarshal(bundleBytes, bundle)) + sb := &sigstorebundle.Bundle{Bundle: bundle} + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := VerifyBundle(context.TODO(), bundleBytes, tc.roots) + err := VerifyTimestamps(sb, tc.roots) require.Error(t, err) assert.True(t, errors.Is(err, tc.expectSentinel), "expected %v, got: %v", tc.expectSentinel, err)