From afc623ec58e90a93d5e53b0e8a94774c9cdbad92 Mon Sep 17 00:00:00 2001 From: Paul Payne Date: Tue, 14 Jul 2026 07:17:17 -0700 Subject: [PATCH] Add `castle secret` CLI and fail-loud unresolved-secret apply gate Writing a secret by hand meant knowing the active backend and its layout; getting it wrong (a file write on an OpenBao fleet) left the value where the resolver never reads it, so ${secret:NAME} silently degraded to the literal placeholder that a service then used as its credential. - `castle secret {list|set|get|rm}` reads/writes the ACTIVE backend, so there's no wrong store to pick. `set NAME` with no value reads a hidden prompt / stdin. - `castle apply` (and --plan) now refuses to converge any deployment whose ${secret:...} refs don't resolve in the active backend: exits non-zero, writes nothing, and names each deployment + secret + the `castle secret set` fix. - New helpers in core/config.py: active_secret_backend(), active_backend_name(), secret_refs(). Gate impl: deploy._unresolved_secrets() + ApplyResult.blocked. - Tests: test_deploy_secret_gate.py, TestSecretRefs, test_secret.py. Docs: AGENTS.md. --- AGENTS.md | 14 ++++ cli/src/castle_cli/commands/apply.py | 8 +++ cli/src/castle_cli/commands/secret.py | 93 +++++++++++++++++++++++++++ cli/src/castle_cli/main.py | 20 ++++++ cli/tests/test_secret.py | 56 ++++++++++++++++ core/src/castle_core/config.py | 39 ++++++++++- core/src/castle_core/deploy.py | 69 ++++++++++++++++++++ core/tests/test_config.py | 17 +++++ core/tests/test_deploy_secret_gate.py | 65 +++++++++++++++++++ 9 files changed, 379 insertions(+), 2 deletions(-) create mode 100644 cli/src/castle_cli/commands/secret.py create mode 100644 cli/tests/test_secret.py create mode 100644 core/tests/test_deploy_secret_gate.py diff --git a/AGENTS.md b/AGENTS.md index 4ab3779..75d37e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,12 @@ castle job ... castle tool list [--json] # each tool's executable + description + install state castle tool info [--json] +# Secrets — read/write the ACTIVE backend (file or openbao); never hand-write the store +castle secret list # names in the active backend +castle secret set [VALUE] # VALUE omitted → hidden prompt / stdin +castle secret get # print a value +castle secret rm [-y] # delete + # Platform-wide castle apply [name] [--plan] # converge runtime to config — the workhorse castle list [--kind ...] [--stack ...] [--json] # all deployments @@ -271,6 +277,14 @@ secrets: via a least-privilege `castle-read` token. Only the bootstrap tier stays as files (`OPENBAO_ROOT_TOKEN`/`OPENBAO_UNSEAL_KEY`, the `cloudflared` creds dir). +**Write secrets with `castle secret set NAME`, never by hand.** It targets the +*active* backend, so a value can't land in a store the resolver never reads — the +failure mode where a file-written secret is silently ignored on an openbao fleet +and `${secret:NAME}` degrades to a literal `` that a service +then uses as its credential. `castle apply` **refuses to converge** (exits +non-zero, writes nothing) any deployment whose `${secret:…}` refs don't resolve in +the active backend, naming the fix. + **Never** put secret *values* in `castle.yaml` or project dirs — use `${secret:…}`. Roots: **`CASTLE_HOME`** (config/code/artifacts/secrets, default `~/.castle`, env-only — it *contains* castle.yaml) and **program data** (base of `${data_dir}`, diff --git a/cli/src/castle_cli/commands/apply.py b/cli/src/castle_cli/commands/apply.py index cf97701..60ee04a 100644 --- a/cli/src/castle_cli/commands/apply.py +++ b/cli/src/castle_cli/commands/apply.py @@ -35,6 +35,14 @@ def run_apply(args: argparse.Namespace) -> int: result = apply(target_name=target, plan=plan) + # Unresolved secrets abort the run before any change — print the error block + # (which names each deployment, the missing ${secret:…}, and the fix) and exit + # non-zero so a broken credential never slips through as a bogus placeholder. + if result.blocked: + for msg in result.messages: + print(f" {_C['deactivate']}{msg}{_C['reset']}") + return 1 + # Surface any warnings the render produced (acme prerequisites, tunnel notes). for msg in result.messages: if msg.startswith("Warning"): diff --git a/cli/src/castle_cli/commands/secret.py b/cli/src/castle_cli/commands/secret.py new file mode 100644 index 0000000..3ee5bf9 --- /dev/null +++ b/cli/src/castle_cli/commands/secret.py @@ -0,0 +1,93 @@ +"""castle secret — read/write secrets in the *active* backend. + +The active backend (file or openbao) is selected by the ``secrets:`` block of +castle.yaml (env overrides). Writing a secret by hand means knowing that choice +and the backend's storage layout — get it wrong and the value lands somewhere the +resolver never reads, so ``${secret:NAME}`` silently degrades to a +```` placeholder that a service then uses as if it were the +real credential (exactly how immich's DB password went to a file on an OpenBao +fleet). This command routes every read/write through +:func:`castle_core.config.active_secret_backend`, so there's no wrong store to +pick. ``castle apply`` refuses to converge a deployment whose secrets don't +resolve here. +""" + +from __future__ import annotations + +import argparse +import getpass +import sys + +from castle_core.config import active_backend_name, active_secret_backend + + +def run_secret(args: argparse.Namespace) -> int: + sub = getattr(args, "secret_command", None) + if not sub: + print("Usage: castle secret {list|set|get|rm}") + return 1 + if sub == "list": + return _list() + if sub == "set": + return _set(args.name, args.value) + if sub == "get": + return _get(args.name) + if sub == "rm": + return _rm(args.name, getattr(args, "yes", False)) + return 1 + + +def _list() -> int: + backend = active_backend_name() + names = active_secret_backend().list_names() + if not names: + print(f"No secrets in the active '{backend}' backend.") + return 0 + print(f"Secrets in the active '{backend}' backend ({len(names)}):") + for n in names: + print(f" {n}") + return 0 + + +def _set(name: str, value: str | None) -> int: + backend = active_backend_name() + if value is None: + # No value on the argv (keeps it out of shell history / ps). Read from a + # hidden prompt when interactive, else from stdin (pipe-friendly). + if sys.stdin.isatty(): + value = getpass.getpass(f"Value for {name}: ") + else: + value = sys.stdin.read().strip() + if not value: + print("Error: empty value — refusing to set.", file=sys.stderr) + return 1 + active_secret_backend().write(name, value) + print(f"Set '{name}' in the active '{backend}' backend.") + return 0 + + +def _get(name: str) -> int: + value = active_secret_backend().read(name) + if value is None: + print( + f"Secret '{name}' not found in the active '{active_backend_name()}' backend.", + file=sys.stderr, + ) + return 1 + print(value) + return 0 + + +def _rm(name: str, yes: bool) -> int: + backend = active_backend_name() + if active_secret_backend().read(name) is None: + print(f"Secret '{name}' not found in the active '{backend}' backend.", file=sys.stderr) + return 1 + if not yes: + reply = input(f"Delete secret '{name}' from the '{backend}' backend? [y/N] ") + if reply.strip().lower() not in ("y", "yes"): + print("Aborted.") + return 1 + active_secret_backend().delete(name) + print(f"Deleted '{name}' from the active '{backend}' backend.") + return 0 diff --git a/cli/src/castle_cli/main.py b/cli/src/castle_cli/main.py index 73bb7d9..ceea3fe 100644 --- a/cli/src/castle_cli/main.py +++ b/cli/src/castle_cli/main.py @@ -212,6 +212,22 @@ def build_parser() -> argparse.ArgumentParser: ) tls_sub.add_parser("status", help="Show each TLS service's cert fingerprint + expiry") + # Secrets — read/write the ACTIVE backend (file or openbao), so a value never + # gets hand-written to the wrong store (the mistake that silently shadows an + # OpenBao fleet's secret with a stray file the vault never reads). + sec = subparsers.add_parser("secret", help="Manage secrets in the active backend") + sec.set_defaults(resource="secret") + sec_sub = sec.add_subparsers(dest="secret_command") + sec_sub.add_parser("list", help="List secret names in the active backend") + p = sec_sub.add_parser("set", help="Set a secret (prompts if VALUE omitted)") + p.add_argument("name", help="Secret name") + p.add_argument("value", nargs="?", help="Value (omit to read from a hidden prompt / stdin)") + p = sec_sub.add_parser("get", help="Read a secret's value") + p.add_argument("name", help="Secret name") + p = sec_sub.add_parser("rm", help="Delete a secret from the active backend") + p.add_argument("name", help="Secret name") + p.add_argument("-y", "--yes", action="store_true", help="Skip confirmation") + # Convergence — the one lifecycle verb. Renders units/Caddyfile/tunnel, then # reconciles the runtime to match config (activate/restart/deactivate). p = subparsers.add_parser( @@ -380,6 +396,10 @@ def main() -> int: from castle_cli.commands.tls import run_tls return run_tls(args) + if cmd == "secret": + from castle_cli.commands.secret import run_secret + + return run_secret(args) if cmd == "apply": from castle_cli.commands.apply import run_apply diff --git a/cli/tests/test_secret.py b/cli/tests/test_secret.py new file mode 100644 index 0000000..3db994a --- /dev/null +++ b/cli/tests/test_secret.py @@ -0,0 +1,56 @@ +"""Tests for the `castle secret` command (reads/writes the active backend).""" + +from __future__ import annotations + +from argparse import Namespace +from pathlib import Path + +import pytest + + +@pytest.fixture +def file_secrets(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Force the active backend to a temp file store.""" + secrets = tmp_path / "secrets" + secrets.mkdir() + monkeypatch.setenv("CASTLE_SECRET_BACKEND", "file") + monkeypatch.setattr("castle_core.config.SECRETS_DIR", secrets) + return secrets + + +def _run(**kw: object) -> int: + from castle_cli.commands.secret import run_secret + + return run_secret(Namespace(**kw)) + + +class TestSecretRoundtrip: + def test_set_get_list_rm(self, file_secrets: Path, capsys: object) -> None: + assert _run(secret_command="set", name="MY_KEY", value="s3cret") == 0 + capsys.readouterr() # type: ignore[attr-defined] # drain the "Set …" line + + assert _run(secret_command="get", name="MY_KEY") == 0 + assert capsys.readouterr().out.strip() == "s3cret" # type: ignore[attr-defined] + + assert _run(secret_command="list") == 0 + assert "MY_KEY" in capsys.readouterr().out # type: ignore[attr-defined] + + assert _run(secret_command="rm", name="MY_KEY", yes=True) == 0 + # Gone → non-zero. + assert _run(secret_command="get", name="MY_KEY") == 1 + + +class TestSecretEdges: + def test_get_missing_is_nonzero(self, file_secrets: Path) -> None: + assert _run(secret_command="get", name="ABSENT") == 1 + + def test_empty_value_refused(self, file_secrets: Path) -> None: + # Explicit empty string on argv is refused (not silently stored). + assert _run(secret_command="set", name="K", value="") == 1 + + def test_rm_missing_is_nonzero(self, file_secrets: Path) -> None: + assert _run(secret_command="rm", name="ABSENT", yes=True) == 1 + + def test_no_subcommand_usage(self, file_secrets: Path, capsys: object) -> None: + assert _run(secret_command=None) == 1 + assert "Usage" in capsys.readouterr().out # type: ignore[attr-defined] diff --git a/core/src/castle_core/config.py b/core/src/castle_core/config.py index 1006934..f6bf406 100644 --- a/core/src/castle_core/config.py +++ b/core/src/castle_core/config.py @@ -6,10 +6,14 @@ import os import re from dataclasses import InitVar, dataclass, field from pathlib import Path +from typing import TYPE_CHECKING import yaml from pydantic import BaseModel, TypeAdapter +if TYPE_CHECKING: + from castle_core.secret_backends import SecretBackend + from castle_core.manifest import ( AgentSpec, DeploymentSpec, @@ -328,15 +332,46 @@ def _secrets_settings() -> dict: return {} +def active_secret_backend() -> SecretBackend: + """The active secret backend (file or openbao), per ``secrets:`` + env. + + The one place to get a handle for reading/writing/listing secrets, so callers + (the ``castle secret`` CLI, preflight checks) act on the *active* backend + rather than guessing the filesystem — the mistake that silently shadows an + OpenBao fleet's secret with a stray file. + """ + from castle_core.secret_backends import build_backend + + return build_backend(SECRETS_DIR, _secrets_settings()) + + +def active_backend_name() -> str: + """Human name of the active backend (``openbao`` | ``file``) for messages.""" + return (os.environ.get("CASTLE_SECRET_BACKEND") or _secrets_settings().get("backend") or "file").lower() + + def read_secret(name: str) -> str | None: """Resolve a secret via the active backend, or None if unset. The public helper for code that reads secrets directly (DNS/supabase/etc.) so everything goes through the one backend rather than the filesystem. """ - from castle_core.secret_backends import build_backend + return active_secret_backend().read(name) - return build_backend(SECRETS_DIR, _secrets_settings()).read(name) + +# ``${secret:NAME}`` references embedded in an env value (the grammar +# :func:`resolve_env_split` resolves). Shared by the deploy-time preflight that +# refuses to converge a deployment whose secrets the active backend can't resolve. +_SECRET_REF_RE = re.compile(r"\$\{secret:([^}]+)\}") + + +def secret_refs(value: str) -> list[str]: + """The secret names a value references via ``${secret:NAME}`` (order-preserving, + deduped).""" + seen: dict[str, None] = {} + for m in _SECRET_REF_RE.finditer(value): + seen.setdefault(m.group(1), None) + return list(seen) def _read_secret(name: str) -> str: diff --git a/core/src/castle_core/deploy.py b/core/src/castle_core/deploy.py index a813095..fdf81e0 100644 --- a/core/src/castle_core/deploy.py +++ b/core/src/castle_core/deploy.py @@ -97,6 +97,11 @@ class ApplyResult: # True for a `--plan` run: the diff was computed but nothing was written or # activated. Lets callers render "would activate…" vs "activated…". planned: bool = False + # Deployments refused because their ``${secret:NAME}`` references don't resolve + # in the active backend. A non-empty list means apply made NO changes and the + # caller should exit non-zero — better than starting a service with a bogus + # ```` value silently standing in for the real credential. + blocked: list[str] = field(default_factory=list) @property def changed(self) -> bool: @@ -279,6 +284,7 @@ def apply( """ import asyncio + from castle_core.config import active_backend_name from castle_core.lifecycle import activate, deactivate, is_active config = load_config(root) @@ -291,6 +297,28 @@ def apply( ] names = [n for _k, n, _d in items] + # Fail loud on unresolved secrets BEFORE any render/write. Building a deployment + # writes its secret env file, so this gate runs first — a service is never + # rendered or (re)started with a bogus `` value silently + # standing in for a real credential (the failure that shipped immich's DB + # password to the wrong backend). Aborts the whole run with a fix hint; nothing + # is written or reconciled. + blocked = _unresolved_secrets(items) + if blocked: + backend = active_backend_name() + result = ApplyResult(planned=plan) + result.blocked = [n for n, _ in blocked] + result.messages.append( + f"Error: unresolved secrets in the active '{backend}' backend — " + f"nothing was {'planned' if plan else 'applied'}." + ) + for n, secrets in blocked: + for s in secrets: + result.messages.append( + f" {n}: ${{secret:{s}}} → not found. Fix: castle secret set {s}" + ) + return result + # Snapshot BEFORE rendering: liveness + current unit bytes (for restart-on-change). before_active = {(k, n): is_active(n, k, config) for k, n, _ in items} before_unit = {(k, n): _unit_bytes(n, k) for k, n, _ in items} @@ -401,6 +429,47 @@ def _stack_preflight( ) +def _unresolved_secrets( + items: Sequence[tuple[str, str, object]], +) -> list[tuple[str, list[str]]]: + """Enabled deployments whose ``${secret:NAME}`` env references the active backend + can't resolve, as ``[(deployment, [missing_secret, ...]), ...]``. + + This is the fail-loud gate for the footgun that shipped a service with a bogus + ```` string standing in for a real credential: a secret + written to the *wrong* backend (a file on an OpenBao fleet) never resolves, yet + the old path substituted the marker and started the service anyway. Checked + against the active backend so it's correct whichever backend is configured. + """ + from castle_core.config import active_secret_backend, secret_refs + + backend = active_secret_backend() + resolved: dict[str, bool] = {} # cache: one backend read per distinct secret + + def _missing(name: str) -> bool: + if name not in resolved: + try: + resolved[name] = backend.read(name) is not None + except Exception: + resolved[name] = False + return not resolved[name] + + out: list[tuple[str, list[str]]] = [] + for _k, n, spec in items: + if not getattr(spec, "enabled", True): + continue + defaults = getattr(spec, "defaults", None) + env = dict(defaults.env) if defaults and defaults.env else {} + missing: list[str] = [] + for value in env.values(): + for ref in secret_refs(str(value)): + if _missing(ref) and ref not in missing: + missing.append(ref) + if missing: + out.append((n, missing)) + return out + + def _record(result: ApplyResult, name: str, action: str) -> None: { "activate": result.activated, diff --git a/core/tests/test_config.py b/core/tests/test_config.py index f974dfa..1e1f540 100644 --- a/core/tests/test_config.py +++ b/core/tests/test_config.py @@ -12,6 +12,7 @@ from castle_core.config import ( resolve_env_split, resolve_env_vars, save_config, + secret_refs, ) from castle_core.manifest import ProgramSpec, SystemdDeployment @@ -182,6 +183,22 @@ class TestResolveEnvVars: assert resolved["API_KEY"] == "" +class TestSecretRefs: + """Tests for extracting ${secret:NAME} references (the apply-gate input).""" + + def test_none_when_no_refs(self) -> None: + assert secret_refs("plain") == [] + assert secret_refs("${port}/${data_dir}") == [] + + def test_single_and_composite(self) -> None: + assert secret_refs("${secret:API_KEY}") == ["API_KEY"] + assert secret_refs("neo4j/${secret:NEO4J_PW}") == ["NEO4J_PW"] + + def test_multiple_deduped_in_order(self) -> None: + value = "${secret:A}:${secret:B}:${secret:A}" + assert secret_refs(value) == ["A", "B"] + + class TestResolveEnvSplit: """Tests for the secret/plain partition used to keep secrets out of units.""" diff --git a/core/tests/test_deploy_secret_gate.py b/core/tests/test_deploy_secret_gate.py new file mode 100644 index 0000000..feeb60c --- /dev/null +++ b/core/tests/test_deploy_secret_gate.py @@ -0,0 +1,65 @@ +"""Tests for the apply-time unresolved-secret gate. + +`castle apply` must refuse to converge a deployment whose ``${secret:NAME}`` env +references the active backend can't resolve — otherwise the value silently +degrades to a ```` placeholder and the service starts with a +bogus credential (the immich-DB-password-to-the-wrong-backend failure). +""" + +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from castle_core.deploy import _unresolved_secrets + + +def _spec(env: dict[str, str], enabled: bool = True) -> SimpleNamespace: + return SimpleNamespace(enabled=enabled, defaults=SimpleNamespace(env=env)) + + +@pytest.fixture +def file_backend(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Force the active backend to a temp file store with a known secret.""" + secrets = tmp_path / "secrets" + secrets.mkdir() + (secrets / "PRESENT").write_text("value\n") + monkeypatch.setenv("CASTLE_SECRET_BACKEND", "file") + monkeypatch.setattr("castle_core.config.SECRETS_DIR", secrets) + return secrets + + +def test_all_resolved_returns_empty(file_backend: Path) -> None: + items = [("service", "svc", _spec({"TOKEN": "${secret:PRESENT}"}))] + assert _unresolved_secrets(items) == [] + + +def test_missing_secret_is_reported(file_backend: Path) -> None: + items = [("service", "svc", _spec({"TOKEN": "${secret:ABSENT}"}))] + assert _unresolved_secrets(items) == [("svc", ["ABSENT"])] + + +def test_disabled_deployment_skipped(file_backend: Path) -> None: + items = [("service", "svc", _spec({"TOKEN": "${secret:ABSENT}"}, enabled=False))] + assert _unresolved_secrets(items) == [] + + +def test_non_secret_placeholders_ignored(file_backend: Path) -> None: + items = [("service", "svc", _spec({"PORT": "${port}", "DIR": "${data_dir}"}))] + assert _unresolved_secrets(items) == [] + + +def test_composite_value_partial_missing(file_backend: Path) -> None: + # A URL mixing a present and an absent secret still flags the absent one. + env = {"URL": "postgres://u:${secret:PRESENT}@h/db?k=${secret:ABSENT}"} + assert _unresolved_secrets([("service", "svc", _spec(env))]) == [("svc", ["ABSENT"])] + + +def test_multiple_deployments_only_broken_flagged(file_backend: Path) -> None: + items = [ + ("service", "ok", _spec({"T": "${secret:PRESENT}"})), + ("service", "bad", _spec({"T": "${secret:ABSENT}"})), + ] + assert _unresolved_secrets(items) == [("bad", ["ABSENT"])]