From 8003fcbebeba68ee8c8ea9faea020114a4b19b07 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Tue, 19 May 2026 08:09:33 -0700 Subject: [PATCH] feat(admin): align turnstone-admin DB config with server (config.toml + env) (#531) * feat(admin): align turnstone-admin DB config with server (config.toml + env) turnstone-admin previously read TURNSTONE_DB_* env vars only, forcing operators with credentials in config.toml to re-export them just to run admin commands. Wire add_config_arg + apply_config(["database"]) into main() so admin honors the same precedence as turnstone-server: CLI / config.toml [database] > TURNSTONE_DB_* env > hardcoded defaults. Also exposes pool_size + sslmode/sslrootcert/sslcert/sslkey to admin, which previously dropped any such config silently. Hardening: load_config() now warns once when config.toml is group- or world-readable, since DB password and TLS key paths live in [database]. Tests cover precedence (default / config / env / partial fallback / empty-string-in-config-beats-env), the real init_storage boundary on a tmp sqlite path, the sys.argv -> main() pre-parser path, and the new permission check (mode 0644 warns, 0600 quiet). * test(admin): unify config import style in test_admin_db_config Use module alias (config_mod.apply_config) instead of mixing 'import turnstone.core.config as config_mod' with 'from turnstone.core.config import apply_config'. Addresses github-code-quality bot feedback on PR #531. --- tests/test_admin_db_config.py | 220 ++++++++++++++++++++++++++++++++++ tests/test_config.py | 35 ++++++ turnstone/admin.py | 54 ++++++--- turnstone/core/config.py | 22 ++++ 4 files changed, 317 insertions(+), 14 deletions(-) create mode 100644 tests/test_admin_db_config.py diff --git a/tests/test_admin_db_config.py b/tests/test_admin_db_config.py new file mode 100644 index 00000000..35b55b1c --- /dev/null +++ b/tests/test_admin_db_config.py @@ -0,0 +1,220 @@ +"""Tests for turnstone-admin DB configuration precedence. + +Locks in the alignment with turnstone-server: + CLI / config.toml [database] > TURNSTONE_DB_* env > hardcoded default + +The motivation is to keep DB secrets in config.toml (see +feedback_secrets_not_in_env) rather than forcing operators to export +TURNSTONE_DB_URL before every admin invocation. +""" + +from __future__ import annotations + +import argparse +from typing import TYPE_CHECKING +from unittest.mock import patch + +import pytest + +if TYPE_CHECKING: + from collections.abc import Iterator + from pathlib import Path + +import turnstone.core.config as config_mod +from turnstone.admin import _get_storage + + +def _reset_cache() -> None: + config_mod._cache = None + config_mod._config_path = None + + +def _build_args(config_path: str | None) -> argparse.Namespace: + """Build an args namespace the way admin.main() does. + + Skips ``add_config_arg`` (which reads ``sys.argv``) — the test + constructs the args programmatically instead. + """ + config_mod.set_config_path(config_path or "/nonexistent/turnstone-admin-test.toml") + parser = argparse.ArgumentParser() + config_mod.apply_config(parser, ["database"]) + sub = parser.add_subparsers(dest="command") + sub.add_parser("list-users") + return parser.parse_args(["list-users"]) + + +@pytest.fixture(autouse=True) +def _clear_db_env(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + """Clean slate: no TURNSTONE_DB_* env vars unless a test sets them.""" + for var in ( + "TURNSTONE_DB_BACKEND", + "TURNSTONE_DB_URL", + "TURNSTONE_DB_PATH", + "TURNSTONE_DB_POOL_SIZE", + "TURNSTONE_DB_SSLMODE", + "TURNSTONE_DB_SSLROOTCERT", + "TURNSTONE_DB_SSLCERT", + "TURNSTONE_DB_SSLKEY", + "TURNSTONE_CONFIG", + ): + monkeypatch.delenv(var, raising=False) + _reset_cache() + yield + _reset_cache() + + +def test_defaults_to_sqlite_when_neither_config_nor_env_set() -> None: + args = _build_args(None) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + assert init.call_args.args == ("sqlite",) + assert init.call_args.kwargs["url"] == "" + assert init.call_args.kwargs["path"] == "" + assert init.call_args.kwargs["pool_size"] == 2 + + +def test_config_toml_database_section_drives_init_storage(tmp_path: Path) -> None: + cfg = tmp_path / "config.toml" + cfg.write_text( + "[database]\n" + 'backend = "postgresql"\n' + 'url = "postgresql+psycopg://fromconfig:x@host/db"\n' + "pool_size = 5\n" + 'sslmode = "verify-full"\n' + 'sslrootcert = "/etc/ssl/ca.pem"\n' + 'sslcert = "/etc/ssl/client.pem"\n' + 'sslkey = "/etc/ssl/client.key"\n' + ) + args = _build_args(str(cfg)) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + assert init.call_args.args == ("postgresql",) + kw = init.call_args.kwargs + assert kw["url"] == "postgresql+psycopg://fromconfig:x@host/db" + assert kw["pool_size"] == 5 + assert kw["sslmode"] == "verify-full" + assert kw["sslrootcert"] == "/etc/ssl/ca.pem" + assert kw["sslcert"] == "/etc/ssl/client.pem" + assert kw["sslkey"] == "/etc/ssl/client.key" + + +def test_env_used_as_fallback_when_config_absent(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("TURNSTONE_DB_BACKEND", "postgresql") + monkeypatch.setenv("TURNSTONE_DB_URL", "postgresql+psycopg://fromenv:x@host/db") + monkeypatch.setenv("TURNSTONE_DB_POOL_SIZE", "7") + monkeypatch.setenv("TURNSTONE_DB_SSLMODE", "require") + + args = _build_args(None) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + assert init.call_args.args == ("postgresql",) + kw = init.call_args.kwargs + assert kw["url"] == "postgresql+psycopg://fromenv:x@host/db" + assert kw["pool_size"] == 7 + assert kw["sslmode"] == "require" + + +def test_config_toml_wins_over_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """config.toml beats env — operators should put secrets in TOML.""" + monkeypatch.setenv("TURNSTONE_DB_BACKEND", "sqlite") + monkeypatch.setenv("TURNSTONE_DB_URL", "postgresql+psycopg://fromenv:x@host/db") + monkeypatch.setenv("TURNSTONE_DB_SSLMODE", "require") + + cfg = tmp_path / "config.toml" + cfg.write_text( + "[database]\n" + 'backend = "postgresql"\n' + 'url = "postgresql+psycopg://fromconfig:x@host/db"\n' + 'sslmode = "verify-full"\n' + ) + args = _build_args(str(cfg)) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + assert init.call_args.args == ("postgresql",) + kw = init.call_args.kwargs + assert kw["url"] == "postgresql+psycopg://fromconfig:x@host/db" + assert kw["sslmode"] == "verify-full" + + +def test_partial_config_falls_through_to_env_per_key( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A key missing from [database] should fall back to its env var.""" + monkeypatch.setenv("TURNSTONE_DB_SSLMODE", "require") + monkeypatch.setenv("TURNSTONE_DB_POOL_SIZE", "9") + + cfg = tmp_path / "config.toml" + cfg.write_text( + '[database]\nbackend = "postgresql"\nurl = "postgresql+psycopg://fromconfig:x@host/db"\n' + ) + args = _build_args(str(cfg)) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + kw = init.call_args.kwargs + assert kw["url"] == "postgresql+psycopg://fromconfig:x@host/db" + assert kw["sslmode"] == "require" + assert kw["pool_size"] == 9 + + +def test_empty_string_in_config_beats_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """`url = ""` in config.toml beats an env var. + + Locks in the `is not None` guard — a falsy-but-present TOML value + should NOT silently fall through to the env fallback. + """ + monkeypatch.setenv("TURNSTONE_DB_URL", "postgresql+psycopg://fromenv:x@host/db") + cfg = tmp_path / "config.toml" + cfg.write_text('[database]\nbackend = "sqlite"\nurl = ""\n') + args = _build_args(str(cfg)) + with patch("turnstone.core.storage.init_storage") as init: + _get_storage(args) + assert init.call_args.kwargs["url"] == "" + + +def test_main_threads_config_toml_through_real_argv( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """End-to-end: ``turnstone-admin --config list-users`` honors TOML. + + Covers the ``add_config_arg`` -> ``apply_config`` -> ``_get_storage`` + chain that the programmatic ``_build_args`` helper skips. + """ + cfg = tmp_path / "config.toml" + cfg.write_text( + '[database]\nbackend = "postgresql"\nurl = "postgresql+psycopg://fromcli:x@host/db"\n' + ) + monkeypatch.setattr("sys.argv", ["turnstone-admin", "--config", str(cfg), "list-users"]) + + fake_storage = patch("turnstone.core.storage.init_storage").start() + fake_storage.return_value.list_users.return_value = [] + try: + from turnstone.admin import main + + main() + finally: + patch.stopall() + + assert fake_storage.call_args.args == ("postgresql",) + assert fake_storage.call_args.kwargs["url"] == "postgresql+psycopg://fromcli:x@host/db" + + +def test_get_storage_initializes_real_sqlite_backend(tmp_path: Path) -> None: + """Drives the real ``init_storage`` boundary on a fresh sqlite file. + + Mock-only tests would miss a kwarg-name typo (sslmode -> ssl_mode). + This test trips on any such drift because Alembic + the backend + actually run. + """ + from turnstone.core.storage import reset_storage + + db_file = tmp_path / "admin.db" + cfg = tmp_path / "config.toml" + cfg.write_text(f'[database]\nbackend = "sqlite"\npath = "{db_file}"\n') + args = _build_args(str(cfg)) + + reset_storage() + try: + storage = _get_storage(args) + assert storage.list_users() == [] + finally: + reset_storage() diff --git a/tests/test_config.py b/tests/test_config.py index 2c2e0948..2a5c3701 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -50,6 +50,41 @@ def test_load_config_invalid_toml(tmp_path): assert load_config() == {} +def test_load_config_warns_when_world_readable(tmp_path, caplog): + """Secrets in config.toml — warn if anyone but the owner can read it.""" + import logging + import os + + _reset_cache() + cfg = tmp_path / "config.toml" + cfg.write_text('[database]\nurl = "postgresql+psycopg://u:secret@h/d"\n') + os.chmod(cfg, 0o644) + set_config_path(str(cfg)) + + with caplog.at_level(logging.WARNING, logger="turnstone.core.config"): + load_config() + + messages = [r.getMessage() for r in caplog.records] + assert any("group/world-readable" in m for m in messages) + + +def test_load_config_quiet_when_mode_0600(tmp_path, caplog): + import logging + import os + + _reset_cache() + cfg = tmp_path / "config.toml" + cfg.write_text('[database]\nurl = "postgresql+psycopg://u:secret@h/d"\n') + os.chmod(cfg, 0o600) + set_config_path(str(cfg)) + + with caplog.at_level(logging.WARNING, logger="turnstone.core.config"): + load_config() + + messages = [r.getMessage() for r in caplog.records] + assert not any("group/world-readable" in m for m in messages) + + def test_load_config_caches(tmp_path): _reset_cache() cfg = tmp_path / "config.toml" diff --git a/turnstone/admin.py b/turnstone/admin.py index 7bceb18e..3362efc7 100644 --- a/turnstone/admin.py +++ b/turnstone/admin.py @@ -12,14 +12,36 @@ import uuid from typing import Any -def _get_storage() -> Any: - """Initialize and return the storage backend.""" +def _get_storage(args: argparse.Namespace) -> Any: + """Initialize and return the storage backend. + + Precedence (matches turnstone-server): CLI / config.toml ``[database]`` + > ``TURNSTONE_DB_*`` env vars > hardcoded defaults. + """ from turnstone.core.storage import init_storage - db_backend = os.environ.get("TURNSTONE_DB_BACKEND", "sqlite") - db_url = os.environ.get("TURNSTONE_DB_URL", "") - db_path = os.environ.get("TURNSTONE_DB_PATH", "") - return init_storage(db_backend, path=db_path, url=db_url) + def _pick(arg_name: str, env_name: str, default: str = "") -> Any: + # `is not None` (not truthy) so a legitimate falsy TOML value + # like `pool_size = 0` or `url = ""` still beats the env fallback. + val = getattr(args, arg_name, None) + if val is not None: + return val + return os.environ.get(env_name, default) + + db_backend = str(_pick("db_backend", "TURNSTONE_DB_BACKEND", "sqlite")) + db_url = str(_pick("db_url", "TURNSTONE_DB_URL")) + db_path = str(_pick("db_path", "TURNSTONE_DB_PATH")) + db_pool_size = int(_pick("db_pool_size", "TURNSTONE_DB_POOL_SIZE", "2")) + return init_storage( + db_backend, + path=db_path, + url=db_url, + pool_size=db_pool_size, + sslmode=str(_pick("db_sslmode", "TURNSTONE_DB_SSLMODE")), + sslrootcert=str(_pick("db_sslrootcert", "TURNSTONE_DB_SSLROOTCERT")), + sslcert=str(_pick("db_sslcert", "TURNSTONE_DB_SSLCERT")), + sslkey=str(_pick("db_sslkey", "TURNSTONE_DB_SSLKEY")), + ) def _cmd_create_user(args: argparse.Namespace) -> None: @@ -37,7 +59,7 @@ def _cmd_create_user(args: argparse.Namespace) -> None: print("Error: invalid username (1-64 chars: letters, digits, . _ -)", file=sys.stderr) sys.exit(1) - storage = _get_storage() + storage = _get_storage(args) user_id = uuid.uuid4().hex # Prompt for password @@ -76,7 +98,7 @@ def _cmd_create_user(args: argparse.Namespace) -> None: def _cmd_create_token(args: argparse.Namespace) -> None: from turnstone.core.auth import generate_token, hash_token, token_prefix - storage = _get_storage() + storage = _get_storage(args) if storage.get_user(args.user) is None: print(f"Error: user {args.user} not found", file=sys.stderr) @@ -110,7 +132,7 @@ def _cmd_create_token(args: argparse.Namespace) -> None: def _cmd_list_users(args: argparse.Namespace) -> None: - storage = _get_storage() + storage = _get_storage(args) users = storage.list_users() if not users: print("No users found.") @@ -120,7 +142,7 @@ def _cmd_list_users(args: argparse.Namespace) -> None: def _cmd_list_tokens(args: argparse.Namespace) -> None: - storage = _get_storage() + storage = _get_storage(args) tokens = storage.list_api_tokens(args.user) if not tokens: print(f"No tokens found for user {args.user}.") @@ -134,7 +156,7 @@ def _cmd_list_tokens(args: argparse.Namespace) -> None: def _cmd_revoke_token(args: argparse.Namespace) -> None: - storage = _get_storage() + storage = _get_storage(args) if storage.delete_api_token(args.token_id): print(f"Revoked token {args.token_id}") else: @@ -297,7 +319,7 @@ def _cmd_list_node_metadata(args: argparse.Namespace) -> None: """List metadata for a node.""" import json - storage = _get_storage() + storage = _get_storage(args) rows = storage.get_node_metadata(args.node_id) if not rows: print(f"No metadata for node: {args.node_id}") @@ -324,7 +346,7 @@ def _cmd_set_node_metadata(args: argparse.Namespace) -> None: """Set a metadata key on a node.""" import json - storage = _get_storage() + storage = _get_storage(args) # Check for auto-source conflict existing = storage.get_node_metadata(args.node_id) @@ -345,7 +367,7 @@ def _cmd_set_node_metadata(args: argparse.Namespace) -> None: def _cmd_delete_node_metadata(args: argparse.Namespace) -> None: """Delete a metadata key from a node.""" - storage = _get_storage() + storage = _get_storage(args) existing = storage.get_node_metadata(args.node_id) for r in existing: @@ -395,6 +417,10 @@ def main() -> None: prog="turnstone-admin", description="Turnstone user and token administration", ) + from turnstone.core.config import add_config_arg, apply_config + + add_config_arg(parser) + apply_config(parser, ["database"]) sub = parser.add_subparsers(dest="command") p_cu = sub.add_parser("create-user", help="Create a new user") diff --git a/turnstone/core/config.py b/turnstone/core/config.py index 60ca523b..c23f2323 100644 --- a/turnstone/core/config.py +++ b/turnstone/core/config.py @@ -54,6 +54,27 @@ def set_config_path(path: str) -> None: _cache = None # invalidate cache so next load_config() re-reads +def _warn_if_world_readable(cfg_path: Path) -> None: + """Warn once if config.toml is group- or world-readable. + + DB passwords, OIDC client secrets, and TLS key paths live in this + file — operators usually want it at 0600. POSIX-only; no-ops where + ``stat()`` modes are meaningless (Windows). + """ + try: + mode = cfg_path.stat().st_mode & 0o777 + except OSError: + return + if mode & 0o077: + log.warning( + "%s is mode %04o (group/world-readable); secrets live here — " + "run `chmod 0600 %s` to restrict access", + cfg_path, + mode, + cfg_path, + ) + + def load_config(section: str | None = None) -> dict[str, Any]: """Load config.toml and return the full dict or a specific section. @@ -66,6 +87,7 @@ def load_config(section: str | None = None) -> dict[str, Any]: cfg_path = _resolve_config_path() if cfg_path.is_file(): try: + _warn_if_world_readable(cfg_path) _cache = tomllib.loads(cfg_path.read_text(encoding="utf-8")) except Exception as exc: log.warning("Failed to parse %s: %s", cfg_path, exc)