diff --git a/keel/commands/setup.py b/keel/commands/setup.py
index 173235d..e7402d4 100644
--- a/keel/commands/setup.py
+++ b/keel/commands/setup.py
@@ -75,9 +75,21 @@
class StepKind(str, Enum):
- """Who can perform a step -- which is what decides whether a wizard may touch it."""
+ """Who can perform a step -- which is what decides whether a wizard may touch it.
+
+ `OPERATOR_INPUT` was split out of `JUDGEMENT` when the credential step arrived (#437), and the
+ distinction is worth the fourth member. A JUDGEMENT is a DECISION only a human may make -- a
+ Shariah classification, a promotion -- and a wizard that made one would be making a compliance
+ ruling on the operator's behalf. An OPERATOR_INPUT is a FACT only the operator possesses: an
+ API key. A wizard may record one and cannot possibly invent one, so it is safe to offer as a
+ form in a way a judgement is not.
+
+ Collapsing the two would have forced one of two bad outcomes: either the browser could record
+ an attestation (wrong), or it could never accept a credential (which leaves a desktop user
+ with no way to configure keel at all, since they have no terminal to type one in)."""
MECHANICAL = "mechanical"
+ OPERATOR_INPUT = "operator_input"
JUDGEMENT = "judgement"
OFF_VENUE = "off_venue"
@@ -154,6 +166,21 @@ def blocking(self) -> bool:
),
how="keel rules seed (seeds candidates only; promotion is a separate, deliberate step)",
),
+ Step(
+ key="credentials",
+ title="A market-data credential",
+ kind=StepKind.OPERATOR_INPUT,
+ stage=Stage.PAPER,
+ why=(
+ "Candle history is fetched through an authenticated client, so `keel fetch` without a "
+ "key fails outright -- even in paper mode, where no order can be placed. Only you "
+ "have this key; keel can store it and cannot obtain or guess one."
+ ),
+ how=(
+ "keel credentials set CDP_API_KEY (a free, read-only Coinbase Developer Platform "
+ "key is enough for market data)"
+ ),
+ ),
Step(
key="market_data",
title="Market data",
@@ -342,6 +369,8 @@ def inspect(config_path: str | Path, db_path: str | Path) -> DeploymentState:
f"{config_file} -- mode {mode}, {len(allowlist)} allowlisted asset(s)",
)
+ observations["credentials"] = _credential_observation(config_file)
+
# -- everything that needs the database --
conn: sqlite3.Connection | None = None
if db_file.exists():
@@ -444,6 +473,34 @@ def _database_observations(
return out
+#: The pair `keel fetch` needs. Both must be present: a key with no secret authenticates nothing.
+MARKET_DATA_SECRETS: tuple[str, ...] = ("CDP_API_KEY", "CDP_API_SECRET")
+
+
+def _credential_observation(config_file: Path) -> tuple[bool | None, str]:
+ """Whether keel can see a market-data credential, and WHERE it is coming from.
+
+ The source is reported because it is the difference between the two support questions this
+ can produce -- "keel cannot see my key" and "keel is using a different key than the one I just
+ typed". The VALUE is never read into the message, and `ResolvedSecret` will not print one even
+ if a future edit tries."""
+ from keel_core.secrets import SecretSource, read_secret
+
+ env_path = config_file.parent / ".env"
+ resolved = [read_secret(name, env_path=env_path) for name in MARKET_DATA_SECRETS]
+ missing = [item.name for item in resolved if not item.found]
+ if missing:
+ return (False, f"missing: {', '.join(missing)}")
+ sources = {item.source for item in resolved}
+ if sources == {SecretSource.ENV_FILE}:
+ return (True, "found in your .env file")
+ if sources == {SecretSource.KEYCHAIN}:
+ return (True, "stored in the OS keychain")
+ if sources == {SecretSource.ENVIRONMENT}:
+ return (True, "set in this process's environment")
+ return (True, "found (" + ", ".join(sorted(s.value for s in sources)) + ")")
+
+
def _asset_observation(conn: sqlite3.Connection, config: Any) -> tuple[bool | None, str]:
"""Attested against the ALLOWLIST, not against the count.
@@ -566,17 +623,27 @@ def _state_as_json(state: DeploymentState) -> dict[str, Any]:
#
# Everything below WRITES. Read the rules before adding to it:
#
-# 1. Only steps declared `MECHANICAL` may appear here. `JUDGEMENT` steps are the operator's --
-# a wizard may collect and record them but must never decide them -- and `OFF_VENUE` steps
-# happen somewhere keel cannot reach. `tests/commands/test_setup_actions.py` enforces this
-# against `STEPS`, so a new action for a judgement step fails rather than shipping.
+# 1. Only `MECHANICAL` and `OPERATOR_INPUT` steps may appear here, and the line between them and
+# the rest is the point. A MECHANICAL step has no input: the machine simply does it. An
+# OPERATOR_INPUT step is a FACT only the operator has -- an API key -- which a wizard can
+# record and could not possibly invent.
+#
+# `JUDGEMENT` steps are DECISIONS: a Shariah classification, a promotion. A form that recorded
+# one would be making a compliance ruling on the operator's behalf, and no amount of "but they
+# clicked it" makes that the same thing as their having decided it. `OFF_VENUE` steps happen
+# somewhere keel cannot reach at all. Neither may ever be an action here, and the tests
+# enforce it against `STEPS` rather than against a list kept in this file.
#
-# 2. Every action is IDEMPOTENT and NEVER destructive. A setup flow is something a nervous user
+# 2. An action with `inputs` records ONLY what was submitted. It has no defaults, no fallbacks and
+# no "sensible guess" -- an action that could fill in a field the operator left blank is one
+# that could record something they never supplied.
+#
+# 3. Every action is IDEMPOTENT and NEVER destructive. A setup flow is something a nervous user
# clicks twice, and a browser reload re-submits. "The config already exists" is a successful
# outcome that changed nothing -- never an overwrite, and there is deliberately no `force`
# parameter for any web caller to pass.
#
-# 3. Nothing here increases what keel can DO. Not one of the eleven capability-increasing actions
+# 4. Nothing here increases what keel can DO. Not one of the eleven capability-increasing actions
# in `keel/capabilities.py` is reachable from this module, and a test asserts the two sets are
# disjoint. Creating a config, a schema and a library of CANDIDATE rules leaves an engine that
# still places nothing: candidates trade nothing until a human promotes them, and promotion is
@@ -609,16 +676,36 @@ class ActionResult:
message: str
+@dataclass(frozen=True)
+class ActionInput:
+ """One field an action needs from the operator.
+
+ `secret=True` means the value must never be rendered back into a page, echoed into a log, or
+ put in a URL. It is the difference between a `password` field and a `text` one, and between a
+ form that can be submitted safely and one that leaks its own contents into browser history."""
+
+ name: str
+ label: str
+ secret: bool = False
+
+
@dataclass(frozen=True)
class Action:
key: str
title: str
#: What it will do, in the operator's words, shown before they choose it.
detail: str
- run: Callable[[Path, Path], ActionResult]
+ run: Callable[[Path, Path, dict[str, str]], ActionResult]
+ #: Empty for an action the machine performs unaided. Non-empty means it records what the
+ #: operator supplied and nothing else -- see rule 2 above.
+ inputs: tuple[ActionInput, ...] = ()
+
+ @property
+ def needs_input(self) -> bool:
+ return bool(self.inputs)
-def create_config(config_path: Path, db_path: Path) -> ActionResult:
+def create_config(config_path: Path, _db_path: Path, _values: dict[str, str]) -> ActionResult:
"""Write the PAPER template if there is no config. Never overwrites.
Paper deliberately, and never the live template from here: paper places nothing at all, which
@@ -632,7 +719,7 @@ def create_config(config_path: Path, db_path: Path) -> ActionResult:
return ActionResult("config", True, f"wrote {config_path} (paper -- places no orders)")
-def create_database(config_path: Path, db_path: Path) -> ActionResult:
+def create_database(_config_path: Path, db_path: Path, _values: dict[str, str]) -> ActionResult:
"""Create the database if absent and apply outstanding migrations.
Migrations run every time, not only on first run, because that is what makes an upgrade
@@ -653,7 +740,7 @@ def create_database(config_path: Path, db_path: Path) -> ActionResult:
return ActionResult("database", True, f"created {db_path} at the current schema")
-def seed_rule_library(config_path: Path, db_path: Path) -> ActionResult:
+def seed_rule_library(config_path: Path, db_path: Path, _values: dict[str, str]) -> ActionResult:
"""Seed one CANDIDATE rule per (kind, allowlisted product).
Candidates trade nothing. Promoting one is a separate, deliberate, human step -- which is why
@@ -699,6 +786,51 @@ def seed_rule_library(config_path: Path, db_path: Path) -> ActionResult:
)
+def store_market_data_credential(
+ config_path: Path, _db_path: Path, values: dict[str, str]
+) -> ActionResult:
+ """Record the CDP key and secret in the OS keychain.
+
+ Records EXACTLY what was submitted. Both fields are required and neither has a default: an
+ action that could fill in a field the operator left blank is one that could record something
+ they never supplied.
+
+ Nothing about the value reaches the result message, and `ResolvedSecret` refuses to print one
+ even if a future edit tries. The confirmation is read back through the same resolver a real
+ caller uses, so if a `.env` shadows what was just stored, the operator hears it now rather
+ than at the first fetch that used the other key.
+ """
+ from keel_core.secrets import SecretSource, read_secret, store_secret
+
+ missing = [name for name in MARKET_DATA_SECRETS if not values.get(name, "").strip()]
+ if missing:
+ return ActionResult(
+ "credentials", False, f"nothing saved -- {', '.join(missing)} was blank"
+ )
+
+ try:
+ for name in MARKET_DATA_SECRETS:
+ store_secret(name, values[name].strip())
+ except Exception as exc:
+ # The message names the `.env` alternative; see `keel_core.secrets.store_secret`.
+ return ActionResult("credentials", False, str(exc))
+
+ env_path = config_path.parent / ".env"
+ shadowed = [
+ name
+ for name in MARKET_DATA_SECRETS
+ if read_secret(name, env_path=env_path).source is not SecretSource.KEYCHAIN
+ ]
+ if shadowed:
+ return ActionResult(
+ "credentials",
+ True,
+ "saved to the OS keychain, but keel will still read "
+ f"{', '.join(shadowed)} from your .env or environment, which takes precedence",
+ )
+ return ActionResult("credentials", True, "saved to the OS keychain")
+
+
#: THE closed set of steps a machine may perform on the operator's behalf.
ACTIONS: tuple[Action, ...] = (
Action(
@@ -725,6 +857,20 @@ def seed_rule_library(config_path: Path, db_path: Path) -> ActionResult:
),
run=seed_rule_library,
),
+ Action(
+ key="credentials",
+ title="Save a market-data credential",
+ detail=(
+ "A free, read-only Coinbase Developer Platform key is enough for candle history. It "
+ "is stored in your operating system's keychain, not in a file, and keel never "
+ "displays it again."
+ ),
+ run=store_market_data_credential,
+ inputs=(
+ ActionInput("CDP_API_KEY", "CDP API key"),
+ ActionInput("CDP_API_SECRET", "CDP API secret", secret=True),
+ ),
+ ),
)
#: Mechanical steps that are deliberately NOT offered as one-click actions, and why. Recorded as
diff --git a/keel/web/render.py b/keel/web/render.py
index 2439f9d..e71e446 100644
--- a/keel/web/render.py
+++ b/keel/web/render.py
@@ -90,6 +90,10 @@
padding: 1rem 1.25rem; }
pre { white-space: pre-wrap; word-break: break-word; margin: 0; font-size: 0.85rem; }
form { margin: 0.5rem 0 0; }
+.field { display: flex; flex-direction: column; gap: 0.2rem; margin: 0.6rem 0; max-width: 26rem; }
+.field span { font-size: 0.8rem; color: var(--muted); }
+.field input { font: inherit; padding: 0.4rem 0.6rem; border-radius: 7px;
+ border: 1px solid var(--line); background: var(--bg); color: var(--fg); }
button { font: inherit; font-weight: 550; padding: 0.35rem 0.9rem; border-radius: 7px;
border: 1px solid var(--accent); background: var(--accent); color: var(--card);
cursor: pointer; }
@@ -705,14 +709,31 @@ def render_setup(
def _action_form(action: Any, csrf: str) -> str:
- """One button, one action key, one write token. No hidden parameters: an action takes no
- arguments at all, so there is nothing a crafted form could ask for that the registry does not
- already fix."""
+ """One action key, one write token, and only the fields the action itself declares.
+
+ A field marked `secret` renders as `type="password"` and is NEVER given a `value` -- not even
+ on a re-render after a failure. Pre-filling a secret field puts the secret in the page source,
+ where it survives a screenshot, a "view source", and anything that saves the page. The cost is
+ that a failed submission must be retyped; that is the correct cost.
+
+ `autocomplete="off"` on the secret fields keeps a browser password manager from offering to
+ store an exchange API key as though it were a website login."""
+ fields = ""
+ for field in getattr(action, "inputs", ()):
+ kind = "password" if field.secret else "text"
+ extra = ' autocomplete="off" spellcheck="false"' if field.secret else ""
+ fields += (
+ '"
+ )
return (
f'"
)
diff --git a/keel/web/server.py b/keel/web/server.py
index 021a26c..4aa7d38 100644
--- a/keel/web/server.py
+++ b/keel/web/server.py
@@ -303,15 +303,20 @@ def page_glossary(_cfg: ServeConfig, _query: dict[str, list[str]]) -> tuple[str,
SETUP_ACTION_PREFIX = "/setup/"
-def run_setup_action(cfg: ServeConfig, key: str) -> Any:
- """Perform one declared mechanical action. Returns its `ActionResult`, or `None` for a key
- that is not in the closed set -- never a lookup that falls through to something else."""
+def run_setup_action(cfg: ServeConfig, key: str, form: dict[str, str]) -> Any:
+ """Perform one declared setup action. Returns its `ActionResult`, or `None` for a key that is
+ not in the closed set -- never a lookup that falls through to something else.
+
+ Only the fields the action DECLARES are passed through. A submitted field the action did not
+ ask for is dropped rather than forwarded: the form is attacker-shaped input the moment anyone
+ can craft a POST, and an action should never receive a key it has no name for."""
from keel.commands.setup import action_for
action = action_for(key)
if action is None:
return None
- return action.run(Path(cfg.config_path), Path(cfg.db_path))
+ values = {field.name: form.get(field.name, "") for field in action.inputs}
+ return action.run(Path(cfg.config_path), Path(cfg.db_path), values)
def _close(repo: Any) -> None:
@@ -463,7 +468,7 @@ def do_POST(self) -> None: # noqa: N802 - stdlib's naming, not ours
key = parsed.path[len(SETUP_ACTION_PREFIX) :]
try:
- result = run_setup_action(self.cfg, key)
+ result = run_setup_action(self.cfg, key, body)
except Exception as exc:
self._refuse(500, "That step could not be completed", f"{type(exc).__name__}: {exc}")
return
@@ -474,6 +479,11 @@ def do_POST(self) -> None: # noqa: N802 - stdlib's naming, not ours
# POST/redirect/GET: a browser reload must not re-submit. The actions are idempotent, so
# a re-submission would be harmless -- but "harmless" is not a reason to leave a
# re-submitting page in a setup flow someone is clicking nervously.
+ #
+ # The Location carries the step KEY and nothing else. A submitted value in a redirect URL
+ # is a secret in browser history, in the Referer header of anything the page later loads,
+ # and in any proxy log between here and nowhere -- which is the whole reason the form is a
+ # POST in the first place.
self._send(303, "", extra=(("Location", f"/setup?ran={quote(result.step_key)}"),))
def _read_form(self) -> dict[str, str]:
diff --git a/tests/web/test_server.py b/tests/web/test_server.py
index eb10ce1..8f81579 100644
--- a/tests/web/test_server.py
+++ b/tests/web/test_server.py
@@ -150,21 +150,61 @@ def test_post_is_refused_everywhere_except_the_setup_actions(
# -- the guarantee that replaced "no POST at all" ---------------------------------------------
-def test_the_write_surface_is_exactly_the_mechanical_steps() -> None:
- """`ACTIONS` is the whole write surface, and every member must be a step declared
- MECHANICAL. A judgement step is the operator's -- a wizard may record one but must never
- decide it -- and an off-venue step happens where keel cannot reach. This is what stops a
- button for "attest this asset" from being added as markup."""
+def test_the_write_surface_never_covers_a_judgement_or_an_off_venue_step() -> None:
+ """The line that matters, and it is not "mechanical only" any more.
+
+ A MECHANICAL step has no input: the machine does it. An OPERATOR_INPUT step is a FACT only the
+ operator has -- an API key -- which a wizard can record and could not possibly invent. Both
+ are safe to offer as a form.
+
+ A JUDGEMENT is a DECISION: a Shariah classification, a promotion. A form that recorded one
+ would be making a compliance ruling on the operator's behalf, and no amount of "but they
+ clicked it" makes that the same thing as their having decided it. An OFF_VENUE step happens
+ somewhere keel cannot reach at all. Neither may ever be an action.
+ """
from keel.commands.setup import ACTIONS, STEPS, StepKind
- mechanical = {step.key for step in STEPS if step.kind is StepKind.MECHANICAL}
+ by_key = {step.key: step for step in STEPS}
declared = {action.key for action in ACTIONS}
- assert declared <= mechanical, sorted(declared - mechanical)
assert declared, "an empty write surface would make every test below vacuous"
- for step in STEPS:
- if step.kind is not StepKind.MECHANICAL:
- assert step.key not in declared, step.key
+ for key in declared:
+ assert key in by_key, f"{key} is an action over no declared step"
+ assert by_key[key].kind in (StepKind.MECHANICAL, StepKind.OPERATOR_INPUT), key
+
+ forbidden = {
+ step.key for step in STEPS if step.kind in (StepKind.JUDGEMENT, StepKind.OFF_VENUE)
+ }
+ assert forbidden, "no judgement or off-venue steps exist, so this proves nothing"
+ assert not (declared & forbidden), sorted(declared & forbidden)
+
+
+def test_an_action_declares_inputs_exactly_when_its_step_needs_them() -> None:
+ """So a mechanical action cannot quietly start accepting operator data, and an operator-input
+ action cannot quietly stop requiring it -- either drift would move a step across the line the
+ test above draws, without touching that test."""
+ from keel.commands.setup import ACTIONS, STEPS, StepKind
+
+ by_key = {step.key: step for step in STEPS}
+ for action in ACTIONS:
+ needs = by_key[action.key].kind is StepKind.OPERATOR_INPUT
+ assert action.needs_input is needs, action.key
+
+
+def test_the_credential_form_never_renders_a_value_back_into_the_page() -> None:
+ """Pre-filling a secret field puts the secret in the page source, where it survives a
+ screenshot, a "view source", and anything that saves the page. A failed submission must be
+ retyped; that is the correct cost."""
+ from keel.commands.setup import ACTIONS
+ from keel.web import render
+
+ secret_actions = [a for a in ACTIONS if any(f.secret for f in a.inputs)]
+ assert secret_actions, "no secret fields exist, so this proves nothing"
+ for action in secret_actions:
+ html = render._action_form(action, "csrf-token")
+ assert 'type="password"' in html
+ assert "value=" not in html.split('type="password"')[1].split(">")[0]
+ assert 'autocomplete="off"' in html
def test_no_capability_increasing_action_is_reachable_from_the_web_layer() -> None:
@@ -677,3 +717,105 @@ def render_esc(value: str) -> str:
from keel.web import render
return render.esc(value)
+
+
+# -- the credential form, over the wire --------------------------------------------------------
+
+
+def test_a_submitted_secret_never_appears_in_a_response_or_a_redirect(
+ empty_machine: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ """A secret in a redirect URL is a secret in browser history, in the Referer header of
+ anything the page later loads, and in any proxy log in between -- which is the whole reason
+ the form is a POST. This drives the real wire and then re-reads every page."""
+ from keel.commands import setup as setup_mod
+
+ stored: dict[str, str] = {}
+ monkeypatch.setattr(
+ "keel_core.secrets.store_secret", lambda name, value: stored.__setitem__(name, value)
+ )
+ monkeypatch.setattr("keel_core.secrets.keychain_available", lambda: True)
+ monkeypatch.setattr("keel_core.secrets._from_keychain", lambda name: stored.get(name))
+
+ secret = "cdp-secret-that-must-never-be-echoed"
+ status, headers, body = _request(
+ empty_machine,
+ "/setup/credentials",
+ method="POST",
+ cookie=_session(empty_machine),
+ form={
+ "csrf": _csrf(empty_machine),
+ "CDP_API_KEY": "cdp-key-value",
+ "CDP_API_SECRET": secret,
+ },
+ )
+ assert status == 303
+ assert headers["Location"] == "/setup?ran=credentials"
+ assert secret not in headers["Location"]
+ assert secret not in body
+ assert stored["CDP_API_SECRET"] == secret
+
+ for path in ROUTES:
+ _status, _headers, page = _request(empty_machine, path, cookie=_session(empty_machine))
+ assert secret not in page, path
+ assert "cdp-key-value" not in page, path
+
+ assert setup_mod.MARKET_DATA_SECRETS == ("CDP_API_KEY", "CDP_API_SECRET")
+
+
+def test_a_blank_field_records_nothing(
+ empty_machine: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ """An action that could fill in a field the operator left blank is one that could record
+ something they never supplied."""
+ stored: dict[str, str] = {}
+ monkeypatch.setattr(
+ "keel_core.secrets.store_secret", lambda name, value: stored.__setitem__(name, value)
+ )
+ status, _headers, _body = _request(
+ empty_machine,
+ "/setup/credentials",
+ method="POST",
+ cookie=_session(empty_machine),
+ form={"csrf": _csrf(empty_machine), "CDP_API_KEY": "k", "CDP_API_SECRET": " "},
+ )
+ assert status == 303
+ assert stored == {}
+
+
+def test_a_field_the_action_did_not_declare_is_dropped(
+ empty_machine: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ """The form is attacker-shaped input the moment anyone can craft a POST, so an action should
+ never receive a key it has no name for."""
+ seen: dict[str, str] = {}
+
+ def _capture(_config: object, _db: object, values: dict[str, str]) -> object:
+ seen.update(values)
+ from keel.commands.setup import ActionResult
+
+ return ActionResult("credentials", False, "captured")
+
+ from keel.commands import setup as setup_mod
+
+ monkeypatch.setattr(
+ setup_mod,
+ "ACTIONS",
+ tuple(
+ a if a.key != "credentials" else type(a)(a.key, a.title, a.detail, _capture, a.inputs)
+ for a in setup_mod.ACTIONS
+ ),
+ )
+ _request(
+ empty_machine,
+ "/setup/credentials",
+ method="POST",
+ cookie=_session(empty_machine),
+ form={
+ "csrf": _csrf(empty_machine),
+ "CDP_API_KEY": "k",
+ "CDP_API_SECRET": "s",
+ "SOMETHING_ELSE": "should not arrive",
+ },
+ )
+ assert set(seen) == {"CDP_API_KEY", "CDP_API_SECRET"}