Repository navigation
revocation,trust-record-verify: reject private EC JWK members, enforce ECMA-262 regex on bundle schemas, and normalize integer-by-value bundle timestamps - #607
Conversation
…e ECMA-262 regex on bundle schemas, and normalize integer-by-value bundle timestamps 1. Reject RFC 7517 / RFC 7518 private and symmetric JWK parameters (d, p, q, dp, dq, qi, oth, k) and non-dict JWKs in revocation._bundle_pubkey_from_jwk for ES256 and ES384, and check JWK_PRIVATE_MEMBERS in trust-record-verify's importEcdsa, matching ed25519 key import under GHSA-vc4p-h84j-7qxj. 2. Extend Draft202012Validator in revocation._bundle_validator() with ECMA-262 pattern semantics ($ -> \Z) across both trace-revocation-bundle.json and the referenced trace-revocation.json statement schema so trailing newlines in sig.value fail as bundle_malformed in Python just as in trust-record-verify. 3. Normalize issued_at and valid_until via _integer_value in revocation.check_bundle so whole-valued floats stay int in RevocationCheck.evidence. Closes agentrust-io#604 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>
dinakarjs
left a comment
There was a problem hiding this comment.
Reviewed 6eb77b4. Independent Linux/Python 3.12.14 run using the repository's hash-locked dev dependencies: tests/test_revocation*.py gives 90 passed. npm ci --ignore-scripts followed by npm test gives 161 passed; targeted Ruff and source mypy pass. My first pytest command named a nonexistent test_revocation.py; it collected nothing, and the corrected glob is the passing run.
The public-key rejection is applied to both EC curves without printing supplied private values. The extended pattern validator is retained across the locally registered statement reference, and both top-level and nested newline cases are exercised. Whole-valued timestamp normalization preserves canonical numeric meaning and the explicit unchecked/malformed revocation results. No additional blocker found in this bounded diff. Nonblocking cleanup: git diff --check reports a new blank line at EOF in tests/test_revocation_bundle.py:559. I did not rerun the author's mutation trials or independently execute every cross-language differential input. Required maintainer approval remains separate. Reviewed with OpenAI Codex.
lywinged
left a comment
There was a problem hiding this comment.
REVIEW on #607, head 6eb77b4: Approve
Measured on 6eb77b4 with the hash-pinned requirements, on 3.11 and on 3.12: ruff, mypy and the dash check (tools/check_dashes.py) clean; 4685 passed and 48 skipped here; the experiment (experiments/verifier-context-replay) 116 passed and 4 skipped; the portability run 4684 passed and 49 skipped. npm test 161 pass. The differential (differential/compare.py --expect-external 12) 1646 cases, 1645 identical verdicts, 1 known divergence, 0 unexpected. The sweep's line is the merge base's line, unchanged. Against the merge base 295c36d the review bar sorts the five new tests as failing on behaviour and 56 as pass or skip, which is what the description reports. The four files the description names collect and pass 494 here, 0 skipped; it says 487 passed, and I cannot see from here where the seven went. The change closes the three gaps #604 lists, and the CHANGELOG entry under Unreleased says what the code does, with the one framing point in item 1.
-
What the private-member check reaches. On the head and on the merge base alike,
check_bundlegiven a trusted bundle key that carriesdorkraises before_bundle_pubkey_from_jwkruns:_trusted_bundle_keylooks the key up through_key_identifiers, whosejwk_thumbprintrefuses a JWK carrying a private member and names the member. Measured on vectors 29 and 30, withdand withk, on both, same exception and same message.trust-record-verifydoes the same one call earlier, intrustedBundleKey, which rethrows the thumbprint'sjwk_invalidasinvalid_argumentwith "a trusted bundle key is not a usable JWK" and the member named, on the head build and on a build with theimportEcdsacheck removed. So the public path failed closed before this change, in both languages, and what the change closes is the two helpers, which the verifiers reach only through that lookup. The tests are consistent with that: thecheck_bundlecalls intest_bundle_pubkey_from_jwk_rejects_private_ec_membersand thecheckRevocationBundlecalls in the TypeScript test give the same refusal with the new checks removed, exercised as direct calls, since each test stops at its direct-call assertion first, which is what the review bar's "DID NOT RAISE" names and what removing the TypeScript check reproduces, one test, atunits.test.mjs:350, "Missing expected rejection". One sentence in the description and the CHANGELOG saying the lookup already refused such a key and this closes the helpers would keep a reader from taking the advisory parity in its first heading as a verifier that accepted such keys. -
_BUNDLE_PATTERNSis a second closed table, and the first one has a guard this one lacks._patterns.pysays a new schema pattern needs explicit review, andtest_every_shipped_pattern_has_an_explicit_adapterholds the trust-record schema's set of patterns equal to the table's, so a schema edit fails a test rather than a verifier. The bundle table has no such test. Apatternadded to either packaged bundle schema would make_ecma_bundle_patternraiseKeyErrorwhen validation reaches a string at that path, out of the iteration ofiter_errors, whichcheck_bundledoes not catch: measured on an in-memory schema,list(_BundleValidator({"type": "string", "pattern": "^a$"}).iter_errors("ab"))raisesKeyError('^a$'), and a pattern on a member every bundle carries,log_idsay, would raise on every bundle. Today each packaged bundle schema carries exactly one pattern, the one in the table, so nothing reaches it. Please add the same assertion over the two bundle schemas. -
Confirmed. The
$schemapop is load-bearing: with it removed,test_bundle_validator_rejects_trailing_newline_in_statement_sig_valuefails and the bundle-level test passes, the description's cold-review finding reproduced. The stock validator with the pop kept fails both newline tests. A check ondalone, the narrower guard a reviewer would have written, failstest_bundle_pubkey_from_jwk_rejects_private_ec_members, which loops over all eight members, so the loop is what holds the width. Reverting the_integer_valuenormalization failstest_check_bundle_normalizes_whole_float_timestamps_in_evidence. Each run on the whole suite on a copy, one mutation at a time, 4685 passing before any. The normalization changes the evidence types and nothing else: on vector 01,issued_atas1e21,2**53+2,9007199254740993,-1,1.5,Trueand the whole float, andvalid_untilas1e21, give the same outcome on the head and on the merge base, because the schema'stype,minimumandmaximumrefuse the non-integer and out-of-range ones before_integer_valuesees them, so no newValueErrorpath opens.git diff --checkreports the blank line at the end oftests/test_revocation_bundle.pythat dinakarjs noted.
Approve. Neither item holds the approval: the description edit needs no commit, and the CHANGELOG sentence and the guard can come in one push or as a follow-up.
What this changes
Closes #604.
Closes three verification and cross-language parity gaps across
src/agentrust_trace/revocation.pyandtrust-record-verify:Reject private and symmetric JWK members in EC bundle-signing keys across Python and TypeScript (GHSA-vc4p-h84j-7qxj parity):
revocation._bundle_pubkey_from_jwknow checksisinstance(jwk, dict)and rejects any JWK carrying_JWK_PRIVATE_PARAMS(d,p,q,dp,dq,qi,oth,k) forES256(P-256) andES384(P-384), matchingsign._pubkey_from_jwkfored25519.trust-record-verify'simportEcdsa(trust-record-verify/src/crypto.ts) now checksJWK_PRIVATE_MEMBERSright afterktyandcrv, matchingimportEd25519without echoing secret values.Enforce ECMA-262 regex semantics (
$->\Z) inrevocation._bundle_validator():schema/trace-revocation-bundle.jsonandschema/trace-revocation.jsonconstrainsig.valuewith"pattern": "^[A-Za-z0-9_-]+$". Plainjsonschema.Draft202012Validatoruses Pythonre.search, where$matches before a trailing\n, so a bundle or nested statement whosesig.valueended with"\n"passed schema validation in Python whiletrust-record-verifyrejected it asbundle_malformed.revocation._bundle_validator()now usesjsonschema.validators.extend(jsonschema.Draft202012Validator, {"pattern": _ecma_bundle_pattern})and pops"$schema"from the in-memorystatement_schemabefore registering it withreferencing.jsonschema.DRAFT202012soValidator.evolvepreserves the extendedpatternvalidator across the$refboundary.Normalize whole-valued float bundle timestamps in
revocation.check_bundle:revocation.check_bundlenow normalizesissued_at = _integer_value(bundle["issued_at"], "issued_at")andvalid_until = _integer_value(bundle["valid_until"], "valid_until")so JSON Schema-valid whole floats (1785000000.0, perspec/trace-v0.2.mdsection 3.2.2) are stored asintinRevocationCheck.evidence["issued_at"]andevidence["valid_until"].Independent cold review of the diff caught two subtleties that were fixed before opening the PR:
jsonschemaresolvesstatements.items.$reftotrace-revocation.json,Validator.evolve(schema=resolved.contents)inspectsresolved.contents.get("$schema")viavalidator_for()and resets an extendedDraft202012Validatorback to stockDraft202012Validator, leavingstatements[].sig.valuewith Pythonre.searchsemantics. Stripping"$schema"from the in-memorystatement_schemapassed toreferencing.Resource.from_contents(..., default_specification=DRAFT202012)keeps_BundleValidatoractive inside referenced statements, verified bytest_bundle_validator_rejects_trailing_newline_in_statement_sig_value.tests/test_revocation_bundle.pyensurestools/review_bar.py --base origin/main --head HEADobserves all five behavioural failures independently rather than stopping at the first assertion in a combined test.Type of change
Spec section
spec/trace-v0.2.mdsections 3.2.2 and 3.2.3 (reference verifier andtrust-record-verifyimplementation parity; no normative spec text changes).Tests for a fix
Output of
python tools/review_bar.py --base origin/main --head HEAD(295c36d..6eb77b4):tests.test_revocation_bundle::test_bundle_pubkey_from_jwk_rejects_non_dict_jwk: fails onorigin/mainwithAttributeError: 'NoneType' object has no attribute 'get'.tests.test_revocation_bundle::test_bundle_pubkey_from_jwk_rejects_private_ec_members: fails onorigin/mainwithFailed: DID NOT RAISE ValueError.tests.test_revocation_bundle::test_bundle_validator_rejects_trailing_newline_in_bundle_sig_value: fails onorigin/mainwithassert False(_bundle_validator()acceptssig.valueending with"\n").tests.test_revocation_bundle::test_bundle_validator_rejects_trailing_newline_in_statement_sig_value: fails onorigin/mainwithassert False(and if"$schema"is not popped fromstatement_schema, fails onstatements/0/sig/value).tests.test_revocation_bundle::test_check_bundle_normalizes_whole_float_timestamps_in_evidence: fails onorigin/mainwithAssertionError: assert <class 'float'> is int.importEcdsa and checkRevocationBundle refuse EC JWKs carrying private key materialintrust-record-verify/tests/units.test.mjsfails when theJWK_PRIVATE_MEMBERScheck inimportEcdsais switched off.Validation commands and measured results:
pytest tests/test_revocation_bundle.py tests/test_schema_regex_semantics.py tests/test_public_functions_raise_what_they_document.py tests/test_integer_by_value.py: passed (487 passed).npm --prefix trust-record-verify test: passed (161 pass, 0 fail).python trust-record-verify/differential/compare.py --expect-external 12(aftergenerate_cases.py,oracle.py, andnpm --prefix trust-record-verify run differential): passed (1646 cases, 1645 identical verdicts, 1 known divergence, 0 unexpected).python tools/sweep_public_surface.py --strict: passed (89 parameters swept, 140 accepted junk values, 0 unfiled leak(s)).python tools/review_bar.py --base origin/main --head HEAD: passed (5 fail on behaviour, 0 fail on a missing name or file, 56 pass or skip).python tools/check_dashes.py: passed (no dashes outside examples/, spec/trace-v0.1.md).ruff check src/agentrust_trace/revocation.py tests/test_revocation_bundle.py: passed (All checks passed!).mypy --strict src/agentrust_trace/revocation.py: passed (Success: no issues found in 1 source file).Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text