mirror of
https://github.com/kevinveenbirkenbach/docker-volume-backup.git
synced 2026-11-06 02:57:59 +00:00
fix(security): keep database passwords out of every argv
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
d672388f63
commit
328520169b
15 files changed
+320
-67
No files matched your search
@@ -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):
|
||||
|
||||
@@ -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__":
|
||||
|
||||
@@ -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()
|
||||
@@ -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()
|
||||
Reference in new issue
Block a user