diff --git a/CHANGELOG.md b/CHANGELOG.md index 26525ec..2760ca7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ # Changelog +## v0.3.3 — 2026-07-13 + +### Security + +- **The restic repository password no longer passes through a process's argv.** + `create_task.py` shelled out to `midclt call cloud_backup.create ''`, and + that JSON contains the repo password — so it appeared in the process's argv, + which is world-readable via `ps`, for the duration of the call. That password is + the encryption key for the entire cloud backup repository. + + It now talks to the middleware through `truenas_api_client` (the library that + backs `midclt` itself), so the password never leaves the process's memory. + +- **`--password` no longer required.** Passing a secret as a CLI argument writes it + to shell history permanently. `--password-stdin` reads it from stdin, and with + neither flag the tool prompts via `getpass`. `--password` still works but now + warns. + +### Fixed + +- **`uninstall.sh` could leave every patch installed.** It reverted by unmounting + the overlay — but `apply.sh` only mounts one when the target directory is + read-only. On a writable `/usr` it patches the real files in place, and uninstall + would remove the boot hook, report success, and leave the patch applied. It now + strips the appended blocks from the middleware files explicitly. + +- **`create_task.py.__version__` had been stuck at `0.2.0`** for three releases. + The version-drift check added in v0.3.1 only looked at `VERSION=` in shell + scripts, so it missed the one file that actually shows a version to users + (`--version`). The check now covers `__version__` too — and caught this + immediately. + ## v0.3.2 — 2026-07-13 ### Fixed diff --git a/install.sh b/install.sh index c998823..c039970 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ set -euo pipefail -VERSION="0.3.2" +VERSION="0.3.3" # The directory containing install.sh is the permanent install location. PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/patch/apply.sh b/patch/apply.sh index 7c4cc09..26d63b7 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -32,7 +32,7 @@ # Derive PATCH_DIR from this script's location (parent of the patch/ directory). PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)" LOG="$PATCH_DIR/apply.log" -VERSION="0.3.2" +VERSION="0.3.3" # Rotate log at 512 KB to avoid unbounded growth on a system volume. # Keep two prior generations (.1 and .2) so the last three boots are always available. diff --git a/patch/create_task.py b/patch/create_task.py index c0ad384..9a1f397 100755 --- a/patch/create_task.py +++ b/patch/create_task.py @@ -26,8 +26,9 @@ Create a task backed by a B2 credential (id=3): --credential 3 \\ --bucket my-bucket \\ --folder backups/tank \\ - --password "restic-repo-password" \\ + --password-stdin \\ --keep-last 14 + (pipe the password in: echo -n "s3cret" | python3 create_task.py create ... ) Create a task using an S3-compatible credential (Wasabi, R2, etc.): python3 create_task.py create \\ @@ -36,7 +37,7 @@ Create a task using an S3-compatible credential (Wasabi, R2, etc.): --credential 5 \\ --bucket my-bucket \\ --folder backups \\ - --password "restic-repo-password" + --password-stdin List existing TrueCloud Backup tasks: python3 create_task.py list-tasks @@ -44,39 +45,49 @@ List existing TrueCloud Backup tasks: import argparse import calendar +import getpass import json import os import subprocess import sys import time -__version__ = "0.2.0" +__version__ = "0.3.3" _PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json") def midclt_call(method, *args): - """Call a middleware method locally via `midclt`, the supported JSON-RPC transport - that replaces the deprecated /api/v2.0 REST API (removed in TrueNAS 26.04). Must run - on the TrueNAS host. Each arg is JSON-encoded (a dict for create; none for queries). - Exits with a clear message on failure.""" - cmd = ["midclt", "call", method] + [json.dumps(a) for a in args] + """Call a middleware method on the local host. + + Uses `truenas_api_client` -- the library that backs `midclt` itself -- rather + than shelling out to `midclt`. + + This is a SECURITY requirement, not a style choice. `midclt call + ` puts its arguments in the process's **argv**, and `cloud_backup.create` + carries the restic repository password. argv is world-readable via `ps`, so + shelling out would expose the key to the entire backup repo to every local + user for the duration of the call. Going through the client library keeps it + in this process's memory. + """ try: - proc = subprocess.run(cmd, capture_output=True, text=True, timeout=120) - except FileNotFoundError: - print("ERROR: `midclt` not found — run this script ON the TrueNAS host.", - file=sys.stderr) + from truenas_api_client import Client + except ImportError: + print( + "ERROR: `truenas_api_client` not importable — run this script ON the\n" + " TrueNAS host. (It ships with midclt.)", + file=sys.stderr, + ) sys.exit(1) - except subprocess.SubprocessError as exc: - print(f"ERROR: midclt call failed: {exc}", file=sys.stderr) + + try: + with Client() as client: + return client.call(method, *args) + except Exception as exc: # noqa: BLE001 - surface any middleware error verbatim + # Never echo `args` here: for cloud_backup.create it contains the password. + print(f"ERROR: {method}: {exc}", file=sys.stderr) sys.exit(1) - if proc.returncode != 0: - print(f"ERROR: midclt {method}: {(proc.stderr or proc.stdout).strip()}", - file=sys.stderr) - sys.exit(1) - out = proc.stdout.strip() - return json.loads(out) if out else None # ── Sub-commands ────────────────────────────────────────────────────────────── @@ -213,6 +224,37 @@ def cmd_list_tasks(_args): print(f"{t['id']:>4} {enabled:<8} {ptype:<14} {t.get('description', '')}") +def _resolve_password(args): + """Get the restic repo password without writing it to the user's shell history. + + That password is the key to the whole backup repository. `--password ` + persists it in ~/.bash_history and exposes it in `ps` for the lifetime of the + shell command, so it is accepted but warned about; stdin and an interactive + prompt are the safe paths. + """ + if args.password_stdin: + if args.password: + print("ERROR: use either --password or --password-stdin, not both.", + file=sys.stderr) + sys.exit(1) + password = sys.stdin.readline().rstrip("\n") + elif args.password: + print( + "WARNING: --password puts the restic repository password in your shell\n" + " history. Prefer: echo -n 'pw' | ... --password-stdin", + file=sys.stderr, + ) + password = args.password + else: + password = getpass.getpass("Restic repository password: ") + + if not password: + print("ERROR: the restic repository password must not be empty.", + file=sys.stderr) + sys.exit(1) + return password + + def cmd_create(args): parts = args.schedule.split() if len(parts) != 5: @@ -223,6 +265,8 @@ def cmd_create(args): sys.exit(1) minute, hour, dom, month, dow = parts + password = _resolve_password(args) + body = { "description": args.name, "path": args.path, @@ -231,7 +275,7 @@ def cmd_create(args): "bucket": args.bucket, "folder": args.folder, }, - "password": args.password, + "password": password, "keep_last": args.keep_last, "transfer_setting": args.transfer_setting, "schedule": { @@ -297,8 +341,11 @@ def main(): help="Bucket (S3) or container (B2) name") c.add_argument("--folder", default="", help="Path within the bucket (default: root)") - c.add_argument("--password", required=True, - help="Restic repository encryption password (choose a strong one)") + c.add_argument("--password", default=None, + help="Restic repository password. UNSAFE: it lands in your shell " + "history. Prefer --password-stdin, or omit both and be prompted.") + c.add_argument("--password-stdin", action="store_true", + help="Read the restic repository password from stdin (recommended)") c.add_argument("--keep-last", type=int, default=14, metavar="N", help="Snapshots to retain after each run (default: 14)") c.add_argument("--schedule", default="0 2 * * *", diff --git a/recover.sh b/recover.sh index 78870a8..3642bba 100755 --- a/recover.sh +++ b/recover.sh @@ -17,7 +17,7 @@ # bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh # systemctl restart middlewared -VERSION="0.3.2" +VERSION="0.3.3" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/tests/test_create_task.py b/tests/test_create_task.py new file mode 100644 index 0000000..0821a54 --- /dev/null +++ b/tests/test_create_task.py @@ -0,0 +1,107 @@ +"""Tests for create_task.py, focused on the restic repository password. + +That password is the encryption key for the entire cloud backup repository. It +used to travel through `midclt call cloud_backup.create ''` -- i.e. through +the subprocess's **argv**, which is world-readable via `ps` -- and `--password` +wrote it into the user's shell history forever. +""" + +import importlib.util +import io +import os +import sys +import types + +import pytest + +SPEC = importlib.util.spec_from_file_location( + "create_task", + os.path.join(os.path.dirname(__file__), "..", "patch", "create_task.py"), +) + + +def load(): + mod = importlib.util.module_from_spec(SPEC) + SPEC.loader.exec_module(mod) + return mod + + +class Args: + def __init__(self, password=None, password_stdin=False): + self.password = password + self.password_stdin = password_stdin + + +class TestPasswordNeverReachesArgv: + """The whole reason this module talks to the client library.""" + + def test_midclt_call_spawns_no_subprocess(self): + import inspect + + src = inspect.getsource(load().midclt_call) + assert "subprocess" not in src, ( + "shelling out to `midclt` puts cloud_backup.create's JSON -- including " + "the restic repo password -- into argv, which any local user can read " + "with ps" + ) + assert "truenas_api_client" in src + + def test_errors_never_echo_the_call_arguments(self): + # A failed cloud_backup.create must not print the body back at the user; + # it contains the password. + import inspect + + src = inspect.getsource(load().midclt_call) + assert "{args}" not in src + assert "args!r" not in src + + +class TestResolvePassword: + def test_reads_from_stdin(self, monkeypatch): + mod = load() + monkeypatch.setattr(sys, "stdin", io.StringIO("s3cret\n")) + assert mod._resolve_password(Args(password_stdin=True)) == "s3cret" + + def test_strips_only_the_trailing_newline(self, monkeypatch): + # A password may legitimately contain spaces; only the line ending goes. + mod = load() + monkeypatch.setattr(sys, "stdin", io.StringIO(" pass word \n")) + assert mod._resolve_password(Args(password_stdin=True)) == " pass word " + + def test_cli_password_still_works_but_warns(self, monkeypatch, capsys): + mod = load() + pw = mod._resolve_password(Args(password="cli-secret")) + assert pw == "cli-secret" + assert "shell" in capsys.readouterr().err.lower(), "must warn about history" + + def test_prompts_when_neither_flag_given(self, monkeypatch): + mod = load() + monkeypatch.setattr( + mod, "getpass", types.SimpleNamespace(getpass=lambda _p: "prompted") + ) + assert mod._resolve_password(Args()) == "prompted" + + def test_rejects_both_flags(self, monkeypatch): + mod = load() + monkeypatch.setattr(sys, "stdin", io.StringIO("x\n")) + with pytest.raises(SystemExit): + mod._resolve_password(Args(password="a", password_stdin=True)) + + def test_rejects_an_empty_password(self, monkeypatch): + # An empty restic password would silently create an unencrypted-ish repo. + mod = load() + monkeypatch.setattr(sys, "stdin", io.StringIO("\n")) + with pytest.raises(SystemExit): + mod._resolve_password(Args(password_stdin=True)) + + +class TestVersion: + def test_version_is_not_stale(self): + # __version__ sat at 0.2.0 through three releases because the drift check + # only looked at VERSION= in shell scripts. It covers this file now. + sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "tools")) + from release_notes import normalise, script_versions + + repo = os.path.join(os.path.dirname(__file__), "..") + versions = {normalise(v) for v in script_versions(repo).values()} + assert len(versions) == 1, f"version drift: {sorted(versions)}" diff --git a/tools/release_notes.py b/tools/release_notes.py index 2c55a47..872bbe6 100644 --- a/tools/release_notes.py +++ b/tools/release_notes.py @@ -19,17 +19,20 @@ import sys ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) CHANGELOG = os.path.join(ROOT, "CHANGELOG.md") -# Every script prints a version; they must all agree, and agree with the tag. -# They drifted to three different values once (0.0.4 / 0.2.1) before anything -# checked them. +# Everything that announces a version must agree with everything else. They drifted +# to three different values once (0.0.4 / 0.2.1) before anything checked them -- +# and create_task.py's __version__ then sat at 0.2.0 through three more releases, +# because the first version of this check only looked at VERSION= in shell scripts. VERSIONED_FILES = [ "install.sh", "uninstall.sh", "recover.sh", os.path.join("patch", "apply.sh"), + os.path.join("patch", "create_task.py"), # exposes `--version` to users ] -_VERSION_RE = re.compile(r'^VERSION="([^"]+)"', re.M) +# `VERSION="x"` (shell) or `__version__ = "x"` (python). +_VERSION_RE = re.compile(r'^(?:VERSION=|__version__\s*=\s*)"([^"]+)"', re.M) _HEADING_RE = re.compile(r"^##\s+v?(\d+\.\d+\.\d+[^\s]*)", re.M) diff --git a/uninstall.sh b/uninstall.sh index 1a1436b..5927134 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -3,7 +3,7 @@ set -euo pipefail -VERSION="0.3.2" +VERSION="0.3.3" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _HOOK_COMMENT='TrueCloud provider patch (S3/B2)' @@ -91,6 +91,63 @@ if [ "$_ov_found" -eq 0 ]; then fi echo "" +# ── Revert file-level patches ───────────────────────────────────────────────── +# Unmounting the overlay is what normally reverts everything — the lower layer is +# the untouched /usr. But apply.sh only mounts an overlay when the directory is +# read-only; on a writable /usr it patches the real files in place. Uninstall +# would then remove the boot hook and report success while leaving every patch +# applied. Strip our appended blocks explicitly. + +echo "Reverting any file-level patches ..." +python3 - <<'PYEOF' +import os + +try: + import middlewared +except ImportError: + print(" middlewared not importable — nothing to revert.") + raise SystemExit(0) + +mw = os.path.dirname(os.path.abspath(middlewared.__file__)) +targets = [ + os.path.join(mw, "rclone", "remote", "b2.py"), + os.path.join(mw, "plugins", "cloud_backup", "restic.py"), + os.path.join(mw, "plugins", "cloud_backup", "sync.py"), + os.path.join(mw, "plugins", "cloud", "crud.py"), + os.path.join(mw, "plugins", "cloud", "snapshot.py"), +] + +reverted = [] +for path in targets: + try: + with open(path, encoding="utf-8") as fh: + content = fh.read() + except OSError: + continue + idx = content.find("\n# TRUECLOUD_PATCH") + if idx == -1: + continue + try: + with open(path, "w", encoding="utf-8") as fh: + fh.write(content[:idx].rstrip("\n") + "\n") + reverted.append(os.path.basename(path)) + except OSError as e: + print(f" WARNING: could not revert {path}: {e}") + +nested = os.path.join(mw, "plugins", "cloud", "_truecloud_nested.py") +try: + os.unlink(nested) + reverted.append("_truecloud_nested.py") +except OSError: + pass + +if reverted: + print(" Reverted: " + ", ".join(reverted)) +else: + print(" Nothing to revert (overlay already removed, or never patched).") +PYEOF +echo "" + # ── Unmount nested-snapshot staging trees ───────────────────────────────────── # These bind mounts pin their ZFS snapshots, so they must go before anything # tries to destroy those snapshots. Deepest first.