Compare commits

..
1 Commits
Author SHA1 Message Date
flan 60b3ac4557 v0.3.3: keep the restic repo password out of argv and shell history
Security
--------
create_task.py shelled out to `midclt call cloud_backup.create '<json>'`, and that
JSON carries the restic repository password -- so it sat in the subprocess'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 this process's memory. Verified on a
live box: list-tasks and list-credentials work through the new transport.

--password is also no longer required, because 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 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. This also covers the case where the
overlay unmount fails.

create_task.py's __version__ had been stuck at 0.2.0 through 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.

118 tests, ruff and shellcheck clean.
2026-07-13 15:43:24 +00:00
8 changed files with 277 additions and 31 deletions
+32
View File
@@ -1,5 +1,37 @@
# Changelog # 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 '<json>'`, 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 ## v0.3.2 — 2026-07-13
### Fixed ### Fixed
+1 -1
View File
@@ -18,7 +18,7 @@
set -euo pipefail set -euo pipefail
VERSION="0.3.2" VERSION="0.3.3"
# The directory containing install.sh is the permanent install location. # The directory containing install.sh is the permanent install location.
PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
+1 -1
View File
@@ -32,7 +32,7 @@
# Derive PATCH_DIR from this script's location (parent of the patch/ directory). # Derive PATCH_DIR from this script's location (parent of the patch/ directory).
PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)" PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)"
LOG="$PATCH_DIR/apply.log" 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. # 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. # Keep two prior generations (.1 and .2) so the last three boots are always available.
+70 -23
View File
@@ -26,8 +26,9 @@ Create a task backed by a B2 credential (id=3):
--credential 3 \\ --credential 3 \\
--bucket my-bucket \\ --bucket my-bucket \\
--folder backups/tank \\ --folder backups/tank \\
--password "restic-repo-password" \\ --password-stdin \\
--keep-last 14 --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.): Create a task using an S3-compatible credential (Wasabi, R2, etc.):
python3 create_task.py create \\ python3 create_task.py create \\
@@ -36,7 +37,7 @@ Create a task using an S3-compatible credential (Wasabi, R2, etc.):
--credential 5 \\ --credential 5 \\
--bucket my-bucket \\ --bucket my-bucket \\
--folder backups \\ --folder backups \\
--password "restic-repo-password" --password-stdin
List existing TrueCloud Backup tasks: List existing TrueCloud Backup tasks:
python3 create_task.py list-tasks python3 create_task.py list-tasks
@@ -44,39 +45,49 @@ List existing TrueCloud Backup tasks:
import argparse import argparse
import calendar import calendar
import getpass
import json import json
import os import os
import subprocess import subprocess
import sys import sys
import time import time
__version__ = "0.2.0" __version__ = "0.3.3"
_PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
_STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json") _STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json")
def midclt_call(method, *args): def midclt_call(method, *args):
"""Call a middleware method locally via `midclt`, the supported JSON-RPC transport """Call a middleware method on the local host.
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). Uses `truenas_api_client` -- the library that backs `midclt` itself -- rather
Exits with a clear message on failure.""" than shelling out to `midclt`.
cmd = ["midclt", "call", method] + [json.dumps(a) for a in args]
This is a SECURITY requirement, not a style choice. `midclt call <method>
<json>` 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: try:
proc = subprocess.run(cmd, capture_output=True, text=True, timeout=120) from truenas_api_client import Client
except FileNotFoundError: except ImportError:
print("ERROR: `midclt` not found — run this script ON the TrueNAS host.", print(
file=sys.stderr) "ERROR: `truenas_api_client` not importable — run this script ON the\n"
" TrueNAS host. (It ships with midclt.)",
file=sys.stderr,
)
sys.exit(1) 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) 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 ────────────────────────────────────────────────────────────── # ── Sub-commands ──────────────────────────────────────────────────────────────
@@ -213,6 +224,37 @@ def cmd_list_tasks(_args):
print(f"{t['id']:>4} {enabled:<8} {ptype:<14} {t.get('description', '')}") 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 <secret>`
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): def cmd_create(args):
parts = args.schedule.split() parts = args.schedule.split()
if len(parts) != 5: if len(parts) != 5:
@@ -223,6 +265,8 @@ def cmd_create(args):
sys.exit(1) sys.exit(1)
minute, hour, dom, month, dow = parts minute, hour, dom, month, dow = parts
password = _resolve_password(args)
body = { body = {
"description": args.name, "description": args.name,
"path": args.path, "path": args.path,
@@ -231,7 +275,7 @@ def cmd_create(args):
"bucket": args.bucket, "bucket": args.bucket,
"folder": args.folder, "folder": args.folder,
}, },
"password": args.password, "password": password,
"keep_last": args.keep_last, "keep_last": args.keep_last,
"transfer_setting": args.transfer_setting, "transfer_setting": args.transfer_setting,
"schedule": { "schedule": {
@@ -297,8 +341,11 @@ def main():
help="Bucket (S3) or container (B2) name") help="Bucket (S3) or container (B2) name")
c.add_argument("--folder", default="", c.add_argument("--folder", default="",
help="Path within the bucket (default: root)") help="Path within the bucket (default: root)")
c.add_argument("--password", required=True, c.add_argument("--password", default=None,
help="Restic repository encryption password (choose a strong one)") 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", c.add_argument("--keep-last", type=int, default=14, metavar="N",
help="Snapshots to retain after each run (default: 14)") help="Snapshots to retain after each run (default: 14)")
c.add_argument("--schedule", default="0 2 * * *", c.add_argument("--schedule", default="0 2 * * *",
+1 -1
View File
@@ -17,7 +17,7 @@
# bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh # bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh
# systemctl restart middlewared # systemctl restart middlewared
VERSION="0.3.2" VERSION="0.3.3"
PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
+107
View File
@@ -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 '<json>'` -- 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)}"
+7 -4
View File
@@ -19,17 +19,20 @@ import sys
ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
CHANGELOG = os.path.join(ROOT, "CHANGELOG.md") CHANGELOG = os.path.join(ROOT, "CHANGELOG.md")
# Every script prints a version; they must all agree, and agree with the tag. # Everything that announces a version must agree with everything else. They drifted
# They drifted to three different values once (0.0.4 / 0.2.1) before anything # to three different values once (0.0.4 / 0.2.1) before anything checked them --
# 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 = [ VERSIONED_FILES = [
"install.sh", "install.sh",
"uninstall.sh", "uninstall.sh",
"recover.sh", "recover.sh",
os.path.join("patch", "apply.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) _HEADING_RE = re.compile(r"^##\s+v?(\d+\.\d+\.\d+[^\s]*)", re.M)
+58 -1
View File
@@ -3,7 +3,7 @@
set -euo pipefail set -euo pipefail
VERSION="0.3.2" VERSION="0.3.3"
PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)"
_HOOK_COMMENT='TrueCloud provider patch (S3/B2)' _HOOK_COMMENT='TrueCloud provider patch (S3/B2)'
@@ -91,6 +91,63 @@ if [ "$_ov_found" -eq 0 ]; then
fi fi
echo "" 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 ───────────────────────────────────── # ── Unmount nested-snapshot staging trees ─────────────────────────────────────
# These bind mounts pin their ZFS snapshots, so they must go before anything # These bind mounts pin their ZFS snapshots, so they must go before anything
# tries to destroy those snapshots. Deepest first. # tries to destroy those snapshots. Deepest first.