From eca61cb2a7cc184bd1a65a47f8e060de769a870e Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Mon, 24 Aug 2026 04:55:59 -0400 Subject: [PATCH] =?UTF-8?q?[FIX]=20db=5Frestore:=20la=20sonde=20du=20mot?= =?UTF-8?q?=20de=20passe=20ma=C3=AEtre=20ne=20validait=20rien?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Elle interrogeait `db --list`, qui ne LIT jamais le mot de passe : mesuré, `MASTER_PWD="ceci_est_faux" odoo-bin db --list` sort en 0. La boucle des dix essais acceptait donc le premier mot saisi, juste ou faux, et le refus n'arrivait qu'au `--restore` — une fois la base déjà supprimée. C'était exactement ce que cette boucle devait éviter. Seule l'action `drop` consulte le secret, et elle le fait AVANT de regarder la base : `check_super` d'abord, `db_exists` ensuite. Sur un nom tiré d'un uuid4, elle répond sans rien toucher. Mesuré : mauvais mot de passe → code 1 et AccessDenied ; bon → code 0 ; huit bases avant, huit après. --- EN --- It probed `db --list`, which never READS the master password: measured, `MASTER_PWD="ceci_est_faux" odoo-bin db --list` exits 0. The ten-attempt loop therefore accepted the first password typed, right or wrong, and the refusal only came at `--restore` — once the database was already dropped. Precisely what that loop existed to avoid. Only the `drop` action reads the secret, and it does so BEFORE looking at the database: `check_super` first, `db_exists` after. On a uuid4 name it answers without touching anything. Measured: wrong password → exit 1 and AccessDenied; right → exit 0; eight databases before, eight after. Assisted-by: Claude Opus 5 --- script/database/db_restore.py | 35 +++++++++--- test/test_master_password_retry.py | 92 +++++++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 16 deletions(-) diff --git a/script/database/db_restore.py b/script/database/db_restore.py index a394f4f..60c4300 100755 --- a/script/database/db_restore.py +++ b/script/database/db_restore.py @@ -8,8 +8,9 @@ import getpass import logging import os import shutil -import sys import subprocess +import sys +import uuid from subprocess import check_output sys.path.append( @@ -104,12 +105,32 @@ def password_refused(sortie): return "AccessDenied" in (sortie or "") -def probe_master_password(arg_base, mot): - """(accepté, sortie) — éprouver le mot de passe sur `--list`. +def probe_name(): + """Un nom de base qui ne peut appartenir à personne. - La commande la plus inoffensive : elle ne touche à rien et rend le - même refus qu'une restauration. Valider ici évite d'échouer à - mi-parcours, une fois la base déjà supprimée. + La sonde DEMANDE une suppression : si le nom désignait une vraie + base et que le mot de passe était bon, on la perdrait. Un uuid4 rend + la collision impossible en pratique, et le préfixe dit d'où il vient + à qui le verrait passer dans un journal. + """ + return f"el_probe_{uuid.uuid4().hex}" + + +def probe_master_password(arg_base, mot): + """(accepté, sortie) — éprouver le mot de passe pour de vrai. + + `--list` ne lit JAMAIS le mot de passe : mesuré, + `MASTER_PWD="ceci_est_faux" odoo-bin db --list` sort en 0. La sonde + d'avant acceptait donc le premier mot saisi, juste ou faux, et la + boucle des dix essais ne servait à rien — le refus n'arrivait qu'au + `--restore`, une fois la base déjà supprimée. + + Seule l'action `drop` consulte le mot de passe, et elle le fait + AVANT de regarder la base : `check_super` d'abord, `db_exists` + ensuite. Sur un nom qui n'existe pas, elle ne touche donc rien et + répond quand même. Mesuré sur la machine d'essai : mauvais mot de + passe → code 1 et `AccessDenied` dans la trace ; bon mot de passe → + code 0, silence, et les huit bases toujours là. Le secret passe par l'environnement, jamais par argv : /proc//cmdline est lisible par tout utilisateur de la machine. @@ -117,7 +138,7 @@ def probe_master_password(arg_base, mot): env = os.environ.copy() env["MASTER_PWD"] = mot done = subprocess.run( - f"{arg_base} --list".split(" "), + f"{arg_base} --drop --database {probe_name()}".split(" "), capture_output=True, text=True, env=env, diff --git a/test/test_master_password_retry.py b/test/test_master_password_retry.py index b6fd3e3..808b644 100644 --- a/test/test_master_password_retry.py +++ b/test/test_master_password_retry.py @@ -131,8 +131,10 @@ class TestTheRetryLoop(unittest.TestCase): self.assertIn("OperationalError", "\n".join(journal.output)) def test_the_probe_carries_the_password_and_touches_nothing(self): - # `--list` ne modifie rien : valider ici évite d'échouer à - # mi-parcours, une fois la base déjà supprimée. + # Valider ici évite d'échouer à mi-parcours, une fois la base + # déjà supprimée. La sonde demande un `drop` sur un nom qui + # n'existe pas : `check_super` s'exécute AVANT `db_exists`, donc + # elle répond sans rien toucher. self.branche(["bon"], [(True, "db1")]) self.lance() self.assertEqual(len(self.sondes), 1) @@ -164,18 +166,90 @@ class TestTheWiring(unittest.TestCase): src = self.source() self.assertIn("master_password = ask_master_password(arg_base)", src) - def test_the_probe_uses_list_which_changes_nothing(self): - src = self.source() - debut = src.index("def probe_master_password") - fin = src.index("def ask_master_password") - bloc = src[debut:fin] - self.assertIn("--list", bloc) - for danger in ("--drop", "--restore", "--clone"): + def bloc_sonde(self): + """Le CODE de la sonde, docstring retirée. + + La docstring EXPLIQUE pourquoi `--list` et `--restore` ne + conviennent pas : un test qui lit tout le texte les y trouve et + se croit trompé. Troisième fois que ce piège se referme sur moi + dans ce dépôt — voir tasks/lessons.md. + """ + import ast + + for noeud in ast.walk(ast.parse(self.source())): + if ( + isinstance(noeud, ast.FunctionDef) + and noeud.name == "probe_master_password" + ): + sans_texte = [ + n + for n in noeud.body + if not ( + isinstance(n, ast.Expr) + and isinstance(n.value, ast.Constant) + and isinstance(n.value.value, str) + ) + ] + return ast.dump(ast.Module(body=sans_texte, type_ignores=[])) + return "" + + def test_the_scan_finds_the_probe_at_all(self): + # Sans cette borne, les tests suivants passeraient sur une chaîne + # vide le jour où la fonction est renommée. + self.assertTrue(self.bloc_sonde()) + + def test_the_probe_uses_the_only_action_that_reads_the_password(self): + # `--list` ne LIT jamais le mot de passe : mesuré, + # MASTER_PWD="ceci_est_faux" … db --list sort en 0. La sonde + # d'avant acceptait donc le premier mot saisi, et la boucle des + # dix essais ne servait à rien. Seul `drop` consulte le secret. + bloc = self.bloc_sonde() + self.assertIn("--drop", bloc) + self.assertNotIn("--list", bloc) + + def test_it_never_names_a_database_that_could_exist(self): + # La sonde DEMANDE une suppression : sur un vrai nom et avec un + # bon mot de passe, on perdrait la base. Le nom vient d'un uuid. + bloc = self.bloc_sonde() + # L'arbre nomme les appels : `Call(func=Name(id='probe_name'))`. + self.assertIn("id='probe_name'", bloc) + # …et jamais la base que l'appelant vise. + self.assertNotIn("id='database'", bloc) + + def test_it_never_asks_to_restore_or_clone(self): + bloc = self.bloc_sonde() + for danger in ("--restore", "--clone"): self.assertNotIn(danger, bloc) def test_the_bound_is_ten(self): self.assertEqual(db_restore.MAX_ESSAIS_MOT_DE_PASSE, 10) +class TestTheProbeName(unittest.TestCase): + """Le nom que la sonde demande de supprimer. + + C'est le seul endroit de cet outil où une erreur coûterait une base. + """ + + def test_it_is_prefixed_so_a_log_says_where_it_came_from(self): + self.assertTrue(db_restore.probe_name().startswith("el_probe_")) + + def test_two_calls_never_give_the_same_name(self): + noms = {db_restore.probe_name() for _ in range(50)} + self.assertEqual(50, len(noms)) + + def test_it_is_long_enough_to_be_unguessable(self): + # Un uuid4 en hexadécimal : 32 caractères après le préfixe. + self.assertGreaterEqual( + len(db_restore.probe_name()), len("el_probe_") + 32 + ) + + def test_it_is_a_legal_postgresql_identifier(self): + # Un nom que PostgreSQL refuserait ferait échouer la sonde pour + # une raison qui n'a rien à voir avec le mot de passe, et l'on + # redemanderait dix fois un secret parfaitement bon. + self.assertRegex(db_restore.probe_name(), r"^[a-z][a-z0-9_]{0,62}$") + + if __name__ == "__main__": unittest.main()