diff --git a/keel/cli.py b/keel/cli.py index fc63ef9..50985bd 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -141,6 +141,7 @@ UNREADABLE_PREVIEW_MARKER, _interactive_confirm, ) +from keel.commands.credentials import credentials_group from keel.commands.db import db_group from keel.commands.fetch import assess_products as _assess_products # noqa: F401 -- pinned by tests @@ -1350,6 +1351,14 @@ def simulate( cli.add_command(setup_cmd) +# -- credentials (the OS keychain, so a desktop user need never create a .env) -------------------- + +# `.env` stays fully supported and still takes precedence; the keychain only answers where the +# file is silent. It exists because the desktop product's user has no terminal and cannot create a +# `.env` at all (#437). Defined in `keel.commands.credentials` over `keel_core.secrets`. +cli.add_command(credentials_group) + + # -- install-plan (the machine interface an installer script calls) ------------------------------ # The desktop product has no self-update (#439), so the INSTALLER is the update path -- and "what diff --git a/keel/commands/credentials.py b/keel/commands/credentials.py new file mode 100644 index 0000000..3acd79f --- /dev/null +++ b/keel/commands/credentials.py @@ -0,0 +1,150 @@ +"""`keel credentials` -- put an API key in the OS keychain instead of a plaintext file. + +Three subcommands, and the shape of each is a security decision rather than a UI one. + +`set` NEVER takes the value as an argument. A secret on a command line is in shell history, in +`ps` output for every other process on the machine while it runs, and in any terminal recording. +It is prompted for with echo off, or piped in on stdin for a script that already has it. + +`show` never prints a value. It answers "is it set, and which of the three places is keel actually +reading it from" -- the two questions that are worth asking, and the only ones answerable without +putting the secret on a screen someone may be sharing. + +`forget` only ever touches the keychain. A `.env` file is the operator's own artifact and keel does +not edit it; deleting a line out of a file someone hand-wrote, on their behalf, is not something a +credential command should do. It says so when a `.env` value is what is actually in use, because +otherwise "forget" would appear not to work. +""" + +from __future__ import annotations + +import sys + +import click +from keel_core.secrets import ( + KEYCHAIN_SERVICE, + SecretSource, + delete_secret, + describe_sources, + keychain_available, + read_secret, + store_secret, +) + +#: The credentials keel knows how to talk about, with what each is for. Not a closed list of what +#: may be STORED -- an adapter's own names work too -- but these are the ones `show` reports on +#: unasked, because a blank report is not an answer. +KNOWN: tuple[tuple[str, str], ...] = ( + ("CDP_API_KEY", "Coinbase Developer Platform key -- market data, and orders if trade-enabled"), + ("CDP_API_SECRET", "the matching CDP secret"), + ("ALPACA_API_KEY_ID", "Alpaca key id (US equities)"), + ("ALPACA_API_SECRET_KEY", "the matching Alpaca secret"), + ("ROBINHOOD_API_KEY_CREDENTIAL", "Robinhood API credential identifier"), + ("ROBINHOOD_PRIVATE_KEY", "the matching Robinhood Ed25519 private seed"), +) + +_SOURCE_NOTE = { + SecretSource.ENVIRONMENT: "from the environment (set by whoever launched this process)", + SecretSource.ENV_FILE: "from your .env file", + SecretSource.KEYCHAIN: "from the OS keychain", + SecretSource.ABSENT: "not set anywhere keel looks", +} + + +@click.group("credentials") +def credentials_group() -> None: + """Store API credentials in the OS keychain, and see which one keel is using.""" + + +@credentials_group.command("show") +@click.pass_context +def credentials_show(ctx: click.Context) -> None: + """Which credentials keel can see, and where each comes from. Never prints a value. + + The source is the useful half. "keel cannot see your key" and "keel is using a different key + than the one you just typed" are the two questions worth asking, and only the source tells + them apart. + """ + obj = ctx.obj or {} + env_path = obj.get("env_path") + resolved = describe_sources(tuple(name for name, _why in KNOWN), env_path=env_path) + notes = dict(KNOWN) + for item in resolved: + mark = "set " if item.found else "unset" + click.echo(f"{mark} {item.name}") + click.echo(f" {_SOURCE_NOTE[item.source]}") + click.echo(f" {notes[item.name]}") + click.echo("") + if keychain_available(): + click.echo(f"keychain: available (service {KEYCHAIN_SERVICE!r})") + else: + click.echo( + "keychain: NOT available on this machine -- use a .env file beside your deployment." + ) + click.echo( + "precedence: environment, then .env, then the keychain. A value you can see wins over " + "one you cannot." + ) + + +@credentials_group.command("set") +@click.argument("name") +@click.option( + "--stdin", + "from_stdin", + is_flag=True, + default=False, + help="Read the value from stdin instead of prompting (for scripts that already hold it).", +) +def credentials_set(name: str, from_stdin: bool) -> None: + """Store NAME in the OS keychain. The value is never taken as an argument. + + A secret on a command line is in shell history, in `ps` output for every other process on the + machine while the command runs, and in any terminal recording. So it is prompted for with echo + off, or piped in. + + This does not touch your `.env`. If a `.env` already holds this name it will keep winning -- + `keel credentials show` says which one is in use, and that is deliberate: a value you can see + beats one you cannot. + """ + if from_stdin: + value = sys.stdin.read().strip() + else: + value = click.prompt(f"value for {name}", hide_input=True, default="", show_default=False) + value = value.strip() + if not value: + raise click.ClickException("no value given; nothing was stored.") + + try: + store_secret(name, value) + except Exception as exc: + raise click.ClickException(str(exc)) from exc + + # Read it back through the SAME resolver a real caller uses, so the confirmation reflects what + # keel will actually do rather than what was just written. If a `.env` shadows it, this is + # where the operator finds out -- not at the first request that used the wrong key. + resolved = read_secret(name) + click.echo(f"stored {name} in the OS keychain.") + if resolved.source is not SecretSource.KEYCHAIN: + click.echo( + f"note: keel will still read {name} {_SOURCE_NOTE[resolved.source]}, which takes " + "precedence. Remove it there if you meant the keychain value to be used." + ) + + +@credentials_group.command("forget") +@click.argument("name") +def credentials_forget(name: str) -> None: + """Remove NAME from the OS keychain. Never edits your `.env`.""" + removed = delete_secret(name) + if removed: + click.echo(f"removed {name} from the OS keychain.") + else: + click.echo(f"{name} was not in the OS keychain; nothing removed.") + + resolved = read_secret(name) + if resolved.found: + click.echo( + f"note: keel can still see {name} {_SOURCE_NOTE[resolved.source]}. keel does not edit " + "that file -- it is yours." + ) diff --git a/packages/keel-core/keel_core/config.py b/packages/keel-core/keel_core/config.py index d96955f..9074559 100644 --- a/packages/keel-core/keel_core/config.py +++ b/packages/keel-core/keel_core/config.py @@ -955,21 +955,28 @@ def load_config(path: str | Path) -> Config: def load_secrets(env_path: str | Path | None = None) -> dict: - """Load CDP API credentials from a git-ignored `.env` file. + """Load CDP API credentials from the environment, a git-ignored `.env`, or the OS keychain. - Returns `{"api_key": ..., "api_secret": ...}` when both are present, `{}` when the file is - absent or empty so offline commands keep working without secrets configured. + Returns `{"api_key": ..., "api_secret": ...}` when either is present, `{}` when neither is, + so offline commands keep working without secrets configured. + + **The `.env` file still wins over the keychain**, and every pre-existing deployment therefore + behaves byte-identically: the keychain only answers where the file was silent. `keel_core. + secrets` states the full precedence and why -- in one line, a value the operator can see beats + one they cannot when the two disagree, and a stale keychain entry silently overriding an + edited `.env` is a debugging session nobody should have to have. + + The keychain exists because the desktop product's user has no terminal and cannot create a + `.env` at all (#437). It is not a migration: nothing moves a credential out of a file. """ + from keel_core.secrets import read_secret + # `None` resolves against the state root -- the deployment folder when there is one, the # OS app-data directory otherwise (see `keel_core.paths`). An explicit path is honoured # unchanged, which is what every caller passing one already relies on. - env_path = Path(env_path) if env_path is not None else default_env_path() - if not env_path.exists(): - return {} - - values = dotenv_values(env_path) - api_key = values.get("CDP_API_KEY") - api_secret = values.get("CDP_API_SECRET") + resolved = Path(env_path) if env_path is not None else default_env_path() + api_key = read_secret("CDP_API_KEY", env_path=resolved).value + api_secret = read_secret("CDP_API_SECRET", env_path=resolved).value if not api_key and not api_secret: return {} return {"api_key": api_key, "api_secret": api_secret} diff --git a/packages/keel-core/keel_core/secrets.py b/packages/keel-core/keel_core/secrets.py new file mode 100644 index 0000000..7b05e21 --- /dev/null +++ b/packages/keel-core/keel_core/secrets.py @@ -0,0 +1,169 @@ +"""Where a credential comes from, and the OS keychain as one of the places it can (#437). + +Until now a credential lived in exactly one place: a git-ignored `.env` beside the deployment. +That is a fine answer for an operator who chose the folder and can see the file. It is the wrong +answer for the desktop product, where the person installing keel has no terminal, no editor open +on a dotfile, and no way to create one -- and it is a worse answer than it needs to be even for +an operator, because a plaintext secret at rest is a plaintext secret at rest. + +So there are three sources now, and the ORDER is the whole design: + +1. **The real environment.** Explicit, ephemeral, and set by whoever launched the process. It has + always won for the venues that read it, and it still does. +2. **The `.env` file.** The operator's own artifact, in a folder they chose, which they can read + and diff and delete. Deliberately ABOVE the keychain: every existing deployment keeps + behaving byte-identically, and a value someone can see beats one they cannot when the two + disagree. A stale keychain entry silently overriding an edited `.env` is a debugging session + nobody should have to have. +3. **The OS keychain** -- macOS Keychain, Windows Credential Manager, Secret Service on Linux, + through `keyring`. What the first-run wizard writes, because it is the only one of the three a + person with no terminal can populate. + +`ResolvedSecret` carries the SOURCE alongside the value, and that is not decoration: "keel cannot +see your key" and "keel is using a different key than the one you just typed" are the two support +questions this module exists to make answerable, and only the source distinguishes them. + +**Nothing here logs a value, ever.** `ResolvedSecret.__repr__` is overridden for the same reason: +a dataclass that prints its own secret in a traceback has published it to every log the traceback +reaches. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from enum import Enum +from pathlib import Path + +#: The keychain "service" every keel secret is filed under. One namespace, so an operator can +#: find and revoke the whole set in Keychain Access or Credential Manager without knowing which +#: names keel happens to use this release. +KEYCHAIN_SERVICE = "keel-trader" + + +class SecretSource(str, Enum): + ENVIRONMENT = "environment" + ENV_FILE = "env-file" + KEYCHAIN = "keychain" + ABSENT = "absent" + + +@dataclass(frozen=True) +class ResolvedSecret: + name: str + value: str | None + source: SecretSource + + @property + def found(self) -> bool: + return self.value is not None + + def __repr__(self) -> str: + """Never the value. A dataclass that prints its own secret in a traceback has published + it to every log that traceback reaches, and tracebacks travel further than anything else + in a program.""" + state = "set" if self.found else "unset" + return f"ResolvedSecret(name={self.name!r}, source={self.source.value!r}, {state})" + + +def keychain_available() -> bool: + """Whether a real keychain backend is present. + + `keyring` always imports and always answers; on a machine with no usable backend it selects + `fail.Keyring`, whose every operation raises. Detecting that HERE means the caller can offer + the `.env` path instead of showing someone a form that will throw when they submit it -- + which is the difference between a headless Linux box and a broken install. + """ + try: + import keyring + from keyring.backends import fail + except Exception: + return False + try: + return not isinstance(keyring.get_keyring(), fail.Keyring) + except Exception: + return False + + +def _from_keychain(name: str) -> str | None: + if not keychain_available(): + return None + try: + import keyring + + return keyring.get_password(KEYCHAIN_SERVICE, name) + except Exception: + # A locked keychain, a denied prompt, a backend that broke after being selected. None of + # those is a reason to take the process down: the caller falls through to "absent" and + # says so, which is the same message it would give for a credential never set. + return None + + +def read_secret(name: str, *, env_path: str | Path | None = None) -> ResolvedSecret: + """Resolve one secret, reporting WHERE it came from. Never raises.""" + from keel_core.config import default_env_path + + value = os.environ.get(name) + if value: + return ResolvedSecret(name, value, SecretSource.ENVIRONMENT) + + resolved = Path(env_path) if env_path is not None else default_env_path() + if resolved.exists(): + try: + from dotenv import dotenv_values + + file_value = dotenv_values(resolved).get(name) + except Exception: + file_value = None + if file_value: + return ResolvedSecret(name, file_value, SecretSource.ENV_FILE) + + stored = _from_keychain(name) + if stored: + return ResolvedSecret(name, stored, SecretSource.KEYCHAIN) + return ResolvedSecret(name, None, SecretSource.ABSENT) + + +def store_secret(name: str, value: str) -> None: + """Write one secret to the OS keychain. + + Raises when there is no usable backend rather than silently discarding the value -- a form + that appears to save a credential and does not is worse than one that refuses, because the + operator only finds out at the first request that needs it. + """ + if not value: + raise ValueError(f"refusing to store an empty value for {name}") + if not keychain_available(): + raise RuntimeError( + "no OS keychain is available on this machine, so the credential was NOT saved. " + "Put it in a .env file beside your deployment instead." + ) + import keyring + + keyring.set_password(KEYCHAIN_SERVICE, name, value) + + +def delete_secret(name: str) -> bool: + """Remove one secret from the keychain. `False` when there was nothing to remove. + + Only ever touches the keychain: a `.env` file is the operator's own artifact and keel does not + edit it. Deleting a line out of a file someone hand-wrote, on their behalf, is not a thing a + setup flow should do. + """ + if not keychain_available(): + return False + try: + import keyring + import keyring.errors + + keyring.delete_password(KEYCHAIN_SERVICE, name) + return True + except Exception: + return False + + +def describe_sources( + names: tuple[str, ...], *, env_path: str | Path | None = None +) -> list[ResolvedSecret]: + """Resolve several secrets for display. Values are carried but must not be rendered.""" + return [read_secret(name, env_path=env_path) for name in names] diff --git a/packages/keel-core/pyproject.toml b/packages/keel-core/pyproject.toml index 14b8787..cf4576a 100644 --- a/packages/keel-core/pyproject.toml +++ b/packages/keel-core/pyproject.toml @@ -4,7 +4,16 @@ version = "0.10.0" description = "Shared domain types, configuration, and logging for keel" license = "Apache-2.0" requires-python = ">=3.11" -dependencies = ["pyyaml>=6.0.3", "python-dotenv>=1.2.2"] +dependencies = [ + "pyyaml>=6.0.3", + "python-dotenv>=1.2.2", + # The OS keychain (macOS Keychain, Windows Credential Manager, Secret Service). Needed by + # `keel_core.secrets` because the desktop product's user has no terminal and cannot create a + # `.env`; `.env` stays fully supported and still takes precedence over it. Verified to work + # inside a PyInstaller bundle -- keyring selects its backend dynamically, which is exactly + # the kind of thing freezing usually breaks, so it was checked rather than assumed. + "keyring>=25.6", +] [build-system] requires = ["uv_build>=0.10.4,<0.13.0"] diff --git a/tests/commands/test_credentials.py b/tests/commands/test_credentials.py new file mode 100644 index 0000000..f7a46a4 --- /dev/null +++ b/tests/commands/test_credentials.py @@ -0,0 +1,133 @@ +"""`keel credentials` -- the shape of each subcommand is a security decision, not a UI one.""" + +from __future__ import annotations + +import inspect +from pathlib import Path + +import pytest +from click.testing import CliRunner +from keel_core import secrets as secrets_mod + +from keel.cli import cli +from keel.commands import credentials as credentials_mod + +SECRET = "s3cret-value-that-must-never-be-printed" + + +@pytest.fixture(autouse=True) +def fake_keychain(monkeypatch: pytest.MonkeyPatch) -> dict[str, str]: + store: dict[str, str] = {} + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: True) + monkeypatch.setattr(secrets_mod, "_from_keychain", lambda name: store.get(name)) + monkeypatch.setattr( + secrets_mod, "store_secret", lambda name, value: store.__setitem__(name, value) + ) + monkeypatch.setattr( + secrets_mod, "delete_secret", lambda name: store.pop(name, None) is not None + ) + monkeypatch.setattr(credentials_mod, "store_secret", secrets_mod.store_secret) + monkeypatch.setattr(credentials_mod, "delete_secret", secrets_mod.delete_secret) + monkeypatch.setattr(credentials_mod, "keychain_available", secrets_mod.keychain_available) + return store + + +def test_set_does_not_accept_the_value_as_an_argument() -> None: + """A secret on a command line is in shell history, in `ps` output for every other process on + the machine while the command runs, and in any terminal recording. + + Asserted off the signature, so a `--value` option added later fails here rather than shipping + as a convenience.""" + params = { + p.name + for p in inspect.signature(credentials_mod.credentials_set.callback).parameters.values() + } + assert params == {"name", "from_stdin"} + assert "value" not in params + + +def test_set_prompts_with_echo_off() -> None: + """`hide_input` is the difference between typing a key and displaying it on a screen someone + may be sharing.""" + source = inspect.getsource(credentials_mod.credentials_set.callback) + assert "hide_input=True" in source + + +def test_set_stores_from_stdin_without_echoing_the_value( + fake_keychain: dict[str, str], monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.delenv("CDP_API_KEY", raising=False) + monkeypatch.setattr(secrets_mod, "default_env_path", lambda: tmp_path / "absent", raising=False) + result = CliRunner().invoke( + cli, ["credentials", "set", "CDP_API_KEY", "--stdin"], input=SECRET + "\n" + ) + assert result.exit_code == 0, result.output + assert fake_keychain["CDP_API_KEY"] == SECRET + assert SECRET not in result.output + + +def test_an_empty_value_stores_nothing(fake_keychain: dict[str, str]) -> None: + result = CliRunner().invoke( + cli, ["credentials", "set", "CDP_API_KEY", "--stdin"], input=" \n" + ) + assert result.exit_code != 0 + assert "CDP_API_KEY" not in fake_keychain + + +def test_show_never_prints_a_value(fake_keychain: dict[str, str]) -> None: + """It answers "is it set, and which of the three places is keel reading it from" -- the only + questions answerable without putting the secret on a screen.""" + fake_keychain["CDP_API_KEY"] = SECRET + result = CliRunner().invoke(cli, ["credentials", "show"]) + assert result.exit_code == 0 + assert SECRET not in result.output + assert "CDP_API_KEY" in result.output + assert "keychain" in result.output + + +def test_show_names_the_precedence(fake_keychain: dict[str, str]) -> None: + """ "keel cannot see your key" and "keel is using a different key than the one you just typed" + are different problems, and only the source distinguishes them.""" + result = CliRunner().invoke(cli, ["credentials", "show"]) + assert "precedence" in result.output + assert ".env" in result.output + + +def test_setting_a_shadowed_name_says_so( + fake_keychain: dict[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + """Otherwise the operator finds out at the first request that used the wrong key.""" + monkeypatch.setenv("CDP_API_KEY", "from-the-environment") + result = CliRunner().invoke( + cli, ["credentials", "set", "CDP_API_KEY", "--stdin"], input=SECRET + "\n" + ) + assert result.exit_code == 0 + assert "takes precedence" in result.output + assert SECRET not in result.output + + +def test_forget_says_when_a_file_still_holds_the_value( + fake_keychain: dict[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + """keel does not edit a `.env`. Saying nothing would make "forget" appear not to work.""" + fake_keychain["CDP_API_KEY"] = SECRET + monkeypatch.setenv("CDP_API_KEY", "from-the-environment") + result = CliRunner().invoke(cli, ["credentials", "forget", "CDP_API_KEY"]) + assert "removed" in result.output + assert "can still see" in result.output + assert "CDP_API_KEY" not in fake_keychain + + +def test_forget_on_an_absent_name_is_not_an_error(fake_keychain: dict[str, str]) -> None: + result = CliRunner().invoke(cli, ["credentials", "forget", "NOT_STORED"]) + assert result.exit_code == 0 + assert "nothing removed" in result.output + + +def test_every_known_credential_says_what_it_is_for() -> None: + """A list of variable names is not help. `show` prints these unasked, because a blank report + is not an answer.""" + assert credentials_mod.KNOWN + for name, why in credentials_mod.KNOWN: + assert name.isupper() + assert len(why) > 15 diff --git a/tests/core/test_secrets.py b/tests/core/test_secrets.py new file mode 100644 index 0000000..d0c8783 --- /dev/null +++ b/tests/core/test_secrets.py @@ -0,0 +1,233 @@ +"""Where a credential comes from, and what must never happen to it on the way (#437). + +These do NOT touch the real keychain. CI has no usable backend, and a test suite that wrote to a +developer's login keychain would be leaving state on their machine to make an assertion. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from keel_core import secrets as secrets_mod +from keel_core.secrets import ( + ResolvedSecret, + SecretSource, + delete_secret, + read_secret, + store_secret, +) + + +@pytest.fixture +def fake_keychain(monkeypatch: pytest.MonkeyPatch) -> dict[str, str]: + """An in-memory stand-in, wired at the two seams every function here goes through.""" + store: dict[str, str] = {} + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: True) + monkeypatch.setattr(secrets_mod, "_from_keychain", lambda name: store.get(name)) + return store + + +# -- precedence, which is the whole design ----------------------------------------------------- + + +def test_the_environment_wins_over_everything( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + env_file = tmp_path / ".env" + env_file.write_text("SOME_KEY=from-file\n") + fake_keychain["SOME_KEY"] = "from-keychain" + monkeypatch.setenv("SOME_KEY", "from-environment") + + resolved = read_secret("SOME_KEY", env_path=env_file) + assert resolved.value == "from-environment" + assert resolved.source is SecretSource.ENVIRONMENT + + +def test_the_env_file_wins_over_the_keychain( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + """Deliberately, and it is the decision that keeps every existing deployment byte-identical. + + A value the operator can SEE beats one they cannot when the two disagree. A stale keychain + entry silently overriding an edited `.env` is a debugging session nobody should have to have. + """ + monkeypatch.delenv("SOME_KEY", raising=False) + env_file = tmp_path / ".env" + env_file.write_text("SOME_KEY=from-file\n") + fake_keychain["SOME_KEY"] = "from-keychain" + + resolved = read_secret("SOME_KEY", env_path=env_file) + assert resolved.value == "from-file" + assert resolved.source is SecretSource.ENV_FILE + + +def test_the_keychain_answers_where_the_file_is_silent( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + """The case the desktop product depends on: no terminal, so no `.env` was ever created.""" + monkeypatch.delenv("SOME_KEY", raising=False) + fake_keychain["SOME_KEY"] = "from-keychain" + + resolved = read_secret("SOME_KEY", env_path=tmp_path / "does-not-exist") + assert resolved.value == "from-keychain" + assert resolved.source is SecretSource.KEYCHAIN + + +def test_nothing_anywhere_is_absent_not_an_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + monkeypatch.delenv("SOME_KEY", raising=False) + resolved = read_secret("SOME_KEY", env_path=tmp_path / "nope") + assert resolved.source is SecretSource.ABSENT + assert not resolved.found + + +def test_an_empty_value_never_satisfies_a_lookup( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + """An empty environment variable is how a credential goes missing while looking configured -- + it must fall through to the next source, not shadow it with nothing.""" + monkeypatch.setenv("SOME_KEY", "") + fake_keychain["SOME_KEY"] = "from-keychain" + assert read_secret("SOME_KEY", env_path=tmp_path / "nope").source is SecretSource.KEYCHAIN + + +# -- what must never happen to a secret -------------------------------------------------------- + + +def test_repr_never_contains_the_value() -> None: + """A dataclass that prints its own secret in a traceback has published it to every log that + traceback reaches -- and tracebacks travel further than anything else in a program.""" + resolved = ResolvedSecret("SOME_KEY", "super-secret-value", SecretSource.KEYCHAIN) + text = repr(resolved) + assert "super-secret-value" not in text + assert "SOME_KEY" in text and "keychain" in text + assert "set" in text + + +def test_str_and_format_do_not_leak_either() -> None: + """`repr` is not the only way a value reaches a log line.""" + resolved = ResolvedSecret("SOME_KEY", "super-secret-value", SecretSource.KEYCHAIN) + assert "super-secret-value" not in str(resolved) + assert "super-secret-value" not in f"{resolved}" + assert "super-secret-value" not in f"{resolved!r}" + + +def test_storing_an_empty_value_is_refused(fake_keychain: dict[str, str]) -> None: + """Storing "" would look like success and read back as absent.""" + with pytest.raises(ValueError): + store_secret("SOME_KEY", "") + + +def test_storing_without_a_keychain_raises_rather_than_discarding( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A form that appears to save a credential and does not is worse than one that refuses: the + operator only finds out at the first request that needed it.""" + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: False) + with pytest.raises(RuntimeError, match="NOT saved"): + store_secret("SOME_KEY", "value") + + +def test_a_locked_or_broken_keychain_reads_as_absent_rather_than_crashing( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A locked keychain, a denied prompt, a backend that broke after being selected -- none is a + reason to take the process down. The caller falls through to "absent" and says so, which is + the same message it would give for a credential that was never set. + + Patched at `keyring.get_password` rather than at `_from_keychain`, because `_from_keychain` + IS the code under test: swapping it out would test the stub instead. + """ + keyring = pytest.importorskip("keyring") + monkeypatch.delenv("SOME_KEY", raising=False) + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: True) + + def _boom(_service: str, _name: str) -> str: + raise RuntimeError("the keychain is locked") + + monkeypatch.setattr(keyring, "get_password", _boom) + + assert secrets_mod._from_keychain("SOME_KEY") is None + assert read_secret("SOME_KEY", env_path=tmp_path / "nope").source is SecretSource.ABSENT + + +def test_the_broken_keychain_test_is_not_vacuous(monkeypatch: pytest.MonkeyPatch) -> None: + """The one above passes trivially if `_from_keychain` never calls `get_password`. This proves + it does.""" + keyring = pytest.importorskip("keyring") + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: True) + calls: list[tuple[str, str]] = [] + monkeypatch.setattr( + keyring, "get_password", lambda service, name: calls.append((service, name)) or "v" + ) + assert secrets_mod._from_keychain("SOME_KEY") == "v" + assert calls == [(secrets_mod.KEYCHAIN_SERVICE, "SOME_KEY")] + + +def test_delete_never_touches_the_env_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + """A `.env` is the operator's own artifact. Deleting a line out of a file someone hand-wrote, + on their behalf, is not something a credential command should do.""" + monkeypatch.delenv("SOME_KEY", raising=False) + env_file = tmp_path / ".env" + env_file.write_text("SOME_KEY=from-file\nOTHER=keep-me\n") + before = env_file.read_text() + + monkeypatch.setattr(secrets_mod, "delete_secret", lambda name: True) + delete_secret("SOME_KEY") + + assert env_file.read_text() == before + assert read_secret("SOME_KEY", env_path=env_file).source is SecretSource.ENV_FILE + + +def test_no_keychain_means_delete_reports_nothing_removed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: False) + assert delete_secret("SOME_KEY") is False + + +# -- backward compatibility --------------------------------------------------------------------- + + +def test_load_secrets_still_reads_a_plain_env_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Every pre-existing deployment must behave byte-identically.""" + from keel_core.config import load_secrets + + monkeypatch.delenv("CDP_API_KEY", raising=False) + monkeypatch.delenv("CDP_API_SECRET", raising=False) + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: False) + + env_file = tmp_path / ".env" + env_file.write_text("CDP_API_KEY=k\nCDP_API_SECRET=s\n") + assert load_secrets(env_file) == {"api_key": "k", "api_secret": "s"} + + +def test_load_secrets_is_empty_when_nothing_is_configured( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Offline commands keep working with no secrets at all.""" + from keel_core.config import load_secrets + + monkeypatch.delenv("CDP_API_KEY", raising=False) + monkeypatch.delenv("CDP_API_SECRET", raising=False) + monkeypatch.setattr(secrets_mod, "keychain_available", lambda: False) + assert load_secrets(tmp_path / "absent") == {} + + +def test_load_secrets_falls_back_to_the_keychain( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fake_keychain: dict[str, str] +) -> None: + """The desktop case: a machine with no `.env` at all.""" + from keel_core.config import load_secrets + + monkeypatch.delenv("CDP_API_KEY", raising=False) + monkeypatch.delenv("CDP_API_SECRET", raising=False) + fake_keychain["CDP_API_KEY"] = "k" + fake_keychain["CDP_API_SECRET"] = "s" + assert load_secrets(tmp_path / "absent") == {"api_key": "k", "api_secret": "s"} diff --git a/uv.lock b/uv.lock index 8f57cba..c344051 100644 --- a/uv.lock +++ b/uv.lock @@ -67,6 +67,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/df/73/b6e24bd22e6720ca8ee9a85a0c4a2971af8497d8f3193fa05390cbd46e09/backoff-2.2.1-py3-none-any.whl", hash = "sha256:63579f9a0628e06278f7e47b7d7d5b6ce20dc65c5e96a6f3ca99a6adca0396e8", size = 15148, upload-time = "2022-10-05T19:19:30.546Z" }, ] +[[package]] +name = "backports-tarfile" +version = "1.2.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/86/72/cd9b395f25e290e633655a100af28cb253e4393396264a98bd5f5951d50f/backports_tarfile-1.2.0.tar.gz", hash = "sha256:d75e02c268746e1b8144c278978b6e98e85de6ad16f8e4b0844a154557eca991", size = 86406, upload-time = "2024-05-28T17:01:54.731Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b9/fa/123043af240e49752f1c4bd24da5053b6bd00cad78c2be53c0d1e8b975bc/backports.tarfile-1.2.0-py3-none-any.whl", hash = "sha256:77e284d754527b01fb1e6fa8a1afe577858ebe4e9dad8919e34c862cb399bc34", size = 30181, upload-time = "2024-05-28T17:01:53.112Z" }, +] + [[package]] name = "certifi" version = "2026.6.17" @@ -469,6 +478,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/1e/5e/d4e9f1a599fb8e573b7b87160658329fbf28d19eac2718f51fc3def3aa5a/idna-3.18-py3-none-any.whl", hash = "sha256:7f952cbe720b688055e3f87de14f5c3e5fdaa8bc3928985c4077ca689de849a2", size = 65455, upload-time = "2026-06-02T14:34:06.319Z" }, ] +[[package]] +name = "importlib-metadata" +version = "9.0.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "zipp" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/a9/01/15bb152d77b21318514a96f43af312635eb2500c96b55398d020c93d86ea/importlib_metadata-9.0.0.tar.gz", hash = "sha256:a4f57ab599e6a2e3016d7595cfd72eb4661a5106e787a95bcc90c7105b831efc", size = 56405, upload-time = "2026-03-20T06:42:56.999Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/38/3d/2d244233ac4f76e38533cfcb2991c9eb4c7bf688ae0a036d30725b8faafe/importlib_metadata-9.0.0-py3-none-any.whl", hash = "sha256:2d21d1cc5a017bd0559e36150c21c830ab1dc304dedd1b7ea85d20f45ef3edd7", size = 27789, upload-time = "2026-03-20T06:42:55.665Z" }, +] + [[package]] name = "iniconfig" version = "2.3.0" @@ -478,6 +499,51 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/cb/b1/3846dd7f199d53cb17f49cba7e651e9ce294d8497c8c150530ed11865bb8/iniconfig-2.3.0-py3-none-any.whl", hash = "sha256:f631c04d2c48c52b84d0d0549c99ff3859c98df65b3101406327ecc7d53fbf12", size = 7484, upload-time = "2025-10-18T21:55:41.639Z" }, ] +[[package]] +name = "jaraco-classes" +version = "3.4.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "more-itertools" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/06/c0/ed4a27bc5571b99e3cff68f8a9fa5b56ff7df1c2251cc715a652ddd26402/jaraco.classes-3.4.0.tar.gz", hash = "sha256:47a024b51d0239c0dd8c8540c6c7f484be3b8fcf0b2d85c13825780d3b3f3acd", size = 11780, upload-time = "2024-03-31T07:27:36.643Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/7f/66/b15ce62552d84bbfcec9a4873ab79d993a1dd4edb922cbfccae192bd5b5f/jaraco.classes-3.4.0-py3-none-any.whl", hash = "sha256:f662826b6bed8cace05e7ff873ce0f9283b5c924470fe664fff1c2f00f581790", size = 6777, upload-time = "2024-03-31T07:27:34.792Z" }, +] + +[[package]] +name = "jaraco-context" +version = "6.1.2" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "backports-tarfile", marker = "python_full_version < '3.12'" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/af/50/4763cd07e722bb6285316d390a164bc7e479db9d90daa769f22578f698b4/jaraco_context-6.1.2.tar.gz", hash = "sha256:f1a6c9d391e661cc5b8d39861ff077a7dc24dc23833ccee564b234b81c82dfe3", size = 16801, upload-time = "2026-03-20T22:13:33.922Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/f2/58/bc8954bda5fcda97bd7c19be11b85f91973d67a706ed4a3aec33e7de22db/jaraco_context-6.1.2-py3-none-any.whl", hash = "sha256:bf8150b79a2d5d91ae48629d8b427a8f7ba0e1097dd6202a9059f29a36379535", size = 7871, upload-time = "2026-03-20T22:13:32.808Z" }, +] + +[[package]] +name = "jaraco-functools" +version = "4.6.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "more-itertools" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/6c/1f/c23395957d41ccf27c4e535c3d334c4051e5395b3752057ba4cbaec35c56/jaraco_functools-4.6.0.tar.gz", hash = "sha256:880c577ec9720b3a052d5bc611fb9f2269b3d87902ef42440df443b88e443280", size = 20837, upload-time = "2026-07-14T01:28:02.544Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/02/36/ecc85bc96c273dc8a11273ed4782272975e6338d4a3e9228621175edf0e3/jaraco_functools-4.6.0-py3-none-any.whl", hash = "sha256:99e3dc0060c5cbe8fcd1cdb36258e2a65ca40f1566b2033b12abb1bb44dd3c30", size = 11677, upload-time = "2026-07-14T01:28:01.59Z" }, +] + +[[package]] +name = "jeepney" +version = "0.9.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/7b/6f/357efd7602486741aa73ffc0617fb310a29b588ed0fd69c2399acbb85b0c/jeepney-0.9.0.tar.gz", hash = "sha256:cf0e9e845622b81e4a28df94c40345400256ec608d0e55bb8a3feaa9163f5732", size = 106758, upload-time = "2025-02-27T18:51:01.684Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b2/a3/e137168c9c44d18eff0376253da9f1e9234d0239e0ee230d2fee6cea8e55/jeepney-0.9.0-py3-none-any.whl", hash = "sha256:97e5714520c16fc0a45695e5365a2e11b81ea79bba796e26f9f1d178cb182683", size = 49010, upload-time = "2025-02-27T18:51:00.104Z" }, +] + [[package]] name = "keel-broker-alpaca" version = "0.10.0" @@ -571,12 +637,14 @@ name = "keel-core" version = "0.10.0" source = { editable = "packages/keel-core" } dependencies = [ + { name = "keyring" }, { name = "python-dotenv" }, { name = "pyyaml" }, ] [package.metadata] requires-dist = [ + { name = "keyring", specifier = ">=25.6" }, { name = "python-dotenv", specifier = ">=1.2.2" }, { name = "pyyaml", specifier = ">=6.0.3" }, ] @@ -624,6 +692,24 @@ dev = [ { name = "ruff", specifier = ">=0.15.21" }, ] +[[package]] +name = "keyring" +version = "25.7.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "importlib-metadata", marker = "python_full_version < '3.12'" }, + { name = "jaraco-classes" }, + { name = "jaraco-context" }, + { name = "jaraco-functools" }, + { name = "jeepney", marker = "sys_platform == 'linux'" }, + { name = "pywin32-ctypes", marker = "sys_platform == 'win32'" }, + { name = "secretstorage", marker = "sys_platform == 'linux'" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/43/4b/674af6ef2f97d56f0ab5153bf0bfa28ccb6c3ed4d1babf4305449668807b/keyring-25.7.0.tar.gz", hash = "sha256:fe01bd85eb3f8fb3dd0405defdeac9a5b4f6f0439edbb3149577f244a2e8245b", size = 63516, upload-time = "2025-11-16T16:26:09.482Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/81/db/e655086b7f3a705df045bf0933bdd9c2f79bb3c97bfef1384598bb79a217/keyring-25.7.0-py3-none-any.whl", hash = "sha256:be4a0b195f149690c166e850609a477c532ddbfbaed96a404d4e43f8d5e2689f", size = 39160, upload-time = "2025-11-16T16:26:08.402Z" }, +] + [[package]] name = "librt" version = "0.13.0" @@ -699,6 +785,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/5f/5d/3dcec2884ba1b0806d1408612555c38dd5d68e90156b59f75f6e36435c3a/librt-0.13.0-cp314-cp314t-win_arm64.whl", hash = "sha256:2f281549a4c52ac7bb97997f14353f8bd0e53a34ca0dad1c905cfd0b4a58ae99", size = 110771, upload-time = "2026-07-08T12:26:12.303Z" }, ] +[[package]] +name = "more-itertools" +version = "11.1.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/de/1d/f4da6f02cdffe04d6362210b807146a26044c88d839208aec273bb0d9184/more_itertools-11.1.0.tar.gz", hash = "sha256:48e8f4d9e7e5878571ecf6f2b4e57634f93cd474cc8cfbd2376f2d11b396e30d", size = 145772, upload-time = "2026-05-22T14:14:29.909Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/e8/3d/1087453384dbde46a8c7f9356eead2c58be8a7bf156bca40243377c85715/more_itertools-11.1.0-py3-none-any.whl", hash = "sha256:4b65538ae22f6fed0ce4874efd317463a7489796a0939fa66824dd542125a192", size = 72226, upload-time = "2026-05-22T14:14:28.824Z" }, +] + [[package]] name = "mypy" version = "2.3.0" @@ -888,6 +983,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/0b/d7/1959b9648791274998a9c3526f6d0ec8fd2233e4d4acce81bbae76b44b2a/python_dotenv-1.2.2-py3-none-any.whl", hash = "sha256:1d8214789a24de455a8b8bd8ae6fe3c6b69a5e3d64aa8a8e5d68e694bbcb285a", size = 22101, upload-time = "2026-03-01T16:00:25.09Z" }, ] +[[package]] +name = "pywin32-ctypes" +version = "0.2.3" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/85/9f/01a1a99704853cb63f253eea009390c88e7131c67e66a0a02099a8c917cb/pywin32-ctypes-0.2.3.tar.gz", hash = "sha256:d162dc04946d704503b2edc4d55f3dba5c1d539ead017afa00142c38b9885755", size = 29471, upload-time = "2024-08-14T10:15:34.626Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/de/3d/8161f7711c017e01ac9f008dfddd9410dff3674334c233bde66e7ba65bbf/pywin32_ctypes-0.2.3-py3-none-any.whl", hash = "sha256:8a1513379d709975552d202d942d9837758905c8d01eb82b8bcc30918929e7b8", size = 30756, upload-time = "2024-08-14T10:15:33.187Z" }, +] + [[package]] name = "pyyaml" version = "6.0.3" @@ -983,6 +1087,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/dd/75/e90ab9aeece218a9fc5a5bc3ec97d0ee6bb3c4ff95869463c1de58e29a1c/ruff-0.15.21-py3-none-win_arm64.whl", hash = "sha256:6e83115d4b9377c1cbc13abf0e051f069fab0ef815ea0504a8a008cee24dd0a8", size = 11375265, upload-time = "2026-07-09T20:01:31.772Z" }, ] +[[package]] +name = "secretstorage" +version = "3.5.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "cryptography" }, + { name = "jeepney" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/1c/03/e834bcd866f2f8a49a85eaff47340affa3bfa391ee9912a952a1faa68c7b/secretstorage-3.5.0.tar.gz", hash = "sha256:f04b8e4689cbce351744d5537bf6b1329c6fc68f91fa666f60a380edddcd11be", size = 19884, upload-time = "2025-11-23T19:02:53.191Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b7/46/f5af3402b579fd5e11573ce652019a67074317e18c1935cc0b4ba9b35552/secretstorage-3.5.0-py3-none-any.whl", hash = "sha256:0ce65888c0725fcb2c5bc0fdb8e5438eece02c523557ea40ce0703c266248137", size = 15554, upload-time = "2025-11-23T19:02:51.545Z" }, +] + [[package]] name = "tomli" version = "2.4.1" @@ -1096,3 +1213,12 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/41/d8/63d6194aae711d7263df4498200c690a9c39fb437ede10f3e157a6343e0d/websockets-13.1-cp313-cp313-win_amd64.whl", hash = "sha256:c518e84bb59c2baae725accd355c8dc517b4a3ed8db88b4bc93c78dae2974bf2", size = 159144, upload-time = "2024-09-21T17:33:25.96Z" }, { url = "https://files.pythonhosted.org/packages/56/27/96a5cd2626d11c8280656c6c71d8ab50fe006490ef9971ccd154e0c42cd2/websockets-13.1-py3-none-any.whl", hash = "sha256:a9a396a6ad26130cdae92ae10c36af09d9bfe6cafe69670fd3b6da9b07b4044f", size = 152134, upload-time = "2024-09-21T17:34:19.904Z" }, ] + +[[package]] +name = "zipp" +version = "4.1.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/b9/d8/eab98a517c14134c0b2eb4e2387bc5f457334293ec5d2dd3857ec2966802/zipp-4.1.0.tar.gz", hash = "sha256:4cb57381f544315db7688e976e922a2b18cdb513d21cc194eb42232ba2a3e602", size = 26214, upload-time = "2026-05-18T20:08:57.967Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/3a/13/547360d81e6d88d58492968ffda9f9542854f11310ee556fef14260cc886/zipp-4.1.0-py3-none-any.whl", hash = "sha256:25ad4e16390cd314347dd8f1de67a2ac538ae658ed4ab9db16029c07c188e97f", size = 10238, upload-time = "2026-05-18T20:08:57.045Z" }, +]