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.
This commit is contained in:
@@ -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 '<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
|
||||
|
||||
### Fixed
|
||||
|
||||
+1
-1
@@ -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)"
|
||||
|
||||
+1
-1
@@ -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.
|
||||
|
||||
+70
-23
@@ -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 <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:
|
||||
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 <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):
|
||||
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 * * *",
|
||||
|
||||
+1
-1
@@ -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)"
|
||||
|
||||
|
||||
@@ -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)}"
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
+58
-1
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user