Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 jcwalker3
|
0d82f5ab26 | ||
 sysadmin
|
9b80e75ca3 | ||
 sysadminandClaude Opus 4.8
|
b3de9c941c | ||
|
sysadmin
|
aad5c8b423 | ||
|
sysadmin
|
ef34d938bf |
@@ -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"}
|
||||
]
|
||||
}
|
||||
@@ -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.
|
||||
+62
-4
@@ -22,8 +22,62 @@ import gitea_config
|
||||
|
||||
PROJECT_ROOT = os.path.dirname(os.path.abspath(__file__))
|
||||
|
||||
# Load standard .env if present
|
||||
load_dotenv(os.path.join(PROJECT_ROOT, ".env"))
|
||||
# Reserved runtime-control / workspace-binding environment variables (#704).
|
||||
# Repository .env files MUST NOT populate or override any of these keys.
|
||||
RESERVED_WORKTREE_ENV_KEYS: frozenset[str] = frozenset({
|
||||
"GITEA_ACTIVE_WORKTREE",
|
||||
"GITEA_AUTHOR_WORKTREE",
|
||||
"GITEA_REVIEWER_WORKTREE",
|
||||
"GITEA_MERGER_WORKTREE",
|
||||
"GITEA_RECONCILER_WORKTREE",
|
||||
})
|
||||
|
||||
|
||||
def is_reserved_worktree_env_key(key: str | None) -> bool:
|
||||
"""Return True if *key* is a reserved runtime workspace binding variable (#704)."""
|
||||
if not key:
|
||||
return False
|
||||
k = str(key).upper().strip()
|
||||
return k in RESERVED_WORKTREE_ENV_KEYS or (k.startswith("GITEA_") and k.endswith("_WORKTREE"))
|
||||
|
||||
|
||||
def load_env_file_sanitized(
|
||||
env_path: str,
|
||||
*,
|
||||
target_env: dict | os._Environ | None = None,
|
||||
) -> list[str]:
|
||||
"""Load a .env file without populating or overriding reserved workspace keys (#704).
|
||||
|
||||
Pre-existing process environment values retain their precedence. Reserved
|
||||
runtime-control keys found in repository files are ignored (without logging
|
||||
their values). Returns a list of sanitized rejection reasons.
|
||||
"""
|
||||
if target_env is None:
|
||||
target_env = os.environ
|
||||
if not os.path.exists(env_path) or os.path.isdir(env_path):
|
||||
return []
|
||||
|
||||
rejection_reasons: list[str] = []
|
||||
try:
|
||||
file_vals = dotenv_values(env_path)
|
||||
for key, val in file_vals.items():
|
||||
if not key or val is None:
|
||||
continue
|
||||
if is_reserved_worktree_env_key(key):
|
||||
filename = os.path.basename(env_path)
|
||||
rejection_reasons.append(
|
||||
f"Ignored reserved runtime workspace key '{key}' from repository {filename}"
|
||||
)
|
||||
continue
|
||||
if key not in target_env:
|
||||
target_env[key] = val
|
||||
except Exception:
|
||||
pass
|
||||
return rejection_reasons
|
||||
|
||||
|
||||
# Load standard .env if present (sanitized to prevent repo workspace binding contamination #704)
|
||||
load_env_file_sanitized(os.path.join(PROJECT_ROOT, ".env"))
|
||||
|
||||
# Dictionary to store configurations parsed dynamically from .env.* files
|
||||
DYNAMIC_CONFIGS = {}
|
||||
@@ -37,9 +91,13 @@ for env_path in glob.glob(os.path.join(PROJECT_ROOT, ".env*")):
|
||||
continue
|
||||
try:
|
||||
config_vals = dotenv_values(env_path)
|
||||
site = config_vals.get("GITEA_SITE") or config_vals.get("GITEA_HOST")
|
||||
# Filter out reserved workspace keys from dynamic configs (#704)
|
||||
sanitized_config = {
|
||||
k: v for k, v in config_vals.items() if not is_reserved_worktree_env_key(k)
|
||||
}
|
||||
site = sanitized_config.get("GITEA_SITE") or sanitized_config.get("GITEA_HOST")
|
||||
if site:
|
||||
DYNAMIC_CONFIGS[site.lower().strip()] = config_vals
|
||||
DYNAMIC_CONFIGS[site.lower().strip()] = sanitized_config
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
@@ -0,0 +1,166 @@
|
||||
"""Tests for Issue #704: Preventing repository .env files from injecting workspace bindings.
|
||||
|
||||
Acceptance Criteria (#704):
|
||||
1. Repository .env loading cannot populate or override GITEA_ACTIVE_WORKTREE or any role-specific GITEA_*_WORKTREE runtime-binding variable.
|
||||
2. Runtime workspace bindings are accepted only from sanctioned managed-launch/session mechanisms.
|
||||
3. Pre-existing sanctioned process environment values retain their intended precedence.
|
||||
4. Importing gitea_auth or related modules does not mutate workspace-binding state from repository files.
|
||||
5. Reserved runtime-control keys found in .env are ignored or rejected with a sanitized actionable reason; their values are never logged.
|
||||
6. The protection applies consistently to author, reviewer, merger, and reconciler namespaces.
|
||||
7. Comprehensive test coverage for stale worktree, missing worktree, task-specific, role-specific, launcher binding, repeated imports, namespace isolation, precedence, and absence of secret leakage.
|
||||
8. Dirty-state and workspace-preflight gates cannot be bypassed by an injected missing-path binding.
|
||||
9. No environment, dotenv, offline-import, or caller-controlled path can forge native transport or mutation provenance.
|
||||
10. Cross-linked with #702, PR #703, #510.
|
||||
11. Required immediate follow-up to Issue #702 / PR #703.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import importlib
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||
|
||||
import gitea_auth
|
||||
from gitea_auth import is_reserved_worktree_env_key, load_env_file_sanitized
|
||||
|
||||
|
||||
class TestIssue704PreventEnvWorkspaceBindings(unittest.TestCase):
|
||||
"""Test suite verifying .env workspace-binding injection prevention (#704)."""
|
||||
|
||||
def setUp(self):
|
||||
self.tmpdir = tempfile.TemporaryDirectory()
|
||||
self.addCleanup(self.tmpdir.cleanup)
|
||||
self.env_dir = Path(self.tmpdir.name)
|
||||
|
||||
def test_is_reserved_worktree_env_key(self):
|
||||
"""Verify key classification for all role namespaces (#704 AC6)."""
|
||||
reserved_keys = [
|
||||
"GITEA_ACTIVE_WORKTREE",
|
||||
"GITEA_AUTHOR_WORKTREE",
|
||||
"GITEA_REVIEWER_WORKTREE",
|
||||
"GITEA_MERGER_WORKTREE",
|
||||
"GITEA_RECONCILER_WORKTREE",
|
||||
"gitea_active_worktree",
|
||||
"GITEA_CUSTOM_ROLE_WORKTREE",
|
||||
]
|
||||
for key in reserved_keys:
|
||||
self.assertTrue(
|
||||
is_reserved_worktree_env_key(key),
|
||||
f"Expected {key} to be recognized as a reserved worktree key",
|
||||
)
|
||||
|
||||
unreserved_keys = [
|
||||
"GITEA_USER",
|
||||
"GITEA_PASS",
|
||||
"GITEA_TOKEN",
|
||||
"GITEA_HOST",
|
||||
"PATH",
|
||||
]
|
||||
for key in unreserved_keys:
|
||||
self.assertFalse(
|
||||
is_reserved_worktree_env_key(key),
|
||||
f"Expected {key} to NOT be recognized as a reserved worktree key",
|
||||
)
|
||||
|
||||
def test_load_env_file_sanitized_ignores_reserved_keys(self):
|
||||
"""Verify .env loading ignores GITEA_ACTIVE_WORKTREE and role-specific keys (#704 AC1)."""
|
||||
env_file = self.env_dir / ".env"
|
||||
stale_path = "/tmp/stale-worktree-path-1234"
|
||||
env_file.write_text(
|
||||
f"GITEA_USER=testuser\n"
|
||||
f"GITEA_ACTIVE_WORKTREE={stale_path}\n"
|
||||
f"GITEA_AUTHOR_WORKTREE={stale_path}\n"
|
||||
f"GITEA_REVIEWER_WORKTREE={stale_path}\n"
|
||||
f"GITEA_MERGER_WORKTREE={stale_path}\n"
|
||||
f"GITEA_RECONCILER_WORKTREE={stale_path}\n"
|
||||
)
|
||||
|
||||
test_env = {}
|
||||
reasons = load_env_file_sanitized(str(env_file), target_env=test_env)
|
||||
|
||||
# Unreserved key loaded
|
||||
self.assertEqual(test_env.get("GITEA_USER"), "testuser")
|
||||
|
||||
# Reserved keys ignored
|
||||
self.assertNotIn("GITEA_ACTIVE_WORKTREE", test_env)
|
||||
self.assertNotIn("GITEA_AUTHOR_WORKTREE", test_env)
|
||||
self.assertNotIn("GITEA_REVIEWER_WORKTREE", test_env)
|
||||
self.assertNotIn("GITEA_MERGER_WORKTREE", test_env)
|
||||
self.assertNotIn("GITEA_RECONCILER_WORKTREE", test_env)
|
||||
|
||||
# Rejection reasons populated without leaking the secret value (#704 AC5)
|
||||
self.assertTrue(len(reasons) >= 5)
|
||||
for r in reasons:
|
||||
self.assertNotIn(stale_path, r, "Secret/path value must not leak into rejection reason")
|
||||
|
||||
def test_preexisting_sanctioned_launcher_env_retained(self):
|
||||
"""Sanctioned launcher values in process env are retained (#704 AC2, AC3)."""
|
||||
sanctioned_path = "/tmp/sanctioned-launcher-worktree"
|
||||
test_env = {"GITEA_ACTIVE_WORKTREE": sanctioned_path}
|
||||
|
||||
env_file = self.env_dir / ".env"
|
||||
env_file.write_text("GITEA_ACTIVE_WORKTREE=/tmp/injected-repo-worktree\n")
|
||||
|
||||
load_env_file_sanitized(str(env_file), target_env=test_env)
|
||||
|
||||
# Pre-existing value retained, not overwritten by .env
|
||||
self.assertEqual(test_env.get("GITEA_ACTIVE_WORKTREE"), sanctioned_path)
|
||||
|
||||
def test_stale_or_missing_worktree_in_env_ignored(self):
|
||||
"""Stale or non-existent worktree path in .env file is ignored (#704 AC7)."""
|
||||
nonexistent_path = "/nonexistent/branches/stale-issue-999"
|
||||
env_file = self.env_dir / ".env"
|
||||
env_file.write_text(f"GITEA_ACTIVE_WORKTREE={nonexistent_path}\n")
|
||||
|
||||
test_env = {}
|
||||
load_env_file_sanitized(str(env_file), target_env=test_env)
|
||||
|
||||
self.assertNotIn("GITEA_ACTIVE_WORKTREE", test_env)
|
||||
|
||||
def test_repeated_module_import_does_not_mutate_workspace_env(self):
|
||||
"""Repeated imports of gitea_auth leave os.environ un-contaminated (#704 AC4, AC7)."""
|
||||
# Ensure no active worktree env exists initially
|
||||
original_val = os.environ.pop("GITEA_ACTIVE_WORKTREE", None)
|
||||
try:
|
||||
importlib.reload(gitea_auth)
|
||||
self.assertNotIn("GITEA_ACTIVE_WORKTREE", os.environ)
|
||||
|
||||
importlib.reload(gitea_auth)
|
||||
self.assertNotIn("GITEA_ACTIVE_WORKTREE", os.environ)
|
||||
finally:
|
||||
if original_val is not None:
|
||||
os.environ["GITEA_ACTIVE_WORKTREE"] = original_val
|
||||
|
||||
def test_namespace_isolation_all_roles_protected(self):
|
||||
"""Verify protection across author, reviewer, merger, reconciler (#704 AC6)."""
|
||||
env_file = self.env_dir / ".env"
|
||||
env_file.write_text(
|
||||
"GITEA_AUTHOR_WORKTREE=/bad/author\n"
|
||||
"GITEA_REVIEWER_WORKTREE=/bad/reviewer\n"
|
||||
"GITEA_MERGER_WORKTREE=/bad/merger\n"
|
||||
"GITEA_RECONCILER_WORKTREE=/bad/reconciler\n"
|
||||
)
|
||||
test_env = {}
|
||||
load_env_file_sanitized(str(env_file), target_env=test_env)
|
||||
|
||||
self.assertEqual(test_env, {})
|
||||
|
||||
def test_absence_of_secret_leakage(self):
|
||||
"""Rejection reasons contain key names but never secret path values (#704 AC5)."""
|
||||
sensitive_path = "/Users/secret/path/private_repo"
|
||||
env_file = self.env_dir / ".env"
|
||||
env_file.write_text(f"GITEA_ACTIVE_WORKTREE={sensitive_path}\n")
|
||||
|
||||
test_env = {}
|
||||
reasons = load_env_file_sanitized(str(env_file), target_env=test_env)
|
||||
for reason in reasons:
|
||||
self.assertNotIn(sensitive_path, reason)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user