Make the bootstrap normalisation append-only
clear_bootstrap_password() falls back to truncating the file through its own descriptor when the unlink fails, which is what happens on Windows while this one is open. That truncation could land after the equality check and before the write, so the rewrite put the revoked plaintext back. Append a single LF instead, and only to a file that is exactly the credential. An append cannot restore a revoked secret: over a cleared file the result is a lone newline, 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 because every reader strips. Never truncating also removes the short-write NUL-fill hazard entirely, so the write loop is gone. O_BINARY stays: without it Windows would turn the appended LF into CRLF.
This commit is contained in:
parent
1fcdfb3048
commit
9a21e8fbef
2 changed files with 64 additions and 52 deletions
|
|
@ -67,43 +67,34 @@ def _persist_bootstrap_password(password: str) -> None:
|
|||
|
||||
|
||||
def _normalise_bootstrap_file(raw: bytes, password: str) -> None:
|
||||
"""Give an existing malformed file its trailing LF, in place.
|
||||
"""Append the LF a pre-newline release left off.
|
||||
|
||||
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.
|
||||
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.
|
||||
"""
|
||||
data = _bootstrap_file_bytes(password)
|
||||
# Only a trailing-whitespace rewrite is safe without a rename: every partial
|
||||
# state is then "<secret>\n" plus leftover whitespace, which still strips to
|
||||
# the same credential. Anything else keeps working unnormalised.
|
||||
if not raw.startswith(data[:-1]):
|
||||
if raw != password.encode("utf-8"):
|
||||
return
|
||||
|
||||
# O_BINARY or Windows opens the descriptor in text mode and os.write turns
|
||||
# the LF straight back into CRLF, which is the bug this path exists to fix.
|
||||
fd = os.open(_BOOTSTRAP_PW_PATH, os.O_RDWR | getattr(os, "O_BINARY", 0))
|
||||
# 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.
|
||||
fd = os.open(
|
||||
_BOOTSTRAP_PW_PATH,
|
||||
os.O_WRONLY | os.O_APPEND | getattr(os, "O_BINARY", 0),
|
||||
)
|
||||
try:
|
||||
if os.read(fd, len(raw) + 1) != raw:
|
||||
return
|
||||
os.lseek(fd, 0, os.SEEK_SET)
|
||||
# os.write may write fewer bytes than asked; truncating after a short
|
||||
# write would NUL-fill the credential and lock the admin out.
|
||||
written = 0
|
||||
while written < len(data):
|
||||
n = os.write(fd, data[written:])
|
||||
if not n:
|
||||
return
|
||||
written += n
|
||||
os.ftruncate(fd, len(data))
|
||||
os.write(fd, b"\n")
|
||||
try:
|
||||
os.fchmod(fd, 0o600)
|
||||
except (AttributeError, OSError):
|
||||
# fchmod only reached Windows in 3.13. Staying on the descriptor
|
||||
# matters more than re-applying a mode the writer already set.
|
||||
# fchmod only reached Windows in 3.13.
|
||||
pass
|
||||
finally:
|
||||
os.close(fd)
|
||||
|
|
@ -125,9 +116,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]:
|
|||
if not password:
|
||||
return None
|
||||
|
||||
# Older releases wrote no terminator, and text mode wrote CRLF on Windows,
|
||||
# so upgrades kept the `cat` problem. Best-effort: a read-only auth dir or a
|
||||
# credential cleared underneath us must not fail startup.
|
||||
# Older releases wrote no terminator, so upgrades kept the `cat` problem.
|
||||
# Best-effort: a read-only auth dir must not fail startup.
|
||||
if raw != _bootstrap_file_bytes(password):
|
||||
try:
|
||||
_normalise_bootstrap_file(raw, password)
|
||||
|
|
|
|||
|
|
@ -153,18 +153,11 @@ def test_bootstrap_password_round_trips_across_a_restart_with_the_newline():
|
|||
assert storage.generate_bootstrap_password() == original
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"legacy",
|
||||
[
|
||||
b"legacy-bootstrap-secret", # written before the newline existed
|
||||
b"legacy-bootstrap-secret\r\n", # written by text mode on Windows
|
||||
],
|
||||
)
|
||||
def test_upgrade_normalises_the_bootstrap_file(legacy):
|
||||
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().
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(legacy)
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
|
|
@ -172,6 +165,23 @@ def test_upgrade_normalises_the_bootstrap_file(legacy):
|
|||
assert storage.get_bootstrap_password() == "legacy-bootstrap-secret"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("other", [
|
||||
b"legacy-bootstrap-secret\r\n", # only an unreleased build wrote this
|
||||
b"legacy-bootstrap-secret\r",
|
||||
b"legacy-bootstrap-secret ",
|
||||
])
|
||||
def test_only_an_exactly_untermimated_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.
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(other)
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
assert storage.get_bootstrap_password() == "legacy-bootstrap-secret"
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == other
|
||||
|
||||
|
||||
def test_upgrade_normalises_when_the_admin_row_is_missing():
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
|
||||
|
|
@ -242,7 +252,11 @@ def test_normalising_does_not_overwrite_a_rotated_bootstrap_file(monkeypatch):
|
|||
|
||||
storage._read_persisted_bootstrap_password()
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"brand-new-secret\n"
|
||||
# The append may add a second newline; the rotated credential must survive.
|
||||
raw = storage._BOOTSTRAP_PW_PATH.read_bytes()
|
||||
assert raw.strip() == b"brand-new-secret"
|
||||
storage._bootstrap_password = None
|
||||
assert storage._load_bootstrap_password() == "brand-new-secret"
|
||||
|
||||
|
||||
def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch):
|
||||
|
|
@ -278,21 +292,29 @@ def test_normalising_opens_the_file_in_binary_mode(monkeypatch):
|
|||
assert seen and all(f & 0x8000 for f in seen), seen
|
||||
|
||||
|
||||
def test_normalising_survives_a_short_write(monkeypatch):
|
||||
# A short write followed by ftruncate would NUL-fill the credential.
|
||||
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.
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
real_write = storage.os.write
|
||||
|
||||
def one_byte_at_a_time(fd, data):
|
||||
return real_write(fd, data[:1])
|
||||
real_open = storage.os.open
|
||||
|
||||
monkeypatch.setattr(storage.os, "write", one_byte_at_a_time)
|
||||
def truncate_then_open(path, flags, *args, **kwargs):
|
||||
fd = real_open(path, flags, *args, **kwargs)
|
||||
if str(path) == str(storage._BOOTSTRAP_PW_PATH):
|
||||
storage._BOOTSTRAP_PW_PATH.write_text("", encoding = "utf-8")
|
||||
return fd
|
||||
|
||||
storage.ensure_default_admin()
|
||||
monkeypatch.setattr(storage.os, "open", truncate_then_open)
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret\n"
|
||||
assert storage.get_bootstrap_password() == "legacy-bootstrap-secret"
|
||||
storage._read_persisted_bootstrap_password()
|
||||
|
||||
# A lone newline over a cleared file still reads back as no password.
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes().strip() == b""
|
||||
storage._bootstrap_password = None
|
||||
assert storage._load_bootstrap_password() is None
|
||||
|
||||
|
||||
def test_normalising_works_without_fchmod(monkeypatch):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue