Tighten the bootstrap newline comments

This commit is contained in:
danielhanchen 2026-07-29 07:45:36 +00:00
commit e573a6d9a6
4 changed files with 33 additions and 43 deletions

View file

@ -32,20 +32,18 @@ _bootstrap_password: Optional[str] = None
def _bootstrap_file_bytes(password: str) -> bytes:
"""The exact on-disk form: the secret plus one LF.
"""Exact on-disk form: the secret plus one LF.
Encoded rather than written as text because text mode translates "\\n" to
CRLF on Windows, and `$(cat ...)` strips the LF but leaves the CR attached
to the credential.
Bytes, not text: text mode writes CRLF on Windows, and `$(cat ...)` strips
the LF but leaves the CR attached to the credential.
"""
return (password + "\n").encode("utf-8")
def _persist_bootstrap_password(password: str) -> None:
"""Write the bootstrap password 0600, with a trailing LF on every OS.
"""Atomically write the bootstrap password 0600, LF terminated on every OS.
Atomic: this can rewrite a live file, and a partial write would destroy the
only plaintext copy of the recovery credential.
A partial write would destroy the only plaintext recovery credential.
"""
fd, tmp_name = tempfile.mkstemp(
prefix = f".{_BOOTSTRAP_PW_PATH.name}.", dir = _BOOTSTRAP_PW_PATH.parent
@ -69,22 +67,20 @@ def _persist_bootstrap_password(password: str) -> None:
def _normalise_bootstrap_file(raw: bytes, password: str) -> None:
"""Append the LF a pre-newline release left off.
Append-only, and only to a file that is exactly the credential. Rewriting
could restore revoked plaintext, because clear_bootstrap_password() may
unlink or (when unlink fails, notably on Windows while this descriptor is
open) truncate the file through another descriptor at any point after we
read it. Appending cannot: the worst case is a lone "\\n" over a cleared
file, which strips to empty and reads back as no bootstrap password.
Releases before the newline wrote the password with no terminator at all,
so that is the only shape in the wild. Anything else is left alone and
keeps working, since every reader strips.
Append-only, and only when the file is exactly the credential:
clear_bootstrap_password() may unlink or (when unlink fails, notably on
Windows while this descriptor is open) truncate through another descriptor
after we read, so a rewrite could restore revoked plaintext. An append
cannot: worst case is a lone "\\n" over a cleared file, which strips back to
no bootstrap password. Pre-newline releases wrote no terminator at all, so
that is the only shape in the wild; anything else reads fine, since every
reader strips, and is left alone.
"""
if raw != password.encode("utf-8"):
return
# O_BINARY or Windows opens the descriptor in text mode and turns the LF
# straight back into CRLF, which is the bug this exists to fix.
# O_BINARY: without it Windows opens in text mode and turns the LF straight
# back into CRLF, the bug being fixed.
fd = os.open(
_BOOTSTRAP_PW_PATH,
os.O_WRONLY | os.O_APPEND | getattr(os, "O_BINARY", 0),
@ -106,8 +102,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]:
return None
# No caller handles a raise, so an unreadable file has to mean "no bootstrap
# password", not a dead backend. We write UTF-8, so bytes that will not
# decode are damage whose plaintext is worthless anyway.
# password", not a dead backend. We write UTF-8, so undecodable bytes are
# damage whose plaintext is worthless anyway.
try:
raw = _BOOTSTRAP_PW_PATH.read_bytes()
password = raw.decode("utf-8").strip()
@ -116,8 +112,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]:
if not password:
return None
# Older releases wrote no terminator, so upgrades kept the `cat` problem.
# Best-effort: a read-only auth dir must not fail startup.
# Older releases wrote no terminator; best-effort, a read-only auth dir must
# not fail startup.
if raw != _bootstrap_file_bytes(password):
try:
_normalise_bootstrap_file(raw, password)
@ -166,9 +162,9 @@ def get_bootstrap_password() -> Optional[str]:
def _load_bootstrap_password() -> Optional[str]:
"""Load an existing bootstrap password without creating one.
This, not generate_bootstrap_password(), is the path an upgraded install
takes (ensure_default_admin short-circuits once the admin row exists), so
the normalisation has to happen here too.
Upgrades take this path, not generate_bootstrap_password()
(ensure_default_admin short-circuits once the admin row exists), so it has
to normalise too.
"""
global _bootstrap_password
_bootstrap_password = _read_persisted_bootstrap_password()

View file

@ -135,10 +135,10 @@ def test_ensure_default_admin_loads_existing_bootstrap_after_restart(monkeypatch
def test_bootstrap_password_file_ends_with_a_newline():
# Otherwise `cat` welds the passphrase to the shell prompt and both get copied.
# Otherwise `cat` welds the passphrase onto the shell prompt.
storage.ensure_default_admin()
# Bytes, not read_text: that decodes CRLF back to "\n" and hides a CR.
# Bytes: read_text would decode CRLF back to "\n" and hide a CR.
raw = storage._BOOTSTRAP_PW_PATH.read_bytes()
assert raw == storage.get_bootstrap_password().encode("utf-8") + b"\n"
@ -154,8 +154,7 @@ def test_bootstrap_password_round_trips_across_a_restart_with_the_newline():
def test_upgrade_normalises_the_bootstrap_file():
# The upgrade path is ensure_default_admin() on an install that already has
# the admin row, which never reaches generate_bootstrap_password().
# Upgrade path: the admin row exists, so generate_bootstrap_password() never runs.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
@ -174,8 +173,7 @@ def test_upgrade_normalises_the_bootstrap_file():
],
)
def test_only_an_exactly_unterminated_bootstrap_file_is_touched(other):
# Appending is safe precisely because it is restricted to the one shape
# released code produced. Everything else reads fine and is left alone.
# Appending is safe only because it is restricted to the one released shape.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(other)
@ -222,8 +220,7 @@ def test_migration_failure_does_not_break_startup(monkeypatch):
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.
# A rename would resurrect revoked plaintext if the password changed after the read.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
@ -263,8 +260,7 @@ def test_normalising_does_not_overwrite_a_rotated_bootstrap_file(monkeypatch):
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.
# An in-place rewrite is not atomic, so only the exact unterminated shape is touched.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(b" legacy-bootstrap-secret ")
@ -275,8 +271,7 @@ def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch):
def test_normalising_opens_the_file_in_binary_mode(monkeypatch):
# Without O_BINARY, Windows opens the descriptor in text mode and os.write
# turns the LF back into CRLF, reintroducing the bug being fixed.
# Without O_BINARY, Windows text mode turns the written LF back into CRLF.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
monkeypatch.setattr(storage.os, "O_BINARY", 0x8000, raising = False)
@ -296,9 +291,8 @@ def test_normalising_opens_the_file_in_binary_mode(monkeypatch):
def test_clearing_by_truncation_mid_normalisation_is_not_undone(monkeypatch):
# clear_bootstrap_password() truncates through its own descriptor when the
# unlink fails, which is what happens on Windows while ours is open. The
# append must not put the revoked plaintext back.
# clear_bootstrap_password() truncates through its own descriptor when the unlink
# fails (Windows, while ours is open); the append must not restore the plaintext.
seed_user()
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")

View file

@ -483,7 +483,7 @@ def _write_auth_secret(path: Path, secret: str) -> None:
os.chmod(tmp_path, 0o600)
except OSError:
pass
# newline pins LF: text mode would write CRLF on Windows, and `$(cat ...)`
# newline pins LF: text mode writes CRLF on Windows, and `$(cat ...)`
# strips the LF but leaves the CR glued to the credential.
with os.fdopen(fd, "w", encoding = "utf-8", newline = "\n") as f:
fd = -1

View file

@ -1211,7 +1211,7 @@ def test_write_auth_secret_terminates_the_file_with_a_newline(monkeypatch, tmp_p
studio_mod._write_auth_secret(path, "desktop-abc123")
# Bytes, not read_text: that decodes CRLF back to "\n" and hides a CR.
# Bytes: read_text would decode CRLF back to "\n" and hide a CR.
assert path.read_bytes() == b"desktop-abc123\n"