[samuv]

The bug

isVerificationResultMatchingServerProvenance gated the attestation comparison behind a four-way AND (container/verifier/verifier.go:170):

// If the attestations are not set, we can skip this check
if p.Attestation != nil && r.Statement != nil && p.Attestation.Predicate != nil && r.Statement.Predicate != nil {
    if p.Attestation.PredicateType != r.Statement.PredicateType {
        return false
    }
    return reflect.DeepEqual(p.Attestation.Predicate, r.Statement.Predicate)
}

return true

When the provenance declared an attestation but the artifact carried none — r.Statement == nil, or a statement with no predicate — the guard was false, control fell through to return true, and the artifact verified. The constraint was skipped in exactly the case it exists to catch.

The comment is only sound for the expected side. Applying it to the actual side is the defect.

A second bug in the opposite direction

The same expression hid a second problem: when both sides were populated, the comparison could never succeed.

p.Attestation.Predicate is decoded registry data. r.Statement.Predicate is a *structpb.Struct (in-toto/attestation@v1.2.0, statement.pb.go:34). reflect.DeepEqual on two different dynamic types is unconditionally false, so any entry that set predicate was unverifiable. Confirmed empirically:

expected dyn type: map[string]interface {}
actual   dyn type: *structpb.Struct
DeepEqual(expected, actual) = false

So the one expression was fail-open when the artifact had no attestation, and fail-closed-but-unusable when it did.

The fix

Extracted into compareAttestation, following the per-field convention compareBaseProperties already uses — an unset expectation means "don't constrain that dimension", not "skip the check entirely":

Expected	Behaviour
p.Attestation == nil	unconstrained, skip
p.Attestation != nil	require r.Statement != nil
PredicateType != ""	must equal r.Statement.PredicateType
Predicate != nil	both sides normalized through structpb, then compared
&VerifiedAttestation{}	must carry some attestation, without constraining which

The explicit r.Statement.Predicate == nil check is load-bearing: structpb's AsMap() is nil-safe and returns an empty map, so without it an expected {} would match a missing predicate.

VerifyServer's signature is unchanged and compareBaseProperties keeps its existing semantics.

Predicate equality is independent of serialisation format

Normalising only the actual side would leave equality dependent on how the registry entry was decoded. AsMap() renders every number as float64; encoding/json decodes numbers to float64, but yaml.v3 decodes them to int. Two semantically identical predicates would verify from JSON and fail from YAML. registry.Provenance carries both json and yaml tags and registry data is YAML-decodable, so that is reachable, not theoretical.

So both sides round through structpb before comparison:

expected, err := normalizeAttestationPredicate(p.Attestation.Predicate)
if err != nil {
    // An expected predicate we cannot normalise is one we cannot
    // confirm, so treat it as a mismatch rather than a pass.
    slog.Error("cannot normalize expected attestation predicate", "error", err)
    return false
}
if !reflect.DeepEqual(expected, r.Statement.Predicate.AsMap()) {
    return false
}

normalizeAttestationPredicate asserts the expectation is an object, builds a *structpb.Struct, and returns its AsMap(). Comparing AsMap() on both sides yields plain Go maps with consistent types — which also avoids reflect.DeepEqual on protobuf messages and their internal state fields.

Verified the two decoders disagree before normalisation and agree after, for nested objects and lists as well as top-level scalars:

yaml: map[string]interface {}{"attempt":1,   "list":[]interface {}{3},   "nested":map[string]interface {}{"count":7}}
json: map[string]interface {}{"attempt":1.0, "list":[]interface {}{3.0}, "nested":map[string]interface {}{"count":7.0}}
raw DeepEqual        = false
normalized DeepEqual = true

Normalisation introduces two failure modes, and neither may recreate the old fall-through. Both return no-match:

• Non-object expectation — an in-toto predicate is always an object, so a scalar expectation cannot match one.
• Unrepresentable expectation — a value structpb cannot encode is one we cannot confirm.

Behaviour change

A server entry that declares provenance.attestation and passes today against an unattested artifact will now fail. That's the intent.

I grepped toolhive-catalog (both registries/ and pkg/catalog/toolhive/data/*.json) and toolhive/pkg for an "attestation": key under provenance — zero hits. The only matches anywhere are the word "attestations" inside MCP tool descriptions and swagger $defs. Nothing regresses on upgrade, so this needs a release note for downstream authors rather than a migration.

Downstream toolhive picks this up on a go.mod bump.

Tests

isVerificationResultMatchingServerProvenance had no direct test. Added table-driven coverage for the guard clauses and every attestation branch.

Reverting verifier.go to the pre-fix logic fails 7 cases, covering both directions:

• Fail-open: expected attestation but statement absent (the regression), empty expected attestation but statement absent, expected predicate but statement carries none, predicate type mismatch
• Never-matched: predicate type and predicate both match, predicate matches with predicate type unconstrained, numeric predicate matches as float64

Decoder independence gets a dedicated test. It decodes the same provenance from a JSON literal and a YAML literal — numbers at top level, nested in an object, and inside a list — and asserts both verify against one statement. It guards its own premise first, so it cannot silently go vacuous if the decoders ever converge:

require.NotEqual(t, fromJSON.Attestation.Predicate, fromYAML.Attestation.Predicate,
    "decoders expected to disagree on numeric types; test is vacuous otherwise")

Alongside it, numeric predicate matches as int and matches as float64 both assert a match, and numeric predicate still mismatches on value pins that normalisation widens what is accepted without weakening value comparison. The non-object and unrepresentable predicates are covered too.

task passes clean (lint 0 issues, all tests green). task license-check passes. Package coverage 79.6%, with isVerificationResultMatchingServerProvenance, compareAttestation, and normalizeAttestationPredicate all at 100%. No live Sigstore or network needed — verifier_integration_test.go is separate and network-gated.

One thing worth a reviewer's eye

go.mod churn. github.com/in-toto/attestation and google.golang.org/protobuf move from // indirect to direct — same versions, no new modules, go.sum unchanged. Unavoidable: VerificationResult.Statement is *in_toto.Statement, so any direct test of this function must import it, and the normalisation uses structpb in non-test code. Flagging it against the "don't modify go.mod deps" rule since it's go mod tidy bookkeeping rather than a dependency decision.

Follow-on (not in this PR)

Once this lands, skill.schema.json's $defs/provenance can accept attestation again — #247 removed it deliberately because of this bug, as a stopgap. That is better done as a follow-up than by rebasing #247 onto this, so the security fix isn't gated on the schema change. Suggest keeping "additionalProperties": false regardless: it turns a misspelled constraint key into a loud error instead of a silently dropped security guarantee.

The separate question of renaming VerifyServer → Verify, now that it has non-server callers, is its own discussion and is not folded in here.

🤖 Generated with Claude Code