feat: Support MCP Authentication - #194
Conversation
…d), run discover_mcp_tools in a loop Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…mprovements Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…-fabric into david-mcp-auth Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…mprovements Signed-off-by: David Gardner <dagardner@nvidia.com>
…-fabric into david-mcp-auth Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…g the loop Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…odels Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
Thanks David!! Used powerful AI to help me review, and this is the first round, as i just realized this PR is in draft now!
| if headers := server.get("custom_headers"): | ||
| try: | ||
| result[name]["headers"] = mcp_auth.normalize_custom_headers( | ||
| name, headers | ||
| ) | ||
| except mcp_auth.McpAuthConfigError as error: | ||
| raise AdapterConfigError( | ||
| "claude_invalid_configuration", str(error) |
There was a problem hiding this comment.
Nitty thing here — custom_headers values here would be written to the mcp.json and would bypass the NEMO_FABRIC_CLAUDE_MCP_<sha256> env projection that _stage_mcp_config applies to env values and OAuth tokens; would this be an expected behavior?
There was a problem hiding this comment.
Similar thing for the hermes adapter
There was a problem hiding this comment.
In short no, this will need to be a documentation item for Claude Code, as custom_headers is serialized to disk doing:
config.add_mcp_server(..., authentication=None, custom_headers={"Authorization": "Bearer XYZ"})Will cause the token to be serialized to disk (it is created with permissions set to 600, but this isn't supported on Windows, and the file is remove on a graceful shutdown, but will remain on disk if the process exits unexpectedly like receiving a SIGKILL).
Instead the more secure approach would be:
config.add_mcp_server(..., authentication=None, custom_headers={"Authorization": "Bearer ${MCP_ACCESS_TOKEN}"}, env={"MCP_ACCESS_TOKEN": "${MCP_ACCESS_TOKEN}"})Which will avoid having the token serialized to disk.
| ) -> None: | ||
| for name in _authenticated_mcp_servers(payload): | ||
| if self._mcp_authentication_checked.get(name, False): | ||
| continue |
There was a problem hiding this comment.
The OAuth provider and the refresh token it obtained are discarded after _prefetch_mcp_oauth_tokens projects a one-shot static access token into mcp.json at start, and _authenticate_mcp_servers skips servers that are already marked in _mcp_authentication_checked, so an expired token may never detected or refreshed for the life of the runtime?
|
|
||
| try: | ||
| async with asyncio.timeout(timeout): | ||
| async with httpx.AsyncClient(auth=provider) as http_client: |
There was a problem hiding this comment.
Should we also include custom_headers here, so servers requiring both a static header and OAuth never issue the 401 challenge?
| readme = "pypi.md" | ||
| requires-python = ">=3.11" | ||
| dependencies = [ | ||
| "nemo-fabric-adapters-common == 0.2.0", |
There was a problem hiding this comment.
Should we also declare the same thing for Claude adapter too, because Claude adapter also uses mcp_auth.create_mcp_oauth_provider which is in this mcp-oauth extra?
There was a problem hiding this comment.
oops it did not catch the next line loll, but it is "nemo-fabric-adapters-common[mcp-oauth] == 0.2.0",
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] | ||
| pub struct McpServerConfig { | ||
| /// MCP transport. |
There was a problem hiding this comment.
Nitty thing, but should we market this PR as a breaking change too?
| else None | ||
| ) | ||
|
|
||
| await self._authenticate_mcp_servers() |
There was a problem hiding this comment.
This is a review left by agent that I didn't verify, because I feel like this is exactly something that an agent would be really good at to verify if the claim is true 😂
"Hermes _authenticate_mcp_servers raises lifecycle.LifecycleError out of invoke(), which the shared lifecycle host wraps as _AdapterCallError and records as state.failed=True (lifecycle.py:363-365), permanently rejecting all later invokes with lifecycle_runtime_failed (lifecycle.py:258-260) — so the retry intent encoded by leaving self._mcp_authentication_checked False on failure (asserted by test_runtime_reports_failed_oauth_mcp_authentication) is unreachable, unlike Claude and Codex which return structured failure outputs for the same condition and stay retryable."
zhongxuanwang-nv
left a comment
There was a problem hiding this comment.
oops this was supposed to get in for the last round
| if not target: | ||
| raise ValueError("MCP server mapping requires a URL") | ||
|
|
||
| if transport == "stdio": | ||
| if server.authentication: | ||
| raise ValueError("MCP authentication is not supported for stdio transport") | ||
| if server.custom_headers: | ||
| raise ValueError("MCP custom_headers are not supported for stdio transport") | ||
| return common_utils.without_none( |
There was a problem hiding this comment.
is the removal intentional?
#### Overview * Add support for MCP Oauth2 * Oauth service account is stubbed out in the config, but is currently unimplemented. * Currently Oauth2 is supported by Hermes Agent and Codex as both provide an API for initiating authentication. * Adds `custom_headers` field to `McpServerConfig` allowing for static API Key based auth * MCP transport is now an enum (stdio, sse, streamable-http) * Add `McpOAuth2Config` & `McpServiceAccountConfig` to adapter contract * Add a `just schemas` recipe for regenerating the schemas. (Unrelated improvement) * This is a slimmed down version of PR #194 #### Where should the reviewer start? * `crates/fabric-core/src/config.rs` #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes FABRIC-170 - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. ## Summary by CodeRabbit ## Summary by CodeRabbit * **New Features** * Added OAuth 2.0 and service-account authentication for supported HTTP MCP servers. * Added custom HTTP headers with validation and adapter-specific secret handling. * Added explicit MCP transport options: stdio, SSE, and streamable HTTP. * Added interactive OAuth sign-in, status tracking, and tool refresh. * **Bug Fixes** * Improved timeout handling, cleanup, error reporting, and configuration validation. * **Documentation** * Updated SDK, configuration, schema, and API references. Authors: - David Gardner (https://github.com/dagardner-nv) Approvers: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) URL: #211
Overview
custom_headersfield toMcpServerConfigallowing for static API Key based authmcp-oauthextra tonemo-fabric-adapters-commonadapters/common/src/nemo_fabric_adapters/common/mcp_auth.pyEach harness has it's own support (or lack of) MCP Auth from the SDK, however most assume the user will run
<harness> mcp login <mcp name>and authenticate an MCP server globally.Claude Code
claude mcp loginin a subprocess requires an interactive terminal.NEMO_FABRIC_CLAUDE_MCP_<sha256 hash>mcp.jsonfile is written to disk referencing the environment variables ensuring credentials are never written to disk, ex:Codex
CodexRuntime.invokeFabric queries Codex for the auth status of each MCP servermcpServer/oauth/loginendpoint.Deepagents
Hermes Agent
HermesRuntime.invokea call is made totools.mcp_tool.get_mcp_statusif there are any MCP servers marked as disconnected, a call is made totools.mcp_oauth.force_interactive_oauthtoolsnamespace is provided by thehermes-agentPython package.Where should the reviewer start?
crates/fabric-core/src/config.rsRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-170
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
New Features
Documentation
Tests