Fix a race that could hand a worker an empty secret key

Creating the key with O_CREAT|O_EXCL makes the file appear before its
contents are written, so a process arriving in that window took the
'someone else created it' branch and read an empty file -- an empty flask
session key.  CI caught this through the concurrency test (which passed
locally by timing luck).

Write the key to a temporary file, created owner-readable only by
mkstemp, and link it into place once complete: under its final name the
file is never empty or half-written, and the link failing with
FileExistsError is what tells the losers of the race to use the existing
key.

The regression tests now repeat the threaded race and add a
multi-process variant (the gunicorn-worker case), and assert that every
reader got a complete 32-character key rather than only that they agree.
Verified fail-without/pass-with by widening the create-to-write window:
5/5 runs broken before, 0/5 after.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: roed314

lmfdb/config.py

@@ -35,6 +35,7 @@
 import secrets
 import string
 import socket
+import tempfile
 from contextlib import closing
 from logging import INFO
 
@@ -133,20 +134,27 @@ def get_secret_key(config_file=None):
     """
     if config_file is None:
         config_file = find_config_file()
-    secret_key_file = os.path.join(os.path.dirname(os.path.abspath(config_file)), "secret_key")
+    directory = os.path.dirname(os.path.abspath(config_file))
+    secret_key_file = os.path.join(directory, "secret_key")
     if not os.path.exists(secret_key_file):
-        os.makedirs(os.path.dirname(secret_key_file), exist_ok=True)
+        os.makedirs(directory, exist_ok=True)
         key = "".join(secrets.choice(string.ascii_letters + string.digits) for _ in range(32))
-        # create the file atomically, readable only by the owner; if
-        # another process creates it first, its key is used instead, so
-        # concurrent workers always end up with the same key
+        # Write the key to a temporary file (created readable only by the
+        # owner) and link it into place once it is complete: under its
+        # final name the file is therefore never empty or half-written, so
+        # workers starting simultaneously either create the key or read a
+        # complete one.  The link fails if the name already exists, which
+        # is how the losers of the race learn to use the existing key.
+        fd, tmp = tempfile.mkstemp(dir=directory)
         try:
-            fd = os.open(secret_key_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
-        except FileExistsError:
-            pass
-        else:
             with os.fdopen(fd, "w") as F:
                 F.write(key)
+            try:
+                os.link(tmp, secret_key_file)
+            except FileExistsError:
+                pass
+        finally:
+            os.unlink(tmp)
     with open(secret_key_file) as F:
         return F.read()
 

lmfdb/test_config.py

@@ -77,22 +77,47 @@ def test_created_next_to_config_file(self, tmp_path):
         assert mode == 0o600
 
     def test_concurrent_creation_yields_one_key(self, tmp_path):
-        # concurrent workers must end up with the same key
+        # Concurrent workers must all end up with the same complete key.
+        # Repeated, because the window in which a reader could observe a
+        # created-but-not-yet-written file is small.
+        for i in range(20):
+            directory = tmp_path / ("run%d" % i)
+            directory.mkdir()
+            cfg = str(directory / "config.ini")
+            barrier = threading.Barrier(8)
+            keys = []
+
+            def worker():
+                barrier.wait()
+                keys.append(config_mod.get_secret_key(cfg))
+
+            threads = [threading.Thread(target=worker) for _ in range(8)]
+            for t in threads:
+                t.start()
+            for t in threads:
+                t.join()
+            assert all(len(k) == 32 for k in keys), "a worker read an incomplete key: %r" % keys
+            assert len(set(keys)) == 1, keys
+
+    def test_concurrent_creation_across_processes(self, tmp_path):
+        # the same race between separate processes, as when several
+        # gunicorn workers start at once
         cfg = str(tmp_path / "config.ini")
-        barrier = threading.Barrier(8)
-        keys = []
-
-        def worker():
-            barrier.wait()
-            keys.append(config_mod.get_secret_key(cfg))
-
-        threads = [threading.Thread(target=worker) for _ in range(8)]
-        for t in threads:
-            t.start()
-        for t in threads:
-            t.join()
-        assert len(set(keys)) == 1
-        assert len(keys[0]) == 32
+        code = (
+            "import sys\n"
+            "from lmfdb.config import get_secret_key\n"
+            "sys.stdout.write(get_secret_key(%r))\n" % cfg
+        )
+        procs = [
+            subprocess.Popen(
+                [sys.executable, "-c", code], stdout=subprocess.PIPE, text=True
+            )
+            for _ in range(6)
+        ]
+        keys = [p.communicate()[0] for p in procs]
+        assert all(p.returncode == 0 for p in procs)
+        assert all(len(k) == 32 for k in keys), "a process read an incomplete key: %r" % keys
+        assert len(set(keys)) == 1, keys
 
 
 class TestConfiguration: