Skip to content

fix(provenance): apply the structural rules on the consumer side too - #146

Merged
imran-siddique merged 1 commit into
mainfrom
fix/verify-record-structural-rules
Aug 9, 2026
Merged

imran-siddique merged 1 commit into
mainfrom
fix/verify-record-structural-rules

Conversation

@imran-siddique

Copy link
Copy Markdown
Member

Closes #142.

Four structural rules lived only in build_record:

  • a tee-attested record must carry attestation evidence
  • an endpoint identity must carry spki_sha256
  • an artifact identity must carry digest
  • issued_at must be a usable timestamp

A record does not have to come from build_record. Anyone can write the JSON and sign it, so a rule on the producer side is a rule an attacker never runs. @lywinged's reproduction on #142 shows the consequence: a record claiming kind: "tee-attested" with attestation: null, an endpoint identity that is a bare URL, and no issued_at, signed by a key the consumer already trusts, passes both steps of §5. The consumer is handed the top trust tier with nothing behind it.

The rules move into a single _check_structure that both build_record and verify_record call. Copying them into the verifier would have worked today and drifted later, and the copy that matters is the consumer's.

This is the same principle #144 made explicit in the C2PA assertion next door: a check a verifier must not be able to skip is required, not offered. @lywinged drew that connection on the issue.

Tests. Nine cases, one per rule plus the reported forgery verbatim and a well-formed control. Run against the currently published agentrust-trace, eight of the nine fail and the control passes, so they are guarding the actual defect.

Compatibility. This rejects records the verifier previously accepted. Every such record is one build_record would have refused to emit, so anything produced through the SDK is unaffected.

🤖 Generated with Claude Code

Four rules were enforced only in build_record: a tee-attested record needs
attestation, an endpoint identity needs spki_sha256, an artifact identity needs
digest, and issued_at must be a usable timestamp. A record does not have to come
from build_record, so an attacker simply never runs them.

Reported in #142 with a reproduction: a hand-assembled record claiming
kind="tee-attested" with attestation: null, an endpoint identity that is a bare
URL, and no issued_at, signed by a key the consumer already trusts, passed both
steps of section 5.

The rules move into one _check_structure called by build_record and
verify_record, rather than being copied into the verifier, because two copies
drift and the copy that matters is the consumer's.

Closes #142

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lywinged

lywinged commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks, pulling the structural rules onto the verify path via one _check_structure
is exactly it, and rejecting bool for issued_at is a nice catch I'd missed.

Two smaller things from the bottom of #142 are in the same function and untouched by
this PR, if you want them in the same pass:

  • The regexes still anchor with $ (lines 43–44). In Python $ matches before a single
    trailing newline, so publisher = "did:web:acme.example\n" and a tool_catalog.hash
    with one trailing newline both still pass verify_record. \Z is the intended anchor.
    (The hash case fails safe — check_tool_catalog rejects it on the != — but the
    publisher one does not.)

  • The embedded-key guard is embedded != trusted_jwk (line 253), a dict comparison, so
    a legitimately signed record whose cnf.jwk carries an extra member like kid is
    refused even though the key material is identical. Comparing key material, or the JWK
    thumbprint, would avoid turning away honest publishers.

Both are minor next to the main fix; flagging only because they're the same few lines
you're already in.

@imran-siddique
imran-siddique merged commit acbf1bc into main Aug 9, 2026
8 checks passed
@imran-siddique
imran-siddique deleted the fix/verify-record-structural-rules branch August 9, 2026 23:45
lywinged added a commit to lywinged/trace-spec that referenced this pull request Aug 11, 2026
verify_record compared the record's cnf.jwk against the trusted key with dict
equality. A JWK is identified by the key material; kid, use and alg are optional
members that carry none of it. key_to_jwk emits the bare {crv, kty, x}, while a
key resolved from a JWKS endpoint normally carries kid, because that is how JWKS
distinguishes keys across rotation. The two are the same public key and not the
same dict, so a consumer holding its trusted key from a JWKS rejected every
record, with a message accusing the publisher of substituting a key:

    ProvenanceError: the record's embedded key is not the trusted key. A record
    signed by some other key is a record about a server somebody else is
    describing.

It was signed by exactly the right key. The signature verified; only the
comparison failed.

Compared by RFC 7638 thumbprint instead, which sign.jwk_thumbprint already
implements and whose docstring already states the property needed here: stable
across JWKs that differ in optional members such as kid, alg or use. A different
key still fails, and a cnf.jwk with no usable kty is now a refusal rather than an
uncaught ValueError.

This is the same comparison agentrust-io#157 has since landed in sign.verify_record, and it
is written the same way, compare_digest over the two thumbprints, so that the two
verifiers answer "is this the trusted key" identically. The error type differs
because each module raises its own: ProvenanceError here, ValueError there. After
agentrust-io#157 the divergence is visible in one tree, one verifier identifying a key by its
material and the other by its dict shape.

The regexes anchored with $, which in Python also matches before one final
newline, so publisher "did:web:acme.example\n" and a tool_catalog.hash with a
trailing newline passed verify_record. Both now anchor with \Z.

Raised on agentrust-io#146 and not part of that fix; agentrust-io#142 is closed, so both were untracked.

Six tests, each load-bearing: reverting the thumbprint comparison fails three,
reverting the anchors fails two. The embedded-key case is signed by hand, since
sign_record builds cnf itself and this implementation cannot emit a cnf.jwk
carrying kid - another implementation can, and the format permits it.

Rebased onto dd1b6e6. 345 tests pass; ruff and mypy clean.

Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
imran-siddique added a commit that referenced this pull request Sep 12, 2026
)

`build_record()` ran `int(issued_at ...)` and handed the result to
`_check_structure()`, whose guard carries the comment "bool is an int
subclass, and True would otherwise pass as a timestamp". The order defeated
it: True arrived as 1, False as 0, 1.9 as 1, "123" as 123, and -0.5 as 0, so
all five satisfied the non-negative-integer test and were written into the
record. A negative non-integer becoming an accepted timestamp is the
diagnostic case.

The same line leaked two exception classes the module does not document:
issued_at=[1] left build_record as a TypeError and issued_at="abc" as a
ValueError, where every other public function here is held to ProvenanceError.

An explicitly supplied value now reaches _check_structure() untouched and only
an omitted one is stamped with int(time.time()), which puts isinstance in front
of the conversion and closes both at once.

This is distinct from #142 and #146. Those moved the structural rules into the
shared helper, and the helper was always strict; the caller path defeated it by
normalizing first.

tests/test_public_functions_raise_what_they_document.py listed
provenance.build_record under NO_ARGUMENT_TO_SWEEP because it has no positional
argument, which is why the junk matrix never reached it. It is now wired into
that sweep with issued_at as the varied argument, with a witness pinning that
the sweep arrives.

Reported by @altrudev in #320. The two undocumented exception classes and the
reason the sweep never saw them were found by @lywinged.


Claude-Session: https://claude.ai/code/session_01X27MBo6tGVw1UmmchuGFnW

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

provenance.verify_record accepts records build_record refuses, including tee-attested with no evidence

2 participants