From dce21b5685073e5b82161da318f5f3823ad66d85 Mon Sep 17 00:00:00 2001 From: monte Date: Tue, 21 Jul 2026 23:11:41 +0200 Subject: [PATCH 1/2] feat(backend): read and unban auto-ban stick-table entries via HAProxy Runtime API Adds an admin-level stats socket to the generated haproxy.cfg (the existing socket is operator-level, which show table but not clear table) and a BanListService that lists tracked/banned source IPs across every auto-ban-enabled vhost and clears an IP from all of its ban tables on unban, exposed via GET/DELETE /security/banned-ips. --- configs/haproxy/haproxy.cfg | 4 + src/backend/app/config.py | 4 + src/backend/app/main.py | 2 + src/backend/app/routers/security.py | 49 ++++ src/backend/app/schemas/security.py | 29 ++ src/backend/app/services/ban_list_service.py | 181 +++++++++++++ src/backend/app/templates/haproxy.cfg.j2 | 4 + .../tests/integration/test_security_router.py | 144 ++++++++++ .../tests/unit/test_ban_list_service.py | 255 ++++++++++++++++++ .../unit/test_config_renderer_haproxy.py | 7 + 10 files changed, 679 insertions(+) create mode 100644 src/backend/app/routers/security.py create mode 100644 src/backend/app/schemas/security.py create mode 100644 src/backend/app/services/ban_list_service.py create mode 100644 src/backend/tests/integration/test_security_router.py create mode 100644 src/backend/tests/unit/test_ban_list_service.py diff --git a/configs/haproxy/haproxy.cfg b/configs/haproxy/haproxy.cfg index 760e592..e17c0c1 100644 --- a/configs/haproxy/haproxy.cfg +++ b/configs/haproxy/haproxy.cfg @@ -7,6 +7,10 @@ global maxconn 2000 # Operator level avoids exposing administrative Runtime API commands. stats socket /tmp/haproxy.sock mode 660 level operator + # Admin level, reachable only from the backend over the shared runtime + # volume. Needed for `clear table` (unban) — see #276 — which the + # operator-level socket above deliberately cannot perform. + stats socket /var/run/haproxy/admin.sock mode 660 level admin defaults mode http diff --git a/src/backend/app/config.py b/src/backend/app/config.py index 9088bbd..d06b5ed 100644 --- a/src/backend/app/config.py +++ b/src/backend/app/config.py @@ -76,6 +76,8 @@ class Settings(EnvFileSettings): haproxy_validation_timeout_seconds: int = 10 haproxy_master_socket_path: str = "/var/run/haproxy/master.sock" haproxy_reload_timeout_seconds: int = 10 + haproxy_stats_socket_path: str = "/var/run/haproxy/admin.sock" + haproxy_stats_timeout_seconds: int = 10 log_retention_days: int = 30 @field_validator("database_url") @@ -87,6 +89,7 @@ def database_url_must_not_be_empty(cls, value: str) -> str: "runtime_generated_config_root", "haproxy_validation_binary", "haproxy_master_socket_path", + "haproxy_stats_socket_path", ) @classmethod def runtime_paths_must_not_be_empty(cls, value: str) -> str: @@ -97,6 +100,7 @@ def runtime_paths_must_not_be_empty(cls, value: str) -> str: @field_validator( "haproxy_validation_timeout_seconds", "haproxy_reload_timeout_seconds", + "haproxy_stats_timeout_seconds", ) @classmethod def timeout_settings_must_be_positive(cls, value: int) -> int: diff --git a/src/backend/app/main.py b/src/backend/app/main.py index e41327b..de2eced 100644 --- a/src/backend/app/main.py +++ b/src/backend/app/main.py @@ -32,6 +32,7 @@ rule_exclusions, rule_overrides, runtime_status, + security, vhosts, ) from app.services.config_apply import seed_runtime_config @@ -143,6 +144,7 @@ def _seed_runtime_config() -> None: app.include_router(rule_exclusions.router) app.include_router(rule_overrides.router) app.include_router(runtime_status.router) +app.include_router(security.router) app.include_router(vhosts.router) diff --git a/src/backend/app/routers/security.py b/src/backend/app/routers/security.py new file mode 100644 index 0000000..dbc1c6a --- /dev/null +++ b/src/backend/app/routers/security.py @@ -0,0 +1,49 @@ +"""Security API router — auto-ban list read/unban via HAProxy Runtime API.""" + +from fastapi import APIRouter, Depends, HTTPException, status +from sqlalchemy.orm import Session + +from app.database import get_db +from app.dependencies import require_admin +from app.models.user import User +from app.schemas.security import BannedIpListResponse, UnbanResponse +from app.services.ban_list_service import ( + BanListService, + InvalidIpError, + RuntimeApiError, +) + +router = APIRouter(prefix="/security", tags=["security"]) + + +@router.get("/banned-ips", response_model=BannedIpListResponse) +def list_banned_ips( + db: Session = Depends(get_db), + _: User = Depends(require_admin), +) -> BannedIpListResponse: + """Returns tracked/banned source IPs across auto-ban-enabled vhosts (admin only).""" + service = BanListService(db) + items = service.list_banned() + return BannedIpListResponse(items=items, total=len(items)) + + +@router.delete("/banned-ips/{ip}", response_model=UnbanResponse) +def unban_ip( + ip: str, + db: Session = Depends(get_db), + _: User = Depends(require_admin), +) -> UnbanResponse: + """Clears a source IP from every active ban stick-table (admin only).""" + service = BanListService(db) + try: + return service.unban(ip) + except InvalidIpError as error: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail=str(error), + ) from error + except RuntimeApiError as error: + raise HTTPException( + status_code=status.HTTP_502_BAD_GATEWAY, + detail="Failed to reach HAProxy Runtime API", + ) from error diff --git a/src/backend/app/schemas/security.py b/src/backend/app/schemas/security.py new file mode 100644 index 0000000..23d65ae --- /dev/null +++ b/src/backend/app/schemas/security.py @@ -0,0 +1,29 @@ +"""Pydantic schemas for the ban-list / unban security endpoints.""" + +from pydantic import BaseModel + + +class BannedIpResponse(BaseModel): + """One tracked entry in a vhost's auto-ban stick-table.""" + + ip: str + vhost_id: int + domain: str + gpc0: int + ban_threshold: int + banned: bool + expires_in_seconds: int + + +class BannedIpListResponse(BaseModel): + """Response body returned by GET /security/banned-ips.""" + + items: list[BannedIpResponse] + total: int + + +class UnbanResponse(BaseModel): + """Response body returned by DELETE /security/banned-ips/{ip}.""" + + ip: str + cleared: int diff --git a/src/backend/app/services/ban_list_service.py b/src/backend/app/services/ban_list_service.py new file mode 100644 index 0000000..2460a9c --- /dev/null +++ b/src/backend/app/services/ban_list_service.py @@ -0,0 +1,181 @@ +"""Ban-list service — reads/clears auto-ban entries via HAProxy Runtime API. + +Talks to the admin-level stats socket (`settings.haproxy_stats_socket_path`) +emitted in the generated `haproxy.cfg` (see #275, #276). Each vhost with +DDoS protection and auto-ban enabled owns a stick-table named +`st_ban_vhost_` (see `config_generator.py::_to_haproxy_context`); this +module enumerates those tables' contents and can clear entries from them. +""" + +from __future__ import annotations + +import ipaddress +import logging +import re +import socket +from dataclasses import dataclass + +from sqlalchemy.orm import Session, selectinload + +from app.config import settings +from app.models.vhost import VHost +from app.schemas.security import BannedIpResponse, UnbanResponse + +logger = logging.getLogger(__name__) + +_TABLE_ENTRY_RE = re.compile( + r"key=(?P\S+).*?exp=(?P\d+).*?gpc0=(?P\d+)" +) + + +class BanListError(Exception): + """Base class for ban-list domain errors.""" + + +class InvalidIpError(BanListError): + """Raised when a supplied IP address string cannot be parsed.""" + + def __init__(self, ip: str) -> None: + self.ip = ip + super().__init__(f"'{ip}' is not a valid IP address") + + +class RuntimeApiError(BanListError): + """Raised when the HAProxy Runtime API socket cannot be reached at all.""" + + +@dataclass(frozen=True) +class BanTableEntry: + """One parsed row from a `show table` response.""" + + ip: str + gpc0: int + expires_in_seconds: int + + +def _send_runtime_command(command: str) -> str: + """Send one Runtime API command over the admin stats socket and return output.""" + socket_path = settings.haproxy_stats_socket_path + try: + with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as client: + client.settimeout(settings.haproxy_stats_timeout_seconds) + client.connect(socket_path) + client.sendall(command.encode("utf-8") + b"\n") + client.shutdown(socket.SHUT_WR) + output_chunks: list[bytes] = [] + while True: + chunk = client.recv(4096) + if not chunk: + break + output_chunks.append(chunk) + except OSError as error: + raise RuntimeApiError(str(error)) from error + + return b"".join(output_chunks).decode("utf-8", errors="replace") + + +def _parse_show_table(raw: str) -> list[BanTableEntry]: + """Parse `show table ` output into structured entries. + + Example line: + 0x...: key=192.0.2.7 use=0 exp=540000 gpc0=15 + `exp` is milliseconds remaining; converted to whole seconds (rounded up + so a live entry never reports 0 remaining). + """ + entries: list[BanTableEntry] = [] + for line in raw.splitlines(): + match = _TABLE_ENTRY_RE.search(line) + if match is None: + continue + exp_ms = int(match.group("exp")) + entries.append( + BanTableEntry( + ip=match.group("key"), + gpc0=int(match.group("gpc0")), + expires_in_seconds=(exp_ms + 999) // 1000, + ) + ) + return entries + + +class BanListService: + """Reads and clears auto-ban stick-table entries via the Runtime API.""" + + def __init__(self, db: Session) -> None: + self.db = db + + def list_banned(self) -> list[BannedIpResponse]: + """Return every tracked/banned entry across all auto-ban-enabled vhosts.""" + results: list[BannedIpResponse] = [] + for vhost, table_name, ban_threshold in self._active_ban_tables(): + try: + raw = _send_runtime_command(f"show table {table_name}") + entries = _parse_show_table(raw) + except RuntimeApiError: + logger.exception( + "ban-list failed to read table %s for vhost %s", + table_name, + vhost.id, + ) + continue + + for entry in entries: + results.append( + BannedIpResponse( + ip=entry.ip, + vhost_id=vhost.id, + domain=vhost.domain, + gpc0=entry.gpc0, + ban_threshold=ban_threshold, + banned=entry.gpc0 > ban_threshold, + expires_in_seconds=entry.expires_in_seconds, + ) + ) + return results + + def unban(self, ip: str) -> UnbanResponse: + """Clear an IP from every active ban table; return the number cleared. + + Tolerates individual table failures (one dead table must not block + clearing the others) but raises `RuntimeApiError` if every attempted + table failed, so a fully unreachable Runtime API surfaces as an + error instead of a silent no-op `cleared=0`. + """ + try: + ipaddress.ip_address(ip) + except ValueError as error: + raise InvalidIpError(ip) from error + + tables = self._active_ban_tables() + cleared = 0 + failures = 0 + for _vhost, table_name, _threshold in tables: + try: + _send_runtime_command(f"clear table {table_name} key {ip}") + cleared += 1 + except RuntimeApiError: + logger.exception( + "ban-list failed to clear key %s in table %s", ip, table_name + ) + failures += 1 + + if tables and failures == len(tables): + raise RuntimeApiError("Failed to reach HAProxy Runtime API") + + return UnbanResponse(ip=ip, cleared=cleared) + + def _active_ban_tables(self) -> list[tuple[VHost, str, int]]: + vhosts = ( + self.db.query(VHost) + .options(selectinload(VHost.policy)) + .order_by(VHost.id.asc()) + .all() + ) + return [ + (vhost, f"st_ban_vhost_{vhost.id}", vhost.policy.ban_threshold) + for vhost in vhosts + if vhost.is_active + and vhost.policy is not None + and vhost.policy.ddos_protection_enabled + and vhost.policy.auto_ban_enabled + ] diff --git a/src/backend/app/templates/haproxy.cfg.j2 b/src/backend/app/templates/haproxy.cfg.j2 index 291b9cb..71356d8 100644 --- a/src/backend/app/templates/haproxy.cfg.j2 +++ b/src/backend/app/templates/haproxy.cfg.j2 @@ -7,6 +7,10 @@ global maxconn 2000 # Operator level avoids exposing administrative Runtime API commands. stats socket /tmp/haproxy.sock mode 660 level operator + # Admin level, reachable only from the backend over the shared runtime + # volume. Needed for `clear table` (unban) — see #276 — which the + # operator-level socket above deliberately cannot perform. + stats socket /var/run/haproxy/admin.sock mode 660 level admin defaults mode http diff --git a/src/backend/tests/integration/test_security_router.py b/src/backend/tests/integration/test_security_router.py new file mode 100644 index 0000000..a077fb7 --- /dev/null +++ b/src/backend/tests/integration/test_security_router.py @@ -0,0 +1,144 @@ +"""Integration tests for the /security ban-list endpoints.""" + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy.orm import Session + +from app.models.policy import Policy +from app.models.vhost import VHost +from app.services import ban_list_service + + +def _create_banned_vhost(db: Session) -> VHost: + policy = Policy( + name="Auto-ban policy", + ddos_protection_enabled=True, + rate_limit_requests=100, + rate_limit_window_seconds=10, + max_connections_per_ip=20, + auto_ban_enabled=True, + ban_threshold=10, + ban_duration_seconds=600, + ) + vhost = VHost( + domain="app.example.com", + backend_url="http://backend:8000", + is_active=True, + ssl_enabled=False, + policy=policy, + ) + db.add_all([policy, vhost]) + db.commit() + return vhost + + +def test_list_banned_ips_requires_auth(client: TestClient) -> None: + response = client.get("/security/banned-ips") + + assert response.status_code == 401 + + +def test_list_banned_ips_forbidden_for_viewer( + client: TestClient, viewer_token: dict[str, str] +) -> None: + response = client.get("/security/banned-ips", headers=viewer_token) + + assert response.status_code == 403 + + +def test_list_banned_ips_returns_entries_for_admin( + client: TestClient, + admin_token: dict[str, str], + db: Session, + monkeypatch: pytest.MonkeyPatch, +) -> None: + vhost = _create_banned_vhost(db) + + monkeypatch.setattr( + ban_list_service, + "_send_runtime_command", + lambda _cmd: "0x1: key=203.0.113.5 use=0 exp=60000 gpc0=99\n", + ) + + response = client.get("/security/banned-ips", headers=admin_token) + + assert response.status_code == 200 + body = response.json() + assert body["total"] == 1 + assert body["items"] == [ + { + "ip": "203.0.113.5", + "vhost_id": vhost.id, + "domain": "app.example.com", + "gpc0": 99, + "ban_threshold": 10, + "banned": True, + "expires_in_seconds": 60, + } + ] + + +def test_list_banned_ips_returns_empty_when_nothing_tracked( + client: TestClient, admin_token: dict[str, str] +) -> None: + response = client.get("/security/banned-ips", headers=admin_token) + + assert response.status_code == 200 + assert response.json() == {"items": [], "total": 0} + + +def test_unban_requires_auth(client: TestClient) -> None: + response = client.delete("/security/banned-ips/203.0.113.5") + + assert response.status_code == 401 + + +def test_unban_forbidden_for_viewer( + client: TestClient, viewer_token: dict[str, str] +) -> None: + response = client.delete( + "/security/banned-ips/203.0.113.5", headers=viewer_token + ) + + assert response.status_code == 403 + + +def test_unban_clears_entry_for_admin( + client: TestClient, + admin_token: dict[str, str], + db: Session, + monkeypatch: pytest.MonkeyPatch, +) -> None: + _create_banned_vhost(db) + monkeypatch.setattr(ban_list_service, "_send_runtime_command", lambda _cmd: "") + + response = client.delete("/security/banned-ips/203.0.113.5", headers=admin_token) + + assert response.status_code == 200 + assert response.json() == {"ip": "203.0.113.5", "cleared": 1} + + +def test_unban_returns_422_for_invalid_ip( + client: TestClient, admin_token: dict[str, str] +) -> None: + response = client.delete("/security/banned-ips/not-an-ip", headers=admin_token) + + assert response.status_code == 422 + + +def test_unban_returns_502_when_runtime_api_unreachable( + client: TestClient, + admin_token: dict[str, str], + db: Session, + monkeypatch: pytest.MonkeyPatch, +) -> None: + _create_banned_vhost(db) + + def fake_send(_command: str) -> str: + raise ban_list_service.RuntimeApiError("connection refused") + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + response = client.delete("/security/banned-ips/203.0.113.5", headers=admin_token) + + assert response.status_code == 502 diff --git a/src/backend/tests/unit/test_ban_list_service.py b/src/backend/tests/unit/test_ban_list_service.py new file mode 100644 index 0000000..8151f99 --- /dev/null +++ b/src/backend/tests/unit/test_ban_list_service.py @@ -0,0 +1,255 @@ +"""Unit tests for the ban-list Runtime API service.""" + +from unittest.mock import Mock + +import pytest +from sqlalchemy.orm import Session + +from app.models.policy import Policy +from app.models.vhost import VHost +from app.services import ban_list_service +from app.services.ban_list_service import ( + BanListService, + BanTableEntry, + InvalidIpError, + RuntimeApiError, + _parse_show_table, +) + + +def _sample_policy(**overrides: object) -> Policy: + defaults = dict( + name="DDoS+Ban", + ddos_protection_enabled=True, + rate_limit_requests=100, + rate_limit_window_seconds=10, + max_connections_per_ip=20, + auto_ban_enabled=True, + ban_threshold=10, + ban_duration_seconds=600, + ) + defaults.update(overrides) + return Policy(**defaults) + + +def _sample_vhost(policy: Policy | None, **overrides: object) -> VHost: + defaults = dict( + domain="app.example.com", + backend_url="http://backend:8000", + is_active=True, + ssl_enabled=False, + policy=policy, + ) + defaults.update(overrides) + return VHost(**defaults) + + +class TestParseShowTable: + def test_parses_banned_entry(self) -> None: + raw = ( + "# table: st_ban_vhost_1, type: ip, size:102400, used:1\n" + "0x7f: key=192.0.2.7 use=0 exp=540000 gpc0=15\n" + ) + + entries = _parse_show_table(raw) + + assert entries == [ + BanTableEntry(ip="192.0.2.7", gpc0=15, expires_in_seconds=540) + ] + + def test_parses_sub_threshold_tracked_entry(self) -> None: + raw = "0x1: key=198.51.100.9 use=1 exp=1500 gpc0=2\n" + + entries = _parse_show_table(raw) + + assert entries[0].gpc0 == 2 + assert entries[0].expires_in_seconds == 2 # ceil(1500ms) -> 2s + + def test_empty_table_returns_no_entries(self) -> None: + raw = "# table: st_ban_vhost_1, type: ip, size:102400, used:0\n" + + assert _parse_show_table(raw) == [] + + def test_header_only_output_returns_no_entries(self) -> None: + assert _parse_show_table("") == [] + + def test_ignores_unparseable_lines(self) -> None: + raw = "some unrelated garbage line\nkey=only-partial\n" + + assert _parse_show_table(raw) == [] + + +class TestListBanned: + def test_lists_entries_only_for_ddos_and_auto_ban_enabled_active_vhosts( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + eligible_policy = _sample_policy(name="Eligible") + no_ban_policy = _sample_policy(name="NoBan", auto_ban_enabled=False) + no_ddos_policy = _sample_policy( + name="NoDdos", ddos_protection_enabled=False, auto_ban_enabled=False + ) + + eligible_vhost = _sample_vhost(eligible_policy, domain="eligible.example.com") + no_ban_vhost = _sample_vhost(no_ban_policy, domain="noban.example.com") + no_ddos_vhost = _sample_vhost(no_ddos_policy, domain="noddos.example.com") + no_policy_vhost = _sample_vhost(None, domain="nopolicy.example.com") + inactive_vhost = _sample_vhost( + eligible_policy, domain="inactive.example.com", is_active=False + ) + + db.add_all( + [ + eligible_policy, + no_ban_policy, + no_ddos_policy, + eligible_vhost, + no_ban_vhost, + no_ddos_vhost, + no_policy_vhost, + inactive_vhost, + ] + ) + db.flush() + + sent_commands: list[str] = [] + + def fake_send(command: str) -> str: + sent_commands.append(command) + return "0x1: key=203.0.113.5 use=0 exp=60000 gpc0=99\n" + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + service = BanListService(db) + results = service.list_banned() + + assert sent_commands == [f"show table st_ban_vhost_{eligible_vhost.id}"] + assert len(results) == 1 + entry = results[0] + assert entry.ip == "203.0.113.5" + assert entry.vhost_id == eligible_vhost.id + assert entry.domain == "eligible.example.com" + assert entry.gpc0 == 99 + assert entry.ban_threshold == 10 + assert entry.banned is True + + def test_marks_sub_threshold_entries_as_not_banned( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy = _sample_policy(ban_threshold=10) + vhost = _sample_vhost(policy) + db.add_all([policy, vhost]) + db.flush() + + monkeypatch.setattr( + ban_list_service, + "_send_runtime_command", + lambda _cmd: "0x1: key=192.0.2.1 use=0 exp=1000 gpc0=3\n", + ) + + results = BanListService(db).list_banned() + + assert results[0].banned is False + + def test_skips_table_that_raises_and_continues_with_others( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy_a = _sample_policy(name="A") + policy_b = _sample_policy(name="B") + vhost_a = _sample_vhost(policy_a, domain="a.example.com") + vhost_b = _sample_vhost(policy_b, domain="b.example.com") + db.add_all([policy_a, policy_b, vhost_a, vhost_b]) + db.flush() + + def fake_send(command: str) -> str: + if f"vhost_{vhost_a.id}" in command: + raise RuntimeApiError("connection refused") + return "0x1: key=192.0.2.1 use=0 exp=1000 gpc0=1\n" + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + results = BanListService(db).list_banned() + + assert len(results) == 1 + assert results[0].vhost_id == vhost_b.id + + def test_returns_empty_list_when_no_vhost_qualifies(self, db: Session) -> None: + assert BanListService(db).list_banned() == [] + + +class TestUnban: + def test_clears_ip_from_every_active_table( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy_a = _sample_policy(name="A") + policy_b = _sample_policy(name="B") + vhost_a = _sample_vhost(policy_a, domain="a.example.com") + vhost_b = _sample_vhost(policy_b, domain="b.example.com") + db.add_all([policy_a, policy_b, vhost_a, vhost_b]) + db.flush() + + sent_commands: list[str] = [] + + def fake_send(command: str) -> str: + sent_commands.append(command) + return "" + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + result = BanListService(db).unban("203.0.113.9") + + assert result.ip == "203.0.113.9" + assert result.cleared == 2 + assert sent_commands == [ + f"clear table st_ban_vhost_{vhost_a.id} key 203.0.113.9", + f"clear table st_ban_vhost_{vhost_b.id} key 203.0.113.9", + ] + + def test_rejects_invalid_ip_without_calling_socket(self, db: Session) -> None: + send = Mock() + with pytest.MonkeyPatch.context() as mp: + mp.setattr(ban_list_service, "_send_runtime_command", send) + with pytest.raises(InvalidIpError): + BanListService(db).unban("not-an-ip") + send.assert_not_called() + + def test_partial_table_failure_still_counts_successes( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy_a = _sample_policy(name="A") + policy_b = _sample_policy(name="B") + vhost_a = _sample_vhost(policy_a, domain="a.example.com") + vhost_b = _sample_vhost(policy_b, domain="b.example.com") + db.add_all([policy_a, policy_b, vhost_a, vhost_b]) + db.flush() + + def fake_send(command: str) -> str: + if f"vhost_{vhost_a.id}" in command: + raise RuntimeApiError("connection refused") + return "" + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + result = BanListService(db).unban("203.0.113.9") + + assert result.cleared == 1 + + def test_raises_runtime_api_error_when_every_table_fails( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy = _sample_policy() + vhost = _sample_vhost(policy) + db.add_all([policy, vhost]) + db.flush() + + def fake_send(_command: str) -> str: + raise RuntimeApiError("connection refused") + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + with pytest.raises(RuntimeApiError): + BanListService(db).unban("203.0.113.9") + + def test_returns_zero_cleared_when_no_vhost_qualifies(self, db: Session) -> None: + result = BanListService(db).unban("203.0.113.9") + + assert result.cleared == 0 diff --git a/src/backend/tests/unit/test_config_renderer_haproxy.py b/src/backend/tests/unit/test_config_renderer_haproxy.py index c2168fd..01d4802 100644 --- a/src/backend/tests/unit/test_config_renderer_haproxy.py +++ b/src/backend/tests/unit/test_config_renderer_haproxy.py @@ -62,6 +62,13 @@ def test_haproxy_template_renders_m1_reference_modulo_whitespace() -> None: assert _normalise_config(rendered) == _normalise_config(reference) +def test_haproxy_template_renders_admin_stats_socket_for_runtime_api() -> None: + rendered = render_haproxy_cfg(_m1_reference_context()) + + assert "stats socket /tmp/haproxy.sock mode 660 level operator" in rendered + assert "stats socket /var/run/haproxy/admin.sock mode 660 level admin" in rendered + + def test_haproxy_template_parameterises_vhost_and_backend() -> None: rendered = render_haproxy_cfg( HaproxyRenderContext( From 01e1849127c15e0d17ee820be70d6d29957076b5 Mon Sep 17 00:00:00 2001 From: monte Date: Tue, 21 Jul 2026 23:31:49 +0200 Subject: [PATCH 2/2] fix(backend): address ban-list review feedback on socket perms and error handling - Fix admin stats socket to mode 666: HAProxy runs as root while the backend drops to an unprivileged user via gosu, so a root-owned mode 660 socket was unreadable/unwritable by the backend, breaking every list/unban call. Applied to the template, static bootstrap config, and release seed config for parity. - Classify HAProxy Runtime API error replies (unknown table, permission denied, ...) inside _send_runtime_command, since HAProxy reports those as plain text rather than a socket-level failure; previously an unban could report a table as cleared even when HAProxy rejected the command. - Make list_banned raise RuntimeApiError when every ban table read fails, mirroring unban, so a fully unreachable Runtime API surfaces as a 502 instead of an empty list indistinguishable from "nothing tracked". - Document the new HAPROXY_STATS_SOCKET_PATH / HAPROXY_STATS_TIMEOUT_SECONDS settings in .env.example. --- configs/haproxy/haproxy.cfg | 10 ++-- release/haproxy/haproxy.cfg | 8 +++ src/backend/.env.example | 2 + src/backend/app/routers/security.py | 8 ++- src/backend/app/services/ban_list_service.py | 47 +++++++++++++++-- src/backend/app/templates/haproxy.cfg.j2 | 10 ++-- .../tests/integration/test_security_router.py | 18 +++++++ .../tests/unit/test_ban_list_service.py | 52 +++++++++++++++++++ .../unit/test_config_renderer_haproxy.py | 2 +- 9 files changed, 145 insertions(+), 12 deletions(-) diff --git a/configs/haproxy/haproxy.cfg b/configs/haproxy/haproxy.cfg index e17c0c1..ba1bbc0 100644 --- a/configs/haproxy/haproxy.cfg +++ b/configs/haproxy/haproxy.cfg @@ -8,9 +8,13 @@ global # Operator level avoids exposing administrative Runtime API commands. stats socket /tmp/haproxy.sock mode 660 level operator # Admin level, reachable only from the backend over the shared runtime - # volume. Needed for `clear table` (unban) — see #276 — which the - # operator-level socket above deliberately cannot perform. - stats socket /var/run/haproxy/admin.sock mode 660 level admin + # volume (not exposed to the host). Needed for `clear table` (unban) — + # see #276 — which the operator-level socket above deliberately cannot + # perform. mode 666 matches the master socket (docker-compose command, + # `-S ...master.sock,mode,666,...`): HAProxy runs as root while the + # backend drops to an unprivileged user via gosu, so a root-owned 660 + # socket would not be readable/writable by the backend. + stats socket /var/run/haproxy/admin.sock mode 666 level admin defaults mode http diff --git a/release/haproxy/haproxy.cfg b/release/haproxy/haproxy.cfg index 37c34f1..a2d3d8a 100644 --- a/release/haproxy/haproxy.cfg +++ b/release/haproxy/haproxy.cfg @@ -7,6 +7,14 @@ global maxconn 2000 # Operator level avoids exposing administrative Runtime API commands. stats socket /tmp/haproxy.sock mode 660 level operator + # Admin level, reachable only from the backend over the shared runtime + # volume (not exposed to the host). Needed for `clear table` (unban) — + # see #276 — which the operator-level socket above deliberately cannot + # perform. mode 666 matches the master socket (docker-compose command, + # `-S ...master.sock,mode,666,...`): HAProxy runs as root while the + # backend drops to an unprivileged user via gosu, so a root-owned 660 + # socket would not be readable/writable by the backend. + stats socket /var/run/haproxy/admin.sock mode 666 level admin defaults mode http diff --git a/src/backend/.env.example b/src/backend/.env.example index 9b1573e..18615ad 100644 --- a/src/backend/.env.example +++ b/src/backend/.env.example @@ -42,6 +42,8 @@ DEBUG=false # HAPROXY_VALIDATION_TIMEOUT_SECONDS=10 # HAPROXY_MASTER_SOCKET_PATH=/var/run/haproxy/master.sock # HAPROXY_RELOAD_TIMEOUT_SECONDS=10 +# HAPROXY_STATS_SOCKET_PATH=/var/run/haproxy/admin.sock +# HAPROXY_STATS_TIMEOUT_SECONDS=10 # Optional: used by scripts/seed_admin.py # ADMIN_EMAIL=admin@example.com diff --git a/src/backend/app/routers/security.py b/src/backend/app/routers/security.py index dbc1c6a..b203ec2 100644 --- a/src/backend/app/routers/security.py +++ b/src/backend/app/routers/security.py @@ -23,7 +23,13 @@ def list_banned_ips( ) -> BannedIpListResponse: """Returns tracked/banned source IPs across auto-ban-enabled vhosts (admin only).""" service = BanListService(db) - items = service.list_banned() + try: + items = service.list_banned() + except RuntimeApiError as error: + raise HTTPException( + status_code=status.HTTP_502_BAD_GATEWAY, + detail="Failed to reach HAProxy Runtime API", + ) from error return BannedIpListResponse(items=items, total=len(items)) diff --git a/src/backend/app/services/ban_list_service.py b/src/backend/app/services/ban_list_service.py index 2460a9c..f929fda 100644 --- a/src/backend/app/services/ban_list_service.py +++ b/src/backend/app/services/ban_list_service.py @@ -27,6 +27,17 @@ r"key=(?P\S+).*?exp=(?P\d+).*?gpc0=(?P\d+)" ) +# HAProxy's Runtime API reports failures (unknown table, permission denied, +# unsupported command, ...) as plain text in the command's own response +# rather than as a socket-level error, so a successful `recv()` does not +# mean the command succeeded. Anchored to the start of a line, mirroring +# `config_apply._RELOAD_ERROR_RE`, so a `key=...` data line or the +# `# table: ...` header can never false-positive. +_RUNTIME_API_ERROR_RE = re.compile( + r"^\s*(unknown|no such|can't find|permission denied|invalid|error)\b", + re.IGNORECASE | re.MULTILINE, +) + class BanListError(Exception): """Base class for ban-list domain errors.""" @@ -54,7 +65,13 @@ class BanTableEntry: def _send_runtime_command(command: str) -> str: - """Send one Runtime API command over the admin stats socket and return output.""" + """Send one Runtime API command over the admin stats socket and return output. + + Raises `RuntimeApiError` both when the socket itself is unreachable and + when HAProxy accepts the connection but replies with an error message + (e.g. an unknown table) — the caller cannot otherwise tell a successful + `clear`/`show` from one HAProxy silently rejected. + """ socket_path = settings.haproxy_stats_socket_path try: with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as client: @@ -71,7 +88,11 @@ def _send_runtime_command(command: str) -> str: except OSError as error: raise RuntimeApiError(str(error)) from error - return b"".join(output_chunks).decode("utf-8", errors="replace") + output = b"".join(output_chunks).decode("utf-8", errors="replace") + if _RUNTIME_API_ERROR_RE.search(output): + raise RuntimeApiError(output.strip() or "HAProxy Runtime API returned an error") + + return output def _parse_show_table(raw: str) -> list[BanTableEntry]: @@ -105,9 +126,18 @@ def __init__(self, db: Session) -> None: self.db = db def list_banned(self) -> list[BannedIpResponse]: - """Return every tracked/banned entry across all auto-ban-enabled vhosts.""" + """Return every tracked/banned entry across all auto-ban-enabled vhosts. + + Tolerates individual table failures (one dead table must not blank + the whole list) but raises `RuntimeApiError` if every attempted + table failed, so a fully unreachable Runtime API surfaces as an + error instead of an empty list indistinguishable from "nothing + tracked" — mirroring `unban`'s all-tables-failed handling below. + """ + tables = self._active_ban_tables() results: list[BannedIpResponse] = [] - for vhost, table_name, ban_threshold in self._active_ban_tables(): + failures = 0 + for vhost, table_name, ban_threshold in tables: try: raw = _send_runtime_command(f"show table {table_name}") entries = _parse_show_table(raw) @@ -117,6 +147,7 @@ def list_banned(self) -> list[BannedIpResponse]: table_name, vhost.id, ) + failures += 1 continue for entry in entries: @@ -131,11 +162,19 @@ def list_banned(self) -> list[BannedIpResponse]: expires_in_seconds=entry.expires_in_seconds, ) ) + + if tables and failures == len(tables): + raise RuntimeApiError("Failed to reach HAProxy Runtime API") + return results def unban(self, ip: str) -> UnbanResponse: """Clear an IP from every active ban table; return the number cleared. + `cleared` counts tables where HAProxy confirmed the `clear table` + command (including a no-op when the key was already absent) — not + merely tables where the socket write succeeded, since + `_send_runtime_command` raises on an HAProxy-reported error too. Tolerates individual table failures (one dead table must not block clearing the others) but raises `RuntimeApiError` if every attempted table failed, so a fully unreachable Runtime API surfaces as an diff --git a/src/backend/app/templates/haproxy.cfg.j2 b/src/backend/app/templates/haproxy.cfg.j2 index 71356d8..1a8b9f7 100644 --- a/src/backend/app/templates/haproxy.cfg.j2 +++ b/src/backend/app/templates/haproxy.cfg.j2 @@ -8,9 +8,13 @@ global # Operator level avoids exposing administrative Runtime API commands. stats socket /tmp/haproxy.sock mode 660 level operator # Admin level, reachable only from the backend over the shared runtime - # volume. Needed for `clear table` (unban) — see #276 — which the - # operator-level socket above deliberately cannot perform. - stats socket /var/run/haproxy/admin.sock mode 660 level admin + # volume (not exposed to the host). Needed for `clear table` (unban) — + # see #276 — which the operator-level socket above deliberately cannot + # perform. mode 666 matches the master socket (docker-compose command, + # `-S ...master.sock,mode,666,...`): HAProxy runs as root while the + # backend drops to an unprivileged user via gosu, so a root-owned 660 + # socket would not be readable/writable by the backend. + stats socket /var/run/haproxy/admin.sock mode 666 level admin defaults mode http diff --git a/src/backend/tests/integration/test_security_router.py b/src/backend/tests/integration/test_security_router.py index a077fb7..0d94ce7 100644 --- a/src/backend/tests/integration/test_security_router.py +++ b/src/backend/tests/integration/test_security_router.py @@ -87,6 +87,24 @@ def test_list_banned_ips_returns_empty_when_nothing_tracked( assert response.json() == {"items": [], "total": 0} +def test_list_banned_ips_returns_502_when_runtime_api_unreachable( + client: TestClient, + admin_token: dict[str, str], + db: Session, + monkeypatch: pytest.MonkeyPatch, +) -> None: + _create_banned_vhost(db) + + def fake_send(_command: str) -> str: + raise ban_list_service.RuntimeApiError("connection refused") + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + response = client.get("/security/banned-ips", headers=admin_token) + + assert response.status_code == 502 + + def test_unban_requires_auth(client: TestClient) -> None: response = client.delete("/security/banned-ips/203.0.113.5") diff --git a/src/backend/tests/unit/test_ban_list_service.py b/src/backend/tests/unit/test_ban_list_service.py index 8151f99..bcedeee 100644 --- a/src/backend/tests/unit/test_ban_list_service.py +++ b/src/backend/tests/unit/test_ban_list_service.py @@ -9,6 +9,7 @@ from app.models.vhost import VHost from app.services import ban_list_service from app.services.ban_list_service import ( + _RUNTIME_API_ERROR_RE, BanListService, BanTableEntry, InvalidIpError, @@ -79,6 +80,41 @@ def test_ignores_unparseable_lines(self) -> None: assert _parse_show_table(raw) == [] +class TestRuntimeApiErrorRegex: + """Mirrors `_RELOAD_ERROR_RE` coverage in test_config_apply.py. + + The Runtime API reports failures as plain text in the command's own + response rather than as a socket error, so `_send_runtime_command` must + classify replies itself — a real `show table`/`clear table` data line + or header must never false-positive as an error. + """ + + def test_does_not_match_show_table_output(self) -> None: + benign_outputs = [ + "# table: st_ban_vhost_1, type: ip, size:102400, used:1", + "0x7f: key=192.0.2.7 use=0 exp=540000 gpc0=15", + "", + "\n", + ] + for output in benign_outputs: + assert not _RUNTIME_API_ERROR_RE.search(output), ( + f"Regex incorrectly matched benign output: {output!r}" + ) + + def test_matches_known_haproxy_error_replies(self) -> None: + error_outputs = [ + "Unknown command. Please enter one of the following commands only:\n", + "No such table\n", + "Can't find table\n", + "Permission denied\n", + "Invalid key\n", + ] + for output in error_outputs: + assert _RUNTIME_API_ERROR_RE.search(output), ( + f"Regex did not match expected error output: {output!r}" + ) + + class TestListBanned: def test_lists_entries_only_for_ddos_and_auto_ban_enabled_active_vhosts( self, db: Session, monkeypatch: pytest.MonkeyPatch @@ -175,6 +211,22 @@ def fake_send(command: str) -> str: def test_returns_empty_list_when_no_vhost_qualifies(self, db: Session) -> None: assert BanListService(db).list_banned() == [] + def test_raises_runtime_api_error_when_every_table_fails( + self, db: Session, monkeypatch: pytest.MonkeyPatch + ) -> None: + policy = _sample_policy() + vhost = _sample_vhost(policy) + db.add_all([policy, vhost]) + db.flush() + + def fake_send(_command: str) -> str: + raise RuntimeApiError("connection refused") + + monkeypatch.setattr(ban_list_service, "_send_runtime_command", fake_send) + + with pytest.raises(RuntimeApiError): + BanListService(db).list_banned() + class TestUnban: def test_clears_ip_from_every_active_table( diff --git a/src/backend/tests/unit/test_config_renderer_haproxy.py b/src/backend/tests/unit/test_config_renderer_haproxy.py index 01d4802..11d1a6d 100644 --- a/src/backend/tests/unit/test_config_renderer_haproxy.py +++ b/src/backend/tests/unit/test_config_renderer_haproxy.py @@ -66,7 +66,7 @@ def test_haproxy_template_renders_admin_stats_socket_for_runtime_api() -> None: rendered = render_haproxy_cfg(_m1_reference_context()) assert "stats socket /tmp/haproxy.sock mode 660 level operator" in rendered - assert "stats socket /var/run/haproxy/admin.sock mode 660 level admin" in rendered + assert "stats socket /var/run/haproxy/admin.sock mode 666 level admin" in rendered def test_haproxy_template_parameterises_vhost_and_backend() -> None: