diff --git a/app-catalog/services/linkwarden/manifest.yaml b/app-catalog/services/linkwarden/manifest.yaml index 2cbab7022..030cb026c 100644 --- a/app-catalog/services/linkwarden/manifest.yaml +++ b/app-catalog/services/linkwarden/manifest.yaml @@ -19,7 +19,7 @@ install: - data:/data/data ports: [3000] env: - NEXTAUTH_SECRET: "changeme" + NEXTAUTH_SECRET: "{secret_key}" NEXTAUTH_URL: "http://localhost:3000" DATABASE_URL: "postgresql://postgres:postgres@localhost:5432/linkwarden" diff --git a/changelog.d/tsk-teaogm-env-secret-key-substitution.md b/changelog.d/tsk-teaogm-env-secret-key-substitution.md new file mode 100644 index 000000000..ccb8fe1f6 --- /dev/null +++ b/changelog.d/tsk-teaogm-env-secret-key-substitution.md @@ -0,0 +1,16 @@ +### Security +- DockerInstaller now substitutes the per-app `{secret_key}` placeholder in + `install.env` values (not just `config_files` content), so Linkwarden's + `NEXTAUTH_SECRET` is a stable 64-hex-char secret persisted in + `/.secret_key` instead of the shipped default. Previously every host + ran Linkwarden with the publicly-known session-signing secret `changeme`, + allowing session forgery. +- Linkwarden manifest restores the required `DATABASE_URL: "postgresql://postgres:postgres@localhost:5432/linkwarden"`. + Database is now explicitly declared in the manifest to match upstream Linkwarden. +- Generated docker-compose.yaml and config files are now written with permissions 0o600 + to protect any secret substitutions (previous default umask 0o644 exposed secrets + in the live session-signing key). Applies to all files written by + `_write_config_files` and `install` that contain a `{secret_key}` substitution. + +### Fixed +- Fix-forward #2816: restore linkwarden DATABASE_URL (false SQLite premise) and chmod 0600 the generated docker-compose.yml that now carries the real secret diff --git a/tests/test_installers.py b/tests/test_installers.py index df2e52506..a71c39070 100644 --- a/tests/test_installers.py +++ b/tests/test_installers.py @@ -1,18 +1,24 @@ -import json -import pytest from pathlib import Path -from unittest.mock import AsyncMock, patch, MagicMock +from unittest.mock import AsyncMock, patch + +import pytest +import yaml + from tinyagentos.installers.base import get_installer -from tinyagentos.installers.pip_installer import PipInstaller from tinyagentos.installers.docker_installer import DockerInstaller from tinyagentos.installers.download_installer import DownloadInstaller +from tinyagentos.installers.pip_installer import PipInstaller from tinyagentos.installers.port_allocator import ( + _POOL_END, + _POOL_START, RESERVED_PORTS, allocate_host_port, - _POOL_START, - _POOL_END, ) +# Anchor the catalog path on this file so tests never depend on cwd. The repo +# layout is tests/test_installers.py -> /app-catalog. +_CATALOG_ROOT = Path(__file__).resolve().parent.parent / "app-catalog" + class TestGetInstaller: def test_returns_pip(self): @@ -55,20 +61,79 @@ async def test_uninstall_removes_dir(self, tmp_path): class TestDockerInstaller: @pytest.mark.asyncio - async def test_install_writes_compose(self, tmp_path): + def test_compose_file_is_0600(self, tmp_path): + """RED: _generate_compose must write docker-compose.yaml at 0o600 permissions + to protect any secret substitutions (previous default umask 0o644 exposed + secrets in the live session-signing key). Applies to all files + _write_config_files writes whose content had a {secret_key} substitution. + """ installer = DockerInstaller(apps_dir=tmp_path) install_config = { "method": "docker", - "image": "gitea/gitea:1.22", - "volumes": ["data:/data"], - "env": {"ROOT_URL": "http://localhost:3000"}, + "image": "ghcr.io/linkwarden/linkwarden:latest", + "volumes": ["data:/data/data"], + "ports": [3000], + "env": { + "NEXTAUTH_SECRET": "{secret_key}", + "NEXTAUTH_URL": "http://localhost:3000", + "DATABASE_URL": "postgresql://postgres:postgres@localhost:5432/linkwarden" + } } + # Mock run_cmd to avoid docker dependency with patch("tinyagentos.installers.docker_installer.run_cmd", new_callable=AsyncMock) as mock_run: mock_run.return_value = (0, "") - result = await installer.install("gitea", install_config) - assert result["success"] is True - compose_file = tmp_path / "gitea" / "docker-compose.yaml" - assert compose_file.exists() + import asyncio + asyncio.run(installer.install("linkwarden", install_config)) + + compose_path = tmp_path / "linkwarden" / "docker-compose.yaml" + assert compose_path.exists() + stat_mode = compose_path.stat().st_mode + assert (stat_mode & 0o777) == 0o600, f"Expected 0o600, got {oct(stat_mode & 0o777)}" + + def test_preexisting_config_file_is_0600(self, tmp_path): + """When _write_config_files writes a file with {secret_key} substitution, + it must harden the mode even if it already existed at 0o644 from a previous + install (O_CREAT only applies the mode on creation).""" + installer = DockerInstaller(apps_dir=tmp_path) + # Create a pre-existing settings.yml at 0o644 + app_dir = tmp_path / "linkwarden" + app_dir.mkdir(parents=True) + config_file = app_dir / "settings.yml" + config_file.write_text("") + config_file.chmod(0o644) + assert (config_file.stat().st_mode & 0o777) == 0o644 + + # Write config files with {secret_key} substitution + installer._write_config_files("linkwarden", { + "config_files": [ + {"path": "settings.yml", "content": 'secret_key: "{secret_key}"'} + ] + }) + + # The config file should now be 0o600 + stat_mode = config_file.stat().st_mode + assert (stat_mode & 0o777) == 0o600, f"Expected 0o600, got {oct(stat_mode & 0o777)}" + + def test_config_file_with_secret_key_substitution_is_0600(self, tmp_path): + """Config files whose content had a {secret_key} substitution must be written + at 0o600 permissions.""" + installer = DockerInstaller(apps_dir=tmp_path) + installer._write_config_files("searxng", { + "config_files": [ + {"path": "settings.yml", "content": 'secret_key: "{secret_key}"'} + ] + }) + + settings_yml = tmp_path / "searxng" / "settings.yml" + assert settings_yml.exists() + stat_mode = settings_yml.stat().st_mode + assert (stat_mode & 0o777) == 0o600, f"Expected 0o600, got {oct(stat_mode & 0o777)}" + + # The secret key file should also be 0o600 + secret_path = tmp_path / "searxng" / ".secret_key" + assert secret_path.exists() + stat_mode = secret_path.stat().st_mode + assert (stat_mode & 0o777) == 0o600, f"Expected 0o600, got {oct(stat_mode & 0o777)}" def test_generate_compose_declares_named_volumes_and_omits_version(self, tmp_path): # Regression: named volumes (e.g. searxng's "config:/etc/searxng") must @@ -218,6 +283,83 @@ async def test_start_runs_compose_up(self, tmp_path): assert any("up" in c and "-d" in c for c in calls) +class TestLinkwardenSecretSubstitution: + """tsk-teaogm: {secret_key} must be substituted into install.env, not just + config_files content, so Linkwarden's NEXTAUTH_SECRET is per-app and stable. + """ + + @pytest.mark.asyncio + async def test_env_secret_key_substituted_in_compose(self, tmp_path): + manifest_path = _CATALOG_ROOT / "services" / "linkwarden" / "manifest.yaml" + manifest = yaml.safe_load(manifest_path.read_text()) + install_config = manifest["install"] + + async def _render(app_id): + installer = DockerInstaller(apps_dir=tmp_path) + with patch( + "tinyagentos.installers.docker_installer.run_cmd", + new_callable=AsyncMock, + ) as mock_run: + mock_run.return_value = (0, "") + await installer.install(app_id, install_config) + compose = yaml.safe_load( + (tmp_path / app_id / "docker-compose.yaml").read_text() + ) + return compose["services"][app_id]["environment"]["NEXTAUTH_SECRET"] + + secret_a = await _render("linkwarden-a") + secret_b = await _render("linkwarden-b") + + # The substituted secret must be a 64-char hex string, not the + # placeholder literal or the shipped default. + assert len(secret_a) == 64 + assert all(c in "0123456789abcdef" for c in secret_a) + assert secret_a != "{secret_key}" + assert secret_b != "{secret_key}" + + # Different app_dirs get different per-app secrets. + assert secret_a != secret_b + + # Same app_dir rendered twice must reuse the persisted .secret_key. + secret_a_again = await _render("linkwarden-a") + assert secret_a == secret_a_again + + +class TestCatalogManifestAudit: + """tsk-teaogm: catalog manifests must not ship literal secrets. + + Every *_SECRET / *_KEY env value must carry the ``{secret_key}`` placeholder + so the DockerInstaller substitutes a per-app secret, and no env value may be + the shipped default ``"changeme"``. + """ + + def test_no_hardcoded_secrets_in_manifests(self): + manifest_paths = sorted((_CATALOG_ROOT / "services").rglob("manifest.yaml")) + assert manifest_paths, "no service manifests found under app-catalog/services" + + failures: list[str] = [] + secret_suffixes = ("_SECRET", "_KEY") + for mp in manifest_paths: + data = yaml.safe_load(mp.read_text()) + if not isinstance(data, dict): + continue + env = (data.get("install") or {}).get("env") or {} + if not isinstance(env, dict): + continue + rel = mp.relative_to(_CATALOG_ROOT) + for key, val in env.items(): + if not isinstance(val, str): + continue + if val == "changeme": + failures.append(f"{rel}: {key} == 'changeme'") + if key.endswith(secret_suffixes) and "{secret_key}" not in val: + failures.append( + f"{rel}: {key} is a literal secret (no {{secret_key}} placeholder)" + ) + + assert not failures, "; ".join(failures) + + class TestDownloadInstaller: @pytest.mark.asyncio async def test_install_downloads_file(self, tmp_path): diff --git a/tinyagentos/installers/docker_installer.py b/tinyagentos/installers/docker_installer.py index 4fb372af1..4a22ada42 100644 --- a/tinyagentos/installers/docker_installer.py +++ b/tinyagentos/installers/docker_installer.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import secrets import shutil from pathlib import Path @@ -19,6 +20,30 @@ def __init__(self, apps_dir: Path | None = None): def _compose_path(self, app_id: str) -> Path: return self.apps_dir / app_id / "docker-compose.yaml" + def _get_or_create_secret_key(self, app_id: str) -> str: + """Load or create the per-app secret key, persisted in ``/.secret_key``. + + Reused by both ``_write_config_files`` and ``_generate_compose`` so that + apps shipping ``{secret_key}`` in either ``config_files`` content or + ``install.env`` values get the same stable per-app secret. The key is + regenerated when a prior write left it missing, empty, or malformed. + """ + app_dir = self.apps_dir / app_id + secret_key_path = app_dir / ".secret_key" + secret_key = "" + if secret_key_path.exists(): + secret_key = secret_key_path.read_text().strip() + if len(secret_key) != 64 or not all(c in "0123456789abcdef" for c in secret_key): + secret_key = secrets.token_hex(32) + app_dir.mkdir(parents=True, exist_ok=True) + secret_key_path.write_text(secret_key) + secret_key_path.chmod(0o600) + return secret_key + + def _substitute_secret_key(self, value: str, secret_key: str) -> str: + """Replace the ``{secret_key}`` placeholder in a single string value.""" + return value.replace("{secret_key}", secret_key) + def _write_config_files(self, app_id: str, install_config: dict) -> None: """Write declarative config files from the manifest to the app directory. @@ -68,24 +93,20 @@ def _write_config_files(self, app_id: str, install_config: dict) -> None: # Persist secret_key per app so re-installs don't rotate it. It signs # sessions, so keep it owner-only and regenerate if a prior write left # it empty or malformed. - secret_key_path = app_dir / ".secret_key" - secret_key = "" - if secret_key_path.exists(): - secret_key = secret_key_path.read_text().strip() - if len(secret_key) != 64 or not all(c in "0123456789abcdef" for c in secret_key): - secret_key = secrets.token_hex(32) - app_dir.mkdir(parents=True, exist_ok=True) - secret_key_path.write_text(secret_key) - secret_key_path.chmod(0o600) + secret_key = self._get_or_create_secret_key(app_id) for entry in config_files: path = entry["path"] content = entry["content"] if "{secret_key}" in content: - content = content.replace("{secret_key}", secret_key) + content = self._substitute_secret_key(content, secret_key) full_path = app_dir / path full_path.parent.mkdir(parents=True, exist_ok=True) - full_path.write_text(content) + # Write config file with 0600 permissions to protect any secret substitutions + fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w") as f: + f.write(content) + os.chmod(full_path, 0o600) @staticmethod def _is_named_volume(source: str) -> bool: @@ -125,7 +146,18 @@ def _generate_compose( if self._is_named_volume(source): named_volumes[source] = None if "env" in install_config: - service["environment"] = install_config["env"] + # Substitute the per-app {secret_key} placeholder in every env + # string value (e.g. NEXTAUTH_SECRET), reusing the persisted key in + # /.secret_key so re-installs don't rotate it. The key is + # only created when an env value actually carries the placeholder. + env = install_config["env"] + if any("{secret_key}" in v for v in env.values() if isinstance(v, str)): + secret_key = self._get_or_create_secret_key(app_id) + env = { + k: self._substitute_secret_key(v, secret_key) if isinstance(v, str) else v + for k, v in env.items() + } + service["environment"] = env # Collect the container-internal ports from the manifest. container_ports: list[int] = [] @@ -172,7 +204,11 @@ async def install(self, app_id: str, install_config: dict, **kwargs) -> dict: compose, host_port = self._generate_compose(app_id, install_config) compose_path = self._compose_path(app_id) - compose_path.write_text(yaml.dump(compose, default_flow_style=False)) + # Write docker-compose.yaml with 0600 permissions to protect any secret substitutions + fd = os.open(compose_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w") as f: + f.write(yaml.dump(compose, default_flow_style=False)) + os.chmod(compose_path, 0o600) # Pull image code, output = await run_cmd(