fix(provenance): anchor with \Z, so a trailing newline is not part of an identifier - #154
Conversation
… an identifier
In Python `$` matches at the end of the string *and* immediately before a single
trailing newline. Both patterns in this module are anchored `^...$`, so every
field they guard accepted its own value with a newline glued to the end, and
`verify_record` returned cleanly.
Four fields, since two patterns are reused:
publisher "did:web:acme.example\n" accepted
artifact.digest "sha256:...\n" accepted
endpoint.spki_sha256 "sha256:...\n" accepted
tool_catalog.hash "sha256:...\n" accepted by verify_record
Only the last has anything downstream to catch it, and only because
`check_tool_catalog` recomputes the digest and compares. The other three are
accepted and nothing later disagrees.
`\Z` is the end of the string and nothing else. One comment on the pair says why,
because the two anchors look interchangeable and are not.
Four parametrized cases, one per field. They fail on all four before this change
and pass after, which I checked rather than assumed. 327 passed, 1 skipped; ruff
and mypy clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: lywinged <48041247+lywinged@users.noreply.github.com>
|
❔ Contributor Check: UNKNOWN
Automated check by AgenTrust Contributor Check. |
imran-siddique
left a comment
There was a problem hiding this comment.
Confirmed the behaviour directly rather than from the description: re.match(r'^sha256:[0-9a-f]{4}$', 'sha256:abcd\n') matches, and the same pattern with \Z does not. So all four fields guarded by these two patterns accepted a trailing newline, and verify_record returned cleanly on three of them with nothing downstream to disagree.
The parametrized test is the right shape here: the bug is one property of two shared patterns, and covering all four fields means a future field added to either pattern inherits the guard instead of quietly re-opening it.
Merging this first. #149 now carries the same two lines after your rebase, so I will rebase it down to just the thumbprint comparison.
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 landed in sign.verify_record, 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.
Rebased onto 5367970. The \Z anchor change this branch also carried is now
upstream via agentrust-io#154, so it is dropped here along with the two newline tests it
duplicated; agentrust-io#154's four parametrized cases cover the same ground and more.
Four tests, each load-bearing: reverting the thumbprint comparison fails three.
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.
347 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>
…pe (#149) 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 #157 landed in sign.verify_record, 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. Rebased onto 5367970. The \Z anchor change this branch also carried is now upstream via #154, so it is dropped here along with the two newline tests it duplicated; #154's four parametrized cases cover the same ground and more. Four tests, each load-bearing: reverting the thumbprint comparison fails three. 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. 347 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>
Per the review: max_age_seconds had none of the validation max_future_skew_seconds had, so max_age_seconds=-1 was not reported as an invalid verifier configuration - it classified every record ever issued as stale, uniformly, with no error naming the cause. Both parameters now go through one _check_seconds, which rejects non-integers and negatives and excludes bool explicitly, since bool subclasses int in Python and True would otherwise pass as one second. Boundary tests cover negative, boolean, float, string and object inputs for each parameter, plus the case that motivates keeping them distinct: 0 is the strictest expressible bound and None disables the bound, and a validator that treated 0 as falsy would silently accept everything under the strictest policy a caller can write. Merged main in as well (the branch predated agentrust-io#149 and agentrust-io#154); 424 pass, ruff clean. Removing the max_age check fails 4 tests.
Follows the anchoring point from my review comment on #146, which nobody has picked up. Two lines of pattern and one parametrized test.
What it is. In Python
$matches at the end of the string and immediately before a single trailing newline. Both patterns in this module are anchored^...$, so every field they guard accepts its own value with a newline glued to the end, andverify_recordreturns cleanly.Two patterns, four fields:
Only the last has anything downstream to catch it, and only because
check_tool_catalogrecomputes the digest and compares. The other three are accepted and nothing later disagrees.\Zis the end of the string and nothing else. One comment sits on the pair, because the two anchors look interchangeable and are not.Correcting my own report. On #146 I said this affected two fields and that "the hash case fails safe". Both halves were too narrow. It is four fields, because the two patterns are reused, and fails safe is true of
tool_catalog.hashand false ofartifact.digest, which is the same digest pattern with no recomputation behind it. I had checked one field and described the pattern.Verified. Four parametrized cases, one per field. They fail on all four before the change and pass after; I ran it both ways rather than assuming. 327 passed, 1 skipped.
ruffandmypyclean.Not in this PR: the regexes are permissive in other ways.
did:[a-z0-9]+:.+accepts a DID with a space in it, and.+accepts a great deal. That is a separate question from anchoring and I have not touched it.