Skip to content

fix(intent_bridge,successor_observation): validate caller options and JWKs at entry and deep-copy observation (#617) - #618

Open
drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-intent-bridge-successor-617
Open

drk-ram wants to merge 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-intent-bridge-successor-617

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

What this changes

Closes #617.

In src/agentrust_trace/intent_bridge.py and src/agentrust_trace/successor_observation.py:

  1. verify_bridge and verify_successor_artifact validate trusted_authorizer_jwk and trusted_observer_jwk with _pubkey_from_jwk at entry before comparing kid or inspecting the untrusted artifact:

    • verify_bridge checks isinstance(trusted_authorizer_jwk, dict) and calls _pubkey_from_jwk(trusted_authorizer_jwk) at entry, and then checks isinstance(trusted_kid, str) and bool(trusted_kid) and trusted_kid == authorization["authorizer_key_id"]. An unusable trusted_authorizer_jwk (carrying private key material "d", wrong "kty", wrong "crv", missing "x", or non-canonical base64url "x") whose "kid" also differs from authorization["authorizer_key_id"] now raises IntentBridgeError("trusted_authorizer_jwk is unusable: ...") instead of blaming authorizer_key_id.
    • verify_successor_artifact checks isinstance(trusted_observer_jwk, dict) and calls _pubkey_from_jwk(trusted_observer_jwk) at entry before comparing trusted_kid with observer_key_id or comparing authorization_id.
  2. verify_bridge, verify_successor_artifact, and _bind_successor_observation validate caller-supplied options at entry:

    • verify_bridge validates now (int | None within 0..JCS_SAFE_INTEGER, excluding bool), declaration (dict with an RFC 8785 canonical form via digest_jcs), tool_call (dict with an RFC 8785 canonical form via digest_jcs), pic_intent_digest (sha256:<64 lowercase hex>), and pic_args_digest (sha256:<64 lowercase hex>) at entry before signature verification and before if authorization["decision"] != "allow": raise AuthorizationDenied(...), so a caller option error is never masked by AuthorizationDenied or "authorization signature is invalid".
    • verify_successor_artifact validates trusted_observer and authorization_id as non-empty strings at entry before inspecting artifact or raising AuthorizationMismatch.
    • _bind_successor_observation validates expected_successor_digest with _digest before inspecting after.
  3. evaluate_successor_observation deep-copies observation before calling predicate:

    • evaluate_successor_observation passes copy.deepcopy(observation) to predicate so a caller-supplied predicate cannot mutate after["observation"] (or nested dicts/lists within it) in place, matching platform_measurement.py:139.

What has no answer

  • Wire schemas (schema/pic-trace-bridge-v1.json, schema/pic-trace-successor-v1.json), published vectors under examples/pic-successor-boundaries/, and docs/integration/pic-trace-bridge-v1.md are left unchanged because the wire format and documented error messages (authorization.pic.intent_digest must be a sha256 digest and intent_digest must be a sha256 digest) are unchanged.
  • When verify_bridge, verify_successor_artifact, or _bind_successor_observation cannot decide because a caller option or JWK is malformed, each fails closed by raising IntentBridgeError (and never AuthorizationDenied or AuthorizationMismatch), pinned by test_verify_bridge_validates_caller_options_before_signature_and_denial, test_verify_successor_artifact_validates_caller_options_at_entry, and test_bind_successor_observation_validates_expected_digest_before_envelope.
  • Class of input not fed by the new tests: concurrent thread mutation of after during digest_jcs(after) before evaluate_successor_observation reaches predicate.

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 (114 in tests/test_intent_bridge.py, 25 in tests/test_successor_observation.py, 46 in tests/test_pic_successor_boundaries.py, 194 in tests/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 skip across the 139 test cases in tests/test_intent_bridge.py (114) and tests/test_successor_observation.py (25). All 31 new test cases fail on origin/main (a8e3559) with AssertionError.
  • 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_digest at entry preserved the exact field name (intent_digest rather than pic_intent_digest) quoted in docs/integration/pic-trace-bridge-v1.md and tests/test_pic_successor_boundaries.py, and whether test_verify_successor_artifact_validates_caller_options_at_entry failed on origin/main for authorization_id as well as trusted_observer. Both were verified: _digest(pic_intent_digest, "intent_digest") and _digest(pic_args_digest, "args_digest") preserve the documented message, and test_verify_successor_artifact_validates_caller_options_at_entry tests against both a mismatched authorization_id and an unknown profile so all 4 cases fail on origin/main.

Type of change

  • Editorial (typo, link fix, clarification: no normative effect)
  • Non-breaking spec change (new optional field, new platform profile, informative addition)
  • Breaking spec change (requires 14-day comment period and Project Lead sign-off)
  • Schema change
  • Example addition

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, and CHANGELOG.md only).

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 on origin/main (a8e3559) because verify_bridge raised IntentBridgeError("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"]) and tests/test_intent_bridge.py::test_verify_successor_artifact_rejects_unusable_jwk_before_kid_or_link_mismatch (6 cases): fail on origin/main because verify_successor_artifact checked authorization_id and kid before validating trusted_observer_jwk.
  • tests/test_intent_bridge.py::test_verify_successor_artifact_validates_caller_options_at_entry (4 cases: trusted_observer and authorization_id set to "" and None): fails on origin/main because verify_successor_artifact checked artifact["profile"] and authorization_id mismatch before validating caller options.
  • tests/test_intent_bridge.py::test_verify_bridge_validates_caller_options_before_signature_and_denial (9 cases across now, declaration, tool_call, pic_intent_digest, and pic_args_digest): fails on origin/main because verify_bridge raised AuthorizationDenied or IntentBridgeError("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 on origin/main because _bind_successor_observation raised AuthorizationMismatch on after before validating expected_successor_digest.
  • tests/test_successor_observation.py::test_predicate_receives_deep_copy_and_cannot_mutate_after_observation (1 case): fails on origin/main because evaluate_successor_observation passed bound["observation"] directly to predicate, mutating after["observation"] in place.

Checklist

  • DCO sign-off on all commits (git commit -s)
  • CHANGELOG.md updated (for any normative change)
  • Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text
  • Backward compatibility statement included (for breaking changes)

@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners October 23, 2026 03:47
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Oct 23, 2026
lywinged added a commit to drk-ram/trace-spec that referenced this pull request Oct 23, 2026
…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>
@lywinged

Copy link
Copy Markdown
Collaborator

From here on, pull requests do not edit CHANGELOG.md: put the entry you propose in the description under ## Changelog entry, and I write it under Unreleased on main at merge, crediting you. That is what #486 was asked on 12 October. The template and the 19 October note on #537 said otherwise, and five entries added at the same place in one file is why #537, #608 and #610 now conflict with main and with each other. For this one and #619, please drop the CHANGELOG.md hunk and move the text into the description; the conflicts on #537, #608 and #610 I take at merge. CONTRIBUTING and the template change to say so in a separate pull request, with a check that fails a pull request that touches the file.

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@drk-ram

drk-ram commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @lywinged! Dropped the CHANGELOG.md hunk from #618, #619, #620, #621, #622, and #588 and moved each entry under ## Changelog entry in the PR description.

@rajnisht7 rajnisht7 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes. Reviewed at 25ddaf9 on c5d49a5.

What I checked rather than read:

  • The three defects in #617 reproduce on main through the public functions and are gone here. A JWK carrying d under the wrong kid raises trusted_authorizer_jwk is unusable, now=-1 with a signed deny raises the now error, and a predicate writing to its argument leaves after alone.
  • 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_kid clause.
  • For the observations I tried, the predicate gets a separate copy of the observation, unchanged when the caller later edits after.

Two things.

  1. rajnisht7's review of 25 October says the kid check still runs after the artifact is parsed, so a valid key with a missing, empty or non-string kid surfaces artifact errors first. It does: a correct key with "kid": 7, authorization_id="auth-7" and an artifact signed for auth-other gives AuthorizationMismatch from verify_successor_artifact, which the description rules out for a malformed JWK. Two more facts bear on moving it. Where it stands, or not trusted_kid cannot change a result for any str, because authorizer_key_id passed _nonempty_string on line 389 and observer_key_id on 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_rejected wants key_to_jwk(key), which has no kid, to raise does not identify the trusted key, and the authorizer_key_id case of test_runtime_verifier_enforces_schema_scalar_constraints passes an empty kid and 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.

  1. copy.deepcopy gives up before the digest does. With after["observation"] = json.loads('{"k":' * 600 + '{}' + '}' * 600) and its own digest, evaluate_successor_observation with a predicate reading one key returns established on main and raises RecursionError here, on 3.12 and 3.11. The fuzz harness, .clusterfuzzlite/fuzz_intent_bridge.py, states the property this breaks: evaluate_successor_observation documents SuccessorObservationError, and "any other exception is that evidence crashing it". A copy that can go as deep as the digest would keep main's answer; at the least, the RecursionError wants to become SuccessorObservationError. The copy in platform_measurement.py that the description points to sits inside a handler that turns any exception into appraiser_raised; this one has none, and tests/test_untrusted_input_stays_on_the_documented_error.py has no case for this function yet.

Things I would not hold the merge for:

  • transcript is a parameter verify_bridge's entry block does not check: transcript="garbage" gives AuthorizationDenied with a signed deny and AuthorizationMismatch with a signed allow, as on main. #617 does not name it, though the description's promise covers it.

Generated by Claude Code

@drk-ram

drk-ram commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Thank you @rajnisht7 and @lywinged! Addressed both points in a12c467 (merged with origin/main at e9ab302):

  1. Entry-point kid validation on trusted_authorizer_jwk and trusted_observer_jwk:
    • Hoisted trusted_kid = trusted_*_jwk.get("kid") and if not isinstance(trusted_kid, str) or not trusted_kid: raise IntentBridgeError("trusted_*_jwk kid must be a non-empty string") immediately after _pubkey_from_jwk(...) in both verify_bridge and verify_successor_artifact, and reduced the later comparison to !=.
    • Updated test_runtime_verifier_enforces_schema_scalar_constraints and test_bridge_naming_another_authorizer_key_id_is_rejected, and added test_verify_bridge_rejects_bad_trusted_authorizer_kid_at_entry and test_verify_successor_artifact_rejects_bad_trusted_observer_kid_at_entry pairing a valid public key (None / "" / 7 / ["kid"]) with malformed or mismatched artifacts.
  2. Iterative observation cloning (_clone_observation) in evaluate_successor_observation:
    • Replaced copy.deepcopy(observation) with an iterative stack-based clone (_clone_observation) so a 600-level nested after["observation"] that passes digest_jcs evaluates cleanly to established without raising RecursionError while still detaching after["observation"] from predicate mutations (test_evaluate_successor_observation_clones_600_deep_observation).

@drk-ram
drk-ram force-pushed the drk-ram-fix-intent-bridge-successor-617 branch 3 times, most recently from a12c467 to 1479860 Compare October 29, 2026 17:13

@rajnisht7 rajnisht7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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>
@drk-ram
drk-ram force-pushed the drk-ram-fix-intent-bridge-successor-617 branch from 1479860 to 76cacd8 Compare October 30, 2026 06:31
@drk-ram

drk-ram commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@rajnisht7 Rebased onto origin/main (b938790) in 76cacd8, updated _clone_observation in src/agentrust_trace/successor_observation.py to iteratively clone tuple containers and reconstruct nested tuples bottom-up so mutable dict/list elements inside tuples are detached, and added test_predicate_cannot_mutate_nested_objects_inside_tuple_array in tests/test_successor_observation.py.

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

intent_bridge,successor_observation: validate caller options and trusted JWKs at entry and deep-copy observation before predicate

4 participants