Repository navigation
tools(acta_receipt_verifier): reject private JWKs, non-canonical base64url keys, and non-canonical signature.sig hex - #608
Conversation
d6e7d4a to
a36e946
Compare
…64url keys, and non-canonical signature.sig hex
- Reject JWKs carrying RFC 7517 / RFC 7518 / RFC 8037 private or symmetric
key parameters (d, p, q, dp, dq, qi, oth, k) or non-canonical unpadded
base64url x values in issuer_key with Undecidable("malformed_key_set").
- Require signature.sig in signature_verifies to match [0-9a-f]{128}\Z before
hex decoding, rejecting uppercase hex and ASCII whitespace.
- Reject duplicate JSON object keys and catch RecursionError in _read_json as
Undecidable("malformed"), and catch RecursionError / UnicodeEncodeError in
signature_verifies and chain_link_matches.
Closes agentrust-io#605
Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com>
Co-authored-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
a36e946 to
60bc7f2
Compare
dinakarjs
left a comment
There was a problem hiding this comment.
Reviewed 60bc7f2. Independent Linux/Python 3.12.14 run with the repository's hash-locked dev dependencies: python -m pytest tests/test_acta_receipt_verifier.py tests/test_acta_fixtures.py -q gives 160 passed; targeted Ruff and diff whitespace checks pass.
The changed path validates the signature's lowercase fixed-width form before decoding, requires a canonical unpadded public-key encoding, rejects supplied private/symmetric JWK members, and maps duplicate JSON keys/deep input failures to the documented undecidable outcome. Uncanonicalizable predecessor and payload failures stay scoped to chain-link/signature failure. The selected tests retain valid signed fixtures and meaningful negative controls. No additional blocker found in this bounded source review. I did not rerun the author's guard-removal trials or a live receipt service; this is not normative acceptance of the external draft. Required maintainer disposition remains separate. Reviewed with OpenAI Codex.
lywinged
left a comment
There was a problem hiding this comment.
REVIEW on #608, head 60bc7f2: Approve
Measured on 60bc7f2 with the hash-pinned requirements, on 3.11 and on 3.12, the steps ci.yml runs: ruff with tools/acta_receipt_verifier.py added, as the description runs it, mypy on src/agentrust_trace, mypy --strict on the tool, which the project's strict configuration does not cover, and the dash check, all clean; 4716 passed and 48 skipped on both; the verifier context replay experiment 116 passed and 4 skipped; the portability step 4715 passed and 49 skipped. tests/test_acta_receipt_verifier.py collects 80 and the two Acta files 160, as the description says. Against the merge base 295c36d the review bar sorts the 36 cases added here as failing on behaviour and the 44 the merge base already had as pass or skip, which is the description's count, and the description measures on this head, after the two force-pushes. The change does what #605 asks, all three gaps, and the CHANGELOG entry under Unreleased says what the code does.
-
Confirmed, on the whole suite on a copy, one mutation at a time, 4716 passing before any, each guard failing its own cases and nothing else. The signature regex removed fails the six cases of
test_a_non_canonical_signature_hex_is_refused. The private-parameter check removed fails the eight oftest_a_jwk_carrying_private_key_material_is_refused. The duplicate-key hook removed fails the four oftest_duplicate_json_keys_are_undecidable.RecursionErrorleft out of_read_json's except fails the three oftest_deeply_nested_json_is_undecidable. The two JCS guards narrowed back to what the merge base caught fail the five oftest_an_uncanonicalizable_predecessor_or_deep_payload_does_not_crash. Two narrower fixes a reviewer might have written instead: the alphabet and length checks onxkept with the round-trip comparison dropped fails exactly the three trailing-bit cases oftest_a_jwk_with_non_canonical_base64url_x_is_refused, so that comparison is what those three hold, and a case-insensitive signature regex,[0-9a-fA-F]{128}, failsuppercaseandmixed_caseand passes the four whitespace cases, so the lowercase decision rests on those two cases alone.bytes.fromhexaccepts uppercase, leading and trailing whitespace and a space or a tab between byte pairs, measured on both interpreters, which is the gap #605 describes. The elevensigvalues in the committed fixtures are lowercase, the test module's docstring cites section 6.6 for the rule, andgen.mjsrecords the same rule from revision 02, "sig = 128 lowercase hex chars" under its 2.1.1. -
The verdict classes, which the tests fix without saying so. For a predecessor that JCS cannot serialize,
chain_link_mismatchis fair where nothing conforming could have been signed, aNaNor a lone surrogate. A payload nested 1500 deep, or a payload or predecessor holding 2 to the 53, is different: both are legal JSON,gen.mjswould serialize the integer, since itscanonicalizeisJSON.stringifyfor a number and node prints 9007199254740992 for it, measured, while Python's rfc8785 refuses it, so the verdict for either,signature_invalidorchain_link_mismatch, is the verifier's library speaking rather than the receipt. Which class the depth gets also depends on the interpreter: a signed envelope with 1500 levels added to its payload after signing issignature_invalidin memory on both, and read from a file it ismalformedon 3.11 andsignature_invalidon 3.12, measured, becausejson.loadsgives up below a thousand levels on 3.11 and near ten thousand on 3.12 whilejcsgives up near a thousand on both. The test adds the nesting after signing, so it cannot tell the classes apart. Whether depth should beundecidableon every path is a question about the tool's verdict table rather than about this change, and I would not hold the change for it. One more of the same kind:_read_jsonstill takes theNaNandInfinityliterals thatjson.loadsaccepts by default andjson.dumpswrites, so a predecessor file holding one ischain_link_mismatchrather thanmalformed, measured.parse_constantis the one-line way to make the parse-time rule uniform if you want it. -
Small things. The template has no box for a tooling fix, and CONTRIBUTING names tooling as its own kind beside editorial changes, so "Editorial (typo, link fix, clarification: no normative effect)" is checked here for a change that makes the verifier refuse input it accepted. Leaving all five unchecked reads truer.
_envelope_partsalready refuses a non-stringsig, so theisinstanceinsignature_verifiesserves direct callers only, whichtests/test_acta_fixtures.pyis, so it is right to keep.
Approve. Nothing above waits on anything.
…n at merge The entry this pull request proposed is written on main at merge by the merging maintainer, as CONTRIBUTING step 5 now asks, so open pull requests stop conflicting on the one file they would all touch. Signed-off-by: lywinged <48041247+lywinged@users.noreply.github.com>
|
Re-approving at |
dinakarjs
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 97ff392, including the changes since my earlier approval. The receipt verifier now rejects duplicate JSON members, private verification JWK fields, noncanonical base64url and malformed signatures through the documented refusal path. Local receipt/fixture tests: 160 passed; targeted Ruff and git diff checks passed. Current hosted CI, conformance and review-bar checks are successful. No blocking finding in the changed paths.
What this changes
Closes #605.
Hardens
tools/acta_receipt_verifier.py(the reference verifier fordraft-farley-acta-signed-receipts-03exercised bytests/test_acta_fixtures.pyandtests/test_acta_receipt_verifier.py) across three input and verification boundaries:Private JWK parameters and canonical unpadded base64url
xinissuer_key(tools/acta_receipt_verifier.py):issuer_key(jwks, kid)now rejects any JWK carrying RFC 7517 / RFC 7518 / RFC 8037 private or symmetric key parameters ("d","p","q","dp","dq","qi","oth","k") withUndecidable("malformed_key_set").issuer_key(jwks, kid)now requiresxto be canonical unpadded base64url ([A-Za-z0-9_-]+\Z,len(x) % 4 != 1, andbase64.urlsafe_b64encode(raw).rstrip(b"=").decode("ascii") == x), raisingUndecidable("malformed_key_set")on=padding, standard-alphabet+or/, ASCII whitespace or ignored non-alphabet characters, or non-zero trailing bits in the final character.Lowercase 128-character hex
signature.siginsignature_verifies(tools/acta_receipt_verifier.py):signature_verifies(envelope, key)now validates thatenvelope["signature"]["sig"]is astrmatching[0-9a-f]{128}\Zbefore callingbytes.fromhex(sig), returningFalse(("invalid", "signature_invalid")fromverify_envelope) on uppercase hex or ASCII whitespace, consistent withSHA256_HEXin_link_is_well_formed.Duplicate JSON object keys, deep nesting, and uncanonicalizable JCS inputs (
tools/acta_receipt_verifier.py):_read_json(path)now passesobject_pairs_hook=_no_duplicate_keystojson.loadsto reject duplicate JSON object keys and catchesRecursionErroralongside(OSError, UnicodeDecodeError, ValueError), raisingUndecidable("malformed").signature_verifiesandchain_link_matchesnow catch(ValueError, RecursionError, UnicodeEncodeError)when canonicalizingpayloadorpredecessorviajcs, returningFalse(("invalid", "signature_invalid")or("invalid", "chain_link_mismatch")) instead of crashing with an unhandled exception.Independent cold-reader review verified that none of the new tests in
tests/test_acta_receipt_verifier.pyimport any private helpers added by this change (ED25519_SIG_HEX,B64URL,PRIVATE_JWK_PARAMS,_no_duplicate_keys), so all 36 added test cases run directly againstorigin/mainand fail on behaviour rather than on a missing name.Type of change
Spec section
None in
spec/trace-v0.2.md(informative section 3.3.3 tooling intools/acta_receipt_verifier.pyandtests/test_acta_receipt_verifier.py).Tests for a fix
Measured on commit
60bc7f2against merge base295c36d:python tools/review_bar.py --base origin/main --head HEAD: 36 fail on behaviour, 0 fail on a missing name or file, 44 pass or skip (out of 80 tests intests/test_acta_receipt_verifier.py):test_a_jwk_carrying_private_key_material_is_refused(8 cases:paramin["d", "p", "q", "dp", "dq", "qi", "oth", "k"]): passes onHEAD; when switched off onorigin/main, all 8 cases fail withFailed: DID NOT RAISE Undecidable.test_a_jwk_with_non_canonical_base64url_x_is_refused(10 cases:CANONICAL_X + "=",CANONICAL_X + "====","+" + CANONICAL_X[1:],CANONICAL_X.replace("_", "/")," " + CANONICAL_X,CANONICAL_X + " \n",CANONICAL_X + "....",CANONICAL_X[:-1] + "h",CANONICAL_X[:-1] + "i",CANONICAL_X[:-1] + "j"): passes onHEAD; when switched off onorigin/main, all 10 cases fail withFailed: DID NOT RAISE Undecidable.test_a_non_canonical_signature_hex_is_refused(6 cases:"uppercase","mixed_case","leading_space","trailing_newline","internal_space","uppercase_and_whitespace"): passes onHEAD; when switched off onorigin/main, all 6 cases fail withAssertionError: assert not True.test_duplicate_json_keys_are_undecidable(4 cases:"receipt_payload","receipt_envelope","jwks","context"): passes onHEAD; when switched off onorigin/main, all 4 cases fail withAssertionError: assert (0, {'code': None, 'verdict': 'valid'}) == (2, {'code': 'malformed', 'verdict': 'undecidable'}).test_deeply_nested_json_is_undecidable(3 cases:"receipt","jwks","context"over"[" * 200_000 + "]" * 200_000): passes onHEAD; when switched off onorigin/main, all 3 cases fail withRecursionError: maximum recursion depth exceeded while decoding a JSON array from a unicode string.test_an_uncanonicalizable_predecessor_or_deep_payload_does_not_crash(5 cases:"deep_payload","unsafe_int_predecessor","nan_predecessor","surrogate_predecessor","deep_predecessor"): passes onHEAD; when switched off onorigin/main, all 5 cases fail with unhandledRecursionError,IntegerDomainError,FloatDomainError, orCanonicalizationError.Validation commands run on
HEAD:pytest tests/test_acta_fixtures.py tests/test_acta_receipt_verifier.py-> passed (160 passed: 80 intests/test_acta_fixtures.pyand 80 intests/test_acta_receipt_verifier.py).ruff check src tests scripts tools/acta_receipt_verifier.py-> passed (All checks passed!).mypy --strict tools/acta_receipt_verifier.pyandmypy src/agentrust_trace-> passed (Success: no issues found).python tools/check_dashes.py-> passed (no dashes outside examples/, spec/trace-v0.1.md).Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text