diff --git a/packages/sie_server/README.md b/packages/sie_server/README.md index 67991e6ab..db25937a0 100644 --- a/packages/sie_server/README.md +++ b/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 ` 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 ` 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: diff --git a/packages/sie_server/src/sie_server/core/server_auth.py b/packages/sie_server/src/sie_server/core/server_auth.py index 958b4b5e4..e19398d57 100644 --- a/packages/sie_server/src/sie_server/core/server_auth.py +++ b/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# " - 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 "" ' + 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: diff --git a/packages/sie_server/src/sie_server/ipc_server.py b/packages/sie_server/src/sie_server/ipc_server.py index 80db324c7..1d47e16bf 100644 --- a/packages/sie_server/src/sie_server/ipc_server.py +++ b/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), diff --git a/packages/sie_server/tests/core/test_server_auth.py b/packages/sie_server/tests/core/test_server_auth.py index c25524ab8..790c5e551 100644 --- a/packages/sie_server/tests/core/test_server_auth.py +++ b/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# {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 "" {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: diff --git a/packages/sie_server/tests/test_ipc_server.py b/packages/sie_server/tests/test_ipc_server.py index 05b2e54ac..be75a3bc8 100644 --- a/packages/sie_server/tests/test_ipc_server.py +++ b/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 # ----------------------------------------------------------------------------- diff --git a/packages/sie_server_sidecar/README.md b/packages/sie_server_sidecar/README.md index d60531015..9bc6ba727 100644 --- a/packages/sie_server_sidecar/README.md +++ b/packages/sie_server_sidecar/README.md @@ -36,7 +36,12 @@ and the legacy representation for mixed-version compatibility. The local socket is an access-controlled process boundary, not an authentication protocol. The listener restricts its socket node, validates a domain-separated request digest and route identity, caps frame/media/request -sizes, and rejects unresolved payload references. +sizes, and rejects unresolved payload references. The socket node's mode +(`0600`) is the access control, and on macOS it does not remove an entry that +allows access, which a socket can inherit from its directory's access control +list. On macOS, put `SIE_SIDECAR_LOCAL_SOCKET` in a directory whose access +control list has no entry that allows access (`ls -led ` lists the +entries). NATS credentials come from `SIE_NATS_USER` and `SIE_NATS_PASSWORD`, which are read from the environment only and must be set together. Credentials embedded