[Atom-oh]

Summary

Part 2 of 3, stacked on #183 (fix/datasource-connector-bounds). Merge after #183, then retarget #185 to dev. This PR is agent-core only; the reference doc and guides moved to #185 so they describe code that exists below them.

• The custom MCP transport (agent/pinned_http.py, used by streamable_http_sigv4.py) connects to the address set validated for that invocation, preserving Host, TLS SNI and certificate verification, and rejects every 3xx before the MCP SDK can follow it. IPv6 pins are bracketed, so AAAA-only endpoints connect.
• Connect and read budgets are separate: CONNECT_TIMEOUT = 8 per pinned address, READ_TIMEOUT = 60 for time-to-headers / between SSE events (sse_read_timeout), inside the 120s chat budget. The transport deadline follows the read budget, never the connect budget. Discarded per-address failures are logged so a certificate failure on one address is not hidden behind the last one.
• agent.py SSRF checks return the validated addresses, use the endpoint's port, classify a malformed port as SsrfBlocked, and reject backslashes. api_key header names and values and stored bearer tokens are validated; a pasted trailing newline in a value is stripped, not rejected.
• httpx 0.28.1 / httpcore 1.0.9 are pinned (requirements.txt, Dockerfile) — the versions the SNI extension contract was tested against.
• datasource_http.py's docstring now describes both pinned transports (this PR lands the module the sentence refers to).
• The test TLS server sets a TLS 1.2 minimum (CodeQL py/insecure-protocol).

Takes effect after the next Deploy AgentCore.

Review follow-ups (round 1)

IPv6 breakage → bracketed host; 300s→8s read collapse → separate READ_TIMEOUT; hardcoded 8 → module constants imported by agent.py; parsed.port ValueError → SsrfBlocked; "request-local" → "invocation-local"; discarded errors logged; 3xx message no longer says "redirect" for Location-less responses. The doc contradictions (apiPrefix, per-kind windows, Jaeger limit disclosure, 55s form, oauth_3lo) are addressed by stacking: those docs now live in #185 above the code they describe. The "custom MCP contradicts ADR-007" finding was rejected by the chair and is also settled by #177 (merged).

Tests

• bash scripts/v2/merge-verify.sh on Python 3.12: web vitest PASS, Terraform fmt PASS, deployment Node tests PASS; only the codec-sandbox tests fail for the known environmental reason (same on dev).
• cd agent && pytest test_pinned_http.py test_agent.py test_readiness.py tests/test_health_tools.py tests/test_anthropic_loop.py → 182 passed, with new IPv6, read-budget, factory-timeout, malformed-port and whitespace-strip cases. Needs Python ≥ 3.11 (asyncio.timeout).

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

_Reviewer responses: codex/ALL claude/ALL _

Status: BLOCKED — CRITICAL/MAJOR issues must be resolved before merge

All disputed claims are now verified. The final synthesized review follows.

---

Chair review — PR #184: fix(agent): pin custom MCP connections to the validated address

IMAGE_COVERAGE: NOT_REQUIRED

1. Summary

The core change is a genuine security improvement: it replaces the documented resolve-and-recheck DNS-rebinding window on the custom MCP integration transport with real IP pinning (validated address set, preserved Host/SNI, certificate verification, in-transport redirect rejection), plus a header-injection gate on stored credentials — and it is well-tested, including against a real TLS server. However, the pinning transport breaks IPv6-only endpoints, the new timeout wiring silently collapses the integration read budget from 300s to 8s, and the new docs/reference/external-connections.md (plus the four-locale docs-site rewrite) asserts several behaviors (settings.apiPrefix, per-kind window forwarding, Jaeger limit disclosure, a 55s form display, registration-time oauth_3lo rejection) that do not exist in the resulting code — despite the doc declaring itself "source-supported."

2. Issues per lens

L2 — Code correctness

MAJOR (claude flagged; chair-confirmed) — Bare IPv6 pinned addresses break AAAA-only endpoints. agent/pinned_http.py:28 pins via request.url.copy_with(host=address) where address is the unbracketed string from getaddrinfo (e.g. 2606:4700::1111). In the pinned httpx 0.28.1, encode_host only takes the IPv6 branch for hosts matching ^\[.*\]$; a bare colon-bearing host falls through to the reg-name path (percent-encoding/invalid authority). For an endpoint resolving only to AAAA records, every pin attempt fails, raise last_error fires, and an integration that worked on BASE is dropped. All tests use IPv4 literals, so the path is unexercised. Fix: bracket :-containing addresses before copy_with and add an IPv6 test case.

MAJOR (claude flagged; chair-confirmed) — Integration read budget collapses from 300s to 8s. agent/agent.py now passes sse_read_timeout=INTEGRATION_CONNECT_TIMEOUT (= 8, agent/agent.py:641) at both call sites; the MCP SDK builds httpx.Timeout(timeout, read=sse_read_timeout), so 8s becomes the read timeout for every request on the session, not just SSE. On BASE, read was the SDK default 300s (bounded in practice by the 120s chat budget). Additionally pinned_http.py:20-21 wraps the whole time-to-response-headers in a deadline derived from the connect timeout. Any integration tool whose upstream takes more than 8s to start responding now fails with ReadTimeout — a silent behavior change not mentioned anywhere, including the new reference doc. Keep connect at 8s but give read/SSE a separate, deliberate budget.

MINOR (claude) — agent.py:566 parsed.port raises an uncaught ValueError for a malformed port (https://host:abc/); only socket.gaierror is converted to SsrfBlocked. Isolated by gather_integration_tools, but the error is less precise than BASE.
MINOR (claude) — pinned_http.py hardcodes 8 twice instead of referencing INTEGRATION_CONNECT_TIMEOUT; a future bump in agent.py silently won't propagate.
MINOR (claude) — a control character in the api_key value reports "header configuration is invalid" (misleading cause); a trailing newline in a pasted token now hard-fails the integration — consider .strip() before validating.
MINOR (claude) — the 3xx rejection covers 300–399 including 304, so the "redirect rejected" message can be wrong for Location-less responses. Harmless for MCP POSTs.
MINOR (codex) — agent/test_pinned_http.py is not covered by the repo's required agent gate (python3 -m pytest test_agent.py test_readiness.py -q per root CLAUDE.md), so these transport regressions aren't release-gated. Add it to the required invocation.

L3 — Security / AWS mutation safety

No CRITICAL/MAJOR. The patch adds no AWS-mutating call, IAM statement, or hardcoded secret; the only AWS API touched is the pre-existing read-only GetSecretValue. The pin is strictly stronger than BASE (trust_env=False, follow_redirects=False, in-transport 3xx rejection, verified-cert test), and the auth_headers gate closes a real header-injection path (stored secret setting Host/Content-Length or smuggling CRLF).

Rejected (codex flagged MAJOR) — "Custom external MCP registration contradicts ADR-007 / frozen BYO-MCP." This is a false positive. BYO-MCP is an ADR-004 capability tier explicitly listed in docs/reference/05-agentcore.md:240-242 ("built-in vs custom tiers; per-account Agent Spaces; BYO-MCP"), and the registration/connection machinery (INTEGRATION_TRANSPORTS in web/lib/integration-validation.ts:11, the integrations migration, _connect_integration in BASE agent.py) all predate this PR — this PR only hardens the existing transport. What is FROZEN per ADR-005 is AWS mutation/autonomy (remediation.tf, integrations_write_enabled), which the new doc correctly states is unchanged. No repo rule freezes read-only custom MCP registration.

MINOR (claude) — the comment at agent.py:718 ("the validated address set is request-local") overstates: the pin is resolved once per _connect_integration and serves the whole MCPClient session, which may outlive the DNS TTL. Correct design, wrong label — say "invocation-local."
MINOR (claude) — certificate-verification failures surface as httpx.ConnectError and are silently retried on the next pinned address, with only the last error kept; log discarded errors to preserve the TLS-misconfiguration diagnosis.
MINOR (claude) — the agent's forbidden-header set intentionally omits authorization (unlike datasource_http.py's); one comment would prevent that reading as an omission.
MINOR (claude) — oauth_3lo remains accepted at registration (validation + migration CHECK) and only fails closed at connect time; acceptable defense, but see the L5 doc claim.

L4 — Observability / data-integration correctness

No Steampipe/CIS/diagnosis code is modified; the findings are contradictions between this PR's new documentation and the data-integration code it describes.

MAJOR (claude flagged; chair-confirmed) — the "search tools receive start/end" / "selected bounds are forwarded" contract is false for four of eight kinds. web/app/api/datasources/query/route.ts:102 applies the validated window only if (spec.range) — the comment literally says "otherwise the validated window is ignored (instant)" — and only Prometheus/Mimir/Loki have range tools. Tempo/Jaeger/Dynatrace/Datadog silently fall back to their 1h connector defaults, and maxWindow is a flat 30 days for all non-Loki kinds (route.ts:89), so the doc's per-kind 7-day ceilings are unenforced. Also "Unsupported SQL API ranges are rejected" is wrong: an object range is validated then ignored for ClickHouse, not rejected.

MAJOR (claude flagged; chair-confirmed) — Jaeger limit disclosure does not exist. The doc claims "Jaeger discloses its requested limit and retained count; reaching the requested limit is potentially partial." agent/lambda/jaeger_mcp.py:116 returns only truncated: len(traces) > MAX_TRACES (50); with the default limit=20, a result of exactly 20 traces reports truncated: false with no limit/count fields — i.e. it renders as verified-complete, the exact failure mode the doc says is prevented.

MINOR (claude) — the doc's 4 MiB success / 64 KiB error body caps have no implementation; datasource_http.py reads unbounded with only a 4000-char non-JSON display fallback. (The ok/empty/partial/unknown/error status union and HTTP-206 handling are real.)

L5 — Docs/ADR consistency

MAJOR (claude flagged; chair-confirmed) — settings.apiPrefix does not exist anywhere. Zero matches repo-wide; sanitizeDsSettings (web/lib/datasources.ts:32-45) retains only timeoutS and database, and the Mimir Lambda hardcodes /prometheus/api/v1. The claim ("defaults to /prometheus… empty value selects the root… 128 chars… not appended twice") appears in the new reference doc and all four docs-site locales — an operator following it will silently get the hardcoded prefix.

MAJOR (claude flagged; chair-confirmed) — the 55-second "form shows the effective maximum" claim is false, and the 55s clamp is ClickHouse-only. DatasourceForm.tsx:181-182 shows "Timeout (초, 1–60 · 선택)" / placeholder "기본 10" with no 55s anywhere, and timeoutS rides the connection config only for ClickHouse (datasources.ts:191).

MAJOR (claude flagged; chair-confirmed) — "oauth_3lo … is rejected at registration/enablement" is unsupported. It is an accepted member of INTEGRATION_TRANSPORTS and of the migration's transport CHECK; the only enforcement is the connect-time ValueError in agent.auth_headers. Either add the write-path rejection or restate as "not executable; fails closed at connect."

These doc findings gate because docs/CLAUDE.md requires reference docs to "verify … descriptions against current source" and to "distinguish source-supported capability from actual execution evidence," and the new doc opens by declaring itself the "source-supported contract." Fix is either code or doc text, but the contradiction cannot merge as-is.

MINOR (claude) — agent/lambda/datasource_http.py:27-29 still says the agent's _assert_host_allowed gap is "untouched here … needs its own fix"; this PR is that fix — update the stale cross-reference along with the removed KNOWN LIMITATION block.
MINOR (claude) — the removed comment's reference to web/lib/ssrf-guard.ts as the matching resolve-based tier is gone; one clause clarifying that registration remains resolve-based while connection is now pinned would keep the ADR-011 layering legible.
MINOR (claude) — the retired "deferred to P3 hardening" posture is worth a line in the published decisions page (docs-site/docs/faq/decisions.md); there are no in-tree ADR files to update (private upstream, consistent with repo convention).
Agreement notes: codex found the docs otherwise consistent and found no L2 code issues; its sole MAJOR is rejected above. The two panels did not independently converge on any gating finding — every adopted MAJOR is single-model but was chair-verified against the diff, BASE source, and the pinned httpx version before adoption.

3. Suggestions

1. Bracket IPv6 pinned addresses (f"[{a}]" if ":" in a else a) before copy_with, and add an AAAA test case to test_pinned_http.py.
2. Keep sse_read_timeout on its own constant (or the SDK default bounded by the 120s chat budget) and give pinned_http.py an explicit connect budget sourced from INTEGRATION_CONNECT_TIMEOUT rather than a hardcoded 8 and the request's connect extension.
3. Reconcile docs/reference/external-connections.md and the four docs-site locales with the code: drop or implement apiPrefix, the per-kind window forwarding/ceilings, the Jaeger limit disclosure, the body caps, the 55s form claim, and restate the oauth_3lo posture as connect-time fail-closed.
4. Catch ValueError around parsed.port and raise SsrfBlocked; log discarded per-address errors in the fallback loop; update the stale cross-reference comment in datasource_http.py; add test_pinned_http.py to the required agent test command.

4. Verdict

The security direction is right and the transport tests are strong, but two chair-confirmed functional regressions (IPv6-only endpoint breakage, 8s read-budget collapse) and multiple chair-confirmed contradictions between the PR's own new "source-supported" documentation and the shipped code require a revision cycle.

---

Triggered by commit 55ad447b60bf4a889628b3b6dca6b09c35e9a3a1 · workflow: .github/workflows/pr-review.yml

[Atom-oh]

Status note for reviewers:

• The BLOCKED review above was produced for the pre-fix head 55ad447b. Every item in it was addressed on 8b66c8d5 (IPv6 bracketing, separate 60s read budget, malformed-port classification, per-address failure logging, trailing-whitespace strip, docstring hand-off; the doc contradictions moved to fix(web): show datasource completeness, limits and shared scope #185 where the referenced code exists).
• This PR is stacked on fix(datasources): bound connector reads and verify real query access #183 (fix/datasource-connector-bounds). pr-review.yml and merge-verify.yml only trigger for PRs whose base is main/dev, so no CI runs here until fix(datasources): bound connector reads and verify real query access #183 merges and this PR is retargeted to dev — that retarget is the point at which the AI review will re-run on the current head.
• Rebased on the current fix(datasources): bound connector reads and verify real query access #183 head (1cb5d405, which carries fix: enforce default-off custom MCP governance #186's default-off custom MCP governance). The pin now composes with fix: enforce default-off custom MCP governance #186: validate_custom_mcp_spec and the attested secret version run first, then the validated address set is pinned for that session (_connect_integration). The custom SigV4 transport fix: enforce default-off custom MCP governance #186 removed is not reintroduced.
• Added on this head (cf85a326): translator-embedded IPv4 forms (IPv4-mapped, 6to4, Teredo client, NAT64 64:ff9b::/96 and 64:ff9b:1::/48) are classified by the embedded IPv4, so a NAT64 path cannot launder the metadata/loopback targets through the SSRF check.

Agent suites on this head: 189 passed (test_pinned_http.py test_agent.py test_readiness.py test_custom_mcp_policy.py tests/).