diff --git a/src/app/endpoints/mcp_auth.py b/src/app/endpoints/mcp_auth.py index 62aea7615..9cd7b326a 100644 --- a/src/app/endpoints/mcp_auth.py +++ b/src/app/endpoints/mcp_auth.py @@ -3,6 +3,7 @@ from typing import Annotated, Any from fastapi import APIRouter, Depends, Request +from opentelemetry import trace import constants from authentication import get_auth_dependency @@ -21,8 +22,10 @@ from models.common import MCPServerAuthInfo from models.config import Action from utils.endpoints import check_configuration_loaded +from utils.otel_tracing import SpanAttributes, set_span_attributes logger = get_logger(__name__) +tracer = trace.get_tracer(__name__) router = APIRouter(prefix="/mcp-auth", tags=["mcp-auth"]) @@ -68,27 +71,32 @@ async def get_mcp_client_auth_options( # Nothing interesting in the request _ = request - check_configuration_loaded(configuration) + with tracer.start_as_current_span("mcp_auth.get_client_options") as span: + set_span_attributes(span, {SpanAttributes.MCP_OPERATION: "get_client_options"}) - servers_info = [] + check_configuration_loaded(configuration) - for mcp_server in configuration.mcp_servers: - if not mcp_server.authorization_headers: - continue + servers_info = [] - # Find headers with "client" value - client_headers = [ - header_name - for header_name, header_value in mcp_server.authorization_headers.items() - if header_value.strip() == constants.MCP_AUTH_CLIENT - ] + for mcp_server in configuration.mcp_servers: + if not mcp_server.authorization_headers: + continue - if client_headers: - servers_info.append( - MCPServerAuthInfo( - name=mcp_server.name, - client_auth_headers=client_headers, + # Find headers with "client" value + client_headers = [ + header_name + for header_name, header_value in mcp_server.authorization_headers.items() + if header_value.strip() == constants.MCP_AUTH_CLIENT + ] + + if client_headers: + servers_info.append( + MCPServerAuthInfo( + name=mcp_server.name, + client_auth_headers=client_headers, + ) ) - ) - return MCPClientAuthOptionsResponse(servers=servers_info) + set_span_attributes(span, {SpanAttributes.MCP_SERVERS_COUNT: len(servers_info)}) + + return MCPClientAuthOptionsResponse(servers=servers_info) diff --git a/src/app/endpoints/mcp_servers.py b/src/app/endpoints/mcp_servers.py index 8ec6c5583..449336c85 100644 --- a/src/app/endpoints/mcp_servers.py +++ b/src/app/endpoints/mcp_servers.py @@ -3,6 +3,7 @@ from typing import Annotated, Any from fastapi import APIRouter, Depends, HTTPException, Request, status +from opentelemetry import trace from authentication import get_auth_dependency from authentication.interface import AuthTuple @@ -25,8 +26,10 @@ from models.common import MCPServerInfo from models.config import Action, ModelContextProtocolServer from utils.endpoints import check_configuration_loaded +from utils.otel_tracing import SpanAttributes, set_span_attributes logger = get_logger(__name__) +tracer = trace.get_tracer(__name__) router = APIRouter(tags=["mcp-servers"]) @@ -71,26 +74,37 @@ async def register_mcp_server_handler( _ = auth _ = request - check_configuration_loaded(configuration) + with tracer.start_as_current_span("mcp_server.register") as span: + set_span_attributes( + span, + { + SpanAttributes.MCP_OPERATION: "register", + SpanAttributes.MCP_SERVER_NAME: body.name, + SpanAttributes.MCP_SERVER_PROVIDER_ID: body.provider_id + or "model-context-protocol", + }, + ) - mcp_server = ModelContextProtocolServer.model_validate( - body.model_dump(exclude_none=True) - ) + check_configuration_loaded(configuration) - try: - configuration.add_mcp_server(mcp_server) - except ValueError as e: - response = ConflictResponse(resource="MCP server", resource_id=body.name) - raise HTTPException(**response.model_dump()) from e + mcp_server = ModelContextProtocolServer.model_validate( + body.model_dump(exclude_none=True) + ) - logger.info("Dynamically registered MCP server: %s at %s", body.name, body.url) + try: + configuration.add_mcp_server(mcp_server) + except ValueError as e: + response = ConflictResponse(resource="MCP server", resource_id=body.name) + raise HTTPException(**response.model_dump()) from e - return MCPServerRegistrationResponse( - name=mcp_server.name, - url=mcp_server.url, - provider_id=mcp_server.provider_id, - message=f"MCP server '{mcp_server.name}' registered successfully", - ) + logger.info("Dynamically registered MCP server: %s at %s", body.name, body.url) + + return MCPServerRegistrationResponse( + name=mcp_server.name, + url=mcp_server.url, + provider_id=mcp_server.provider_id, + message=f"MCP server '{mcp_server.name}' registered successfully", + ) list_responses: dict[int | str, dict[str, Any]] = { @@ -125,19 +139,26 @@ async def list_mcp_servers_handler( _ = auth _ = request - check_configuration_loaded(configuration) + with tracer.start_as_current_span("mcp_server.list") as span: + set_span_attributes(span, {SpanAttributes.MCP_OPERATION: "list"}) - servers = [ - MCPServerInfo( - name=mcp.name, - url=mcp.url, - provider_id=mcp.provider_id, - source="api" if configuration.is_dynamic_mcp_server(mcp.name) else "config", - ) - for mcp in configuration.mcp_servers - ] + check_configuration_loaded(configuration) - return MCPServerListResponse(servers=servers) + servers = [ + MCPServerInfo( + name=mcp.name, + url=mcp.url, + provider_id=mcp.provider_id, + source=( + "api" if configuration.is_dynamic_mcp_server(mcp.name) else "config" + ), + ) + for mcp in configuration.mcp_servers + ] + + set_span_attributes(span, {SpanAttributes.MCP_SERVERS_COUNT: len(servers)}) + + return MCPServerListResponse(servers=servers) delete_responses: dict[int | str, dict[str, Any]] = { @@ -175,19 +196,30 @@ async def delete_mcp_server_handler( _ = auth _ = request - check_configuration_loaded(configuration) + with tracer.start_as_current_span("mcp_server.delete") as span: + set_span_attributes( + span, + { + SpanAttributes.MCP_OPERATION: "delete", + SpanAttributes.MCP_SERVER_NAME: name, + }, + ) + + check_configuration_loaded(configuration) + + if not configuration.is_dynamic_mcp_server(name): + static_mcp_names = {s.name for s in configuration.mcp_servers} + if name in static_mcp_names: + response = ForbiddenResponse.mcp_server_static_config(name) + raise HTTPException(**response.model_dump()) - if not configuration.is_dynamic_mcp_server(name): - static_mcp_names = {s.name for s in configuration.mcp_servers} - if name in static_mcp_names: - response = ForbiddenResponse.mcp_server_static_config(name) - raise HTTPException(**response.model_dump()) + try: + configuration.remove_mcp_server(name) + local_deleted = True + except ValueError as e: + logger.error("Failed to remove MCP server from configuration: %s", e) + local_deleted = False - try: - configuration.remove_mcp_server(name) - local_deleted = True - except ValueError as e: - logger.error("Failed to remove MCP server from configuration: %s", e) - local_deleted = False + set_span_attributes(span, {SpanAttributes.MCP_SERVER_DELETED: local_deleted}) - return MCPServerDeleteResponse(deleted=local_deleted, name=name) + return MCPServerDeleteResponse(deleted=local_deleted, name=name) diff --git a/src/utils/otel_tracing.py b/src/utils/otel_tracing.py index 1fdf3ca54..e72a31c7b 100644 --- a/src/utils/otel_tracing.py +++ b/src/utils/otel_tracing.py @@ -43,6 +43,11 @@ class SpanAttributes(StrEnum): SKILL_ACTIVATIONS = "skill.activations" RLS_TEMPLATE_OK = "rls.template.ok" TOPIC_SUMMARY_SUCCESS = "topic.summary.success" + MCP_SERVER_NAME = "mcp.server.name" + MCP_SERVER_PROVIDER_ID = "mcp.server.provider_id" + MCP_SERVERS_COUNT = "mcp.servers.count" + MCP_OPERATION = "mcp.operation" + MCP_SERVER_DELETED = "mcp.server.deleted" class SpanEvents(StrEnum): diff --git a/tests/unit/app/endpoints/test_mcp_auth.py b/tests/unit/app/endpoints/test_mcp_auth.py index 0e06c3688..8c4e88347 100644 --- a/tests/unit/app/endpoints/test_mcp_auth.py +++ b/tests/unit/app/endpoints/test_mcp_auth.py @@ -1,9 +1,14 @@ -# pylint: disable=protected-access +# pylint: disable=protected-access,redefined-outer-name # pyright: reportCallIssue=false """Unit tests for MCP auth endpoint.""" +from typing import Any + import pytest +from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( + InMemorySpanExporter, +) from pytest_mock import MockerFixture # Import the function directly to bypass decorators @@ -110,7 +115,7 @@ def mock_configuration_no_client_auth() -> Configuration: @pytest.mark.asyncio async def test_get_mcp_client_auth_options_success( mocker: MockerFixture, - mock_configuration_with_client_auth: Configuration, # pylint: disable=redefined-outer-name + mock_configuration_with_client_auth: Configuration, ) -> None: """Test successful retrieval of MCP servers with client auth options.""" # Mock configuration - wrap in AppConfig @@ -146,7 +151,7 @@ async def test_get_mcp_client_auth_options_success( @pytest.mark.asyncio async def test_get_mcp_client_auth_options_mixed_auth( mocker: MockerFixture, - mock_configuration_mixed_auth: Configuration, # pylint: disable=redefined-outer-name + mock_configuration_mixed_auth: Configuration, ) -> None: """Test retrieval with mixed auth types - should only return client auth servers.""" # Mock configuration - wrap in AppConfig @@ -181,7 +186,7 @@ async def test_get_mcp_client_auth_options_mixed_auth( @pytest.mark.asyncio async def test_get_mcp_client_auth_options_no_client_auth( mocker: MockerFixture, - mock_configuration_no_client_auth: Configuration, # pylint: disable=redefined-outer-name + mock_configuration_no_client_auth: Configuration, ) -> None: """Test retrieval when no servers have client auth - should return empty list.""" # Mock configuration - wrap in AppConfig @@ -337,3 +342,105 @@ async def test_get_mcp_client_auth_options_multiple_headers_single_server( "X-API-Key", "X-Custom-Token", } + + +class TestMcpAuthOtelSpans: + """OTEL instrumentation tests for the /mcp-auth endpoints.""" + + @pytest.mark.asyncio + async def test_get_client_options_span_attributes( + self, + mocker: MockerFixture, + mock_configuration_with_client_auth: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that get_mcp_client_auth_options emits a span with correct attributes.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_auth.tracer", tracer) + + app_config = AppConfig() + app_config._configuration = mock_configuration_with_client_auth + mocker.patch("app.endpoints.mcp_auth.configuration", app_config) + mocker.patch( + "app.endpoints.mcp_auth.authorize", + lambda action: lambda func: func, + ) + + mock_request = mocker.Mock() + await mcp_auth.get_mcp_client_auth_options.__wrapped__( # type: ignore + mock_request, MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + span = spans[0] + assert span.name == "mcp_auth.get_client_options" + attrs = dict(span.attributes or {}) + assert attrs["mcp.operation"] == "get_client_options" + assert attrs["mcp.servers.count"] == 2 + + @pytest.mark.asyncio + async def test_get_client_options_span_empty_result( + self, + mocker: MockerFixture, + mock_configuration_no_client_auth: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test span when no servers have client auth — count should be 0.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_auth.tracer", tracer) + + app_config = AppConfig() + app_config._configuration = mock_configuration_no_client_auth + mocker.patch("app.endpoints.mcp_auth.configuration", app_config) + mocker.patch( + "app.endpoints.mcp_auth.authorize", + lambda action: lambda func: func, + ) + + mock_request = mocker.Mock() + await mcp_auth.get_mcp_client_auth_options.__wrapped__( # type: ignore + mock_request, MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + attrs = dict(spans[0].attributes or {}) + assert attrs["mcp.servers.count"] == 0 + + @pytest.mark.asyncio + async def test_get_client_options_span_no_secrets( + self, + mocker: MockerFixture, + mock_configuration_with_client_auth: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Verify that no auth tokens, headers, or secrets appear in span attributes.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_auth.tracer", tracer) + + app_config = AppConfig() + app_config._configuration = mock_configuration_with_client_auth + mocker.patch("app.endpoints.mcp_auth.configuration", app_config) + mocker.patch( + "app.endpoints.mcp_auth.authorize", + lambda action: lambda func: func, + ) + + mock_request = mocker.Mock() + await mcp_auth.get_mcp_client_auth_options.__wrapped__( # type: ignore + mock_request, MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + attrs = dict(spans[0].attributes or {}) + forbidden_keys = {"authorization", "token", "secret", "header", "key"} + for attr_key in attrs: + assert not any( + word in str(attr_key).lower() for word in forbidden_keys + ), f"Span attribute '{attr_key}' may contain sensitive data" + for attr_val in attrs.values(): + val_lower = str(attr_val).lower() + assert "bearer" not in val_lower + assert "client" not in val_lower or attr_val == "get_client_options" diff --git a/tests/unit/app/endpoints/test_mcp_servers.py b/tests/unit/app/endpoints/test_mcp_servers.py index 595935811..7796908a2 100644 --- a/tests/unit/app/endpoints/test_mcp_servers.py +++ b/tests/unit/app/endpoints/test_mcp_servers.py @@ -7,6 +7,10 @@ import pytest from fastapi import HTTPException, status +from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( + InMemorySpanExporter, +) +from opentelemetry.trace import StatusCode from pydantic import AnyHttpUrl, SecretStr from pytest_mock import MockerFixture @@ -397,3 +401,197 @@ def test_mcp_server_registration_rejects_arbitrary_value() -> None: authorization_headers={"Authorization": "Bearer my-static-token"}, provider_id="MCP provider ID", ) + + +class TestMcpServersOtelSpans: + """OTEL instrumentation tests for the /mcp-servers endpoints.""" + + @pytest.mark.asyncio + async def test_register_span_success( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that register emits a span with server name and provider_id.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + body = MCPServerRegistrationRequest( + name="new-mcp", + url="http://localhost:4000", + provider_id="model-context-protocol", + ) + await mcp_servers.register_mcp_server_handler( + request=mocker.Mock(), body=body, auth=MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + span = spans[0] + assert span.name == "mcp_server.register" + attrs = dict(span.attributes or {}) + assert attrs["mcp.operation"] == "register" + assert attrs["mcp.server.name"] == "new-mcp" + assert attrs["mcp.server.provider_id"] == "model-context-protocol" + + @pytest.mark.asyncio + async def test_register_span_conflict( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that register 409 conflict records error on the span.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + body = MCPServerRegistrationRequest( + name="static-mcp", + url="http://localhost:4000", + provider_id="model-context-protocol", + ) + with pytest.raises(HTTPException) as exc_info: + await mcp_servers.register_mcp_server_handler( + request=mocker.Mock(), body=body, auth=MOCK_AUTH + ) + assert exc_info.value.status_code == status.HTTP_409_CONFLICT + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + assert spans[0].name == "mcp_server.register" + assert spans[0].status.status_code == StatusCode.ERROR + + @pytest.mark.asyncio + async def test_list_span_with_count( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that list emits a span with mcp.servers.count.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + await mcp_servers.list_mcp_servers_handler( + request=mocker.Mock(), auth=MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + span = spans[0] + assert span.name == "mcp_server.list" + attrs = dict(span.attributes or {}) + assert attrs["mcp.operation"] == "list" + assert attrs["mcp.servers.count"] == 1 + + @pytest.mark.asyncio + async def test_delete_span_success( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that delete emits a span with server name and deleted status.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + app_config = _make_app_config(mocker, mock_configuration) + app_config.add_mcp_server( + ModelContextProtocolServer( + name="dynamic-mcp", + provider_id="model-context-protocol", + url="http://localhost:4001", + ) + ) + + await mcp_servers.delete_mcp_server_handler( + request=mocker.Mock(), name="dynamic-mcp", auth=MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + span = spans[0] + assert span.name == "mcp_server.delete" + attrs = dict(span.attributes or {}) + assert attrs["mcp.operation"] == "delete" + assert attrs["mcp.server.name"] == "dynamic-mcp" + assert attrs["mcp.server.deleted"] is True + + @pytest.mark.asyncio + async def test_delete_span_static_forbidden( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test that deleting a static server records error on span.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + with pytest.raises(HTTPException) as exc_info: + await mcp_servers.delete_mcp_server_handler( + request=mocker.Mock(), name="static-mcp", auth=MOCK_AUTH + ) + assert exc_info.value.status_code == status.HTTP_403_FORBIDDEN + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + assert spans[0].name == "mcp_server.delete" + assert spans[0].status.status_code == StatusCode.ERROR + + @pytest.mark.asyncio + async def test_delete_span_not_found( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Test span when deleting a nonexistent server — deleted=False.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + await mcp_servers.delete_mcp_server_handler( + request=mocker.Mock(), name="no-such-server", auth=MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + attrs = dict(spans[0].attributes or {}) + assert attrs["mcp.server.name"] == "no-such-server" + assert attrs["mcp.server.deleted"] is False + + @pytest.mark.asyncio + async def test_spans_contain_no_secrets( + self, + mocker: MockerFixture, + mock_configuration: Configuration, + otel: tuple[Any, InMemorySpanExporter], + ) -> None: + """Verify no auth tokens, headers, or secrets leak into span attributes.""" + tracer, exporter = otel + mocker.patch("app.endpoints.mcp_servers.tracer", tracer) + _make_app_config(mocker, mock_configuration) + + body = MCPServerRegistrationRequest( + name="secret-check", + url="http://localhost:4000", + provider_id="model-context-protocol", + authorization_headers={"Authorization": "client"}, + ) + await mcp_servers.register_mcp_server_handler( + request=mocker.Mock(), body=body, auth=MOCK_AUTH + ) + + spans = exporter.get_finished_spans() + assert len(spans) == 1 + attrs = dict(spans[0].attributes or {}) + forbidden_keys = {"authorization", "token", "secret", "header", "key"} + for attr_key in attrs: + assert not any( + word in str(attr_key).lower() for word in forbidden_keys + ), f"Span attribute '{attr_key}' may contain sensitive data"