feat(mcp): OAuth 2.1 resource-server discovery surface for /mcp - #859
feat(mcp): OAuth 2.1 resource-server discovery surface for /mcp#859membphis wants to merge 2 commits into
Conversation
Implements the inbound-leg half of AISIX-Cloud#1143. When an environment projects the new mcp_auth_settings row (canonical /mcp resource URL) and has at least one enabled oidc_provider, the gateway: - serves the RFC 9728 Protected Resource Metadata document on both /.well-known/oauth-protected-resource and its path-insertion /mcp form, derived entirely from configuration (resource URL, enabled providers' issuers, union of their required_scopes); - attaches WWW-Authenticate challenges to /mcp auth failures: bare resource_metadata on missing credentials, error=invalid_token on rejected credentials, and error=insufficient_scope naming only the currently-required scopes on a scope failure (the scope failure is split into a dedicated JwtInsufficientScope variant that renders byte-identically to JwtClaimsRejected, so /v1 is unchanged; bound_claims policy denials deliberately carry no challenge). Without the settings row the surface is dormant: the well-known routes 404 and no header is attached — every existing environment behaves byte-identically to before. A malformed row (path other than /mcp, query/fragment, non-http scheme) keeps the surface dormant with one process-wide warning. The resources file treats mcp_auth_settings as a singleton: a second entry is a load error. The row is wired through the full config path: schema variant (schemas/resources/mcp_auth_settings.schema.json via dump-schema), snapshot table, etcd loader + watch supervisor (put/delete/clone), declarative filesource and export.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds the singleton ChangesMCP OAuth support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyRouter
participant MCPAuth
participant Snapshot
participant OIDCProviders
Client->>ProxyRouter: Request OAuth protected-resource metadata
ProxyRouter->>MCPAuth: Handle discovery request
MCPAuth->>Snapshot: Read mcp_auth_settings
MCPAuth->>OIDCProviders: Collect enabled issuers and scopes
MCPAuth-->>Client: Return PRM metadata or 404
Client->>ProxyRouter: Request /mcp
ProxyRouter->>MCPAuth: Apply challenge middleware
MCPAuth-->>Client: Return scoped WWW-Authenticate challenge
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/aisix-core/src/models/mcp_auth_settings.rs (2)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse public API wording for model documentation.
These comments expose etcd/control-plane and loader implementation details. Describe
runtime_idas a runtime-only identity omitted from serialized configuration without exposing storage topology.As per coding guidelines, model comments should be public API reference text and avoid internal shorthand.
Also applies to: 32-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/mcp_auth_settings.rs` around lines 1 - 3, Update the module and field documentation in McpAuthSettings to use public API terminology: describe runtime_id as a runtime-only identity omitted from serialized configuration, and remove references to etcd, control-plane keying, loader details, and internal shorthand.Source: Coding guidelines
5-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse generic references in shipped comments across the MCP settings flow.
These comments expose product-specific ticket identifiers in public model, snapshot, loader-test, and supervisor-test documentation.
crates/aisix-core/src/models/mcp_auth_settings.rs#L5-L10: replace the ticket reference with generic MCP OAuth discovery wording.crates/aisix-core/src/models/snapshot.rs#L60-L65: remove the product-specific ticket from the snapshot documentation.crates/aisix-core/src/filesource/tests.rs#L554-L555: use generic singleton-resource wording.crates/aisix-etcd/src/supervisor.rs#L1049-L1051: use generic watch-activation wording.crates/aisix-etcd/src/supervisor.rs#L1141-L1142: use generic watch-deactivation wording.As per coding guidelines, shipped code and comments should refer to other products generically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/mcp_auth_settings.rs` around lines 5 - 10, Replace product-specific ticket or product references with generic wording in the comments at crates/aisix-core/src/models/mcp_auth_settings.rs:5-10, crates/aisix-core/src/models/snapshot.rs:60-65, and crates/aisix-core/src/filesource/tests.rs:554-555; use generic MCP OAuth discovery and singleton-resource descriptions. Update the watch-activation and watch-deactivation comments at crates/aisix-etcd/src/supervisor.rs:1049-1051 and :1141-1142 similarly, without changing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-etcd/src/loader.rs`:
- Around line 297-308: Update the mcp_auth_settings branch in the
snapshot-building loader to enforce a single valid entry: reject or fail the
snapshot when another entry is already present instead of inserting every parsed
row. Preserve normal insertion for the first entry and ensure duplicate
singleton settings cannot be published or selected by a downstream resolver.
---
Nitpick comments:
In `@crates/aisix-core/src/models/mcp_auth_settings.rs`:
- Around line 1-3: Update the module and field documentation in McpAuthSettings
to use public API terminology: describe runtime_id as a runtime-only identity
omitted from serialized configuration, and remove references to etcd,
control-plane keying, loader details, and internal shorthand.
- Around line 5-10: Replace product-specific ticket or product references with
generic wording in the comments at
crates/aisix-core/src/models/mcp_auth_settings.rs:5-10,
crates/aisix-core/src/models/snapshot.rs:60-65, and
crates/aisix-core/src/filesource/tests.rs:554-555; use generic MCP OAuth
discovery and singleton-resource descriptions. Update the watch-activation and
watch-deactivation comments at crates/aisix-etcd/src/supervisor.rs:1049-1051 and
:1141-1142 similarly, without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1909f638-ed40-48e1-8085-bab92fdcd1d9
📒 Files selected for processing (16)
crates/aisix-core/src/bin/dump-schema.rscrates/aisix-core/src/filesource/desugar.rscrates/aisix-core/src/filesource/mod.rscrates/aisix-core/src/filesource/tests.rscrates/aisix-core/src/models/mcp_auth_settings.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/schema.rscrates/aisix-core/src/models/snapshot.rscrates/aisix-etcd/src/loader.rscrates/aisix-etcd/src/supervisor.rscrates/aisix-proxy/src/error.rscrates/aisix-proxy/src/jwt.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/mcp_auth.rscrates/aisix-server/src/export/document.rsschemas/resources/mcp_auth_settings.schema.json
| "mcp_auth_settings" => { | ||
| if let Some(entry) = validate_and_parse::<McpAuthSettings>( | ||
| &raw.key, | ||
| raw.revision, | ||
| parsed, | ||
| &value, | ||
| validate_mcp_auth_settings, | ||
| &mut stats, | ||
| ) { | ||
| snapshot.mcp_auth_settings.insert(entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)crates/aisix-etcd/src/loader.rs$|mcp_auth_settings|aisix-etcd' || true
echo "== loader outline =="
ast-grep outline crates/aisix-etcd/src/loader.rs --view expanded 2>/dev/null || true
echo "== relevant loader lines =="
sed -n '240,340p' crates/aisix-etcd/src/loader.rsRepository: api7/aisix
Length of output: 5844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate references without executing repository code.
rg -n "mcp_auth_settings|McpAuthSettings|validate_mcp_auth_settings|loader|snapshot|insert_into|load|watch" crates -SRepository: api7/aisix
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== loader full parse/insert flow =="
sed -n '109,330p' crates/aisix-etcd/src/loader.rs
echo "== mcp auth settings model =="
sed -n '1,110p' crates/aisix-core/src/models/mcp_auth_settings.rs
echo "== snapshot and resource table definitions =="
ast-grep outline crates/aisix-core/src/snapshot.rs --view expanded 2>/dev/null || true
sed -n '1,220p' crates/aisix-core/src/snapshot.rs
sed -n '1,140p' crates/aisix-core/src/resource.rs
echo "== mcp gateway duplicate selection relevant sections =="
sed -n '130,230p' crates/aisix-mcp/src/bridge.rs
sed -n '270,320p' crates/aisix-mcp/src/gateway.rsRepository: api7/aisix
Length of output: 30316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mcp auth settings usages =="
rg -n "mcp_auth_settings|McpAuthSettings" crates/aisix-mcp crates/aisix-core crates -S --glob '!target/**'
echo "== protected resource / auth discovery code =="
rg -n "oauth-protected-resource|WWW-Authenticate|resource_server|find_unique_by|auth_settings|protected resource" crates/aisix-mcp crates/aisix-proxy crates -S --glob '!target/**'
echo "== resource file duplicate detection =="
rg -n "mcp_auth_settings|one row|singleton|at most one|duplicate|ResourceTable|find_unique_by|build_snapshot" crates/aisix-admin crates/aisix-core crates/aisix-etcd tests -S --glob '!target/**' | head -n 200Repository: api7/aisix
Length of output: 50366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mcp_auth.rs resolver =="
sed -n '1,120p' crates/aisix-proxy/src/mcp_auth.rs
sed -n '720,870p' crates/aisix-proxy/src/lib.rs
echo "== etcd integration test around duplicate singleton behavior in loader =="
sed -n '340,450p' crates/aisix-admin/tests/etcd_integration.rsRepository: api7/aisix
Length of output: 13616
Enforce the mcp_auth_settings singleton at snapshot build time.
The etcd loader accepts every valid /mcp_auth_settings/<id> row, so stale/migrated keys can publish multiple singleton settings with the same fixed resource name. The runtime picker currently falls back to the smallest id, but this violates the per-environment singleton model and can silently expose the wrong discovery URL/resource metadata. Reject duplicate entries before publishing the snapshot, or align the downstream resolver to fail closed when it sees more than one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aisix-etcd/src/loader.rs` around lines 297 - 308, Update the
mcp_auth_settings branch in the snapshot-building loader to enforce a single
valid entry: reject or fail the snapshot when another entry is already present
instead of inserting every parsed row. Preserve normal insertion for the first
entry and ensure duplicate singleton settings cannot be published or selected by
a downstream resolver.
Addresses the independent pre-merge audit of #859: - reject userinfo in resource_url (validate_resource_url + a filesource load error mirroring the OIDC issuer/jwks_uri rule): the URL is published verbatim on the unauthenticated protected-resource-metadata endpoint, so an embedded credential must never activate the surface. - extract claims_rejection_error() and unit-test both arms, pinning the single construction site of JwtInsufficientScope (scope failures must carry the provider's required scopes; bound-claims denials must keep the challenge-less variant). - register the well-known routes with any() and gate GET/HEAD inside the handler, so a dormant environment answers the pre-existing bare 404 for every method (previously non-GET flipped to 405 even while dormant); active non-GET/HEAD now 405s with an Allow header. Tests pin both. - sanitize challenge-header interpolations to RFC 6750 NQCHAR: a space can no longer corrupt the scope list and a control byte loses one character instead of silently dropping the whole WWW-Authenticate header. - deterministic multi-row pick test, and a comment on the middleware's second snapshot load (accepted eventual consistency).
|
Independent pre-merge audit completed (six angles: correctness / reliability / security / sensitive-info leakage / breaking changes / test coverage). Findings and resolutions:
Clean angles per the audit: RFC 9728/6750 conformance, Post-fix verification: |
1 participant
Part of api7/AISIX-Cloud#1143 (the inbound leg; the upstream-leg client-side MUSTs are tracked separately in api7/AISIX-Cloud#1152).
What
Makes the gateway's
/mcpsurface discoverable as a spec-compliant OAuth 2.1 resource server per the MCP authorization spec (2025-11-25). When an environment projects the newmcp_auth_settingsrow (its canonical/mcpresource URL) AND has at least one enabledoidc_provider:GET /.well-known/oauth-protected-resourceand its RFC 9728 path-insertion sibling.../mcpserve the Protected Resource Metadata document, derived entirely from configuration:resource= the configured URL (never the request Host),authorization_servers= enabled providers' issuers,scopes_supported= the union of theirrequired_scopes,bearer_methods_supported=["header"]./mcpauth failures carry RFC 6750WWW-Authenticatechallenges: bareresource_metadataon missing credentials;error="invalid_token"on any rejected credential;error="insufficient_scope"naming ONLY the currently-required scopes on a scope failure (SEP-2350 forward-compatible: no echo of previously granted scopes).bound_claimspolicy denial deliberately carries NO challenge (re-consenting cannot cure it);JwksUnavailable(503) likewise.Without the settings row the surface is dormant: the well-known routes 404 and no header is attached — every existing environment is byte-identical to before.
Wire compatibility
The scope failure is split out of
JwtClaimsRejectedinto a dedicatedJwtInsufficientScopevariant so the/mcpmiddleware can classify it. It renders byte-identically on the wire (same 403,permission_deniedtype,jwt_claims_rejectedcode, same message) — pinned by a test — so/v1callers cannot observe the split. Token validation itself is unchanged: the existing JWT chain (signature/iss/exp/aud with inclusion semantics, scope/claim enforcement, jwt_subject key binding) is reused as-is.New resource: mcp_auth_settings
Env-scoped singleton (the CP keys the row by the environment id) with one field,
resource_url(absolute http(s), path exactly/mcp, no query/fragment — malformed rows keep the surface dormant with one process-wide warning). Wired through the full config path: schema variant (schemas/resources/mcp_auth_settings.schema.json, emitted by dump-schema), snapshot table, etcd loader + watch supervisor (put/delete/clone/resource-counts), declarative filesource (a second entry per file is a load error) and export.Sequencing (important)
AISIX-Cloud's nightly cross-plane contract check fails on any unregistered DP schema. The registration entry (AISIX-Cloud side) must merge the same day as this PR, before the next nightly run. The AISIX-Cloud counterpart PRs (config surface + dashboard + the full-chain live-DP e2e) follow this one because the DP drops projected rows with unknown fields.
Prior art
Surveyed eight mainstream AI/MCP gateway products before designing (per repo rule):
Testing
cargo testacross aisix-core / aisix-etcd / aisix-proxy / aisix-server: all green (1400+ tests), clippy + fmt clean.bound_claims403, on dormant environments, or on non-/mcproutes); exactinsufficient_scopescope attribute;/v1scope-failure rendering unchanged; audience inclusion in both directions; filesource singleton enforcement; watch-supervisor propagation for the new kind (put + delete).Summary by CodeRabbit
WWW-Authenticatechallenges for applicable MCP authentication failures.