Skip to content

feat: replace RemoteAuthProvider with OIDCProxy and add pluggable backend token strategies#23

Open
mdanish98 wants to merge 19 commits into
mainfrom
feat/enhance-auth-options
Open

feat: replace RemoteAuthProvider with OIDCProxy and add pluggable backend token strategies#23
mdanish98 wants to merge 19 commits into
mainfrom
feat/enhance-auth-options

Conversation

@mdanish98

Copy link
Copy Markdown
Member

Summary

Replaces the previous RemoteAuthProvider-based OAuth implementation with FastMCP's OIDCProxy, enabling universal OIDC support for any standards-compliant IdP (SAP IAS, Azure AD, Keycloak, Okta) without requiring Dynamic Client Registration (DCR) support on the IdP side. Additionally introduces a pluggable backend token strategy system (BackendTokenProvider) supporting three modes: forward, RFC 8693 token exchange, and none.

Changes

Added

  • src/aas_mcp_server/backend_auth.py — pluggable backend token strategy system with ForwardStrategy, TokenExchangeStrategy (RFC 8693), and NoneStrategy; TokenExchangeStrategy holds a shared httpx.AsyncClient with aclose() and async context manager support
  • tests/test_backend_auth.py — full unit test coverage for all three strategies, factory logic, response validation, client reuse, and graceful shutdown
  • docs/2026-06-06-kyma-backend-auth-guide-design.md — Kyma backend auth design guide

Modified

  • src/aas_mcp_server/server.py
    • build_auth_provider: migrated from RemoteAuthProvider to OIDCProxy; hardens wildcard bind guard, normalises issuer URL, validates client_id; emits a WARNING when OAUTH_REQUIRED_SCOPES is set explaining scope enforcement is per-authorization-flow only, not per-request (SEC-262)
    • build_mcp_server: wires backend_token_provider into HTTP client; wires _provider_lifespan into FastMCP.from_openapi(lifespan=...) so TokenExchangeStrategy.aclose() is called on server shutdown (SEC-309)
    • _provider_lifespan: new @asynccontextmanager that calls aclose() on any provider that supports it
  • src/aas_mcp_server/http_client.py
    • BearerTokenAuth: delegates token acquisition to BackendTokenProvider; removes token prefix from debug logs — only strategy name and token length are logged
    • Removes unused get_access_token import
  • src/aas_mcp_server/constants.py — adds BACKEND_AUTH_* env var constants, strategy name constants, and RFC 8693 grant type constants
  • config.yaml.template — full migration guide from RemoteAuthProvider to OIDCProxy; new SCOPE ENFORCEMENT CHANGE section; OAUTH_REQUIRED_SCOPES example corrected to comma-separated format
  • configs/config.yaml.template — corrects misleading # REQUIRED labels on both official_spec and implementation_spec; adds configuration guide for all three valid spec combinations
  • tests/test_auth.py, tests/test_http_client.py, tests/test_server.py — updated for OIDCProxy migration; new tests for token log safety, lifespan shutdown, scope delegation warning; test isolation hardened (clear=True on env patches)
  • .gitignore — removes broad docs/ and charts/ ignore rules

Removed

  • RemoteAuthProvider usage and associated JWT verifier configuration

Technical Details

OIDCProxy architecture

OIDCProxy acts as a local OAuth 2.1 Authorization Server, proxying the PKCE flow to the upstream IdP using pre-registered server-side credentials. Works with any OIDC-compliant IdP without requiring RFC 7591 DCR support.

Scope enforcement change (operators upgrading from JWTVerifier)

Previously OAUTH_REQUIRED_SCOPES was enforced per-request via JWTVerifier. With OIDCProxy, enforcement is per-authorization-flow: scopes are requested during PKCE and the IdP refuses authorization to users who do not hold them. FastMCP-issued tokens are not re-validated per-request because MCP clients register via DCR without declaring scopes. A WARNING is emitted at startup when OAUTH_REQUIRED_SCOPES is set.

Backend token strategies

Strategy When to use
forward (default) Backend accepts the same token the user authenticated with
token_exchange Backend expects a token with its own aud; preserves user sub
none Public backend or mTLS

Auto-selection: if BACKEND_AUTH_AUDIENCE is set, token_exchange is selected automatically.

Connection lifecycle (SEC-309)

TokenExchangeStrategy holds a single httpx.AsyncClient created at construction (not per-request). aclose() is wired into the FastMCP server lifespan via _provider_lifespan.

Compatibility & Impact

Breaking Changes

  • OAUTH_REQUIRED_SCOPES: enforcement changes from per-request to per-authorization-flow. See migration guide in config.yaml.template.
  • OAUTH_JWKS_URI no longer read — auto-discovered from OIDC config. Remove from existing deployments.
  • OAUTH_CLIENT_ID now required when OAUTH_ISSUER_URL is set.
  • OAUTH_CLIENT_SECRET required for confidential clients.

API Changes

New OAuth endpoints exposed by OIDCProxy:

  • /.well-known/oauth-authorization-server (RFC 8414)
  • /authorize, /token, /register, /auth/callback

Testing

  • 222 passing, 10 skipped, 0 failing
  • New tests/test_backend_auth.py — 20 tests covering all strategy paths, error conditions, client reuse, graceful shutdown
  • Token content absent from debug logs verified by direct log handler inspection
  • Server lifespan shutdown wiring verified via _lifespan_manager()
  • Test isolation hardened (clear=True on env patches)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated
  • All tests pass (222 passed, 10 skipped)

mdanish98 and others added 19 commits June 1, 2026 22:28
- Refactor TokenExchangeStrategy to reuse a single httpx.AsyncClient instance for all requests, reducing connection overhead.
- Add error handling for missing access_token in the response and non-JSON responses.
- Update documentation to clarify OAuth scopes and their enforcement.
- Improve test coverage for token exchange scenarios and logging behavior.
ricogu

This comment was marked as duplicate.

@ricogu ricogu dismissed their stale review July 2, 2026 19:04

Reposting findings as inline comments.

@ricogu ricogu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reposting the P1/P2 findings inline for easier follow-up.

forward_resource=False, # Disable RFC 8707 resource param — rejected by SAP IAS
# and unnecessary for non-resource-indicator IdPs.
require_authorization_consent="external", # Consent handled by upstream IdP
client_storage=MemoryStore(), # Avoid disk storage — container home dir may not exist

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes OAuth state process-local. FastMCP stores transactions, DCR clients, JTI mappings, refresh metadata, and upstream token references in this store, so auth can break after restarts or across multiple replicas/load-balanced pods. Please avoid hard-coding MemoryStore; use FastMCP's default persistent store or make storage configurable and document the production requirement for shared storage or sticky sessions.

"Either set BACKEND_AUTH_TOKEN_ENDPOINT explicitly, or set OAUTH_ISSUER_URL "
"so the token endpoint can be derived automatically."
)
token_endpoint = _derive_token_endpoint_from_issuer(issuer_url)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deriving <issuer>/oauth2/token is not a general OIDC rule and is wrong for common providers like Keycloak and Azure AD. Since BACKEND_AUTH_AUDIENCE auto-selects token exchange, this can create a broken default config. Please discover token_endpoint from OIDC metadata, or require BACKEND_AUTH_TOKEN_ENDPOINT for token exchange.

Comment thread config.yaml.template
# If you were using the previous RemoteAuthProvider-based OAuth configuration,
# the following changes are required:
#
# REMOVED: OAUTH_JWKS_URI (auto-discovered from OIDC config — remove this var)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration note is good, but the public docs still need the same update. README.md and client_config_examples.txt still mention OAUTH_JWKS_URI, JWKS path troubleshooting, and the old auth flow, so users following them will end up with stale config. Please update those docs in this PR too.

extra_authorize_params["scope"] = " ".join(
dict.fromkeys(["openid", *required_scopes])
)
# If required_scopes is not set, don't send scope parameter at all.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above says openid is requested by default, but this branch sends no scope parameter unless OAUTH_REQUIRED_SCOPES is set. Some OIDC providers treat a request without openid as plain OAuth and may not return the expected OIDC identity data. Please request at least openid by default, or make omitting scopes an explicit opt-out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants