diff --git a/docs/specs.md b/docs/specs.md index 82c7fa9..1db93b3 100644 --- a/docs/specs.md +++ b/docs/specs.md @@ -5,8 +5,8 @@ `logos-evm-keystore-module` is the **keystore** for the Logos multi-chain EVM wallet. It is a Rust **cdylib Logos module** (`type: core`, `interface: cdylib`) that owns everything to do with private keys: generating and importing them, -encrypting them at rest as scrypt vaults, holding them unlocked in memory for a -bounded time, and producing **secp256k1 signatures** — both EIP-191 +encrypting them at rest as scrypt vaults, and producing **secp256k1 signatures** +**only when a human has approved them** — both EIP-191 `personal_sign` messages and signed EIP-1559 / legacy (EIP-155) transactions. Its defining property is **isolation**: the module does **no networking**, and a @@ -35,8 +35,8 @@ process-isolated Logos modules over a typed RPC bridge: This module is a **leaf**: it declares **no dependencies** (`"dependencies": []`) and calls no other module. It is driven *by* the `logos-evm-wallet-backend-module` -coordinator (which calls `unlock` then `sign_transaction` / `sign_message` as the -signing leg of its send pipeline) or directly by the headless `logoscore` +coordinator (which *requests* an approval as the signing leg of its +send pipeline, and never handles a vault password) or directly by the headless `logosctl` runtime. Keeping the keystore a dependency-free leaf is deliberate: the component that holds private keys has the smallest possible attack surface and pulls in no network-capable code. @@ -48,8 +48,9 @@ network-capable code. The module is two layers that are deliberately decoupled by a Cargo feature: * **The crypto core** (`rust-lib/src/keystore.rs`) — pure, offline, Logos-free - Rust. The `Keystore` struct manages a directory of scrypt vault files plus the - set of currently-unlocked in-memory signers. Unit-tested on its own with + Rust. The `Keystore` struct manages a directory of scrypt vault files. It holds **no** + decrypted keys: a signer is derived per approval and zeroized before the call + returns. Unit-tested on its own with `cargo test --no-default-features`. * **The Logos glue** (`rust-lib/src/glue.rs`) — the `KeystoreModule` contract trait and its implementation, compiled only behind the default `logos_module` @@ -60,8 +61,9 @@ The module is two layers that are deliberately decoupled by a Cargo feature: ```mermaid flowchart TB subgraph Callers["Callers (over Logos bridge)"] - BE["wallet_backend_module
(send pipeline: unlock → sign)"] - LC["logoscore daemon
(call / module-info)"] + BE["wallet_backend_module
(requester: request_approval)"] + SU["signer_ui
(the ONLY approver)"] + LC["logosctl daemon
(Tier C only)"] end subgraph Module["keystore_module (Rust cdylib, type: core)"] @@ -69,14 +71,15 @@ flowchart TB DISP["Generated C-ABI dispatch + install()
(injected from generated/provider_gen.rs)"] subgraph Glue["Logos glue — src/glue.rs (feature logos_module)"] TRAIT["trait KeystoreModule
(the IPC contract)"] - IMPL["KeystoreModuleImpl { ks: Option<Keystore> }"] + IMPL["KeystoreModuleImpl
{ ks, approvals, approver }"] + GATE["Tier gate
current_caller() → A / B / C"] EV["KeystoreModuleEvents::accounts_changed
→ emit_accounts_changed(count)"] CTX["on_context_ready(ctx)
ks = Keystore::new(ctx.instance_persistence_path/keystore)"] end subgraph Core["Crypto core — src/keystore.rs (no network, no Logos)"] - KS["struct Keystore
{ dir, unlocked: HashMap<Address, Unlocked> }"] + KS["struct Keystore { dir }
(no signer cache)"] VAULTS["scrypt vault files
<lowercase-hex-addr>.json"] - MEM["in-memory unlocked signers
(PrivateKeySigner + optional TTL)"] + MEM["signer: a LOCAL inside approve()
derived per approval, then zeroized"] end end @@ -86,10 +89,12 @@ flowchart TB BIP39["coins-bip39 / coins-bip32
(via alloy re-export)"] end - BE -->|invokeRemoteMethod| DISP - LC -->|call| DISP + BE -->|Tier B: request_approval| DISP + SU -->|Tier A: acknowledge / approve| DISP + LC -->|Tier C only| DISP DISP --> TRAIT - TRAIT --> IMPL + TRAIT --> GATE + GATE --> IMPL IMPL --> KS IMPL -.->|on account set change| EV EV -.->|event| Callers @@ -105,10 +110,13 @@ flowchart TB ``` **Key isolation, restated against the diagram:** raw private-key bytes exist only -inside `Core` — encrypted in the vault files and decrypted briefly into the -in-memory `unlocked` map. They are never returned through `TRAIT` to a caller. -Only `Address` strings, signature/transaction hex, and re-encrypted keystore JSON -travel back across the `DISP` boundary. +inside `Core` — encrypted in the vault files, and decrypted **only into a local +variable inside `approve()`**, which zeroizes it before returning. There is no +map, no cache and no TTL, so there is no window during which some *other* caller +could reach a live signer. Keys are never returned through `TRAIT`. Only +`Address` strings, signature/transaction hex, and re-encrypted keystore JSON +travel back across the `DISP` boundary — and a signature only ever leaves as the +result of a `Rendered` request a human approved. --- @@ -116,44 +124,51 @@ travel back across the `DISP` boundary. This module is a **leaf** — it makes **no outbound calls** to any other module and opens no sockets. The interesting flow is therefore how a **caller drives it**. -The canonical driver is `wallet_backend_module`, whose "send" pipeline uses the -keystore as its signing leg, and `logoscore` for ad-hoc/test calls. +The canonical flow has **three** parties, not two: a **requester** that may ask +but never approve, the **approver** (`signer_ui`) that renders and takes the vault +password, and the human. `logosctl` can reach Tier C only. ```mermaid sequenceDiagram autonumber - participant BE as wallet_backend_module (caller) + participant BE as wallet_backend_module (requester) participant KS as keystore_module (this repo) - participant DISK as scrypt vault dir
(instance_persistence_path/keystore) - participant MEM as in-memory unlocked map + participant SU as signer_ui (the ONLY approver) + participant H as the human + participant DISK as scrypt vault dir - Note over KS: on_context_ready(ctx) →
Keystore::new(ctx.instance_persistence_path/keystore) + Note over KS: on_context_ready(ctx) → Keystore::new(...)
approver read from keystore.json (default "signer_ui") - BE->>KS: import_private_key(priv_hex, password) - KS->>DISK: encrypt_key (scrypt) → <addr>.json - KS-->>BE: { ok:true, address } - Note right of KS: emits accounts_changed(count) + BE->>KS: request_approval({ address, purpose, legs }) + Note right of KS: caller must be a NAMED module (Tier B) + KS-->>BE: { ok, handle, receipt } %% receipt returned exactly once + KS--)SU: event approval_offered(handle) %% handle only — no token, no intent - BE->>KS: unlock(address, password) - KS->>DISK: decrypt_key(<addr>.json, password) - KS->>MEM: insert PrivateKeySigner (no TTL) - KS-->>BE: true + SU->>KS: acknowledge(handle) + Note right of KS: Tier A — refuses anyone but the approver.
Demotes any other Rendered record. + KS-->>SU: { bundle_id, requester, render_lines } + SU->>H: render_lines VERBATIM + bundle_id + Note over H: no timeout on the human - BE->>KS: sign_transaction(address, unsigned_tx_json, chain_id) - KS->>MEM: live_signer(address) (evict if TTL elapsed) - Note right of KS: build TxEip1559 / TxLegacy,
sign hash, EIP-2718 encode - KS-->>BE: { ok:true, raw: "0x02…" } + H->>SU: vault password + SU->>KS: approve(handle, bundle_id, password) + KS->>DISK: decrypt vault (scrypt) → signer (a local) + Note right of KS: re-parse intent, re-derive commitment,
compare to bundle_id, sign every leg, ZEROIZE + KS-->>SU: { ok, signed_count: n } %% a COUNT — the approver never gets the signatures + KS--)BE: event approval_settled(handle, "approved") + + BE->>KS: fetch_result(handle, receipt) + KS-->>BE: { ok, signed: [...] } %% idempotent until ack_result + BE->>KS: ack_result(handle, receipt) Note over BE: backend broadcasts raw tx via eth_rpc_module
(keystore never touches the network) - - BE->>KS: lock(address) - KS->>MEM: remove signer - KS-->>BE: true ``` -The signed `raw` transaction hex that `sign_transaction` returns is what the -backend then hands to `eth_rpc_module` for `eth_sendRawTransaction`. The keystore -itself never performs that broadcast — it has no network code at all. +The signed values the backend collects are what it hands to `eth_rpc_module` for +`eth_sendRawTransaction`. The keystore itself never performs that broadcast — it +has no network code at all. Note the password crosses **only** the `signer_ui` → +`keystore` edge: the requester never sees it, never sees `render_lines`, and +cannot produce a signature. --- @@ -171,17 +186,17 @@ is *defaulted*, so it is a framework hook and **not** part of the IPC contract. JSON object that is either `{ "ok": true, … }` on success or `{ "ok": false, "error": "" }` on failure. The `err()` helper produces the error shape; the message text comes from `KeystoreError` (`Display`). -* **Boolean methods** (`has_address`, `delete_account`, `unlock`, `timed_unlock`, - `lock`, `is_unlocked`) return a bare `bool` and are **fail-soft**: any internal +* **Boolean methods** (`has_address`, `delete_account`, `ack_result`, + `cancel_approval`, `reject`) return a bare `bool` and are **fail-soft**: any internal error (bad address, wrong password, keystore not yet initialized) maps to `false` rather than an error object. * **Addresses** are accepted with or without a `0x` prefix, in any case; no EIP-55 checksum is required (`parse_address` decodes the 20 raw bytes directly). The - `logoscore` CLI auto-types a `0x…` argument as a number, so in CLI examples + `logosctl` CLI auto-types a `0x…` argument as a number, so in CLI examples addresses are passed as **bare hex**. * **Numeric transaction fields** cross as hex (`0x…`) **or** decimal strings to avoid precision loss over JSON; empty/missing numeric fields default to `0`. -* **`logoscore call` argument typing:** `true`/`false` → bool, integers → int, +* **`logosctl call` argument typing:** `true`/`false` → bool, integers → int, else string; `@file` loads file contents as the argument. ### Method index @@ -197,12 +212,16 @@ is *defaulted*, so it is a framework hook and **not** part of the IPC contract. | `list_accounts` | — | `{ ok, accounts: [..] }` | no | | `has_address` | `address: String` | `bool` | no | | `delete_account` | `address, password: String` | `bool` | yes → event | -| `unlock` | `address, password: String` | `bool` | no | -| `timed_unlock` | `address, password: String, seconds: i64` | `bool` | no | -| `lock` | `address: String` | `bool` | no | -| `is_unlocked` | `address: String` | `bool` | no | -| `sign_transaction` | `address, unsigned_tx_json: String, chain_id: i64` | `{ ok, raw }` | no | -| `sign_message` | `address, message: String` | `{ ok, signature }` | no | +| `request_approval` | `intent_json: String` | `{ ok, handle, receipt, state }` | no | +| `approval_status` | `handle, receipt: String` | `{ ok, state, reason? }` | no | +| `fetch_result` | `handle, receipt: String` | `{ ok, signed: [..] }` | no | +| `ack_result` | `handle, receipt: String` | `bool` | no | +| `cancel_approval` | `handle, receipt: String` | `bool` | no | +| `pending` | — | `{ ok, pending: [..] }` | no | +| `acknowledge` | `handle: String` | `{ ok, bundle_id, requester, render_lines }` | no | +| `approve` | `handle, bundle_id, password: String` | `{ ok, signed_count: n }` | no | +| `reject` | `handle: String` | `bool` | no | +| `caller_identity` | — | `{ ok, kind, identity, approver }` | no | --- @@ -220,7 +239,7 @@ but it is seed material — treat it as a secret). **Error:** `{ "ok": false, "error": "word count must be 12/15/18/21/24, got 13" }` ```bash -logoscore call keystore_module create_mnemonic 12 +logosctl call keystore_module create_mnemonic 12 # → {"ok":true,"phrase":"… twelve words …"} ``` @@ -246,7 +265,7 @@ emits `accounts_changed`. **Error:** `{ "ok": false, "error": "" }` ```bash -logoscore call keystore_module import_mnemonic \ +logosctl call keystore_module import_mnemonic \ @mnemonic.json # {"phrase":"test test … junk","accountIndex":0,"password":"pw"} # → {"ok":true,"address":"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"} ``` @@ -264,7 +283,7 @@ scrypt vault under `password`. Emits `accounts_changed`. **Error:** `{ "ok": false, "error": "vault error: …" }` (e.g. I/O failure) ```bash -logoscore call keystore_module new_account hunter2 +logosctl call keystore_module new_account hunter2 ``` --- @@ -282,7 +301,7 @@ call, and it is never returned. Emits `accounts_changed`. **Error:** `{ "ok": false, "error": "invalid private key: …" }` ```bash -logoscore call keystore_module import_private_key \ +logosctl call keystore_module import_private_key \ ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 pw # → {"ok":true,"address":"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"} ``` @@ -331,7 +350,7 @@ the list is sorted. **Error:** `{ "ok": false, "error": "keystore not initialized (context not ready)" }` ```bash -logoscore call keystore_module list_accounts +logosctl call keystore_module list_accounts # → {"ok":true,"accounts":["0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"]} ``` @@ -345,7 +364,7 @@ or uninitialized keystore returns `false`. * **`address`** — address to check. ```bash -logoscore call keystore_module has_address f39fd6e51aad88f6f4ce6ab8827279cfffb92266 +logosctl call keystore_module has_address f39fd6e51aad88f6f4ce6ab8827279cfffb92266 # → true ``` @@ -354,8 +373,7 @@ logoscore call keystore_module has_address f39fd6e51aad88f6f4ce6ab8827279cfffb92 ### `delete_account(address: String, password: String) -> bool` Permanently delete an account's vault. **Password-gated**: the vault is decrypted -with `password` first; only on success is the file removed and any unlocked signer -evicted. Emits `accounts_changed`. Returns `true` only when a vault existed, the +with `password` first; only on success is the file removed. Emits `accounts_changed`. Returns `true` only when a vault existed, the password was correct, and the file was removed; `false` otherwise (missing vault, wrong password, parse error). @@ -363,125 +381,364 @@ wrong password, parse error). * **`password`** — its vault password (required to authorize deletion). ```bash -logoscore call keystore_module delete_account
pw +logosctl call keystore_module delete_account
pw # → true ``` --- -### `unlock(address: String, password: String) -> bool` +## Human-approved signing -Decrypt the account's vault with `password` and hold the resulting -`PrivateKeySigner` in memory **with no expiry**, enabling subsequent signing. -Returns `true` on success, `false` on any failure (missing vault, wrong password, -invalid key). The decrypted key lives only in the in-memory `unlocked` map. +There is **no way to make this module sign anything except by a human approving +it.** The methods that used to sign on demand — `unlock`, `timed_unlock`, `lock`, +`is_unlocked`, `sign_transaction`, `sign_message`, `sign_digest` — are **deleted +from the contract**, not merely gated. There is no unlocked-signer cache: the +signing key is derived from the vault password *inside* `approve()`, used, and +zeroized before the call returns. -* **`address`** — account to unlock. -* **`password`** — its vault password. +### The three tiers -```bash -logoscore call keystore_module unlock f39fd6e51aad88f6f4ce6ab8827279cfffb92266 pw -# → true +Every request is classified by the **caller identity** the platform reports +(`logos_rust_sdk::current_caller()`). + +| Tier | Methods | Admits | +|------|---------|--------| +| **A** | `pending`, `acknowledge`, `approve`, `reject` | the configured **approver only** (default `signer_ui`) | +| **B** | `request_approval`, `approval_status`, `fetch_result`, `ack_result`, `cancel_approval` | any **named module**; `fetch`/`ack`/`cancel`/`status` additionally require the **receipt** | +| **C** | account management (`create_mnemonic` … `delete_account`) | ungated / password-gated | + +Tiers A and B both return the identical string `{"ok":false,"error":"not authorized"}` +on refusal, so a caller cannot use the error text to probe which tier it failed. + +### Caller identity — live, and what it reports + +Caller identity **works**. Measured 2026-08-26 against a `logoscore` daemon built from +master, with `keystore_module` and a purpose-built `caller_probe` module: + +| call | `caller_identity()` reports | +|------|------------------------------| +| `logosctl` → keystore | `{"kind":"host","identity":""}` | +| `logosctl` → probe (`current_caller()` directly) | `HostAnchor` | +| **probe module → keystore** (real module plane) | **`{"kind":"module","identity":"caller_probe"}`** | +| **probe → keystore `request_approval`** (Tier B) | **`{"ok":true,"handle":"ksh_…","receipt":"ksc_…"}`** | + +Row 3 is the positive path: a named module is named correctly. Row 4 is the **first +end-to-end proof that the tier gate admits a legitimate caller** rather than merely +refusing everyone. + +The host now carries the accessor the plugin pulls (`currentCallerJson` present in +`logos_host_qt`, and exactly **one** `logos-qt-host` in the closure), and the +`No such method LogosAPI::currentCallerJson` warning that used to fire on every +dispatch is **gone**. Three defects had to be fixed for this, in this order — the +sequencing is worth keeping because it looked like unrelated work: + +1. **qt-host provenance.** `logos-qt-sdk` exported the `logos-qt-host` it was itself + built against — by propagation, by a baked absolute-store-path `HINTS`, and by + forwarding headers containing literal `#include "/nix/store/…"`. So the module host + linked a stale `LogosAPI` with no `Q_INVOKABLE currentCallerJson`, the plugin's + cross-image `invokeMethod` failed, and the pushed caller document was empty. +2. **The announced origin.** `logos-rust-sdk` hardcoded `"core"` as the origin of a + module's *outbound* client. `lidl-gen` now emits `LOGOS_MODULE_NAME` from the + contract and latches it before the install hook. (C++ was never affected.) +3. **Naming the anchor.** `authorize()` enforced "an anchor key is never a module name" + on the credential store but not on the caller-keyed one, so `"core"` arriving there + would have been spelled as a module. + +**`HostAnchor` is refused at Tier A and Tier B, and that is deliberate.** The CLI now +reports honestly as the host rather than as `unknown`, and is still refused — which is +the intended property, not a gap. The rule does not depend on any past defect: +`"core"` and `"capability_module"` hold **one token value under two keys**, so nothing +presenting it is distinguishable from anything else presenting it. **A tier admitting +`HostAnchor` admits an unbounded set, not a trusted party.** It must not be relaxed on +the reasoning that "the host is trusted anyway". + +### Impersonation of the approver: closed upstream + +An earlier revision of this document recorded a live exposure: any loaded module could +reach Tier A by naming `signer_ui`, because `capability_module.requestModule` took the +requesting identity as a **plain argument** and checked only that the name was loaded. +**That is fixed** (`capability_module_impl.cpp:77-92`): + +```cpp +const logos::LogosCaller caller = logos::currentCaller(); +std::string callerName; +if (caller.isHost()) callerName = "core"; +else if (caller.isModule() && !caller.name.empty()) callerName = caller.name; +else { /* unnamed / unknown / derived / operator -> REFUSE */ return {}; } +if (!fromModuleName.empty() && fromModuleName != callerName) { + warn("ignoring leftover fromModuleName='%s' (token-bound caller is '%s')", …); +} ``` ---- +Three properties matter here, and all three hold: -### `timed_unlock(address: String, password: String, seconds: i64) -> bool` +* The identity comes from the platform's caller document, **not** the argument. The + argument is dead ABI — it survives only to be warned about. +* It **fails closed** when there is no named caller. A fallback to the argument on an + unnameable dispatch would have re-opened the hole entirely; there is none. +* The **binding** uses the derived name: the token is delivered as + `informModuleTokenTo(…, /*moduleName=*/callerName, …)`. `fromModuleName` appears + nowhere in the binding path. -Like `unlock`, but the in-memory signer carries a **TTL** of `seconds`. After the -deadline elapses the signer is **lazily evicted** the next time it is accessed -(`live_signer` checks `Instant::now() >= expires_at` on every signing/`is_unlocked` -call and removes the entry). Negative `seconds` is clamped to `0` (immediate -expiry). Returns `true` on successful decrypt+insert. +**Why it is closed in principle, not merely in this code path.** Ignoring the argument +would be worth little if an attacker could instead poison the map the name is recovered +from. It cannot. To be named `Y`, attacker `X` would have to make the naming scan match +`X`'s presented token against key `Y` — and that caller-keyed map has exactly two +writers: the host, which files each module's root token under its real name +(`module_manager.cpp:226`, whose comment states the purpose: *"capability stores +(name, token) so authorize can name the caller from the presented token rather than from +a self-asserted fromModuleName"*), and `capability_module` itself. Writing to it via +`informModuleToken` is gated on the **target's own credential** +(`module_proxy.cpp:413-421`), which no API hands out — `requestModule` returns a fresh +UUID, never the target's token. And the anchor role labels are masked out of the naming +fold (`module_proxy.cpp:332-337`, `out.fold.offer(match & ~isAnchor, …)`), so a caller +filed under `core`/`capability_module` resolves to `Unknown` and is refused rather than +being spelled as a module. -* **`address`** — account to unlock. -* **`password`** — its vault password. -* **`seconds`** — auto-relock TTL in seconds (`>= 0`). +The fail-closed behaviour is pinned by tests, and was contested during development: +`requestModule_rejects_unnamed_caller` asserts an empty result even when +`fromModuleName` names a seeded, loaded module, and +`requestModule_denies_spoofed_fromModuleName` covers the spoof directly. A commit that +added *"fall back when mocks/old hosts omit the caller"* was **reverted** in favour of +simulating identity in the test harness — which is the right call, and exactly the +fallback that would have re-opened this. -```bash -logoscore call keystore_module timed_unlock
pw 300 -# → true (auto-relocks ~5 min later, on next access) +Confirmed live — the mechanism firing, verbatim from the daemon log: + +``` +[capability_module] ignoring leftover fromModuleName='signer_ui' (token-bound caller is 'core') ``` ---- +That is a `logosctl` request to be minted as `signer_ui`, overridden to the CLI's real +token-bound identity. Measured in two composing halves: a module's `currentCaller()` is +its own name (row 3 above), and `requestModule` binds to `currentCaller()`, not to its +argument (source, all paths). A forged argument therefore cannot change the binding. -### `lock(address: String) -> bool` +### The access policy is now a real control — with one residual gap -Remove the in-memory signer for `address`, re-locking it. Returns `true` if a -signer was present and removed, `false` if it was already locked or the address is -invalid. The on-disk vault is untouched. +A previous revision of this document said `--access-policy enforce` was **not** a +mitigation, because the policy arm filtered the same self-asserted name. **That is no +longer true** and the correction matters: the allowlist is now consulted against the +derived `callerName`: -```bash -logoscore call keystore_module lock
-# → true +```cpp +auto it = m_restrictions.find(moduleName); +if (it != m_restrictions.end() && it->second.count(callerName) == 0) { /* deny */ } ``` ---- +The residual gap is documented in the code and is a rollout decision, not an oversight: -### `is_unlocked(address: String) -> bool` +> `TODO(access-policy): still fail-OPEN — a target with no registered restriction is +> unrestricted. Intentional for back-compat during rollout.` -Return `true` iff `address` currently has a live (non-expired) in-memory signer. -This call **also evicts** an expired timed-unlock signer as a side effect (it goes -through `live_signer`). Invalid address → `false`. +So a policy that is *registered* is now enforced against a real identity; a target with +**no** policy remains reachable by any named module. -```bash -logoscore call keystore_module is_unlocked
-# → true | false +**Why keystore does not simply register one.** `registerRestriction` is **per-target, +not per-method**. This module deliberately needs a *wide* Tier B (any named module may +*request*) and a *narrow* Tier A (exactly one may *approve*). A blanket allowlist +naming only `signer_ui` would lock out every legitimate requester — +`wallet_backend_module` among them. So the tier gate inside this module stays the +mechanism that separates asking from approving, and the access policy is a coarse +complement for deployments that want to bound the requester set. Operators who want +both should register the requester set as the restriction and leave the approver +distinction to the tier gate. + +### What identity still does not guarantee + +Identity being live changes what the gate can *do*; it does not make the name a +cryptographic fact. Five residuals, each of which bounds a claim this document makes. + +**1. The name is *token-bound*, not verified.** `logos_caller_scope.h` says so in its +own words — token-bound is *"the strongest honest word … chosen over 'verified' or +'authenticated' deliberately"*, because the name is the key under which **this module** +recorded the token the caller presented. It is exactly as strong as that recording. It +is not a signature, and nothing here should be read as authentication. + +**2. Only the QtRO path yields a caller at all.** Identity is resolved from the +meta-object dispatch. `plain` tcp / tcp_ssl operator tokens carry no name (the validator +returns a bool), and in-process paths do not go through the proxy. Because +`requestModule` now **fails closed** on an unnamed dispatch, a deployment on those +transports does not degrade to a weaker check — it stops working. That is the right +direction for a signer, but it is a deployment constraint, not a detail: **this module +requires the QtRO transport to be usable at all.** + +**3. Same-process impersonation is not closed, and this is the one that matters here.** +`TokenManager::isolateIdentity` is idempotent and `LogosAPI::forIdentity` hands back the +*same* store for an already-isolated name, so native code running inside the host +process can obtain another plugin's identity. In Basecamp every `ui_qml` plugin — this +module's approver among them — lives in **one process**. So the Tier A boundary is a +**code-authority boundary enforced by a name, not a process boundary**: it holds against +another *module* (which is a separate process), and it does not hold against hostile +**native code already inside the shell**. An attacker who has that has already won more +than this gate was defending. + +**4. A lying `fromModuleName` is warned about, not refused.** `capability_module` logs +*"ignoring leftover fromModuleName=…"* and proceeds. The binding is unaffected — that is +what matters — but there is no counter and no event, so an operator cannot observe +attempts programmatically. Worth a metric upstream. + +**5. `registerRestriction` was not converted.** It still authenticates by a +self-presented `authToken` compared against the trust-root tokens rather than by +`currentCaller()`. That is a *secret*, not a name, so it is a different class from the +hole that was closed — but it means the policy-writing path did not move with the +policy-checking path. + +One quieter failure mode is worth knowing because it is *not* the one that was fixed: +the `Q_INVOKABLE currentCallerJson` **declaration** is unguarded while its **body** is +`#if`-guarded on protocol ≥ 0.6. Under a sub-0.6 protocol the old symptom would not +reappear as `No such method` — `invokeMethod` would succeed and return an **empty +string**, which collapses to `unknown`. The loud failure is the one that has been fixed; +a silent one remains reachable on an old protocol. And the check that would catch it +(`caller-invokable`, the only one that actually calls `invokeMethod` by name against a +real `LogosAPI`) is exposed as a flake check but is **not wired into CI**, so the +property is not continuously verified. + +### What the gate does and does not guarantee + +With identity live and impersonation closed, Tier A means what it is meant to mean: the +caller *is* the configured approver package. Two limits remain worth stating plainly. + +**`module:signer_ui` names a plugin package, not a human.** A `ui_qml` plugin's QML view +and its `ui-host` backend are one identity by design, and this module cannot distinguish +them — it must not pretend to. What the entry asserts is that *the operator designated +this package as the code permitted to approve*. It does not assert that a human saw +anything, and no token check can make it. (This design routes **all** keystore calls +through the backend, so it is the backend's identity that is checked.) + +**Defence in depth is not resting on identity alone**, and this was tested rather than +asserted. While the impersonation hole was open, the design degraded to *intent +disclosure and denial of service* — never to unauthorised signing — because the +properties that stop signing do not depend on who the caller is: + +* `approve()` requires the **vault password**, and no signer is cached; impersonating + the approver does not produce one. +* The single-`Rendered` rule plus the echoed `bundle_id` mean demoting the human's + render makes their `approve()` **fail**, not misapply. +* `fetch_result`/`ack_result` authorise on the per-request **receipt**, so requesters + cannot collect each other's signatures even if they share a name. + +Those choices are why a premise failure cost confidentiality and availability rather +than integrity, and they should survive any future change to how identity is delivered. + +### Lifecycle + +``` +request_approval ──▶ Offered ──acknowledge──▶ Rendered ──approve──▶ Settled(Approved) + │ │ │ └────▶ Settled(Rejected) + │ └──(no ack in 3000 ms)──▶ Settled(ExpiredNoAck) + └──cancel_approval─────────────────────────────────────────▶ Settled(Cancelled) ``` ---- +* **`handle` vs `receipt`.** `request_approval` returns both, and the **receipt is + returned exactly once**. The handle is what the event plane announces (it carries + no token, so anything richer would publish the intent to every subscriber); the + **receipt is what authorises collecting the result**. A module that learns a + handle from the event plane still cannot fetch someone else's signatures. +* **At most one record is `Rendered`.** `acknowledge` demotes any other rendered + request, so exactly one thing can be on screen — this is what binds the text the + human read to the `approve` call that follows. +* **The 3000 ms window is on the *event* path only.** It bounds how long the + approver has to acknowledge receipt, not how long the human has to decide. Once + `Rendered`, there is **no timeout on the human**. +* **Settled records are retained** for 120 s so a requester polling `approval_status` + learns *why* a request ended (`expired_no_ack`, `rejected`, `cancelled`) rather + than getting `not_found`. +* **Results are idempotent until `ack_result`**, so a dropped reply does not cost + the human a second password entry. -### `sign_transaction(address: String, unsigned_tx_json: String, chain_id: i64) -> String` +### `request_approval(intent_json: String) -> String` -Sign an unsigned transaction and return the **raw, broadcast-ready** signed -transaction as an EIP-2718-encoded hex envelope. The account **must be unlocked** -(via `unlock`/`timed_unlock`), otherwise the call errors with `account is locked`. +**Tier B.** Returns immediately — it does **not** block on the human. -* **`address`** — signing account (must be unlocked). -* **`unsigned_tx_json`** — JSON object describing the unsigned tx (see - [Unsigned-transaction JSON](#unsigned-transaction-json-unsignedtx)). -* **`chain_id`** — chain id for replay protection (EIP-155 for legacy, the - `chainId` field for EIP-1559). +The intent is `{ address, purpose, legs: [...] }`, where each leg is one of: -The `fee_mode` field selects the envelope type: - -* `"eip1559"` (default / anything that isn't `"legacy"`) → `TxEip1559`, returns a - typed `0x02…` envelope. Uses `max_fee_per_gas` / `max_priority_fee_per_gas`; - `access_list` is empty. -* `"legacy"` (case-insensitive) → `TxLegacy` with EIP-155 `chain_id`, returns an - RLP legacy envelope. Uses `gas_price`. - -**Success:** `{ "ok": true, "raw": "0x02…" }` (EIP-1559) or `{ "ok": true, "raw": "0x…" }` (legacy) -**Error:** `{ "ok": false, "error": "account is locked: …" }` or -`{ "ok": false, "error": "invalid parameters: tx json: …" }` - -```bash -logoscore call keystore_module sign_transaction
@tx.json 1 -# → {"ok":true,"raw":"0x02f86c01808459682f00…"} +```jsonc +{ "kind": "tx", "chain_id": 1, "tx": { …UnsignedTx… } } +{ "kind": "message", "text": "…" } +{ "kind": "digest", "digest": "0x…32 bytes…", "purpose": "…" } ``` ---- - -### `sign_message(address: String, message: String) -> String` - -Produce an **EIP-191 `personal_sign`** signature over the UTF-8 bytes of -`message`. The account **must be unlocked**. Returns a 65-byte signature as -`0x…` hex (`r ‖ s ‖ v`). - -* **`address`** — signing account (must be unlocked). -* **`message`** — the message to sign (signed as raw bytes via - `sign_message_sync`, which applies the standard `"\x19Ethereum Signed Message:\n"` - prefix). - -**Success:** `{ "ok": true, "signature": "0x…(130 hex chars)…" }` -**Error:** `{ "ok": false, "error": "account is locked: …" }` +A **bundle** of several legs is **one human decision**: all legs are signed, or +none are. `purpose` is requester-supplied and is always rendered as *claimed by the +requester*, never as fact. ```bash -logoscore call keystore_module sign_message
hello-logos -# → {"ok":true,"signature":"0x…"} +# → {"ok":true,"handle":"ksh_…","receipt":"ksc_…","state":"offered"} ``` ---- +Limits: at most 4 pending per requester, 16 total, 64 KiB of intent. + +### `approval_status(handle: String, receipt: String) -> String` + +**Tier B.** Bare state for the requester — never the intent, never the results. +`{ ok, state, reason? }`. + +### `fetch_result(handle: String, receipt: String) -> String` + +**Tier B.** Collect the signatures: `{ ok, signed: [ … ] }`, one entry per leg in +request order. Idempotent until `ack_result`. + +### `ack_result(handle: String, receipt: String) -> bool` + +**Tier B.** The requester has the signatures; erase them. + +### `cancel_approval(handle: String, receipt: String) -> bool` + +**Tier B.** The requester gave up. + +### `pending() -> String` + +**Tier A.** Queue **summaries** — never leg detail. `{ ok, pending: [...] }`. + +### `acknowledge(handle: String) -> String` + +**Tier A.** Claim a request for display: +`{ ok, handle, bundle_id, requester, render_lines }`. + +`render_lines` are authored **by this module** from the parsed intent and **must be +displayed verbatim** — not reformatted, elided, truncated or re-ordered. keystore is +the only party that parsed the intent and therefore the only one that can tell +requester-supplied text from its own; it escapes control characters, bidi controls +and zero-width characters before they enter a line. + +The full calldata is always shown in full and **never elided**. A `digest` leg +renders an explicit admission that the signer cannot show what it authorises. + +### `approve(handle: String, bundle_id: String, password: String) -> String` + +**Tier A.** The human said yes. `bundle_id` must be the value that was displayed; +a mismatch is refused. The intent is **re-parsed and the commitment re-derived +inside this call** before anything is signed, so what is signed is what was +committed to. + +One key derivation, every leg signed, then wiped → `{ ok, signed_count: n }`. + +**The approver never receives what it authorised.** `approve()` answers with a +**count**, not the signatures. Only the requester can collect those, and only +with the receipt it was handed at request time — so a compromised approver can +cause a signature to exist but cannot walk away with it. That is also why this +field is named differently from `fetch_result`'s `signed` array: the two must +never be mistaken for one another. + +**At most once per handle:** a second `approve` for a handle that has left +`Rendered` returns the recorded outcome and never re-signs. Because a scrypt +derivation runs inside the call, callers must use an async entry point with a +timeout comfortably above worst-case KDF — a dispatched call that times out still +executes here. + +The `bundle_id` is SHA-256 over this module's **own canonical re-encoding** of the +parsed intent — deliberately **not** keccak256, so a bundle id can never be +mistaken for a signable Ethereum digest. + +### `reject(handle: String) -> bool` + +**Tier A.** The human said no. + +### `caller_identity() -> String` + +Ungated observability: `{ ok, kind, identity, approver }` where `kind` is one of +`unknown` | `host` | `module` | `derived` | `operator`. ### Events @@ -509,7 +766,7 @@ std-typed (`i64`). | Field | Value | Notes | |-------|-------|-------| -| `name` | `keystore_module` | Module id used by `logoscore`/`lgpm` | +| `name` | `keystore_module` | Module id used by `logosctl`/`lgpm` | | `version` | `1.0.0` | | | `type` | `core` | Core (non-UI) module | | `interface` | `cdylib` | Rust-first cdylib module | @@ -548,22 +805,26 @@ directory. ### In-memory state ```rust -struct Unlocked { signer: PrivateKeySigner, expires_at: Option } - pub struct Keystore { - dir: PathBuf, - unlocked: HashMap, // only while unlocked + dir: PathBuf, // that is the whole of it — no signer cache } ``` -A decrypted key exists **only** inside an `Unlocked.signer` and only between -`unlock`/`timed_unlock` and `lock` (or TTL eviction). `expires_at = None` means -"until explicitly locked"; `Some(instant)` is the timed-unlock deadline, checked -lazily by `live_signer`. +**There is no unlocked-signer cache, by construction.** A decrypted key exists only +as a local inside `approve()`: derived from the vault password, used to sign every +leg of the bundle, then zeroized (`Zeroizing` wraps the password, the derived key +and the signer's key bytes) before the call returns. Nothing outside that call +frame can reach a key, so there is no TTL to get wrong and no "unlocked forever" +state to leak — the defect this design replaced was exactly an `unlock` that passed +`ttl: None` and therefore never expired. + +The only cross-call state is the approval ledger (`approval.rs`), which holds +intents, render text and — briefly — signed *outputs*, never keys. ### Unsigned-transaction JSON (`UnsignedTx`) -The `unsigned_tx_json` argument to `sign_transaction` deserializes into: +The `tx` object of a `tx` leg (and the transaction the human is shown) +deserializes into: | Field | Type | Default | Meaning | |-------|------|---------|---------| @@ -623,13 +884,13 @@ compile under the builder's `rustc 1.89`. The Logos SDK `logos_module` feature, so `cargo test --no-default-features` builds the crypto core in isolation without the SDK or generated scaffold. -### Run / drive via `logoscore` +### Run / drive via `logosctl` -The module is loaded into a headless `logoscore` daemon and called over IPC. +The module is loaded into a headless `logosctl` daemon and called over IPC. End-to-end, as the doc-test does it: ```bash -# 1. Build logoscore + lgpm from their flakes +# 1. Build logosctl + lgpm from their flakes nix build 'github:logos-co/logos-logoscore-cli#cli' --out-link ./logos nix build 'github:logos-co/logos-package-manager#cli' -o lgpm @@ -639,20 +900,21 @@ mkdir -p modules && cp -RL ./logos/modules/. ./modules/ # bundled capability_m ./lgpm/bin/lgpm --modules-dir ./modules --allow-unsigned install --file keystore-lgx/*.lgx # 3. Start the daemon, load, and drive -logoscore -D -m ./modules > logs.txt & +logosctl --config-dir . daemon start --detach sleep 3 -logoscore load-module keystore_module -logoscore module-info keystore_module # lists sign_transaction, sign_message, … -logoscore call keystore_module create_mnemonic 12 -logoscore call keystore_module import_private_key pw # → {address} -logoscore call keystore_module list_accounts -logoscore call keystore_module unlock
pw -logoscore call keystore_module sign_message
hello-logos -logoscore call keystore_module sign_transaction
@tx.json 1 -logoscore stop +logosctl module load keystore_module +logosctl module show keystore_module # note: no unlock/sign_* methods exist +logosctl call keystore_module create_mnemonic 12 +logosctl call keystore_module import_private_key pw # → {address} +logosctl call keystore_module list_accounts +logosctl call keystore_module caller_identity # → {"kind":"host", ...} + +# Tier A and Tier B are UNREACHABLE from the CLI, by design: +logosctl call keystore_module pending # → {"ok":false,"error":"not authorized"} +logosctl daemon stop ``` -The bundled `capability_module` (shipped with `logoscore`) handles the load-time +The bundled `capability_module` (shipped with `logosctl`) handles the load-time auth handshake, which is why it is seeded into `./modules` before installing this module. @@ -669,9 +931,22 @@ Foundry's canonical test mnemonic / account 0 * `create_mnemonic 12` output contains `phrase`; * `import_private_key` returns the expected address (`f39Fd6e51aad…`), proving the key stayed inside while only the address came out; -* `unlock` returns `true`; `sign_message` returns a `0x…` `signature`; -* `sign_transaction … @tx.json 1` returns a `raw` value starting with `0x02` - (a typed EIP-1559 envelope). +* `list_accounts` then contains that address; +* `caller_identity` reports `"kind":"host"` — the CLI is the host anchor, named + honestly; +* every Tier A / Tier B method refuses the CLI with the identical + `{"ok":false,"error":"not authorized"}` — asserted rather than assumed, and now + for the *right* reason: the caller is named and is not admitted, rather than + unnameable. + +Signing cannot be exercised from `logosctl` — that is the point, and it is the +property to guard. The **Tier B** half of the positive path is now reachable +headlessly, since a named module calling `request_approval` is admitted and gets a +handle and receipt (measured). The **Tier A** half still needs a real approver +plugin, so the full request → acknowledge → approve → broadcast proof remains a +**Basecamp** doctest — not because `logoscore` cannot name callers (it can), but +because `approve()` requires a human at a rendered surface, and `ui-host` is a +`QCoreApplication` that cannot face one. The CI workflow resolves the commit under test and passes `--release-for logos-evm-keystore-module=` so the spec's `{release}` @@ -687,11 +962,42 @@ GitHub Pages. the Foundry mnemonic matches known addresses. * `private_key_import_matches_address` — imported PK yields the expected address. * `create_mnemonic_lengths` — 12/24 words succeed, 13 fails. -* `vault_roundtrip_and_listing` — import → `has_address`/`list_accounts`; wrong - password fails to unlock, correct password unlocks, then `lock` re-locks. +* `vault_roundtrip_and_listing` — import → `has_address`/`list_accounts`. +* `there_is_no_unlocked_state_to_reuse` — the structural guarantee: nothing + survives a signing call that a later caller could reuse. * `sign_message_recovers_signer` — EIP-191 signature recovers to the signer. -* `locked_account_cannot_sign` — signing a locked account yields - `KeystoreError::Locked`. +* `a_wrong_password_cannot_sign` — the password is the only key to the vault. +* `nonce_that_overflows_u64_is_rejected_not_truncated`, + `unknown_tx_fields_are_rejected`, + `absent_to_is_refused_rather_than_deploying_a_contract`, + `an_access_list_is_refused_rather_than_silently_dropped`, + `fee_mode_is_a_closed_set_and_is_trimmed` — the parser refuses what it cannot + faithfully render, instead of silently dropping or truncating it. +* `sign_message_refuses_text_that_renders_differently_than_it_signs` — no bidi, + control or zero-width characters may enter a rendered line. +* `hostile_kdf_params_are_rejected_before_any_derivation`, + `the_vault_directory_and_files_are_not_group_or_world_readable`, + `importing_a_vault_leaves_no_temp_copy_behind`. + +**Approval state machine** (`rust-lib/src/approval.rs`, pure and offline): + +* `approve_refuses_a_handle_that_is_not_the_one_being_rendered` — the + at-most-one-`Rendered` rule, which is what binds what the human read to what + gets signed. +* `approve_requires_the_bundle_id_that_was_displayed`. +* `the_commitment_covers_the_parsed_value_not_the_requesters_bytes` — a requester + cannot get one thing rendered and another signed. +* `the_receipt_not_the_handle_is_what_authorises_collection` — knowing a handle + from the event plane does not let you collect someone else's signatures. +* `a_wrong_password_neither_signs_nor_settles_the_record`. +* `results_are_idempotent_until_acked_then_erased`. +* `a_bundle_is_one_decision_over_several_legs`. +* `an_unacknowledged_request_expires_and_says_why` — a requester learns the + reason, rather than seeing the record vanish. +* `the_render_shows_full_calldata_and_flags_contract_creation`, + `an_opaque_digest_is_rendered_as_opaque`. +* `a_requester_cannot_flood_the_queue`, + `an_oversize_or_empty_intent_is_refused_before_parsing`. * `sign_eip1559_recovers_signer` / `sign_legacy_recovers_signer` — decode the raw signed tx (EIP-2718) and recover the original signer; the EIP-1559 envelope starts with `0x02`. @@ -722,19 +1028,40 @@ This module is the wallet's **secret-holding boundary**. Its security properties scrypt-encrypted JSON, one file per account, in the module's isolated `instance_persistence_path`. -4. **In memory: bounded by unlock.** A decrypted key exists in RAM only inside an - `Unlocked.signer`, only between `unlock`/`timed_unlock` and `lock`/TTL - eviction. Signing requires a live signer — a locked account returns - `account is locked`. `timed_unlock` provides automatic re-locking - (lazily enforced on next access). +4. **In memory: bounded by one call.** A decrypted key exists in RAM only as a + local inside `approve()` — derived from the vault password, used for every leg + of the bundle, then zeroized before the call returns. There is no signer cache, + so there is no TTL to misconfigure and no "unlocked" state another caller could + ride. The defect this replaced was precisely an `unlock` that passed + `ttl: None` unconditionally while eviction only ever fired for `Some`, making + every unlock a permanent, unattributable signing oracle. -5. **Password-gated destructive ops.** `delete_account` and `export_keystore_json` +5. **A signature requires a human.** No method signs outside `approve()`, and + `approve()` is reachable only by the configured approver. `render == sign` is + enforced rather than assumed: the intent is re-parsed and its commitment + re-derived *inside* `approve()` and compared against the `bundle_id` that was + displayed, so a requester cannot get one thing rendered and another signed. + The `bundle_id` is SHA-256 over the module's own canonical re-encoding — + deliberately **not** keccak256, so it can never be mistaken for a signable + Ethereum digest. + +6. **Password-gated destructive ops.** `delete_account` and `export_keystore_json` both require the correct vault password (they decrypt to verify) before acting, so neither can be abused by a caller that doesn't already hold the password. + **Known gap, tracked:** `delete_account` is an *uncounted password oracle* that + destroys the vault on a correct guess. Rate limiting on vault-decrypting methods + is a named follow-up, deliberately out of scope for this landing. -6. **Replay protection.** `sign_transaction` always binds the `chain_id` - (EIP-155 for legacy, the `chainId` field for EIP-1559), so a signed tx cannot - be replayed on another chain. +7. **Replay protection.** Every signed transaction binds the `chain_id` (EIP-155 + for legacy, the `chainId` field for EIP-1559), so a signed tx cannot be replayed + on another chain. The chain is also shown to the human on its own render line. + +8. **The gate fails closed, and that was load-bearing while it had to be.** Where a + caller cannot be named, Tiers A and B refuse *everyone* — the failure mode is + "nothing can be signed", never "anyone can sign". Identity is live now, so the + gate admits legitimate callers rather than refusing all of them; the fail-closed + direction stays because it is what makes a future regression in the identity path + an availability problem instead of a signing one. --- @@ -743,9 +1070,15 @@ This module is the wallet's **secret-holding boundary**. Its security properties The keystore declares **no `concurrency` field** in `metadata.json`, so it runs in the framework's **default single-handler dispatch**: the runtime processes one call at a time. This is appropriate here because the operations mutate shared -state (`Keystore.unlocked` map, vault files) and are fast (local scrypt + signing, -no network latency), so there is no benefit to concurrent dispatch and serial -execution avoids data races on the unlocked-signer map without extra locking. +state (the approval ledger, vault files) and are fast (local scrypt + signing, no +network latency), so there is no benefit to concurrent dispatch and serial +execution avoids data races on the ledger without extra locking. + +Serial dispatch is also what lets the lease be a **lazy sweep** rather than a +reaper thread: a stale `Rendered` record is demoted on the next Tier A/Tier B +call — precisely the call it would otherwise block. That is why +`concurrency: "multi"` is *not* needed here, despite `approve()` running a +deliberately slow scrypt derivation inside the call. This contrasts with the wallet's `concurrency: "multi"` modules (`eth_rpc_module`, `uniswap_module`), which fan out network-bound RPC calls diff --git a/doctests/keystore-module-runtime.test.yaml b/doctests/keystore-module-runtime.test.yaml index a46ab01..7c5dfee 100644 --- a/doctests/keystore-module-runtime.test.yaml +++ b/doctests/keystore-module-runtime.test.yaml @@ -17,15 +17,16 @@ intro: | 2. Build this module's installable `.lgx` from its `#lgx` output. 3. Install it into a `./modules` directory with `lgpm`. 4. Start a `logoscore` daemon, load `keystore_module`, and drive it: generate a - mnemonic, import a known private key, unlock the account, and produce a real - EIP-191 signature and a signed EIP-1559 transaction. + mnemonic, import a known private key, and then **demonstrate that the CLI + cannot make it sign anything** — signing requires a human approving through + the dedicated signer UI. what_you_build: "This `keystore_module`, packaged as `.lgx`, installed with `lgpm`, and called through a `logoscore` daemon." what_you_learn: - How a Rust (rust-first cdylib) Logos module is packaged as an installable `.lgx` - How to install it with `lgpm` and load it into a `logoscore` daemon - - How to import a key, unlock it, and sign a message and a transaction over IPC + - Why a signature cannot be obtained from the CLI, and what refuses it - How the keystore keeps private keys inside the module (only addresses and signed payloads come out) prerequisites: @@ -51,6 +52,17 @@ sections: - title: "Build logoscore" run: "nix build 'github:logos-co/logos-logoscore-cli#cli' --out-link ./logos" check_file: "logos/bin/logoscore" + - title: "Prove the CLI actually runs" + run: "./logos/bin/logoscore --version" + code_block: "logoscore --version" + # A positive control, not decoration. `check_file` did NOT fail when this + # path was wrong: the step reported PASS while every later command died + # with exit 127 ("No such file or directory"). Actually RUNNING the + # binary is the only assertion here that cannot pass on a tree where it + # is missing. + expect_contains: + - "logoscore version" + - title: "Build lgpm" run: "nix build 'github:logos-co/logos-package-manager#cli' -o lgpm" check_file: "lgpm/bin/lgpm" @@ -93,19 +105,6 @@ sections: auto-types a `0x…` argument as a number, so addresses are passed as **bare hex** — the keystore accepts either form. steps: - - title: "Write an unsigned transaction" - file: - path: tx.json - content: | - { - "to": "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", - "value": "0xde0b6b3a7640000", - "nonce": "0x0", - "gas_limit": "0x5208", - "max_fee_per_gas": "0x77359400", - "max_priority_fee_per_gas": "0x3b9aca00", - "fee_mode": "eip1559" - } - title: "Start the daemon" run: "sh -c './logos/bin/logoscore -D -m ./modules > logs.txt 2>&1 &'" code_block: "logoscore -D -m ./modules > logs.txt &" @@ -119,8 +118,8 @@ sections: run: "./logos/bin/logoscore module-info keystore_module" code_block: "logoscore module-info keystore_module" expect_contains: - - "sign_transaction" - - "sign_message" + - "request_approval" + - "caller_identity" - title: "Generate a BIP-39 mnemonic" run: "./logos/bin/logoscore call keystore_module create_mnemonic 12" code_block: "logoscore call keystore_module create_mnemonic 12" @@ -139,27 +138,35 @@ sections: code_block: "logoscore call keystore_module list_accounts" expect_contains: - "f39Fd6e51aad88F6F4ce6aB8827279cffFb92266" - - title: "Unlock the account" - run: "./logos/bin/logoscore call keystore_module unlock f39fd6e51aad88f6f4ce6ab8827279cfffb92266 pw" - code_block: "logoscore call keystore_module unlock
pw" - expect_contains: - - "true" - - title: "Sign a message (EIP-191)" - run: "./logos/bin/logoscore call keystore_module sign_message f39fd6e51aad88f6f4ce6ab8827279cfffb92266 hello-logos" - code_block: "logoscore call keystore_module sign_message
hello-logos" - expect_contains: - - "signature" - - "0x" - - title: "Sign an EIP-1559 transaction" + - title: "Ask the module who it thinks is calling" text: | - `sign_transaction` signs the unsigned tx in `tx.json` for chain 1 and - returns the raw, broadcast-ready signed transaction (a `0x02…` typed - envelope). - run: "./logos/bin/logoscore call keystore_module sign_transaction f39fd6e51aad88f6f4ce6ab8827279cfffb92266 @tx.json 1" - code_block: "logoscore call keystore_module sign_transaction
@tx.json 1" + The CLI presents a host bootstrap anchor, so the keystore names it + `host` — honestly, and without inventing a module name. `core` and + `capability_module` share one token value under two keys, so an anchor + cannot be attributed to any particular party. That is the whole reason + the next two steps refuse: the caller is named, and is not admitted. + run: "./logos/bin/logoscore call keystore_module caller_identity" + code_block: "logoscore call keystore_module caller_identity" expect_contains: - - "raw" - - "0x02" + - "host" + - title: "Request a signature from the CLI — refused" + text: | + `request_approval` is Tier B: any **named module** may ask for a + signature. The CLI is not a named module, so it cannot even ask. + run: "./logos/bin/logoscore call keystore_module request_approval '{\"address\":\"0x0000000000000000000000000000000000000001\",\"purpose\":\"doc-test\",\"legs\":[{\"kind\":\"message\",\"text\":\"hello-logos\"}]}'" + code_block: "logoscore call keystore_module request_approval '{...intent...}'" + expect_contains: + - "not authorized" + - title: "Approve a signature from the CLI — refused" + text: | + `approve` is Tier A: **only** the configured approver (`signer_ui`) may + approve, and it must be a human doing it. There is no flag, token or + argument that makes this succeed from a shell — which is the property + this module exists to provide. + run: "./logos/bin/logoscore call keystore_module pending" + code_block: "logoscore call keystore_module pending" + expect_contains: + - "not authorized" - title: "Stop the daemon" run: "./logos/bin/logoscore stop" code_block: "logoscore stop" diff --git a/rust-lib/Cargo.lock b/rust-lib/Cargo.lock index f954522..081b3f6 100644 --- a/rust-lib/Cargo.lock +++ b/rust-lib/Cargo.lock @@ -1868,8 +1868,10 @@ dependencies = [ "rand 0.8.6", "serde", "serde_json", + "sha2", "tempfile", "thiserror 2.0.18", + "zeroize", ] [[package]] @@ -1919,7 +1921,7 @@ checksum = "953f07c43838f8e6f9758cab68bf5bed85465e7587ebe0b823f1bcd81978ad3a" [[package]] name = "logos-rust-sdk" -version = "0.2.0" +version = "0.3.0" dependencies = [ "serde", "serde_json", diff --git a/rust-lib/Cargo.toml b/rust-lib/Cargo.toml index aee82bb..5e6b489 100644 --- a/rust-lib/Cargo.toml +++ b/rust-lib/Cargo.toml @@ -32,6 +32,12 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" thiserror = "2" hex = "0.4" +# Wipes decrypted secrets on drop. Already in the lock graph transitively; this +# only promotes it to a direct dependency. +zeroize = "1" +# SHA-256 for the approval commitment. Deliberately NOT keccak256: a bundle id +# must never be mistakable for a signable Ethereum digest. +sha2 = "0.10" # The Logos runtime SDK is staged by the builder as a sibling crate. It is # optional + enabled by the default `logos_module` feature so `cargo test diff --git a/rust-lib/src/approval.rs b/rust-lib/src/approval.rs new file mode 100644 index 0000000..4f6908b --- /dev/null +++ b/rust-lib/src/approval.rs @@ -0,0 +1,775 @@ +//! The approval record: the state machine between "a module asked for a +//! signature" and "a human approved it". +//! +//! Pure and offline — no Logos dependency — so the whole machine is unit +//! testable with `cargo test --no-default-features`. The caller-identity gate +//! and the JSON envelope live in `glue.rs`; everything about *what is signed* +//! and *what a human is shown* lives here. +//! +//! Two invariants shape the design and are asserted by the tests: +//! +//! 1. **One parse.** The intent is parsed ONCE into typed legs. The render the +//! human reads and the bytes that get signed are produced from that same +//! parsed value, and `approve` re-derives the commitment before signing, so +//! the two cannot drift apart. +//! 2. **At most one record is `Rendered`.** Acknowledging a second record +//! demotes the first. Without this, an asynchronous UI update between "the +//! human read handle A" and "the human clicked Approve" could submit +//! handle B — every other invariant intact, and a signature over something +//! nobody saw. + +use std::time::{Duration, Instant}; + +use serde::Deserialize; +use sha2::{Digest as _, Sha256}; +use zeroize::Zeroizing; + +use crate::keystore::{ + check_displayable, sign_digest_with, sign_message_with, sign_parsed_tx, Keystore, KeystoreError, + UnsignedTx, +}; + +type Result = std::result::Result; + +/// How long an offered record waits for an approver to acknowledge it. After +/// the ack there is deliberately NO deadline: a human is deciding. +pub const ACK_DEADLINE: Duration = Duration::from_millis(3000); + +/// Caps. A requester cannot flood the approver's queue. +pub const MAX_PENDING_PER_REQUESTER: usize = 4; +pub const MAX_PENDING_TOTAL: usize = 16; +/// Largest intent we will parse, before parsing it. +pub const MAX_INTENT_BYTES: usize = 64 * 1024; +/// How long a settled record is kept so the requester can still read its +/// outcome. Dropping it immediately turns "your approval expired" into "no such +/// handle", which a requester cannot distinguish from a bug of its own. +pub const SETTLED_RETENTION: Duration = Duration::from_secs(120); + +// ── the intent ────────────────────────────────────────────────────────────── + +/// One thing to sign. Legs are signed in order, under a single key derivation. +#[derive(Debug, Deserialize)] +#[serde(tag = "kind", deny_unknown_fields, rename_all = "snake_case")] +pub enum Leg { + /// A transaction. `tx` is the same shape the signer has always taken. + Tx { chain_id: u64, tx: UnsignedTx }, + /// EIP-191 personal_sign over printable text. + Message { text: String }, + /// A raw 32-byte digest. Opaque by construction: `purpose` is the only + /// thing that can be shown, and it is a claim by the requester. + Digest { digest: String, purpose: String }, +} + +/// What a requester submits. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Intent { + /// The account to sign with. + pub address: String, + /// A short requester-supplied label for the whole bundle, shown as a claim. + #[serde(default)] + pub purpose: String, + pub legs: Vec, +} + +// ── the record ────────────────────────────────────────────────────────────── + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum State { + /// Offered to approvers; not yet claimed. + Offered, + /// An approver has fetched the render and is showing it to a human. + Rendered, + /// Terminal. + Settled(Outcome), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Outcome { + Approved, + Rejected, + ExpiredNoAck, + Cancelled, +} + +impl Outcome { + pub fn as_str(self) -> &'static str { + match self { + Outcome::Approved => "approved", + Outcome::Rejected => "rejected", + Outcome::ExpiredNoAck => "expired_no_ack", + Outcome::Cancelled => "cancelled", + } + } +} + +struct Record { + handle: String, + receipt_hash: [u8; 32], + requester: String, + intent: Intent, + bundle_id: [u8; 32], + render_lines: Vec, + state: State, + offered_at: Instant, + settled_at: Option, + /// Signatures, once approved. Erased on ack_result. + results: Option>, +} + +/// The pending-approval store. +pub struct Approvals { + records: Vec, + /// Monotonic counter folded into handles so two records minted in the same + /// instant cannot collide. + seq: u64, + /// Overridable so the expiry path is testable without sleeping. + ack_deadline: Duration, +} + +impl Default for Approvals { + fn default() -> Self { + Self::new() + } +} + +/// What `acknowledge` hands an approver. +pub struct Rendered { + pub handle: String, + pub bundle_id: String, + pub requester: String, + pub render_lines: Vec, +} + +/// A queue summary — never leg detail. +pub struct Summary { + pub handle: String, + pub requester: String, + pub state: &'static str, + pub purpose: String, + pub leg_count: usize, + pub age_ms: u128, +} + +impl Approvals { + pub fn new() -> Self { + Self { records: Vec::new(), seq: 0, ack_deadline: ACK_DEADLINE } + } + + /// Override the ack deadline. Shortening it only ever fails closed sooner. + pub fn set_ack_deadline(&mut self, d: Duration) { + self.ack_deadline = d; + } + + /// Demote any `Rendered` record whose ack deadline has passed, and settle + /// `Offered` records that nobody claimed. Lazy: run at the head of every + /// entry point, so the call a stale record would otherwise block is exactly + /// the call that clears it. No background thread. + fn sweep(&mut self) { + let now = Instant::now(); + for r in &mut self.records { + if r.state == State::Offered && now.duration_since(r.offered_at) > self.ack_deadline { + r.state = State::Settled(Outcome::ExpiredNoAck); + } + if matches!(r.state, State::Settled(_)) && r.settled_at.is_none() { + r.settled_at = Some(now); + } + } + // A settled record is kept until its results have been collected AND + // the retention window has passed, so a requester always gets a real + // answer rather than NotFound. + self.records.retain(|r| { + let Some(at) = r.settled_at else { return true }; + r.results.is_some() || now.duration_since(at) < SETTLED_RETENTION + }); + } + + fn find(&mut self, handle: &str) -> Option<&mut Record> { + self.records.iter_mut().find(|r| r.handle == handle) + } + + /// Open an approval request. Returns `(handle, receipt)`. The receipt is + /// returned exactly once and is never stored in the clear — it is what + /// binds `fetch_result` to the requester, because the handle itself is + /// broadcast on an unauthenticated event plane. + pub fn request(&mut self, requester: &str, intent_json: &str) -> Result<(String, String)> { + self.sweep(); + + if intent_json.len() > MAX_INTENT_BYTES { + return Err(KeystoreError::InvalidParams(format!( + "intent: {} bytes exceeds the {MAX_INTENT_BYTES}-byte limit", + intent_json.len() + ))); + } + let live = |r: &&Record| !matches!(r.state, State::Settled(_)); + if self.records.iter().filter(live).count() >= MAX_PENDING_TOTAL { + return Err(KeystoreError::InvalidParams("too many pending approvals".into())); + } + if self + .records + .iter() + .filter(live) + .filter(|r| r.requester == requester) + .count() + >= MAX_PENDING_PER_REQUESTER + { + return Err(KeystoreError::InvalidParams( + "too many pending approvals for this requester".into(), + )); + } + + let intent: Intent = serde_json::from_str(intent_json) + .map_err(|e| KeystoreError::InvalidParams(format!("intent: {e}")))?; + if intent.legs.is_empty() { + return Err(KeystoreError::InvalidParams("intent: no legs".into())); + } + check_displayable(&intent.purpose, "purpose")?; + + let bundle_id = commitment(&intent)?; + let render_lines = render(&intent, &bundle_id)?; + + self.seq += 1; + let handle = format!("ksh_{}", token_hex(self.seq)); + let receipt = format!("ksc_{}", token_hex(self.seq)); + + self.records.push(Record { + handle: handle.clone(), + receipt_hash: sha256(receipt.as_bytes()), + requester: requester.to_string(), + intent, + bundle_id, + render_lines, + state: State::Offered, + offered_at: Instant::now(), + settled_at: None, + results: None, + }); + Ok((handle, receipt)) + } + + pub fn pending(&mut self) -> Vec { + self.sweep(); + let now = Instant::now(); + self.records + .iter() + .filter(|r| !matches!(r.state, State::Settled(_))) + .map(|r| Summary { + handle: r.handle.clone(), + requester: r.requester.clone(), + state: match r.state { + State::Offered => "offered", + State::Rendered => "rendered", + State::Settled(_) => "settled", + }, + purpose: r.intent.purpose.clone(), + leg_count: r.intent.legs.len(), + age_ms: now.duration_since(r.offered_at).as_millis(), + }) + .collect() + } + + /// Claim a record for display. **Demotes any other `Rendered` record**, so + /// exactly one thing can be on screen at a time. + pub fn acknowledge(&mut self, handle: &str) -> Result { + self.sweep(); + if !self.records.iter().any(|r| r.handle == handle) { + return Err(KeystoreError::NotFound(handle.to_string())); + } + for r in &mut self.records { + if r.state == State::Rendered && r.handle != handle { + r.state = State::Offered; + } + } + let r = self.find(handle).expect("checked above"); + match r.state { + State::Settled(o) => { + return Err(KeystoreError::InvalidParams(format!( + "already settled: {}", + o.as_str() + ))) + } + _ => r.state = State::Rendered, + } + Ok(Rendered { + handle: r.handle.clone(), + bundle_id: hex::encode(r.bundle_id), + requester: r.requester.clone(), + render_lines: r.render_lines.clone(), + }) + } + + /// The human said yes. Re-derives the commitment from the stored parsed + /// intent and checks it against what the approver echoed back, derives the + /// key ONCE, signs every leg in order, then wipes. + pub fn approve( + &mut self, + ks: &Keystore, + handle: &str, + bundle_id_echo: &str, + password: &str, + ) -> Result { + self.sweep(); + let r = self + .find(handle) + .ok_or_else(|| KeystoreError::NotFound(handle.to_string()))?; + + if r.state != State::Rendered { + return Err(KeystoreError::InvalidParams( + "approve: this handle was not the one being rendered".into(), + )); + } + + // Re-derive rather than trust the stored value, and check the approver + // echoed the same thing it displayed. + let fresh = commitment(&r.intent)?; + if fresh != r.bundle_id { + return Err(KeystoreError::InvalidParams("approve: commitment drift".into())); + } + if bundle_id_echo.trim().trim_start_matches("0x") != hex::encode(fresh) { + return Err(KeystoreError::InvalidParams( + "approve: bundle_id does not match the rendered request".into(), + )); + } + + // ONE derivation for the whole bundle. + let signer = ks.signer_for(&r.intent.address, password)?; + let mut out = Vec::with_capacity(r.intent.legs.len()); + for leg in &r.intent.legs { + out.push(match leg { + Leg::Tx { chain_id, tx } => sign_parsed_tx(&signer, tx, *chain_id)?, + Leg::Message { text } => sign_message_with(&signer, text)?, + Leg::Digest { digest, .. } => sign_digest_with(&signer, digest)?, + }); + } + drop(signer); + + let n = out.len(); + r.results = Some(out); + r.state = State::Settled(Outcome::Approved); + Ok(n) + } + + pub fn reject(&mut self, handle: &str) -> Result<()> { + self.sweep(); + let r = self + .find(handle) + .ok_or_else(|| KeystoreError::NotFound(handle.to_string()))?; + if let State::Settled(o) = r.state { + return Err(KeystoreError::InvalidParams(format!("already settled: {}", o.as_str()))); + } + r.state = State::Settled(Outcome::Rejected); + Ok(()) + } + + /// Bare state for the requester. Never the intent, never the results. + pub fn status(&mut self, handle: &str, receipt: &str) -> Result<(&'static str, Option<&'static str>)> { + self.sweep(); + let want = sha256(receipt.as_bytes()); + let r = self + .records + .iter() + .find(|r| r.handle == handle && ct_eq(&r.receipt_hash, &want)) + .ok_or_else(|| KeystoreError::NotFound(handle.to_string()))?; + Ok(match r.state { + State::Offered => ("offered", None), + State::Rendered => ("rendered", None), + State::Settled(o) => ("settled", Some(o.as_str())), + }) + } + + /// Collect the signatures. Idempotent until `ack_result` — a dropped reply + /// must not cost the human a second password entry. + pub fn fetch_result(&mut self, handle: &str, receipt: &str) -> Result> { + self.sweep(); + let want = sha256(receipt.as_bytes()); + let r = self + .records + .iter() + .find(|r| r.handle == handle && ct_eq(&r.receipt_hash, &want)) + .ok_or_else(|| KeystoreError::NotFound(handle.to_string()))?; + r.results + .clone() + .ok_or_else(|| KeystoreError::InvalidParams("no result for this handle".into())) + } + + /// The requester has the signatures. Erase them. + pub fn ack_result(&mut self, handle: &str, receipt: &str) -> Result<()> { + let want = sha256(receipt.as_bytes()); + let Some(r) = self + .records + .iter_mut() + .find(|r| r.handle == handle && ct_eq(&r.receipt_hash, &want)) + else { + return Err(KeystoreError::NotFound(handle.to_string())); + }; + r.results = None; + self.sweep(); + Ok(()) + } + + /// The requester gave up. + pub fn cancel(&mut self, handle: &str, receipt: &str) -> Result<()> { + let want = sha256(receipt.as_bytes()); + let Some(r) = self + .records + .iter_mut() + .find(|r| r.handle == handle && ct_eq(&r.receipt_hash, &want)) + else { + return Err(KeystoreError::NotFound(handle.to_string())); + }; + if !matches!(r.state, State::Settled(_)) { + r.state = State::Settled(Outcome::Cancelled); + } + r.results = None; + self.sweep(); + Ok(()) + } +} + +// ── commitment and render ─────────────────────────────────────────────────── + +/// SHA-256 over OUR canonical re-encoding of the parsed intent — never over the +/// requester's bytes, so reformatting, key order and whitespace cannot change +/// what the commitment covers. +fn commitment(intent: &Intent) -> Result<[u8; 32]> { + let mut h = Sha256::new(); + h.update(b"logos-keystore-approval-v1\n"); + h.update(intent.address.trim().to_lowercase().as_bytes()); + h.update(b"\n"); + for leg in &intent.legs { + match leg { + Leg::Tx { chain_id, tx } => { + h.update(b"tx\n"); + h.update(chain_id.to_string().as_bytes()); + h.update(b"\n"); + for field in [ + tx.to.clone().unwrap_or_default(), + tx.create.to_string(), + tx.value.clone(), + tx.nonce.clone(), + tx.gas_limit.clone(), + tx.data.clone(), + tx.fee_mode.clone(), + tx.max_fee_per_gas.clone(), + tx.max_priority_fee_per_gas.clone(), + tx.gas_price.clone(), + ] { + h.update(field.trim().to_lowercase().as_bytes()); + h.update(b"\x1f"); + } + } + Leg::Message { text } => { + h.update(b"message\n"); + h.update(text.as_bytes()); + } + Leg::Digest { digest, purpose } => { + h.update(b"digest\n"); + h.update(digest.trim().trim_start_matches("0x").to_lowercase().as_bytes()); + h.update(b"\x1f"); + h.update(purpose.as_bytes()); + } + } + h.update(b"\n"); + } + Ok(h.finalize().into()) +} + +/// The lines an approver shows, VERBATIM. Produced here because this is the +/// only party that has parsed the intent — an approver cannot tell +/// requester-supplied text from the signer's own. +fn render(intent: &Intent, bundle_id: &[u8; 32]) -> Result> { + let mut out = Vec::new(); + out.push(format!("Account: {}", intent.address.trim())); + if !intent.purpose.trim().is_empty() { + out.push(format!("Purpose (claimed by the requester): {}", intent.purpose.trim())); + } + out.push(format!("Commitment: {}", hex::encode(bundle_id))); + out.push(format!("{} item(s) to sign:", intent.legs.len())); + + for (i, leg) in intent.legs.iter().enumerate() { + let n = i + 1; + match leg { + Leg::Tx { chain_id, tx } => { + out.push(format!(" [{n}] Transaction on chain {chain_id}")); + match tx.to.as_deref().map(str::trim).filter(|s| !s.is_empty()) { + Some(to) => out.push(format!(" To: {to}")), + None => out.push(" ** CONTRACT CREATION — no recipient **".into()), + } + out.push(format!(" Value: {}", norm_num(&tx.value))); + out.push(format!(" Nonce: {}", norm_num(&tx.nonce))); + out.push(format!(" Gas limit: {}", norm_num(&tx.gas_limit))); + let data = tx.data.trim(); + if data.is_empty() || data == "0x" { + out.push(" Data: (none)".into()); + } else { + let d = data.trim_start_matches("0x"); + if d.len() >= 8 { + out.push(format!(" Selector: 0x{}", &d[..8])); + } + // The full calldata, never elided: a summary a human cannot + // check against the bytes is worse than no summary. + out.push(format!(" Data: 0x{d}")); + } + } + Leg::Message { text } => { + check_displayable(text, "message")?; + out.push(format!(" [{n}] Sign text message")); + out.push(format!(" Text: {text}")); + out.push(format!(" Bytes: 0x{}", hex::encode(text.as_bytes()))); + } + Leg::Digest { digest, purpose } => { + check_displayable(purpose, "purpose")?; + out.push(format!(" [{n}] Sign an OPAQUE 32-byte digest")); + out.push(format!(" Purpose (claimed by the requester): {purpose}")); + out.push(format!(" Digest: {}", digest.trim())); + out.push(" This signer cannot show you what this authorises.".into()); + } + } + } + Ok(out) +} + +/// Render a hex-or-decimal numeric field as both, so a human is not asked to +/// convert in their head. +fn norm_num(s: &str) -> String { + let t = s.trim(); + if t.is_empty() { + return "0".into(); + } + match t.strip_prefix("0x").or_else(|| t.strip_prefix("0X")) { + Some(h) => match u128::from_str_radix(h, 16) { + Ok(v) => format!("{t} ({v})"), + Err(_) => t.to_string(), + }, + None => t.to_string(), + } +} + +// ── small helpers ─────────────────────────────────────────────────────────── + +fn sha256(bytes: &[u8]) -> [u8; 32] { + let mut h = Sha256::new(); + h.update(bytes); + h.finalize().into() +} + +/// Constant-time compare for the receipt digest. +fn ct_eq(a: &[u8; 32], b: &[u8; 32]) -> bool { + let mut diff = 0u8; + for i in 0..32 { + diff |= a[i] ^ b[i]; + } + diff == 0 +} + +/// 128 bits of randomness, rendered so it can never be mistaken for a number or +/// a hex quantity by an argument parser that guesses types. +fn token_hex(seq: u64) -> String { + use rand::RngCore; + let mut b = [0u8; 16]; + rand::thread_rng().fill_bytes(&mut b); + // Fold in the sequence so two mints in one instant cannot collide even if + // the RNG is somehow degenerate. + format!("{}{:x}", hex::encode(b), seq) +} + +/// Zeroize a password the caller handed us as a plain `String`. +pub fn scrub(mut s: String) { + let z = Zeroizing::new(std::mem::take(&mut s)); + drop(z); +} + +#[cfg(test)] +mod tests { + use super::*; + + const PK: &str = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; + const ACCT0: &str = "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"; + + fn fixture() -> (tempfile::TempDir, Keystore, Approvals) { + let dir = tempfile::tempdir().unwrap(); + let ks = Keystore::new(dir.path()); + ks.import_private_key(PK, "pw").unwrap(); + (dir, ks, Approvals::new()) + } + + fn tx_intent(nonce: &str) -> String { + format!( + r#"{{"address":"{ACCT0}","purpose":"send","legs":[ + {{"kind":"tx","chain_id":1,"tx":{{ + "to":"{ACCT0}","value":"0x0","nonce":"{nonce}","gas_limit":"0x5208", + "fee_mode":"eip1559","max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}}}}]}}"# + ) + } + + #[test] + fn approve_refuses_a_handle_that_is_not_the_one_being_rendered() { + let (_d, ks, mut ap) = fixture(); + let (h1, _r1) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + let (h2, _r2) = ap.request("wallet_backend", &tx_intent("0x2")).unwrap(); + + let v1 = ap.acknowledge(&h1).unwrap(); + // A second acknowledge DEMOTES the first: an async UI update between + // render and click must not be able to submit the other handle. + let _v2 = ap.acknowledge(&h2).unwrap(); + + let err = ap.approve(&ks, &h1, &v1.bundle_id, "pw").unwrap_err(); + assert!(format!("{err}").contains("not the one being rendered"), "got {err}"); + + // The one actually on screen still approves. + let v2 = ap.acknowledge(&h2).unwrap(); + assert_eq!(ap.approve(&ks, &h2, &v2.bundle_id, "pw").unwrap(), 1); + } + + #[test] + fn approve_requires_the_bundle_id_that_was_displayed() { + let (_d, ks, mut ap) = fixture(); + let (h, _r) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + ap.acknowledge(&h).unwrap(); + let wrong = "00".repeat(32); + assert!(ap.approve(&ks, &h, &wrong, "pw").is_err()); + } + + #[test] + fn a_wrong_password_neither_signs_nor_settles_the_record() { + let (_d, ks, mut ap) = fixture(); + let (h, r) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + + assert!(ap.approve(&ks, &h, &v.bundle_id, "wrong").is_err()); + // Still rendered, so the human can simply retype. + assert_eq!(ap.status(&h, &r).unwrap().0, "rendered"); + assert_eq!(ap.approve(&ks, &h, &v.bundle_id, "pw").unwrap(), 1); + } + + #[test] + fn results_are_idempotent_until_acked_then_erased() { + let (_d, ks, mut ap) = fixture(); + let (h, r) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + ap.approve(&ks, &h, &v.bundle_id, "pw").unwrap(); + + // A dropped reply must not cost a second password entry. + let a = ap.fetch_result(&h, &r).unwrap(); + let b = ap.fetch_result(&h, &r).unwrap(); + assert_eq!(a, b); + assert!(a[0].starts_with("0x")); + + ap.ack_result(&h, &r).unwrap(); + assert!(ap.fetch_result(&h, &r).is_err()); + } + + #[test] + fn the_receipt_not_the_handle_is_what_authorises_collection() { + let (_d, ks, mut ap) = fixture(); + let (h, r) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + ap.approve(&ks, &h, &v.bundle_id, "pw").unwrap(); + // The handle is broadcast on an unauthenticated event plane, so it must + // suffice for nothing on its own. + assert!(ap.fetch_result(&h, "ksc_not_the_receipt").is_err()); + assert!(ap.fetch_result(&h, &r).is_ok()); + } + + #[test] + fn the_commitment_covers_the_parsed_value_not_the_requesters_bytes() { + let (_d, _ks, mut ap) = fixture(); + let pretty = tx_intent("0x1"); + let compact: String = pretty.split_whitespace().collect::>().join(" "); + let (h1, _) = ap.request("a", &pretty).unwrap(); + let (h2, _) = ap.request("b", &compact).unwrap(); + let v1 = ap.acknowledge(&h1).unwrap(); + let v2 = ap.acknowledge(&h2).unwrap(); + assert_eq!(v1.bundle_id, v2.bundle_id, "reformatting must not change the commitment"); + + // But a changed field must. + let (h3, _) = ap.request("c", &tx_intent("0x2")).unwrap(); + let v3 = ap.acknowledge(&h3).unwrap(); + assert_ne!(v1.bundle_id, v3.bundle_id); + } + + #[test] + fn the_render_shows_full_calldata_and_flags_contract_creation() { + let (_d, _ks, mut ap) = fixture(); + let intent = format!( + r#"{{"address":"{ACCT0}","legs":[ + {{"kind":"tx","chain_id":1,"tx":{{ + "create":true,"value":"0x0","nonce":"0x1","gas_limit":"0x5208", + "data":"0xdeadbeefcafe","fee_mode":"eip1559", + "max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}}}}]}}"# + ); + let (h, _) = ap.request("wallet_backend", &intent).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + let all = v.render_lines.join("\n"); + assert!(all.contains("CONTRACT CREATION"), "{all}"); + assert!(all.contains("0xdeadbeefcafe"), "calldata must not be elided: {all}"); + assert!(all.contains("Selector: 0xdeadbeef"), "{all}"); + } + + #[test] + fn an_opaque_digest_is_rendered_as_opaque() { + let (_d, _ks, mut ap) = fixture(); + let intent = format!( + r#"{{"address":"{ACCT0}","legs":[{{"kind":"digest", + "digest":"0x{}","purpose":"ERC-4337 UserOperation"}}]}}"#, + "11".repeat(32) + ); + let (h, _) = ap.request("railgun_module", &intent).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + let all = v.render_lines.join("\n"); + assert!(all.contains("OPAQUE"), "{all}"); + assert!(all.contains("cannot show you what this authorises"), "{all}"); + assert!(all.contains("claimed by the requester"), "purpose must be marked a claim: {all}"); + } + + #[test] + fn a_bundle_is_one_decision_over_several_legs() { + let (_d, ks, mut ap) = fixture(); + let intent = format!( + r#"{{"address":"{ACCT0}","purpose":"shield","legs":[ + {{"kind":"tx","chain_id":1,"tx":{{"to":"{ACCT0}","value":"0x0","nonce":"0x1", + "gas_limit":"0x5208","fee_mode":"eip1559","max_fee_per_gas":"0x1", + "max_priority_fee_per_gas":"0x1"}}}}, + {{"kind":"tx","chain_id":1,"tx":{{"to":"{ACCT0}","value":"0x0","nonce":"0x2", + "gas_limit":"0x5208","fee_mode":"eip1559","max_fee_per_gas":"0x1", + "max_priority_fee_per_gas":"0x1"}}}}]}}"# + ); + let (h, r) = ap.request("wallet_backend", &intent).unwrap(); + let v = ap.acknowledge(&h).unwrap(); + // One password entry, two signatures. + assert_eq!(ap.approve(&ks, &h, &v.bundle_id, "pw").unwrap(), 2); + assert_eq!(ap.fetch_result(&h, &r).unwrap().len(), 2); + } + + #[test] + fn an_unacknowledged_request_expires_and_says_why() { + let (_d, _ks, mut ap) = fixture(); + ap.set_ack_deadline(Duration::from_millis(0)); + let (h, r) = ap.request("wallet_backend", &tx_intent("0x1")).unwrap(); + std::thread::sleep(Duration::from_millis(5)); + let (state, reason) = ap.status(&h, &r).unwrap(); + assert_eq!(state, "settled"); + assert_eq!(reason, Some("expired_no_ack")); + // And it can no longer be acknowledged. + assert!(ap.acknowledge(&h).is_err()); + } + + #[test] + fn a_requester_cannot_flood_the_queue() { + let (_d, _ks, mut ap) = fixture(); + for i in 0..MAX_PENDING_PER_REQUESTER { + ap.request("noisy", &tx_intent(&format!("0x{i}"))).unwrap(); + } + assert!(ap.request("noisy", &tx_intent("0xff")).is_err()); + // A different requester is unaffected. + assert!(ap.request("quiet", &tx_intent("0x1")).is_ok()); + } + + #[test] + fn an_oversize_or_empty_intent_is_refused_before_parsing() { + let (_d, _ks, mut ap) = fixture(); + let huge = format!(r#"{{"address":"{ACCT0}","purpose":"{}","legs":[]}}"#, "a".repeat(MAX_INTENT_BYTES)); + assert!(ap.request("x", &huge).is_err()); + assert!(ap.request("x", &format!(r#"{{"address":"{ACCT0}","legs":[]}}"#)).is_err()); + } +} diff --git a/rust-lib/src/glue.rs b/rust-lib/src/glue.rs index 79982bd..840c9b3 100644 --- a/rust-lib/src/glue.rs +++ b/rust-lib/src/glue.rs @@ -14,8 +14,12 @@ use serde::Deserialize; use serde_json::json; use std::time::Duration; +use crate::approval::Approvals; use crate::keystore::Keystore; +/// Default approver. Overridden by `approver` in `/keystore.json`. +const DEFAULT_APPROVER: &str = "signer_ui"; + /// The keystore module's IPC contract. Each non-defaulted method is a callable /// module method. Private keys never appear in any signature — only addresses, /// signed payloads, and (re-encrypted) keystore JSON cross the boundary. @@ -37,20 +41,39 @@ pub trait KeystoreModule: Send + 'static { fn list_accounts(&mut self) -> String; fn has_address(&mut self, address: String) -> bool; fn delete_account(&mut self, address: String, password: String) -> bool; - fn unlock(&mut self, address: String, password: String) -> bool; - fn timed_unlock(&mut self, address: String, password: String, seconds: i64) -> bool; - fn lock(&mut self, address: String) -> bool; - fn is_unlocked(&mut self, address: String) -> bool; - /// Sign an unsigned tx (JSON) for `chain_id` → `{ ok, raw }` (signed tx hex). - fn sign_transaction(&mut self, address: String, unsigned_tx_json: String, chain_id: i64) -> String; - /// EIP-191 personal_sign → `{ ok, signature }`. - fn sign_message(&mut self, address: String, message: String) -> String; - /// Sign a raw 32-byte digest (hex) with `address`'s key → `{ ok, signature }` - /// (65-byte hex). For protocol digests — an ERC-4337 UserOperation hash or an - /// EIP-7702 authorization hash — that aren't a transaction or an EIP-191 - /// message. ⚠️ Signs an opaque hash (see `Keystore::sign_digest`); for trusted - /// in-app callers only. The account must be unlocked. - fn sign_digest(&mut self, address: String, digest_hex: String) -> String; + // ── Tier B: any NAMED module may ask ──────────────────────────────── + /// Ask a human to approve signing. Returns immediately with + /// `{ ok, handle, receipt }` — it does NOT block on the human. `handle` is + /// announced on the event plane; `receipt` is returned exactly once and is + /// what authorises collecting the result. + fn request_approval(&mut self, intent_json: String) -> String; + /// Bare state for the requester — never the intent, never the results. + /// `{ ok, state, reason? }`. + fn approval_status(&mut self, handle: String, receipt: String) -> String; + /// Collect the signatures. Idempotent until `ack_result`, so a dropped + /// reply does not cost the human a second password entry. + fn fetch_result(&mut self, handle: String, receipt: String) -> String; + /// The requester has the signatures; erase them. + fn ack_result(&mut self, handle: String, receipt: String) -> bool; + /// The requester gave up. + fn cancel_approval(&mut self, handle: String, receipt: String) -> bool; + + // ── Tier A: the configured approver only ──────────────────────────── + /// Queue summaries — never leg detail. `{ ok, pending: [...] }`. + fn pending(&mut self) -> String; + /// Claim a request for display. Returns the lines to show VERBATIM plus the + /// commitment to echo back. Demotes any other rendered request, so exactly + /// one thing can be on screen. `{ ok, handle, bundle_id, requester, render_lines }`. + fn acknowledge(&mut self, handle: String) -> String; + /// The human said yes. One key derivation, every leg signed, then wiped. + /// `bundle_id` must be the value that was displayed. `{ ok, signed }`. + fn approve(&mut self, handle: String, bundle_id: String, password: String) -> String; + /// The human said no. + fn reject(&mut self, handle: String) -> bool; + + /// Observability: what this module currently sees as its caller. Ungated + /// and side-effect-free — identity cannot report its own absence. + fn caller_identity(&mut self) -> String; /// Framework hook — defaulted, so it is NOT part of the IPC contract. fn on_context_ready(&mut self, _ctx: &RustModuleContext) {} } @@ -58,6 +81,12 @@ pub trait KeystoreModule: Send + 'static { /// Typed events — emitted whenever the set of accounts changes. pub trait KeystoreModuleEvents { fn accounts_changed(&self, count: i64); + /// A new request is waiting. Payload is the HANDLE ONLY: the event plane + /// carries no token, so anything richer would publish the intent to every + /// subscriber. + fn approval_offered(&self, handle: String); + /// A request reached a terminal state. + fn approval_settled(&self, handle: String, state: String); } // The builder injects the generated module-impl scaffold here: `install`, @@ -68,6 +97,12 @@ include!(concat!(env!("CARGO_MANIFEST_DIR"), "/generated/provider_gen.rs")); #[derive(Default)] struct KeystoreModuleImpl { ks: Option, + approvals: Approvals, + /// The one module name permitted to approve. Read from a config file in + /// `on_context_ready`, i.e. from the loader's own directory — never from a + /// method, which would make "who may approve a signature" remotely + /// writable. + approver: String, } impl KeystoreModuleImpl { @@ -85,6 +120,34 @@ fn err(e: impl std::fmt::Display) -> String { json!({ "ok": false, "error": e.to_string() }).to_string() } +/// Refused identically whether the handle is unknown or simply not yours, in +/// content and in shape — a distinct message would answer "does this handle +/// exist?" for a caller that has no business knowing. +fn not_authorized() -> String { + json!({ "ok": false, "error": "not authorized" }).to_string() +} + +impl KeystoreModuleImpl { + /// Tier A: the configured approver, and nothing else. + /// + /// `HostAnchor` is refused deliberately. It is one undifferentiated bag + /// covering the shells, `core_service` and every relayed CLI token, so + /// admitting it here would make a plain `logosctl call … approve` a legal + /// bypass of the human. + fn is_approver(&self) -> bool { + logos_rust_sdk::current_caller().is_module(&self.approver) + } + + /// Tier B: any NAMED module. Returns the name to record against the + /// request, so results can only be collected by the module that asked. + fn named_caller(&self) -> Option { + match logos_rust_sdk::current_caller() { + logos_rust_sdk::LogosCaller::Module { name, .. } => Some(name), + _ => None, + } + } +} + #[derive(Deserialize)] struct ImportMnemonicParams { phrase: String, @@ -97,8 +160,17 @@ struct ImportMnemonicParams { impl KeystoreModule for KeystoreModuleImpl { fn on_context_ready(&mut self, ctx: &RustModuleContext) { - let dir = std::path::Path::new(&ctx.instance_persistence_path).join("keystore"); - self.ks = Some(Keystore::new(dir)); + let base = std::path::Path::new(&ctx.instance_persistence_path); + self.ks = Some(Keystore::new(base.join("keystore"))); + + // Who may approve is configuration, not a method: a `set_approver` call + // would be a remotely-writable answer to "who may authorise a + // signature". This file is written by whoever deploys the module. + self.approver = std::fs::read_to_string(base.join("keystore.json")) + .ok() + .and_then(|t| serde_json::from_str::(&t).ok()) + .and_then(|v| v.get("approver").and_then(|a| a.as_str()).map(str::to_string)) + .unwrap_or_else(|| DEFAULT_APPROVER.to_string()); } fn create_mnemonic(&mut self, words: i64) -> String { @@ -206,51 +278,136 @@ impl KeystoreModule for KeystoreModuleImpl { } } - fn unlock(&mut self, address: String, password: String) -> bool { - self.ks().map(|ks| ks.unlock(&address, &password, None).is_ok()).unwrap_or(false) - } + // ── Tier B ────────────────────────────────────────────────────────── - fn timed_unlock(&mut self, address: String, password: String, seconds: i64) -> bool { - let ttl = Duration::from_secs(seconds.max(0) as u64); - self.ks().map(|ks| ks.unlock(&address, &password, Some(ttl)).is_ok()).unwrap_or(false) - } - - fn lock(&mut self, address: String) -> bool { - self.ks().map(|ks| ks.lock(&address)).unwrap_or(false) - } - - fn is_unlocked(&mut self, address: String) -> bool { - self.ks().map(|ks| ks.is_unlocked(&address)).unwrap_or(false) - } - - fn sign_transaction(&mut self, address: String, unsigned_tx_json: String, chain_id: i64) -> String { - match self.ks() { - Ok(ks) => match ks.sign_transaction(&address, &unsigned_tx_json, chain_id as u64) { - Ok(raw) => json!({ "ok": true, "raw": raw }).to_string(), - Err(e) => err(e), - }, + fn request_approval(&mut self, intent_json: String) -> String { + let Some(requester) = self.named_caller() else { + return not_authorized(); + }; + if self.approver.is_empty() { + return err("no approver configured"); + } + match self.approvals.request(&requester, &intent_json) { + Ok((handle, receipt)) => { + emit_approval_offered(&handle); + json!({ "ok": true, "handle": handle, "receipt": receipt }).to_string() + } Err(e) => err(e), } } - fn sign_message(&mut self, address: String, message: String) -> String { - match self.ks() { - Ok(ks) => match ks.sign_message(&address, &message) { - Ok(sig) => json!({ "ok": true, "signature": sig }).to_string(), - Err(e) => err(e), - }, + fn approval_status(&mut self, handle: String, receipt: String) -> String { + if self.named_caller().is_none() { + return not_authorized(); + } + match self.approvals.status(&handle, &receipt) { + Ok((state, reason)) => json!({ "ok": true, "state": state, "reason": reason }).to_string(), + Err(_) => not_authorized(), + } + } + + fn fetch_result(&mut self, handle: String, receipt: String) -> String { + if self.named_caller().is_none() { + return not_authorized(); + } + match self.approvals.fetch_result(&handle, &receipt) { + // `signed`, matching the documented contract and `approve`'s count + // field name-for-meaning: this is the array the requester collects. + Ok(results) => json!({ "ok": true, "signed": results }).to_string(), + Err(_) => not_authorized(), + } + } + + fn ack_result(&mut self, handle: String, receipt: String) -> bool { + self.named_caller().is_some() && self.approvals.ack_result(&handle, &receipt).is_ok() + } + + fn cancel_approval(&mut self, handle: String, receipt: String) -> bool { + self.named_caller().is_some() && self.approvals.cancel(&handle, &receipt).is_ok() + } + + // ── Tier A ────────────────────────────────────────────────────────── + + fn pending(&mut self) -> String { + if !self.is_approver() { + return not_authorized(); + } + let items: Vec<_> = self + .approvals + .pending() + .into_iter() + .map(|s| { + json!({ + "handle": s.handle, "requester": s.requester, "state": s.state, + "purpose": s.purpose, "leg_count": s.leg_count, "age_ms": s.age_ms as u64, + }) + }) + .collect(); + json!({ "ok": true, "pending": items }).to_string() + } + + fn acknowledge(&mut self, handle: String) -> String { + if !self.is_approver() { + return not_authorized(); + } + match self.approvals.acknowledge(&handle) { + Ok(r) => json!({ + "ok": true, "handle": r.handle, "bundle_id": r.bundle_id, + "requester": r.requester, "render_lines": r.render_lines, + }) + .to_string(), Err(e) => err(e), } } - fn sign_digest(&mut self, address: String, digest_hex: String) -> String { - match self.ks() { - Ok(ks) => match ks.sign_digest(&address, &digest_hex) { - Ok(sig) => json!({ "ok": true, "signature": sig }).to_string(), - Err(e) => err(e), - }, - Err(e) => err(e), + fn approve(&mut self, handle: String, bundle_id: String, password: String) -> String { + if !self.is_approver() { + return not_authorized(); } + let ks = match self.ks.as_ref() { + Some(k) => k, + None => return err("keystore not initialized (context not ready)"), + }; + let out = self.approvals.approve(ks, &handle, &bundle_id, &password); + crate::approval::scrub(password); + match out { + Ok(n) => { + emit_approval_settled(&handle, "approved"); + // A COUNT, not the signatures. The approver authorises a + // bundle; it never receives what it authorised. Only the + // requester can collect that, and only with the receipt it was + // handed at request time. Distinct key from fetch_result's + // `signed` array so the two can never be confused. + json!({ "ok": true, "signed_count": n }).to_string() + } + // Deliberately coarse: a wrong password, an unknown handle and a + // stale commitment must not be distinguishable to a caller probing + // the surface. The approver shows the human a generic retry. + Err(_) => err("approval failed"), + } + } + + fn reject(&mut self, handle: String) -> bool { + if !self.is_approver() { + return false; + } + let ok = self.approvals.reject(&handle).is_ok(); + if ok { + emit_approval_settled(&handle, "rejected"); + } + ok + } + + fn caller_identity(&mut self) -> String { + let c = logos_rust_sdk::current_caller(); + let (kind, name) = match &c { + logos_rust_sdk::LogosCaller::Unknown => ("unknown", String::new()), + logos_rust_sdk::LogosCaller::HostAnchor => ("host", String::new()), + logos_rust_sdk::LogosCaller::Module { name, .. } => ("module", name.clone()), + logos_rust_sdk::LogosCaller::Derived { parent, leaf } => ("derived", format!("{parent}.{leaf}")), + logos_rust_sdk::LogosCaller::Operator { name } => ("operator", name.clone()), + }; + json!({ "ok": true, "kind": kind, "identity": name, "approver": self.approver }).to_string() } } diff --git a/rust-lib/src/keystore.rs b/rust-lib/src/keystore.rs index 6f0ba42..895ad6b 100644 --- a/rust-lib/src/keystore.rs +++ b/rust-lib/src/keystore.rs @@ -7,9 +7,7 @@ //! private key across its API — only addresses, signed payloads, and //! (re-encrypted) vault JSON. -use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::time::{Duration, Instant}; use alloy::consensus::{SignableTransaction, TxEip1559, TxEnvelope, TxLegacy}; use alloy::eips::eip2718::Encodable2718; @@ -21,6 +19,7 @@ use alloy::signers::local::{ use alloy::signers::SignerSync; use serde::Deserialize; use thiserror::Error; +use zeroize::Zeroizing; /// BIP-44 Ethereum account path, account 0, external chain: m/44'/60'/0'/0/. fn eth_derivation_path(index: u32) -> String { @@ -49,40 +48,54 @@ pub enum KeystoreError { type Result = std::result::Result; -/// An unlocked, in-memory signer with an optional auto-relock deadline. -struct Unlocked { - signer: PrivateKeySigner, - expires_at: Option, -} - -/// Manages a directory of scrypt vault files plus the set of currently-unlocked -/// signers. One vault file per account, named `.json`. +/// Manages a directory of scrypt vault files. One vault file per account, named +/// `.json`. +/// +/// There is deliberately NO cache of unlocked signers. Signing *is* vault +/// access: a key is derived from the vault password for one operation and wiped +/// when that operation ends. The previous design kept a `HashMap` whose entries carried an *optional* deadline, and the only caller +/// passed `None` — so an unlock was an unlimited, process-lifetime signer that +/// any module able to reach this one could spend. pub struct Keystore { dir: PathBuf, - unlocked: HashMap, } /// Fields of an unsigned transaction, as JSON from the caller. All numeric /// fields are hex (`0x…`) or decimal strings to avoid precision loss across the /// JSON boundary. `fee_mode` selects EIP-1559 (default) vs legacy. #[derive(Debug, Deserialize)] -struct UnsignedTx { - to: Option, +#[serde(deny_unknown_fields)] +pub struct UnsignedTx { + pub to: Option, + /// Contract creation must be asked for explicitly. Previously an absent or + /// blank `to` fell through to `TxKind::Create`, so a transfer whose + /// recipient failed to render became a contract deployment that burned the + /// value. #[serde(default)] - value: String, - nonce: String, + pub create: bool, + /// Present only so an access-list-bearing tx is REFUSED with a reason + /// rather than silently stripped: the signer previously hardcoded + /// `access_list: Default::default()`, so a caller's EIP-2930 list was + /// dropped and the signature covered a different transaction than the one + /// requested. #[serde(default)] - gas_limit: String, + pub access_list: Option, #[serde(default)] - data: String, + pub value: String, + pub nonce: String, #[serde(default)] - fee_mode: String, // "eip1559" (default) | "legacy" + pub gas_limit: String, #[serde(default)] - max_fee_per_gas: String, + pub data: String, #[serde(default)] - max_priority_fee_per_gas: String, + pub fee_mode: String, // "eip1559" (default) | "legacy" #[serde(default)] - gas_price: String, + pub max_fee_per_gas: String, + #[serde(default)] + pub max_priority_fee_per_gas: String, + #[serde(default)] + pub gas_price: String, } fn parse_u128(s: &str, what: &str) -> Result { @@ -99,7 +112,13 @@ fn parse_u128(s: &str, what: &str) -> Result { } fn parse_u64(s: &str, what: &str) -> Result { - Ok(parse_u128(s, what)? as u64) + // A wrapping `as u64` here silently truncated: `0x10000000000000005` and + // `0x5` produced BYTE-IDENTICAL signed transactions, so a render built from + // the caller's string and a signature built from this value could disagree + // about the nonce. Reject instead. + let wide = parse_u128(s, what)?; + u64::try_from(wide) + .map_err(|_| KeystoreError::InvalidParams(format!("{what}: {wide} does not fit in u64"))) } fn parse_u256(s: &str, what: &str) -> Result { @@ -127,6 +146,20 @@ fn parse_address(s: &str) -> Result
{ Ok(Address::from_slice(&bytes)) } +/// Tighten a path's mode. No-op off unix, where the enclosing directory ACL is +/// the control instead. +fn restrict_permissions(path: &Path, mode: u32) -> Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) + .map_err(|e| KeystoreError::Io(e.to_string()))?; + } + #[cfg(not(unix))] + let _ = (path, mode); + Ok(()) +} + fn vault_name(addr: &Address) -> String { // lowercase hex, no 0x — stable filename + easy listing format!("{:x}", addr) @@ -134,11 +167,27 @@ fn vault_name(addr: &Address) -> String { impl Keystore { pub fn new(dir: impl Into) -> Self { - Self { dir: dir.into(), unlocked: HashMap::new() } + Self { dir: dir.into() } + } + + /// Derive the signer for `address` from its vault. The returned signer is + /// live only for the caller's scope — there is nowhere else it is kept. + pub fn signer_for(&self, address: &str, password: &str) -> Result { + let addr = parse_address(address)?; + let path = self.vault_path(&addr); + if !path.exists() { + return Err(KeystoreError::NotFound(address.to_string())); + } + let key = Zeroizing::new( + eth_keystore::decrypt_key(&path, password).map_err(|e| KeystoreError::Vault(e.to_string()))?, + ); + PrivateKeySigner::from_slice(&key).map_err(|e| KeystoreError::InvalidKey(e.to_string())) } fn ensure_dir(&self) -> Result<()> { - std::fs::create_dir_all(&self.dir).map_err(|e| KeystoreError::Io(e.to_string())) + std::fs::create_dir_all(&self.dir).map_err(|e| KeystoreError::Io(e.to_string()))?; + // create_dir_all honours the umask, so the vault directory was 0755. + restrict_permissions(&self.dir, 0o700) } fn vault_path(&self, addr: &Address) -> PathBuf { @@ -161,11 +210,15 @@ impl Keystore { fn persist_signer(&self, signer: &PrivateKeySigner, password: &str) -> Result
{ self.ensure_dir()?; let addr = signer.address(); - let key: B256 = signer.to_bytes(); + // `to_bytes()` hands back the raw secp256k1 secret. Own it in a + // Zeroizing so it is wiped when this scope ends, on every path. + let key = Zeroizing::new(signer.to_bytes().0); let mut rng = rand::thread_rng(); let name = format!("{}.json", vault_name(&addr)); eth_keystore::encrypt_key(&self.dir, &mut rng, key.as_slice(), password, Some(&name)) .map_err(|e| KeystoreError::Vault(e.to_string()))?; + // encrypt_key uses File::create, i.e. 0644 at the default umask. + restrict_permissions(&self.vault_path(&addr), 0o600)?; Ok(addr) } @@ -190,8 +243,12 @@ impl Keystore { /// Import an existing scrypt keystore JSON, re-encrypting under `new_password`. pub fn import_keystore_json(&self, key_json: &str, password: &str, new_password: &str) -> Result
{ + check_kdf_params(key_json)?; let tmp = tempfile_with(key_json)?; - let key = eth_keystore::decrypt_key(&tmp, password).map_err(|e| KeystoreError::Vault(e.to_string()))?; + let key = Zeroizing::new( + eth_keystore::decrypt_key(tmp.path(), password) + .map_err(|e| KeystoreError::Vault(e.to_string()))?, + ); let signer = PrivateKeySigner::from_slice(&key).map_err(|e| KeystoreError::InvalidKey(e.to_string()))?; self.persist_signer(&signer, new_password) } @@ -230,7 +287,7 @@ impl Keystore { } } - pub fn delete_account(&mut self, address: &str, password: &str) -> Result { + pub fn delete_account(&self, address: &str, password: &str) -> Result { let addr = parse_address(address)?; let path = self.vault_path(&addr); if !path.exists() { @@ -239,139 +296,141 @@ impl Keystore { // Require the correct password before destroying the vault. eth_keystore::decrypt_key(&path, password).map_err(|e| KeystoreError::Vault(e.to_string()))?; std::fs::remove_file(&path).map_err(|e| KeystoreError::Io(e.to_string()))?; - self.unlocked.remove(&addr); Ok(true) } - pub fn unlock(&mut self, address: &str, password: &str, ttl: Option) -> Result<()> { - let addr = parse_address(address)?; - let path = self.vault_path(&addr); - if !path.exists() { - return Err(KeystoreError::NotFound(address.to_string())); + /// EIP-191 personal_sign over `message` — derives, signs, wipes. + pub fn sign_message(&self, address: &str, password: &str, message: &str) -> Result { + sign_message_with(&self.signer_for(address, password)?, message) + } + + /// Sign an unsigned tx, returning the broadcast-ready EIP-2718 envelope hex. + pub fn sign_transaction( + &self, + address: &str, + password: &str, + unsigned_tx_json: &str, + chain_id: u64, + ) -> Result { + sign_transaction_with(&self.signer_for(address, password)?, unsigned_tx_json, chain_id) + } + + /// Sign a raw 32-byte digest. See [`sign_digest_with`] for the caveat. + pub fn sign_digest(&self, address: &str, password: &str, digest_hex: &str) -> Result { + sign_digest_with(&self.signer_for(address, password)?, digest_hex) + } +} + +/// EIP-191 personal_sign. Refuses text that can render differently than it +/// signs (see [`check_displayable`]). +pub fn sign_message_with(signer: &PrivateKeySigner, message: &str) -> Result { + check_displayable(message, "message")?; + let sig = signer + .sign_message_sync(message.as_bytes()) + .map_err(|e| KeystoreError::Signing(e.to_string()))?; + Ok(format!("0x{}", hex::encode(sig.as_bytes()))) +} + +/// Sign an unsigned tx and return the raw, broadcast-ready signed tx hex +/// (EIP-2718 envelope). Supports legacy (EIP-155) and EIP-1559. +pub fn sign_transaction_with( + signer: &PrivateKeySigner, + unsigned_tx_json: &str, + chain_id: u64, +) -> Result { + let tx: UnsignedTx = serde_json::from_str(unsigned_tx_json) + .map_err(|e| KeystoreError::InvalidParams(format!("tx json: {e}")))?; + sign_parsed_tx(signer, &tx, chain_id) +} + +/// Sign an ALREADY-PARSED transaction. The approval path parses once, commits +/// to the parsed value, and signs from that same value — so the bytes a human +/// was shown and the bytes that get signed cannot drift apart. +pub fn sign_parsed_tx(signer: &PrivateKeySigner, tx: &UnsignedTx, chain_id: u64) -> Result { + let to = match (tx.to.as_deref().map(str::trim).filter(|s| !s.is_empty()), tx.create) { + (Some(_), true) => { + return Err(KeystoreError::InvalidParams( + "tx json: `to` and `create: true` are mutually exclusive".into(), + )) } - let key = eth_keystore::decrypt_key(&path, password).map_err(|e| KeystoreError::Vault(e.to_string()))?; - let signer = PrivateKeySigner::from_slice(&key).map_err(|e| KeystoreError::InvalidKey(e.to_string()))?; - let expires_at = ttl.map(|d| Instant::now() + d); - self.unlocked.insert(addr, Unlocked { signer, expires_at }); - Ok(()) - } - - pub fn lock(&mut self, address: &str) -> bool { - match parse_address(address) { - Ok(addr) => self.unlocked.remove(&addr).is_some(), - Err(_) => false, + (Some(s), false) => TxKind::Call(parse_address(s)?), + (None, true) => TxKind::Create, + (None, false) => { + return Err(KeystoreError::InvalidParams( + "tx json: `to` is required; set `create: true` to deploy a contract".into(), + )) } + }; + + if tx.access_list.as_ref().is_some_and(|v| !v.is_null()) { + return Err(KeystoreError::InvalidParams( + "tx json: access lists are not supported by this signer — remove `access_list` \ + rather than have it silently dropped from the signed payload" + .into(), + )); } - pub fn is_unlocked(&mut self, address: &str) -> bool { - match parse_address(address) { - Ok(addr) => self.live_signer(&addr).is_some(), - Err(_) => false, + let value = parse_u256(&tx.value, "value")?; + let nonce = parse_u64(&tx.nonce, "nonce")?; + let gas_limit = parse_u64(&tx.gas_limit, "gas_limit")?; + let input = parse_bytes(&tx.data)?; + + let legacy = match tx.fee_mode.trim() { + "" | "eip1559" => false, + "legacy" => true, + other => { + return Err(KeystoreError::InvalidParams(format!( + "tx json: fee_mode must be \"eip1559\" or \"legacy\", got {other:?}" + ))) } - } + }; - /// Fetch an unlocked signer, evicting it first if its TTL has elapsed. - fn live_signer(&mut self, addr: &Address) -> Option<&PrivateKeySigner> { - if let Some(u) = self.unlocked.get(addr) { - if let Some(exp) = u.expires_at { - if Instant::now() >= exp { - self.unlocked.remove(addr); - return None; - } - } - } - self.unlocked.get(addr).map(|u| &u.signer) - } - - /// EIP-191 personal_sign over `message`. Returns 65-byte signature hex. - pub fn sign_message(&mut self, address: &str, message: &str) -> Result { - let addr = parse_address(address)?; - let signer = self.live_signer(&addr).ok_or_else(|| KeystoreError::Locked(address.to_string()))?; - let sig = signer - .sign_message_sync(message.as_bytes()) - .map_err(|e| KeystoreError::Signing(e.to_string()))?; - Ok(format!("0x{}", hex::encode(sig.as_bytes()))) - } - - /// Sign an unsigned tx and return the raw, broadcast-ready signed tx hex - /// (EIP-2718 envelope). Supports legacy (EIP-155) and EIP-1559. - pub fn sign_transaction(&mut self, address: &str, unsigned_tx_json: &str, chain_id: u64) -> Result { - let addr = parse_address(address)?; - let signer = self - .live_signer(&addr) - .ok_or_else(|| KeystoreError::Locked(address.to_string()))? - .clone(); - - let tx: UnsignedTx = serde_json::from_str(unsigned_tx_json) - .map_err(|e| KeystoreError::InvalidParams(format!("tx json: {e}")))?; - - let to = match tx.to.as_deref() { - Some(s) if !s.trim().is_empty() => TxKind::Call(parse_address(s)?), - _ => TxKind::Create, + let raw = if legacy { + let t = TxLegacy { + chain_id: Some(chain_id), + nonce, + gas_price: parse_u128(&tx.gas_price, "gas_price")?, + gas_limit, + to, + value, + input, }; - let value = parse_u256(&tx.value, "value")?; - let nonce = parse_u64(&tx.nonce, "nonce")?; - let gas_limit = parse_u64(&tx.gas_limit, "gas_limit")?; - let input = parse_bytes(&tx.data)?; - - let raw = if tx.fee_mode.eq_ignore_ascii_case("legacy") { - let t = TxLegacy { - chain_id: Some(chain_id), - nonce, - gas_price: parse_u128(&tx.gas_price, "gas_price")?, - gas_limit, - to, - value, - input, - }; - let sig = signer - .sign_hash_sync(&t.signature_hash()) - .map_err(|e| KeystoreError::Signing(e.to_string()))?; - let signed = t.into_signed(sig); - TxEnvelope::Legacy(signed).encoded_2718() - } else { - let t = TxEip1559 { - chain_id, - nonce, - gas_limit, - max_fee_per_gas: parse_u128(&tx.max_fee_per_gas, "max_fee_per_gas")?, - max_priority_fee_per_gas: parse_u128(&tx.max_priority_fee_per_gas, "max_priority_fee_per_gas")?, - to, - value, - input, - access_list: Default::default(), - }; - let sig = signer - .sign_hash_sync(&t.signature_hash()) - .map_err(|e| KeystoreError::Signing(e.to_string()))?; - let signed = t.into_signed(sig); - TxEnvelope::Eip1559(signed).encoded_2718() - }; - - Ok(format!("0x{}", hex::encode(raw))) - } - - /// Sign a raw 32-byte `digest` with `address`'s key (ECDSA over the hash — - /// no EIP-191/EIP-712 prefix). Returns the 65-byte signature hex. - /// - /// ⚠️ SECURITY: this signs an *opaque* hash. Unlike [`Self::sign_transaction`], - /// whose fields a UI can display, the caller fully controls the preimage — and - /// a 32-byte digest could be a transaction's `signature_hash`, so anyone able - /// to reach an *unlocked* account here could obtain a draining-tx signature. - /// Expose it only to trusted in-app modules signing protocol digests (an - /// ERC-4337 UserOperation hash or an EIP-7702 authorization hash), never to - /// untrusted input. The account must be unlocked (same gate as the others). - pub fn sign_digest(&mut self, address: &str, digest_hex: &str) -> Result { - let addr = parse_address(address)?; - let digest = parse_b256(digest_hex)?; - let signer = self - .live_signer(&addr) - .ok_or_else(|| KeystoreError::Locked(address.to_string()))?; let sig = signer - .sign_hash_sync(&digest) + .sign_hash_sync(&t.signature_hash()) .map_err(|e| KeystoreError::Signing(e.to_string()))?; - Ok(format!("0x{}", hex::encode(sig.as_bytes()))) - } + TxEnvelope::Legacy(t.into_signed(sig)).encoded_2718() + } else { + let t = TxEip1559 { + chain_id, + nonce, + gas_limit, + max_fee_per_gas: parse_u128(&tx.max_fee_per_gas, "max_fee_per_gas")?, + max_priority_fee_per_gas: parse_u128(&tx.max_priority_fee_per_gas, "max_priority_fee_per_gas")?, + to, + value, + input, + access_list: Default::default(), + }; + let sig = signer + .sign_hash_sync(&t.signature_hash()) + .map_err(|e| KeystoreError::Signing(e.to_string()))?; + TxEnvelope::Eip1559(t.into_signed(sig)).encoded_2718() + }; + + Ok(format!("0x{}", hex::encode(raw))) +} + +/// Sign a raw 32-byte digest (ECDSA over the hash — no EIP-191/712 prefix). +/// +/// This signs an OPAQUE hash: unlike a transaction, nothing here can be +/// rendered, so the approval layer must commit to a typed preimage and describe +/// that instead. +pub fn sign_digest_with(signer: &PrivateKeySigner, digest_hex: &str) -> Result { + let digest = parse_b256(digest_hex)?; + let sig = signer + .sign_hash_sync(&digest) + .map_err(|e| KeystoreError::Signing(e.to_string()))?; + Ok(format!("0x{}", hex::encode(sig.as_bytes()))) } /// Parse a 32-byte hash hex (`0x`-prefixed or bare) into a `B256`. @@ -388,6 +447,104 @@ fn parse_b256(s: &str) -> Result { Ok(B256::from_slice(&bytes)) } +/// Upper bounds on the KDF work a *caller-supplied* vault may ask us to +/// perform. `eth_keystore::decrypt_key` feeds these straight to scrypt, so an +/// unclamped `n` is a one-file denial of service: `n = u32::MAX` asks for a +/// ~4 TiB allocation and aborts the whole module process. +const MAX_SCRYPT_LOG_N: u32 = 18; // 2^18 = 262144, the Web3/geth standard +const MAX_SCRYPT_R: u64 = 16; +const MAX_SCRYPT_P: u64 = 16; +const MAX_PBKDF2_C: u64 = 10_000_000; +const MAX_KDF_MEMORY_BYTES: u64 = 512 * 1024 * 1024; + +/// Reject a vault whose KDF parameters are out of range, BEFORE any derivation +/// is attempted. +fn check_kdf_params(key_json: &str) -> Result<()> { + let v: serde_json::Value = serde_json::from_str(key_json) + .map_err(|e| KeystoreError::Vault(format!("keystore json: {e}")))?; + let crypto = v + .get("crypto") + .or_else(|| v.get("Crypto")) + .ok_or_else(|| KeystoreError::Vault("keystore json: missing `crypto`".into()))?; + let kdf = crypto.get("kdf").and_then(|k| k.as_str()).unwrap_or_default(); + let params = crypto + .get("kdfparams") + .ok_or_else(|| KeystoreError::Vault("keystore json: missing `crypto.kdfparams`".into()))?; + let num = |k: &str| params.get(k).and_then(|x| x.as_u64()); + + let bad = |m: String| Err(KeystoreError::Vault(format!("keystore json: {m}"))); + + if let Some(dklen) = num("dklen") { + if dklen != 32 { + return bad(format!("dklen must be 32, got {dklen}")); + } + } + + match kdf { + "scrypt" => { + let n = num("n").ok_or_else(|| KeystoreError::Vault("keystore json: scrypt `n` missing".into()))?; + let r = num("r").unwrap_or(8); + let p = num("p").unwrap_or(1); + if !n.is_power_of_two() { + return bad(format!("scrypt n must be a power of two, got {n}")); + } + if n.trailing_zeros() > MAX_SCRYPT_LOG_N { + return bad(format!("scrypt n = {n} exceeds 2^{MAX_SCRYPT_LOG_N}")); + } + if r > MAX_SCRYPT_R || p > MAX_SCRYPT_P { + return bad(format!("scrypt r/p out of range: r={r}, p={p}")); + } + // 128 * r * n is scrypt's working-set size. + let mem = 128u64.saturating_mul(r).saturating_mul(n); + if mem > MAX_KDF_MEMORY_BYTES { + return bad(format!("scrypt would need {mem} bytes, over the {MAX_KDF_MEMORY_BYTES} limit")); + } + } + "pbkdf2" => { + let c = num("c").ok_or_else(|| KeystoreError::Vault("keystore json: pbkdf2 `c` missing".into()))?; + if c > MAX_PBKDF2_C { + return bad(format!("pbkdf2 c = {c} exceeds {MAX_PBKDF2_C}")); + } + } + other => return bad(format!("unsupported kdf {other:?}")), + } + Ok(()) +} + +/// Longest message we will sign. Anything larger cannot be shown to a human in +/// full, and a signer must not sign what an approver cannot display. +const MAX_MESSAGE_BYTES: usize = 8 * 1024; + +/// Refuse text whose rendering can differ from its bytes: C0/C1 controls, +/// bidirectional overrides, and zero-width characters. These are exactly the +/// characters that let a display say one thing while the signature covers +/// another. +pub(crate) fn check_displayable(text: &str, what: &str) -> Result<()> { + if text.len() > MAX_MESSAGE_BYTES { + return Err(KeystoreError::InvalidParams(format!( + "{what}: {} bytes exceeds the {MAX_MESSAGE_BYTES}-byte limit", + text.len() + ))); + } + for c in text.chars() { + let bad = matches!(c, + // C0 controls except tab/newline/carriage-return, and DEL + C1. + '\u{0}'..='\u{8}' | '\u{B}' | '\u{C}' | '\u{E}'..='\u{1F}' | '\u{7F}'..='\u{9F}' + // Bidirectional embedding/override/isolate controls. + | '\u{202A}'..='\u{202E}' | '\u{2066}'..='\u{2069}' | '\u{200E}' | '\u{200F}' + // Zero-width and other invisible formatting. + | '\u{200B}'..='\u{200D}' | '\u{2060}' | '\u{FEFF}' + ); + if bad { + return Err(KeystoreError::InvalidParams(format!( + "{what}: refusing U+{:04X} — it can render differently than it signs", + c as u32 + ))); + } + } + Ok(()) +} + fn parse_bytes(s: &str) -> Result { let s = s.trim(); if s.is_empty() { @@ -416,21 +573,49 @@ fn signer_from_mnemonic(phrase: &str, passphrase: &str, index: u32) -> Result Result { +/// A temp file that deletes itself. `eth-keystore` is path-based, so importing +/// a vault JSON requires putting it on disk briefly. +/// +/// The previous version wrote `$TMPDIR/logos-ks-import-.json` at the +/// default umask (0644) and **never removed it**, so every import left a +/// world-readable copy of the caller's encrypted vault — salt and KDF params +/// included — in shared temp, under a guessable name. +struct TempVaultFile(PathBuf); + +impl Drop for TempVaultFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +impl TempVaultFile { + fn path(&self) -> &Path { + &self.0 + } +} + +fn tempfile_with(contents: &str) -> Result { + use rand::RngCore; use std::io::Write; + + let mut nonce = [0u8; 16]; + rand::thread_rng().fill_bytes(&mut nonce); let mut path = std::env::temp_dir(); - // A best-effort unique name; collisions are astronomically unlikely and the - // file is short-lived. - let nanos = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_nanos()) - .unwrap_or(0); - path.push(format!("logos-ks-import-{nanos}.json")); - let mut f = std::fs::File::create(&path).map_err(|e| KeystoreError::Io(e.to_string()))?; + path.push(format!("logos-ks-import-{}.json", hex::encode(nonce))); + + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); // O_EXCL: never adopt an existing path + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + let mut f = opts.open(&path).map_err(|e| KeystoreError::Io(e.to_string()))?; + // Own the path before the first fallible write, so an error still unlinks. + let guard = TempVaultFile(path); f.write_all(contents.as_bytes()).map_err(|e| KeystoreError::Io(e.to_string()))?; - Ok(path) + f.sync_all().map_err(|e| KeystoreError::Io(e.to_string()))?; + Ok(guard) } #[cfg(test)] @@ -445,6 +630,197 @@ mod tests { const ACCT0: Address = address!("f39Fd6e51aad88F6F4ce6aB8827279cffFb92266"); const ACCT0_PK: &str = "ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; + // ---- hardening regressions ------------------------------------------- + // One test per defect fixed in this pass. Each asserts the DANGEROUS + // behaviour is gone, not merely that the happy path still works. + + /// Build a signed tx for `tx_json`. There is no unlock step: the key is + /// derived from the vault password for this one signature. + fn sign_with(dir: &std::path::Path, tx_json: &str) -> Result { + let ks = Keystore::new(dir); + let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); + ks.sign_transaction(&addr.to_string(), "pw", tx_json, 1) + } + + #[test] + fn there_is_no_unlocked_state_to_reuse() { + let dir = tempfile::tempdir().unwrap(); + let ks = Keystore::new(dir.path()); + let a = ks.import_private_key(ACCT0_PK, "pw").unwrap().to_string(); + + // A correct-password signature must not leave anything behind that a + // later wrong-password call could spend. Under the old cache, the + // second call here succeeded. + assert!(ks.sign_message(&a, "pw", "first").is_ok()); + assert!(ks.sign_message(&a, "wrong", "second").is_err()); + assert!(ks.sign_message(&a, "pw", "third").is_ok()); + } + + #[test] + fn nonce_that_overflows_u64_is_rejected_not_truncated() { + let dir = tempfile::tempdir().unwrap(); + let tx = |nonce: &str| { + format!( + r#"{{"to":"0x{:x}","value":"0x0","nonce":"{nonce}","gas_limit":"0x5208", + "fee_mode":"eip1559","max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}}"#, + ACCT0 + ) + }; + // 0x10000000000000005 truncates to 0x5 in a wrapping cast: the two used + // to produce byte-identical signed transactions. + let big = sign_with(dir.path(), &tx("0x10000000000000005")); + assert!(big.is_err(), "an out-of-range nonce must not sign"); + let small = sign_with(dir.path(), &tx("0x5")).unwrap(); + assert!(!small.is_empty()); + } + + #[test] + fn unknown_tx_fields_are_rejected() { + let dir = tempfile::tempdir().unwrap(); + let tx = format!( + r#"{{"to":"0x{:x}","value":"0x0","nonce":"0x1","gas_limit":"0x5208", + "fee_mode":"eip1559","max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1", + "chainId":"0x1"}}"#, + ACCT0 + ); + // A camelCase or typo'd key silently defaulted to 0 before. + assert!(sign_with(dir.path(), &tx).is_err()); + } + + #[test] + fn absent_to_is_refused_rather_than_deploying_a_contract() { + let dir = tempfile::tempdir().unwrap(); + let no_to = r#"{"value":"0x0","nonce":"0x1","gas_limit":"0x5208","fee_mode":"eip1559", + "max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}"#; + let blank_to = r#"{"to":"","value":"0x0","nonce":"0x1","gas_limit":"0x5208","fee_mode":"eip1559", + "max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}"#; + for tx in [no_to, blank_to] { + let e = sign_with(dir.path(), tx).unwrap_err(); + assert!(format!("{e}").contains("`to` is required"), "got {e}"); + } + // Deployment is still possible, but only when asked for explicitly. + let create = r#"{"create":true,"value":"0x0","nonce":"0x1","gas_limit":"0x5208", + "data":"0x60006000","fee_mode":"eip1559", + "max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}"#; + assert!(sign_with(dir.path(), create).is_ok()); + } + + #[test] + fn fee_mode_is_a_closed_set_and_is_trimmed() { + let dir = tempfile::tempdir().unwrap(); + let tx = |mode: &str| { + format!( + r#"{{"to":"0x{:x}","value":"0x0","nonce":"0x1","gas_limit":"0x5208", + "fee_mode":"{mode}","gas_price":"0x7", + "max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1"}}"#, + ACCT0 + ) + }; + // " legacy" used to fall through to EIP-1559 and silently drop gas_price. + assert!(sign_with(dir.path(), &tx(" legacy")).is_ok()); + // A typo used to be indistinguishable from the default. + let e = sign_with(dir.path(), &tx("eip1599")).unwrap_err(); + assert!(format!("{e}").contains("fee_mode"), "got {e}"); + } + + #[test] + fn an_access_list_is_refused_rather_than_silently_dropped() { + let dir = tempfile::tempdir().unwrap(); + let tx = format!( + r#"{{"to":"0x{:x}","value":"0x0","nonce":"0x1","gas_limit":"0x5208", + "fee_mode":"eip1559","max_fee_per_gas":"0x1","max_priority_fee_per_gas":"0x1", + "access_list":[{{"address":"0x{:x}","storageKeys":[]}}]}}"#, + ACCT0, ACCT0 + ); + let e = sign_with(dir.path(), &tx).unwrap_err(); + assert!(format!("{e}").contains("access list"), "got {e}"); + } + + #[test] + fn sign_message_refuses_text_that_renders_differently_than_it_signs() { + let dir = tempfile::tempdir().unwrap(); + let ks = Keystore::new(dir.path()); + let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); + let a = addr.to_string(); + + for bad in ["send 1 ETH\u{202E}drow", "zero\u{200B}width", "nul\u{0}byte"] { + assert!(ks.sign_message(&a, "pw", bad).is_err(), "must refuse {bad:?}"); + } + // Ordinary text, including newlines, still signs. + assert!(ks.sign_message(&a, "pw", "hello\nworld").is_ok()); + // And an oversize message is refused rather than signed unseen. + let huge = "a".repeat(MAX_MESSAGE_BYTES + 1); + assert!(ks.sign_message(&a, "pw", &huge).is_err()); + } + + #[test] + fn hostile_kdf_params_are_rejected_before_any_derivation() { + // n = u32::MAX asked scrypt for a ~4 TiB allocation, aborting the + // process from a single caller-supplied file. + let hostile = r#"{"version":3,"crypto":{"kdf":"scrypt","ciphertext":"00","cipher":"aes-128-ctr", + "cipherparams":{"iv":"00"},"mac":"00", + "kdfparams":{"n":4294967295,"r":8,"p":1,"dklen":32,"salt":"00"}}}"#; + // u32::MAX is not a power of two, so it is caught by that rule first. + let e = check_kdf_params(hostile).unwrap_err(); + assert!(format!("{e}").contains("power of two"), "got {e}"); + + // A power of two that is merely far too large hits the size rule, which + // is the one that stops the enormous allocation. + let huge = hostile.replace("4294967295", "1073741824"); // 2^30 + let e = check_kdf_params(&huge).unwrap_err(); + assert!(format!("{e}").contains("exceeds") || format!("{e}").contains("bytes"), "got {e}"); + + // Oversized r is refused too. + let big_r = hostile.replace("4294967295", "262144").replace("\"r\":8", "\"r\":64"); + assert!(check_kdf_params(&big_r).is_err()); + + // A standard vault passes. + let ok = hostile.replace("4294967295", "262144"); + assert!(check_kdf_params(&ok).is_ok()); + } + + #[cfg(unix)] + #[test] + fn the_vault_directory_and_files_are_not_group_or_world_readable() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("vaults"); + let ks = Keystore::new(&sub); + let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); + + let dmode = std::fs::metadata(&sub).unwrap().permissions().mode() & 0o777; + assert_eq!(dmode, 0o700, "vault dir was {dmode:o}"); + let vault = sub.join(format!("{:x}.json", addr)); + let fmode = std::fs::metadata(&vault).unwrap().permissions().mode() & 0o777; + assert_eq!(fmode, 0o600, "vault file was {fmode:o}"); + } + + #[test] + fn importing_a_vault_leaves_no_temp_copy_behind() { + let src = tempfile::tempdir().unwrap(); + let ks_src = Keystore::new(src.path()); + let addr = ks_src.import_private_key(ACCT0_PK, "pw").unwrap(); + let json = ks_src.export_keystore_json(&addr.to_string(), "pw").unwrap(); + + let before = temp_import_files(); + let dst = tempfile::tempdir().unwrap(); + Keystore::new(dst.path()).import_keystore_json(&json, "pw", "pw2").unwrap(); + let after = temp_import_files(); + assert_eq!(before, after, "import left a copy of the vault in the temp dir"); + } + + fn temp_import_files() -> Vec { + let mut v: Vec<_> = std::fs::read_dir(std::env::temp_dir()) + .into_iter() + .flatten() + .flatten() + .map(|e| e.file_name()) + .filter(|n| n.to_string_lossy().starts_with("logos-ks-import-")) + .collect(); + v.sort(); + v + } + #[test] fn hd_derivation_matches_known_vector() { let signer = signer_from_mnemonic(TEST_MNEMONIC, "", 0).unwrap(); @@ -469,27 +845,24 @@ mod tests { #[test] fn vault_roundtrip_and_listing() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); assert_eq!(addr, ACCT0); assert!(ks.has_address(&addr.to_string())); assert_eq!(ks.list_accounts(), vec![ACCT0]); - // wrong password fails, correct password unlocks - assert!(ks.unlock(&addr.to_string(), "wrong", None).is_err()); - ks.unlock(&addr.to_string(), "pw", None).unwrap(); - assert!(ks.is_unlocked(&addr.to_string())); - ks.lock(&addr.to_string()); - assert!(!ks.is_unlocked(&addr.to_string())); + // The vault password is the gate on every signature — there is no + // unlocked state to be in. + assert!(ks.signer_for(&addr.to_string(), "wrong").is_err()); + assert_eq!(ks.signer_for(&addr.to_string(), "pw").unwrap().address(), ACCT0); } #[test] fn sign_message_recovers_signer() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); - ks.unlock(&addr.to_string(), "pw", None).unwrap(); - let sig_hex = ks.sign_message(&addr.to_string(), "hello logos").unwrap(); + let sig_hex = ks.sign_message(&addr.to_string(), "pw", "hello logos").unwrap(); let sig: alloy::primitives::Signature = sig_hex.strip_prefix("0x").unwrap().parse::().unwrap(); let recovered = sig.recover_address_from_msg("hello logos").unwrap(); @@ -497,47 +870,45 @@ mod tests { } #[test] - fn locked_account_cannot_sign() { + fn a_wrong_password_cannot_sign() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); - assert!(matches!(ks.sign_message(&addr.to_string(), "x"), Err(KeystoreError::Locked(_)))); + assert!(ks.sign_message(&addr.to_string(), "wrong", "x").is_err()); } #[test] fn sign_digest_recovers_signer_from_prehash() { use alloy::primitives::b256; let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); - ks.unlock(&addr.to_string(), "pw", None).unwrap(); // A raw 32-byte digest (e.g. an ERC-4337 UserOperation hash) — signed with // no prefix, so it recovers via the prehash (not the EIP-191 msg) path. let digest = b256!("00000000000000000000000000000000000000000000000000000000deadbeef"); - let sig_hex = ks.sign_digest(&addr.to_string(), &digest.to_string()).unwrap(); + let sig_hex = ks.sign_digest(&addr.to_string(), "pw", &digest.to_string()).unwrap(); let sig: alloy::primitives::Signature = sig_hex.strip_prefix("0x").unwrap().parse().unwrap(); assert_eq!(sig.recover_address_from_prehash(&digest).unwrap(), ACCT0); // Bare (no 0x) hex also accepted; wrong length rejected. - assert!(ks.sign_digest(&addr.to_string(), &hex::encode(digest)).is_ok()); - assert!(ks.sign_digest(&addr.to_string(), "0x1234").is_err()); + assert!(ks.sign_digest(&addr.to_string(), "pw", &hex::encode(digest)).is_ok()); + assert!(ks.sign_digest(&addr.to_string(), "pw", "0x1234").is_err()); } #[test] - fn sign_digest_requires_unlock() { + fn sign_digest_requires_the_vault_password() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); let digest = "0x00000000000000000000000000000000000000000000000000000000deadbeef"; - assert!(matches!(ks.sign_digest(&addr.to_string(), digest), Err(KeystoreError::Locked(_)))); + assert!(ks.sign_digest(&addr.to_string(), "wrong", digest).is_err()); } #[test] fn sign_eip1559_recovers_signer() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); - ks.unlock(&addr.to_string(), "pw", None).unwrap(); let unsigned = serde_json::json!({ "to": "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", "value": "0xde0b6b3a7640000", @@ -548,7 +919,7 @@ mod tests { "fee_mode": "eip1559" }) .to_string(); - let raw = ks.sign_transaction(&addr.to_string(), &unsigned, 1).unwrap(); + let raw = ks.sign_transaction(&addr.to_string(), "pw", &unsigned, 1).unwrap(); assert!(raw.starts_with("0x02")); // typed EIP-1559 envelope // decode + recover let bytes = hex::decode(raw.strip_prefix("0x").unwrap()).unwrap(); @@ -559,9 +930,8 @@ mod tests { #[test] fn sign_legacy_recovers_signer() { let dir = tempfile::tempdir().unwrap(); - let mut ks = Keystore::new(dir.path()); + let ks = Keystore::new(dir.path()); let addr = ks.import_private_key(ACCT0_PK, "pw").unwrap(); - ks.unlock(&addr.to_string(), "pw", None).unwrap(); let unsigned = serde_json::json!({ "to": "0x70997970C51812dc3A010C7d01b50e0d17dc79C8", "value": "0x1", @@ -571,7 +941,7 @@ mod tests { "fee_mode": "legacy" }) .to_string(); - let raw = ks.sign_transaction(&addr.to_string(), &unsigned, 1).unwrap(); + let raw = ks.sign_transaction(&addr.to_string(), "pw", &unsigned, 1).unwrap(); let bytes = hex::decode(raw.strip_prefix("0x").unwrap()).unwrap(); let env = TxEnvelope::decode_2718(&mut bytes.as_slice()).unwrap(); assert_eq!(env.recover_signer().unwrap(), ACCT0); diff --git a/rust-lib/src/lib.rs b/rust-lib/src/lib.rs index 024bfb6..aae21d5 100644 --- a/rust-lib/src/lib.rs +++ b/rust-lib/src/lib.rs @@ -16,3 +16,5 @@ pub use keystore::{Keystore, KeystoreError}; // via the default `logos_module` feature. #[cfg(feature = "logos_module")] mod glue; + +pub mod approval;