Add cove-cspp crate with MasterKey and HKDF key derivation#596
Merged
praveenperera merged 9 commits intomasterfrom Feb 19, 2026
Merged
Add cove-cspp crate with MasterKey and HKDF key derivation#596praveenperera merged 9 commits intomasterfrom
praveenperera merged 9 commits intomasterfrom
Conversation
Generate, store, and load a random 32-byte master key from the platform keychain. Uses the same Cryptor encrypt-then-store pattern as tap signer backups. MasterKey takes &Keychain for testability. Closes: #558
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Cspp
participant Keychain
participant Cryptor
participant MasterKey
Client->>Cspp: get_or_create_master_key()
Cspp->>Keychain: get("cspp:master_key")
alt key exists
Keychain-->>Cspp: Some(encrypted_blob)
Cspp->>Keychain: get("cspp:master_key:meta")
Keychain-->>Cspp: Some(metadata)
Cspp->>Cryptor: decrypt(ciphertext, metadata)
Cryptor-->>Cspp: master_key_hex
Cspp->>MasterKey: from_bytes(decoded)
MasterKey-->>Cspp: MasterKey
Cspp-->>Client: MasterKey
else not found
Keychain-->>Cspp: None
Cspp->>MasterKey: generate()
MasterKey-->>Cspp: MasterKey
Cspp->>Cryptor: encrypt(master_key_hex)
Cryptor-->>Cspp: (ciphertext, metadata)
Cspp->>Keychain: save("cspp:master_key", ciphertext)
Cspp->>Keychain: save("cspp:master_key:meta", metadata)
Keychain-->>Cspp: Ok
Cspp-->>Client: MasterKey
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
rust/crates/cove-cspp/src/store.rs (1)
1-6: Consider returningResultfromgetanddeletemethods to distinguish store errors from missing keys.For security-critical key material, returning
Option<String>andboolconflates "key not found" with "store I/O failure," potentially masking underlying system errors. Changing the trait tofn get(&self, key: String) -> Result<Option<String>, Self::Error>andfn delete(&self, key: String) -> Result<bool, Self::Error>would improve robustness, though this requires updating the Keychain implementation to expose underlying platform keychain errors and refactoring callers throughout cspp.rs and keychain.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/store.rs` around lines 1 - 6, Update the CsppStore trait to surface I/O errors instead of conflating them with missing keys by changing the signatures of get and delete: modify trait CsppStore to use fn get(&self, key: String) -> Result<Option<String>, Self::Error> and fn delete(&self, key: String) -> Result<bool, Self::Error> (keep save as Result<(), Self::Error>), then update the Keychain implementation to return platform keychain I/O errors as the defined Error type for CsppStore and refactor all callers in cspp.rs and keychain.rs to handle Result<Option<String>, E> from get and Result<bool, E> from delete (propagate or map errors as appropriate).rust/crates/cove-cspp/src/master_key.rs (2)
35-67: Tests look solid, covering the essential cases.Consider adding one more test asserting that
sensitive_data_key()andcritical_data_key()produce different outputs for the sameMasterKeyinstance — this is already tested at thekey_derivationlevel but would provide defence-in-depth at this API layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 35 - 67, Add a new unit test in the tests module that generates a MasterKey and asserts that calling sensitive_data_key() and critical_data_key() on the same MasterKey returns different values; locate the tests module and add the test alongside generate_produces_32_bytes/from_bytes_roundtrip/sensitive_data_key_derivation/critical_data_key_derivation so it references MasterKey::generate(), key.sensitive_data_key(), and key.critical_data_key() to validate they are not equal.
1-33: Good security posture withZeroize/ZeroizeOnDropon the master key.The struct correctly prevents accidental
Debuglogging (noDebugderive) and ensures the inner bytes are zeroed on drop. The use ofrand::rng().fill()is the correct CSPRNG API forrand0.9.One consideration:
sensitive_data_key()andcritical_data_key()return plain[u8; 32]values that are not zeroized on drop. If callers hold these derived keys for any duration, they remain in memory. Consider wrapping them in aZeroizing<[u8; 32]>wrapper from thezeroizecrate to extend the same guarantees to derived key material.♻️ Suggested improvement for derived key return types
+use zeroize::Zeroizing; + /// Derive the sensitive data encryption key (used for redb encryption) - pub fn sensitive_data_key(&self) -> [u8; 32] { - crate::key_derivation::derive_sensitive_data_key(&self.0) + pub fn sensitive_data_key(&self) -> Zeroizing<[u8; 32]> { + Zeroizing::new(crate::key_derivation::derive_sensitive_data_key(&self.0)) } /// Derive the critical data encryption key - pub fn critical_data_key(&self) -> [u8; 32] { - crate::key_derivation::derive_critical_data_key(&self.0) + pub fn critical_data_key(&self) -> Zeroizing<[u8; 32]> { + Zeroizing::new(crate::key_derivation::derive_critical_data_key(&self.0)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 1 - 33, The derived key methods sensitive_data_key and critical_data_key currently return plain [u8; 32] which are not zeroized; change their signatures on MasterKey to return zeroize::Zeroizing<[u8; 32]> and wrap the values returned from crate::key_derivation::derive_sensitive_data_key and derive_critical_data_key with Zeroizing::new(...) before returning so the derived key material is zeroed on drop; also add the necessary use import (zeroize::Zeroizing) at the top of the file.rust/crates/cove-cspp/src/cspp.rs (2)
27-33: Partial-save risk: if the secondsavefails, orphaned encrypted data remains.The encrypted master key is persisted (line 28) before its decryption metadata (line 32). If the second save fails, the encrypted key is stored but unrecoverable. Consider saving the encryption metadata first (it's useless without the encrypted payload, so orphaning it is harmless), or wrapping both writes in a rollback on failure.
Swap save order so a partial failure leaves only harmless metadata
- self.0 - .save(MASTER_KEY_NAME.into(), encrypted) - .map_err(|e| CsppError::Save(e.to_string()))?; - self.0 .save(MASTER_KEY_ENCRYPTION_KEY_AND_NONCE.into(), encryption_key) .map_err(|e| CsppError::Save(e.to_string()))?; + self.0 + .save(MASTER_KEY_NAME.into(), encrypted) + .map_err(|e| CsppError::Save(e.to_string()))?; + Ok(())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/cspp.rs` around lines 27 - 33, Swap the two persistence operations so the metadata is saved first to avoid orphaning the encrypted master key: change the order of the two self.0.save(...) calls so MASTER_KEY_ENCRYPTION_KEY_AND_NONCE is persisted before MASTER_KEY_NAME, and preserve the same error mapping (CsppError::Save) on both; alternatively, implement a transactional write or rollback around the pair of saves so that if saving MASTER_KEY_NAME fails the earlier metadata save is undone—locate the calls to self.0.save, MASTER_KEY_NAME, MASTER_KEY_ENCRYPTION_KEY_AND_NONCE and CsppError::Save in cspp.rs to apply the change.
70-78: TOCTOU note onget_or_create_master_key.If
Cspp<S>is ever shared across threads (e.g., behind anArc), concurrent calls toget_or_create_master_keywhen no key exists could each generate a different key and race to save, with the last writer winning. This may be fine for the current single-threaded usage, but worth keeping in mind if this struct is later shared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/cspp.rs` around lines 70 - 78, get_or_create_master_key currently has a TOCTOU race: concurrent callers can each run MasterKey::generate and race on save_master_key, so add synchronization around the critical section to ensure only one key is created; for example, add an internal Mutex/lock on Cspp<S> and acquire it at the start of get_or_create_master_key (or use an atomic create API on the backing store), then re-check get_master_key() after acquiring the lock and only call MasterKey::generate() and save_master_key() if still missing — reference get_or_create_master_key, get_master_key, save_master_key, MasterKey::generate, and Cspp<S> when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 64-67: The delete_master_key method currently discards the result
of self.0.delete(MASTER_KEY_NAME.into()) and only returns the result of deleting
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE; change it so both delete results are
observed and the method returns a combined outcome (e.g., store the first result
from self.0.delete(MASTER_KEY_NAME.into()) in a variable, call
self.0.delete(MASTER_KEY_ENCRYPTION_KEY_AND_NONCE.into()), and return the
logical AND of both results or otherwise propagate both failure states) so
callers see failure if either deletion fails.
In `@rust/src/app.rs`:
- Around line 89-92: The call to
crate::database::encrypted_backend::set_encryption_key(key) references a missing
module and will not compile; either add and export an encrypted_backend module
with a public set_encryption_key function (implementing secure storage of the
derived key) or remove/replace this call with the actual encryption-key
initialization implementation in the existing database module; ensure you locate
this call in App::new where Cspp::new and Keychain::global() are used, confirm
the platform initializes the Keychain before App::new() (Keychain::global()
currently .expect(...) if uninitialized), and ensure the derived key type
returned by Cspp::sensitive_data_key() implements Zeroize or ZeroizeOnDrop so
sensitive key material is zeroed when dropped.
---
Nitpick comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 27-33: Swap the two persistence operations so the metadata is
saved first to avoid orphaning the encrypted master key: change the order of the
two self.0.save(...) calls so MASTER_KEY_ENCRYPTION_KEY_AND_NONCE is persisted
before MASTER_KEY_NAME, and preserve the same error mapping (CsppError::Save) on
both; alternatively, implement a transactional write or rollback around the pair
of saves so that if saving MASTER_KEY_NAME fails the earlier metadata save is
undone—locate the calls to self.0.save, MASTER_KEY_NAME,
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE and CsppError::Save in cspp.rs to apply the
change.
- Around line 70-78: get_or_create_master_key currently has a TOCTOU race:
concurrent callers can each run MasterKey::generate and race on save_master_key,
so add synchronization around the critical section to ensure only one key is
created; for example, add an internal Mutex/lock on Cspp<S> and acquire it at
the start of get_or_create_master_key (or use an atomic create API on the
backing store), then re-check get_master_key() after acquiring the lock and only
call MasterKey::generate() and save_master_key() if still missing — reference
get_or_create_master_key, get_master_key, save_master_key, MasterKey::generate,
and Cspp<S> when making the change.
In `@rust/crates/cove-cspp/src/master_key.rs`:
- Around line 35-67: Add a new unit test in the tests module that generates a
MasterKey and asserts that calling sensitive_data_key() and critical_data_key()
on the same MasterKey returns different values; locate the tests module and add
the test alongside
generate_produces_32_bytes/from_bytes_roundtrip/sensitive_data_key_derivation/critical_data_key_derivation
so it references MasterKey::generate(), key.sensitive_data_key(), and
key.critical_data_key() to validate they are not equal.
- Around line 1-33: The derived key methods sensitive_data_key and
critical_data_key currently return plain [u8; 32] which are not zeroized; change
their signatures on MasterKey to return zeroize::Zeroizing<[u8; 32]> and wrap
the values returned from crate::key_derivation::derive_sensitive_data_key and
derive_critical_data_key with Zeroizing::new(...) before returning so the
derived key material is zeroed on drop; also add the necessary use import
(zeroize::Zeroizing) at the top of the file.
In `@rust/crates/cove-cspp/src/store.rs`:
- Around line 1-6: Update the CsppStore trait to surface I/O errors instead of
conflating them with missing keys by changing the signatures of get and delete:
modify trait CsppStore to use fn get(&self, key: String) ->
Result<Option<String>, Self::Error> and fn delete(&self, key: String) ->
Result<bool, Self::Error> (keep save as Result<(), Self::Error>), then update
the Keychain implementation to return platform keychain I/O errors as the
defined Error type for CsppStore and refactor all callers in cspp.rs and
keychain.rs to handle Result<Option<String>, E> from get and Result<bool, E>
from delete (propagate or map errors as appropriate).
praveenperera
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
praveenperera
force-pushed
the
cspp-phase-2-setup
branch
from
6161e92 to
c8ca808
Compare
Derive purpose-specific encryption keys from the master key using HKDF-SHA256 with distinct info strings. Adds sensitive_data_key() and critical_data_key() methods to MasterKey for the redb EncryptedBackend and future critical storage needs. Closes: #557
praveenperera
force-pushed
the
cspp-phase-2-setup
branch
2 times, most recently
from
6c98ef5 to
965379b
Compare
praveenperera
force-pushed
the
cspp-phase-2-setup
branch
2 times, most recently
from
cb274e8 to
d43b21a
Compare
Greptile SummaryIntroduces Key improvements:
Minor issues:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cspp<S: CsppStore>] -->|uses| B[MasterKey]
A -->|stores via| C[CsppStore trait]
C -->|implemented by| D[Keychain]
D -->|wraps| E[KeychainAccess trait]
B -->|derives| F[sensitive_data_key]
B -->|derives| G[critical_data_key]
F -->|HKDF-SHA256| H[cspp:v1:sensitive]
G -->|HKDF-SHA256| I[cspp:v1:critical]
A -->|encrypts with| J[Cryptor ChaCha20Poly1305]
K[get_or_create_master_key] -->|fast path| L[MASTER_KEY_CACHE]
K -->|slow path| M[INIT_LOCK double-checked]
M -->|generates| B
M -->|stores encrypted| N[cspp::v1::master_key]
M -->|stores encryption key| O[cspp::v1::master_key_encryption_key_and_nonce]
N -.->|encrypted by| J
O -.->|decrypts| N
Last reviewed commit: cea26ee |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust/crates/cove-device/src/keychain.rs (1)
147-148: First delete result is discarded indelete_wallet_key.
self.0.delete(key)on line 147 discards its return value. If the mnemonic deletion fails but the encryption key deletion succeeds, the method returnstruewhile the encrypted mnemonic remains orphaned. The same pattern exists indelete_tap_signer_backup(lines 305-306) and was flagged incspp.rs'sdelete_master_keyin a past review. Consider applying the same fix consistently:Proposed fix
fn delete_wallet_key(&self, id: &WalletId) -> bool { let encryption_key_key = wallet_mnemonic_encryption_and_nonce_key_name(id); let key = wallet_mnemonic_key_name(id); - self.0.delete(key); - self.0.delete(encryption_key_key) + let key_deleted = self.0.delete(key); + let enc_deleted = self.0.delete(encryption_key_key); + key_deleted && enc_deleted }Same for
delete_tap_signer_backup:- self.0.delete(backup_key); - self.0.delete(encryption_key_key) + let backup_deleted = self.0.delete(backup_key); + let enc_deleted = self.0.delete(encryption_key_key); + backup_deleted && enc_deleted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-device/src/keychain.rs` around lines 147 - 148, In delete_wallet_key (and similarly in delete_tap_signer_backup) don't discard the result of self.0.delete(key); instead capture both deletion results (e.g., let r1 = self.0.delete(key); let r2 = self.0.delete(encryption_key_key)) and return a combined outcome (e.g., r1 && r2 or propagate an Err if you use Result) so the function only reports success when both deletions succeed; update both delete_wallet_key and delete_tap_signer_backup to use this pattern (referencing the self.0.delete(...) calls and the function return values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 44-66: get_master_key currently returns Ok(None) when the
encryption metadata (MASTER_KEY_ENCRYPTION_KEY_AND_NONCE) exists but the
encrypted master key (MASTER_KEY_NAME) is missing, leaving orphaned metadata;
modify get_master_key so that if encryption_secret is present but encrypted is
missing it either logs a warning and then removes the stale metadata entry from
the backing store (e.g., call
self.0.remove(MASTER_KEY_ENCRYPTION_KEY_AND_NONCE.into())) or at minimum logs a
warning via your logger/tracing, then return Ok(None); reference get_master_key,
save_master_key, MASTER_KEY_ENCRYPTION_KEY_AND_NONCE and MASTER_KEY_NAME to
locate the relevant logic.
- Around line 75-83: The get_or_create_master_key method has a TOCTOU race:
concurrent callers can both see None from get_master_key(), generate different
MasterKey values, and both call save_master_key(), causing an overwrite and
potential data loss; to fix, add an atomic compare-and-set operation to the
storage layer: extend the CsppStore trait with a new get_or_create_master_key(or
compare_and_set_master_key) method that performs an atomic "if empty then set"
(CAS) and use that from Cspp::get_or_create_master_key instead of separate
get_master_key() and save_master_key() calls; update implementations of
CsppStore to implement the atomic method so concurrent callers cannot overwrite
each other.
In `@rust/crates/cove-cspp/src/master_key.rs`:
- Around line 24-27: The doc comment on the method sensitive_data_key (which
calls crate::key_derivation::derive_sensitive_data_key(&self.0)) incorrectly
refers to "redb encryption" — verify which storage engine actually uses this key
(SQLCipher/local DB vs. redb) and update the triple-slash doc comment above pub
fn sensitive_data_key accordingly to accurately state the key's purpose (e.g.,
"used for local DB (SQLCipher) encryption" or "used for redb encryption") so the
comment matches the real usage.
---
Duplicate comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 69-72: delete_master_key currently ignores the boolean result of
the first call to self.0.delete(MASTER_KEY_NAME.into()), so update
delete_master_key to return a combined result (e.g. return the logical AND of
both deletions) instead of discarding the first value; specifically change the
body of delete_master_key to return self.0.delete(MASTER_KEY_NAME.into()) &&
self.0.delete(MASTER_KEY_ENCRYPTION_KEY_AND_NONCE.into()) so both results are
preserved and the method reflects whether both deletes succeeded.
---
Nitpick comments:
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 147-148: In delete_wallet_key (and similarly in
delete_tap_signer_backup) don't discard the result of self.0.delete(key);
instead capture both deletion results (e.g., let r1 = self.0.delete(key); let r2
= self.0.delete(encryption_key_key)) and return a combined outcome (e.g., r1 &&
r2 or propagate an Err if you use Result) so the function only reports success
when both deletions succeed; update both delete_wallet_key and
delete_tap_signer_backup to use this pattern (referencing the self.0.delete(...)
calls and the function return values).
d43b21a to
8c750f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
rust/crates/cove-cspp/src/master_key.rs (2)
39-43:generate_produces_32_bytesis a trivially-passing test.
MasterKeyis a newtype over[u8; 32], soas_bytes().len()is statically guaranteed to be 32 — this test can never fail. Consider replacing it with a test that validates the generated key is non-zero or that two consecutive calls produce different keys.♻️ Suggested replacement
- #[test] - fn generate_produces_32_bytes() { - let key = MasterKey::generate(); - assert_eq!(key.as_bytes().len(), 32); - } + #[test] + fn generate_produces_unique_keys() { + let key1 = MasterKey::generate(); + let key2 = MasterKey::generate(); + // Two independently generated keys must differ (with overwhelming probability) + assert_ne!(*key1.as_bytes(), *key2.as_bytes()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 39 - 43, The test generate_produces_32_bytes is useless because MasterKey is a newtype over [u8; 32]; replace it with a meaningful property test: call MasterKey::generate twice and assert the two keys are not equal (ensures randomness) and/or assert that the generated key.as_bytes() is not all zeros (ensures non-trivial key); update the test name (e.g., generate_is_nonzero_or_unique) and reference MasterKey::generate, MasterKey, and as_bytes() when making the checks.
25-32: Derived key material is not zeroed on drop.
sensitive_data_key()andcritical_data_key()both return a bare[u8; 32]. Callers have no guarantee the derived key bytes will be zeroed from stack or heap after use. For a Bitcoin wallet, consider returningzeroize::Zeroizing<[u8; 32]>so the key is wiped automatically when the caller's binding goes out of scope.♻️ Proposed change
+use zeroize::Zeroizing; - pub fn sensitive_data_key(&self) -> [u8; 32] { + pub fn sensitive_data_key(&self) -> Zeroizing<[u8; 32]> { - crate::key_derivation::derive_sensitive_data_key(&self.0) + Zeroizing::new(crate::key_derivation::derive_sensitive_data_key(&self.0)) } - pub fn critical_data_key(&self) -> [u8; 32] { + pub fn critical_data_key(&self) -> Zeroizing<[u8; 32]> { - crate::key_derivation::derive_critical_data_key(&self.0) + Zeroizing::new(crate::key_derivation::derive_critical_data_key(&self.0)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 25 - 32, The derived key methods sensitive_data_key() and critical_data_key() return bare [u8; 32], so callers cannot guarantee the key bytes are wiped; change their signatures to return zeroize::Zeroizing<[u8; 32]> and wrap the derived arrays in Zeroizing before returning (or update crate::key_derivation::derive_* to return Zeroizing directly), and add the necessary use/import of zeroize::Zeroizing so the key material is automatically zeroed when the caller's binding is dropped.rust/crates/cove-cspp/src/cspp.rs (1)
31-48: Key material in intermediateString/Vec<u8>is never zeroed.In
save_master_key,let hex = hex::encode(master_key.as_bytes())allocates a heapStringcontaining the hex-encoded master key. This string is not zeroed when it goes out of scope, leaving key bytes recoverable from the heap. Likewise inget_master_key(line 63-68), the decrypted hexStringand theVec<u8>produced byhex::decodeboth carry raw key material without zeroing.Use
zeroize::Zeroizingwrappers:♻️ Proposed change for save_master_key
+use zeroize::Zeroizing; pub fn save_master_key(&self, master_key: &MasterKey) -> Result<(), CsppError> { - let hex = hex::encode(master_key.as_bytes()); + let hex = Zeroizing::new(hex::encode(master_key.as_bytes())); let cryptor = Cryptor::new(); ...♻️ Proposed change for get_master_key
- let hex = cryptor + let hex = Zeroizing::new(cryptor .decrypt_from_string(&encrypted) - .map_err(|e| CsppError::Decrypt(e.to_string()))?; + .map_err(|e| CsppError::Decrypt(e.to_string()))?); - let bytes: [u8; 32] = hex::decode(hex) + let raw_bytes = Zeroizing::new( + hex::decode(hex.as_str()) .map_err(|e| CsppError::InvalidData(e.to_string()))? - .try_into() - .map_err(|_| CsppError::InvalidData("master key not 32 bytes".into()))?; + ); + let bytes: [u8; 32] = (*raw_bytes) + .try_into() + .map_err(|_| CsppError::InvalidData("master key not 32 bytes".into()))?;Additionally,
MASTER_KEY_CACHEstoresArc<[u8; 32]>whose bytes are never zeroed even when theArcis dropped. ConsiderArc<Zeroizing<[u8; 32]>>for the cached value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/cspp.rs` around lines 31 - 48, Replace transient heap buffers that hold key material with zeroizing wrappers: in save_master_key wrap the hex::encode(master_key.as_bytes()) result in zeroize::Zeroizing<String> (or Zeroizing<Vec<u8>> if you prefer) before passing to Cryptor::encrypt_to_string, and ensure any intermediate String from cryptor.serialize_to_string() or similar is dropped/zeroed; in get_master_key wrap the decrypted hex String in Zeroizing and hex::decode result in Zeroizing<Vec<u8>> so the raw key bytes are zeroed after use and before returning; finally change the cached MASTER_KEY_CACHE type from Arc<[u8;32]> to Arc<zeroize::Zeroizing<[u8;32]>> (or Arc<Zeroizing<Box<[u8;32]>>> as appropriate) so cached master key bytes are zeroed when the Arc is dropped. Ensure references to save_master_key, get_master_key, MASTER_KEY_CACHE, hex::encode/hex::decode and Cryptor are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 13-15: The static INIT_LOCK and MASTER_KEY_CACHE
(LazyLock/ArcSwapOption) cause cross-instance cache contamination and a race
between delete_master_key and get_or_create_master_key; fix by making the lock
and cache per-Cspp instance (move INIT_LOCK and MASTER_KEY_CACHE into the
Cspp<S> struct as e.g. init_lock: Mutex<()> and master_key_cache:
ArcSwapOption<[u8;32]>, initialize them in Cspp::new), update all uses in
get_or_create_master_key, get_master_key, and delete_master_key to reference the
instance fields, and ensure delete_master_key acquires the instance init_lock
before clearing the cache and deleting from the store while
get_or_create_master_key holds the same lock across the check-load-write
critical section to prevent the race.
In `@rust/crates/cove-device/Cargo.toml`:
- Around line 29-30: Remove the redundant direct dependencies by deleting the
two entries for rand and zeroize from the cove-device Cargo.toml since they are
not referenced in cove-device source code and are already provided transitively
via the cove-cspp crate; confirm you only remove the lines declaring rand = {
workspace = true } and zeroize = { workspace = true } and then run cargo build
to ensure no direct references in cove-device (e.g., in cove-device/src/)
require them before committing.
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 305-306: In delete_tap_signer_backup, the result of the first
delete call is being dropped so only the second delete's Result is returned;
change the first call to propagate errors (e.g. use the ? operator) so failures
from self.0.delete(backup_key) aren't ignored: replace the bare
self.0.delete(backup_key) with self.0.delete(backup_key)? (or otherwise
handle/propagate its Result) before calling self.0.delete(encryption_key_key) in
the delete_tap_signer_backup implementation.
- Around line 147-148: The delete_wallet_key function currently calls
self.0.delete(key) and then returns only the result of
self.0.delete(encryption_key_key), silently discarding the first result; capture
both deletion results (the call on key and the call on encryption_key_key) and
return a combined outcome (e.g., logical AND of both booleans or propagate an
error if either fails) so that callers see failure when the encrypted mnemonic
deletion fails; update references in delete_wallet_key to use the captured
variables from self.0.delete(key) and self.0.delete(encryption_key_key) instead
of dropping the first call.
---
Duplicate comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 76-80: The delete_master_key function currently discards the
result of self.0.delete(MASTER_KEY_NAME.into()) so callers can't see if that
first delete failed; capture both delete results and return a combined boolean
(e.g., r1 && r2) instead of dropping the first one, keeping
MASTER_KEY_CACHE.store(None) as-is and referencing MASTER_KEY_NAME and
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE to locate the two delete calls.
- Around line 52-58: In get_master_key: detect the case where
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE exists but MASTER_KEY_NAME is missing, and
don’t silently return Ok(None); instead emit a warning (using the crate's
logging/tracing facility) that the encryption metadata is orphaned and attempt
to clean it up by removing the stale MASTER_KEY_ENCRYPTION_KEY_AND_NONCE entry
from the underlying store (self.0) so the stale metadata isn’t left
indefinitely; reference MASTER_KEY_ENCRYPTION_KEY_AND_NONCE, MASTER_KEY_NAME,
get_master_key, and self.0 when locating where to add the log-and-delete
behavior.
---
Nitpick comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 31-48: Replace transient heap buffers that hold key material with
zeroizing wrappers: in save_master_key wrap the
hex::encode(master_key.as_bytes()) result in zeroize::Zeroizing<String> (or
Zeroizing<Vec<u8>> if you prefer) before passing to Cryptor::encrypt_to_string,
and ensure any intermediate String from cryptor.serialize_to_string() or similar
is dropped/zeroed; in get_master_key wrap the decrypted hex String in Zeroizing
and hex::decode result in Zeroizing<Vec<u8>> so the raw key bytes are zeroed
after use and before returning; finally change the cached MASTER_KEY_CACHE type
from Arc<[u8;32]> to Arc<zeroize::Zeroizing<[u8;32]>> (or
Arc<Zeroizing<Box<[u8;32]>>> as appropriate) so cached master key bytes are
zeroed when the Arc is dropped. Ensure references to save_master_key,
get_master_key, MASTER_KEY_CACHE, hex::encode/hex::decode and Cryptor are
updated accordingly.
In `@rust/crates/cove-cspp/src/master_key.rs`:
- Around line 39-43: The test generate_produces_32_bytes is useless because
MasterKey is a newtype over [u8; 32]; replace it with a meaningful property
test: call MasterKey::generate twice and assert the two keys are not equal
(ensures randomness) and/or assert that the generated key.as_bytes() is not all
zeros (ensures non-trivial key); update the test name (e.g.,
generate_is_nonzero_or_unique) and reference MasterKey::generate, MasterKey, and
as_bytes() when making the checks.
- Around line 25-32: The derived key methods sensitive_data_key() and
critical_data_key() return bare [u8; 32], so callers cannot guarantee the key
bytes are wiped; change their signatures to return zeroize::Zeroizing<[u8; 32]>
and wrap the derived arrays in Zeroizing before returning (or update
crate::key_derivation::derive_* to return Zeroizing directly), and add the
necessary use/import of zeroize::Zeroizing so the key material is automatically
zeroed when the caller's binding is dropped.
Contributor
Author
|
@greptile-apps re-review |
Introduce CsppStore trait and Cspp<S> struct in cove-cspp that owns all master key persistence logic (encryption, hex encoding, store/load) and key derivation. Keychain now just implements CsppStore as a dumb key-value store. The call site in app.rs uses Cspp::new(keychain) instead of calling master key methods on Keychain directly.
If the encrypted data is saved first and the encryption key save fails, the data becomes permanently unrecoverable. Swap the save order in save_master_key and save_tap_signer_backup to match save_wallet_key.
The keychain already provides at-rest encryption, but wrapping values with a Cryptor prevents accidental plaintext exposure when other code enumerates or inspects keychain entries.
Mirrors the save ordering fix: on delete, remove the encrypted data first so a partial failure never leaves unrecoverable encrypted entries.
Two threads could both observe no master key and generate different keys, with the second writer silently overwriting the first. Use an ArcSwapOption cache for lock-free reads and a Mutex for serializing initialization.
praveenperera
force-pushed
the
cspp-phase-2-setup
branch
from
0f7d297 to
cea26ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rust/crates/cove-cspp/src/master_key.rs (3)
39-43:generate_produces_32_bytesis a tautological test.
key.as_bytes().len()is always32because the underlying type is[u8; 32]; the assertion can never fail regardless of implementation. Consider replacing it with a test that validates randomness intent — e.g. that two generated keys are distinct (or at a minimum assert the array is non-zero, which would catch a brokenfillimplementation).♻️ Suggested replacement
#[test] -fn generate_produces_32_bytes() { - let key = MasterKey::generate(); - assert_eq!(key.as_bytes().len(), 32); +fn generate_produces_distinct_keys() { + let key1 = MasterKey::generate(); + let key2 = MasterKey::generate(); + assert_ne!(*key1.as_bytes(), *key2.as_bytes()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 39 - 43, The test generate_produces_32_bytes is tautological because MasterKey's inner type is [u8; 32]; replace it with a meaningful check: call MasterKey::generate twice and assert the two keys are not equal (or at minimum assert that key.as_bytes() is not all zeros) to validate randomness; update the test function (generate_produces_32_bytes) to use MasterKey::generate and compare via != on key.as_bytes() or use an all-zero check to catch a broken fill implementation.
52-66: Add security-property tests for key derivation.The two determinism tests only verify that calling the same derivation twice on the same key is idempotent. They leave two important security properties untested:
sensitive_data_key() ≠ critical_data_key()for the same master key (domain separation).- Different master keys produce different derived keys (key uniqueness).
Both are cheap to add and catch regressions in the HKDF info constants or a wiring swap.
♻️ Proposed additional tests
+ #[test] + fn sensitive_and_critical_keys_are_distinct() { + let key = MasterKey::generate(); + assert_ne!(key.sensitive_data_key(), key.critical_data_key()); + } + + #[test] + fn different_master_keys_produce_different_derived_keys() { + let key1 = MasterKey::generate(); + let key2 = MasterKey::generate(); + assert_ne!(key1.sensitive_data_key(), key2.sensitive_data_key()); + assert_ne!(key1.critical_data_key(), key2.critical_data_key()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 52 - 66, Tests only check idempotence; add assertions to ensure domain separation and key uniqueness: verify that for a single MasterKey::generate() the outputs of sensitive_data_key() and critical_data_key() are not equal, and verify that two distinct MasterKey::generate() instances produce different sensitive_data_key() (and/or different critical_data_key()) values. Update or add test functions referencing MasterKey::generate, sensitive_data_key, and critical_data_key to include these inequality checks to catch HKDF info or wiring regressions.
9-12:rand::Rngwill be renamed torand::RngExtin rand 0.10.The API usage (
rand::rng()+use rand::Rng as _) is correct for the pinned rand 0.9.0. However, rand 0.10 was released in February 2026, and the 0.10 changelog renamesRngtoRngExt, which will break theuse rand::Rng as _import if the dependency is ever bumped. Worth noting when planning the next dependency update cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-cspp/src/master_key.rs` around lines 9 - 12, The generate() function currently relies on the rand::Rng import which will be renamed in rand 0.10; update code to use the new trait name and APIs: replace any use rand::Rng as _ with use rand::RngExt as _ and ensure the call rand::rng().fill(&mut bytes) uses the corresponding RngExt method (or switch to a stable source like rand::rngs::OsRng and its fill_bytes method) so that the master_key::generate function continues to compile after bumping rand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 13-15: MASTER_KEY_CACHE currently stores Arc<[u8; 32]> which won't
be zeroed on drop, defeating MasterKey's ZeroizeOnDrop; change the cache's
stored type from Arc<[u8; 32]> to Arc<Zeroizing<[u8; 32]>> (use
zeroize::Zeroizing) so that when delete_master_key calls
MASTER_KEY_CACHE.store(None) the underlying key bytes are zeroed; update any
related places that construct or read the cache (e.g., where MASTER_KEY_CACHE is
stored/loaded and where MasterKey is created) to wrap/unwrap the Zeroizing
wrapper accordingly.
---
Duplicate comments:
In `@rust/crates/cove-cspp/src/cspp.rs`:
- Around line 92-98: get_master_key currently returns Ok(None) when
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE exists but MASTER_KEY_NAME is missing,
leaving orphaned encryption metadata if save_master_key previously crashed;
modify get_master_key to detect this partial state (check
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE and missing MASTER_KEY_NAME), log a warning
via tracing::warn! identifying the missing pair, and remove the stale metadata
from the store (use the same storage API you use elsewhere on self.0 to delete
MASTER_KEY_ENCRYPTION_KEY_AND_NONCE) so the store is consistent; reference the
get_master_key method, MASTER_KEY_ENCRYPTION_KEY_AND_NONCE, MASTER_KEY_NAME, and
save_master_key when making the change.
- Around line 57-62: The delete_master_key function currently drops the result
of the first call to self.0.delete(MASTER_KEY_NAME.into()) so callers can get a
misleading true if only the second delete succeeds; capture both delete results
(e.g. let ok1 = self.0.delete(MASTER_KEY_NAME.into()); let ok2 =
self.0.delete(MASTER_KEY_ENCRYPTION_KEY_AND_NONCE.into())) and return the
combined outcome (ok1 && ok2) so the function only returns true when both
entries were successfully removed; keep the existing INIT_LOCK and
MASTER_KEY_CACHE.store(None) behavior unchanged.
- Around line 13-15: The global static INIT_LOCK and MASTER_KEY_CACHE cause
cross-instance cache contamination: make the lock and cache instance fields on
Cspp<S> instead of statics so each Cspp<S> has its own Mutex and ArcSwapOption;
add fields (e.g., init_lock: Mutex<()>, master_key_cache:
ArcSwapOption<[u8;32]>/OnceCell) to the Cspp struct, initialize them in
Cspp::new(), and update all usages that currently reference the statics
(INIT_LOCK and MASTER_KEY_CACHE) to use the corresponding instance fields
(self.init_lock / self.master_key_cache) so different Cspp stores do not share
in-memory keys.
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 147-148: In delete_wallet_key capture and check the result of
self.0.delete(key) instead of discarding it: call self.0.delete(key) and combine
its boolean/result with the subsequent self.0.delete(encryption_key_key) so the
function returns false (or an Err) if either delete fails; reference the
delete_wallet_key function and the two calls self.0.delete(key) and
self.0.delete(encryption_key_key) and ensure the final return/propagation
reflects both outcomes.
- Around line 305-306: In delete_tap_signer_backup ensure the result of
self.0.delete(backup_key) is not discarded: call self.0.delete(backup_key)? (or
otherwise check/propagate its Result) before calling and returning
self.0.delete(encryption_key_key), so any error deleting the backup ciphertext
is surfaced to callers; reference the delete_tap_signer_backup function and the
two calls self.0.delete(backup_key) and self.0.delete(encryption_key_key).
---
Nitpick comments:
In `@rust/crates/cove-cspp/src/master_key.rs`:
- Around line 39-43: The test generate_produces_32_bytes is tautological because
MasterKey's inner type is [u8; 32]; replace it with a meaningful check: call
MasterKey::generate twice and assert the two keys are not equal (or at minimum
assert that key.as_bytes() is not all zeros) to validate randomness; update the
test function (generate_produces_32_bytes) to use MasterKey::generate and
compare via != on key.as_bytes() or use an all-zero check to catch a broken fill
implementation.
- Around line 52-66: Tests only check idempotence; add assertions to ensure
domain separation and key uniqueness: verify that for a single
MasterKey::generate() the outputs of sensitive_data_key() and
critical_data_key() are not equal, and verify that two distinct
MasterKey::generate() instances produce different sensitive_data_key() (and/or
different critical_data_key()) values. Update or add test functions referencing
MasterKey::generate, sensitive_data_key, and critical_data_key to include these
inequality checks to catch HKDF info or wiring regressions.
- Around line 9-12: The generate() function currently relies on the rand::Rng
import which will be renamed in rand 0.10; update code to use the new trait name
and APIs: replace any use rand::Rng as _ with use rand::RngExt as _ and ensure
the call rand::rng().fill(&mut bytes) uses the corresponding RngExt method (or
switch to a stable source like rand::rngs::OsRng and its fill_bytes method) so
that the master_key::generate function continues to compile after bumping rand.
Wrap the cached master key bytes in zeroize::Zeroizing so the in-memory key is zeroed on drop. Update MASTER_KEY_CACHE type to ArcSwapOption<Zeroizing<[u8; 32]>>, import Zeroizing, and adjust loads/stores to dereference the Zeroizing wrapper and store Zeroizing::new(...) when caching the key. This prevents sensitive master key material from lingering in memory.
This was referenced Feb 27, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cove-csppcrate withCspp<S>entry-point struct,MasterKey, and HKDF-SHA256 key derivationCsppStoretrait abstracts persistence —Keychainimplements it as a simple key-value storesensitive_data_key()(for local DB encryption) andcritical_data_key()(for cloud backup encryption)Closes #557, Closes #558
Summary by CodeRabbit
New Features
Bug Fixes
Dependencies
Documentation