diff --git a/CHANGELOG.md b/CHANGELOG.md index a229c8143..caf5e7fd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,33 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). block redirect header leaks (2.1.0, #936), make `AsyncRwTransport::receive` cancel-safe (#941/#947), fail orphaned streamable HTTP responses on reinit (#914), and negotiate protocol version in the handler (#930). No source changes required in `crates/zeph-mcp` (#5897). +- **A2A**: `AgentRegistry` (JWS Agent Card signature verification + `card_trust_policy`, + #5928) had no runtime construction site anywhere outside `crates/zeph-a2a`'s own tests — + setting `[a2a_client].card_trust_policy = "require"` had no effect on a running agent. + Wired `AgentRegistry::discover` into `zeph --connect ` (`src/tui_remote.rs`): the + peer's card is now fetched and its signature/URL-origin trust policy enforced before the + SSE session is established, using an explicit (no-wildcard-by-name) conversion from + `zeph_config::channels::CardTrustPolicy`/`TrustedAgentKey` to their `zeph-a2a` + counterparts. The discovery fetch is hardened with the same `require_tls`/ + `ssrf_protection` posture and DNS-rebinding-safe address pinning already applied to the + `A2aClient` connection to the same URL. Fixes a bug where the discovery URL was + constructed from the full `--connect` target (including its RPC path, e.g. + `/a2a/stream`) instead of the origin root where `/.well-known/agent.json` is actually + served. A discovery-fetch failure (peer serves no card, network error, timeout) only + aborts `--connect` when `card_trust_policy = "require"`; under the default `ignore` (and + `prefer`) it is logged and tolerated so a peer that serves no agent card at all still + connects, matching pre-#6200 behavior. A trust-check rejection (untrusted signature or + URL-origin mismatch) always aborts regardless of policy, since `check_trust` has already + folded the policy into that verdict (#6200). +- **A2A**: `card_signing::canonical_payload` canonicalized the raw received card JSON + verbatim (`signatures` key removed only), which rejects a genuinely valid card from any + signer that strips proto3-default-valued fields (empty string/`false`/`0`/empty + array/object) before signing per the A2A spec text, while transmitting the card with + those defaults present — a fail-closed availability bug. `canonical_payload` now strips + the same proto3-default fields recursively before JCS canonicalization, so a signature + computed over either shape verifies against the other; covered by a new synthetic + regression test (real `a2a-sdk` interop is still unvalidated — `card_trust_policy` + remains `"ignore"` by default pending a real signed-card vector) (#6201). - **Docs**: `specs/010-security/spec.md` and `specs/014-a2a/spec.md` described two different IBCT (Invocation-Bound Capability Token) wire formats, and neither fully matched the actual implementation in `crates/zeph-a2a/src/ibct.rs`. Reconciled both specs against `ibct.rs` and @@ -153,17 +180,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). "require"` is set without the `card-signing` feature compiled in, rather than silently degrading or bricking discovery. New `ZEPH_A2A_CARD_TRUST_POLICY` env override and `--migrate-config` step 82 (commented advisory block for existing configs). - - **Known limitations, tracked as follow-ups**: the JCS canonicalization and signing-input - construction were implemented from the A2A 1.0.0 spec text, not validated against a real - `a2a-sdk`-produced signed card (no network access to obtain a reference vector in this - environment) — treat `require` as unproven for real-peer interop until a vector lands. - `AgentRegistry` still has no runtime construction site in `zeph-core`/`src/`, so - `card_trust_policy` is a fully-implemented library knob with no consumer yet (pre-existing - gap, not created by this change). The well-known discovery path remains - `/.well-known/agent.json` (0.2.x); a pure-1.0.0 peer serving `/.well-known/agent-card.json` - is not yet discoverable. `A2A_PROTOCOL_VERSION` stays `"0.2.1"` — this change is one additive - 1.0.0 feature, not full 1.0.0 conformance. `jku`/JWKS auto-fetch, EdDSA/RS256, and signing - our own served card are all deferred. + - **Known limitations, tracked as follow-ups**: `AgentRegistry` had no runtime construction + site and the JCS canonicalization was unvalidated against a real `a2a-sdk`-produced signed + card — both addressed later in this same `[Unreleased]` section, see the two `A2A` entries + above (#6200, #6201). The well-known discovery path remains `/.well-known/agent.json` + (0.2.x); a pure-1.0.0 peer serving `/.well-known/agent-card.json` is not yet discoverable. + `A2A_PROTOCOL_VERSION` stays `"0.2.1"` — this change is one additive 1.0.0 feature, not full + 1.0.0 conformance. `jku`/JWKS auto-fetch, EdDSA/RS256, and signing our own served card are + all still deferred. - **Worktree**: added disk-quota and automatic reconciliation to the `zeph-worktree` subsystem (#5924). Four new `[worktree]` config fields: `max_worktrees` (creation-time admission cap, enforced as `WorktreeError::QuotaExceeded`), `disk_quota_mb` (soft total-disk-usage threshold), diff --git a/crates/zeph-a2a/src/card_signing.rs b/crates/zeph-a2a/src/card_signing.rs index f01656c5e..325df7b3e 100644 --- a/crates/zeph-a2a/src/card_signing.rs +++ b/crates/zeph-a2a/src/card_signing.rs @@ -36,18 +36,21 @@ //! //! # Known limitation — unvalidated against a real peer //! -//! // TODO(critic): real a2a-sdk interop vector not obtainable in this environment — -//! // canonicalization implemented per A2A spec §8.4 verbatim but UNVALIDATED against a -//! // real peer; verify before relying on `require` in production (#5928 follow-up) -//! //! The JCS canonicalization and signing-input construction below were implemented from //! the A2A 1.0.0 spec text (§8.4.1–§8.4.3) retrieved verbatim during design review, not //! from a real signed-card test vector produced by a reference implementation (e.g. the -//! Python/JS `a2a-sdk`). The `self_signed_round_trip_verifies` and -//! `raw_json_canonicalization_differs_from_typed_struct_reserialization` unit tests below -//! prove internal self-consistency and guard the exact bug class this module exists to avoid, -//! but neither proves interoperability with a real A2A peer's signer. Treat `require` -//! as unproven until a real vector is obtained and checked in. +//! Python/JS `a2a-sdk`). `canonical_payload` (private, used by both [`verify_card_signatures`] +//! and [`sign_card`]) strips proto3-default-valued fields (empty +//! string, `false`, `0`, empty array/object, recursively through nested objects) before +//! JCS, matching the spec text's canonicalization rules — this closes the specific +//! divergence a compliant signer that strips defaults before signing would otherwise +//! trigger against our verifier canonicalizing the full transmitted card (#6201; see +//! `signature_over_default_stripped_payload_verifies_against_full_transmitted_card` +//! below). This, `self_signed_round_trip_verifies`, and +//! `raw_json_canonicalization_differs_from_typed_struct_reserialization` prove internal +//! self-consistency and guard the bug classes this module exists to avoid, but none of +//! them prove interoperability with a real A2A peer's signer. Treat `require` as unproven +//! until a real vector is obtained and checked in. #[cfg(feature = "card-signing")] use base64::Engine as _; @@ -290,24 +293,80 @@ pub fn sign_card( }) } -/// RFC 8785 JCS canonicalization of `raw_card` with the `signatures` key removed. +/// RFC 8785 JCS canonicalization of `raw_card` with the `signatures` key removed and +/// proto3-default-valued fields stripped (#6201). /// /// Operates on the raw received [`Value`] — never on a re-serialization of the typed /// [`AgentCard`](crate::AgentCard) struct. See module docs. -// TODO(critic): if the S1 real-vector gate (see module docs) ever finds a mismatch against a -// real a2a-sdk-signed card, the likely fix is a recursive proto3-default strip (drop keys whose -// value is `""`, `false`, `0`, `[]`, or `{}`, recursively through nested objects/arrays) applied -// to `card` below *before* JCS — not a bug in the JCS library itself. Flagging this now so a -// future fix isn't misdiagnosed as a `serde_json_canonicalizer` correctness issue. +/// +/// A compliant signer may drop proto3-default-valued fields (empty string, `false`, `0`, +/// empty array/object) from the card JSON before canonicalizing and signing, per the A2A +/// spec text, while the transmitted card still carries them explicitly. Stripping the same +/// fields here — recursively, bottom-up so an object that becomes empty after its own +/// fields are stripped is itself dropped from its parent — normalizes both shapes to the +/// same canonical bytes, so a signature computed over either verifies against the other. #[cfg(feature = "card-signing")] fn canonical_payload(raw_card: &Value) -> Result, String> { let mut card = raw_card.clone(); if let Value::Object(map) = &mut card { map.remove("signatures"); } + strip_proto3_defaults(&mut card); serde_json_canonicalizer::to_vec(&card).map_err(|e| e.to_string()) } +/// `true` when `value` is a proto3 default: empty string, `false`, `0` (integer or +/// float), an empty array, or an empty object. `null` is not a proto3 JSON-mapping +/// default value and is left untouched. +// TODO(critic): the `{}` (empty object) and `0` (number) cases are the highest-risk, +// unvalidated part of this heuristic (S2, #6201 follow-up). Proto3 JSON mapping has +// *message presence*: a message field explicitly **set** to an empty message serializes +// to `{}` and is distinct from a field left **unset** (which is omitted entirely) — a +// real signer that emits `{}` for a deliberately-set-but-empty message would sign +// *with* that key present, while this function strips it, reproducing the exact +// canonical-bytes divergence #6201 exists to eliminate. The same shape applies to a +// semantically meaningful `0`. This is invisible to every in-tree test because +// `canonical_payload` is applied symmetrically to both `sign_card` and +// `verify_card_signatures` (see module docs' "unvalidated against a real peer" +// section) — it only bites against a real external `a2a-sdk` signer. If a real vector +// ever mismatches specifically on an empty-object or zero-valued field, narrow this +// function (e.g. drop the `{}`/`0` arms, keeping only string/bool/array) rather than +// assuming the JCS library itself is at fault. +#[cfg(feature = "card-signing")] +fn is_proto3_default(value: &Value) -> bool { + match value { + Value::Null => false, + Value::Bool(b) => !b, + Value::Number(n) => n.as_f64() == Some(0.0), + Value::String(s) => s.is_empty(), + Value::Array(a) => a.is_empty(), + Value::Object(o) => o.is_empty(), + } +} + +/// Recursively drops object keys whose value is a proto3 default (see +/// [`is_proto3_default`]), processing children first so a nested object that becomes +/// empty only after its own defaults are stripped is also removed from its parent. +/// Array elements are recursed into but never removed — a repeated field's cardinality +/// is significant and unlike a struct field has no "default value" to omit. +#[cfg(feature = "card-signing")] +fn strip_proto3_defaults(value: &mut Value) { + match value { + Value::Object(map) => { + map.retain(|_, v| { + strip_proto3_defaults(v); + !is_proto3_default(v) + }); + } + Value::Array(arr) => { + for v in arr.iter_mut() { + strip_proto3_defaults(v); + } + } + Value::Null | Value::Bool(_) | Value::Number(_) | Value::String(_) => {} + } +} + #[cfg(feature = "card-signing")] mod imp { use base64::Engine as _; @@ -566,29 +625,31 @@ mod tests { } /// Regression test for the S1 bug class: JCS **must** canonicalize the raw received - /// JSON with `signatures` removed, never a re-serialization of the typed `AgentCard` - /// struct. A signer that omits proto3-default fields (empty string, `false`, `0`, - /// empty array) before JCS produces canonical bytes that differ from what - /// `serde_json::to_value(&typed_card)` re-materializes, because `#[serde(default)]` - /// fields without `skip_serializing_if` are always emitted by the typed struct's - /// `Serialize` impl. Canonicalizing the typed struct's re-serialization instead of the - /// raw bytes would make a genuinely valid signature fail verification. + /// JSON, never a re-serialization of the typed `AgentCard` struct. The typed struct + /// silently drops any JSON key it doesn't recognize (no `deny_unknown_fields`, no + /// catch-all field) — canonicalizing the typed struct's re-serialization instead of + /// the raw bytes would make a genuinely valid signature fail verification whenever a + /// peer's card carries a vendor extension field the schema doesn't model. + /// + /// Before #6201's proto3-default-stripping fix, this test's premise was a *different* + /// divergence source (proto3-default fields the raw JSON omitted but the typed + /// struct's `Serialize` impl always re-materializes) — that source is now normalized + /// away by [`canonical_payload`]'s stripping, so the test uses an irreducible + /// divergence (an unknown field) that stripping cannot close. #[test] fn raw_json_canonicalization_differs_from_typed_struct_reserialization() { - // Raw wire JSON as a signer would emit it: `pushNotifications` and - // `stateTransitionHistory` (both `false`, the proto3 default) are omitted. let raw_json = serde_json::json!({ "name": "peer", - "description": "", + "description": "a peer agent", "url": "http://peer.example.com", "version": "0.1.0", "protocolVersion": "0.2.1", "capabilities": {"streaming": true}, - "skills": [], + "vendorExtension": {"trustScore": 42}, }); - // Deserializing into the typed `AgentCard` and re-serializing re-materializes - // every `#[serde(default)]` field the raw JSON omitted. + // Deserializing into the typed `AgentCard` and re-serializing silently drops + // `vendorExtension` — it has no field to land in. let typed: crate::types::AgentCard = serde_json::from_value(raw_json.clone()).unwrap(); let reserialized = serde_json::to_value(&typed).unwrap(); @@ -598,9 +659,84 @@ mod tests { assert_ne!( raw_canonical, reserialized_canonical, "raw and re-serialized-typed-struct canonical bytes must differ when the raw \ - JSON omits proto3-default fields — if this assertion fails, the typed struct's \ - Serialize impl started matching the raw wire shape exactly and this test's \ - premise no longer holds" + JSON carries a field the AgentCard schema doesn't model — if this assertion \ + fails, unknown fields are somehow surviving the typed round-trip and this \ + test's premise no longer holds" + ); + } + + /// Regression test for #6201: a compliant A2A signer may strip proto3-default-valued + /// fields (empty string/`false`/`0`/empty array/object) from the card JSON before JCS + /// canonicalization and signing (A2A spec §8.4.1), while the transmitted card still + /// carries those defaults explicitly. Before this fix, `canonical_payload` canonicalized + /// the raw received JSON verbatim (`signatures` removed only), so a signature computed + /// over the signer's default-stripped payload would fail to verify against the full + /// transmitted card — a fail-closed availability bug rejecting a genuinely valid, + /// untampered card. + #[test] + fn signature_over_default_stripped_payload_verifies_against_full_transmitted_card() { + let signing_key = SigningKey::from_bytes(&[44u8; 32].into()).unwrap(); + + // What a compliant signer canonicalizes and signs: proto3-default fields + // (`description`, `defaultInputModes`, `pushNotifications`, ...) are absent. + let signer_payload = serde_json::json!({ + "name": "peer-agent", + "url": "http://peer.example.com", + "version": "0.1.0", + "protocolVersion": "0.2.1", + "capabilities": {"streaming": true}, + }); + let sig = sign_card(&signer_payload, "key-1", &signing_key).unwrap(); + + // What actually arrives over the wire: the same card with every proto3-default + // field present and explicit. + let transmitted_card = serde_json::json!({ + "name": "peer-agent", + "description": "", + "url": "http://peer.example.com", + "version": "0.1.0", + "protocolVersion": "0.2.1", + "capabilities": { + "streaming": true, + "pushNotifications": false, + "stateTransitionHistory": false, + "images": false, + "audio": false, + "files": false + }, + "defaultInputModes": [], + "defaultOutputModes": [], + "skills": [], + "signatures": [&sig], + }); + + let trusted = vec![trusted_key_for("key-1", &signing_key)]; + let result = verify_card_signatures(&transmitted_card, &[sig], &trusted); + assert_eq!( + result, + SignatureVerification::Verified, + "verification must succeed even when the signer stripped proto3-default \ + fields before signing but the transmitted card carries them explicitly" + ); + } + + #[test] + fn strip_proto3_defaults_removes_nested_object_that_becomes_empty() { + let mut value = serde_json::json!({ + "name": "peer", + "capabilities": {"streaming": false, "images": false}, + "skills": [{"id": "s1", "tags": []}], + }); + strip_proto3_defaults(&mut value); + assert_eq!( + value, + serde_json::json!({ + "name": "peer", + "skills": [{"id": "s1"}], + }), + "an object whose fields are all proto3 defaults must itself be dropped from \ + its parent, and array elements must be recursed into (never removed from \ + the array itself)" ); } } diff --git a/crates/zeph-a2a/src/discovery.rs b/crates/zeph-a2a/src/discovery.rs index 184321679..295c61e2f 100644 --- a/crates/zeph-a2a/src/discovery.rs +++ b/crates/zeph-a2a/src/discovery.rs @@ -23,9 +23,10 @@ const WELL_KNOWN_PATH: &str = "/.well-known/agent.json"; /// Mirrors `zeph_config::channels::CardTrustPolicy` (TOML-facing) as an independent /// type, the same way `zeph_mcp::ToolDiscoveryStrategy` mirrors its `zeph-config` /// counterpart: `zeph-config` must not depend on protocol crates, so config-side and -/// protocol-side enums are converted at the `zeph-core` wiring layer. -// TODO(critic): no runtime construction site consumes card_trust_policy yet — file -// wire-X follow-up before advertising the knob as enforcing (#5928). +/// protocol-side enums are converted where both crates are in scope — the top-level +/// `zeph` binary crate (`src/tui_remote.rs::convert_card_trust_policy`), which wires +/// `[a2a_client].card_trust_policy`/`trusted_agent_keys` into the `AgentRegistry::discover` +/// call performed before `zeph --connect ` establishes an A2A session (#6200). #[non_exhaustive] #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum CardTrustPolicy { @@ -236,18 +237,19 @@ impl AgentRegistry { /// ``` #[must_use] pub fn with_trust(mut self, policy: CardTrustPolicy, trusted_keys: Vec) -> Self { - // Breadcrumb for operators enabling enforcement: the S1 canonicalization/signing-input - // construction is implemented per the A2A spec text but has not been validated against - // a real peer's signer (see `crate::card_signing` module docs). Without this, a - // `require`-policy reject-all failure mode is loud in logs but the *cause* (unproven - // interop, not a real attack) is not obvious. Logged here rather than only in a doc - // comment, which the operator flipping the knob at runtime will never read. + // Breadcrumb for operators enabling enforcement (S3, restored after #6200/#6201 + // review): card-signature interop with a real A2A peer is still unvalidated — the + // JCS canonicalization (including #6201's proto3-default stripping) is implemented + // per the A2A spec text only, never checked against a real `a2a-sdk`-produced + // signed card (see `crate::card_signing` module docs). This condition doesn't + // change based on which gap is currently open, so it's logged here rather than + // only in a doc comment an operator flipping the knob at runtime will never read. if policy != CardTrustPolicy::Ignore { tracing::warn!( policy = ?policy, "a2a discovery: card signature interop is unvalidated against a real A2A peer \ - (#5928) — canonicalization/signing-input construction is implemented per spec \ - text only; `require` may reject genuinely valid signed peers" + (#5928/#6201) — canonicalization is implemented per spec text only; `require` \ + may reject genuinely valid signed peers" ); } self.trust = TrustConfig { diff --git a/crates/zeph-a2a/src/lib.rs b/crates/zeph-a2a/src/lib.rs index 35da8f3bd..80f19e32d 100644 --- a/crates/zeph-a2a/src/lib.rs +++ b/crates/zeph-a2a/src/lib.rs @@ -22,9 +22,10 @@ //! # Architecture //! //! `zeph-a2a` is an optional feature-gated dependency of the main `zeph` binary. The -//! `A2aServer` is started by `zeph-core` as a background service when `[a2a]` is enabled -//! in config. The [`A2aClient`] is used by the agent to delegate tasks to peer agents -//! discovered through the [`AgentRegistry`]. +//! `A2aServer` is started as a background service when `[a2a]` is enabled in config. The +//! [`AgentRegistry`] verifies a peer's [`AgentCard`] (signature + URL-origin trust policy, +//! A2A 1.0.0 §8.4) before `zeph --connect ` establishes a session via [`A2aClient`] +//! (#6200); see `src/tui_remote.rs` in the `zeph` binary crate for the wiring. //! //! # Features //! diff --git a/crates/zeph-config/src/channels.rs b/crates/zeph-config/src/channels.rs index 991080c67..806187bec 100644 --- a/crates/zeph-config/src/channels.rs +++ b/crates/zeph-config/src/channels.rs @@ -1000,10 +1000,10 @@ impl Default for A2aClientConfig { /// /// Mirrors `zeph_a2a::discovery::CardTrustPolicy` (protocol-crate-facing) as an /// independent type — `zeph-config` must not depend on protocol crates, the same reason -/// [`McpTrustLevel`] has no `zeph-mcp` counterpart dependency. Conversion happens at the -/// `zeph-core` wiring layer once a runtime `AgentRegistry` construction site exists -/// (currently none does — see the `card_trust_policy` field doc and the `discovery.rs` -/// TODO in `zeph-a2a`). +/// [`McpTrustLevel`] has no `zeph-mcp` counterpart dependency. Conversion happens in the +/// top-level `zeph` binary crate (`src/tui_remote.rs::convert_card_trust_policy`), which +/// constructs the `AgentRegistry` used before `zeph --connect ` establishes an A2A +/// session (#6200). #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] #[non_exhaustive] diff --git a/specs/014-a2a/spec.md b/specs/014-a2a/spec.md index b368bedd7..3553bb080 100644 --- a/specs/014-a2a/spec.md +++ b/specs/014-a2a/spec.md @@ -282,17 +282,29 @@ re-scope it) — do not bump the version as a side effect of landing one more 1. ### Current limitations -- **No live consumer** (#6200): `AgentRegistry` has no runtime construction site anywhere in - the codebase outside `crates/zeph-a2a` itself. Nothing in the running agent currently - performs A2A peer discovery, so `card_trust_policy` — including `require` — enforces nothing - in a running Zeph instance today. It is a fully implemented and tested library + config - primitive, not yet wired into `zeph-core`. -- **Unvalidated interop** (#6201): the RFC 8785 JCS canonicalization in `card_signing.rs` is - implemented from the A2A 1.0.0 spec text only, never checked against a real `a2a-sdk` - reference-implementation signed-card vector (no network access during development). If a - real peer strips proto3-default fields before signing but transmits the full card on the - wire, verification could incorrectly reject a genuinely valid card. Treat `require` as - unproven against real peers until a real vector is obtained and checked in as a test case. +- **Live consumer wired to `--connect` only** (#6200, fixed): `AgentRegistry::discover` is now + called by `src/tui_remote.rs::run_tui_remote` before `zeph --connect ` establishes its + SSE session — the origin (not the RPC path) is fetched and `card_trust_policy` enforces + against it, with `require_tls`/`ssrf_protection`/address-pinning applied to the discovery + fetch itself. A discovery-fetch failure (peer serves no card, network error, timeout) only + aborts `--connect` under `card_trust_policy = "require"`; under `ignore`/`prefer` it is + logged and tolerated so a peer serving no agent card at all still connects, matching + pre-#6200 behavior — a trust-check rejection (untrusted signature or URL-origin mismatch) + always aborts regardless of policy, since `check_trust` has already folded the policy into + that verdict (`discovery_error_is_fatal` in `src/tui_remote.rs`). This closes the "no live + consumer" gap for the `--connect` client path specifically. It remains true that no *agent- + to-agent delegation* consumer exists — no tool or code path lets the running agent loop + discover and call an arbitrary peer on its own initiative; `AgentRegistry`'s only live + caller is the `--connect` attach flow. +- **Unvalidated interop, partially addressed** (#6201): the RFC 8785 JCS canonicalization in + `card_signing.rs` now strips proto3-default-valued fields (empty string/`false`/`0`/empty + array/object, recursively) before canonicalizing, closing the specific divergence where a + real peer that strips defaults before signing but transmits the full card on the wire would + have its signature incorrectly rejected — covered by a synthetic regression test. This is + still not validated against a real `a2a-sdk` reference-implementation signed-card vector (no + network access during development), so `require` remains unproven against real peers in + general until such a vector is obtained and checked in as a test case; `card_trust_policy` + intentionally still defaults to `"ignore"`, not `"prefer"`. --- diff --git a/src/tui_remote.rs b/src/tui_remote.rs index dacf805e5..ce6799149 100644 --- a/src/tui_remote.rs +++ b/src/tui_remote.rs @@ -36,6 +36,156 @@ fn resolve_client_security_policy( } } +/// Converts `zeph_config::channels::CardTrustPolicy` (TOML-facing) to +/// `zeph_a2a::CardTrustPolicy` (protocol-facing) — the two enums are independent (no +/// cross-crate dependency between `zeph-config` and `zeph-a2a`, see both types' doc +/// comments), so every currently-known variant is mapped explicitly by name. +/// +/// Both enums are `#[non_exhaustive]`, so the compiler requires a trailing wildcard arm +/// here despite every variant being named — that arm is unreachable today and is a fail +/// **closed** fallback (maps to `Require`, the strictest policy), not a silent downgrade, +/// should a future variant land on one side without a matching update here. The round-trip +/// test below enumerates every variant explicitly so drift is caught in CI rather than at +/// runtime. +#[cfg(all(feature = "tui", feature = "a2a"))] +fn convert_card_trust_policy( + policy: zeph_core::config::CardTrustPolicy, +) -> zeph_a2a::CardTrustPolicy { + match policy { + zeph_core::config::CardTrustPolicy::Ignore => zeph_a2a::CardTrustPolicy::Ignore, + zeph_core::config::CardTrustPolicy::Prefer => zeph_a2a::CardTrustPolicy::Prefer, + zeph_core::config::CardTrustPolicy::Require => zeph_a2a::CardTrustPolicy::Require, + _ => { + tracing::error!( + ?policy, + "a2a_client.card_trust_policy: unrecognized variant, failing closed to `require`" + ); + zeph_a2a::CardTrustPolicy::Require + } + } +} + +/// Converts `[a2a_client].trusted_agent_keys` into `zeph_a2a::TrustedKey`s, parsing each +/// entry's `alg` string via [`zeph_a2a::SigAlg::from_jws_alg`]. An entry with an +/// unrecognized `alg` is dropped (with a `tracing::warn!`) rather than carried through as +/// a key nothing can ever match — `verify_card_signatures` only matches on `kid` + `alg`. +#[cfg(all(feature = "tui", feature = "a2a"))] +fn convert_trusted_agent_keys( + keys: &[zeph_core::config::TrustedAgentKey], +) -> Vec { + keys.iter() + .filter_map(|k| { + if let Some(alg) = zeph_a2a::SigAlg::from_jws_alg(&k.alg) { + Some(zeph_a2a::TrustedKey { + kid: k.kid.clone(), + alg, + key_material: k.jwk_or_pem.clone(), + }) + } else { + tracing::warn!( + kid = %k.kid, + alg = %k.alg, + "a2a_client.trusted_agent_keys: unsupported alg, key ignored" + ); + None + } + }) + .collect() +} + +/// Strips a `--connect` URL down to its origin (`scheme://host[:port]`, no path) for use +/// with `AgentRegistry::discover` (#6200). +/// +/// The agent card is served at the origin root (`/.well-known/agent.json`), independent of +/// whatever path the `--connect` URL's RPC endpoint is mounted at (e.g. `/a2a/stream` per +/// the CLI usage example) — see `crates/zeph-a2a/src/server/router.rs`'s route table. +/// Passing the full `--connect` URL (path included) to `discover` would fetch +/// `.../a2a/stream/.well-known/agent.json`, which does not exist. +/// +/// Falls back to `url` unchanged if it fails to parse — `discover`'s own `url::Url::parse` +/// inside `check_origin` will then surface the same parse failure as a mismatch. +#[cfg(all(feature = "tui", feature = "a2a"))] +fn discovery_origin(url: &str) -> String { + url::Url::parse(url).map_or_else(|_| url.to_owned(), |u| u.origin().ascii_serialization()) +} + +/// Decides whether a `discover()` failure should abort `--connect`, or be logged and +/// tolerated so the SSE session still establishes (S1 critic finding on #6200). +/// +/// [`A2aError::UntrustedCard`]/[`A2aError::UrlMismatch`] come from `check_trust`, which has +/// already folded `card_trust_policy` into its verdict (`prefer` only rejects a *tampered* +/// signature; `require` rejects any unverifiable/mismatched card) — that verdict is the +/// policy's own decision and stays fatal under every policy, since silently connecting +/// anyway would defeat `prefer`'s one hard guarantee as well as `require`'s. +/// +/// Every other error (network failure, non-2xx, malformed JSON, timeout) is a fetch/parse +/// failure, not a trust decision. Before #6200, `--connect` never attempted discovery at +/// all, so a peer that speaks A2A JSON-RPC/streaming but serves no card at all (an +/// older/non-compliant/non-zeph peer, or a transient outage) connected fine. Regressing +/// that path is only justified under [`CardTrustPolicy::Require`], where a card is +/// mandatory to enforce anything — under `ignore`/`prefer`, discovery is best-effort. +/// +/// [`A2aError::UntrustedCard`]: zeph_a2a::A2aError::UntrustedCard +/// [`A2aError::UrlMismatch`]: zeph_a2a::A2aError::UrlMismatch +/// [`CardTrustPolicy::Require`]: zeph_a2a::CardTrustPolicy::Require +#[cfg(all(feature = "tui", feature = "a2a"))] +fn discovery_error_is_fatal(error: &zeph_a2a::A2aError, policy: zeph_a2a::CardTrustPolicy) -> bool { + match error { + zeph_a2a::A2aError::UntrustedCard { .. } | zeph_a2a::A2aError::UrlMismatch { .. } => true, + _ => policy == zeph_a2a::CardTrustPolicy::Require, + } +} + +/// Builds the `reqwest::Client` used for the one-shot `AgentRegistry::discover` call +/// performed before `--connect` establishes its SSE session (#6200), applying the same +/// `require_tls`/`ssrf_protection` posture `security` already carries for the `A2aClient` +/// itself — the discovery fetch of the very same URL must not silently bypass the posture +/// the operator configured for it. Mirrors `A2aClient`'s internal hardened-client +/// construction: redirects disabled, TLS enforced via `https_only`, and (when +/// `ssrf_protection` is set) the connection pinned to the addresses validated by +/// [`resolve_and_validate`](zeph_common::net::resolve_and_validate), closing the same +/// DNS-rebinding TOCTOU window `A2aClient` closes for its own requests. +/// +/// # Errors +/// +/// Returns an error if `require_tls` is set and `url` is not `https://`, if +/// `ssrf_protection` is set and `url`'s host resolves to a private/loopback/link-local +/// address, or if the underlying `reqwest::Client` fails to build. +#[cfg(all(feature = "tui", feature = "a2a"))] +async fn hardened_discovery_client( + url: &str, + security: zeph_a2a::SecurityPolicy, +) -> anyhow::Result { + if security.require_tls && !url.starts_with("https://") { + anyhow::bail!("a2a_client.require_tls is set but --connect target uses http://: {url}"); + } + + let mut builder = reqwest::Client::builder() + .user_agent(concat!( + "zeph/", + env!("CARGO_PKG_VERSION"), + " (a2a-discovery)" + )) + .redirect(reqwest::redirect::Policy::none()); + if security.require_tls { + builder = builder.https_only(true); + } + if security.ssrf_protection + && let Ok(parsed) = url::Url::parse(url) + && let Some(host) = parsed.host_str() + { + let port = parsed.port_or_known_default().unwrap_or(443); + let addrs = zeph_common::net::resolve_and_validate(host, port) + .await + .map_err(|e| anyhow::anyhow!("a2a discovery SSRF validation failed for {url}: {e}"))?; + builder = builder.resolve_to_addrs(host, &addrs); + } + + builder + .build() + .map_err(|e| anyhow::anyhow!("failed to build hardened discovery client: {e}")) +} + #[cfg(all(feature = "tui", feature = "a2a"))] #[allow(clippy::too_many_lines)] pub(crate) async fn run_tui_remote( @@ -54,6 +204,39 @@ pub(crate) async fn run_tui_remote( // `[a2a_client]` is a dedicated client-side policy for this `--connect` path — distinct // from `[a2a]` (`A2aServerConfig`), which only governs this process's own A2A server (#5878). let security = resolve_client_security_policy(&url, &config.a2a_client); + + // Verify the peer's AgentCard (signature + URL-origin trust policy, A2A 1.0.0 §8.4) + // before establishing the SSE session — this is the `AgentRegistry` construction site + // that makes `[a2a_client].card_trust_policy` actually enforce (#6200); previously + // nothing in the codebase ever called `AgentRegistry::discover`. + let discovery_base_url = discovery_origin(&url); + let discovery_client = hardened_discovery_client(&discovery_base_url, security).await?; + let trust_policy = convert_card_trust_policy(config.a2a_client.card_trust_policy); + let registry = zeph_a2a::AgentRegistry::new(discovery_client, Duration::from_mins(5)) + .with_trust( + trust_policy, + convert_trusted_agent_keys(&config.a2a_client.trusted_agent_keys), + ); + match registry.discover(&discovery_base_url).await { + Ok(peer_card) => { + tracing::info!( + peer = %peer_card.name, + policy = ?config.a2a_client.card_trust_policy, + "a2a discovery: peer card verified per card_trust_policy" + ); + } + Err(e) if discovery_error_is_fatal(&e, trust_policy) => { + anyhow::bail!("A2A peer discovery/trust check failed for {discovery_base_url}: {e}"); + } + Err(e) => { + tracing::warn!( + error = %e, + policy = ?config.a2a_client.card_trust_policy, + "a2a discovery: could not fetch peer card, proceeding without trust verification" + ); + } + } + let client = zeph_a2a::A2aClient::new(zeph_core::http::default_client()).with_security(security); @@ -216,8 +399,11 @@ pub(crate) async fn run_tui_remote( #[cfg(all(test, feature = "tui", feature = "a2a"))] mod tests { - use super::resolve_client_security_policy; - use zeph_core::config::A2aClientConfig; + use super::{ + convert_card_trust_policy, convert_trusted_agent_keys, discovery_error_is_fatal, + discovery_origin, hardened_discovery_client, resolve_client_security_policy, + }; + use zeph_core::config::{A2aClientConfig, CardTrustPolicy, TrustedAgentKey}; fn hardened_client_cfg() -> A2aClientConfig { A2aClientConfig::default() @@ -365,4 +551,223 @@ mod tests { assert!(policy.require_tls); assert!(policy.ssrf_protection); } + + // --- convert_card_trust_policy / convert_trusted_agent_keys (#6200) --- + + #[test] + fn card_trust_policy_round_trip_covers_all_known_variants() { + assert_eq!( + convert_card_trust_policy(CardTrustPolicy::Ignore), + zeph_a2a::CardTrustPolicy::Ignore + ); + assert_eq!( + convert_card_trust_policy(CardTrustPolicy::Prefer), + zeph_a2a::CardTrustPolicy::Prefer + ); + assert_eq!( + convert_card_trust_policy(CardTrustPolicy::Require), + zeph_a2a::CardTrustPolicy::Require + ); + } + + #[test] + fn trusted_agent_keys_recognized_alg_converts() { + let keys = vec![TrustedAgentKey { + kid: "key-1".into(), + alg: "ES256".into(), + jwk_or_pem: "pem-data".into(), + }]; + let converted = convert_trusted_agent_keys(&keys); + assert_eq!(converted.len(), 1); + assert_eq!(converted[0].kid, "key-1"); + assert_eq!(converted[0].alg, zeph_a2a::SigAlg::Es256); + assert_eq!(converted[0].key_material, "pem-data"); + } + + #[test] + fn trusted_agent_keys_unsupported_alg_is_dropped() { + let keys = vec![ + TrustedAgentKey { + kid: "key-1".into(), + alg: "ES256".into(), + jwk_or_pem: "pem-data".into(), + }, + TrustedAgentKey { + kid: "key-2".into(), + alg: "EdDSA".into(), + jwk_or_pem: "pem-data-2".into(), + }, + ]; + let converted = convert_trusted_agent_keys(&keys); + assert_eq!( + converted.len(), + 1, + "the unsupported-alg key must be dropped, not carried through unmatchable" + ); + assert_eq!(converted[0].kid, "key-1"); + } + + #[test] + fn trusted_agent_keys_empty_input_converts_to_empty() { + assert!(convert_trusted_agent_keys(&[]).is_empty()); + } + + // --- discovery_origin (#6200) --- + + #[test] + fn discovery_origin_strips_rpc_path() { + // Regression test: the well-known agent card is served at the origin root, not + // relative to the `--connect` URL's RPC path — passing the full URL through + // unchanged would make `discover` fetch `.../a2a/stream/.well-known/agent.json`, + // which does not exist (see `crates/zeph-a2a/src/server/router.rs`'s route table). + assert_eq!( + discovery_origin("http://127.0.0.1:8080/a2a/stream"), + "http://127.0.0.1:8080" + ); + } + + #[test] + fn discovery_origin_strips_path_and_query() { + assert_eq!( + discovery_origin("https://agent.example.com/a2a/stream?foo=bar"), + "https://agent.example.com" + ); + } + + #[test] + fn discovery_origin_omits_default_port() { + assert_eq!( + discovery_origin("https://agent.example.com:443/a2a/stream"), + "https://agent.example.com" + ); + } + + #[test] + fn discovery_origin_preserves_non_default_port() { + assert_eq!( + discovery_origin("https://agent.example.com:9443/a2a/stream"), + "https://agent.example.com:9443" + ); + } + + #[test] + fn discovery_origin_falls_back_to_input_on_parse_failure() { + assert_eq!(discovery_origin("not a url"), "not a url"); + } + + // --- discovery_error_is_fatal (S1, #6200) --- + + #[test] + fn discovery_error_untrusted_card_is_always_fatal() { + let err = zeph_a2a::A2aError::UntrustedCard { + reason: "bad signature".into(), + }; + for policy in [ + zeph_a2a::CardTrustPolicy::Ignore, + zeph_a2a::CardTrustPolicy::Prefer, + zeph_a2a::CardTrustPolicy::Require, + ] { + assert!( + discovery_error_is_fatal(&err, policy), + "UntrustedCard must stay fatal under {policy:?} — it's the policy's own verdict" + ); + } + } + + #[test] + fn discovery_error_url_mismatch_is_always_fatal() { + let err = zeph_a2a::A2aError::UrlMismatch { + queried: "http://a".into(), + advertised: "http://b".into(), + }; + for policy in [ + zeph_a2a::CardTrustPolicy::Ignore, + zeph_a2a::CardTrustPolicy::Prefer, + zeph_a2a::CardTrustPolicy::Require, + ] { + assert!(discovery_error_is_fatal(&err, policy)); + } + } + + #[test] + fn discovery_error_fetch_failure_not_fatal_under_ignore_or_prefer() { + let discovery_err = zeph_a2a::A2aError::Discovery { + url: "http://peer.example.com/.well-known/agent.json".into(), + reason: "HTTP 404".into(), + }; + let timeout_err = zeph_a2a::A2aError::Timeout(std::time::Duration::from_secs(10)); + for err in [&discovery_err, &timeout_err] { + assert!( + !discovery_error_is_fatal(err, zeph_a2a::CardTrustPolicy::Ignore), + "a card-less/unreachable peer must still connect under `ignore` (pre-#6200 \ + behavior) — {err}" + ); + assert!( + !discovery_error_is_fatal(err, zeph_a2a::CardTrustPolicy::Prefer), + "discovery is best-effort under `prefer` too — {err}" + ); + } + } + + #[test] + fn discovery_error_fetch_failure_fatal_under_require() { + let err = zeph_a2a::A2aError::Discovery { + url: "http://peer.example.com/.well-known/agent.json".into(), + reason: "HTTP 404".into(), + }; + assert!( + discovery_error_is_fatal(&err, zeph_a2a::CardTrustPolicy::Require), + "require` cannot enforce a trust policy without a card, so a fetch failure must \ + still abort --connect" + ); + } + + // --- hardened_discovery_client (#6200) --- + + #[tokio::test] + async fn discovery_client_rejects_http_when_require_tls_set() { + let security = zeph_a2a::SecurityPolicy { + require_tls: true, + ssrf_protection: false, + }; + let result = hardened_discovery_client("http://agent.example.com/", security).await; + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("require_tls")); + } + + #[tokio::test] + async fn discovery_client_permissive_loopback_builds_without_network() { + // Mirrors the loopback carve-out `resolve_client_security_policy` already applies: + // no TLS/SSRF check should require any network access for a permissive policy. + let security = zeph_a2a::SecurityPolicy { + require_tls: false, + ssrf_protection: false, + }; + let result = hardened_discovery_client("http://127.0.0.1:8080/a2a/stream", security).await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn discovery_client_accepts_https_when_require_tls_set() { + let security = zeph_a2a::SecurityPolicy { + require_tls: true, + ssrf_protection: false, + }; + let result = hardened_discovery_client("https://agent.example.com/", security).await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn discovery_client_ssrf_protection_rejects_loopback_target() { + let security = zeph_a2a::SecurityPolicy { + require_tls: false, + ssrf_protection: true, + }; + // Unlike the loopback carve-out in `resolve_client_security_policy` (which only + // applies when the *policy* is computed for a loopback target), this directly + // exercises `resolve_and_validate`'s own private-address rejection when SSRF + // protection is explicitly requested against a loopback address. + let result = hardened_discovery_client("http://127.0.0.1:8080/a2a/stream", security).await; + assert!(result.is_err()); + } }