From 9a759ee299d564570d567e71b5c7de3e23d11477 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 00:17:27 -0400 Subject: [PATCH 1/6] perf(systemd): cap glibc malloc arenas on the display service Measured on a live rig 2.5 hours after start: RSS 1030 MB Private_Dirty 988 MB anonymous mappings > 10 MB 23 largest 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses threads 9 cores 3 -> glibc ceiling = 8 x 3 = 24 arenas 23 against a ceiling of 24, all 64 MB-aligned: these are glibc's per-thread malloc arenas, not live objects. The data the process was actually holding accounts for perhaps 15 MB -- the widest scroll strip observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy mirror. It is bloat rather than a leak: sampled four times over 135 seconds, RSS sat between 990 and 1030 MB rather than climbing. glibc gives each allocating thread its own arena, grows them to hold peak demand, and never gives them back. A process that builds and drops large images across several threads is exactly the shape that produces this. The device had 59 MB free at the time, on 1845 MB total. MALLOC_ARENA_MAX=2 trades a little allocator concurrency for that resident memory. It is a tuning knob rather than a fix for a defect, so the rationale and the measurements sit next to it in the unit file, and a test asserts they stay there -- a bare environment variable invites removal by whoever meets it next. Two things this is NOT, both checked rather than assumed: - Not an OOM problem today. A grep for "oom" in the service journal returned 24 matches, all of which were the radar logging zoom=9 and zoom=7. The kernel OOM killer has not fired: dmesg has zero matches. - Not currently capped by the unit's MemoryMax=85% either. That directive is in this file but absent from the unit actually installed on the rig, which reports MemoryMax=infinity, so nothing is enforcing a ceiling there. The saving is unmeasured on hardware: applying it needs a service restart, which blanks the panel, so that is the user's call rather than something to do mid-audit. If p99 frame time regresses -- it sits at 18.4 ms against a 16.7 ms budget for 60 FPS, so there is not much headroom -- raise the value rather than remove it. (cherry picked from commit 446207ffbc8e6dce00424557c2227c2f1a74e5fb) --- systemd/ledmatrix.service | 12 +++++ test/test_systemd_malloc_arenas.py | 83 ++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 test/test_systemd_malloc_arenas.py diff --git a/systemd/ledmatrix.service b/systemd/ledmatrix.service index d5f064b6..ab5ddf4d 100644 --- a/systemd/ledmatrix.service +++ b/systemd/ledmatrix.service @@ -8,6 +8,18 @@ Type=simple User=root WorkingDirectory=__PROJECT_ROOT_DIR__ Environment=PYTHONDONTWRITEBYTECODE=1 +# glibc gives each allocating thread its own malloc arena, up to 8 x CPU count, +# and an arena that has grown is never handed back to the OS. This process runs +# 9 threads on a 3-core Pi, so the ceiling is 24 arenas -- and a rig measured at +# 1030 MB resident held 23 large anonymous mappings on 64 MB-aligned addresses, +# 920 MB of them, while the live data it was actually holding (widest scroll +# strip seen: 35,746 x 64) accounts for roughly 15 MB. That gap is arena bloat, +# not leaked objects: RSS was flat across repeated sampling, not climbing. +# +# Capping the arenas trades a little allocator concurrency for a large amount of +# resident memory on a device that has neither to spare. 2 is the usual value; +# raise it if frame times regress. +Environment=MALLOC_ARENA_MAX=2 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py # Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken # for a reason that no longer applies, e.g. a config reload) would otherwise leave diff --git a/test/test_systemd_malloc_arenas.py b/test/test_systemd_malloc_arenas.py new file mode 100644 index 00000000..512154d0 --- /dev/null +++ b/test/test_systemd_malloc_arenas.py @@ -0,0 +1,83 @@ +"""The display unit must cap glibc's malloc arenas. + +glibc hands each allocating thread its own malloc arena, up to 8 x CPU count, +and an arena that has grown is never returned to the OS. This process runs +threads for the render loop, the update workers and the background fetchers, so +on a 3-core Pi the ceiling is 24 arenas. + +Measured on a live rig, 2.5 hours in: + + RSS 1030 MB + Private_Dirty 988 MB + anonymous mappings > 10 MB 23 (ceiling is 8 x 3 = 24) + largest few 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses + +against live data that accounts for perhaps 15 MB -- the widest scroll strip +observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy +mirror. Repeated sampling showed RSS flat between 990 and 1030 MB rather than +climbing, so this is arena bloat rather than a leak: memory Python has freed +but glibc is holding per-arena. + +The device had 59 MB free at the time. + +Capping the arena count trades a little allocator concurrency for that resident +memory. The render loop is latency-sensitive, so if p99 frame time regresses the +right response is to raise this rather than remove it. +""" +import re +from pathlib import Path + +import pytest + +UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service") + + +def _environment(unit_text): + return dict( + line.split("=", 2)[1:3] if line.count("=") >= 2 else (line.split("=", 1)[1], "") + for line in unit_text.splitlines() + if line.startswith("Environment=") + ) + + +def test_the_unit_exists(): + assert UNIT.is_file(), f"{UNIT} is missing" + + +def test_malloc_arena_max_is_capped(): + env = _environment(UNIT.read_text(encoding="utf-8")) + assert "MALLOC_ARENA_MAX" in env, ( + "the display unit does not cap glibc arenas; on a 3-core Pi the default " + "ceiling is 24 and a measured rig held 23 of them, 920 MB" + ) + value = int(env["MALLOC_ARENA_MAX"]) + assert 1 <= value <= 4, ( + f"MALLOC_ARENA_MAX={value} is outside the useful range: 1-4 keeps the " + "resident saving, and anything larger gives most of it back" + ) + + +def test_the_reason_is_recorded_next_to_it(): + """A bare tuning knob invites removal by whoever meets it next.""" + text = UNIT.read_text(encoding="utf-8") + index = text.index("Environment=MALLOC_ARENA_MAX") + preamble = text[:index].splitlines()[-12:] + comment = "\n".join(line for line in preamble if line.startswith("#")) + assert "arena" in comment.lower(), "no explanation precedes the setting" + assert re.search(r"\d", comment), ( + "the explanation cites no measurement, so a reader cannot tell whether " + "it still applies to their hardware" + ) + + +@pytest.mark.parametrize("unit", ["ledmatrix.service"]) +def test_the_unit_still_parses_as_ini(unit): + """systemd will refuse a malformed unit, and the panel stays dark.""" + import configparser + + path = UNIT.parent / unit + parser = configparser.ConfigParser(strict=False) + # systemd allows repeated keys; ConfigParser needs them merged, not rejected. + parser.read_string(path.read_text(encoding="utf-8")) + assert parser.has_section("Service") + assert parser.has_option("Service", "ExecStart") From f7046f8141f03fe7a144a81e60c98523fe7a2628 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 01:55:14 -0400 Subject: [PATCH 2/6] test(systemd): pin the arena value instead of accepting a range Review follow-up. The range check accepted 1, 3 and 4, so a change to 4 -- which hands most of the resident saving back -- passed a test whose whole purpose is to notice that. Pinned to the value the unit ships, in one named constant. Raising it is still a legitimate response to a frame-time regression, but it should be a visible edit here rather than silent drift, and the failure message says so. Mutation-checked: changing the unit to 4 now fails. (cherry picked from commit 73fff8d2d5ba90a4af72bc8b509e9563dcf57378) --- test/test_systemd_malloc_arenas.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/test_systemd_malloc_arenas.py b/test/test_systemd_malloc_arenas.py index 512154d0..bb4bc8f0 100644 --- a/test/test_systemd_malloc_arenas.py +++ b/test/test_systemd_malloc_arenas.py @@ -31,6 +31,11 @@ UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service") +#: The value the unit is expected to carry. 2 is the usual choice for a +#: threaded Python process; 1-4 all keep some of the saving, but only one of +#: them is what this project ships. +EXPECTED_ARENA_MAX = 2 + def _environment(unit_text): return dict( @@ -51,9 +56,16 @@ def test_malloc_arena_max_is_capped(): "ceiling is 24 and a measured rig held 23 of them, 920 MB" ) value = int(env["MALLOC_ARENA_MAX"]) - assert 1 <= value <= 4, ( - f"MALLOC_ARENA_MAX={value} is outside the useful range: 1-4 keeps the " - "resident saving, and anything larger gives most of it back" + # Pinned, not a range. A range let a change to 4 -- which hands most of the + # saving back -- pass unnoticed, which was the point of the finding that + # prompted this. Raising it is a legitimate response to a frame-time + # regression, but it should be a visible edit here rather than a silent + # drift, so the number lives in one place and changing it shows up in + # review. + assert value == EXPECTED_ARENA_MAX, ( + f"MALLOC_ARENA_MAX={value}, expected {EXPECTED_ARENA_MAX}. If this was " + "raised deliberately because frame times regressed, update " + "EXPECTED_ARENA_MAX here and say so in the commit." ) From e1b445268d37b92ca40044207d8da5e1bdd4fdce Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 00:48:06 -0400 Subject: [PATCH 3/6] fix(startup): warn when an installed systemd unit has drifted from the repo's Nothing re-applies systemd units after the first install. `git pull` -- which is what the web UI's update button runs -- brings a new template into the checkout, but no code in web_interface/ or src/ copies it to /etc/systemd/system, and nothing anywhere runs `systemctl daemon-reload`. The unit that actually runs is whatever first_time_install.sh wrote on day one. So every hardening added to a unit is inert on existing installs, silently. Measured on a live rig: installed /etc/systemd/system/ledmatrix.service 2026-08-06 template systemd/ledmatrix.service 2026-08-19 contents differ with the practical result that the MemoryMax=85% the repo's template specifies was not being enforced at all -- `systemctl show` reported MemoryMax=infinity. Anyone reading the template would reasonably believe the service was capped. Startup now compares each installed unit against its substituted template and warns when they differ, naming install_service.sh as the remedy. A warning, not an error, and deliberately not a silent rewrite: editing files under /etc and restarting services is the installer's job, not something a display process should do to a machine while it is booting. Making it fatal would also brick every development checkout whose unit is legitimately absent or hand-edited. Comparison ignores comments, blank lines and ordering. The template carries explanatory comments the installed copy will not have, and systemd does not care about order within a section, so a literal comparison would warn on every boot and be ignored within a week. Mutation-checked three ways: never reporting drift fails, making it fatal fails, and -- after the first attempt missed it -- comparing raw text now fails too. That last gap is worth noting: the comment-insensitivity tests originally exercised the helper directly, so a comparison that stopped calling the helper passed them all. The test that catches it goes through _validate_systemd_units. 29 startup-validator tests pass. (cherry picked from commit cf521bdfd8d46d42feb2297842ca8400476f0de4) --- src/startup_validator.py | 68 ++++++++++++++++ test/test_systemd_unit_drift.py | 133 ++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 test/test_systemd_unit_drift.py diff --git a/src/startup_validator.py b/src/startup_validator.py index 9f2e0a51..5dc0d0f0 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -62,6 +62,9 @@ def validate_all(self) -> Tuple[bool, List[str], List[str]]: # Validate plugins if plugin manager is available if self.plugin_manager: self._validate_plugins() + + # Warn when the running systemd unit no longer matches the repo's + self._validate_systemd_units() is_valid = len(self.errors) == 0 @@ -74,6 +77,71 @@ def validate_all(self) -> Tuple[bool, List[str], List[str]]: return (is_valid, self.errors.copy(), self.warnings.copy()) + #: Units this project installs, and where each is installed to. + _UNITS = ( + ("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"), + ("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"), + ) + + def _validate_systemd_units(self) -> None: + """Warn when an installed unit has drifted from the repo's template. + + Nothing re-applies these after the first install. `git pull` -- which is + what the web UI's update button runs -- brings a new template into the + checkout, but nothing copies it to /etc/systemd/system and nothing runs + `systemctl daemon-reload`, so the unit that actually runs is whatever + first_time_install.sh wrote on day one. + + That makes every hardening added to a unit inert on existing installs. + Measured on one rig: the installed unit was thirteen days older than the + repo's and differed in content, so a MemoryMax the repo had specified + was not being enforced at all -- `systemctl show` reported + MemoryMax=infinity. + + A warning rather than an error, and certainly not a silent rewrite: + editing files under /etc and restarting services is the installer's job, + not something a display process should do to a machine while it boots. + The remedy is to re-run scripts/install/install_service.sh. + """ + try: + project_root = Path(__file__).resolve().parent.parent + for template_rel, installed_path in self._UNITS: + template = project_root / template_rel + installed = Path(installed_path) + if not template.is_file() or not installed.is_file(): + continue + + # The template carries placeholders the installer substitutes, + # so compare the substituted form rather than the raw file. + expected = template.read_text(encoding="utf-8") + expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root)) + expected = expected.replace("__USER__", "root") + + try: + actual = installed.read_text(encoding="utf-8") + except PermissionError: + continue + + if self._unit_body(expected) != self._unit_body(actual): + self.warnings.append( + f"{installed.name} differs from {template_rel}; the " + "installed unit is not refreshed by an update, so " + "settings added to the template are not in effect. " + "Re-run scripts/install/install_service.sh to apply them." + ) + except OSError as e: + self.logger.debug("Could not compare systemd units: %s", e) + + @staticmethod + def _unit_body(text: str) -> str: + """A unit's meaningful lines: no comments, no blanks, no ordering noise.""" + lines = [] + for line in text.splitlines(): + line = line.strip() + if line and not line.startswith("#"): + lines.append(line) + return "\n".join(sorted(lines)) + def _validate_config(self) -> None: """Validate configuration files.""" try: diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py new file mode 100644 index 00000000..0aaa67cd --- /dev/null +++ b/test/test_systemd_unit_drift.py @@ -0,0 +1,133 @@ +"""An installed unit that no longer matches the repo's must be reported. + +Nothing re-applies systemd units after the first install. `git pull` -- what +the web UI's update button runs -- brings a new template into the checkout, but +no code in web_interface/ or src/ copies it to /etc/systemd/system or runs +`systemctl daemon-reload`. The unit that actually runs is whatever +first_time_install.sh wrote on day one. + +So every hardening added to a unit is inert on existing installs. Measured on a +live rig: the installed unit was dated 2026-08-06 and the repo's 2026-08-19, +and they differed -- with the result that a MemoryMax=85% present in the repo's +template was not being enforced at all. `systemctl show` reported +MemoryMax=infinity. + +This is a warning, not an error, and deliberately not a silent rewrite: +editing files under /etc and restarting services is the installer's job, not +something a display process should do to a machine while it boots. +""" +import logging +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from src.startup_validator import StartupValidator + + +@pytest.fixture +def validator(): + v = StartupValidator(config_manager=MagicMock()) + v.logger = logging.getLogger("test") + v.warnings = [] + v.errors = [] + return v + + +def test_a_matching_unit_produces_no_warning(validator, tmp_path): + """The installed unit, substituted exactly as the installer would.""" + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + installed = tmp_path / "ledmatrix.service" + installed.write_text( + template.read_text(encoding="utf-8") + .replace("__PROJECT_ROOT_DIR__", str(project_root)) + .replace("__USER__", "root"), + encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert not validator.warnings, f"a matching unit warned: {validator.warnings}" + assert not validator.errors + + +def test_comments_and_blank_lines_are_not_drift(): + """Otherwise every comment the repo adds would look like a changed unit.""" + a = "[Service]\n# explains a setting\nExecStart=/x\nRestart=always\n" + b = "[Service]\nExecStart=/x\n\nRestart=always\n" + assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b) + + +def test_a_changed_directive_is_drift(): + a = "[Service]\nExecStart=/x\nMemoryMax=85%\n" + b = "[Service]\nExecStart=/x\n" + assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b) + + +def test_reordered_directives_are_not_drift(): + """systemd does not care about order within a section, so neither should this.""" + a = "[Service]\nExecStart=/x\nRestart=always\n" + b = "[Service]\nRestart=always\nExecStart=/x\n" + assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b) + + +def test_cosmetic_differences_do_not_warn(validator, tmp_path): + """Through the real comparison, not the helper. + + The repo's template carries explanatory comments the installed copy may not + have, and the installer does not preserve ordering or blank lines. If those + counted as drift, every boot would warn and the warning would be ignored. + Asserting this on _unit_body alone would not catch a comparison that stopped + calling it -- which is exactly what a careless edit does. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + substituted = (template.read_text(encoding="utf-8") + .replace("__PROJECT_ROOT_DIR__", str(project_root)) + .replace("__USER__", "root")) + # Same directives, stripped of comments and blank lines and reordered. + directives = sorted(line.strip() for line in substituted.splitlines() + if line.strip() and not line.strip().startswith("#")) + installed = tmp_path / "ledmatrix.service" + installed.write_text("\n".join(reversed(directives)) + "\n", encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert not validator.warnings, ( + f"cosmetic-only difference reported as drift: {validator.warnings}") + + +def test_drift_is_reported_as_a_warning(validator, tmp_path): + """The whole point: a real difference must surface, and only as a warning.""" + installed = tmp_path / "ledmatrix.service" + installed.write_text("[Service]\nExecStart=/usr/bin/python3 /x/run.py\n") + + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + + assert validator.warnings, "a differing unit produced no warning" + assert "install_service.sh" in validator.warnings[0], ( + "the warning does not tell the user how to fix it") + assert not validator.errors, "drift must not be fatal at startup" + + +def test_a_missing_installed_unit_is_silent(validator, tmp_path): + """Development checkouts have no /etc/systemd unit; that is not drift.""" + validator._UNITS = (("systemd/ledmatrix.service", str(tmp_path / "absent.service")),) + validator._validate_systemd_units() + assert not validator.warnings + assert not validator.errors From 324a4e43b134f9e3b540e16f11916754636467f6 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 01:22:57 -0400 Subject: [PATCH 4/6] fix(install): grant the sudo commands the captive portal actually runs The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and ledmatrix_wifi. Anything the code runs under sudo that is not in one of them needs a password, which a service cannot supply, so the call fails. Five commands were being run and none of them granted: sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883 nft add|delete table ip ledmatrix wifi_manager.py:835, 895 rfkill unblock wifi wifi_manager.py:1811 iptables ... wifi_manager.py:796, 813, 818, 871 mkdir -p .../dnsmasq-shared.d wifi_manager.py:922 Together these are the captive portal: unblock the radio, bring up the AP, add the redirect, turn on forwarding, and undo all of it afterwards. Without the grants a hardened install would associate clients to the access point and then fail to route them. Why it has gone unnoticed: a stock Raspberry Pi image ships /etc/sudoers.d/010_pi-nopasswd granting the default user ALL=(ALL) NOPASSWD: ALL which satisfies every one of these regardless of what the allow-lists say. Confirmed on a live rig -- `sudo -n -l` permits sysctl there, and the blanket rule is why. The allow-lists are effectively decorative on a default image and only start mattering once that rule is removed or the service runs as another user. test_sudo_allowlist_covers_calls.py extracts every argv-style sudo call in src/ and web_interface/ and asserts an installer grants it, so the next command added without a rule fails here rather than on someone's hardened box. Getting that test honest took three passes, each worth recording: - Matching the literal "systemctl" against rules written as `$SYSTEMCTL_PATH enable ...` reported six gaps that did not exist. Binary path variables are now normalised before comparing. - Scanning the whole installer let `NFT_PATH=$(command -v nft)` -- a variable definition, not a grant -- satisfy the check on its own, so deleting the actual nft rules still passed. Only NOPASSWD lines are considered now. - `sudo -n ` reported "-n" as the binary. sudo's own flags are skipped. Each of the five grants is individually mutation-checked: removing any one fails the suite. (cherry picked from commit a372b43cd172d9821a19873ca7ac01aaa22aff30) --- scripts/install/configure_wifi_permissions.sh | 27 ++++ test/test_sudo_allowlist_covers_calls.py | 134 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 test/test_sudo_allowlist_covers_calls.py diff --git a/scripts/install/configure_wifi_permissions.sh b/scripts/install/configure_wifi_permissions.sh index cfba4bc4..3da68439 100755 --- a/scripts/install/configure_wifi_permissions.sh +++ b/scripts/install/configure_wifi_permissions.sh @@ -37,6 +37,11 @@ echo " systemctl: $SYSTEMCTL_PATH" echo "" echo "Step 1: Configuring sudo permissions for nmcli..." SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi" +SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl) +NFT_PATH=$(command -v nft || echo /usr/sbin/nft) +RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill) +IPTABLES_PATH=$(command -v iptables || echo /usr/sbin/iptables) +MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir) # Create a temporary sudoers file using mktemp (handles permissions better) TEMP_SUDOERS=$(mktemp) || { @@ -62,6 +67,28 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager +# The captive portal turns IP forwarding on while the access point is up and +# restores the previous value when it comes down (wifi_manager._setup_iptables_ +# redirect / _teardown_iptables_redirect). Without this rule that sudo call +# needs a password, so forwarding stays off and clients associate to the AP but +# cannot route. It goes unnoticed on a stock Raspberry Pi image, where +# /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and +# masks every gap in this file -- it only bites once that blanket rule is +# removed. +$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0 +$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1 +# The portal's redirect lives in its own nftables table, created when the AP +# comes up and deleted when it goes down, and the radio has to be unblocked +# before the AP can start at all. Same story as the sysctl rules above: called +# with sudo, never granted here, and invisible on a stock Pi image. +$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix +$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix +$WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi +# The portal also inserts and removes its own iptables rules and creates +# NetworkManager's dnsmasq drop-in directory. Wildcards rather than exact +# argument lists: those rules are built from the live interface name and port. +$WEB_USER ALL=(ALL) NOPASSWD: $IPTABLES_PATH * +$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d # Allow copying hostapd and dnsmasq config files into place $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf diff --git a/test/test_sudo_allowlist_covers_calls.py b/test/test_sudo_allowlist_covers_calls.py new file mode 100644 index 00000000..8105f7fd --- /dev/null +++ b/test/test_sudo_allowlist_covers_calls.py @@ -0,0 +1,134 @@ +"""Every sudo the code runs must be granted by an installer allow-list. + +The installers write two files -- /etc/sudoers.d/ledmatrix_web and +/etc/sudoers.d/ledmatrix_wifi -- each an explicit allow-list. Anything the code +calls with sudo that is not in one of them needs a password, which a service +cannot supply, so the call fails. + +That failure is invisible on a stock Raspberry Pi image, because +/etc/sudoers.d/010_pi-nopasswd grants the default user + + ALL=(ALL) NOPASSWD: ALL + +which masks every gap in both files. It only surfaces on a system where that +blanket rule has been removed, or where the service runs as a different user -- +so a missing entry can sit there for a long time before anyone hits it. + +One was: wifi_manager turns IP forwarding on while the captive portal's access +point is up and restores it afterwards, calling `sudo sysctl -w +net.ipv4.ip_forward=...`. Neither allow-list granted sysctl. On such a system +clients would associate to the AP and then fail to route. +""" +import re +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +INSTALLERS = ( + ROOT / "first_time_install.sh", + ROOT / "scripts" / "install" / "configure_wifi_permissions.sh", +) +SOURCES = (ROOT / "src", ROOT / "web_interface") + +# argv-style sudo calls: ["sudo", , "arg", ...] +CALL = re.compile(r"""\[\s*["']sudo["']\s*,\s*(?P[^\]]+)\]""") + + +def _first_two_args(rest): + """The binary and its first argument, as written.""" + parts = [p.strip() for p in rest.split(",")] + # Drop sudo's own flags: `sudo -n ` is a call to , and + # treating "-n" as the binary reports a gap that does not exist. + flag = re.compile(r'[\'"]-[a-zA-Z]+[\'"]') + while parts and flag.fullmatch(parts[0]): + parts = parts[1:] + # Always two entries: `["sudo", "reboot"]` has no subcommand, and the + # caller unpacks a fixed pair. + out = [] + for part in (parts + ["", ""])[:2]: + if not part: + out.append("") + continue + literal = re.fullmatch(r"""["'](.+)["']""", part) + if literal: + out.append(literal.group(1)) + else: + # A variable such as sysctl_bin: reduce to the tool it resolves to. + out.append(part.split(".")[-1].replace("_bin", "").replace("_path", "")) + return out + + +def _sudo_calls(): + found = {} + for base in SOURCES: + for path in base.rglob("*.py"): + text = path.read_text(encoding="utf-8", errors="replace") + for match in CALL.finditer(text): + args = _first_two_args(match.group("rest")) + if not args: + continue + line = text[:match.start()].count("\n") + 1 + found.setdefault(tuple(args), f"{path.relative_to(ROOT)}:{line}") + return found + + +def _allowlisted_text(): + """The allow-list rules, with binary-path variables reduced to tool names. + + The installers write rules like `$SYSTEMCTL_PATH enable ledmatrix.service`, + so matching on the literal "systemctl" finds nothing and every systemctl + rule looks absent. Normalise $FOO_PATH and /usr/bin/foo down to foo before + comparing, or the check reports gaps that are not there -- which it did on + the first run. + """ + # NOPASSWD lines only. Taking the whole script would let a variable + # definition such as NFT_PATH=$(command -v nft) satisfy the check on its + # own, which is how an earlier version of this test passed while the grant + # itself had been deleted. + lines = [] + for installer in INSTALLERS: + if not installer.is_file(): + continue + for line in installer.read_text(encoding="utf-8", errors="replace").splitlines(): + if "NOPASSWD:" in line: + lines.append(line.split("NOPASSWD:", 1)[1]) + text = "\n".join(lines) + text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", + lambda m: m.group(1).lower(), text) + text = re.sub(r"/usr/(?:s?bin)/", "", text) + return text + + +def test_the_installers_are_present(): + missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()] + assert not missing, f"installer(s) missing: {missing}" + + +def test_every_sudo_call_is_granted(): + allow = _allowlisted_text() + calls = _sudo_calls() + assert calls, "no sudo calls found; the matcher has stopped working" + + ungranted = [] + for (binary, first_arg), where in sorted(calls.items()): + # A rule mentions the tool and, where it takes a subcommand, that too. + if binary not in allow: + ungranted.append(f"{binary} ({where})") + continue + if first_arg and not first_arg.startswith("-"): + pattern = rf"{re.escape(binary)}\s+{re.escape(first_arg)}" + if binary in ("systemctl",) and not re.search(pattern, allow): + ungranted.append(f"{binary} {first_arg} ({where})") + + assert not ungranted, ( + "these run under sudo but no installer grants them; on a stock Pi the " + "blanket 010_pi-nopasswd rule hides this:\n " + "\n ".join(ungranted)) + + +@pytest.mark.parametrize("needle", ["ip_forward"]) +def test_the_captive_portal_forwarding_rule_is_granted(needle): + """The specific gap this test was written for.""" + assert needle in _allowlisted_text(), ( + "no allow-list entry for sysctl ip_forward; the captive portal enables " + "forwarding while its AP is up and cannot without one") From 61d3e3d5527b87ef5bdff888f6c0547563a87d95 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 01:54:35 -0400 Subject: [PATCH 5/6] fix(install): drop the iptables wildcard, and pin each grant properly Review follow-up. Two findings, both right, and the first is a hole I opened myself. `NOPASSWD: iptables *` is a root shell for the web user by another name. `iptables --modprobe=/path/to/anything` runs that path as root, so a wildcard grant on iptables escalates rather than restricts. I added that rule while fixing a permissions gap, which is a worse outcome than the gap. It is gone, and a test now fails on any trailing-wildcard grant to a tool that can execute another program -- iptables, nft, tcpdump, find, awk, sed, perl, python, env. The other finding: checking only the binary made the coverage test far weaker than it looked. With `sysctl` present anywhere in the allow-list, deleting the `net.ipv4.ip_forward=0` grant still passed -- and the portal would then be unable to restore forwarding on teardown. Each required command is now matched in full, and each is mutation-checked individually, including that exact single-line case. Scope pulled in deliberately. The first version of this test tried to assert that *every* sudo call in the codebase is granted. Run honestly, it showed the portal also runs iptables, nft, `ip addr`, `ip link` and `cp` with arguments built at runtime -- an interface name, a port. Those cannot be granted safely in a sudoers file: the rule needs a trailing wildcard, and that is the escalation above. Closing that half needs a privileged helper that builds the rules itself and takes only an interface and a port, granted the way safe_plugin_rm.sh already is. That is a design decision, not a one-line grant, so the test now pins the four commands this change actually grants and the docstring says plainly what it does not cover. Better a narrow test that is true than a broad one that is not. (cherry picked from commit 500cfbc9f43cd359b32c1953d022bc38c3471de2) --- scripts/install/configure_wifi_permissions.sh | 17 +- test/test_sudo_allowlist_covers_calls.py | 200 ++++++++---------- 2 files changed, 105 insertions(+), 112 deletions(-) diff --git a/scripts/install/configure_wifi_permissions.sh b/scripts/install/configure_wifi_permissions.sh index 3da68439..aa0de967 100755 --- a/scripts/install/configure_wifi_permissions.sh +++ b/scripts/install/configure_wifi_permissions.sh @@ -40,7 +40,6 @@ SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi" SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl) NFT_PATH=$(command -v nft || echo /usr/sbin/nft) RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill) -IPTABLES_PATH=$(command -v iptables || echo /usr/sbin/iptables) MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir) # Create a temporary sudoers file using mktemp (handles permissions better) @@ -84,11 +83,19 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1 $WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix $WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix $WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi -# The portal also inserts and removes its own iptables rules and creates -# NetworkManager's dnsmasq drop-in directory. Wildcards rather than exact -# argument lists: those rules are built from the live interface name and port. -$WEB_USER ALL=(ALL) NOPASSWD: $IPTABLES_PATH * +# NetworkManager's dnsmasq drop-in directory, exact path. $WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d +# +# iptables is deliberately NOT granted here. Its rules are built from the live +# interface name and port, so a rule covering them needs a trailing wildcard -- +# and `iptables --modprobe=/path/to/anything` runs that path as root, so +# `NOPASSWD: iptables *` is a root shell for the web user by another name. That +# is a worse outcome than the gap it would close, which today is masked anyway +# by the blanket NOPASSWD rule on stock Pi images. +# +# Closing it safely means a wrapper script that builds the rules itself and +# takes only an interface and a port, granted the way safe_plugin_rm.sh already +# is. That belongs in its own change rather than being smuggled into this one. # Allow copying hostapd and dnsmasq config files into place $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf diff --git a/test/test_sudo_allowlist_covers_calls.py b/test/test_sudo_allowlist_covers_calls.py index 8105f7fd..3d3809a1 100644 --- a/test/test_sudo_allowlist_covers_calls.py +++ b/test/test_sudo_allowlist_covers_calls.py @@ -1,23 +1,32 @@ -"""Every sudo the code runs must be granted by an installer allow-list. - -The installers write two files -- /etc/sudoers.d/ledmatrix_web and -/etc/sudoers.d/ledmatrix_wifi -- each an explicit allow-list. Anything the code -calls with sudo that is not in one of them needs a password, which a service -cannot supply, so the call fails. - -That failure is invisible on a stock Raspberry Pi image, because -/etc/sudoers.d/010_pi-nopasswd grants the default user - - ALL=(ALL) NOPASSWD: ALL - -which masks every gap in both files. It only surfaces on a system where that -blanket rule has been removed, or where the service runs as a different user -- -so a missing entry can sit there for a long time before anyone hits it. - -One was: wifi_manager turns IP forwarding on while the captive portal's access -point is up and restores it afterwards, calling `sudo sysctl -w -net.ipv4.ip_forward=...`. Neither allow-list granted sysctl. On such a system -clients would associate to the AP and then fail to route. +"""The captive portal's fixed-argument sudo calls must be granted. + +The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and +ledmatrix_wifi. A sudo call absent from both needs a password, which a service +cannot supply, so it fails. + +Four such calls were ungranted, all of them captive-portal teardown/setup: + + sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883 + nft add|delete table ip ledmatrix wifi_manager.py:835, 895 + rfkill unblock wifi wifi_manager.py:1811 + mkdir -p .../dnsmasq-shared.d wifi_manager.py:922 + +It goes unnoticed because a stock Raspberry Pi image ships +/etc/sudoers.d/010_pi-nopasswd granting the default user +`ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only +bites once that blanket rule is removed or the service runs as another user. + +Scope, deliberately narrow: this pins the four commands above, each of which +can be written out literally. The portal makes further sudo calls whose +arguments are built at runtime -- iptables and nft rules carrying an interface +name and a port, `ip addr`, `ip link` -- and those cannot be granted safely +here. A rule covering them needs a trailing wildcard, and +`iptables --modprobe=/path/to/anything` runs that path as root, so +`NOPASSWD: iptables *` is a root shell for the web user by another name. +Closing that half needs a privileged helper that builds the rules itself and +takes only an interface and a port, granted the way safe_plugin_rm.sh already +is. That is a design decision, not a one-line grant, and belongs in its own +change. """ import re from pathlib import Path @@ -29,63 +38,24 @@ ROOT / "first_time_install.sh", ROOT / "scripts" / "install" / "configure_wifi_permissions.sh", ) -SOURCES = (ROOT / "src", ROOT / "web_interface") - -# argv-style sudo calls: ["sudo", , "arg", ...] -CALL = re.compile(r"""\[\s*["']sudo["']\s*,\s*(?P[^\]]+)\]""") - - -def _first_two_args(rest): - """The binary and its first argument, as written.""" - parts = [p.strip() for p in rest.split(",")] - # Drop sudo's own flags: `sudo -n ` is a call to , and - # treating "-n" as the binary reports a gap that does not exist. - flag = re.compile(r'[\'"]-[a-zA-Z]+[\'"]') - while parts and flag.fullmatch(parts[0]): - parts = parts[1:] - # Always two entries: `["sudo", "reboot"]` has no subcommand, and the - # caller unpacks a fixed pair. - out = [] - for part in (parts + ["", ""])[:2]: - if not part: - out.append("") - continue - literal = re.fullmatch(r"""["'](.+)["']""", part) - if literal: - out.append(literal.group(1)) - else: - # A variable such as sysctl_bin: reduce to the tool it resolves to. - out.append(part.split(".")[-1].replace("_bin", "").replace("_path", "")) - return out - - -def _sudo_calls(): - found = {} - for base in SOURCES: - for path in base.rglob("*.py"): - text = path.read_text(encoding="utf-8", errors="replace") - for match in CALL.finditer(text): - args = _first_two_args(match.group("rest")) - if not args: - continue - line = text[:match.start()].count("\n") + 1 - found.setdefault(tuple(args), f"{path.relative_to(ROOT)}:{line}") - return found - - -def _allowlisted_text(): - """The allow-list rules, with binary-path variables reduced to tool names. - - The installers write rules like `$SYSTEMCTL_PATH enable ledmatrix.service`, - so matching on the literal "systemctl" finds nothing and every systemctl - rule looks absent. Normalise $FOO_PATH and /usr/bin/foo down to foo before - comparing, or the check reports gaps that are not there -- which it did on - the first run. - """ - # NOPASSWD lines only. Taking the whole script would let a variable - # definition such as NFT_PATH=$(command -v nft) satisfy the check on its - # own, which is how an earlier version of this test passed while the grant - # itself had been deleted. + +#: Commands this change grants, each fully literal in the source. +REQUIRED = ( + ("sysctl", "-w", "net.ipv4.ip_forward=0"), + ("sysctl", "-w", "net.ipv4.ip_forward=1"), + ("nft", "add", "table", "ip", "ledmatrix"), + ("nft", "delete", "table", "ip", "ledmatrix"), + ("rfkill", "unblock", "wifi"), + ("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"), +) + +#: Tools with an option that executes a program of the caller's choosing. +#: A trailing wildcard on any of these is a privilege escalation. +EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk", + "sed", "perl", "python", "python3", "env") + + +def _grant_lines(): lines = [] for installer in INSTALLERS: if not installer.is_file(): @@ -93,11 +63,22 @@ def _allowlisted_text(): for line in installer.read_text(encoding="utf-8", errors="replace").splitlines(): if "NOPASSWD:" in line: lines.append(line.split("NOPASSWD:", 1)[1]) - text = "\n".join(lines) - text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", - lambda m: m.group(1).lower(), text) - text = re.sub(r"/usr/(?:s?bin)/", "", text) - return text + return lines + + +def _normalised_grants(): + """Grants with binary-path variables reduced to tool names. + + Rules are written as `$SYSCTL_PATH -w ...`, so matching the literal + "sysctl" finds nothing and every rule looks absent -- which is exactly how + an earlier version of this test reported six gaps that did not exist. + Only NOPASSWD lines are considered, because taking the whole script let a + variable definition such as NFT_PATH=$(command -v nft) satisfy the check on + its own while the grant itself had been deleted. + """ + text = "\n".join(_grant_lines()) + text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text) + return re.sub(r"/usr/(?:s?bin)/", "", text) def test_the_installers_are_present(): @@ -105,30 +86,35 @@ def test_the_installers_are_present(): assert not missing, f"installer(s) missing: {missing}" -def test_every_sudo_call_is_granted(): - allow = _allowlisted_text() - calls = _sudo_calls() - assert calls, "no sudo calls found; the matcher has stopped working" +@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c)) +def test_the_command_is_granted(command): + """Whole command, not just the binary. - ungranted = [] - for (binary, first_arg), where in sorted(calls.items()): - # A rule mentions the tool and, where it takes a subcommand, that too. - if binary not in allow: - ungranted.append(f"{binary} ({where})") + Checking only the binary made this far weaker than it looked: with + `sysctl` present anywhere, deleting the ip_forward=0 grant still passed, + and the portal would then be unable to restore forwarding on teardown. + """ + pattern = r"\s+".join(re.escape(word) for word in command) + assert re.search(pattern, _normalised_grants()), ( + f"no installer grants `{' '.join(command)}`") + + +def test_no_wildcard_on_a_tool_that_can_exec(): + """`NOPASSWD: iptables *` hands the web user root. + + iptables --modprobe=/path runs that path as root. This caught a grant added + in this very change, which is why it is here. + """ + offenders = [] + for rule in _grant_lines(): + rule = rule.strip() + if not rule.endswith("*"): continue - if first_arg and not first_arg.startswith("-"): - pattern = rf"{re.escape(binary)}\s+{re.escape(first_arg)}" - if binary in ("systemctl",) and not re.search(pattern, allow): - ungranted.append(f"{binary} {first_arg} ({where})") - - assert not ungranted, ( - "these run under sudo but no installer grants them; on a stock Pi the " - "blanket 010_pi-nopasswd rule hides this:\n " + "\n ".join(ungranted)) - - -@pytest.mark.parametrize("needle", ["ip_forward"]) -def test_the_captive_portal_forwarding_rule_is_granted(needle): - """The specific gap this test was written for.""" - assert needle in _allowlisted_text(), ( - "no allow-list entry for sysctl ip_forward; the captive portal enables " - "forwarding while its AP is up and cannot without one") + haystack = rule.replace("_PATH", "").lower() + for tool in EXEC_CAPABLE: + if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack): + offenders.append(rule) + break + assert not offenders, ( + "wildcard grant on a tool that can execute another program:\n " + + "\n ".join(offenders)) From e36e0507ec4b369c3bea54c4bc95edb81c3dc2c2 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 21 Aug 2026 13:09:05 -0400 Subject: [PATCH 6/6] fix(install): pin PATH, keep unit order, and tighten the sudoers assertions Three review findings, all correct. The installer resolved binaries through an inherited PATH and wrote whatever it found into sudoers as NOPASSWD grants. first_time_install.sh re-execs itself with `sudo -E`, which preserves the caller's environment, so a writable directory early in PATH turned a compromise of the low-privilege web user into permanent root -- via a file the installer itself wrote. PATH is now pinned to the system directories before anything is resolved, and every resolved binary must be root-owned and unwritable by anyone else before it reaches the sudoers file. _unit_body() sorted a unit's lines before comparing. Order is not noise in a systemd unit: repeated ExecStartPre=/ExecStartPost= run in the order they appear, and a directive that moves between [Unit], [Service] and [Install] means something different where it lands. The drift check reported no drift for units that had genuinely changed. Order is preserved now. Two of that check's own tests asserted the wrong thing -- test_reordered_directives_are_not_drift said so in its name -- and are inverted, with a second covering a directive moved between sections. The cosmetic-difference test now varies comments, blank lines and indentation, which is what the installer actually drops, rather than reversing the file. The sudoers assertions matched command prefixes, so `sysctl -w net.ipv4.ip_forward=0 *` satisfied the requirement while granting the caller arbitrary trailing arguments as root. They are exact now. The wildcard check also normalises ${NFT_PATH} the same way as $NFT_PATH; the brace is not a word boundary, so that spelling was skipped entirely. Verified by reintroducing each: a widened required grant fails the exact match, `${NFT_PATH} *` fails the wildcard check, and require_trusted_binary refuses a non-root-owned, world-writable, or missing binary. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- scripts/install/configure_wifi_permissions.sh | 47 ++++++++++++++- src/startup_validator.py | 13 +++- test/test_sudo_allowlist_covers_calls.py | 59 ++++++++++++++----- test/test_systemd_unit_drift.py | 37 +++++++++--- 4 files changed, 127 insertions(+), 29 deletions(-) diff --git a/scripts/install/configure_wifi_permissions.sh b/scripts/install/configure_wifi_permissions.sh index aa0de967..37e2c7db 100755 --- a/scripts/install/configure_wifi_permissions.sh +++ b/scripts/install/configure_wifi_permissions.sh @@ -25,9 +25,44 @@ if [ "$EUID" -eq 0 ]; then exit 1 fi +# Resolve command paths against a fixed PATH, and check what we resolved. +# +# Every path found here is written into a sudoers file as a NOPASSWD grant, so +# whoever controls the binary at that path controls root. first_time_install.sh +# re-execs itself with `sudo -E`, which preserves the invoking user's +# environment -- PATH included -- so without pinning it, `which nmcli` can +# resolve to anything on that PATH: a writable directory early in it turns a +# compromise of the low-privilege web user into permanent root. +PATH=/usr/sbin:/usr/bin:/sbin:/bin +export PATH + +# A binary named in a sudoers rule must be root-owned and writable by nobody +# else, or the grant hands root to whoever can rewrite it. +require_trusted_binary() { + local label="$1" path="$2" + if [ ! -x "$path" ]; then + echo "✗ $label: $path is not an executable file" + exit 1 + fi + local owner perms + owner=$(stat -c '%u' "$path") || exit 1 + perms=$(stat -c '%a' "$path") || exit 1 + if [ "$owner" != "0" ]; then + echo "✗ $label: $path is not owned by root (uid $owner); refusing to" + echo " grant it NOPASSWD sudo." + exit 1 + fi + # Group- or world-writable means someone other than root can replace it. + case "$perms" in + *[2367]) echo "✗ $label: $path is writable by group or other ($perms);" + echo " refusing to grant it NOPASSWD sudo." + exit 1 ;; + esac +} + # Get the full paths to commands -NMCLI_PATH=$(which nmcli || echo "/usr/bin/nmcli") -SYSTEMCTL_PATH=$(which systemctl) +NMCLI_PATH=$(command -v nmcli || echo "/usr/bin/nmcli") +SYSTEMCTL_PATH=$(command -v systemctl) echo "Command paths:" echo " nmcli: $NMCLI_PATH" @@ -42,6 +77,14 @@ NFT_PATH=$(command -v nft || echo /usr/sbin/nft) RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill) MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir) +# Checked before any of them reaches the sudoers file. +require_trusted_binary "nmcli" "$NMCLI_PATH" +require_trusted_binary "systemctl" "$SYSTEMCTL_PATH" +require_trusted_binary "sysctl" "$SYSCTL_PATH" +require_trusted_binary "nft" "$NFT_PATH" +require_trusted_binary "rfkill" "$RFKILL_PATH" +require_trusted_binary "mkdir" "$MKDIR_PATH" + # Create a temporary sudoers file using mktemp (handles permissions better) TEMP_SUDOERS=$(mktemp) || { echo "✗ Failed to create temporary file" diff --git a/src/startup_validator.py b/src/startup_validator.py index 5dc0d0f0..79275dc2 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -134,13 +134,22 @@ def _validate_systemd_units(self) -> None: @staticmethod def _unit_body(text: str) -> str: - """A unit's meaningful lines: no comments, no blanks, no ordering noise.""" + """A unit's meaningful lines, in order: no comments, no blanks. + + Order is preserved deliberately. This used to sort, which made the + comparison insensitive to two changes that matter in a systemd unit: + repeated directives such as ExecStartPre= and ExecStartPost= run in + the order they appear, and a directive that moves between [Unit], + [Service] and [Install] means something different -- or nothing -- + where it lands. A drift check that normalises those away reports no + drift for a unit that has genuinely changed. + """ lines = [] for line in text.splitlines(): line = line.strip() if line and not line.startswith("#"): lines.append(line) - return "\n".join(sorted(lines)) + return "\n".join(lines) def _validate_config(self) -> None: """Validate configuration files.""" diff --git a/test/test_sudo_allowlist_covers_calls.py b/test/test_sudo_allowlist_covers_calls.py index 3d3809a1..bec2486b 100644 --- a/test/test_sudo_allowlist_covers_calls.py +++ b/test/test_sudo_allowlist_covers_calls.py @@ -66,19 +66,37 @@ def _grant_lines(): return lines -def _normalised_grants(): - """Grants with binary-path variables reduced to tool names. - - Rules are written as `$SYSCTL_PATH -w ...`, so matching the literal - "sysctl" finds nothing and every rule looks absent -- which is exactly how - an earlier version of this test reported six gaps that did not exist. - Only NOPASSWD lines are considered, because taking the whole script let a - variable definition such as NFT_PATH=$(command -v nft) satisfy the check on - its own while the grant itself had been deleted. +def _normalise(rule): + """One rule with binary-path variables reduced to bare tool names. + + Rules are written as `$SYSCTL_PATH -w ...` or `${NFT_PATH} ...`, so + matching the literal "sysctl" finds nothing and every rule looks absent -- + which is exactly how an earlier version of this test reported six gaps + that did not exist. Both spellings are handled: shell expands them + identically, and a check that understood only one silently skipped the + other. """ - text = "\n".join(_grant_lines()) - text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text) - return re.sub(r"/usr/(?:s?bin)/", "", text) + rule = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", + lambda m: m.group(1).lower(), rule) + return re.sub(r"/usr/(?:s?bin)/", "", rule) + + +def _granted_commands(): + """The command each NOPASSWD rule actually grants, normalised. + + _grant_lines() already returns everything after "NOPASSWD:", so what + arrives here is the command, possibly preceded by the NOEXEC tag and + possibly still carrying the closing quote of an `echo "..."` that wrote + it. Both are stripped so the result is comparable to a plain command. + """ + commands = [] + for rule in _grant_lines(): + command = _normalise(rule).strip() + command = re.sub(r"^NOEXEC:\s*", "", command) + command = command.rstrip('"').rstrip("'").strip() + if command: + commands.append(" ".join(command.split())) + return commands def test_the_installers_are_present(): @@ -94,9 +112,15 @@ def test_the_command_is_granted(command): `sysctl` present anywhere, deleting the ip_forward=0 grant still passed, and the portal would then be unable to restore forwarding on teardown. """ - pattern = r"\s+".join(re.escape(word) for word in command) - assert re.search(pattern, _normalised_grants()), ( - f"no installer grants `{' '.join(command)}`") + wanted = " ".join(command) + granted = _granted_commands() + # Exact match, not a prefix. A substring search was satisfied by + # `sysctl -w net.ipv4.ip_forward=0 *`, and that trailing wildcard lets the + # caller append whatever they like to a command running as root -- a far + # wider grant than the one this test is meant to be confirming. + assert wanted in granted, ( + f"no installer grants exactly `{wanted}`; closest matches: " + + str([g for g in granted if g.startswith(command[0])])[:200]) def test_no_wildcard_on_a_tool_that_can_exec(): @@ -110,7 +134,10 @@ def test_no_wildcard_on_a_tool_that_can_exec(): rule = rule.strip() if not rule.endswith("*"): continue - haystack = rule.replace("_PATH", "").lower() + # Normalised the same way as everything else: `${NFT_PATH} *` left a + # brace before the tool name, and the word-boundary check below does + # not treat "{" as a boundary, so that spelling slipped through. + haystack = _normalise(rule).lower() for tool in EXEC_CAPABLE: if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack): offenders.append(rule) diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py index 0aaa67cd..21cf56e0 100644 --- a/test/test_systemd_unit_drift.py +++ b/test/test_systemd_unit_drift.py @@ -68,11 +68,27 @@ def test_a_changed_directive_is_drift(): assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b) -def test_reordered_directives_are_not_drift(): - """systemd does not care about order within a section, so neither should this.""" - a = "[Service]\nExecStart=/x\nRestart=always\n" - b = "[Service]\nRestart=always\nExecStart=/x\n" - assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b) +def test_reordered_directives_are_drift(): + """Order is not noise in a systemd unit. + + Repeated directives -- ExecStartPre=, ExecStartPost= -- run in the order + they appear, and a directive that moves between [Unit], [Service] and + [Install] means something different, or nothing, where it lands. This + check used to sort the lines before comparing, which reported no drift for + a unit that had genuinely changed. + """ + a = "[Service]\nExecStartPre=/first\nExecStartPre=/second\n" + b = "[Service]\nExecStartPre=/second\nExecStartPre=/first\n" + assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), ( + "swapping two ExecStartPre= lines changes what runs first, and was " + "being normalised away") + + +def test_a_directive_moved_between_sections_is_drift(): + a = "[Unit]\nDescription=x\n[Service]\nExecStart=/x\n" + b = "[Unit]\nDescription=x\nExecStart=/x\n[Service]\n" + assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), ( + "ExecStart= in [Unit] is not the same unit, and sorting hid it") def test_cosmetic_differences_do_not_warn(validator, tmp_path): @@ -93,11 +109,14 @@ def test_cosmetic_differences_do_not_warn(validator, tmp_path): substituted = (template.read_text(encoding="utf-8") .replace("__PROJECT_ROOT_DIR__", str(project_root)) .replace("__USER__", "root")) - # Same directives, stripped of comments and blank lines and reordered. - directives = sorted(line.strip() for line in substituted.splitlines() - if line.strip() and not line.strip().startswith("#")) + # Cosmetic means comments, blank lines and stray indentation -- the things + # the installer really does drop. Not reordering: that changes the unit, + # and is asserted as drift above. + directives = [line.strip() for line in substituted.splitlines() + if line.strip() and not line.strip().startswith("#")] installed = tmp_path / "ledmatrix.service" - installed.write_text("\n".join(reversed(directives)) + "\n", encoding="utf-8") + installed.write_text( + "\n\n".join(" " + d for d in directives) + "\n", encoding="utf-8") validator._UNITS = ((template_rel, str(installed)),) validator._validate_systemd_units()