From 18f245f900aa44d7ac46db98e26707a4919ebe2d Mon Sep 17 00:00:00 2001 From: ModernOps888 <69460199+ModernOps888@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:46:49 +0100 Subject: [PATCH] Fix RBAC self-assertion bypass, harden dashboard/cache/config, make stdio handshake timeout configurable (#20) Security fixes: - RBAC self-assertion bypass (HIGH): dispatch_real_tool no longer falls back to a client-supplied _mcplex_role param; role now comes exclusively from the server-verified trusted_role set by the auth middleware. - Dashboard /api/* routes now support optional gateway.dashboard_api_key auth. - Tool response cache is now partitioned by caller role to prevent cross-tenant leakage in multi-key deployments. - validate_config rejects api_key/dashboard_api_key/api_keys that resolve to an empty string (e.g. an unset ${ENV_VAR}), which previously turned "auth required" into a silent no-op. - Expanded audit log redaction list (pwd, auth, cookie, session, bearer). - Startup warning when RBAC is enabled with no api_key/api_keys configured. Fixes #20: the stdio MCP handshake (initialize) and steady-state tool calls now use independent, configurable timeouts (security.default_handshake_timeout_secs / servers[].handshake_timeout_secs vs. security.request_timeout_secs), so slow-cold-start servers no longer need to loosen the steady-state timeout. Also implements "0 = no timeout" for both, previously documented but unused. Bumps version to 0.7.1, updates README/CHANGELOG, and adds a cargo audit step to CI. 38 tests passing (up from 32). Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 11 ++++ CHANGELOG.md | 18 +++++ Cargo.toml | 2 +- README.md | 42 +++++++++++- src/config.rs | 128 +++++++++++++++++++++++++++++++++++- src/main.rs | 27 +++++++- src/observe/dashboard.rs | 45 ++++++++++++- src/protocol/cache.rs | 76 ++++++++++++++------- src/protocol/multiplexer.rs | 20 +++++- src/protocol/stdio.rs | 72 +++++++++++++++----- src/protocol/transport.rs | 35 +++++----- src/security/allowlist.rs | 2 + src/security/audit.rs | 5 ++ src/security/mod.rs | 40 +++++++++++ 14 files changed, 460 insertions(+), 63 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f71231..185ee19 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,17 @@ jobs: - name: Run tests run: cargo test --all-features + audit: + name: Supply-chain audit + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + - uses: taiki-e/install-action@cargo-audit + - name: cargo audit + run: cargo audit + build: name: Build (${{ matrix.target }}) needs: test diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d3c3ac..87cc773 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,23 @@ All notable changes to MCPlex are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.7.1] โ€” 2026-07-21 + +### ๐Ÿ”’ Security +- **RBAC self-assertion bypass (HIGH)** โ€” `dispatch_real_tool` fell back to a client-supplied `_mcplex_role` request param whenever no server-verified role was established (i.e. no `api_key`/`api_keys` configured). Any caller could self-assert `role: "admin"` and bypass RBAC entirely. Role now comes exclusively from the auth middleware's server-verified `trusted_role` โ€” the client-controlled fallback has been removed. +- **Dashboard authentication** โ€” `/api/*` dashboard routes had no authentication, only permissive CORS. Added optional `gateway.dashboard_api_key`; when set, all dashboard API routes require a matching `Authorization: Bearer` or `X-API-Key` header (constant-time compared). Unset by default for backward compatibility, with a startup warning recommending a localhost-only bind in that case. +- **Cross-tenant cache leakage** โ€” the tool response cache was keyed only by tool name + arguments, so in a multi-key deployment one tenant's cached response could be served to another. Cache entries are now partitioned by caller role. +- **Empty-string secrets from unset env vars** โ€” `${ENV_VAR}` expansion silently produced an empty string for unset variables, which could make `gateway.api_key`/`dashboard_api_key`/`api_keys` resolve to `""` and turn "auth required" into a no-op. `validate_config` now rejects empty-string keys/roles at startup instead of silently bypassing auth. +- **Audit log redaction list expanded** โ€” added `pwd`, `auth`, `cookie`, `session`, `bearer` to the sensitive-key redaction list used before writing audit log entries. +- **RBAC-without-auth warning** โ€” startup now warns when `security.enable_rbac = true` but no `api_key`/`api_keys` are configured, since every tool call will correctly deny-by-default in that configuration (previously silent). + +### โœจ Features +- **Configurable stdio handshake timeout** (fixes [#20](https://github.com/ModernOps888/mcplex/issues/20)) โ€” the initial stdio MCP handshake (`initialize`) previously used a hardcoded 30s timeout shared with steady-state tool calls, causing spurious failures for servers with a slow cold start (e.g. `npx` resolving/downloading a package on first run). The handshake timeout is now independently configurable via `security.default_handshake_timeout_secs` (global default, 30s) and `servers[].handshake_timeout_secs` (per-server override), fully decoupled from `security.request_timeout_secs`, which now applies only to steady-state requests once connected. +- **`0 = no timeout` is now implemented** for both `request_timeout_secs` and `default_handshake_timeout_secs` โ€” previously documented but never actually honored anywhere in the code; a `0` value now waits indefinitely instead of timing out immediately. + +### ๐Ÿงช Testing +- 38 tests passing (up from 32) โ€” new regression tests for the RBAC self-assertion fix, role-partitioned cache, and handshake-timeout config parsing/defaults. + ## [0.7.0] โ€” 2026-07-21 ### ๐Ÿค– Model Ecosystem Refresh (July 2026) @@ -144,6 +161,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - CLI with `--config`, `--verbose`, `--listen`, `--dashboard`, and `--check` options - MIT licensed +[0.7.1]: https://github.com/ModernOps888/mcplex/compare/v0.7.0...v0.7.1 [0.7.0]: https://github.com/ModernOps888/mcplex/compare/v0.6.0...v0.7.0 [0.6.0]: https://github.com/ModernOps888/mcplex/compare/v0.5.0...v0.6.0 [0.5.0]: https://github.com/ModernOps888/mcplex/compare/v0.4.0...v0.5.0 diff --git a/Cargo.toml b/Cargo.toml index a90aea1..1bcdf3a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mcplex" -version = "0.7.0" +version = "0.7.1" edition = "2021" description = "MCPlex - The MCP Smart Gateway. Semantic tool routing, security guardrails, and real-time observability for AI agents." license = "MIT" diff --git a/README.md b/README.md index 8cfa735..e522cf8 100644 --- a/README.md +++ b/README.md @@ -422,6 +422,8 @@ allowed_tools = ["*/list_*", "*/get_*"] blocked_tools = ["*/delete_*", "*/drop_*"] ``` +> **Role trust boundary:** a caller's role is only ever established server-side, from a verified `api_key`/`api_keys` match โ€” never from anything a client sends in the request body. When `enable_rbac = true` and no `api_key`/`api_keys` are configured, every tool call is denied by default (no role can be established), and the gateway logs a startup warning so this isn't mistaken for a bug. + ### Per-Server Tool Blocklists ```toml @@ -457,6 +459,13 @@ Built-in observability dashboard at `http://localhost:9090`: Glassmorphism design with animated gradients. Auto-refreshes every 3 seconds. Zero configuration. +By default the dashboard's `/api/*` routes are unauthenticated โ€” fine for `127.0.0.1`-only binds, but if you expose the dashboard beyond localhost, set `gateway.dashboard_api_key` so requests need an `Authorization: Bearer ` (or `X-API-Key`) header: + +```toml +[gateway] +dashboard_api_key = "${MCPLEX_DASHBOARD_API_KEY}" +``` + ## ๐Ÿ“ฆ Response Caching Avoid redundant upstream calls for read-only tools: @@ -468,7 +477,7 @@ ttl_seconds = 300 # 5 minute TTL max_entries = 1000 # Max cached responses ``` -MCPlex **auto-detects** read-only tools by prefix (`list_*`, `get_*`, `search_*`, `query_*`, `describe_*`, `show_*`). You can override with custom patterns: +Cached responses are partitioned by caller role, so a cache entry from one role/tenant is never served to another. MCPlex **auto-detects** read-only tools by prefix (`list_*`, `get_*`, `search_*`, `query_*`, `describe_*`, `show_*`). You can override with custom patterns: ```toml [cache] @@ -527,6 +536,7 @@ MCPlex aggregates and forwards **all three** MCP capability types from upstream | `hot_reload` | bool | `true` | Auto-reload config on file change | | `name` | string | `mcplex` | Gateway instance name | | `api_key` | string | โ€” | API key for client auth (supports `${ENV}`) | +| `dashboard_api_key` | string | โ€” | API key for the dashboard `/api/*` routes (supports `${ENV}`). If unset, the dashboard is unauthenticated โ€” bind it to `127.0.0.1` in that case. | | `rate_limit_rps` | int | `0` | Max requests/sec per client (0 = unlimited) | ### `[router]` @@ -549,7 +559,8 @@ MCPlex aggregates and forwards **all three** MCP capability types from upstream | `max_log_size_mb` | int | `100` | Max log file size before rotation (keeps 5 backups) | | `circuit_breaker_max_crashes` | int | `5` | Max crashes within the window before respawns are blocked | | `circuit_breaker_window_secs` | int | `60` | Sliding window (seconds) for crash counting | -| `request_timeout_secs` | int | `30` | Per-server request timeout for HTTP upstream calls (0 = no timeout) | +| `request_timeout_secs` | int | `30` | Steady-state per-request timeout for HTTP and stdio upstream calls, once connected (0 = no timeout) | +| `default_handshake_timeout_secs` | int | `30` | Default timeout for the initial stdio MCP handshake (`initialize`), before a server is considered failed and enters the respawn loop. Overridable per server โ€” see `servers[].handshake_timeout_secs` below. (0 = no timeout) | ### `[[servers]]` @@ -564,11 +575,22 @@ MCPlex aggregates and forwards **all three** MCP capability types from upstream | `blocked_tools` | list | โ€” | Tool blocklist patterns (glob) | | `allowed_tools` | list | โ€” | Tool allowlist patterns (glob) | | `enabled` | bool | `true` | Enable/disable this server | +| `handshake_timeout_secs` | int | โ€” | Per-server override for the initial stdio handshake timeout. Falls back to `security.default_handshake_timeout_secs` when unset. Useful for servers with a slow cold start, e.g. `npx` resolving/downloading a package on first run (0 = no timeout) | โšก = One of `command` or `url` is required > **Note:** For stdio servers, `command` should be the **executable path** (e.g. `npx`, `/usr/bin/python3`). Additional arguments go in the `args` array. +> **Handshake timeout vs. request timeout:** the initial stdio `initialize` handshake and steady-state `tools/call` requests use two independent timeouts. A slow-starting server (e.g. `npx` fetching a package on first run) can be given a longer `handshake_timeout_secs` without loosening the timeout applied to every later tool call: +> ```toml +> [[servers]] +> name = "slow-npx-server" +> command = "npx" +> args = ["-y", "@some/mcp-server"] +> handshake_timeout_secs = 90 # default 30s, 0 = no timeout +> ``` + + ### `[roles.]` | Key | Type | Description | @@ -763,7 +785,18 @@ Contributions are welcome! Please: MIT License โ€” see [LICENSE](LICENSE) for details. -## ๐Ÿ”ง Recent Changes (v0.7.0 โ€” Model Ecosystem Refresh) +## ๐Ÿ”ง Recent Changes (v0.7.1 โ€” Security Hardening & Configurable Handshake Timeout) + +- **RBAC Self-Assertion Bypass Fixed (HIGH)** โ€” `dispatch_real_tool` previously fell back to a client-supplied `_mcplex_role` request param whenever no server-verified role was established (e.g. no `api_key`/`api_keys` configured). Any caller could self-assert `role: "admin"` and bypass RBAC entirely. The role now comes exclusively from the auth middleware's server-verified `trusted_role`. +- **Dashboard Authentication** โ€” `/api/*` dashboard routes were previously unauthenticated. Added optional `gateway.dashboard_api_key`; when set, dashboard routes require a matching `Authorization: Bearer`/`X-API-Key` header. Unset by default (backward compatible), with a startup warning to bind localhost-only in that case. +- **Role-Partitioned Cache** โ€” The tool response cache is now partitioned by caller role, closing a cross-tenant leakage gap in multi-key deployments. +- **Empty-Secret Rejection** โ€” `validate_config` now rejects `gateway.api_key`/`dashboard_api_key`/`api_keys` that resolve to an empty string (e.g. from an unset `${ENV_VAR}`), instead of silently turning "auth required" into a no-op. +- **Configurable Stdio Handshake Timeout** (fixes [#20](https://github.com/ModernOps888/mcplex/issues/20)) โ€” the initial `initialize` handshake and steady-state tool calls now use independent timeouts: `security.default_handshake_timeout_secs` (global, default 30s) and `servers[].handshake_timeout_secs` (per-server override) for the handshake, vs. `security.request_timeout_secs` for steady-state calls. Fixes spurious failures on slow-cold-start servers (e.g. `npx` downloading a package on first run). `0 = no timeout` is now actually implemented for both. +- **Audit Redaction List Expanded** โ€” added `pwd`, `auth`, `cookie`, `session`, `bearer` to the sensitive-key redaction list. +- **38 tests passing** (up from 32) โ€” new regression tests for the RBAC fix, cache partitioning, and handshake-timeout config. + +
+Previous (v0.7.0 โ€” Model Ecosystem Refresh) - **Compatible Models Dashboard Panel** โ€” Built-in dashboard now shows a `๐Ÿค– Compatible Models` panel with colour-coded provider badges for all 9 active frontier models: GPT-5.6 Sol/Terra/Luna, Claude Fable 5/Mythos 5/Sonnet 5, Gemini 3.5 Flash/3.1 Pro, Grok 4.5. - **Ecosystem Footer** โ€” Dashboard footer now links to AgentLens, The Forge, and GitHub with the current MCP protocol version displayed. @@ -773,6 +806,8 @@ MIT License โ€” see [LICENSE](LICENSE) for details. - **Fixed** Python example protocol version `2025-03-26` โ†’ `2025-11-25`. - **Fixed** missing CHANGELOG comparison links for v0.4.0โ€“v0.6.0. +
+
Previous (v0.6.0 โ€” Audit Hardening & Denial Telemetry) @@ -787,6 +822,7 @@ MIT License โ€” see [LICENSE](LICENSE) for details. + - **Constant-Time API Key Verification** โ€” Key comparison is no longer vulnerable to timing side-channels. - **Trusted Role Binding** โ€” Multi-tenant API keys now bind their RBAC role at the middleware layer (`X-MCPlex-Role` injected server-side). Clients can no longer self-assert a role via `_mcplex_role` when authenticated. - **Tool Call Input Validation** โ€” Tool names are restricted to a safe charset (max 128 chars); arguments are capped at 64KB with a max JSON nesting depth of 16. Malformed calls are rejected before touching upstream servers. diff --git a/src/config.rs b/src/config.rs index 170a575..9b6d4f1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -49,6 +49,10 @@ pub struct GatewayConfig { /// Maximum requests per second per client (0 = unlimited) #[serde(default)] pub rate_limit_rps: u32, + /// API key for the observability dashboard (optional โ€” if unset, the + /// dashboard is unauthenticated and should only be bound to localhost). + /// Supports ${ENV_VAR} expansion, e.g. "${MCPLEX_DASHBOARD_API_KEY}" + pub dashboard_api_key: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -93,9 +97,18 @@ pub struct SecurityConfig { /// Circuit breaker: sliding window duration in seconds #[serde(default = "default_cb_window_secs")] pub circuit_breaker_window_secs: u64, - /// Per-server request timeout in seconds for HTTP upstream calls (0 = no timeout) + /// Per-server request timeout in seconds for HTTP upstream calls, and the + /// steady-state per-request timeout for stdio upstream calls once + /// connected (0 = no timeout). See `default_handshake_timeout_secs` for + /// the separate timeout that covers the initial stdio handshake. #[serde(default = "default_request_timeout")] pub request_timeout_secs: u64, + /// Default timeout in seconds for the initial stdio MCP handshake + /// (`initialize`) before a server is considered failed and enters the + /// respawn loop. Can be overridden per server via + /// `servers[].handshake_timeout_secs`. 0 = no timeout. + #[serde(default = "default_handshake_timeout")] + pub default_handshake_timeout_secs: u64, } /// Cache configuration for tool response caching @@ -157,6 +170,12 @@ pub struct ServerConfig { /// Whether this server is enabled #[serde(default = "default_true")] pub enabled: bool, + /// Override for how long the initial stdio MCP handshake (`initialize`) + /// may take before this server is considered failed and enters the + /// respawn loop. Falls back to `security.default_handshake_timeout_secs` + /// when unset. Useful for servers with a slow cold start (e.g. `npx` + /// resolving/downloading a package on first run). 0 = no timeout. + pub handshake_timeout_secs: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -242,6 +261,9 @@ fn default_cb_window_secs() -> u64 { fn default_request_timeout() -> u64 { 30 } +fn default_handshake_timeout() -> u64 { + 30 +} fn default_agentlens_url() -> String { "http://127.0.0.1:3000/api/ingest".to_string() } @@ -321,6 +343,7 @@ impl Default for SecurityConfig { circuit_breaker_max_crashes: default_cb_max_crashes(), circuit_breaker_window_secs: default_cb_window_secs(), request_timeout_secs: default_request_timeout(), + default_handshake_timeout_secs: default_handshake_timeout(), } } } @@ -407,6 +430,43 @@ fn validate_config(config: &AppConfig) -> anyhow::Result<()> { anyhow::bail!("router.similarity_threshold must be between 0.0 and 1.0"); } + // An api_key/dashboard_api_key set to "${SOME_UNSET_ENV_VAR}" expands to an + // empty string rather than None โ€” silently turning "auth required" into + // "auth is a no-op" (an empty bearer token would match). Treat that as a + // hard config error rather than a silent bypass. + if config.gateway.api_key.as_deref() == Some("") { + anyhow::bail!( + "gateway.api_key resolved to an empty string โ€” check that its ${{ENV_VAR}} is set" + ); + } + if config.gateway.dashboard_api_key.as_deref() == Some("") { + anyhow::bail!( + "gateway.dashboard_api_key resolved to an empty string โ€” check that its ${{ENV_VAR}} is set" + ); + } + for (key, cfg) in &config.api_keys { + if key.is_empty() { + anyhow::bail!("api_keys contains an empty key โ€” check its ${{ENV_VAR}} is set"); + } + if cfg.role.is_empty() { + anyhow::bail!("api_keys.\"{}\".role must not be empty", key); + } + } + + // RBAC only has an effect when the gateway can determine a caller's role, + // which requires api_key/api_keys auth. Without it, is_tool_allowed() + // correctly denies every call by default (see SecurityEngine) โ€” but that + // makes RBAC-enabled-with-no-auth look like "the gateway rejects + // everything" rather than a misconfiguration, so warn loudly at startup. + if config.security.enable_rbac && config.gateway.api_key.is_none() && config.api_keys.is_empty() + { + warn!( + "โš ๏ธ security.enable_rbac is true but no gateway.api_key/api_keys are configured โ€” \ + every tool call will be denied by default since no caller role can be established. \ + Configure api_key/api_keys, or disable RBAC." + ); + } + Ok(()) } @@ -484,3 +544,69 @@ pub async fn watch_config(config_path: &str, state: Arc) -> any Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression test for GitHub issue #20: the handshake timeout must be + /// independently configurable per-server, and default to 30s globally, + /// distinct from the steady-state `request_timeout_secs`. + #[test] + fn handshake_timeout_defaults_and_per_server_override_parse() { + let toml_str = r#" + [gateway] + + [security] + request_timeout_secs = 15 + default_handshake_timeout_secs = 45 + + [[servers]] + name = "slow-npx-server" + command = "npx" + handshake_timeout_secs = 90 + + [[servers]] + name = "fast-server" + command = "some-binary" + "#; + + let config: AppConfig = toml::from_str(toml_str).expect("config must parse"); + + assert_eq!(config.security.request_timeout_secs, 15); + assert_eq!(config.security.default_handshake_timeout_secs, 45); + assert_eq!( + config.servers[0].handshake_timeout_secs, + Some(90), + "per-server override must be honored" + ); + assert_eq!( + config.servers[1].handshake_timeout_secs, None, + "server without an override must fall back to the global default at use-site" + ); + } + + #[test] + fn security_config_defaults_handshake_timeout_to_30s() { + let config: AppConfig = + toml::from_str("[gateway]\n").expect("minimal config with defaults must parse"); + assert_eq!(config.security.default_handshake_timeout_secs, 30); + assert_eq!(config.security.request_timeout_secs, 30); + } + + #[test] + fn empty_expanded_api_key_is_rejected() { + let mut config: AppConfig = + toml::from_str("[gateway]\n").expect("minimal config with defaults must parse"); + config.gateway.api_key = Some(String::new()); + assert!(validate_config(&config).is_err()); + } + + #[test] + fn empty_expanded_dashboard_api_key_is_rejected() { + let mut config: AppConfig = + toml::from_str("[gateway]\n").expect("minimal config with defaults must parse"); + config.gateway.dashboard_api_key = Some(String::new()); + assert!(validate_config(&config).is_err()); + } +} diff --git a/src/main.rs b/src/main.rs index 879e5cf..674d9e0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -593,7 +593,32 @@ async fn dead_server_monitor(state: Arc, mut death_rx: DeathReceiver) attempt, MAX_RESPAWN_ATTEMPTS, name_for_respawn, delay ); - match protocol::stdio::StdioConnection::connect(&config, death_tx.clone()).await { + let request_timeout = std::time::Duration::from_secs( + state_for_respawn + .config + .read() + .await + .security + .request_timeout_secs, + ); + let handshake_timeout = std::time::Duration::from_secs( + config.handshake_timeout_secs.unwrap_or( + state_for_respawn + .config + .read() + .await + .security + .default_handshake_timeout_secs, + ), + ); + match protocol::stdio::StdioConnection::connect( + &config, + death_tx.clone(), + request_timeout, + handshake_timeout, + ) + .await + { Ok((conn, capabilities)) => { let tools_restored = { let mut mux = state_for_respawn.multiplexer.write().await; diff --git a/src/observe/dashboard.rs b/src/observe/dashboard.rs index e05bb55..d863c22 100644 --- a/src/observe/dashboard.rs +++ b/src/observe/dashboard.rs @@ -9,8 +9,9 @@ use axum::{ }; use std::sync::Arc; use tower_http::cors::CorsLayer; -use tracing::info; +use tracing::{info, warn}; +use crate::protocol::transport::{constant_time_eq, extract_api_key}; use crate::AppState; // v0.4.0: Import export module to wire up previously-dead Prometheus endpoint use crate::observe::export; @@ -21,6 +22,15 @@ pub struct DashboardServer; impl DashboardServer { /// Start the dashboard HTTP server pub async fn start(addr: &str, state: Arc) -> anyhow::Result<()> { + let dashboard_api_key = state.config.read().await.gateway.dashboard_api_key.clone(); + + if dashboard_api_key.is_none() { + warn!( + "๐Ÿ“Š Dashboard has no api_key configured โ€” it is unauthenticated. \ + Bind it to localhost only, or set gateway.dashboard_api_key." + ); + } + let app = Router::new() .route("/", get(serve_dashboard)) .route("/api/metrics", get(api_metrics)) @@ -30,6 +40,10 @@ impl DashboardServer { .route("/api/config", get(api_config)) // v0.4.0: Prometheus-compatible metrics endpoint (was defined but never mounted) .route("/api/prometheus", get(api_prometheus)) + .layer(axum::middleware::from_fn_with_state( + dashboard_api_key, + dashboard_auth_middleware, + )) .layer(CorsLayer::permissive()) .with_state(state); @@ -41,6 +55,35 @@ impl DashboardServer { } } +/// Gate every dashboard route behind `gateway.dashboard_api_key` when configured. +/// No-op (all requests pass through) when the key is unset, matching the +/// pre-existing default of an open dashboard for local/dev use. +async fn dashboard_auth_middleware( + State(expected_key): State>, + request: axum::extract::Request, + next: axum::middleware::Next, +) -> axum::response::Response { + let Some(expected_key) = expected_key else { + return next.run(request).await; + }; + + let authorized = extract_api_key(&request) + .map(|provided| constant_time_eq(&provided, &expected_key)) + .unwrap_or(false); + + if !authorized { + return ( + axum::http::StatusCode::UNAUTHORIZED, + Json(serde_json::json!({ + "error": "Unauthorized โ€” provide dashboard API key via Authorization: Bearer or X-API-Key header" + })), + ) + .into_response(); + } + + next.run(request).await +} + /// Serve the dashboard HTML async fn serve_dashboard() -> impl IntoResponse { Html(DASHBOARD_HTML.replace("__MCPLEX_VERSION__", env!("CARGO_PKG_VERSION"))) diff --git a/src/protocol/cache.rs b/src/protocol/cache.rs index ace70c2..1874d00 100644 --- a/src/protocol/cache.rs +++ b/src/protocol/cache.rs @@ -44,13 +44,14 @@ impl ToolCache { pub fn get( &self, tool_name: &str, + role: Option<&str>, arguments: &Option, ) -> Option { if !self.is_cacheable(tool_name) { return None; } - let key = Self::cache_key(tool_name, arguments); + let key = Self::cache_key(tool_name, role, arguments); if let Ok(mut entries) = self.entries.write() { if let Some(entry) = entries.get_mut(&key) { @@ -72,6 +73,7 @@ impl ToolCache { pub fn put( &self, tool_name: &str, + role: Option<&str>, arguments: &Option, value: serde_json::Value, ) { @@ -79,7 +81,7 @@ impl ToolCache { return; } - let key = Self::cache_key(tool_name, arguments); + let key = Self::cache_key(tool_name, role, arguments); if let Ok(mut entries) = self.entries.write() { // Evict if at capacity โ€” remove oldest expired entries first @@ -176,15 +178,24 @@ impl ToolCache { }) } - /// Generate a deterministic cache key from tool name + arguments - fn cache_key(tool_name: &str, arguments: &Option) -> String { + /// Generate a deterministic cache key from tool name + caller role + arguments. + /// Partitioning by role prevents a cached response fetched under one + /// API key/role from leaking to a caller with a different identity in + /// multi-tenant deployments. `tool_name` stays the leading segment so + /// `invalidate()`'s prefix match still works across all roles. + fn cache_key( + tool_name: &str, + role: Option<&str>, + arguments: &Option, + ) -> String { match arguments { Some(args) => format!( - "{}:{}", + "{}:{}:{}", tool_name, + role.unwrap_or(""), serde_json::to_string(args).unwrap_or_default() ), - None => format!("{}:()", tool_name), + None => format!("{}:{}:()", tool_name, role.unwrap_or("")), } } } @@ -208,14 +219,14 @@ mod tests { let args = Some(serde_json::json!({"filter": "active"})); // Miss - assert!(cache.get("list_tables", &args).is_none()); + assert!(cache.get("list_tables", None, &args).is_none()); // Put let result = serde_json::json!({"tables": ["users", "orders"]}); - cache.put("list_tables", &args, result.clone()); + cache.put("list_tables", None, &args, result.clone()); // Hit - let cached = cache.get("list_tables", &args); + let cached = cache.get("list_tables", None, &args); assert!(cached.is_some()); assert_eq!(cached.unwrap(), result); } @@ -228,8 +239,8 @@ mod tests { let args = Some(serde_json::json!({"name": "test"})); let result = serde_json::json!({"id": 1}); - cache.put("create_issue", &args, result); - assert!(cache.get("create_issue", &args).is_none()); + cache.put("create_issue", None, &args, result); + assert!(cache.get("create_issue", None, &args).is_none()); } #[test] @@ -237,11 +248,11 @@ mod tests { let cache = ToolCache::new(0, 100, vec![]); // 0 second TTL let args = None; - cache.put("list_tables", &args, serde_json::json!({})); + cache.put("list_tables", None, &args, serde_json::json!({})); // Should be expired immediately std::thread::sleep(std::time::Duration::from_millis(10)); - assert!(cache.get("list_tables", &args).is_none()); + assert!(cache.get("list_tables", None, &args).is_none()); } #[test] @@ -252,27 +263,46 @@ mod tests { let val = serde_json::json!("ok"); // Exact match - cache.put("my_tool", &args, val.clone()); - assert!(cache.get("my_tool", &args).is_some()); + cache.put("my_tool", None, &args, val.clone()); + assert!(cache.get("my_tool", None, &args).is_some()); // Glob match - cache.put("custom_query", &args, val.clone()); - assert!(cache.get("custom_query", &args).is_some()); + cache.put("custom_query", None, &args, val.clone()); + assert!(cache.get("custom_query", None, &args).is_some()); // Non-match (list_ wouldn't match with custom patterns set) - cache.put("list_tables", &args, val); - assert!(cache.get("list_tables", &args).is_none()); + cache.put("list_tables", None, &args, val); + assert!(cache.get("list_tables", None, &args).is_none()); } #[test] fn test_invalidation() { let cache = ToolCache::new(60, 100, vec![]); - cache.put("list_tables", &None, serde_json::json!({"a": 1})); - cache.put("list_repos", &None, serde_json::json!({"b": 2})); + cache.put("list_tables", None, &None, serde_json::json!({"a": 1})); + cache.put("list_repos", None, &None, serde_json::json!({"b": 2})); cache.invalidate("list_tables"); - assert!(cache.get("list_tables", &None).is_none()); - assert!(cache.get("list_repos", &None).is_some()); + assert!(cache.get("list_tables", None, &None).is_none()); + assert!(cache.get("list_repos", None, &None).is_some()); + } + + #[test] + fn test_role_partitioned_cache() { + let cache = ToolCache::new(60, 100, vec![]); + let args = Some(serde_json::json!({"id": 1})); + + // tenant "admin" stores a result... + cache.put( + "get_record", + Some("admin"), + &args, + serde_json::json!({"secret": true}), + ); + + // ...a different role with the same tool+args must NOT see it + assert!(cache.get("get_record", Some("viewer"), &args).is_none()); + // the same role does see its own cached result + assert!(cache.get("get_record", Some("admin"), &args).is_some()); } } diff --git a/src/protocol/multiplexer.rs b/src/protocol/multiplexer.rs index 8486d92..f73ae86 100644 --- a/src/protocol/multiplexer.rs +++ b/src/protocol/multiplexer.rs @@ -88,7 +88,21 @@ impl Multiplexer { discover_http_server(server_config).await } else if server_config.command.is_some() { // โ”€โ”€ Stdio transport (persistent connection) โ”€โ”€โ”€โ”€ - discover_stdio_server(server_config, &mut stdio_connections, death_tx.clone()).await + let request_timeout = + std::time::Duration::from_secs(config.security.request_timeout_secs); + let handshake_timeout = std::time::Duration::from_secs( + server_config + .handshake_timeout_secs + .unwrap_or(config.security.default_handshake_timeout_secs), + ); + discover_stdio_server( + server_config, + &mut stdio_connections, + death_tx.clone(), + request_timeout, + handshake_timeout, + ) + .await } else { warn!( "Server '{}' has neither 'url' nor 'command' configured", @@ -855,13 +869,15 @@ async fn discover_stdio_server( config: &ServerConfig, stdio_connections: &mut HashMap, death_tx: DeathSender, + request_timeout: std::time::Duration, + handshake_timeout: std::time::Duration, ) -> ( Vec, Vec, Vec, bool, ) { - match StdioConnection::connect(config, death_tx).await { + match StdioConnection::connect(config, death_tx, request_timeout, handshake_timeout).await { Ok((conn, capabilities)) => { let has_tools = capabilities.get("tools").is_some(); let has_resources = capabilities.get("resources").is_some(); diff --git a/src/protocol/stdio.rs b/src/protocol/stdio.rs index afe8882..d7dca50 100644 --- a/src/protocol/stdio.rs +++ b/src/protocol/stdio.rs @@ -32,6 +32,7 @@ pub struct StdioConnection { pending: Arc>>>, next_id: AtomicI64, server_name: String, + request_timeout: Duration, _reader_handle: tokio::task::JoinHandle<()>, _child_handle: tokio::task::JoinHandle<()>, } @@ -45,9 +46,17 @@ impl StdioConnection { /// On drop, the background tasks are detached and the child is reaped. /// /// `death_tx` is used to notify the dead-server monitor when this child exits. + /// `request_timeout` bounds how long `send_request` waits for a correlated + /// response before giving up (wired to `security.request_timeout_secs`). + /// `handshake_timeout` bounds only the initial `initialize` call, and may + /// be longer than `request_timeout` to tolerate slow cold starts (e.g. + /// `npx` resolving/downloading a package on first run) without loosening + /// the timeout used for every steady-state tool call afterward. pub async fn connect( config: &ServerConfig, death_tx: DeathSender, + request_timeout: Duration, + handshake_timeout: Duration, ) -> anyhow::Result<(Self, serde_json::Value)> { let command = config .command @@ -141,13 +150,17 @@ impl StdioConnection { pending, next_id: AtomicI64::new(10), // 1-9 reserved for handshake server_name, + request_timeout, _reader_handle, _child_handle, }; // โ”€โ”€ MCP Handshake โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ + // Uses `handshake_timeout` rather than the connection's steady-state + // `request_timeout` โ€” a slow cold start here shouldn't require + // loosening the timeout applied to every later tool call. let init_result = conn - .send_request( + .send_request_with_timeout( "initialize", serde_json::json!({ "protocolVersion": "2025-11-25", // v0.4.0: Updated from 2025-03-26 @@ -157,6 +170,7 @@ impl StdioConnection { "version": env!("CARGO_PKG_VERSION"), } }), + handshake_timeout, ) .await .map_err(|e| { @@ -180,7 +194,8 @@ impl StdioConnection { Ok((conn, capabilities)) } - /// Send a JSON-RPC request and wait for the correlated response. + /// Send a JSON-RPC request and wait for the correlated response, using + /// the connection's steady-state `request_timeout`. /// /// Returns the `result` field on success, or an error if the upstream /// returns a JSON-RPC error, the request times out, or the child dies. @@ -188,6 +203,20 @@ impl StdioConnection { &self, method: &str, params: serde_json::Value, + ) -> anyhow::Result { + self.send_request_with_timeout(method, params, self.request_timeout) + .await + } + + /// Same as `send_request`, but with an explicit timeout override โ€” used + /// for the initial handshake, which may need a longer (or shorter) + /// timeout than steady-state calls (see `handshake_timeout_secs`). + /// A zero-duration timeout means "wait indefinitely". + async fn send_request_with_timeout( + &self, + method: &str, + params: serde_json::Value, + timeout: Duration, ) -> anyhow::Result { let id = self.next_id.fetch_add(1, Ordering::SeqCst); let (tx, rx) = oneshot::channel(); @@ -233,24 +262,35 @@ impl StdioConnection { debug!("๐Ÿ“ค [{}] โ†’ {} (id={})", self.server_name, method, id); - // Wait for response with timeout - let result = tokio::time::timeout(Duration::from_secs(30), rx) - .await - .map_err(|_| { - self.remove_pending(id); - anyhow::anyhow!( - "Request to '{}' timed out after 30s (method={}, id={})", - self.server_name, - method, - id - ) - })? - .map_err(|_| { + // Wait for response โ€” 0 duration means "no timeout, wait indefinitely" + let recv_result = if timeout.is_zero() { + rx.await.map_err(|_| { anyhow::anyhow!( "Response channel for '{}' dropped (server may have died)", self.server_name ) - })?; + }) + } else { + tokio::time::timeout(timeout, rx) + .await + .map_err(|_| { + self.remove_pending(id); + anyhow::anyhow!( + "Request to '{}' timed out after {}s (method={}, id={})", + self.server_name, + timeout.as_secs(), + method, + id + ) + })? + .map_err(|_| { + anyhow::anyhow!( + "Response channel for '{}' dropped (server may have died)", + self.server_name + ) + }) + }; + let result = recv_result?; match result { Ok(value) => { diff --git a/src/protocol/transport.rs b/src/protocol/transport.rs index d304091..87af6fb 100644 --- a/src/protocol/transport.rs +++ b/src/protocol/transport.rs @@ -755,15 +755,14 @@ async fn dispatch_real_tool( ) -> JsonRpcResponse { let tool_name = params.name.clone(); - // Trusted role from authenticated API key takes precedence. - let role = trusted_role.or_else(|| { - request - .params - .as_ref() - .and_then(|p| p.get("_mcplex_role")) - .and_then(|r| r.as_str()) - .map(|s| s.to_string()) - }); + // Role must come from the server-verified `trusted_role` (set by the auth + // middleware only after a successful API key match โ€” see + // `auth_and_rate_limit_middleware`). Never honor a client-supplied + // `_mcplex_role` in the request params: when no api_key/api_keys are + // configured, trusted_role is always None, and falling back to a + // client-controlled value here would let any caller self-assert + // "admin" and bypass RBAC entirely. + let role = trusted_role; // Security check (with role if available) let security = state.security.read().await; @@ -791,7 +790,10 @@ async fn dispatch_real_tool( drop(config); if cache_enabled { - if let Some(cached_result) = state.cache.get(&tool_name, ¶ms.arguments) { + if let Some(cached_result) = state + .cache + .get(&tool_name, role.as_deref(), ¶ms.arguments) + { let elapsed = start.elapsed(); state.metrics.record_event(EventType::ToolCall { tool_name: tool_name.clone(), @@ -830,9 +832,12 @@ async fn dispatch_real_tool( Ok(result_value) => { // Store in cache if enabled if cache_enabled { - state - .cache - .put(&tool_name, ¶ms.arguments, result_value.clone()); + state.cache.put( + &tool_name, + role.as_deref(), + ¶ms.arguments, + result_value.clone(), + ); } JsonRpcResponse::success(request.id.clone(), result_value) } @@ -1138,7 +1143,7 @@ fn json_max_depth(value: &serde_json::Value, depth: usize) -> usize { } } -fn constant_time_eq(a: &str, b: &str) -> bool { +pub(crate) fn constant_time_eq(a: &str, b: &str) -> bool { if a.len() != b.len() { return false; } @@ -1150,7 +1155,7 @@ fn constant_time_eq(a: &str, b: &str) -> bool { diff == 0 } -fn extract_api_key(request: &axum::extract::Request) -> Option { +pub(crate) fn extract_api_key(request: &axum::extract::Request) -> Option { request .headers() .get("authorization") diff --git a/src/security/allowlist.rs b/src/security/allowlist.rs index 4d59e1a..dc78428 100644 --- a/src/security/allowlist.rs +++ b/src/security/allowlist.rs @@ -89,6 +89,7 @@ mod tests { blocked_tools: vec!["drop_*".to_string(), "delete_*".to_string()], allowed_tools: vec![], enabled: true, + handshake_timeout_secs: None, }]; let engine = AllowlistEngine::new(&servers); @@ -112,6 +113,7 @@ mod tests { blocked_tools: vec![], allowed_tools: vec!["list_*".to_string(), "get_*".to_string()], enabled: true, + handshake_timeout_secs: None, }]; let engine = AllowlistEngine::new(&servers); diff --git a/src/security/audit.rs b/src/security/audit.rs index e41af1b..1e9e905 100644 --- a/src/security/audit.rs +++ b/src/security/audit.rs @@ -225,14 +225,19 @@ fn now_iso8601() -> String { const SENSITIVE_KEY_PARTS: &[&str] = &[ "password", "passwd", + "pwd", "secret", "token", "api_key", "apikey", "authorization", + "auth", "credential", "private_key", "access_key", + "cookie", + "session", + "bearer", ]; /// Recursively redact values whose keys look sensitive. diff --git a/src/security/mod.rs b/src/security/mod.rs index b536e24..f7e8a1b 100644 --- a/src/security/mod.rs +++ b/src/security/mod.rs @@ -81,3 +81,43 @@ impl SecurityEngine { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::RoleConfig; + + fn rbac_config_with_admin_role() -> AppConfig { + let mut config: AppConfig = + toml::from_str("[gateway]\n").expect("minimal config with defaults must parse"); + config.security.enable_rbac = true; + config.roles.insert( + "admin".to_string(), + RoleConfig { + allowed_tools: vec!["*".to_string()], + blocked_tools: vec![], + }, + ); + config + } + + /// Regression test: RBAC must deny when no server-verified role is + /// established, even though an "admin" (allow-all) role exists in + /// config. This guards the fix that removed the client-controlled + /// `_mcplex_role` params fallback in `dispatch_real_tool` โ€” a client + /// could previously self-assert `role: "admin"` and bypass RBAC + /// whenever no api_key/api_keys were configured. + #[test] + fn rbac_denies_when_no_trusted_role_established() { + let config = rbac_config_with_admin_role(); + let engine = SecurityEngine::new(&config); + assert!(!engine.is_tool_allowed("some_tool", None)); + } + + #[test] + fn rbac_allows_with_a_trusted_role() { + let config = rbac_config_with_admin_role(); + let engine = SecurityEngine::new(&config); + assert!(engine.is_tool_allowed("some_tool", Some("admin"))); + } +}