From 680ad7a6249e3d01bd66546ef9059a269a8f587e Mon Sep 17 00:00:00 2001 From: Jonas Bengtsson Date: Sat, 19 Sep 2026 18:08:02 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=A7=20WIP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 15 ++-- backend/app/configgen.py | 11 +++ backend/app/main.py | 5 +- backend/app/routers/broker.py | 65 +++++++++++++++- backend/app/routers/system.py | 80 +++++++++++++++++++ backend/tests/conftest.py | 21 +++++ backend/tests/test_api.py | 64 +++++++++++++++ backend/tests/test_configgen.py | 14 ++++ broker-control/control.py | 21 +++++ frontend/src/App.tsx | 28 ++++++- frontend/src/api.ts | 5 ++ frontend/src/pages/BrokerConfig.tsx | 25 ++++-- frontend/src/pages/System.tsx | 116 ++++++++++++++++++++++++++++ frontend/src/styles.css | 27 +++++++ 14 files changed, 483 insertions(+), 14 deletions(-) create mode 100644 backend/app/routers/system.py create mode 100644 frontend/src/pages/System.tsx diff --git a/README.md b/README.md index dfffba6..8a64924 100644 --- a/README.md +++ b/README.md @@ -152,12 +152,15 @@ Hardening done beyond the plan: - [x] Unpublished-port warning: listeners on ports not mapped by compose.yaml warn in the preview/deploy (reachable only inside the Docker network; keep PUBLISHED_PORTS in sync with compose) +- [x] Restart-required detection + restart from the UI: deploy compares + listener lines against the previous deployed revision (reload + cannot apply listener changes, §28) and sets a pending-restart + flag; a glowing sidebar indicator links to Administration → + System, where the broker restarts via the control sidecar + (SIGTERM to PID 1, Docker policy revives it; audited as + BROKER_RESTARTED, §29/§38) - [ ] Configuration diff view (revision N-1 → N) - [ ] Configuration rollback (creates a new revision, §62) -- [ ] Reload-vs-restart classification (§28) — the deploy health check - passes on reload, but a NEW listener still needs an explicit - broker restart (docker compose restart mosquitto); surfacing - that in the UI is pending - [ ] CA import (existing external CA) - [ ] Root-key export + removal workflow (§18) - [ ] CSR signing (client-side key generation) @@ -170,9 +173,11 @@ Hardening done beyond the plan: - [x] Audit logging (backend records every privileged action: LOGIN, LOGOUT, CLIENT_*, GROUP_*, ROLE_*, ACL_CHANGED, LISTENER_*, - BROKER_CONFIG_DEPLOYED, USER_*, …) + BROKER_CONFIG_DEPLOYED, BROKER_RESTARTED, USER_*, …) - [x] Client metadata/tags (owner / purpose / tags in PostgreSQL; API accepts them on create) +- [x] Administration → System page: broker status + restart (the + §28 restart gap is closed end-to-end from the UI) - [ ] Audit browsing UI (events are recorded; no viewer page yet) - [ ] Forced password-change UI (backend flag + API exist; the frontend does not enforce `must_change_password` yet) diff --git a/backend/app/configgen.py b/backend/app/configgen.py index a1f93d8..ceb9368 100644 --- a/backend/app/configgen.py +++ b/backend/app/configgen.py @@ -17,6 +17,13 @@ from app.models import BrokerSettings, Certificate, ListenerConfig _BOOL = {True: "true", False: "false"} +# BrokerSettings keys that are INTERNAL platform state, not broker +# options — they must never be emitted into the generated mosquitto.conf +# (a leaked key makes `mosquitto --test-config` fail with "Unknown +# configuration variable"). +RESTART_REQUIRED_KEY = "restart_required" +RESERVED_SETTINGS_KEYS = frozenset({RESTART_REQUIRED_KEY}) + @dataclass class GenerationResult: @@ -31,6 +38,10 @@ def _global_section(db: Session, warnings: list[str]) -> list[str]: value = setting.value.strip() if not key or key.startswith("#"): continue + if key in RESERVED_SETTINGS_KEYS: + # Internal platform state, not a broker option — emitting it + # would fail validation (see RESERVED_SETTINGS_KEYS above). + continue lines.append(f"{key} {value}") return lines diff --git a/backend/app/main.py b/backend/app/main.py index f898029..27c163f 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -125,10 +125,11 @@ def health_ready(): # Routers -from app.routers import auth, broker, mqtt, pki, users # noqa: E402 +from app.routers import auth, broker, mqtt, pki, system, users # noqa: E402 app.include_router(auth.router) app.include_router(users.router) app.include_router(mqtt.router) app.include_router(broker.router) -app.include_router(pki.router) \ No newline at end of file +app.include_router(pki.router) +app.include_router(system.router) \ No newline at end of file diff --git a/backend/app/routers/broker.py b/backend/app/routers/broker.py index 5ec250b..5b3615d 100644 --- a/backend/app/routers/broker.py +++ b/backend/app/routers/broker.py @@ -20,7 +20,7 @@ from sqlalchemy.orm import Session from app import audit from app.config import get_settings -from app.configgen import generate_config +from app.configgen import RESTART_REQUIRED_KEY, generate_config from app.db import get_db from app.deps import CurrentUser, require_roles from app.models import ( @@ -65,6 +65,51 @@ def _next_revision_number(db: Session) -> int: return (highest or 0) + 1 +# BrokerSettings key holding the pending-restart flag (value "true"/"false"). +# Defined in configgen (RESERVED_SETTINGS_KEYS keeps it out of the +# generated config); the bootstrap config the broker starts with must +# match the generator's explicit listener form for the comparison. +_BOOTSTRAP_LISTENERS = {"listener 1883 0.0.0.0"} + + +def _listener_signature(config_text: str) -> set[str]: + """The listener DEFINITION lines of a generated config. + + Mosquitto's reload cannot add, remove or redefine listeners (spec + section 28): textual changes to `listener ` lines are + treated as different listeners and only take effect on restart. + Everything else (cert paths, TLS versions, max_connections, ...) is + reload-safe. + """ + import re + + return set(re.findall(r"^listener \d+.*$", config_text, re.MULTILINE)) + + +def _set_restart_required(db: Session, required: bool) -> None: + setting = db.scalar( + select(BrokerSettings).where(BrokerSettings.key == RESTART_REQUIRED_KEY) + ) + if setting is None: + db.add( + BrokerSettings( + key=RESTART_REQUIRED_KEY, + value="true" if required else "false", + value_type="bool", + ) + ) + else: + setting.value = "true" if required else "false" + + +def restart_required(db: Session) -> bool: + """Whether the deployed config has listener changes pending a restart.""" + setting = db.scalar( + select(BrokerSettings.value).where(BrokerSettings.key == RESTART_REQUIRED_KEY) + ) + return setting == "true" + + def _call_control( path: str, *, method: str = "POST", json_body: dict | None = None ) -> dict: @@ -306,6 +351,23 @@ def deploy_config( if response.get("status") != "deployed": raise HTTPException(status.HTTP_422_UNPROCESSABLE_ENTITY, response) + # Spec section 28: a reload cannot apply listener definition changes. + # Compare the listener lines against the previously DEPLOYED revision + # (or the bootstrap config when none exists yet) and flag a pending + # restart so the UI can surface it (glowing indicator -> System page). + previous = db.scalar( + select(ConfigRevision) + .where(ConfigRevision.deployed.is_(True)) + .order_by(ConfigRevision.revision_number.desc()) + ) + previous_listeners = ( + _listener_signature(previous.config_text) + if previous is not None + else set(_BOOTSTRAP_LISTENERS) + ) + needs_restart = _listener_signature(result.config_text) != previous_listeners + _set_restart_required(db, needs_restart) + revision = ConfigRevision( revision_number=_next_revision_number(db), created_by=user.username, @@ -329,6 +391,7 @@ def deploy_config( "status": "deployed", "revision": revision.revision_number, "broker_healthy": response.get("broker_healthy"), + "restart_required": needs_restart, } diff --git a/backend/app/routers/system.py b/backend/app/routers/system.py new file mode 100644 index 0000000..c8921c5 --- /dev/null +++ b/backend/app/routers/system.py @@ -0,0 +1,80 @@ +"""System administration (spec sections 29, 41, 46). + +Broker status and restart through the control sidecar. The restart +primitive is SIGTERM to PID 1 in the broker's (shared) PID namespace — +the broker shuts down gracefully (persistence saved) and Docker's +`restart: unless-stopped` policy brings the container back. No Docker +socket is involved (spec section 29). +""" + +from __future__ import annotations + +from typing import Annotated + +from fastapi import APIRouter, Depends, Request +from sqlalchemy.orm import Session + +from app import audit +from app.db import get_db +from app.deps import CurrentUser, require_roles +from app.models import ManagementRole +from app.routers.broker import _call_control, restart_required + +router = APIRouter(prefix="/api/v1/system", tags=["system"]) + +BrokerAdmin = Annotated[CurrentUser, Depends(require_roles(ManagementRole.BROKER_ADMIN))] +AnyReader = Annotated[ + CurrentUser, + Depends( + require_roles( + ManagementRole.BROKER_ADMIN, + ManagementRole.OPERATOR, + ManagementRole.AUDITOR, + ) + ), +] +DbDep = Annotated[Session, Depends(get_db)] + + +@router.get("/status") +def system_status(db: DbDep, _user: AnyReader): + """Broker process state + pending-restart flag (spec section 46). + + Used by the sidebar indicator (polls lightly) and the System page. + The control call is allowed to fail — an unreachable sidecar is a + status, not an error. + """ + try: + control = _call_control("/status", method="GET") + broker_alive = bool(control.get("broker_pid_alive")) + except Exception: # noqa: BLE001 - status endpoint must not 500 + broker_alive = False + return { + "broker_alive": broker_alive, + "restart_required": restart_required(db), + } + + +@router.post("/broker/restart") +def restart_broker(request: Request, db: DbDep, user: BrokerAdmin): + """Restart the broker via the control sidecar (spec section 29). + + The sidecar acknowledges and then SIGTERMs PID 1; Docker restarts + the container. The pending-restart flag is cleared here — the config + on disk is what the broker will boot with, so listener changes take + effect by definition once the restart completes. + """ + from app.routers.broker import _set_restart_required + + response = _call_control("/restart") + _set_restart_required(db, False) + audit.record( + db, + "BROKER_RESTARTED", + request=request, + user=user.username, + object_type="broker", + object_id="mosquitto", + result="initiated", + ) + return {"status": "restarting", "broker_alive": False} \ No newline at end of file diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 6f776db..966150b 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -224,6 +224,27 @@ def client(db_session, monkeypatch): # The PKI client-cert endpoint validates the dynsec client exists. monkeypatch.setattr(pki_router, "dynsec_client", fake) + # The control sidecar is not running in tests: fake its responses so + # deploy/system endpoints can be exercised (broker_alive always true). + from app.routers import broker as broker_router + from app.routers import system as system_router + + def _fake_call_control(path, *, method="POST", json_body=None): + if path == "/deploy": + return { + "status": "deployed", + "broker_healthy": True, + "validation": {"valid": True}, + } + if path == "/restart": + return {"status": "restarting"} + if path == "/status": + return {"broker_pid_alive": True, "config_file": "/mosquitto/config/mosquitto.conf"} + raise AssertionError(f"unexpected control path in tests: {path}") + + monkeypatch.setattr(broker_router, "_call_control", _fake_call_control) + monkeypatch.setattr(system_router, "_call_control", _fake_call_control) + with TestClient(main_module.app) as test_client: yield test_client diff --git a/backend/tests/test_api.py b/backend/tests/test_api.py index 51b42b5..c2c5d57 100644 --- a/backend/tests/test_api.py +++ b/backend/tests/test_api.py @@ -130,6 +130,70 @@ def test_client_id_binding(admin_client): assert refused.status_code == 400 +# --- System: broker restart + restart-required flag (spec 28, 29) ------------- + + +def test_deploy_sets_restart_required_on_listener_change(admin_client): + """Adding a listener definition vs the bootstrap config must flag a + pending restart (reload cannot add listeners, spec section 28).""" + # No deployed revision yet -> compared against the bootstrap config + # (listener 1883 0.0.0.0). A listener on 8883 differs from that. + admin_client.post( + "/api/v1/broker/listeners", + json={"name": "New TLS", "port": 8883, "protocol": "mqtt"}, + ) + deploy = admin_client.post( + "/api/v1/broker/config/deploy", json={"comment": "listener added"} + ) + assert deploy.status_code == 200 + body = deploy.json() + assert body["restart_required"] is True + + # The status endpoint reflects the same flag (drives the glow). + status = admin_client.get("/api/v1/system/status").json() + assert status["restart_required"] is True + assert status["broker_alive"] is True + + +def test_deploy_without_listener_change_needs_no_restart(admin_client): + """Deploying a config whose listener lines match the previous deployed + revision must NOT set the flag (everything else is reload-safe).""" + admin_client.post( + "/api/v1/broker/listeners", + json={"name": "Baseline", "port": 1883, "protocol": "mqtt"}, + ) + first = admin_client.post( + "/api/v1/broker/config/deploy", json={"comment": "baseline"} + ).json() + # 1883 matches the bootstrap signature on the FIRST deploy; the flag + # may or may not be set then. The SECOND deploy (identical listener + # lines) must clear it. + second = admin_client.post( + "/api/v1/broker/config/deploy", json={"comment": "no change"} + ).json() + assert second["restart_required"] is False + + +def test_restart_clears_the_flag(admin_client): + admin_client.post( + "/api/v1/broker/config/deploy", json={"comment": "flag set"} + ) + restarted = admin_client.post("/api/v1/system/broker/restart") + assert restarted.status_code == 200 + assert restarted.json()["status"] == "restarting" + + status = admin_client.get("/api/v1/system/status").json() + assert status["restart_required"] is False + + +def test_system_endpoints_rbac(auditor_client): + """Status is read-only (auditor may read); restart is privileged.""" + assert auditor_client.get("/api/v1/system/status").status_code == 200 + assert ( + auditor_client.post("/api/v1/system/broker/restart").status_code == 403 + ) + + def test_role_acl_lifecycle(admin_client): admin_client.post("/api/v1/mqtt/roles", json={"rolename": "telemetry"}) diff --git a/backend/tests/test_configgen.py b/backend/tests/test_configgen.py index c6e067c..c26bae7 100644 --- a/backend/tests/test_configgen.py +++ b/backend/tests/test_configgen.py @@ -84,6 +84,20 @@ def test_published_port_listener_does_not_warn(session): assert not any("not published to the host" in w for w in result.warnings) +def test_internal_settings_never_leak_into_config(session): + """Platform-internal BrokerSettings keys (restart_required) are state, + not broker options: emitting them fails `--test-config` with 'Unknown + configuration variable' (found live in the restart E2E).""" + session.add(ListenerConfig(name="l", port=1883, protocol="mqtt")) + session.add(BrokerSettings(key="restart_required", value="true", value_type="bool")) + session.add(BrokerSettings(key="max_packet_size", value="2000000", value_type="int")) + session.commit() + + result = generate_config(session) + assert "max_packet_size 2000000" in result.config_text + assert "restart_required" not in result.config_text + + def test_tls_listener_without_pki_warns(session): session.add( ListenerConfig( diff --git a/broker-control/control.py b/broker-control/control.py index 49c8994..9091b5f 100644 --- a/broker-control/control.py +++ b/broker-control/control.py @@ -115,6 +115,21 @@ def _signal_reload() -> None: raise _Error(500, f"mosquitto_signal failed: {proc.stderr.strip()}") +def _restart_broker() -> None: + """Gracefully stop the broker; Docker's restart policy revives it. + + The broker is PID 1 in the shared PID namespace. SIGTERM makes it + shut down cleanly (persistence saved); `restart: unless-stopped` + on the mosquitto service then starts the container again. This + sidecar is a separate container and survives, so it can report + status while the broker is down (spec section 29: restart broker + where explicitly supported — no Docker socket involved). + """ + # The HTTP response MUST be flushed before PID 1 dies, so the kill + # happens after the handler returns — orchestrated by the route. + os.kill(1, signal.SIGTERM) + + def _broker_pid_alive() -> bool: try: # Broker is PID 1 in the shared PID namespace. @@ -215,6 +230,12 @@ class Handler(BaseHTTPRequestHandler): try: if self.path == "/deploy": self._handle_deploy() + elif self.path == "/restart": + # Acknowledge BEFORE the broker dies: the response must + # reach the client first, then the process is signaled + # in a daemon thread so the handler completes cleanly. + self._send(200, {"status": "restarting"}) + threading.Thread(target=_restart_broker, daemon=True).start() elif self.path == "/reload": _signal_reload() self._send(200, {"status": "reloading", "broker_healthy": _post_broker_health()}) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 0238929..20505c2 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -1,11 +1,14 @@ import { NavLink, Navigate, Route, Routes, useLocation } from "react-router-dom"; +import { useQuery } from "@tanstack/react-query"; import { useAuth } from "./auth"; +import { api, type SystemStatus } from "./api"; import { LoginPage } from "./pages/Login"; import { ClientsPage } from "./pages/Clients"; import { RolesPage } from "./pages/Roles"; import { GroupsPage } from "./pages/Groups"; import { BrokerConfigPage } from "./pages/BrokerConfig"; import { CertificatesPage } from "./pages/Certificates"; +import { SystemPage } from "./pages/System"; // Navigation structure per spec section 57. Entries with a `to` route are // implemented; entries without one are planned pages, rendered greyed out @@ -59,17 +62,32 @@ const NAV_SECTIONS: { { label: "Roles", hint: "Management roles admin (Release 3)" }, { label: "Audit Log", hint: "Audit viewer (Release 3)" }, { label: "Backup", hint: "Backup/restore (Release 3)" }, - { label: "System", hint: "System settings (Release 3)" }, + { label: "System", to: "/system" }, ], }, ]; +function RestartIndicator() { + const { data } = useQuery({ + queryKey: ["system-status"], + queryFn: () => api.get("/system/status"), + refetchInterval: 15000, + }); + if (!data?.restart_required) return null; + return ( + + ⚠ Restart needed — listener changes are pending + + ); +} + function Layout({ children }: { children: React.ReactNode }) { const { session, logout } = useAuth(); return (