diff --git a/docs/remote-mcp/threat-model-anchors.json b/docs/remote-mcp/threat-model-anchors.json new file mode 100644 index 0000000..670420f --- /dev/null +++ b/docs/remote-mcp/threat-model-anchors.json @@ -0,0 +1,80 @@ +{ + "_comment": [ + "Machine-checkable anchor table for docs/remote-mcp/threat-model.md (#956).", + "Every file:line anchor cited in the threat model must appear here, and the", + "source line at that anchor must contain the 'expect' substring.", + "tests/test_issue_956_threat_model.py enforces both directions, so a refactor", + "that shifts a line number fails the suite instead of silently rotting the", + "document. #930's inventory had no such guard and its gitea_mcp_server.py", + "anchors drifted between 7bf4f125 and aad5c8b4." + ], + "generated_against_commit": "aad5c8b42361d380a8eeb07b94b90815e594c2c5", + "anchors": [ + {"anchor": "gitea_mcp_server.py:24721", "expect": "bind_native_mcp_transport(transport=\"stdio\")"}, + {"anchor": "mcp_daemon_guard.py:45", "expect": "_PRODUCTION_TRANSPORTS = frozenset({\"stdio\"})"}, + {"anchor": "mcp_daemon_guard.py:174", "expect": "def bind_native_mcp_transport"}, + {"anchor": "irrecoverable_provenance.py:497", "expect": "def assess_transport_for_auth_mint"}, + {"anchor": "gitea_mcp_server.py:9129", "expect": "assess_transport_for_auth_mint()"}, + {"anchor": "gitea_mcp_server.py:9378", "expect": "assess_transport_for_auth_mint()"}, + {"anchor": "mcp_server.py:4", "expect": "Runs over stdio."}, + + {"anchor": "gitea_mcp_server.py:15412", "expect": "def _is_client_managed_process"}, + {"anchor": "gitea_mcp_server.py:15442", "expect": "def _provenance_mutation_block"}, + {"anchor": "gitea_mcp_server.py:15450", "expect": "unsupported_manual_launch"}, + {"anchor": "gitea_mcp_server.py:19001", "expect": "server_provenance"}, + {"anchor": "gitea_mcp_server.py:21442", "expect": "def _check_mcp_runtimes_diagnostics"}, + {"anchor": "gitea_mcp_server.py:21462", "expect": "\"ps\", \"-o\", \"pid,lstart,command\""}, + {"anchor": "gitea_mcp_server.py:21506", "expect": "\"ps\", \"eww\""}, + {"anchor": "gitea_config.py:1172", "expect": "RECOGNIZED_GITEA_ENV_KEYS"}, + {"anchor": "gitea_config.py:1233", "expect": "GITEA_CLIENT_MANAGED"}, + + {"anchor": "gitea_config.py:54", "expect": "ENV_PROFILE = \"GITEA_MCP_PROFILE\""}, + {"anchor": "gitea_config.py:97", "expect": "_REVIEW_MERGE_OPS"}, + {"anchor": "gitea_config.py:499", "expect": "repository authorization scope"}, + + {"anchor": "gitea_config.py:956", "expect": "def _keychain_token"}, + {"anchor": "gitea_config.py:974", "expect": "def resolve_token"}, + {"anchor": "gitea_config.py:1015", "expect": "def keychain_auth"}, + {"anchor": "gitea_config.py:294", "expect": "def _validate_identity_auth"}, + {"anchor": "mcp_daemon_guard.py:440", "expect": "def assert_keychain_access_allowed"}, + {"anchor": "gitea_mcp_server.py:19258", "expect": "def gitea_list_profiles"}, + {"anchor": "gitea_mcp_server.py:19309", "expect": "gitea_config.resolve_token(p)"}, + {"anchor": "gitea_mcp_server.py:19552", "expect": "def gitea_audit_config"}, + {"anchor": "gitea_mcp_server.py:19574", "expect": "service_summaries(config)"}, + + {"anchor": "gitea_config.py:704", "expect": "def resolve_service"}, + {"anchor": "gitea_config.py:837", "expect": "def service_summaries"}, + {"anchor": "gitea_config.py:851", "expect": "_keychain_token(auth.get(\"id\"))"}, + {"anchor": "gitea_mcp_server.py:17707", "expect": "\"jenkins-mcp\""}, + {"anchor": "gitea_mcp_server.py:17713", "expect": "external-mcp"}, + {"anchor": "gitea_mcp_server.py:17734", "expect": "\"glitchtip-mcp\""}, + {"anchor": "gitea_mcp_server.py:17739", "expect": "external-mcp"}, + {"anchor": "mcp_discoverability.py:9", "expect": "EXPECTED_JENKINS_TOOLS"}, + {"anchor": "mcp_discoverability.py:17", "expect": "EXPECTED_GLITCHTIP_TOOLS"}, + + {"anchor": "sentry_incident_bridge.py:36", "expect": "SENTRY_AUTH_TOKEN"}, + {"anchor": "sentry_incident_bridge.py:190", "expect": "def resolve_token"}, + {"anchor": "sentry_incident_bridge.py:289", "expect": "Authorization"}, + {"anchor": "sentry_observability.py:55", "expect": "SENTRY_DSN"}, + + {"anchor": "master_parity_gate.py:168", "expect": "def capture_startup_parity"}, + {"anchor": "master_parity_gate.py:255", "expect": "mutation_safe"}, + {"anchor": "gitea_mcp_server.py:19102", "expect": "def gitea_assess_master_parity"}, + + {"anchor": "gitea_mcp_server.py:190", "expect": "ACTIVE_WORKTREE_ENV"}, + {"anchor": "gitea_mcp_server.py:191", "expect": "AUTHOR_WORKTREE_ENV"}, + {"anchor": "gitea_mcp_server.py:2348", "expect": "/tmp/gitea_issue_lock.json"}, + {"anchor": "gitea_mcp_server.py:10894", "expect": "def gitea_bootstrap_author_issue_worktree"}, + {"anchor": "mcp_server.py:10", "expect": "/tmp/mcp_server_stderr.log"}, + + {"anchor": "issue_lock_store.py:26", "expect": "DEFAULT_LOCK_DIR"}, + {"anchor": "issue_lock_store.py:83", "expect": "def session_pointer_path"}, + {"anchor": "issue_lock_store.py:98", "expect": "def is_process_alive"}, + {"anchor": "mcp_session_state.py:27", "expect": "DEFAULT_STATE_DIR"}, + {"anchor": "control_plane_db.py:47", "expect": "DEFAULT_DB_PATH"}, + {"anchor": "control_plane_db.py:380", "expect": "mode=0o700"}, + {"anchor": "control_plane_db.py:386", "expect": "sqlite3.connect"}, + {"anchor": "control_plane_db.py:1145", "expect": "os.getpid()"}, + {"anchor": "gitea_mcp_server.py:12801", "expect": "owner_pid_alive"} + ] +} diff --git a/docs/remote-mcp/threat-model.md b/docs/remote-mcp/threat-model.md new file mode 100644 index 0000000..28c8038 --- /dev/null +++ b/docs/remote-mcp/threat-model.md @@ -0,0 +1,403 @@ +# Remote-MCP threat model, trust boundaries, and service decomposition + +What the adversary is, what each boundary protects, and which services may share a process. + +- **Issue:** #956 (Remote-MCP threat model), child of epic #929, cross-linked to #955. +- **Depends on:** #930 (closed) — `docs/remote-mcp/coupling-inventory.md`. +- **Blocks:** #932, #933, #934, #938. +- **Generated against commit:** `aad5c8b42361d380a8eeb07b94b90815e594c2c5` (`master`). +- **Scope:** documentation only. This child changes no server behavior. It adds one + document, one anchor fixture, and the test that enforces them. + +## Relationship to #930 + +#930 asked *what breaks when the process stops being local*. This document asks *what an +attacker gets, and where we stop them*. The two are deliberately different axes: #930 +classifies each coupling as portable, seam, replacement, or cannot-be-remote; this document +classifies each **credential** by blast radius and each **boundary** by what crossing it +requires. An entry can be perfectly portable and still be a trust disaster — +`gitea_config.py:851` is portable Python that reads a CI secret from inside the Gitea server. + +### Anchors are enforced, not asserted + +Every `file:line` in this document is declared in `docs/remote-mcp/threat-model-anchors.json` +with the substring that must appear at that line, and +`tests/test_issue_956_threat_model.py` fails if any anchor does not resolve or if the +document cites an anchor the fixture does not cover. + +This guard exists because #930 did not have one. Its inventory was generated at +`7bf4f125`; by `aad5c8b4` its `gitea_mcp_server.py` anchors had drifted — the transport +bind it cited at line 23750 now lives at `gitea_mcp_server.py:24721`, and its +client-managed provenance anchor at 14588 now lands in an unrelated function. Nothing +failed, because nothing checked. Anchors into a ~24,700-line module rot silently, and a +security document that cannot prove its own citations is worse than none, because it is +trusted. + +--- + +## 1. Assets + +What an adversary wants. Ordered by consequence, not by likelihood. + +| ID | Asset | Why it matters | +| -- | ----- | -------------- | +| A1 | Merge authority on `Scaled-Tech-Consulting/Gitea-Tools` | This repository *is* the control plane. Code merged here becomes the gate that authorizes every future mutation, so merge authority is self-amplifying: one merge can disable every other control in this document. | +| A2 | Write authority on the `mdcps` tenant | A second, unrelated organization reachable from the same configuration. Compromise here is a cross-organization incident, not an internal one. | +| A3 | The eight Gitea role credentials | Long-lived bearer tokens. Possession is authority; there is no second factor at the API. | +| A4 | Jenkins read access (`mdcps`, enabled) | Build logs routinely carry deployment topology, internal hostnames, and accidentally-echoed secrets. | +| A5 | Error-tracking read access (GlitchTip / Sentry) | Event payloads carry stack frames, request context, and production user data. | +| A6 | Coordination-state integrity | The locks, leases, and review-decision records that make "exactly one owner" true. Corrupting them needs no Gitea credential and produces duplicate or lost work. | +| A7 | The operator's checkout and worktrees | Unmerged code, branch state, and the filesystem the author tools write to. | +| A8 | The macOS login keychain | The meta-credential. Everything in A3, A4, and A5 resolves from it. | +| A9 | Separation of duty between review and merge | The property that no single actor both approves and lands a change. An *asset*, not a control, because it is what the controls exist to produce. | +| A10 | Audit and provenance records | Determine whether an incident is reconstructable. An attacker who can forge provenance makes an intrusion indistinguishable from normal work. | + +## 2. Adversaries + +| ID | Adversary | Capability assumed | Not assumed | +| -- | --------- | ------------------ | ----------- | +| ADV1 | **Compromised LLM client** | Full control of one MCP client. Issues arbitrary tool calls, in any order, with any arguments, at machine speed. Sees every tool result. | Cannot read the operator's disk except through tools; cannot execute arbitrary local code outside the tool surface. | +| ADV2 | **Prompt injection** via repository content | Controls text the model reads and treats as instruction — issue bodies, PR descriptions, review comments, commit messages, file contents. Reaches the model on any read of untrusted content. | Holds no credential and issues no call directly. Its entire power is causing an *authorized* client to act. | +| ADV3 | **Malicious tool arguments** | Supplies hostile values to any parameter — paths, branch names, session identifiers, worktree paths, issue numbers — including traversal, injection, and confusion between look-alike identifiers. | Cannot bypass a gate that actually validates its input. | +| ADV4 | **Network attacker** | Observes and modifies traffic between client, server, and Gitea. Attempts downgrade, replay, and endpoint impersonation. | Does not hold a valid credential at the start. | +| ADV5 | **Curious operator** | Legitimate local access to the workstation: process table, `/tmp`, home directory, keychain prompts. Not malicious, but not authorized for every role either. | Does not defeat the OS keychain's own access control without a prompt. | + +ADV2 is the adversary this architecture most under-models. Every other adversary must first +obtain something. Prompt injection obtains nothing: it borrows authority the client already +holds and is indistinguishable at the tool boundary from legitimate work. Each boundary +below therefore states whether it constrains ADV2 at all — and most do not, because they +authenticate the *caller*, not the *intent*. + +## 3. Trust boundaries + +"Crossing requires today" is what the code actually enforces at +`aad5c8b42361d380a8eeb07b94b90815e594c2c5`, not what the design intends. + +| ID | Boundary | Protects | Crossing requires today | Crossing must require remotely | +| -- | -------- | -------- | ----------------------- | ------------------------------ | +| B1 | LLM client ↔ MCP server session | A1, A3, A10 — that a mutating session was established through the sanctioned client path | A literal `stdio` bind (`gitea_mcp_server.py:24721`) inside a closed allowlist (`mcp_daemon_guard.py:45`, `mcp_daemon_guard.py:174`); client-managed provenance (`gitea_mcp_server.py:15412`) or a refusal (`gitea_mcp_server.py:15450`); production transport before recovery-authorization mint (`irrecoverable_provenance.py:497`, consumed at `gitea_mcp_server.py:9129` and `gitea_mcp_server.py:9378`) | An authenticated handshake issuing a server-side session identity bound to a principal, with the transport recorded in provenance. The physical proof (a pipe) must become a cryptographic one. | +| B2 | Role ↔ role | A9 — that author, reviewer, merger, and reconciler are distinct authorities | **The process boundary only.** The role is a property of the process, read once from `GITEA_MCP_PROFILE` (`gitea_config.py:54`). A caller gets author permissions by connecting to the author process. Review and merge are the operations singled out for extra care (`gitea_config.py:97`) | A per-request principal, so the role follows from the credential presented and cannot be selected by reaching a different endpoint. | +| B3 | MCP server ↔ credential store | A3, A8 — that only sanctioned code turns a profile into a token | `_keychain_token` shelling out to the login keychain (`gitea_config.py:956`), dispatched by `resolve_token` (`gitea_config.py:974`) with the reference type built at `gitea_config.py:1015`, gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:440`). Inline secrets are rejected at config load (`gitea_config.py:294`) | A credential provider keyed by the *request* principal, returning only that principal's credential, with the source recorded and the value never returned. | +| B4 | MCP server ↔ Gitea | A1, A2 — that only authorized calls reach the forge | A bearer token over TLS. Server-side, nothing distinguishes one role's token from another beyond the account it belongs to | Unchanged at the forge; the endpoint in front of it must refuse unauthenticated and plaintext connections before tool dispatch. | +| B5 | MCP server ↔ caller's filesystem | A7 — that a tool acts on the *caller's* disk or refuses | Nothing. The server's disk *is* the caller's disk. Worktree bootstrap writes directly (`gitea_mcp_server.py:10894`); the active workspace is process-global (`gitea_mcp_server.py:190`, `gitea_mcp_server.py:191`) | An explicit per-tool classification, enforced at dispatch, refusing filesystem tools over a transport that cannot reach the caller's disk. A green verdict about the wrong disk is the failure to prevent. | +| B6 | MCP server ↔ coordination state | A6, A9 — mutual exclusion | Local files and a local SQLite database, with liveness judged from the local process table (`issue_lock_store.py:98`), keyed on paths under one user's home (`issue_lock_store.py:26`, `mcp_session_state.py:27`, `control_plane_db.py:47`) and on `os.getpid()` (`control_plane_db.py:1145`, `gitea_mcp_server.py:12801`). A legacy global slot still exists at `gitea_mcp_server.py:2348`, and the session-pointer file is named per PID (`issue_lock_store.py:83`) | One authority per ownership question, with liveness from session identity and expiry, and atomic acquire, renew, and release across hosts. | +| B7 | Gitea integration ↔ unrelated integrations | A4, A5 — that a Gitea compromise is not a CI and observability compromise | **Nothing.** See §5. The Gitea server reads Jenkins and GlitchTip secrets (`gitea_config.py:851`, reached from `gitea_config.py:837`) and holds the Sentry token (`sentry_incident_bridge.py:190`) | A hard process boundary. This is the boundary #956 exists to create. | +| B8 | Tenant ↔ tenant (`prgs` / `mdcps` / `local-lab`) | A2 — that one organization's compromise is not another's | Convention. One configuration declares all three contexts; `resolve_service` fails closed on a *disabled* context (`gitea_config.py:704`) but the credentials of enabled ones remain reachable in-process. A per-profile repository scope exists (`gitea_config.py:499`) | Separate deployments, or at minimum per-tenant credential scopes with no process able to resolve both. | +| B9 | Deployed code ↔ merged policy | A1, A10 — that the running server enforces the rules that were actually merged | Comparing this process's startup commit against this disk (`master_parity_gate.py:168`), conjoined into a single verdict (`master_parity_gate.py:255`) published by `gitea_mcp_server.py:19102` | Freshness defined against the deployed build identity, with an explicit fail-closed verdict when undeterminable. | + +### What no boundary constrains + +None of B1–B9 constrains **ADV2**. Every one authenticates a caller or a process; prompt +injection supplies neither. An injected instruction that reaches an authorized author +session crosses B1, B2, B3, and B5 legitimately, because at each of those boundaries it *is* +the author. The only controls that bite ADV2 are those constraining what an authenticated +principal may do regardless of what it asks for — the per-role permission split (B2), the +repository scope at `gitea_config.py:499`, and separation of duty (A9). Sizing those +controls correctly matters more after the migration, not less, because a remote endpoint +raises the number of clients that can be injected into. + +## 4. Data flows + +Flows that cross a boundary. `==>` carries a credential; `-->` does not. + +``` + B1 B4 + [LLM client] ====================> [MCP server] ========> [Gitea] + ^ stdio pipe today | ^ (A1,A2) + | session identity | | + | after migration | | + | | | B3 + untrusted repository content | +======> [macOS login keychain] (A8) + read back into the model (ADV2) | resolves A3, A4, A5 + ^ | + +----------------------------------+ + | + B5 | B6 + [operator checkout / worktrees] <--------+-------> [locks · leases · sqlite] + (A7) | (A6) + | + B7 <-- boundary does not exist today + | + +========================+========================+ + | | | + [Jenkins] (A4) [GlitchTip] (A5) [Sentry] (A5) + external MCP server external MCP server in-process bridge +``` + +Two flows deserve attention because neither is obvious from the code: + +1. **The keychain flow fans out.** B3 is drawn once but resolves credentials for *every* + configured profile and service, not only the active one. `gitea_list_profiles` + (`gitea_mcp_server.py:19258`) reports each profile's credential status by calling + `resolve_token` on it (`gitea_mcp_server.py:19309`), and `gitea_audit_config` + (`gitea_mcp_server.py:19552`) reports service credential status through + `service_summaries` (`gitea_mcp_server.py:19574`). +2. **The return path is a flow too.** Content read from Gitea travels back into the model + and is treated as instruction. This is the ADV2 edge, and it is the only edge in the + diagram with no authentication on it, because it is not a request. + +## 5. Per-boundary credential inventory + +**14 credentials in total.** Blast radius is stated as what the credential yields *on its +own*, assuming every gate not backed by the credential itself has been bypassed — because +an attacker holding a token calls the API, not our tools. + +| ID | Credential | Holder | Boundary | Blast radius | +| -- | ---------- | ------ | -------- | ------------ | +| CR1 | `prgs-author` Gitea token — account `jcwalker3` | macOS keychain; resolved in-process (`gitea_config.py:974`) | B3 → B4 | Create branches, push, commit, open PRs, create/close/comment issues on the control-plane repo. Cannot approve or merge. The one credential whose identity is genuinely distinct. | +| CR2 | `prgs-reviewer` Gitea token — account `sysadmin` | macOS keychain | B3 → B4 | Approve and request changes. **Shares one Gitea account with CR3, CR4, CR5.** | +| CR3 | `prgs-merger` Gitea token — account `sysadmin` | macOS keychain | B3 → B4 | Merge to `master` — A1 in full. Same account as CR2. | +| CR4 | `prgs-reconciler` Gitea token — account `sysadmin` | macOS keychain | B3 → B4 | Close PRs, delete branches, irrecoverable decision-lock recovery. Same account as CR2. | +| CR5 | `prgs-controller` Gitea token — account `sysadmin` | macOS keychain | B3 → B4 | Same operation set as CR4. Same account as CR2. | +| CR6 | `mdcps-author` Gitea token — account `913443` | macOS keychain | B3 → B4, B8 | Author operations on a second organization. **Shares one account with CR7 and CR8.** | +| CR7 | `mdcps-reviewer` Gitea token — account `913443` | macOS keychain | B3 → B4, B8 | Approve and request changes on `mdcps`. Same account as CR6. | +| CR8 | `mdcps-merger` Gitea token — account `913443` | macOS keychain | B3 → B4, B8 | Merge on `mdcps` — A2 in full. Same account as CR6. | +| CR9 | MDCPS Jenkins read credential | macOS keychain, read from the Gitea server process (`gitea_config.py:851`) | B7 | Read CI jobs, builds, and logs (A4). Enabled today. | +| CR10 | MDCPS GlitchTip read credential | macOS keychain, read from the Gitea server process (`gitea_config.py:851`) | B7 | Read error events and their payloads (A5). Enabled today. | +| CR11 | `SENTRY_AUTH_TOKEN` | Process environment, read in-process (`sentry_incident_bridge.py:36`, `sentry_incident_bridge.py:190`), sent as a bearer header (`sentry_incident_bridge.py:289`) | B7 | Read and reconcile Sentry issues (A5). Not a keychain credential — an env var, so it is inherited by anything the process spawns. | +| CR12 | `SENTRY_DSN` | Process environment (`sentry_observability.py:55`) | B7 | Write events into the observability project. Low read value, real forgery value: an attacker can inject fabricated events into the record (A10). | +| CR13 | macOS login keychain access | The operator's login session; gated by `assert_keychain_access_allowed` (`mcp_daemon_guard.py:440`) | B3, ADV5 | **Every other credential in this table except CR11 and CR12.** This is the aggregation point. | +| CR14 | Coordination-store access (no secret) | Filesystem permissions — `control_plane_db.py:47`, created `0o700` (`control_plane_db.py:380`), opened with a local file lock (`control_plane_db.py:386`) | B6, ADV5 | Full read/write of locks, leases, and decision records (A6). **There is no credential here at all** — anything running as the operator can rewrite ownership. | + +### Findings + +**Finding 1 — Role separation is not credential separation.** Four `prgs` roles resolve to +one Gitea account (`sysadmin`): reviewer, merger, reconciler, and controller. A stolen +reviewer credential *is* a merger credential. A9 — separation of duty between approving and +landing — is therefore enforced entirely by which local process a call reaches (B2), and not +at all by the forge. It survives exactly as long as B2 does, and B2 is the boundary the +migration dissolves. + +**Finding 2 — The `mdcps` tenant has no role separation at all.** Author, reviewer, and +merger all resolve to account `913443`. One credential can open a PR, approve it, and merge +it. The in-process self-review check compares the authenticated username against the PR +author and would refuse — but that check runs on our side of B4. It is not a property of +the credential, and an attacker holding the token does not call our tools. + +**Finding 3 — Any one role process can resolve every other role's credential.** This is not +inferred; it is demonstrated by tool output. `gitea_list_profiles` +(`gitea_mcp_server.py:19258`) called from the **author** session reports +`identity_status: "credentials present"` for `prgs-merger`, `prgs-reviewer`, +`prgs-reconciler`, and every `mdcps` profile, because it calls `resolve_token` on each one +(`gitea_mcp_server.py:19309`). The author process does not merely *have access to* the +merger's credential — it reads it to answer a status query. B2 is not a credential boundary +in either direction. + +**Finding 4 — The Gitea server reads CI and observability secrets.** `gitea_audit_config` +(`gitea_mcp_server.py:19552`) reports `MDCPS Jenkins: enabled, read-only, authenticated`. +That word `authenticated` is produced by `service_summaries` (`gitea_mcp_server.py:19574`, +defined at `gitea_config.py:837`), whose default check calls `_keychain_token` on the +service's own keychain reference (`gitea_config.py:851`). Producing that one line requires +the Gitea MCP server to read the Jenkins secret and the GlitchTip secret out of the +keychain. B7 does not exist. + +**Finding 5 — Jenkins and GlitchTip are already decomposed; the reach is residual.** Their +tools live in separately registered servers, marked `external-mcp` +(`gitea_mcp_server.py:17707`, `gitea_mcp_server.py:17713`, `gitea_mcp_server.py:17734`, +`gitea_mcp_server.py:17739`) with their own expected tool sets (`mcp_discoverability.py:9`, +`mcp_discoverability.py:17`). The correct decomposition was already chosen. What remains is +a leak across it: the credential *references* still live in the Gitea configuration and are +still resolved by the Gitea process. #75 bundled these services into one control-plane +umbrella; the tools were separated afterwards, the credentials were not. + +**Finding 6 — Sentry is the exception that is not decomposed.** Unlike Jenkins and +GlitchTip, the Sentry bridge runs *inside* the Gitea server, resolving its token from the +process environment (`sentry_incident_bridge.py:190`) and sending it as a bearer header +(`sentry_incident_bridge.py:289`). Being an environment variable rather than a keychain item +makes it strictly worse: it needs no keychain prompt and is inherited by every subprocess the +server spawns — including the `ps` invocations at `gitea_mcp_server.py:21462` and +`gitea_mcp_server.py:21506`, reached from `gitea_mcp_server.py:21442`. + +**Finding 7 — The highest-value coordination asset has the weakest gate.** A6 is protected +by filesystem permissions alone (CR14). Corrupting a lease requires no Gitea credential, +produces no forge-side audit record, and breaks the mutual exclusion the entire workflow +assumes. Every other asset costs an attacker a credential; this one costs nothing beyond +local access, which is exactly ADV5's position. + +**Finding 8 — Provenance authenticates the launch, not the caller.** `server_provenance` is +reported as exactly `client_managed` or `manual_launch` (`gitea_mcp_server.py:19001`), +derived from environment inspection (`gitea_mcp_server.py:15412`) with the recognized-key +allowlist at `gitea_config.py:1172` and the generator that emits the marker at +`gitea_config.py:1233`. Every one of those facts is fixed at process start. A client that is +trustworthy at launch and compromised a minute later remains `client_managed` for the life +of the process, and the stdio contract that underwrites it is stated as a property of the +server itself (`mcp_server.py:4`). + +## 6. Decomposition ruling + +This section is the ruling #956 requires. It is a decision, not a recommendation. + +**D1 — No unrelated co-residency.** A single integration process **must not** hold, resolve, +or be able to resolve credentials for services it does not itself integrate with. +Concretely: the Gitea MCP service may hold Gitea credentials and nothing else. Jenkins, +GlitchTip, Sentry, and any database credential are **not permitted** to co-reside with Gitea +credentials in one process. + +*Rationale.* A process is the smallest unit an attacker takes whole. Once ADV1 or ADV2 +controls execution in a process, every credential that process can resolve is theirs, and no +in-process check helps, because the checks are in the process too. Blast radius is therefore +a property of the process boundary and nothing finer. Findings 4 and 6 show that today one +compromise of the Gitea server yields CI read access, error-tracking read access, and — via +CR13 — every role credential on both tenants. That is the single largest reduction in blast +radius available anywhere in epic #929, and it costs no new mechanism: the decomposition +already exists (Finding 5) and is merely leaked across. + +**D2 — Separation of duty must be backed by credentials.** Two roles whose separation is a +security property must not resolve to the same forge account. Specifically, reviewer and +merger must be distinct accounts. Today they are not, on either tenant (Findings 1 and 2). + +*Rationale.* B2 is a process boundary, and the migration's entire purpose is to replace +process boundaries with request-level ones. A separation enforced only by which process a +call reaches does not survive that replacement — and it is already bypassable by anyone who +holds the token and calls the API instead of the tool. + +**D3 — Credential resolution is scoped to the request principal.** A session must resolve its +own credential and must have no path to any other principal's. The resolve-every-profile +behavior behind `gitea_mcp_server.py:19309` and `gitea_mcp_server.py:19574` must report +configured-or-not from configuration alone, without resolving the secret. + +*Rationale.* Finding 3. An audit surface that proves a credential exists by fetching it is a +credential-aggregation primitive wearing a diagnostic's clothes. + +**D4 — Coordination state is a protected asset with its own authority.** Access to locks, +leases, and decision records must require an authenticated session, not merely local +filesystem access. + +*Rationale.* Finding 7. #937 already moves this store for concurrency reasons; the +authorization requirement must land with it, or the store becomes remotely reachable while +still being authorized by nothing. + +### Exceptions + +**One, time-boxed.** During the dual-run window defined by #939, the **local** stdio fleet +may continue to resolve Jenkins and GlitchTip credential *references* from the shared +configuration, because removing them from the local configuration is not a prerequisite for +standing up the remote endpoint and would strand the operator's existing local workflow. + +This exception is bounded by all of: + +- It applies to the local stdio deployment only. The remote endpoint (#938) must be + configured with Gitea credentials and no others from its first day. +- It expires when #939 completes. It does not survive cutover. +- It does not extend to Sentry: CR11 and CR12 are process-environment credentials in the + Gitea server (Finding 6) and must be absent from the remote deployment's environment + regardless of dual-run state. + +No exception is granted to D2, D3, or D4. + +### Consequences for the target architecture + +- The remote endpoint serves **Gitea only**. It is not a general control-plane endpoint. +- Jenkins and GlitchTip keep their existing separate servers, and their credential + references move out of the Gitea configuration. +- The Sentry bridge either moves behind its own service boundary or is absent from the + remote deployment. It does not travel with the Gitea server. +- Reviewer and merger accounts diverge before the endpoint is trusted for merges, or A9 is + recorded as unenforced. + +## 7. Child-to-boundary mapping + +Every #929 child from 2 through 10, mapped to the boundary it implements. A child +implementing more than one boundary names its primary first. + +| Child | Issue | Boundaries | What it must establish | Rulings it must honor | +| ----: | ----- | ---------- | ---------------------- | --------------------- | +| 2 | #931 | B1, B9 | The bound transport becomes a validated value that provenance and freshness can both key on. Without it neither B1 nor B9 has an input. | — | +| 3 | #932 | B2 | The role becomes a property of the request, not the process — the boundary the migration otherwise deletes. | D2, D3 | +| 4 | #933 | B3, B7 | Credentials come from a provider keyed by principal. This is where D1 and D3 are either enforced or permanently lost. | D1, D3 | +| 5 | #934 | B1 | Session provenance replaces pipe-and-process-table proof with an authenticated session identity. | — | +| 6 | #935 | B9 | Freshness redefined against deployed build identity, with an explicit undeterminable verdict. | — | +| 7 | #936 | B5 | Every tool classified and the filesystem boundary enforced at dispatch, so a tool cannot return green about the wrong disk. | — | +| 8 | #937 | B6 | One authority per ownership question, with session-identity liveness and atomic transitions. | D4 | +| 9 | #938 | B4, B1, B8 | The endpoint: authentication, principal binding, transport security, and — critically — the deployed credential set. | D1, D2, D3 | +| 10 | #939 | B6 | Dual-run with exactly one coordination authority at every instant, and the rollback that proves the way back. | D1 exception expiry | + +Boundary coverage: B1 (#931, #934, #938), B2 (#932), B3 (#933), B4 (#938), B5 (#936), +B6 (#937, #939), B7 (#933), B8 (#938), B9 (#931, #935). + +B7 has exactly one owner, #933, and that is deliberate. B7 is not created by standing up an +endpoint; it is created by deciding which credentials a process may resolve, which is +precisely what the credential-provider child does. + +## 8. Adversarial walkthrough: a fully compromised LLM client + +The required negative case. ADV1 holds one MCP client and issues arbitrary tool calls. What +does it reach at each boundary? + +### Before the migration + +The client is attached to the local fleet over stdio. + +| Boundary | What ADV1 reaches | Stopped by | +| -------- | ----------------- | ---------- | +| B1 | Everything the fleet serves. The client *is* the sanctioned launcher: it satisfies the client-managed check (`gitea_mcp_server.py:15412`) by construction, and provenance is never re-verified after launch (Finding 8). | Nothing. The guard authenticates the launch, not the caller. | +| B2 | All five roles — it is attached to all five namespaces. It can author a PR, approve it from the reviewer namespace, and merge it from the merger namespace. | Only the in-process self-review check, which compares `jcwalker3` (author) against `sysadmin` (reviewer) and **passes**, because Finding 1 made them different accounts while leaving reviewer and merger identical. A9 falls in one sequence of legitimate calls. | +| B3 | Every credential in CR1–CR10 via CR13, with no additional prompt — the daemon is already sanctioned, so `assert_keychain_access_allowed` (`mcp_daemon_guard.py:440`) returns immediately. | Nothing. | +| B4 | A1 and A2 in full. | Branch protection at the forge, to the extent configured. | +| B5 | The operator's checkout and every worktree, through the author tools (`gitea_mcp_server.py:10894`), plus the shared stderr path at `mcp_server.py:10`. | Nothing; the server's disk is the target disk. | +| B6 | All coordination state — no credential required (CR14). It can forge lease ownership and clear decision locks. | Filesystem permissions, which it already satisfies. | +| B7 | Jenkins (A4) and GlitchTip (A5) secrets via Finding 4, and CR11/CR12 from its own environment. | Nothing. | +| B8 | Both tenants. | Nothing in-process; only the disabled-context check (`gitea_config.py:704`), which does not apply to enabled contexts. | +| B9 | Can it run stale code? Only by advancing the checkout, which parity detects (`master_parity_gate.py:255`). | This gate holds — the one boundary that behaves as designed against ADV1. | + +**Result: total.** One compromised client yields A1 through A8 and A10. The only asset with +real resistance is A1 via branch protection, and the client holds the merger credential +anyway. Nine boundaries, one meaningful stop. + +### After the migration + +The same client authenticates to the remote endpoint with one role's credential, assuming +#931–#939 land **and honor D1–D4**. + +| Boundary | What ADV1 reaches | Stopped by | +| -------- | ----------------- | ---------- | +| B1 | One authenticated session, bound to one principal. | #934: a forged or expired session identity is refused; the client cannot mint one. | +| B2 | **One role.** Presenting the author credential yields author permissions only. | #932: the principal comes from the credential, not from which endpoint was reached. | +| B3 | **One credential — its own.** | #933 with D3: the provider resolves by principal, and no diagnostic resolves the others. | +| B4 | That role's authority on the forge. | Endpoint authentication (#938); plaintext and unauthenticated attempts refused before dispatch. | +| B5 | **Nothing.** Filesystem tools are refused over the remote transport with a named blocker. | #936. | +| B6 | Its own leases; contention resolves to exactly one winner. | #937 with D4: authenticated session required, not filesystem access. | +| B7 | **Nothing.** No CI or observability credential exists in the process. | D1 — the single largest reduction on this table. | +| B8 | One tenant. | D1 and #938: the deployment carries one tenant's credentials. | +| B9 | Cannot induce stale enforcement. | #935: explicit fail-closed verdict, including undeterminable. | + +**Result: bounded.** The compromise is contained to one role on one tenant, with no +filesystem reach and no lateral credential access. A9 survives *only if D2 lands* — if +reviewer and merger still share `sysadmin`, a compromised reviewer session still merges, and +this row reads the same after the migration as before it. + +### What the migration does not fix + +Against **ADV2**, both tables are identical. Prompt injection does not need to cross a +boundary: it arrives inside an authorized session and asks that session to do what it is +already permitted to do. Every "stopped by" above authenticates a principal, and the +injected instruction has the correct principal. The migration reduces ADV1's blast radius by +roughly an order of magnitude and reduces ADV2's by nothing. + +The controls that do constrain ADV2 are per-principal permission scope (#932), repository +scope (`gitea_config.py:499`), and credential-backed separation of duty (D2) — each limiting +what an authenticated session may do *regardless of what it is asked for*. #955's +secure-isolation end state should be read with that distinction in mind: removing credentials +from clients defeats ADV1 and ADV5, and does not by itself defeat ADV2. + +Two further items are explicitly out of scope here and unowned by #929: + +- **Session-credential rotation and revocation.** #938 names rotation as documentation, but + no child owns proving that a revoked credential stops an in-flight session. +- **ADV3** (malicious tool arguments) is diffused across every child rather than owned. The + per-request principal work in #932 is the natural place to assert that identifiers taken + from the request never authorize anything on their own. + +## 9. How to verify this document + +1. `PYTHONPATH=. pytest tests/test_issue_956_threat_model.py` — resolves every anchor + against the working tree and checks the document's structural obligations. +2. Pick any five anchors at random and read them; the fixture states what each line must + contain. +3. Reproduce Findings 3 and 4 live: call `gitea_list_profiles` and `gitea_audit_config` + from the **author** namespace. Credential presence reported for roles other than the + active one is Finding 3; `MDCPS Jenkins: enabled, read-only, authenticated` is Finding 4. + +If the anchor test fails after an unrelated refactor, the anchors moved and the fixture +needs regenerating — the claims are still true, but they are no longer traceable, which +#956 treats as the same defect. diff --git a/tests/test_issue_956_threat_model.py b/tests/test_issue_956_threat_model.py new file mode 100644 index 0000000..6a67ac3 --- /dev/null +++ b/tests/test_issue_956_threat_model.py @@ -0,0 +1,323 @@ +"""Validation tooling for the remote-MCP threat model (#956). + +#956 requires that "every boundary claim [is] traceable to a file and line +anchor that resolves at the reviewed commit". A prose document cannot enforce +that about itself, and #930 demonstrated the failure mode: its inventory cited +``gitea_mcp_server.py`` anchors generated at ``7bf4f125`` which no longer point +at the described code at ``aad5c8b4``. Nothing failed, because nothing checked. + +These tests are that check. They enforce, in both directions: + +* every ``file.py:NNN`` anchor cited in the prose is declared in the fixture; +* every declared anchor resolves — the file exists, the line exists, and the + source line actually contains the substring the fixture claims for it; +* the document's structural obligations (assets, adversaries, boundaries, + credential rows, the co-residency ruling, and the child mapping) are present + and internally consistent. + +A refactor that shifts a line number therefore breaks the suite instead of +silently rotting the security documentation. +""" + +import json +import os +import re +import unittest + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +DOC_PATH = os.path.join(REPO_ROOT, "docs", "remote-mcp", "threat-model.md") +FIXTURE_PATH = os.path.join( + REPO_ROOT, "docs", "remote-mcp", "threat-model-anchors.json" +) + +# ``module.py:123`` as it appears inside markdown inline code spans. +ANCHOR_RE = re.compile(r"`([A-Za-z0-9_./-]+\.py):(\d+)`") + +# The epic children this document must map to a boundary (#929 children 2-10). +REQUIRED_CHILDREN = [931, 932, 933, 934, 935, 936, 937, 938, 939] + +# The adversaries #956 names explicitly. +REQUIRED_ADVERSARIES = [ + "compromised LLM client", + "prompt injection", + "malicious tool arguments", + "network attacker", + "curious operator", +] + + +def _read(path): + with open(path, "r", encoding="utf-8") as fh: + return fh.read() + + +def _heading_re(title): + """Match a level-2 heading by title, with or without section numbering. + + The document numbers its sections ('## 6. Decomposition ruling'), so an + exact-substring assertion would break on renumbering without the document + having actually lost anything. + """ + return re.compile( + r"^##\s+(?:\d+\.\s+)?" + re.escape(title), re.MULTILINE + ) + + +def _section_body(doc, title): + """Return the text of section *title*, bounded by the next level-2 heading. + + Bounding matters: an unbounded slice runs to end-of-document, so the + walkthrough tables in a later section leak into the child-to-boundary + mapping and satisfy its coverage check with rows that assign no owner. + """ + match = _heading_re(title).search(doc) + if match is None: + return None + rest = doc[match.end():] + nxt = re.search(r"^##\s", rest, re.MULTILINE) + return rest[: nxt.start()] if nxt else rest + + +def _source_line(rel_path, lineno): + """Return the 1-based *lineno* of *rel_path*, or None if out of range.""" + abs_path = os.path.join(REPO_ROOT, rel_path) + if not os.path.exists(abs_path): + return None + with open(abs_path, "r", encoding="utf-8", errors="replace") as fh: + for idx, line in enumerate(fh, start=1): + if idx == lineno: + return line + return None + + +class ThreatModelFixtureTests(unittest.TestCase): + """The fixture itself must be well-formed before it can prove anything.""" + + def setUp(self): + self.fixture = json.loads(_read(FIXTURE_PATH)) + + def test_fixture_declares_a_generation_commit(self): + sha = self.fixture.get("generated_against_commit") or "" + self.assertRegex( + sha, + r"^[0-9a-f]{40}$", + "the fixture must record the full commit its anchors were taken at", + ) + + def test_fixture_anchors_are_unique_and_well_formed(self): + seen = set() + for entry in self.fixture["anchors"]: + anchor = entry["anchor"] + self.assertNotIn(anchor, seen, f"duplicate anchor entry: {anchor}") + seen.add(anchor) + self.assertRegex(anchor, r"^[A-Za-z0-9_./-]+\.py:[1-9]\d*$", anchor) + self.assertTrue( + (entry.get("expect") or "").strip(), + f"anchor {anchor} declares no 'expect' substring, so it proves nothing", + ) + + +class ThreatModelAnchorResolutionTests(unittest.TestCase): + """#956 required positive test: every anchor resolves at the reviewed commit.""" + + def setUp(self): + self.fixture = json.loads(_read(FIXTURE_PATH)) + self.doc = _read(DOC_PATH) + + def test_every_declared_anchor_resolves_to_the_claimed_source_line(self): + failures = [] + for entry in self.fixture["anchors"]: + rel_path, _, raw_lineno = entry["anchor"].partition(":") + lineno = int(raw_lineno) + line = _source_line(rel_path, lineno) + if line is None: + failures.append(f"{entry['anchor']}: file or line does not exist") + continue + if entry["expect"] not in line: + failures.append( + f"{entry['anchor']}: expected {entry['expect']!r}, " + f"found {line.strip()!r}" + ) + self.assertEqual( + [], failures, "unresolved threat-model anchors:\n" + "\n".join(failures) + ) + + def test_every_anchor_cited_in_the_document_is_declared_in_the_fixture(self): + declared = {e["anchor"] for e in self.fixture["anchors"]} + cited = {f"{m.group(1)}:{m.group(2)}" for m in ANCHOR_RE.finditer(self.doc)} + undeclared = sorted(cited - declared) + self.assertEqual( + [], + undeclared, + "document cites anchors that no test verifies: " + ", ".join(undeclared), + ) + + def test_the_document_actually_cites_anchors(self): + cited = {f"{m.group(1)}:{m.group(2)}" for m in ANCHOR_RE.finditer(self.doc)} + self.assertGreaterEqual( + len(cited), + 30, + "a boundary document with almost no anchors is not traceable", + ) + + def test_unresolvable_anchor_is_detected(self): + """Negative control: the checker must fail on a deliberately bad anchor. + + Without this, a checker that silently passed everything would look + identical to a correct one. + """ + self.assertIsNone(_source_line("gitea_config.py", 10**9)) + self.assertIsNone(_source_line("no_such_module_for_956.py", 1)) + real = _source_line("gitea_config.py", 54) + self.assertIsNotNone(real) + self.assertNotIn("this substring is not on that line", real) + + +class ThreatModelStructureTests(unittest.TestCase): + """The document must contain what #956's acceptance criteria demand.""" + + def setUp(self): + self.doc = _read(DOC_PATH) + + def test_records_the_commit_it_was_generated_against(self): + fixture = json.loads(_read(FIXTURE_PATH)) + self.assertIn( + fixture["generated_against_commit"], + self.doc, + "the document must state the commit its anchors resolve at", + ) + + def test_names_every_required_adversary(self): + low = self.doc.lower() + for adversary in REQUIRED_ADVERSARIES: + self.assertIn(adversary.lower(), low, f"adversary not covered: {adversary}") + + def test_maps_every_epic_child_from_two_through_ten(self): + for number in REQUIRED_CHILDREN: + self.assertIn( + f"#{number}", + self.doc, + f"epic child #{number} is not mapped to a boundary", + ) + + def test_credential_rows_declare_holder_boundary_and_blast_radius(self): + for column in ("Holder", "Boundary", "Blast radius"): + self.assertIn( + column, + self.doc, + f"the credential inventory must state each credential's {column.lower()}", + ) + + def test_states_an_explicit_co_residency_ruling(self): + """AC3/AC5: an explicit ruling, not an implication.""" + self.assertIsNotNone( + _heading_re("Decomposition ruling").search(self.doc), + "the document must contain an explicit decomposition-ruling section", + ) + for service in ("Jenkins", "GlitchTip", "Sentry", "database"): + self.assertIn(service, self.doc, f"ruling does not address {service}") + self.assertRegex( + self.doc, + r"D1\b.*must not", + "the ruling must state the prohibition, not merely discuss it", + ) + + def test_contains_the_compromised_client_walkthrough(self): + """#956 required negative/adversarial test.""" + self.assertIsNotNone( + _heading_re("Adversarial walkthrough").search(self.doc), + "the required compromised-client walkthrough is missing", + ) + self.assertIn("Before the migration", self.doc) + self.assertIn("After the migration", self.doc) + + def test_every_boundary_states_what_it_protects_and_what_crossing_requires(self): + boundary_ids = set(re.findall(r"\bB(\d+)\b", self.doc)) + self.assertGreaterEqual( + len(boundary_ids), 5, "too few trust boundaries to be a decomposition" + ) + for column in ( + "Protects", + "Crossing requires today", + "Crossing must require remotely", + ): + self.assertIn(column, self.doc, f"boundary table is missing '{column}'") + + def test_declares_itself_documentation_only(self): + self.assertIn("documentation only", self.doc.lower()) + + +class ThreatModelConsistencyTests(unittest.TestCase): + """Counts stated in prose must match the rows actually present.""" + + def setUp(self): + self.doc = _read(DOC_PATH) + + def _declared_ids(self, prefix): + # Table rows begin '| CR1 |' / '| B3 |' / '| A2 |'. + return sorted( + { + int(m) + for m in re.findall( + r"^\|\s*%s(\d+)\s*\|" % prefix, self.doc, re.MULTILINE + ) + } + ) + + def test_identifier_sequences_have_no_gaps(self): + for prefix, label in ( + ("A", "assets"), + ("B", "boundaries"), + ("CR", "credentials"), + ): + ids = self._declared_ids(prefix) + self.assertTrue(ids, f"no {label} declared") + self.assertEqual( + list(range(1, len(ids) + 1)), + ids, + f"{label} identifiers must run 1..n with no gaps; got {ids}", + ) + + def test_stated_credential_count_matches_the_rows(self): + ids = self._declared_ids("CR") + match = re.search(r"(\d+)\s+credential(?:s)? in total", self.doc) + self.assertIsNotNone(match, "the credential inventory must state its own total") + self.assertEqual( + len(ids), + int(match.group(1)), + "stated credential total disagrees with the number of rows", + ) + + def test_every_boundary_is_owned_by_at_least_one_child(self): + """Each boundary must be owned by a child *in the mapping table*. + + Scanning the whole section would let a prose summary line ("Boundary + coverage: ... B5 (#936)") satisfy the assertion while the table row + that actually assigns the owner had been emptied — verified by + deliberately blanking a row and watching a whole-section check still + pass. Only table rows count. + """ + mapping_section = _section_body(self.doc, "Child-to-boundary mapping") + self.assertIsNotNone( + mapping_section, "child-to-boundary mapping section is missing" + ) + rows = [ + line + for line in mapping_section.splitlines() + if line.lstrip().startswith("|") and re.search(r"#93\d", line) + ] + self.assertGreaterEqual( + len(rows), len(REQUIRED_CHILDREN), "mapping table has too few child rows" + ) + mapped = set(re.findall(r"\bB(\d+)\b", "\n".join(rows))) + declared = {str(i) for i in self._declared_ids("B")} + unmapped = sorted(declared - mapped, key=int) + self.assertEqual( + [], + unmapped, + "boundaries with no owning child: " + ", ".join("B" + u for u in unmapped), + ) + + +if __name__ == "__main__": + unittest.main()