diff --git a/CHANGELOG.md b/CHANGELOG.md index 45a74636d..038c6fda6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Keys saved on this device but not on the identity's key lists are now + listed**: a key can be saved here while appearing on none of the identity's + key lists — for example when adding it to the network did not finish. The + identity's key list now shows such keys in their own section, so they can be + opened and their saved private key removed. Previously nothing could reach + them, even when a message asked exactly that. + - **Restore keys an upgrade left behind**: an identity that was already in the app before the update — a masternode loaded from its ProTxHash, or one that held only some of its keys — kept its remaining keys in the previous @@ -94,6 +101,75 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A key held in the clear is used without asking for a password**: for a key + an earlier version had saved in two places, one of them password-protected, + using the key could bring up a password prompt even though a copy needing no + password was on this device — and dismissing that prompt then refused the + key outright. The copy that needs no password is now used first, so the + prompt only appears when it is genuinely required. + +- **Show and Sign find a key whose first copy is unreadable**: for a key saved + in two places where only one copy's stored bytes were still present — + as after restoring the app's data without its key store — "Show private + key" and "Sign" could fail on the empty copy while the readable one sat + unused. Both now reach whichever copy is actually readable. + +- **Showing or signing with a key no longer advises saving it**: pressing + "Show private key" or "Sign" on a key whose place on the identity could not + be worked out answered with advice about saving the key again — about a key + the user never entered. Both messages now name a step either situation can + take: refresh the identity and open the key again. + +- **Cancelling a password request is taken as an answer**: for a key an earlier + version had saved in more than one place, dismissing the password prompt + brought up the same prompt again for the same key. Cancelling now ends the + attempt, and the message reflects the cancellation rather than an unrelated + earlier problem with another copy of the key. + +- **Messages about a key that cannot be used now say what to do**: being told a + key is not saved on this device, or cannot be saved here, left nowhere to go + next — or worse, named a step that could not work, such as freeing disk space + when the identity's keys are password-protected, or entering a key the same + screen would then refuse. Each of these messages now names the step that + actually resolves its situation. + +- **A key that could not be saved no longer looks saved**: when entering a + private key was refused — including when saving it to this device failed — + the key's page still showed it as saved until the page was left and reopened, + offering to reveal it, to sign with it, and to remove it, none of which could + work. The page now reports a refused key as not saved, which is what it is; + likewise, a removal that could not be saved no longer shows the key as + already gone. + +- **The identities list sees a key saved by an earlier version**: the Keys + popup on the identities list showed such a key as not saved on this device — + even though it is — and opened the key's page in the same wrong state. The + popup now finds a saved key wherever the version that saved it filed it, as + the rest of the app already does. + +- **A key's wallet is found even when the key is filed twice**: a key that an + earlier version had saved in two places, wallet-derived in only one of them, + was treated as belonging to no wallet at all — so the wallet was never offered + for unlocking and signing with that key could not proceed. The wallet that + derives a key is now found wherever the key is filed. + +- **A key two lists appear to share can be saved again**: when a masternode's + own record and its voting identity each carried a key with the same number and + the same public key, entering the private key of either was refused with a + message saying the key does not belong to this identity — although it plainly + does, and what the two keys are for is what tells them apart. Such a key is + now saved where it belongs. When a key really is on two lists at once, the + message now says so and what to do about it. + +- **Entering a key can no longer erase a different one**: keys of a masternode's + own record and of its voting identity are numbered separately, so two + different keys can carry the same number. Entering the private key of one of + them used to take the other's place without a word, and the replaced key's + private half was gone — with no copy to restore it from if it had been + imported by hand. Dash Evo Tool now refuses that and explains what happened, + leaving the saved key untouched. Re-entering a key you already saved still + replaces itself, as before. + - **A saved voting key can now actually sign**: a voting key held on an identity's own record — rather than on a separate voting identity — was saved and shown as being on this device, but nothing could use it. Signing looked for @@ -107,6 +183,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). key could remove the other's private half, and a key could be reported as saved on the strength of an unrelated key being present. +- **Removing a key now removes all of it**: "Remove private key" on a key's page + also erases the copy of that key held in this device's secure storage. + Previously only the entry naming it was cleared, so the key itself stayed + behind with nothing pointing at it — it could not be used or brought back, and + deleting the whole identity afterwards did not clear it either. If the secure + storage cannot be written to, the removal now stops and says so with the key + left exactly as it was, so it can simply be tried again. + +- **A key is checked before it is used**: showing a saved key or signing with it + now confirms the key held on this device really is the key on screen. Should + the two disagree — records an older version left inconsistent — the action + stops and says so, rather than signing with a key nobody would recognise as + this identity's. + - **An identity's keys are reachable again**: the keys list under an identity's Settings → Advanced now opens each key's own page, so keys can be inspected and restored — and, once a key is on this device, signed with or diff --git a/Cargo.toml b/Cargo.toml index 618668f26..420b53f76 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,8 +46,9 @@ serde = "1.0.219" serde_json = "1.0.140" serde_yaml_ng = { version = "0.10.0" } tokio = { version = "1.46.1", features = ["full"] } -# Pin is deliberate; do not "fix" the audit warning by bumping. bincode is flagged -# unmaintained (RUSTSEC-2025-0141, INFO-level — no known vulnerability). The advisory +# INTENTIONAL(bincode-unmaintained-pin): the pin is deliberate; do not "fix" the audit +# warning by bumping. bincode is flagged unmaintained (RUSTSEC-2025-0141, INFO-level — +# no known vulnerability). The advisory # covers *all* versions, so no bump can clear it: 2.0.1 is the last functional release # and 3.0.0 is a tombstone whose lib.rs is a bare `compile_error!`. This crate encodes # the on-disk wallet-secret envelopes and QualifiedIdentity blobs, so switching encoders diff --git a/docs/ai-design/2026-07-30-key-placement-resolution/design.md b/docs/ai-design/2026-07-30-key-placement-resolution/design.md index dc3c2a8e8..85116476e 100644 --- a/docs/ai-design/2026-07-30-key-placement-resolution/design.md +++ b/docs/ai-design/2026-07-30-key-placement-resolution/design.md @@ -39,7 +39,7 @@ The derivation conflated two questions that have different answers. | Question | Answer | Used by | |---|---|---| | Where **is** this key's private half filed? | `KeyStorage::candidates` — probes each store at the key's id, keeping only entries whose stored public-key data matches | every read and delete | -| Where **should** a new private half go? | `QualifiedIdentity::placement_of` — reads the identity's own on-chain key lists | the write path only | +| Where **should** a new private half go, and which list names this key? | `QualifiedIdentity::placement_of` — reads the identity's own on-chain key lists | the Key Info paste path, and role naming | `candidates` is three `BTreeMap` probes, not a scan, in a fixed [`PROBE_ORDER`] — so resolution never depends on map iteration order. It accepts @@ -55,6 +55,24 @@ would hand out or delete material the requested key does not own. state, not a failure: `add_key_to_identity` inserts a key before broadcasting the transition that publishes it, so a key on no list is the steady state there. +`add_key_to_identity` is also the reason the backend write path does not consult +`placement_of` at all: it mints the key at `max_id + 1` on the main identity, so +`PrivateKeyOnMainIdentity` is the only list that can publish it. The slot is +still guarded: `max_id` comes from the freshly published record while the store +is local, so an entry saved here but never broadcast can occupy `max_id + 1` +with a different key, and the write refuses rather than overwrites. The paste +path in Key Info is the one writer that has to choose, because the key it is +handed already exists somewhere. + +Synchronous callers get one shared approximation of §3's rule: +`KeyStorage::first_live_candidate` — the first candidate whose bytes are +resident, else the first candidate at all. A UI frame cannot await the honest +bytes-yielding resolution, so every screen that names a placement or shows held +material routes through it (or `held_private_key_data`, built on it). One rule +means the placement a screen names and the material it displays can never come +from two different stores; it approximates liveness without opening the vault by +trusting an `InVault` placeholder only when no resident sibling exists. + ## 3. Resolve to bytes, not to a match `resolve_private_key_bytes` takes the public key and returns the first placement @@ -70,6 +88,20 @@ With nothing to fall through to, the first failure is returned rather than `Ok(None)`, so "the vault is not open" never degrades into "you never had that key". +The walk is resident-first: placements whose bytes are resident resolve with no +chokepoint access and are tried before vault-backed or wallet-derived ones — the +async mirror of `first_live_candidate`'s rule, so the two resolvers agree about +which copy of a dual-filed key answers first. Within each group, probe order +decides. + +One exception ends the walk early: a cancelled password prompt +(`TaskError::SecretPromptCancelled`) is returned as-is, outranking any earlier +placement's mechanical failure. It is the user's answer about the key — falling +through to a sibling placement would re-ask for what was just declined, one +dialog per store, and reporting a prior placement's failure instead would deny +that anything was asked. Because resident placements walk first, the carve-out +can only fire when no prompt-free copy existed to serve. + ## 4. The map key and the vault label are one address A vault-backed key's bytes live under the label @@ -122,41 +154,120 @@ A crash cannot strand a key because nothing is ever in motion. * `KeyInfoScreen` no longer carries a `target` field. It resolves on demand, so there is no state to thread through constructors and nothing for the `ScreenType` round trip to drop. -* `KeyStorage.private_keys` is **private**. With correctness concentrated in the - resolver, a caller reaching past it can silently miss a key that is present. - Explicit-placement accessors remain for callers that legitimately know one — a - loader walking the list it read a key from, and legacy recovery, which is - *about* the placements an old blob recorded and must not be routed through a - target-blind resolver. +* `KeyStorage.private_keys` is **private**, and the *write-side* accessors that + name a placement (`insert_at`, `entry_at`, `remove_at`, `insert_if_absent`, + `has`) are `pub(crate)`. With correctness concentrated in the resolver, a + caller reaching past it can silently miss a key that is present; keeping the + placement-naming surface inside the crate keeps the set of such callers + enumerable. They exist for the two that legitimately know a placement — the + loader folding a previously-loaded record into a fresh one, and legacy + recovery, which is *about* the placements an old blob recorded and must not + be routed through a target-blind resolver. `insert_at` and `entry_at` turned + out to have no production caller left and are `#[cfg(test)]`. + The narrowing is not complete: several `pub` methods still take a + caller-named placement (`get_resolve_local`, `get_resolve_with_seed`, + `get_cloned_private_key_data_and_wallet_info`, `mark_in_vault`, `is_in_vault`, + `public_key_for`, `wallet_seed_hash_for`), and `mark_in_vault` zeroizes the + slot's occupant with no `same_key` guard — so `pub` alone does not yet mean a + caller cannot file or destroy at a placement of its choosing. The residual is + tracked in §8 and marked in the code as `TODO(placement-named-pub-surface)`. ## 8. What is not covered -* **A deleted key's vault secret is not removed** (`key_info_screen.rs`, the - remove-private-key dialog): the map entry goes, the vault entry stays, so bytes - the user believes deleted remain on disk with nothing pointing at them. Its own - fix, with its own ordering argument (vault first, then map) and its own review. +* ~~**A deleted key's vault secret is not removed**~~ — **closed.** The + remove-private-key path now deletes the vault secrets of the placements + `candidates` resolved, through `AppContext::delete_identity_key_secrets`, and + does so *before* the map entries go: the map is what makes a vault label + enumerable, so the reverse order strands the bytes beyond every later path, + the whole-identity sweep included. A vault failure aborts with map and blob + untouched. * **Keying by `(Identifier, KeyID)`** — the right end state, since it removes the role enum entirely. Mechanical once the store is known-consistent. -* Validating the signing path's vault-resolved key against the requested public - key, deferred separately. +* ~~Validating the signing path's vault-resolved key against the requested + public key~~ — **closed.** `with_identity_secret_key` — the chokepoint both + `SignMessageWithIdentityKey` and `DeriveIdentityKeyForDisplay` read through — + matches the vault's bytes against the public key the stored identity records + at the requested placement before the closure runs, and refuses a disagreement + with `TaskError::IdentityKeyMismatch`. This was the last place trusting a + caller-supplied placement; the callers still carry `(target, key_id)` fields, + but they are now checked rather than believed. A key type this build cannot + derive a public half for skips the check, as `key_exclusion` does. The + chokepoint also carries §3's fallthrough: the caller names its placement from + the synchronous approximation, which cannot see a dead vault label, so the + fetch serves the first placement of the same key whose label is live — a dead + placeholder cannot shadow a live sibling on the Show/Sign path. +* **Several `pub` `KeyStorage` accessors still take a caller-named placement** + (marked `TODO(placement-named-pub-surface)` on the struct): + `get_resolve_local`, `get_resolve_with_seed`, + `get_cloned_private_key_data_and_wallet_info`, `mark_in_vault`, `is_in_vault`, + `public_key_for`, `wallet_seed_hash_for`. `mark_in_vault` is the sharp edge — + it zeroizes whatever occupies the slot and repoints it at a vault label with + no `same_key` guard; its single production caller is safe only because + `insert_non_encrypted` refused a foreign occupant earlier in the same flow. + Narrow them to `pub(crate)` or guard them, then restore §7's stronger claim. +* **Proof generation cannot use a locally-added, not-yet-broadcast key** + (`backend_task/grovestark.rs`, marked `TODO(grovestark-unpublished-key)`): the + requested key id is resolved against the identity's published keys before the + resolver runs, so a key in the normal unpublished state (§2) fails + indistinguishably from "no such key". ## 9. Test coverage -| Test | Pins | -|---|---| -| `a_held_voting_key_on_the_main_identity_is_signable_under_either_placement` | the regression lock — a saved key must be usable under **both** placements, so writer and reader can never drift apart again | -| `voting_key_on_the_main_identity_is_found_where_the_loader_files_it` | the defect: the shape the authoritative loader writes | -| `a_voting_key_an_older_build_filed_under_voter_stays_findable` | the no-migration constraint | -| `an_authentication_key_on_the_voter_identity_is_found` | the mirror defect | -| `two_different_keys_sharing_an_id_are_never_confused` | the id collision | -| `two_keys_sharing_id_and_material_are_told_apart_by_purpose` | the collision `data` alone cannot resolve | -| `a_key_disabled_since_it_was_saved_is_still_found` | `disabled_at` is the one field that legitimately moves | -| `removing_one_key_leaves_a_different_key_sharing_its_id_alone` | the delete path: confirmed RED against the purpose-derived removal, which left the key the user asked to delete in place and removed another | -| `a_dead_vault_placeholder_falls_through_to_a_live_placement` | the fallthrough rule (§3) | -| `a_lone_dead_placement_surfaces_its_error_rather_than_absence` | the other half of it | -| `duplicate_placements_are_returned_in_probe_order` | determinism, not iteration order | -| `an_entry_whose_material_disagrees_is_not_a_candidate` | the assumption material matching rests on | -| `an_operator_filed_key_from_a_legacy_blob_stays_reachable` | `PrivateKeyOnOperatorIdentity` has no live writer but is legacy-reachable | -| `both_keys_of_a_real_v093_blob_resolve_to_their_own_material` | a **real** v0.9.3 blob's keys are reachable, not merely decodable | +The layer column tells near-homonyms apart: the *resolver* rows exercise +`candidates` / `resolve_private_key_bytes` (`model/qualified_identity/mod.rs`), +the *naming* rows `placement_of` (same file), the *key store* rows `KeyStorage`'s +own guards and helpers (`encrypted_key_storage.rs`), the *vault* rows the +vault-secret-lifecycle chokepoints (`AppContext::delete_identity_key_secrets` in +`context/identity_db.rs`, `with_identity_secret_key` in `backend_task/wallet/mod.rs`), +and the UI rows the screens that consume them. + +| Test | Layer | Pins | +|---|---|---| +| `a_held_voting_key_on_the_main_identity_is_signable_under_either_placement` | resolver | the regression lock — a saved key must be usable under **both** placements, so writer and reader can never drift apart again | +| `voting_key_on_the_main_identity_is_found_where_the_loader_files_it` | resolver | the defect: the shape the authoritative loader writes | +| `a_voting_key_an_older_build_filed_under_voter_stays_findable` | resolver | the no-migration constraint | +| `an_authentication_key_on_the_voter_identity_is_found` | resolver | the mirror defect | +| `two_different_keys_sharing_an_id_are_never_confused` | resolver | the id collision | +| `two_keys_sharing_id_and_material_are_told_apart_by_purpose` | resolver | the collision `data` alone cannot resolve | +| `two_keys_sharing_id_and_material_are_placed_by_purpose` | naming | the same collision on the *naming* question — `placement_of` tells the twins apart too | +| `a_key_disabled_since_it_was_saved_is_still_found` | resolver | `disabled_at` is the one field that legitimately moves | +| `a_key_disabled_since_it_was_saved_still_has_a_placement` | naming | naming survives on-chain disabling as reading does | +| `removing_one_key_leaves_a_different_key_sharing_its_id_alone` | Key Info screen | the delete path: confirmed RED against the purpose-derived removal, which left the key the user asked to delete in place and removed another | +| `a_dead_vault_placeholder_falls_through_to_a_live_placement` | resolver | the fallthrough rule (§3) | +| `a_lone_dead_placement_surfaces_its_error_rather_than_absence` | resolver | the other half of it | +| `a_key_with_no_placement_resolves_to_absence` | resolver | absence is `Ok(None)` — never an error, never another key's material | +| `cancelling_the_prompt_stops_asking_for_the_same_key` | resolver | §3's cancellation carve-out: one refusal, one dialog | +| `a_cancellation_outranks_an_earlier_placements_failure` | resolver | the cancellation is what surfaces, not a prior placement's mechanical failure | +| `a_resident_sibling_resolves_without_prompting_for_a_sealed_copy` | resolver | the resident-first walk: a sealed copy cannot put a prompt in front of bytes held in the clear | +| `duplicate_placements_are_returned_in_probe_order` | resolver | determinism, not iteration order | +| `an_entry_whose_material_disagrees_is_not_a_candidate` | resolver | the assumption material matching rests on | +| `an_operator_filed_key_from_a_legacy_blob_stays_reachable` | resolver | `PrivateKeyOnOperatorIdentity` has no live writer but is legacy-reachable | +| `both_keys_of_a_real_v093_blob_resolve_to_their_own_material` | migration | a **real** v0.9.3 blob's keys are reachable, not merely decodable | +| `a_different_key_cannot_take_an_occupied_slot` | key store | the write guard: a foreign occupant refuses the write and survives it unchanged | +| `the_same_key_still_overwrites_itself` | key store | re-entering a saved key corrects it rather than duplicating it | +| `the_occupied_slot_refusal_names_a_performable_remedy` | key store | the refusal's message names removal — the remedy that exists — not a refresh | +| `same_key_ignores_a_key_being_disabled` | key store | the `same_key` carve-out: `disabled_at` may move after a key is saved | +| `same_key_rejects_a_disagreement_anywhere_else` | key store | any other field disagreeing means a different key | +| `a_resident_placement_wins_over_a_vault_placeholder` | key store | `first_live_candidate` prefers resident bytes (§2's synchronous approximation) | +| `a_lone_placement_is_named_whatever_it_holds` | key store | with a single candidate there is nothing to prefer | +| `an_unheld_key_has_no_placement_and_no_material` | key store | an unheld key answers `None` to both placement questions | +| `wallet_derived_at_looks_past_a_placement_that_is_not_derived` | key store | the wallet probe is typed, not liveness-based — a duplicate's first placement need not be the derived one | +| `a_wallet_is_found_under_a_later_placement_too` | wallet lookup | `get_selected_wallet` finds a key's wallet under any placement it is filed at | +| `a_key_that_cannot_be_placed_is_not_reported_as_held` | Key Info screen | a paste with no store to file under is refused and never shown as held | +| `a_key_the_persist_refuses_is_not_reported_as_held` | Key Info screen | a persist refusal rolls the paste back — on screen and in the in-memory record | +| `a_removal_that_cannot_persist_keeps_the_key_held` | Key Info screen | a removal that could not persist leaves the key held on screen | +| `a_write_behind_the_screen_survives_a_paste` | Key Info screen | the paste is a locked read-modify-write — no concurrent writer's key is written away | +| `a_write_behind_the_screen_survives_a_removal` | Key Info screen | the removal edits the record as stored now, same lost-update guard | +| `a_show_request_that_cannot_resolve_speaks_through_the_typed_error` | Key Info screen | Show/Sign failures surface each variant's own remedy | +| `the_placement_errors_advise_an_action_both_paths_can_perform` | Key Info screen | the placement errors' remedies fit the Show/Sign read path as well as the paste path | +| `a_held_key_published_on_no_list_still_gets_a_row` | keys list | a held-but-unpublished key is reachable, so the occupied-slot remedy is performable from the add-key flow too | +| `the_keys_popup_finds_a_main_key_an_older_build_filed_under_voter` | identities list | the Keys popup is placement-blind for main-identity keys | +| `the_keys_popup_finds_a_voter_key_an_older_build_filed_under_main` | identities list | its voter-list mirror | +| `removing_a_key_also_removes_its_vault_secret` | vault | §8's second bullet: confirmed RED against the map-only removal, which left the bytes on disk unreachable | +| `delete_identity_key_secrets_drops_only_the_named_placement` | vault | the per-key delete is not the whole-identity sweep, and repeating it is harmless | +| `a_vault_secret_that_is_not_the_recorded_key_is_refused` | vault | §8's third bullet: RED against the unchecked chokepoint, which signed with the planted key | +| `a_placement_the_identity_does_not_record_is_refused` | vault | the other half of it — an orphaned label is not a key of this identity | +| `a_secret_matching_its_recorded_key_still_resolves` | vault | the check costs a healthy install nothing | +| `a_dead_placeholder_at_the_named_placement_falls_through_to_a_live_sibling` | vault | §3's fallthrough at the named-target chokepoint: Show/Sign reach a live sibling behind a dead placeholder | [`PROBE_ORDER`]: ../../../src/model/qualified_identity/key_placement.rs diff --git a/docs/user-stories.md b/docs/user-stories.md index eace4f72c..4ceedd58f 100644 --- a/docs/user-stories.md +++ b/docs/user-stories.md @@ -579,6 +579,7 @@ As a user, I want to view all keys associated with my identity so that I can aud - Each row names the key by its role in words that suit the identity — a user identity's keys in plain language, a masternode's in its registration terms — and says whether the key is saved on this device, in words rather than by colour alone. A key the network has retired says so in its name. - The on-chain specifics — key id, Platform purpose, security level, key type, read-only — are Expert-view detail, on both the list and the key's own page. The Everyday view gets the role and the held state, which is what it can act on. - A key's own page shows its public key, hash and address, and offers to view the private half, sign a message with it, or add and remove it. +- A key saved on this device that is on none of the identity's key lists — a saved key whose publication never happened — is listed in its own section below the published keys, so its saved private half can still be opened and removed. The section appears only when such a key exists. - The offer to restore keys an upgrade left behind also appears here; its criteria are IDN-020's. ### IDN-013: Password-protect an identity's signing keys (SEC-001) [Implemented] diff --git a/src/backend_task/error.rs b/src/backend_task/error.rs index 5714d60a5..ed2e8063e 100644 --- a/src/backend_task/error.rs +++ b/src/backend_task/error.rs @@ -450,6 +450,38 @@ pub enum TaskError { )] IdentityKeyMissing, + /// More than one of an identity's key lists publishes the key, so nothing + /// may guess which list the private half belongs to. A stale local record + /// is the usual cause. Surfaces on Key Info's paste path and its Show/Sign + /// path alike, so the remedy presumes neither. Fieldless: no upstream + /// error and, by design, never any secret. + #[error( + "This key appears on more than one of this identity's key lists, so where it belongs is unclear. Refresh this identity and open this key again." + )] + IdentityKeyPlacementAmbiguous, + + /// None of an identity's key lists publishes the key, and nothing is held + /// for it either, so there is no store to file it under or read it from. + /// Surfaces on Key Info's paste path and its Show/Sign path alike, so the + /// remedy presumes neither. Fieldless: no upstream error and, by design, + /// never any secret. + #[error( + "This key is not on any of this identity's key lists. Refresh this identity and open this key again; if it still does not appear, it belongs to a different identity." + )] + IdentityKeyNotOnIdentityRecord, + + /// The `(placement, key id)` slot a write would take is already held by a + /// *different* key. The voter and main id spaces overlap, so two keys can + /// share an id; the write is refused because the occupant's private half is + /// frequently the only copy in existence. The remedy is removing that + /// locally saved half — a refresh updates published keys but evicts no + /// local private half. Fieldless: no upstream error and, by design, never + /// any secret. + #[error( + "A different key of this identity is already saved on this device under the number this key would use. Open that key in this identity's key list, remove its saved private key from this device, then try again." + )] + IdentityKeySlotOccupied, + /// An identity private key was found in the vault but its bytes are not a /// usable signing key (vault corruption or a truncated write). Distinct /// from [`Self::IdentityKeyMissing`] (genuinely absent) so the user gets @@ -460,6 +492,19 @@ pub enum TaskError { )] IdentityKeyMalformed, + /// The private key stored at a placement does not derive the public key the + /// identity records there — the vault and the stored key map disagree about + /// which key that slot holds. Distinct from [`Self::IdentityKeyMalformed`] + /// (bytes present but unusable) and [`Self::IdentityKeyMissing`] (nothing + /// there at all): these bytes are a perfectly usable key, just not this one, + /// so signing with them would produce a signature no verifier attributes to + /// this identity. Fieldless: the callsite logs the placement; no key + /// material or raw error string is stored here. + #[error( + "This identity's signing key does not match the key it is saved for on this device. Re-import the identity to refresh its keys." + )] + IdentityKeyMismatch, + /// The password supplied for a password-protected identity key does not /// unseal it. The just-in-time chokepoint catches this inside its re-ask /// loop and re-prompts; it surfaces to the UI when removing protection with diff --git a/src/backend_task/grovestark.rs b/src/backend_task/grovestark.rs index ff12b16fd..266a095b1 100644 --- a/src/backend_task/grovestark.rs +++ b/src/backend_task/grovestark.rs @@ -41,6 +41,12 @@ pub async fn run_grovestark_task( // The key id is resolved to the identity's own published key first, // so a request naming a key this identity does not have fails here // rather than reaching the vault. + // + // TODO(grovestark-unpublished-key): resolve_private_key_bytes now + // requires the key be published on the main identity; a + // locally-added-but-not-yet-broadcast key (a normal state per + // docs/ai-design/2026-07-30-key-placement-resolution/design.md §2) + // fails here indistinguishably from "no such key". let signing_key = identity .identity .get_public_key_by_id(key_id) diff --git a/src/backend_task/identity/add_key_to_identity.rs b/src/backend_task/identity/add_key_to_identity.rs index 11e248ac4..b8b4babc8 100644 --- a/src/backend_task/identity/add_key_to_identity.rs +++ b/src/backend_task/identity/add_key_to_identity.rs @@ -57,13 +57,18 @@ impl AppContext { public_key_to_add .identity_public_key .set_id(qualified_identity.identity.get_public_key_max_id() + 1); + // `max_id` comes from the freshly published record, but the slot is + // checked against the LOCAL store: an entry saved here but never + // broadcast (e.g. restored from an old blob) can hold `max_id + 1`, + // and it may be a misfiled key's only private half — so refuse rather + // than overwrite. qualified_identity.private_keys.insert_non_encrypted( ( PrivateKeyOnMainIdentity, public_key_to_add.identity_public_key.id(), ), (public_key_to_add.clone(), private_key), - ); + )?; // Track balance before operation for fee calculation let balance_before = qualified_identity.identity.balance(); let estimated_fee = self.fee_estimator().estimate_identity_update(); diff --git a/src/backend_task/wallet/mod.rs b/src/backend_task/wallet/mod.rs index 8aab39e4d..c918d6e9c 100644 --- a/src/backend_task/wallet/mod.rs +++ b/src/backend_task/wallet/mod.rs @@ -23,6 +23,7 @@ use dash_sdk::dpp::dashcore::secp256k1::{Message, Secp256k1, SecretKey}; use dash_sdk::dpp::dashcore::sign_message::{MessageSignature, signed_msg_hash}; use dash_sdk::dpp::dashcore::{OutPoint, PrivateKey}; use dash_sdk::dpp::identity::core_script::CoreScript; +use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dash_sdk::dpp::identity::{KeyID, KeyType}; use dash_sdk::dpp::key_wallet::bip32::DerivationPath; use dash_sdk::platform::Identifier; @@ -104,6 +105,29 @@ impl AppContext { /// The raw key zeroizes when the closure returns — only `f`'s result /// crosses back to the caller. Shared by the identity-key sign and /// display tasks. + /// + /// The placement is the caller's word, and it addresses the vault label + /// directly, so the bytes are matched against the public key the stored + /// identity records at exactly that placement before `f` ever sees them. + /// Nothing else on this path proves the caller named the slot its key + /// actually occupies. A key type this build cannot derive a public half for + /// skips the check rather than failing it, as the legacy-recovery key check + /// does: unverifiable is not wrong. + /// + /// The caller names its placement synchronously, without opening the + /// vault, so it cannot see a dead placeholder — an `InVault` entry whose + /// vault label holds nothing — sitting beside a sibling placement that + /// files the same key with the live secret. The fetch therefore serves the + /// first such placement whose label is actually present, starting from the + /// named one, and falls back to the named placement when every label is + /// absent so an all-dead key still fails with its honest error. + /// + /// # Errors + /// + /// [`TaskError::IdentityKeyMissing`] when the identity is not stored on this + /// device or records no key at that placement, and + /// [`TaskError::IdentityKeyMismatch`] when the vault holds a key there that + /// is not the one recorded. async fn with_identity_secret_key( self: &Arc, identity_id: Identifier, @@ -111,12 +135,46 @@ impl AppContext { key_id: KeyID, f: impl FnOnce(SecretKey) -> Result, ) -> Result { + let identity = self + .get_local_qualified_identity(&identity_id)? + .ok_or(TaskError::IdentityKeyMissing)?; + let recorded = identity + .private_keys + .public_key_for(&(target.clone(), key_id)) + .map(|public_key| public_key.identity_public_key.clone()) + .ok_or(TaskError::IdentityKeyMissing)?; + let backend = self.wallet_backend()?; + // Serve the first placement of this key whose vault label is live, + // named placement first. A liveness probe, not a fetch — the same + // probe-then-act shape `IdentityKeyView::store` documents, bounded by + // the same store-level serialization. + let named = (target, key_id); + let view = crate::wallet_backend::IdentityKeyView::new( + backend.secret_store(), + identity_id.to_buffer(), + ); + let (target, key_id) = std::iter::once(named.clone()) + .chain( + identity + .private_keys + .candidates(&recorded) + .filter(|placement| { + *placement != named && identity.private_keys.is_in_vault(placement) + }), + ) + .find(|(target, key_id)| { + matches!( + view.scheme(target, *key_id), + Ok(scheme) if scheme != crate::wallet_backend::secret_seam::SecretScheme::Absent + ) + }) + .unwrap_or(named); + let network = self.network; let scope = crate::wallet_backend::SecretScope::IdentityKey { identity_id: identity_id.to_buffer(), target, key_id, }; - let backend = self.wallet_backend()?; backend .secret_access() .with_secret(&scope, |plaintext| { @@ -129,6 +187,18 @@ impl AppContext { tracing::warn!(error = %detail, "Identity-key secret construction failed"); TaskError::IdentityKeyMalformed })?; + if let Ok(derived) = recorded + .key_type() + .public_key_data_from_private_key_data(key, network) + && derived.as_slice() != recorded.data().as_slice() + { + tracing::warn!( + identity = %identity_id, + key_id, + "Vault key at the requested placement is not the key recorded there", + ); + return Err(TaskError::IdentityKeyMismatch); + } f(secret_key) }) .await @@ -284,14 +354,24 @@ mod tests { use crate::app_dir::ensure_env_file; use crate::context::connection_status::ConnectionStatus; use crate::database::test_helpers::create_database_at_path; + use crate::model::qualified_identity::encrypted_key_storage::{KeyStorage, PrivateKeyData}; + use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; + use crate::model::qualified_identity::{IdentityStatus, IdentityType, QualifiedIdentity}; use crate::model::wallet::Wallet; use crate::model::wallet::birth_height::WalletOrigin; use crate::utils::egui_mpsc::SenderAsync; use crate::utils::tasks::TaskManager; + use crate::wallet_backend::IdentityKeyView; use dash_sdk::dpp::dashcore::Network; use dash_sdk::dpp::dashcore::secp256k1::PublicKey; use dash_sdk::dpp::dashcore::sign_message::{MessageSignature, signed_msg_hash}; + use dash_sdk::dpp::identity::accessors::IdentityGettersV0; + use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dash_sdk::dpp::identity::{Identity, Purpose, SecurityLevel}; use dash_sdk::dpp::key_wallet::bip32::ChildNumber; + use dash_sdk::dpp::platform_value::BinaryData; + use dash_sdk::dpp::version::PlatformVersion; + use dash_sdk::platform::IdentityPublicKey; use std::sync::atomic::{AtomicBool, Ordering}; use tokio::sync::mpsc::Receiver; @@ -439,4 +519,225 @@ mod tests { fn recovers_signer_pubkey_uncompressed() { assert_recovers(false); } + + const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + const VOTER: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnVoterIdentity; + + /// Store an identity holding one ECDSA key at `(Main, key_id)` whose public + /// half genuinely is `secret`'s. The insert moves the plaintext into the + /// vault, so label and record agree — the state a healthy install is in. + fn store_identity_holding( + ctx: &Arc, + seed: u8, + key_id: KeyID, + secret: [u8; 32], + ) -> Identifier { + let platform_version = PlatformVersion::latest(); + let public_key = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: key_id, + purpose: Purpose::AUTHENTICATION, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new( + KeyType::ECDSA_SECP256K1 + .public_key_data_from_private_key_data(&secret, Network::Testnet) + .expect("derive the public half"), + ), + disabled_at: None, + }); + let mut private_keys = KeyStorage::default(); + private_keys.insert_at( + (MAIN, key_id), + ( + QualifiedIdentityPublicKey::from(public_key), + PrivateKeyData::Clear(secret), + ), + ); + let qi = QualifiedIdentity { + identity: Identity::create_basic_identity( + Identifier::from([seed; 32]), + platform_version, + ) + .expect("basic identity"), + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::User, + alias: None, + private_keys, + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: Network::Testnet, + }; + ctx.insert_local_qualified_identity(&qi, &None) + .expect("store the identity"); + qi.identity.id() + } + + /// The happy path this chokepoint exists for: a vault secret that really is + /// the key recorded at the requested placement reaches the closure intact. + /// Verifying the placement must not cost a healthy install its signing. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_secret_matching_its_recorded_key_still_resolves() { + let fixture = wallet_fixture().await; + let secret = [0x21; 32]; + let identity_id = store_identity_holding(&fixture.ctx, 0x90, 7, secret); + + let bytes = fixture + .ctx + .with_identity_secret_key(identity_id, MAIN, 7, |key| Ok(key.secret_bytes())) + .await + .expect("a key that matches its record must resolve"); + + assert_eq!(bytes, secret, "the recorded key's own bytes come back"); + } + + /// The placement is the caller's word, and the vault label it names can hold + /// a different key than the record files there — a stale label an older + /// build wrote, or a write that landed while the record moved. Signing with + /// those bytes yields a signature no verifier attributes to this identity, + /// so the chokepoint must refuse them rather than hand them over. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_vault_secret_that_is_not_the_recorded_key_is_refused() { + let fixture = wallet_fixture().await; + let identity_id = store_identity_holding(&fixture.ctx, 0x91, 7, [0x21; 32]); + let backend = fixture.ctx.wallet_backend().expect("backend"); + IdentityKeyView::new(backend.secret_store(), identity_id.to_buffer()) + .store(&MAIN, 7, &[0x99; 32]) + .expect("plant a different key at the same label"); + + let reached_key = Arc::new(AtomicBool::new(false)); + let probe = Arc::clone(&reached_key); + let result = fixture + .ctx + .with_identity_secret_key(identity_id, MAIN, 7, move |_key| { + probe.store(true, Ordering::SeqCst); + Ok(()) + }) + .await; + + assert!( + matches!(result, Err(TaskError::IdentityKeyMismatch)), + "a secret that is not the recorded key must be refused, got {result:?}" + ); + assert!( + !reached_key.load(Ordering::SeqCst), + "the wrong key must never reach the caller's closure" + ); + } + + /// A placement the identity records nothing at is not a key of this + /// identity, whatever the vault happens to hold there. An orphaned label — + /// one an older build left behind — must not be served just because a caller + /// asked for it by name. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_placement_the_identity_does_not_record_is_refused() { + let fixture = wallet_fixture().await; + let identity_id = store_identity_holding(&fixture.ctx, 0x92, 7, [0x21; 32]); + let backend = fixture.ctx.wallet_backend().expect("backend"); + IdentityKeyView::new(backend.secret_store(), identity_id.to_buffer()) + .store(&VOTER, 7, &[0x88; 32]) + .expect("plant an orphan where the record names nothing"); + + let reached_key = Arc::new(AtomicBool::new(false)); + let probe = Arc::clone(&reached_key); + let result = fixture + .ctx + .with_identity_secret_key(identity_id, VOTER, 7, move |_key| { + probe.store(true, Ordering::SeqCst); + Ok(()) + }) + .await; + + assert!( + matches!(result, Err(TaskError::IdentityKeyMissing)), + "an unrecorded placement must be refused, got {result:?}" + ); + assert!( + !reached_key.load(Ordering::SeqCst), + "an orphaned secret must never reach the caller's closure" + ); + } + + /// The caller names its placement from the synchronous approximation, which + /// cannot see whether a vault label is live. A dead placeholder — an + /// `InVault` entry whose label holds nothing — can therefore be named while + /// a sibling placement files the same key with the live secret; Show and + /// Sign must reach those bytes, not fail on the guess. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_dead_placeholder_at_the_named_placement_falls_through_to_a_live_sibling() { + let fixture = wallet_fixture().await; + let secret = [0x27; 32]; + let public_key = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 7, + purpose: Purpose::VOTING, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new( + KeyType::ECDSA_SECP256K1 + .public_key_data_from_private_key_data(&secret, Network::Testnet) + .expect("derive the public half"), + ), + disabled_at: None, + }); + // One key, filed `InVault` under both stores — the dual-filed shape a + // blob written under two conventions carries. + let mut private_keys = KeyStorage::default(); + for target in [MAIN, VOTER] { + private_keys.insert_at( + (target, 7), + ( + QualifiedIdentityPublicKey::from(public_key.clone()), + PrivateKeyData::InVault, + ), + ); + } + let qi = QualifiedIdentity { + identity: Identity::create_basic_identity( + Identifier::from([0x93; 32]), + PlatformVersion::latest(), + ) + .expect("basic identity"), + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::Masternode, + alias: None, + private_keys, + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: Network::Testnet, + }; + fixture + .ctx + .insert_local_qualified_identity(&qi, &None) + .expect("store the identity"); + let identity_id = qi.identity.id(); + // The live secret sits only under the Voter label; Main stays a dead + // placeholder, the state a blob restored without its vault is in. + let backend = fixture.ctx.wallet_backend().expect("backend"); + IdentityKeyView::new(backend.secret_store(), identity_id.to_buffer()) + .store(&VOTER, 7, &secret) + .expect("file the live secret under the sibling placement"); + + let bytes = fixture + .ctx + .with_identity_secret_key(identity_id, MAIN, 7, |key| Ok(key.secret_bytes())) + .await + .expect("the sibling placement's live bytes must be served"); + + assert_eq!(bytes, secret, "the key's own bytes come back"); + } } diff --git a/src/context/identity_db.rs b/src/context/identity_db.rs index 2cab1b96c..9b792a4cc 100644 --- a/src/context/identity_db.rs +++ b/src/context/identity_db.rs @@ -2,11 +2,12 @@ use super::AppContext; use crate::backend_task::contested_names::ScheduledDPNSVote; use crate::backend_task::error::TaskError; use crate::model::qualified_identity::{ - DPNSNameInfo, IdentityStatus, IdentityType, QualifiedIdentity, + DPNSNameInfo, IdentityStatus, IdentityType, PrivateKeyTarget, QualifiedIdentity, }; use crate::model::wallet::{Wallet, WalletSeedHash}; use crate::wallet_backend::{DetKv, DetScope, KvAdapterError}; use dash_sdk::dpp::dashcore::Network; +use dash_sdk::dpp::identity::KeyID; use dash_sdk::dpp::identity::accessors::IdentityGettersV0; use dash_sdk::dpp::voting::vote_choices::resource_vote_choice::ResourceVoteChoice; use dash_sdk::platform::Identifier; @@ -622,6 +623,38 @@ impl AppContext { self.write_local_qualified_identity_locked(qualified_identity) } + /// Read-modify-write one stored identity under its record guard. + /// + /// Re-reads the record inside [`Self::identity_record_lock`], hands the + /// fresh copy to `edit`, and persists the result through + /// [`Self::write_local_qualified_identity_locked`] — so the edit applies + /// to what is on disk *now*, never to a caller's earlier snapshot, and a + /// concurrent writer's change cannot be silently written away. Returns + /// the persisted record for the caller to adopt in place of any clone it + /// holds. + /// + /// # Errors + /// + /// [`TaskError::IdentityNotFoundLocally`] when nothing is stored under + /// `identity_id`, and whatever `edit` returns — in both cases nothing is + /// persisted. + pub fn edit_local_qualified_identity( + &self, + identity_id: &Identifier, + edit: impl FnOnce(&mut QualifiedIdentity) -> std::result::Result<(), TaskError>, + ) -> std::result::Result { + let lock = self.identity_record_lock(*identity_id); + let _guard = lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let mut fresh = self + .get_local_qualified_identity(identity_id)? + .ok_or(TaskError::IdentityNotFoundLocally)?; + edit(&mut fresh)?; + self.write_local_qualified_identity_locked(&fresh)?; + Ok(fresh) + } + /// The write half of [`Self::update_local_qualified_identity`], for a /// caller that already holds this identity's /// [`identity_record_lock`](Self::identity_record_lock) across a wider @@ -1129,6 +1162,26 @@ impl AppContext { view.delete_all(qi.private_keys.keys_set()) } + /// Delete the vault secrets filed at `placements` for `identity_id`, leaving + /// the identity's other keys in place. + /// + /// The per-key counterpart of the whole-identity sweep + /// [`Self::clear_identity_vault_keys`], for a caller dropping this device's + /// copy of a single key. Call it *before* the placement leaves the stored + /// key map: that map is where the sweep reads its delete set, so a secret + /// orphaned by an earlier map eviction is reachable by nothing afterwards. + /// + /// Idempotent — a placement whose vault label is already absent is not an + /// error, so a caller need not know whether the key was vault-backed. + pub fn delete_identity_key_secrets( + &self, + identity_id: &Identifier, + placements: impl IntoIterator, + ) -> std::result::Result<(), TaskError> { + crate::wallet_backend::IdentityKeyView::new(&self.secret_store, identity_id.to_buffer()) + .delete_all(placements) + } + /// Devnet-only sweep: drop every locally-stored identity for the /// current network. Matches the pre-C7 /// `delete_all_local_qualified_identities_in_devnet` guard — no-op on @@ -2337,4 +2390,79 @@ mod tests { "a different identity's vault key must be untouched (isolation)" ); } + + /// An offline `AppContext` over a throwaway data dir, plus the very vault it + /// was built on so a test can probe what the context wrote. + async fn ctx_with_vault() -> ( + Arc, + Arc, + tempfile::TempDir, + ) { + use crate::app_dir::ensure_env_file; + use crate::context::connection_status::ConnectionStatus; + use crate::database::test_helpers::create_database_at_path; + use crate::utils::tasks::TaskManager; + + let temp_dir = tempfile::tempdir().expect("tempdir"); + let data_dir = temp_dir.path().to_path_buf(); + ensure_env_file(&data_dir); + let db = Arc::new(create_database_at_path(&data_dir.join("data.db")).expect("db")); + let app_kv = AppContext::open_app_kv(&data_dir).expect("app kv"); + let secret_store = AppContext::open_secret_store(&data_dir).expect("secret store"); + let ctx = AppContext::new( + data_dir, + Network::Testnet, + db, + Arc::new(TaskManager::new()), + Arc::new(ConnectionStatus::new()), + egui::Context::default(), + app_kv, + Arc::clone(&secret_store), + crate::model::user_role::UserRoleCell::default(), + ) + .expect("offline testnet AppContext::new"); + (ctx, secret_store, temp_dir) + } + + /// Per-key vault deletion drops the placements it is given and nothing else. + /// That is what separates it from `clear_identity_vault_keys`, which empties + /// the identity: dropping one key must leave the identity's remaining keys — + /// and every other identity's — exactly where they were. Idempotent, because + /// the remove path calls it without first knowing whether the label is there. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn delete_identity_key_secrets_drops_only_the_named_placement() { + const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + + let (ctx, store, _dir) = ctx_with_vault().await; + let victim = Identifier::from(id(0x61)); + let bystander = Identifier::from(id(0x62)); + for owner in [victim, bystander] { + let view = IdentityKeyView::new(&store, owner.to_buffer()); + view.store(&MAIN, 0, &[0x01; 32]).unwrap(); + view.store(&MAIN, 1, &[0x02; 32]).unwrap(); + } + + ctx.delete_identity_key_secrets(&victim, [(MAIN, 0)]) + .expect("delete the named placement"); + + let victim_view = IdentityKeyView::new(&store, victim.to_buffer()); + assert!( + victim_view.get(&MAIN, 0).unwrap().is_none(), + "the named placement's secret must be gone", + ); + assert!( + victim_view.get(&MAIN, 1).unwrap().is_some(), + "the identity's other key must survive a single-key removal", + ); + assert!( + IdentityKeyView::new(&store, bystander.to_buffer()) + .get(&MAIN, 0) + .unwrap() + .is_some(), + "another identity's key at the same placement must be untouched", + ); + + ctx.delete_identity_key_secrets(&victim, [(MAIN, 0)]) + .expect("deleting an already-gone placement is not an error"); + } } diff --git a/src/model/qualified_identity/encrypted_key_storage.rs b/src/model/qualified_identity/encrypted_key_storage.rs index 7cf0a04c4..167d1b884 100644 --- a/src/model/qualified_identity/encrypted_key_storage.rs +++ b/src/model/qualified_identity/encrypted_key_storage.rs @@ -1,3 +1,4 @@ +use crate::backend_task::error::TaskError; use crate::model::qualified_identity::PrivateKeyTarget; use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; use crate::model::wallet::{Wallet, WalletSeedHash}; @@ -36,7 +37,7 @@ pub type ResolvedPrivateKey = (QualifiedIdentityPublicKey, Zeroizing<[u8; 32]>); /// only thing telling them apart — and a lookup that conflates them can hand out, /// or delete, material the requested key does not own. So this excludes the one /// field that legitimately moves and nothing else. -pub fn same_key(stored: &IdentityPublicKey, live: &IdentityPublicKey) -> bool { +pub(crate) fn same_key(stored: &IdentityPublicKey, live: &IdentityPublicKey) -> bool { use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; let IdentityPublicKey::V0(stored) = stored; @@ -253,10 +254,24 @@ impl fmt::Display for PrivateKeyData { /// The map is private on purpose. Which store a key is filed under is not /// something a caller should be deriving for itself — that is what produced a /// saved key no signing path could find — so reads go through -/// [`candidates`](Self::candidates), which selects on key material. The -/// remaining direct accessors exist for callers that legitimately name a -/// placement: a loader that knows structurally where a key belongs, and legacy -/// recovery, which is *about* specific stored placements. +/// [`candidates`](Self::candidates), which selects on key material. +/// +/// Every accessor that *names* a placement is `pub(crate)`, so the callers that +/// legitimately know one stay enumerable: the loader folding a previously-loaded +/// record into a fresh one, and legacy recovery, which is *about* the placements +/// an old blob recorded. [`insert_at`](Self::insert_at) and +/// [`entry_at`](Self::entry_at) have no production caller left at all and are +/// `#[cfg(test)]`. What stays `pub` cannot quietly file a key wrongly: the +/// target-blind enumerators, [`insert_non_encrypted`](Self::insert_non_encrypted), +/// which refuses an occupied slot, and the whole-map `From` conversions, where +/// the map's own keys are the placements. +// TODO(placement-named-pub-surface): the narrowing above is overstated — several +// pub methods still take a caller-named placement (get_resolve_local, +// get_resolve_with_seed, get_cloned_private_key_data_and_wallet_info, +// mark_in_vault, is_in_vault, public_key_for, wallet_seed_hash_for), and +// mark_in_vault zeroizes the occupant with no same_key guard. Narrow them to +// pub(crate) or guard them, then align this rustdoc and restore design.md §7's +// stronger claim (§8 tracks this residual). #[derive(Debug, Encode, Decode, Clone, PartialEq, Default)] pub struct KeyStorage { private_keys: BTreeMap<(PrivateKeyTarget, KeyID), (QualifiedIdentityPublicKey, PrivateKeyData)>, @@ -467,7 +482,7 @@ impl KeyStorage { .map(|(public_key, _)| public_key) } - pub fn has(&self, key: &(PrivateKeyTarget, KeyID)) -> bool { + pub(crate) fn has(&self, key: &(PrivateKeyTarget, KeyID)) -> bool { self.private_keys.contains_key(key) } @@ -500,6 +515,56 @@ impl KeyStorage { }) } + /// The placement a synchronous caller should name for `key`: the first + /// candidate whose bytes are resident, or the first candidate at all. + /// + /// [`candidates`](Self::candidates) can name a [`PrivateKeyData::InVault`] + /// placeholder ahead of an entry that carries its own material. Resolving + /// bytes is the honest way to choose between them, but that is async and a + /// UI frame cannot await, so this picks the placement whose material is + /// visible without opening the vault and falls back to the first match when + /// none is. Every synchronous site shares this one rule, so the placement a + /// screen names and the material it displays can never come from two + /// different stores. + pub fn first_live_candidate( + &self, + key: &IdentityPublicKey, + ) -> Option<(PrivateKeyTarget, KeyID)> { + // Two lazy passes rather than one collected Vec: this runs per key per + // frame, and the fallback re-probe costs at most three map reads. + self.candidates(key) + .find(|placement| !self.is_in_vault(placement)) + .or_else(|| self.candidates(key).next()) + } + + /// What this identity holds for `key` — its stored [`PrivateKeyData`] and + /// the wallet derivation info recorded with it — taken from the placement + /// [`first_live_candidate`](Self::first_live_candidate) names. + /// + /// Clones the entry, so it copies raw key bytes for a plaintext-carrying + /// key: call it when the material is actually needed, not to answer whether + /// a key is held. + pub fn held_private_key_data( + &self, + key: &IdentityPublicKey, + ) -> Option<(PrivateKeyData, Option)> { + let placement = self.first_live_candidate(key)?; + self.get_cloned_private_key_data_and_wallet_info(&placement) + } + + /// The wallet derivation path `key` is filed at, under any placement. + /// + /// A typed probe, not a liveness one: it answers "is this a wallet-derived + /// key, and from which wallet", so a key filed under two placements where + /// the first is not wallet-derived still finds its wallet. + pub fn wallet_derived_at(&self, key: &IdentityPublicKey) -> Option<&WalletDerivationPath> { + self.candidates(key) + .find_map(|placement| match self.private_keys.get(&placement) { + Some((_, PrivateKeyData::AtWalletDerivationPath(path))) => Some(path), + _ => None, + }) + } + /// Returns all stored key identifiers. pub fn keys_set(&self) -> BTreeSet<(PrivateKeyTarget, KeyID)> { self.private_keys.keys().cloned().collect() @@ -539,7 +604,8 @@ impl KeyStorage { /// Names a placement directly, so it answers "is *this* slot occupied", not /// "where is this key". Prefer [`candidates`](Self::candidates) for the /// latter: this cannot tell a key from a different one sharing its id. - pub fn entry_at( + #[cfg(test)] + pub(crate) fn entry_at( &self, key: &(PrivateKeyTarget, KeyID), ) -> Option<&(QualifiedIdentityPublicKey, PrivateKeyData)> { @@ -550,10 +616,13 @@ impl KeyStorage { /// /// For callers that know a placement structurally — a loader walking the /// identity list it read a key from, or legacy recovery restoring an entry - /// to the placement the old blob recorded. Anything choosing a placement for - /// *new* material should take it from - /// [`QualifiedIdentity::placement_of`](crate::model::qualified_identity::QualifiedIdentity::placement_of). - pub fn insert_at( + /// to the placement the old blob recorded. A caller that has to *choose* one + /// for new material asks + /// [`QualifiedIdentity::placement_of`](crate::model::qualified_identity::QualifiedIdentity::placement_of), + /// unless construction already fixes it: `add_key_to_identity` mints its key + /// at `max_id + 1` on the main identity, so only the main store can hold it. + #[cfg(test)] + pub(crate) fn insert_at( &mut self, key: (PrivateKeyTarget, KeyID), value: (QualifiedIdentityPublicKey, PrivateKeyData), @@ -565,7 +634,7 @@ impl KeyStorage { /// write. Used when folding a previously-loaded record into a fresh one, so /// a key the new load did not resupply is kept rather than dropped, and one /// it did resupply is not overwritten with the stale copy. - pub fn insert_if_absent( + pub(crate) fn insert_if_absent( &mut self, key: (PrivateKeyTarget, KeyID), value: (QualifiedIdentityPublicKey, PrivateKeyData), @@ -590,7 +659,7 @@ impl KeyStorage { /// Removing *a key* rather than a slot means removing every placement that /// holds it — see [`candidates`](Self::candidates), which selects on key /// material so a removal cannot land on a different key sharing the id. - pub fn remove_at( + pub(crate) fn remove_at( &mut self, key: &(PrivateKeyTarget, KeyID), ) -> Option<(QualifiedIdentityPublicKey, PrivateKeyData)> { @@ -604,22 +673,39 @@ impl KeyStorage { .collect() } - /// Inserts an unencrypted key into `ClearKeyStorage`. Returns an error if the storage is closed. + /// File a key's plaintext private half at exactly `key`, replacing that + /// key's own earlier entry if it has one. + /// + /// A `MEDIUM`-security key is stored as [`PrivateKeyData::AlwaysClear`], + /// anything else as [`PrivateKeyData::Clear`]. + /// + /// # Errors + /// + /// [`TaskError::IdentityKeySlotOccupied`] when the slot holds a *different* + /// key — one [`same_key`] rejects. The voter and main id spaces overlap, so + /// a slot can legitimately belong to another key, whose private half is + /// often the only copy there is; taking the slot would destroy it silently. pub fn insert_non_encrypted( &mut self, key: (PrivateKeyTarget, KeyID), value: (QualifiedIdentityPublicKey, [u8; 32]), - ) { - match value.0.identity_public_key.security_level() { - SecurityLevel::MEDIUM => { - self.private_keys - .insert(key, (value.0, PrivateKeyData::AlwaysClear(value.1))); - } - _ => { - self.private_keys - .insert(key, (value.0, PrivateKeyData::Clear(value.1))); - } + ) -> Result<(), TaskError> { + let (public_key, private_key_bytes) = value; + if let Some((occupant, _)) = self.private_keys.get(&key) + && !same_key( + &occupant.identity_public_key, + &public_key.identity_public_key, + ) + { + return Err(TaskError::IdentityKeySlotOccupied); } + + let data = match public_key.identity_public_key.security_level() { + SecurityLevel::MEDIUM => PrivateKeyData::AlwaysClear(private_key_bytes), + _ => PrivateKeyData::Clear(private_key_bytes), + }; + self.private_keys.insert(key, (public_key, data)); + Ok(()) } /// Mark `key` as a vault placeholder ([`PrivateKeyData::InVault`]), wiping @@ -913,6 +999,242 @@ mod tests { assert_no_leak_bytes(&rendered, &medium, "migrated KeyStorage blob (medium)"); } + const MAIN: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnMainIdentity; + + /// A slot already holding a *different* key is not free. The voter and main + /// id spaces overlap, so two keys sharing an id is an ordinary masternode + /// shape — and overwriting the occupant destroys the only copy of its + /// private half, in this map and in the vault it is the sole pointer to. + #[test] + fn a_different_key_cannot_take_an_occupied_slot() { + let pv = PlatformVersion::latest(); + let occupant = IdentityPublicKey::random_key(0, Some(1), pv); + let intruder = IdentityPublicKey::random_key(0, Some(2), pv); + assert_ne!( + occupant.data(), + intruder.data(), + "the fixture holds two distinct keys under one id" + ); + + let mut ks = KeyStorage::default(); + ks.insert_non_encrypted( + (MAIN, 0), + (QualifiedIdentityPublicKey::from(occupant), [0x11; 32]), + ) + .expect("an empty slot accepts the first key"); + let before = ks.entry_at(&(MAIN, 0)).cloned().expect("occupant stored"); + + let refused = ks.insert_non_encrypted( + (MAIN, 0), + (QualifiedIdentityPublicKey::from(intruder), [0x22; 32]), + ); + + assert!( + matches!(refused, Err(TaskError::IdentityKeySlotOccupied)), + "a foreign occupant must refuse the write, got {refused:?}" + ); + assert_eq!( + ks.entry_at(&(MAIN, 0)), + Some(&before), + "the occupant's entry survives the refused write unchanged" + ); + } + + /// The refusal's remedy must be one the user can actually perform. The + /// occupant is a *locally saved* private half, so removing it is the exit; + /// a refresh updates published keys but evicts no local private half — and + /// the backend add path has refetched the identity moments before this + /// refusal fires, so advising a refresh there sends the user around a loop + /// that recomputes the identical collision. + #[test] + fn the_occupied_slot_refusal_names_a_performable_remedy() { + let message = TaskError::IdentityKeySlotOccupied.to_string(); + assert!( + message.contains("remove its saved private key"), + "the remedy is removing the occupant's local private half, got: {message}" + ); + assert!( + !message.contains("Refresh"), + "refreshing cannot evict the occupying private half, got: {message}" + ); + } + + /// Re-entering the same key at its own slot replaces it. The paste path + /// relies on this to let a user correct a key already saved, rather than + /// growing a second copy under another store. + #[test] + fn the_same_key_still_overwrites_itself() { + let pv = PlatformVersion::latest(); + let key = IdentityPublicKey::random_key(0, Some(1), pv); + let mut ks = KeyStorage::default(); + ks.insert_non_encrypted( + (MAIN, 0), + (QualifiedIdentityPublicKey::from(key.clone()), [0x11; 32]), + ) + .expect("an empty slot accepts the key"); + + ks.insert_non_encrypted( + (MAIN, 0), + (QualifiedIdentityPublicKey::from(key), [0x22; 32]), + ) + .expect("a key may replace itself at its own slot"); + + assert_eq!(ks.len(), 1, "replacing in place adds no second entry"); + match ks.entry_at(&(MAIN, 0)).expect("entry present") { + (_, PrivateKeyData::Clear(bytes) | PrivateKeyData::AlwaysClear(bytes)) => { + assert_eq!(bytes, &[0x22; 32], "the re-entered bytes replaced the old") + } + (_, other) => panic!("expected plaintext bytes, got {other:?}"), + } + } + + const VOTER: PrivateKeyTarget = PrivateKeyTarget::PrivateKeyOnVoterIdentity; + + /// `disabled_at` is the one field Platform lets move after a key is added, + /// so a key disabled on chain since its private half was saved must still + /// match the stored snapshot — otherwise a key the device demonstrably + /// holds is reported missing the moment it is retired. + #[test] + fn same_key_ignores_a_key_being_disabled() { + use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; + + let stored = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + let mut disabled_since = stored.clone(); + disabled_since.set_disabled_at(1_700_000_000); + + assert!( + same_key(&stored, &disabled_since), + "a snapshot taken before the key was disabled still names that key" + ); + assert!( + same_key(&disabled_since, &stored), + "and the comparison does not depend on which side moved" + ); + } + + /// Every other field identifies the key. `read_only` stands for the six: + /// disagreeing on any of them makes this a different key, and treating it + /// as the same one hands out or deletes material it does not own. + #[test] + fn same_key_rejects_a_disagreement_anywhere_else() { + let stored = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + let IdentityPublicKey::V0(mut altered) = stored.clone(); + altered.read_only = !altered.read_only; + + assert!( + !same_key(&stored, &IdentityPublicKey::V0(altered)), + "a key differing outside disabled_at is not this key" + ); + } + + /// A storage filing one key under several placements, each with the given + /// stored data. + fn filed_under( + key: &IdentityPublicKey, + placements: &[(PrivateKeyTarget, PrivateKeyData)], + ) -> KeyStorage { + let mut ks = KeyStorage::default(); + for (target, data) in placements { + ks.insert_at( + (target.clone(), key.id()), + (QualifiedIdentityPublicKey::from(key.clone()), data.clone()), + ); + } + ks + } + + fn derivation_path(seed_hash: u8) -> WalletDerivationPath { + WalletDerivationPath { + wallet_seed_hash: [seed_hash; 32], + derivation_path: DerivationPath::from(vec![]), + } + } + + /// A vault placeholder probed first must not hide resident material for the + /// same key under a later placement — a screen that named the placeholder + /// would show a key it holds as unusable, and would send the vault a label + /// its bytes were never stored under. + #[test] + fn a_resident_placement_wins_over_a_vault_placeholder() { + let key = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + let ks = filed_under( + &key, + &[ + (MAIN, PrivateKeyData::InVault), + (VOTER, PrivateKeyData::Clear([0x33; 32])), + ], + ); + + assert_eq!( + ks.first_live_candidate(&key), + Some((VOTER, key.id())), + "the resident placement is the one to name" + ); + assert!( + matches!( + ks.held_private_key_data(&key), + Some((PrivateKeyData::Clear(bytes), None)) if bytes == [0x33; 32] + ), + "the material comes from the placement that was named" + ); + } + + /// With one placement there is nothing to prefer: it is named whether its + /// bytes are resident or in the vault, so a vault-backed key stays usable. + #[test] + fn a_lone_placement_is_named_whatever_it_holds() { + let key = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + + for data in [PrivateKeyData::InVault, PrivateKeyData::Clear([0x44; 32])] { + let ks = filed_under(&key, &[(MAIN, data.clone())]); + assert_eq!( + ks.first_live_candidate(&key), + Some((MAIN, key.id())), + "a lone {data:?} placement is still the answer" + ); + } + } + + /// A key nothing is filed for has no placement to name and no material. + #[test] + fn an_unheld_key_has_no_placement_and_no_material() { + let key = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + let ks = KeyStorage::default(); + + assert_eq!(ks.first_live_candidate(&key), None); + assert!(ks.held_private_key_data(&key).is_none()); + } + + /// `wallet_derived_at` asks a typed question, not a liveness one: a key + /// whose first placement carries its own plaintext still finds the wallet + /// it is derived from under a later placement. + #[test] + fn wallet_derived_at_looks_past_a_placement_that_is_not_derived() { + let key = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + let ks = filed_under( + &key, + &[ + (MAIN, PrivateKeyData::Clear([0x55; 32])), + ( + VOTER, + PrivateKeyData::AtWalletDerivationPath(derivation_path(0x66)), + ), + ], + ); + + assert_eq!( + ks.wallet_derived_at(&key).map(|path| path.wallet_seed_hash), + Some([0x66; 32]), + "the wallet is found under the placement that names it" + ); + + let plaintext_only = filed_under(&key, &[(MAIN, PrivateKeyData::Clear([0x55; 32]))]); + assert!( + plaintext_only.wallet_derived_at(&key).is_none(), + "a key no placement derives has no wallet" + ); + } + /// `is_in_vault` and `public_key_for` probes: a vault placeholder reports /// `true` and still surfaces its public key; a plaintext key reports /// `false`. diff --git a/src/model/qualified_identity/key_placement.rs b/src/model/qualified_identity/key_placement.rs index eb915365a..17db916d0 100644 --- a/src/model/qualified_identity/key_placement.rs +++ b/src/model/qualified_identity/key_placement.rs @@ -5,9 +5,12 @@ //! * *Where is this key's private half filed?* — [`KeyStorage::candidates`], //! which matches on the stored public-key material instead of trusting a //! derived target. Every read and delete asks this one. -//! * *Where should a private half we are about to store go?* — -//! [`QualifiedIdentity::placement_of`], which reads the identity's own -//! on-chain key lists. Only the write path asks this one. +//! * *Where does a private half we are about to store belong, and which list +//! names this key?* — [`QualifiedIdentity::placement_of`], which reads the +//! identity's own on-chain key lists. The Key Info paste path asks it to +//! choose a store; role naming asks it to label a key. `add_key_to_identity` +//! does not ask at all: it mints its key at `max_id + 1` on the main +//! identity, so no other list can publish it. //! //! [`KeyStorage::candidates`]: super::encrypted_key_storage::KeyStorage::candidates //! [`QualifiedIdentity::placement_of`]: super::QualifiedIdentity::placement_of diff --git a/src/model/qualified_identity/mod.rs b/src/model/qualified_identity/mod.rs index b5659d650..56a5161ad 100644 --- a/src/model/qualified_identity/mod.rs +++ b/src/model/qualified_identity/mod.rs @@ -8,7 +8,9 @@ pub mod qualified_identity_public_key; // requires making that secret-seam chokepoint generic over the closure error // type — a wallet_backend change out of scope here. use crate::backend_task::error::TaskError; -use crate::model::qualified_identity::encrypted_key_storage::{KeyStorage, ResolvedPrivateKey}; +use crate::model::qualified_identity::encrypted_key_storage::{ + KeyStorage, ResolvedPrivateKey, same_key, +}; use crate::model::qualified_identity::key_placement::KeyPlacement; use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; use crate::model::user_role::UserRole; @@ -607,8 +609,10 @@ impl QualifiedIdentity { /// /// Reads the identity's own on-chain records (main, voter, operator), /// matching on key id **and** public-key data, so a key id that appears on - /// two lists with different material cannot be confused. Used by the write - /// path only; a read asks + /// two lists with different material cannot be confused. Asked by the Key + /// Info paste path, to choose a store for a private half the user just + /// entered, and by role naming, to label a key by the list that publishes + /// it. A read asks /// [`KeyStorage::candidates`](encrypted_key_storage::KeyStorage::candidates) /// where the private half actually is, which is not always the same answer. /// @@ -638,7 +642,11 @@ impl QualifiedIdentity { .into_iter() .filter_map(|(target, identity)| { let published = identity?.public_keys().get(&key.id())?; - (published.data() == key.data()).then_some(target) + // The same rule `candidates` files a key by. Material alone + // cannot tell a main identity's voting key from a voter + // identity's — they can carry identical `data` under one id — + // so comparing it would call an unambiguous key ambiguous. + same_key(published, key).then_some(target) }) .collect(); @@ -665,6 +673,13 @@ impl QualifiedIdentity { /// the first match would report such a key unusable while its bytes are one /// probe away. /// + /// The walk is resident-first: placements whose bytes are resident resolve + /// with no chokepoint access and are tried before vault-backed or + /// wallet-derived ones, so a sealed copy of a key never puts a password + /// prompt in front of a sibling placement holding the same bytes in the + /// clear — the same rule [`KeyStorage::first_live_candidate`] applies + /// synchronously. Within each group, probe order decides. + /// /// Because the placement is discovered rather than supplied, the vault scope /// is built from the store the bytes were **found** under. The map key and /// the vault label are one composite address, so a caller cannot be trusted @@ -686,7 +701,11 @@ impl QualifiedIdentity { /// /// `Ok(None)` when no placement holds this key. When every candidate failed, /// the first failure is returned rather than `None`, so a lone dead entry - /// still surfaces its own typed error instead of a silent miss. + /// still surfaces its own typed error instead of a silent miss. A cancelled + /// prompt ([`TaskError::SecretPromptCancelled`]) ends the walk immediately + /// and is itself the error returned, outranking any earlier placement's + /// mechanical failure — it is the user's answer about this key, and asking + /// again for the next placement would be one dialog per store. /// /// [`PrivateKeyData::AtWalletDerivationPath`]: encrypted_key_storage::PrivateKeyData::AtWalletDerivationPath /// [`PrivateKeyData::InVault`]: encrypted_key_storage::PrivateKeyData::InVault @@ -697,13 +716,36 @@ impl QualifiedIdentity { ) -> Result, TaskError> { let mut first_failure = None; - for (target, key_id) in self.private_keys.candidates(key).collect::>() { + // Prompt-free placements first: an entry that is neither vault-backed + // nor wallet-derived carries its bytes resident and resolves with no + // chokepoint access, so a sealed copy of the key never puts a password + // prompt — or its cancellation — in front of a sibling holding the + // same bytes in the clear. The async mirror of `first_live_candidate`; + // within each group, probe order. + let (resident, prompting): (Vec<_>, Vec<_>) = + self.private_keys.candidates(key).partition(|placement| { + !self.private_keys.is_in_vault(placement) + && self.private_keys.wallet_seed_hash_for(placement).is_none() + }); + + for (target, key_id) in resident.into_iter().chain(prompting) { match self.resolve_private_key_bytes_at(target, key_id).await { Ok(Some(resolved)) => return Ok(Some(resolved)), // This placement holds no usable bytes. Keep looking: another // store may hold the same key's live material. Ok(None) => {} Err(failure) => { + // A cancellation answers for the key, not for one store of + // it: trying the next candidate would re-ask for what the + // user just declined, one dialog per placement — and the + // user's decision outranks an earlier placement's + // mechanical failure, so it is returned as-is. + // `SecretPromptUnavailable` is not a decision but a + // property of the host (no window to ask in), so a sibling + // placement that needs no prompt is still worth trying. + if matches!(failure, TaskError::SecretPromptCancelled) { + return Err(failure); + } if first_failure.is_none() { first_failure = Some(failure); } @@ -804,7 +846,7 @@ impl QualifiedIdentity { /// `BTreeMap`, so the first key is the lowest seed /// hash — a stable, content-derived choice that does not depend on /// insertion order. Both sides call this one helper so the rule lives in - /// exactly one place (SEC-W-001). + /// exactly one place. pub fn dashpay_wallet_seed_hash(&self) -> Option { self.associated_wallets.keys().next().copied() } @@ -1229,6 +1271,7 @@ mod key_placement_tests { use super::*; use crate::model::qualified_identity::encrypted_key_storage::PrivateKeyData; use crate::model::qualified_identity::key_placement::{KeyPlacement, PROBE_ORDER}; + use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dash_sdk::dpp::version::PlatformVersion; use dash_sdk::platform::Identifier; @@ -1592,6 +1635,51 @@ mod key_placement_tests { assert_eq!(identity.placement_of(&fresh).resolved(), None); } + /// Two keys can share an id *and* their material and still be two keys — + /// `purpose` is what tells a main identity's voting key from a voter + /// identity's. Comparing material alone reports both lists as publishing + /// one key, so the paste path would refuse to file a key whose placement + /// was never in doubt. + #[test] + fn two_keys_sharing_id_and_material_are_placed_by_purpose() { + let on_main = key(0, Purpose::VOTING, 0xAA); + let on_voter = key(0, Purpose::AUTHENTICATION, 0xAA); + assert_eq!( + on_main.data(), + on_voter.data(), + "the fixture shares material" + ); + + let identity = qi( + std::slice::from_ref(&on_main), + Some(std::slice::from_ref(&on_voter)), + &[], + ); + + assert_eq!( + identity.placement_of(&on_main), + KeyPlacement::Resolved(MAIN), + "the main identity's voting key belongs to Main, not to both lists" + ); + assert_eq!( + identity.placement_of(&on_voter), + KeyPlacement::Resolved(VOTER), + ); + } + + /// A key disabled on chain since its private half was saved is still the + /// same key, so its placement is still answerable — `disabled_at` is the + /// one field Platform lets move. + #[test] + fn a_key_disabled_since_it_was_saved_still_has_a_placement() { + let saved = key(2, Purpose::AUTHENTICATION, 0xAA); + let mut disabled = saved.clone(); + disabled.set_disabled_at(1_700_000_000); + let identity = qi(std::slice::from_ref(&disabled), None, &[]); + + assert_eq!(identity.placement_of(&saved), KeyPlacement::Resolved(MAIN)); + } + /// The same key published on two lists is `Ambiguous` — reported, never /// silently collapsed to one of them. #[test] @@ -1767,6 +1855,187 @@ mod key_resolution_tests { ); } + /// Cancelling the password prompt answers for the key, not for one of the + /// stores it happens to be filed under. Carrying on to the next candidate + /// re-asks for the key the user just declined to unlock — one dialog per + /// placement, each looking like the app ignored the last answer. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn cancelling_the_prompt_stops_asking_for_the_same_key() { + use crate::wallet_backend::secret_prompt::test_support::{ScriptedAnswer, TestPrompt}; + use crate::wallet_backend::secret_seam::SecretSeam; + use crate::wallet_backend::single_key::open_secret_store; + use crate::wallet_backend::{SecretAccess, SecretScope}; + use platform_wallet_storage::secrets::{ + SecretBytes, SecretString, WalletId as SecretWalletId, + }; + + let key = voting_key(0); + // The id `masternode_with` builds its identity under. + let identity_id = [1u8; 32]; + + let dir = tempfile::tempdir().expect("tempdir"); + let store = + Arc::new(open_secret_store(&dir.path().join("secrets.pwsvault")).expect("open vault")); + // Sealed Tier-2 under both placements: either one alone would prompt, + // so the prompt count is what tells stopping from carrying on. + for target in [MAIN, VOTER] { + SecretSeam::new(&store) + .put_secret_protected( + &SecretWalletId::from(identity_id), + &SecretScope::identity_key_label(&target, key.id()), + &SecretBytes::from_slice(&[0x99; 32]), + &SecretString::new("the-object-password"), + ) + .expect("seal the identity key"); + } + + let prompt = Arc::new(TestPrompt::new([ + ScriptedAnswer::Cancel, + ScriptedAnswer::Cancel, + ])); + let mut identity = masternode_with( + &key, + &[ + (MAIN, PrivateKeyData::InVault), + (VOTER, PrivateKeyData::InVault), + ], + ); + identity.secret_access = Some(SecretAccess::new(store, prompt.clone(), Network::Testnet)); + + let error = identity + .resolve_private_key_bytes(&key) + .await + .expect_err("a cancelled prompt resolves nothing"); + assert!( + matches!(error, TaskError::SecretPromptCancelled), + "the user's refusal is what surfaces, got {error:?}", + ); + assert_eq!( + prompt.ask_count(), + 1, + "one refusal ends the attempt; it must not open the next placement's prompt", + ); + } + + /// The cancellation is the answer that surfaces even when an earlier + /// placement already failed for a mechanical reason. A dead placeholder + /// probed ahead of a sealed placement must not have its failure reported + /// over the user's own refusal — the user dismissed a password prompt and + /// would otherwise be told the key is missing from this device. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_cancellation_outranks_an_earlier_placements_failure() { + use crate::wallet_backend::secret_prompt::test_support::{ScriptedAnswer, TestPrompt}; + use crate::wallet_backend::secret_seam::SecretSeam; + use crate::wallet_backend::single_key::open_secret_store; + use crate::wallet_backend::{SecretAccess, SecretScope}; + use platform_wallet_storage::secrets::{ + SecretBytes, SecretString, WalletId as SecretWalletId, + }; + + let key = voting_key(0); + // The id `masternode_with` builds its identity under. + let identity_id = [1u8; 32]; + + let dir = tempfile::tempdir().expect("tempdir"); + let store = + Arc::new(open_secret_store(&dir.path().join("secrets.pwsvault")).expect("open vault")); + // Only the second-probed placement is sealed (Tier-2, so it prompts). + // The first-probed placement is a dead placeholder: an `InVault` entry + // with no vault secret behind it, which fails without a prompt. + SecretSeam::new(&store) + .put_secret_protected( + &SecretWalletId::from(identity_id), + &SecretScope::identity_key_label(&VOTER, key.id()), + &SecretBytes::from_slice(&[0x99; 32]), + &SecretString::new("the-object-password"), + ) + .expect("seal the identity key"); + + let prompt = Arc::new(TestPrompt::new([ScriptedAnswer::Cancel])); + let mut identity = masternode_with( + &key, + &[ + (MAIN, PrivateKeyData::InVault), + (VOTER, PrivateKeyData::InVault), + ], + ); + identity.secret_access = Some(SecretAccess::new(store, prompt.clone(), Network::Testnet)); + + let error = identity + .resolve_private_key_bytes(&key) + .await + .expect_err("a cancelled prompt resolves nothing"); + assert!( + matches!(error, TaskError::SecretPromptCancelled), + "the user's refusal outranks the dead placeholder's failure, got {error:?}", + ); + assert_eq!( + prompt.ask_count(), + 1, + "only the sealed placement prompts; the dead placeholder must not", + ); + } + + /// A prompt belongs only to a sealed copy of a key. When a sibling + /// placement holds the same key's bytes in the clear, that copy resolves + /// with no vault access — so probing the sealed copy first would put a + /// password dialog, and its cancellation, in front of bytes the identity + /// already holds. The walk must take prompt-free placements first: the + /// resident-first rule `first_live_candidate` applies synchronously. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_resident_sibling_resolves_without_prompting_for_a_sealed_copy() { + use crate::wallet_backend::secret_prompt::test_support::{ScriptedAnswer, TestPrompt}; + use crate::wallet_backend::secret_seam::SecretSeam; + use crate::wallet_backend::single_key::open_secret_store; + use crate::wallet_backend::{SecretAccess, SecretScope}; + use platform_wallet_storage::secrets::{ + SecretBytes, SecretString, WalletId as SecretWalletId, + }; + + let key = voting_key(0); + // The id `masternode_with` builds its identity under. + let identity_id = [1u8; 32]; + let secret = [0x55; 32]; + + let dir = tempfile::tempdir().expect("tempdir"); + let store = + Arc::new(open_secret_store(&dir.path().join("secrets.pwsvault")).expect("open vault")); + // Main — probed first — is sealed Tier-2, so resolving it opens a + // password prompt. Voter carries the same key in the clear. + SecretSeam::new(&store) + .put_secret_protected( + &SecretWalletId::from(identity_id), + &SecretScope::identity_key_label(&MAIN, key.id()), + &SecretBytes::from_slice(&[0x99; 32]), + &SecretString::new("the-object-password"), + ) + .expect("seal the identity key"); + + // Scripted to cancel, so a walk that opens the sealed copy's prompt + // fails loudly instead of quietly answering it. + let prompt = Arc::new(TestPrompt::new([ScriptedAnswer::Cancel])); + let mut identity = masternode_with( + &key, + &[ + (MAIN, PrivateKeyData::InVault), + (VOTER, PrivateKeyData::Clear(secret)), + ], + ); + identity.secret_access = Some(SecretAccess::new(store, prompt.clone(), Network::Testnet)); + + let (_, resolved) = identity + .resolve_private_key_bytes(&key) + .await + .expect("a prompt-free copy exists, so resolution must not fail") + .expect("the resident copy must be found"); + assert_eq!(*resolved, secret, "the resident bytes are the ones served"); + assert_eq!( + prompt.ask_count(), + 0, + "a key held in the clear must resolve without any prompt", + ); + } + /// A key this identity holds no private half for resolves to `None` — an /// absence, not an error, and never another key's material. #[tokio::test] @@ -2278,9 +2547,9 @@ mod withdrawal_key_tests { } } -/// Regression coverage for the `from_bytes` decode-limit fix (SEC-001 from the -/// PR #885 grumpy-review): a corrupted or length-inflated blob must decode to -/// a graceful `Err`, never abort the process. +/// Regression coverage for the `from_bytes` decode-limit fix (PR #885): a +/// corrupted or length-inflated blob must decode to a graceful `Err`, never +/// abort the process. /// /// This deliberately does NOT decode a full `QualifiedIdentity` blob. Crafting /// a byte-exact corruption of a real encoded identity is fragile — it would diff --git a/src/ui/components/message_banner.rs b/src/ui/components/message_banner.rs index 99c21443f..608eead12 100644 --- a/src/ui/components/message_banner.rs +++ b/src/ui/components/message_banner.rs @@ -837,6 +837,13 @@ fn get_banners(ctx: &egui::Context) -> Vec { .unwrap_or_default() } +/// The texts of every global banner currently set, oldest first. Test-only: +/// lets a unit test assert which message a screen surfaced. +#[cfg(test)] +pub(crate) fn global_banner_texts(ctx: &egui::Context) -> Vec { + get_banners(ctx).into_iter().map(|b| b.text).collect() +} + /// Writes the global banner list to egui context data. /// Removes the entry entirely when the list is empty. fn set_banners(ctx: &egui::Context, banners: Vec) { diff --git a/src/ui/identities/identities_screen.rs b/src/ui/identities/identities_screen.rs index d4a17f94b..3dce69f79 100644 --- a/src/ui/identities/identities_screen.rs +++ b/src/ui/identities/identities_screen.rs @@ -4,9 +4,6 @@ use crate::backend_task::BackendTask; use crate::backend_task::identity::IdentityTask; use crate::context::AppContext; use crate::model::contested_name::PendingUsername; -use crate::model::qualified_identity::PrivateKeyTarget::{ - PrivateKeyOnMainIdentity, PrivateKeyOnVoterIdentity, -}; use crate::model::qualified_identity::{IdentityStatus, IdentityType, QualifiedIdentity}; use crate::model::wallet::WalletSeedHash; use crate::ui::components::left_panel::add_left_panel; @@ -809,12 +806,17 @@ impl IdentitiesScreen { // Main Identity Keys if !public_keys.is_empty() { - for (key_id, key) in public_keys.iter() { - let holding_private_key = qualified_identity.private_keys - .get_cloned_private_key_data_and_wallet_info(&(PrivateKeyOnMainIdentity, *key_id)); + for key in public_keys.values() { + // Placement-blind: the private half may be filed + // under a store an older build chose. A presence + // check rather than a fetch: cloning the entry + // copies raw key bytes, and this runs every frame + // for every key. + let held = qualified_identity.private_keys + .candidates(key).next().is_some(); let key_label = self.format_key_name(key); - let button = if holding_private_key.is_some() { + let button = if held { egui::Button::new(&key_label).fill(DashColors::selected(dark_mode)) } else { egui::Button::new(&key_label) @@ -824,7 +826,7 @@ impl IdentitiesScreen { action |= AppAction::AddScreen(Screen::KeyInfoScreen(KeyInfoScreen::new( qualified_identity.clone(), key.clone(), - holding_private_key, + qualified_identity.private_keys.held_private_key_data(key), &self.app_context, ))); ui.close_kind(egui::UiKind::Menu); @@ -840,12 +842,14 @@ impl IdentitiesScreen { ui.add_space(5.0); } - for (key_id, key) in voter_public_keys.iter() { - let holding_private_key = qualified_identity.private_keys - .get_cloned_private_key_data_and_wallet_info(&(PrivateKeyOnVoterIdentity, *key_id)); + for key in voter_public_keys.values() { + // A presence check, as above — material is + // fetched only on the click that needs it. + let held = qualified_identity.private_keys + .candidates(key).next().is_some(); let key_label = self.format_key_name(key); - let button = if holding_private_key.is_some() { + let button = if held { egui::Button::new(&key_label).fill(DashColors::selected(dark_mode)) } else { egui::Button::new(&key_label) @@ -855,7 +859,7 @@ impl IdentitiesScreen { action |= AppAction::AddScreen(Screen::KeyInfoScreen(KeyInfoScreen::new( qualified_identity.clone(), key.clone(), - holding_private_key, + qualified_identity.private_keys.held_private_key_data(key), &self.app_context, ))); ui.close_kind(egui::UiKind::Menu); diff --git a/src/ui/identities/keys/key_info_screen.rs b/src/ui/identities/keys/key_info_screen.rs index e36230fb8..b2a458e72 100644 --- a/src/ui/identities/keys/key_info_screen.rs +++ b/src/ui/identities/keys/key_info_screen.rs @@ -8,6 +8,7 @@ use crate::model::legacy_recovery::RecoveryItem; use crate::model::qualified_identity::encrypted_key_storage::{ PrivateKeyData, WalletDerivationPath, }; +use crate::model::qualified_identity::key_placement::KeyPlacement; use crate::model::qualified_identity::{PrivateKeyTarget, QualifiedIdentity}; use crate::model::secret::Secret; use crate::model::wallet::Wallet; @@ -297,7 +298,7 @@ impl ScreenLike for KeyInfoScreen { ui.label(RichText::new("Purpose:").strong().color(text_primary)); let (role, role_tip) = key_role_label( KeyVocabulary::from(self.identity.identity_type), - &self.naming_target(), + &self.published_on(), &self.key, ); let purpose_label = ui.label(RichText::new(role).color(text_primary)); @@ -761,10 +762,11 @@ impl ScreenLike for KeyInfoScreen { let wants_display = std::mem::take(&mut self.pending_identity_key_display); let wants_sign = std::mem::take(&mut self.pending_identity_sign); if wants_display || wants_sign { - // The vault stores each key under the store it is filed in, so the - // request has to name the placement the material is actually at. - match self.target() { - Some(target) => { + // The request names the placement this screen sees; the backend + // chokepoint verifies that label is live and falls through to a + // sibling placement filing the same key when it is not. + match self.filed_at() { + Ok(target) => { if wants_display { action |= AppAction::BackendTask(BackendTask::WalletTask( WalletTask::DeriveIdentityKeyForDisplay { @@ -786,12 +788,10 @@ impl ScreenLike for KeyInfoScreen { )); } } - None => { - MessageBanner::set_global( - ctx, - "This key is not saved on this device, so it cannot be shown or used to sign.", - MessageType::Error, - ); + Err(error) => { + // Each failure mode speaks its own typed message and + // remedy, worded for this read path as much as the paste. + MessageBanner::set_global_with_error(ctx, error); } } } @@ -901,14 +901,14 @@ impl KeyInfoScreen { /// The store this key is *published* under, for naming it. /// - /// Deliberately the structural answer rather than [`Self::target`]'s: a key's + /// Deliberately the structural answer rather than [`Self::filed_at`]'s: a key's /// name follows the identity list it belongs to — which is what /// `identity_keys` pairs it with on every list that shows it — not wherever /// its private half happens to be filed. Naming it from the material's /// location would let one key be called two things depending on which build /// saved it. `Unknown` names the main identity, which is where a key not yet /// on any list is being added. - fn naming_target(&self) -> PrivateKeyTarget { + fn published_on(&self) -> PrivateKeyTarget { self.identity .placement_of(&self.key) .resolved() @@ -951,26 +951,51 @@ impl KeyInfoScreen { /// nothing is held does the identity's on-chain lists decide where a new /// private half would go. /// - /// `None` when the key is on none of this identity's lists and nothing is - /// held for it, which is the one case where no store can be named honestly. - fn target(&self) -> Option { - self.identity - .private_keys - .candidates(&self.key) - .next() - .map(|(target, _)| target) - .or_else(|| self.identity.placement_of(&self.key).resolved()) + /// The placement is picked by + /// [`first_live_candidate`](crate::model::qualified_identity::encrypted_key_storage::KeyStorage::first_live_candidate), + /// the same rule that produced the `private_key_data` this screen displays. + /// The two must agree: the vault reads the label this names, so a screen + /// naming one store while showing material from another asks the vault for + /// a key it never stored. + /// + /// # Errors + /// + /// With nothing held for the key, the identity's own lists decide, and both + /// their failure modes are distinct answers the user can act on: + /// [`TaskError::IdentityKeyPlacementAmbiguous`] when several lists publish + /// it, [`TaskError::IdentityKeyNotOnIdentityRecord`] when none does. + fn filed_at(&self) -> Result { + Self::filed_at_in(&self.identity, &self.key) + } + + /// [`Self::filed_at`]'s rule against a caller-supplied record. The paste + /// path applies it to the record freshly re-read inside its locked + /// read-modify-write, where `self.identity` would be the stale clone. + fn filed_at_in( + identity: &QualifiedIdentity, + key: &IdentityPublicKey, + ) -> Result { + if let Some((target, _)) = identity.private_keys.first_live_candidate(key) { + return Ok(target); + } + match identity.placement_of(key) { + KeyPlacement::Resolved(target) => Ok(target), + KeyPlacement::Ambiguous(_) => Err(TaskError::IdentityKeyPlacementAmbiguous), + KeyPlacement::Unknown => Err(TaskError::IdentityKeyNotOnIdentityRecord), + } } /// Re-read this screen's identity from the store, after a backend task /// wrote it. /// - /// The screen keeps a clone taken when it opened, and its own key add / - /// remove paths persist that whole clone. Any change another writer makes - /// while the screen is open therefore has to be picked up here, or the next - /// key edit writes it away. The key on screen is refreshed from the same - /// record. A read failure leaves the clone alone and says so — the change - /// landed, this screen just cannot show it. + /// The screen keeps a clone taken when it opened. Its key add / remove + /// paths persist through a locked read-modify-write of the record and + /// adopt the result, so a stale clone cannot write another writer's change + /// away — but everything the screen *shows* between edits comes from the + /// clone, so a change another writer makes still has to be picked up here. + /// The key on screen is refreshed from the same record. A read failure + /// leaves the clone alone and says so — the change landed, this screen + /// just cannot show it. fn reload_identity(&mut self) { let identity_id = self.identity.identity.id(); match self.app_context.get_local_qualified_identity(&identity_id) { @@ -978,16 +1003,7 @@ impl KeyInfoScreen { // Resolved against the record just read, not the stale clone: // the write being picked up here may be the one that filed this // key in the first place. - self.private_key_data = - fresh - .private_keys - .candidates(&self.key) - .next() - .and_then(|placement| { - fresh - .private_keys - .get_cloned_private_key_data_and_wallet_info(&placement) - }); + self.private_key_data = fresh.private_keys.held_private_key_data(&self.key); self.identity = fresh; } Ok(None) => {} @@ -1085,35 +1101,50 @@ impl KeyInfoScreen { ) .with_details(error); } else if validation_result.expect("invariant: Err handled in the preceding branch") { - // If valid, store the private key in the context and reset the input field - self.private_key_data = Some((PrivateKeyData::Clear(private_key_bytes), None)); - // An existing placement is reused so a re-entered key overwrites - // itself rather than growing a second copy under another store; - // otherwise the identity's own lists say where it belongs. Both - // agree with where the resolver will look for it. - let Some(target) = self.target() else { - MessageBanner::set_global( - self.app_context.egui_ctx(), - "This key does not belong to this identity, so it cannot be saved here.", - MessageType::Error, - ); - return; - }; - self.identity.private_keys.insert_non_encrypted( - (target, self.key.id()), - (self.key.clone().into(), private_key_bytes), - ); - if let Err(error) = self + // Every reason the key might not be storable is settled before the + // screen calls it held: a key shown as held offers to sign, to be + // revealed and to be removed, none of which a refused key can do. + // + // The whole edit is a locked read-modify-write of the record on + // disk, so both the placement decision and the occupied-slot check + // see what is stored *now* — a key another writer landed while + // this screen was open survives the paste instead of being written + // away with the screen's stale clone. An existing placement is + // reused so a re-entered key overwrites itself rather than growing + // a second copy under another store; otherwise the identity's own + // lists say where it belongs. + let identity_id = self.identity.identity.id(); + let updated = self .app_context - .update_local_qualified_identity(&self.identity) - { - let handle = MessageBanner::set_global( - self.app_context.egui_ctx(), - "The private key could not be saved. Check available disk space and try again.", - MessageType::Error, - ); - handle.with_details(error); - handle.disable_auto_dismiss(); + .edit_local_qualified_identity(&identity_id, |fresh| { + let target = Self::filed_at_in(fresh, &self.key)?; + fresh.private_keys.insert_non_encrypted( + (target, self.key.id()), + (self.key.clone().into(), private_key_bytes), + ) + }); + match updated { + Ok(fresh) => { + // Resolved from the record just persisted, so the screen + // shows exactly what was filed. + self.private_key_data = fresh.private_keys.held_private_key_data(&self.key); + self.identity = fresh; + } + Err(error) => { + // An input refusal dismisses on its own; a record that + // could not be persisted stays up until acknowledged. + let refused_input = matches!( + error, + TaskError::IdentityKeySlotOccupied + | TaskError::IdentityKeyPlacementAmbiguous + | TaskError::IdentityKeyNotOnIdentityRecord + ); + let banner = + MessageBanner::set_global_with_error(self.app_context.egui_ctx(), error); + if !refused_input { + banner.disable_auto_dismiss(); + } + } } } else { MessageBanner::set_global( @@ -1302,20 +1333,14 @@ impl KeyInfoScreen { if result == ConfirmationStatus::Confirmed && let Err(error) = self.remove_held_private_key() { - let handle = MessageBanner::set_global( - ui.ctx(), - "The private-key change could not be saved. Check available disk space and try again.", - MessageType::Error, - ); - handle.with_details(error); - handle.disable_auto_dismiss(); + MessageBanner::set_global_with_error(ui.ctx(), error).disable_auto_dismiss(); } } } } - /// Drop this device's copy of the on-screen key's private half and persist - /// the record. + /// Drop this device's copy of the on-screen key's private half — its vault + /// secret and its record entry both — and persist the result. /// /// Removes **every** placement holding *this* key, so a duplicate written /// under another convention cannot survive the removal the user asked for. @@ -1325,18 +1350,39 @@ impl KeyInfoScreen { /// on whichever key happens to occupy the derived slot, and on a masternode /// the voter and main id spaces overlap, so that can be a different key /// entirely. + /// + /// Vault first, then the record: the stored placement is the only thing + /// that makes a vault label enumerable, so bytes outliving their entry are + /// bytes nothing can reach or delete afterwards. A vault-secret failure + /// aborts with the record, the screen, and the vault as they were, + /// leaving a removal the user can simply repeat. + /// + /// The whole removal is a locked read-modify-write of the record on disk, + /// so the placements deleted are the ones stored *now* — a key another + /// writer landed while this screen was open survives, instead of being + /// written away with the screen's stale clone — and the screen's own + /// state changes only after the persist succeeds, so it never reports a + /// key as gone that the record on disk still holds. A persist refusal + /// reached *after* the vault secrets are already gone leaves the stored + /// map entry as the only remaining place tracking the key as held; + /// retrying the removal finds an already-empty vault and simply finishes + /// the job. fn remove_held_private_key(&mut self) -> Result<(), TaskError> { + let identity_id = self.identity.identity.id(); + let updated = self + .app_context + .edit_local_qualified_identity(&identity_id, |fresh| { + let placements: Vec<_> = fresh.private_keys.candidates(&self.key).collect(); + self.app_context + .delete_identity_key_secrets(&identity_id, placements.clone())?; + for placement in &placements { + fresh.private_keys.remove_at(placement); + } + Ok(()) + })?; self.private_key_data = None; - for placement in self - .identity - .private_keys - .candidates(&self.key) - .collect::>() - { - self.identity.private_keys.remove_at(&placement); - } - self.app_context - .update_local_qualified_identity(&self.identity) + self.identity = updated; + Ok(()) } // --- Identity key password protection (per-identity at-rest key encryption) --- @@ -1826,6 +1872,51 @@ mod tests { ); } + /// The bytes behind a saved key live in the vault, addressed by the same + /// placement the stored map files the key under. Removing this device's copy + /// has to take those bytes with it: the map entry is what makes the vault + /// label enumerable, so a secret left behind once the entry is gone is + /// reachable by nothing — not even the whole-identity delete sweep, which + /// reads its delete set from that same map. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn removing_a_key_also_removes_its_vault_secret() { + let (app_context, _dir) = offline_ctx().await; + + let on_screen = public_key(3, Purpose::AUTHENTICATION); + let stored = identity_with(0x7C, &[(on_screen.clone(), [0x33; 32])]); + let identity_id = stored.identity.id(); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + + let backend = app_context.wallet_backend().expect("backend"); + let vault = IdentityKeyView::new(backend.secret_store(), identity_id.to_buffer()); + assert_eq!( + vault + .get(&MAIN, on_screen.id()) + .expect("vault read") + .as_deref() + .copied(), + Some([0x33; 32]), + "saving the key put its bytes in the vault", + ); + + let mut screen = KeyInfoScreen::new(stored, on_screen.clone(), None, &app_context); + screen + .remove_held_private_key() + .expect("the removal must persist"); + + assert!( + vault + .get(&MAIN, on_screen.id()) + .expect("vault read") + .is_none(), + "the removed key's bytes must not outlive the record that points at them", + ); + + backend.shutdown().await; + } + /// Write `key` into `identity_id`'s stored record, the way a restore or any /// other backend writer does — behind whatever screen holds a clone of it. fn write_key_behind_the_screen( @@ -1937,6 +2028,378 @@ mod tests { .await; } + /// The paste persists through a locked read-modify-write of the record on + /// disk — never by writing out the clone this screen opened with. A key + /// another writer lands while the screen is open must survive the next + /// paste, with no refresh delivered in between; nothing at runtime + /// guarantees one arrives. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_write_behind_the_screen_survives_a_paste() { + let (app_context, _dir) = offline_ctx().await; + + let (pasted, secret) = keypair(7, Purpose::AUTHENTICATION); + let mut stored = identity_with(0x51, &[]); + stored.identity = Identity::new_with_id_and_keys( + Identifier::from([0x51; 32]), + BTreeMap::from([(pasted.id(), pasted.clone())]), + PlatformVersion::latest(), + ) + .expect("identity publishing the pasted key"); + let identity_id = stored.identity.id(); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + let mut screen = KeyInfoScreen::new(stored, pasted.clone(), None, &app_context); + + // Another writer lands a key after the screen took its clone. + let concurrent = public_key(2, Purpose::TRANSFER); + write_key_behind_the_screen(&app_context, identity_id, &concurrent, [0x22; 32]); + + screen.private_key_input.set_text(hex::encode(secret)); + screen.validate_and_store_private_key(); + + let on_disk = app_context + .get_local_qualified_identity(&identity_id) + .expect("read back") + .expect("still stored"); + assert!( + on_disk.private_keys.has(&(MAIN, concurrent.id())), + "the concurrent writer's key must survive the paste", + ); + assert!( + on_disk.private_keys.candidates(&pasted).next().is_some(), + "and the pasted key was saved alongside it", + ); + assert!( + screen.identity.private_keys.has(&(MAIN, concurrent.id())), + "the screen must adopt the record it persisted, not keep its stale clone", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// The removal's mirror of `a_write_behind_the_screen_survives_a_paste`: + /// removing one key must not write away a key another writer landed while + /// the screen was open — for a private key, silently and irreversibly. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_write_behind_the_screen_survives_a_removal() { + let (app_context, _dir) = offline_ctx().await; + + let on_screen = public_key(1, Purpose::AUTHENTICATION); + let stored = identity_with(0x52, &[(on_screen.clone(), [0x11; 32])]); + let identity_id = stored.identity.id(); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + let mut screen = KeyInfoScreen::new(stored, on_screen.clone(), None, &app_context); + + let concurrent = public_key(2, Purpose::TRANSFER); + write_key_behind_the_screen(&app_context, identity_id, &concurrent, [0x22; 32]); + + screen + .remove_held_private_key() + .expect("the removal must persist"); + + let on_disk = app_context + .get_local_qualified_identity(&identity_id) + .expect("read back") + .expect("still stored"); + assert!( + on_disk.private_keys.has(&(MAIN, concurrent.id())), + "the concurrent writer's key must survive the removal", + ); + assert!( + on_disk.private_keys.candidates(&on_screen).next().is_none(), + "while the key the user asked to remove is gone", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// A key and the private key that opens it, so the paste path's validation + /// passes and the placement check is what decides the outcome. + fn keypair(id: KeyID, purpose: Purpose) -> (IdentityPublicKey, [u8; 32]) { + let secret_bytes = [0x2A; 32]; + let secp = Secp256k1::new(); + let secret = SecretKey::from_byte_array(&secret_bytes).expect("a valid secret key"); + let data = PrivateKey::new(secret, Network::Testnet) + .public_key(&secp) + .to_bytes(); + ( + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(data), + disabled_at: None, + }), + secret_bytes, + ) + } + + /// A key on none of this identity's lists has no store to be filed under, + /// so the paste is refused. The screen must not report it as held anyway: + /// a key the app just rejected would otherwise offer to sign, to be shown, + /// and to be removed — none of which it can do, since nothing was saved. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_key_that_cannot_be_placed_is_not_reported_as_held() { + let (app_context, _dir) = offline_ctx().await; + + // The record publishes no keys at all, so this one is on no list. + let (unlisted, secret) = keypair(7, Purpose::AUTHENTICATION); + let stored = identity_with(0x6B, &[]); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("insert the record"); + + let mut screen = KeyInfoScreen::new(stored, unlisted.clone(), None, &app_context); + screen.private_key_input.set_text(hex::encode(secret)); + screen.validate_and_store_private_key(); + + assert!( + screen.private_key_data.is_none(), + "a refused key must not be left on screen as held", + ); + assert!( + screen + .identity + .private_keys + .candidates(&unlisted) + .next() + .is_none(), + "and nothing was filed for it either", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// A password-protected identity for `app_context`'s vault: `sealed` is + /// published, held as an `InVault` placeholder, and its secret is sealed + /// Tier-2 — so any keyless persist of new resident plaintext is refused + /// with [`TaskError::IdentityKeyProtectionDowngrade`]. The cheapest real + /// persist refusal there is: no I/O fault injection needed. + fn protected_identity( + app_context: &Arc, + id_byte: u8, + sealed: &IdentityPublicKey, + also_published: &[IdentityPublicKey], + ) -> QualifiedIdentity { + let identifier = Identifier::from([id_byte; 32]); + crate::wallet_backend::SecretSeam::new(&app_context.secret_store()) + .put_secret_protected( + &platform_wallet_storage::secrets::WalletId::from(identifier.to_buffer()), + &crate::wallet_backend::SecretScope::identity_key_label(&MAIN, sealed.id()), + &platform_wallet_storage::secrets::SecretBytes::from_slice(&[0x31; 32]), + &platform_wallet_storage::secrets::SecretString::new("identity-object-password"), + ) + .expect("seal the existing key Tier-2"); + + let mut private_keys = KeyStorage::default(); + private_keys.insert_at( + (MAIN, sealed.id()), + ( + QualifiedIdentityPublicKey::from(sealed.clone()), + PrivateKeyData::InVault, + ), + ); + let mut identity = identity_with(id_byte, &[]); + identity.identity = Identity::new_with_id_and_keys( + identifier, + std::iter::once(sealed) + .chain(also_published) + .map(|key| (key.id(), key.clone())) + .collect(), + PlatformVersion::latest(), + ) + .expect("identity publishing its keys"); + identity.private_keys = private_keys; + identity + } + + /// A key whose persist is refused must not be reported as held, and the + /// refusal must be explained by the refusal itself. On a password-protected + /// identity the keyless paste persist is refused every time; leaving the + /// key on screen as held offers sign/reveal/remove for material the record + /// never accepted, and blaming disk space names a remedy that cannot work + /// while the real one — remove the protection first — goes unsaid. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_key_the_persist_refuses_is_not_reported_as_held() { + let (app_context, _dir) = offline_ctx().await; + + let sealed = public_key(0, Purpose::AUTHENTICATION); + let (pasted, secret) = keypair(1, Purpose::AUTHENTICATION); + let stored = protected_identity(&app_context, 0x7C, &sealed, std::slice::from_ref(&pasted)); + app_context + .insert_local_qualified_identity(&stored, &None) + .expect("a record with no resident plaintext inserts cleanly"); + + let mut screen = KeyInfoScreen::new(stored, pasted.clone(), None, &app_context); + screen.private_key_input.set_text(hex::encode(secret)); + screen.validate_and_store_private_key(); + + assert!( + screen.private_key_data.is_none(), + "a key the persist refused must not be left on screen as held", + ); + assert!( + screen + .identity + .private_keys + .candidates(&pasted) + .next() + .is_none(), + "the in-memory record must stay what is on disk", + ); + let banner_texts = + crate::ui::components::message_banner::global_banner_texts(app_context.egui_ctx()); + assert!( + banner_texts.contains(&TaskError::IdentityKeyProtectionDowngrade.to_string()), + "the refusal must speak through its own typed message, got {banner_texts:?}", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// The mirror of the paste refusal: a removal that cannot persist must not + /// leave the screen claiming the key is gone. The locked read-modify-write + /// edits the record as stored, so a record that vanished while this screen + /// was open — the identity removed from another screen — refuses the + /// removal with its own typed error, and the screen keeps reporting what + /// it still holds rather than declaring anything removed. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_removal_that_cannot_persist_keeps_the_key_held() { + let (app_context, _dir) = offline_ctx().await; + + // Deliberately never inserted: the record is gone by the time the + // removal runs, the way another screen's identity removal leaves it. + let on_screen = public_key(1, Purpose::AUTHENTICATION); + let identity = identity_with(0x7D, &[(on_screen.clone(), [0x22; 32])]); + + let mut screen = KeyInfoScreen::new( + identity, + on_screen.clone(), + Some((PrivateKeyData::Clear([0x22; 32]), None)), + &app_context, + ); + let error = screen + .remove_held_private_key() + .expect_err("with no stored record there is nothing to remove from"); + assert!( + matches!(error, TaskError::IdentityNotFoundLocally), + "expected the missing-record refusal, got {error:?}", + ); + assert!( + screen + .identity + .private_keys + .candidates(&on_screen) + .next() + .is_some(), + "a removal that did not persist must leave the in-memory record alone", + ); + assert!( + screen.private_key_data.is_some(), + "and the screen must keep reporting the key as held", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// Pressing Show or Sign on a key whose placement cannot be resolved must + /// surface the typed error's own message. The two failure modes carry + /// different remedies, and for a key on two lists at once "enter its + /// private key on this page" is the one instruction guaranteed to be + /// refused — by the same ambiguity, one screen interaction later. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_show_request_that_cannot_resolve_speaks_through_the_typed_error() { + let (app_context, _dir) = offline_ctx().await; + + // The key published on both the main and the voter list — ambiguous — + // and held nowhere. + let key = public_key(0, Purpose::VOTING); + let published = |id_byte: u8| { + Identity::new_with_id_and_keys( + Identifier::from([id_byte; 32]), + BTreeMap::from([(key.id(), key.clone())]), + PlatformVersion::latest(), + ) + .expect("identity publishing the key") + }; + let mut identity = identity_with(0x7E, &[]); + identity.identity = published(0x7E); + identity.associated_voter_identity = Some((published(0x7F), key.clone())); + identity.identity_type = IdentityType::Masternode; + + let mut screen = KeyInfoScreen::new(identity, key, None, &app_context); + screen.pending_identity_key_display = true; + + let mut harness = egui_kittest::Harness::builder() + .with_size(egui::vec2(1100.0, 900.0)) + .build_ui(move |ui| { + screen.ui(ui); + }); + harness.run_steps(2); + + let banner_texts = crate::ui::components::message_banner::global_banner_texts(&harness.ctx); + assert!( + banner_texts.contains(&TaskError::IdentityKeyPlacementAmbiguous.to_string()), + "the ambiguity's own message and remedy must reach the user, got {banner_texts:?}", + ); + + app_context + .wallet_backend() + .expect("backend") + .shutdown() + .await; + } + + /// Both placement errors surface on the Show/Sign read path as well as the + /// paste path — `filed_at` is one function serving both — so neither + /// remedy may presume the user was saving a key: a user who pressed Show + /// entered nothing and asked to save nothing. + #[test] + fn the_placement_errors_advise_an_action_both_paths_can_perform() { + for error in [ + TaskError::IdentityKeyPlacementAmbiguous, + TaskError::IdentityKeyNotOnIdentityRecord, + ] { + let message = error.to_string(); + for save_word in ["sav", "enter"] { + assert!( + !message.to_lowercase().contains(save_word), + "a remedy shown for a Show or Sign press must not presume a save: {message}", + ); + } + assert!( + message.contains("Refresh this identity"), + "the remedy both paths share is refreshing the identity: {message}", + ); + } + } + /// A restore dispatched from one identity's Key Info screen can complete /// after the user has opened another's, and results reach whichever screen /// is visible. The stray completion must touch nothing here: not the clone, diff --git a/src/ui/identities/keys/keys_screen.rs b/src/ui/identities/keys/keys_screen.rs index cf99978d1..aa8799373 100644 --- a/src/ui/identities/keys/keys_screen.rs +++ b/src/ui/identities/keys/keys_screen.rs @@ -11,7 +11,9 @@ use crate::backend_task::error::TaskError; use crate::backend_task::{BackendTaskContext, BackendTaskSuccessResult}; use crate::context::AppContext; use crate::model::legacy_recovery::RecoveryItem; -use crate::model::qualified_identity::QualifiedIdentity; +use crate::model::qualified_identity::encrypted_key_storage::same_key; +use crate::model::qualified_identity::key_placement::KeyPlacement; +use crate::model::qualified_identity::{PrivateKeyTarget, QualifiedIdentity}; use crate::model::user_role::UserRole; use crate::ui::components::MessageBanner; use crate::ui::components::left_panel::add_left_panel; @@ -38,6 +40,11 @@ const HELD: &str = "This key is saved on this device."; /// user with stranded keys came here to find, so it is stated in words rather /// than signalled by colour alone. const NOT_HELD: &str = "This key is not saved on this device."; +/// Heads the section of held keys no on-chain list publishes. Their rows are +/// what makes the occupied-slot refusal's remedy performable — such a key is +/// reachable from nowhere else. +const UNPUBLISHED_EXPLAINER: &str = + "These keys are saved on this device but are not on this identity's key lists."; pub struct KeysScreen { pub identity: QualifiedIdentity, @@ -154,14 +161,39 @@ impl KeysScreen { } } + /// The held keys no on-chain list publishes: a locally saved entry whose + /// broadcast never happened, or one restored from an older version's + /// data. One entry per key — a key filed under two placements is still + /// one key. + fn unpublished_held_keys(&self) -> Vec<(PrivateKeyTarget, IdentityPublicKey)> { + let mut keys: Vec<(PrivateKeyTarget, IdentityPublicKey)> = Vec::new(); + for ((target, _), (stored, _)) in self.identity.private_keys.iter() { + let key = &stored.identity_public_key; + if !matches!(self.identity.placement_of(key), KeyPlacement::Unknown) { + continue; + } + if keys.iter().any(|(_, listed)| same_key(listed, key)) { + continue; + } + keys.push((target.clone(), key.clone())); + } + keys + } + /// One row per key: what it is for, whether this device holds it, and the /// way into its own page. Ungated — a key the device does not hold is /// exactly the key a user comes here to do something about. + /// + /// Below the published keys, a section lists held keys that are on none + /// of the identity's lists. Those appear nowhere else, and one of them is + /// exactly what the occupied-slot refusal asks the user to open and + /// remove — the section only exists when there is such a key to show. fn render_key_list(&self, ui: &mut egui::Ui, dark_mode: bool) -> AppAction { let mut action = AppAction::None; let keys = identity_keys(&self.identity); + let unpublished = self.unpublished_held_keys(); - if keys.is_empty() { + if keys.is_empty() && unpublished.is_empty() { ui.add_space(8.0); // Not "no keys saved on this device": these rows come from the // identity's on-chain public keys, and that phrase means held=false @@ -180,47 +212,83 @@ impl KeysScreen { let expert = self.app_context.user_role().at_least(UserRole::Power); let vocabulary = KeyVocabulary::from(self.identity.identity_type); - let labels = manage_keys_labels(vocabulary, &keys); - for ((_, key), (label, tip)) in keys.into_iter().zip(labels) { + // One labelling pass over both sections, so a role they share is + // still disambiguated by key id. + let mut labels = manage_keys_labels( + vocabulary, + &keys + .iter() + .cloned() + .chain(unpublished.iter().cloned()) + .collect::>(), + ); + let unpublished_labels = labels.split_off(keys.len()); + for ((_, key), labelled) in keys.into_iter().zip(labels) { // Where this key's private half actually is, whichever store filed // it. A presence check rather than a fetch: cloning the entry copies // raw key bytes out of the vault unscrubbed, and this runs every // frame for every key. - let filed_at = self.identity.private_keys.candidates(&key).next(); - let held = if filed_at.is_some() { HELD } else { NOT_HELD }; + let held = self.identity.private_keys.candidates(&key).next().is_some(); + action |= self.key_row(ui, dark_mode, expert, &key, labelled, held); + } + + if !unpublished.is_empty() { + ui.add_space(10.0); + ui.separator(); ui.add_space(4.0); - ui.horizontal(|ui| { - let button = - ComponentStyles::add_secondary_button(ui, format!("{label} ›"), dark_mode); - let button = match tip { - Some(tip) => button.clickable_tooltip(tip), - None => button, - }; - if button.clicked() { - let holding = filed_at.as_ref().and_then(|placement| { - self.identity - .private_keys - .get_cloned_private_key_data_and_wallet_info(placement) - }); - action |= AppAction::AddScreen(Screen::KeyInfoScreen( - KeyInfoScreen::new( - self.identity.clone(), - key.clone(), - holding, - &self.app_context, - ) - .with_parent(PARENT_CRUMB), - )); - } - ui.label(RichText::new(held).color(DashColors::text_secondary(dark_mode))); - }); - if expert { - Self::render_expert_detail(ui, &key, dark_mode); + ui.label( + RichText::new(UNPUBLISHED_EXPLAINER).color(DashColors::text_primary(dark_mode)), + ); + for ((_, key), labelled) in unpublished.into_iter().zip(unpublished_labels) { + // Held by construction — being held is what put the key here. + action |= self.key_row(ui, dark_mode, expert, &key, labelled, true); } } action } + /// One key's row: the way into its page, and its held state in words. + /// `labelled` is the key's `manage_keys_labels` entry — its caption and + /// optional tooltip. + fn key_row( + &self, + ui: &mut egui::Ui, + dark_mode: bool, + expert: bool, + key: &IdentityPublicKey, + labelled: (String, Option<&'static str>), + held: bool, + ) -> AppAction { + let (label, tip) = labelled; + let mut action = AppAction::None; + let held_text = if held { HELD } else { NOT_HELD }; + ui.add_space(4.0); + ui.horizontal(|ui| { + let button = ComponentStyles::add_secondary_button(ui, format!("{label} ›"), dark_mode); + let button = match tip { + Some(tip) => button.clickable_tooltip(tip), + None => button, + }; + if button.clicked() { + let holding = self.identity.private_keys.held_private_key_data(key); + action |= AppAction::AddScreen(Screen::KeyInfoScreen( + KeyInfoScreen::new( + self.identity.clone(), + key.clone(), + holding, + &self.app_context, + ) + .with_parent(PARENT_CRUMB), + )); + } + ui.label(RichText::new(held_text).color(DashColors::text_secondary(dark_mode))); + }); + if expert { + Self::render_expert_detail(ui, key, dark_mode); + } + action + } + /// The on-chain specifics of one key, for the Expert view. Everyday view /// gets the role word and held state, which is what it can act on. fn render_expert_detail(ui: &mut egui::Ui, key: &IdentityPublicKey, dark_mode: bool) { diff --git a/src/ui/identities/mod.rs b/src/ui/identities/mod.rs index d49ac9adf..5e8f89da5 100644 --- a/src/ui/identities/mod.rs +++ b/src/ui/identities/mod.rs @@ -6,10 +6,7 @@ use dash_sdk::{ use crate::{ context::AppContext, - model::{ - qualified_identity::{QualifiedIdentity, encrypted_key_storage::PrivateKeyData}, - wallet::Wallet, - }, + model::{qualified_identity::QualifiedIdentity, wallet::Wallet}, }; pub mod add_existing_identity_screen; @@ -80,21 +77,113 @@ pub fn get_selected_wallet( selected_key.ok_or_else(|| "No key provided when getting selected wallet".to_string())? }; - // Once we have the public key (either from DPNS or directly), look up - // the matching private key data wherever it is filed. - let filed_at = qualified_identity + // Once we have the public key (either from DPNS or directly), ask which + // wallet derives it — under any placement, since a key filed under two + // stores need not be wallet-derived under the first one probed. + match qualified_identity .private_keys - .candidates(public_key) - .next(); - if let Some((_, PrivateKeyData::AtWalletDerivationPath(wallet_derivation_path))) = - filed_at.and_then(|placement| qualified_identity.private_keys.entry_at(&placement)) + .wallet_derived_at(public_key) { - // If found, return the associated wallet (cloned to preserve Arc). - Ok(qualified_identity + Some(wallet_derivation_path) => Ok(qualified_identity .associated_wallets .get(&wallet_derivation_path.wallet_seed_hash) - .cloned()) - } else { - Ok(None) + .cloned()), + None => Ok(None), + } +} + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use dash_sdk::dpp::dashcore::Network; + use dash_sdk::dpp::identity::Identity; + use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; + use dash_sdk::dpp::key_wallet::bip32::DerivationPath; + use dash_sdk::dpp::version::PlatformVersion; + use dash_sdk::platform::Identifier; + + use super::*; + use crate::model::qualified_identity::encrypted_key_storage::{ + KeyStorage, PrivateKeyData, WalletDerivationPath, + }; + use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; + use crate::model::qualified_identity::{ + IdentityStatus, IdentityType, PrivateKeyTarget, QualifiedIdentity, + }; + + /// An identity publishing `key`, holding `private_keys`, linked to + /// `wallets` — the three things a test here varies; every other field is + /// an inert default. + fn identity_with( + key: &IdentityPublicKey, + private_keys: KeyStorage, + wallets: BTreeMap>>, + ) -> QualifiedIdentity { + QualifiedIdentity { + identity: Identity::new_with_id_and_keys( + Identifier::from([1u8; 32]), + BTreeMap::from([(key.id(), key.clone())]), + PlatformVersion::latest(), + ) + .expect("identity"), + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::Masternode, + alias: None, + private_keys, + dpns_names: vec![], + associated_wallets: wallets, + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: Network::Testnet, + } + } + + /// A key filed under two placements, wallet-derived only under the second. + /// Taking whichever placement is probed first answers "no wallet" — the + /// same answer as an identity with no wallet at all — and the screen then + /// offers no unlock for a wallet it needs. + #[test] + fn a_wallet_is_found_under_a_later_placement_too() { + let seed_hash = [0x66; 32]; + let wallet = Wallet::new_from_seed([0x11; 64], Network::Testnet, None, None) + .expect("build a test wallet"); + let key = IdentityPublicKey::random_key(0, Some(1), PlatformVersion::latest()); + + let mut private_keys = KeyStorage::default(); + private_keys.insert_at( + (PrivateKeyTarget::PrivateKeyOnMainIdentity, key.id()), + ( + QualifiedIdentityPublicKey::from(key.clone()), + PrivateKeyData::Clear([0x22; 32]), + ), + ); + private_keys.insert_at( + (PrivateKeyTarget::PrivateKeyOnVoterIdentity, key.id()), + ( + QualifiedIdentityPublicKey::from(key.clone()), + PrivateKeyData::AtWalletDerivationPath(WalletDerivationPath { + wallet_seed_hash: seed_hash, + derivation_path: DerivationPath::from(vec![]), + }), + ), + ); + + let qualified_identity = identity_with( + &key, + private_keys, + BTreeMap::from([(seed_hash, Arc::new(RwLock::new(wallet)))]), + ); + + let selected = get_selected_wallet(&qualified_identity, None, Some(&key)) + .expect("a key given directly needs no DPNS contract"); + assert!( + selected.is_some(), + "the wallet deriving this key must be found whichever placement names it" + ); } } diff --git a/src/ui/identities/withdraw_screen.rs b/src/ui/identities/withdraw_screen.rs index 56afb6353..ea3386cd2 100644 --- a/src/ui/identities/withdraw_screen.rs +++ b/src/ui/identities/withdraw_screen.rs @@ -4,7 +4,6 @@ use crate::backend_task::{BackendTask, BackendTaskSuccessResult, FeeResult}; use crate::context::AppContext; use crate::model::amount::Amount; use crate::model::fee_estimation::{format_credits_as_dash, max_spendable_credits}; -use crate::model::qualified_identity::encrypted_key_storage::PrivateKeyData; use crate::model::qualified_identity::{IdentityType, QualifiedIdentity}; use crate::model::user_role::UserRole; use crate::model::wallet::Wallet; @@ -493,12 +492,8 @@ impl ScreenLike for WithdrawalScreen { // Render wallet unlock component if needed if let Some(selected_key) = self.selected_key.as_ref() { // If there is an associated wallet then render the wallet unlock component for it if its locked - let filed_at = self.identity.private_keys.candidates(selected_key).next(); - if let Some(( - _, - PrivateKeyData::AtWalletDerivationPath(wallet_derivation_path), - )) = filed_at - .and_then(|placement| self.identity.private_keys.entry_at(&placement)) + if let Some(wallet_derivation_path) = + self.identity.private_keys.wallet_derived_at(selected_key) { let new_wallet = self .identity diff --git a/src/ui/masternodes/detail_screen.rs b/src/ui/masternodes/detail_screen.rs index 16022110d..88bb2b768 100644 --- a/src/ui/masternodes/detail_screen.rs +++ b/src/ui/masternodes/detail_screen.rs @@ -27,9 +27,7 @@ use crate::context::AppContext; use crate::model::contested_name::{ContestedName, MasternodeContestSummary}; use crate::model::fee_estimation::format_credits_as_dash; use crate::model::legacy_recovery::RecoveryItem; -use crate::model::qualified_identity::{ - IdentityType, MasternodeKeyPresence, PrivateKeyTarget, QualifiedIdentity, -}; +use crate::model::qualified_identity::{IdentityType, MasternodeKeyPresence, QualifiedIdentity}; use crate::model::secret::Secret; use crate::ui::components::MessageBanner; use crate::ui::components::component_trait::Component; @@ -664,7 +662,7 @@ impl MasternodeDetailView { let keys = identity_keys(&self.identity); // This page only ever shows masternode and evonode identities. let labels = manage_keys_labels(KeyVocabulary::from(self.identity.identity_type), &keys); - for ((_target, key), (label, tip)) in keys.into_iter().zip(labels) { + for ((_, key), (label, tip)) in keys.into_iter().zip(labels) { let button = ui.button(format!("{label} ›")); let button = match tip { Some(tip) => button.clickable_tooltip(tip), @@ -680,7 +678,7 @@ impl MasternodeDetailView { // lives inside `KeyInfoScreen`. Open the first held key so the user // lands directly on the interactive seal flow. if tier.offers_add_protection() - && let Some((_target, key)) = self.first_protectable_key() + && let Some(key) = self.first_protectable_key() && ui.button("Add password protection…").clicked() { action = Some(self.open_key_info_with_protection_prompt(&key)); @@ -722,12 +720,14 @@ impl MasternodeDetailView { /// checks presence, so no raw key bytes are cloned out of the vault here — /// unlike `open_key_info_with_mode`, which needs the actual secret and thus /// pays for the clone. - fn first_protectable_key( - &self, - ) -> Option<(PrivateKeyTarget, dash_sdk::platform::IdentityPublicKey)> { + fn first_protectable_key(&self) -> Option { identity_keys(&self.identity) .into_iter() + // Presence only: this gates a button, it never acts on the + // placement, so which of several placements is the liveliest one + // makes no difference to the answer. .find(|(_, key)| self.identity.private_keys.candidates(key).next().is_some()) + .map(|(_, key)| key) } /// Build the `AddScreen` action that opens `KeyInfoScreen` for one key, @@ -755,16 +755,7 @@ impl MasternodeDetailView { // the retired purpose-derived convention — a main-identity voting key // entered by hand — and report a key as unheld here while the identity // keys list shows it as saved on this device. - let holding = self - .identity - .private_keys - .candidates(key) - .next() - .and_then(|placement| { - self.identity - .private_keys - .get_cloned_private_key_data_and_wallet_info(&placement) - }); + let holding = self.identity.private_keys.held_private_key_data(key); let identity = self.identity.clone(); let key = key.clone(); let screen = match mode { @@ -1151,6 +1142,7 @@ mod tests { use crate::context::connection_status::ConnectionStatus; use crate::database::test_helpers::create_database_at_path; use crate::model::qualified_identity::IdentityStatus; + use crate::model::qualified_identity::PrivateKeyTarget; use crate::model::qualified_identity::encrypted_key_storage::{KeyStorage, PrivateKeyData}; use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; use crate::utils::egui_mpsc::SenderAsync; diff --git a/tests/backend-e2e/identity_in_vault_sign.rs b/tests/backend-e2e/identity_in_vault_sign.rs index 9914dc955..3f85cf0dc 100644 --- a/tests/backend-e2e/identity_in_vault_sign.rs +++ b/tests/backend-e2e/identity_in_vault_sign.rs @@ -149,7 +149,8 @@ async fn ts_sign_e2e_01_in_vault_identity_signs_and_broadcasts() { dash_evo_tool::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey::from(new_ipk.clone()), new_private_key_bytes, ), - ); + ) + .expect("the freshly minted key's slot is free"); let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( &identity, @@ -269,5 +270,6 @@ async fn materialize_master_key_as_clear( .expect("resolve HD seed and derive MASTER private key"); qi.private_keys - .insert_non_encrypted(map_key, (master_pub, master_bytes)); + .insert_non_encrypted(map_key, (master_pub, master_bytes)) + .expect("the MASTER key's own slot accepts it"); } diff --git a/tests/backend-e2e/z_broadcast_st_tasks.rs b/tests/backend-e2e/z_broadcast_st_tasks.rs index 7285daf6c..e3076e1c3 100644 --- a/tests/backend-e2e/z_broadcast_st_tasks.rs +++ b/tests/backend-e2e/z_broadcast_st_tasks.rs @@ -98,13 +98,15 @@ async fn step_broadcast_valid( // `try_from_identity_with_signer`. let mut qi = si.qualified_identity.clone(); qi.identity = identity.clone(); - qi.private_keys.insert_non_encrypted( - (PrivateKeyOnMainIdentity, new_ipk.id()), - ( - QualifiedIdentityPublicKey::from(new_ipk.clone()), - new_private_key_bytes, - ), - ); + qi.private_keys + .insert_non_encrypted( + (PrivateKeyOnMainIdentity, new_ipk.id()), + ( + QualifiedIdentityPublicKey::from(new_ipk.clone()), + new_private_key_bytes, + ), + ) + .expect("the freshly minted key's slot is free"); let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( &identity, @@ -246,13 +248,16 @@ async fn step_broadcast_invalid( // Register the new key's private key in the signer so // try_from_identity_with_signer can sign it. let mut signer_qi = refreshed_qi.clone(); - signer_qi.private_keys.insert_non_encrypted( - (PrivateKeyOnMainIdentity, new_ipk.id()), - ( - QualifiedIdentityPublicKey::from(new_ipk.clone()), - new_private_key_bytes, - ), - ); + signer_qi + .private_keys + .insert_non_encrypted( + (PrivateKeyOnMainIdentity, new_ipk.id()), + ( + QualifiedIdentityPublicKey::from(new_ipk.clone()), + new_private_key_bytes, + ), + ) + .expect("the freshly minted key's slot is free"); let invalid_state_transition = IdentityUpdateTransition::try_from_identity_with_signer( identity, diff --git a/tests/kittest/identities_screen.rs b/tests/kittest/identities_screen.rs index ca37cdc1a..5778bc4e2 100644 --- a/tests/kittest/identities_screen.rs +++ b/tests/kittest/identities_screen.rs @@ -1,5 +1,21 @@ -use crate::support::with_isolated_data_dir; +use crate::support::{fresh_app_context, with_isolated_data_dir}; +use dash_evo_tool::app::AppAction; +use dash_evo_tool::model::qualified_identity::encrypted_key_storage::KeyStorage; +use dash_evo_tool::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; +use dash_evo_tool::model::qualified_identity::{ + IdentityStatus, IdentityType, PrivateKeyTarget, QualifiedIdentity, +}; +use dash_evo_tool::ui::identities::identities_screen::IdentitiesScreen; +use dash_evo_tool::ui::{Screen, ScreenLike}; +use dash_sdk::dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0 as _; +use dash_sdk::dpp::identity::{Identity, KeyID, Purpose}; +use dash_sdk::dpp::version::PlatformVersion; +use dash_sdk::platform::{Identifier, IdentityPublicKey}; use egui_kittest::Harness; +use egui_kittest::kittest::Queryable; +use std::cell::RefCell; +use std::collections::BTreeMap; +use std::rc::Rc; /// Test that the identities screen can be rendered #[test] @@ -65,6 +81,154 @@ fn test_window_resize() { }); } +/// A voting-purpose key with deterministic material, since the popup's row +/// label is derived from id, purpose and security level. +fn voting_key(id: KeyID) -> IdentityPublicKey { + use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dash_sdk::dpp::identity::{KeyType, SecurityLevel}; + use dash_sdk::dpp::platform_value::BinaryData; + IdentityPublicKeyV0 { + id, + key_type: KeyType::ECDSA_HASH160, + purpose: Purpose::VOTING, + security_level: SecurityLevel::CRITICAL, + read_only: false, + data: BinaryData::new(vec![id as u8; 20]), + disabled_at: None, + contract_bounds: None, + } + .into() +} + +/// A masternode whose keys are filed where an older build put them — each +/// under the *other* identity's store. The main identity publishes `main_key`, +/// held under the voter store; the voter identity publishes `voter_key`, held +/// under the main store. Both are held, whichever store an old install chose. +fn masternode_with_legacy_filed_keys( + main_key: &IdentityPublicKey, + voter_key: &IdentityPublicKey, +) -> QualifiedIdentity { + let pv = PlatformVersion::latest(); + let build = |id_byte: u8, key: &IdentityPublicKey| { + Identity::new_with_id_and_keys( + Identifier::from([id_byte; 32]), + BTreeMap::from([(key.id(), key.clone())]), + pv, + ) + .expect("identity publishing one key") + }; + QualifiedIdentity { + identity: build(0x51, main_key), + associated_voter_identity: Some((build(0x52, voter_key), voter_key.clone())), + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::Masternode, + alias: Some("legacy-filed".to_string()), + private_keys: KeyStorage::from(BTreeMap::from([ + ( + (PrivateKeyTarget::PrivateKeyOnVoterIdentity, main_key.id()), + ( + QualifiedIdentityPublicKey::from(main_key.clone()), + [0x11u8; 32], + ), + ), + ( + (PrivateKeyTarget::PrivateKeyOnMainIdentity, voter_key.id()), + ( + QualifiedIdentityPublicKey::from(voter_key.clone()), + [0x22u8; 32], + ), + ), + ])), + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + status: IdentityStatus::Active, + network: dash_sdk::dpp::dashcore::Network::Testnet, + } +} + +/// Drive `screen` in a bare harness, capturing the last non-`None` action. +fn harness_for(mut screen: IdentitiesScreen) -> (Harness<'static>, Rc>) { + let action = Rc::new(RefCell::new(AppAction::None)); + let capture = action.clone(); + let mut harness = Harness::builder() + .with_size(egui::vec2(1400.0, 800.0)) + .build_ui(move |ui| { + let act = screen.ui(ui); + if act != AppAction::None { + *capture.borrow_mut() = act; + } + }); + harness.run_steps(3); + (harness, action) +} + +/// Open the Keys popup, click the row labelled `key_label`, and return the +/// Key Info screen that click opened. +fn open_key_from_popup( + harness: &mut Harness<'_>, + action: &Rc>, + key_label: &str, +) -> dash_evo_tool::ui::identities::keys::key_info_screen::KeyInfoScreen { + harness.get_by_label("Keys").click(); + harness.run_steps(2); + harness.get_by_label(key_label).click(); + harness.run_steps(2); + let opened = std::mem::replace(&mut *action.borrow_mut(), AppAction::None); + let AppAction::AddScreen(Screen::KeyInfoScreen(key_info)) = opened else { + panic!("clicking the key row must open Key Info, got a different action"); + }; + key_info +} + +/// The identities list's Keys popup must see a key as held wherever its +/// private half is filed. A main-identity voting key an older build filed +/// under the voter store is a documented on-disk shape; probing only the +/// store matching the list being walked misses it, so the popup calls a held +/// key unsaved and opens its Key Info page in the wrong state. +#[test] +fn the_keys_popup_finds_a_main_key_an_older_build_filed_under_voter() { + with_isolated_data_dir(|| { + let (_rt, app_context) = fresh_app_context(); + let main_key = voting_key(3); + let identity = masternode_with_legacy_filed_keys(&main_key, &voting_key(0)); + app_context + .insert_local_qualified_identity(&identity, &None) + .expect("store the identity for the list to load"); + + let (mut harness, action) = harness_for(IdentitiesScreen::new(&app_context)); + let key_info = open_key_from_popup(&mut harness, &action, "3 - V - Critical"); + assert!( + key_info.private_key_data.is_some(), + "a held main-identity key filed under the voter store must open as held", + ); + }); +} + +/// The voter-list mirror of the same defect: a voter identity's key misfiled +/// under the main store must still be seen as held by the voter rows. +#[test] +fn the_keys_popup_finds_a_voter_key_an_older_build_filed_under_main() { + with_isolated_data_dir(|| { + let (_rt, app_context) = fresh_app_context(); + let voter_key = voting_key(0); + let identity = masternode_with_legacy_filed_keys(&voting_key(3), &voter_key); + app_context + .insert_local_qualified_identity(&identity, &None) + .expect("store the identity for the list to load"); + + let (mut harness, action) = harness_for(IdentitiesScreen::new(&app_context)); + let key_info = open_key_from_popup(&mut harness, &action, "0 - V - Critical"); + assert!( + key_info.private_key_data.is_some(), + "a held voter-identity key filed under the main store must open as held", + ); + }); +} + /// Test multiple frame batches #[test] fn test_frame_batch_processing() { diff --git a/tests/kittest/key_info_screen.rs b/tests/kittest/key_info_screen.rs index 24232b00d..b3fbd11c8 100644 --- a/tests/kittest/key_info_screen.rs +++ b/tests/kittest/key_info_screen.rs @@ -12,7 +12,7 @@ use crate::support::{mount_app, with_isolated_data_dir}; use dash_evo_tool::app::TaskResult; -use dash_evo_tool::model::qualified_identity::encrypted_key_storage::{KeyStorage, PrivateKeyData}; +use dash_evo_tool::model::qualified_identity::encrypted_key_storage::KeyStorage; use dash_evo_tool::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; use dash_evo_tool::model::qualified_identity::{ IdentityStatus, IdentityType, PrivateKeyTarget, QualifiedIdentity, @@ -142,13 +142,16 @@ fn a_write_that_lands_while_key_info_is_open_survives_the_next_key_edit() { .expect("read the record") .expect("record stored"); record.identity.add_public_key(restored.clone()); - record.private_keys.insert_at( - (MAIN, restored.id()), - ( - QualifiedIdentityPublicKey::from(restored.clone()), - PrivateKeyData::Clear([0x22; 32]), - ), - ); + record + .private_keys + .insert_non_encrypted( + (MAIN, restored.id()), + ( + QualifiedIdentityPublicKey::from(restored.clone()), + [0x22; 32], + ), + ) + .expect("the restored slot is free"); app_context .update_local_qualified_identity(&record) .expect("the other writer's write"); @@ -160,7 +163,8 @@ fn a_write_that_lands_while_key_info_is_open_survives_the_next_key_edit() { .expect("queue the refresh the app dispatches"); harness.run_steps(3); - // What every key add and remove on this screen does with its clone. + // The clone is what the screen displays; writing it out whole proves + // the refresh actually made it current. let Some(Screen::KeyInfoScreen(screen)) = harness.state().screen_stack.last() else { panic!("Key Info must still be the open screen"); }; @@ -174,7 +178,9 @@ fn a_write_that_lands_while_key_info_is_open_survives_the_next_key_edit() { .expect("read back") .expect("still stored") .private_keys - .has(&(MAIN, restored.id())), + .candidates(&restored) + .next() + .is_some(), "a key edit on this screen must not erase a key written while it was open", ); }); diff --git a/tests/kittest/keys_screen.rs b/tests/kittest/keys_screen.rs index 53c39ceb1..f695538f9 100644 --- a/tests/kittest/keys_screen.rs +++ b/tests/kittest/keys_screen.rs @@ -568,6 +568,68 @@ fn key_info_agrees_with_the_list_about_a_voting_key_held_on_the_main_identity() }); } +/// A key saved on this device but published on no list — the state a local +/// save whose broadcast never happened leaves behind, and the occupant the +/// occupied-slot refusal points at — must still get a row: it appears in no +/// on-chain key list, so without one, nothing can reach it and the refusal's +/// remedy ("open that key in this identity's key list, remove its saved +/// private key") names an entry that does not exist. +#[test] +fn a_held_key_published_on_no_list_still_gets_a_row() { + const UNPUBLISHED_EXPLAINER: &str = + "These keys are saved on this device but are not on this identity's key lists."; + + with_isolated_data_dir(|| { + let (_rt, app_context) = fresh_app_context(); + + // The identity publishes one authentication key; the device also + // holds a transfer key no list publishes. + let mut identity = stranded_identity(0x44, &[Purpose::AUTHENTICATION], "unpublished"); + let orphan = key(5, Purpose::TRANSFER); + identity.private_keys = KeyStorage::from(BTreeMap::from([( + (PrivateKeyTarget::PrivateKeyOnMainIdentity, orphan.id()), + ( + QualifiedIdentityPublicKey::from(orphan.clone()), + PrivateKeyData::Clear([0x44; 32]), + ), + )])); + + let (mut harness, action) = harness_for(KeysScreen::new(identity, &app_context)); + assert!( + harness.query_by_label(UNPUBLISHED_EXPLAINER).is_some(), + "the section must say what these keys are" + ); + assert!( + harness.query_by_label(TRANSFER_ROW).is_some(), + "the unpublished key must get a row of its own" + ); + + harness.get_by_label(TRANSFER_ROW).click(); + harness.run_steps(2); + let opened = std::mem::replace(&mut *action.borrow_mut(), AppAction::None); + let AppAction::AddScreen(Screen::KeyInfoScreen(key_info)) = opened else { + panic!("the unpublished key's row must open Key Info"); + }; + assert!( + key_info.private_key_data.is_some(), + "Key Info must receive the held material, so its saved private key \ + can be removed from there" + ); + + // An identity whose held keys are all published shows no such section. + let plain = identity_holding_key( + 0x45, + Purpose::AUTHENTICATION, + PrivateKeyTarget::PrivateKeyOnMainIdentity, + ); + let (plain, _) = harness_for(KeysScreen::new(plain, &app_context)); + assert!( + plain.query_by_label(UNPUBLISHED_EXPLAINER).is_none(), + "the section only appears when there is something to show" + ); + }); +} + /// AC-3: the offer is identity-scoped, so it must be visible without opening /// any individual key, and it must sit above the key list — a user who arrived /// because their keys are missing must not have to read past the keys they do