Umbrella: connector, credential and egress hardening (external-implementation comparison) #13623

Review of an external connector-gateway implementation covering the same problem domain (credential custody, provider egress, agent-facing action catalog), compared line-by-line against ours. Full audit in docs/research/connector-credential-egress-audit.md.

Comparing our connector/credential/egress layer against an implementation built for the same job at roughly 100x the provider count surfaced two live bugs, two fail-open guards, and five structural gaps. Every item below cites a file read, not a filename guess.

Two were found by reading the reference implementation's own code comments — that codebase had already hit the bug and recorded why the obvious fix is also wrong. We have the same bug.

Task tree

• security(ssrf): cross-origin redirect in pinned_request_with_redirects replays the caller Authorization header #13624 — security(ssrf): cross-origin redirect replays the caller's credential headers
• security(connectors): six connectors with user-configured hosts bypass the SSRF guard — needs a deployment-gated private-network opt-in #13625 — security(connectors): six connectors bypass the SSRF guard; no private-network opt-in for self-hosted instances
• bug(oauth): a refresh response without expires_in permanently disables token refresh — connector auth dies silently #13626 — bug(oauth): a refresh response without expires_in permanently disables refresh
• bug(oauth): concurrent get_access_token both refresh — rotating-refresh-token providers lose the valid token #13627 — bug(oauth): concurrent get_access_token can lose a rotating refresh token
• security(secrets): connector credential owner check fails open on empty created_by; rotate() decrypts with no audit attribution #13628 — security(secrets): connector owner check fails open on empty created_by; rotate() decrypts without attribution
• feat(connectors): stored credentials carry no account identity — nobody can tell which account an agent acts as #13629 — feat(connectors): credentials carry no account identity
• canonical(oauth): app client secret duplicated into every user credential bundle; OAuthProvider too thin for real providers #13630 — canonical(oauth): app client secret duplicated into every user credential bundle; OAuthProvider too thin for real providers
• feat(connectors): no declared-vs-executable capability flags — unwired features are only found by retrospective audits #13631 — feat(connectors): declared-vs-executable capability flags
• canonical(connectors): two connector families model one concept (knowledge/connectors vs integrations) — owner decision needed #13632 — canonical(connectors): two connector families for one concept — decision needed
• canonical(secrets): integrations/ bypasses the secrets manager entirely — credentials passed as bare values, no grant, audit, revocation or dependency edge #13643 — canonical(secrets): integrations/ bypasses the secrets manager entirely (also Umbrella/PRD: unify AutoBot secrets — one RBAC-governed store, vaults + one key abstraction (replaces 11 stores, 5 keys, 2 UIs) #10088 Task 5)

Sequencing

Security first (children 1-5), then capability (6-8). Child 9 is a decision for the owner and blocks nothing.

#13643 is the platform rule, not a one-off fix: anywhere credentials or connectors are used, they go through the canonical secrets manager — one framework for passing credentials to modules, agents and users under grant, audit and revocation. It belongs to #10088 Task 5 and is cross-filed here because it lands in the same files as #13629/#13630.

Folded into existing work, not filed separately

• Policy decisions carrying their reasoning (the reference returns a decision object naming the layer and matched rule for allows and denials; resolve_forbidden_tools returns a bare frozenset[str]) belongs in security(agents): resolve_forbidden_tools fails open on unknown agent id — two disjoint id namespaces silently unbound an agent #13588/feat(agent-loop): agent cannot ask the user a question mid-task — the only human channel is a yes/no approval #13592 — the agent seam already suffers from having two policy paths, and adding a third mechanism is the same mistake. Noted here so it is not re-filed.
• Reading that layered policy model sharpened security(agents): resolve_forbidden_tools fails open on unknown agent id — two disjoint id namespaces silently unbound an agent #13588: it treats an empty allowlist as "this layer imposes no constraint" by design, whereas resolve_forbidden_tools reaches the same empty set from an unknown agent id. Same value, opposite intent, and no audit trail distinguishes them at runtime.

Deliberately NOT proposed — do not re-file

• Progressive tool discovery / search-then-execute. The reference fronts a ~10,000-action catalog with a handful of search/read/execute tools, and it is the most elegant idea in it. We have ~40 tools (autobot-backend/tools/tool_registry.py:849-894); the whole manifest fits in context. Adopting it would buy nothing and cost an extra LLM round trip per tool use plus a new failure mode (search misses a tool and the agent cannot know what it did not see). Revisit trigger: if services/mcp_bridge_workers starts federating third-party MCP servers and the aggregate tool count crosses a few hundred.
• A provider-catalog generator / mass provider onboarding. That very large provider set ships with no provider-level regression tests in its open repository, by explicit policy. Twelve tested connectors with checkpointing, change detection, and config migration are worth more to a KB-ingesting system than 1,270 untested ones.
• Adopting an external gateway as a runtime dependency. A second gateway process, credential store, and policy model is a net loss for a system that needs roughly a dozen integrations.

Where we are already ahead (context for reviewers)

• DNS-rebind defence is real, not advisory. safe_aiohttp_resolver pins the socket to a pre-verified IP with use_dns_cache=False, and pinned_request_with_redirects builds a fresh pinned connector per hop. The reference cannot do this at all — connection pinning is not expressible over the fetch API it is built on.
• Fail-closed DNS. is_public_url returns False on any resolution failure; the reference deliberately falls through to the transport.
• Auth is a different class of system. The reference is admin token + runtime tokens, single operator, no multi-user RBAC. We have full user management, SSO/SCIM, per-user RBAC, run-JWT scopes, pre-auth throttling, constant-time comparison.