From 62903a7f053758329ede7b73e41dca13407bdd54 Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Sun, 13 Sep 2026 00:26:05 +0200 Subject: [PATCH] fix: status.json fuer normale Nutzer lesbar machen, Lesefehler nicht mehr verschlucken server.py legt status.json ueber mkstemp() (Modus 0600) + os.replace() an; da os.replace() den Modus der temporaeren Datei uebernimmt, war die Datei nach jedem Schreiben nur fuer root lesbar - der Dienst laeuft als root, das CLI liest aber als normaler Benutzer. os.chmod(tmp, 0o644) vor dem Rename behebt das umask-fest, auch bei wiederholten Schreibvorgaengen. ninlight-station status hat den fehlenden Lesezugriff bisher stillschweigend wie "noch nie ausgeliefert" behandelt (beides fiel durch denselben "-r"-Test). Jetzt wird zwischen "Datei fehlt" (unauffaellig) und "Datei da, aber nicht lesbar" (Fehlermeldung auf stderr) unterschieden. Neuer Test prueft, dass status.json nach dem ersten und nach einem weiteren Schreibvorgang world-readable bleibt, auch unter restriktivem umask. Co-Authored-By: Claude Sonnet 5 --- tools/station/ninlight-station | 14 ++++++++++- tools/station/server.py | 22 ++++++++++++++--- tools/station/tests/test_server.py | 39 ++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/tools/station/ninlight-station b/tools/station/ninlight-station index 5354271..6c540fc 100755 --- a/tools/station/ninlight-station +++ b/tools/station/ninlight-station @@ -113,7 +113,19 @@ PY # "status" soll mit 0 enden, sobald es seine Auskunft geben konnte, auch wenn noch nie # ausgeliefert wurde - ohne das "|| true" waere der Exit-Code des Skripts hier der des # fehlgeschlagenen Tests (1), weil das die letzte Anweisung im Zweig ist. - [ -r /run/ninlight-station/status.json ] && { echo "Zuletzt ausgeliefert:"; sed 's/^/ /' /run/ninlight-station/status.json; } + # + # "existiert nicht" (Station lief noch nie) und "existiert, ist aber nicht lesbar" + # sind zwei verschiedene Zustaende und duerfen nicht beide stillschweigend im + # "-r"-Test verschwinden: sonst faellt ein Rechte-Fehler auf status.json (siehe + # server.py, schreibt als root) nie auf, weil die Ausgabe genauso aussieht wie + # "noch nie ausgeliefert". + STATUS_FILE=/run/ninlight-station/status.json + if [ -r "$STATUS_FILE" ]; then + echo "Zuletzt ausgeliefert:" + sed 's/^/ /' "$STATUS_FILE" + elif [ -e "$STATUS_FILE" ]; then + echo "Fehler: $STATUS_FILE existiert, ist aber nicht lesbar (Rechte pruefen)" >&2 + fi exit 0 ;; *) diff --git a/tools/station/server.py b/tools/station/server.py index 59d05c9..5bfaacc 100755 --- a/tools/station/server.py +++ b/tools/station/server.py @@ -10,6 +10,7 @@ Standard library only - the Pi gets no extra packages. from __future__ import annotations import argparse +import contextlib import json import os import re @@ -85,9 +86,24 @@ class Station: try: self.state_path.parent.mkdir(parents=True, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=self.state_path.parent, prefix=".status-") - with os.fdopen(fd, "w") as f: - json.dump(payload, f, indent=2) - os.replace(tmp, self.state_path) + try: + with os.fdopen(fd, "w") as f: + json.dump(payload, f, indent=2) + # The service runs as root and mkstemp() creates the file 0600, so + # without this an operator reading status.json as a normal user (see + # tools/station/ninlight-station) gets "permission denied" - and since + # os.replace() preserves the mode it arrives with, that wrong mode + # would otherwise survive onto the live file on every single write, + # not just the first. os.chmod() ignores the process umask (unlike the + # mode argument to mkstemp/open), so a restrictive umask on the Pi + # can't quietly bring the 0600 back. Set it before the rename so + # there's never a window where the path holds a 0600 file. + os.chmod(tmp, 0o644) + os.replace(tmp, self.state_path) + except BaseException: + with contextlib.suppress(OSError): + os.unlink(tmp) + raise except OSError as e: print(f"warning: cannot write {self.state_path}: {e}", flush=True) diff --git a/tools/station/tests/test_server.py b/tools/station/tests/test_server.py index a027273..2b6d80e 100644 --- a/tools/station/tests/test_server.py +++ b/tools/station/tests/test_server.py @@ -1,7 +1,9 @@ import hashlib import http.client import json +import os import socket +import stat import subprocess import sys import threading @@ -173,6 +175,43 @@ def test_state_file_records_a_missing_image(station): assert state["recent"][-1]["target"] == "g95_cw" +def test_state_file_is_world_readable_even_under_a_restrictive_umask(tmp_path): + """status.json is written by a root service but read by a normal-user CLI. + + tempfile.mkstemp() creates its file 0600 and os.replace() preserves whatever mode + the temp file had, so the server must explicitly widen the mode before the rename - + and must do so with chmod() rather than relying on the mode passed to mkstemp/open, + because that one is still narrowed by the process umask. A restrictive umask (as + root services on the Pi may well run under) must not bring the 0600 back. + """ + old_umask = os.umask(0o077) + try: + s = Station(tmp_path) + finally: + os.umask(old_umask) + try: + make_image(s.image_dir / "ninlight_evk_ota.img", "evk", "1.0.0", b"w" * 100) + + def world_readable() -> bool: + mode = stat.S_IMODE(s.state.stat().st_mode) + return bool(mode & stat.S_IROTH) + + # Written once already at startup (Station.__init__ -> write_state()). + s.read_state(lambda _: world_readable()) + assert world_readable(), "status.json is not world-readable after the initial write" + + # A second write (a real delivery) must not lose the mode either - e.g. if the + # widening were only applied to the file's very first creation instead of to + # every temp file, mode 0600 would silently come back on every later write. + c = s.conn() + c.request("GET", "/ninlight_evk_ota.img") + c.getresponse().read() + s.read_state(lambda st: st["served"] >= 1) + assert world_readable(), "status.json lost its mode on a later overwrite" + finally: + s.stop() + + def test_recent_is_capped_at_eight(station): make_image(station.image_dir / "ninlight_evk_ota.img", "evk", "1.0.0", b"q" * 10) for _ in range(12):