Repository navigation
fix(intent_bridge,successor_observation): validate caller options and JWKs at entry and deep-copy observation (#617) - #618
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
…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>
|
From here on, pull requests do not edit |
dinakarjs
left a comment
There was a problem hiding this comment.
Reviewed exact head 25ddaf9. Caller configuration is validated before untrusted bridge input is consumed, signed timestamp values are retained, and successor predicates operate on detached observation state. Local focused intent-bridge/successor tests: 139 passed; Ruff and strict mypy (16 source files) passed. Hosted CI/conformance/review-bar checks are successful. No blocking finding.
|
Thanks @lywinged! Dropped the |
There was a problem hiding this comment.
The _pubkey_from_jwk move is good, but the trusted kid shape check is still not at entry.
In both verify_bridge and verify_successor_artifact, isinstance(trusted_kid, str) and bool(trusted_kid) still happens after the untrusted artifact has already been parsed. A cryptographically valid JWK with missing / empty / non-string kid can therefore surface artifact errors first.
#617 explicitly asks for the trusted JWK and its kid shape to be validated before any artifact inspection. The new tests cover unusable key material, but not this remaining ordering case, kindly hoist the non-empty-string check on trusted_*_jwk["kid"] right after _pubkey_from_jwk, and add a couple of regression tests that pair a valid public key (no/empty/bad kid) with a malformed artifact so the config error wins.
lywinged
left a comment
There was a problem hiding this comment.
Request changes. Reviewed at 25ddaf9 on c5d49a5.
What I checked rather than read:
- The three defects in #617 reproduce on
mainthrough the public functions and are gone here. A JWK carryingdunder the wrongkidraisestrusted_authorizer_jwk is unusable,now=-1with a signeddenyraises thenowerror, and a predicate writing to its argument leavesafteralone. - CI's commands, run on 3.12 and 3.11, pass, the full suite at 4817 passed and 48 skipped on both. The review bar finds all 31 test cases this adds failing on behaviour at the merge base.
- One at a time, I removed or moved back the 16 checks the diff touches, and the copy. Against the tests that reach the two modules, every mutant fails a test except the two that delete an
or not trusted_kidclause. - For the observations I tried, the predicate gets a separate copy of the observation, unchanged when the caller later edits
after.
Two things.
- rajnisht7's review of 25 October says the
kidcheck still runs after the artifact is parsed, so a valid key with a missing, empty or non-stringkidsurfaces artifact errors first. It does: a correct key with"kid": 7,authorization_id="auth-7"and an artifact signed forauth-othergivesAuthorizationMismatchfromverify_successor_artifact, which the description rules out for a malformed JWK. Two more facts bear on moving it. Where it stands,or not trusted_kidcannot change a result for anystr, becauseauthorizer_key_idpassed_nonempty_stringon line 389 andobserver_key_idon line 174; deleting both clauses leaves the whole suite green on 3.12. And moving the check changes behaviour that two existing tests pin:test_bridge_naming_another_authorizer_key_id_is_rejectedwantskey_to_jwk(key), which has nokid, to raisedoes not identify the trusted key, and theauthorizer_key_idcase oftest_runtime_verifier_enforces_schema_scalar_constraintspasses an emptykidand expects the artifact's error. I would move it, with a message that names the key:
trusted_kid = trusted_authorizer_jwk.get("kid")
if not isinstance(trusted_kid, str) or not trusted_kid:
raise IntentBridgeError("trusted_authorizer_jwk kid must be a non-empty string")right after _pubkey_from_jwk, the later comparison reduced to !=, the same for trusted_observer_jwk, and those two tests updated; the whole suite then passes on 3.12. It also wants the tests rajnisht7 asked for, one per function, in which a valid key with a missing, empty or non-string kid meets a malformed artifact and must raise the kid error: with only the two tests updated, moving the observer-side check back still passes every bridge test.
copy.deepcopygives up before the digest does. Withafter["observation"] = json.loads('{"k":' * 600 + '{}' + '}' * 600)and its own digest,evaluate_successor_observationwith a predicate reading one key returnsestablishedonmainand raisesRecursionErrorhere, on 3.12 and 3.11. The fuzz harness,.clusterfuzzlite/fuzz_intent_bridge.py, states the property this breaks:evaluate_successor_observationdocumentsSuccessorObservationError, and "any other exception is that evidence crashing it". A copy that can go as deep as the digest would keepmain's answer; at the least, theRecursionErrorwants to becomeSuccessorObservationError. The copy inplatform_measurement.pythat the description points to sits inside a handler that turns any exception intoappraiser_raised; this one has none, andtests/test_untrusted_input_stays_on_the_documented_error.pyhas no case for this function yet.
Things I would not hold the merge for:
transcriptis a parameterverify_bridge's entry block does not check:transcript="garbage"givesAuthorizationDeniedwith a signeddenyandAuthorizationMismatchwith a signedallow, as onmain. #617 does not name it, though the description's promise covers it.
Generated by Claude Code
|
Thank you @rajnisht7 and @lywinged! Addressed both points in
|
a12c467 to
1479860
Compare
rajnisht7
left a comment
There was a problem hiding this comment.
_clone_observation only walks dict and list. A tuple falls through to a plain assignment, so nested mutable objects stay shared
the project’s own JCS path accepts tuples as arrays (_refuse_whole_floats_out_of_range walks (list, tuple), and rfc8785.dumps treats them identically to lists). digest_jcs therefore succeeds on an observation that contains them, binding and evaluation succeed, and a predicate can still mutate the caller’s original:
obs = {"tuple_array": ({"secret": "original"},)}
after = {"observation": obs, "observer": "observer-1", "observed_at": 160}
# digest_jcs(after) succeeds; evaluate_successor_observation(...) returns established
# after["observation"]["tuple_array"][0]["secret"] is now "MUTATED"copy.deepcopy isolates this correctly but the iterative clone does not. existing tests only cover pure dict/list trees, so they stay green, kindly treat tuple the same as list in the clone (or otherwise detach it) and add a regression that exercises a tuple containing a mutable dict/list.
… JWKs at entry and deep-copy observation (agentrust-io#617) Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
1479860 to
76cacd8
Compare
|
@rajnisht7 Rebased onto |
dinakarjs
left a comment
There was a problem hiding this comment.
Re-reviewed current head 76cacd8. This supersedes the limited caller-configuration/isolation assessment in my older 25ddaf9 review: subsequent review correctly found kid-validation ordering, deep-copy depth and mutable objects inside tuples that it had not established. The current source now checks trusted kid shape at entry and iteratively detaches dict/list/tuple observations.
In this head's fresh hash-locked Python 3.12.14 environment, tests/test_intent_bridge.py plus tests/test_successor_observation.py pass 149 cases normally and under -O, including the deep and tuple controls. Changed-file Ruff and strict mypy for the two changed modules pass. No remaining blocker found in these selected paths. I did not rerun the full TRACE suite for this review; normal current-head CI, other reviewers' disposition and maintainer policy remain prerequisites. No withdrawal/status-service or production acceptance claim. Prepared with OpenAI Codex.
What this changes
Closes #617.
In
src/agentrust_trace/intent_bridge.pyandsrc/agentrust_trace/successor_observation.py:verify_bridgeandverify_successor_artifactvalidatetrusted_authorizer_jwkandtrusted_observer_jwkwith_pubkey_from_jwkat entry before comparingkidor inspecting the untrusted artifact:verify_bridgechecksisinstance(trusted_authorizer_jwk, dict)and calls_pubkey_from_jwk(trusted_authorizer_jwk)at entry, and then checksisinstance(trusted_kid, str) and bool(trusted_kid) and trusted_kid == authorization["authorizer_key_id"]. An unusabletrusted_authorizer_jwk(carrying private key material"d", wrong"kty", wrong"crv", missing"x", or non-canonical base64url"x") whose"kid"also differs fromauthorization["authorizer_key_id"]now raisesIntentBridgeError("trusted_authorizer_jwk is unusable: ...")instead of blamingauthorizer_key_id.verify_successor_artifactchecksisinstance(trusted_observer_jwk, dict)and calls_pubkey_from_jwk(trusted_observer_jwk)at entry before comparingtrusted_kidwithobserver_key_idor comparingauthorization_id.verify_bridge,verify_successor_artifact, and_bind_successor_observationvalidate caller-supplied options at entry:verify_bridgevalidatesnow(int | Nonewithin0..JCS_SAFE_INTEGER, excludingbool),declaration(dictwith an RFC 8785 canonical form viadigest_jcs),tool_call(dictwith an RFC 8785 canonical form viadigest_jcs),pic_intent_digest(sha256:<64 lowercase hex>), andpic_args_digest(sha256:<64 lowercase hex>) at entry before signature verification and beforeif authorization["decision"] != "allow": raise AuthorizationDenied(...), so a caller option error is never masked byAuthorizationDeniedor"authorization signature is invalid".verify_successor_artifactvalidatestrusted_observerandauthorization_idas non-empty strings at entry before inspectingartifactor raisingAuthorizationMismatch._bind_successor_observationvalidatesexpected_successor_digestwith_digestbefore inspectingafter.evaluate_successor_observationdeep-copiesobservationbefore callingpredicate:evaluate_successor_observationpassescopy.deepcopy(observation)topredicateso a caller-suppliedpredicatecannot mutateafter["observation"](or nested dicts/lists within it) in place, matchingplatform_measurement.py:139.What has no answer
schema/pic-trace-bridge-v1.json,schema/pic-trace-successor-v1.json), published vectors underexamples/pic-successor-boundaries/, anddocs/integration/pic-trace-bridge-v1.mdare left unchanged because the wire format and documented error messages (authorization.pic.intent_digest must be a sha256 digestandintent_digest must be a sha256 digest) are unchanged.verify_bridge,verify_successor_artifact, or_bind_successor_observationcannot decide because a caller option or JWK is malformed, each fails closed by raisingIntentBridgeError(and neverAuthorizationDeniedorAuthorizationMismatch), pinned bytest_verify_bridge_validates_caller_options_before_signature_and_denial,test_verify_successor_artifact_validates_caller_options_at_entry, andtest_bind_successor_observation_validates_expected_digest_before_envelope.afterduringdigest_jcs(after)beforeevaluate_successor_observationreachespredicate.Measurements on commit
d6fd914(Python 3.11.9)pytest tests/test_intent_bridge.py tests/test_successor_observation.py tests/test_pic_successor_boundaries.py tests/test_public_functions_raise_what_they_document.py -q:379 passed(114intests/test_intent_bridge.py,25intests/test_successor_observation.py,46intests/test_pic_successor_boundaries.py,194intests/test_public_functions_raise_what_they_document.py;0 skipped,0 xfailed,0 errors).python tools/review_bar.py --base origin/main --head HEAD(a8e3559..d6fd914):31 fail on behaviour, 0 fail on a missing name or file, 108 pass or skipacross the139test cases intests/test_intent_bridge.py(114) andtests/test_successor_observation.py(25). All31new test cases fail onorigin/main(a8e3559) withAssertionError.python tools/sweep_public_surface.py --strict:89 parameters swept, 140 accepted junk values, 0 unfiled leak(s).python tools/check_dashes.py:no dashes outside examples/, spec/trace-v0.1.md.ruff check src/agentrust_trace/intent_bridge.py src/agentrust_trace/successor_observation.py tests/test_intent_bridge.py tests/test_successor_observation.py:All checks passed!.mypy --strict src/agentrust_trace/intent_bridge.py src/agentrust_trace/successor_observation.py:Success: no issues found in 2 source files.Independent review of the diff and description checked whether validating
pic_intent_digestat entry preserved the exact field name (intent_digestrather thanpic_intent_digest) quoted indocs/integration/pic-trace-bridge-v1.mdandtests/test_pic_successor_boundaries.py, and whethertest_verify_successor_artifact_validates_caller_options_at_entryfailed onorigin/mainforauthorization_idas well astrusted_observer. Both were verified:_digest(pic_intent_digest, "intent_digest")and_digest(pic_args_digest, "args_digest")preserve the documented message, andtest_verify_successor_artifact_validates_caller_options_at_entrytests against both a mismatchedauthorization_idand an unknownprofileso all 4 cases fail onorigin/main.Type of change
Spec section
None (
src/agentrust_trace/intent_bridge.py,src/agentrust_trace/successor_observation.py,tests/test_intent_bridge.py,tests/test_successor_observation.py, andCHANGELOG.mdonly).Tests for a fix
tests/test_intent_bridge.py::test_verify_bridge_rejects_unusable_trusted_authorizer_jwk_before_kid_mismatch(6 cases:{"kty": "RSA", "kid": "wrong-kid"},{"kty": "OKP", "crv": "X25519", "kid": "wrong-kid"},{"kty": "OKP", "crv": "Ed25519", "kid": "wrong-kid"},{"kty": "OKP", "crv": "Ed25519", "x": "!!!", "kid": "wrong-kid"},{"kty": "OKP", "crv": "Ed25519", "x": "AA", "kid": "wrong-kid"}, and a valid Ed25519 JWK with"d": "secret-private-key"and"kid": "wrong-kid"): fails onorigin/main(a8e3559) becauseverify_bridgeraisedIntentBridgeError("authorizer_key_id does not identify the trusted key")before calling_pubkey_from_jwk.tests/test_intent_bridge.py::test_verify_successor_artifact_rejects_non_object_trusted_observer_jwk(4 cases:None,"not-a-jwk",123,["kid"]) andtests/test_intent_bridge.py::test_verify_successor_artifact_rejects_unusable_jwk_before_kid_or_link_mismatch(6 cases): fail onorigin/mainbecauseverify_successor_artifactcheckedauthorization_idandkidbefore validatingtrusted_observer_jwk.tests/test_intent_bridge.py::test_verify_successor_artifact_validates_caller_options_at_entry(4 cases:trusted_observerandauthorization_idset to""andNone): fails onorigin/mainbecauseverify_successor_artifactcheckedartifact["profile"]andauthorization_idmismatch before validating caller options.tests/test_intent_bridge.py::test_verify_bridge_validates_caller_options_before_signature_and_denial(9 cases acrossnow,declaration,tool_call,pic_intent_digest, andpic_args_digest): fails onorigin/mainbecauseverify_bridgeraisedAuthorizationDeniedorIntentBridgeError("authorization signature is invalid")before checking caller options.tests/test_intent_bridge.py::test_bind_successor_observation_validates_expected_digest_before_envelope(1 case): fails onorigin/mainbecause_bind_successor_observationraisedAuthorizationMismatchonafterbefore validatingexpected_successor_digest.tests/test_successor_observation.py::test_predicate_receives_deep_copy_and_cannot_mutate_after_observation(1 case): fails onorigin/mainbecauseevaluate_successor_observationpassedbound["observation"]directly topredicate, mutatingafter["observation"]in place.Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text