From 729cb51e440e2eab33dd423a3cd54e5879b478a1 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Wed, 29 Jul 2026 05:19:43 +0000 Subject: [PATCH] Normalise the bootstrap file in place so a cleared credential stays cleared The rename-based rewrite could recreate the file: if a password change ran clear_bootstrap_password(), or the CLI cleanup deleted it, between the read and the write, os.replace put the revoked plaintext back on disk, where a later auth.db reset would re-seed it. Open the existing file without O_CREAT instead, so a deleted file cannot be resurrected, and re-check the contents through that descriptor so an in-place truncation or a rotated credential is not overwritten either. That gives up the atomic rename, so the in-place path is restricted to trailing-whitespace fixes. Every partial state is then the secret plus leftover whitespace, which still strips to the same credential. Files with leading whitespace are left alone; every reader strips, so they keep working. Creation still goes through the atomic writer. --- studio/backend/auth/storage.py | 38 +++++++++++++-- studio/backend/tests/test_desktop_auth.py | 59 +++++++++++++++++++++-- 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/studio/backend/auth/storage.py b/studio/backend/auth/storage.py index d7e890593c..f05e71ac05 100644 --- a/studio/backend/auth/storage.py +++ b/studio/backend/auth/storage.py @@ -66,6 +66,38 @@ def _persist_bootstrap_password(password: str) -> None: raise +def _normalise_bootstrap_file(raw: bytes, password: str) -> None: + """Give an existing malformed file its trailing LF, in place. + + Deliberately not the atomic replace above: a rename would recreate the file + if clear_bootstrap_password() or the CLI cleanup deleted it after we read + it, putting revoked plaintext back on disk for a later reset to re-seed. + Opening without O_CREAT cannot resurrect a deleted file, and the contents + are re-checked through the descriptor so an in-place truncation is not + undone either. + """ + data = _bootstrap_file_bytes(password) + # Only a trailing-whitespace rewrite is safe without a rename: every partial + # state is then "\n" plus leftover whitespace, which still strips to + # the same credential. Anything else keeps working unnormalised. + if not raw.startswith(data[:-1]): + return + + fd = os.open(_BOOTSTRAP_PW_PATH, os.O_RDWR) + try: + if os.read(fd, len(raw) + 1) != raw: + return + os.lseek(fd, 0, os.SEEK_SET) + os.write(fd, data) + os.ftruncate(fd, len(data)) + try: + os.fchmod(fd, 0o600) + except OSError: + pass + finally: + os.close(fd) + + def _read_persisted_bootstrap_password() -> Optional[str]: """Read the persisted password, normalising the file if it is malformed.""" if not _BOOTSTRAP_PW_PATH.is_file(): @@ -83,11 +115,11 @@ def _read_persisted_bootstrap_password() -> Optional[str]: return None # Older releases wrote no terminator, and text mode wrote CRLF on Windows, - # so upgrades kept the `cat` problem. Rewrite anything that isn't exactly - # "\n". Best-effort: a read-only auth dir must not fail startup. + # so upgrades kept the `cat` problem. Best-effort: a read-only auth dir or a + # credential cleared underneath us must not fail startup. if raw != _bootstrap_file_bytes(password): try: - _persist_bootstrap_password(password) + _normalise_bootstrap_file(raw, password) except OSError: pass return password diff --git a/studio/backend/tests/test_desktop_auth.py b/studio/backend/tests/test_desktop_auth.py index 0920b8837c..72a2eeff3b 100644 --- a/studio/backend/tests/test_desktop_auth.py +++ b/studio/backend/tests/test_desktop_auth.py @@ -193,10 +193,14 @@ def test_migration_failure_does_not_break_startup(monkeypatch): seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") - def refuse(*args, **kwargs): - raise PermissionError("read-only auth dir") + real_open = storage.os.open - monkeypatch.setattr(storage.tempfile, "mkstemp", refuse) + def refuse(path, flags, *args, **kwargs): + if str(path) == str(storage._BOOTSTRAP_PW_PATH): + raise PermissionError("read-only auth dir") + return real_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(storage.os, "open", refuse) storage.ensure_default_admin() @@ -204,6 +208,55 @@ def test_migration_failure_does_not_break_startup(monkeypatch): assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret" +def test_normalising_never_recreates_a_cleared_bootstrap_file(monkeypatch): + # A rename would resurrect the file if the password changed after the read, + # leaving revoked plaintext for a later auth.db reset to re-seed. + seed_user() + storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") + + real_open = storage.os.open + + def clear_then_open(path, flags, *args, **kwargs): + if str(path) == str(storage._BOOTSTRAP_PW_PATH): + storage._BOOTSTRAP_PW_PATH.unlink(missing_ok = True) + return real_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(storage.os, "open", clear_then_open) + + assert storage._read_persisted_bootstrap_password() == "legacy-bootstrap-secret" + assert not storage._BOOTSTRAP_PW_PATH.exists() + + +def test_normalising_does_not_overwrite_a_rotated_bootstrap_file(monkeypatch): + seed_user() + storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") + + real_open = storage.os.open + + def rotate_then_open(path, flags, *args, **kwargs): + if str(path) == str(storage._BOOTSTRAP_PW_PATH): + storage._BOOTSTRAP_PW_PATH.write_bytes(b"brand-new-secret\n") + return real_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(storage.os, "open", rotate_then_open) + + storage._read_persisted_bootstrap_password() + + assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"brand-new-secret\n" + + +def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch): + # Only trailing whitespace is rewritten in place: without a rename there is + # no atomicity, and a partial rewrite here could mis-strip. + seed_user() + storage._BOOTSTRAP_PW_PATH.write_bytes(b" legacy-bootstrap-secret ") + + storage.ensure_default_admin() + + assert storage.get_bootstrap_password() == "legacy-bootstrap-secret" + assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b" legacy-bootstrap-secret " + + def test_persisting_the_bootstrap_password_is_atomic(monkeypatch, tmp_path): # A partial write would destroy the only plaintext recovery credential. storage._persist_bootstrap_password("original-secret")