diff --git a/CHANGELOG.md b/CHANGELOG.md index 2760ca7..2f57bba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,29 @@ # Changelog +## v0.3.4 — 2026-07-13 + +### Changed + +- **One implementation of apply/revert (`patch/mw_patch.py`).** The "strip the + `TRUECLOUD_PATCH` block" logic existed twice — in `apply.sh`'s heredoc and in an + inline heredoc in `uninstall.sh` — and the uninstall copy was the untested one. + That is exactly how the two could have drifted apart, with `apply.sh` reverting + one set of files and `uninstall.sh` another. Both now call the same tested + module (17 new tests, including that `revert_nested` never touches `restic.py`, + which belongs to the providers module and whose removal would silently break B2 + backups). + + `apply.sh` imports it fail-safe: if it cannot, the backend patch is skipped and + middlewared starts stock, which is this script's whole design principle. The + import uses `sys.path.append`, never `insert(0)` — prepending would give + `patch/` precedence over the stdlib for that interpreter, so a future + `patch/json.py` would shadow the real `json` and break the boot. + +### Docs + +- The README's `create_task.py` example still taught `--password `, which + is how a security fix quietly fails to land. It now shows `--password-stdin`. + ## v0.3.3 — 2026-07-13 ### Security diff --git a/README.md b/README.md index 9ec2111..66f3de5 100644 --- a/README.md +++ b/README.md @@ -376,18 +376,25 @@ host address or API key: # List your cloud credentials to find the right ID python3 /mnt/tank/truenas-truecloud-patch/patch/create_task.py list-credentials -# Create a task with a B2 credential (id=3) +# Create a task with a B2 credential (id=3). +# The restic repo password is read from stdin, so it never lands in your shell +# history — nor in any process's argv, where `ps` would expose it. +printf '%s' 'restic-repo-password' | \ python3 /mnt/tank/truenas-truecloud-patch/patch/create_task.py create \ --name "tank-to-b2" \ --path /mnt/tank/data \ --credential 3 \ --bucket my-bucket \ --folder backups/tank \ - --password "restic-repo-password" \ + --password-stdin \ --cache-path /mnt/tank/.restic-cache \ --keep-last 14 ``` +Omit `--password-stdin` and you'll be prompted for the password instead. `--password +` still works but warns: that password is the encryption key for the whole +repository, and a CLI argument persists in your shell history forever. + > **Always pass `--cache-path`.** Without it TrueNAS runs restic with `--no-cache`, > which re-fetches all repo metadata from the provider every run — glacially slow > on large repos. Point it at a writable dir on a pool with free space. diff --git a/install.sh b/install.sh index c039970..1a5ead3 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ set -euo pipefail -VERSION="0.3.3" +VERSION="0.3.4" # 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 26d63b7..608cc1a 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.3" +VERSION="0.3.4" # 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. @@ -468,65 +468,24 @@ if _tc_nested is not None: """ -def patch_file(path, block): - with open(path, encoding="utf-8") as fh: - content = fh.read() - marker = "\n# TRUECLOUD_PATCH" - idx = content.find(marker) - base = content[:idx] if idx != -1 else content - with open(path, "w", encoding="utf-8") as fh: - fh.write(base.rstrip("\n") + "\n" + block) +# Single implementation of the block apply/revert logic (patch/mw_patch.py), so +# uninstall.sh and apply.sh cannot drift apart. Fail-safe: if it cannot be +# imported, skip the backend patch entirely -- middlewared then starts stock, +# which is the whole design principle of this script. +# APPEND, never insert(0): this dir would otherwise take precedence over the +# stdlib for this interpreter, so a future patch/json.py (say) would shadow the +# real json module and break the boot. Appending fails safe -- worst case our +# import misses and the backend patch is skipped. +sys.path.append(os.path.dirname(nested_src)) +try: + from mw_patch import patch_file, revert_nested +except ImportError as _e: + print(f'WARNING: cannot import patch/mw_patch.py ({_e}) — skipping backend patch.') + print('WARNING: middlewared will start with stock (unpatched) modules.') + sys.exit(1) - -def unpatch_file(path): - """Strip our appended block, restoring the stock file. True if it was patched.""" - try: - with open(path, encoding="utf-8") as fh: - content = fh.read() - except OSError: - return False - idx = content.find("\n# TRUECLOUD_PATCH") - if idx == -1: - return False - try: - with open(path, "w", encoding="utf-8") as fh: - fh.write(content[:idx].rstrip("\n") + "\n") - except OSError: - return False - return True - - -def revert_nested(cloud_dir, sync_path): - """Undo the nested patch. Returns the names of what was actually reverted. - - Skipping the patch is NOT enough to disable the feature. The overlay persists - for the whole boot, so an earlier run this boot may already have written the - patched files -- and middlewared re-imports them on the restart that - install.sh performs. Without this, `install.sh --disable-nested-snapshots` - would report "disabled" while the feature kept running until the next reboot. - """ - reverted = [] - - # Remove the module FIRST. Every injected block is guarded by - # `if _tc_nested is not None`, so once it is gone they all no-op even if a - # later step here fails -- the guard is restored no matter what. - try: - os.unlink(os.path.join(cloud_dir, '_truecloud_nested.py')) - reverted.append('_truecloud_nested.py') - except OSError: - pass - - # NB: restic.py also carries a TRUECLOUD_PATCH block, but that belongs to the - # providers module. Only these three are ours to revert. - for name, path in ( - ('crud.py', os.path.join(cloud_dir, 'crud.py')), - ('sync.py', sync_path), - ('snapshot.py', os.path.join(cloud_dir, 'snapshot.py')), - ): - if unpatch_file(path): - reverted.append(name) - - return reverted +# .../middlewared/plugins/cloud -> .../middlewared +mw_dir = os.path.dirname(os.path.dirname(cloud_dir)) b2_ok = restic_ok = False nested_ok = False @@ -582,7 +541,7 @@ if not nested_needed: nested_detail = 'not needed' print('INFO: Nested module skipped.') - reverted = revert_nested(cloud_dir, sync_path) + reverted = revert_nested(mw_dir) if reverted: print('OK: Reverted a previously-applied nested patch (' + ', '.join(reverted) + ').') print(' The stock nesting guard is restored once middlewared restarts.') diff --git a/patch/create_task.py b/patch/create_task.py index 9a1f397..c64285d 100755 --- a/patch/create_task.py +++ b/patch/create_task.py @@ -52,7 +52,7 @@ import subprocess import sys import time -__version__ = "0.3.3" +__version__ = "0.3.4" _PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json") diff --git a/patch/mw_patch.py b/patch/mw_patch.py new file mode 100644 index 0000000..27d8f28 --- /dev/null +++ b/patch/mw_patch.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +"""Apply and revert truecloud-patch's blocks in middlewared's modules. + +Every patch this project makes to a middlewared module is an appended block that +begins with the MARKER line. That makes patching idempotent (truncate at the +marker, re-append) and reverting exact (truncate at the marker, stop). + +This is the single implementation of that. It used to live in two places -- +apply.sh's heredoc and an inline heredoc in uninstall.sh -- and the uninstall copy +was the untested one. + + python3 mw_patch.py revert-all # remove every block + the nested module + python3 mw_patch.py revert-nested # remove only the nested module's blocks +""" + +from __future__ import annotations + +import os +import sys + +MARKER = "\n# TRUECLOUD_PATCH" + +#: Modules the providers module (B2/S3) patches. +PROVIDER_RELPATHS = [ + ("rclone", "remote", "b2.py"), + ("plugins", "cloud_backup", "restic.py"), +] + +#: Modules the nested-snapshot module patches. Order matters on revert -- see +#: revert(): the loadable module goes first. +NESTED_RELPATHS = [ + ("plugins", "cloud", "crud.py"), + ("plugins", "cloud_backup", "sync.py"), + ("plugins", "cloud", "snapshot.py"), +] + +#: The importable module the nested blocks depend on. +NESTED_MODULE = ("plugins", "cloud", "_truecloud_nested.py") + + +def patch_file(path, block): + """Append `block`, replacing any block we appended before. Idempotent.""" + with open(path, encoding="utf-8") as fh: + content = fh.read() + idx = content.find(MARKER) + base = content[:idx] if idx != -1 else content + with open(path, "w", encoding="utf-8") as fh: + fh.write(base.rstrip("\n") + "\n" + block) + + +def unpatch_file(path): + """Strip our appended block, restoring the stock file. True if it was patched.""" + try: + with open(path, encoding="utf-8") as fh: + content = fh.read() + except OSError: + return False + idx = content.find(MARKER) + if idx == -1: + return False + try: + with open(path, "w", encoding="utf-8") as fh: + fh.write(content[:idx].rstrip("\n") + "\n") + except OSError: + return False + return True + + +def revert(mw_dir, relpaths, module_relpath=None): + """Remove our blocks from `relpaths`, and the module at `module_relpath`. + + The module is deleted FIRST. Every injected block is guarded by + `if _tc_nested is not None`, so once the module is gone the blocks all no-op + even if a later unpatch fails -- the stock guard comes back regardless. + + Returns the names of what was actually reverted. + """ + reverted = [] + + if module_relpath: + try: + os.unlink(os.path.join(mw_dir, *module_relpath)) + reverted.append(module_relpath[-1]) + except OSError: + pass + + for rel in relpaths: + if unpatch_file(os.path.join(mw_dir, *rel)): + reverted.append(rel[-1]) + + return reverted + + +def revert_nested(mw_dir): + """Undo the nested-snapshot patch only. Leaves the providers patch alone. + + restic.py also carries a block, but it belongs to the providers module -- + reverting it would silently break B2 backups. + """ + return revert(mw_dir, NESTED_RELPATHS, NESTED_MODULE) + + +def revert_all(mw_dir): + """Undo every patch this project applies.""" + return revert(mw_dir, NESTED_RELPATHS + PROVIDER_RELPATHS, NESTED_MODULE) + + +def find_middlewared_dir(): + """Directory of the installed `middlewared` package, or None.""" + try: + import middlewared + except ImportError: + return None + return os.path.dirname(os.path.abspath(middlewared.__file__)) + + +def main(argv): + if len(argv) < 2 or argv[1] not in ("revert-all", "revert-nested"): + print(__doc__, file=sys.stderr) + return 2 + + mw_dir = find_middlewared_dir() + if mw_dir is None: + print(" middlewared not importable — nothing to revert.") + return 0 + + fn = revert_all if argv[1] == "revert-all" else revert_nested + reverted = fn(mw_dir) + if reverted: + print(" Reverted: " + ", ".join(reverted)) + else: + print(" Nothing to revert (overlay already removed, or never patched).") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/recover.sh b/recover.sh index 3642bba..9b4fd5f 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.3" +VERSION="0.3.4" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index f0a67af..0d89139 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -277,42 +277,22 @@ class TestOptIn: running until the next reboot. """ src = heredoc_source() - assert "def unpatch_file(" in src - assert "def revert_nested(" in src - # The revert must run on every not-needed path (opt-out, superseded). + # The implementation lives in patch/mw_patch.py (see test_mw_patch.py); + # apply.sh must import and actually call it. + assert "from mw_patch import patch_file, revert_nested" in src gate = src.index("if not nested_needed:") revert = src.index("reverted = revert_nested(") patch = src.index("patch_file(crud_py, CRUD_BLOCK)") assert gate < revert < patch, "revert belongs in the not-needed branch" - def test_revert_removes_the_module_before_unpatching_files(self): - # Every injected block is guarded by `if _tc_nested is not None`, so - # deleting the module first means the guard is restored even if a later - # unpatch step fails. + def test_import_failure_skips_the_patch_rather_than_crashing(self): + # apply.sh runs at PREINIT. If mw_patch.py cannot be imported it must + # degrade to "middlewared starts stock", never take the boot down. src = heredoc_source() - body = src[src.index("def revert_nested("):src.index("def patch_file(") if - src.index("def patch_file(") > src.index("def revert_nested(") else len(src)] - body = src[src.index("def revert_nested("):] - body = body[:body.index("\n\n\n")] if "\n\n\n" in body else body - assert body.index("_truecloud_nested.py") < body.index("crud.py") - - def test_revert_never_touches_the_providers_patch(self): - # restic.py also carries a TRUECLOUD_PATCH block, but it belongs to the - # providers module. Reverting it would silently break B2 backups. - src = heredoc_source() - body = src[src.index("def revert_nested("):] - body = body[:body.index("return reverted")] - # Comments legitimately *mention* restic.py to explain why it is excluded; - # what matters is that no code line touches it. - code = "\n".join( - ln for ln in body.splitlines() if not ln.lstrip().startswith("#") - ) - assert "restic" not in code - assert "b2.py" not in code - # It must only ever revert these three, plus the module itself. - assert "crud.py" in code - assert "sync_path" in code - assert "snapshot.py" in code + i = src.index("from mw_patch import") + tail = src[i:i + 400] + assert "except ImportError" in tail + assert "skipping backend patch" in tail def test_guard_is_relaxed_only_after_traversal_is_installed(): diff --git a/tests/test_mw_patch.py b/tests/test_mw_patch.py new file mode 100644 index 0000000..8f342c1 --- /dev/null +++ b/tests/test_mw_patch.py @@ -0,0 +1,160 @@ +"""Tests for mw_patch — the single implementation of apply/revert. + +apply.sh and uninstall.sh both go through this. It used to be duplicated in an +untested shell heredoc, which is exactly how the two could have drifted apart: +apply.sh reverting one set of files and uninstall.sh another. +""" + +import os +import sys + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "patch")) + +from mw_patch import ( # noqa: E402 + MARKER, + NESTED_MODULE, + NESTED_RELPATHS, + PROVIDER_RELPATHS, + patch_file, + revert_all, + revert_nested, + unpatch_file, +) + +STOCK = "import os\n\n\ndef stock():\n return 1\n" +BLOCK = "\n# TRUECLOUD_PATCH\ninjected = 1\n" + + +def build_mw(root): + """A fake middlewared tree with every file this project touches.""" + mw = os.path.join(root, "middlewared") + for rel in NESTED_RELPATHS + PROVIDER_RELPATHS: + path = os.path.join(mw, *rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w", encoding="utf-8") as fh: + fh.write(STOCK) + with open(os.path.join(mw, *NESTED_MODULE), "w", encoding="utf-8") as fh: + fh.write("# module\n") + return mw + + +def read(mw, rel): + with open(os.path.join(mw, *rel), encoding="utf-8") as fh: + return fh.read() + + +class TestPatchFile: + def test_appends_the_block(self, tmp_path): + p = tmp_path / "m.py" + p.write_text(STOCK) + patch_file(str(p), BLOCK) + assert MARKER in p.read_text() + assert p.read_text().startswith("import os") + + def test_is_idempotent(self, tmp_path): + # apply.sh runs on EVERY boot. Without truncate-then-append, repeated runs + # would stack duplicate copies of the block into a middlewared module. + p = tmp_path / "m.py" + p.write_text(STOCK) + for _ in range(5): + patch_file(str(p), BLOCK) + assert p.read_text().count("# TRUECLOUD_PATCH") == 1 + assert p.read_text().count("injected = 1") == 1 + + def test_round_trips_back_to_stock(self, tmp_path): + p = tmp_path / "m.py" + p.write_text(STOCK) + patch_file(str(p), BLOCK) + assert unpatch_file(str(p)) is True + assert p.read_text() == STOCK + + +class TestUnpatchFile: + def test_returns_false_on_an_unpatched_file(self, tmp_path): + p = tmp_path / "m.py" + p.write_text(STOCK) + assert unpatch_file(str(p)) is False + assert p.read_text() == STOCK + + def test_returns_false_on_a_missing_file(self, tmp_path): + assert unpatch_file(str(tmp_path / "nope.py")) is False + + +class TestRevertNested: + def test_reverts_only_the_nested_files(self, tmp_path): + mw = build_mw(str(tmp_path)) + for rel in NESTED_RELPATHS + PROVIDER_RELPATHS: + patch_file(os.path.join(mw, *rel), BLOCK) + + reverted = revert_nested(mw) + + for rel in NESTED_RELPATHS: + assert read(mw, rel) == STOCK, f"{rel[-1]} should be stock" + assert rel[-1] in reverted + + def test_never_touches_the_providers_patch(self, tmp_path): + # restic.py carries a TRUECLOUD_PATCH block too, but it belongs to the + # providers module. Reverting it would silently break B2 backups. + mw = build_mw(str(tmp_path)) + for rel in NESTED_RELPATHS + PROVIDER_RELPATHS: + patch_file(os.path.join(mw, *rel), BLOCK) + + revert_nested(mw) + + for rel in PROVIDER_RELPATHS: + assert MARKER in read(mw, rel), f"{rel[-1]} must keep its providers block" + + def test_removes_the_module(self, tmp_path): + mw = build_mw(str(tmp_path)) + assert os.path.exists(os.path.join(mw, *NESTED_MODULE)) + reverted = revert_nested(mw) + assert not os.path.exists(os.path.join(mw, *NESTED_MODULE)) + assert "_truecloud_nested.py" in reverted + + def test_module_is_removed_before_the_files_are_unpatched(self, tmp_path): + # Every injected block is guarded by `if _tc_nested is not None`, so once + # the module is gone they all no-op — the stock guard is restored even if + # a later unpatch fails. + mw = build_mw(str(tmp_path)) + for rel in NESTED_RELPATHS: + patch_file(os.path.join(mw, *rel), BLOCK) + reverted = revert_nested(mw) + assert reverted[0] == "_truecloud_nested.py" + + def test_is_idempotent(self, tmp_path): + mw = build_mw(str(tmp_path)) + for rel in NESTED_RELPATHS: + patch_file(os.path.join(mw, *rel), BLOCK) + revert_nested(mw) + assert revert_nested(mw) == [] + + +class TestRevertAll: + def test_reverts_providers_and_nested(self, tmp_path): + mw = build_mw(str(tmp_path)) + for rel in NESTED_RELPATHS + PROVIDER_RELPATHS: + patch_file(os.path.join(mw, *rel), BLOCK) + + revert_all(mw) + + for rel in NESTED_RELPATHS + PROVIDER_RELPATHS: + assert read(mw, rel) == STOCK, f"{rel[-1]} should be stock" + assert not os.path.exists(os.path.join(mw, *NESTED_MODULE)) + + def test_is_a_noop_on_a_stock_tree(self, tmp_path): + mw = build_mw(str(tmp_path)) + os.unlink(os.path.join(mw, *NESTED_MODULE)) + assert revert_all(mw) == [] + + +class TestTargetsAreDisjoint: + def test_no_file_is_in_both_module_lists(self): + # If restic.py ever appeared in NESTED_RELPATHS, revert_nested would break + # B2 backups. + assert not set(NESTED_RELPATHS) & set(PROVIDER_RELPATHS) + + @pytest.mark.parametrize("rel", PROVIDER_RELPATHS) + def test_provider_targets_are_not_nested_targets(self, rel): + assert rel not in NESTED_RELPATHS diff --git a/uninstall.sh b/uninstall.sh index 5927134..bb5a7a7 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -3,7 +3,7 @@ set -euo pipefail -VERSION="0.3.3" +VERSION="0.3.4" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _HOOK_COMMENT='TrueCloud provider patch (S3/B2)' @@ -98,54 +98,11 @@ echo "" # would then remove the boot hook and report success while leaving every patch # applied. Strip our appended blocks explicitly. +# Same implementation apply.sh uses (patch/mw_patch.py) — a second shell copy of +# this would be the untested one. 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 +python3 "$PATCH_DIR/patch/mw_patch.py" revert-all || \ + echo " WARNING: could not revert file-level patches." echo "" # ── Unmount nested-snapshot staging trees ─────────────────────────────────────