From 702e8b8e558356357fb0234a27a716b6bc913ec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Halis=20KILI=C3=87?= Date: Fri, 12 Jun 2026 00:30:34 +0300 Subject: [PATCH] =?UTF-8?q?chore:=20full=20review=20hardening=20=E2=80=94?= =?UTF-8?q?=20bind-IP=20validation,=20TLS=20e2e=20test,=20doc=20fixes=20(v?= =?UTF-8?q?2.6.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-dimension review (engine correctness, web/security, config/packaging, design/docs/tests). Conclusion: no critical bugs — the engine's _clients reads are race-free (synchronous comprehensions, no await mid-iteration), the auth/RBAC/CSRF/ SSRF model is sound, config round-trips losslessly, and edit-preservation already guards match/RS-485/open-close strings. Concrete fixes: - config: validate admin_ui.bind_ip in AppConfig.validate() (a hand-edited bad value now fails with a clear ConfigError instead of crashing uvicorn at bind time). - tests: add test_tls_bridge.py — per-mapping TLS data bridge e2e (asserts the channel is encrypted and a plaintext client is rejected); this was the top coverage gap. Add an admin bind-IP validation case to test_config_validation.py. Suite is now 33 green. - docs: clarify that RS-485 is set via JSON import / REST (no web UI yet) and that the global logs are size-rotated (distinct from per-mapping 100 MB/15-day trim). - ROADMAP: record the review outcome + refreshed backlog (cluster hardening, RS-485 UI, Modbus write, Node-24 CI bump as a non-breaking maintenance item). --- README.en.md | 5 +- README.md | 5 +- ROADMAP.md | 24 ++++++-- app/__init__.py | 2 +- app/config.py | 10 ++++ tests/run_all.py | 1 + tests/test_config_validation.py | 10 ++++ tests/test_tls_bridge.py | 103 ++++++++++++++++++++++++++++++++ 8 files changed, 151 insertions(+), 9 deletions(-) create mode 100644 tests/test_tls_bridge.py diff --git a/README.en.md b/README.en.md index 65a9d08..e0ec7f2 100644 --- a/README.en.md +++ b/README.en.md @@ -62,7 +62,8 @@ everything from a **password-protected web UI** — no CLI or hand-edited config (pySerial `rfc2217://`, Linux `socat`, Windows com0com+com2tcp / HW VSP3) — see [`docs/VIRTUAL-COM.md`](docs/VIRTUAL-COM.md). - **Full serial config:** baud (incl. custom), data/stop bits, parity, flow control - (none/RTS-CTS/XON-XOFF/DSR-DTR), RTS/DTR on open, exclusive open, RS-485. + (none/RTS-CTS/XON-XOFF/DSR-DTR), RTS/DTR on open, exclusive open. (RS-485 hardware + mode is set via JSON import / the REST API today; a web-UI control is planned.) - **Live port list:** privilege-free polling + optional event hotplug (Linux pyudev / Windows WM_DEVICECHANGE), falling back to polling. **IP picker** from the machine's own addresses (localhost / LAN / 0.0.0.0) or custom. @@ -236,7 +237,7 @@ All state lives in the **data dir** (default `data/`). The data directory is loc owner-only (POSIX `0700`; Windows `icacls` owner/SYSTEM/Administrators), so the files inside aren't readable by other users: - `config.json` — admin IP, password hash, all mappings (atomic write). -- `all.log` — global activity/audit; `audit.log` — config changes. +- `all.log` — global activity/audit (size-rotated); `audit.log` — config changes (rotated). - `logs/.log` — per-mapping history (hourly maintenance: trimmed >15 days / >100 MB). - `tls/` — certificate/key if self-signed is generated. diff --git a/README.md b/README.md index e87de10..12976c5 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,8 @@ düzenlemeye gerek yok. reçeteler (pyserial `rfc2217://`, Linux `socat`, Windows com0com+com2tcp / HW VSP3) — bkz. [`docs/VIRTUAL-COM.md`](docs/VIRTUAL-COM.md). - **Tam seri yapılandırma:** baud (custom dahil), data bit, parity, stop bit, akış - kontrolü (none/RTS-CTS/XON-XOFF/DSR-DTR), açılışta RTS/DTR, exclusive open, RS-485. + kontrolü (none/RTS-CTS/XON-XOFF/DSR-DTR), açılışta RTS/DTR, exclusive open. (RS-485 + donanım modu şu an JSON import / REST API ile ayarlanır; web-UI kontrolü planlı.) - **Canlı port listesi:** ayrıcalık gerektirmeyen polling + isteğe bağlı olay-tabanlı hotplug (Linux pyudev / Windows WM_DEVICECHANGE), yoksa polling'e düşer. - **IP seçici:** makineye atanmış IP'ler (localhost / LAN / 0.0.0.0) veya custom. @@ -235,7 +236,7 @@ Tüm durum **veri dizininde** (varsayılan `data/`). Veri dizini yalnızca sahib kilitlenir (POSIX `0700`; Windows'ta `icacls` ile sahip/SYSTEM/Administrators), böylece içindeki dosyalar başka kullanıcılarca okunamaz: - `config.json` — admin IP, parola hash'i, tüm eşlemeler (atomik yazım). -- `all.log` — global etkinlik/audit; `audit.log` — config değişiklikleri. +- `all.log` — global etkinlik/audit (boyutla döner); `audit.log` — config değişiklikleri (döner). - `logs/.log` — eşleme başına geçmiş (saatlik bakım: >15 gün veya >100MB kırpılır). - `tls/` — self-signed üretilirse sertifika/anahtar. diff --git a/ROADMAP.md b/ROADMAP.md index 503d848..9e417b6 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1,6 +1,6 @@ # ser2net — Roadmap -> Current release: **v2.6.1**. CI is green across Linux + Windows × Python 3.10–3.13. +> Current release: **v2.6.2**. CI is green across Linux + Windows × Python 3.10–3.13. ## Shipped @@ -90,6 +90,17 @@ - **Manual peers** (`host:port`) for routed/L3 networks broadcast can't reach, aggregated alongside auto-discovered nodes +### v2.6.2 — review hardening +- Full multi-dimension review (engine correctness, web/security, config/packaging, + design/docs/tests). Net result: no critical bugs — the engine's `_clients` snapshots + are race-free (synchronous comprehensions), auth/RBAC/CSRF/SSRF model is sound. +- Validate `admin_ui.bind_ip` in `AppConfig.validate()` (clear error instead of a + uvicorn bind crash on a hand-edited config) +- Closed the top test gap: **per-mapping TLS data-bridge e2e** (`test_tls_bridge.py` — + verifies the channel is encrypted + plaintext is rejected) and a bind-IP validation case +- Doc accuracy: RS-485 is config/REST/JSON-settable (no web UI yet); global logs are + size-rotated (distinct from the per-mapping 100 MB/15-day trim) + --- ## Planned @@ -100,10 +111,11 @@ key-guarded peer endpoints; optional IPv6 (multicast) discovery ### v2.7 — industrial/IIoT depth +- **RS-485 hardware auto-RTS** UI (`TIOCSRS485`) — the config/REST already carry the + fields; expose them in the mapping form (closes the docs gap noted in v2.6.2) - **Sparkplug B** edge payloads (Modbus register + MQTT plumbing already in place) - **Modbus write** support (FC 5/6/15/16), per-point MQTT→register control, RTU inter-frame tuning -- **RS-485 hardware auto-RTS** (`TIOCSRS485`) UI ### v2.8 — packaging & migration - Windows service installer (Shawl); `.deb` / `.rpm` packages @@ -111,8 +123,12 @@ - Classic `ser2net.yaml` import for migration ### Maintenance -- Bump pinned GitHub Actions off Node 20 (deprecated 2026-06-16) to current - `actions/*@v5` to clear the CI/release deprecation warnings +- Bump pinned GitHub Actions off Node 20 (`checkout@v4`, `setup-python@v5`, + `action-gh-release@v2`) to the current Node-24 majors to clear the deprecation + warnings. Non-breaking — GitHub force-runs them on Node 24 from 2026-06-16, so this + is cosmetic; bump only after confirming the new tags exist. +- Test backlog: confirm RFC2217 read-only also blocks control (baud/RTS/DTR) commands, + and add a UDP `allowed_client_ips` reject case if not already explicit. ### Icebox / conditional - Thread-per-port serial backend — only if a real Windows high-throughput / diff --git a/app/__init__.py b/app/__init__.py index 77bc75e..7683a3e 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,4 +1,4 @@ """ser2net — serial-to-network bridge with a web config UI.""" # Single source of truth for the version (ser2net.py and the web layer import this). -__version__ = "2.6.1" +__version__ = "2.6.2" diff --git a/app/config.py b/app/config.py index 226ed8b..f15a11e 100644 --- a/app/config.py +++ b/app/config.py @@ -722,6 +722,16 @@ def validate(self) -> None: if not os.path.isfile(path): raise ConfigError(f"TLS file not found or not readable: {path}") + # Admin UI bind IP (set via the console / --reconfigure, or a hand-edited + # config). Catch a bad value here with a clear error instead of letting + # uvicorn crash at bind time. + if self.admin_ui.bind_ip: + try: + ipaddress.ip_address(self.admin_ui.bind_ip) + except ValueError: + raise ConfigError( + f"Invalid admin UI bind IP: {self.admin_ui.bind_ip}") from None + self.ldap.validate() self.oidc.validate() self.cluster.validate() diff --git a/tests/run_all.py b/tests/run_all.py index 70f9ed5..94d4722 100644 --- a/tests/run_all.py +++ b/tests/run_all.py @@ -54,6 +54,7 @@ "test_access_priority.py", "test_fullduplex.py", "test_v2_transports.py", + "test_tls_bridge.py", "test_v2_console.py", "test_v2_web.py", "test_web_e2e.py", diff --git a/tests/test_config_validation.py b/tests/test_config_validation.py index 6b24828..71e1c3e 100644 --- a/tests/test_config_validation.py +++ b/tests/test_config_validation.py @@ -218,8 +218,18 @@ def test_config_round_trip(): print("config round-trips with every subsystem (auth/ldap/oidc/mqtt/modbus) intact OK") +def test_invalid_admin_bind_ip_rejected(): + cfg = AppConfig.from_dict({"admin_ui": {"bind_ip": "not-an-ip", "port": 8080}}) + _expect_error(cfg, "bind IP") + # valid wildcard/loopback/IP must pass + for ip in ("0.0.0.0", "::", "127.0.0.1", "192.168.1.5"): + AppConfig.from_dict({"admin_ui": {"bind_ip": ip, "port": 8080}}).validate() + print("invalid admin UI bind IP rejected; valid ones accepted OK") + + def main(): test_config_round_trip() + test_invalid_admin_bind_ip_rejected() test_ldap_validation() test_modbus_poll_requires_gateway() test_mqtt_validation() diff --git a/tests/test_tls_bridge.py b/tests/test_tls_bridge.py new file mode 100644 index 0000000..bcd43c3 --- /dev/null +++ b/tests/test_tls_bridge.py @@ -0,0 +1,103 @@ +"""Per-mapping TLS data bridge: a TLS-wrapped TCP server mapping <-> serial, e2e. + +Brings up a real bridge with network.tls=True (self-signed cert), connects a TLS +client, and verifies the channel is actually encrypted and bytes flow both ways +between the TLS socket and the serial device. Needs socat + openssl (skips if absent). + +Run: python3 tests/test_tls_bridge.py +""" +import asyncio +import os +import re +import shutil +import ssl +import subprocess +import sys +import tempfile + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "lib")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from app.config import MappingConfig +from app.engine.supervisor import Supervisor + + +async def socat(): + p = await asyncio.create_subprocess_exec( + "socat", "-d", "-d", "pty,raw,echo=0", "pty,raw,echo=0", + stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE) + devs = [] + while len(devs) < 2: + m = re.search(rb"PTY is (\S+)", await p.stderr.readline()) + if m: + devs.append(m.group(1).decode()) + return p, devs[0], devs[1] + + +def gen_cert(tmp): + cert, key = os.path.join(tmp, "cert.pem"), os.path.join(tmp, "key.pem") + subprocess.run(["openssl", "req", "-x509", "-newkey", "rsa:2048", "-nodes", + "-keyout", key, "-out", cert, "-days", "1", "-subj", "/CN=localhost"], + check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + return cert, key + + +async def main(): + if not shutil.which("socat") or not shutil.which("openssl"): + print("SKIP: socat/openssl not available") + return + import serial + + tmp = tempfile.mkdtemp(prefix="ser2net_tls_") + cert, key = gen_cert(tmp) + p, dev_a, dev_b = await socat() + sup = Supervisor(logger=lambda s: None) + port = 47360 + m = MappingConfig.from_dict({ + "name": "tls", "enabled": True, "serial": {"port": dev_a, "baudrate": 115200}, + "network": {"mode": "server", "protocol": "raw", "bind_ip": "127.0.0.1", + "port": port, "tls": True, "tls_cert": cert, "tls_key": key}}) + try: + ok, msg = await sup.apply_mapping(m) + assert ok, msg + await asyncio.sleep(0.5) + device = serial.Serial(dev_b, 115200, timeout=1) + + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE # self-signed; we only assert encryption here + r, w = await asyncio.open_connection("127.0.0.1", port, ssl=ctx) + + sslobj = w.get_extra_info("ssl_object") + assert sslobj is not None, "the data connection is not TLS" + + # a plaintext client must NOT be able to talk to the TLS listener + try: + pr, pw = await asyncio.open_connection("127.0.0.1", port) + pw.write(b"plain"); await pw.drain() + assert await asyncio.wait_for(pr.read(16), timeout=2) == b"", "plaintext accepted on TLS port" + pw.close() + except (ssl.SSLError, ConnectionError, asyncio.TimeoutError, OSError): + pass # rejected, as expected + + # serial -> TLS client + await asyncio.to_thread(device.write, b"hello-tls"); await asyncio.to_thread(device.flush) + got = await asyncio.wait_for(r.read(32), timeout=3) + assert b"hello-tls" in got, got + # TLS client -> serial + w.write(b"to-serial"); await w.drain() + await asyncio.sleep(0.3) + back = await asyncio.to_thread(device.read, 32) + assert back == b"to-serial", back + print(f"per-mapping TLS bridge: encrypted ({sslobj.version()}) bidirectional OK") + + w.close(); device.close() + print("\nPASS: per-mapping TLS data bridge") + finally: + await sup.stop_all() + p.terminate(); await p.wait() + shutil.rmtree(tmp, ignore_errors=True) + + +if __name__ == "__main__": + asyncio.run(main())