mirror of
https://github.com/Joulenap/joulenap.git
synced 2026-08-11 13:21:43 +02:00
fix: backend robustness + config secret-file permissions
- BE-B3: open SQLite with WAL + busy_timeout + foreign_keys via a connect listener, so a running cycle's frequent commits don't risk "database is locked" against dashboard polling, and the CASCADE FKs actually enforce. - BE-S2: write config.yaml owner-only (0600) — it holds API tokens, the session key and notification secrets, so it must not be world-readable on the host/bind mount (matches the SSH key). Best-effort; re-asserted on save. - BE-B5: hold the single-run lock across a manual power-off (JobService .exclusive()) so a scheduled cycle can't start in the check-then-act gap and get its PBS shut down mid-backup. - BE-B6: if the worker thread fails to start, fail the run and release the single-run lock instead of leaking it (which would 409 every later run). - BE-B7: re-arm the daily history-prune job on rearm so a runtime timezone change moves it into the new zone instead of the boot-time zone.
This commit is contained in:
@@ -12,6 +12,7 @@ from pydantic import BaseModel
|
||||
|
||||
from ..connectors.errors import ConnectorError
|
||||
from ..core.config_store import ConfigStore
|
||||
from ..jobs import AlreadyRunningError
|
||||
from .deps import JobService, get_config_store, get_job_service, require_auth
|
||||
|
||||
router = APIRouter(prefix="/power", dependencies=[Depends(require_auth)], tags=["power"])
|
||||
@@ -43,13 +44,16 @@ def power_off(
|
||||
store: ConfigStore = Depends(get_config_store),
|
||||
job_service: JobService = Depends(get_job_service),
|
||||
) -> PowerResult:
|
||||
if job_service.is_running:
|
||||
# Hold the single-run lock across the poweroff so a scheduled cycle can't start in the
|
||||
# gap between "is a run active?" and the SSH poweroff and get its PBS shut down mid-backup.
|
||||
try:
|
||||
with job_service.exclusive():
|
||||
job_service.deps.build_power(store.config).poweroff()
|
||||
except AlreadyRunningError as exc:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_409_CONFLICT,
|
||||
detail="A backup or GC run is in progress; cannot power off the PBS",
|
||||
)
|
||||
try:
|
||||
job_service.deps.build_power(store.config).poweroff()
|
||||
) from exc
|
||||
except ConnectorError as exc:
|
||||
raise HTTPException(status_code=status.HTTP_502_BAD_GATEWAY, detail=str(exc)) from exc
|
||||
return PowerResult(ok=True)
|
||||
|
||||
+21
-3
@@ -243,13 +243,25 @@ def load_config(path: Path | None = None) -> Config:
|
||||
return Config.model_validate(raw)
|
||||
|
||||
|
||||
def restrict_secret_file(path: Path) -> None:
|
||||
"""Best-effort ``chmod 0600`` so config.yaml's plaintext secrets (API tokens, secret_key,
|
||||
SMTP/bot passwords) aren't world-readable — matching the SSH key's perms. Silently ignored
|
||||
where it isn't meaningful: a foreign-owned/exotic mount, or a filesystem (Windows/NTFS)
|
||||
without POSIX permission bits."""
|
||||
try:
|
||||
os.chmod(path, 0o600)
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
||||
def save_config(cfg: Config, path: Path | None = None) -> None:
|
||||
"""Write the full config (real secrets) back to disk.
|
||||
|
||||
Prefers an atomic temp-file + ``os.replace`` so a crash mid-write can't truncate the
|
||||
live config. When the target is a single-file Docker bind mount (``config.yaml`` mapped
|
||||
in directly), the rename can't replace the mount point (EBUSY) — and a cross-device tmp
|
||||
can't be renamed (EXDEV) — so we fall back to an in-place overwrite. Raises a clear error
|
||||
can't be renamed (EXDEV) — so we fall back to an in-place overwrite. The file is written
|
||||
owner-only (0600) so the plaintext secrets aren't world-readable. Raises a clear error
|
||||
if the file isn't writable (e.g. mounted read-only).
|
||||
"""
|
||||
p = path or paths.config_path()
|
||||
@@ -257,7 +269,10 @@ def save_config(cfg: Config, path: Path | None = None) -> None:
|
||||
text = yaml.safe_dump(data, sort_keys=False, allow_unicode=True, default_flow_style=False)
|
||||
tmp = p.with_suffix(p.suffix + ".tmp")
|
||||
try:
|
||||
with tmp.open("w", encoding="utf-8") as fh:
|
||||
# Create the temp with owner-only perms up front so the plaintext secrets never sit in
|
||||
# a world-readable file; os.replace then carries those perms onto config.yaml.
|
||||
fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as fh:
|
||||
fh.write(text)
|
||||
try:
|
||||
os.replace(tmp, p)
|
||||
@@ -267,8 +282,11 @@ def save_config(cfg: Config, path: Path | None = None) -> None:
|
||||
# onto a single-file bind mount, which is how the compose example maps config.yaml.
|
||||
if exc.errno not in (errno.EBUSY, errno.EXDEV):
|
||||
raise
|
||||
with p.open("w", encoding="utf-8") as fh:
|
||||
fd = os.open(p, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as fh:
|
||||
fh.write(text)
|
||||
# O_CREAT doesn't change an already-existing file's mode, so tighten it explicitly.
|
||||
restrict_secret_file(p)
|
||||
tmp.unlink(missing_ok=True)
|
||||
except PermissionError as exc:
|
||||
tmp.unlink(missing_ok=True)
|
||||
|
||||
@@ -14,7 +14,7 @@ from collections.abc import Callable
|
||||
from pathlib import Path
|
||||
|
||||
from .. import paths
|
||||
from ..config import Config, load_config, save_config
|
||||
from ..config import Config, load_config, restrict_secret_file, save_config
|
||||
|
||||
log = logging.getLogger("joulenap.config")
|
||||
|
||||
@@ -40,6 +40,7 @@ class ConfigStore:
|
||||
example = paths.config_example_path()
|
||||
log.warning("config.yaml not found at %s — creating from %s", target, example)
|
||||
shutil.copyfile(example, target)
|
||||
restrict_secret_file(target) # owner-only from the start (secrets land here later)
|
||||
|
||||
config = load_config(target)
|
||||
store = cls(config, target)
|
||||
|
||||
@@ -126,15 +126,18 @@ class Scheduler:
|
||||
self._scheduler.shutdown(wait=False)
|
||||
|
||||
def rearm(self, config: Config) -> None:
|
||||
"""(Re)build the config-driven cron jobs (backup + scheduled verify). Each existing
|
||||
job is removed first so a disabled/empty schedule leaves nothing armed. The prune
|
||||
housekeeping job (armed separately) is left untouched."""
|
||||
"""(Re)build the config-driven cron jobs (backup + scheduled verify) and re-arm the
|
||||
prune housekeeping job so its timezone stays in sync. Each config-driven job is
|
||||
removed first so a disabled/empty schedule leaves nothing armed."""
|
||||
# Re-resolve app.timezone here so a timezone changed at runtime (e.g. saved via
|
||||
# Settings, which calls rearm) actually takes effect. Without this the zone is fixed
|
||||
# at construction and a new schedule would stay in the old zone until restart.
|
||||
self._timezone = resolve_timezone(config.app.timezone)
|
||||
self._rearm_backup(config)
|
||||
self._rearm_verify(config)
|
||||
# Prune isn't config-driven, but its trigger carries a timezone too, so re-arm it in
|
||||
# the (possibly new) zone — otherwise it keeps firing in the boot-time zone (BE-B7).
|
||||
self.arm_prune()
|
||||
|
||||
def _rearm_backup(self, config: Config) -> None:
|
||||
if self._scheduler.get_job(BACKUP_JOB_ID):
|
||||
@@ -204,9 +207,10 @@ class Scheduler:
|
||||
log.exception("Scheduled verify run failed to start")
|
||||
|
||||
def arm_prune(self) -> None:
|
||||
"""Arm the daily history-prune job. No-op when no prune callback was provided.
|
||||
Independent of backup config, so it survives ``rearm`` and runs even when backups
|
||||
are disabled."""
|
||||
"""Arm (or, via ``replace_existing``, re-arm) the daily history-prune job in the
|
||||
current timezone. No-op when no prune callback was provided. Independent of backup
|
||||
config, so it runs even when backups are disabled; ``rearm`` calls it to keep the
|
||||
prune trigger's timezone in sync."""
|
||||
if self._run_prune is None:
|
||||
return
|
||||
self._scheduler.add_job(
|
||||
|
||||
+20
-2
@@ -11,7 +11,7 @@ from collections.abc import Iterator
|
||||
from contextlib import contextmanager
|
||||
from pathlib import Path
|
||||
|
||||
from sqlalchemy import create_engine
|
||||
from sqlalchemy import create_engine, event
|
||||
from sqlalchemy.orm import DeclarativeBase, Session, sessionmaker
|
||||
|
||||
from .. import paths
|
||||
@@ -26,12 +26,30 @@ class Base(DeclarativeBase):
|
||||
|
||||
def _make_engine(db_file: Path):
|
||||
# check_same_thread=False: FastAPI may touch a session from a threadpool worker.
|
||||
return create_engine(
|
||||
engine = create_engine(
|
||||
f"sqlite:///{db_file.as_posix()}",
|
||||
future=True,
|
||||
connect_args={"check_same_thread": False},
|
||||
)
|
||||
|
||||
@event.listens_for(engine, "connect")
|
||||
def _set_sqlite_pragmas(dbapi_conn, _record):
|
||||
# A running backup cycle commits after every step/log line while the dashboard
|
||||
# polls (and writes datastore usage on GET), so concurrent access is the norm:
|
||||
# - WAL lets those readers keep going while the cycle writes, instead of
|
||||
# blocking each other (the default rollback journal serialises them).
|
||||
# - busy_timeout makes a genuinely contended write WAIT rather than fail
|
||||
# instantly with "database is locked".
|
||||
# - foreign_keys makes the ondelete=CASCADE relationships actually enforce
|
||||
# (SQLite leaves FK enforcement off by default).
|
||||
cur = dbapi_conn.cursor()
|
||||
cur.execute("PRAGMA journal_mode=WAL")
|
||||
cur.execute("PRAGMA busy_timeout=5000")
|
||||
cur.execute("PRAGMA foreign_keys=ON")
|
||||
cur.close()
|
||||
|
||||
return engine
|
||||
|
||||
|
||||
def init_db(db_file: Path | None = None) -> None:
|
||||
"""Create the engine, session factory and tables. Idempotent; call at startup."""
|
||||
|
||||
@@ -9,10 +9,12 @@ from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import threading
|
||||
from collections.abc import Iterator
|
||||
from contextlib import contextmanager
|
||||
|
||||
from ..core.config_store import ConfigStore
|
||||
from ..db import session_scope
|
||||
from ..db.models import RunKind, RunTrigger
|
||||
from ..db.models import RunKind, RunStatus, RunTrigger
|
||||
from ..db.prune import PruneResult, prune_history
|
||||
from .backup_cycle import run_backup_cycle, run_gc_cycle, run_verify_cycle
|
||||
from .deps import CycleDeps
|
||||
@@ -35,6 +37,18 @@ class JobService:
|
||||
def is_running(self) -> bool:
|
||||
return self._lock.locked()
|
||||
|
||||
@contextmanager
|
||||
def exclusive(self) -> Iterator[None]:
|
||||
"""Hold the single-run lock for a non-job operation (e.g. a manual power-off) so it
|
||||
can't race a run *starting* in the gap of a check-then-act. Raises AlreadyRunningError
|
||||
if a run already holds the lock; releases it when the block exits."""
|
||||
if not self._lock.acquire(blocking=False):
|
||||
raise AlreadyRunningError("A backup or GC run is already in progress")
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
self._lock.release()
|
||||
|
||||
# --- blocking entry points (internal / tests) ----------------------------
|
||||
|
||||
def run_backup(
|
||||
@@ -131,5 +145,16 @@ class JobService:
|
||||
finally:
|
||||
self._lock.release()
|
||||
|
||||
threading.Thread(target=worker, name=f"joulenap-{kind.value}", daemon=True).start()
|
||||
try:
|
||||
threading.Thread(target=worker, name=f"joulenap-{kind.value}", daemon=True).start()
|
||||
except BaseException:
|
||||
# The worker (and its lock-release + recorder finalisation) never runs, so do it
|
||||
# here — otherwise the run is stuck RUNNING and the single-run lock is held forever,
|
||||
# 409-ing every later run until restart (BE-B6).
|
||||
try:
|
||||
recorder.finish(RunStatus.FAILURE, error="worker thread failed to start")
|
||||
finally:
|
||||
recorder.close()
|
||||
self._lock.release()
|
||||
raise
|
||||
return run_id
|
||||
|
||||
@@ -415,7 +415,9 @@ def test_power_off_conflict_when_busy(app_ctx):
|
||||
client, app = app_ctx
|
||||
|
||||
class _Busy:
|
||||
is_running = True
|
||||
def exclusive(self):
|
||||
# A run holds the lock: entering the guard raises, mapping to 409.
|
||||
raise AlreadyRunningError("A backup or GC run is already in progress")
|
||||
|
||||
app.state.job_service = _Busy()
|
||||
assert client.post("/api/power/off").status_code == 409
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
@@ -77,6 +78,31 @@ def test_save_falls_back_to_in_place_when_rename_is_busy(tmp_path: Path, monkeyp
|
||||
assert not (tmp_path / "config.yaml.tmp").exists() # temp cleaned up
|
||||
|
||||
|
||||
@pytest.mark.skipif(os.name != "posix", reason="POSIX file modes only (Windows/NTFS has no 0600)")
|
||||
def test_save_config_is_owner_only(tmp_path: Path, monkeypatch):
|
||||
# config.yaml holds plaintext secrets (tokens, secret_key, passwords), so it must not be
|
||||
# world-readable (BE-S2) — mirrors the SSH key's 0600.
|
||||
import errno
|
||||
import stat
|
||||
|
||||
cfg = load_config(EXAMPLE)
|
||||
out = tmp_path / "config.yaml"
|
||||
|
||||
# Atomic path (temp + rename): the fresh file lands owner-only.
|
||||
save_config(cfg, out)
|
||||
assert stat.S_IMODE(os.stat(out).st_mode) == 0o600
|
||||
|
||||
# In-place fallback (bind-mount EBUSY) re-tightens even a loosened existing file.
|
||||
os.chmod(out, 0o644)
|
||||
|
||||
def busy_replace(_src, _dst):
|
||||
raise OSError(errno.EBUSY, "Device or resource busy")
|
||||
|
||||
monkeypatch.setattr(os, "replace", busy_replace)
|
||||
save_config(cfg, out)
|
||||
assert stat.S_IMODE(os.stat(out).st_mode) == 0o600
|
||||
|
||||
|
||||
def test_redaction_masks_secrets_keeps_empty():
|
||||
cfg = load_config(EXAMPLE)
|
||||
cfg.app.secret_key = "supersecret"
|
||||
|
||||
@@ -2,12 +2,21 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy import select, text
|
||||
|
||||
from app.db import session_scope
|
||||
from app.db.models import LogEvent, LogLevel, Run, RunKind, RunStatus, RunTrigger
|
||||
|
||||
|
||||
def test_sqlite_pragmas_applied(temp_db):
|
||||
# WAL + busy_timeout + foreign_keys keep concurrent access safe while a running cycle
|
||||
# commits and the dashboard polls (BE-B3). Assert they're set on a fresh connection.
|
||||
with session_scope() as s:
|
||||
assert s.execute(text("PRAGMA journal_mode")).scalar() == "wal"
|
||||
assert s.execute(text("PRAGMA foreign_keys")).scalar() == 1
|
||||
assert s.execute(text("PRAGMA busy_timeout")).scalar() == 5000
|
||||
|
||||
|
||||
def test_create_run_with_logs(temp_db):
|
||||
with session_scope() as s:
|
||||
run = Run(kind=RunKind.BACKUP, trigger=RunTrigger.MANUAL)
|
||||
|
||||
@@ -116,6 +116,26 @@ def test_rearm_preserves_prune_job():
|
||||
assert {j.id for j in sched._scheduler.get_jobs()} == {PRUNE_JOB_ID}
|
||||
|
||||
|
||||
def test_rearm_updates_prune_job_timezone():
|
||||
# A runtime timezone change (via rearm) must move the prune job into the new zone, not
|
||||
# leave it firing in the boot-time zone (BE-B7). Started so replace_existing dedups the
|
||||
# prune job (as it does in production, where rearm always runs on a started scheduler).
|
||||
sched = Scheduler(lambda _trigger: None, run_prune=lambda: None, timezone="UTC")
|
||||
sched.start()
|
||||
try:
|
||||
sched.arm_prune()
|
||||
assert sched.prune_job is not None
|
||||
assert str(sched.prune_job.trigger.timezone) == "UTC"
|
||||
|
||||
cfg = _config()
|
||||
cfg.app.timezone = "Europe/Rome"
|
||||
sched.rearm(cfg)
|
||||
assert sched.prune_job is not None
|
||||
assert str(sched.prune_job.trigger.timezone) == "Europe/Rome"
|
||||
finally:
|
||||
sched.shutdown()
|
||||
|
||||
|
||||
def test_fire_prune_invokes_callback():
|
||||
calls: list[int] = []
|
||||
sched = Scheduler(lambda _trigger: None, run_prune=lambda: calls.append(1))
|
||||
|
||||
@@ -6,6 +6,7 @@ import threading
|
||||
|
||||
import pytest
|
||||
from fakes import make_deps
|
||||
from sqlalchemy import select
|
||||
|
||||
from app.core.config_store import ConfigStore
|
||||
from app.db import session_scope
|
||||
@@ -63,3 +64,58 @@ def test_overlapping_run_is_rejected(temp_config, temp_db):
|
||||
worker.join(timeout=5)
|
||||
|
||||
assert service.is_running is False
|
||||
|
||||
|
||||
def test_exclusive_blocks_while_a_run_holds_the_lock(temp_config, temp_db):
|
||||
# exclusive() (used by manual power-off) must not enter while a run holds the lock, so a
|
||||
# poweroff can't race a starting cycle (BE-B5).
|
||||
started = threading.Event()
|
||||
release = threading.Event()
|
||||
|
||||
def blocking_wol(_config):
|
||||
started.set()
|
||||
assert release.wait(timeout=5)
|
||||
|
||||
deps, _pve, _pbs, _power = make_deps(wol=blocking_wol)
|
||||
service = JobService(ConfigStore.load_or_create(), deps=deps)
|
||||
|
||||
worker = threading.Thread(target=service.run_backup)
|
||||
worker.start()
|
||||
try:
|
||||
assert started.wait(timeout=5)
|
||||
with pytest.raises(AlreadyRunningError), service.exclusive():
|
||||
pass # pragma: no cover - the guard raises before the body runs
|
||||
finally:
|
||||
release.set()
|
||||
worker.join(timeout=5)
|
||||
|
||||
# Lock free again -> exclusive() yields, and releases on exit.
|
||||
with service.exclusive():
|
||||
assert service.is_running is True
|
||||
assert service.is_running is False
|
||||
|
||||
|
||||
def test_submit_releases_lock_if_thread_fails_to_start(temp_config, temp_db, monkeypatch):
|
||||
# If Thread.start() raises (e.g. thread/memory exhaustion), the worker's finally never
|
||||
# runs, so _submit must release the lock and fail the run itself (BE-B6) — otherwise every
|
||||
# later run 409s forever and the run is stuck RUNNING.
|
||||
deps, _pve, _pbs, _power = make_deps()
|
||||
service = JobService(ConfigStore.load_or_create(), deps=deps)
|
||||
|
||||
class _BadThread:
|
||||
def __init__(self, *args, **kwargs):
|
||||
pass
|
||||
|
||||
def start(self):
|
||||
raise RuntimeError("can't start new thread")
|
||||
|
||||
monkeypatch.setattr("app.jobs.service.threading.Thread", _BadThread)
|
||||
|
||||
with pytest.raises(RuntimeError):
|
||||
service.submit_backup()
|
||||
|
||||
assert service.is_running is False # lock released, not leaked
|
||||
with session_scope() as s:
|
||||
run = s.scalars(select(Run)).one()
|
||||
assert run.status == RunStatus.FAILURE
|
||||
assert run.finished_at is not None
|
||||
|
||||
Reference in New Issue
Block a user