a2a: honour timeout and allow_private_ips config (with SSRF protection)#2882
a2a: honour timeout and allow_private_ips config (with SSRF protection)#2882dgageot wants to merge 3 commits into
timeout and allow_private_ips config (with SSRF protection)#2882Conversation
- allow `allow_private_ips: true` on a2a toolsets in validate.go - register only the SSRF-safe JSON-RPC transport via WithDefaultsDisabled so the gRPC default cannot bypass httpclient.NewSafeClient - fall back to http.DefaultTransport before wrapping with the header transport when private IPs are allowed (NewSafeClient leaves Transport nil in that branch) - add direct tests for both the rejection-by-default and the end-to-end streaming-with-allow paths Assisted-By: docker-agent
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes correctly align the a2a toolset with the other HTTP-based builtins:
- SSRF protection: Both the agent-card resolver and the JSON-RPC streaming client now use
httpclient.NewSafeClient, closing the bypass that the oldhttpclient.NewHTTPClientleft open. WithDefaultsDisabled(): Properly prevents a2a-go's own default transport from silently re-opening a gRPC/plaintext path that would circumvent the safe client.niltransport guard: Thebase == nil → http.DefaultTransportfallback before wrapping withHeaderTransportis sound —NewSafeClientalways sets a transport, so this path is a belt-and-suspenders guard that does no harm.- Timeout propagation: Default is set in
NewToolset, options are applied after,CreateToolSetwires config values correctly — no ordering bug. - Validation:
allow_private_ipswhitelist extended to includea2ain both the outer type check and the MCP-local-command guard; error messages updated consistently. - Constant consolidation:
DefaultToolHTTPTimeoutcleanly replaces the three independent30 * time.Secondliterals with no change in behaviour. - Tests:
TestToolSetRejectsPrivateIPForAgentCardexercises the SSRF rejection path;TestToolSetStreamingWithAllowPrivateIPsexercises the full happy path with a real in-process a2a server. Coverage is proportional to the risk.
No bugs or security issues found in the changed code.
aheritier
added
area/tools
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
SSRF fix is complete and correct. Both HTTP call sites — agent-card resolution and streaming JSON-RPC — are now routed through httpclient.NewSafeClient, the nil-transport guard is sound, WithDefaultsDisabled() closes the gRPC-fallback bypass, and the DefaultToolHTTPTimeout consolidation has no behavioural change. All CI checks pass.
Three non-blocking items below.
1. Slightly misleading error message in the local-command MCP branch (validate.go ~line 238)
The message "allow_private_ips can only be used with type 'fetch', 'api', 'openapi', 'a2a' or remote MCP toolsets" is only reachable when type: mcp with a local command (no remote.url or ref). Listing 'a2a' as a valid alternative may confuse a user into switching to a2a rather than understanding that their MCP toolset must be remote. A message like "allow_private_ips requires a remote MCP server (remote.url or ref); local MCP commands do not make outbound HTTP calls" would be unambiguous. Nit — not a blocker.
2. No test for WithTimeout behaviour
a2a_test.go covers SSRF rejection and the allow_private_ips happy path, but there is no test that a slow server is cut off when a short timeout is configured (e.g. httptest server with an artificial delay + WithTimeout(1*time.Millisecond)). Given the simplicity of the delegation to NewSafeClient, this is acceptable, but would make the wiring observable in CI.
3. Missing example YAML for new config features
AGENTS.md says: "When adding new features to the config: Create an example YAML that demonstrates the new feature." The PR adds timeout and allow_private_ips support to a2a toolsets but does not include an example YAML. A snippet like:
toolsets:
- type: a2a
url: https://my-internal-agent.example.com
timeout: 60
allow_private_ips: truewould satisfy the convention. Non-blocking, but worth a follow-up.
| } | ||
| if t.AllowPrivateIPsEnabled() && t.Remote.URL == "" && t.Ref == "" { | ||
| return errors.New("allow_private_ips can only be used with type 'fetch', 'api', 'openapi' or remote MCP toolsets") | ||
| return errors.New("allow_private_ips can only be used with type 'fetch', 'api', 'openapi', 'a2a' or remote MCP toolsets") |
There was a problem hiding this comment.
This error is only reachable for type: mcp with a local command (the case "mcp" block, inside t.AllowPrivateIPsEnabled() && t.Remote.URL == "" && t.Ref == ""). Listing 'a2a' in the message can mislead an operator into thinking they should switch to a2a, rather than understanding the real issue: their MCP toolset must use a remote URL or ref.
A more precise message, e.g.:
allow_private_ips requires a remote MCP server (remote.url or ref); local MCP commands do not make outbound HTTP calls
would make the corrective action obvious. Nit — not a blocker.
|
|
||
| func (testA2AHandler) Cancel(context.Context, *a2asrv.RequestContext, eventqueue.Queue) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
No test for WithTimeout behaviour. Consider adding one: spin up a httptest server that sleeps before responding, call NewToolset with WithTimeout(1*time.Millisecond), and assert that Start() returns a deadline-exceeded error. The current tests cover SSRF rejection and the allow_private_ips happy path, but the timeout wiring is unobserved in CI. Non-blocking given the simplicity of the delegation.
3 participants
Aligns the
a2abuiltin toolset with the other HTTP-based builtin toolsets (fetch,api,openapi, remotemcp) so it honours the sharedtimeoutandallow_private_ipsconfiguration fields.Before this change,
a2awas the odd one out. It silently ignoredtimeout:andallow_private_ips:from the config, and it usedhttpclient.NewHTTPClientinstead ofhttpclient.NewSafeClient, meaning an a2aurl:could reach loopback, RFC1918, and link-local addresses (notably the cloud-metadata endpoint at169.254.169.254) — bypassing the SSRF protection applied to every other HTTP-based builtin toolset. There was also no operator knob to override the default timeout for a slow a2a peer.This change adds
WithTimeoutandWithAllowPrivateIPsoptions on the a2a toolset, updatesStartto usehttpclient.NewSafeClient(timeout, allowPrivateIPs)for both the agent-card resolver and the JSON-RPC streaming HTTP client, hoists the duplicated30 * time.Seconddefault intohttpclient.DefaultToolHTTPTimeoutfor a single source of truth, and validatesallow_private_ipsona2awhile passinga2aclient.WithDefaultsDisabled()to avoid SSRF-safe HTTP client bypass via gRPC transport fallback.