diff --git a/docs/ADR-005-token-audience-validation.md b/docs/ADR-005-token-audience-validation.md index 29bde78..3e6c4d6 100644 --- a/docs/ADR-005-token-audience-validation.md +++ b/docs/ADR-005-token-audience-validation.md @@ -1,10 +1,19 @@ # ADR-005: Token Audience Validation and Security Compliance -**Status**: Accepted +**Status**: Implemented **Date**: 2025-01-05 -**Related**: Issue #261, ADR-004, upstream-oauth.md +**Updated**: 2025-11-05 +**Related**: Issue #261, ADR-004, upstream-oauth.md, RFC 7519, RFC 8707, RFC 9728 **Supersedes**: Token passthrough mode in ADR-004 +## Implementation Note + +This ADR has been fully implemented with key simplifications based on RFC 7519 Section 4.1.3: +- MCP server validates only its own audience (not Nextcloud's) +- OAuth requests include `resource` parameter (RFC 8707) +- Clients discover resource via PRM endpoint (RFC 9728) +- Nextcloud OIDC app uses client-specific resource URLs + ## Executive Summary This ADR addresses a critical security vulnerability where the MCP server was passing tokens intended for itself directly to Nextcloud APIs (token passthrough). We will: @@ -65,13 +74,14 @@ Based on analysis of the existing code and python-sdk constraints, we will: ### Mode 1: Multi-Audience Token Validation (Default) -Use multi-audience tokens directly. Per RFC 7519 Section 4.1.3, the MCP server validates only its own presence in the audience claim. Nextcloud independently validates its own audience when receiving API calls. This is the default mode when `ENABLE_TOKEN_EXCHANGE` is false or not set. +Use multi-audience tokens directly. Per RFC 7519 Section 4.1.3, resource servers validate only their own presence in the audience claim. The MCP server validates its own audience; Nextcloud independently validates its own audience when receiving API calls. This is the default mode when `ENABLE_TOKEN_EXCHANGE` is false or not set. **Requirements**: -- Token must have `aud` claim containing: - - **MCP server**: Client ID OR MCP server URL (e.g., `http://localhost:8000`) -- For Nextcloud API access to work, token should also include: - - **Nextcloud**: Nextcloud resource URI (e.g., `http://localhost:8080`) +- Token must have `aud` claim containing MCP server audience: + - Client ID OR + - MCP server URL (e.g., `http://localhost:8001`) OR + - MCP server URL with /mcp suffix (e.g., `http://localhost:8001/mcp`) +- For Nextcloud API access to work, token should also include Nextcloud audience (validated by Nextcloud, not MCP) - Single token works for both MCP authentication and Nextcloud API access - IdP must support multi-audience tokens for full functionality @@ -95,40 +105,33 @@ NEXTCLOUD_RESOURCE_URI=http://localhost:8080 # Nextcloud resource identifier OIDC_CLIENT_ID=nextcloud-mcp-server ``` -**Token validation logic (RFC 7519 compliant)**: +**Token validation logic (RFC 7519 compliant) - Actual Implementation**: ```python -async def validate_token_audiences(token: dict, settings: Settings) -> bool: +def _has_mcp_audience(self, payload: dict[str, Any]) -> bool: """ - Validate token has MCP audience per RFC 7519. + Check if token has MCP audience. - Resource servers validate only their own presence in the audience claim. - Nextcloud will independently validate its own audience when receiving API calls. - This is NOT token passthrough (we validate the token). This IS token reuse - which is allowed by RFC 8707 for multi-audience tokens between trusted services. + Per RFC 7519 Section 4.1.3, resource servers should only validate their own + presence in the audience claim. We don't validate Nextcloud's audience - that's + Nextcloud's responsibility when it receives the token. """ - audiences = token.get("aud", []) + audiences = payload.get("aud", []) if isinstance(audiences, str): audiences = [audiences] audiences_set = set(audiences) - # MCP validates ONLY its own audience (client_id OR server_url OR server_url/mcp) - mcp_valid = ( - settings.oidc_client_id in audiences_set or - settings.nextcloud_mcp_server_url in audiences_set or - f"{settings.nextcloud_mcp_server_url}/mcp" in audiences_set - ) - - if not mcp_valid: - logger.error( - f"Token rejected: Missing MCP audience. " - f"Got {audiences}, need MCP ({settings.oidc_client_id} or " - f"{settings.nextcloud_mcp_server_url})" + # MCP must have at least one: client_id OR server_url OR server_url/mcp + return bool( + self.settings.oidc_client_id in audiences_set + or ( + self.settings.nextcloud_mcp_server_url + and ( + self.settings.nextcloud_mcp_server_url in audiences_set + or f"{self.settings.nextcloud_mcp_server_url}/mcp" in audiences_set + ) ) - return False - - # Note: We do NOT validate Nextcloud's audience - that's Nextcloud's responsibility - return True + ) ``` ### Mode 2: RFC 8693 Token Exchange (Opt-in) @@ -272,23 +275,39 @@ class UnifiedTokenVerifier(TokenVerifier): """ Verify token according to MCP TokenVerifier protocol. - CRITICAL: This method only validates tokens - it does NOT perform exchange. - Token exchange happens later in context_helper.py when creating NextcloudClient. + Per RFC 7519, we validate only MCP audience. The mode determines what + happens AFTER verification in context_helper.py: + - Multi-audience mode: Use token directly (Nextcloud validates its own audience) + - Exchange mode: Exchange for Nextcloud-audience token via RFC 8693 - Multi-audience mode: Validates token has BOTH MCP and Nextcloud audiences - Exchange mode: Validates token has MCP audience ONLY (exchange happens later) - """ - if self.mode == "multi-audience": - return await self._verify_multi_audience_token(token) - else: - # Exchange mode: Only validate MCP audience here - # Actual exchange happens in context_helper.py - return await self._verify_mcp_audience_only(token) + Args: + token: Bearer token to verify - async def _verify_multi_audience_token(self, token: str) -> AccessToken | None: + Returns: + AccessToken if valid with MCP audience, None otherwise """ - Validate token has both MCP and Nextcloud audiences (Mode 1). - Token can be used directly without exchange. + # Check cache first + cached = self._get_cached_token(token) + if cached: + logger.debug("Token found in cache") + return cached + + # Both modes do the same validation (MCP audience only) + return await self._verify_mcp_audience(token) + + async def _verify_mcp_audience(self, token: str) -> AccessToken | None: + """ + Validate token has MCP audience. + + Per RFC 7519 Section 4.1.3, resource servers validate only their own + presence in the audience claim. We don't validate Nextcloud's audience - + that's Nextcloud's responsibility when it receives the token. + + Args: + token: Bearer token to verify + + Returns: + AccessToken if valid with MCP audience, None otherwise """ try: # Attempt JWT verification first @@ -300,80 +319,28 @@ class UnifiedTokenVerifier(TokenVerifier): if not payload: return None - # Validate both audiences are present - if not self._validate_multi_audience(payload): - logger.error( - f"Token rejected: Missing required audiences. " - f"Got {payload.get('aud')}, need both MCP and Nextcloud" - ) - return None - - return self._create_access_token(token, payload) - - except Exception as e: - logger.error(f"Multi-audience validation failed: {e}") - return None - - async def _verify_mcp_audience_only(self, token: str) -> AccessToken | None: - """ - Validate token has MCP audience only (Mode 2). - Token will be exchanged later in context_helper.py. - """ - try: - # Attempt JWT verification first - if self._is_jwt_format(token) and self.jwks_client: - payload = await self._verify_jwt_signature(token) - else: - # Fall back to introspection for opaque tokens - payload = await self._introspect_token(token) - if not payload: - return None - - # Only validate MCP audience (exchange will handle Nextcloud) + # Validate MCP audience is present if not self._has_mcp_audience(payload): + audiences = payload.get("aud", []) logger.error( f"Token rejected: Missing MCP audience. " - f"Got {payload.get('aud')}, need {self.settings.oidc_client_id} " - f"or {self.settings.nextcloud_mcp_server_url}" + f"Got {audiences}, need MCP ({self.settings.oidc_client_id} or " + f"{self.settings.nextcloud_mcp_server_url})" ) return None + # Log based on mode for clarity + if self.mode == "multi-audience": + logger.info( + "MCP audience validated - token can be used directly " + "(Nextcloud will validate its own audience)" + ) + else: + logger.info( + "MCP audience validated - token will be exchanged for Nextcloud access" + ) + return self._create_access_token(token, payload) - - except Exception as e: - logger.error(f"MCP audience validation failed: {e}") - return None - - def _validate_multi_audience(self, payload: dict) -> bool: - """Check if token has both MCP and Nextcloud audiences.""" - audiences = payload.get("aud", []) - if isinstance(audiences, str): - audiences = [audiences] - - audiences_set = set(audiences) - - # MCP must have at least one: client_id OR server_url - mcp_valid = ( - self.settings.oidc_client_id in audiences_set or - self.settings.nextcloud_mcp_server_url in audiences_set - ) - - # Nextcloud must have its resource URI - nextcloud_valid = self.settings.nextcloud_resource_uri in audiences_set - - return mcp_valid and nextcloud_valid - - def _has_mcp_audience(self, payload: dict) -> bool: - """Check if token has MCP audience (for exchange mode).""" - audiences = payload.get("aud", []) - if isinstance(audiences, str): - audiences = [audiences] - - audiences_set = set(audiences) - return ( - self.settings.oidc_client_id in audiences_set or - self.settings.nextcloud_mcp_server_url in audiences_set - ) ``` **Key Design Decisions**: @@ -455,7 +422,57 @@ def validate_oauth_configuration(settings: Settings): logger.info("Multi-audience mode enabled - tokens must contain both MCP and Nextcloud audiences") ``` -### 5. Context Helper Updates +### 5. OAuth Resource Parameters and PRM Discovery + +To ensure tokens have the correct audience, OAuth authorization requests must include the `resource` parameter (RFC 8707): + +**OAuth Authorization Requests**: +```python +# Flow 1 (MCP Client Authentication) +idp_params = { + "client_id": idp_client_id, + "redirect_uri": callback_uri, + "response_type": "code", + "scope": scopes, + "state": idp_state, + "prompt": "consent", + "resource": f"{oauth_config['mcp_server_url']}/mcp", # MCP server audience +} + +# Flow 2 (Nextcloud Resource Access) +idp_params = { + ... + "resource": oauth_config["nextcloud_resource_uri"], # Nextcloud audience +} +``` + +**Protected Resource Metadata (PRM) Endpoint**: + +The MCP server exposes PRM metadata at `/.well-known/oauth-protected-resource` (RFC 9728): +```json +{ + "resource": "http://localhost:8001/mcp", + "scopes_supported": ["notes:read", "notes:write", ...], + "authorization_servers": ["http://localhost:8080"], + "bearer_methods_supported": ["header"], + "resource_signing_alg_values_supported": ["RS256"] +} +``` + +**Client Discovery Pattern**: +```python +# Clients should discover resource identifier from PRM +prm_url = f"{mcp_server_url}/.well-known/oauth-protected-resource" +async with httpx.AsyncClient() as client: + prm_response = await client.get(prm_url, timeout=10) + prm_data = prm_response.json() + resource_identifier = prm_data.get("resource") + +# Use discovered resource in OAuth request +auth_url = f"{authorization_endpoint}?resource={quote(resource_identifier, safe='')}&..." +``` + +### 6. Context Helper Updates Update `context.py` to handle token exchange at the NextcloudClient creation point: @@ -969,22 +986,25 @@ This separation ensures: ## Implementation Checklist -- [ ] Create `UnifiedTokenVerifier` class replacing both existing verifiers -- [ ] Remove pass-through mode from `context.py` entirely -- [ ] Update `context_helper.py` to implement token exchange with caching -- [ ] Implement multi-audience validation in unified verifier -- [ ] Implement MCP-only validation for exchange mode in unified verifier -- [ ] Add token exchange caching mechanism in context helper layer -- [ ] Update docker-compose.yml with resource URI configuration: +- [x] Create `UnifiedTokenVerifier` class replacing both existing verifiers +- [x] Remove pass-through mode from `context.py` entirely +- [x] Update `context_helper.py` to implement token exchange with caching +- [x] Implement RFC 7519 compliant validation in unified verifier (MCP audience only) +- [x] Add token exchange caching mechanism in context helper layer +- [x] Add OAuth resource parameters to authorization requests (RFC 8707) +- [x] Implement PRM endpoint for resource discovery (RFC 9728) +- [x] Update tests to discover resource from PRM endpoint +- [x] Fix Nextcloud OIDC app to use client-specific resource_url +- [x] Update docker-compose.yml with resource URI configuration: - `NEXTCLOUD_MCP_SERVER_URL` (required) - `NEXTCLOUD_RESOURCE_URI` (required) - `TOKEN_EXCHANGE_CACHE_TTL` (optional, default: 300) -- [ ] Configure Nextcloud OIDC `default_resource_identifier` +- [x] Configure Nextcloud OIDC `default_resource_identifier` - [ ] Configure Keycloak resource servers with proper audiences -- [ ] Remove `NextcloudTokenVerifier` class -- [ ] Remove `ProgressiveConsentTokenVerifier` class -- [ ] Write unit tests for unified verifier (both modes) -- [ ] Write integration tests for token exchange flow -- [ ] Update documentation with IdP configuration guides +- [x] Remove `NextcloudTokenVerifier` class +- [x] Remove `ProgressiveConsentTokenVerifier` class +- [x] Write unit tests for unified verifier +- [x] Write integration tests for OAuth flows +- [x] Update documentation with IdP configuration guides - [ ] Add performance benchmarks to CI pipeline - [ ] Update CHANGELOG.md with breaking changes notice \ No newline at end of file diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 35a79f2..71fb618 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -867,6 +867,9 @@ def get_app(transport: str = "sse", enabled_apps: list[str] | None = None): mcp_server_url = os.getenv( "NEXTCLOUD_MCP_SERVER_URL", "http://localhost:8000" ) + nextcloud_resource_uri = os.getenv( + "NEXTCLOUD_RESOURCE_URI", nextcloud_host + ) discovery_url = os.getenv( "OIDC_DISCOVERY_URL", f"{nextcloud_host}/.well-known/openid-configuration", @@ -884,6 +887,7 @@ def get_app(transport: str = "sse", enabled_apps: list[str] | None = None): "client_secret": client_secret, # From setup_oauth_config (DCR or static) "scopes": scopes, "nextcloud_host": nextcloud_host, + "nextcloud_resource_uri": nextcloud_resource_uri, "oauth_provider": oauth_provider, }, } diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index c2afc6f..5f35f02 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -252,6 +252,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: "scope": scopes, "state": idp_state, "prompt": "consent", # Ensure refresh token + "resource": f"{oauth_config['mcp_server_url']}/mcp", # MCP server audience } auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" @@ -359,6 +360,7 @@ async def oauth_authorize_nextcloud( "state": state, "prompt": "consent", # Force consent to show resource access "access_type": "offline", # Request refresh token + "resource": oauth_config["nextcloud_resource_uri"], # Nextcloud audience } auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" diff --git a/tests/conftest.py b/tests/conftest.py index 47f0f21..b98e5bb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2193,17 +2193,35 @@ async def _get_oauth_token_for_user( logger.info(f"Getting OAuth token for user: {username}...") logger.info(f"Using shared OAuth client: {client_id[:16]}...") + # Fetch resource identifier from PRM endpoint (RFC 9728) + mcp_server_url = os.getenv("NEXTCLOUD_MCP_SERVER_URL", "http://localhost:8001") + prm_url = f"{mcp_server_url}/.well-known/oauth-protected-resource" + + logger.debug(f"Fetching PRM metadata from: {prm_url}") + async with httpx.AsyncClient() as client: + prm_response = await client.get(prm_url, timeout=10) + if prm_response.status_code != 200: + logger.warning(f"Failed to fetch PRM metadata: {prm_response.status_code}") + # Fallback to default if PRM fetch fails + mcp_server_resource = f"{mcp_server_url}/mcp" + else: + prm_data = prm_response.json() + mcp_server_resource = prm_data.get("resource", f"{mcp_server_url}/mcp") + logger.info(f"Using resource from PRM: {mcp_server_resource}") + # Generate unique state parameter for this OAuth flow state = secrets.token_urlsafe(32) logger.debug(f"Generated state for {username}: {state[:16]}...") # Construct authorization URL with state parameter + # Include resource parameter discovered from PRM endpoint auth_url = ( f"{authorization_endpoint}?" f"response_type=code&" f"client_id={client_id}&" f"redirect_uri={quote(callback_url, safe='')}&" f"state={state}&" + f"resource={quote(mcp_server_resource, safe='')}&" # Resource URI from PRM f"scope=openid%20profile%20email%20notes:read%20notes:write%20calendar:read%20calendar:write%20contacts:read%20contacts:write%20cookbook:read%20cookbook:write%20deck:read%20deck:write%20tables:read%20tables:write%20files:read%20files:write%20sharing:read%20sharing:write" ) diff --git a/third_party/oidc b/third_party/oidc index 19cad6e..e83dabb 160000 --- a/third_party/oidc +++ b/third_party/oidc @@ -1 +1 @@ -Subproject commit 19cad6e5b69127e62e4a7d03b139ada1dedc92a1 +Subproject commit e83dabbac17142f7ece6219e5b33ca145fc99ae6