Repository navigation
sign: validate caller options at verify_record entry (#556) - #562
imran-siddique merged 2 commits into
Conversation
Validate caller configuration options at verify_record entry before inspecting the untrusted record: - Reject negative now (< 0) at entry alongside non-integer and bool values even when revocation_bundle is None. - Validate trusted_bundle_keys via _sequence_of(..., dict) whenever trusted_bundle_keys is not None, even when revocation_bundle is None. - Validate max_age_seconds, expected_nonce, revocation, and trusted_bundle_keys at entry before checking record structure. Closes agentrust-io#556 Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
the implementation looks correct, options are validated at entry, negative now is rejected, and trusted_bundle_keys is checked even when there’s no bundle
kindly add regressions that lock the ordering/materialization contract:
- One-shot trusted_bundle_keys generator + real successful bundle verification
- Callback spies - invalid options -> revocation (and ideally citation/platform) call_count == 0
- Valid signed record + invalid max_age_seconds / expected_nonce / malformed revocation
Optional but useful
- Valid trusted_bundle_keys + revocation_bundle=None -> still no_check_performed
- now=0 accepted (boundary)
…t-io#562 Signed-off-by: Srinivasa Consultants <dinakarjs@gmail.com>
dinakarjs
left a comment
There was a problem hiding this comment.
Independent review of 237276f: the entry validation and retained materialization appear correct. CI is green. I prepared the supplementary controls requested by @rajnisht7 as a tests-only, DCO-signed commit for the author to cherry-pick: dinakarjs@adaadf6
Evidence: 506 tests passed across the existing option-ordering, sign, revocation bundle/policy-type and documented-public-error suites plus 14 new cases. Ruff and strict mypy on the new file pass. Import provenance was the exact export src, with 0.11.0 metadata built from that export.
The new controls exercise a real signed bundle with list versus one-shot generator keys and compare the complete result; invalid options on valid signed records with callback spies; malformed revocation values; no-bundle no_check_performed; and now=0 with adjacent explicit skew boundaries. A positive control shows revocation and platform callbacks are reachable under valid configuration. Citation spies assert absence of side effects, not a demonstrated reachable positive citation path on this fixture.
Mutation check: keeping _sequence_of validation but discarding its materialized result makes the generator test fail on actual revocation semantics (bundle_key_untrusted instead of verified), while the list control succeeds. Restoring the implementation passes.
Fresh-reader feedback to add UTF-8 fixture loading and a positive callback control has been addressed. I could not open the intended companion PR against the author's fork because the integration returned HTTP 403; the commit is available above without duplicating the implementation PR. No merge performed.
…t-io#562 Signed-off-by: Srinivasa Consultants <dinakarjs@gmail.com> Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks @rajnisht7 for the thorough review and @dinakarjs for preparing the supplementary regression suite! Added ests/test_verify_record_option_controls.py (9bf3945) covering all requested ordering and materialization controls:
ow=0 boundary acceptance and future-skew rejection ( est_zero_timestamp_is_accepted_with_explicit_future_skew_budget) |
dinakarjs
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 9bf3945. The requested supplementary controls are incorporated, and the author's entry-validation/materialization implementation retains the intended behavior. The selected option/sign/revocation/public-error suites pass 506 tests using this head's hash-locked dependencies and checkout imports.
The one-shot generator test compares real successful signed-bundle verification with list keys. Discarding only the validated materialized result in memory makes that control fail on revocation semantics while the other 13 controls pass. Invalid-option tests suppress callbacks, valid configuration reaches revocation/platform callbacks, no bundle retains no_check_performed, and the zero-time/future-skew boundary is explicit. Citation spies establish absence of a call on invalid options, not a demonstrated positive citation path in that fixture.
Ruff, strict typing of the added control file and dash/whitespace checks pass. Current-head CI and Conformance CI succeed; ClusterFuzzLite was still running when checked. No remaining blocking finding in this bounded review. I contributed the supplementary tests; this technical approval does not replace another maintainer's assessment of that contribution or authorize merge.
rajnisht7
left a comment
There was a problem hiding this comment.
The changes looks good, cleanly closes #556. Caller options (negative now, trusted_bundle_keys even without a bundle, etc.) are now validated at entry before any record inspection or callbacks. The new tests cover the gaps well, including one-shot keys and callback spies.
small nits (non-blocking):
- The non-callable
revocationshape check is now duplicated between entry and_check_not_revoked. Fine for this PR. - drop the
(#556)/(#562)from the top of both new test module docstrings existing tests in the suite keep the module docstring focused on what is being tested rather than the issue/PR number.
What this changes
Contributor: Raghuvar Durga (@drk-ram)
Co-Contributor: Srinivasa Consultants (@dinakarjs)
Closes #556
Validates caller configuration options at
verify_recordentry (src/agentrust_trace/sign.py) before inspecting the untrustedrecord:nowat entry:verify_recordpreviously checkedisinstance(now, bool) or not isinstance(now, int)without rejectingnow < 0, allowing negative timestamps whenrevocation_bundle=Nonewhile failing mid-pipeline insiderevocation.check_bundleonly when a bundle was supplied.now < 0is now rejected at entry withValueError("now must be an integer Unix timestamp in seconds (must be non-negative), or None").trusted_bundle_keyswhenrevocation_bundle is None:trusted_bundle_keysis now validated via_sequence_of(trusted_bundle_keys, "trusted_bundle_keys", dict)whenevertrusted_bundle_keys is not None, rather than silently ignoring non-iterable or non-dict values (such as a bare JWKdict,"str",False,0, or[123]) whenrevocation_bundle=None.record:max_age_seconds,expected_nonce,revocation, andtrusted_bundle_keysare validated at entry alongsidenow,allow_embedded_key,max_bundle_age_seconds,max_future_skew_seconds,citation_resolver,platform_appraiser, andaccepted_profilesbefore inspectingrecord.Type of change
Spec section
src/agentrust_trace/sign.py,verify_recordimplementingspec/trace-v0.2.md§3.2.2 and §3.2.3)Tests for a fix
Added
tests/test_verify_record_option_ordering.pyandtests/test_verify_record_option_controls.py:test_verify_record_rejects_negative_now: fails onmain(DID NOT RAISE ValueErrorwhennow=-1ornow=-100withrevocation_bundle=None) and passes on this branch.test_verify_record_rejects_invalid_trusted_bundle_keys_when_bundle_is_none: fails onmain(DID NOT RAISE ValueErrorfor bare JWKdict,"str",b"bytes",False,0,123,[123],[None]) and passes on this branch.test_verify_record_validates_caller_options_at_entry_before_record: fails onmain(raises record errors instead of naming the invalid caller optionmax_age_seconds,expected_nonce,revocation,trusted_bundle_keys, ornow) and passes on this branch.test_one_shot_bundle_keys_preserve_successful_signed_bundle_verification: verifies that materializing a one-shot generator fortrusted_bundle_keysat entry preserves real signed revocation-bundle verification without exhausting the iterator beforecheck_bundle.test_invalid_options_on_valid_record_never_invoke_callbacks&test_malformed_revocation_on_valid_record_suppresses_resolvers: verifies on a valid signed record that invalid caller options fail at entry beforerevocation,citation_resolver, orplatform_appraisercallbacks are invoked (call_count == 0).test_valid_bundle_keys_without_bundle_do_not_claim_revocation_check: verifies that validtrusted_bundle_keyswithrevocation_bundle=Nonestill reportsoutcome="no_check_performed".test_zero_timestamp_is_accepted_with_explicit_future_skew_budget: verifies thenow=0boundary.Validation commands and results:
pytest tests/test_verify_record_option_ordering.py tests/test_verify_record_option_controls.py tests/test_sign.py tests/test_revocation_bundle.py tests/test_revocation_policy_types.py tests/test_public_functions_raise_what_they_document.py-> passed (506 passed)python tools/sweep_public_surface.py --strict-> passed (exit code 0)python tools/review_bar.py --base origin/main --head HEAD-> passed (36 fail on behaviour, 0 fail on a missing name or file, 9 pass or skip)python tools/check_dashes.py-> passed (exit code 0)ruff check src/agentrust_trace/sign.py tests/test_verify_record_option_ordering.py tests/test_verify_record_option_controls.py-> passed (All checks passed!)mypy --strict src/agentrust_trace/sign.py tests/test_verify_record_option_ordering.py tests/test_verify_record_option_controls.py-> passed (Success: no issues found in 3 source files)Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text