Repository navigation
fix(agent-manifest)!: authorize the signing key for the manifest issuer - #743
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
rajnisht7
left a comment
There was a problem hiding this comment.
Overall changes looks good, a few things needs to be addressed:
- SPIFFE check if too weak, that is only
startswith("spiffe://"), same validation as the Agent Manifest SDK can be used so that malformed values dont land in the policy - Duplicate key_ids silently overwrites, these can be rejected with a ConfigError instead of taking the last entry
- Trust-anchor parsing is duplicated, keys and issuer policy should come from one parse of the same file, not two independent loaders
- Mock tests doesnt assert trusted_key_issuers,it would be good to capture and check it so a regression can’t stay green
- a few more tests can be added like empty issuers list, multiple issuers on one key, multi-key wrong signer, duplicate key_id, malformed SPIFFE, and trusted_key_issuers={}
- also a few lines lack test coverage, reported by codecov
carloshvp
left a comment
There was a problem hiding this comment.
Reviewed 8fd3c15 in isolated Python 3.12 environments with both the locked agent-manifest 0.15.0 and agent-manifest #494 at 1e8872d0664b8299a44865addc424befc4d08009 (0.16.0).
One correctness issue needs correction, detailed inline: the new issuer policy loader accepts malformed SPIFFE IDs and they actually produce AgentManifestBinding objects. This substantiates the SPIFFE-validation concern already raised by rajnisht7; the SDK does not reject these forms in either version I tested. Please reject malformed policy identities at load time and retain valid-issuer controls.
Required repository correction, separate from that finding: the PR's sole commit, 8fd3c15, has no Signed-off-by trailer. CONTRIBUTING.md requires DCO sign-off on every commit. Please add it before approval.
Validation otherwise succeeded: the complete CI test selection (unit, conformance, integration and confinement) passed with 2446 passed and 29 skipped against SDK 0.15.0, and again with 2446 passed and 29 skipped against SDK 0.16.0 at the paired PR head. Ruff, mypy on both runtime and verifier packages, Bandit, the locked-dependency audit, the strict documentation build, the quickstart execution check and git diff --check passed. Docker/hardware/optional fixtures that skipped were not independently executed.
A separate seven-case binding matrix refused missing policy, empty policy, an empty key grant, wrong-issuer authorization and subject-as-issuer authorization, while accepting the authorized and multiple-issuer controls. A mismatched key-id/public-key duplicate probe was correctly refused by the existing key-hash check; I did not establish a cross-key authorization bypass from duplicate entries.
The failed maintainer gate is a review-policy gate, distinct from these validation results. Because src/cmcp_verify/ changes, the current base policy requires two independent maintainer approvals. This PR must land before cMCP advances its locked agent-manifest dependency to 0.16.0. Requesting changes for the malformed-identity acceptance and missing DCO sign-off.
Agent Manifest spec 5.3.3 forbids a VALID manifest whose signing key has not been authorized for its declared issuer. cMCP never passed an issuer policy to the SDK, and agent-manifest 0.15.0 skips the check without one, so any trusted key could sign a manifest naming any issuer and the gateway would bind it. The trust anchor now carries an issuers list per key, read by load_agent_manifest_issuer_policy and passed to the SDK as trusted_key_issuers. verify_agent_manifest_binding refuses a binding with no policy, startup stops with AGENT_MANIFEST_BINDING_FAILED, and an unauthorized key gets its own message. cmcp verify and verify_trace_claim take the same policy and report the binding unverified without it. Works with agent-manifest 0.15.0 (pinned) and 0.16.0. Found by litzki-systems on agentrust-io/agent-manifest#493. BREAKING CHANGE: a trust anchor without issuers no longer binds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Imran Siddique <45405841+imran-siddique@users.noreply.github.com>
…or once load_agent_manifest_trust_policy reads keys and issuer policy in one parse; startup and cmcp verify use it, and the two existing loaders delegate to it. An issuer must be a well-formed SPIFFE ID: a non-empty trust domain with no userinfo, port, query or fragment, and path segments that are not empty, "." or "..". A repeated key_id is refused instead of silently replacing the earlier key. Tests cover each malformed form carloshvp and rajnisht7 named with valid controls, a repeated key_id, an empty issuers list, an empty policy, a key authorized for several issuers, another key's authorization not covering the signer, the loader's error branches, and that the policy reaches the SDK context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Imran Siddique <45405841+imran-siddique@users.noreply.github.com>
|
Rebuilt on current main as two signed commits:
The CI selection passes locally with SDK 0.15.0: 2,467 passed, 31 skipped. |
8fd3c15 to
e3e2728
Compare
rajnisht7
left a comment
There was a problem hiding this comment.
The changes looks good, fail-closed on missing/unauthorized issuer policy looks correct, SPIFFE checks and tests cover the important cases, and the binding path is clean
two small follow-ups:
- _is_spiffe_id can raise UnicodeEncodeError on a lone surrogate in the trust-anchor JSON (e.g. \ud800), soinstead of returning False the loader should always surface
ConfigError, not an unhandled traceback. - verify_agent_manifest_signature still calls _verify_with_sdk with trusted_key_issuers=None. Fine on 0.15.0; but will break pure signature checks once the pin moves to 0.16.0
… a traceback _is_spiffe_id encoded the value to measure its length, and a lone surrogate, which json.loads accepts, raised UnicodeEncodeError instead of returning False. The loader now reports ConfigError. Raised by @rajnisht7. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Imran Siddique <45405841+imran-siddique@users.noreply.github.com>
|
@rajnisht7 both taken. The lone surrogate is fixed in 283352c: @carloshvp your SPIFFE finding is fixed in e3e2728. Could you re-review by Thursday 16 October? |
carloshvp
left a comment
There was a problem hiding this comment.
Re-reviewed 283352c. My malformed-SPIFFE and DCO blockers are resolved: the combined loader validates issuers before accepting the policy, rejects repeated key IDs, and reads keys and grants from one parse. All three replacement commits carry Signed-off-by.
Independent loader probes rejected 29 malformed/type/boundary cases through each of the three public loaders (87 rejections), including my original four forms and lone surrogates, with six valid controls including the 255-byte domain and 2048-byte ID boundaries. A signed-manifest matrix rejected missing/empty policy, empty grants, wrong issuer, subject-as-issuer and another key's grant, while accepting authorized and multiple-issuer controls. These probes passed with both SDK versions.
Validation on isolated Python 3.12: the complete CI test selection passed with 2470 passed and 29 skipped against locked agent-manifest 0.15.0, and again with the paired #494 head bf21623f896c4ca1b8bec7e55a807331f10fbfa0 (0.16.0). Ruff, mypy on runtime and verifier, Bandit, the locked-dependency audit, strict docs build, quickstart execution and diff checks passed. Merge simulation against current main was clean. I inspected the hosted native-Linux run https://github.com/agentrust-io/cmcp/actions/runs/38625731903: its merge of this head into 8998b18 passed 68 confinement tests and three subtests. Other skipped hardware/optional fixtures were not independently executed.
The signature-only helper follow-up remains tied to the 0.16.0 pin change: I reproduced its refusal under that SDK, and it currently has no cMCP callers. This is not a blocker for the retained 0.15.0 pin; update it before advancing the dependency.
Approving this head. The security-path policy still requires two independent approvals on this exact head; the earlier approval on e3e2728 does not satisfy that gate.
Agent Manifest spec §5.3.3 forbids
VALIDunless the signing key is authorized for the manifest's declaredissuer. cMCP never passed an issuer policy to the SDK, and agent-manifest 0.15.0 skips the check when there is none. So today any key intrust_anchor_pathcan sign a manifest naming any issuer, and the gateway binds it. @litzki-systems surfaced the SDK side on agentrust-io/agent-manifest#493.The policy lives in the trust anchor. Each key in
trust_anchor_pathgains anissuerslist of SPIFFE URIs, in the single-key form or per entry ofkeys[].load_agent_manifest_issuer_policyreads it, andverify_agent_manifest_bindingpasses it to the SDK astrusted_key_issuers. A key withoutissuersis authorized for nothing.Fail closed. With no policy,
verify_agent_manifest_bindingraises before the SDK is called, so the gateway does not depend on the SDK's default. Startup stops withAGENT_MANIFEST_BINDING_FAILEDwhen no key listsissuers, and when the signing key is not authorized for the manifest'sissuer(new message naming §5.3.3).cmcp verifyandverify_trace_claimtake the same policy (trusted_agent_manifest_key_issuers) and reportagent_manifest.bindingunverified without it.Blast radius: no config in demos, examples or integrations uses a cMCP manifest trust anchor. In this repo, 43 tests across 6 files needed
issuersadded, plusdocs/configuration.md. A deployment whose trust anchor lacksissuersstops at startup with a message saying what to add.This touches
src/cmcp_verify/, asecurity_pathsentry, so it needs two approvals.Evidence: 2444 passed, 31 skipped (2437 before); ruff and mypy clean; the same suite also passes against agent-manifest 0.16.0 from the SDK branch. Against pre-fix source, both new startup tests fail with "DID NOT RAISE SystemExit": the old gateway starts with an anchor that authorizes no issuer, and with one that authorizes a different issuer. The old offline verifier reports the binding verified with no issuer policy at all.
Merge order: this must land before cMCP moves its pin off
agent-manifest==0.15.0. Main fails 23 tests against 0.16.0.🤖 Generated with Claude Code