feat: swap to using aw-mcpg

[jamesadevine]

No description provided.

[github-actions]

🔍 Rust PR Review

Summary: Architecture replacement looks solid overall, but there are a few correctness bugs and security concerns worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• safeoutputs omitted from MCPG config when no MCP servers are configured (src/compile/standalone.rs, generate_mcpg_config call site)

  generate_mcpg_config always inserts safeoutputs as an HTTP backend — but the call site only invokes it when !front_matter.mcp_servers.is_empty(). When a pipeline has zero mcp-servers: entries it falls through to the hardcoded {"mcpServers":{}} string, which omits safeoutputs entirely. That silently breaks safe-output tools for any agent that doesn't configure an extra MCP.

  Fix: call generate_mcpg_config unconditionally (or move safeoutputs insertion into the else branch):

  // Always generate — safeoutputs is always required
  let config = generate_mcpg_config(front_matter);
  let mcpg_config_json = serde_json::to_string_pretty(&config)
      .unwrap_or_else(|_| r#"{"mcpServers":{}}"#.to_string());

• rt.block_on() called on a Tokio worker thread (src/mcp.rs, run_http)

  The factory closure passed to StreamableHttpService::new is almost certainly called from within the Tokio runtime when a new session is established. Calling Handle::current().block_on(…) on a Tokio worker thread panics with "Cannot start a runtime from within a runtime". SafeOutputs::new is an async fn, so the only safe patterns here are either making the closure async, or initializing SafeOutputs once outside the closure and cloning an Arc:

  // Pre-initialize and share via Arc
  let safe_outputs = Arc::new(SafeOutputs::new(&bounding, &output).await?);
  let mcp_service = StreamableHttpService::new(
      move || Ok((*safe_outputs).clone()),  // if SafeOutputs: Clone
      …
  );

• Readiness wait loop has no error path (templates/base.yml, "Start SafeOutputs HTTP server" / "Start MCP Gateway" steps)

  Both 30-iteration poll loops break on success but fall through silently on timeout. If either service fails to start, the pipeline continues and produces confusing failures later. A minimal guard:

  if ! curl -sf "(localhost/redacted) > /dev/null 2>&1; then
    echo "##vso[task.complete result=Failed]SafeOutputs did not become ready"
    exit 1
  fi

🔒 Security Concerns

• host.docker.internal added to CORE_ALLOWED_HOSTS (src/allowed_hosts.rs)

  This hostname resolves to the Docker host from inside a container. Adding it to the core allow-list means every pipeline — including those that don't use MCPG at all — will have the AWF container able to reach any service running on the host. A safer approach is to add it only when MCPG is configured (similar to how MCP-specific hosts are added per-MCP in generate_allowed_domains).

• Docker socket mounted into MCPG container (templates/base.yml)

  -v /var/run/docker.sock:/var/run/docker.sock \

  Mounting the Docker socket gives the MCPG container full control over the Docker daemon (spawn containers, access all volumes, escape network isolation). If MCPG doesn't strictly require socket access, remove it. If it does, this deserves an explicit security comment explaining why.

• Weak fallback API key (src/mcp.rs, run_http)

  The generated fallback (format!("{:x}", SystemTime::now().as_nanos())) is ~48 bits of a predictable, time-based value. In practice the pipeline always passes --api-key $(openssl rand …), so the risk is low — but the Rust fallback will be hit in any dev/test invocation without --api-key. Consider using uuid::Uuid::new_v4() or any cryptographic RNG (e.g., fill a [u8; 32] from std::os::unix::fs::File on /dev/urandom) for the fallback path.

⚠️ Suggestions

• Hardcoded gateway port 80 (src/compile/standalone.rs, McpgGatewayConfig)

  port: 80 is baked into the generated JSON. If MCPG ever changes its default port, this will silently produce a broken config. Consider surfacing this as a constant (MCPG_PORT) alongside MCPG_VERSION/MCPG_IMAGE, or deriving it from a template marker that can be replaced at compile time.

• 1ES generate_mcp_configuration now includes all non-custom MCPs without checking whether a service connection exists (src/compile/onees.rs)

  The old code guarded on is_builtin_mcp. The new code passes any enabled, non-custom MCP through — including names like phantom that have no service connection and no builtin backing. The generated YAML will reference a non-existent serviceConnection, which fails at queue time. Worth adding a comment or a validation that warns when a non-custom MCP is used in 1ES mode.

✅ What Looks Good

• Clean removal of mcp_firewall.rs and mcp_metadata.rs — the module boundaries are now much simpler.
• McpgServerConfig / McpgConfig types are well-structured with appropriate skip_serializing_if guards.
• The sed-based runtime substitution is safe: SAFE_OUTPUTS_API_KEY is generated with tr -d '/+=' leaving only [A-Za-z0-9], so no sed injection risk.
• Auth middleware correctly skips /health and enforces Bearer token on all other routes.
• Test coverage was updated proportionally with the deleted code.

Generated by Rust PR Reviewer for issue #19 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good architectural refactoring — needs a couple of security fixes before merge.

The MCPG migration is clean and well-motivated. The removal of the custom MCP firewall layer in favour of the established gateway simplifies the stack significantly. The core logic is sound.

Findings

🔒 Security Concerns

• src/mcp.rs:460-466 — Weak API key fallback: When --api-key is omitted, the generated key is format!("{:x}", SystemTime::now().as_nanos()) — a hex-encoded nanosecond timestamp (~40 bits, predictable and not cryptographically random). The pipeline always passes --api-key so this path is unlikely to be hit in practice, but anyone invoking ado-aw mcp-http directly would get a weak key. Use OsRng / getrandom or at minimum generate something non-deterministic:

  // Option: use the `rand` crate (already in dep tree via rmcp?)
  use rand::Rng;
  rand::thread_rng().gen::(u128)()

  Or surface a clearer error/warning that --api-key is required.

• src/mcp.rs:529 — API key printed to log file: println!("SAFE_OUTPUTS_API_KEY={}", api_key) writes the secret to stdout, which the pipeline redirects to $(Agent.TempDirectory)/staging/logs/safeoutputs.log. If that log directory is ever published as a build artifact (or captured in diagnostics), the key is exposed. This println! is also redundant — the pipeline generates the key itself and passes it via --api-key, so the server doesn't need to echo it back. Remove or gate behind a debug log.

• templates/base.yml:244 — Docker socket mount into MCPG: -v /var/run/docker.sock:/var/run/docker.sock gives the MCPG container full control over the Docker daemon on the host. This is a significant privilege escalation risk in shared pipeline agents. If MCPG only needs it to spawn stdio MCP servers as sibling processes, this should be justified in a comment and ideally scoped as tightly as possible (or handled through a daemon instead of socket pass-through).

🐛 Bugs / Logic Issues

• src/compile/standalone.rs:166-167 — Silent serialization failure produces broken MCPG config:

  let mcpg_config_json = serde_json::to_string_pretty(&config)
      .unwrap_or_else(|_| r#"{"mcpServers":{}}"#.to_string());

  The fallback JSON is missing the gateway section, so MCPG would fail at startup with a cryptic error if this ever triggers. Since compile() returns Result, propagate with ?:

  let mcpg_config_json = serde_json::to_string_pretty(&config)
      .context("Failed to serialize MCPG config")?;

⚠️ Suggestions

• src/compile/standalone.rs:~428 — args: Some(opts.args.clone()) unconditionally set: For custom MCPs with no args, the serialized JSON will include "args": []. Prefer if opts.args.is_empty() { None } else { Some(opts.args.clone()) } to match the clean handling already applied to env and tools — makes the generated config easier to read.

✅ What Looks Good

• No leftover {{ firewall_config }} / mcp-firewall-config markers — the migration is complete and clean, verified by updated integration tests.
• SafeOutputs is always injected as an HTTP backend regardless of mcp-servers: content — correct, since it's always required.
• Auth middleware correctly bypasses the /health endpoint without requiring a token.
• Cleanup steps (Stop MCPG and SafeOutputs) are guarded with condition: always() — good for failure scenarios.
• 1ES path correctly warns and skips custom MCPs rather than silently generating broken config.
• The generate_mcp_configuration fallback in onees.rs (mcp-{name}-service-connection) is a sensible convention for Enabled(true) MCPs.

Generated by Rust PR Reviewer for issue #19 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good architectural direction — replacing the in-process MCP firewall with MCPG is the right call. Two security issues need addressing before merge; everything else is clean.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs:364 — safeoutputs name can be silently overwritten

  In generate_mcpg_config, the hard-coded SafeOutputs HTTP backend is inserted first, then the loop over front_matter.mcp_servers runs. If a user names their custom MCP server "safeoutputs":

  mcp_servers.insert("safeoutputs".to_string(), /* HTTP backend */);
  // ...loop overwrites it with user's stdio command
  mcp_servers.insert(name.clone(), McpgServerConfig { server_type: "stdio", command: Some("evil"), ... });

  The SafeOutputs HTTP backend gets replaced by a custom stdio command. The agent would then route all safeoutputs tool calls to the attacker-controlled process instead of the validated HTTP handler. Fix: guard the loop with if name == "safeoutputs" { log::warn!(...); continue; }.

🔒 Security Concerns

• templates/base.yml — MCPG API key printed to pipeline logs before ADO masking applies

  In the "Start MCP Gateway (MCPG)" step, MCP_GATEWAY_API_KEY is a local bash variable. The ##vso[task.setvariable ...; issecret=true] command registers it as a secret only for subsequent steps — the current step's log output is not retroactively masked. The config (with the key substituted in) is then echoed:

  MCP_GATEWAY_API_KEY=$(openssl rand ...)
  echo "##vso[task.setvariable variable=MCP_GATEWAY_API_KEY;issecret=true]$MCP_GATEWAY_API_KEY"
  MCPG_CONFIG=$(... | sed "s|\${MCP_GATEWAY_API_KEY}|$MCP_GATEWAY_API_KEY|g")
  echo "$MCPG_CONFIG" | python3 -m json.tool   # ← key in plain text

  $(SAFE_OUTPUTS_API_KEY) is fine (it's an ADO secret from a prior step, so ADO masks it at runtime). But $MCP_GATEWAY_API_KEY is a shell-local variable here — ADO has no value to mask yet.

  Fix: either move the echo before the substitution (log the template, not the rendered config), or set the API key as a secret in a dedicated prior step so ADO has the value to mask.

⚠️ Suggestions

• src/mcp.rs — entropy fallback is weak

  The /dev/urandom fallback uses a nanosecond SystemTime seed with a fixed multiplier. On Linux (the only supported pipeline target), /dev/urandom is always present, so this path is dead in production. Consider using rand::rngs::OsRng or the getrandom crate for a cleaner cross-platform solution that removes the dead code without relying on platform assumptions.

• src/compile/allowed_hosts.rs comment vs. reality

  The comment says host.docker.internal is "added conditionally … when MCPG is configured." In practice it's added unconditionally in the standalone compiler (because MCPG is now always used). The comment is slightly misleading — worth updating to say "always added for standalone pipelines."

✅ What Looks Good

• The MCPG config is always generated (SafeOutputs always present) — solid invariant; no conditional logic to forget.
• Health-check polling loops with proper failure propagation (##vso[task.complete result=Failed]) before the agent step starts.
• Cleanup step runs with condition: always() — both MCPG and SafeOutputs are stopped even on failure.
• SAFE_OUTPUTS_PID capture + kill in the cleanup step is correct.
• Test coverage for all the new MCPG config generation paths (always_includes_safeoutputs, custom_mcp, mcp_without_command_skipped, disabled_mcp_skipped, gateway_defaults).
• Rename is_builtin_mcp → is_custom_mcp with inverted logic is clean and the new semantics ("all MCPs are custom") are accurately reflected.

Generated by Rust PR Reviewer for issue #19 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good architectural direction — the MCPG swap is clean and well-structured. A few reliability and security issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• templates/base.yml — Hardcoded container name mcpg breaks retries

  docker run -i --rm --name mcpg --network host ...

  --rm only removes the container on clean exit. If the agent is interrupted (OOM kill, SIGKILL, network partition), the container may remain and the next pipeline retry will fail with docker: Error response from daemon: Conflict. The container name "/mcpg" is already in use. Add a pre-cleanup line before the docker run:

  docker rm -f mcpg 2>/dev/null || true

• src/mcp.rs:run_http — Weak fallback entropy

  The last-resort API key path (when /dev/urandom is unavailable) derives entropy from only 8 bytes of SystemTime nanoseconds, then XOR-multiplies for the second half:

  buf[..16].copy_from_slice(&seed.to_le_bytes());
  buf[16..].copy_from_slice(&seed.wrapping_mul(0x517cc1b727220a95).to_le_bytes());

  The second 16 bytes are fully determined by the first 16 — this is a 64-bit key disguised as 256-bit. Since the binary only runs on Linux agents, /dev/urandom is always available in practice, but the fallback should either anyhow::bail!("cannot generate secure random key: ...") or use getrandom::getrandom. A silently weak key is worse than a clear error.

🔒 Security Concerns

• src/mcp.rs:run_http — Non-constant-time API key comparison

  if auth_str == format!("Bearer {}", expected) {

  Short-circuit string equality leaks timing information proportional to how many leading bytes match. While the SafeOutputs HTTP server is only reachable via host.docker.internal (host-local), a compromised AWF container could use timing to brute-force the key incrementally. Swap for subtle::ConstantTimeEq or the constant_time_eq crate — both are already common in the Rust ecosystem.

• templates/base.yml — Docker socket mount (intentional, but flagging)

  -v /var/run/docker.sock:/var/run/docker.sock

  The comment correctly documents this is intentional (MCPG spawns stdio MCP servers as sibling containers). Just ensuring reviewers are aware this grants the MCPG process full Docker daemon access on the host, meaning a compromised MCP server could escape isolation. Given the overall trust model this may be acceptable, but consider whether --network host + a restricted socket proxy (e.g. docker-socket-proxy) would provide adequate containment if stdio MCPs are untrusted.

⚠️ Suggestions

• src/compile/standalone.rs:generate_mcpg_config — Duplicate warning arms

  The two else branches at the end of the MCP iteration loop produce identical log messages for two different conditions (an MCP with options but no command, vs. a bare McpConfig::Enabled(true) with no options). Collapsing them or adding distinguishing context ("MCP '{}' specified as boolean true — this format required built-in MCPs which no longer exist") would help users understand the migration path.

• src/mcp.rs:run_http — SafeOutputs::new clone-based sharing

  Pre-initializing SafeOutputs and sharing via clone() works, but only if SafeOutputs's internal state (directories, file handles) is either immutable or Arc-wrapped. If SafeOutputs::clone() does a shallow copy of mutable state, concurrent sessions could race on writes to the staging directory. Worth a quick audit or a doc comment on the Clone impl confirming it's safe for concurrent use.

✅ What Looks Good

• sed injection safety: tr -d '/+=' correctly strips all sed metacharacters from both SAFE_OUTPUTS_API_KEY and MCP_GATEWAY_API_KEY, making the runtime substitution safe.
• "safeoutputs" name reservation with a clear warning log is a good defensive check.
• condition: always() on both cleanup steps ensures MCPG and SafeOutputs are torn down even after agent failure.
• context() added to MCPG config serialization (replacing the old unwrap_or_else) is a genuine improvement.
• Test suite is cleanly updated to match the new architecture — no orphaned test helpers or references to removed types.
• The 1ES target correctly warns (rather than silently dropping) when MCPs fall back to the naming convention for service connections.

Generated by Rust PR Reviewer for issue #19 · ◷