diff --git a/first_time_install.sh b/first_time_install.sh index 55d4f455..5749f533 100755 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1504,6 +1504,9 @@ echo "------------------------------------------------" # Create sudoers configuration for the web interface echo "Creating sudoers configuration..." SUDOERS_FILE="/etc/sudoers.d/ledmatrix_web" +# A predictable name in a world-writable directory is a symlink target; +# root writes the rules here, so let mktemp pick the name. +SUDOERS_TMP=$(mktemp "${TMPDIR:-/tmp}/ledmatrix_web_sudoers.XXXXXX") # Get command paths PYTHON_PATH=$(which python3) @@ -1514,7 +1517,7 @@ BASH_PATH=$(which bash) JOURNALCTL_PATH=$(which journalctl 2>/dev/null || true) # Create sudoers content -cat > /tmp/ledmatrix_web_sudoers << EOF +cat > "$SUDOERS_TMP" << EOF # LED Matrix Web Interface passwordless sudo configuration # This allows the web interface user to run specific commands without a password @@ -1541,7 +1544,7 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/ $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/safe_pip_install.sh * EOF if [ -n "$JOURNALCTL_PATH" ]; then - cat >> /tmp/ledmatrix_web_sudoers << EOF + cat >> "$SUDOERS_TMP" << EOF # NOEXEC, because these rules end in a wildcard and journalctl starts a pager # when its output is a terminal. From that pager (less) a "!sh" is a root # shell -- the standard journalctl escalation. The web interface always passes @@ -1555,17 +1558,38 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix * EOF fi -if [ -f "$SUDOERS_FILE" ] && cmp -s /tmp/ledmatrix_web_sudoers "$SUDOERS_FILE"; then - echo "Sudoers configuration already up to date" - rm /tmp/ledmatrix_web_sudoers +# Never install rules we have not parsed. A malformed drop-in in +# /etc/sudoers.d makes sudo refuse every command for every user, which on a +# headless Pi leaves no way in at all. If the rules do not parse, say so and +# keep whatever is already installed. +SUDOERS_VALID=1 +if command -v visudo >/dev/null 2>&1; then + if ! visudo -c -f "$SUDOERS_TMP" >/dev/null 2>&1; then + SUDOERS_VALID=0 + echo "⚠ The generated sudoers rules did not parse:" >&2 + visudo -c -f "$SUDOERS_TMP" >&2 || true + echo "⚠ Leaving $SUDOERS_FILE unchanged. The web interface cannot control" >&2 + echo " the display service until this is fixed." >&2 + fi else - echo "Installing/updating sudoers configuration..." - cp /tmp/ledmatrix_web_sudoers "$SUDOERS_FILE" - chmod 440 "$SUDOERS_FILE" - rm /tmp/ledmatrix_web_sudoers + echo "⚠ visudo not found; installing the sudoers rules unvalidated" fi -echo "✓ Passwordless sudo access configured" +if [ "$SUDOERS_VALID" = "0" ]; then + rm -f "$SUDOERS_TMP" +elif [ -f "$SUDOERS_FILE" ] && cmp -s "$SUDOERS_TMP" "$SUDOERS_FILE"; then + echo "Sudoers configuration already up to date" + rm -f "$SUDOERS_TMP" +else + echo "Installing/updating sudoers configuration..." + cp "$SUDOERS_TMP" "$SUDOERS_FILE" + chmod 440 "$SUDOERS_FILE" + rm -f "$SUDOERS_TMP" +fi + +if [ "$SUDOERS_VALID" = "1" ]; then + echo "✓ Passwordless sudo access configured" +fi echo "" CURRENT_STEP="Configure WiFi management permissions" diff --git a/scripts/install/configure_web_sudo.sh b/scripts/install/configure_web_sudo.sh index bbcc14ed..e73f5916 100755 --- a/scripts/install/configure_web_sudo.sh +++ b/scripts/install/configure_web_sudo.sh @@ -130,6 +130,19 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$" echo "$WEB_USER ALL=(ALL) NOPASSWD: $BASH_PATH $SAFE_PIP_INSTALL_PATH *" } > "$TEMP_SUDOERS" +# Never offer to install rules we have not parsed. A malformed drop-in in +# /etc/sudoers.d makes sudo refuse every command for every user. +if command -v visudo >/dev/null 2>&1; then + if ! visudo -c -f "$TEMP_SUDOERS" >/dev/null 2>&1; then + echo "" + echo "✗ The generated sudoers rules did not parse:" >&2 + visudo -c -f "$TEMP_SUDOERS" >&2 || true + echo "Nothing was changed." >&2 + rm -f "$TEMP_SUDOERS" + exit 1 + fi +fi + echo "" echo "Generated sudoers configuration:" echo "--------------------------------" diff --git a/test/test_sudoers_is_validated.py b/test/test_sudoers_is_validated.py new file mode 100644 index 00000000..a29c5779 --- /dev/null +++ b/test/test_sudoers_is_validated.py @@ -0,0 +1,120 @@ +"""The generated sudoers rules must parse before they reach /etc/sudoers.d. + +A malformed drop-in there makes sudo refuse every command for every user. On a +headless Pi that is unrecoverable without pulling the SD card, so both +installers run `visudo -c` on the file they generated before installing it. + +The render test also gives us the check neither installer had: that the rules +they actually emit are valid sudoers syntax on a real Linux box. +""" + +import os +import shutil +import subprocess +import sys +import tempfile + +import pytest + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +FIRST_TIME = os.path.join(REPO_ROOT, "first_time_install.sh") +CONFIGURE = os.path.join(REPO_ROOT, "scripts", "install", "configure_web_sudo.sh") + +VISUDO = shutil.which("visudo") or ( + "/usr/sbin/visudo" if os.path.exists("/usr/sbin/visudo") else None +) + + +def _read(path): + with open(path, "r", encoding="utf-8") as handle: + return handle.read() + + +def test_first_time_install_validates_before_installing(): + body = _read(FIRST_TIME) + assert 'visudo -c -f "$SUDOERS_TMP"' in body + install = body.index('cp "$SUDOERS_TMP" "$SUDOERS_FILE"') + validate = body.index('visudo -c -f "$SUDOERS_TMP"') + assert validate < install, "the rules must be checked before they are installed" + + +def test_the_install_is_gated_on_the_check(): + """Checking and then installing anyway would be worse than not checking.""" + body = _read(FIRST_TIME) + assert "SUDOERS_VALID=0" in body + gate = body.index('if [ "$SUDOERS_VALID" = "0" ]') + install = body.index('cp "$SUDOERS_TMP" "$SUDOERS_FILE"') + assert gate < install + + +def test_first_time_install_does_not_use_a_predictable_temp_file(): + body = _read(FIRST_TIME) + assert "mktemp" in body + assert "> /tmp/ledmatrix_web_sudoers" not in body + assert ">> /tmp/ledmatrix_web_sudoers" not in body + + +def test_configure_web_sudo_validates_before_installing(): + body = _read(CONFIGURE) + assert 'visudo -c -f "$TEMP_SUDOERS"' in body + install = body.index('cp "$TEMP_SUDOERS" /etc/sudoers.d/ledmatrix_web') + validate = body.index('visudo -c -f "$TEMP_SUDOERS"') + assert validate < install, "the rules must be checked before they are installed" + + +def _render_first_time_sudoers(project_root, user): + """Run the installer's own sudoers heredoc with realistic values.""" + body = _read(FIRST_TIME) + start = body.index("# Create sudoers content") + end = body.index("# Never install rules we have not parsed.") + block = body[start:end] + out = os.path.join(project_root, "rendered") + script = "\n".join( + [ + "set -euo pipefail", + f"ACTUAL_USER={user}", + f"PROJECT_ROOT_DIR={project_root}", + 'SUDOERS_TMP="$(mktemp)"', + "PYTHON_PATH=$(which python3)", + "SYSTEMCTL_PATH=/usr/bin/systemctl", + "REBOOT_PATH=/usr/sbin/reboot", + "POWEROFF_PATH=/usr/sbin/poweroff", + "BASH_PATH=$(which bash)", + "JOURNALCTL_PATH=/usr/bin/journalctl", + block, + f'cp "$SUDOERS_TMP" {out}', + ] + ) + subprocess.run(["bash", "-c", script], check=True) + return out + + +@pytest.mark.skipif(sys.platform == "win32", reason="visudo is POSIX only") +@pytest.mark.skipif(VISUDO is None, reason="visudo not installed") +def test_the_rules_the_installer_emits_actually_parse(): + with tempfile.TemporaryDirectory() as tmp: + rendered = _render_first_time_sudoers(tmp, "ledmatrix") + os.chmod(rendered, 0o440) + result = subprocess.run( + [VISUDO, "-c", "-f", rendered], capture_output=True, text=True + ) + assert result.returncode == 0, result.stdout + result.stderr + + +@pytest.mark.skipif(sys.platform == "win32", reason="visudo is POSIX only") +@pytest.mark.skipif(VISUDO is None, reason="visudo not installed") +def test_a_broken_rule_is_caught_rather_than_installed(): + """The guard is only worth having if visudo rejects what it should.""" + with tempfile.TemporaryDirectory() as tmp: + rendered = _render_first_time_sudoers(tmp, "ledmatrix") + with open(rendered, "r", encoding="utf-8") as handle: + good = handle.read() + broken = os.path.join(tmp, "broken") + with open(broken, "w", encoding="utf-8") as handle: + # An empty command path is what an unset $BASH_PATH would produce. + handle.write(good + "\nledmatrix ALL=(ALL) NOPASSWD:\n") + os.chmod(broken, 0o440) + result = subprocess.run( + [VISUDO, "-c", "-f", broken], capture_output=True, text=True + ) + assert result.returncode != 0