feat(oauth): discover issuer metadata - #6442
Conversation
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
1f3960a to
e1c0600
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
msureshkumar88
left a comment
There was a problem hiding this comment.
Nice work getting the RFC 8414 → OIDC fallback flow wrapped for the new public endpoint, and the singleflight lock for concurrent cold-cache requests is a clean solve for the User Story 2 requirement in #5717. The structured error-code mapping and audit logging are also solid additions.
I found a few things worth addressing before merge, verified directly against the diff and the current call sites of discover_as_metadata().
Blocking
1. New endpoint isn't reachable with a token scoped to the permission the issue requires
_PERMISSION_PATTERNS in mcpgateway/middleware/token_scoping.py has no entry for POST /gateways/discover-metadata — the existing gateway patterns need either an exact /gateways match or a trailing-slash sub-resource, neither of which this path satisfies. It falls through to the default-deny at the end of _check_permission_restrictions. Confirmed empirically:
/v1/gateways/discover-metadata scoped[gateways.create]: False <- 403
/gateways scoped[gateways.create]: True
Issue #5717's security requirement #1 asks for this endpoint to require the same permission as MCP server creation — but a token scoped to exactly that permission gets rejected by the middleware layer before the route's own @require_permission("gateways.create") even runs. Only unscoped/* tokens currently work. Could you add a pattern entry alongside the other gateway routes?
2. HTTPS-only validation was added to the shared discover_as_metadata(), not just the new endpoint
Pre-PR, discover_as_metadata() did no issuer validation at all (config.py's validation_allowed_url_schemes still allows http://). This PR adds an HTTPS-only / no-query / no-fragment check inside _validate_discovery_issuer(), which is also called from oauth_router.py (DCR auto-registration on gateway OAuth login) and gateway_service.py (gateway registration).
oauth_router.pycatchesDcrErrorand raisesHTTPException(500, "...check your OAuth server supports RFC 7591")— for an existing gateway with an HTTP issuer, this becomes a hard 500 with a misleading message pointing at the wrong root cause.gateway_service.pyandadmin.pydegrade more gracefully (warning log / 502), so the practical blast radius is narrower than it first looks, but theoauth_router.pypath is a real regression for any existing non-HTTPS gateway.
Since #5717 scopes the HTTPS-only requirement to the new discovery endpoint (and even suggests wrapping rather than modifying discover_as_metadata() in place), could this validation be scoped to the new endpoint only — or, if tightening it fleet-wide is intentional, could we fix the oauth_router.py error path and add regression coverage for all three existing callers?
3. Malformed issuer_url returns 200 instead of non-2xx
OAuthMetadataDiscoveryRequest.issuer_url in schemas.py has no URL-format validation (plain str, length-bounded only). A value like "not-a-url" reaches _validate_discovery_issuer, which raises DcrError(code="blocked"), and the route returns HTTP 200 with "This issuer URL is blocked by the outbound security policy." — even though the actual problem is a malformed input.
Issue #5717's endpoint contract is explicit: "Non-2xx is reserved for invalid input (missing/malformed issuer_url)...". Adding URL-format validation to the Pydantic field (e.g. a field_validator or AnyHttpUrl) would let FastAPI 422 on this case per the contract, and also stop a typo from being reported as a security-policy rejection.
4. Response-size cap is enforced after the full body is already downloaded
_fetch_metadata_document() uses client.get(), non-streaming, so response.content is fully buffered before the 256 KiB check at the bottom of the function ever runs. The Content-Length check helps for honest servers, but a chunked response with no declared length bypasses it entirely — bounded only by the 5s timeout and available bandwidth. Since #5717 calls out response-size capping as a must-have security requirement, would it be possible to switch to client.stream() and abort once the byte count crosses the cap, so this is enforced at the network layer rather than post-hoc?
5. A few required deny-path tests from #5717 aren't covered
Security requirement #6 lists unauthenticated caller, non-HTTPS target, link-local target, loopback target, and metadata-IP target (169.254.169.254) as required regression tests. The current test file covers permission-denied (403) but not a fully unauthenticated request, and the SSRF-adjacent tests mock validate_url_for_connection_pinning to raise ValueError directly rather than exercising the actual policy against a loopback/link-local/metadata-IP URL. Worth adding real coverage for at least loopback and 169.254.169.254, since those are the classic SSRF probes this endpoint is meant to resist.
Suggestions (non-blocking)
_fetch_metadata_document()now raisesDcrErroron anyhttpx.HTTPErrorfrom the RFC 8414 fetch, where the pre-PR code caughthttpx.HTTPErrorand fell through to the OIDC fallback. A transport-level failure (DNS blip, TLS handshake reset) on the RFC 8414 path will now skip OIDC discovery entirely, where it previously wouldn't have. Worth double-checking this is intentional._metadata_cachehas no eviction — the singleflight lock cleanup at the end ofdiscover_as_metadata()even calls out "Keeping one for every user-supplied issuer would let this public endpoint grow process memory forever", but the cache entry itself (which is larger than the lock) has no equivalent cleanup. An LRU cap or active TTL eviction would close this off now that the function is reachable with arbitrary user-supplied issuers via the public endpoint.- None of the commits reference
#5717— would be good to addCloses #5717(or similar) for traceability. - No documentation updates for the new endpoint or its validation behavior — worth a short doc note, especially if #2 above lands as an intentional fleet-wide tightening.
Everything else — the alembic/schema question (none needed, correct), rate-limiter wiring, and endpoint placement in main.py (no routers/gateways.py exists, so this is the right spot despite the issue text suggesting that filename) — looked good.
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Pull Request
🔗 Related Issue
Closes #5717
📝 Summary
Adds secure OAuth issuer metadata discovery for gateway setup. Client sends issuer URL; gateway discovers provider metadata before user must manually enter authorization/token endpoints.
What changed / achieved
POST /v1/gateways/discover-metadata, protected bygateways.create.not_found,invalid_metadata,blocked, ortimeoutresult. No upstream error body leaks to client.📏 Reviewability
triage🏷️ Type of Change
🧪 Verification
pytest tests/unit/mcpgateway/services/test_dcr_service.py tests/unit/mcpgateway/test_oauth_metadata_discovery.py -qmake ruffmake lintmake testmake coverage✅ Checklist
make black isort pre-commit)📓 Notes (optional)
Frontend issuer-field integration intentionally deferred. This PR provides backend discovery API for later catalog/OAuth setup flow (#5967).