From 328520169b8352f8403fc8bb2821d5331fd72cb6 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Thu, 5 Nov 2026 18:14:39 +0000 Subject: [PATCH] fix(security): keep database passwords out of every argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MariaDB dumps and restores read the password from MYSQL_PWD, and docker exec forwards env variables by name only, so no process listing shows a password. baudolo-restore gains --db-password-stdin; --db-password still works but warns. 🎯 Fix: 96 % 💥 Break: 3 % 💸 Cost: – ⏱️ Time: – 🛡️ Security: ~100 % 🏆 Score: 93 🚦 Gate: ✅ 90/5, cost and time <= 100, security 100 % Co-Authored-By: Claude Opus 5.5 --- README.md | 12 +- src/baudolo/backup/db.py | 3 +- src/baudolo/restore/__main__.py | 41 ++++++- src/baudolo/restore/db/mariadb.py | 8 +- src/baudolo/restore/db/version.py | 2 +- src/baudolo/restore/run.py | 45 ++++++- tests/e2e/helpers/process.py | 11 ++ tests/e2e/test_e2e_mariadb_full.py | 6 +- .../e2e/test_e2e_postgres_cluster_restore.py | 28 +---- tests/e2e/test_e2e_postgres_full.py | 5 +- tests/e2e/test_e2e_version_gate.py | 4 +- tests/unit/backup/test_db_mariadb_dump.py | 43 ++++--- tests/unit/backup/test_docker_exec_env.py | 2 +- tests/unit/restore/test_cli_password_stdin.py | 65 ++++++++++ tests/unit/restore/test_password_off_argv.py | 112 ++++++++++++++++++ 15 files changed, 320 insertions(+), 67 deletions(-) create mode 100644 tests/unit/restore/test_cli_password_stdin.py create mode 100644 tests/unit/restore/test_password_off_argv.py diff --git a/README.md b/README.md index b70e0f5..4ab9356 100644 --- a/README.md +++ b/README.md @@ -174,10 +174,14 @@ baudolo-restore postgres \ \ --container postgres \ --db-name appdb \ - --db-password secret \ - --empty + --db-password-stdin \ + --empty < password-file ``` +`--db-password-stdin` reads the password from stdin. The older +`--db-password ` still works but warns: it is visible in the process +list. + ### Restore MariaDB / MySQL ```bash @@ -187,8 +191,8 @@ baudolo-restore mariadb \ \ --container mariadb \ --db-name shopdb \ - --db-password secret \ - --empty + --db-password-stdin \ + --empty < password-file ``` > `baudolo` automatically detects whether `mariadb` or `mysql` diff --git a/src/baudolo/backup/db.py b/src/baudolo/backup/db.py index 1e5027f..d4ea281 100644 --- a/src/baudolo/backup/db.py +++ b/src/baudolo/backup/db.py @@ -132,11 +132,12 @@ def backup_database( "--protocol=tcp", "-u", user, - f"-p{password}", db_name, ], + forward_env=["MYSQL_PWD"], ), dump_file, + env={"MYSQL_PWD": password}, ) produced = True continue diff --git a/src/baudolo/restore/__main__.py b/src/baudolo/restore/__main__.py index 705f7b2..c9bfae5 100644 --- a/src/baudolo/restore/__main__.py +++ b/src/baudolo/restore/__main__.py @@ -10,6 +10,29 @@ from .files import restore_volume_files from .paths import BackupPaths +def _password(parser: argparse.ArgumentParser, args: argparse.Namespace) -> str: + """ + Args: + parser: reports a password given both ways or not at all. + args: the parsed engine subcommand. + + Returns: + The database password, from stdin or from the deprecated argument. + """ + if args.db_password_stdin == (args.db_password is not None): + parser.error( + "give the password either via --db-password-stdin or --db-password" + ) + if args.db_password_stdin: + return sys.stdin.read().removesuffix("\n") + print( + "WARNING: --db-password is visible in the process list; " + "use --db-password-stdin", + file=sys.stderr, + ) + return args.db_password + + def _add_common_backup_args(p: argparse.ArgumentParser) -> None: p.add_argument("volume_name", help="Docker volume name (target volume)") p.add_argument("backup_hash", help="Hashed machine id") @@ -29,7 +52,16 @@ def _add_common_backup_args(p: argparse.ArgumentParser) -> None: def _add_common_engine_args(p: argparse.ArgumentParser) -> None: p.add_argument("--container", required=True) - p.add_argument("--db-password", required=True) + p.add_argument( + "--db-password", + default=None, + help="Deprecated: visible in the process list; use --db-password-stdin", + ) + p.add_argument( + "--db-password-stdin", + action="store_true", + help="Read the database password from stdin", + ) p.add_argument("--empty", action="store_true") p.add_argument( "--no-version-check", @@ -91,6 +123,7 @@ def main(argv: list[str] | None = None) -> int: p_mdb.add_argument("--db-user", default=None, help="Defaults to db-name if omitted") args = parser.parse_args(argv) + password = None if args.cmd == "files" else _password(parser, args) try: if args.cmd == "files": @@ -115,7 +148,7 @@ def main(argv: list[str] | None = None) -> int: container=args.container, db_name=args.db_name, user=user, - password=args.db_password, + password=password, sql_path=BackupPaths( args.volume_name, args.backup_hash, @@ -132,7 +165,7 @@ def main(argv: list[str] | None = None) -> int: restore_cluster_sql( container=args.container, user=args.db_user, - password=args.db_password, + password=password, sql_path=BackupPaths( args.volume_name, args.backup_hash, @@ -151,7 +184,7 @@ def main(argv: list[str] | None = None) -> int: container=args.container, db_name=args.db_name, user=user, - password=args.db_password, + password=password, sql_path=BackupPaths( args.volume_name, args.backup_hash, diff --git a/src/baudolo/restore/db/mariadb.py b/src/baudolo/restore/db/mariadb.py index 26f4eb8..6f63277 100644 --- a/src/baudolo/restore/db/mariadb.py +++ b/src/baudolo/restore/db/mariadb.py @@ -47,6 +47,8 @@ def restore_mariadb_sql( if not Path(sql_path).is_file(): raise FileNotFoundError(sql_path) + docker_env = {"MYSQL_PWD": password} + if check_version: guard( sql_path=sql_path, @@ -65,12 +67,12 @@ def restore_mariadb_sql( client, "-u", user, - f"--password={password}", "-N", "-e", f"SELECT table_name FROM information_schema.tables WHERE table_schema = '{db_name}';", # noqa: S608 - validate_database() constrains the name to ^[a-zA-Z0-9_][a-zA-Z0-9_-]*$ ], capture=True, + docker_env=docker_env, ) tables = result.stdout.decode().split() @@ -90,15 +92,15 @@ def restore_mariadb_sql( client, "-u", user, - f"--password={password}", "-e", drop_sql, ], + docker_env=docker_env, ) with Path(sql_path).open("rb") as f: docker_exec( - container, [client, "-u", user, f"--password={password}", db_name], stdin=f + container, [client, "-u", user, db_name], stdin=f, docker_env=docker_env ) print(f"MariaDB/MySQL restore complete for db '{db_name}'.") diff --git a/src/baudolo/restore/db/version.py b/src/baudolo/restore/db/version.py index 9ad4172..19a7388 100644 --- a/src/baudolo/restore/db/version.py +++ b/src/baudolo/restore/db/version.py @@ -102,13 +102,13 @@ def server_version( client or "mariadb", "-u", user, - f"--password={password}", "-N", "-B", "-e", "SELECT VERSION()", ], capture=True, + docker_env={"MYSQL_PWD": password}, ) ) diff --git a/src/baudolo/restore/run.py b/src/baudolo/restore/run.py index dcc2ac5..105d353 100644 --- a/src/baudolo/restore/run.py +++ b/src/baudolo/restore/run.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import subprocess import sys @@ -46,6 +47,38 @@ def stdout_of(completed: subprocess.CompletedProcess) -> str: return (raw.decode() if isinstance(raw, bytes) else raw).strip() +def docker_exec_cmd( + container: str, argv: list[str], docker_env: dict[str, str] | None +) -> list[str]: + """ + Args: + container: the container to run in. + argv: the command, already split. + docker_env: variables to hand to the container; only their names + reach the returned argv. + + Returns: + The ``docker exec -i`` argv. + """ + forwarded = [arg for name in docker_env or {} for arg in ("-e", name)] + return ["docker", "exec", "-i", *forwarded, container, *argv] + + +def child_env(env: dict | None, docker_env: dict[str, str] | None) -> dict | None: + """ + Args: + env: the docker CLI's environment, or None to inherit this process's. + docker_env: variables whose values the docker CLI copies into the + container for a bare ``-e NAME``. + + Returns: + The environment for the docker CLI, or None to inherit unchanged. + """ + if not docker_env: + return env + return {**(os.environ if env is None else env), **docker_env} + + def docker_exec( container: str, argv: list[str], @@ -55,12 +88,12 @@ def docker_exec( env: dict | None = None, docker_env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess: - cmd: list[str] = ["docker", "exec", "-i"] - if docker_env: - for k, v in docker_env.items(): - cmd.extend(["-e", f"{k}={v}"]) - cmd.extend([container, *argv]) - return run(cmd, stdin=stdin, capture=capture, env=env) + return run( + docker_exec_cmd(container, argv, docker_env), + stdin=stdin, + capture=capture, + env=child_env(env, docker_env), + ) def docker_exec_sh( diff --git a/tests/e2e/helpers/process.py b/tests/e2e/helpers/process.py index 80eb929..ae31480 100644 --- a/tests/e2e/helpers/process.py +++ b/tests/e2e/helpers/process.py @@ -13,7 +13,17 @@ def run( capture: bool = True, check: bool = True, cwd: str | None = None, + stdin_text: str | None = None, ) -> subprocess.CompletedProcess: + """ + Args: + cmd: argv, the program first. + capture: keep stdout and stderr on the result. + check: raise on a non-zero exit. + cwd: working directory. + stdin_text: text fed to the command's stdin, e.g. a password for + ``--db-password-stdin``. + """ try: return subprocess.run( cmd, @@ -21,6 +31,7 @@ def run( cwd=cwd, text=True, capture_output=capture, + input=stdin_text, ) except subprocess.CalledProcessError as e: # Print captured output so failing E2E tests are "live" / debuggable in CI logs diff --git a/tests/e2e/test_e2e_mariadb_full.py b/tests/e2e/test_e2e_mariadb_full.py index edab656..abf09af 100644 --- a/tests/e2e/test_e2e_mariadb_full.py +++ b/tests/e2e/test_e2e_mariadb_full.py @@ -137,10 +137,10 @@ class TestE2EMariaDBFull(unittest.TestCase): cls.db_name, "--db-user", cls.db_user, - "--db-password", - cls.db_password, + "--db-password-stdin", "--empty", - ] + ], + stdin_text=cls.db_password, ) @classmethod diff --git a/tests/e2e/test_e2e_postgres_cluster_restore.py b/tests/e2e/test_e2e_postgres_cluster_restore.py index c011cac..5aa264e 100644 --- a/tests/e2e/test_e2e_postgres_cluster_restore.py +++ b/tests/e2e/test_e2e_postgres_cluster_restore.py @@ -89,33 +89,12 @@ class TestE2EPostgresClusterRestore(unittest.TestCase): / f"{cls.pg_container}.cluster.backup.sql" ) - run( - [ - "baudolo-restore", - "cluster", - cls.pg_volume, - cls.hash, - cls.version, - "--backups-dir", - cls.backups_dir, - "--repo-name", - cls.repo_name, - "--container", - cls.pg_container, - "--instance", - cls.pg_container, - "--db-user", - "postgres", - "--db-password", - "pgpw", - "--empty", - ] - ) + run(cls._restore_argv(), stdin_text="pgpw") for statement in SIBLING_SQL: cls._psql("postgres", statement) cls._psql("sibling", SIBLING_PAYLOAD) - cls.refused = run(cls._restore_argv(), check=False) + cls.refused = run(cls._restore_argv(), check=False, stdin_text="pgpw") @classmethod def _restore_argv(cls) -> list: @@ -135,8 +114,7 @@ class TestE2EPostgresClusterRestore(unittest.TestCase): cls.pg_container, "--db-user", "postgres", - "--db-password", - "pgpw", + "--db-password-stdin", "--empty", ] diff --git a/tests/e2e/test_e2e_postgres_full.py b/tests/e2e/test_e2e_postgres_full.py index 9d15104..ede27a2 100644 --- a/tests/e2e/test_e2e_postgres_full.py +++ b/tests/e2e/test_e2e_postgres_full.py @@ -94,7 +94,7 @@ class TestE2EPostgresFull(unittest.TestCase): ) # Restore - run( + cls.restored = run( [ "baudolo-restore", "postgres", @@ -129,6 +129,9 @@ class TestE2EPostgresFull(unittest.TestCase): ) self.assertTrue(p.is_file(), f"Expected dump file at: {p}") + def test_the_deprecated_password_argument_still_restores_but_warns(self) -> None: + self.assertIn("visible in the process list", self.restored.stderr) + def test_data_restored(self) -> None: p = run( [ diff --git a/tests/e2e/test_e2e_version_gate.py b/tests/e2e/test_e2e_version_gate.py index 5211b3f..a4692b7 100644 --- a/tests/e2e/test_e2e_version_gate.py +++ b/tests/e2e/test_e2e_version_gate.py @@ -73,12 +73,12 @@ class GateCase: cls.db_name, "--db-user", cls.db_user, - "--db-password", - cls.db_password, + "--db-password-stdin", "--empty", *extra, ], check=False, + stdin_text=cls.db_password, ) @classmethod diff --git a/tests/unit/backup/test_db_mariadb_dump.py b/tests/unit/backup/test_db_mariadb_dump.py index d485596..597b727 100644 --- a/tests/unit/backup/test_db_mariadb_dump.py +++ b/tests/unit/backup/test_db_mariadb_dump.py @@ -16,7 +16,7 @@ def _capture_dumps(*, db_type, rows, container, dump_tool="mariadb-dump"): captured = [] def _capture(command, out_file, *, env=None): - captured.append((list(command), env)) + captured.append((list(command), env, out_file)) with ( tempfile.TemporaryDirectory() as td, @@ -46,14 +46,13 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): ) self.assertEqual(len(captured), 1, f"expected one dump, got: {captured}") - argv, env = captured[0] - self.assertEqual(argv[:3], ["docker", "exec", "mariadb"]) + argv, _env, _out = captured[0] + self.assertEqual(argv[:2], ["docker", "exec"]) + self.assertEqual(argv[argv.index(dump_tool_of(argv)) - 1], "mariadb") self.assertIn("--protocol=tcp", argv) self.assertEqual(argv[argv.index("-h") + 1], "127.0.0.1") self.assertEqual(argv[argv.index("-u") + 1], "appuser") - self.assertIn("-ps3cret", argv) self.assertEqual(argv[-1], "appdb") - self.assertIsNone(env) def test_the_probed_client_is_the_one_invoked(self): captured = _capture_dumps( @@ -62,7 +61,7 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): container="mariadb", dump_tool="mysqldump", ) - argv, _env = captured[0] + argv, _env, _out = captured[0] self.assertIn("mysqldump", argv) self.assertNotIn("mariadb-dump", argv) @@ -72,20 +71,32 @@ class TestMariaDBDumpUsesTCP(unittest.TestCase): rows=[("pg", "appdb", "appuser", "s3cret")], container="pg", ) - argv, _env = captured[0] + argv, _env, _out = captured[0] self.assertIn("pg_dump", argv) self.assertNotIn("--protocol=tcp", argv) def test_the_password_travels_in_the_environment_not_the_argv(self): - """A process listing shows argv; PGPASSWORD must not be in it.""" - captured = _capture_dumps( - db_type="postgres", - rows=[("pg", "appdb", "appuser", "s3cret")], - container="pg", - ) - argv, env = captured[0] - self.assertEqual(env, {"PGPASSWORD": "s3cret"}) - self.assertNotIn("s3cret", argv) + for db_type, container, variable in ( + ("postgres", "pg", "PGPASSWORD"), + ("mariadb", "mariadb", "MYSQL_PWD"), + ): + with self.subTest(engine=db_type): + argv, env, _out = _capture_dumps( + db_type=db_type, + rows=[(container, "appdb", "appuser", "s3cret")], + container=container, + )[0] + self.assertEqual(env, {variable: "s3cret"}) + self.assertEqual(argv[argv.index("-e") + 1], variable) + self.assertLess(argv.index("-e"), argv.index(container)) + self.assertFalse( + [arg for arg in argv if "s3cret" in arg], + f"password reached the argv: {argv}", + ) + + +def dump_tool_of(argv): + return next(arg for arg in argv if arg in ("mariadb-dump", "mysqldump")) class TestNoShellReachesTheDump(unittest.TestCase): diff --git a/tests/unit/backup/test_docker_exec_env.py b/tests/unit/backup/test_docker_exec_env.py index 15d7077..289ab0c 100644 --- a/tests/unit/backup/test_docker_exec_env.py +++ b/tests/unit/backup/test_docker_exec_env.py @@ -54,7 +54,7 @@ class TestPostgresDumpCarriesThePassword(unittest.TestCase): self.assertIn("-e", seen["command"]) self.assertEqual(seen["command"][seen["command"].index("-e") + 1], "PGPASSWORD") self.assertEqual(seen["env"], {"PGPASSWORD": "secret"}) - self.assertNotIn("secret", seen["command"]) + self.assertFalse([arg for arg in seen["command"] if "secret" in arg]) if __name__ == "__main__": diff --git a/tests/unit/restore/test_cli_password_stdin.py b/tests/unit/restore/test_cli_password_stdin.py new file mode 100644 index 0000000..080733a --- /dev/null +++ b/tests/unit/restore/test_cli_password_stdin.py @@ -0,0 +1,65 @@ +import contextlib +import io +import unittest +from unittest.mock import patch + +from baudolo.restore import __main__ as cli + +BASE = [ + "postgres", + "app_vol", + "hash", + "20260817000000", + "--repo-name", + "repo", + "--container", + "db", + "--db-name", + "app", +] + + +class TestRestorePasswordChannel(unittest.TestCase): + def call(self, extra, stdin=""): + stderr = io.StringIO() + with ( + patch.object(cli, "restore_postgres_sql") as restore, + patch("sys.stdin", io.StringIO(stdin)), + contextlib.redirect_stderr(stderr), + ): + code = cli.main([*BASE, *extra]) + return code, restore, stderr.getvalue() + + def test_stdin_carries_the_password_without_its_newline(self): + code, restore, stderr = self.call(["--db-password-stdin"], stdin="s3cret\n") + self.assertEqual(code, 0) + self.assertEqual(restore.call_args.kwargs["password"], "s3cret") + self.assertNotIn("WARNING", stderr) + + def test_the_argument_still_works_but_warns(self): + code, restore, stderr = self.call(["--db-password", "s3cret"]) + self.assertEqual(code, 0) + self.assertEqual(restore.call_args.kwargs["password"], "s3cret") + self.assertIn("visible in the process list", stderr) + + def test_both_ways_is_a_usage_error(self): + with self.assertRaises(SystemExit) as caught: + self.call(["--db-password", "x", "--db-password-stdin"], stdin="y") + self.assertEqual(caught.exception.code, 2) + + def test_neither_way_is_a_usage_error(self): + with self.assertRaises(SystemExit) as caught: + self.call([]) + self.assertEqual(caught.exception.code, 2) + + def test_files_needs_no_password(self): + with patch.object(cli, "restore_volume_files", return_value=0) as restore: + code = cli.main( + ["files", "app_vol", "hash", "20260817000000", "--repo-name", "repo"] + ) + self.assertEqual(code, 0) + restore.assert_called_once() + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/restore/test_password_off_argv.py b/tests/unit/restore/test_password_off_argv.py new file mode 100644 index 0000000..c5b3cea --- /dev/null +++ b/tests/unit/restore/test_password_off_argv.py @@ -0,0 +1,112 @@ +import tempfile +import unittest +from pathlib import Path +from unittest.mock import MagicMock, patch + +from baudolo.restore import run as run_mod +from baudolo.restore.db import cluster as cluster_mod +from baudolo.restore.db import mariadb as mdb_mod +from baudolo.restore.db import postgres as pg_mod +from baudolo.restore.db import version as ver + +from .test_version_gate import MARIADB_HEADER, POSTGRES_HEADER, cluster_header + +SECRET = "s3cret-Pw" + + +class TestDockerExecForwardsByName(unittest.TestCase): + def test_the_value_stays_out_of_the_argv(self): + with patch.object(run_mod, "run") as run: + run_mod.docker_exec("db", ["psql"], docker_env={"PGPASSWORD": SECRET}) + cmd = run.call_args.args[0] + self.assertEqual( + cmd, ["docker", "exec", "-i", "-e", "PGPASSWORD", "db", "psql"] + ) + self.assertFalse([arg for arg in cmd if SECRET in arg]) + self.assertEqual(run.call_args.kwargs["env"]["PGPASSWORD"], SECRET) + + def test_the_rest_of_the_environment_is_kept(self): + with ( + patch.dict(run_mod.os.environ, {"PATH": "/x"}), + patch.object(run_mod, "run") as run, + ): + run_mod.docker_exec("db", ["psql"], docker_env={"PGPASSWORD": SECRET}) + self.assertEqual(run.call_args.kwargs["env"]["PATH"], "/x") + + def test_no_docker_env_inherits_unchanged(self): + with patch.object(run_mod, "run") as run: + run_mod.docker_exec("db", ["true"]) + self.assertIsNone(run.call_args.kwargs["env"]) + self.assertEqual(run.call_args.args[0], ["docker", "exec", "-i", "db", "true"]) + + +class TestNoRestoreArgvCarriesThePassword(unittest.TestCase): + def setUp(self): + self.argvs = [] + self.envs = [] + self.dir = Path(tempfile.mkdtemp()) + + def fake_run(cmd, *, stdin=None, capture=False, env=None): + self.argvs.append(cmd) + self.envs.append(env or {}) + stdout = b"17.11\n" if "SHOW server_version" in cmd else b"t1\n" + if "SELECT VERSION()" in cmd: + stdout = b"11.8.8-MariaDB\n" + return MagicMock(stdout=stdout) + + patch.object(run_mod, "run", side_effect=fake_run).start() + self.addCleanup(patch.stopall) + + def dump(self, name: str, text: str) -> str: + path = self.dir / name + path.write_text(text, encoding="utf-8") + return str(path) + + def assert_clean(self, variable: str): + self.assertTrue(self.argvs) + for cmd in self.argvs: + self.assertFalse([arg for arg in cmd if SECRET in arg], cmd) + self.assertTrue(all(env.get(variable) == SECRET for env in self.envs)) + + def test_postgres(self): + pg_mod.restore_postgres_sql( + container="db", + db_name="app", + user="app", + password=SECRET, + sql_path=self.dump("app.backup.sql", POSTGRES_HEADER), + empty=True, + ) + self.assert_clean("PGPASSWORD") + + def test_cluster(self): + with patch.object(cluster_mod, "assert_instance_matches_dump"): + cluster_mod.restore_cluster_sql( + container="db", + user="postgres", + password=SECRET, + sql_path=self.dump("c.cluster.backup.sql", cluster_header(roles=1)), + empty=True, + ) + self.assert_clean("PGPASSWORD") + + def test_mariadb(self): + with patch.object(mdb_mod, "_pick_client", return_value="mariadb"): + mdb_mod.restore_mariadb_sql( + container="db", + db_name="app", + user="app", + password=SECRET, + sql_path=self.dump("app.backup.sql", MARIADB_HEADER), + empty=True, + ) + self.assert_clean("MYSQL_PWD") + self.assertEqual(len(self.argvs), 4, "version, list, drop, replay") + + def test_mariadb_version_probe(self): + ver.server_version("db", "mariadb", "app", SECRET, client="mariadb") + self.assert_clean("MYSQL_PWD") + + +if __name__ == "__main__": + unittest.main()