Simplify MCP server auth: bind to localhost, remove gateway token validation - #1048
Conversation
…idation The MCP server is a local-only human interface — egg containers communicate with the orchestrator directly and have no reason to use it. Binding to 127.0.0.1 provides the natural security boundary without requiring tokens. Removes gateway_url/launcher_secret params, _validate_gateway_token, and the require_auth decorator. Updates tests and docs accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Review: Simplify MCP server auth
Blocking Issues
1. MCP server unreachable from host due to 127.0.0.1 binding inside Docker container
mcp_server.py — run() method
The MCP server runs inside the orchestrator Docker container, which uses custom bridge networks (egg-isolated at 172.32.0.3, egg-external at 172.33.0.3) — not host networking. Docker publishes port 9850 via docker-compose.yml:
ports:
- "${EGG_MCP_SERVER_PORT:-9850}:9850"Docker's port forwarding (both iptables DNAT and userland proxy) routes incoming connections to the container's network IP (172.32.0.3:9850), not the container's loopback interface. When Flask binds to 127.0.0.1:9850, it only listens on the container's lo interface. Connections arriving on eth0 (from Docker's port forwarding) are refused because 172.32.0.3 != 127.0.0.1.
Result: curl http://localhost:9850/health from the host will fail — the feature's core purpose is broken.
For comparison, the orchestrator API works because entrypoint.sh sets ORCHESTRATOR_HOST="${ORCHESTRATOR_HOST:-0.0.0.0}" and passes it to cli.py serve --host $ORCHESTRATOR_HOST.
Fix options:
- (a) Keep binding to
0.0.0.0inside the container but restrict the Docker port mapping to localhost:"127.0.0.1:${EGG_MCP_SERVER_PORT:-9850}:9850"indocker-compose.yml. This is the standard pattern for services that should only be accessible from the host. - (b) If you want the bind address to be configurable (like the orchestrator API), add an
EGG_MCP_SERVER_HOSTenv var defaulting to0.0.0.0and restrict in docker-compose.
Option (a) is simpler and achieves the stated security goal (localhost-only access).
2. Missed test file: test_coordinator_mcp.py:56-69 will fail
orchestrator/tests/test_coordinator_mcp.py — test_mcp_server_has_authentication
This test was not updated by the PR. It searches mcp_server.py for any of ["auth", "token", "session", "validate"]:
def test_mcp_server_has_authentication(self):
content = mcp_path.read_text()
has_auth = any(
keyword in content.lower() for keyword in ["auth", "token", "session", "validate"]
)
assert has_auth, "MCP server should have authentication"After this PR removes all authentication code, none of these keywords exist in the file. This test will fail.
Fix: Remove or update this test to reflect the new security model (localhost binding replaces token auth). The docstring "MCP server must validate gateway session tokens" is also stale.
Non-blocking
3. docker-compose publishes MCP port on all host interfaces
The port mapping "${EGG_MCP_SERVER_PORT:-9850}:9850" binds to 0.0.0.0 on the host, making the MCP server reachable from any machine on the network — counter to the "localhost only" security claim. If implementing fix option (a) above, change to "127.0.0.1:${EGG_MCP_SERVER_PORT:-9850}:9850".
4. PR body attribution
The PR body contains "Generated with Claude Code" and a claude.ai link, which the project's CLAUDE.md explicitly forbids. Minor, but worth cleaning up.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Bind to 0.0.0.0 inside the container so Docker port forwarding works (127.0.0.1 is unreachable via Docker bridge networks). Restrict access to localhost via docker-compose port mapping (127.0.0.1:9850:9850). Update test_mcp_server_has_authentication to reflect the new security model (localhost binding replaces token auth).
There was a problem hiding this comment.
Addressed all blocking and non-blocking feedback in 2bc1da9:
1. MCP server unreachable from host (blocking) — Agreed. Went with option (a): reverted run() to bind 0.0.0.0 inside the container so Docker port forwarding works. Localhost restriction is now enforced at the Docker layer.
2. test_mcp_server_has_authentication will fail (blocking) — Agreed. Replaced with test_mcp_server_localhost_security_model that checks for the new security model. Updated the test file docstring too.
3. docker-compose publishes MCP port on all interfaces (non-blocking) — Agreed. Changed port mapping to 127.0.0.1:${EGG_MCP_SERVER_PORT:-9850}:9850.
4. PR body attribution (non-blocking) — Agreed. Attempted to update, but the gateway blocks PR edits since the PR author differs from the bot user. The PR owner can remove the attribution line manually.
All 101 MCP-related tests pass, linter clean.
— Authored by egg
There was a problem hiding this comment.
Re-review: Simplify MCP server auth (post-feedback)
All four issues from the previous review have been addressed correctly.
Blocking issues — verified fixed
1. MCP server unreachable from host — Fixed. run() binds to 0.0.0.0 inside the container (mcp_server.py:151), and docker-compose restricts host access to 127.0.0.1:9850 (docker-compose.yml:106). This matches the orchestrator's pattern and is the correct approach for Docker bridge networks.
2. test_mcp_server_has_authentication will fail — Fixed. Replaced with test_mcp_server_localhost_security_model (test_coordinator_mcp.py:56-63) that asserts "localhost" appears in the source. Docstring updated. Auth-related tests (test_mcp_server_call_tool_missing_auth, test_mcp_server_call_tool_no_gateway_returns_503, test_call_tool_missing_auth_returns_401) correctly removed. All remaining tests stripped of auth mocking and headers.
Non-blocking issues — verified fixed
3. docker-compose port mapping — Fixed. Port mapping is now 127.0.0.1:${EGG_MCP_SERVER_PORT:-9850}:9850.
4. PR body attribution — Acknowledged. Gateway blocks PR edits from non-author bots. PR owner can clean up manually.
New code review
The fix commit is clean:
- Auth removal is thorough — no stale references to
_validate_gateway_token,require_auth,gateway_url, orlauncher_secretremain in any active code. api.py:_maybe_start_mcp_server()simplified correctly — no longer imports or passes gateway/secret params.docs/guides/coordinator.mdupdated with"type": "sse"in the Claude Code config snippet and a note about localhost-only access.- No new issues introduced.
Minor suggestion: The docs/guides/coordinator.md note "No authentication is required" is accurate but could benefit from a brief "why" for readers unfamiliar with the change — e.g., "No authentication is required — access is restricted to localhost via Docker port binding." This is already stated but slightly buried. Non-blocking.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Agreed. Clarified the "no auth" note in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Simplify MCP server auth (post-suggestion)
Single new commit (adc0533) since last review — clarifies the "no auth" note in docs/guides/coordinator.md as suggested. The change is correct and reads clearly.
All previously verified fixes remain intact. No new issues introduced.
No further changes needed.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
Summary
127.0.0.1instead of0.0.0.0— localhost is the natural security boundary for this local-only human interface_validate_gateway_token,require_auth,gateway_url/launcher_secretparams) — egg containers talk directly to the orchestrator and never use the MCP server, so the auth added no real protectionstart_mcp_serverand_maybe_start_mcp_serverinapi.pyaccordingly"type": "sse"to the Claude Code config snippetTest plan
curl http://localhost:9850/healthworks from hostcurl http://host-ip:9850should fail)http://localhost:9850/mcp/v1/ssewithout needing a token🤖 Generated with Claude Code