From 6476695d2984065f25d69731983d081e31cb855e Mon Sep 17 00:00:00 2001 From: Mike Teehan Date: Fri, 4 Sep 2026 21:38:55 +0000 Subject: [PATCH] fix: ACME cert list self-heals when account.conf is left owner-only The startup normalize and _run_acme_preflight covered daemon startup and issue/renew, but the recurring collector poll called lib.acme.list_certs() without normalizing ACME_HOME. A non-daemon run (e.g. a manual run as the WebUI user) re-creating account.conf owner-only made every acme.sh --list exit 2, so the collector returned certs=[] and the UI showed no certs until the next issue/renew or daemon restart. - collector: normalize_acme_home() before list_certs() so the poll self-heals - issue pre-check: normalize before the direct lib.acme.list_certs() - _parse_account_conf: read acme.sh v3 account.conf (not just .account.conf) - _collect_acme: actionable status.error for the account.conf perm case - install.sh: chown ACME_HOME conf files to the daemon user --- daemon/collectors/acme.py | 45 ++++++++++++++++++-- daemon/handlers/acme.py | 5 ++- scripts/install.sh | 10 +++++ tests/test_state.py | 90 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 5 deletions(-) diff --git a/daemon/collectors/acme.py b/daemon/collectors/acme.py index 726b8f9..8599e4b 100644 --- a/daemon/collectors/acme.py +++ b/daemon/collectors/acme.py @@ -66,9 +66,17 @@ def _parse_account_conf(acme_home: Path | None = None) -> dict[str, Any]: "key_length": None, } - # 1. Legacy .account.conf (acme.sh v2.x) - account_path = acme_home / ".account.conf" - if account_path.is_file(): + # 1. acme.sh account file. Modern acme.sh (v3.x) writes ``account.conf``; + # older v2.x wrote ``.account.conf``. Check both so the account card + # reflects the real acme.sh account rather than only the declarative + # fallback below. + account_path = None + for name in ("account.conf", ".account.conf"): + candidate = acme_home / name + if candidate.is_file(): + account_path = candidate + break + if account_path is not None: try: text = account_path.read_text() except OSError: @@ -129,6 +137,26 @@ def _get_acme_email() -> str: return _read_acme_email() +def _friendly_acme_error(exc: Exception) -> str: + """Turn a collection exception into an actionable message. + + The collector already self-heals by normalizing ACME_HOME permissions + first, so the one remaining permission case is when that normalize could + not run (e.g. the sudo step was denied). For that case surface a concrete + remediation instead of the raw acme.sh exit-2 text; otherwise return the + original message unchanged. + """ + text = str(exc) + if "account.conf" in text and "Permission denied" in text: + return ( + f"{text} — account.conf is not readable by the daemon; repair it " + "with: sudo chown : /account.conf " + "&& sudo chmod 0640 /account.conf, then restart " + "vacuum-walld" + ) + return text + + def _collect_acme() -> schema.AcmeState: """Collect ACME certificate list and email. @@ -143,13 +171,22 @@ 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. + from daemon.handlers.acme import normalize_acme_home from lib.acme import list_certs + normalize_acme_home() certs = list_certs() except Exception as exc: logger.warning("ACME state collection failed", exc_info=True) certs = [] - cert_error = str(exc) + cert_error = _friendly_acme_error(exc) account = _parse_account_conf() diff --git a/daemon/handlers/acme.py b/daemon/handlers/acme.py index d6abfae..7f54977 100644 --- a/daemon/handlers/acme.py +++ b/daemon/handlers/acme.py @@ -807,8 +807,11 @@ async def issue_cert(_request: Any, body: dict[str, Any] | None) -> dict[str, An "status": "existing", } - # Check if cert already exists — call acme.sh directly, not via state + # Check if cert already exists — call acme.sh directly, not via state. + # Normalize ACME_HOME first (same reason as the preflight): a prior run by + # another user can leave account.conf owner-only and make `--list` exit 2. try: + normalize_acme_home() certs = lib.acme.list_certs() except RuntimeError as exc: raise RuntimeError(f"Cannot check existing certificates: {exc}") from exc diff --git a/scripts/install.sh b/scripts/install.sh index 7476bd7..cbfd1e1 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -231,6 +231,16 @@ mkdir -p "$ACME_HOME/deploy" cp "${PROJECT_DIR}/system/acme-deploy.sh" "$ACME_HOME/deploy/acme-deploy.sh" chown "$USER_DAEMON_NAME:$USER_GROUP" "$ACME_HOME/deploy/acme-deploy.sh" chmod 0755 "$ACME_HOME/deploy/acme-deploy.sh" +# Ensure the daemon user owns acme.sh's runtime conf files (account.conf and +# any per-domain .conf). acme.sh hardens these owner-only (600); if a +# non-daemon user ever (re)creates them the daemon cannot source account.conf +# and every acme.sh call exits 2. The daemon self-heals on the next run, but +# fixing ownership here avoids the initial broken window on fresh installs. +if [ -d "$ACME_HOME" ]; then + find "$ACME_HOME" -maxdepth 1 -type f -name '*.conf' \ + -exec chown "$USER_DAEMON_NAME:$USER_GROUP" {} + 2>/dev/null || true + [ -f "$ACME_HOME/account.conf" ] && chmod 0640 "$ACME_HOME/account.conf" +fi # --- 3. Setup directories --- log "Creating config and data directories..." diff --git a/tests/test_state.py b/tests/test_state.py index 1e552e9..e2f1816 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -267,6 +267,7 @@ class TestAcmeCollectNonFatal: patch.object( daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" ), + patch("daemon.handlers.acme.normalize_acme_home"), patch( "lib.acme.list_certs", side_effect=RuntimeError("acme.sh failed with exit code 2"), @@ -289,6 +290,7 @@ class TestAcmeCollectNonFatal: patch.object( daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" ), + patch("daemon.handlers.acme.normalize_acme_home"), patch("lib.acme.list_certs", return_value=[]), patch.object( daemon.collectors.acme, "_parse_account_conf", return_value=_ACCOUNT @@ -298,6 +300,94 @@ class TestAcmeCollectNonFatal: assert result["status"] == {"error": None} + def test_self_heal_normalizes_before_list(self): + from daemon.collectors.acme import _collect_acme + + order: list[str] = [] + + def _norm(): + order.append("normalize") + + def _list(): + order.append("list") + return [{"domain": "example.com"}] + + with ( + patch.object( + daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" + ), + 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() + + # The poll must normalize ACME_HOME perms before listing, 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_permission_error_is_actionable(self): + from daemon.collectors.acme import _collect_acme + + msg = "acme.sh failed with exit code 2: .../account.conf: Permission denied" + with ( + patch.object( + daemon.collectors.acme, "_get_acme_email", return_value="a@b.c" + ), + patch("daemon.handlers.acme.normalize_acme_home"), + patch("lib.acme.list_certs", side_effect=RuntimeError(msg)), + patch.object( + daemon.collectors.acme, "_parse_account_conf", return_value=_ACCOUNT + ), + ): + result = _collect_acme() + + assert result["status"]["error"] is not None + assert "sudo chown" in result["status"]["error"] + + +class TestParseAccountConf: + """_parse_account_conf reads acme.sh v3's account.conf (no leading dot).""" + + def test_reads_no_dot_account_conf(self, tmp_path): + from daemon.collectors.acme import _parse_account_conf + + (tmp_path / "account.conf").write_text( + "ACME_LEEMAIL='me@example.com'\nACME_MCA='zerossl'\nACME_CERTKEYSIZE=256\n" + ) + acct = _parse_account_conf(acme_home=tmp_path) + assert acct["registered"] is True + assert acct["email"] == "me@example.com" + assert acct["ca"] == "ZeroSSL" + assert acct["key_length"] == 256 + + def test_prefers_no_dot_over_legacy_dot(self, tmp_path): + from daemon.collectors.acme import _parse_account_conf + + (tmp_path / "account.conf").write_text( + "ACME_LEEMAIL='new@example.com'\nACME_MCA='letsencrypt'\n" + ) + (tmp_path / ".account.conf").write_text( + "ACME_LEEMAIL='old@example.com'\nACME_MCA='zerossl'\n" + ) + acct = _parse_account_conf(acme_home=tmp_path) + assert acct["email"] == "new@example.com" + + def test_falls_back_to_legacy_dot(self, tmp_path): + from daemon.collectors.acme import _parse_account_conf + + (tmp_path / ".account.conf").write_text( + "ACME_LEEMAIL='legacy@example.com'\nACME_MCA='zerossl'\n" + ) + acct = _parse_account_conf(acme_home=tmp_path) + assert acct["registered"] is True + assert acct["email"] == "legacy@example.com" + assert acct["ca"] == "ZeroSSL" + class TestStateVersions: def test_version_starts_at_zero(self):