feat(secrets): the OS keychain as a place a credential can live, with .env still winning

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

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."
+        )

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}

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]