From fd5e4b71f37e1e54be8795b5755b53930888e41c Mon Sep 17 00:00:00 2001 From: latte Date: Wed, 14 Oct 2026 17:14:13 +0200 Subject: [PATCH] fix: record the calling client and only schema-known names in audit entries (SEC-12, SEC-18) --- docs/audit-logging.md | 11 +++- src/aegis_gitea_mcp/audit.py | 9 ++- src/aegis_gitea_mcp/server.py | 53 +++++++++++++++-- tests/security/test_audit_fail_closed.py | 74 +++++++++++++++++++++++- tests/test_server.py | 4 +- 5 files changed, 141 insertions(+), 10 deletions(-) diff --git a/docs/audit-logging.md b/docs/audit-logging.md index 88ff6ac..4494ceb 100644 --- a/docs/audit-logging.md +++ b/docs/audit-logging.md @@ -28,8 +28,15 @@ A tool invocation payload records: - the HTTP status when Gitea answered, and the outcome: success, or the error kind (`not_found`, `forbidden`, `invalid_argument`, `internal_error`, ...); - the duration in milliseconds and the correlation id; -- the argument names, sorted. Argument values are never logged, so request bodies, secret values - and file contents do not reach the log. +- who called: the MCP client id of the verified token (`aegis-static-bearer` for the static + bearer), a 12 character hash of the OAuth grant's session id (`session`, only for OAuth + tokens, never the id or a token) and the source address (`client_ip`, HTTP only; behind a + proxy it is the forwarded address only when the proxy is in `FORWARDED_ALLOW_IPS`); +- the argument names, sorted, and only names that are properties of the tool's schema; any other + name is counted in `rejected_argument_count`. Argument values are never logged, so request + bodies, secret values and file contents do not reach the log. A call to a tool that does not + exist is recorded as `(unknown tool, N chars)`: the caller's string is neither logged nor + echoed in the error. All payloads also pass through secret masking before they are written (`SECRET_DETECTION_MODE` other than `off`). diff --git a/src/aegis_gitea_mcp/audit.py b/src/aegis_gitea_mcp/audit.py index 141910a..78ed295 100644 --- a/src/aegis_gitea_mcp/audit.py +++ b/src/aegis_gitea_mcp/audit.py @@ -129,13 +129,17 @@ class AuditLogger: identity_login: str | None = None, identity_mode: str | None = None, sudo: str | None = None, + client_id: str | None = None, + session: str | None = None, + client_ip: str | None = None, outcome: str | None = None, duration_ms: float | None = None, ) -> str: """Log an MCP tool invocation. The keyword-only fields describe a server tool call (tool domain, HTTP - method, path template, identity, sudo target, outcome and duration). They + method, path template, identity, sudo target, the MCP client id, a short hash + of the grant's session and the source address, outcome and duration). They are only written when given. Callers must pass argument names, never argument values, in ``params``. """ @@ -154,6 +158,9 @@ class AuditLogger: "identity_login": identity_login, "identity_mode": identity_mode, "sudo": sudo, + "client_id": client_id, + "session": session, + "client_ip": client_ip, "outcome": outcome, "duration_ms": duration_ms, } diff --git a/src/aegis_gitea_mcp/server.py b/src/aegis_gitea_mcp/server.py index 927b4c9..c5bbe9e 100644 --- a/src/aegis_gitea_mcp/server.py +++ b/src/aegis_gitea_mcp/server.py @@ -11,6 +11,8 @@ values are never logged or echoed in validation errors. from __future__ import annotations +import hashlib +import ipaddress import json import logging import time @@ -78,7 +80,7 @@ _MAX_ALLOWED_NAMES = 40 _MAX_NAME_CHARS = 64 _MAX_AUDIT_ARGUMENT_NAMES = 64 _MAX_AUDIT_PATH_CHARS = 200 -_MAX_UNKNOWN_NAME_CHARS = 128 +_MAX_CLIENT_ID_CHARS = 128 #: Slack added to the upload-derived HTTP body cap for JSON framing and other arguments. BODY_OVERHEAD_BYTES = 1024 * 1024 @@ -357,6 +359,34 @@ class _Record: is_error: bool = True http_status: int | None = None argument_names: list[str] = field(default_factory=list) + rejected_arguments: int = 0 + client_id: str | None = None + session: str | None = None + client_ip: str | None = None + + +def _source_ip(ctx: ServerRequestContext[dict[str, Any]]) -> str | None: + """Client address of an HTTP request (None on stdio); only a valid IP address is recorded.""" + client = getattr(getattr(ctx, "request", None), "client", None) + host = getattr(client, "host", None) + if not isinstance(host, str): + return None + try: + return str(ipaddress.ip_address(host.split("%")[0])) + except ValueError: + return None + + +def _caller_fields(record: _Record, ctx: ServerRequestContext[dict[str, Any]]) -> None: + """Fill the client, session and source address of the call from the verified token.""" + access = get_access_token() + if access is not None: + record.client_id = access.client_id[:_MAX_CLIENT_ID_CHARS] + session_id = (access.claims or {}).get("session_id") + if isinstance(session_id, str) and session_id: + # A short hash: enough to correlate entries of one grant, useless as a credential. + record.session = hashlib.sha256(session_id.encode()).hexdigest()[:12] + record.client_ip = _source_ip(ctx) def _error_payload(kind: str, message: str, **extra: str) -> dict[str, Any]: @@ -409,8 +439,12 @@ class _Dispatcher: request_id = uuid.uuid4().hex set_request_id(request_id) arguments = params.arguments or {} - record = _Record(tool=params.name[:_MAX_UNKNOWN_NAME_CHARS], request_id=request_id) - record.argument_names = sorted(_clip(key) for key in arguments)[:_MAX_AUDIT_ARGUMENT_NAMES] + # The tool name and the argument names come from the caller: only names the + # registry knows are recorded, as the audit entry must never hold a secret a + # model pasted into a name. + record = _Record(tool=f"(unknown tool, {len(params.name)} chars)", request_id=request_id) + record.rejected_arguments = len(arguments) + _caller_fields(record, ctx) # Fail closed: the audit logger is obtained BEFORE anything runs, so a call is # never executed (and its side effects never happen) without a way to record it. try: @@ -428,8 +462,14 @@ class _Dispatcher: entry = self._registry.get(params.name) if entry is None: record.outcome = "unknown_tool" - raise MCPError(types.INVALID_PARAMS, f"Unknown tool: {record.tool}") + raise MCPError(types.INVALID_PARAMS, "Unknown tool") + record.tool = entry.tool.name record.domain = entry.domain + known = set(entry.tool.input_schema.get("properties", {})) + record.argument_names = sorted(key for key in arguments if key in known)[ + :_MAX_AUDIT_ARGUMENT_NAMES + ] + record.rejected_arguments = len(arguments) - len(record.argument_names) # Write-ahead entry: a call whose start cannot be recorded does not run. try: audit.log_tool_call_started(entry.tool.name, request_id, entry.domain) @@ -529,6 +569,8 @@ class _Dispatcher: execution, in :meth:`call_tool`. """ params: dict[str, Any] = {"argument_names": record.argument_names} + if record.rejected_arguments: + params["rejected_argument_count"] = record.rejected_arguments if record.http_status is not None: params["http_status"] = record.http_status try: @@ -543,6 +585,9 @@ class _Dispatcher: identity_login=record.identity.login if record.identity else None, identity_mode=record.identity.mode if record.identity else None, sudo=record.sudo, + client_id=record.client_id, + session=record.session, + client_ip=record.client_ip, outcome=record.outcome, duration_ms=round(duration_ms, 2), ) diff --git a/tests/security/test_audit_fail_closed.py b/tests/security/test_audit_fail_closed.py index ae1965f..bded80a 100644 --- a/tests/security/test_audit_fail_closed.py +++ b/tests/security/test_audit_fail_closed.py @@ -3,13 +3,16 @@ from __future__ import annotations import json +import re from typing import Any import pytest +from mcp import MCPError from pytest_httpx import HTTPXMock from aegis_gitea_mcp.audit import get_audit_logger -from tests.test_server_helpers import API, connected, mock_user +from tests.test_oauth_helpers import BEARER_TOKEN, browser, build_oauth_app, mcp_client, sign_in +from tests.test_server_helpers import API, audit_entries, connected, mock_user REPO_ARGS = {"owner": "octo", "repo": "demo"} @@ -77,3 +80,72 @@ async def test_the_refusal_does_not_leak_the_failure_reason() -> None: text = result.content[0].text # type: ignore[union-attr] assert "No space left" not in text and "OSError" not in text + + +# SEC-12: who made the call + + +async def test_oauth_call_entry_names_the_client_and_never_a_token_or_session_id( + httpx_mock: HTTPXMock, +) -> None: + oauth = build_oauth_app() + flow = sign_in(browser(oauth.app), httpx_mock) + session_ids = list(oauth.store.sessions) + httpx_mock.add_response(url=f"{API}/repos/octo/demo", json={"full_name": "octo/demo"}) + access = flow.tokens["access_token"] + + async with mcp_client(oauth.app, access) as mcp: + result = await mcp.call_tool("gitea_repo_get", REPO_ARGS) + assert not result.is_error + + audit_text = get_audit_logger().log_path.read_text(encoding="utf-8") + (entry,) = audit_entries() + assert entry["client_id"] == flow.client_id + assert re.fullmatch(r"[0-9a-f]{12}", entry["session"]) + assert entry["client_ip"] == "127.0.0.1" + assert entry["identity_login"] == "alice" + assert access not in audit_text + assert flow.tokens["refresh_token"] not in audit_text + assert all(session_id not in audit_text for session_id in session_ids) + + +async def test_static_bearer_call_entry_names_the_static_client(httpx_mock: HTTPXMock) -> None: + mock_user(httpx_mock) + httpx_mock.add_response(url=f"{API}/repos/octo/demo", json={}) + oauth = build_oauth_app() + async with mcp_client(oauth.app, BEARER_TOKEN) as mcp: + await mcp.call_tool("gitea_repo_get", REPO_ARGS) + + (entry,) = audit_entries() + assert entry["client_id"] == "aegis-static-bearer" + assert "session" not in entry + assert BEARER_TOKEN not in get_audit_logger().log_path.read_text(encoding="utf-8") + + +# SEC-18: caller-chosen names + +TOKEN_LIKE = "0123456789abcdef0123456789abcdef01234567" + + +async def test_extra_argument_names_are_counted_not_recorded() -> None: + async with connected() as client: + result = await client.call_tool("gitea_repo_get", {**REPO_ARGS, TOKEN_LIKE: "x"}) + + assert result.is_error + assert TOKEN_LIKE not in result.content[0].text # type: ignore[union-attr] + (entry,) = audit_entries() + assert entry["params"] == {"argument_names": ["owner", "repo"], "rejected_argument_count": 1} + assert TOKEN_LIKE not in get_audit_logger().log_path.read_text(encoding="utf-8") + + +async def test_unknown_tool_names_are_neither_echoed_nor_recorded() -> None: + name = "gitea_" + TOKEN_LIKE + async with connected() as client: + with pytest.raises(MCPError) as caught: + await client.call_tool(name, {TOKEN_LIKE: 1}) + + assert TOKEN_LIKE not in caught.value.message + (entry,) = audit_entries() + assert entry["tool_name"] == f"(unknown tool, {len(name)} chars)" + assert entry["params"] == {"argument_names": [], "rejected_argument_count": 1} + assert TOKEN_LIKE not in get_audit_logger().log_path.read_text(encoding="utf-8") diff --git a/tests/test_server.py b/tests/test_server.py index 84c7281..2d3b165 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -354,9 +354,9 @@ async def test_unknown_tool_is_a_protocol_error_and_is_audited() -> None: await client.call_tool("gitea_does_not_exist", {"a": 1}) assert caught.value.code == -32602 - assert "gitea_does_not_exist" in caught.value.message + assert "gitea_does_not_exist" not in caught.value.message entry = audit_entries()[-1] - assert entry["tool_name"] == "gitea_does_not_exist" + assert entry["tool_name"] == "(unknown tool, 20 chars)" assert entry["outcome"] == "unknown_tool"