diff --git a/docs/changelog.md b/docs/changelog.md index 2fd260d672..7e0f7bf191 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,38 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## πŸ“‹ docs(planning): SaaS connectors & outbound integration hardening plan (2026-08-11) + +**Repo:** EDDI (`docs/saas-connectors-plan`) + +New [`planning/saas-connectors-plan.md`](../planning/saas-connectors-plan.md) β€” the design record for connecting EDDI agents to third-party SaaS platforms (Jira, Amplitude, Google Drive, …) over REST, MCP, or A2A with real auth, including per-end-user OAuth. Planning only; no code changed. + +**What the investigation established** (all file:line claims spot-checked against source; see the plan's Verified header): +- EDDI has **eleven outbound integration paths** with **five independent credential-resolution implementations**, all static-credential only. There is **zero OAuth client machinery** anywhere β€” no callback, no PKCE, no token exchange, no refresh β€” and **no per-user credential dimension** (`SecretReference` is `(tenantId, keyName)`; `autoVaultSecret` keys per-agent, so two users of one agent overwrite each other). +- Vendor-hosted MCP servers (Atlassian, Amplitude) are unreachable: they require OAuth 2.1, and a 401 from an MCP server just trips the circuit breaker. The MCP client is streamable-HTTP-only; stdio is hard-rejected (though `StdioMcpTransport` ships in the pinned langchain4j-mcp jar). +- Exposure findings that gate the work: `/mcp` (33 tools) is unauthenticated under the shipped `authorization.enabled=false` default and `AuthStartupGuard` effectively never fires (every shipped compose/k8s file sets the escape hatch); two REST endpoints take credentials as query params, one echoing the credential back in its response; tool results enter the LLM verbatim while only tool *descriptions* are sanitized; `SecretScrubber` misses array elements and URL-embedded credentials on export. + +**Plan shape** β€” six delivery phases (0–5) plus a deferred Phase 6: (0) close the exposure gap, (1) govern tool results + MCP/A2A approval pinning, (2) **unify** the five credential paths behind a new `ConnectionConfiguration` resource + `${connection:name}` reference with `SERVICE | PER_USER` binding, (3) stdio via sidecar bridge, (4) OAuth client-credentials + MCP RFC 9728 discovery, (5) per-user authorization-code + PKCE + Manager "Connect" flow; (6) deferred β€” native stdio behind an admin template catalog, A2A peer store, built-in tool credentials, NATS auth. + +**Design decisions worth remembering:** +- Connection resolution deliberately does **not** live in `SecretResolver` β€” that would break the two properties the vault grant-enforcement design depends on (no identity in the resolver; `ChatModelRegistry` caches on unresolved params). Connections are a layer above the vault, not an extension of it. +- The OAuth callback must be a `permit` path (`application-type=service` β€” no bearer on a browser redirect); single-use DB-persisted `state` is its only guard; PKCE mandatory. +- **`PER_USER` connections refuse to deploy when `authorization.enabled=false`** β€” without verified identity, any caller claiming `userId=alice` would resolve Alice's tokens (the `/v1` adapter's `trust-user-headers` precedent shows exactly how). Same guard refuses OAuth connections when the vault is inert (no plaintext fallback for refresh tokens, ever). +- Global `authorization.enabled` default stays untouched (per-surface guard pattern, per house precedent); the SSRF default flip is flagged as needing product sign-off because the `false` default is documented intent, not an oversight. + +**Rebased onto #668**, which landed two things the plan had to absorb: the MCP **resource bridge** (`exposeResources`) is a second untrusted-content surface on the existing MCP connection β€” same credential, so not a new connector path, but resource text is returned verbatim and the server-authored listing metadata is ungoverned, both now folded into G5 and covered by Phase 1.1's hook; and **strict task-level `toolApprovals`**, which closes the adjacent bypass where a task replaced the agent gate wholesale. The plan's "gate fails open on naming" constraint (C11) was re-checked against `ToolApprovalGate.classify` and still holds β€” #668's union semantics for `requireApproval` reinforce it rather than supersede it. + +**Review round (CodeRabbit, #669) β€” 14 findings, all valid, all applied.** Three changed the design rather than the prose: +- **`/secretstore` has the same exposure as `/mcp`** (their only Critical, and correct). `IRestSecretStore` is `@RolesAllowed("eddi-admin")` with **no** path-specific policy, so it falls to the catch-all `authenticated` policy β€” and `DisabledAuthController` disables both under the shipped `authorization.enabled=false`. Vault writes, DEK rotation and `reset` are unauthenticated out of the box. The draft flagged `/mcp` and missed the credential store itself, in a plan about credentials. New Phase 0.8, gating Phase 2. +- **Two TOCTOU races the draft got wrong.** Token refresh relied on a `version` CAS, which is checked at *write* time β€” after both replicas have already called the token endpoint and the provider has already rotated the refresh token. Now: an atomic cross-replica claim *before* the network call, CAS retained only as the final write guard. Same class in the OAuth callback: validate-then-consume let two callbacks both redeem one code; now a single conditional `SET consumedAt … WHERE consumedAt IS NULL`. +- **`tokenUrl` was excluded from allowlisting** while receiving the vault-resolved `clientSecret` β€” a client-secret exfiltration path strictly worse than a misdirected access token. Credential endpoints now validate against their own trusted-provider allowlist, separate from `baseUrlAllowlist`, with one canonical origin format. + +Also applied: `StaticAuth` defined (it was referenced but never specified) with reference-only secret fields; `extraAuthParams` restricted to non-secret protocol parameters; the deprecated GET discovery routes lose the credential parameter entirely rather than rejecting it after it has already passed through proxies and access logs; RFC 9728 discovery constrained (same-origin metadata URL, `resource` match, authorization servers must be pre-approved β€” discovery may select among them, never introduce one); grant-lifecycle routes excluded from operator write scope alongside config writes; the complete `client_credentials` request contract specified; `publicBaseUrl` parsed rather than prefix-matched; the sidecar isolation claim qualified (it bounds blast radius, it does not remove process-execution or supply-chain risk); metric tags reduced to bounded categoricals (author-supplied connection names and server URLs were an unbounded-cardinality and tenant-disclosure vector); and six storage/authorization boundary tests added. + +**Next:** Phase 0 items (McpStartupGuard, `/secretstore` policy, credential-param removal, console redaction, scrubber holes) are independently shippable and gate everything else. Open questions in the plan's Β§13 β€” the group-conversation principal question (Β§13.1) must be answered before Phase 5. + --- ## πŸ“šπŸ”€πŸ›‘οΈ feat(docs+mcp+hitl): docs for agents on every surface, an MCP resource bridge, and strict task-level toolApprovals (2026-08-11) diff --git a/planning/saas-connectors-plan.md b/planning/saas-connectors-plan.md new file mode 100644 index 0000000000..45919d5e7d --- /dev/null +++ b/planning/saas-connectors-plan.md @@ -0,0 +1,1007 @@ +# Implementation Plan β€” SaaS Connectors & Outbound Integration Hardening + +**Status:** Planning +**Created:** 2026-08-11 +**Scope:** Backend (EDDI repo), plus EDDI-Manager surfaces in Phases 0.2 and 5 +**Verified:** 2026-08-11 against `main` @ `00420daa5` (post-#668) β€” Β§2 claims spot-checked against source +(auth defaults, `DisabledAuthController`, both credential-in-query endpoints and +the `apiAuth` echo, verbatim tool-result append, single `governDescription` call +site, per-request `customHeaders` supplier, `SecretScrubber` whole-string and +array-element holes, `LogCaptureFilter` pass-through, empty resolver maps in +`McpToolsProvider`/`A2AToolsProvider`, transport sets, zero OAuth machinery) + +> **Read Β§3 before Β§5.** This plan is greenfield β€” no existing planning doc covers +> outbound OAuth, a per-user credential store, a connection resource type, or MCP +> stdio β€” but it lands in a repo with strong existing opinions about outbound HTTP +> (`SafeHttpClient`), credential storage (`SecretsVault` + deploy-time grant +> enforcement) and new auth surfaces (path-scoped policy + fail-closed startup +> guard, never a change to the global `authorization.enabled` default). Β§3 lists +> those constraints with citations; several of them invalidate the obvious design. + +> **Why hardening and connectors are one plan, not two.** Phases 0 and 1 are not +> adjacent nice-to-haves: they are preconditions. Connectors multiply the blast +> radius of three defects that already exist (unauthenticated `/mcp`, unsanitized +> tool results, credentials in query strings). Storing per-user Google refresh +> tokens behind an admin API that ships unauthenticated is the bad outcome this +> sequencing exists to prevent. Phases 0-1 are independently valuable and can ship +> alone; Phases 2+ must not ship without them. + +--- + +## Table of Contents + +1. [Goal](#1-goal) +2. [Current state β€” honest inventory](#2-current-state--honest-inventory) +3. [Constraints inherited from existing decisions](#3-constraints-inherited-from-existing-decisions) +4. [Design options for the connection model](#4-design-options-for-the-connection-model) +5. [The `ConnectionConfiguration` model](#5-the-connectionconfiguration-model) +6. [OAuth design](#6-oauth-design) +7. [MCP transport work](#7-mcp-transport-work) +8. [Phases](#8-phases) +9. [Files that would change](#9-files-that-would-change) +10. [Metrics](#10-metrics) +11. [Tests and verification](#11-tests-and-verification) +12. [What I would NOT do](#12-what-i-would-not-do) +13. [Open questions](#13-open-questions) + +--- + +## 1. Goal + +Let an agent designer connect an EDDI agent to a third-party SaaS platform β€” +Jira, Amplitude, Google Drive, Notion, Linear β€” over REST **or** MCP **or** A2A, +with the platform's real auth model, including per-end-user OAuth where the +platform requires it, without hardcoding any provider in Java. + +Three concrete acceptance scenarios, in ascending difficulty: + +| Scenario | Auth shape | Today | +|---|---|---| +| **Amplitude analytics** β€” one org-wide API key + secret, shared by all users of the agent | Static, Basic | ⚠️ Works mechanically; unsafe defaults around it | +| **Jira via Atlassian's hosted MCP server** | OAuth 2.1, service account or per-user | ❌ Impossible β€” static Bearer only | +| **Google Drive, each end user seeing only their own files** | OAuth 2.0 authorization code + refresh, per end user | ❌ Impossible β€” no per-user credential dimension exists | + +The unifying requirement is **one credential model across all outbound paths**, +because there are currently five independent ones (Β§2.2) and adding OAuth to each +separately is a five-times problem with five sets of bugs. + +### Non-goals + +Stated here so the phase list can be read without ambiguity; expanded in Β§12. + +- Not a multi-tenancy implementation. This plan consumes + [`multi-tenancy-plan.md`](multi-tenancy-plan.md) Phase 1 when it lands and + defaults to `"default"` until then. +- Not a guardrails implementation. Phase 1 adds the **tool-result** hook that + [`langchain4j-recommendations.md:250`](langchain4j-recommendations.md#L250) + identifies as the acknowledged gap in R0, and stops there. +- Not 250 provider integrations. Β§4 Option D keeps the long tail delegable. + +--- + +## 2. Current state β€” honest inventory + +### 2.1 What already works + +- **`SecretsVault`** β€” AES-256-GCM envelope encryption, per-tenant DEK wrapped by + a PBKDF2-derived KEK (`crypto/EnvelopeCrypto.java`, `crypto/VaultSaltManager.java`, + `impl/VaultSecretProvider.java:137`, `:178`), Mongo and Postgres persistence keyed + `(tenantId, keyName)`, write-only REST under `@RolesAllowed("eddi-admin")` + (`rest/IRestSecretStore.java:27-29`). `${vault:key}` / `${vault:tenant/key}` + parsing in `model/SecretReference.java:42`, `:54`; Caffeine-cached resolution in + `SecretResolver.java:145-207`. **This is the right foundation and this plan builds on + it rather than beside it.** +- **`VaultGrantChecker`** β€” `allowedAgents` enforced at deployment by serializing + the agent + workflow-extension configs and scanning for `${vault:...}` + (`:122-155`, `:166-196`, mcpcalls at `:222-224`), defaulting to `enforce` as of + 2026-08-11. +- **`${caller:token}`** β€” `engine/security/CallerIdentityResolver.java`, headers only, + same-origin only (`:263-279`), fails closed on unauthenticated turns. +- **`SafeHttpClient`** β€” `Redirect.NEVER` (`:65`), per-hop `validateUrl` (`:237`), + `Authorization`/`Cookie` stripped on cross-origin redirect (`:55`, `:197`). +- **HITL tool approvals** β€” gate covers all seven tool sources including `mcp` and + `a2a`; fail-closed refusal for rule-triggered MCP calls (`McpCallsTask.java:237-246`, + `:399-409`). +- **OpenAPI β†’ tool generation** β€” `engine/mcp/McpApiToolBuilder.java` turns a spec + URL into httpcall configs. Under-advertised; it is the shortest path to a working + Jira or Amplitude integration today. + +### 2.2 The eleven outbound paths + +Every way EDDI currently reaches something external, with its auth model. The +column that matters is **Vault** and **Per-user** β€” the fragmentation this plan exists +to collapse. + +| # | Path | Primary class | Auth | Vault | Per-user | `SafeHttpClient` | +|---|---|---|---|---|---|---| +| 1 | httpcalls (REST) | `modules/apicalls/impl/ApiCallExecutor.java:603-682` | any header / param | βœ… | βœ… `${caller:}` same-origin | n/a (Vert.x) | +| 2 | OpenAPI auto-discovery | `engine/mcp/McpApiToolBuilder.java:250-251` | `apiAuth` header string | βœ… | ❌ | ❌ | +| 3 | MCP client β€” tools, plus **resources** via `exposeResources` (#668) | `modules/llm/impl/McpToolProviderManager.java:617-653`, `:475` | single `apiKey` β†’ Bearer | βœ… | βœ… same-origin | ❌ (lc4j transport) | +| 4 | A2A client | `modules/llm/impl/A2AToolProviderManager.java:245-331` | static Bearer | βœ… | ❌ | ❌ | +| 5 | Vector stores (5 types) | `modules/llm/impl/EmbeddingStoreFactory.java:181` | per-store | βœ… | ❌ | n/a | +| 6 | Embedding models (8) | `modules/llm/impl/EmbeddingModelFactory.java:97` | provider key | βœ… | ❌ | n/a | +| 7 | LLM providers (11) | `modules/llm/impl/ChatModelRegistry.java:217-218` | provider key | βœ… | ❌ | n/a | +| 8 | Built-in tools (4 networked) | `modules/llm/tools/impl/` | MicroProfile config | ❌ | ❌ | βœ… (2 of 4 also `validateUrl`) | +| 9 | Slack outbound | `integrations/slack/SlackWebApiClient.java:172` | workspace bot token | βœ… | ❌ | ❌ | +| 10 | Agent Sync (EDDI↔EDDI) | `backup/impl/RemoteApiResourceSource.java:415-417` | `X-Source-Authorization` relay | n/a | relay only | ❌ | +| 11 | NATS event bus | `engine/runtime/internal/NatsConversationCoordinator.java:165-183` | **none** | ❌ | ❌ | n/a | + +**Five distinct credential-resolution implementations** (rows 1, 3, 4, 5-7, 9), +each with its own `apiKey`-shaped field and its own +`globalVariableResolver β†’ secretResolver` snippet. Row 8 is the only outbound +credential class in EDDI that never touches the vault. + +> [!NOTE] +> Row 3 gained a second *content* surface in #668: `exposeResources` synthesizes +> `_list_resources` / `_read_resource` tools, so an MCP server's +> **resources** β€” not just its tools β€” reach the agent. Same connection, same +> credential, so it is not a twelfth path and needs no separate connector work. +> It does widen G5: resource text is returned **verbatim** (capped at +> `RESOURCE_CONTENT_MAX_CHARS`), and the server-authored resource *names and +> descriptions* rendered by `renderResourceList` are ungoverned third-party text +> on the same channel. Phase 1.1's hook covers both automatically (Β§8.1). + +Inbound, for completeness: EDDI's own MCP server (33 tools), the OpenAI-compatible +`/v1` adapter, and the A2A server endpoint. For some platforms the cheaper +integration is inverting the direction β€” letting the vendor's agent call EDDI β€” +and that needs no work here beyond Phase 0. + +### 2.3 What does NOT exist + +| Gap | Detail | +|---|---| +| **G1 β€” No OAuth client, anywhere** | Repo-wide search for `authorization_code`, `grant_type`, `refresh_token`, `redirect_uri`, `token_endpoint` across `.java`/`.md`/`.properties` returns three hits: two `grant_type=password` calls against Keycloak in `install.sh:847` / `gcp/provision-vm.sh:562`, and a sample URL in `keycloak-eddi-theme.md:414`. In `src/main/java` the only hit is the redaction denylist `SecretScrubber.java:66`. No callback resource, no PKCE, no token exchange, no refresh storage, no `state` store. | +| **G2 β€” No per-user credential dimension** | `SecretReference` is `(tenantId, keyName)`. No `userId` in `SecretMetadata` or either persistence impl. The closest thing, `PropertySetterTask.autoVaultSecret()` (`:446-449`), keys on `agentId + "." + keyName` β€” two end users of the same agent **overwrite each other**. | +| **G3 β€” MCP 401 is not an auth challenge** | `McpToolProviderManager.java:362-367` treats it as a discovery failure and trips the circuit breaker (3 failures / 60s, `:112-114`). No `WWW-Authenticate` parsing, no `/.well-known/oauth-protected-resource` discovery, no dynamic client registration. This is the exact hook OAuth needs. | +| **G4 β€” MCP transport is HTTP-only** | `stdio` hard-rejected at `:555-565`; `sse` survives as a deprecated alias (`:487`, `:576-582`) but is served over StreamableHTTP anyway. `McpCallsConfiguration.SUPPORTED_TRANSPORTS` (`:80`) omits `sse`, so the REST write path 400s a config the runtime would accept β€” a live inconsistency. Most vendor-shipped MCP servers are stdio binaries. | +| **G5 β€” Tool results enter the LLM verbatim** | `ToolLoopRunner.java:460-465`, comment: *"append the raw result verbatim"*. `governDescription` / `DIRECTIVE_PATTERN` (`McpToolProviderManager.java:590-612`, `:101-108`) is applied at exactly one call site (`:331`) and only to MCP tool **descriptions**. A2A descriptions, which come from a remote agent card, are ungoverned (`A2AToolProviderManager.java:152-175`), as is MCP **resource** content and its listing metadata (`:475+`, #668). | +| **G6 β€” Credentials in query strings** | Two endpoints: `GET /mcpcallsstore/mcpcalls/discover-tools?apiKey=` (`IRestMcpCallsStore.java:91-94`) and `GET /apicallsstore/apicalls/discover-endpoints?apiAuth=` (`IRestApiCallsStore.java:91-94`). The second takes a full `Authorization` header value **and echoes it back** in every generated ApiCall's headers (`RestApiCallsStore.java:97`). `IRestImportService.java:65` already does this correctly with `@HeaderParam` β€” the in-repo pattern to copy. | +| **G7 β€” Authorization is off by default** | `engine/security/DisabledAuthController.java:17-24` is an `@Alternative` `AuthorizationController` returning `authorization.enabled`, which defaults to `${quarkus.oidc.tenant-enabled}` = `false` (`application.properties:318`, `:335`). When false, Quarkus disables path policies **and** `@RolesAllowed`. `/mcp` has no permit entry, so it falls to the catch-all and is wide open β€” stated outright in the banner at `application.properties:429-446`. `AuthStartupGuard` exempts dev/test (`:54`) and every shipped `docker-compose*.yml` / k8s manifest sets `EDDI_SECURITY_ALLOW_UNAUTHENTICATED=true`, so **the guard effectively never fires**. | +| **G8 β€” SSRF protection defaults off** | `application.properties:344` and `defaultValue="false"` at `ApiCallExecutor.java:96`, `A2AToolProviderManager.java:70`, `McpToolProviderManager.java:143`. With it off, MCP URL validation degrades to a scheme check (`:533-547`), and `discover-tools` echoes the response β€” a read primitive against `169.254.169.254`, described in that class's own Javadoc at `:496-507`. | +| **G9 β€” Console logs unredacted** | `LogCaptureFilter.java:65-76` always returns `true`; redaction happens on a **copy** inside `BoundedLogStore.capture()` (`:149`). Ring buffer, DB and SSE are clean; container stdout is not. Unredacted throwables at `McpToolProviderManager.java:363`, `HttpClientWrapper.java:303`, `:316`; raw exception text returned to the HTTP caller at `RestMcpCallsStore.java:161`. | +| **G10 β€” MCP/A2A approvals cannot be pinned** | `ToolApprovalGateSupport.pinResolvedRequest` (`:284`, `:315`) fires only for tools with a `ToolRequestResolver`. `HttpCallToolsProvider.java:205` supplies resolvers; `McpToolsProvider.java:80` passes `Map.of()` and `A2AToolsProvider.java:51` passes none. A gated MCP call shows the approver a tool name and `argumentsRedacted` β€” no URL, no request fingerprint. | +| **G11 β€” Export scrubbing has holes** | `SecretScrubber` catches `apikey` / `authorization` by field name, so mcpcalls and httpcalls Authorization headers are scrubbed. Not caught: a header named `X-Api-Token` (normalizes to `xapitoken`, falls to the entropy heuristic, and `KEY_LIKE_PATTERN` at `:53` requires a whole-string match so `Bearer abc…` with its space never matches); URL-embedded credentials (`?api_key=`, `https://user:pass@host` β€” the `?`/`=`/`:` defeat the same pattern); and **any string inside an array** (`scrubNode:134` recurses with the parent field name into a node that is neither object nor array, and does nothing). Separately, `ChannelIntegrationConfiguration` is **not exported at all**, so `botToken` never reaches a ZIP β€” but channel integrations are also silently missing from backups. | +| **G12 β€” Stale credential caches** | `ChatModelRegistry` (`:152-167`), `EmbeddingModelFactory` (`:74-77`) and `EmbeddingStoreFactory` (`:146-147`) register `SecretResolver` invalidation listeners. `McpToolProviderManager` does not β€” its apiKey is resolved once per cached client (`:626-628`), so a rotated vault secret is not picked up until eviction. `ChannelTargetRouter` polls at 60s (`:55`). `A2AToolProviderManager` re-resolves per call but caches agent cards by URL for 5 min. | + +--- + +## 3. Constraints inherited from existing decisions + +Load-bearing. Several invalidate the obvious design; each is cited. + +**C1 β€” Never change the global `authorization.enabled` default.** The established +pattern for a new or under-protected surface is a path-scoped +`quarkus.http.auth.permission.*` policy plus a startup guard that fails closed, +per [`openai-api-adapter-plan.md:465-471`](openai-api-adapter-plan.md#L465) and +`:592-595` (*"`enabled=false` by default, and startup fails on the +unauthenticated-and-keyless combination"*). `mcp-hitl-surface-plan.md:15` sets the +matching expectation for MCP: *"Mirror REST auth exactly… When +`authorization.enabled=false` (dev default), `OwnershipValidator` no-ops β€” +identical to REST today."* Flipping the global would break every shipped compose +file and k8s manifest at once. + +**C2 β€” All outbound HTTP goes through `SafeHttpClient`.** +[`security-hardening-remaining.md:141`](security-hardening-remaining.md#L141): +*"ALL outbound HTTP from LLM tools. Never create `HttpClient.newBuilder()` in tool +code."* `:104-107` already lists four services that bypass it +(`A2AToolProviderManager`, `SlackWebApiClient`, `RemoteApiResourceSource`, +`HttpCallsTask`). A connector adding a fifth is a regression, not neutral. DNS +rebinding remains an accepted documented risk (`:94`). + +**C3 β€” Connection resolution must not live inside `SecretResolver`.** The 2026-08-10 +changelog decision placed `allowedAgents` enforcement at deployment *specifically +because* `SecretResolver` sees only a string with no agent identity, and +`ChatModelRegistry` caches built models keyed on **unresolved** parameters. A +per-user, per-request, network-calling resolver inside that class would break both +properties. + +**C4 β€” Only a vault *reference* is ever inherited, never a plaintext key** +(changelog, `fix/sub-agent-setup-hardening`). Applies verbatim to connections: +configs carry `${connection:name}`, never a token. + +**C5 β€” Approve the fingerprint, not the tool name.** +[`operator-write-scope-plan.md:120`](operator-write-scope-plan.md#L120): +*"Closing that properly needs a server-side redacted preview plus an immutable +fingerprint of the resolved request, re-checked immediately before execution."* +This is why G10 is a Phase 1 item and not a nicety. + +**C6 β€” `/secretstore` writes are excluded from operator write scope** +([`operator-write-scope-plan.md:190`](operator-write-scope-plan.md#L190)). +Connection writes are the same class of capability β€” an egress channel plus a +credential β€” and must be excluded identically. Compare `:97`, which excludes +`PUT /llmstore/llms` because *"an LLM config carries a provider `baseUrl`… an +update is an egress channel plus a prompt rewrite."* + +**C7 β€” Guardrails: never throw out of `GuardrailService`** +([`langchain4j-recommendations.md:2417`](langchain4j-recommendations.md#L2417)); a +terminal `BLOCK` is a returned value, because a thrown message quoting user text +collides with `AgentExecutionHelper.isRetryableError`. And the fix location for +`tool_call`/`tool_result` redaction is **the producer** β€” the `AgentOrchestrator` +maps β€” per `:1082`. + +**C8 β€” Guardrail design ownership.** `langchain4j-recommendations.md:2290` +supersedes parts of `guardrails-architecture.md`: adopt lc4j core types, do **not** +create `IGuardrail` or a `Verdict` enum with a `PASS` member; persisted actions stay +EDDI-owned and neutral (`block | redact | reprompt | warn`, `:261`). + +**C9 β€” Tenant scoping follows the existing plan, not a bespoke one.** +[`multi-tenancy-plan.md`](multi-tenancy-plan.md) is Planning status with only +Phase 5 landed. Its Phase 1 design is a `@RequestScoped TenantContext` + +`TenantResolverFilter` that **fails closed with 403** on a missing claim (`:213`), +consumed by `@ApplicationScoped` stores via `Provider` (`:306-311`), +and inside the pipeline read from a `ConversationProperty` rather than the request +scope (`:381`). New stores add a `tenantId` field defaulting to `"default"` and a +compound `(tenantId, _id)` index (`:257`). + +**C10 β€” Golden Rule 1.** Provider behavior is JSON configuration; Java is the +engine (`AGENTS.md:181`). No provider may be hardcoded. Conversely, +`mcp-hitl-surface-plan.md:40` establishes that a full store/descriptor/REST stack +needs explicit justification β€” Β§4 provides it. + +**C11 β€” The gate fails open on naming.** `ToolApprovalGate.classify` returns the +whole batch as allowed when `requireApproval` is null or empty +(`ToolApprovalGate.java:57-59`), and unmatched names fall through +(`operator-write-scope-plan.md:23`). Any connector-related approval default +must be expressed as a wildcard plus an explicit exempt list, never an enumerated +write list. Still true after #668 β€” that change closed the *adjacent* bypass where +a task-level `toolApprovals` replaced the agent gate wholesale, and its merge +semantics reinforce this rule: `requireApproval` unions (so `[]` can no longer +weaken anything) while a task's `exempt` additions are ignored outright +(`TaskToolApprovalsResolver`, `eddi.hitl.tool.task-approvals.mode=strict`). + +--- + +## 4. Design options for the connection model + +### Option A β€” Add OAuth fields to each existing config type + +`McpCallsConfiguration`, `Request` headers, `A2AAgentConfig`, the embedding/vector +param maps, and `ChannelIntegrationConfiguration.platformConfig` each grow +`oauth` blocks and each resolution site grows a token-refresh call. + +> **Not recommended.** Five implementations, five caches, five refresh +> concurrency bugs. Directly contradicts `AGENTS.md Β§4.7` ("unification over +> duplication… prefer unified systems with legacy compat methods over dual +> storage"). It also puts an HTTP-calling refresh path inside +> `ChatModelRegistry`'s build-time resolution, which violates C3's caching +> assumption. + +### Option B β€” A `ConnectionConfiguration` resource plus a `${connection:}` reference βœ… RECOMMENDED + +One new resource type describing *how to authenticate to a system*. Every existing +config references it by name; a new `ConnectionResolver` sits alongside +`SecretResolver` in each of the five resolution chains and returns a **per-request** +credential. + +> **Recommended.** It is one implementation of OAuth, one grant store, one refresh +> path, one audit surface. It preserves Golden Rule 1 (the connection is JSON +> config, the engine is generic). It reuses `EnvelopeCrypto` and the existing +> tenant DEK rather than inventing storage. The cost is a full store/REST/descriptor +> stack β€” justified by C10 because a connection is genuinely a new first-class +> config document with its own lifecycle, not a field on an existing one. + +### Option C β€” Extend `SecretsVault` with a self-refreshing "dynamic secret" type + +`${vault:google-drive}` transparently returns a fresh access token. + +> **Not recommended, and blocked by C3.** `SecretResolver` deliberately has no +> agent or user identity, and `ChatModelRegistry` caches on unresolved parameters β€” +> both are load-bearing properties of the 2026-08-10 grant-enforcement design. +> Smuggling an HTTP client and a per-user dimension into the vault would break the +> reasoning that made deploy-time enforcement sound. The vault stays a static +> secret store; connections are a layer above it that *uses* it for client secrets. + +### Option D β€” Delegate to an external managed-auth layer + +Point EDDI's existing MCP client at a managed integration platform (Nango, +Composio, Zapier MCP) that holds per-end-user tokens for a few hundred providers. + +> **Not rejected β€” complementary, and the escape hatch for the long tail.** It +> requires zero EDDI code beyond Phase 0-1 and Phase 4 (Β§7), and delivers per-user +> Google Drive sooner than Phase 5 will. It costs a dependency and a third party in +> the credential path, which is disqualifying for some deployments and fine for +> others. **Decision: build Option B for first-party providers and EDDI's own +> control, and document Option D as the supported path for everything else.** This +> is an open question only in how many providers Phase 5 ships natively (Β§13.2). + +--- + +## 5. The `ConnectionConfiguration` model + +### 5.1 Resource + +New store `connectionstore/connections`, following the standard stack +(`AGENTS.md Β§4.3`): configuration record, `IResourceStore`, Mongo + Postgres +stores, JAX-RS interface + impl, `ExtensionDescriptor`, unit tests. Versioned like +every other resource. + +```java +public record ConnectionConfiguration( + String name, // referenced as ${connection:name} + String tenantId, // defaults "default" until multi-tenancy Phase 1 (C9) + String description, + AuthType authType, // STATIC | BASIC | OAUTH2_CLIENT_CREDENTIALS | OAUTH2_AUTHORIZATION_CODE + Binding binding, // SERVICE | PER_USER + StaticAuth staticAuth, // authType STATIC/BASIC + OAuthConfig oauth, // authType OAUTH2_* + List baseUrlAllowlist, // origins this connection's credential may be sent to + Integer timeoutMs) { } + +public record StaticAuth( + String headerName, // e.g. "Authorization", "X-Api-Key" β€” non-secret + String valueTemplate, // e.g. "Bearer ${vault:jira-token}" β€” see rule below + String username, // BASIC only; non-secret + String passwordRef) { } // BASIC only; MUST be a ${vault:...} reference + +``` + +`binding` is the field that makes Amplitude and Google Drive the same system: +`SERVICE` resolves one grant shared by every user of the agent; `PER_USER` +resolves the calling user's grant and fails closed if absent. It is a config +choice, never a global mode. + +```java +public record OAuthConfig( + String authorizationUrl, // null for CLIENT_CREDENTIALS + String tokenUrl, + String clientId, + String clientSecret, // MUST be a ${vault:...} reference (C4) + List scopes, + Map extraAuthParams, // NON-SECRET protocol params only (prompt, audience, …) + boolean usePkce, // forced true for AUTHORIZATION_CODE + String discoveryUrl) { } // optional RFC 8414 / 9728 metadata URL +``` + +**Every secret-bearing field is reference-only.** `validate()` rejects any value +in `clientSecret`, `passwordRef`, or a `valueTemplate`'s interpolated segment +that is not a `${vault:...}` (or `${vars:...}`) reference, and rejects +`extraAuthParams` keys matching the credential-shaped denylist +(`SecretScrubber.SECRET_FIELD_NAMES`). Without this, a plaintext key in +`StaticAuth` or `extraAuthParams` would sit in the connection document, bypassing +C4, export scrubbing, and `VaultGrantChecker`'s `${vault:}` scan in one move β€” +`extraAuthParams` in particular is an arbitrary string map and would otherwise be +the obvious place for an author to paste one. + +**Canonical origin format, applied to two separate allowlists.** Every allowlist +entry is a bare origin β€” `scheme://host[:port]`, lowercase scheme and host, no +path, query, fragment, userinfo, or trailing slash β€” parsed with `URI.create` and +re-serialized, never string-compared. `validate()` rejects anything else, so +`api.atlassian.com` (no scheme) fails loudly rather than silently never matching. + +- **`baseUrlAllowlist`** governs where the *access token* may be sent. It is a + list because one provider's credential legitimately spans hosts (Google: + `https://www.googleapis.com`, `https://drive.googleapis.com`, …). +- **Credential endpoints** β€” `tokenUrl`, `authorizationUrl`, `discoveryUrl` β€” are + validated **separately**, against their own trusted-provider allowlist. An + earlier draft of this plan said the token URL "does not need to be in the + allowlist at all"; that was wrong. The vault-resolved `clientSecret` is sent to + `tokenUrl`, so an unvalidated `tokenUrl` is a direct client-secret exfiltration + path β€” a strictly worse leak than a misdirected access token. Their origins + routinely differ from the API's (Atlassian: `auth.atlassian.com` vs + `api.atlassian.com`), which is why they need their own list rather than being + folded into `baseUrlAllowlist`. + +`validate()` also rejects: blank `name`; `AUTHORIZATION_CODE` without +`authorizationUrl`; `PER_USER` binding on any `authType` other than +`AUTHORIZATION_CODE`. + +### 5.2 The grant store + +Separate collection/table from the config, because grants are runtime state with a +different lifecycle, different access control, and must never appear in an export. + +``` +connection_grants + tenantId string ─┐ + connectionId string β”œβ”€ unique compound index + principal string β”€β”˜ userId, or the sentinel "__service__" + accessToken bytes EnvelopeCrypto, tenant DEK + refreshToken bytes EnvelopeCrypto, tenant DEK + expiresAt instant + scopes list + status enum ACTIVE | EXPIRED | REVOKED | REFRESH_FAILED + createdAt / updatedAt / lastRefreshAt + version long optimistic lock β€” see 5.4 +``` + +Reuses `EnvelopeCrypto` and the per-tenant DEK; no new crypto. Never exported +(Β§9), never returned in plaintext by any REST endpoint, revocable by deleting the +row. + +### 5.3 The `${connection:name}` reference + +Parsed by a new `ConnectionReference` mirroring `SecretReference`'s shape. +**Critically, it is not a string substitution like `${vault:}`** β€” it resolves to a +credential bound to the *current caller*, per request. + +That distinction drives one integration decision per path: + +| Path | How it resolves per request | Work | +|---|---|---| +| httpcalls | `ApiCallExecutor.buildRequest` already resolves per request (`:603-682`); add a fourth resolution step after `secretResolver`, headers-only, same restriction set as `${caller:}` | Small | +| MCP client | The client is cached, but `customHeaders` is already a **per-request supplier lambda** (`McpToolProviderManager.java:649`) β€” the exact mechanism `${caller:token}` uses. Reuse it verbatim. | Small | +| A2A client | Re-resolves per call at `:203` / `:272` | Small | +| Vector / embedding / LLM | Cached on unresolved params by design (C3). **`SERVICE` binding only** β€” reject `PER_USER` at validate time. Register a `SecretResolver`-style invalidation listener. | Small | +| Slack | 60s poll refresh; `SERVICE` only | Small | + +`ConnectionResolver` is `@ApplicationScoped`, injects `Provider` +(C9) and reads the principal from `CallerIdentityContext` where available. In the +pipeline, where request scope may be absent, the principal comes from conversation +memory β€” the same rule `multi-tenancy-plan.md:381` sets for `tenantId`. + +**Fail-closed rules**, mirroring `CallerIdentityResolver`: + +- On the HTTP-templating paths (httpcalls, MCP, A2A), `${connection:}` may appear + **only in headers** β€” in a URL, body, or query param it is rejected at validate + time, same restriction set as `${caller:}`. On the non-templating paths + (LLM / embedding / vector-store parameter maps) there are no headers; there the + reference may appear only in the field the builder designates as the credential, + and only with `SERVICE` binding. +- **`PER_USER` requires a *verified* principal, not merely a present one.** The + principal must come from an authenticated OIDC identity + (`CallerIdentityContext` backed by a validated token) β€” never from a + client-supplied header. This is not hypothetical: `OpenAiAuthFilter.java:35-39` + documents that `X-OpenWebUI-User-Id` is believed verbatim, and with + `authorization.enabled=false` (the shipped default) there is no verified + identity anywhere in the system. Without this rule, anyone claiming + `userId=alice` resolves Alice's Google token. Enforced twice: at grant-store + read time in `ConnectionResolver`, and at startup by the guard (Β§6.2 + Decision 4). +- `PER_USER` with no resolvable principal (scheduled turn, trigger, retry on a + callback thread) β†’ `ConnectionException`, never a silent fallback to the service + grant. This matches `CallerIdentityResolver.java:713-721`'s existing choice to + send nothing rather than the wrong authority. +- Target origin not in `baseUrlAllowlist` β†’ rejected. This is the analogue of + the same-origin rule that makes `${caller:token}` safe, generalised: a connection + names the origins its credential may reach, so a config edit cannot redirect a + Google token to an attacker's host. + +### 5.4 Refresh concurrency + +The production-scale concern `AGENTS.md Β§4.7` demands be answered up front. Two +conversations hitting an expired grant simultaneously both call the token endpoint; +with rotating refresh tokens (Google, Atlassian) the second call invalidates the +first, and the user is silently logged out. + +**Design: a cross-replica claim taken *before* the token call, with optimistic +locking as the final write guard.** Ordering is the whole point β€” an earlier +draft of this plan relied on a `version` CAS alone, which is checked at *write* +time, i.e. after both replicas have already called the token endpoint and the +provider has already rotated the refresh token out from under one of them. The +CAS would then dutifully serialize two writes, one of which carries a token the +provider has already invalidated. Correct order: + +1. **Claim** β€” atomic conditional update on the grant row + (`refreshInProgress IS NULL OR refreshLeaseExpiresAt < now`), setting a lease + with a short TTL. This is the cross-replica gate and it happens *before* any + network call. +2. **Claimant refreshes**; non-claimants poll the row for the new token until the + lease expires, then retry the claim rather than refreshing blind. +3. **Write** guarded by the `version` CAS, and clear the lease. + +The lease TTL must exceed the token endpoint timeout, or a slow provider frees +the lease while the claimant is still in flight and reintroduces the double +refresh. In-process, a `ConcurrentHashMap>` +still collapses concurrent callers before they contend on the row β€” an +optimization on top of the claim, not a substitute for it. Failure semantics distinguish the OAuth error from +the transport error: `invalid_grant` (revoked/expired refresh token) is terminal β†’ +`REFRESH_FAILED`, surfaced as "reconnect required"; a network timeout or 5xx from +the token endpoint is **not** terminal β€” the grant stays `ACTIVE`/`EXPIRED` and the +next request retries. Conflating the two logs users out on every provider blip. + +A proactive sweeper is explicitly **not** in Phase 5 β€” lazy refresh with +single-flight is correct and simpler, and `ScheduleFireExecutor` is available later +if telemetry shows cold-start latency matters. + +--- + +## 6. OAuth design + +### 6.1 Service account β€” `client_credentials` (Phase 4) + +The request contract, spelled out because a plan meant to be implemented cold +should not leave it to inference (RFC 6749 Β§3.2.1, Β§4.4.2): + +- `POST tokenUrl`, body `application/x-www-form-urlencoded`, containing + `grant_type=client_credentials` and `scope` (space-delimited). +- **Client authentication defaults to HTTP Basic** (`client_id:client_secret`, + base64) β€” the method RFC 6749 Β§2.3.1 says servers MUST support β€” with + `client_secret_post` (credentials in the form body) as a per-connection + opt-in, since some providers only accept that. The choice is a + `clientAuthMethod` field on `OAuthConfig`, not a guess at runtime. +- Response parsed for `access_token`, `token_type`, `expires_in`; a missing + `expires_in` is treated as a short fixed TTL rather than "never expires". +- Sent via `SafeHttpClient.sendValidated` (C2), to a `tokenUrl` already validated + against the credential-endpoint allowlist (Β§5.1). + +Cache the access token against `expiresAt` in the grant store under principal +`__service__`, refresh lazily. No callback, no browser, no user +interaction. This is what Atlassian and Amplitude service integrations need, and it +is the phase that proves the whole `ConnectionConfiguration` machinery end-to-end +with a fraction of Phase 5's surface. + +### 6.2 Per-user β€” authorization code + PKCE (Phase 5) + +The flow, with the three decisions that matter: + +``` +1. User clicks "Connect" in Manager + β†’ POST /connectionstore/connections/{id}/authorize (authenticated, user's own token) + β†’ server generates code_verifier + state, persists {tenantId, connectionId, + principal, nonce, codeVerifier, expiresAt} single-use, TTL 10 min β€” + persisted in the DB, NOT in-memory: behind a load balancer the callback + routinely lands on a different replica than the one that issued the state + β†’ returns the provider authorization URL + +2. Browser β†’ provider β†’ user consents β†’ provider redirects to + GET /connections/callback?code=…&state=… (PERMIT path β€” see below) + +3. Server ATOMICALLY CLAIMS the state row β€” conditional update + `SET consumedAt = now WHERE state = ? AND consumedAt IS NULL AND expiresAt > now`, + proceeding only if it updated exactly one row β€” then exchanges code + + code_verifier at tokenUrl, encrypts and stores the grant under `principal`, + redirects to a Manager success page + +4. Agent turn β†’ ConnectionResolver β†’ grant for (tenant, connection, callerUserId) + β†’ access token, refreshed if expired +``` + +**Decision 1 β€” the callback must be a `permit` path, and `state` is its only +guard.** The redirect arrives from the provider through the user's browser as a +top-level GET carrying no bearer token (`quarkus.oidc.application-type=service`, +`application.properties:314-326` β€” the comment at `:328` states the same +constraint for Keycloak: *"service mode returns 401 (no redirect)"*). So the +callback cannot be `@Authenticated`. It is secured by a single-use, server-stored, +HMAC-signed, short-TTL `state` that binds the tenant, connection and principal β€” +the callback never trusts a request parameter for identity. New entry in +`application.properties`: + +```properties +quarkus.http.auth.permission.oauth-callback.paths=/connections/callback +quarkus.http.auth.permission.oauth-callback.policy=permit +quarkus.http.auth.permission.oauth-callback.methods=GET +``` + +The claim in step 3 must be the *first* thing the callback does, and it must be a +single conditional write rather than a read-then-write: validating the row and +then marking it consumed lets two concurrent callbacks both observe it unconsumed +and both redeem the code. + +**Decision 2 β€” PKCE is mandatory, not configurable.** `usePkce` is forced true for +`AUTHORIZATION_CODE` at validate time. A public redirect endpoint without PKCE is +an authorization-code interception vector, and there is no legacy provider in scope +that lacks S256 support. + +**Decision 3 β€” a fail-closed startup guard, per C1.** `redirect_uri` must be an +exact match at the provider, so EDDI must know its own public base URL. Following +the `OpenAiStartupGuard` precedent: + +```java +// ConnectionStartupGuard +if (connectionsEnabled && publicBaseUrl.isBlank()) { + throw new IllegalStateException( + "eddi.connections.enabled=true requires eddi.connections.public-base-url"); +} +// Parse, do not prefix-match: startsWith("https://") accepts paths, queries, +// fragments, userinfo and malformed authorities, any of which yields a +// redirect_uri that will not match the provider's exact registration. +URI base = URI.create(publicBaseUrl); // throws on malformed +if (connectionsEnabled && !launchMode.isDevOrTest() + && (!"https".equals(base.getScheme()) || base.getUserInfo() != null + || base.getQuery() != null || base.getFragment() != null + || (base.getPath() != null && !base.getPath().isEmpty() && !"/".equals(base.getPath())))) { + throw new IllegalStateException( + "eddi.connections.public-base-url must be a bare https origin " + + "(scheme://host[:port]) β€” got: " + publicBaseUrl); +} +``` + +`eddi.connections.enabled` defaults to **false**, matching the +`openai-compat` precedent (`openai-api-adapter-plan.md:592-595`). + +**Decision 4 β€” the guard also refuses the two configurations that would make +per-user grants meaningless:** + +```java +if (perUserConnectionsConfigured && !authorizationEnabled) { + throw new IllegalStateException( + "PER_USER connections require authorization.enabled=true β€” without a " + + "verified identity, any caller can claim any userId and resolve that " + + "user's tokens (see OpenAiAuthFilter's trust-user-headers caveat)"); +} +if (oauthConnectionsConfigured && !vaultActive) { + throw new IllegalStateException( + "OAuth connections require an active SecretsVault (EDDI_VAULT_MASTER_KEY) " + + "β€” grants are envelope-encrypted with the tenant DEK and there is no " + + "plaintext fallback"); +} +``` + +The first closes the identity hole described in Β§5.3: with +`authorization.enabled=false`, `@Authenticated` on the authorize endpoint is a +no-op via `DisabledAuthController`, so the flow would happily mint grants for +self-asserted principals. The second refuses the `autoVaultSecret` antipattern β€” +that path falls back to storing plaintext with an ERROR log when the vault is +inert (`AGENTS.md Β§5.4`); a refresh-token store must fail to deploy instead. +`SERVICE`-bound static/basic connections remain usable without either +precondition, so dev-mode Jira/Amplitude work is unaffected. + +### 6.3 MCP OAuth discovery (Phase 4b) + +Turn G3 from a circuit-breaker trip into an auth challenge. On a 401 from an MCP +server, parse `WWW-Authenticate` for a `resource_metadata` URL, fetch +`/.well-known/oauth-protected-resource` (RFC 9728) then the authorization server +metadata (RFC 8414), and either use a matching `ConnectionConfiguration` or surface +a precise "this server needs connection X" error to the Manager. Dynamic client +registration (RFC 7591) is **out of scope** β€” an admin registers the client once and +stores the ID/secret in a connection. + +**Discovery input is attacker-influenced and must be treated as such.** The +`resource_metadata` URL arrives in a header from the very server we are failing +to authenticate against, and the document it returns names the authorization +server. Three rules, none optional: + +1. Every discovery fetch goes through **`SafeHttpClient.sendValidated`** (C2) β€” + URL validation, `Redirect.NEVER`, cross-origin credential stripping. This is + the one new outbound path in the plan, so it starts compliant rather than + joining the four existing bypasses at `security-hardening-remaining.md:104-107`. +2. The `resource_metadata` URL must share an origin with the MCP server that + returned it. A server may not redirect discovery to a host of its choosing. +3. RFC 9728 Β§3.3: the metadata's `resource` value must match the MCP server URL + being accessed, and any `authorization_servers` entry must be on the + credential-endpoint allowlist (Β§5.1) before a token is requested from it. + Discovery may *select* among pre-approved authorization servers; it may never + *introduce* one. + +The circuit breaker must learn to distinguish `401 β†’ needs auth` from +`connection refused β†’ server down`; today both increment the same counter +(`:362-367`). + +--- + +## 7. MCP transport work + +`StdioMcpTransport` is already present in the pinned `langchain4j-mcp` +1.18.1-beta28 jar (`dev/langchain4j/mcp/client/transport/stdio/StdioMcpTransport`), +alongside an unused `WebSocketMcpTransport`. The transport is free; the cost is +everything around it. + +### 7.1 Phase 3a β€” sidecar bridge (recommended first, XS) + +Document and ship a compose example running `mcp-proxy` / `supergateway` as a +sidecar that bridges stdio β†’ streamable HTTP. EDDI talks to it over the transport +it already has. The child process stays in its own container with its own limits +and its own image; **EDDI's** process gains no code-execution surface, no +interpreter in its runtime image, and no process lifecycle code. **This unlocks +the majority of vendor MCP servers for a docs page.** + +To be precise about what this does and does not buy, since "sidecar" is easy to +over-read as "solved": the MCP server binary still executes, and it still speaks +to EDDI over a network channel. Container separation bounds the blast radius; it +removes neither process-execution nor supply-chain risk β€” a malicious +`npx some-mcp-server` is still malicious, just contained. The docs page must +therefore cover: image provenance and pinning (digest, not tag β€” the same rule +`AGENTS.md` already applies to the EDDI base image), non-root execution, +CPU/memory limits, a network policy restricting the sidecar's egress to the +provider it needs, and authentication on the bridge itself, so the stdio server +is not reachable by anything else on the pod network. + +Also in 3a, cheaply: fix the `sse` inconsistency by adding it to +`SUPPORTED_TRANSPORTS` (`McpCallsConfiguration.java:80`) as a deprecated alias, so +the REST write path stops 400-ing a value the runtime accepts. + +### 7.2 Phase 6 β€” native stdio, gated (M, only if 3a proves insufficient) + +Deferred deliberately, because a config-editable `command` array is arbitrary code +execution as the EDDI uid β€” and given G7, that reads as unauthenticated RCE until +Phase 0 lands. + +If built, the shape is constrained: + +- **Not a free-form command.** An admin-defined catalog of server templates + (`{name β†’ command, allowedEnv}`) in `application.properties` or a dedicated + admin-only store; agent configs reference a template **by name** only. +- `eddi.mcp.stdio.enabled` defaults false; startup guard refuses `enabled=true` + when `authorization.enabled=false`. +- Config model becomes polymorphic on transport: `mcpServerUrl` is currently + required and must be http(s) (`McpCallsConfiguration.java:105-117`); stdio needs + `templateName` + `env` instead, and the client cache key (`:437-450`) must digest + those rather than url+apiKey. +- SSRF validation and `${caller:token}` are meaningless for stdio β€” reject both at + validate time rather than silently ignoring them. +- Process lifecycle: spawn, reap, restart-on-crash, memory/CPU caps, drain on + shutdown. Note every replica spawns its own copy β€” a 5-pod deploy runs 5 servers + with 5 independent token states. + +--- + +## 8. Phases + +Each phase must build cleanly, compile, and pass tests before the next begins +(`multi-tenancy-plan.md:176`). Phases 0-2 are independently shippable and worth +doing even if the connector work never starts. + +### Phase 0 β€” Close the exposure gap (~2-3 days) Β· P0 + +Preconditions for everything else. Nothing here is connector work. + +| Item | Change | +|---|---| +| **0.1** | `McpStartupGuard`, mirroring `OpenAiStartupGuard.isUnprotected()` (`:74-76`): refuse `NORMAL` launch when `/mcp` is reachable with `authorization.enabled=false` unless an explicit `eddi.mcp.allow-unauthenticated=true` opt-out is set. Add the matching path-policy entry. **Do not touch the global default** (C1). | +| **0.2** | `discover-tools` and `discover-endpoints` β†’ `POST` with a request body; credential moves to a `@HeaderParam` following `IRestImportService.java:65`. Stop returning `apiAuth` inside generated ApiCall headers (`RestApiCallsStore.java:97` puts `configsByGroup` β€” each containing the pasted credential via `McpApiToolBuilder.java:250-251` β€” straight into the response body). **Manager lockstep required:** the endpoint's own Javadoc says "Used by the Manager UI for selective API call import", so the Manager's discovery calls must switch to POST in the same release. **The GET forms lose the credential parameter entirely** β€” an earlier draft kept them for one release "rejecting a non-empty credential param", which does not remove the leak: by the time the handler rejects it, the value has already been through ingress, any reverse proxy, access logs, browser history, and APM traces. The deprecated GET either serves credential-free discovery (public specs) or 410s with a pointer to POST; the parameter does not survive the release. | +| **0.3** | `LogCaptureFilter` redacts in place, or a console formatter wraps `SecretRedactionFilter`, so stdout matches the ring buffer. | +| **0.4** | Stop returning raw `e.getMessage()` to HTTP callers (`RestMcpCallsStore.java:161`); log the exception class only on the outbound-failure paths, following the discipline already used at `HttpCallToolsProvider.java:252-262`. | +| **0.5** | `SecretScrubber`: recurse into array elements (`:134`), add an `x-*`/`*-token`/`*-key` header-name rule, and scrub URL-embedded credentials by reusing `RequestRedactor.redactUri`'s logic rather than the whole-string `KEY_LIKE_PATTERN`. | +| **0.6** | Apply `governDescription` to A2A agent-card and skill descriptions (`A2AToolProviderManager.java:152-175`). One call site, closes an asymmetry with MCP. | +| **0.7** | SSRF protection: **needs an explicit product decision, not a silent flip.** The `false` default is documented intent, not an oversight β€” the comment at `application.properties:341-343` says *"Default OFF to preserve calls to internal/private APIs in self-hosted deployments β€” enable for multi-tenant / internet-facing."* Flipping `%prod` to `true` breaks every self-hosted agent that calls an internal API. Proposed resolution: keep the global default, but have `eddi.connections.enabled=true` force SSRF protection on (connections by definition target third parties, and Phase 5 stores tokens worth stealing via `discover-tools`'s echo primitive), plus a release-notes migration note. Sign-off required either way. | +| **0.8** | **Protect `/secretstore` β€” the credential store has the same exposure as `/mcp`.** `IRestSecretStore` is `@RolesAllowed("eddi-admin")` but has **no path-specific policy**, so it falls to the catch-all `authenticated` policy at `application.properties:396-398` β€” and `DisabledAuthController` disables *both* under the shipped `authorization.enabled=false`. Vault writes, DEK rotation and `reset` are unauthenticated out of the box. Add a `/secretstore/*` path policy and extend the Phase 0.1 guard to cover it. **This must land before Phase 2**, and arguably before anything: a plan about storing credentials cannot leave the existing credential store open. (Caught in review β€” the original draft flagged `/mcp` and missed this.) | + +### Phase 1 β€” Govern what comes back (~4-5 days) Β· P0 + +| Item | Change | +|---|---| +| **1.1** | Provenance-mark tool results: delimit and label third-party content untrusted. Hook point is **`ToolLoopRunner.executeSingleToolCallResult`** β€” its own doc comment names it "the single shared copy" serving both the live loop (which appends verbatim at `:464`) and the resume path, so one change covers all seven tool sources, both execution paths, and (because the bridge's executors return ordinary tool results) MCP resource content and listings for free. (The `AgentOrchestrator` `tool_result` map fix from `langchain4j-recommendations.md:1082` is the *redaction* producer for SSE/audit β€” a related but separate change; do both, conflate neither.) | +| **1.2** | Extend the R0 `GuardrailService` contract to tool results, closing the gap `langchain4j-recommendations.md:250-252` names. Reuses R0's action vocabulary (`block \| redact \| reprompt \| warn`, C8). Never throws (C7). | +| **1.3** | `ToolRequestResolver` implementations for MCP and A2A, so `McpToolsProvider.java:80` and `A2AToolsProvider.java:51` stop passing empty maps and gated calls get a redacted preview plus a re-checked fingerprint (C5). The fingerprint is computed over the **redacted** request β€” credential header values excluded β€” otherwise a token refresh between approval and execution changes the hash and every approval of a connection-backed call spuriously fails the re-check. | +| **1.4** | `SecretResolver` invalidation listeners for `McpToolProviderManager` and `ChannelTargetRouter`, matching `ChatModelRegistry.java:152-167`. | + +### Phase 2 β€” Unify (~5-7 days) Β· P1 β€” **the gate** + +`ConnectionConfiguration` + store + REST + descriptor + `ConnectionResolver`, with +`STATIC` and `BASIC` auth types only. No OAuth yet. Wire the resolver into all five +resolution chains (Β§5.3). `BASIC` finally gives Jira and Amplitude first-class +support β€” today you must vault a pre-base64-encoded blob because nothing encodes +for you. + +Also: register connection **configuration writes and grant-lifecycle routes** +in the operator write-scope exclusion list (C6). Both, not just the first β€” +`POST /connections/{id}/authorize` and the disconnect/revoke routes create and +destroy credentials without touching a connection *document*, so an exclusion +written against config writes alone would leave the operator able to mint or +delete grants. Each excluded route gets its own authorization test (Β§11), +and add the store to `VaultGrantChecker`'s traversal so a connection's +`${vault:}` client secret participates in deploy-time grant enforcement. + +Migration is additive β€” every existing `apiKey` field keeps working. Connections +are the recommended path, not a forced one. + +### Phase 3 β€” Transports (~1 day) Β· P1 + +3a from Β§7.1: sidecar docs, compose example, `sse` alias fix. Phase 6 (native +stdio) stays deferred. + +### Phase 4 β€” OAuth service account (~4-5 days) Β· P2 + +`OAUTH2_CLIENT_CREDENTIALS`, the grant store, `EnvelopeCrypto` reuse, lazy refresh +with single-flight (Β§5.4). Then 4b: MCP `WWW-Authenticate` / RFC 9728 discovery +(Β§6.3) and splitting the circuit breaker's auth-vs-down counters. + +### Phase 5 β€” OAuth per-user (~8-12 days) Β· P3 + +`OAUTH2_AUTHORIZATION_CODE` + PKCE, the state store, the permit-path callback, the +`ConnectionStartupGuard`, and the Manager surfaces: an admin connection list, and a +per-user "your linked accounts" page with connect/disconnect. Plus a deploy-time +check that an agent referencing a `PER_USER` connection surfaces "users must +connect" rather than failing at turn time. + +### Phase 6 β€” Deferred / opportunistic Β· P4 + +Native stdio (Β§7.2), A2A peer store extraction from `LlmConfiguration`, built-in +tool credentials β†’ vault, NATS auth, channel export. + +--- + +## 9. Files that would change + +#### CREATE + +| File | Purpose | +|---|---| +| `engine/security/McpStartupGuard.java` | 0.1 | +| `modules/llm/guardrails/ToolResultGuardrail.java` | 1.2 | +| `modules/llm/impl/McpToolRequestResolver.java`, `A2AToolRequestResolver.java` | 1.3 | +| `configs/connections/model/ConnectionConfiguration.java`, `OAuthConfig.java`, `StaticAuth.java` | 2 | +| `configs/connections/IConnectionStore.java`, `mongo/ConnectionStore.java`, `postgres/ConnectionStore.java` | 2 | +| `configs/connections/IRestConnectionStore.java`, `rest/RestConnectionStore.java` | 2 | +| `connections/ConnectionResolver.java`, `model/ConnectionReference.java`, `ConnectionException.java` | 2 | +| `connections/grants/IConnectionGrantStore.java` + Mongo/Postgres impls, `model/ConnectionGrant.java` | 4 | +| `connections/oauth/OAuthTokenService.java`, `OAuthStateStore.java`, `rest/RestOAuthCallback.java` | 4-5 | +| `connections/McpAuthChallengeParser.java` | 4b | +| `connections/ConnectionStartupGuard.java` | 5 | +| `docs/connections.md`, `docs/mcp-client.md` (**does not exist today**) | 2-5 | + +#### MODIFY + +| File | Change | +|---|---| +| `configs/mcpcalls/IRestMcpCallsStore.java`, `rest/RestMcpCallsStore.java` | 0.2, 0.4 | +| `configs/apicalls/IRestApiCallsStore.java`, `rest/RestApiCallsStore.java` | 0.2 | +| `engine/runtime/LogCaptureFilter.java` | 0.3 | +| `secrets/sanitize/SecretScrubber.java` | 0.5 | +| `modules/llm/impl/A2AToolProviderManager.java` | 0.6, Β§5.3, C2 | +| `src/main/resources/application.properties` | 0.1, 0.7, Β§6.2 permit path, `eddi.connections.*` | +| `modules/llm/impl/AgentOrchestrator.java` | 1.1 | +| `modules/llm/impl/McpToolsProvider.java`, `A2AToolsProvider.java` | 1.3 | +| `modules/llm/impl/McpToolProviderManager.java` | 1.4, Β§5.3 supplier reuse, 4b, Β§7 | +| `integrations/channels/ChannelTargetRouter.java` | 1.4 | +| `modules/apicalls/impl/ApiCallExecutor.java` | Β§5.3 fourth resolution step | +| `modules/llm/impl/ChatModelRegistry.java`, `EmbeddingModelFactory.java`, `EmbeddingStoreFactory.java` | Β§5.3 `SERVICE`-only | +| `secrets/VaultGrantChecker.java` | Phase 2 β€” traverse connection configs | +| `configs/mcpcalls/model/McpCallsConfiguration.java` | 3a `sse` alias; Β§7.2 if built | +| `EDDI-Manager/src/lib/operator/tool-scopes.ts` | C6 β€” exclude connection writes | +| `docs/changelog.md`, `AGENTS.md Β§3` roadmap | every phase | + +#### DELETE + +None. All changes are additive; existing `apiKey` fields keep working. + +--- + +## 10. Metrics + +Required for new features per the repo guideline (`operator-write-scope-plan.md:173-181`). +Micrometer, exposed at `/q/metrics`. + +**Tags are bounded categoricals only.** Connection names and MCP server URLs are +author-supplied and unbounded, so tagging with them would let a config author +mint Micrometer series without limit (a cardinality-explosion DoS on the metrics +backend) and would publish tenant and provider names to anyone who can read +`/q/metrics` β€” which, per G7, is anyone at all by default. The specific +identifier belongs in a sanitized log line or an exemplar, never a tag. + +| Metric | Type | Tags (all bounded) | +|---|---|---| +| `connection.resolve.count` | Counter | `authType`, `binding`, `outcome` | +| `connection.resolve.time` | Timer | `authType`, `binding` | +| `connection.grant.missing.count` | Counter | `binding` β€” the per-user "not connected yet" signal | +| `connection.oauth.authorize.count` | Counter | `authType`, `outcome` | +| `connection.oauth.callback.count` | Counter | `outcome` (`success`/`bad_state`/`expired_state`/`replayed`/`exchange_failed`) | +| `connection.token.refresh.count` | Counter | `authType`, `outcome` (incl. `invalid_grant` vs `transient`, Β§5.4) | +| `connection.token.refresh.claim.count` | Counter | `outcome` (`claimed`/`awaited`/`lease_expired`) β€” validates the Β§5.4 claim | +| `connection.grant.status` | Gauge | `status` | +| `mcp.auth.challenge.count` | Counter | `outcome` β€” 4b (`server` deliberately omitted: URLs are unbounded) | +| `guardrail.toolresult.count` | Counter | `action`, `source` β€” 1.2 | + +--- + +## 11. Tests and verification + +```bash +./mvnw compile && ./mvnw test +``` + +Integration tests and anything binding a loopback socket are CI-only +(`AGENTS.md`, and the local-sandbox caveat). Treat a green CI run as the source of +truth for `*IT.java`. + +Behavioural tests that must exist, each written so that reverting the fix makes it +fail: + +- **0.1** β€” startup fails with `/mcp` reachable, `authorization.enabled=false`, no + opt-out; boots with the opt-out; boots in dev. +- **0.2** β€” the GET forms reject a non-empty credential param; the POST forms work; + the generated ApiCall no longer contains `apiAuth`. +- **0.5** β€” a secret inside a `List`, a header named `X-Api-Token`, and + `https://user:pass@host` in `targetServerUrl` are all scrubbed on export. These + are the three documented holes; assert each separately. +- **1.1** β€” a tool result containing `DIRECTIVE_PATTERN` text arrives at the model + delimited and labelled. +- **1.3** β€” a gated MCP call produces an approval payload containing target origin + and a fingerprint, and a fingerprint mismatch at execution time refuses. +- **Β§5.3** β€” `${connection:}` in a URL, body or query param is rejected at + validate time; a `PER_USER` connection on a scheduled turn throws rather than + falling back to the service grant; a target origin outside `baseUrlAllowlist` is + refused. +- **Β§5.4** β€” two concurrent resolves of an expired grant produce exactly one token + request (assert on the claim counter), the non-claimant adopts the claimant's + token, and β€” the case the in-process map cannot cover β€” two *separate* resolver + instances contending on the same row still yield one token request, proving the + claim is the cross-replica gate and not just a local optimization. Plus: an + expired lease is reclaimable, and `invalid_grant` marks `REFRESH_FAILED` while a + timeout does not. +- **Β§6.2 step 3** β€” two concurrent callbacks with the same valid `state` result in + exactly one code exchange; the loser gets a clean error, not a second redemption. + +**Storage and authorization boundaries** β€” one behavioral test each, all required +before Phase 2 (first four) and Phase 5 (rest) can be called done. These pin +declared invariants that are otherwise only prose: + +- A connection grant never appears in an agent export ZIP, under any `authType`, + including when the grant store is populated. +- `VaultGrantChecker` traverses connection configs and reports a `${vault:}` + client secret the agent is not granted β€” the serialize-and-scan path, not an + enumerated field list (C-changelog 2026-08-10). +- Connection **configuration** writes are outside operator write scope. +- Connection **grant-lifecycle** routes (`authorize`, disconnect/revoke) are + outside operator write scope β€” asserted per route, since these mutate + credentials without touching a config document. +- Tenant isolation holds in **both** persistence backends: a grant written under + tenant A is not readable under tenant B, tested against Mongo and Postgres + rather than one and assumed for the other. +- `/secretstore/*` and `/mcp` reject unauthenticated access once their Phase 0 + policies are in place (0.1, 0.8). +- **Β§6.2** β€” a replayed `state` is refused; an expired `state` is refused; a + `state` for user A cannot install a grant for user B; a `state` issued by + replica 1 is honoured by replica 2 (persistent state store). +- **Β§6.2 Decision 4** β€” startup fails when a `PER_USER` connection exists and + `authorization.enabled=false`; startup fails when an OAuth connection exists + and the vault is inert; a `SERVICE` static connection deploys fine under both + conditions. +- **Β§5.3 verified principal** β€” with a client-supplied user header and no OIDC + identity, `PER_USER` resolution throws; it never resolves a grant keyed on the + asserted userId. Mutation-check this one: revert the verified-principal check + and confirm the test fails. + +Note the `-Dtest=Class#method` caveat for `@Nested` classes β€” filter by whole +class or the run silently passes with zero tests. + +--- + +## 12. What I would NOT do + +- **Not flip the global `authorization.enabled` default.** Per-surface guard plus + path policy (C1). Flipping it breaks every shipped compose file and k8s manifest + simultaneously, and the repo has already chosen the other pattern twice. +- **Not resolve connections inside `SecretResolver`.** C3. The vault stays a static + secret store; connections use it for client secrets and live above it. +- **Not store a token anywhere outside `EnvelopeCrypto`**, and not let a plaintext + token appear in a config, an export, a log, conversation memory, or a REST + response. Only `${connection:name}` is inherited (C4). +- **Not add a sixth bespoke `HttpClient`.** The OAuth token client uses + `SafeHttpClient.sendValidated` (C2). Ideally Phase 4 also converts + `A2AToolProviderManager` while it is being touched anyway. +- **Not offer `PER_USER` binding on LLM, embedding or vector-store connections.** + Those cache built objects on unresolved parameters by design; a per-request + identity there is incoherent. Reject at validate time rather than degrading. +- **Not build native stdio in the first pass.** Sidecar first (Β§7.1); native only + behind an admin template catalog, and never with a config-editable command array. +- **Not implement dynamic client registration (RFC 7591).** An admin registers once. +- **Not build a proactive refresh sweeper in Phase 5.** Lazy + single-flight is + correct; add the sweeper only if telemetry shows cold-start latency matters. +- **Not add bespoke tenant scoping.** `tenantId` field + `"default"` + + `Provider` when Phase 1 of the multi-tenancy plan lands (C9). + Register the new stores in that plan's Phase 2 table rather than forking it. +- **Not accept a client-supplied header as the grant principal β€” ever.** Not even + behind a `trust-user-headers`-style toggle. The `/v1` adapter's toggle trades + impersonation risk for Open WebUI compatibility on *conversation* data; a token + store raises the stakes to credential theft, and no compatibility argument + survives that. +- **Not treat `toolsWhitelist`/`toolsBlacklist` as a security boundary.** They are + context-window management. The security boundary is the connection's + `baseUrlAllowlist` plus the HITL gate. #668 makes this concrete: the resource + bridge is *deliberately* not subject to `toolsWhitelist`, because that filter + governs server-advertised names and the bridge's are EDDI-synthesized β€” so a + whitelist is not even a complete inventory of what a config exposes. +- **Not enumerate credential field names anywhere.** The 2026-08-10 vault decision + chose serialize-and-scan precisely because *"enumeration is how this kind of check + rots when a new credential field appears."* `VaultGrantChecker`'s connection + traversal follows the same rule. +- **Not export connection grants**, and not add connections to agent ZIPs without a + scrubbing story β€” G11 shows what happens when export outruns scrubbing. + +--- + +## 13. Open questions + +1. **Group conversations and per-user tokens.** When a group agent acts inside a + `GroupConversation`, whose Drive token does it use? The invoking user's, the + group owner's, or a service grant? `GroupConversationService.discuss()` creates + individual member conversations, so the principal may not be the human at all. + This needs a decision before Phase 5 and may add a `groupBinding` field. +2. **How many providers ship natively in Phase 5** before Option D (Β§4) takes over + the long tail. A buy/build call, not a technical one. +3. **Row-level tenant filtering vs database-per-tenant** β€” inherited unresolved + from `multi-tenancy-plan.md:509`. The grant store's index design follows whatever + that resolves to. +4. **Should connection-backed writes require HITL approval by default?** Argument + for: a per-user OAuth token plus an LLM plus a write tool is the configuration + most likely to cause real damage. Argument against: it makes every connector feel + broken out of the box. If yes, it must be expressed as a wildcard plus an exempt + list (C11), never an enumerated write list. +5. **Does `${connection:}` supersede `${caller:token}`** as a `SERVICE`-vs-relay + distinction, or do both stay? They overlap for EDDI-calling-EDDI. Leaning: both + stay, `${caller:token}` documented as the same-origin special case. +6. **Grant revocation propagation.** Deleting a grant row stops resolution + immediately, but should EDDI also call the provider's revocation endpoint? Doing + so is correct and adds a per-provider config field; not doing so leaves live + tokens at the provider after a user disconnects.