spec: optional agent-identity binding in the Trust Record (#33)#37
spec: optional agent-identity binding in the Trust Record (#33)#37aryanputta wants to merge 1 commit into
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AGT Contributor Check. |
imran-siddique
left a comment
There was a problem hiding this comment.
Good direction and the backward-compatibility story is sound. Before this merges there are several things that need addressing -- some are correctness issues, others are gaps a verifier implementer would hit.
1. binding is a free string with no enumeration or registry
The only value shown anywhere is "svid-matched". If binding carries semantic meaning for verifiers (and it must, or why carry it?), a free string breaks interoperability: two implementations using "svid-matched" and "SVID_MATCH" describe the same thing but won't compare equal. Either enumerate the initial allowed values in the spec (e.g. svid-matched, manifest-presented, operator-asserted) and define an extension registry, or clarify that binding is informational-only and verifiers MUST NOT make trust decisions based on its value. Right now the spec implies it is meaningful but gives verifiers nothing to act on.
2. AgentIdentity allows all three fields to be None simultaneously
The model sets all three fields optional individually. That means agent: {} and agent: {"binding": "svid-matched"} (no agent_id, no manifest_id) are valid records. An agent block with no agent_id and no manifest_id is unverifiable and carries zero evidence -- it is noise. The spec should state whether a present agent block MUST have at minimum agent_id + manifest_id to be considered binding evidence (even if binding is optional). If a partial block is valid and "just informational," say so explicitly and specify what a verifier does with it.
3. manifest_id has no format constraint
The example shows a UUID v7. The field is a bare str with no pattern. A verifier implementing the §3.3 cross-check (step 2: agent.manifest_id equals manifest manifest_id) needs to know the format to reliably compare values. Is it a UUID? A URI? A hash? If UUID v7 is the intended format, add a pattern. If it is format-agnostic, state that explicitly and note that comparison is byte-equal string matching.
4. §3.3 cross-check step 3 references a catalog hash field that does not exist in the schema
"The policy and catalog hashes the manifest binds equal
policy.bundle_hash(and the catalog hash where the profile carries one)"
The current TRACE schema has policy.bundle_hash but no catalog hash field anywhere. "Where the profile carries one" implies a future or profile-specific extension. A verifier implementing step 3 today has nothing to check for the catalog half of this claim. Either remove the catalog hash part (it is unimplementable against v0.1 records), mark it explicitly as a future extension (with a TODO note and issue reference), or add the field to the schema now.
5. No example JSON showing the agent block
The examples/ directory has intel-tdx.json, amd-sev-snp.json, nvidia-h100.json. None will show the agent block after this merges. A verifier implementer reading the spec needs a concrete end-to-end example. Add one -- either a new example file or an annotated variant of one of the existing ones.
6. The spec says subject and agent.agent_id are "distinct" but does not forbid equality
In the cMCP reference design the session SVID and agent identity are always different, but nothing in the spec prevents agent.agent_id == subject. Should a verifier reject or warn on this? If it is always a mistake, add a MUST NOT. If there are valid deployments where they can be equal, say so.
7. __pycache__ files committed
The diff includes binary .pyc files (src/agentrust_trace/__pycache__/, tests/__pycache__/). These should not be committed. Add __pycache__/ and *.pyc to .gitignore and amend.
8. CHANGELOG conflict with #38
Both #37 and #38 add a new ## [Unreleased] section at the same position in CHANGELOG.md. Merging both as-is will produce a conflict or a duplicate section. One of them should rebase on the other so the changelog entries are merged into a single [Unreleased] block. Given that #38 is guidance-only and #37 changes the schema, #38 should rebase on #37.
The core idea is solid. Resolve 1-4 (the correctness and spec-completeness issues) first. 5-8 are mechanical but should be done before merge.
imran-siddique
left a comment
There was a problem hiding this comment.
Good work on the direction here - separating the agent identity from the gateway session subject is exactly right. A few things to work through before this is ready to merge.
Compiled bytecode files in the diff
The diff includes src/agentrust_trace/__pycache__/*.pyc and tests/__pycache__/*.pyc. These should never be committed; add __pycache__/ and *.pyc to .gitignore (or fix the existing .gitignore if it has them already), then strip these files from the branch before merge. Committing bytecode makes the diff unreadable, bloats history, and will cause spurious conflicts on any Python version change.
Spec-to-implementation misalignment
The spec agent block has three fields: agent_id, manifest_id, binding. But the cMCP reference implementation (PR #315) carries gateway.agent_identity with seven fields: manifest_id, agent_id, authenticated_subject, issuer, issuer_key_id, policy_bundle_hash, tool_catalog_hash. These do not align. authenticated_subject, issuer, issuer_key_id, policy_bundle_hash, and tool_catalog_hash are present in the implementation but absent from the spec's agent block. Either the spec needs to expand the agent block to match what cMCP actually emits, or the implementation needs to be trimmed to match the spec. As it stands, a verifier reading the TRACE spec will not know to look for policy_bundle_hash in the agent block; a verifier reading a cMCP Trust Record will see fields the spec does not describe.
binding is a free-form string with one documented value
binding is described as "how the gateway established the binding (e.g. svid-matched)". There is only one example value. For TRACE to be interoperable, binding types need to be an enumerated set with defined semantics. svid-matched is presumably "the authenticated SVID subject matched agent_id" but this needs to be normative text, not just an example. A verifier cannot meaningfully interpret binding without a definition.
Empty agent block passes validation
All three fields in AgentIdentity are optional (None by default). This means "agent": {} is valid. An empty agent block carries no information but claims "agent identity binding was configured." Add a model validator that requires at least agent_id and manifest_id when the agent block is present, or make both required.
Test coverage gap
The test test_agent_id_rejects_http_scheme covers the SPIFFE/DID pattern enforcement. Add a parallel test for a valid DID URI (e.g. did:web:factory.example) to confirm DID-type identities are accepted. Currently only the rejection path is tested.
Add an OPTIONAL agent block (agent_id, manifest_id, binding) distinct from subject so the manifest-to-session binding (cMCP #302) is verifiable offline. subject still identifies the gateway session; records without the block stay valid. Adds the offline agent-identity cross-check to the verification protocol. Review feedback (imran-siddique): - agent block now requires agent_id + manifest_id when present (no empty/partial blocks); binding is optional and informational only, with a registered value set (svid-matched, manifest-presented, operator-asserted) verifiers MUST NOT base trust on. - manifest_id documented format-agnostic (byte-equal comparison). - catalog-hash half of the cross-check marked a future extension (no v0.1 field); tracked as §7 open questions plus the binding registry. - documented canonical block vs profile addenda: cMCP carries richer evidence (issuer, hashes, authenticated_subject) in gateway.agent_identity; the canonical block is the portable subset every TRACE verifier relies on. - subject == agent.agent_id explicitly permitted. - synced the packaged schema (src/agentrust_trace/schema/trace-v0.1.json) that validate() actually uses, in addition to schema/trace-claim.json. - added examples/agent-bound-tdx.json and a positive DID acceptance test. - added .gitignore; no bytecode committed. 47 tests pass; ruff clean. Signed-off-by: Aryan Putta <aryansputta@gmail.com>
06ff42c to
3cb0611
Compare
|
Thanks for the thorough pass. Addressed all points; force-pushed a single clean commit.
Spec-to-impl misalignment (#315): rather than expand the canonical block, I documented the layering: the canonical Coverage gap — added Also synced |
Closes #33.
Problem
The Trust Record
subjectis the gateway session SVID. It proves a session ran an approved policy and catalog, but not that the agent the operator reviewed (the signed Agent Manifestagent_id) is the agent that acted. cMCP #302 (Option A) binds the manifest to the session at the gateway; for the Option B offline cross-check, the bound identity has to travel in the Trust Record.Change
Adds an OPTIONAL
agentblock, distinct fromsubject:spec/trace-v0.1.md): schema-table row, new §3.1.1, and an optional offline agent-identity cross-check in §3.3. Normative additions marked with<!-- CHANGED: #33 -->.schema/trace-claim.json): optionalagentobject withagent_id(SPIFFE/DID pattern),manifest_id,binding;additionalProperties: false.src/agentrust_trace/models.py):AgentIdentity+ optionalagentfield onTrustRecord, exported.Compatibility
Backward compatible: all fields OPTIONAL,
subjectunchanged, existing records validate unchanged. A present-but-uncheckableagentblock is unverified, not invalid. Affects Level 1+ (identity binding); existing conformance cases unaffected.DCO signed-off. 41/41 tests pass locally.