From 78fcb018776b780984183b01cc597a982c1a1933 Mon Sep 17 00:00:00 2001 From: Mike Teehan Date: Sat, 5 Sep 2026 00:38:34 +0000 Subject: [PATCH] fix: install.sh loop abort, ACME poll sudo gate, /static/ sub-paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - install.sh: the traversal-chmod loop assigned _d but looped over the never-set $d; under set -u every fresh install aborted with "d: unbound variable" at that line. Loop over $_d. - acme collector: the self-heal normalize (sudo chmod g+rwX) now runs only when a no-sudo group-read-bit probe detects a lost bit — acme.sh re-hardens the tree 600 on every run, so the steady-state poll makes no sudo call. The group bit (not daemon readability) is what the two-user model keeps for the WebUI user. - lib.acme: new get_acme_home() accessor (ACME_HOME env, default data/acme), reused by _run_acme; _summarize_acme_output preserves a "Permission denied" line even when it is not among the final two, so the collector's actionable-error matcher keeps firing. - nginx template: emit location /static/ for any is_management path (not only '/'); the SPA references /static/... at the domain root regardless of the management backend path. - tests: probe, summarizer, and nginx-subpath cases in test_state.py, test_acme.py, test_nginx.py. --- daemon/collectors/acme.py | 39 +++++++++++++---- lib/acme.py | 20 +++++++-- scripts/install.sh | 6 +-- system/nginx/server_block.conf | 2 +- tests/test_acme.py | 32 ++++++++++++++ tests/test_nginx.py | 20 +++++++++ tests/test_state.py | 80 +++++++++++++++++++++++++++++++++- 7 files changed, 182 insertions(+), 17 deletions(-) diff --git a/daemon/collectors/acme.py b/daemon/collectors/acme.py index 8599e4b..5aae767 100644 --- a/daemon/collectors/acme.py +++ b/daemon/collectors/acme.py @@ -3,9 +3,11 @@ import logging import os from pathlib import Path +from stat import S_IRGRP from typing import Any from lib import schema +from lib.acme import get_acme_home from lib.common import load_json from lib.state import PROJECT_DIR, _now_iso, register_collector @@ -157,6 +159,25 @@ def _friendly_acme_error(exc: Exception) -> str: return text +def _acme_home_needs_normalize() -> bool: + """Cheap no-sudo probe: has any ACME_HOME file lost its group-read bit? + + acme.sh re-hardens its tree (``chmod 600``) on every run, so the daemon's + self-heal (``normalize_acme_home``) is only needed after a run by another + user (e.g. a manual run as the WebUI user) stripped group read. The probe + checks the group bit — not the daemon's own readability — because group + read is what the two-user model keeps for the WebUI user; a file the + daemon can read but the group cannot must still be healed. + """ + try: + for p in get_acme_home().rglob("*"): + if p.is_file() and not (p.stat().st_mode & S_IRGRP): + return True + except OSError: + return True + return False + + def _collect_acme() -> schema.AcmeState: """Collect ACME certificate list and email. @@ -171,17 +192,19 @@ def _collect_acme() -> schema.AcmeState: # `status.error` so the poll diff still detects recovery. cert_error: str | None = None try: - # Self-heal ACME_HOME permissions before listing, exactly like the - # handler preflight (_run_acme_preflight). acme.sh dot-sources - # account.conf on startup; a prior run by another user (e.g. a manual - # run as the WebUI user) can leave it owner-only and make `--list` - # exit 2. The startup normalize only covers the first collection, so - # the poll must normalize too or a mid-lifetime ownership flip would - # blank the cert list until the next issue/renew or daemon restart. + # Self-heal ACME_HOME permissions before listing, but only when the + # probe detects a lost group-read bit — the steady-state poll then + # makes no sudo call. acme.sh dot-sources account.conf on startup; a + # prior run by another user (e.g. a manual run as the WebUI user) can + # leave it owner-only and make `--list` exit 2. The startup normalize + # only covers the first collection, so the poll must probe too or a + # mid-lifetime ownership flip would blank the cert list until the + # next issue/renew or daemon restart. from daemon.handlers.acme import normalize_acme_home from lib.acme import list_certs - normalize_acme_home() + if _acme_home_needs_normalize(): + normalize_acme_home() certs = list_certs() except Exception as exc: logger.warning("ACME state collection failed", exc_info=True) diff --git a/lib/acme.py b/lib/acme.py index 41b3d50..a629daa 100644 --- a/lib/acme.py +++ b/lib/acme.py @@ -32,6 +32,11 @@ _ACME_ENVIRON = { _WEBROOT = PROJECT_DIR / "data" / "acme" / "www" +def get_acme_home() -> Path: + """Resolve the ACME home directory (``ACME_HOME`` env, default ``data/acme``).""" + return Path(os.environ.get("ACME_HOME", str(_ACME_HOME))) + + def _find_acme() -> str: """Locate the acme.sh binary on the system. @@ -87,7 +92,7 @@ def _run_acme(args: list[str]) -> str: acme_bin = _find_acme() # Check for ACME_HOME env var (set by systemd in production) - acme_home_env = os.environ.get("ACME_HOME", str(_ACME_HOME)) + acme_home_env = str(get_acme_home()) cmd: list[str] = [ acme_bin, @@ -143,14 +148,22 @@ def _summarize_acme_output(output: str) -> str: in the final lines (e.g. "The retryafter=86400 value is too large (> 600), will not retry anymore."). Strips per-line timestamps and the "Please check log file" pointer so the summary stays toast- - sized. The full transcript remains in the log and acme.sh.log. + sized. A "Permission denied" diagnostic is preserved even when it + is not among the final lines — the actionable-error matcher in + daemon/collectors/acme.py keys off it. The full transcript remains + in the log and acme.sh.log. """ lines = [line.strip() for line in output.strip().splitlines() if line.strip()] lines = [re.sub(r"^\[[^\]]*\] ", "", line) for line in lines] lines = [line for line in lines if not line.startswith("Please check log file")] if not lines: return "(no output)" - return "; ".join(lines[-2:]) + tail = list(lines[-2:]) + for line in reversed(lines): + if "Permission denied" in line and line not in tail: + tail.insert(0, line) + break + return "; ".join(tail) def set_email(email: str) -> None: @@ -649,6 +662,7 @@ __all__ = [ "days_until_expiry", "deploy", "find_cert_dir", + "get_acme_home", "get_cert_info", "get_cert_paths", "get_email", diff --git a/scripts/install.sh b/scripts/install.sh index cbfd1e1..25541f4 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -253,9 +253,9 @@ mkdir -p /etc/dnsmasq chmod -R a+rX "${PROJECT_DIR}/webui/static" # ...and traversal (x only) up the parent chain, so repo-in-$HOME installs work. _d="${PROJECT_DIR}" -while [[ "$d" != "/" && -n "$d" ]]; do - chmod a+x "$d" 2>/dev/null || true - d="$(dirname "$d")" +while [[ "$_d" != "/" && -n "$_d" ]]; do + chmod a+x "$_d" 2>/dev/null || true + _d="$(dirname "$_d")" done # Set ownership: daemon owns project dir in prod, repo owner keeps ownership in dev. # The top-level .git (directory or worktree pointer file) is left untouched so diff --git a/system/nginx/server_block.conf b/system/nginx/server_block.conf index cc72f66..18257bf 100644 --- a/system/nginx/server_block.conf +++ b/system/nginx/server_block.conf @@ -53,7 +53,7 @@ server { {% endif %} {% for ppath, pcfg in paths.items() %} -{% if pcfg.is_management and ppath == '/' %} +{% if pcfg.is_management %} # SPA static assets — served from disk, no Flask round-trip. # no-cache: browsers revalidate every load; unchanged files are 304s. location /static/ { diff --git a/tests/test_acme.py b/tests/test_acme.py index 56b3457..f4a7979 100644 --- a/tests/test_acme.py +++ b/tests/test_acme.py @@ -277,3 +277,35 @@ class TestHasAutoRenew: with patch.object(acme, "_ACME_HOME", acme_dir): result = acme._has_auto_renew("nonexistent.com") assert result is False + + +class TestSummarizeAcmeOutput: + def test_last_two_lines(self): + out = ( + "[2026-09-04] line one\n[2026-09-04] retry failed\n[2026-09-04] giving up\n" + ) + assert acme._summarize_acme_output(out) == "retry failed; giving up" + + def test_strips_timestamps_and_log_pointer(self): + out = "[ts] work\nPlease check log file /x/acme.sh.log\n[ts] done\n" + assert acme._summarize_acme_output(out) == "work; done" + + def test_empty_returns_placeholder(self): + assert acme._summarize_acme_output("") == "(no output)" + + def test_preserves_permission_denied_outside_tail(self): + out = ( + "[ts] starting\n" + "[ts] /data/acme/account.conf: Permission denied\n" + "[ts] step three\n" + "[ts] step four\n" + ) + summary = acme._summarize_acme_output(out) + # The permission line is not among the final two, but the + # actionable-error matcher (daemon/collectors/acme.py) keys off it. + assert "account.conf: Permission denied" in summary + assert summary.count("; ") == 2 # capped at three lines + + def test_permission_denied_in_tail_not_duplicated(self): + out = "[ts] ok\n[ts] account.conf: Permission denied\n" + assert acme._summarize_acme_output(out) == "ok; account.conf: Permission denied" diff --git a/tests/test_nginx.py b/tests/test_nginx.py index e602476..23b7323 100644 --- a/tests/test_nginx.py +++ b/tests/test_nginx.py @@ -349,6 +349,26 @@ class TestGenerateServerConf: out = nginx.generate_server_conf(cfg) assert "location /static/" not in out + def test_management_static_location_on_subpath(self, temp_data_dir): + # The SPA references /static/... at the domain root regardless of the + # management backend path, so the block is emitted for any + # is_management path, not only '/'. + cfg = { + "domain": "mgmt.example.com", + "paths": { + "/app": { + "backend": {"host": "127.0.0.1", "port": 9090, "proto": "http"}, + "is_management": True, + }, + }, + "force_ssl": True, + "cert": "acme", + } + out = nginx.generate_server_conf(cfg) + assert "location /static/ {" in out + static_root = str(nginx.PROJECT_DIR / "webui" / "static") + assert f"alias {static_root}/;" in out + def test_websocket_path(self, temp_data_dir): cfg = { "domain": "mgmt.example.com", diff --git a/tests/test_state.py b/tests/test_state.py index e2f1816..1d57009 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -1,6 +1,7 @@ """Tests for lib/state.py — state store and collect functions.""" import json +import os from unittest.mock import patch import daemon.collectors.acme @@ -267,6 +268,9 @@ class TestAcmeCollectNonFatal: patch.object( daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" ), + patch( + "daemon.collectors.acme._acme_home_needs_normalize", return_value=True + ), patch("daemon.handlers.acme.normalize_acme_home"), patch( "lib.acme.list_certs", @@ -290,6 +294,9 @@ class TestAcmeCollectNonFatal: patch.object( daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" ), + patch( + "daemon.collectors.acme._acme_home_needs_normalize", return_value=True + ), patch("daemon.handlers.acme.normalize_acme_home"), patch("lib.acme.list_certs", return_value=[]), patch.object( @@ -316,6 +323,9 @@ class TestAcmeCollectNonFatal: patch.object( daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" ), + patch( + "daemon.collectors.acme._acme_home_needs_normalize", return_value=True + ), patch("daemon.handlers.acme.normalize_acme_home", side_effect=_norm), patch("lib.acme.list_certs", side_effect=_list), patch.object( @@ -324,12 +334,78 @@ class TestAcmeCollectNonFatal: ): result = _collect_acme() - # The poll must normalize ACME_HOME perms before listing, so a - # mid-lifetime ownership flip self-heals without a restart. + # The poll must normalize ACME_HOME perms before listing when the + # probe detects a lost group-read bit, so a mid-lifetime ownership + # flip self-heals without a restart. assert order == ["normalize", "list"] assert result["certs"] == [{"domain": "example.com"}] assert result["status"] == {"error": None} + def test_no_normalize_when_probe_clean(self): + from daemon.collectors.acme import _collect_acme + + order: list[str] = [] + + def _norm(): + order.append("normalize") + + def _list(): + order.append("list") + return [] + + with ( + patch.object( + daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" + ), + patch( + "daemon.collectors.acme._acme_home_needs_normalize", return_value=False + ), + patch("daemon.handlers.acme.normalize_acme_home", side_effect=_norm), + patch("lib.acme.list_certs", side_effect=_list), + patch.object( + daemon.collectors.acme, "_parse_account_conf", return_value=_ACCOUNT + ), + ): + result = _collect_acme() + + # Steady state: the probe sees group-read bits intact, so the poll + # must not pay for a sudo normalize. + assert order == ["list"] + assert result["certs"] == [] + assert result["status"] == {"error": None} + + +class TestAcmeHomeProbe: + """_acme_home_needs_normalize probes the group-read bit without sudo.""" + + def test_flags_file_without_group_read(self, tmp_path, monkeypatch): + from daemon.collectors.acme import _acme_home_needs_normalize + + monkeypatch.setenv("ACME_HOME", str(tmp_path)) + (tmp_path / "account.conf").write_text("x") + os.chmod(tmp_path / "account.conf", 0o600) + assert _acme_home_needs_normalize() is True + + def test_clean_when_group_read_set(self, tmp_path, monkeypatch): + from daemon.collectors.acme import _acme_home_needs_normalize + + monkeypatch.setenv("ACME_HOME", str(tmp_path)) + (tmp_path / "account.conf").write_text("x") + os.chmod(tmp_path / "account.conf", 0o640) + assert _acme_home_needs_normalize() is False + + def test_clean_on_empty_home(self, tmp_path, monkeypatch): + from daemon.collectors.acme import _acme_home_needs_normalize + + monkeypatch.setenv("ACME_HOME", str(tmp_path)) + assert _acme_home_needs_normalize() is False + + def test_clean_on_missing_home(self, tmp_path, monkeypatch): + from daemon.collectors.acme import _acme_home_needs_normalize + + monkeypatch.setenv("ACME_HOME", str(tmp_path / "does-not-exist")) + assert _acme_home_needs_normalize() is False + def test_permission_error_is_actionable(self): from daemon.collectors.acme import _collect_acme