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
This commit is contained in:
@@ -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 <daemon-user>:<group> <ACME_HOME>/account.conf "
|
||||
"&& sudo chmod 0640 <ACME_HOME>/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()
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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..."
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user