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 <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 <tool>` 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 a372b43cd1)
2026-08-20 09:59:09 +00:00 · 2026-08-20 03:54:09 -04:00
parent e1b445268d
commit 324a4e43b1
2 changed files with 161 additions and 0 deletions
@@ -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
@@ -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
+
+    <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", <binary-or-var>, "arg", ...]
+CALL = re.compile(r"""\[\s*["']sudo["']\s*,\s*(?P<rest>[^\]]+)\]""")
+
+
+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 <tool>` is a call to <tool>, 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")