Refactor large files into smaller modules #67
Conversation
|
Warning Rate limit exceeded@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
WalkthroughAdds comprehensive CLI command infrastructure across eight new modules: agent (MCP server), bitcoin (address/PSBT operations), enclave (attestation/signing), FROST (threshold signatures), FROST hardware integration, FROST network coordination, vault management, and serve. Refactors memory-locking utilities (MlockedBox/MlockedVec) from signer module to dedicated mlock module and extracts attestation verification logic to independent attestation module in keep-frost-net. Introduces helper functions for password input, confirmation prompts, and hidden vault detection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
kwsantiago
force-pushed
the
refactor/split-large-files
branch
2 times, most recently
from
e6b63d2 to
97e963d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @keep-cli/src/commands/bitcoin.rs:
- Around line 31-35: The secret copied from slot (let mut secret =
*slot.expose_secret()) is not zeroized after use; add use zeroize::Zeroize at
the top and call secret.zeroize() immediately after creating the BitcoinSigner
(i.e. after BitcoinSigner::new(...) returns) to wipe the stack copy, and apply
the same pattern in the cmd_bitcoin_descriptor and cmd_bitcoin_sign handlers for
their local secret variables to ensure secrets are cleared as soon as they are
no longer needed.
In @keep-cli/src/commands/enclave.rs:
- Around line 166-175: The mock branch handling pubkey conversion (the match on
pubkey.as_slice().try_into() that assigns pubkey_arr) currently logs via
out.error and returns Ok(()), which is inconsistent with the Linux branch;
change the mock branch to return the same Err used by the Linux path instead of
Ok(()). Replace the early return in that Err(_) arm so it constructs/returns the
same error type/value used in the Linux path (use the same error
constructor/variant and message) so both paths handle invalid pubkey length
identically.
- Around line 347-351: The secret is cloned into EnclaveRequest::ImportKey via
secret.clone(), so zeroizing the original (secret.zeroize()) still leaves the
clone alive; fix by transferring ownership instead of cloning: change the code
that builds keep_enclave_host::EnclaveRequest::ImportKey to take the owned
secret (e.g., move a Zeroizing<Vec<u8>> into ImportKey or use std::mem::take on
the secret variable) so there is only one copy to be zeroized, or update
EnclaveRequest::ImportKey to accept and zeroize the Zeroizing<Vec<u8>> it
receives; remove the secret.clone() and ensure secret.zeroize() is applied to
the single owned buffer.
In @keep-cli/src/commands/frost_network.rs:
- Around line 848-858: The code is encrypting DKG shares to the sender because
nip44::encrypt is called with keys.public_key(); replace that second argument
with the intended recipient's public key (e.g., lookup the recipient's pubkey
from the collected round 1 packages / participant key map) so each share is
encrypted to the recipient, not the sender; keep using the sender secret via
keys.secret_key(), use the recipient pubkey when calling nip44::encrypt for each
share (and propagate a clear error via KeepError::Other if the recipient key is
missing).
- Around line 1158-1170: The loop pre-generating nonces uses rand::random() to
call frost_commit with dummy_session and dummy_message and stores commitments
(commitments_hex, nonces) but those commitments are not used during actual
signing; update the code and implementation as follows: remove or disable this
unused pre-generation loop (the block that creates dummy_session/dummy_message
and pushes to commitments_hex/nonces) unless you intend to support offline
pre-commitments, and if you keep it ensure the hardware signer enforces
message-binding in frost_sign so that frost_sign(group_npub, &session_id,
&message_arr) will reject any prior commitment not bound to the real message;
additionally audit the hardware signer implementation of frost_sign and
frost_commit to explicitly check/derive binding between session_id, message, and
commitment and add tests asserting that pre-generated dummy commitments are
rejected during real signing.
In @keep-cli/src/commands/frost.rs:
- Around line 219-229: The input loop in frost.rs that accumulates JSON into the
mutable String `json` is unbounded and can exhaust memory; add a hard size limit
(e.g., const MAX_JSON_BYTES: usize = 10 * 1024 * 1024) and track a running total
(e.g., `total_bytes`) each iteration, and after reading `line` check if
`total_bytes + line.len() > MAX_JSON_BYTES` and return a KeepError (or a new
descriptive error) if exceeded; otherwise append the line as before. Update the
block around the `read_line(&mut line)` loop and the `json`/`line` variables to
enforce this limit and fail fast when the input is too large.
In @keep-cli/src/commands/serve.rs:
- Line 123: Replace the direct .unwrap() on tokio::runtime::Runtime::new() with
the same error mapping used earlier (lines ~58-59) so the runtime creation
returns a proper Err variant instead of panicking; specifically change the
Runtime::new().unwrap() call to map or ? the Result into the command’s error
type (use the same map_err/anyhow conversion pattern used at lines 58-59) and
apply the identical fix to the other Runtime::new() occurrences referenced
(lines ~173, 250, 280, 338, 368) so all runtime initializations use consistent
non-panicking error handling.
In @keep-cli/src/commands/vault.rs:
- Around line 44-49: The Password::with_theme(...).interact().unwrap() call can
panic on interaction failure; replace this raw unwrap with the same
error-handling flow used elsewhere by calling get_password_with_confirm (or
using its implementation pattern) to obtain the password and propagate or return
a Result on failure, then construct the SecretString from the successful
password; specifically, remove the unwrap in the block containing
Password::with_theme and either call get_password_with_confirm(...) or map the
Result from .interact() to an appropriate error type and return it instead of
panicking, keeping SecretString::from(pw) only for the successful branch.
- Around line 517-520: The local stack buffer secret containing key material
isn't zeroed after use; after calling NostrKeypair::from_secret_bytes(&mut
secret) (and similarly after the other call around line 561), call
secret.zeroize() to wipe the stack buffer immediately once the keypair is
created so the secret bytes do not remain in memory.
🧹 Nitpick comments (8)
keep-enclave/enclave/src/mlock.rs (1)
117-128: Consider adding a comment explaining the Vec reconstruction.The
Vec::from_raw_partsat line 125 reconstructs a Vec solely to leverage Rust's allocator for deallocation. While safe (the Vec is immediately dropped without reading), this pattern may confuse future maintainers.📝 Suggested documentation
impl Drop for MlockedVec { fn drop(&mut self) { unsafe { // Zero the full capacity, not just len, to catch any leftover data memsec::memzero(self.ptr, self.capacity); if self.locked { memsec::munlock(self.ptr, self.capacity); } + // Reconstruct the Vec to use Rust's allocator for deallocation. + // The Vec is immediately dropped (data is already zeroed). let _ = Vec::from_raw_parts(self.ptr, self.len, self.capacity); } } }keep-cli/src/signer/nonce_store.rs (1)
140-141: Consider removing#[allow(dead_code)]once callers are wired up.These public methods appear to be prepared for use by new CLI command flows (e.g.,
cmd_frost_network_nonce_precommit). If they're part of the intended public API, consider adding tests or wiring up callers, then removing the#[allow(dead_code)]attributes. If they're genuinely unused, consider making thempub(crate)or removing them to reduce API surface.Also applies to: 174-175, 183-184, 204-205
keep-cli/src/commands/agent.rs (1)
46-48: Consider documenting the permissive session scope.
SessionScope::full()with a 24-hour duration grants broad permissions. This may be intentional for a CLI MCP server, but consider adding a comment explaining the scope choice or making it configurable.keep-cli/src/commands/bitcoin.rs (1)
165-167: Consider using random bytes for the dummy signer.Using a fixed
[1u8; 32]pattern for the dummy secret is predictable. While this is only for PSBT analysis (no signing), using random bytes would be more defensive.Proposed change
-let mut dummy_secret = [1u8; 32]; +let mut dummy_secret: [u8; 32] = keep_core::crypto::random_bytes();keep-cli/src/commands/serve.rs (1)
238-247: Significant code duplication betweencmd_serve_outerandcmd_serve_hidden.The decryption and keyring loading logic at lines 238-247 and 326-335 is nearly identical. The TUI thread spawn logic at lines 279-301 and 367-389 is also duplicated. Consider extracting shared helpers.
Suggested extraction
fn load_keyring_from_records( records: Vec<KeyRecord>, data_key: &DataKey, ) -> Result<Keyring> { let mut keyring = Keyring::new(); for record in records { let encrypted = EncryptedData::from_bytes(&record.encrypted_secret)?; let secret_bytes = crypto::decrypt(&encrypted, data_key)?; let mut secret = [0u8; 32]; let decrypted = secret_bytes.as_slice()?; secret.copy_from_slice(&decrypted); keyring.load_key(record.pubkey, secret, record.key_type, record.name)?; secret.zeroize(); } Ok(keyring) }This would reduce duplication and ensure consistent secret handling.
Also applies to: 326-335
keep-cli/src/commands/vault.rs (1)
131-207: Consider extracting common logic to reduce duplication.The
cmd_generate_outerandcmd_generate_hiddenfunctions share nearly identical logic, differing only in the unlock method called (unlock_outervsunlock_hidden) and UI messages. A similar pattern repeats across import, list, export, and delete variants.Consider extracting a helper that takes a closure or enum to parameterize the unlock behavior, reducing maintenance burden and risk of divergent bugs.
keep-cli/src/commands/frost_network.rs (2)
192-204: Consider using a configuration struct to reduce argument count.The function signature has 10 parameters, triggering
#[allow(clippy::too_many_arguments)]. A configuration struct would improve readability and make it easier to add optional parameters in the future.pub struct FrostNetworkSignConfig<'a> { pub path: &'a Path, pub group_npub: &'a str, pub message: &'a str, pub relay: &'a str, pub share_index: Option<u16>, pub hardware: Option<&'a str>, pub warden_url: Option<&'a str>, pub threshold: Option<u16>, pub participants: Option<u16>, }
926-929: Silent decryption failures may hide issues.Failed decryptions are silently skipped with
continue. While this prevents crashes from malformed events, it may hide legitimate issues. Consider adding debug-level logging for failed decryptions to aid troubleshooting.- let decrypted = match nip44::decrypt(keys.secret_key(), &ev.pubkey, &ev.content) { - Ok(d) => d, - Err(_) => continue, - }; + let decrypted = match nip44::decrypt(keys.secret_key(), &ev.pubkey, &ev.content) { + Ok(d) => d, + Err(e) => { + tracing::debug!("Failed to decrypt share from event: {}", e); + continue; + } + };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
keep-enclave/enclave/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
keep-cli/src/commands/agent.rskeep-cli/src/commands/bitcoin.rskeep-cli/src/commands/enclave.rskeep-cli/src/commands/frost.rskeep-cli/src/commands/frost_hardware.rskeep-cli/src/commands/frost_network.rskeep-cli/src/commands/mod.rskeep-cli/src/commands/serve.rskeep-cli/src/commands/vault.rskeep-cli/src/main.rskeep-cli/src/signer/nonce_store.rskeep-enclave/enclave/src/kms.rskeep-enclave/enclave/src/main.rskeep-enclave/enclave/src/mlock.rskeep-enclave/enclave/src/signer.rskeep-frost-net/src/attestation.rskeep-frost-net/src/lib.rskeep-frost-net/src/node.rs
🧰 Additional context used
🧬 Code graph analysis (10)
keep-cli/src/commands/frost_hardware.rs (4)
keep-cli/src/commands/mod.rs (2)
get_confirm(41-50)get_password(16-26)keep-cli/src/output.rs (1)
spinner(88-98)keep-cli/src/signer/hardware.rs (1)
serialize_share_for_hardware(407-423)keep-core/src/keys.rs (1)
npub_to_bytes(148-162)
keep-cli/src/commands/bitcoin.rs (3)
keep-cli/src/commands/mod.rs (1)
get_password(16-26)keep-cli/src/output.rs (1)
spinner(88-98)keep-bitcoin/src/psbt.rs (2)
parse_psbt_base64(214-220)serialize_psbt_base64(226-229)
keep-cli/src/commands/agent.rs (4)
keep-cli/src/commands/mod.rs (1)
get_password(16-26)keep-agent/src/session.rs (1)
scope(146-148)keep-cli/src/output.rs (1)
spinner(88-98)keep-agent/src/mcp/server.rs (1)
with_signing(62-70)
keep-cli/src/commands/frost_network.rs (3)
keep-cli/src/signer/nonce_store.rs (1)
open(31-44)keep-core/src/keys.rs (2)
npub_to_bytes(148-162)npub(138-145)keep-cli/src/commands/frost.rs (1)
check_warden_policy(248-328)
keep-cli/src/commands/serve.rs (6)
keep-cli/src/commands/mod.rs (2)
get_password(16-26)is_hidden_vault(52-54)keep-cli/src/output.rs (1)
spinner(88-98)keep-core/src/keys.rs (2)
npub_to_bytes(148-162)npub(138-145)keep-cli/src/signer/frost_signer.rs (1)
with_shared_node(87-89)keep-core/src/crypto.rs (1)
random_bytes(365-369)keep-cli/src/server.rs (2)
new_network_frost(99-130)bunker_url(132-134)
keep-frost-net/src/node.rs (2)
keep-frost-net/src/attestation.rs (2)
verify_peer_attestation(47-81)verify_peer_attestation(84-94)keep-agent/src/attestation.rs (1)
verify_peer_attestation(151-170)
keep-cli/src/commands/frost.rs (3)
keep-core/src/keys.rs (3)
bytes_to_npub(164-167)npub(138-145)npub_to_bytes(148-162)keep-cli/src/commands/mod.rs (3)
get_password(16-26)get_password_with_confirm(28-39)get_confirm(41-50)keep-cli/src/output.rs (1)
spinner(88-98)
keep-enclave/enclave/src/mlock.rs (2)
keep-enclave/enclave/src/kms.rs (1)
new(24-28)keep-enclave/enclave/src/signer.rs (1)
new(74-88)
keep-cli/src/commands/vault.rs (3)
keep-cli/src/output.rs (1)
spinner(88-98)keep-core/src/keys.rs (2)
bytes_to_npub(164-167)npub(138-145)keep-cli/src/commands/mod.rs (4)
get_confirm(41-50)get_password(16-26)get_password_with_confirm(28-39)is_hidden_vault(52-54)
keep-frost-net/src/lib.rs (1)
keep-frost-net/src/attestation.rs (3)
derive_attestation_nonce(39-44)verify_peer_attestation(47-81)verify_peer_attestation(84-94)
🔇 Additional comments (43)
keep-enclave/enclave/src/kms.rs (1)
4-4: LGTM!The import path update correctly reflects the extraction of
MlockedBoxto the dedicatedmlockmodule.keep-enclave/enclave/src/main.rs (2)
1-6: LGTM!The safety comment accurately documents the unsafe code location and the reviewed commit reference. This provides good traceability for future audits.
16-16: LGTM!The
mlockmodule is correctly declared unconditionally since the memory-locking types are used across all platforms (with graceful degradation whenmlocksyscalls aren't available).keep-enclave/enclave/src/signer.rs (1)
5-5: LGTM!The import correctly references both
MlockedBoxandMlockedVecfrom the newmlockmodule. All usages throughout the file remain consistent with the types' APIs.keep-enclave/enclave/src/mlock.rs (5)
1-8: LGTM!Good module documentation explaining the security purpose. The
#![allow(unsafe_code)]is appropriately scoped to this module, keeping the rest of the crate safe.
20-36: Well-designed secure memory initialization.The pattern of copy → zeroize source → mlock is correct. Storing the
lockedflag to conditionallymunlockhandles the case wheremlockfails gracefully without causing errors on drop.
58-68: Correct cleanup order in Drop.The sequence memzero → munlock → dealloc is the right order: clear secrets before unlocking (to prevent a window where unzeroed data could be swapped), then release the lock, then free memory.
76-77: Verify Send/Sync soundness.The unsafe
SendandSyncimplementations appear sound sinceMlockedBoxexclusively owns its heap allocation with no interior mutability or shared references. However, since this is security-critical code, confirm this was covered in the referenced audit (commit 731ef80).
136-137: Same Send/Sync consideration as MlockedBox.Exclusive ownership makes these implementations sound. Same verification recommendation applies.
keep-frost-net/src/lib.rs (1)
56-56: LGTM! Clean module extraction.The attestation module declaration and re-exports are correctly structured. The refactoring properly separates attestation concerns into a dedicated module while maintaining the public API surface.
Also applies to: 67-67, 71-71
keep-frost-net/src/node.rs (1)
18-18: LGTM! Import correctly references the extracted attestation module.The attestation functionality is now properly imported from the dedicated module, maintaining the same behavior while improving code organization.
keep-frost-net/src/attestation.rs (8)
1-10: LGTM! Well-defined module boundaries.The module structure is clean with appropriate safety guarantees (
forbid(unsafe_code)) and reasonable attestation freshness constants (5-minute max age, 30-second clock skew tolerance).
11-37: LGTM! ExpectedPcrs implementation is correct.The struct properly encapsulates PCR validation with:
- Correct 48-byte PCR size for AWS Nitro Enclaves
- Proper hex parsing with length validation
- Clear error messages
39-44: LGTM! Nonce derivation is cryptographically sound.The function uses proper domain separation with a versioned prefix and deterministically derives nonces from the group public key using SHA-256.
46-81: LGTM! Comprehensive attestation verification.The nitro-attestation implementation provides defense in depth by:
- Verifying the attestation document signature with the derived nonce
- Cross-checking PCR values between the verified document and claimed values
- Validating timestamp freshness
83-94: LGTM! Appropriate fallback for non-nitro environments.The simplified verification path is suitable for development and testing, maintaining PCR and timestamp validation without requiring full document verification.
96-122: LGTM! Timestamp validation is well-implemented.The function properly validates attestation timestamp freshness with:
- Appropriate bounds checking (5-minute max age, 30-second future tolerance)
- Saturating arithmetic to prevent overflow/underflow
- Clear error messages
The cast from
u128tou64on line 100 is safe in practice (timestamps won't exceed 2^64 milliseconds for many centuries).
124-141: LGTM! PCR verification is straightforward and correct.The function provides clear validation with detailed error messages including hex-encoded expected and actual values for debugging.
143-336: LGTM! Comprehensive test coverage.The test suite thoroughly validates:
- PCR hex parsing (valid and invalid inputs)
- Nonce derivation (determinism and uniqueness)
- Attestation verification success and failure paths
- Edge cases (invalid lengths, timestamp boundaries)
- Both nitro and non-nitro implementations
keep-cli/src/commands/mod.rs (4)
16-26: LGTM! Environment variable support for automation is well-implemented.The
KEEP_PASSWORDenvironment variable pattern is standard for CLI tools needing automation. The debug log at line 18 only logs the fact that the env var is used, not the password value itself, which is correct.
28-39: LGTM!Password confirmation flow with environment variable override is consistent with
get_password.
41-50: LGTM!The
KEEP_YESenvironment variable for auto-confirmation is useful for scripted/CI scenarios.
52-54: LGTM!Simple and effective hidden vault detection.
keep-cli/src/commands/agent.rs (2)
40-44: Good secret handling with proper zeroization.The secret is copied, passed to
McpServer::with_signing, and immediately zeroized. This minimizes the window where the secret exists in memory.
72-84: LGTM! stdin/stdout loop is straightforward.The loop properly handles empty lines, propagates I/O errors, and flushes after each response.
keep-cli/src/commands/bitcoin.rs (1)
206-217: LGTM! Network parsing with clear error message.The parser handles common aliases (
mainnet/bitcoin) and provides a helpful error message listing valid options.keep-cli/src/commands/frost_hardware.rs (3)
109-138: Excellent use ofZeroizing<>wrapper for sensitive data.The secret share bytes, hardware share buffer, and hex-encoded share are all wrapped in
Zeroizing<>, ensuring automatic cleanup when they go out of scope. This is a robust pattern for handling cryptographic material.
171-174: Good UX: confirmation prompt for destructive operation.Requiring explicit confirmation before deleting a share from hardware prevents accidental data loss.
198-204: Good input validation for session ID.Validating both hex decoding and length ensures malformed input is caught early with a clear error message.
keep-cli/src/commands/enclave.rs (1)
380-384: Good pattern: zeroize before error propagation.Zeroizing the secret before checking the result ensures cleanup happens even on failure. The Linux path handles this correctly.
keep-cli/src/commands/serve.rs (2)
242-246: Good: Proper secret zeroization in keyring loading.Both paths correctly zeroize the decrypted secret after loading into the keyring.
Also applies to: 330-334
61-92: LGTM! FROST network mode is well-structured.The FROST network flow properly:
- Connects to the network and announces
- Spawns the node task
- Creates the network signer
- Runs the server with appropriate logging
keep-cli/src/commands/vault.rs (3)
15-97: LGTM with minor note on initialization flow.The
cmd_initfunction properly validates password length, ensures hidden password differs from outer password, and provides appropriate user feedback. The initialization flow is well-structured.
325-370: LGTM on list command implementation.The key listing logic handles empty results gracefully and displays a formatted table with truncated public keys for readability.
575-611: LGTM on delete command with confirmation.The delete flow properly requires user confirmation before performing an irreversible operation, which is good UX for destructive actions.
keep-cli/src/commands/frost.rs (4)
15-29: Good security warning for trusted dealer mode.The explicit warning about trusted dealer mode and the recommendation to use DKG for production is excellent user guidance for a sensitive cryptographic operation.
478-481: Good session ID validation.Validating that incoming messages match the expected session ID prevents cross-session confusion attacks and ensures protocol integrity.
348-360: Feature gate handling is correct.The warden feature is properly gated with
#[cfg(feature = "warden")]and provides a clear error message when the feature is not compiled in but a warden URL is supplied.
416-558: Interactive signing flow is well-structured.The two-round FROST signing protocol is correctly implemented with proper commitment collection, signature share generation, and aggregation. Error handling is thorough throughout.
keep-cli/src/commands/frost_network.rs (4)
403-411: Good nonce reuse prevention.The check for previously used nonces before proceeding with signing, combined with immediately marking the nonce as used, correctly prevents the catastrophic key compromise that would result from nonce reuse in Schnorr-based signatures.
655-670: Placeholder implementation is appropriate.The explicit "not implemented" error with guidance to use an alternative command is better than silently failing or returning incorrect results.
690-702: Good input validation for threshold and participant bounds.The validation ensures
2 <= threshold <= participantsand1 <= our_index <= participants, preventing invalid DKG configurations that could cause cryptographic failures.
15-104: LGTM on network serve implementation.The serve command properly initializes the FROST node, subscribes to events, and handles the main event loop with appropriate logging for peer discovery and signing events.
kwsantiago
force-pushed
the
refactor/split-large-files
branch
from
97e963d to
0893be6
Compare
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.