fix(furtka): audit follow-ups — placeholder secrets, isolate reconcile, .env perms
Addresses the four issues raised in the slice-3 audit before pushing. #1 (critical) — refuse to finish install when .env still contains placeholder secrets like "changeme". Without this, `furtka app install fileshare` would happily start an SMB server with a publicly-known password — the kind of default that ends up screenshotted on Hacker News. PLACEHOLDER_SECRETS lives in installer.py; new tests cover placeholder rejection, post-edit retry, and quoted values. #3 — reconciler now catches DockerError / FileNotFoundError / OSError per-app instead of letting a single broken app abort the whole boot-scan. Errors get surfaced as Action(kind="error", …) and has_errors() drives the CLI exit code so systemd still shows red, but the other apps actually got reconciled. #4 — chmod 0600 on .env after install so app secrets aren't world- readable on multi-user boxes. Done before the placeholder check so even the half-installed state is safe. #5 — load_manifest() got an optional expected_name. The scanner passes the folder name (filesystem source-of-truth contract); installer leaves it None so `furtka app install /tmp/some-fork/` works regardless of what the source folder is named. #2 — TODO comment on dperson/samba:latest. Switching to a digest needs a verified upstream release; left for the test-day pin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
9f4e514d8a
commit
ff68dd5ae6
9 changed files with 216 additions and 19 deletions
|
|
@ -4,6 +4,13 @@
|
||||||
# from the manifest's "volumes" list before this compose file is brought up;
|
# from the manifest's "volumes" list before this compose file is brought up;
|
||||||
# it's referenced as `external: true` here so docker compose doesn't try
|
# it's referenced as `external: true` here so docker compose doesn't try
|
||||||
# to manage its lifecycle.
|
# to manage its lifecycle.
|
||||||
|
#
|
||||||
|
# TODO(image-pin): `:latest` is shaky for production — pin to a digest
|
||||||
|
# (`dperson/samba@sha256:...`) or a stable tag once we've verified one
|
||||||
|
# against the upstream registry. For the MVP run we accept the drift
|
||||||
|
# risk to keep the install reproducible against whatever the upstream
|
||||||
|
# image happens to be on test day; revisit before any non-developer
|
||||||
|
# touches this.
|
||||||
|
|
||||||
services:
|
services:
|
||||||
smbd:
|
smbd:
|
||||||
|
|
|
||||||
|
|
@ -55,7 +55,7 @@ def _cmd_app_install(args: argparse.Namespace) -> int:
|
||||||
actions = reconciler.reconcile(apps_dir())
|
actions = reconciler.reconcile(apps_dir())
|
||||||
for a in actions:
|
for a in actions:
|
||||||
print(f" {a.describe()}")
|
print(f" {a.describe()}")
|
||||||
return 0
|
return 1 if reconciler.has_errors(actions) else 0
|
||||||
|
|
||||||
|
|
||||||
def _cmd_app_remove(args: argparse.Namespace) -> int:
|
def _cmd_app_remove(args: argparse.Namespace) -> int:
|
||||||
|
|
@ -84,7 +84,10 @@ def _cmd_reconcile(args: argparse.Namespace) -> int:
|
||||||
print(f" {a.describe()}")
|
print(f" {a.describe()}")
|
||||||
if args.dry_run:
|
if args.dry_run:
|
||||||
print("(dry-run — nothing changed)")
|
print("(dry-run — nothing changed)")
|
||||||
return 0
|
# Exit non-zero on any per-app failure so systemd marks furtka-reconcile
|
||||||
|
# red — but only AFTER all apps were attempted, so a broken app doesn't
|
||||||
|
# hide healthy ones.
|
||||||
|
return 1 if reconciler.has_errors(actions) else 0
|
||||||
|
|
||||||
|
|
||||||
def build_parser() -> argparse.ArgumentParser:
|
def build_parser() -> argparse.ArgumentParser:
|
||||||
|
|
|
||||||
|
|
@ -4,11 +4,32 @@ from pathlib import Path
|
||||||
from furtka.manifest import ManifestError, load_manifest
|
from furtka.manifest import ManifestError, load_manifest
|
||||||
from furtka.paths import apps_dir, bundled_apps_dir
|
from furtka.paths import apps_dir, bundled_apps_dir
|
||||||
|
|
||||||
|
# Values that an app's .env.example may use as obvious "fill me in" markers.
|
||||||
|
# If any of these reach the live .env, install refuses — otherwise we'd ship
|
||||||
|
# an SMB share with password "changeme" out of the box, which is the kind of
|
||||||
|
# default that ends up screenshotted on Hacker News.
|
||||||
|
PLACEHOLDER_SECRETS: frozenset[str] = frozenset({"changeme"})
|
||||||
|
|
||||||
|
|
||||||
class InstallError(RuntimeError):
|
class InstallError(RuntimeError):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
def _placeholder_keys(env_path: Path) -> list[str]:
|
||||||
|
if not env_path.exists():
|
||||||
|
return []
|
||||||
|
bad: list[str] = []
|
||||||
|
for raw in env_path.read_text().splitlines():
|
||||||
|
line = raw.strip()
|
||||||
|
if not line or line.startswith("#") or "=" not in line:
|
||||||
|
continue
|
||||||
|
key, _, value = line.partition("=")
|
||||||
|
value = value.strip().strip('"').strip("'")
|
||||||
|
if value in PLACEHOLDER_SECRETS:
|
||||||
|
bad.append(key.strip())
|
||||||
|
return bad
|
||||||
|
|
||||||
|
|
||||||
def resolve_source(source: str) -> Path:
|
def resolve_source(source: str) -> Path:
|
||||||
"""Resolve a `furtka app install <source>` arg to a real source folder.
|
"""Resolve a `furtka app install <source>` arg to a real source folder.
|
||||||
|
|
||||||
|
|
@ -30,8 +51,11 @@ def install_from(src: Path) -> Path:
|
||||||
"""Copy a validated app folder into /var/lib/furtka/apps/<name>/.
|
"""Copy a validated app folder into /var/lib/furtka/apps/<name>/.
|
||||||
|
|
||||||
Preserves an existing .env on upgrade. Bootstraps .env from .env.example
|
Preserves an existing .env on upgrade. Bootstraps .env from .env.example
|
||||||
on first install if .env wasn't shipped.
|
on first install if .env wasn't shipped. Refuses to finish (raises
|
||||||
Returns the target folder.
|
InstallError) if the resulting .env still has placeholder secrets — the
|
||||||
|
target folder is left in place so the user can edit and re-run.
|
||||||
|
|
||||||
|
Returns the target folder on success.
|
||||||
"""
|
"""
|
||||||
manifest_path = src / "manifest.json"
|
manifest_path = src / "manifest.json"
|
||||||
if not manifest_path.exists():
|
if not manifest_path.exists():
|
||||||
|
|
@ -59,6 +83,19 @@ def install_from(src: Path) -> Path:
|
||||||
if not env.exists() and env_example.exists():
|
if not env.exists() and env_example.exists():
|
||||||
shutil.copy2(env_example, env)
|
shutil.copy2(env_example, env)
|
||||||
|
|
||||||
|
# .env carries app secrets — lock to root-only. Done before the placeholder
|
||||||
|
# check so even the half-installed state is at least not world-readable.
|
||||||
|
if env.exists():
|
||||||
|
env.chmod(0o600)
|
||||||
|
|
||||||
|
bad = _placeholder_keys(env)
|
||||||
|
if bad:
|
||||||
|
raise InstallError(
|
||||||
|
f"{m.name}: {env} still has placeholder values for {', '.join(bad)}. "
|
||||||
|
f"Edit the file (set real values), then re-run "
|
||||||
|
f"`furtka app install {m.name}`."
|
||||||
|
)
|
||||||
|
|
||||||
return target
|
return target
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -35,7 +35,14 @@ class Manifest:
|
||||||
return f"furtka_{self.name}_{short}"
|
return f"furtka_{self.name}_{short}"
|
||||||
|
|
||||||
|
|
||||||
def load_manifest(path: Path) -> Manifest:
|
def load_manifest(path: Path, expected_name: str | None = None) -> Manifest:
|
||||||
|
"""Parse and validate a manifest.json.
|
||||||
|
|
||||||
|
`expected_name` is used by the scanner (where the install location's folder
|
||||||
|
name IS the source of truth and must match the manifest). For loading from
|
||||||
|
arbitrary source folders during install, leave it None — the manifest's own
|
||||||
|
`name` field decides the install target.
|
||||||
|
"""
|
||||||
try:
|
try:
|
||||||
raw = json.loads(path.read_text())
|
raw = json.loads(path.read_text())
|
||||||
except json.JSONDecodeError as e:
|
except json.JSONDecodeError as e:
|
||||||
|
|
@ -50,9 +57,8 @@ def load_manifest(path: Path) -> Manifest:
|
||||||
name = raw["name"]
|
name = raw["name"]
|
||||||
if not isinstance(name, str) or not name:
|
if not isinstance(name, str) or not name:
|
||||||
raise ManifestError(f"{path}: name must be a non-empty string")
|
raise ManifestError(f"{path}: name must be a non-empty string")
|
||||||
expected = path.parent.name
|
if expected_name is not None and name != expected_name:
|
||||||
if name != expected:
|
raise ManifestError(f"{path}: name {name!r} must equal {expected_name!r}")
|
||||||
raise ManifestError(f"{path}: name {name!r} must equal containing folder {expected!r}")
|
|
||||||
|
|
||||||
volumes = raw["volumes"]
|
volumes = raw["volumes"]
|
||||||
if not isinstance(volumes, list) or not all(isinstance(v, str) and v for v in volumes):
|
if not isinstance(volumes, list) or not all(isinstance(v, str) and v for v in volumes):
|
||||||
|
|
|
||||||
|
|
@ -18,18 +18,35 @@ class Action:
|
||||||
|
|
||||||
|
|
||||||
def reconcile(apps_root: Path, dry_run: bool = False) -> list[Action]:
|
def reconcile(apps_root: Path, dry_run: bool = False) -> list[Action]:
|
||||||
|
"""Walk the apps tree and bring docker into the desired state.
|
||||||
|
|
||||||
|
Failures during one app's reconcile (Docker errors, missing binary, …) are
|
||||||
|
captured as Action(kind='error', …) and do NOT abort the whole sweep — the
|
||||||
|
other apps still get reconciled. Callers inspect the returned actions to
|
||||||
|
decide overall success.
|
||||||
|
"""
|
||||||
actions: list[Action] = []
|
actions: list[Action] = []
|
||||||
for result in scan(apps_root):
|
for result in scan(apps_root):
|
||||||
if not result.ok:
|
if not result.ok:
|
||||||
actions.append(Action("skip", result.path.name, result.error or ""))
|
actions.append(Action("skip", result.path.name, result.error or ""))
|
||||||
continue
|
continue
|
||||||
m = result.manifest
|
m = result.manifest
|
||||||
for vol_short in m.volumes:
|
try:
|
||||||
full = m.volume_name(vol_short)
|
for vol_short in m.volumes:
|
||||||
actions.append(Action("ensure_volume", full))
|
full = m.volume_name(vol_short)
|
||||||
|
actions.append(Action("ensure_volume", full))
|
||||||
|
if not dry_run:
|
||||||
|
dockerops.ensure_volume(full)
|
||||||
|
actions.append(Action("compose_up", m.name))
|
||||||
if not dry_run:
|
if not dry_run:
|
||||||
dockerops.ensure_volume(full)
|
dockerops.compose_up(result.path, m.name)
|
||||||
actions.append(Action("compose_up", m.name))
|
except (dockerops.DockerError, FileNotFoundError, OSError) as e:
|
||||||
if not dry_run:
|
# Catch broad enough to cover: docker daemon down, docker binary
|
||||||
dockerops.compose_up(result.path, m.name)
|
# missing on the box, compose file unreadable. Narrow enough that
|
||||||
|
# programmer errors (KeyError etc.) still surface.
|
||||||
|
actions.append(Action("error", m.name, str(e)))
|
||||||
return actions
|
return actions
|
||||||
|
|
||||||
|
|
||||||
|
def has_errors(actions: list[Action]) -> bool:
|
||||||
|
return any(a.kind == "error" for a in actions)
|
||||||
|
|
|
||||||
|
|
@ -27,7 +27,7 @@ def scan(apps_root: Path) -> list[ScanResult]:
|
||||||
out.append(ScanResult(path=entry, manifest=None, error="manifest.json missing"))
|
out.append(ScanResult(path=entry, manifest=None, error="manifest.json missing"))
|
||||||
continue
|
continue
|
||||||
try:
|
try:
|
||||||
m = load_manifest(manifest_path)
|
m = load_manifest(manifest_path, expected_name=entry.name)
|
||||||
except ManifestError as e:
|
except ManifestError as e:
|
||||||
out.append(ScanResult(path=entry, manifest=None, error=str(e)))
|
out.append(ScanResult(path=entry, manifest=None, error=str(e)))
|
||||||
continue
|
continue
|
||||||
|
|
|
||||||
|
|
@ -92,6 +92,20 @@ def test_install_from_rejects_missing_manifest(tmp_path, fake_dirs):
|
||||||
installer.install_from(src)
|
installer.install_from(src)
|
||||||
|
|
||||||
|
|
||||||
|
def test_install_from_arbitrary_source_folder_name(tmp_path, fake_dirs):
|
||||||
|
# Source folder named "downloaded-fileshare-fork-v2" but manifest says
|
||||||
|
# "fileshare" — install lands at /var/lib/furtka/apps/fileshare/ regardless.
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path,
|
||||||
|
"downloaded-fileshare-fork-v2",
|
||||||
|
VALID_MANIFEST,
|
||||||
|
env_example="A=real-value",
|
||||||
|
)
|
||||||
|
target = installer.install_from(src)
|
||||||
|
assert target.name == "fileshare"
|
||||||
|
assert (target / "manifest.json").exists()
|
||||||
|
|
||||||
|
|
||||||
def test_install_from_rejects_invalid_manifest(tmp_path, fake_dirs):
|
def test_install_from_rejects_invalid_manifest(tmp_path, fake_dirs):
|
||||||
bad = dict(VALID_MANIFEST)
|
bad = dict(VALID_MANIFEST)
|
||||||
del bad["volumes"]
|
del bad["volumes"]
|
||||||
|
|
@ -116,3 +130,62 @@ def test_remove_unknown_raises(fake_dirs):
|
||||||
def test_bundled_apps_dir_uses_env_override(fake_dirs):
|
def test_bundled_apps_dir_uses_env_override(fake_dirs):
|
||||||
_, bundled = fake_dirs
|
_, bundled = fake_dirs
|
||||||
assert bundled_apps_dir() == bundled
|
assert bundled_apps_dir() == bundled
|
||||||
|
|
||||||
|
|
||||||
|
def test_install_refuses_placeholder_password(tmp_path, fake_dirs):
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path, "fileshare", VALID_MANIFEST, env_example="SMB_PASSWORD=changeme"
|
||||||
|
)
|
||||||
|
with pytest.raises(installer.InstallError, match="placeholder values for SMB_PASSWORD"):
|
||||||
|
installer.install_from(src)
|
||||||
|
# Files should still have landed so the user can vim the .env in place.
|
||||||
|
target = apps_dir() / "fileshare"
|
||||||
|
assert (target / ".env").exists()
|
||||||
|
assert (target / "manifest.json").exists()
|
||||||
|
|
||||||
|
|
||||||
|
def test_install_succeeds_after_user_edits_env(tmp_path, fake_dirs):
|
||||||
|
# First run: refuses placeholder.
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path, "fileshare", VALID_MANIFEST, env_example="SMB_PASSWORD=changeme"
|
||||||
|
)
|
||||||
|
with pytest.raises(installer.InstallError):
|
||||||
|
installer.install_from(src)
|
||||||
|
# User edits the live .env to a real secret.
|
||||||
|
target = apps_dir() / "fileshare"
|
||||||
|
(target / ".env").write_text("SMB_PASSWORD=hunter2\n")
|
||||||
|
# Re-run: now succeeds, user .env preserved.
|
||||||
|
installer.install_from(src)
|
||||||
|
assert (target / ".env").read_text().strip() == "SMB_PASSWORD=hunter2"
|
||||||
|
|
||||||
|
|
||||||
|
def test_install_locks_env_permissions(tmp_path, fake_dirs):
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path, "fileshare", VALID_MANIFEST, env_example="SMB_PASSWORD=hunter2"
|
||||||
|
)
|
||||||
|
installer.install_from(src)
|
||||||
|
target = apps_dir() / "fileshare"
|
||||||
|
mode = (target / ".env").stat().st_mode & 0o777
|
||||||
|
assert mode == 0o600, f"expected 0o600 on .env, got {oct(mode)}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_placeholder_check_ignores_comments_and_blanks(tmp_path, fake_dirs):
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path,
|
||||||
|
"fileshare",
|
||||||
|
VALID_MANIFEST,
|
||||||
|
env_example="# default values\n\nSMB_PASSWORD=real-secret\n",
|
||||||
|
)
|
||||||
|
# Should NOT raise — only commented "changeme" mentions, no actual placeholder.
|
||||||
|
installer.install_from(src)
|
||||||
|
|
||||||
|
|
||||||
|
def test_placeholder_check_handles_quoted_values(tmp_path, fake_dirs):
|
||||||
|
src = _write_app_source(
|
||||||
|
tmp_path,
|
||||||
|
"fileshare",
|
||||||
|
VALID_MANIFEST,
|
||||||
|
env_example='SMB_PASSWORD="changeme"\n',
|
||||||
|
)
|
||||||
|
with pytest.raises(installer.InstallError, match="placeholder"):
|
||||||
|
installer.install_from(src)
|
||||||
|
|
|
||||||
|
|
@ -52,10 +52,20 @@ def test_missing_required_field(tmp_path):
|
||||||
load_manifest(path)
|
load_manifest(path)
|
||||||
|
|
||||||
|
|
||||||
def test_name_must_match_folder(tmp_path):
|
def test_name_must_match_when_expected_name_given(tmp_path):
|
||||||
|
# Scanner passes expected_name=<folder name> so /var/lib/furtka/apps/X/
|
||||||
|
# can't lie about its own identity.
|
||||||
path = _write_app(tmp_path, "wrong-folder", VALID_MANIFEST)
|
path = _write_app(tmp_path, "wrong-folder", VALID_MANIFEST)
|
||||||
with pytest.raises(ManifestError, match="must equal containing folder"):
|
with pytest.raises(ManifestError, match="must equal 'wrong-folder'"):
|
||||||
load_manifest(path)
|
load_manifest(path, expected_name="wrong-folder")
|
||||||
|
|
||||||
|
|
||||||
|
def test_name_check_skipped_without_expected_name(tmp_path):
|
||||||
|
# Installer loads from arbitrary source paths (e.g. /tmp/my-tweaked-app/)
|
||||||
|
# — the source folder name shouldn't matter, only the manifest's own name.
|
||||||
|
path = _write_app(tmp_path, "any-folder-name", VALID_MANIFEST)
|
||||||
|
m = load_manifest(path)
|
||||||
|
assert m.name == "fileshare"
|
||||||
|
|
||||||
|
|
||||||
def test_invalid_json(tmp_path):
|
def test_invalid_json(tmp_path):
|
||||||
|
|
|
||||||
|
|
@ -89,3 +89,47 @@ def test_reconcile_skips_broken_manifest(tmp_path, fake_docker):
|
||||||
assert skip_actions[0].target == "broken"
|
assert skip_actions[0].target == "broken"
|
||||||
# Healthy app still got reconciled.
|
# Healthy app still got reconciled.
|
||||||
assert fake_docker["compose_up"] == [(str(tmp_path / "fileshare"), "fileshare")]
|
assert fake_docker["compose_up"] == [(str(tmp_path / "fileshare"), "fileshare")]
|
||||||
|
|
||||||
|
|
||||||
|
def test_reconcile_isolates_per_app_docker_failure(tmp_path, monkeypatch):
|
||||||
|
"""A docker error on one app must not block reconcile of the others."""
|
||||||
|
_make_app(tmp_path, "alpha", dict(VALID_MANIFEST, name="alpha", volumes=["a"]))
|
||||||
|
_make_app(tmp_path, "broken-app", dict(VALID_MANIFEST, name="broken-app", volumes=["b"]))
|
||||||
|
_make_app(tmp_path, "zulu", dict(VALID_MANIFEST, name="zulu", volumes=["z"]))
|
||||||
|
|
||||||
|
succeeded_compose: list[str] = []
|
||||||
|
|
||||||
|
def fake_ensure(name):
|
||||||
|
return True
|
||||||
|
|
||||||
|
def fake_compose_up(app_dir, project):
|
||||||
|
if project == "broken-app":
|
||||||
|
raise dockerops.DockerError("simulated daemon failure")
|
||||||
|
succeeded_compose.append(project)
|
||||||
|
|
||||||
|
monkeypatch.setattr(dockerops, "ensure_volume", fake_ensure)
|
||||||
|
monkeypatch.setattr(dockerops, "compose_up", fake_compose_up)
|
||||||
|
|
||||||
|
actions = reconciler.reconcile(tmp_path)
|
||||||
|
error_actions = [a for a in actions if a.kind == "error"]
|
||||||
|
|
||||||
|
assert reconciler.has_errors(actions)
|
||||||
|
assert len(error_actions) == 1
|
||||||
|
assert error_actions[0].target == "broken-app"
|
||||||
|
# Both healthy apps reconciled despite the broken one in the middle.
|
||||||
|
assert succeeded_compose == ["alpha", "zulu"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_reconcile_isolates_missing_docker_binary(tmp_path, monkeypatch):
|
||||||
|
"""No docker binary on the box still produces a tidy per-app error."""
|
||||||
|
_make_app(tmp_path, "fileshare", VALID_MANIFEST)
|
||||||
|
|
||||||
|
def boom(*args, **kwargs):
|
||||||
|
raise FileNotFoundError("[Errno 2] No such file or directory: 'docker'")
|
||||||
|
|
||||||
|
monkeypatch.setattr(dockerops, "ensure_volume", boom)
|
||||||
|
actions = reconciler.reconcile(tmp_path)
|
||||||
|
assert reconciler.has_errors(actions)
|
||||||
|
error = next(a for a in actions if a.kind == "error")
|
||||||
|
assert error.target == "fileshare"
|
||||||
|
assert "docker" in error.detail
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue