diff --git a/config.example.toml b/config.example.toml index 0c34b34..2a558bc 100644 --- a/config.example.toml +++ b/config.example.toml @@ -65,6 +65,11 @@ enabled = false # key = "aws:secretsmanager:ghpool/mcp-keys:openab" # or env:GHPOOL_KEY_OPENAB # # keys = ["env:GHPOOL_KEY_OPENAB", "env:GHPOOL_KEY_OPENAB_NEXT"] # rotation # tools = ["issue_read", "list_issues", "pull_request_read"] +# # Repository allowlist: "owner/repo" exact or "owner/*" wildcard. +# # Non-empty = every tools/call must resolve to a listed repo from its +# # arguments; calls with no repo target (e.g. search_code, get_me) are +# # DENIED. Empty/omitted = no repository restriction. +# repos = ["openabdev/ghpool", "oablab/*"] # # [[mcp.agents]] # id = "ci-agent" diff --git a/src/config.rs b/src/config.rs index a406fed..422f92e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -99,6 +99,12 @@ pub struct McpAgentConfig { /// proxy; the same list is injected upstream as X-MCP-Tools. #[serde(default)] pub tools: Vec, + /// Repository allowlist: `owner/repo` (exact) or `owner/*` entries. + /// When non-empty, every tools/call must resolve to an allowlisted repo + /// from its arguments; calls with no resolvable repo target are DENIED + /// (deny-if-unresolvable). Empty = no repository restriction. + #[serde(default)] + pub repos: Vec, } impl Default for McpConfig { diff --git a/src/main.rs b/src/main.rs index cb0f6ad..44d527d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,7 @@ mod cache; mod config; mod mcp; +mod policy; mod pool; use axum::{ diff --git a/src/mcp.rs b/src/mcp.rs index 7921a0b..1f2720e 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -92,36 +92,89 @@ pub async fn mcp_proxy( Err(code) => return rpc_error(code, "no upstream identity available"), }; - // Audit log + default-deny tool allowlist (single frame parse) + // Audit log + policy enforcement (single frame parse) if method == Method::POST { - if let Some((rpc_method, tool)) = frame_summary(&body) { - // Phase 2a enforcement: authenticated agents may only call tools - // on their allowlist. This is the authoritative check; the - // injected X-MCP-Tools header is defense-in-depth. - if rpc_method == "tools/call" { - if let (Some(agent), Some(tool_name)) = (agent, tool.as_deref()) { - if !agent.tools.iter().any(|t| t == tool_name) { - tracing::warn!( - "MCP tools/call {} DENIED [agent={}]{}", - tool_name, agent.id, session_suffix(session_id.as_deref()) - ); - return rpc_error( - StatusCode::FORBIDDEN, - "tool not permitted by agent policy", - ); + if let Some(frame) = parse_frame(&body) { + if frame.method == "tools/call" { + if let Some(tool_name) = frame.tool.as_deref() { + let repo = crate::policy::resolve_repo(frame.arguments.as_ref()); + if let Some(agent) = agent { + // 1. Default-deny tool allowlist (authoritative) + if !agent.tools.iter().any(|t| t == tool_name) { + tracing::warn!( + "MCP tools/call {} DENIED (not on allowlist) [agent={}]{}", + tool_name, agent.id, session_suffix(session_id.as_deref()) + ); + return rpc_error( + StatusCode::FORBIDDEN, + "tool not permitted by agent policy", + ); + } + // 2. Write classification: ALL write-classified tools + // are blocked until the write path ships (2b-5). + // Unknown tools classify as writes (conservative). + if crate::policy::classify_tool(tool_name) == crate::policy::ToolKind::Write { + tracing::warn!( + "MCP tools/call {} DENIED (write tools not enabled) [agent={}]{}", + tool_name, agent.id, session_suffix(session_id.as_deref()) + ); + return rpc_error( + StatusCode::FORBIDDEN, + "write tools are not enabled", + ); + } + // 3. Repository allowlist (deny-if-unresolvable) + if !agent.repos.is_empty() { + match &repo { + None => { + tracing::warn!( + "MCP tools/call {} DENIED (no resolvable repo target) [agent={}]{}", + tool_name, agent.id, session_suffix(session_id.as_deref()) + ); + return rpc_error( + StatusCode::FORBIDDEN, + "call has no resolvable repository target", + ); + } + Some((owner, repo_name)) => { + if !crate::policy::repo_allowed(&agent.repos, owner, repo_name) { + tracing::warn!( + "MCP tools/call {} DENIED (repo {}/{} not allowlisted) [agent={}]{}", + tool_name, owner, repo_name, agent.id, + session_suffix(session_id.as_deref()) + ); + return rpc_error( + StatusCode::FORBIDDEN, + "repository not permitted by agent policy", + ); + } + } + } + } } + let repo_suffix = repo + .map(|(o, r)| format!(" repo={}/{}", o, r)) + .unwrap_or_default(); + tracing::info!( + "MCP tools/call {}{} [{}]{}", + tool_name, repo_suffix, + audit_via(agent, &identity.id), + session_suffix(session_id.as_deref()) + ); + } else { + tracing::info!( + "MCP tools/call [{}]{}", + audit_via(agent, &identity.id), + session_suffix(session_id.as_deref()) + ); } - } - let via = audit_via(agent, &identity.id); - match tool { - Some(t) => tracing::info!( - "MCP {} {} [{}]{}", - rpc_method, t, via, session_suffix(session_id.as_deref()) - ), - None => tracing::info!( + } else { + tracing::info!( "MCP {} [{}]{}", - rpc_method, via, session_suffix(session_id.as_deref()) - ), + frame.method, + audit_via(agent, &identity.id), + session_suffix(session_id.as_deref()) + ); } } } @@ -383,20 +436,31 @@ fn build_upstream_headers( Some(h) } -/// Best-effort parse of a JSON-RPC request frame. -/// Returns (method, tool_name) where tool_name is set for tools/call. -fn frame_summary(body: &[u8]) -> Option<(String, Option)> { +/// A best-effort parse of a JSON-RPC request frame. +struct Frame { + method: String, + /// Tool name, for `tools/call` frames. + tool: Option, + /// Tool arguments, for `tools/call` frames. + arguments: Option, +} + +fn parse_frame(body: &[u8]) -> Option { let v: serde_json::Value = serde_json::from_slice(body).ok()?; let method = v.get("method")?.as_str()?.to_string(); - let tool = if method == "tools/call" { - v.get("params") - .and_then(|p| p.get("name")) - .and_then(|n| n.as_str()) - .map(str::to_string) + let (tool, arguments) = if method == "tools/call" { + let params = v.get("params"); + ( + params + .and_then(|p| p.get("name")) + .and_then(|n| n.as_str()) + .map(str::to_string), + params.and_then(|p| p.get("arguments")).cloned(), + ) } else { - None + (None, None) }; - Some((method, tool)) + Some(Frame { method, tool, arguments }) } fn session_suffix(session_id: Option<&str>) -> String { @@ -427,6 +491,14 @@ mod tests { key: None, keys: vec![key.to_string()], tools: tools.iter().map(|s| s.to_string()).collect(), + repos: Vec::new(), + } + } + + fn pin(identity: &str, agent: Option<&str>) -> SessionPin { + SessionPin { + identity_id: identity.to_string(), + agent_id: agent.map(str::to_string), } } @@ -476,25 +548,27 @@ mod tests { } #[test] - fn test_frame_summary_tools_call() { - let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"get_issue","arguments":{"owner":"openabdev"}}}"#; - let (method, tool) = frame_summary(body).unwrap(); - assert_eq!(method, "tools/call"); - assert_eq!(tool.as_deref(), Some("get_issue")); + fn test_parse_frame_tools_call() { + let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"get_issue","arguments":{"owner":"openabdev","repo":"ghpool"}}}"#; + let f = parse_frame(body).unwrap(); + assert_eq!(f.method, "tools/call"); + assert_eq!(f.tool.as_deref(), Some("get_issue")); + assert_eq!(f.arguments.unwrap()["owner"], "openabdev"); } #[test] - fn test_frame_summary_initialize() { + fn test_parse_frame_initialize() { let body = br#"{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"protocolVersion":"2025-06-18"}}"#; - let (method, tool) = frame_summary(body).unwrap(); - assert_eq!(method, "initialize"); - assert!(tool.is_none()); + let f = parse_frame(body).unwrap(); + assert_eq!(f.method, "initialize"); + assert!(f.tool.is_none()); + assert!(f.arguments.is_none()); } #[test] - fn test_frame_summary_invalid() { - assert!(frame_summary(b"not json").is_none()); - assert!(frame_summary(br#"{"jsonrpc":"2.0","id":1,"result":{}}"#).is_none()); + fn test_parse_frame_invalid() { + assert!(parse_frame(b"not json").is_none()); + assert!(parse_frame(br#"{"jsonrpc":"2.0","id":1,"result":{}}"#).is_none()); } #[test] @@ -1126,4 +1200,152 @@ mod tests { .unwrap(); assert_eq!(resp.status(), StatusCode::FORBIDDEN); } + + // ---- 2b-2: write classification + repo allowlists ---- + + fn agent_with_repos( + id: &str, + key: &str, + tools: &[&str], + repos: &[&str], + ) -> config::McpAgentConfig { + let mut a = agent(id, key, tools); + a.repos = repos.iter().map(|s| s.to_string()).collect(); + a + } + + #[tokio::test] + async fn test_write_tool_blocked_even_when_allowlisted() { + // Operator mistake: create_issue on the allowlist while writes are + // not enabled → still 403, never reaches upstream. + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent("bot-a", "key-a", &["issue_read", "create_issue"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"create_issue","arguments":{"owner":"openabdev","repo":"ghpool","title":"x"}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX).await.unwrap(); + let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert!(v["error"]["message"].as_str().unwrap().contains("write tools")); + assert!(captured.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn test_repo_allowlist_allows_matching_repo() { + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent_with_repos("bot-a", "key-a", &["issue_read"], &["openabdev/ghpool"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"issue_read","arguments":{"owner":"openabdev","repo":"ghpool","issue_number":15}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(captured.lock().unwrap().len(), 1); + } + + #[tokio::test] + async fn test_repo_allowlist_denies_other_repo() { + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent_with_repos("bot-a", "key-a", &["issue_read"], &["openabdev/ghpool"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"issue_read","arguments":{"owner":"openabdev","repo":"openab","issue_number":1}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + assert!(captured.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn test_repo_allowlist_denies_unresolvable_target() { + // search_code has no owner/repo arguments → deny-if-unresolvable + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent_with_repos("bot-a", "key-a", &["search_code"], &["openabdev/*"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"search_code","arguments":{"query":"foo"}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX).await.unwrap(); + let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert!(v["error"]["message"].as_str().unwrap().contains("no resolvable repository")); + assert!(captured.lock().unwrap().is_empty()); + } + + #[tokio::test] + async fn test_repo_allowlist_wildcard_owner() { + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent_with_repos("bot-a", "key-a", &["issue_read"], &["openabdev/*"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"issue_read","arguments":{"owner":"openabdev","repo":"anything","issue_number":1}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(captured.lock().unwrap().len(), 1); + } + + #[tokio::test] + async fn test_empty_repos_is_unrestricted_and_search_passes() { + // Backward compat: 2a-style agent (no repos) can use repo-less tools + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full( + &["alice"], &url, &[], + vec![agent("bot-a", "key-a", &["search_code"])], + ); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"search_code","arguments":{"query":"foo"}}}"#, + &[("x-ghpool-key", "key-a")], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(captured.lock().unwrap().len(), 1); + } + + #[tokio::test] + async fn test_phase1_mode_unaffected_by_write_classification() { + // No agents → Phase 1 network-trust mode: the readonly upstream is + // the write barrier; ghpool does not block (backward compatible). + let (url, captured) = spawn_mock_upstream().await; + let state = test_state_full(&["alice"], &url, &[], vec![]); + let resp = mcp_app(state) + .oneshot(post_frame( + r#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"create_issue","arguments":{"owner":"o","repo":"r","title":"t"}}}"#, + &[], + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(captured.lock().unwrap().len(), 1); + } } diff --git a/src/policy.rs b/src/policy.rs new file mode 100644 index 0000000..d030b97 --- /dev/null +++ b/src/policy.rs @@ -0,0 +1,158 @@ +//! MCP tool policy: read/write classification and repository-level +//! authorization (Phase 2b, #17). +//! +//! Classification is rule-based and deliberately conservative: a tool is a +//! READ only if its name matches the read-only surface's naming conventions +//! (verified against the hosted server's /readonly toolset); everything +//! else — including tools we have never seen — is treated as a WRITE. +//! Misclassifying a read as a write is an inconvenience (fix the rule); +//! misclassifying a write as a read would be a security bug. + +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum ToolKind { + Read, + Write, +} + +/// Classify a tool by name. +/// +/// The hosted /readonly surface (observed in #22) consists entirely of +/// `get_*`, `list_*`, `search_*` and `*_read` names. Write-capable tools use +/// action prefixes (`create_*`, `update_*`, `delete_*`, `add_*`, `merge_*`, +/// `push_*`, …) or `*_write` names — none of which match the read rules. +pub fn classify_tool(name: &str) -> ToolKind { + if name.starts_with("get_") + || name.starts_with("list_") + || name.starts_with("search_") + || name.ends_with("_read") + { + ToolKind::Read + } else { + ToolKind::Write + } +} + +/// Extract the target repository from tools/call arguments. +/// GitHub's MCP tools consistently use `owner` + `repo` argument names. +/// Returns None when the call has no resolvable repository target (e.g. +/// `get_me`, cross-repo search) — for repo-restricted agents such calls are +/// DENIED (deny-if-unresolvable, RFC Revision 2). +pub fn resolve_repo(arguments: Option<&serde_json::Value>) -> Option<(String, String)> { + let args = arguments?; + let owner = args.get("owner")?.as_str()?.trim(); + let repo = args.get("repo")?.as_str()?.trim(); + if owner.is_empty() || repo.is_empty() { + return None; + } + Some((owner.to_string(), repo.to_string())) +} + +/// Check a resolved repo against an agent's allowlist. +/// Supported entry forms (case-insensitive, GitHub semantics): +/// - `owner/repo` — exact +/// - `owner/*` — any repo under that owner +/// +/// An EMPTY allowlist means "no repository restriction" — repos is an +/// additional constraint on top of the tool allowlist, and requiring it +/// would break existing agent configs. +pub fn repo_allowed(allowlist: &[String], owner: &str, repo: &str) -> bool { + if allowlist.is_empty() { + return true; + } + let owner = owner.to_lowercase(); + let repo = repo.to_lowercase(); + allowlist.iter().any(|entry| { + let entry = entry.to_lowercase(); + match entry.split_once('/') { + Some((o, "*")) => o == owner, + Some((o, r)) => o == owner && r == repo, + None => false, // malformed entry never matches (fail closed) + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_classify_read_tools() { + // The observed /readonly surface shapes + for name in [ + "issue_read", + "pull_request_read", + "get_file_contents", + "get_me", + "get_commit", + "list_issues", + "list_branches", + "search_code", + ] { + assert_eq!(classify_tool(name), ToolKind::Read, "{}", name); + } + } + + #[test] + fn test_classify_write_tools() { + for name in [ + "create_issue", + "add_issue_comment", + "update_issue", + "merge_pull_request", + "push_files", + "delete_file", + "fork_repository", + "issue_write", + "create_or_update_file", + ] { + assert_eq!(classify_tool(name), ToolKind::Write, "{}", name); + } + } + + #[test] + fn test_classify_unknown_is_write() { + // Conservative default: never-seen names are writes + assert_eq!(classify_tool("frobnicate_widget"), ToolKind::Write); + assert_eq!(classify_tool(""), ToolKind::Write); + } + + #[test] + fn test_resolve_repo() { + let args = serde_json::json!({"owner": "openabdev", "repo": "ghpool", "issue_number": 15}); + assert_eq!( + resolve_repo(Some(&args)), + Some(("openabdev".to_string(), "ghpool".to_string())) + ); + + // unresolvable shapes + assert_eq!(resolve_repo(None), None); + assert_eq!(resolve_repo(Some(&serde_json::json!({}))), None); + assert_eq!(resolve_repo(Some(&serde_json::json!({"owner": "x"}))), None); + assert_eq!(resolve_repo(Some(&serde_json::json!({"owner": "", "repo": "y"}))), None); + assert_eq!(resolve_repo(Some(&serde_json::json!({"owner": 1, "repo": 2}))), None); + // query-only tools have no repo target + assert_eq!(resolve_repo(Some(&serde_json::json!({"query": "foo"}))), None); + } + + #[test] + fn test_repo_allowed_exact_and_wildcard() { + let allow = vec!["openabdev/ghpool".to_string(), "oablab/*".to_string()]; + assert!(repo_allowed(&allow, "openabdev", "ghpool")); + assert!(repo_allowed(&allow, "OpenABdev", "GHPool")); // case-insensitive + assert!(repo_allowed(&allow, "oablab", "chi")); + assert!(repo_allowed(&allow, "oablab", "anything")); + assert!(!repo_allowed(&allow, "openabdev", "openab")); + assert!(!repo_allowed(&allow, "evil", "ghpool")); + } + + #[test] + fn test_repo_allowed_empty_is_unrestricted() { + assert!(repo_allowed(&[], "anyone", "anything")); + } + + #[test] + fn test_repo_allowed_malformed_entry_fails_closed() { + let allow = vec!["justanowner".to_string()]; + assert!(!repo_allowed(&allow, "justanowner", "repo")); + } +}