From 52c58a5a0a052a0fd040d144c0bb9064dfcc2ee0 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 11 Jun 2026 12:53:33 -0400 Subject: [PATCH] Fold in cross_reviewer (GPT-4.1) auth/trifecta findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-family review caught defense-in-depth gaps: - CR-1: validate aud at edge AND server-side (per-tier authorizer doesn't replace design.md §2.5 server enforcement); alarm on wrong-audience tokens. - CR-6: finance-audience token must not reach Gmail/Calendar even with a Google token. - CR-2: verify+enforce received sub is the Google Workspace sub, not a Salesforce id. - CR-3: pre-token Lambda fails closed on cross-audience scope. - CR-5: pre-token + group-sync are the auth SPOF — alarms + group-claim freshness bound. - CR-4/CR-7: restrict per-client Cognito scopes; WAF is defense-in-depth only. Reflected in §0.1 B3/B4, §1h tests, §4 monitoring, §5 cross-review log. --- docs/agentforce-plan.md | 65 ++++++++++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 14 deletions(-) diff --git a/docs/agentforce-plan.md b/docs/agentforce-plan.md index 59a7ff9..3cf3cf4 100644 --- a/docs/agentforce-plan.md +++ b/docs/agentforce-plan.md @@ -109,13 +109,17 @@ Agentforce per-user path (D4) is OpenAPI/Apex, not MCP. Decision: - **Ship the OpenAPI adapter now** — Agentforce **External Service (OpenAPI) actions** are the primary transport; use **Apex `@InvocableMethod`** only for actions needing request/response shaping (e.g. extra redaction, pagination). -- **One facade per trust tier (B3-resolved — the audience boundary stays in infrastructure, not in shared - handlers).** Deploy a **separate API Gateway + Cognito authorizer per server**: `sh-mcp-ops` facade validates - `aud=sh-mcp-ops`, `sh-mcp-finance` facade validates `aud=sh-mcp-finance`. A token minted for ops is rejected at - the finance gateway's authorizer (and vice-versa) — the audience-binding rejection design.md §2.5 requires, - enforced at the edge. A **shared AWS WAF web ACL** fronts both. This is deliberately NOT one shared gateway with - audience checks pushed into application code: keeping per-tier audiences at separate authorizers preserves the - trust-tier firewall even if a handler bug slips through. +- **One facade per trust tier (B3-resolved — the audience boundary stays in infrastructure).** Deploy a + **separate API Gateway + Cognito authorizer per server**: `sh-mcp-ops` facade validates `aud=sh-mcp-ops`, + `sh-mcp-finance` facade validates `aud=sh-mcp-finance`. A token minted for ops is rejected at the finance + gateway's authorizer (and vice-versa). A **shared AWS WAF web ACL** fronts both (WAF is rate-limit/IP + defense-in-depth ONLY, never an authz boundary — CR-7). +- **Audience validated at the edge AND the server (defense in depth — CR-1, design.md §2.5).** The per-tier + authorizer is NOT the only check: every server **independently re-validates issuer + `aud` + required scope on + every call** (design.md §2.5 makes server-side enforcement authoritative — the edge does not replace it). The + per-tier gateway adds an early-reject layer; it does not move the boundary off the server. **Alarm on any token + presented to the wrong audience** (a finance-aud token at the ops endpoint, or vice-versa) — that signature + means either a misconfig or an attack. - **Defer the MCP adapter** until a **named trigger**: (a) Claude Code / IDE consumption becomes a real recurring workflow (not nice-to-have), **or** (b) Salesforce native remote-MCP per-user binding reaches GA (then MCP-native could also collapse the Agentforce-side OpenAPI facade). When triggered, the MCP adapter is a thin add over the @@ -143,10 +147,23 @@ credential that requests **only its tier's scopes**. Mechanism (specified, testa per-agent credential, not per-agent action assignment (which the plan concedes is "a convenience, never the boundary," §1h). - The Cognito pre-token Lambda must therefore scope the minted scopes to the requested `aud` (resource server), - not blanket-union across all audiences. **This is a per-server IAM/authz behavior → mandatory cross-review + not blanket-union across all audiences, and **fail closed**: if a scope from another audience would ever appear + in a token, reject and alarm (CR-3). **This is a per-server IAM/authz behavior → mandatory cross-review (GPT-4.1) before commit** (design.md §8/§10). -- Tested by: "per-agent credential mints only its tier's scopes" (a token obtained via the Exec agent's - credential never contains `finance:read`) — added to the §1h security suite. +- **Backend `sub` validation (CR-2).** Do not assume Salesforce's External Credential forwards the Google `sub` + unchanged — verify in Phase-0a that the JWT our endpoint receives carries the **Google Workspace `sub`** (not a + Salesforce/Cognito-internal id), and have each server **reject any token whose `sub` is not a valid Workspace + user**. Force short token lifetimes; watch for Salesforce-side token caching/reuse across agents. +- **Gmail/Calendar segregation is defense-in-depth, not just credential-shaped (CR-6).** The Finance agent's + credential never carries `gmail:self`/`calendar:self`, but the servers must **also** refuse Gmail/Calendar tool + calls unless the token carries the matching `*:self` scope AND the Google token is ABAC-partitioned by `sub` + (design.md §2.4) — so a finance-audience token can never reach a mailbox even if something upstream misfires. +- **Pre-token + group-sync are the auth SPOF (CR-5).** Alarm on group-sync failure (design.md §2.3 already) AND on + pre-token Lambda error rate / any cross-audience-scope event; enforce a hard freshness bound on group claims so a + stale sync can't silently widen scope. +- Tested by (§1h): "per-agent credential mints only its tier's scopes" (a token via the Exec credential never + contains `finance:read`); "wrong-audience token rejected at edge AND server"; "finance token cannot reach a + Gmail/Calendar tool"; "pre-token fails closed on a cross-audience scope." **Dropped claim:** the "BYOLLM = ~30% fewer Einstein Requests" figure was **not corroborated** by any verified source — removed from cost modeling. @@ -175,6 +192,20 @@ and FIX is addressed below; this revision supersedes the pre-audit text wherever | **N1/N2/N3** | §1f ingestion pinned to **S3 → Data Cloud**; ALARM-only monitoring for the facades + repointed `notion-sync`; Salesforce DevName/kebab boundary noted. | | **Q1/Q2/Q3** | **G15** (Slack plan supports Employee Agents; per-employee Agentforce licensing; `$User.GoogleGroups`/`$Session.Channel` existence) — all unverified-load-bearing, on the §6 verify gate. | +**Cross-family review (GPT-4.1 `cross_reviewer`, 2026-06-11) — auth/trifecta sections.** Run per the mandatory +cross-review gate for auth/IAM design (Fable is Claude-family, not a substitute). Findings folded in: +- **CR-1 (BLOCK):** validate `aud` at the edge **AND** server-side (defense in depth) — the per-tier authorizer + does not replace design.md §2.5 server-side enforcement; alarm on wrong-audience tokens. → §0.1 B3, §1h. +- **CR-6 (BLOCK):** a finance-audience token must not reach Gmail/Calendar even with a valid Google token — backend + refuses unless `*:self` scope present + Google token ABAC-partitioned by `sub`. → §0.1 B4, §1h. +- **CR-2 (FIX):** verify + enforce that the received `sub` is the Google Workspace `sub` (not a Salesforce/Cognito + id); reject otherwise; watch Salesforce token caching. → §0.1 B4, Phase-0a. +- **CR-3 (FIX):** pre-token Lambda fails closed + alarms if any cross-audience scope would appear. → §0.1 B4, §1h. +- **CR-5 (FIX):** pre-token + group-sync are the auth SPOF — alarms + freshness bound on group claims. → §0.1 B4, §4. +- **CR-4/CR-7 (NIT):** restrict each Cognito app client's allowed scopes; WAF is defense-in-depth only. → §0.1. +Re-run `cross_reviewer` on the concrete pre-token Lambda + IAM/Cognito resource-server config once written +(design.md §10 asked for the real artifacts). + --- ## 1. Platform-level design @@ -375,10 +406,13 @@ real home: deprecating each bot (design.md §6, §7.3 parity gate). **Core security tests (authoritative, transport-agnostic, in `sh-mcp`, design.md §7.3):** -**audience-binding rejection at the per-tier facade** (an `aud=sh-mcp-ops` token rejected by the `sh-mcp-finance` -gateway authorizer, and vice-versa — B3); per-tool **server-side** scope enforcement; **per-agent credential -mints only its tier's scopes** (a token obtained via the Lauren Exec app client never contains `finance:read`, -even though the person holds it — B4); deny-list **hard revocation**; minimal-scope Google client (a `gmail:self` +**audience-binding rejection at the per-tier facade AND re-validated server-side** (an `aud=sh-mcp-ops` token +rejected by the `sh-mcp-finance` gateway authorizer *and* by the finance server itself — B3/CR-1, defense in +depth); per-tool **server-side** scope enforcement; **per-agent credential mints only its tier's scopes** (a +token obtained via the Lauren Exec app client never contains `finance:read`, even though the person holds it — +B4); **pre-token fails closed on a cross-audience scope** (CR-3); **a finance-audience token cannot reach a +Gmail/Calendar tool** (CR-6); **backend rejects a token whose `sub` is not a valid Google Workspace user** (CR-2); +deny-list **hard revocation**; minimal-scope Google client (a `gmail:self` token can't mint a Calendar token); **per-user refresh-token ABAC isolation**; **finance PII redaction** (bank/routing/card/SSN masked before egress) **while leaving vendor names/contacts UNMASKED** (legacy Alex deliberately left names unmasked — Trust Layer must not re-mask them, Gap G3); per-tool rate limit + per-session @@ -621,6 +655,9 @@ MCP layer) are unchanged — those stay locked. only, no OK/no-data). - Repointed **`notion-sync`** carries the design.md ALARM-on-sync-failure through to the dual-feed (two-alarm pattern: Errors≥1 missing=notBreaching; Invocations<1 over cadence missing=breaching). +- **Auth SPOF (CR-5):** alarm on **group-sync Lambda** failure (design.md §2.3) and on **pre-token Lambda** error + rate + any **cross-audience-scope** event; enforce a freshness bound on group claims so a stale sync can't widen + scope silently. ALARM action only → site-alerts. **Jira (INFRA project — no migration epic exists yet; related done: INFRA-92 AOSS lockdown, INFRA-37 reminder removal):**