diff --git a/backend/app/services/credential_crypto.py b/backend/app/services/credential_crypto.py index 273e574..65cb930 100644 --- a/backend/app/services/credential_crypto.py +++ b/backend/app/services/credential_crypto.py @@ -80,10 +80,48 @@ class CredentialCrypto: parent = self._key_path.parent parent.mkdir(parents=True, exist_ok=True) os.chmod(parent, 0o700) + + # Written to a temp file and LINKED into place, not written directly. + # + # hypercorn starts several worker processes and each one builds the + # app, so on a first boot they all reach this at once. A plain + # `write_bytes` creates the file at size zero and fills it a moment + # later, which gives a second process an `exists()` of True and a + # `read_bytes()` of b"" — and the app dies with + # + # ValueError: Fernet key must be 32 url-safe base64-encoded bytes. + # + # Seen on run 7368's smoke, and it is a race rather than a certainty: + # the same image had booted cleanly on the three runs before it. A + # first boot that fails one time in five is worse than one that fails + # every time, because it looks like the deployment rather than the code. + # + # `os.link` is the atomic part: it either creates the name or raises + # FileExistsError, and it cannot expose a half-written file. NOT + # `os.replace`, which would succeed — so two processes that both + # generated a key would each think they had won, and the loser's key + # would overwrite the one the winner had already handed to Fernet. key = Fernet.generate_key() - self._key_path.write_bytes(key) - os.chmod(self._key_path, 0o600) - return key + tmp = parent / f".{self._key_path.name}.{os.getpid()}.tmp" + try: + tmp.write_bytes(key) + os.chmod(tmp, 0o600) + try: + os.link(tmp, self._key_path) + except FileExistsError: + # Another process created it between our `exists()` check and + # here. Theirs is as good as ours, and using it is what keeps + # every worker on ONE key. + log.info( + "another process created %s first; using that key", + self._key_path, + ) + finally: + tmp.unlink(missing_ok=True) + # Read back rather than returning `key`: on the losing branch the file + # holds somebody else's, and returning ours would leave this worker + # encrypting with a key no other worker can read. + return self._key_path.read_bytes() def encrypt(self, plaintext: str) -> bytes: return self._fernet.encrypt(plaintext.encode("utf-8")) diff --git a/tests/test_credential_crypto.py b/tests/test_credential_crypto.py index 2ca31b0..689e067 100644 --- a/tests/test_credential_crypto.py +++ b/tests/test_credential_crypto.py @@ -135,3 +135,62 @@ def test_compose_passes_the_bootstrap_variable_through(): compose = (_REPO_ROOT / "docker-compose.yml").read_text() assert f"{_BOOTSTRAP_ENV_VAR}: ${{{_BOOTSTRAP_ENV_VAR}" in compose + + +# --- the first-boot race ----------------------------------------------------- + + +def test_every_process_bootstrapping_at_once_ends_up_with_the_same_key(tmp_path): + """Run 7368's smoke, and the reason the key write is a link rather than a + write. + + hypercorn starts several worker processes and each one builds the app, so + on a first boot they all reach the bootstrap together. `write_bytes` + creates the file at size zero and fills it a moment later, which gave a + second process an `exists()` of True and a `read_bytes()` of b"": + + ValueError: Fernet key must be 32 url-safe base64-encoded bytes. + + Threads rather than processes because the failure is about the ORDER of + two filesystem operations, which threads reproduce and which a subprocess + fixture would make this suite pay for on every run. + + What is asserted is AGREEMENT, not merely that nobody crashed: a race that + left each worker holding its own key would pass a "did it raise" check and + produce a system where a credential written by one worker cannot be read + by the next. + """ + import threading + + path = tmp_path / "secrets" / "credential_key.b64" + keys: list[bytes] = [] + errors: list[Exception] = [] + start = threading.Barrier(8) + + def bootstrap(): + try: + start.wait(timeout=5) + CredentialCrypto(path, bootstrap_ok=True) + keys.append(path.read_bytes()) + except Exception as exc: # noqa: BLE001 — recorded, asserted below + errors.append(exc) + + threads = [threading.Thread(target=bootstrap) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + assert errors == [] + assert len(keys) == 8 + assert len(set(keys)) == 1, "the workers disagree about the credential key" + + +def test_the_bootstrap_leaves_no_temp_file_behind(tmp_path): + """The temp file is an implementation detail of the atomic write and must + not survive it — a stray `.credential_key.b64..tmp` in the secrets + directory is a copy of the key with nothing guarding it.""" + path = tmp_path / "secrets" / "credential_key.b64" + CredentialCrypto(path, bootstrap_ok=True) + + assert sorted(p.name for p in path.parent.iterdir()) == ["credential_key.b64"]