fix(server): address the run-2 findings of the #818 auth review

packages/sie_server/README.md

@@ -1092,7 +1092,11 @@ values that carry different tokens, while the gateway reads only the first
 
 Token auth covers the HTTP port. The other listeners are local: the IPC socket
 that a worker sidecar uses (`SIE_IPC_SOCKET_PATH`, mode `0600`) and the
-loopback ports of the engines the server launches.
+loopback ports of the engines the server launches. On macOS a socket can
+inherit an entry that allows access from its directory's access control list,
+and its mode does not remove that access, so the server refuses to create the
+IPC socket in a directory whose access control list has an entry that allows
+access (`ls -led <directory>` lists the entries).
 
 When token auth is on and no token is configured, the server generates one on
 first start and saves it in `SIE_DATA_DIR`. When stderr is a terminal it prints
@@ -1107,8 +1111,8 @@ containers, set `SIE_AUTH_TOKEN` yourself instead. The server refuses a token
 directory that another user owns or can write to, and a token file that is a
 symbolic link, not a regular file, hard-linked, owned by another user, or
 readable by others. On macOS it also refuses a token directory or file whose
-access control list has an entry that allows access (`ls -le` lists the
-entries); deny entries, such as the one macOS puts on the home folder, are
+access control list has an entry that allows access (`ls -led <path>` lists
+the entries); deny entries, such as the one macOS puts on the home folder, are
 accepted. To rotate a generated token, delete `auth-token` from
 `SIE_DATA_DIR` and restart the server. In Docker, keep the token across
 containers with a volume:

packages/sie_server/src/sie_server/core/server_auth.py

@@ -21,6 +21,7 @@
 import os
 import re
 import secrets
+import shlex
 import stat
 import sys
 from collections.abc import Iterable, Mapping, Sequence
@@ -239,15 +240,17 @@ def check_private_directory(directory: Path) -> None:
         raise TokenFileError(f"Cannot read {directory}: {exc.strerror or exc}") from exc
     try:
         info = os.fstat(fd)
+        quoted = shlex.quote(str(directory))
         if info.st_uid != os.getuid():
             raise TokenFileError(f"{directory} must be owned by the user that runs sie-server.")
         if stat.S_IMODE(info.st_mode) & 0o022:
-            raise TokenFileError(f"{directory} must not be writable by others (chmod 700 {directory}).")
+            raise TokenFileError(f"{directory} must not be writable by others (chmod 700 {quoted}).")
         if _acl_allows_access(fd, directory):
             raise TokenFileError(
                 f"{directory} has an access control list entry that allows access, which its mode does not show. "
-                f"List the entries with ls -led {directory} and remove each allow entry with chmod -a# <number> "
-                f"{directory}, or set {DATA_DIR_ENV} to a directory of its own."
+                f'List the entries with ls -led {quoted} and remove each allow entry with chmod -a "<entry>" '
+                f"{quoted}, giving the entry's text as listed, without its number or the word inherited. "
+                f"Or set {DATA_DIR_ENV} to a directory of its own."
             )
     finally:
         os.close(fd)
@@ -265,11 +268,18 @@ def _libsystem() -> ctypes.CDLL:
 
 
 def _acl_allows_access(fd: int, target: Path) -> bool:
+    try:
+        return acl_allows_access(fd)
+    except OSError as exc:
+        raise TokenFileError(f"Cannot read the access control list of {target}: {exc.strerror}") from exc
+
+
+def acl_allows_access(fd: int) -> bool:
     """Whether the macOS access control list of ``fd`` has an entry other than a deny entry.
 
     Such an entry can grant access that the mode bits do not show. Deny entries, such as the one macOS puts on a
     home folder, only take access away. An entry whose kind cannot be read counts as allowing access, and a list
-    that cannot be read is refused.
+    that cannot be read raises ``OSError``. Always false on other platforms.
     """
     if sys.platform != "darwin":
         return False
@@ -280,7 +290,7 @@ def _acl_allows_access(fd: int, target: Path) -> bool:
         code = ctypes.get_errno()
         if code in _NO_ACL_ERRNOS:
             return False
-        raise TokenFileError(f"Cannot read the access control list of {target}: {os.strerror(code)}")
+        raise OSError(code, os.strerror(code))
     try:
         entry = ctypes.c_void_p()
         tag = ctypes.c_int()
@@ -315,12 +325,14 @@ def read_stored_token(directory: Path) -> str:
             )
         if info.st_uid != os.getuid():
             raise TokenFileError(f"{path} must be owned by the user that runs sie-server.")
+        quoted = shlex.quote(str(path))
         if stat.S_IMODE(info.st_mode) & 0o077:
-            raise TokenFileError(f"{path} must not be readable or writable by others (chmod 600 {path}).")
+            raise TokenFileError(f"{path} must not be readable or writable by others (chmod 600 {quoted}).")
         if _acl_allows_access(fd, path):
             raise TokenFileError(
                 f"{path} has an access control list entry that allows access, so others may have read the token. "
-                f"Delete {path} to have sie-server generate a new one, or remove the entries with chmod -N {path}."
+                f"Delete the file (rm {quoted}) to have sie-server generate a new token. Removing the entries "
+                f"with chmod -N {quoted} instead keeps the current token."
             )
         raw = os.read(fd, _MAX_TOKEN_FILE_BYTES + 1)
     finally:

packages/sie_server/src/sie_server/ipc_server.py

@@ -7,7 +7,9 @@
 import json
 import logging
 import os
+import shlex
 import struct
+import sys
 import time
 from collections.abc import AsyncIterator, Awaitable, Callable, Iterable, Mapping
 from pathlib import Path
@@ -25,6 +27,7 @@
 from sie_server.core.grammar_routing import resolve_grammar_serving_model
 from sie_server.core.numerical_snapshot import numerical_profile_snapshot
 from sie_server.core.readiness import is_ready
+from sie_server.core.server_auth import acl_allows_access
 from sie_server.ipc_types import (
     IPC_VERSION,
     METHOD_APPLY_MODEL_CONFIG,
@@ -199,6 +202,21 @@ async def in_progress(self) -> None:
         await self._sink.send(GenerateEvent(kind="in_progress"))
 
 
+def _refuse_shared_socket_directory(directory: Path) -> None:
+    fd = os.open(directory, os.O_RDONLY | os.O_DIRECTORY)
+    try:
+        shared = acl_allows_access(fd)
+    finally:
+        os.close(fd)
+    if shared:
+        raise PermissionError(
+            f"{directory} has an access control list entry that allows access. A socket created in it can inherit "
+            f"that access, which the socket's mode 0600 does not remove, so other users could connect to it. "
+            f"Remove the allow entries (ls -led {shlex.quote(str(directory))} lists them) or set "
+            f"SIE_IPC_SOCKET_PATH to a socket in a directory without them."
+        )
+
+
 def _named_admission(item: Any) -> str | None:
     """The numerical admission digest that a batch item names, if any."""
     payloads = (item.encode, item.score) if isinstance(item, RunBatchItem) else (item,)
@@ -282,13 +300,15 @@ def last_ping_monotonic(self) -> float | None:
 
     async def start(self) -> None:
         """Bind the UDS and begin serving. Unlinks any stale socket file first."""
+        self._socket_path.parent.mkdir(parents=True, exist_ok=True)
+        if sys.platform == "darwin":
+            _refuse_shared_socket_directory(self._socket_path.parent)
         if self._socket_path.exists():
             try:
                 self._socket_path.unlink()
             except OSError:
                 logger.warning("Could not unlink existing socket at %s", self._socket_path)
 
-        self._socket_path.parent.mkdir(parents=True, exist_ok=True)
         self._server = await asyncio.start_unix_server(
             self._handle_connection,
             path=str(self._socket_path),

packages/sie_server/tests/core/test_server_auth.py

@@ -48,6 +48,15 @@
 _MACOS_ACL = pytest.mark.skipif(sys.platform != "darwin", reason="macOS access control lists, set with chmod +a")
 
 
+@pytest.fixture
+def add_acl_entry(tmp_path: Path) -> Iterator[Callable[[Path, str], None]]:
+    def add(path: Path, entry: str) -> None:
+        subprocess.run(["/bin/chmod", "+a", entry, str(path)], check=True)  # noqa: S603
+
+    yield add
+    subprocess.run(["/bin/chmod", "-RN", str(tmp_path)], check=True)  # noqa: S603
+
+
 @pytest.fixture(autouse=True)
 def _clean_auth_env(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]:
     saved = dict(os.environ)
@@ -169,11 +178,12 @@ def test_later_use_reads_the_same_token(self, tmp_path: Path) -> None:
         assert second.token == first.token
 
     def test_a_token_others_can_read_is_refused(self, tmp_path: Path) -> None:
-        stored = load_or_create_token(tmp_path)
+        directory = tmp_path / "Application Support"
+        stored = load_or_create_token(directory)
         stored.path.chmod(0o644)
 
-        with pytest.raises(TokenFileError, match="chmod 600"):
-            read_stored_token(tmp_path)
+        with pytest.raises(TokenFileError, match=re.escape(f"(chmod 600 '{stored.path}')")):
+            read_stored_token(directory)
 
     def test_concurrent_first_starts_agree_on_one_token(self, tmp_path: Path) -> None:
         for trial in range(10):
@@ -544,57 +554,93 @@ def test_a_hard_linked_token_is_refused(self, tmp_path: Path) -> None:
 
     @pytest.mark.parametrize("mode", [0o770, 0o707, 0o777])
     def test_a_directory_others_can_write_is_refused(self, tmp_path: Path, mode: int) -> None:
-        directory = tmp_path / "shared"
+        directory = tmp_path / "shared dir"
         directory.mkdir()
         directory.chmod(mode)
 
-        with pytest.raises(TokenFileError, match="chmod 700"):
+        with pytest.raises(TokenFileError, match=re.escape(f"(chmod 700 '{directory}')")):
             load_or_create_token(directory)
         assert not token_path(directory).exists()
 
     @_MACOS_ACL
-    def test_a_directory_with_an_access_control_list_entry_that_allows_access_is_refused(self, tmp_path: Path) -> None:
-        existing = tmp_path / "existing"
+    def test_a_directory_with_an_access_control_list_entry_that_allows_access_is_refused(
+        self, tmp_path: Path, add_acl_entry: Callable[[Path, str], None]
+    ) -> None:
+        existing = tmp_path / "Application Support"
         load_or_create_token(existing)
         empty = tmp_path / "empty"
         empty.mkdir(mode=0o700)
         for directory in (existing, empty):
-            _add_acl_entry(directory, "everyone deny delete")
-            _add_acl_entry(directory, "everyone allow add_file,add_subdirectory,delete_child")
+            add_acl_entry(directory, "everyone deny delete")
+            add_acl_entry(directory, "everyone allow add_file,add_subdirectory,delete_child")
 
-        with pytest.raises(TokenFileError, match=re.escape(f"chmod -a# <number> {existing}, or set SIE_DATA_DIR")):
+        with pytest.raises(TokenFileError) as refused:
             read_stored_token(existing)
+        quoted = f"'{existing}'"
+        message = str(refused.value)
+        assert f"List the entries with ls -led {quoted} and remove" in message
+        assert f'remove each allow entry with chmod -a "<entry>" {quoted}, giving the entry' in message
         with pytest.raises(TokenFileError, match="entry that allows access"):
             load_or_create_token(empty)
         assert not token_path(empty).exists()
 
     @_MACOS_ACL
-    def test_deny_entries_like_the_home_folder_default_are_accepted(self, tmp_path: Path) -> None:
+    def test_removing_each_allow_entry_by_its_text_keeps_the_deny_entries(
+        self, tmp_path: Path, add_acl_entry: Callable[[Path, str], None]
+    ) -> None:
+        add_acl_entry(tmp_path, "everyone deny delete,directory_inherit")
+        add_acl_entry(tmp_path, "everyone allow list,directory_inherit")
+        directory = tmp_path / "sie"
+        directory.mkdir(mode=0o700)
+        add_acl_entry(directory, "user:nobody allow list")
+        add_acl_entry(directory, "user:daemon allow search")
+        with pytest.raises(TokenFileError, match="without its number or the word inherited"):
+            load_or_create_token(directory)
+
+        for entry in _acl_entries(directory):
+            if " allow " in entry:
+                subprocess.run(  # noqa: S603
+                    ["/bin/chmod", "-a", entry.replace(" inherited ", " "), str(directory)], check=True
+                )
+
+        assert _acl_entries(directory) == ["group:everyone inherited deny delete,directory_inherit"]
+        assert load_or_create_token(directory).created
+
+    @_MACOS_ACL
+    def test_deny_entries_like_the_home_folder_default_are_accepted(
+        self, tmp_path: Path, add_acl_entry: Callable[[Path, str], None]
+    ) -> None:
         directory = tmp_path / "home"
         directory.mkdir(mode=0o700)
-        _add_acl_entry(directory, "everyone deny delete")
+        add_acl_entry(directory, "everyone deny delete")
 
         stored = load_or_create_token(directory)
-        _add_acl_entry(stored.path, "everyone deny delete")
+        add_acl_entry(stored.path, "everyone deny delete")
 
         assert stored.created
         assert read_stored_token(directory) == stored.token
 
     @_MACOS_ACL
-    def test_a_token_file_with_an_access_control_list_entry_that_allows_access_is_refused(self, tmp_path: Path) -> None:
-        stored = load_or_create_token(tmp_path)
-        _add_acl_entry(stored.path, "everyone allow read")
+    def test_a_token_file_with_an_access_control_list_entry_that_allows_access_is_refused(
+        self, tmp_path: Path, add_acl_entry: Callable[[Path, str], None]
+    ) -> None:
+        directory = tmp_path / "Application Support"
+        stored = load_or_create_token(directory)
+        add_acl_entry(stored.path, "everyone allow read")
 
-        with pytest.raises(TokenFileError, match=re.escape(f"Delete {stored.path} to have sie-server generate")):
-            read_stored_token(tmp_path)
+        with pytest.raises(TokenFileError) as refused:
+            read_stored_token(directory)
+        message = str(refused.value)
+        assert f"so others may have read the token. Delete the file (rm '{stored.path}')" in message
+        assert f"Removing the entries with chmod -N '{stored.path}' instead keeps the current token." in message
 
     @_MACOS_ACL
     def test_a_token_file_that_inherited_an_entry_is_refused_after_the_directory_is_cleared(
-        self, tmp_path: Path
+        self, tmp_path: Path, add_acl_entry: Callable[[Path, str], None]
     ) -> None:
         directory = tmp_path / "sie"
         directory.mkdir(mode=0o700)
-        _add_acl_entry(directory, "everyone allow read,file_inherit")
+        add_acl_entry(directory, "everyone allow read,file_inherit")
         token_path(directory).write_text(f"{TOKEN}\n")
         token_path(directory).chmod(0o600)
         subprocess.run(["/bin/chmod", "-N", str(directory)], check=True)  # noqa: S603
@@ -617,9 +663,9 @@ def failing_read(fd: int, kind: int) -> None:
 
     @_MACOS_ACL
     def test_an_access_control_list_entry_of_unknown_kind_counts_as_allowing_access(
-        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, add_acl_entry: Callable[[Path, str], None]
     ) -> None:
-        _add_acl_entry(tmp_path, "everyone deny delete")
+        add_acl_entry(tmp_path, "everyone deny delete")
         monkeypatch.setattr(server_auth._libsystem(), "acl_get_tag_type", lambda entry, tag: -1)
 
         with pytest.raises(TokenFileError, match="entry that allows access"):
@@ -1125,8 +1171,11 @@ def refusing_unlink(self: Path, missing_ok: bool = False) -> None:
     monkeypatch.setattr(Path, "unlink", refusing_unlink)
 
 
-def _add_acl_entry(path: Path, entry: str) -> None:
-    subprocess.run(["/bin/chmod", "+a", entry, str(path)], check=True)  # noqa: S603
+def _acl_entries(path: Path) -> list[str]:
+    listing = subprocess.run(  # noqa: S603
+        ["/bin/ls", "-led", str(path)], capture_output=True, text=True, check=True
+    ).stdout
+    return [line.split(": ", 1)[1] for line in listing.splitlines()[1:]]
 
 
 def _capture(call: Callable[[], object], outcome: list[BaseException]) -> None:

packages/sie_server/tests/test_ipc_server.py

@@ -4,8 +4,12 @@
 import contextlib
 import hashlib
 import os
+import shutil
 import socket
+import stat
 import struct
+import subprocess
+import sys
 import tempfile
 import threading
 import time
@@ -770,6 +774,56 @@ async def server_and_path():
         await srv.stop(drain_timeout_s=1.0)
 
 
+@pytest.fixture
+def socket_directory(request: pytest.FixtureRequest) -> Iterator[Path]:
+    directory = _short_sock_path().with_suffix("")
+    directory.mkdir(mode=0o700)
+    try:
+        subprocess.run(["/bin/chmod", "+a", request.param, str(directory)], check=True)  # noqa: S603
+        yield directory
+    finally:
+        subprocess.run(["/bin/chmod", "-RN", str(directory)], check=True)  # noqa: S603
+        shutil.rmtree(directory)
+
+
+@pytest.mark.skipif(sys.platform != "darwin", reason="macOS access control lists, set with chmod +a")
+class TestSocketDirectoryAccessControlList:
+    @pytest.mark.parametrize("socket_directory", ["everyone allow read,write,file_inherit"], indirect=True)
+    async def test_a_directory_whose_allow_entry_a_socket_would_inherit_is_refused(
+        self, socket_directory: Path
+    ) -> None:
+        executor, _reg = _make_executor()
+        server = IpcServer(socket_directory / "ipc.sock", executor, worker_id="w")
+
+        with pytest.raises(PermissionError, match=f"ls -led {socket_directory}.*SIE_IPC_SOCKET_PATH"):
+            await server.start()
+        assert not (socket_directory / "ipc.sock").exists()
+
+    @pytest.mark.parametrize("socket_directory", ["everyone allow read,write,file_inherit"], indirect=True)
+    async def test_a_refused_directory_keeps_the_socket_already_there(self, socket_directory: Path) -> None:
+        path = socket_directory / "ipc.sock"
+        with socket.socket(socket.AF_UNIX) as running:
+            running.bind(str(path))
+            bound = path.stat().st_ino
+            executor, _reg = _make_executor()
+            server = IpcServer(path, executor, worker_id="w")
+
+            with pytest.raises(PermissionError):
+                await server.start()
+            assert path.stat().st_ino == bound
+
+    @pytest.mark.parametrize("socket_directory", ["everyone deny delete"], indirect=True)
+    async def test_a_directory_with_only_a_deny_entry_is_accepted(self, socket_directory: Path) -> None:
+        executor, _reg = _make_executor()
+        server = IpcServer(socket_directory / "ipc.sock", executor, worker_id="w")
+
+        await server.start()
+        try:
+            assert stat.S_IMODE((socket_directory / "ipc.sock").stat().st_mode) == 0o600
+        finally:
+            await server.stop(drain_timeout_s=1.0)
+
+
 # -----------------------------------------------------------------------------
 # Transport / framing
 # -----------------------------------------------------------------------------