diff --git a/docs/operator-secrets.md b/docs/operator-secrets.md new file mode 100644 index 00000000..cdcd9a78 --- /dev/null +++ b/docs/operator-secrets.md @@ -0,0 +1,57 @@ +# Live credentials — the trade-enabled key + +The agent reads its Coinbase CDP credentials from one of two places, in strict precedence: + +1. **The encrypted vault** `secrets.enc` — when it exists. **A trade-enabled key belongs here.** +2. **`.env`** — only when no vault exists. Fine for a read-only key; the wrong risk class for a + key that can move money, because it is plaintext on disk. + +⛔ **The precedence is not a fallback.** If a vault exists but cannot be unlocked (no passphrase, +wrong passphrase, tampered file), the agent **fails closed** — it does not quietly read `.env`. A +vault that cannot be opened must not be silently downgraded to a plaintext key. + +## Setting up the vault + +```bash +# 1. Put the CDP key in .env (temporarily): +# CDP_API_KEY=organizations/.../apiKeys/... +# CDP_API_SECRET=-----BEGIN EC PRIVATE KEY----- ... +# 2. Seal it into the vault (prompts twice for a master passphrase): +keel vault init + +# 3. Confirm it unlocks: +KEEL_VAULT_PASSPHRASE='...' keel vault status +# -> "unlocks OK; credential fields present: api_key, api_secret" + +# 4. THEN delete the plaintext .env. The vault is passphrase-protected; .env is not. +rm .env +``` + +`secrets.enc` is portable — copyable between machines (it is not machine-bound to a keychain) — +and is git-ignored. The master passphrase is never stored anywhere by keel. + +## Running the agent against the vault + +The passphrase is read from `KEEL_VAULT_PASSPHRASE` (for a headless loop) or an interactive +prompt. It is never taken from config or the database. + +```bash +KEEL_VAULT_PASSPHRASE='...' keel agent ... +``` + +⚠️ `KEEL_VAULT_PASSPHRASE` in the environment is only as safe as the environment. For a supervised +run this is fine; for an unattended launchd/cron job, prefer a secret-manager indirection over a +plaintext value in the plist. + +## Rotating the passphrase + +```bash +keel vault rekey # prompts for old then new; the secrets are unchanged +``` + +## What the vault does NOT do + +- It does not print secret values. `keel vault status` reports only *which fields are present*. +- It does not gate placing an order. That is the confirm-mode default, the 15 rails, and the + dangerous-action passphrase — all still in front of every live order. The vault only decides + *where the API credential comes from*. diff --git a/keel/cli.py b/keel/cli.py index cf2b5bb8..1d7ef444 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -84,7 +84,8 @@ from keel.research import deflate as deflate_mod from keel.research import ledger as trials_ledger from keel.research import matrix as matrix_mod -from keel.security import authz +from keel.security import authz, secret_source +from keel.security import secrets as secrets_vault from keel.sim import artifact as artifact_mod from keel.sim import benchmark as benchmark_mod from keel.sim import metrics as metrics_mod @@ -197,10 +198,13 @@ def _build_broker(config: Config) -> Any: # pragma: no cover -- exercised only """Construct the real, network-talking `CoinbaseClient`. Tests monkeypatch this function.""" from coinbase.rest import RESTClient - from keel.config import load_secrets from keel.data.cb_client import CoinbaseClient + from keel.security.secret_source import resolve_secrets - secrets = load_secrets() + # Vault first, .env only when no vault exists; vault-present-but-unlockable raises rather + # than silently downgrading (see `secret_source`). Non-interactive callers set + # KEEL_VAULT_PASSPHRASE; `allow_prompt` lets a real TTY be asked. + secrets = resolve_secrets(allow_prompt=True) transport = RESTClient(api_key=secrets.get("api_key"), api_secret=secrets.get("api_secret")) return CoinbaseClient(transport) @@ -791,6 +795,110 @@ def purification(ctx: click.Context) -> None: ) +@cli.group("vault") +def vault_group() -> None: + """Encrypted secrets vault (spec §14). A trade-enabled key belongs here, not in .env.""" + + +def _confirm_new_passphrase() -> str: + """Prompt twice for a new vault passphrase. Non-TTY callers must pass --passphrase.""" + if not (sys.stdin is not None and sys.stdin.isatty()): + raise click.ClickException( + "no TTY -- pass --passphrase explicitly (it will not be echoed in history if you " + "use an environment indirection)." + ) + first = click.prompt("New vault passphrase", hide_input=True) + second = click.prompt("Repeat", hide_input=True) + if first != second: + raise click.ClickException("passphrases did not match") + if len(first) < 8: + raise click.ClickException("passphrase must be at least 8 characters") + return first + + +@vault_group.command("init") +@click.option("--vault", default=secrets_vault.DEFAULT_VAULT_PATH, help="Vault file path.") +@click.option("--from-env", default=".env", help="Read CDP_API_KEY/SECRET from this .env file.") +@click.option("--passphrase", default=None, help="Master passphrase (else prompted twice).") +@click.option("--force", is_flag=True, default=False, help="Overwrite an existing vault.") +@with_disclaimer +def vault_init(vault: str, from_env: str, passphrase: str | None, force: bool) -> None: + """Create the encrypted vault, importing the CDP key from a .env file if present. + + Does NOT delete the .env -- you remove it once you have confirmed the vault unlocks + (`keel vault status`). A trade-enabled key should live only in the vault; the plaintext + .env is the wrong risk class for it. + """ + if secret_source.vault_exists(vault) and not force: + raise click.ClickException(f"a vault already exists at {vault}; pass --force to replace") + + resolved = passphrase or _confirm_new_passphrase() + secrets_vault.migrate_from_env(from_env, passphrase=resolved, path=vault) + loaded = secrets_vault.load_vault(resolved, path=vault) + n = sum(1 for k in ("api_key", "api_secret") if loaded.get(k)) + click.echo(f"vault written to {vault} ({n} credential field(s) imported from {from_env})") + if n: + click.echo( + "verify it unlocks (`keel vault status`), then delete the plaintext .env. The vault " + "is passphrase-protected; .env is not." + ) + else: + click.echo( + f"note: no CDP credentials found in {from_env} -- the vault is empty but valid. " + "Re-run --from-env pointing at a file with CDP_API_KEY/CDP_API_SECRET, or add them " + "later." + ) + + +@vault_group.command("status") +@click.option("--vault", default=secrets_vault.DEFAULT_VAULT_PATH, help="Vault file path.") +@click.option("--passphrase", default=None, help="Master passphrase (else env/prompt).") +def vault_status(vault: str, passphrase: str | None) -> None: + """Report whether the vault exists and (if a passphrase is available) whether it unlocks. + + Never prints secret values -- only whether each expected field is present. + """ + if not secret_source.vault_exists(vault): + click.echo( + f"no vault at {vault}. Live credentials would come from .env (see `vault init`)." + ) + return + click.echo(f"vault present at {vault}") + resolved = secret_source._resolve_passphrase(passphrase, prompt=True) + if resolved is None: + click.echo( + f"locked -- set {secret_source.PASSPHRASE_ENV} or run interactively to " + "check it unlocks." + ) + return + try: + loaded = secrets_vault.load_vault(resolved, path=vault) + except secrets_vault.VaultError as exc: + raise click.ClickException(f"unlock failed: {exc}") from exc + present = [k for k in ("api_key", "api_secret") if loaded.get(k)] + click.echo(f"unlocks OK; credential fields present: {', '.join(present) or '(none)'}") + + +@vault_group.command("rekey") +@click.option("--vault", default=secrets_vault.DEFAULT_VAULT_PATH, help="Vault file path.") +@click.option("--old-passphrase", default=None, help="Current passphrase (else env/prompt).") +@click.option("--new-passphrase", default=None, help="New passphrase (else prompted twice).") +def vault_rekey(vault: str, old_passphrase: str | None, new_passphrase: str | None) -> None: + """Re-encrypt the vault under a new passphrase. The secrets are unchanged.""" + if not secret_source.vault_exists(vault): + raise click.ClickException(f"no vault at {vault}") + old = secret_source._resolve_passphrase(old_passphrase, prompt=True) + if old is None: + raise click.ClickException("no current passphrase available") + try: + loaded = secrets_vault.load_vault(old, path=vault) + except secrets_vault.VaultError as exc: + raise click.ClickException(f"current passphrase rejected: {exc}") from exc + new = new_passphrase or _confirm_new_passphrase() + secrets_vault.save_vault(loaded, new, path=vault) + click.echo("vault re-encrypted under the new passphrase") + + # -- trials ledger -------------------------------------------------------------------------- diff --git a/keel/security/secret_source.py b/keel/security/secret_source.py new file mode 100644 index 00000000..56a45ffa --- /dev/null +++ b/keel/security/secret_source.py @@ -0,0 +1,93 @@ +"""Decide WHERE the live API credentials come from, and fail closed when unsure. + +Two sources, in a deliberate precedence: + +1. **The encrypted vault** (`security/secrets.py`), when `secrets.enc` exists. This is the + intended home for a **trade-enabled** key -- a plaintext `.env` is the wrong risk class for a + credential that can move money. +2. **`.env`**, when no vault exists. Fine for the read-only key and for anyone who has not opted + into the vault; preserves the pre-vault behaviour exactly. + +⛔ **The precedence is not a fallback chain.** If a vault EXISTS but cannot be unlocked -- no +passphrase, wrong passphrase, tampered file -- this raises rather than quietly reading `.env`. +Falling through would let a stale or lower-privilege `.env` key silently stand in for the vault +the operator deliberately created, which is the exact downgrade the vault exists to prevent. + +**The passphrase never comes from a config file or the DB.** It is read from the environment +variable `KEEL_VAULT_PASSPHRASE` (for a headless agent loop) or, failing that, an interactive TTY +prompt. A non-interactive run with no env var and a vault present fails closed with guidance -- +it does not guess and does not degrade to `.env`. + +Secret VALUES never appear in a log, exception message, or return-path other than the dict handed +to the broker constructor. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +from keel.security.secrets import DEFAULT_VAULT_PATH, VaultError, load_vault + +PASSPHRASE_ENV = "KEEL_VAULT_PASSPHRASE" + + +class SecretResolutionError(Exception): + """A vault exists but could not be unlocked. Never contains a secret or a passphrase.""" + + +def vault_exists(vault_path: str | Path = DEFAULT_VAULT_PATH) -> bool: + return Path(vault_path).exists() + + +def _resolve_passphrase(explicit: str | None, prompt: bool) -> str | None: + """Passphrase from (1) an explicit argument, (2) `KEEL_VAULT_PASSPHRASE`, (3) a TTY prompt. + + Returns `None` when none is available and no prompt is possible -- the caller turns that into + a fail-closed error, rather than this function inventing an empty passphrase that would just + produce a confusing "wrong passphrase" downstream. + """ + if explicit: + return explicit + from_env = os.environ.get(PASSPHRASE_ENV) + if from_env: + return from_env + if prompt and sys.stdin is not None and sys.stdin.isatty(): + import click + + return click.prompt("Vault passphrase", hide_input=True) + return None + + +def resolve_secrets( + *, + vault_path: str | Path = DEFAULT_VAULT_PATH, + env_path: str | Path = ".env", + passphrase: str | None = None, + allow_prompt: bool = True, +) -> dict: + """Return `{"api_key": ..., "api_secret": ...}` (or `{}` if nothing is configured). + + Vault-present-but-unlockable raises `SecretResolutionError`; it never silently reads `.env`. + """ + if vault_exists(vault_path): + resolved = _resolve_passphrase(passphrase, allow_prompt) + if resolved is None: + raise SecretResolutionError( + f"a vault exists at {vault_path} but no passphrase is available. Set " + f"{PASSPHRASE_ENV} or run interactively. Refusing to fall back to .env -- a " + "vault that cannot be unlocked must not be silently downgraded." + ) + try: + return load_vault(resolved, path=vault_path) + except VaultError as exc: + # The VaultError message never contains the secret or the passphrase; safe to chain. + raise SecretResolutionError( + f"vault at {vault_path} could not be unlocked: {exc}" + ) from exc + + # No vault: the pre-vault path, unchanged. `.env` (or empty for offline commands). + from keel.config import load_secrets + + return load_secrets(env_path) diff --git a/tests/security/test_secret_source.py b/tests/security/test_secret_source.py new file mode 100644 index 00000000..877b4125 --- /dev/null +++ b/tests/security/test_secret_source.py @@ -0,0 +1,85 @@ +"""Where live credentials come from, and the fail-closed precedence (spec §14).""" + +from __future__ import annotations + +import pytest + +from keel.security import secret_source +from keel.security.secret_source import SecretResolutionError, resolve_secrets +from keel.security.secrets import save_vault + + +def _env(tmp_path, key="k", secret="s"): + p = tmp_path / ".env" + p.write_text(f"CDP_API_KEY={key}\nCDP_API_SECRET={secret}\n") + return str(p) + + +def _vault(tmp_path, passphrase="correct-horse", key="vk", secret="vs"): + p = tmp_path / "secrets.enc" + save_vault({"api_key": key, "api_secret": secret}, passphrase, path=p) + return str(p) + + +def test_no_vault_falls_back_to_env(tmp_path): + env = _env(tmp_path, key="envkey") + out = resolve_secrets(vault_path=str(tmp_path / "absent.enc"), env_path=env) + assert out["api_key"] == "envkey" + + +def test_no_vault_and_no_env_is_empty_not_an_error(tmp_path): + out = resolve_secrets(vault_path=str(tmp_path / "absent.enc"), env_path=str(tmp_path / "x")) + assert out == {} + + +def test_a_vault_takes_precedence_over_env(tmp_path): + """The whole point: once a vault exists, .env is not consulted -- even if it has keys.""" + env = _env(tmp_path, key="envkey") + vault = _vault(tmp_path, passphrase="pw", key="vaultkey") + out = resolve_secrets(vault_path=vault, env_path=env, passphrase="pw") + assert out["api_key"] == "vaultkey" + + +def test_a_vault_that_cannot_be_unlocked_RAISES_never_reads_env(tmp_path): + """⛔ The downgrade this module exists to prevent. + + A vault present but not unlockable must fail closed, not silently stand in a stale/ + lower-privilege .env key for the vault the operator deliberately created. + """ + env = _env(tmp_path, key="envkey") + vault = _vault(tmp_path, passphrase="right") + + with pytest.raises(SecretResolutionError): + resolve_secrets(vault_path=vault, env_path=env, passphrase="WRONG", allow_prompt=False) + + +def test_a_vault_with_no_passphrase_available_fails_closed(tmp_path, monkeypatch): + monkeypatch.delenv(secret_source.PASSPHRASE_ENV, raising=False) + vault = _vault(tmp_path, passphrase="pw") + with pytest.raises(SecretResolutionError, match="no passphrase"): + resolve_secrets(vault_path=vault, env_path=_env(tmp_path), allow_prompt=False) + + +def test_the_passphrase_env_var_unlocks_a_headless_run(tmp_path, monkeypatch): + vault = _vault(tmp_path, passphrase="pw", key="headless") + monkeypatch.setenv(secret_source.PASSPHRASE_ENV, "pw") + out = resolve_secrets(vault_path=vault, env_path=_env(tmp_path), allow_prompt=False) + assert out["api_key"] == "headless" + + +def test_an_explicit_passphrase_beats_the_env_var(tmp_path, monkeypatch): + vault = _vault(tmp_path, passphrase="explicit-pw", key="x") + monkeypatch.setenv(secret_source.PASSPHRASE_ENV, "wrong-env-pw") + out = resolve_secrets(vault_path=vault, env_path=_env(tmp_path), passphrase="explicit-pw") + assert out["api_key"] == "x" + + +def test_the_error_never_contains_the_passphrase_or_a_secret(tmp_path): + vault = _vault(tmp_path, passphrase="right", key="topsecretkey") + try: + resolve_secrets(vault_path=vault, passphrase="hunter2secretpw", allow_prompt=False) + except SecretResolutionError as exc: + assert "hunter2secretpw" not in str(exc) + assert "topsecretkey" not in str(exc) + else: + raise AssertionError("expected a resolution error") diff --git a/tests/security/test_vault_cli.py b/tests/security/test_vault_cli.py new file mode 100644 index 00000000..a1638b9d --- /dev/null +++ b/tests/security/test_vault_cli.py @@ -0,0 +1,116 @@ +"""`keel vault` -- init / status / rekey. Never prints a secret.""" + +from __future__ import annotations + +from click.testing import CliRunner + +from keel.cli import cli +from keel.security.secrets import load_vault + + +def _env(tmp_path, key="cdpkey", secret="cdpsecret"): + p = tmp_path / ".env" + p.write_text(f"CDP_API_KEY={key}\nCDP_API_SECRET={secret}\n") + return str(p) + + +def test_init_imports_env_and_the_vault_unlocks(tmp_path): + vault = str(tmp_path / "secrets.enc") + result = CliRunner().invoke( + cli, + [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path), "--passphrase", "pw1234567", + ], + ) + assert result.exit_code == 0, result.output + assert "imported" in result.output + loaded = load_vault("pw1234567", path=vault) + assert loaded["api_key"] == "cdpkey" + + +def test_init_never_prints_the_secret(tmp_path): + vault = str(tmp_path / "secrets.enc") + result = CliRunner().invoke( + cli, + [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path, secret="TOPSECRET"), "--passphrase", "pw1234567", + ], + ) + assert "TOPSECRET" not in result.output + + +def test_init_refuses_to_clobber_without_force(tmp_path): + vault = str(tmp_path / "secrets.enc") + args = [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path), "--passphrase", "pw1234567", + ] + assert CliRunner().invoke(cli, args).exit_code == 0 + second = CliRunner().invoke(cli, args) + assert second.exit_code != 0 + assert "already exists" in second.output + assert CliRunner().invoke(cli, [*args, "--force"]).exit_code == 0 + + +def test_status_on_no_vault_points_at_env(tmp_path): + result = CliRunner().invoke(cli, ["vault", "status", "--vault", str(tmp_path / "none.enc")]) + assert result.exit_code == 0 + assert "no vault" in result.output + + +def test_status_reports_unlock_and_present_fields(tmp_path): + vault = str(tmp_path / "secrets.enc") + CliRunner().invoke( + cli, + [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path), "--passphrase", "pw1234567", + ], + ) + result = CliRunner().invoke( + cli, ["vault", "status", "--vault", vault, "--passphrase", "pw1234567"] + ) + assert "unlocks OK" in result.output + assert "api_key" in result.output + assert "cdpkey" not in result.output # the value is never shown + + +def test_status_reports_a_wrong_passphrase_without_crashing(tmp_path): + vault = str(tmp_path / "secrets.enc") + CliRunner().invoke( + cli, + [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path), "--passphrase", "pw1234567", + ], + ) + result = CliRunner().invoke(cli, ["vault", "status", "--vault", vault, "--passphrase", "WRONG"]) + assert result.exit_code != 0 + assert "unlock failed" in result.output + + +def test_rekey_changes_the_passphrase_and_preserves_secrets(tmp_path): + vault = str(tmp_path / "secrets.enc") + CliRunner().invoke( + cli, + [ + "vault", "init", "--vault", vault, + "--from-env", _env(tmp_path), "--passphrase", "oldpw12345", + ], + ) + result = CliRunner().invoke( + cli, + ["vault", "rekey", "--vault", vault, + "--old-passphrase", "oldpw12345", "--new-passphrase", "newpw12345"], + ) + assert result.exit_code == 0, result.output + assert load_vault("newpw12345", path=vault)["api_key"] == "cdpkey" + + import pytest + + from keel.security.secrets import VaultError + + with pytest.raises(VaultError): + load_vault("oldpw12345", path=vault)