From 5147cb18f0187ebff08a21fd911a2c0fdf10663a Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 17 May 2026 20:00:25 -0500 Subject: [PATCH 1/2] feat(cli): --signing-key-file flag overrides signing identity per command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a top-level `--signing-key-file ` flag (with `RIVER_SIGNING_KEY_FILE` env var fallback) that reads a raw 32-byte Ed25519 secret key and uses it in place of the room's stored `signing_key_bytes` for the current command. The override is in-memory only; `rooms.json` on disk is never modified. # Why `rooms.json` only stores ONE `signing_key_bytes` per room, but this machine often has multiple identities for the same room (room owner, invite bot, alt accounts). The UI's chat-delegate sync periodically overwrites `rooms.json[room].signing_key_bytes` with whatever the delegate has stored — silently leaving owner ops broken without a manual swap of the on-disk key. Pattern as documented in `river-official-room` skill: swap key → run command → optionally swap back. Fragile and recurring. The flag formalizes the "I have multiple identities, pick at command time" model: nominate the right identity per command, no rooms.json mutation, the existing identity stays loaded. Distinct from `message send --signing-key` which takes a base64- encoded inline key — the global file-based flag is preferred for non-test use because the key doesn't appear in shell history. # Tests - `signing_key_override_is_returned_and_not_persisted` in `cli/src/storage.rs` pins the contract: with override, `get_room` returns the override; without override (fresh Storage), the ORIGINAL stored bytes come back — proving the override is not written back to rooms.json. # Manual verification Sent the river#275/#276/#278 release announcement to the official Freenet River room as Room Owner via `--signing-key-file ~/.config/freenet-river-official/room_owner_signing_key.bin`, no rooms.json swap. Confirmed via `riverctl message list` that the message landed signed by Room Owner. # Follow-up #281 tracks the "perfect world" decentralized-version-pointer follow-up (a `freenet-updates` crate that uses a Freenet contract for tooling version checks). Separate PR. Co-Authored-By: Claude Opus 4.7 --- cli/src/api.rs | 20 ++++++++- cli/src/main.rs | 68 +++++++++++++++++++++++++++- cli/src/storage.rs | 107 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 190 insertions(+), 5 deletions(-) diff --git a/cli/src/api.rs b/cli/src/api.rs index 6aeb5c93..7767f5ab 100644 --- a/cli/src/api.rs +++ b/cli/src/api.rs @@ -64,6 +64,20 @@ impl ApiClient { } pub async fn new(node_url: &str, config: Config, config_dir: Option<&str>) -> Result { + Self::new_with_signing_key_override(node_url, config, config_dir, None).await + } + + /// Construct an [`ApiClient`] with an optional in-memory signing-key + /// override. The override is propagated to [`Storage`] so every + /// `get_room` resolves the signing key from the override rather than + /// the per-room `signing_key_bytes`. See [`Storage::signing_key_override`] + /// for the motivating scenario. + pub async fn new_with_signing_key_override( + node_url: &str, + config: Config, + config_dir: Option<&str>, + signing_key_override: Option, + ) -> Result { // Use the URL as provided - it should already be in the correct format info!("Connecting to Freenet node at: {}", node_url); @@ -77,7 +91,7 @@ impl ApiClient { // Create WebApi instance let web_api = WebApi::start(ws_stream); - let storage = Storage::new(config_dir)?; + let storage = Storage::new_with_override(config_dir, signing_key_override)?; Ok(Self { web_api: Arc::new(Mutex::new(web_api)), @@ -866,7 +880,9 @@ impl ApiClient { } }; - let signing_key = SigningKey::from_bytes(&room_info.signing_key_bytes); + let signing_key = self + .storage + .resolve_signing_key(&room_info.signing_key_bytes); let room_state = room_info.state.clone(); let previous_contract_key_str = room_info.previous_contract_key.clone(); diff --git a/cli/src/main.rs b/cli/src/main.rs index 3f5e90b6..30d1aca1 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -1,5 +1,6 @@ -use anyhow::{Context, Result}; +use anyhow::{anyhow, Context, Result}; use clap::{Parser, Subcommand}; +use ed25519_dalek::SigningKey; use std::path::{Path, PathBuf}; use tracing::info; use tracing_appender::non_blocking::WorkerGuard; @@ -43,6 +44,33 @@ struct Cli { /// Optional path to write log output (stdout remains reserved for command output/JSON) #[arg(long, global = true, value_name = "PATH", env = "RIVERCTL_LOG_FILE")] log_file: Option, + + /// Override the signing identity for this command only. Reads a raw + /// 32-byte Ed25519 secret key from the given file path. + /// + /// The override is in-memory: it does NOT modify `rooms.json`. Use + /// this when you have multiple identities in the same room (e.g., + /// room owner + invite bot + alt accounts) and want to pick which + /// one signs at command time, without the UI's chat-delegate sync + /// silently rewriting `rooms.json[room].signing_key_bytes` away + /// from your intended identity. The override key must be a valid + /// member of the target room or the command will be rejected by + /// the contract. + /// + /// Falls back to the `RIVER_SIGNING_KEY_FILE` env var if the flag + /// is not passed. + /// + /// Distinct from `message send --signing-key`, which takes a + /// base64-encoded key inline as a single-command override — the + /// global `--signing-key-file` flag is preferred for non-test use + /// because the key doesn't appear in shell history. + #[arg( + long, + global = true, + value_name = "PATH", + env = "RIVER_SIGNING_KEY_FILE" + )] + signing_key_file: Option, } #[derive(Subcommand)] @@ -96,8 +124,21 @@ async fn main() -> Result<()> { // Load configuration let config = config::Config::load()?; + // Resolve optional --signing-key-file override (or RIVER_SIGNING_KEY_FILE env var). + let signing_key_override = cli + .signing_key_file + .as_deref() + .map(load_signing_key_from_file) + .transpose()?; + // Create API client - let api_client = api::ApiClient::new(&cli.node_url, config, cli.config_dir.as_deref()).await?; + let api_client = api::ApiClient::new_with_signing_key_override( + &cli.node_url, + config, + cli.config_dir.as_deref(), + signing_key_override, + ) + .await?; // Execute command match cli.command { @@ -115,6 +156,29 @@ async fn main() -> Result<()> { Ok(()) } +/// Load a raw 32-byte Ed25519 secret key from the given file path. +/// Used by the `--signing-key-file` flag / `RIVER_SIGNING_KEY_FILE` env var. +/// Errors are surfaced with a clear message identifying the bad path +/// and the actual length seen, so the user can tell "I pointed at the +/// wrong file" from "I pointed at a base64-encoded file". +fn load_signing_key_from_file(path: &Path) -> Result { + let bytes = std::fs::read(path) + .with_context(|| format!("failed to read signing key file: {}", path.display()))?; + if bytes.len() != 32 { + return Err(anyhow!( + "signing key file {} must be exactly 32 raw bytes, got {} bytes \ + (was this file base64- or hex-encoded? the override expects raw bytes, \ + matching the format written by `riverctl identity export` and the keys \ + in ~/.config/freenet-river-official/*.bin)", + path.display(), + bytes.len() + )); + } + let mut buf = [0u8; 32]; + buf.copy_from_slice(&bytes); + Ok(SigningKey::from_bytes(&buf)) +} + fn init_logging(debug: bool, log_path: Option<&Path>) -> Result> { use std::fs::OpenOptions; use tracing_subscriber::{fmt, layer::SubscriberExt, Registry}; diff --git a/cli/src/storage.rs b/cli/src/storage.rs index 6ef2661c..93333115 100644 --- a/cli/src/storage.rs +++ b/cli/src/storage.rs @@ -43,10 +43,33 @@ pub struct Storage { /// Side file so the larger `rooms.json` blob stays untouched on /// each DM send. JSON-serialized [`OutboundDmStore`]. outbound_dms_path: PathBuf, + /// In-memory signing-key override (from `--signing-key` flag or + /// `RIVER_SIGNING_KEY` env var). When set, every call to + /// [`Storage::get_room`] returns this key in place of the room's + /// stored `signing_key_bytes`. Never written back to disk — the + /// override is a per-command-invocation thing. + /// + /// Motivates: this machine often has multiple identities for the + /// same room (room owner, invite bot, test alts), but `rooms.json` + /// only stores ONE `signing_key_bytes` per room. The UI's chat- + /// delegate sync can silently rewrite that field — leaving owner + /// ops broken without a manual swap. The override lets owner ops + /// nominate the right identity at command time without touching + /// `rooms.json`. See discussion on river#281. + signing_key_override: Option, } impl Storage { pub fn new(config_dir: Option<&str>) -> Result { + Self::new_with_override(config_dir, None) + } + + /// Construct a [`Storage`] with an optional in-memory signing-key + /// override. See the field doc on [`Storage::signing_key_override`]. + pub fn new_with_override( + config_dir: Option<&str>, + signing_key_override: Option, + ) -> Result { // Use provided config_dir, then check environment variable, then use default let data_dir = if let Some(dir) = config_dir { PathBuf::from(dir) @@ -67,9 +90,23 @@ impl Storage { Ok(Self { storage_path, outbound_dms_path, + signing_key_override, }) } + /// Resolve the signing key to use for the current command: prefer + /// the in-memory override if set, otherwise reconstruct from the + /// per-room `signing_key_bytes`. Used by both [`Storage::get_room`] + /// and [`crate::api::ApiClient::ensure_room_migrated`] (which has + /// its own load_rooms snapshot for migration purposes). + pub fn resolve_signing_key(&self, stored_bytes: &[u8; 32]) -> SigningKey { + if let Some(override_key) = &self.signing_key_override { + override_key.clone() + } else { + SigningKey::from_bytes(stored_bytes) + } + } + pub fn load_rooms(&self) -> Result { if !self.storage_path.exists() { return Ok(RoomStorage::default()); @@ -183,7 +220,7 @@ impl Storage { let owner_key_str = bs58::encode(owner_vk.as_bytes()).into_string(); if let Some(room_info) = storage.rooms.get(&owner_key_str) { - let signing_key = SigningKey::from_bytes(&room_info.signing_key_bytes); + let signing_key = self.resolve_signing_key(&room_info.signing_key_bytes); Ok(Some(( signing_key, room_info.state.clone(), @@ -470,4 +507,72 @@ mod tests { "previous_contract_key should be None when WASM hasn't changed" ); } + + /// `--signing-key` / `RIVER_SIGNING_KEY` override: `Storage::get_room` + /// must return the override key in place of the room's stored + /// `signing_key_bytes`, AND the override must NOT be written back to + /// `rooms.json`. This pins the "in-memory only" contract documented + /// on `Storage::signing_key_override` — the persistent on-disk + /// identity must be untouched, so subsequent invocations without + /// the override revert to the stored identity. + #[test] + fn signing_key_override_is_returned_and_not_persisted() { + let temp_dir = TempDir::new().unwrap(); + let stored_sk = create_test_signing_key(); + let override_sk = create_test_signing_key(); + assert_ne!( + stored_sk.to_bytes(), + override_sk.to_bytes(), + "test invariant: stored and override keys must differ" + ); + let owner_vk = stored_sk.verifying_key(); + + // Set up storage with the stored identity (no override). + let storage_no_override = Storage::new(Some(temp_dir.path().to_str().unwrap())).unwrap(); + let state = create_test_state(&stored_sk); + let initial_key = expected_contract_key(&owner_vk); + storage_no_override + .add_room(&owner_vk, &stored_sk, state, &initial_key) + .unwrap(); + + // Sanity: without override, get_room returns the stored key. + let (sk_no_override, _, _) = storage_no_override + .get_room(&owner_vk) + .unwrap() + .expect("room present"); + assert_eq!( + sk_no_override.to_bytes(), + stored_sk.to_bytes(), + "no override → stored key" + ); + + // With override, get_room returns the override. + let storage_with_override = Storage::new_with_override( + Some(temp_dir.path().to_str().unwrap()), + Some(override_sk.clone()), + ) + .unwrap(); + let (sk_with_override, _, _) = storage_with_override + .get_room(&owner_vk) + .unwrap() + .expect("room present"); + assert_eq!( + sk_with_override.to_bytes(), + override_sk.to_bytes(), + "override → override key returned" + ); + + // Critical: rooms.json on disk is untouched. A fresh Storage + // without the override must see the ORIGINAL stored bytes. + let storage_fresh = Storage::new(Some(temp_dir.path().to_str().unwrap())).unwrap(); + let (sk_fresh, _, _) = storage_fresh + .get_room(&owner_vk) + .unwrap() + .expect("room present"); + assert_eq!( + sk_fresh.to_bytes(), + stored_sk.to_bytes(), + "override must NOT be written back to rooms.json" + ); + } } From dd2f26893ae2e7a1c44faf98efcf41225b48a689 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Mon, 18 May 2026 09:40:33 -0500 Subject: [PATCH 2/2] fix(cli): address PR #282 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skeptical HIGH-1: identity export under `--signing-key-file` could produce a wire-format-corrupted token. When the override resolves to a different identity than the cached `self_authorized_member` in rooms.json, the bundled IdentityExport had a signing_key whose verifying_key() did NOT match authorized_member.member.member_vk. Importing such a token would silently fail every subsequent contract operation. Fix: validate `signing_key.verifying_key() == authorized_member.member.member_vk` just before constructing IdentityExport; error out cleanly with a message pointing at the override as the likely cause if they disagree. The owner-self-signed path (line 74-81) and the network-lookup path (line 82-113) already produce coherent pairs; only the cached-from-disk path could mismatch. Code-first/skeptical Important: doc-comment drift on `Storage::signing_key_override` field said `--signing-key` / `RIVER_SIGNING_KEY` but the actual flag/env are `--signing-key-file` / `RIVER_SIGNING_KEY_FILE`. Fixed both the field doc and the test docstring so a future grep for the right names actually hits this code. Code-first Important: the file-loader error message referenced `riverctl identity export` as if it produced raw 32-byte output. It doesn't — it produces an armored multi-field token. Clarified the error to point at the room-key .bin backup format (which IS raw 32 bytes) and explicitly contrast with `identity export`'s armored shape. Skeptical Important #5: added unit tests for the file loader's length validation. Extracted `parse_signing_key_bytes` as a pure helper so the tests don't need a tempfile. Tests cover: 32-byte accept, short reject (with byte-count in message), long reject (with base64 hint in message), empty reject. Other Important items (skeptical #2 info-log on override, #3 file-permissions warning, #5 env-vs-flag precedence test) are documented for follow-up but not blocking — the override-mismatch fix is the load-bearing correctness item from this review round. Co-Authored-By: Claude Opus 4.7 --- cli/src/commands/identity.rs | 23 ++++++++++++++ cli/src/main.rs | 60 +++++++++++++++++++++++++++++++----- cli/src/storage.rs | 6 ++-- 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/cli/src/commands/identity.rs b/cli/src/commands/identity.rs index 6905b4f1..3ace4344 100644 --- a/cli/src/commands/identity.rs +++ b/cli/src/commands/identity.rs @@ -133,6 +133,29 @@ async fn export_identity( Err(_) => (None, None), // Network unavailable; export without extras }; + // Wire-format safety check. The export's signing_key MUST match the + // authorized_member.member.member_vk; otherwise importing the token + // produces an identity whose secret key signs nothing the room + // contract accepts. + // + // This catches the case where `--signing-key-file` overrides the + // signing identity but `self_authorized_member` is read from + // `rooms.json` (which still has the previous identity's + // `AuthorizedMember`). The two pieces would otherwise be packaged + // together with mismatched verifying keys, silently breaking the + // imported identity on first use. + if signing_key.verifying_key() != authorized_member.member.member_vk { + return Err(anyhow!( + "Refusing to export an identity with mismatched signing key and \ + authorized member. The signing key's verifying key does not match \ + the cached AuthorizedMember.member_vk for this room. This usually \ + happens when `--signing-key-file` / `RIVER_SIGNING_KEY_FILE` overrides \ + the signing identity but `rooms.json` still holds another identity's \ + cached membership state. Re-run without the override (or with the \ + override pointing at THIS identity) to produce a coherent token." + )); + } + let export = IdentityExport { room_owner: room_owner_key, signing_key, diff --git a/cli/src/main.rs b/cli/src/main.rs index 30d1aca1..a65f777a 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -164,21 +164,67 @@ async fn main() -> Result<()> { fn load_signing_key_from_file(path: &Path) -> Result { let bytes = std::fs::read(path) .with_context(|| format!("failed to read signing key file: {}", path.display()))?; + parse_signing_key_bytes(&bytes) + .map_err(|reason| anyhow!("{} — file: {}", reason, path.display())) +} + +/// Pure helper for testing the wrong-length / right-length validation +/// without touching the filesystem. +fn parse_signing_key_bytes(bytes: &[u8]) -> std::result::Result { if bytes.len() != 32 { - return Err(anyhow!( - "signing key file {} must be exactly 32 raw bytes, got {} bytes \ - (was this file base64- or hex-encoded? the override expects raw bytes, \ - matching the format written by `riverctl identity export` and the keys \ - in ~/.config/freenet-river-official/*.bin)", - path.display(), + return Err(format!( + "signing key must be exactly 32 raw bytes, got {} bytes \ + (was this file base64- or hex-encoded? the override expects raw \ + bytes — the same format as the room-key backups under \ + ~/.config/freenet-river-official/*.bin; NOT the armored output of \ + `riverctl identity export`, which is a larger multi-field token)", bytes.len() )); } let mut buf = [0u8; 32]; - buf.copy_from_slice(&bytes); + buf.copy_from_slice(bytes); Ok(SigningKey::from_bytes(&buf)) } +#[cfg(test)] +mod cli_tests { + use super::*; + + #[test] + fn parse_signing_key_bytes_accepts_32_byte_input() { + let raw = [7u8; 32]; + let sk = parse_signing_key_bytes(&raw).expect("32 raw bytes is valid"); + assert_eq!(sk.to_bytes(), raw); + } + + #[test] + fn parse_signing_key_bytes_rejects_short_input() { + let raw = [7u8; 16]; + let err = parse_signing_key_bytes(&raw).expect_err("must reject short input"); + assert!(err.contains("32 raw bytes"), "msg: {}", err); + assert!(err.contains("16 bytes"), "msg: {}", err); + } + + #[test] + fn parse_signing_key_bytes_rejects_long_input() { + // 44-byte base64-encoded 32-byte key (the most common user mistake) + let raw = [b'a'; 44]; + let err = parse_signing_key_bytes(&raw).expect_err("must reject long input"); + assert!( + err.contains("base64"), + "must hint at base64 mistake: {}", + err + ); + } + + #[test] + fn parse_signing_key_bytes_rejects_empty() { + let raw: [u8; 0] = []; + let err = parse_signing_key_bytes(&raw).expect_err("must reject empty input"); + assert!(err.contains("0 bytes"), "msg: {}", err); + } +} + fn init_logging(debug: bool, log_path: Option<&Path>) -> Result> { use std::fs::OpenOptions; use tracing_subscriber::{fmt, layer::SubscriberExt, Registry}; diff --git a/cli/src/storage.rs b/cli/src/storage.rs index 93333115..1fdf2659 100644 --- a/cli/src/storage.rs +++ b/cli/src/storage.rs @@ -43,8 +43,8 @@ pub struct Storage { /// Side file so the larger `rooms.json` blob stays untouched on /// each DM send. JSON-serialized [`OutboundDmStore`]. outbound_dms_path: PathBuf, - /// In-memory signing-key override (from `--signing-key` flag or - /// `RIVER_SIGNING_KEY` env var). When set, every call to + /// In-memory signing-key override (from `--signing-key-file` flag or + /// `RIVER_SIGNING_KEY_FILE` env var). When set, every call to /// [`Storage::get_room`] returns this key in place of the room's /// stored `signing_key_bytes`. Never written back to disk — the /// override is a per-command-invocation thing. @@ -508,7 +508,7 @@ mod tests { ); } - /// `--signing-key` / `RIVER_SIGNING_KEY` override: `Storage::get_room` + /// `--signing-key-file` / `RIVER_SIGNING_KEY_FILE` override: `Storage::get_room` /// must return the override key in place of the room's stored /// `signing_key_bytes`, AND the override must NOT be written back to /// `rooms.json`. This pins the "in-memory only" contract documented