Studio: Write auth secret files with a trailing newline (#7576)
* Write auth secret files with a trailing newline * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Pin LF in the auth secret writers and migrate legacy files Both writers used text mode, so on Windows the trailing newline became CRLF. The Windows Studio smoke jobs run under bash and read the file with OLD=$(cat ...), which strips the LF but leaves the CR attached, so the credential goes into the login body as "<secret>\r" and the request fails. Write bytes in the backend and pin newline in the CLI so the file is "<secret>\n" on every platform. generate_bootstrap_password() also returned early on an existing file, so upgraded installs kept the original problem; it now rewrites anything that isn't already exactly "<secret>\n", best-effort so a read-only auth dir cannot fail startup. The raw test assertions used read_text(), which decodes CRLF back to "\n" and would have stayed green on Windows. They read bytes now. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run the newline migration on the path upgrades actually take ensure_default_admin() short-circuits to _load_bootstrap_password() once the admin row exists, so the normalisation added in the previous commit sat on generate_bootstrap_password(), which only fresh installs reach. An upgraded install kept its newline-less file. Both readers now share _read_persisted_bootstrap_password(). Make the write atomic while it is here: it can now rewrite a live file, and a partial write would destroy the only plaintext copy of the recovery credential. Same mkstemp plus os.replace shape the CLI writer already uses. Tests cover the upgrade path through ensure_default_admin(), a well-formed file not being rewritten on every start, a failing migration not blocking startup, and the atomic replace. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * 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. * Open the bootstrap file in binary mode and finish the write Three defects in the in-place normalisation, all on the Windows upgrade path. os.open does not add O_BINARY on Windows and CPython never changes the CRT default of _O_TEXT, so the descriptor was in text mode: os.write turned the LF straight back into CRLF and ftruncate then cut the LF off, leaving "<secret>\r". That is the bug this PR exists to fix, reintroduced by the migration itself, and it is a fixed point that never converges. os.read translates in reverse too, so a genuinely CRLF file failed verification and was silently skipped. os.write may return having written fewer bytes than asked; ftruncate would then NUL-extend the credential so it no longer matched the hash in auth.db. os.fchmod only reached Windows in 3.13 and AttributeError is not OSError, so on 3.9 to 3.12 it escaped both handlers and aborted the first start after upgrade. * 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. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix a typo in a bootstrap normalisation test name * Tighten the bootstrap newline comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <danielhanchen@gmail.com> Co-authored-by: danielhanchen <unslothai@gmail.com>
This commit is contained in:
Nilay
GitHub
parent
076c965723
commit
7348a20497
4 changed files with 357 additions and 27 deletions
|
|
@ -9,6 +9,7 @@ import ipaddress
|
|||
import os
|
||||
import secrets
|
||||
import sqlite3
|
||||
import tempfile
|
||||
import threading
|
||||
from datetime import datetime, timezone
|
||||
from typing import Optional, Tuple
|
||||
|
|
@ -30,6 +31,97 @@ _BOOTSTRAP_PW_PATH = DB_PATH.parent / ".bootstrap_password"
|
|||
_bootstrap_password: Optional[str] = None
|
||||
|
||||
|
||||
def _bootstrap_file_bytes(password: str) -> bytes:
|
||||
"""Exact on-disk form: the secret plus one LF.
|
||||
|
||||
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:
|
||||
"""Atomically write the bootstrap password 0600, LF terminated on every OS.
|
||||
|
||||
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
|
||||
)
|
||||
try:
|
||||
with os.fdopen(fd, "wb") as f:
|
||||
f.write(_bootstrap_file_bytes(password))
|
||||
try:
|
||||
os.chmod(tmp_name, 0o600)
|
||||
except OSError:
|
||||
pass
|
||||
os.replace(tmp_name, _BOOTSTRAP_PW_PATH)
|
||||
except BaseException:
|
||||
try:
|
||||
os.unlink(tmp_name)
|
||||
except OSError:
|
||||
pass
|
||||
raise
|
||||
|
||||
|
||||
def _normalise_bootstrap_file(raw: bytes, password: str) -> None:
|
||||
"""Append the LF a pre-newline release left off.
|
||||
|
||||
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: 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),
|
||||
)
|
||||
try:
|
||||
os.write(fd, b"\n")
|
||||
try:
|
||||
os.fchmod(fd, 0o600)
|
||||
except (AttributeError, OSError):
|
||||
# fchmod only reached Windows in 3.13.
|
||||
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():
|
||||
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 undecodable bytes are
|
||||
# damage whose plaintext is worthless anyway.
|
||||
try:
|
||||
raw = _BOOTSTRAP_PW_PATH.read_bytes()
|
||||
password = raw.decode("utf-8").strip()
|
||||
except (OSError, UnicodeDecodeError):
|
||||
return None
|
||||
if not password:
|
||||
return None
|
||||
|
||||
# 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)
|
||||
except OSError:
|
||||
pass
|
||||
return password
|
||||
|
||||
|
||||
def generate_bootstrap_password() -> str:
|
||||
"""Generate a 4-word diceware passphrase and persist it to disk.
|
||||
|
||||
|
|
@ -43,10 +135,10 @@ def generate_bootstrap_password() -> str:
|
|||
return _bootstrap_password
|
||||
|
||||
# Persisted from a previous run?
|
||||
if _BOOTSTRAP_PW_PATH.is_file():
|
||||
_bootstrap_password = _BOOTSTRAP_PW_PATH.read_text(encoding = "utf-8").strip()
|
||||
if _bootstrap_password:
|
||||
return _bootstrap_password
|
||||
persisted = _read_persisted_bootstrap_password()
|
||||
if persisted:
|
||||
_bootstrap_password = persisted
|
||||
return _bootstrap_password
|
||||
|
||||
# First startup: generate a fresh passphrase.
|
||||
import diceware
|
||||
|
|
@ -57,11 +149,7 @@ def generate_bootstrap_password() -> str:
|
|||
|
||||
# Persist so the same passphrase survives restarts until password change.
|
||||
ensure_dir(_BOOTSTRAP_PW_PATH.parent)
|
||||
_BOOTSTRAP_PW_PATH.write_text(_bootstrap_password, encoding = "utf-8")
|
||||
try:
|
||||
os.chmod(_BOOTSTRAP_PW_PATH, 0o600)
|
||||
except OSError:
|
||||
pass
|
||||
_persist_bootstrap_password(_bootstrap_password)
|
||||
|
||||
return _bootstrap_password
|
||||
|
||||
|
|
@ -72,19 +160,14 @@ def get_bootstrap_password() -> Optional[str]:
|
|||
|
||||
|
||||
def _load_bootstrap_password() -> Optional[str]:
|
||||
"""Load an existing bootstrap password without creating one."""
|
||||
"""Load an existing bootstrap password without creating one.
|
||||
|
||||
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 = None
|
||||
if _BOOTSTRAP_PW_PATH.is_file():
|
||||
# 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.
|
||||
try:
|
||||
bootstrap_password = _BOOTSTRAP_PW_PATH.read_text(encoding = "utf-8").strip()
|
||||
except (OSError, UnicodeDecodeError):
|
||||
return _bootstrap_password
|
||||
if bootstrap_password:
|
||||
_bootstrap_password = bootstrap_password
|
||||
_bootstrap_password = _read_persisted_bootstrap_password()
|
||||
return _bootstrap_password
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -134,6 +134,218 @@ def test_ensure_default_admin_loads_existing_bootstrap_after_restart(monkeypatch
|
|||
assert storage.get_bootstrap_password() == bootstrap_pw
|
||||
|
||||
|
||||
def test_bootstrap_password_file_ends_with_a_newline():
|
||||
# Otherwise `cat` welds the passphrase onto the shell prompt.
|
||||
storage.ensure_default_admin()
|
||||
|
||||
# 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"
|
||||
|
||||
|
||||
def test_bootstrap_password_round_trips_across_a_restart_with_the_newline():
|
||||
storage.ensure_default_admin()
|
||||
original = storage.get_bootstrap_password()
|
||||
|
||||
storage._bootstrap_password = None
|
||||
|
||||
assert storage.generate_bootstrap_password() == original
|
||||
|
||||
|
||||
def test_upgrade_normalises_the_bootstrap_file():
|
||||
# Upgrade path: the admin row exists, so generate_bootstrap_password() never runs.
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret\n"
|
||||
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_unterminated_bootstrap_file_is_touched(other):
|
||||
# Appending is safe only because it is restricted to the one released shape.
|
||||
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")
|
||||
|
||||
assert storage.generate_bootstrap_password() == "legacy-bootstrap-secret"
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret\n"
|
||||
|
||||
|
||||
def test_a_well_formed_bootstrap_file_is_not_rewritten():
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret\n")
|
||||
mtime = storage._BOOTSTRAP_PW_PATH.stat().st_mtime_ns
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.stat().st_mtime_ns == mtime
|
||||
|
||||
|
||||
def test_migration_failure_does_not_break_startup(monkeypatch):
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
|
||||
real_open = storage.os.open
|
||||
|
||||
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()
|
||||
|
||||
assert storage.get_bootstrap_password() == "legacy-bootstrap-secret"
|
||||
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 revoked plaintext if the password changed after the read.
|
||||
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()
|
||||
|
||||
# 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):
|
||||
# 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 ")
|
||||
|
||||
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_normalising_opens_the_file_in_binary_mode(monkeypatch):
|
||||
# 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)
|
||||
seen = []
|
||||
real_open = storage.os.open
|
||||
|
||||
def spy(path, flags, *args, **kwargs):
|
||||
if str(path) == str(storage._BOOTSTRAP_PW_PATH):
|
||||
seen.append(flags)
|
||||
return real_open(path, flags & ~0x8000, *args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(storage.os, "open", spy)
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
assert seen and all(f & 0x8000 for f in seen), seen
|
||||
|
||||
|
||||
def test_clearing_by_truncation_mid_normalisation_is_not_undone(monkeypatch):
|
||||
# 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")
|
||||
|
||||
real_open = storage.os.open
|
||||
|
||||
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
|
||||
|
||||
monkeypatch.setattr(storage.os, "open", truncate_then_open)
|
||||
|
||||
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):
|
||||
# os.fchmod only reached Windows in 3.13; its absence must not raise.
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret")
|
||||
monkeypatch.delattr(storage.os, "fchmod", raising = False)
|
||||
|
||||
storage.ensure_default_admin()
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret\n"
|
||||
assert storage.get_bootstrap_password() == "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")
|
||||
|
||||
def boom(src, dst):
|
||||
raise OSError("crash before replace")
|
||||
|
||||
monkeypatch.setattr(storage.os, "replace", boom)
|
||||
with pytest.raises(OSError):
|
||||
storage._persist_bootstrap_password("new-secret")
|
||||
|
||||
assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"original-secret\n"
|
||||
leftovers = [
|
||||
p.name
|
||||
for p in storage._BOOTSTRAP_PW_PATH.parent.iterdir()
|
||||
if "bootstrap_password." in p.name
|
||||
]
|
||||
assert leftovers == []
|
||||
|
||||
|
||||
def test_ensure_default_admin_does_not_generate_for_empty_existing_bootstrap():
|
||||
seed_user()
|
||||
storage._BOOTSTRAP_PW_PATH.write_text(" \n", encoding = "utf-8")
|
||||
|
|
@ -358,7 +570,7 @@ def test_write_desktop_secret_file_is_0600_on_unix(tmp_path):
|
|||
|
||||
studio_cli._write_auth_secret(path, "desktop-secret")
|
||||
|
||||
assert path.read_text() == "desktop-secret"
|
||||
assert path.read_bytes() == b"desktop-secret\n"
|
||||
if platform.system() != "Windows":
|
||||
assert oct(path.stat().st_mode & 0o777) == "0o600"
|
||||
|
||||
|
|
@ -525,7 +737,8 @@ if result.exit_code != 0:
|
|||
capture_output = True,
|
||||
)
|
||||
assert result.returncode == 0, result.stderr + result.stdout
|
||||
secret = (auth_dir / ".desktop_secret").read_text()
|
||||
# Strip like the src-tauri readers do.
|
||||
secret = (auth_dir / ".desktop_secret").read_text().strip()
|
||||
assert secret.startswith("desktop-")
|
||||
|
||||
conn = sqlite3.connect(auth_dir / "auth.db")
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue