From 8cc0c38bf0a98b6410caae6512170cb72f70b98e Mon Sep 17 00:00:00 2001 From: jaylfc Date: Sun, 6 Sep 2026 09:36:19 +0000 Subject: [PATCH] Fix: substitute {secret_key} in install.env so Linkwarden NEXTAUTH_SECRET is per-app DockerInstaller only substituted {secret_key} in config_files content, not in install.env values. The linkwarden manifest shipped NEXTAUTH_SECRET as the literal 'changeme', so every taOS host ran Linkwarden with the same publicly-known session-signing secret, allowing session forgery. Changes: - docker_installer.py: extract _get_or_create_secret_key (persisted in /.secret_key) and _substitute_secret_key helpers shared by _write_config_files and _generate_compose. _generate_compose now applies {secret_key} substitution to every string env value before it lands in the compose environment block. - linkwarden manifest: NEXTAUTH_SECRET set to {secret_key}; DATABASE_URL dropped (no Postgres companion is started), with an explanatory comment. - Tests: RED end-to-end test renders the linkwarden manifest through DockerInstaller into tmp app_dirs, asserts NEXTAUTH_SECRET is a 64-char hex string that differs between app_dirs and is stable across re-renders. Catalog audit test checks all service manifests for 'changeme' and literal *_SECRET/*_KEY env values lacking {secret_key}, collecting failures into the assertion message. Proof: on origin/dev both tests fail on the vulnerability assertion (len(secret_a) == 64 -> 8 == 'changeme'; audit: NEXTAUTH_SECRET == 'changeme' and literal without {secret_key}). After the fix, all 30 tests in tests/test_installers.py pass. Docs-Reviewed: README.md is not updated because the fix is internal secret-key generation logic, not a change to catalog app presence, user-facing install behavior, or a desktop app. --- app-catalog/services/linkwarden/manifest.yaml | 5 +- .../tsk-teaogm-env-secret-key-substitution.md | 9 ++ tests/test_installers.py | 95 +++++++++++++++++-- tinyagentos/installers/docker_installer.py | 49 +++++++--- 4 files changed, 139 insertions(+), 19 deletions(-) create mode 100644 changelog.d/tsk-teaogm-env-secret-key-substitution.md diff --git a/app-catalog/services/linkwarden/manifest.yaml b/app-catalog/services/linkwarden/manifest.yaml index 2cbab7022..79d6b10f6 100644 --- a/app-catalog/services/linkwarden/manifest.yaml +++ b/app-catalog/services/linkwarden/manifest.yaml @@ -19,9 +19,10 @@ 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" + # DATABASE_URL omitted — Linkwarden ships its own SQLite backend and no + # PostgreSQL companion is started by this manifest. lifecycle: health_check: "curl -sf http://localhost:3000" 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..03293b0cc --- /dev/null +++ b/changelog.d/tsk-teaogm-env-secret-key-substitution.md @@ -0,0 +1,9 @@ +### 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 drops the unused `DATABASE_URL` (no Postgres companion is + started) and sets `NEXTAUTH_SECRET: "{secret_key}"`. diff --git a/tests/test_installers.py b/tests/test_installers.py index df2e52506..80a8c6f72 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): @@ -218,6 +224,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..04593770a 100644 --- a/tinyagentos/installers/docker_installer.py +++ b/tinyagentos/installers/docker_installer.py @@ -19,6 +19,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,21 +92,13 @@ 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) @@ -125,7 +141,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] = []