feat(common): deterministic room secret derivation#233
Merged
Conversation
Add `key_derivation::derive_room_secret`: a pure helper that turns
(signing key seed, owner verifying key, secret version) into a 32-byte
room secret using a blake3 keyed hash with a hard-coded domain
separator (`river-rotate-v1`).
Construction:
blake3::keyed_hash(
key = signing_key_seed,
data = b"river-rotate-v1" || owner_vk || version_le_u32,
)
Tests cover: determinism, version separation, owner separation, key
separation, and a hard-coded known-answer vector that locks the
construction so any accidental change to the algorithm fails CI.
This function is intentionally unused in this PR. PR 2 of
#228 will wire it into the chat delegate so multiple
replicas of the same delegate (e.g. one user on multiple devices)
compute byte-identical room secrets without coordination, which is the
prerequisite for re-enabling private rooms with delegate-driven secret
rotation.
No delegate or contract WASM changes; baseline hashes preserved.
Refs #228
[AI-assisted - Claude]
Address review findings on the initial PR commit (7187516): - Switch construction to two-phase blake3 KDF (`derive_key(ROOT_CONTEXT, seed)` then `keyed_hash(root, vk || ver)`). Separates protocol-version commitment from per-call inputs; future input additions are confined to phase 2 and require bumping the hard-coded `ROOT_CONTEXT` string. Closes the concatenation-fragility concern raised by code-first and skeptical reviewers. - Document the `signing_key_seed` invariant explicitly: must be the raw 32-byte ed25519 seed (not the expanded 64-byte secret, not random bytes). Closes the "any 32 bytes accepted silently" concern. - Document the security trade-off: anyone with the seed can derive past and future secrets; this is the conscious price for multi-device determinism without coordination, and is bounded because the signing key is already terminal. Closes the threat-model-not-in-docstring concern. - Add cross-axis swap test (`derive_locks_input_ordering`) to lock the order of `update(owner_vk)` vs `update(version_le)`. - Add two more known-answer vectors: - multi-byte version `0x01020304` (locks `to_le_bytes` choice; a `to_be_bytes` regression would diverge here) - all-`0xFF` seed (exercises non-zero key bytes; catches a buggy "zero-seed → fall back to unkeyed" regression) - Document an external verification recipe (python `blake3` snippet) in the KAT comment so the vectors can be regenerated independently without re-running the implementation under test. - Add a TODO marker in `ui/src/room_data.rs:441` (`rotate_secret`) so the PR 2 swap site is self-documenting. Tests: 8 passed (was 5), all `key_derivation` tests including new swap test and two new KATs. Full `cargo test -p river-core` clean. `cargo clippy -p river-core --all-targets --all-features` clean. Construction change updates all KAT vectors. WASM hashes still match HEAD (function remains dead code; no migration entry needed). Refs #228 [AI-assisted - Claude]
sanity
added a commit
that referenced
this pull request
May 12, 2026
#235) * feat: move private-room secret rotation into chat delegate (#228 PR 2) Move the private-room secret-rotation pipeline from the UI sync loop into the chat delegate so multi-device replicas converge without coordination. PR 1 (#233) added `derive_room_secret`; this wires it in delegate-side and removes the three UI rotation triggers. ## Approach - Add `EnsureRoomSubscription { room_owner_vk, contract_id }` to `ChatDelegateRequestMsg`. The UI fires this for every owner-mode room after loading from delegate storage. The delegate emits a `SubscribeContractRequest` to the runtime and persists per-room bookkeeping (sub index, contract->room reverse index, signing-key origin) to its secret store. - Handle `InboundDelegateMsg::ContractNotification` in the delegate. When a subscribed room's state changes, the delegate decodes the new state, compares the current member set against the cached last-seen set, and rotates the secret to `current_version + 1` via the deterministic `derive_room_secret` helper. The rotation produces a `SecretsDelta` (signed externally with the room owner's signing key via `AuthorizedSecretVersionRecord::with_signature` / `AuthorizedEncryptedSecretForMember::with_signature`) wrapped in a `ChatRoomStateV1Delta` and shipped via `UpdateContractRequest`. - Extract the ECIES helpers from `ui/src/util/ecies.rs` into `river-core::ecies` behind a new `ecies` Cargo feature (off by default so room-contract WASM stays small). UI and chat-delegate enable the feature; UI's `ecies.rs` is now a thin re-export. - Remove the three UI rotation triggers: - manual `Rotate` button in `edit_room_modal.rs` - on-ban rotation in `ban_button.rs` - weekly rotation in `sync_info.rs` (already commented out) - Mark `RoomData::rotate_secret` and `needs_secret_rotation` as `#[deprecated]` (kept as `pub` API; removing would have wider blast radius). ## Migration - V11 entry added to `legacy_delegates.toml` capturing the OLD chat-delegate hash (`05bc461d…`) so users don't lose room data on the delegate-key change. NEW hash: `44d0025e…`. ## Out of scope (later phases) - Weekly rotation via ScheduleWakeup (waits on freenet-core#3972). - Re-enabling private rooms in the UI. - CRDT canonical merge for SecretVersionRecordV1 duplicate-version resolution. Closes #228 (partial) [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address PR #235 review findings (P1 regressions + high/medium) Reviewers (code-first, testing, skeptical, big-picture, codex) flagged 3 P1 regressions and several high/medium issues against e7739ec. This commit addresses all 13 findings without rebasing or amending the prior commit. Architectural correction ======================== The original PR removed all three UI rotation triggers (manual, on-ban, weekly-sync) on the assumption the chat delegate would handle every case. That was wrong: bans and manual rotations are owner-driven UI events, and making them async via ContractNotification round-trip leaks the owner's next message to the just-banned member before rotation lands. Weekly-sync rotation, however, was correctly removed (it required the UI open, which defeated the point). Corrected strategy: UI handles "owner just clicked something" cases synchronously; delegate handles "background, owner not actively in UI" catch-up. Both use `derive_room_secret` so concurrent rotation produces byte-identical secrets and converges via the contract's duplicate-version dedup at common/src/room_state/secret.rs:140-145. P1 fixes ======== 1. Restore synchronous UI-side rotation for ban + manual. - room_data.rs::rotate_secret un-deprecated, switched to `key_derivation::derive_room_secret(&self_sk.to_bytes(), &owner_vk, new_version)` for deterministic derivation. - ban_button.rs: re-fires rotation synchronously after the ban delta applies (private rooms only). - edit_room_modal.rs: manual "Rotate" button restored (owner-only, private-only). - sync_info.rs weekly-sync trigger NOT restored — removal was correct. 2. Subscribe newly-created rooms to the delegate. - create_room_modal.rs now fires `ensure_room_subscription_once` after `store_signing_key` completes. - chat_delegate.rs adds `ensure_room_subscription_once` with a per-session dedup HashSet so re-loads of `rooms_data` don't spam EnsureRoomSubscription. 3. Cache-before-success bug fixed. - subscription.rs::handle_contract_notification now updates the member-set cache only AFTER the rotation pipeline has built the UpdateContractRequest. Any earlier failure (signing key missing, encode error, version overflow) leaves the cache untouched so the next notification retries the rotation. High fixes ========== 4. CBOR canonicality verified (no wire-format break). - Investigation: ciborium IS deterministic for the specific structs (`SecretVersionRecordV1`, `EncryptedSecretForMemberV1`, BTreeSet of fixed-length MemberIds) — they only contain fixed-order named fields with primitive types or fixed-length byte arrays. CBOR has explicit length prefixes, ciborium iterates BTreeSet in key order. - Decision: keep ciborium, document the property with regression tests rather than break wire format. New tests `concurrent_rotations_produce_identical_signed_records` and `ciborium_serialization_is_deterministic_for_signed_structs` pin determinism for these specific structs over 100 randomized trials each. - Avoids room-contract WASM hash break and the associated migration. 5. EnsureRoomSubscription probes for signing key (WASM-only). - Returns "no signing key on file — call StoreSigningKey first" if absent. Gated on `target_family = "wasm"` because non-WASM `DelegateCtx::get_secret` always returns None. - create_room_modal.rs orders StoreSigningKey before EnsureRoomSubscription to satisfy this. 6. Reverse-index format consistency. - subscription.rs reverse index now CBOR-encoded (was raw [u8;32]). - Decode uses cbor_decode + try_from rather than length-checked copy, eliminating the panic risk on length mismatch (Fix 10). Medium fixes ============ 7. safe_spawn_local everywhere. - response_handler.rs's three direct spawn_local calls (legacy migration, signing-key migration, delegate subscription) converted. Per AGENTS.md "Dioxus WASM Signal Safety", direct spawn_local from inside a polled future causes RefCell re-entrancy panics on Firefox mobile. 8. EnsureRoomSubscription dedup per session. - chat_delegate.rs ENSURE_SUBSCRIPTION_SENT HashSet<RoomKey>. 9. Bail when current_version == u32::MAX. - Both subscription.rs (delegate) and room_data.rs (UI) refuse to wrap. Tests `rotation_bails_at_max_version` and `ui_rotation_bails_at_max_version` lock this behaviour. 10. Defensive try_into instead of length-checked copy_from_slice. - subscription.rs uses TryFrom for the [u8; 32] reconstructions on contract_id_index_key, signing_key_secret_key. On length mismatch we log and return Ok(vec![]) rather than risk a panic. 11. AGENTS.md updated. - Section "Private Room Support" now describes the two-path rotation model, removes the stale weekly-sync claim, lists key files including key_derivation.rs and subscription.rs. 12. Tests added. - chat-delegate: 16 → 20 tests - concurrent_rotations_produce_identical_signed_records - ciborium_serialization_is_deterministic_for_signed_structs - rotation_bails_at_max_version - reverse_index_uses_cbor_encoding - room_data.rs: 12 → 15 tests - ui_rotation_uses_deterministic_derivation - ui_and_delegate_rotation_produce_identical_secrets - ui_rotation_bails_at_max_version - Documented testing-harness gaps in subscription/tests.rs module docstring (mock DelegateCtx requires upstream stdlib changes). 13. created_at comment tightened in subscription.rs to drop the misleading "UI replicas... canonical SecretVersionRecordV1 produced by the contract's view" sentence. Test results ============ Workspace: 305 tests pass, 0 fail. - chat-delegate: 16 → 20 passed - river-ui (bin): 75 passed (was 72) - river-core: 145 passed (unchanged, plus 1 new doc test) cargo fmt and cargo clippy on touched packages clean. Note on WASM ============ Source code in delegates/chat-delegate has changed but ui/public/contracts/chat_delegate.wasm is unchanged in this commit (per the .claude/rules/wasm-safety.md "never `git add -A`" rule). Before deploying this v2, run `cargo make add-migration && cargo make sync-wasm` to record the old delegate key in legacy_delegates.toml and rebuild the WASM. CI's check-delegate-migration.yml only fires when the *committed* WASM changes, so this commit passes that gate; the migration entry will be added in the deploy commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * build: rebuild chat-delegate WASM + V12 migration entry for review fixes Source changes in 4e99bdc altered chat-delegate behavior but the committed WASM was left at the pre-fix hash, so users installing the PR would have run the buggy delegate. This commit closes the gap: - V12 migration entry capturing the pre-fix hash (44d0025e2b...) so existing inboxes carry forward. - ui/public/contracts/chat_delegate.wasm replaced with the post-fix build (26fe16d3b5...). - Room-contract WASM left at HEAD; sync-wasm regenerated it non-deterministically despite no source changes (documented reproducibility quirk), restored via git checkout. cargo make check-migration: passes. cargo test -p river-core --test migration_test: 4/4. cargo check -p river-ui --target wasm32-unknown-unknown --features no-sync: clean. [AI-assisted - Claude] * fix(chat-delegate): direct getrandom dep to fix per-crate WASM build in CI CI builds chat-delegate via `cargo build -p chat-delegate` (single crate), which does not pick up the workspace-level `getrandom = { features = ["js"] }` because room-contract's direct dep is the only thing activating that feature under default unification. Building chat-delegate alone fails with: error: the wasm*-unknown-unknown targets are not supported by default, you may need to enable the "js" feature. Local `cargo make sync-wasm` succeeded because it built both delegate and room-contract together; the room-contract dep on getrandom unified the `js` feature for chat-delegate too. CI only builds the delegate so the unification never happens. Fix: add `getrandom = { workspace = true }` directly to the delegate so the workspace feature applies regardless of which packages are built together. V13 migration entry captures the previous (V12-era) chat-delegate hash so existing inboxes carry forward. Room-contract WASM left at HEAD; sync-wasm regenerated it non-deterministically despite no source change. cargo make check-migration: passes. cargo test -p river-core --test migration_test: 4/4. [AI-assisted - Claude] --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Re-enabling private rooms with delegate-driven secret rotation (#228) requires multiple replicas of the same chat delegate (e.g. a user running River on laptop and phone) to compute byte-identical room secrets without coordination. Today, rotation generates a fresh random secret in
RoomData::rotate_secret(ui/src/room_data.rs:441), which means two replicas rotating concurrently produce divergent state. This blocks the delegate-driven architecture described in #228.We need a deterministic key-derivation function keyed on the owner's signing key seed, with proper domain separation so secrets don't collide across rooms or rotation versions.
Approach
Add a single pure helper,
common::key_derivation::derive_room_secret. It uses a two-phase blake3 construction:Phase 1 (
blake3::derive_key) is the canonical blake3 KDF mode with a hard-coded context string ("river-rotate v1 2026-04 room-secret-root"). Phase 2 keyed-hash mixes in the per-call inputs. Splitting these phases means a future protocol-version bump just changes the context string, and a future input addition is confined to phase 2 — both make a wire-format break visible at code-review time rather than silent.The function is intentionally unused in this PR; PR 2 will wire it into
RoomData::rotate_secret(aTODO(#228 PR 2)marker is included at the swap site). Splitting derivation from wiring keeps the cryptographic construction easy to scrutinize independently.Why not derive directly from
signing_key?An earlier draft proposed a separate
rotation_seed. The supposed benefit was compromise recovery, but in practice signing-key compromise is already terminal (the attacker controls the room identity), and a separate seed adds state without a realistic recovery scenario. Direct derivation from the signing key seed is simpler with the same effective security. See #228 for the full reasoning.Trade-off
Anyone with the signing key seed can derive every past and future secret. This is acceptable for River's threat model — the signing key already authorises every room operation — and buys multi-device determinism without distributed coordination. A removed member who held
secret_v_ncannot derivesecret_v_{n+1}(they don't have the seed), so forward secrecy against removed members holds. Apps that need historical forward secrecy against signing-key compromise must not use this construction.Testing
Eight unit tests in
common/src/key_derivation.rs:derive_is_deterministic— same inputs produce same outputderive_separates_versions— different versions produce different outputsderive_separates_owners— different owners produce different outputsderive_separates_keys— different signing key seeds produce different outputsderive_locks_input_ordering— locks the order ofupdate(owner_vk)vsupdate(version)(catches argument-swap regressions)derive_known_answer_v1_zero_seed_zero_version— KAT forseed = [0u8; 32],version = 0derive_known_answer_v1_multi_byte_version— KAT forversion = 0x01020304(catchesto_le_bytestoto_be_bytesregressions)derive_known_answer_v1_all_ff_seed— KAT forseed = [0xFFu8; 32](exercises non-zero key bytes)The KAT comment includes a Python
blake3snippet for independent verification — vectors can be regenerated outside the implementation-under-test if needed.cargo test -p river-core: 192 passed, 0 failed.cargo clippy -p river-core --all-targets --all-features: clean.No delegate or contract WASM changes (function is dead code; baseline hashes preserved).
cargo make check-migration: "Committed WASM matches HEAD — no migration needed."Review
This PR was reviewed by four parallel internal review agents (code-first, testing, skeptical, big-picture) per the multi-model-review rule. No blockers were raised; the second commit on this branch addresses consensus suggestions:
signing_key_seedinvariants explicitly.TODO(#228 PR 2)marker at the planned swap site.Closes #228 partial — derivation helper only; rotation wiring follows in PR 2.
[AI-assisted - Claude]