diff --git a/docs/configuration.md b/docs/configuration.md index 1750efd..4e24e00 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -50,7 +50,7 @@ when OAuth is on. | `MCP_BEARER_TOKEN` | unset | Static bearer token for HTTP (admin identity). At least 32 characters. Generate with `openssl rand -hex 32`. | | `PUBLIC_BASE_URL` | unset | Public URL behind a reverse proxy. Same URL rules as `GITEA_URL`, and `https` unless the host is loopback. Required for OAuth. | | `ALLOWED_HOSTS` | empty | Extra `Host` header values accepted behind a proxy. The loopback names with the port and the host of `PUBLIC_BASE_URL` are always allowed. Other `Host` values are rejected (DNS-rebinding protection). A non-loopback `MCP_HOST` with neither `PUBLIC_BASE_URL` nor `ALLOWED_HOSTS` logs a startup warning on stderr, because remote clients then get 421. | -| `FORWARDED_ALLOW_IPS` | `127.0.0.1` | Comma list of IP addresses or networks (or `*`) whose `X-Forwarded-For` and `X-Forwarded-Proto` headers are trusted. Set it to the address of the reverse proxy when it is not on the same host, for example a Docker network. Otherwise the per-IP rate limit sees the proxy address for every client. `*` trusts every peer and logs a warning: only use it when the port is reachable solely from the proxy, since clients could otherwise spoof their IP. | +| `FORWARDED_ALLOW_IPS` | `127.0.0.1` | Comma list of IP addresses or networks (or `*`) whose `X-Forwarded-For` and `X-Forwarded-Proto` headers are trusted. Set it to the address of the reverse proxy when it is not on the same host, for example a Docker network. Otherwise the per-IP rate limit sees the proxy address for every client: a request that carries `X-Forwarded-For` from a peer outside this list is not honoured and logs one warning ("add its address to FORWARDED_ALLOW_IPS"). `*` trusts every peer and logs a warning: only use it when the port is reachable solely from the proxy, since clients could otherwise spoof their IP. | ## Per-user OAuth (HTTP only) @@ -96,7 +96,7 @@ listed nor callable. | `SECRET_DETECTION_MODE` | `mask` | `off`, `mask` or `block`. See below. | | `LOG_LEVEL` | `INFO` | `DEBUG`, `INFO`, `WARNING`, `ERROR` or `CRITICAL`. Logs go to stderr. | | `EXPOSE_ERROR_DETAILS` | `false` | When true, the exception class name is added to internal and transport errors. Never a stack trace. Do not enable in production. | -| `RATE_LIMIT_PER_MINUTE` | `120` | At least 1. Per client on the HTTP MCP endpoint, keyed by a hash of the Authorization header, else the client IP. Excess requests get 429. Only the first rejected request per client and minute is written to the audit log. Tracking is in memory and bounded (idle clients are forgotten). | +| `RATE_LIMIT_PER_MINUTE` | `120` | At least 1. Per client on the HTTP MCP endpoint and on the unauthenticated OAuth endpoints (`/register`, `/authorize`, `/token`, `/revoke`, `/oauth/consent`, `/oauth/callback`), keyed by the client IP (an IPv6 client counts per /64) and, on the MCP endpoint, by a hash of the Authorization header. Excess requests get 429. Only the first rejected request per client and minute is written to the audit log. Tracking is in memory and bounded (idle clients are forgotten). | Secret detection: `mask` replaces secret-like strings in results with a placeholder, `block` withholds a whole result that contains one (`secret_blocked`), `off` disables pattern matching. diff --git a/docs/security.md b/docs/security.md index 9f2cb10..cdef3ee 100644 --- a/docs/security.md +++ b/docs/security.md @@ -97,7 +97,7 @@ Denial of service |---|---|---| | Large request bodies | SDK body limit on the MCP endpoint and SDK OAuth routes; upload cap checked before decoding | `server.py` (`max_request_body_size`), `gitea/encoding.py` (consent route: SEC-01) | | Large or hostile responses | 16 MiB text cap, `MAX_BINARY_BYTES`, 64 KiB error bodies, JSON depth 100, item and character limits | `gitea/executor.py` | -| Request floods | Sliding window per IP and per token hash, bounded key sets, rate-limit events audited once per window | `rate_limit.py`, `transports.py` (gaps: SEC-08) | +| Request floods | Sliding window per IP and per token hash, bounded key sets, rate-limit events audited once per window | `rate_limit.py`, `transports.py` (`ForwardedHeadersMiddleware`) | | Unbounded OAuth state | Caps on clients (500), pending sign-ins (1024), codes (1024), sessions per login (50); store pruning | `oauth/provider.py`, `oauth/store.py` (flush by floods: SEC-09) | Elevation of privilege @@ -173,9 +173,10 @@ confirmation, `READ_ONLY`, `TOOLSETS`, `DISABLED_TOOLS` and narrow token scopes - DNS-rebinding protection on the MCP endpoint: `Host` must be a loopback name, the bind address, the `PUBLIC_BASE_URL` host or an `ALLOWED_HOSTS` entry (421 otherwise); a foreign `Origin` gets 403; POST must be `application/json`. -- Rate limits: sliding window per client IP and per Authorization hash on the MCP path and the - unauthenticated OAuth paths, before authentication. Client IPs come from `X-Forwarded-For` - only for `FORWARDED_ALLOW_IPS`. +- Rate limits: sliding window per client IP (IPv6 per /64) and per Authorization hash on the + MCP path and the unauthenticated OAuth paths (`/register`, `/authorize`, `/token`, `/revoke`, + consent, callback), before authentication. Client IPs come from `X-Forwarded-For` only for + `FORWARDED_ALLOW_IPS`; a forwarded header from any other peer is ignored and logged once. - `PUBLIC_BASE_URL` must be https unless loopback. Gitea redirects are never followed. - stdio: stdout carries JSON-RPC only; all logging goes to stderr. diff --git a/src/aegis_gitea_mcp/oauth/components.py b/src/aegis_gitea_mcp/oauth/components.py index 15258d2..350e526 100644 --- a/src/aegis_gitea_mcp/oauth/components.py +++ b/src/aegis_gitea_mcp/oauth/components.py @@ -25,7 +25,14 @@ from aegis_gitea_mcp.oauth.upstream import CALLBACK_PATH, GiteaUpstream from aegis_gitea_mcp.transports import HttpAuthExtras #: Unauthenticated OAuth endpoints that get the per-IP rate limit. -RATE_LIMITED_PATHS = ("/register", "/authorize", CONSENT_PATH, CALLBACK_PATH) +RATE_LIMITED_PATHS = ( + "/register", + "/authorize", + "/token", + "/revoke", + CONSENT_PATH, + CALLBACK_PATH, +) PROTECTED_RESOURCE_PATH = "/.well-known/oauth-protected-resource" diff --git a/src/aegis_gitea_mcp/rate_limit.py b/src/aegis_gitea_mcp/rate_limit.py index f59fcfe..74a095c 100644 --- a/src/aegis_gitea_mcp/rate_limit.py +++ b/src/aegis_gitea_mcp/rate_limit.py @@ -10,6 +10,7 @@ keyed by a hash of the Authorization header and is only a second line. from __future__ import annotations import hashlib +import ipaddress import logging import time from collections import OrderedDict, deque @@ -26,6 +27,24 @@ MAX_IP_KEYS = 100_000 MAX_TOKEN_KEYS = 10_000 +def client_bucket(client_ip: str) -> str: + """Rate-limit key of a client address: IPv6 clients share one bucket per /64. + + A single IPv6 subscriber holds a whole /64, so keying by the full address would + give it an unlimited supply of buckets. IPv4-mapped IPv6 addresses count as + their IPv4 address; anything that is not an IP address is used as is. + """ + try: + address = ipaddress.ip_address(client_ip.split("%")[0]) + except ValueError: + return client_ip + if isinstance(address, ipaddress.IPv6Address): + if address.ipv4_mapped is not None: + return str(address.ipv4_mapped) + return str(ipaddress.IPv6Network((address, 64), strict=False)) + return str(address) + + @dataclass(frozen=True) class RateLimitDecision: """Result of request rate-limit checks.""" @@ -139,8 +158,9 @@ class RequestRateLimiter: Returns: Rate limit decision. """ - if not self._ip_limiter.allow(client_ip): - if self._ip_limiter.first_excess(client_ip): + bucket = client_bucket(client_ip) + if not self._ip_limiter.allow(bucket): + if self._ip_limiter.first_excess(bucket): self._audit_excess( "rate_limit_ip_exceeded", "Per-IP request rate limit exceeded", diff --git a/src/aegis_gitea_mcp/transports.py b/src/aegis_gitea_mcp/transports.py index 6af75bb..0aedab1 100644 --- a/src/aegis_gitea_mcp/transports.py +++ b/src/aegis_gitea_mcp/transports.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio import errno +import ipaddress import logging import sys import time @@ -30,6 +31,7 @@ from starlette.requests import Request from starlette.responses import JSONResponse, Response from starlette.routing import Route from starlette.types import ASGIApp, Receive, Scope, Send +from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware from aegis_gitea_mcp import __version__ from aegis_gitea_mcp.audit import get_audit_logger @@ -189,6 +191,62 @@ class RateLimitMiddleware: await response(scope, receive, send) +# Forwarded headers + + +class ForwardedHeadersMiddleware: + """Applies ``X-Forwarded-For`` and ``-Proto`` from trusted peers and warns about the rest. + + Replaces uvicorn's own proxy-header handling (the runner passes + ``proxy_headers=False``) so the original peer address is still known here: + a request that carries ``X-Forwarded-For`` from a peer outside + ``FORWARDED_ALLOW_IPS`` is not honoured (the peer's own address is used) and + logs one warning, because that usually means the reverse proxy is not listed + (for example a Docker network address) and every client then shares the + proxy's rate-limit bucket. + + Args: + app: The ASGI application. + trusted: ``FORWARDED_ALLOW_IPS`` (``*`` or a comma list of IPs and networks). + """ + + def __init__(self, app: ASGIApp, trusted: str) -> None: + self.trusted = trusted + self._all = trusted.strip() == "*" + self._networks = [ + ipaddress.ip_network(item.strip(), strict=False) + for item in trusted.split(",") + if item.strip() and item.strip() != "*" + ] + self._app = ProxyHeadersMiddleware(app, trusted_hosts=trusted) # type: ignore[arg-type] + self._warned = False + + def _peer_trusted(self, host: str) -> bool: + if self._all: + return True + try: + address = ipaddress.ip_address(host.split("%")[0]) + except ValueError: + return False + return any( + address in network for network in self._networks if network.version == address.version + ) + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] == "http" and not self._warned: + client = scope.get("client") + has_forwarded = any(name == b"x-forwarded-for" for name, _ in scope.get("headers", [])) + if has_forwarded and client and not self._peer_trusted(client[0]): + self._warned = True + logger.warning( + "a request carried X-Forwarded-For from a peer that is not in " + "FORWARDED_ALLOW_IPS, so the header is ignored and the peer address is used " + "as the client address; if this peer is your reverse proxy, add its address " + "to FORWARDED_ALLOW_IPS (all clients otherwise share one rate-limit bucket)" + ) + await self._app(scope, receive, send) # type: ignore[arg-type] + + # HTTP app @@ -501,12 +559,11 @@ def run_http(settings: Settings) -> int: if not any(isinstance(item, AccessLogQueryFilter) for item in access_logger.filters): access_logger.addFilter(AccessLogQueryFilter()) uvicorn.run( - app, + ForwardedHeadersMiddleware(app, settings.forwarded_allow_ips), host=settings.mcp_host, port=settings.mcp_port, log_level=settings.log_level.lower(), - proxy_headers=True, - forwarded_allow_ips=settings.forwarded_allow_ips, + proxy_headers=False, # ForwardedHeadersMiddleware applies them (and warns) server_header=False, ) return 0 diff --git a/tests/security/test_rate_limits.py b/tests/security/test_rate_limits.py new file mode 100644 index 0000000..89057d2 --- /dev/null +++ b/tests/security/test_rate_limits.py @@ -0,0 +1,169 @@ +"""Rate limiting covers the OAuth endpoints, IPv6 subscribers and untrusted proxies (SEC-08).""" + +from __future__ import annotations + +import logging +from typing import Any + +import pytest +from starlette.types import Message, Receive, Scope, Send + +from aegis_gitea_mcp.rate_limit import RequestRateLimiter, client_bucket +from aegis_gitea_mcp.transports import ForwardedHeadersMiddleware +from tests.test_executor_helpers import make_settings +from tests.test_oauth_helpers import browser, build_oauth_app, oauth_settings + +LIMIT = 3 + + +@pytest.fixture(autouse=True) +def _env(mock_env: None) -> None: + """The audit logger and the rate limiter read the process-wide settings.""" + + +@pytest.mark.parametrize( + ("method", "path"), + [ + ("POST", "/token"), + ("POST", "/revoke"), + ("POST", "/register"), + ("GET", "/authorize"), + ("GET", "/oauth/callback"), + ("POST", "/oauth/consent"), + ("POST", "/mcp"), + ], +) +def test_every_unauthenticated_endpoint_is_rate_limited(method: str, path: str) -> None: + client = browser(build_oauth_app(oauth_settings(rate_limit_per_minute=LIMIT)).app) + + statuses = [client.request(method, path).status_code for _ in range(LIMIT + 1)] + + assert statuses[LIMIT] == 429 + assert 429 not in statuses[:LIMIT] + + +def test_the_429_answer_has_retry_after_and_no_detail() -> None: + client = browser(build_oauth_app(oauth_settings(rate_limit_per_minute=1)).app) + client.post("/token") + + response = client.post("/token") + + assert response.status_code == 429 + assert response.headers["retry-after"] == "60" + assert response.json() == {"error": {"kind": "rate_limited", "message": "Too many requests"}} + + +def test_ipv6_addresses_of_one_slash_64_share_a_bucket() -> None: + limiter = RequestRateLimiter(make_settings(rate_limit_per_minute=2)) + + first = limiter.check("2001:db8:1:2::1", None) + second = limiter.check("2001:db8:1:2:ffff:ffff:ffff:ffff", None) + third = limiter.check("2001:db8:1:2::abcd", None) + other_network = limiter.check("2001:db8:1:3::1", None) + + assert first.allowed and second.allowed + assert not third.allowed + assert other_network.allowed + + +def test_ipv4_clients_and_mapped_addresses_keep_their_own_bucket() -> None: + assert client_bucket("192.0.2.10") == "192.0.2.10" + assert client_bucket("::ffff:192.0.2.10") == "192.0.2.10" + assert client_bucket("2001:db8:1:2::1") == "2001:db8:1:2::/64" + assert client_bucket("fe80::1%eth0") == "fe80::/64" + assert client_bucket("testclient") == "testclient" + limiter = RequestRateLimiter(make_settings(rate_limit_per_minute=1)) + assert limiter.check("192.0.2.10", None).allowed + assert limiter.check("192.0.2.11", None).allowed + assert not limiter.check("192.0.2.10", None).allowed + + +# Forwarded headers + + +class Recorder: + """Downstream ASGI app that remembers the client address it saw.""" + + def __init__(self) -> None: + self.clients: list[Any] = [] + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + self.clients.append(scope.get("client")) + + +def http_scope(peer: str, forwarded: str | None) -> Scope: + headers = [(b"host", b"mcp.example.com")] + if forwarded is not None: + headers.append((b"x-forwarded-for", forwarded.encode())) + return { + "type": "http", + "method": "GET", + "path": "/", + "scheme": "http", + "headers": headers, + "client": (peer, 5000), + } + + +async def noop_receive() -> Message: + return {"type": "http.disconnect"} + + +async def noop_send(message: Message) -> None: + return None + + +async def test_forwarded_for_from_an_untrusted_peer_is_ignored_and_warned_about_once( + caplog: pytest.LogCaptureFixture, +) -> None: + app = Recorder() + middleware = ForwardedHeadersMiddleware(app, "127.0.0.1") + + with caplog.at_level(logging.WARNING, logger="aegis_gitea_mcp.transports"): + for _ in range(3): + await middleware(http_scope("172.18.0.5", "203.0.113.9"), noop_receive, noop_send) + + # Spoofed header: the peer address stays the client address. + assert app.clients == [("172.18.0.5", 5000)] * 3 + warnings = [r for r in caplog.records if "FORWARDED_ALLOW_IPS" in r.getMessage()] + assert len(warnings) == 1 + assert "203.0.113.9" not in warnings[0].getMessage() + assert "172.18.0.5" not in warnings[0].getMessage() + + +async def test_forwarded_for_from_a_trusted_peer_is_applied_without_a_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + app = Recorder() + middleware = ForwardedHeadersMiddleware(app, "172.18.0.0/16, 127.0.0.1") + + with caplog.at_level(logging.WARNING, logger="aegis_gitea_mcp.transports"): + await middleware(http_scope("172.18.0.5", "203.0.113.9"), noop_receive, noop_send) + await middleware(http_scope("127.0.0.1", "203.0.113.10"), noop_receive, noop_send) + + assert [client[0] for client in app.clients] == ["203.0.113.9", "203.0.113.10"] + assert caplog.records == [] + + +async def test_requests_without_forwarded_for_never_warn(caplog: pytest.LogCaptureFixture) -> None: + app = Recorder() + middleware = ForwardedHeadersMiddleware(app, "127.0.0.1") + + with caplog.at_level(logging.WARNING, logger="aegis_gitea_mcp.transports"): + await middleware(http_scope("198.51.100.7", None), noop_receive, noop_send) + + assert caplog.records == [] + assert app.clients == [("198.51.100.7", 5000)] + + +async def test_wildcard_trusts_every_peer_without_a_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + app = Recorder() + middleware = ForwardedHeadersMiddleware(app, "*") + + with caplog.at_level(logging.WARNING, logger="aegis_gitea_mcp.transports"): + await middleware(http_scope("198.51.100.7", "203.0.113.9"), noop_receive, noop_send) + + assert app.clients[0][0] == "203.0.113.9" + assert caplog.records == [] diff --git a/tests/test_transports_startup.py b/tests/test_transports_startup.py index d2c7a3c..6881b32 100644 --- a/tests/test_transports_startup.py +++ b/tests/test_transports_startup.py @@ -16,6 +16,7 @@ from aegis_gitea_mcp.cli import main from aegis_gitea_mcp.config import Settings from aegis_gitea_mcp.oauth import OAuthSetupError from aegis_gitea_mcp.transports import ( + ForwardedHeadersMiddleware, HttpAuthExtras, http_startup_errors, load_oauth_components, @@ -83,8 +84,10 @@ def test_non_loopback_bind_with_bearer_is_allowed(uvicorn_spy: UvicornSpy) -> No assert callable(app) assert kwargs["host"] == ANY_HOST assert kwargs["port"] == 9000 - assert kwargs["proxy_headers"] is True - assert kwargs["forwarded_allow_ips"] == "127.0.0.1" + # Proxy headers are applied by ForwardedHeadersMiddleware, not by uvicorn. + assert kwargs["proxy_headers"] is False + assert isinstance(app, ForwardedHeadersMiddleware) + assert app.trusted == "127.0.0.1" assert kwargs["log_level"] == "info" assert kwargs["server_header"] is False @@ -99,7 +102,7 @@ def test_loopback_without_auth_runs(uvicorn_spy: UvicornSpy) -> None: def test_forwarded_allow_ips_setting_is_passed_to_uvicorn(uvicorn_spy: UvicornSpy) -> None: assert run_http(server_settings(forwarded_allow_ips="10.0.0.0/8, 172.18.0.2")) == 0 - assert uvicorn_spy.calls[0][1]["forwarded_allow_ips"] == "10.0.0.0/8,172.18.0.2" + assert uvicorn_spy.calls[0][0].trusted == "10.0.0.0/8,172.18.0.2" def test_forwarded_allow_ips_wildcard_needs_an_explicit_value_and_warns( @@ -109,7 +112,7 @@ def test_forwarded_allow_ips_wildcard_needs_an_explicit_value_and_warns( assert run_http(server_settings(forwarded_allow_ips="*")) == 0 - assert uvicorn_spy.calls[0][1]["forwarded_allow_ips"] == "*" + assert uvicorn_spy.calls[0][0].trusted == "*" assert "FORWARDED_ALLOW_IPS=*" in capsys.readouterr().err